fix: ensure BNPL enablement is not adding unnecessary currencies #8212
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.26 MB ℹ️ View Unchanged
|
frosso
commented
Feb 15, 2024
| @@ -2747,7 +2747,7 @@ protected function get_account_branding_secondary_color( $default_value = '' ): | |||
| * | |||
| * @return string The domestic currency code. | |||
| */ | |||
| protected function get_account_domestic_currency(): string { | |||
| public function get_account_domestic_currency(): string { | |||
There was a problem hiding this comment.
I changed this from protected to public so that it could be called in the PaymentMethodsCompatibility multi-currency class. I think it's fine?
There was a problem hiding this comment.
I think it's fine, but overall, this method seems to be placed in the unexpected class. I'd expect it to be located somewhere in the includes/class-wc-payments-account.php near get_account_country() and get_account_default_currency() methods :)
frosso
requested review from
a team and
timur27
and removed request for
a team
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
When BNPL methods are enabled (either via onboarding or via the settings pages), we're adding unnecessary currencies.
BNPL methods are set only to accept "domestic payments". I.e.: customers can't use BNPL unless they're paying with the same currency as the merchant's account currency.
Since BNPL methods are configured to only accept "domestic" payments, we should only add the "domestic" currency if needed.
This PR fixes the backend logic and the UI when removing currencies from the multi-currency settings.
Before - Klarna shouldn't be disabled if EUR is disabled and the account's default currency is USD:
After - Klarna is no longer displayed in the modal:
The messaging on onboarding is fixed here: #8202
The messaging on the settings page when the payment method requires additional setup will be fixed on a separate PR.
Testing instructions
Test 1
Test 2
giropayas a PM, and not Klarnanpm 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