Skip to content
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.

Sign up for GitHub

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

Merged
merged 26 commits into from
Jun 29, 2022
Merged

Use recommended order currency API for multicurrency subscription renewal | switch | resubscribe #4228

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
e154bb3
use recommended `WC_Order::get_currency()` for getting renewal currency:
May 4, 2022
14eb4e3
add changelog
May 4, 2022
2a78530
Merge branch 'develop' into fix/4152-subscribe-renew-multicurrency-or…
haszari May 4, 2022
e01a19c
Merge branch 'develop' into fix/4152-subscribe-renew-multicurrency-or…
haszari May 8, 2022
20ea6b0
convert id to order/subscription object before calling get_currency:
May 9, 2022
dfbde74
Merge branch 'develop' into fix/4152-subscribe-renew-multicurrency-or…
haszari Jun 14, 2022
c433f4e
fix fatal error on resubscribe – get subscription from ID
Jun 14, 2022
bc31724
Merge remote-tracking branch 'origin/fix/4152-subscribe-renew-multicu…
Jun 14, 2022
23ec842
Merge branch 'develop' into fix/4152-subscribe-renew-multicurrency-or…
haszari Jun 15, 2022
4b733df
fix override_selected_currency test for renewal case:
Jun 17, 2022
37bb933
Repair multicurrency sub switch unit test:
Jun 17, 2022
053bf3c
repair multicurrency subs switch in cart unit test:
Jun 23, 2022
f45d6b9
refactor mock_wcs_get_order_type_cart_items so can pass a real order id
Jun 23, 2022
3d97600
repair multicurrency sub resubscribe unit test:
Jun 23, 2022
4a1cfc8
remove a bunch of extraneous whitespace / php linter fixes
Jun 23, 2022
02cf94e
remove extraneous whitespace / php linter fixes
Jun 23, 2022
1dda55e
add local wrapper for wcs_get_subscription() – attempt psalm fix:
Jun 23, 2022
b21922a
Merge branch 'develop' into fix/4152-subscribe-renew-multicurrency-or…
haszari Jun 23, 2022
73780a4
fix mismatched param name in docblock for get_subscription wrapper
Jun 23, 2022
d3eeea8
Merge branch 'develop' into fix/4152-subscribe-renew-multicurrency-or…
haszari Jun 27, 2022
76365db
remove incorrect return type (array) from get_subscription test helper +
Jun 27, 2022
9290953
Merge remote-tracking branch 'origin/fix/4152-subscribe-renew-multicu…
Jun 27, 2022
6c139f6
remove whitespace offending the linter
Jun 27, 2022
91cfcc0
Merge branch 'develop' into fix/4152-subscribe-renew-multicurrency-or…
haszari Jun 28, 2022
a5ccf31
rename subscription variable – it's resubscribe not switch
Jun 28, 2022
98afc22
Merge remote-tracking branch 'origin/fix/4152-subscribe-renew-multicu…
Jun 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions changelog/fix-4152-subscribe-renew-multicurrency-order-meta-api
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: fix

Use high-level order currency API for multicurrency subscription renewal orders (get_post_meta is not recommended for orders).
6 changes: 3 additions & 3 deletions includes/multi-currency/Compatibility/WooCommerceSubscriptions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Contributor

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;

Copy link
Contributor Author

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

Copy link
Contributor Author

@haszari haszari May 9, 2022 •
edited
Loading

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 🤔

}

$switch_id = $this->get_subscription_switch_id_from_superglobal();
Expand All @@ -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();
Copy link
Contributor

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 😃

}

$subscription_resubscribe = $this->cart_contains_resubscribe();
if ( $subscription_resubscribe ) {
return get_post_meta( $subscription_resubscribe['subscription_resubscribe']['subscription_id'], '_order_currency', true );
return $subscription_resubscribe['subscription_resubscribe']['subscription_id']->get_currency();
}

return $return;
Expand Down