-
Notifications
You must be signed in to change notification settings - Fork 216
Rename billingData to billingAddress #6369
Rename billingData to billingAddress #6369
Conversation
Script Dependencies ReportThe
This comment was automatically generated by the |
|
Size Change: +692 B (0%) Total Size: 864 kB
ℹ️ View Unchanged
|
...s/js/base/context/providers/cart-checkout/payment-methods/use-payment-method-registration.ts
Show resolved
Hide resolved
| cartIsLoading: false, | ||
| cartItemErrors: [], | ||
| cartErrors: [], | ||
| billingData: { |
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.
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.
@wavvves I see that you referred to the file assets/js/base/context/hooks/cart/test/use-store-cart.js, which only contains the test. I assume that the deprecation gate should be added to assets/js/base/context/providers/cart-checkout/payment-methods/use-payment-method-registration.ts instead, correct?
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.
Yes, sorry my bad, I picked up the wrong file after a search. Yes, I didn't want to mention the test file, thanks for noticing 👍
|
@wavvves I addressed your feedback from #6369 (review). Would you mind reviewing this PR again? |
| cart, | ||
| cartTotals, | ||
| cartNeedsShipping, | ||
| billingData, |
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.
After a bit of reviewing with @opr, we found this is being used by registerPaymentMethodExtensionCallbacks (PR and more info here) so billingData should probably still being carried up to this point.
At this point, I wonder if we should weigh all the work involved against the task purpose of just renaming this for consistency 🤔
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.
@wavvves I see you approved the PR but the highlighted code you mentioned is going to cause errors with payment methods if they try to access billingData won't it?
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.
@opr billingData support was added by @nielslange on the latest commit, can you have a look and see if you spot any problem?
| billingData, | ||
| get billingData() { | ||
| /* eslint no-console: ["error", { allow: ["warn"] }] */ | ||
| console.warn( |
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.
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.
@opr I changed the section before to:
get billingData() {
// prettier-ignore
deprecated(
'billingData',
{
alternative: 'billingAddress',
plugin: 'woocommerce-gutenberg-products-block',
link:
'https://github.com/woocommerce/woocommerce-blocks/pull/6369',
}
);
return this.billingAddress;
},Is that what you had in mind?
…o update/5811-rename-billingData-to-billingAddress-internally # Conflicts: # package-lock.json
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.
thanks Niels, I tested and it works! 🙌🏼
The user-facing testing instructions in this PR are incorrect, a user won't be checking out PRs or adding any code to their site, could we come up with an alternative testing strategy, for example using and verifying there are no errors (this plugin accesses billingData on this line: https://github.com/woocommerce/woocommerce/blob/trunk/packages/js/extend-cart-checkout-block/src/js/filters.js.mustache#L17)
woocommerce-test-extension.zip
I approved so you can merge, but please update the instructions!

Fixes #5811
In our codebase, we were using
billingDataandshippingAddress, which both contains the address object. To increase the maintainability of the codebase, we changedbillingDatatobillingAddress.As some extensions are already using
billingDataviauseStoreCart, this PR ensures that the change is backwards compatible.Testing
Automated Tests
For automatic testing, you can either run the entire unit tests using
npm t, the individual unit test that covers this PR usingnpm t -- assets/js/base/context/hooks/cart/test/use-store-cart.jsand the individual integration test usingnpm t -- npm t -- slots.js.User Facing Testing
Regular payments
Deprecation gate
billingData is deprecated. Please use billingAddress instead. See: https://github.com/woocommerce/woocommerce-blocks/pull/6369is visible.WooCommerce Visibility