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] stock_secondary_unit #1769

Closed
wants to merge 24 commits into from

Conversation

BT-aleonard
Copy link

@BT-aleonard BT-aleonard commented Jun 21, 2023

sergio-teruel and others added 23 commits June 22, 2023 08:45
Currently translated at 100.0% (15 of 15 strings)

Translation: stock-logistics-warehouse-12.0/stock-logistics-warehouse-12.0-stock_secondary_unit
Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-12-0/stock-logistics-warehouse-12-0-stock_secondary_unit/zh_CN/
Currently translated at 100.0% (15 of 15 strings)

Translation: stock-logistics-warehouse-12.0/stock-logistics-warehouse-12.0-stock_secondary_unit
Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-12-0/stock-logistics-warehouse-12-0-stock_secondary_unit/zh_CN/
On move line creation, the precision of secondary qty rounding is wrong; the unit of measure factor is being used instead of rounding.
…variants view on common form. Create related template secondary unit field.

TT27057
[IMP] stock_secondary_unit: Use product_secondary_unit mixin model from product-attribute repository.
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: stock-logistics-warehouse-15.0/stock-logistics-warehouse-15.0-stock_secondary_unit
Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-15-0/stock-logistics-warehouse-15-0-stock_secondary_unit/
@BT-aleonard BT-aleonard force-pushed the 16.0-mig-stock_secondary_unit branch from d96af01 to e3c72c0 Compare June 22, 2023 07:18
@BT-aleonard BT-aleonard force-pushed the 16.0-mig-stock_secondary_unit branch from e3c72c0 to ea66351 Compare June 22, 2023 07:24
@BT-aleonard
Copy link
Author

@sergio-teruel can we replace this PR in the issues file?

@BT-aleonard BT-aleonard mentioned this pull request Jun 22, 2023
69 tasks
@rousseldenis
Copy link
Contributor

/ocabot migration stock_secondary_unit.

@BT-aleonard It's usually better to add the link to the depending PR in this one description. Thanks

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Jun 22, 2023
@BT-aleonard
Copy link
Author

The PR #1701 is not attended, so I replaced it with this PR.

@BT-aleonard
Copy link
Author

@sergio-teruel Can you please review this PR, thanks.

Copy link
Contributor

@rousseldenis rousseldenis 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

@yajo yajo left a comment

Choose a reason for hiding this comment

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

After double-checking, you can see in the CI you can see you have these warnings:

2023-06-22 07:27:17,318 245 WARNING odoo py.warnings: /opt/odoo/odoo/fields.py:802: UserWarning: Field stock.move.product_uom_qty cannot be precomputed as it depends on non-precomputed field stock.move.secondary_uom_qty
2023-06-22 07:27:17,322 245 WARNING odoo py.warnings: /opt/odoo/odoo/fields.py:802: UserWarning: Field stock.move.line.qty_done cannot be precomputed as it depends on non-precomputed field stock.move.line.secondary_uom_qty

Follow the suggestions below to fix the situation, please.

copy=True,
precompute=True,
)

Copy link
Member

Choose a reason for hiding this comment

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

Please add this code here:

Suggested change
secondary_uom_qty = fields.Float(
compute="_compute_secondary_uom_qty",
precompute=True,
store=True,
)

qty_done = fields.Float(
store=True, readonly=False, compute="_compute_qty_done", precompute=True
)

Copy link
Member

Choose a reason for hiding this comment

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

Also here:

Suggested change
secondary_uom_qty = fields.Float(
compute="_compute_secondary_uom_qty",
precompute=True,
store=True,
)

@sergio-teruel
Copy link
Contributor

Minor commente... I hava commented in this PR OCA/sale-workflow#2645 you can add the precompute attribute in mixin model in product_secondary_unit module

@yajo
Copy link
Member

yajo commented Aug 24, 2023

I was unsure about that because a lot of modules depend on it. I'm afraid it could break other behaviors unexpectedly...

@sergio-teruel
Copy link
Contributor

Hi @BT-aleonard Can you take into account this PR #1606 from V15??

