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.

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

Examine currency before recognizing as an available payment method #120

Merged
merged 4 commits into from
Aug 2, 2024

Conversation

makotom
Copy link
Contributor

@makotom makotom commented Jul 31, 2024

By default KOMOJU merchants have multicurrency enabled, with which there can by multiple payment methods with the same type_slug, but with different currency or sets of subtypes (card brands).

This PR makes sure that the plugin disposes only payment methods that don't support the currency configured for the WooCommerce instance.

@makotom makotom force-pushed the 20240731-examine-current-currency-only branch 2 times, most recently from 54a7e96 to d506b55 Compare July 31, 2024 03:47
@makotom makotom force-pushed the 20240731-examine-current-currency-only branch from d506b55 to 196369d Compare July 31, 2024 04:19
Copy link
Collaborator

@Dinwy Dinwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, about supporting multi currency, I belive Komoju has currency conversion feature so I am not sure preventing user to pay when the currencies are different.

Below is an example that JPY payment method trying to pay KRW. So, in this case I think we should not filter the payment methods despite WC currency and payment currency is different.

Screenshot 2024-08-02 at 10 21 11

if (isset($methods_by_slug[$slug])) {
$slug = $payment_method['type_slug'];
$pm_currency = $payment_method['currency'];
if (isset($methods_by_slug[$slug]) && $pm_currency !== $wc_currency) {
Copy link
Collaborator

@Dinwy Dinwy Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong to me.
This was checking methods_by_slug is already set, we are skipping the process.
Now it is checking When this slug is already set AND the cureencies are different, then skip
Which means, in the first check when this slug is not set yet, it will always skip this if statement and will set the slug.

Based on the above reason, this should be || or !(A && B)

Copy link
Contributor Author

@makotom makotom Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Actually I intentionally made this logic in this way.

Given that WC has USD as the default "default currency", it's possible that this part of code is called with the default currency set in non-JPY. Then, if we always reject payment methods with mismatched currency (like isset($methods_by_slug[$slug]) || $pm_currency !== $wc_currency), it can end up with no payment method available, because PMs such as Konbini are available only in JPY.
Therefore I relaxed the condition. With the current logic, it will accept payment methods if 1) the currency matches, OR if 2) there is no alternative one available with the same type_slug. IOW, as negation, it rejects PMs that 1) have alternatives, AND 2) the currency does not match.

Yep indeed I first thought it was a bad idea, and eventually I found out that it would be better, seeing how my first attempt here broke our tests.

KOMOJU for WooCommerce Admin -- lets me add and remove specialized payment gateways (failed) (attempt 2)

That said I think that's not an intuitive logic, I'll add a comment into the code for future reference!

Copy link
Collaborator

@Dinwy Dinwy Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I see. This is not about preventing registering the payment method but changing the priority.
In this case, I recommend to change the logic like below for the better redability.

    // If $slug is not set, register
    if (!isset($methods_by_slug[$slug])) {
        $methods_by_slug[$slug] = $payment_method;
    } else {
        // If $slug is already registered and
        // the payment currency matches the WooCommerce currency then override it
        if ($pm_currency === $wc_currency) {
            $methods_by_slug[$slug] = $payment_method;
        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, I've pushed another commit! 🙏

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 👍

@makotom
Copy link
Contributor Author

makotom commented Aug 2, 2024

Also, about supporting multi currency, I belive Komoju has currency conversion feature so I am not sure preventing user to pay when the currencies are different.

Right, this was the decisive fact to employ the logic of accepting PMs that 1) the currency matches, OR if 2) there is no alternative one available with the same type_slug. In this way we just add a preference on PMs with the same currency, but still accepting PMs with different currencies.

@Dinwy
Copy link
Collaborator

Dinwy commented Aug 2, 2024

Also, about supporting multi currency, I belive Komoju has currency conversion feature so I am not sure preventing user to pay when the currencies are different.

Right, this was the decisive fact to employ the logic of accepting PMs that 1) the currency matches, OR if 2) there is no alternative one available with the same type_slug. In this way we just add a preference on PMs with the same currency, but still accepting PMs with different currencies.

Now I understand. It is about the overriding the priority. 😸 No problem. I thought this is about limiting user's payment methods.

@makotom makotom requested a review from Dinwy August 2, 2024 05:26
@Dinwy
Copy link
Collaborator

Dinwy commented Aug 2, 2024

Thank you for the fix!

LGTM

@Dinwy Dinwy merged commit 87c6b4b into master Aug 2, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants