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] sale, _management: portal sale order optional products #32980

Closed

Conversation

kebeclibre
Copy link
Contributor

Forward Port 0c46c8a from v10.0
has dome flows, namely, the new feature of grouping the taxes by tax group was not supported

Now it is supported: when modifying an optional product's quantity, every field is updated with the new values

OPW 1974708

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

Copy link
Contributor

@seb-odoo seb-odoo left a comment

Choose a reason for hiding this comment

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

Here is a quick review, I didn't test it.

I assume this is only for the tax table, and not taking into account discount & pricelist/unit price?

Also commit message contains some errors:

  • dome->some?
  • missing dot at end of sentence
  • _management? Maybe do sale(_management), or put both in full if there is space for it
  • title not starting with verb "this commit will..."

</tr>
</t>
</table>
<t t-call='sale.portal_content_tax_table' />
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately you cannot change the view like that in stable, because it will break all custo xpathing it. You'll have to do a duplicate of the view if you need to render it from the controller too.

@@ -54,6 +54,7 @@ def update_line_dict(self, line_id, remove=False, unlink=False, order_id=None, a
'order_amount_total': format_price(order_sudo.amount_total),
'order_amount_untaxed': format_price(order_sudo.amount_untaxed),
'order_amount_tax': format_price(order_sudo.amount_tax),
'order_tax_table': request.env['ir.ui.view'].render_template('sale.portal_content_tax_table', {'sale_order': order_sudo}),
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to check if the new view exists, because it will not most of the time and this will crash (because no upgrade by default).

.oe_sale_order_tax_table {
margin-bottom: 0;
margin-top: -1rem;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

new line at end of file

@@ -520,7 +520,7 @@
<div id="total" class="row" name="total" style="page-break-inside: avoid;">
<div t-attf-class="#{'col-4' if report_type != 'html' else 'col-sm-7 col-md-5'} ml-auto">
<table class="table table-sm">
<tr class="border-black" style="border-bottom:1px solid #dddddd;">
<tr class="border-black" style="">
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure about this change? I remember I had trouble when I tried to remove it, but maybe it was on another template like the report.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rendering is the same visually
That border specifically will then be handled by the tax_table


.oe_sale_order_tax_table {
margin-bottom: 0;
margin-top: -1rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

margin-top -1rem is needed because default Bootstrap table or table-sm classes have a margin-bottom of 1rem. This is just for compensating (not really elegant though)

@C3POdoo C3POdoo added the OE the report is linked to a support ticket (opw-...) label Apr 26, 2019
@seb-odoo seb-odoo added the RD research & development, internal work label Apr 26, 2019
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Apr 26, 2019
@kebeclibre kebeclibre force-pushed the 12.0-portal-optional-yet-again-lpe branch 2 times, most recently from b09c292 to 5afb62e Compare April 26, 2019 14:42
@kebeclibre
Copy link
Contributor Author

@seb-odoo
changes made

Copy link
Contributor

@seb-odoo seb-odoo left a comment

Choose a reason for hiding this comment

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

Quick review again, still not tested.

Good catch on the translation! I would have forgotten that.

'order_line_product_uom_qty': str(quantity),
'order_line_price_total': format_price(order_line.price_total),
'order_line_price_subtotal': format_price(order_line.price_subtotal),
'order_amount_total': format_price(order_sudo.amount_total),
'order_amount_untaxed': format_price(order_sudo.amount_untaxed),
'order_amount_tax': format_price(order_sudo.amount_tax),
'order_amount_undiscounted': format_price(order_sudo.amount_undiscounted),
'order_totals_table': request.env['ir.ui.view'].render_template('sale.sale_order_portal_content_totals_table', {'sale_order': order_sudo}),
Copy link
Contributor

Choose a reason for hiding this comment

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

you forgot to remove this one

@@ -127,7 +127,7 @@
<t t-set="classes" t-value="'col-lg-auto d-print-none'"/>

<t t-set="title">
<h2 class="mb-0"><b t-field="sale_order.amount_total"/> </h2>
<h2 class="mb-0"><b t-field="sale_order.amount_total" data-id="total_amount"/> </h2>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a benefit to use data-id instead of a class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, no, there is none, but identifying fields started that way, so I'm just continuing in that direction.
Plus, using CSS class not to apply style but for identifying data seems odd

@@ -582,4 +584,47 @@
</div>
</template>

<template id="sale_order_portal_content_totals_table">
<table class="table table-sm">
<tr class="border-black" style="border-bottom:1px solid #dddddd;">
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably don't want to copy this inline style if avoidable?

</td>
</tr>
<t t-foreach="sale_order.amount_by_group" t-as="amount_by_group">
<tr style="border-bottom:1px solid #dddddd;">
Copy link
Contributor

Choose a reason for hiding this comment

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

inline style?

@@ -66,19 +67,20 @@ if (!$('.o_portal_sale_sidebar').length) {
* when the quantity of a product has changed
*
* @private
* @param {Int} order_id
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we usually write it {integer} (and to be exact there is only Number in JS, so we should use that instead and specify in the description that integer is expected).

@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Apr 26, 2019
Forward Port 0c46c8a from v10.0
has dome flows, namely, the new feature of grouping the taxes by tax group was not supported

Now it is supported: when modifying an optional product's quantity, every field is updated with the new values

OPW 1974708
@kebeclibre kebeclibre force-pushed the 12.0-portal-optional-yet-again-lpe branch from 5afb62e to 20d0a08 Compare April 29, 2019 07:47
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Apr 29, 2019
@kebeclibre
Copy link
Contributor Author

robodoo r+

@robodoo robodoo added r+ 👌 CI 🤖 Robodoo has seen passing statuses labels Apr 29, 2019
robodoo pushed a commit that referenced this pull request Apr 29, 2019
Forward Port 0c46c8a from v10.0
has dome flows, namely, the new feature of grouping the taxes by tax group was not supported

Now it is supported: when modifying an optional product's quantity, every field is updated with the new values

OPW 1974708

closes #32980

Signed-off-by: Lucas Perais (lpe) <lpe@odoo.com>
@robodoo
Copy link
Contributor

robodoo commented Apr 29, 2019

Merged, thanks!

@robodoo robodoo closed this Apr 29, 2019
@fw-bot fw-bot deleted the 12.0-portal-optional-yet-again-lpe branch October 19, 2019 10:47
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 OE the report is linked to a support ticket (opw-...) RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants