-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Purchases: Reduxify Stored Cards on Edit Card Details page #6748
Conversation
9964292
to
c600b53
Compare
! state.selectedPurchase.hasLoadedUserPurchasesFromServer || | ||
! state.hasLoadedSites | ||
! state.hasLoadedSites || | ||
state.isFetchingStoredCards |
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.
I'm not super fan of referring here to a prop that is in fact set in the EditCardDetails
component but managing it in this data component proved to be awkward (or maybe that's just me :) ).
This looks good. I had one note about moving the logic to display the loading placeholder and query the stored cards into |
c600b53
to
5434348
Compare
5434348
to
dd79703
Compare
Thank you very much for the review @drewblaisdell. I updated this pull request per your suggestion. It is now ready for another review. |
I'm unable to open Manage Purchase in this branch. I see this error in the console when I click any items in the list in
Edit: Seems it doesn't happen to all purchases, but to some of them. I'm guessing the ones that do not have a payment method? |
dd79703
to
b4444f3
Compare
Thanks for catching this error @fabianapsimoes. I've fixed the issue, this is ready for review again. |
Product 👍 |
fields = this.mergeCard( this.props.card.data, fields ); | ||
|
||
if ( this.props.card ) { | ||
fields.name = this.props.card.name; |
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.
Super minor, but fields
should be a const
now.
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.
Good catch!
I also tested this, and the code looks good to me. 👍 Not remapping keys seems like the right approach for this change, as well. |
This data is now located in the Redux global state tree.
b4444f3
to
18c7e60
Compare
This pull request is a follow-up of #6613, #6628, #6665, #6710 that updates the
Edit Card Details
page to use the global state tree to manage credit cards (stored during previous purchases or when a user added a new payment method to an existing purchase) instead of the former Flux store:It also fixes the prefilling of the card holder name that was broken.
Testing instructions
git checkout update/reduxify-edit-card-details
and start your server, or open a live branchPurchases
pageEdit Payment Method
actionName on Card
field contains the card holder name used in the checkoutSave Card
buttonManage Purchase
page with a success messageBilling History
pageEdit Card Details
pageName on Card
field is prefilled in with the new nameYou may want to check other payment methods, or type of purchases. You could also force a browser refresh on certain pages to make sure it doesn't make a difference.
Additional notes
I opted not to update the Redux selector to remap keys similarly to what the former assembler was doing because as you can see it here the only value we care is the card holder name. We use the keys provided by the API in any other places where we display credit cards. We can address that in a separate pull request though if most of us think it's worth it.
Reviews
@Automattic/sdev-feed