Decouple miscellaneous dependencies from multi-currency module #9302
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
|
ricardo
requested review from
a team and
cesarcosta99
and removed request for
a team
cesarcosta99
requested changes
Aug 26, 2024
…feature flag check in gateway context
…store_api_request()"
ricardo
commented
Aug 28, 2024
cesarcosta99
approved these changes
Aug 28, 2024
There was a problem hiding this comment.
Code looks good and test well! Thanks for working out the improvements 🚀
- Multi-currency: Merchant ✅
- Multi-currency: Shopper ✅
- Test instructions in feat: add Store API multi-currency support #8816 ✅
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes proposed in this Pull Request
API_Exception: The instances where we were handling this error didn't require any specific logic, so a regularExceptioncan be used. The only difference is that it will catch any exception, but that's ok.Base_Exception: The main difference between WCPay exceptions and the defaultExceptionis that it specifies a string error code. That error code was not a requirement so I opted for using the500server error code.Logger: The WCPay logger has a few caveats in that it won't log unless in sandbox mode or WCPay logging setting is enabled, and as discussed in MCCY round table, we opted for making a separate logger class and log file for multi-currency as this also sets up the module for further separation from the gateway.WC_Payments_Features: Replaced or put into the gateway context when initializing multi-currency.WC_Payments::register_script_with_dependencies: This required reimplementation. We're also passing the gateway context to the multi-currency constructor to determine where the assets should be loaded from and the cache version.WC_Payments_Explicit_Price_Formatter: Reimplemented the logic from the one function that was used.Database_Cache: Now implements aMultiCurrencyCacheInterface.Country_CodeandCurrency_Code: The constants used from these classes were replaced by its values. Basically a revert from Add ENUM class for country codes #7925. I considered duplicating the classes into the multi-currency module and also passing them as a reference, but none of these approaches seemed right to me considering v2.WC_Payments_Utils::get_supported_countries: IMO this should belong to the account service since these are the countries supported by Stripe.WC_Payments_Utils::zero_decimal_currencies(): Replaced by the module'sBackendCurrency'sis_zero_decimal_currency.WC_Payments_Utils::is_store_api_request(): This also made more sense to be duplicated into the module'sUtils.tests/unit/multi-currency.Testing instructions
This PR impacts a lot of files, but since we're merging into a feature branch and will do thorough testing before merging into
develop, we should prioritize code review and focus on testing the critical flows for now.I’ve also tested the decoupled components, like logs, ordermeta helper, and subscriptions, but once other pieces are also merged into
multi-currency-v2, we can perform more extensive testing across all scenarios.These critical flows should pass:
Code review
multi-currency/files and ensure you don't see any dependencies from v1 gateway code besides:wc-payments-multi-currency.php: This file should be moved out of the module and refactored later.PaymentMethodsCompatibility.php: There's a separate issue for this – Add Tracks tracking #74.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