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

[FIX] payment_adyen: convert the paymentAmount respecting the Adyen API #31984

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@len-odoo
Copy link
Contributor

len-odoo commented Mar 20, 2019

Adyen requires the amount to be multiplied by 10^k,
where k depends on the currency code.
https://docs.adyen.com/developers/development-resources/currency-codes
Provides a map of codes to factors.

Before, k was assumed to be 2 (which is correct in most cases).

opw 1928918

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@len-odoo len-odoo requested a review from nim-odoo Mar 20, 2019

@robodoo robodoo added the seen 🙂 label Mar 20, 2019

@nim-odoo
Copy link
Contributor

nim-odoo left a comment

Since the code hasn't probably changed between v11 and v12, I would merge in v11.

"""
if currency.name not in CURRENCY_CODE_MAPS:
raise UserError(_("This currency is currently unsupported by Adyen."))
k = CURRENCY_CODE_MAPS[currency.name]

This comment has been minimized.

Copy link
@nim-odoo

nim-odoo Mar 20, 2019

Contributor

k = CURRENCY_CODE_MAPS.get(currency.name, 2) and you save a hundred lines.

This comment has been minimized.

Copy link
@len-odoo

len-odoo Mar 20, 2019

Author Contributor

I prefer to have the explicit map, since any code editor can just fold the lines.
And that would avoid passing an arbitrary value if it is not in the code map.

For your other remark, I've tested what happens by forcing the exception, and you get a red frame displaying the error message.

But in both cases I can do the change if you want.

This comment has been minimized.

Copy link
@nim-odoo

nim-odoo Mar 20, 2019

Contributor

I think it is very unlikely that a user would subscribe to Adyen if the currency he is using is not supported by the service. So I would remove the check.

I also think we should keep the diff minimal. With the get(), we have a good chance that a new currency supported by Adyen would work in Odoo too.

Adyen requires the amount to be multiplied by 10^k,
where k depends on the currency code.
"""
if currency.name not in CURRENCY_CODE_MAPS:

This comment has been minimized.

Copy link
@nim-odoo

nim-odoo Mar 20, 2019

Contributor

Let's not bother with this. This could trigger a 500 error on the frontend while the end user couldn't do anything about it.

@C3POdoo C3POdoo added the OE label Mar 20, 2019

@len-odoo len-odoo force-pushed the odoo-dev:12.0-opw1928918-adyxz-len branch from 1bb2c13 to a5e1e3e Mar 20, 2019

[FIX] payment_adyen: convert the paymentAmount respecting the Adyen API
Adyen requires the amount to be multiplied by 10^k,
where k depends on the currency code.
https://docs.adyen.com/developers/development-resources/currency-codes
Provides a map of codes to factors.

Before, k was assumed to be 2 (which is correct in most cases).

opw 1928918

@len-odoo len-odoo force-pushed the odoo-dev:12.0-opw1928918-adyxz-len branch from a5e1e3e to 800367c Mar 21, 2019

@robodoo robodoo added the CI 🤖 label Mar 21, 2019

@len-odoo

This comment has been minimized.

Copy link
Contributor Author

len-odoo commented Mar 21, 2019

robodoo r+

@robodoo robodoo added the r+ 👌 label Mar 21, 2019

@robodoo robodoo closed this in e02a907 Mar 21, 2019

@robodoo robodoo added merged 🎉 and removed merging 👷 labels Mar 21, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Mar 21, 2019

Merged, thanks!

@len-odoo len-odoo deleted the odoo-dev:12.0-opw1928918-adyxz-len branch Mar 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.