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

[17.0][ADD] product_total_weight: New module product_weight #1799

Open
wants to merge 1 commit into
base: 17.0
Choose a base branch
from

Conversation

peluko00
Copy link

@peluko00 peluko00 commented Dec 4, 2024

This module adds a field in different views associated to product to see how many kilograms are depending of the weight field thats appears on inventory tab of product form.

cc https://github.com/APSL 164365
@miquelalzanillas @lbarry-apsl @javierobcn @mpascuall @BernatObrador @ppyczko please review

@peluko00 peluko00 force-pushed the 17.0-add-product_weight branch from 36069fc to 901ce47 Compare December 4, 2024 12:24
Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

Hi. Thanks for sharing your module.

Change Proposal : I think the name should be better "product_total_weight".

Request Changes :

  • you should handle conversion in all computation. for the time being, if you have a quant with 1 gram and a quant with 1 kilo, the total displayed is 2 kilo. That is wrong.
  • you should not hardcode "kg" in the name, as it is possible to use odoo with other anglo saxon unit system. (See odoo core option).

_inherit = "product.product"

total_weight = fields.Float(
"Total Weight(kg)", compute="_compute_total_weight", store=True
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 not be stored, as it depends on a non stored field. (qty_available) (same for field on product.template).

"Total Weight(kg)", compute="_compute_total_weight", store=True
)

@api.depends("qty_available")
Copy link
Contributor

Choose a reason for hiding this comment

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

this should depends on weight also.

position="inside"
>
<br />
Total Weight: <field name="total_weight" /> kg
Copy link
Contributor

Choose a reason for hiding this comment

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

hardcoded kg can be replaced by the core configuration.

@peluko00 peluko00 force-pushed the 17.0-add-product_weight branch from 901ce47 to 4461864 Compare December 4, 2024 15:14
@peluko00
Copy link
Author

peluko00 commented Dec 4, 2024

Hi. Thanks for sharing your module.

Change Proposal : I think the name should be better "product_total_weight".

Request Changes :

  • you should handle conversion in all computation. for the time being, if you have a quant with 1 gram and a quant with 1 kilo, the total displayed is 2 kilo. That is wrong.
  • you should not hardcode "kg" in the name, as it is possible to use odoo with other anglo saxon unit system. (See odoo core option).

Thanks for the anottations, i refactor the code but i can't undestand what do you want to do when i want to show the uom units in the tree views.

@peluko00 peluko00 force-pushed the 17.0-add-product_weight branch from 4461864 to 6ca6d00 Compare December 4, 2024 15:16
Copy link

@BernatObrador BernatObrador left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@mpascuall mpascuall left a comment

Choose a reason for hiding this comment

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

LGTM, tested in runboat

@peluko00 peluko00 changed the title [ADD] product_weight: New module product_weight [ADD] product_total_weight: New module product_weight Dec 5, 2024
@peluko00 peluko00 changed the title [ADD] product_total_weight: New module product_weight [17.0][ADD] product_total_weight: New module product_weight Dec 5, 2024
@legalsylvain
Copy link
Contributor

Thanks for the anottations, i refactor the code but i can't undestand what do you want to do when i want to show the uom units in the tree views.

I think you should not display it. If you really want, you can simply add a new column, with this field "weight_uom_name".
What do you think ?

@peluko00
Copy link
Author

peluko00 commented Dec 5, 2024

Thanks for the anottations, i refactor the code but i can't undestand what do you want to do when i want to show the uom units in the tree views.

I think you should not display it. If you really want, you can simply add a new column, with this field "weight_uom_name". What do you think ?

Not necessary in my opinion. Can you review again please @legalsylvain. Thanks!

Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

two minors comments. otherwise LGTM.
Note : I just saw that the module was using product_weight from product_logistics_uom. i don't use this module, so I'm not totally sure how to handle this new weight_uom_id.

I let other people reviewing this part.

thanks for the changes.

product_total_weight/views/stock_quant_views.xml Outdated Show resolved Hide resolved
product_total_weight/readme/DESCRIPTION.md Outdated Show resolved Hide resolved
@legalsylvain legalsylvain requested review from legalsylvain and removed request for legalsylvain December 5, 2024 16:10
@peluko00 peluko00 force-pushed the 17.0-add-product_weight branch from 6ca6d00 to f6fe595 Compare December 9, 2024 11:34
@peluko00
Copy link
Author

peluko00 commented Dec 9, 2024

two minors comments. otherwise LGTM. Note : I just saw that the module was using product_weight from product_logistics_uom. i don't use this module, so I'm not totally sure how to handle this new weight_uom_id.

I let other people reviewing this part.

thanks for the changes.

Yes, the module depends of product_logistics_uom because you have option to choose the uom_id of the product on logistics section.
Changes done. Thanks!
It's ready for merge @legalsylvain ?

@peluko00 peluko00 force-pushed the 17.0-add-product_weight branch from f6fe595 to 1b10676 Compare December 9, 2024 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants