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

[payment] Add extra fee to payment form #31730

Closed
wants to merge 2 commits into from

Conversation

jso-odoo
Copy link
Contributor

@jso-odoo jso-odoo commented Mar 11, 2019

Description of the issue/feature this PR addresses:

Current behavior before PR:

  • Customer was not getting any warning regarding the extra fees taken by the payment acquirer.

Desired behavior after PR is merged:

  • Customer will be notified on the payment form regarding extra fees taken by the payment acquirer.

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

@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Mar 11, 2019
@C3POdoo C3POdoo added the RD research & development, internal work label Mar 11, 2019
@mba-odoo mba-odoo force-pushed the master-payment-extra-fees-jso branch from 910cdc7 to a70cda1 Compare March 14, 2019 06:15
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Mar 14, 2019
@mba-odoo mba-odoo force-pushed the master-payment-extra-fees-jso branch from ec3a93e to ff32fe7 Compare March 14, 2019 10:15
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Mar 14, 2019
@jso-odoo jso-odoo force-pushed the master-payment-extra-fees-jso branch from ff32fe7 to 4f4c99e Compare March 15, 2019 12:56
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Mar 15, 2019
@jso-odoo jso-odoo force-pushed the master-payment-extra-fees-jso branch from 4f4c99e to 5f2e772 Compare March 19, 2019 08:41
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Mar 19, 2019
Copy link
Contributor

@rdeodoo rdeodoo left a comment

Choose a reason for hiding this comment

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

Could you rebase the branch + extract the text from the font awesome <i> tag on those 2 lignes?
I know it is not related to your PR but since you are modifying the payment_tokens_list template that would be nice to have.

https://github.com/odoo-dev/odoo/blob/5f2e77297c219af82c0b87a00b978e31f3ef67f5/addons/payment/views/payment_templates.xml#L143

https://github.com/odoo-dev/odoo/blob/5f2e77297c219af82c0b87a00b978e31f3ef67f5/addons/payment/views/payment_templates.xml#L147

Other than that it seems OK, I tested most flows

@@ -63,6 +63,12 @@
t-att-checked="acquirers_count==1 and pms_count==0 or acquirers[0] == acq"/>
</t>
<span class="payment_option_name" t-esc="acq.name"/>
<t t-if="acq_extra_fees and acq_extra_fees.get(acq)">
<span class="badge badge-primary"> + <t t-esc="acq_extra_fees[acq]" t-options='{"widget": "monetary", "display_currency": acq_extra_fees["currency_id"]}'/> Fee </span>
Copy link
Contributor

Choose a reason for hiding this comment

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

badge-secondary to fit other badge price style (eg variant extra on product page)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.!

<t t-if="acq_extra_fees and acq_extra_fees.get(acq)">
<span class="badge badge-primary"> + <t t-esc="acq_extra_fees[acq]" t-options='{"widget": "monetary", "display_currency": acq_extra_fees["currency_id"]}'/> Fee </span>
</t>
<t t-elif="acq.fees_active">
Copy link
Contributor

Choose a reason for hiding this comment

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

This elif case was a bit mysterious when reading at first, can you confirm that it is needed for:

  • Either special controller that would not set acq_extra_fees in values dict to render like the `/website_payment/pay' (that probably can't do it anyway since no currency).
  • Either for payment.acquirer that activated fees but did not override XX_compute_fees, but such a case do not exists in standard Odoo code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @rdeodoo ,
As discussed with FGI, we need it in the case where,

  • extra_fees is active and either the method to calculate extra_fees is not implemented or the extra fees is zero.

cc @mba-odoo

@@ -789,10 +789,6 @@ def extra_info(self, **post):
# ------------------------------------------------------

def _get_shop_payment_values(self, order, **kwargs):
shipping_partner_id = False
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@jso-odoo jso-odoo force-pushed the master-payment-extra-fees-jso branch from 5f2e772 to 47b0029 Compare April 9, 2019 05:50
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Apr 9, 2019
@jso-odoo jso-odoo force-pushed the master-payment-extra-fees-jso branch from 47b0029 to f283d05 Compare April 9, 2019 06:32
Purpose of the task is when extra fee is activated on the payment acquirer,
the customer is no warned about extra cost when choosing the payment acquirer.

so display the extra fees on payment acquirer when doing the payment.

Related Task ID : 1845815
Closes : odoo#31730
@rdeodoo rdeodoo force-pushed the master-payment-extra-fees-jso branch from f283d05 to a5f4181 Compare April 9, 2019 06:45
So text do not inherit from font awesome font style
@rdeodoo rdeodoo force-pushed the master-payment-extra-fees-jso branch from a5f4181 to 7532249 Compare April 9, 2019 06:47
@JKE-be
Copy link
Contributor

JKE-be commented Apr 9, 2019

@robodoo r+

@robodoo robodoo added r+ 👌 CI 🤖 Robodoo has seen passing statuses labels Apr 9, 2019
@JKE-be
Copy link
Contributor

JKE-be commented Apr 9, 2019

@robodoo r+ rebase-ff

@robodoo
Copy link
Contributor

robodoo commented Apr 9, 2019

Merge method set to rebase and fast-forward

@robodoo
Copy link
Contributor

robodoo commented Apr 9, 2019

I'm sorry, @JKE-be. This PR is already reviewed, reviewing it again is useless.

robodoo pushed a commit that referenced this pull request Apr 9, 2019
Purpose of the task is when extra fee is activated on the payment acquirer,
the customer is no warned about extra cost when choosing the payment acquirer.

so display the extra fees on payment acquirer when doing the payment.

Related Task ID : 1845815
Closes : #31730
robodoo pushed a commit that referenced this pull request Apr 9, 2019
So text do not inherit from font awesome font style

closes #31730

Signed-off-by: Jérémy Kersten (jke) <jke@openerp.com>
@robodoo
Copy link
Contributor

robodoo commented Apr 9, 2019

Merged, thanks!

@robodoo robodoo closed this Apr 9, 2019
@rdeodoo rdeodoo deleted the master-payment-extra-fees-jso branch April 25, 2019 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI 🤖 Robodoo has seen passing statuses RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants