Fix Bug: Prevent Multi-Currency Conversion During REST API Request #9477
Conversation
Among the other factors: - is_admin() - DOING_CRON - is_admin_api_request
Default to store currency and do not convert the default price.
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.33 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
This tested well for the issue being described! As we've discussed, it's a bit challenging to determine what the correct approach is here for the REST API, but I feel this approach is the best balance as it still allows currency conversion if desired, but makes it explicit now.
Also as we discussed this could cause breaking changes if there are uses of the REST API that are depending on the currency conversion today, but at least this approach provides an easy update path to get back to the functionality with the currency parameter.
It was already highlighted in the Slack thread by @frosso that this still has a side effect of changing the "shopper's" currency in their meta. The concern raised there was that if we're changing it on a request, we should also honor it on a request. This presents a challenge just given how the functionality is designed today. One possible alternative is to make use of the override_selected_currency filter when the currency parameter is provided as well so that it doesn't pull from the user meta (and thus doesn't have to set it during a REST API request).
Let me know what you think of an approach like that, and @frosso feel free to weigh in here as well!
…g-rest-api-request
Good call. Fixed in 398610c. |
…g-rest-api-request
There was a problem hiding this comment.
Thanks for making those changes @lovo-h! This tests well for me now with no side effects. I confirmed that I can pass in the currency parameter and get the correct price from the REST API and it doesn't update the authenticated user's currency meta. I also confirmed that no currency parameter always returns in the default currency as expected.
I wanted to do some quick regression testing with the Store API side of it and found one issue that is pre-existing, but causes a breaking issue with this PR. In short /batch requests on the Store API will return false on \WC_Payments_Utils::is_store_api_request(). This is because the batch path isn't included in STORE_API_ROUTE_PATTERNS that are compared. This can be observed in the following test:
- Add an item to the cart
- Visit the cart block
- Change currency to non-default currency (I had default of GBP and selected JPY)
- Note that the cart prices are in JPY
- Now update the quantity on an item and this should trigger a
batchrequest - Note that the prices now go back to GBP
I'm unsure the best approach here (add batch to the list or handle this check separately?) my only concern with adding batch to the list is if that has implications outside of this flow that would trigger a WooPay flow. I don't believe that would be a problem as all of our WooPay checks are combined with a is_request_from_woopay check. I also see the is_store_api_request is used in multiple places and surprised this hasn't come up in other areas so wanted to get a double check on my thoughts here?
|
@bborman22 that I don't know if there's an easy fix to that detection mechanism 😬 it could be an enhancement 🤷 |
Good catch. I was able to reproduce the issue.
Agreed. I don't think we should add the In order to keep things narrow, we could make an exception for when we apply the patch. That is, when it's not a if ( ! \WC_Payments_Utils::is_store_batch_request() && ! \WC_Payments_Utils::is_store_api_request() && WC()->is_rest_api_request() ) {
// Apply patch.
}This would maintain the same functionality as before and apply the patch on the REST API requests, as needed. What are your thoughts on this @bborman22 @frosso? |
There was a problem hiding this comment.
I was able to test this again with the above scenarios and confirmed that Store API (batch really) requests now keep the correct currency and the logic is being applied correctly there. I also finished the last test case by testing the REST API with a subscription product and a bookings product and all the pricing behaved as expected by returning in default currency unless the currency parameter was provided.
Thanks for all your effort on getting this here @lovo-h!
LGTM!
lovo-h
deleted the
fix/9181-unintended-mccy-conversion-during-rest-api-request
branch
Fixes #9181
Changes proposed in this Pull Request
The changes in this PR ensure that the REST API requests are idempotent. This is accomplished by preventing multi-currency from converting the currency and price based on the user's session. Previously, if a shopper updated their frontend currency with a currency-switcher block, the REST API requests from the frontend would also be unintentionally impacted.
Testing instructions
Replicate bug: Updating the frontend currency impacts the REST API requests
developbranch.GBP(default) +EUR).GBP) and the ID.EUR).https://merchant.com/wp-json/wc/v2/products/<product_id>?consumer_key=<key>&consumer_secret=<secret>in the browser's search bar.€.Confirm fix: Updating the frontend currency does not impact the REST API requests
fix/9181-unintended-mccy-conversion-during-rest-api-requestbranch.GBP(default) +EUR).GBP) and the ID.EUR).https://merchant.com/wp-json/wc/v2/products/<product_id>?consumer_key=<key>&consumer_secret=<secret>in the browser's search bar.£(default).price_htmlfield.Ensure compatibility: Updating the frontend currency does not impact the REST API requests for compatible plugins
fix/9181-unintended-mccy-conversion-during-rest-api-requestbranch.GBP(default) +EUR).GBP) and the ID.EUR).https://merchant.com/wp-json/wc/v2/products/<product_id>?consumer_key=<key>&consumer_secret=<secret>in the browser's search bar.£(default).price_htmlfield.Ensure unit tests all pass
npm run test:php.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