-
Notifications
You must be signed in to change notification settings - Fork 288
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update checkout to support multiple payments #477
Conversation
breaking changes on API
at checkout Temporarily creating new components here. Will move to RDS
reactioncommerce/reaction-component-library#388 has been merged into the component library. This PR can be updated to use those components rather than the copied over versions. The new component library version (at the time of this comment) is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't essential to this PR, so I've created a placeholder ticket that will be expanded upon: #478
I added a separate billing address to be used with my payment. The separate address is not displayed anywhere in the checkout or order confirmation page. It should probably display inside the billing step of the checkout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the starterkit UI, I still see all the payment methods that have been disabled using the operator UI. I have confirmed they are disabled in Mongo.
In this example, I have disabled the IOU method, but the same thing happens with the Stripe method as well.
You can still select the disabled method, and then you get this message when trying to complete checkout:
However, when I select IOU and try to check out with it, I get a message saying it's not enabled:
In the core PR (reactioncommerce/reaction#4908), there is a line that states the checkboxes were ignored, however there was nothing stating the method enable/disable was ignored: "The payment method settings for "supports" (capture, de-auth, etc.) are no longer respected and the check boxes have been removed from the operator UI
"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When multiple payments are all IOU's (no credit card to take remaining balance), the sum of all payments seems to be off my
However, once I add that extra payment of
My guess is that there is a >
where there should be a >=
somewhere in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kieckhafer I opted not to alter the checkout UI based on enabled/disabled. See #370 (comment) If you think this is confusing, I could make it respect the server setting. It's one additional query that doesn't seem necessary. You would always need client UI code changes deployed to add a payment method, so it wouldn't help in that case. The only benefit might be to temporarily disable one that you've implemented or eventually the list could potentially be different based on if you're a member or not or something similar. I guess I'm kind of on the fence. It's tempting to add it just to avoid confusion, but extra queries slow things down and cost money. I will fix the other bugs you found. |
@kieckhafer All issues you pointed out are fixed. I did decide to make it query and respect the enabled payment methods list from the API, too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All changes look good, but they may have had a ripple effect that causes this new issue.
I don't know if this is an issue for this PR, as you should have a payment method active if you have a show, however, if I turn off all available payment methods, I get a blank screen with just a console message:
Checkout page loads when no payment methods are available. However, there is a warning that appears now when we get to the payment section. I don't think this is a blocker, but just wanted you to be aware before this is merged:
I've created this issue to fix after this is merged: #484 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good! One small issue, but it's not a blocker, I've created a new ticket for the fix.
Impact: breaking
Type: feature
Blocked from merging by reactioncommerce/reaction-component-library#388
Requires reactioncommerce/reaction#4908 for the connected API changes.
Changes
Wires up the new components introduced in reactioncommerce/reaction-component-library#388. NOTE: They are temporarily copied into starter kit. No need to review the component files here because they are identical to what is in the component library PR. After that is released, this PR will be updated to reference the four new components from the library.
Also updates the checkout and checkout complete pages for all of the breaking data changes introduced by reactioncommerce/reaction#4908.
Breaking changes
The app is updated to expect the API responses in the breaking API PR: introduced by reactioncommerce/reaction#4908 Placing an order will no longer work against APIs that don't have those changes.
Testing
paymentMethods
array and verify that the checkout looks and works pretty much as it did before this PR.