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

[16.0][MIG] sale_order_weight: Migration to 16.0 (from 12.0) #3414

Open
wants to merge 22 commits into
base: 16.0
Choose a base branch
from

Conversation

legalsylvain
Copy link
Contributor

@legalsylvain legalsylvain commented Nov 18, 2024

  • Migration of the module sale_order_weight from OCA/sale-workflow/12.0 to 16.0
  • Note another module with the same name exists with similar features (smaller scope) in OCA/sale-reporting/14.0. This PR includes the feature of the "other" module.

Description

Configuration

  • Go to “Sales > Configuration > Settings”

configuration

  • Display Order weight is checked by default.
  • The weight display at line level is hidden by default, to avoid burdening the PDF report.

Usage

  • Go to a sale order.

The ordered weights are displayed on each order lines and at order level.

sale_order_form

Note: The weight are also available on the order tree view. (optional="hide" by default)

  • Print a sale order.

sale_order_report

Change Log

[MIG] sale_order_weight: Migration to 16.0 (from 12.0)

[FIX] change weight on product now change automatically the weight of the sale orders.
So it is not necessary to 'recompute' the weight of the orders, and the value is always correct.
(Also the column 'Unit Weight' is removed at sale.order.line level)

[FIX] Now handle correctly conversion. Use Case :
- product.product (weight : 50kg ; Uom : Unit)
- sale.order.line (Qty : 10 ; UoM : dozen)
-> Before, the weight was 50 * 10 = 500 and was wrong.
-> Now the weight is 50 * 10 * 12 = 6000

[ADD] migration script to recompute correctly values that can be incorrect and obsolete (due to conversion errors)

[IMP] Display weight on sale order report.
Note: this code comes from the other OCA modules name sale_order_weight,
present in the OCA repo sale-reporting, in V14:
See: https://github.com/OCA/sale-reporting/tree/14.0/sale_order_weight

[IMP] add configuration to display the weight or not on the sale order report.
(configuration for the total order, and the order on each lines)

[REM] remove 'delivered' weight for the following reasons:
- The delivered quantity on sale order lines is a non stored field.
  So, the recompute of the field fails, and the field total_delivered_weight is not updated correctly.
- It is legit to want to have only Ordered weight and not Delivered Weight, and the addition of the delivered weight
  make the database bigger, and the process slower.
Better to create a dedicated module for delivered weight

manuelcalerosolis and others added 21 commits November 18, 2024 16:58
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: sale-workflow-12.0/sale-workflow-12.0-sale_order_weight
Translate-URL: https://translation.odoo-community.org/projects/sale-workflow-12-0/sale-workflow-12-0-sale_order_weight/
…e_weight. (by setting correct value in the creation of the sale order lines)
…sale.order.line. Otherwise, if stored field depends on non stored fields, too much recomputation are done.

Also allow to make analysis at order lines level
…and accounting information (after taxes included)
… so that the user can see if a weight is missing on a product

Also because user should have the possibility to change the unit weight if for some reason, a product has a different weight for a single order.
…e view for consistency reason. total_delivered_weight is also hidden. Also order total weight is enough for user
…t field.

- fast precompute of total weights on sale.order.line and sale.order tables
…t_weight field. - fast precompute of total weights on sale.order.line and sale.order tables
@legalsylvain legalsylvain force-pushed the 16.0-mig-sale_order_weight branch 2 times, most recently from 09b050c to 5587baf Compare November 19, 2024 17:00
@legalsylvain legalsylvain marked this pull request as ready for review November 19, 2024 17:01
@legalsylvain
Copy link
Contributor Author

/ocabot migration sale_order_weight

CC : @OCA/crm-sales-marketing-maintainers

@OCA-git-bot
Copy link
Contributor

Sorry @legalsylvain you are not allowed to mark the addon tobe migrated.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

@rousseldenis
Copy link
Contributor

/ocabot migration sale_order_weight

@OCA-git-bot OCA-git-bot mentioned this pull request Nov 19, 2024
99 tasks
Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

@legalsylvain for big refactoring I wouldn't be better to migrate the module 1st as not installable? I think that would ease the review of the changes. Not so big here... Not a blocker, just a suggestion.
One more thing... A reminder that we have commit msg guidelines and that the details of a change go to the description of the commit.

Example:

[FIX] sale_order_weight : store intermediate total_ordered_weight on sale.order.line. Otherwise, if stored field depends on non stored fields, too much recomputation are done.

Also allow to make analysis at order lines level

to

[FIX] sale_order_weight: improve total_ordered_weight compute perf

Store intermediate total_ordered_weight on sale.order.line.
Otherwise, if stored field depends on non stored fields, too many re-computations are done.
It also allow to make analysis at order lines level.


@api.depends("order_line.total_ordered_weight")
def _compute_total_ordered_weight(self):
for order in self:
Copy link
Contributor

Choose a reason for hiding this comment

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

this field can be used in tree views too. It would be better to use a read_group + sum function

Copy link
Contributor Author

@legalsylvain legalsylvain Nov 20, 2024

Choose a reason for hiding this comment

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

Hi. It is a stored field. so the function will not be called by a tree view displaying 10000 sale.order.line. (for exemple).
or did I missed something ?

"product_id.weight", "product_id.uom_id", "product_uom", "product_uom_qty"
)
def _compute_total_ordered_weight(self):
for line in self.filtered(lambda x: x.product_id):
Copy link
Contributor

Choose a reason for hiding this comment

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

pre-filtering in such cases causes to loop twice on the same records. As you have a loop, just filter inside it:

