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

Remove currency check #60

Merged
merged 1 commit into from
Jun 22, 2022
Merged

Remove currency check #60

merged 1 commit into from
Jun 22, 2022

Conversation

Resonious
Copy link
Member

We support multi-currency now, so this code is now more harmful than helpful.

@Resonious Resonious changed the base branch from master to quick-setup-button June 22, 2022 00:48
@Resonious Resonious changed the base branch from quick-setup-button to master June 22, 2022 02:07
@degicat
Copy link

degicat commented Jun 22, 2022

@Agroang can you help us review this PR, please?

@degicat degicat requested a review from Agroang June 22, 2022 02:07
Copy link
Contributor

@Agroang Agroang left a comment

Choose a reason for hiding this comment

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

Forgive my lack of PHP and Woocommerce knowledge but out of curiosity, as far as I can read we are no longer limited to JPY thanks to "multi-currency", so that check for apply_filters('woocommerce_komoju_supported_currencies', ['JPY']) is no longer needed.
When it comes to the actual currencies supported by "multi-currency" (USD, EUR, JPY, SGD, HKD and AUD?), is there any scenario where the user might try to use a different currency not included in the above and create an error? Or is option based among the mentioned above (so they can only choose among the supported ones)?

@Resonious
Copy link
Member Author

Resonious commented Jun 22, 2022

@Agroang I actually don't know the list of currencies we support. I'm guessing this:

[JPY, USD, TWD, EUR, KRW, PLN, AUD, NZD, SGD, HKD, GBP, IDR, MYR, PHP, THB, CNY, USDC]

So one alternative way to do this would be to keep the the code here, but use that list instead of just JPY. The only issue there is that if we add more currencies to our core offering, the plugin needs a code update 😞. Maybe we should add currencies to our /api/v1/meta so that this plugin can pull the list directly from the source.

@Agroang
Copy link
Contributor

Agroang commented Jun 22, 2022

@Resonious I see, I was just picking up the list from our woocommerce read me for supported payout currencies as it was using the same "multi currency" keyword 😅.

It does sounds like a code update per added currency for the plugin is costly and not very efficient.
Something like pulling the list directly as you mentioned as a possible solution sounds like a better approach.

Nevertheless, at least for this PR, my only concern was that taking off the check on JPY would allow users to "try" to use any currency (maybe ones that are not included in what we consider "multi-currency"?) but if that is not possible then I don't think that scenario may arise.

Just to confirm, are you ok with merging as it is? If above is true then I don't want to keep blocking the merge 😅

@Resonious
Copy link
Member Author

@Agroang Ah I see. Good catch with the readme, haha. I think that readme line makes a good case for this PR, because it says "Accept payments in local currency", but right now "local currency" only means "JPY".

You're correct that users can attempt any currency, and our system will fail to create a payment if it is unsupported.

I think in most cases, this is something that the merchant would test before releasing live. I don't anticipate any sudden failures of existing live setups, so I'm okay with merging this 👍 (then hopefully we can find time to put the limit back, but make it dynamic based on what KOMOJU supports 😁)

@Agroang
Copy link
Contributor

Agroang commented Jun 22, 2022

Got it! Thanks for the further explanation and sorry for the delay! 😸

@Agroang Agroang merged commit f6f89c9 into master Jun 22, 2022
@kvhees
Copy link
Contributor

kvhees commented Jun 22, 2022

Our DCC feature will convert if it's not supported by KOMOJU, if they have enabled it.

@Resonious
Copy link
Member Author

Ah alright - sounds like this is fine as-is then 👍

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.

4 participants