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

[MIG][16.0] product total weight from packaging #1221

Merged

Conversation

glitchov
Copy link
Contributor

Migration from 14.0

@glitchov glitchov force-pushed the 16.0-mig-product_total_weight_from_packaging branch 2 times, most recently from ccbd12e to 0ae6102 Compare November 30, 2022 15:48
Copy link
Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

Code review

Copy link
Member

@FrancoMaxime FrancoMaxime left a comment

Choose a reason for hiding this comment

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

code review

Copy link
Contributor

@xavier-bouquiaux xavier-bouquiaux left a comment

Choose a reason for hiding this comment

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

LGTM technical side

Copy link
Contributor

@jbaudoux jbaudoux left a comment

Choose a reason for hiding this comment

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

The max_weight is a concept that was introduced in odoo delivery module to define the transporter weight limit for the shipped packaging. Before v16, the transporter packaging and product packaging was the same model product.packaging. Now the transporter packaging is moved to the model stock.package.type.
In v13, max_weight defined in delivery module on product.packaging was used also for product packaging dimension.
In v14, the weight field was added on the product.packaging and the max_weight was still coming from delivery module. This was introducing inconsistency as some modules migrated from v13 to v14 still rely on max_weight and some rely on the new weight field.
In v16, we have to drop this max_weight on product.packaging as the delivery module moved it to stock.package.type and we should rely only on the weight field.
The migration of the product_packaging_dimension #1161 has to be fixed to not expose the old max_weight field and any data on max_weight must be moved to weight if empty.

This module also deserves a big warning that it is lacking the weight uom conversions as it depends on product_packaging_dimension that depends on product_logistics_uom that allows to set a uom on the weight. The sum in this module is assuming all weights are in kg

product_total_weight_from_packaging/models/product.py Outdated Show resolved Hide resolved
product_total_weight_from_packaging/models/product.py Outdated Show resolved Hide resolved
@victorvermot
Copy link

victorvermot commented Jul 12, 2023

Hello @glitchov, @lmignon I have added a small pr to fix the situation raised by @jbaudoux

@leemannd
Copy link
Contributor

Hello @glitchov , there are conflicts with the branch 16.0, by any chance could you have a look at it and maybe perform a rebase?

@lmignon lmignon force-pushed the 16.0-mig-product_total_weight_from_packaging branch from 2a63fdc to 399adb0 Compare July 12, 2023 13:05
@lmignon
Copy link
Contributor

lmignon commented Jul 12, 2023

@leemannd @victorvermot This branch is now rebased and comments from @jbaudoux are included

Copy link
Contributor

@jbaudoux jbaudoux left a comment

Choose a reason for hiding this comment

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

Thanks @lmignon for the update.

Not related to the migration but this module still deserves a big warning that it is lacking the weight uom conversions as it depends on product_packaging_dimension that depends on product_logistics_uom that allows to set a uom on the weight. The sum in this module is assuming all weights are in kg

@lmignon
Copy link
Contributor

lmignon commented Jul 12, 2023

@jbaudoux Warning added...
image

@lmignon lmignon force-pushed the 16.0-mig-product_total_weight_from_packaging branch from ea04aa5 to e3ebe65 Compare July 12, 2023 13:57
glitchov and others added 2 commits November 6, 2023 15:06
…x_weight


The max_weight is a concept that was introduced in odoo delivery module to define the transporter weight limit for the shipped packaging. Before v16, the transporter packaging and product packaging was the same model product.packaging. Now the transporter packaging is moved to the model stock.package.type.
In v13, max_weight defined in delivery module on product.packaging was used also for product packaging dimension.
In v14, the weight field was added on the product.packaging and the max_weight was still coming from delivery module. This was introducing inconsistency as some modules migrated from v13 to v14 still rely on max_weight and some rely on the new weight field.
In v16, we have to drop this max_weight on product.packaging as the delivery module moved it to stock.package.type and we should rely only on the weight field.
The migration of the product_packaging_dimension OCA#1161 has to be fixed to not expose the old max_weight field and any data on max_weight must be moved to weight if empty.

This module also deserves a big warning that it is lacking the weight uom conversions as it depends on product_packaging_dimension that depends on product_logistics_uom that allows to set a uom on the weight. The sum in this module is assuming all weights are in kg

Co-authored-by: Jacques-Etienne Baudoux <[email protected]>
@glitchov glitchov force-pushed the 16.0-mig-product_total_weight_from_packaging branch from e3ebe65 to 637765a Compare November 6, 2023 15:51
@dreispt
Copy link
Member

dreispt commented Dec 22, 2023

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-1221-by-dreispt-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 9c464c6 into OCA:16.0 Dec 22, 2023
7 of 9 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 258d5ac. Thanks a lot for contributing to OCA. ❤️

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