This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Update canMakePayment to receive cart as argument and make it react to changes in billingData. #4776
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +1.37 kB (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
…r the effect calling refreshCanMakePayments
The initial approach was to debounce billingData and use this value as a dependency for the useEffect that runs refreshCanMakePayments. But because the depencies array can always change we decided to debounce the callback instead, ensuring this way that callback is not called multiple times: for example when typing a field in the billing address. Debounced was chosen instead of throttle because we want to call refreshCanMakePayments once the change event has stopped, with the final value.
ralucaStan
changed the title
ralucaStan
changed the title
ralucaStan
requested review from
senadir
and removed request for
mikejolley
|
@senadir the PR is ready for review. |
3 tasks
ralucaStan
added
status: needs review
type: enhancement
senadir
approved these changes
Sep 23, 2021
| @@ -68,6 +67,7 @@ const usePaymentMethodRegistration = ( | |||
|
|
|||
| useEffect( () => { | |||
| canPayArgument.current = { | |||
| cart, | |||
There was a problem hiding this comment.
How about spreading cart here, which would include cartTotals, cartNeedsShipping, paymentRequirements, and also add the rest of items at the same level?
There was a problem hiding this comment.
I would advise against it, as cart has a lot of keys, and that would make the signature really confusing. After deprecating the keys you mentioned it would just be cart and billingData and that makes it easier to understand where the data comes from.
There was a problem hiding this comment.
Wouldn't having cart as is just complicated one step further? so instead of arg.cartTotals you need arg.cart.cartTotals especially seeing that canPayArgument is made of cart related fields only.
There was a problem hiding this comment.
It's also made of billingData and shipping data from the CustomerDataContext
There was a problem hiding this comment.
Those are also present in Cart, shipping data is shippingAddress and billing data is billingAddress.
The only difference here is that billingAddress reflects what's currently in state, and what's in Cart reflects what comes from the server, so this might not always be in sync. billingData can still replace billingAddress from cart.
This is not a huge point to block the PR on as moving the other direction (spreading) is easier than reversing.
There was a problem hiding this comment.
The data is indeed out of sync. This would be an issue for extensions that need to make calculations on the current billingData or shippingData.
This is not a huge point to block the PR on as moving the other direction (spreading) is easier than reversing.
👍
Comment on lines
+70
to
+84
| beforeEach( async () => { | ||
| fetchMock.mockResponse( ( req ) => { | ||
| if ( req.url.match( /wc\/store\/cart/ ) ) { | ||
| return Promise.resolve( JSON.stringify( previewCart ) ); | ||
| } | ||
| return Promise.resolve( '' ); | ||
| } ); | ||
| // need to clear the store resolution state between tests. | ||
| await dispatch( storeKey ).invalidateResolutionForStore(); | ||
| await dispatch( storeKey ).receiveCart( defaultCartState.cartData ); | ||
| } ); | ||
|
|
||
| afterEach( () => { | ||
| fetchMock.resetMocks(); | ||
| } ); |
There was a problem hiding this comment.
a call was being made by useStoreCart and it needed to be mocked. I improved to mock to make it clearer.
| @@ -87,11 +87,13 @@ A callback to determine whether the payment method should be available as an opt | |||
|
|
|||
| ``` | |||
| canMakePayment( { | |||
| cart: Cart, | |||
| cartCoupons: CartCoupons, | |||
There was a problem hiding this comment.
cartCoupons is no longer exposed
There was a problem hiding this comment.
Really good catch
| @@ -27,6 +28,8 @@ export interface Supports extends SupportsConfiguration { | |||
| } | |||
|
|
|||
| export interface CanMakePaymentArgument { | |||
| cart: Cart; | |||
| cartCoupons: CartResponseCoupons; | |||
github-actions
bot
added
status: ready to merge
and removed
status: needs review
labels
|
Please take another look, I've addressed your requests |
This was referenced Sep 23, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR
cartobject to the argument we pass to thecanMakePaymentcallbackbillingDatato the dependencies list of the effect that callsrefresCanMakePaymentand closes canMakePayment isn’t tracking billingData changes #4766.Fixes:
#4670
#4766
A new ticket has been created for deprecating cart-derived values from the
canMakePaymentargument #4809How to test the changes in this Pull Request:
assets/js/base/context/providers/cart-checkout/payment-methods/use-payment-method-registration.tsand edit for the purpose of testing only:registerPaymentMethodExtensionCallbacksto the existing import from'@woocommerce/blocks-registry'Use same address for billingand write "Alexandra" for First Name in the Billing Address section. Cash on Delivery option should be available as a payment method.Changelog