Add WooCommerce Multi-Currency support on WooPay #7246
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: 0 B Total Size: 1.44 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
@alefesouza Thank you for working on this. As mentioned in the other PR, tests are as expected, but I couldn't place the order, so I also assigend @malithsen to review the PR. I left two questions below.
| @@ -139,4 +139,24 @@ public function test_get_gift_cards_data_while_logged_out_with_zero_balance() { | |||
|
|
|||
| $this->assertEquals( $this->woopay_adapted_extensions->get_gift_cards_data( $this->test_user ), $expected ); | |||
| } | |||
|
|
|||
| public function test_get_extension_data() { | |||
There was a problem hiding this comment.
Should we update the option 'woocommerce_currency' to USD in case it's not USD so that we won't break the test in the future if the currency changes?
There was a problem hiding this comment.
On tests USD is the default one, I've tried using these codes but no success changing the currency during the tests:
add_filter(
'pre_option_woocommerce_currency',
function () {
return 'BRL';
}
);
add_filter(
'woocommerce_currency',
function () {
return 'BRL';
}
);
update_option( 'woocommerce_currency', 'BRL' );
| @@ -19,6 +19,9 @@ class WooPay_Adapted_Extensions extends IntegrationRegistry { | |||
| const GIFT_CARDS_API = 'woocommerce-gift-cards'; | |||
| const GIFT_CARDS_BLOCKS = 'wc-gift-cards-blocks'; | |||
|
|
|||
| const WOOCOMMERCE_MULTICURRENCY = 'woocommerce-multicurrency'; | |||
| const WOOCOMMERCE_MULTICURRENCY_PATH = 'woocommerce-multicurrency/woocommerce-multicurrency.php'; | |||
There was a problem hiding this comment.
Should we check if a constant in the plugin is defined so if the folder name is different then we can prevent critical error?
There was a problem hiding this comment.
Good catch, fixed on f3c67b1.
| @@ -453,6 +453,7 @@ public static function ajax_init_woopay() { | |||
|
|
|||
| $woopay_adapted_extensions = new WooPay_Adapted_Extensions(); | |||
| $body['adapted_extensions'] = $woopay_adapted_extensions->get_adapted_extensions_data( $email ); | |||
| $body['extension_data'] = $woopay_adapted_extensions->get_extension_data(); | |||
There was a problem hiding this comment.
Do we need to set this and adapted_extensions here or do you think we can pass it in the store_data in
There was a problem hiding this comment.
We could pass it on store data, but this would not be backwards compatible as a version with adapted_extensions was already released, the WooPay side would need to have a condition for both cases until we remove support for older versions, what do you think?
There was a problem hiding this comment.
ah true. I like the idea of consolidating data passing to WooPay in a single place. If you agree, we can perhaps move extension_data data into store_data in this PR and migrate adapted_extensions in a separate PR.
There was a problem hiding this comment.
I agree with Malith that the store_data seems the right place to be.
|
As discussed via Slack, currently on I've done some tests and noticed that if we reload the WooPay checkout page or open the same checkout url on a new tab it will load with the fallback currency. This PR fixes this problem too by forcing the currency before the redirection, @malithsen could you review it again? |
There was a problem hiding this comment.
@alefesouza I've been testing these changes with the first-party auth flow, which is now the default on trunk, and haven't been able to get this to work.
session_init, which the associated WooPay PR utilizes is not called during first-party auth flow, instead create_unclaimed_session is used. Although I haven't tested it, this is likely affecting adapted extensions as well.
Can you please have a look?
|
@alefesouza I tested with the first-party auth flow and it doesn't work for me either. Screen.Recording.2023-10-31.at.2.31.04.PM.mov |
|
@malithsen @hsingyuc fixed on 8305bfe , the problem with adapted extensions already has a workaround implemented on #7455, which disables first party auth flow when one adapted extension is enabled, fixing it will be a little bit trickier as it depends on knowing the user email address before the redirect, for the |
Changes proposed in this Pull Request
Send the current currency to WooPay when WooCommerce Multi-Currency is installed.
Testing instructions
npm run changelogto add a changelog file, choosepatchto 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