if move.secondary_uom_id:
uom = self.env["uom.uom"].browse(vals["product_uom_id"])
factor = move.secondary_uom_id.factor * uom.factor
move_line_qty = vals.get("product_uom_qty", vals.get("qty_done", 0.0))

Choose a reason for hiding this comment

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

This product_uom_qty doesn't exist in v16.0
Should be replaced by reserved_uom_qty

https://github.com/odoo/odoo/blob/16.0/addons/stock/models/stock_move.py#L1449

>
<xpath expr="//span[@t-field='move_line.product_id']/.." position="after">
<td>
<span t-field="move_line.secondary_uom_qty" />

Choose a reason for hiding this comment

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

If your stock.picking is validated, the table header is displayed ("Secondary Qty") but the layout is not correct, it's shifted. In others states, the layout is correct.
image

@rousseldenis
Copy link
Contributor

@BT-aleonard Could you attend comments ? Moreover, this deserves some little tests. Thanks

@pedrobaeza
Copy link
Member

Please include #1606

@pedrobaeza
Copy link
Member

Please cherry-pick #1948

@rousseldenis
Copy link
Contributor

@BT-dmoreno Maybe someone still active to continue this ?

Comment on lines +52 to +55
if move.secondary_uom_id:
uom = self.env["uom.uom"].browse(vals["product_uom_id"])
factor = move.secondary_uom_id.factor * uom.factor
move_line_qty = vals.get("product_uom_qty", vals.get("qty_done", 0.0))
Copy link

@Rad0van Rad0van Mar 4, 2024

Choose a reason for hiding this comment

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

I am trying to wrap my head around this because whatever I do the lines end up with 0 secondary qty and None secondary uom id. Why? Because move.secondary_uom_id is None. I cannot find any piece of code that would ever be attempting to write any value there. And I have looked and tested versions 15 and 14 as well. Any insights on this?

Copy link

Choose a reason for hiding this comment

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

So when I confirm Purchase Order and have a look at associated stock picking this is what I get always. Or am I expecting wrongly to have secondary qty and uom displayed here?
image

Copy link

Choose a reason for hiding this comment

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

In my opinion stock.move is missing something like this:

    @api.model_create_multi
    def create(self, vals_list):
        for vals in vals_list:
            product = self.env["product.product"].browse(vals.get("product_id", False))
            if product:
                vals.update(
                    {
                        "secondary_uom_id": product.stock_secondary_uom_id.id
                        or product.product_tmpl_id.stock_secondary_uom_id.id,
                    }
                )
        return super().create(vals_list)

After that the result is what (at least) I expect:
image

distinct_fields += ["secondary_uom_id"]
return distinct_fields


Copy link

@Rad0van Rad0van Mar 5, 2024

Choose a reason for hiding this comment

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

This ensures calculation of secondary qty. See comments in main thread.

Suggested change
@api.model_create_multi
def create(self, vals_list):
for vals in vals_list:
product = self.env["product.product"].browse(vals.get("product_id", False))
if product:
vals.update(
{
"secondary_uom_id": product.stock_secondary_uom_id.id
or product.product_tmpl_id.stock_secondary_uom_id.id,
}
)
return super().create(vals_list)

@jcadhoc
Copy link
Contributor

jcadhoc commented Jun 14, 2024

Hello!
What is the state of this pr? I tried and it works fine more or less but i have an apreciation to make.

Is it active?

@pilarvargas-tecnativa
Copy link
Contributor

Please include #2080

@jbaudoux
Copy link
Contributor

jbaudoux commented Oct 7, 2024

Replaced by #1985

@jbaudoux jbaudoux closed this Oct 7, 2024
@jbaudoux
Copy link
Contributor

jbaudoux commented Oct 7, 2024

Please include #2080

@pilarvargas-tecnativa Can you make a fwd port of that, I'm merging what was already done so far

@pilarvargas-tecnativa
Copy link
Contributor

@pilarvargas-tecnativa Can you make a fwd port of that, I'm merging what was already done so far

Done! Thanks!

#2171

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.