-
-
Notifications
You must be signed in to change notification settings - Fork 534
[18.0][MIG] website_sale_charge_payment_fee: Migration to version 18.0 #1123
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
base: 18.0
Are you sure you want to change the base?
[18.0][MIG] website_sale_charge_payment_fee: Migration to version 18.0 #1123
Conversation
ADD website_sale_charge_payment_fee_delivery: link module ADD website_sale_charge_payment_fee_quote: link module
…o acquirer If there are no acquirers, gets ``` Error to render compiling AST TypeError: 'NoneType' object has no attribute '__getitem__' Template: 929 Path: /templates/t/t/div/div[1]/div/div[2]/input Node: <input type="hidden" t-att-value="selected_acquirer.id if selected_acquirer else acquirers[0].id" name="selected_acquirer_id" data-oe-id="1278" data-oe-xpath="/data/xpath[3]/input" data-oe-model="ir.ui.view" data-oe-field="arch"/> ```
…re is no acquirer
We avoid this way the warning on runbot and it's more tolerant to inheritance.
Currently translated at 100.0% (21 of 21 strings) Translation: e-commerce-14.0/e-commerce-14.0-website_sale_charge_payment_fee Translate-URL: https://translation.odoo-community.org/projects/e-commerce-14-0/e-commerce-14-0-website_sale_charge_payment_fee/es/
b4062d4 to
541f2ef
Compare
| with RecordCapturer(self.env["sale.order"], []) as capture: | ||
| self.start_tour("/shop", "payment_fee_tour", login="portal", step_delay=100) | ||
| created_order = capture.records | ||
| price = 10 / 100 * 99.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this value change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not change the value, actually is calculate the expected value of created_order.amount_payment_fee. That's why next step it and assetEqual.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, why does the calculation change? Before, it was done with price = 10 / 100 * 49.5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you avoid depending on demo data to prevent external changes from altering the behavior here?
| res = super()._compute_website_order_line() | ||
| website_order_line = self.env["sale.order.line"] | ||
| for order in self: | ||
| for line in order.website_order_line: | ||
| if not line.payment_fee_line: | ||
| website_order_line |= line | ||
| order.website_order_line = website_order_line | ||
| return res |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change this code? I think the older code is clearer and more compact.
| "is_published": True, | ||
| } | ||
| ) | ||
| transfer_provider._compute_charge_fee_description() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don’t need the _compute_charge_fee_description function; it should be called automatically.
| amount_payment_fee = 0.0 | ||
| if ( | ||
| self.env["website"] | ||
| .get_current_website() | ||
| .show_line_subtotals_tax_selection | ||
| == "tax_excluded" | ||
| ): | ||
| for line in order.order_line: | ||
| if line.payment_fee_line: | ||
| amount_payment_fee += line.price_subtotal | ||
| else: | ||
| for line in order.order_line: | ||
| if line.payment_fee_line: | ||
| amount_payment_fee += line.price_total |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same here: Why change this code? I think the older code is clearer and more compact.
| browser.location.href = | ||
| "/shop/payment?provider_id=" + | ||
| providerId + | ||
| "&payment_option_id=" + | ||
| paymentOptionId; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I attached a video. I believe the problem occurs when you have two or more payment providers. In this case, I installed Demo and Wire Transfer and selected the second payment option. The window flickers, and internally the sale order changes when another payment method is selected. So, I think the window is not being refreshed correctly.
website_sale_charge_payment_fee.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carlos-lopez-tecnativa @eduezerouali-tecnativa we added this function in the migration to resolve the problem, could you add and test, thanks:
_prepareTransactionRouteParams() {
const transactionRouteParams = this._super(...arguments);
// eslint-disable-next-line no-undef
const cartValueEl = document.querySelector(".oe_currency_value");
const cartValue = cartValueEl?.textContent || "0";
transactionRouteParams.amount = Number(cartValue);
return transactionRouteParams;
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Alexgars73 That problem was already solve or you are talking about express checkout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eduezerouali-tecnativa i'm getting the same error that @carlos-lopez-tecnativa mentioned "The cart has been updated. Please refresh the page".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Alexgars73 try on runboat, it is working fine.


e9cb772 to
1d1663f
Compare
|
@carlos-lopez-tecnativa thanks for your review! changes you requested done. Couldn't replicate what your error. Could you give more details about that? |
| order.amount_payment_fee = sum( | ||
| order.order_line.filtered("payment_fee_line").mapped("price_total") | ||
| order.order_line.filtered("payment_fee_line").mapped( | ||
| "price_subtotal" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why price_subtotal??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be price_total
| charge_fee_description = fields.Text( | ||
| "Fee Description", compute="_compute_charge_fee_description" | ||
| ) | ||
| charge_fee_product_id = fields.Many2one("product.product", string="Fee Product") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this product should have a domain that allows selecting only records of type service. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you. Makes sense, as I can't see a possible scenario where that fee could be a good
| browser.location.href = | ||
| "/shop/payment?provider_id=" + | ||
| providerId + | ||
| "&payment_option_id=" + | ||
| paymentOptionId; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I attached a video. I believe the problem occurs when you have two or more payment providers. In this case, I installed Demo and Wire Transfer and selected the second payment option. The window flickers, and internally the sale order changes when another payment method is selected. So, I think the window is not being refreshed correctly.
website_sale_charge_payment_fee.mp4
1d1663f to
b1edf83
Compare
carlos-lopez-tecnativa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working fine, just a doubt about the class change. I just want to understand the change. Other than that, LGTM.
| with RecordCapturer(self.env["sale.order"], []) as capture: | ||
| self.start_tour("/shop", "payment_fee_tour", login="portal", step_delay=100) | ||
| created_order = capture.records | ||
| price = 10 / 100 * 99.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you avoid depending on demo data to prevent external changes from altering the behavior here?
| @http.route( | ||
| "/shop/payment", type="http", auth="public", website=True, sitemap=False | ||
| ) | ||
| def shop_payment(self, **post): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class PaymentPortal, doesn’t have the shop_payment function in the payment module. This method is added in the website_sale module. Why did you change the class to inherit from it instead of inheriting from the WebsiteSale class?
b1edf83 to
b9e382d
Compare
|
@carlos-lopez-tecnativa Changes done, no it does not depends on demo data for test. |
b9e382d to
d20d908
Compare
|
@eduezerouali-tecnativa I tested and it is not working well when you try to pay directly with X payment. |
| <field name="inherit_id" ref="payment.payment_provider_form" /> | ||
| <field name="arch" type="xml"> | ||
| <xpath expr="//notebook" position="inside"> | ||
| <page string="Charge payment fee"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eduezerouali-tecnativa I tested and it is not working well when you try to pay directly with X payment.
@Reyes4711-S73 The case you mean is when the payment provider supports express checkout, but in this case, it’s out of scope for this module.
@eduezerouali-tecnativa You can add it to the ROADMAP and hide the page in this case.
I don’t know which payment providers support express checkout apart from Demo, which is never used in the real world.
| <page string="Charge payment fee"> | |
| <page string="Charge payment fee" invisible="allow_express_checkout"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented your suggestions, anyways i think that stripe used it too. Also add it to roadmap. Thanks :)
| ) | ||
| cls.product_product_optional = cls.env["product.template"].create( | ||
| { | ||
| "name": "Oprional", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "name": "Oprional", | |
| "name": "Optional", |
| "is_published": True, | ||
| "sale_ok": True, | ||
| "taxes_id": [Command.clear()], | ||
| "optional_product_ids": [Command.set(cls.product_product_optional.ids)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the optional product really necessary? Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really necessary, i did that,just to not adjust the tour. Anyways i did adjust the tour. :)
d20d908 to
9c7ddf1
Compare
carlos-lopez-tecnativa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
/ocabot migration website_sale_charge_payment_fee |
|
@pilarvargas-tecnativa please could you review |








cc @Tecnativa TT58475
ping @pilarvargas-tecnativa @carlos-lopez-tecnativa
Migrated from #1047
Supersedes #1047