Add new card component #668

joshsmith · 2016-11-10T07:27:23Z

What's in this PR?

Should implement:

Month/year dropdown
Button state changes to disabled/clear vs enabled/default based on valid parameters
Generate Stripe token for card when pressing donate https://github.com/code-corps/ember-stripe-service#creating-stripe-tokens-for-cards
Pressing donate takes Stripe token and POSTs to server to create subscription
Shows Stripe token creation and server errors in a list in a paragraph (not client-side validation errors)
Pressing donate button shows loading state; changes on error or success
Create credit card when one is added. List them if user has any

Copied stuff to add later into #669 so we do not have it listed under "incomplete subtasks" on the milestone.

References

Fixes #473

begedin · 2016-11-10T15:47:45Z

I got further on this one, but I had trouble with testing and handling it properly. Turns out, Ember does not detect the promise stripe.card.createToken generates, so it doesn't wait for it. It took me a while to figure out how to wait and test for it and I'm not extremely happy with the result, but it works for now. Simply put, I use Ember.Test.registerWaiter, to wait for an isLoading flag on the controller to be set to false. This means tighter coupling between the test and the code than I would like, but the alternative is to completely override the stripe service in our acceptance test, which sort of defeats the purpose of the acceptance test.

Then again, I'm up for trying that as well.

Either way, I got the process to complete, so now it would be a matter of rendering errors and deciding what to do on success. Do we want to just display a message, or redirect to a whole new route (ex. project.thankyou)?

joshsmith · 2016-11-10T16:16:05Z

@begedin do we need to be fixing the Stripe service? Open an issue there?

joshsmith · 2016-11-10T16:35:21Z

@begedin in particular we were already having conversations over there about this code-corps/ember-stripe-service#12

begedin · 2016-11-10T19:02:06Z

@joshsmith I took a look at that issue already. Really, it's how ember behaves. I'm not really sure if we can do anything about it. Ember handles tracks some type of async stuff correctly, but not all of it. I stubbed out the stripe service for now, and that does the trick, but the downside is, if we change this addon, we will have to update the stub.

The upside is, we're maintaining the addon.

begedin · 2016-11-10T19:06:41Z

Updated the to-do list. It doesn't seem like much was checked, but I think we're past the hard work, so now it's a matter of writing the templates for errors and deciding what to do when the user successfully donates.

begedin · 2016-11-11T13:06:14Z

@joshsmith This should finally be good to review.

The "Thank you page just displays a message for now". We can add a template, which will be mostly static, separately.

begedin · 2016-11-11T15:26:54Z

I've mostly implemented the thank you page. The layout matches the provided wireframe and all that's missing is setting the src of the icon image. Referring #477 here, since it's about 95% solved by this.

begedin · 2016-11-11T20:49:10Z

Got as far as I could tonight.

Add card ui is always shown. Should probably show it only if there are no cards and/or if the user clicks "add card"
Add card UI doesn't go away after card was added. Should go away.
If there are cards present, user can click to select one and then donate. This creates a subscription and redirects to thank you page.
If there are no cards, user can add one. Once added, card is preselected and user can click donate to create subscription.

The process of adding cards needs more work:

We create the token, but we do not use it anywhere. I think we should use it to create a customer, then use the customer to create a card?
We create a stripe customer, but they are just assigned the current user and the current user's email, nothing else. Should probably assign token as source
We always create a customer, need to make this conditional.
We create a card after creating a customer, we assign it the user, the exp month, year, last 4 digits, brand, country, name, from stripe token response. Not sure if we should assign anything else.
We add the card to the card list. User can now click to select it, then click donate. Should probably have some preselection.
Most of the stuff is tested, but still need a good amount of acceptance testing.
The process of adding a card is a long chain of promises. I tried to make it as simple as possible, but it's still complicated. I find that it's easier to debug if I add a console log to controllers/project/donate/_handleError.

I hope this helps.

joshsmith · 2016-11-13T02:35:56Z

Adding these flow notes here for extra reference:

Create a platform customer
Create a customer on the connect account with the same info
Create a plan on the connect account
Create a card for the platform customer
Create a token with the platform card and platform customer with the Stripe-Account header set to the connect account
Create a subscription with the connect account's customer, the connect account's plan, and the token as the source

joshsmith · 2016-11-13T03:18:02Z

Once one or more cards are added, you can select the card you want to pay with. Your default card will show as selected by default.

joshsmith · 2016-11-13T03:18:29Z

This is the add card UI. It will require a cancel link when it's not the first card you're adding. If adding, the card list above won't be shown.

joshsmith · 2016-11-13T06:25:06Z

I've gone about as far as I can tonight. Still a number of failing tests due to changes.

I'm wondering how much we should try to close this PR out soon and put things into separate issues. It's not as though develop really works, so I'm not opposed to getting this passing, merging, and then breaking out into parallelizable issues.

sbatson5

This is looking good so far. Let me know where I can help 👍

sbatson5 · 2016-11-13T13:58:16Z

app/components/donation/card-item.js

+  isSelected: alias('card.selected'),
+
+  click() {
+    this.sendAction('select');


Since we are passing this in as a closure action: this.get('select')();

sbatson5 · 2016-11-13T14:00:12Z

app/components/donation/credit-card.js

+
+  isSubmitting: false,
+
+  date: computed('month', 'year', function() {


Thoughts on simply renaming this to dateString so it is clear we expect it to be a string and not a date object?

sbatson5 · 2016-11-13T14:06:36Z

app/components/donations/create-donation.js


 export default Component.extend({
  classNames: ['create-donation'],
  presetAmounts: [10, 15, 25, 50],
-  amount: 15
+  amount: 10,


Could we add customAmount: null here as well, so it is clear it's a property set on this component?

sbatson5 · 2016-11-13T14:12:54Z

app/controllers/project/donate.js

+    };
+  },
+
+  _updateAddingCardState() {


For clarity (not to be too nitpicky), could we do disableAddingCardState? update implies that it could be anything--whereas this function will only set it to false.

sbatson5 · 2016-11-13T14:21:33Z

app/templates/components/donation/credit-card.hbs

+
+{{#unless isSubmitting}}
+  {{!validation errors can be passed in from parent, using a block}}
+  {{yield}}


👍 I like this.

sbatson5 · 2016-11-13T14:27:03Z

tests/acceptance/project-donate-test.js

+    projectDonatePage.creditCard.cardNumber('4242-4242-4242-4242');
+    projectDonatePage.creditCard.cardCVC('123');
+    projectDonatePage.creditCard.cardMonth('12');
+    projectDonatePage.creditCard.cardYear('2020');


Thoughts on extracting this block to a helper function since it is re-used?

sbatson5 · 2016-11-13T14:36:59Z

tests/integration/components/donation/credit-card-test.js

+  K
+} = Ember;
+
+let setHandler = function(context, submitHandler = K) {


Thoughts on extracting this to a helper file since we use it so often?

sbatson5 · 2016-11-13T14:39:01Z

tests/integration/components/donation/credit-card-test.js

-    assert.notOk($(input).prop('disabled'));
+  stubService(this, 'stripe', {
+    card: {
+      validateCardNumber: () => true,


Since we're importing Ember.K already, we can change these to validateCardNumber: K, as it does the same thing.

Resolved in 37a6024 as Ember.K is no longer needed.

sbatson5 · 2016-11-13T18:22:46Z

tests/integration/components/donation/credit-card-test.js

@@ -50,6 +54,4 @@ test('it sends submit with credit card fields when button is clicked', function(
      .cardYear(expectedProps.year)
      .cardCVC(expectedProps.cvc)
      .clickSubmit();
-
-  assert.ok(page.submitDisabled, 'Submit button got disabled.');


The component is destroyed at this point, so the _afterSubmit method returns early and does not disable the button.

sbatson5 · 2016-11-13T20:09:26Z

tests/integration/components/donation/donation-container-test.js

 });

-test('it renders donation amount and frequency', function(assert) {
+test('it handles clicking card submit button correctly', function(assert) {


@joshsmith I'm questioning what we are testing here. Most of the actions in this particular test are for the credit card component. This test fails because of the submit action on donation/credit-card (the child component). I think the test coverage for submitting should be handled there and we should only worry about rendering what is appropriate here, as this is really just the container of the components that hold the logic.

@sbatson5 agreed. Feel free to modify at your leisure.

@joshsmith Revised here: eb48a06. I think this focuses more on what the component is doing and not worrying about the state of the other components (which our acceptance test should cover)

sbatson5

Just 1 minor comment. But this looks good to me.

sbatson5 · 2016-11-14T01:13:27Z

app/components/donation/donation-container.js

 } = Ember;

 export default Component.extend({
+  // canAddCard: true,


Can we delete this comment?

joshsmith added the in progress label Nov 10, 2016

joshsmith added this to the Donations milestone Nov 10, 2016

begedin force-pushed the 473-new-card-component branch from 8e6dc09 to 5f5acc5 Compare November 10, 2016 10:13

begedin force-pushed the 473-new-card-component branch from fa3c421 to 4e8fb03 Compare November 10, 2016 19:06

begedin mentioned this pull request Nov 11, 2016

Improve donation process (not MVP for donations) #669

Closed

4 tasks

begedin mentioned this pull request Nov 11, 2016

Finish thank you page #477

Closed

7 tasks

joshsmith mentioned this pull request Nov 11, 2016

Add new credit card component #473

Closed

7 tasks

sbatson5 reviewed Nov 13, 2016

View reviewed changes

sbatson5 force-pushed the 473-new-card-component branch 3 times, most recently from 3e90c9f to 973dfce Compare November 13, 2016 18:21

sbatson5 reviewed Nov 13, 2016

View reviewed changes

sbatson5 force-pushed the 473-new-card-component branch from d0b6748 to ad8ce82 Compare November 13, 2016 19:58

sbatson5 reviewed Nov 13, 2016

View reviewed changes

joshsmith force-pushed the 473-new-card-component branch from eb48a06 to 47c9d1a Compare November 13, 2016 23:40

joshsmith self-assigned this Nov 13, 2016

joshsmith force-pushed the 473-new-card-component branch from 8d53747 to 4137e52 Compare November 14, 2016 00:55

sbatson5 approved these changes Nov 14, 2016

View reviewed changes

Implement donating, adding a new card, and selecting a card

5ffb26a

joshsmith force-pushed the 473-new-card-component branch from a30c9fc to 5ffb26a Compare November 14, 2016 01:35

joshsmith merged commit a8eccb3 into develop Nov 14, 2016

joshsmith deleted the 473-new-card-component branch November 14, 2016 01:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new card component #668

Add new card component #668

joshsmith commented Nov 10, 2016 •

edited

Loading

begedin commented Nov 10, 2016

joshsmith commented Nov 10, 2016

joshsmith commented Nov 10, 2016

begedin commented Nov 10, 2016

begedin commented Nov 10, 2016

begedin commented Nov 11, 2016

begedin commented Nov 11, 2016

begedin commented Nov 11, 2016 •

edited by joshsmith

Loading

joshsmith commented Nov 13, 2016

joshsmith commented Nov 13, 2016 •

edited

Loading

joshsmith commented Nov 13, 2016 •

edited

Loading

joshsmith commented Nov 13, 2016

sbatson5 left a comment

sbatson5 Nov 13, 2016

sbatson5 Nov 13, 2016

sbatson5 Nov 13, 2016

sbatson5 Nov 13, 2016 •

edited

Loading

sbatson5 Nov 13, 2016

sbatson5 Nov 13, 2016

sbatson5 Nov 13, 2016

sbatson5 Nov 13, 2016

sbatson5 Nov 13, 2016

sbatson5 Nov 13, 2016

sbatson5 Nov 13, 2016

joshsmith Nov 13, 2016

sbatson5 Nov 13, 2016

sbatson5 left a comment

sbatson5 Nov 14, 2016


		isSubmitting: false,

		date: computed('month', 'year', function() {

Add new card component #668

Add new card component #668

Conversation

joshsmith commented Nov 10, 2016 • edited Loading

What's in this PR?

References

begedin commented Nov 10, 2016

joshsmith commented Nov 10, 2016

joshsmith commented Nov 10, 2016

begedin commented Nov 10, 2016

begedin commented Nov 10, 2016

begedin commented Nov 11, 2016

begedin commented Nov 11, 2016

begedin commented Nov 11, 2016 • edited by joshsmith Loading

joshsmith commented Nov 13, 2016

joshsmith commented Nov 13, 2016 • edited Loading

joshsmith commented Nov 13, 2016 • edited Loading

joshsmith commented Nov 13, 2016

sbatson5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbatson5 Nov 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbatson5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshsmith commented Nov 10, 2016 •

edited

Loading

begedin commented Nov 11, 2016 •

edited by joshsmith

Loading

joshsmith commented Nov 13, 2016 •

edited

Loading

joshsmith commented Nov 13, 2016 •

edited

Loading

sbatson5 Nov 13, 2016 •

edited

Loading