for line in self:
    if not line.product_id:
        continue
[...]

BTW... can you have a line w/o product?

Copy link
Contributor Author

@legalsylvain legalsylvain Nov 20, 2024

Choose a reason for hiding this comment

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

pre-filtering in such cases causes to loop twice on the same records. As you have a loop, just filter inside it:

Even though I've been working with odoo for a few years now, I still find it hard to understand certain optimisations for using ORM. I need to study again the odoo experience talks!

Thanks for your review. Fixed !

BTW... can you have a line w/o product?

Yes, in sale.order.line model, you have 3 "type" of lines. "product", "section", and "note". section and note doesn't have a product_id defined.

Copy link
Member

Choose a reason for hiding this comment

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

Although it's true that technically there are 2 loops, the performance penalty is insignificant - as the first loop caches data, and then it's only passes again through cached data - while writing 3 lines of code instead of 1 makes the code uglier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pedrobaeza : thanks for your extra explanation.

@simahawk Are you agree with @pedrobaeza remarks ? what is the definive syntax to use ?

Thanks !

<xpath expr="//t[@t-set='address']" position="after">
<t
t-set="weight_uom_name"
t-value="doc.env['product.template']._get_weight_uom_name_from_ir_config_parameter()"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would delegate these lookups to a method on sale_order _get_report_weight_params or smth similar.
Eg: weight_uom_name, weight_precision = doc._get_report_weight_params()

Copy link
Contributor Author

@legalsylvain legalsylvain Nov 20, 2024

Choose a reason for hiding this comment

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

Hi.

It's a copy past of the existing V14 module https://github.com/OCA/sale-reporting/blob/14.0/sale_order_weight/views/sale_report.xml#L15-L22

_get_weight_uom_name_from_ir_config_parameter is a native odoo function that is called always that way.

./odoo/addons/delivery/models/stock_picking.py:        return self.env['product.template']._get_weight_uom_name_from_ir_config_parameter()
./odoo/addons/delivery/models/stock_picking.py:            package.weight_uom_name = self.env['product.template']._get_weight_uom_name_from_ir_config_parameter()
./odoo/addons/delivery/models/stock_picking.py:        return self.env['product.template']._get_weight_uom_name_from_ir_config_parameter()
./odoo/addons/delivery/models/stock_picking.py:            package.weight_uom_name = self.env['product.template']._get_weight_uom_name_from_ir_config_parameter()
./odoo/addons/delivery/models/delivery_carrier.py:            weight_uom_name = self.env['product.template']._get_weight_uom_name_from_ir_config_parameter()
./odoo/addons/stock/models/stock_package_type.py:        return self.env['product.template']._get_weight_uom_name_from_ir_config_parameter()
./odoo/addons/stock/models/stock_package_type.py:            package_type.weight_uom_name = self.env['product.template']._get_weight_uom_name_from_ir_config_parameter()
./odoo/addons/stock/models/stock_storage_category.py:        self.weight_uom_name = self.env['product.template']._get_weight_uom_name_from_ir_config_parameter()
./odoo/addons/delivery_stock_picking_batch/models/stock_picking.py:        return self.env['product.template']._get_weight_uom_name_from_ir_config_parameter()
./odoo/addons/delivery_stock_picking_batch/models/stock_picking.py:            picking_type.weight_uom_name = self.env['product.template']._get_weight_uom_name_from_ir_config_parameter()

I don't see the interest to define a new function in python code. I see it more verbose. Could you elaborate ?
Thanks !

[FIX] the weight on sale.order.line is now always correct.

Change weight on product now change automatically the weight of the sale orders.
So it is not necessary to 'recompute' the weight of the orders,
and the value is always correct.
(Also the column 'Unit Weight' is removed at sale.order.line level)

[FIX] Handle correctly UoM conversion.

Use Case :
- product.product (weight : 50kg ; Uom : Unit)
- sale.order.line (Qty : 10 ; UoM : dozen)
-> Before, the weight was 50 * 10 = 500 and was wrong.
-> Now the weight is 50 * 10 * 12 = 6000

[MIG] Add script to fix incorrect weight values

The values can be incorrect (due to conversion errors),
or obsolet (if user didn't click on 'recompute weight')

[IMP] Display weight on sale order report.

Note: this code comes from the other OCA modules name sale_order_weight,
present in the OCA repo sale-reporting, in V14:
See: https://github.com/OCA/sale-reporting/tree/14.0/sale_order_weight

[IMP] Configuration to display the weight or not on the sale order report.

Configuration is available for the total order, and the order on each lines.

[REM] remove 'delivered' weight for the following reasons:

- The delivered quantity on sale order lines is a non stored field.
  So, the recompute of the field fails,
  and the field total_delivered_weight is not updated correctly.
- It is legit to want to have only Ordered weight
  and not Delivered Weight, and the addition of the delivered weight
  make the database bigger, and the process slower.

It is so Better to create a dedicated module for delivered weight
@legalsylvain
Copy link
Contributor Author

Hi @simahawk. thanks a lot for your complete review !

for big refactoring I wouldn't be better to migrate the module 1st as not installable? I think that would ease the review of the changes. Not so big here... Not a blocker, just a suggestion.

Well, in fact, I planned to "simply" migrate this module,but as I worked on it, I realised that there were lots of things to fix and refactor, and the migration commit got bigger and bigger!

I'm sorry if the review is a bit complicated.

One more thing... A reminder that we have commit msg guidelines and that the details of a change go to the description of the commit.

I have rewrote the commit message. Hope it is ok now.

Let me know if there are remaining things to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants