-
Notifications
You must be signed in to change notification settings - Fork 69
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
Use recommended order currency API for multicurrency subscription renewal | switch | resubscribe #4228
Use recommended order currency API for multicurrency subscription renewal | switch | resubscribe #4228
Conversation
- previously used raw `get_post_meta()` - this is no longer recommended, and could break on stores using (forthcoming) custom order tables
Note: I skipped the checks when committing this. There was a problem with phpcs, I'm not sure if it's related to these changes. Here's the error:
|
Interesting! That error might be caused by your node version. Try running |
@@ -148,7 +148,7 @@ public function override_selected_currency( $return ) { | |||
|
|||
$subscription_renewal = $this->cart_contains_renewal(); | |||
if ( $subscription_renewal ) { | |||
return get_post_meta( $subscription_renewal['subscription_renewal']['renewal_order_id'], '_order_currency', true ); | |||
return $subscription_renewal['subscription_renewal']['renewal_order_id']->get_currency(); |
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.
$subscription_renewal['subscription_renewal']['renewal_order_id']
is an order ID/integer which doesn't have a get_currency()
method.
We'll have to get the order object first, i.e.:
$order = wc_get_order( $subscription_renewal['subscription_renewal']['renewal_order_id'] );
return $order ? $order->get_currency() : false;
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.
D'oh! Of course! Fixed in 20ea6b0
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.
Note in all these I'm returning $return
instead of hard-coded "false". Should be equivalent, but feels more in-keeping with the function comment.
Side note – I don't really understand what this function is doing. The early return when passed $return = [currency]
param makes no sense to me; if passing a valid currency code, the function can't "override"?
woocommerce-payments/includes/multi-currency/Compatibility/WooCommerceSubscriptions.php
Lines 136 to 147 in e01a19c
/** | |
* Checks to see if the if the selected currency needs to be overridden. | |
* | |
* @param mixed $return Default is false, but could be three letter currency code. | |
* | |
* @return mixed Three letter currency code or false if not. | |
*/ | |
public function override_selected_currency( $return ) { | |
// If it's not false, return it. | |
if ( $return ) { | |
return $return; | |
} |
We can dig into that when testing, I'm still figuring out how to test this 🤔
@@ -159,12 +159,12 @@ public function override_selected_currency( $return ) { | |||
$switch_cart_items = $this->get_subscription_switch_cart_items(); | |||
if ( 0 < count( $switch_cart_items ) ) { | |||
$switch_cart_item = array_shift( $switch_cart_items ); | |||
return get_post_meta( $switch_cart_item['subscription_switch']['subscription_id'], '_order_currency', true ); | |||
return $switch_cart_item['subscription_switch']['subscription_id']->get_currency(); |
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.
Similarly for these lines as well, however since we're using the subscription ID we should get a subscription object using:
wcs_get_subscription( $subscription_resubscribe['subscription_resubscribe']['subscription_id'] )
Then call get_currency()
on that object 😃
Just tried that. Now on npm 12, still getting same error. Skipped checks again :) |
- fix logic issue in previous commit
…rrency-order-meta-api' into fix/4152-subscribe-renew-multicurrency-order-meta-api
This is ready for another review & test. Changes since previous reviews:
Unit tests?I'm keen to add unit tests - IMO this is a good candidate for unit tests. If we add unit test coverage for all code touched by custom tables changes, we can re-run those tests later with custom table set up (etc). @mattallan @brucealdridge keen to hear your thoughts 💭 Could add unit tests as part of this PR or as follow up. |
We already have tests for this! And one is failing – working on fixing it. |
I think the unit tests are failing because the new implementation depends on various functions / state that isn't mocked:
How can I mock those global functions - is this something we do in our unit tests? Here are the options as I see it…
What's the best approach to get these tests working and providing value long term? |
I'm also seeing 2 tests erroring out due to an infinite loop, but that might just be on my environment. Worth checking once the failing one is fixed though!
Agreed, I think this covers all the options!
Do you have any examples where that has been done for global functions like Since these are global functions I wouldn't recommend mocking them, unless we can find some precedence elsewhere in the codebase
You could do this, but I'd suggest instead encapsulating the global functions in a class (or classes) that could be injected into WooCommerceSubscriptions. That helper class could then be mocked. This doesn't feel like cheating to me, you're just splitting up different responsibilities into different classes. Having said that, I can't find an example of us doing this. The closest I can find is this call to
IMO this is the best approach here, and it matches what's being done in the unit test already. Instead of creating post meta data we'd just create an order. We're using a helper method to create test orders in places like this. Do you see any unique challenges to this approach when it comes to subscriptions? Perhaps extending that WooCommerce Core helper and adding some methods for creating subscription orders would be an option? You're correct when you say this is more of an integration test, while I'd like to see us breaking more of this plugin up into unit testable chunks, WordPress wasn't written with unit testing in mind so it's perfectly acceptable to follow a more integration testy approach IMO. |
- Make a real order with non-default currency - integration test style. - Enhance mock_wcs_cart_contains_renewal so test can customise product and renewal order ids. - Mock the real order instead of hacking post meta (legacy implementation detail). - Fix up all uses of mock_wcs_cart_contains_renewal() for new signature.
- Rename / update comment to clarify test scope. - Mock up a real order (aka subscription) with custom currency. - Mock that order idea in $_GET request params.
Update! I've fixed the first two tests to use a real order, and tweaked These two tests are now working:
Next stepsI'm still working on the others. To get them working I need to mock up more cart state, e.g. add a switch item to the cart to test this code: woocommerce-payments/includes/multi-currency/Compatibility/WooCommerceSubscriptions.php Lines 160 to 165 in 37bb933
@mattallan can you point me to WCS APIs for setting up a // get_subscription_switch_cart_items() is a wrapper for wcs_get_order_type_cart_items()
// How does a `switch` cart item get added to cart – can I call those functions in my test to set up the relevant state?
wcs_get_order_type_cart_items( 'switch' ); |
We don't have a simple API to setup a switch cart item in a unit test environment, however, I would take a look at Here's is the cart item data we use to determine whether an item is a switch:
You could just manually set this data to mock a switch cart item. Alternatively, you could port over something like this WC Subscriptions unit test helper (get_switch_cart_item_instance() which we use to get a switch cart item object |
- use WC_Order for mock subscription - mock subscription and cart APIs as needed
- parameterise wcs_get_order_type_cart_items so can use real order id - mock sub (order) and wcs_get_subscription to return mocked sub - it's fine right
Unit tests are now passing
|
- ERROR: UndefinedFunction $switch_subscription = wcs_get_subscription( $switch_cart_item['subscription_switch']['subscription_id'] );
I think I'm getting psalm errors now, due to using
Added a wrapper method here 1dda55e but that seems to break the tests?
Getting closer! |
+ clarify true type in docblock comment
…rrency-order-meta-api' into fix/4152-subscribe-renew-multicurrency-order-meta-api
Checks have passed! Ready for a final review @mattallan and/or @Jinksi - this has had one review but the test code has not been reviewed. |
Testing instructions added here: Test multicurrency subscription renewal, switch and re-subscribe FYI @Jinksi - slightly more detailed test instructions for your reviewing pleasure 😁 FYI @mattallan - can you check over these instructions to make sure they are effective for testing these changes, and complete enough for GlobalStep (i.e. did I miss a step!) - thanks 🙌 |
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 LGTM @haszari 🎉 🙌
Unit tests pass and cover changes to the override_selected_currency()
function.
I followed the release testing instructions and found it all to be working as expected:
- Prerequisites
- Test multicurrency subscription renewal
- Test multicurrency subscription switch
- Test multicurrency resubscribe
Note there was a discrepancy in Test multicurrency subscription switch step 7, and the Resubscribe button was not available to click. I suggest this change:
- 7. Click `Resubscribe`. Confirm correct (customer) currency is displayed in checkout.
+ 7. Click `Upgrade or Downgrade`. Select a variation to switch to and click `Switch subscription`. Confirm correct (customer) currency is displayed in checkout.
includes/multi-currency/Compatibility/WooCommerceSubscriptions.php
Outdated
Show resolved
Hide resolved
Thanks for the thorough & helpful review & testing @Jinksi !
I've made that change and also fleshed out details for creating a switchable product (variable subscription product. |
…rrency-order-meta-api' into fix/4152-subscribe-renew-multicurrency-order-meta-api
Fixes #4152
Changes proposed in this Pull Request
Custom order tables are coming to WooCommerce core.
To prepare for that, we need to move away from using low-level APIs to access post meta for orders.
This PR updates the multicurrency WC Subscriptions integration to use the more robust and clearer
get_currency()
API. Previously the code usedget_post_meta
, which is not recommended, and could break in future when stores migrate to using custom order tables.Testing instructions
This change should not change any behaviour. As I understand it, there's no buggy behaviour here unless the store has migrated to custom tables (which is not yet available/shipped yet).
For my testing, I added logging to confirm the relevant lines of code were run, and logged the currency code (to ensure it was correct). All the flows below trigger the code when preparing an order for cart/checkout.
In all tests, the currency code should be the shopper's currency, not the store default.
The test instructions below are all a very simple happy path. Reviewers please suggest and test alternative flows that might be impacted.
Prerequisite - set up payments, subscription product, and multi-currency
Products > Add New
in sidebar.Coffee Subscription
.Product data
box and selectSimple subscription
fromProduct type
dropdown.20
inSubscription price ($)
field.every
day
.Payments > Settings
.Multi-currency
tab.Add currencies
and add one or more additional currencies, e.g. Yen, Euro etc.Store settings
.Add a currency switcher to the Storefront theme on breadcrumb section.
.Save changes
.You should now have a subscription product in your store that shoppers can purchase in their chosen currency.
Test multicurrency subscription renewal
Create pending renewal order
, clickUpdate
.My account
, clickSubscriptions
, scroll down toRelated orders
.Pay
. Confirm correct (customer) currency is displayed in checkout.Test multicurrency subscription switch
WooCommerce > Settings > Subscriptions
, enable bothAllow switching
options. (See below pic)Basic | Pro
attribute). Can also switch between grouped products.Products > Add new
in WooCommerce admin dashboard sidebar.Coffee Subscription
.Product data
box and selectVariable subscription
fromProduct type
dropdown.Attributes
.Add
next toCustom product attribute
dropdown.Level
inName:
box.Basic | Regular | Pro
inValue(s)
box.Used for variations
checkbox.Save attributes
.Variations
.Create variations from all attributes
from dropdown and clickGo
. ClickOK
inAre you sure…
modal to continue. ClickOK
in3 variations added
modal.Subscription price ($)
field.Save changes
.Publish
on right to publish the variable subscription product.My account
, clickSubscriptions
.Upgrade or Downgrade
. Select a variation to switch to and clickSwitch subscription
. Confirm correct (customer) currency is displayed in checkout.Test multicurrency resubscribe
Cancel
for the subscription. Subscription should now be cancelled, so shopper can resubscribe.My account
, clickSubscriptions
Resubscribe
. Refer to docs for details - resubscribing is a kind of renewal.npm run changelog
to add a changelog file, choosepatch
to leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge