[IMP] De-normalizing sale.order to add applicable rate on the sale.order #25089

nseinlet · 2018-06-06T09:02:13Z

This speed up a lot reports like sale.report when multiple currencies with multiple rates are used, even with few currencies and few rates.

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

nseinlet · 2018-06-06T09:02:47Z

@odony added rate on sale.order to speed up sale report.

odony

Besides the runbot errors, I would have expected to add denormalized subtotals/totals fields, similarly to what is done in invoices, rather than just the rate.
It does seem cheaper to only store the rate, though, so perhaps @qdp-odoo could help us and explain why it was considered better to include the full amounts for invoices?

nseinlet · 2018-06-07T09:13:36Z

I added only the rate because :

It avoid to store many times the same information for companies using only one currency
It's easy to add computed stored fields in custos (x_* fields) when you have the rate, so it's easy to add for the ones who really need it
the sale report needs the rate for the lines, so it had required to add also the result of computation on the lines
this will trigger the re-computation less often

jem-odoo

Smalls changes required, and some questions.
Otherwise, I totally agree with the principe ;)

jem-odoo · 2018-08-17T11:29:37Z

addons/sale/models/sale.py

+    @api.depends('pricelist_id', 'date_order', 'company_id')
+    def _compute_rate(self):
+        for order in self:
+            order.currency_rate = order.currency_id._get_rates(order.company_id, order.date_order).get(order.currency_id.id, 1.0) if order.currency_id else 1.0


if order.currency_id seems useless since the currency_idfield is required.

jem-odoo · 2018-08-17T11:30:44Z

addons/sale/models/sale.py

@@ -157,7 +163,8 @@ def _compute_tax_id(self):
    amount_untaxed = fields.Monetary(string='Untaxed Amount', store=True, readonly=True, compute='_amount_all', track_visibility='onchange')
    amount_tax = fields.Monetary(string='Taxes', store=True, readonly=True, compute='_amount_all')
    amount_total = fields.Monetary(string='Total', store=True, readonly=True, compute='_amount_all', track_visibility='always')
-
+    currency_rate = fields.Float(digits=(12, 6), default=1.0, help='The rate of the currency to the currency of rate 1 applicable at the date of the order', store=True, readonly=True, compute='_compute_rate')


Not important: order the kwargs; string, compute, store, digits, readonly and help.
Not sur sure the default value is important as this is a computed field.

jem-odoo · 2018-08-17T11:33:38Z

addons/sale/report/report_all_channels_sales.py

@@ -37,10 +36,10 @@ def _so(self):
                    so.user_id AS user_id,
                    pt.categ_id AS categ_id,
                    so.company_id AS company_id,
-                    sol.price_total / COALESCE(cr.rate, 1.0) AS price_total,
+                    sol.price_total / CASE COALESCE(so.currency_rate, 0) WHEN 0 THEN 1.0 ELSE so.currency_rate END AS price_total,


If the so.currency_rate is NULL, then the coalesce will return 0 so we enter the WHEN and take the THEN value. Isn't it simpler to use COALESCE(so.currency_rate, 1.0) directly, or did I miss something ?

(same for the other SQL report ;) )

@jem-odoo if currency_rate is equal to 0, I would like to avoid a 0 division. I can change the COALESCE with 1.0, but the case currency_rate=0 will still be possible.

Indeed ! COALESCE take the first not NULL value, and not the one not evaluated to False. My bad ^^"

nseinlet · 2018-08-20T07:14:36Z

@jem-odoo Other changes done

nhomar · 2018-08-20T07:47:54Z

addons/sale/models/sale.py

+    @api.depends('pricelist_id', 'date_order', 'company_id')
+    def _compute_rate(self):
+        for order in self:
+            order.currency_rate = order.currency_id._get_rates(order.company_id, order.date_order).get(order.currency_id.id, 1.0)


There is a pretty common use case where the rate is agreed between parts.

Example:

In México the rate in a sale order is agreed at the date of the closing date.

In Costa Rica the rate is at the moment of the payment.

In Canada/EEUU relationships the Rate can be set once per year instead per transactions.

In overinflationary countries the rate can be set at the moment of payment.

If you make this field Record = True and make possible to use such field able to be set (with an inverse) this will help A LOT, and almost effortless.

Feature:

By default this logic is perfect, but order by order this can be set manually or using any other logic with a simple write.

