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_secondary_unit: Migration to 16.0 #2645

Closed
wants to merge 37 commits into from

Conversation

anddago78
Copy link

@anddago78 anddago78 commented Aug 18, 2023

This module extends the functionality of sale orders to allow sale products in
secondary unit of distinct category.

MT-3490 @moduon @rafaelbn @fcvalgar @EmilioPascual please reviews :)

This PR superseed this one #2635

Pending:

  • FIX correct calculation of "unit price" when "unit qty" changes
  • FIX correct calculation of "unit price" when "UoM" Changes
  • Fix tests in OCA CI.

See:

PR-16-sale_order_secondaty_unit.mp4

--

@rafaelbn rafaelbn changed the title 16.0-mig-sale_order_secondary_unit: Migration to 16.0 [16.0][MIG] sale_order_secondary_unit: Migration to 16.0 Aug 18, 2023
@rafaelbn
Copy link
Member

/ocabot migration sale_order_secondary_unit

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Aug 18, 2023
@OCA-git-bot OCA-git-bot mentioned this pull request Aug 11, 2023
97 tasks
@rafaelbn
Copy link
Member

Test fails with

secondary_uom_qty

2023-08-18 07:28:23,680 249 WARNING odoo py.warnings: /opt/odoo/odoo/fields.py:802: 
UserWarning: Field sale.order.line.product_uom_qty cannot be precomputed as it depends on 
non-precomputed field sale.order.line.secondary_uom_qty

  File "/opt/odoo/odoo/modules/registry.py", line 439, in _field_triggers
    dependencies = list(field.resolve_depends(self))
  File "/opt/odoo/odoo/fields.py", line 802, in resolve_depends
    warnings.warn(f"Field {self} cannot be precomputed as it depends on non-precomputed field {field}")

price_unit

2023-08-18 07:28:23,681 249 WARNING odoo py.warnings: /opt/odoo/odoo/fields.py:802: 
UserWarning: Field sale.order.line.price_unit cannot be precomputed as it depends on non-precomputed field sale.order.line.product_uom_qty

  File "/opt/odoo/odoo/modules/registry.py", line 439, in _field_triggers
    dependencies = list(field.resolve_depends(self))
  File "/opt/odoo/odoo/fields.py", line 802, in resolve_depends
    warnings.warn(f"Field {self} cannot be precomputed as it depends on non-precomputed field {field}")

This could be due a change in how Odoo works with onchange and compute fields in Odoo 16 vs Odoo 15

What do you think @sergio-teruel ?

@sergio-teruel
Copy link
Contributor

sergio-teruel commented Aug 19, 2023

Hi... you must do precompute this field in another PR
https://github.com/OCA/product-attribute/blob/16.0/product_secondary_unit/models/product_secondary_unit_mixin.py#L39

Add precompute=True

sergio-teruel and others added 20 commits August 22, 2023 13:42
Added domain to limit `secondary_uom_id` to the one defined in product template
OCA-git-bot and others added 16 commits August 22, 2023 13:42
As the test should only test that no change on the UoM quantity is done
when modifying the secondary unit, we remember the existing quantity and
compare it later with the final value, instead of testing against a
fixed value, so any interaction with other modules don't interfere.
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: sale-workflow-15.0/sale-workflow-15.0-sale_order_secondary_unit
Translate-URL: https://translation.odoo-community.org/projects/sale-workflow-15-0/sale-workflow-15-0-sale_order_secondary_unit/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: sale-workflow-15.0/sale-workflow-15.0-sale_order_secondary_unit
Translate-URL: https://translation.odoo-community.org/projects/sale-workflow-15-0/sale-workflow-15-0-sale_order_secondary_unit/
Some inherited fields are set to `precompute=True` for performance.

The test `test_secondary_uom_unit_price` was asserting a buggy behavior
from v15 that is fixed automatically in v16 by the migration. Thus, we
change the assertions. Now, when qtys change, the unit price is
recomputed. That's the upstream behavior. It makes more sense, because
you might have pricelists that depend on the qty. If you want to keep
that old behavior, install `sale_order_qty_change_no_recompute`.

@moduon MT-3490

Co-authored-by: Jairo Llopis <973709+yajo@users.noreply.github.com>
I don't know how it was possible for v15 code to work. Anyways, this compatibility is now tested and fixed. Upstream compute function is reused.

@moduon MT-3490
@yajo yajo force-pushed the 16.0-mig-sale_order_secondary_unit branch from c058e14 to 0fd8960 Compare August 22, 2023 12:42
@yajo yajo marked this pull request as ready for review August 22, 2023 12:42
@yajo yajo marked this pull request as draft August 22, 2023 12:50
This module alters the behavior of sale order lines. Thus, it breaks others' tests. It must be isolated.

@moduon MT-3490
@yajo yajo marked this pull request as ready for review August 23, 2023 06:42
@rafaelbn
Copy link
Member

I guess this migration is straight from previous versions, I've tested slowly and the migration is OK but this module introduce BUGs when working with Packaging at the same time @sergio-teruel @rousseldenis @Shide @yajo @anddago78

See my review:

Odoo.16.-.sale_order_product_secondary_unit.-.rafaelbn.functional.test.mp4

After seesing that this module should be improved to work 100% compatible with packging, I tested packaging un Odoo runbot separately

https://www.loom.com/share/b3d62de28beb4385aadebabf8566baec?sid=847b0ec9-aec0-4eff-84ed-fc9a178ee86b

Important:

  • If you need to use secondary unit is sometimes not always, it's not mandatory
  • If a product don't define secondary unit the fields in sale order line should be read only (TODO)
  • The Odoo core packaging functionality should be 100% compatible with this module installled

This is a complicated module

@yajo yajo marked this pull request as draft August 24, 2023 06:41
@yajo yajo closed this Aug 24, 2023
@yajo yajo deleted the 16.0-mig-sale_order_secondary_unit branch August 24, 2023 12:40
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.