The only thing you need to do is allow the readonly=False and set the inverse to allow securelly write it.

Thanks in advance for hear me. cc @odony @qdp-odoo

cc @jem-odoo

This commit aims to improve the performance, and unfortunately not changing the existing feature. Since we are near the release what you asked can not be done, but I transfert your request on the Product Owner Team ;)

qdp-odoo · 2018-08-20T09:15:53Z

@odony about the fact that we store all the different amounts in the accounting: that's only code legacy. It has been done that way and nobody never challenged it before... but I suppose it'sit's okay since we want to be able to order invoices by these computed totals, and use them easily in the invoice analysis view...

Impacted modules: sale, sale_margin, sale_timesheet, pos_sale This speed up a lot reports like sale.report when multiple currencies with multiple rates are used, even with few currencies and few rates. Using `_select_companies_rates` in SQL view bring lots of JOINs, which slow down the view generation. This commit adds a computed stored field containing the currency rate at the order date to speed up report generation (database schema denormalisation). This commit also use this new column on sale.report extensions and on the project.profitability.report in sale_timesheet module. Since a4d6bff, the pos.report is included in the sale.report. So, the same change had to be applied on the pos.order model. Task #1877418 Closes odoo#25089

Impacted modules: sale, sale_margin, sale_timesheet, pos_sale This speed up a lot reports like sale.report when multiple currencies with multiple rates are used, even with few currencies and few rates. Using `_select_companies_rates` in SQL view bring lots of JOINs, which slow down the view generation. This commit adds a computed stored field containing the currency rate at the order date to speed up report generation (database schema denormalisation). This commit also use this new column on sale.report extensions and on the project.profitability.report in sale_timesheet module. Since a4d6bff, the pos.report is included in the sale.report. So, the same change had to be applied on the pos.order model. Task #1877418 Closes #25089

jem-odoo · 2018-08-22T13:54:28Z

Merged at e8f01b9

C3POdoo added the RD research & development, internal work label Jun 7, 2018

odony reviewed Jun 7, 2018

View reviewed changes

nseinlet force-pushed the master-improvesalereportspeed-nse branch from a26d62d to 59aeb05 Compare June 8, 2018 08:18

jem-odoo suggested changes Aug 17, 2018

View reviewed changes

nseinlet force-pushed the master-improvesalereportspeed-nse branch from 59aeb05 to 19fcec6 Compare August 20, 2018 07:13

nhomar reviewed Aug 20, 2018

View reviewed changes

jem-odoo force-pushed the master-improvesalereportspeed-nse branch from 19fcec6 to 0f26642 Compare August 20, 2018 14:32

jem-odoo force-pushed the master-improvesalereportspeed-nse branch from 2ea5a28 to 6e55e97 Compare August 21, 2018 08:43

jem-odoo force-pushed the master-improvesalereportspeed-nse branch from 6e55e97 to 32c1e08 Compare August 21, 2018 09:16

jem-odoo closed this Aug 22, 2018

jem-odoo deleted the master-improvesalereportspeed-nse branch August 22, 2018 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IMP] De-normalizing sale.order to add applicable rate on the sale.order #25089

[IMP] De-normalizing sale.order to add applicable rate on the sale.order #25089

nseinlet commented Jun 6, 2018

nseinlet commented Jun 6, 2018

odony left a comment

nseinlet commented Jun 7, 2018 •

edited

jem-odoo left a comment

jem-odoo Aug 17, 2018

jem-odoo Aug 17, 2018

jem-odoo Aug 17, 2018

jem-odoo Aug 17, 2018

nseinlet Aug 20, 2018

jem-odoo Aug 20, 2018

nseinlet commented Aug 20, 2018

nhomar Aug 20, 2018

nhomar Aug 20, 2018

jem-odoo Aug 20, 2018

qdp-odoo commented Aug 20, 2018

jem-odoo commented Aug 22, 2018

[IMP] De-normalizing sale.order to add applicable rate on the sale.order #25089

[IMP] De-normalizing sale.order to add applicable rate on the sale.order #25089

Conversation

nseinlet commented Jun 6, 2018

nseinlet commented Jun 6, 2018

odony left a comment

Choose a reason for hiding this comment

nseinlet commented Jun 7, 2018 • edited

jem-odoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nseinlet commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qdp-odoo commented Aug 20, 2018

jem-odoo commented Aug 22, 2018

nseinlet commented Jun 7, 2018 •

edited