[19.0][MIG] sale_order_general_discount_triple: Migration to 19.0#4134
[19.0][MIG] sale_order_general_discount_triple: Migration to 19.0#4134pablo-cort-s73 wants to merge 22 commits intoOCA:19.0from
Conversation
24ab0f6 to
3dcdca4
Compare
Currently translated at 100.0% (11 of 11 strings) Translation: sale-workflow-14.0/sale-workflow-14.0-sale_order_general_discount_triple Translate-URL: https://translation.odoo-community.org/projects/sale-workflow-14-0/sale-workflow-14-0-sale_order_general_discount_triple/it/
Use `update` instead of `write` to avoid caching issue
Changing the field `discount` to a not computed one is breaking standard flows, provoking all sorts of errors when this module is installed, so we disable for now the module while not finding other option that will be reported to original migrators.
Discount behaviuour in v16.0 is to be computed but in this module it was changed. Here we are using compute_discount method to archive the functionallity and also converting the remaining two discounts to computed fields.
Pricelist discount was overide by general discount functionality. With this change, its up to the user to choose in which discount wants to apply general and pricelist disounts.
Currently translated at 100.0% (8 of 8 strings) Translation: sale-workflow-16.0/sale-workflow-16.0-sale_order_general_discount_triple Translate-URL: https://translation.odoo-community.org/projects/sale-workflow-16-0/sale-workflow-16-0-sale_order_general_discount_triple/es/
Currently translated at 100.0% (10 of 10 strings) Translation: sale-workflow-16.0/sale-workflow-16.0-sale_order_general_discount_triple Translate-URL: https://translation.odoo-community.org/projects/sale-workflow-16-0/sale-workflow-16-0-sale_order_general_discount_triple/es/
…or res.config.settings - Field mark as required cause issues with other modules
Currently translated at 100.0% (10 of 10 strings) Translation: sale-workflow-16.0/sale-workflow-16.0-sale_order_general_discount_triple Translate-URL: https://translation.odoo-community.org/projects/sale-workflow-16-0/sale-workflow-16-0-sale_order_general_discount_triple/it/
a2c50e8 to
4e49a80
Compare
0c90af4 to
0acc5e6
Compare
0acc5e6 to
fcacfb2
Compare
alexey-pelykh
left a comment
There was a problem hiding this comment.
Thanks for the migration work on this bridge module, Pablo.
A few things I noticed that should be addressed before merging:
The .pot file appears stale -- it references selection keys like discount (without number) in selection__res_config_settings__general_discount__discount, but the Python code defines discount1, discount2, discount3. The es.po already uses the correct discount1 keys, while the it.po and .pot still use the old discount key from a prior version. The .pot should be regenerated from the current code, and the Italian translation updated accordingly.
The readme/DESCRIPTION.md links still point to 14.0 branches (tree/14.0/sale_order_general_discoun and tree/14.0/sale_triple_discount). These should point to 19.0 -- also note the truncated URL for sale_order_general_discount.
The three _compute_discountN methods in sale_order_line.py are essentially identical aside from the field name. Extracting the shared logic into a single helper (e.g., _compute_discount_field(field_name)) would reduce the duplication considerably and make the code easier to maintain.
Also minor: the HISTORY.md only lists 14.0.1.0.0 as the initial release -- it would be good to add a 19.0.1.0.0 entry noting the migration.
| #. module: sale_order_general_discount_triple | ||
| #: model:ir.model.fields.selection,name:sale_order_general_discount_triple.selection__res_config_settings__general_discount__discount3 | ||
| #: model:ir.model.fields.selection,name:sale_order_general_discount_triple.selection__res_config_settings__pricelist_discount__discount3 | ||
| msgid "Discount 3" |
There was a problem hiding this comment.
This references selection__res_config_settings__general_discount__discount (plain discount), but the current Python code defines selection keys discount1, discount2, discount3. The .pot needs to be regenerated from the current module source.
| @@ -0,0 +1,6 @@ | |||
| A bridge between Sale Order General | |||
| Discount(<https://github.com/OCA/sale-workflow/tree/14.0/sale_order_general_discoun>) | |||
There was a problem hiding this comment.
This URL still points to 14.0 and is also truncated (missing the trailing t in sale_order_general_discount). Should be updated to 19.0.
| val = line._get_pricelist_discount() | ||
| elif ( | ||
| general_discount == "discount1" | ||
| and line.product_id |
There was a problem hiding this comment.
The _compute_discount2 method is nearly identical to _compute_discount1 and _compute_discount3 -- the only difference is the field name being assigned. Consider extracting the shared logic into a helper like _compute_discount_field(self, field_name, pricelist_discount, general_discount) and calling it from each compute method. This would cut ~60 lines of duplication.
| ) | ||
| ) | ||
| if general_discount != "no_apply": | ||
| for record in self: |
There was a problem hiding this comment.
Calling compute methods directly (line._compute_discount1() etc.) from business logic bypasses the ORM's dependency tracking. Since these fields already have proper @api.depends decorators, consider whether an invalidate_recordset or triggering the dependency via field writes would be more idiomatic for 19.0.
| @@ -0,0 +1,3 @@ | |||
| ## 14.0.1.0.0 | |||
There was a problem hiding this comment.
This only lists the 14.0 initial release. A 19.0.1.0.0 entry noting the migration would be appropriate.
alexey-pelykh
left a comment
There was a problem hiding this comment.
Checking in on this -- no new commits since my last review, so the previous points still stand (stale .pot/it.po, DESCRIPTION.md links pointing to 14.0, duplicated _compute_discountN methods, missing 19.0.1.0.0 entry in HISTORY.md).
A couple of additional things I noticed on closer look:
The res_config_settings.py selection fields only offer discount1/discount2/discount3, but onchange_general_discount in sale_order.py checks for "no_apply" as a special case. The old version of this module had a ("no_apply", "No Apply") option in the selection. Without it, there's no way for users to disable the general discount propagation through Settings. That option should be restored.
The _get_pricelist_discount method references self.pricelist_item_id and calls _show_discount() on it -- worth double-checking these are available in the 19.0 sale_triple_discount dependency, since the Odoo pricelist internals changed quite a bit between versions.
The test-requirements.txt pulls from unmerged PR refs (#3965 and account-invoicing#2144) which is fine for CI, but the [DON'T MERGE] commit message is a good reminder that this file needs to be cleaned up before final merge.
Happy to re-review once these are addressed along with the earlier feedback.
PR Description - Migration to 19.0 & CI Optimization
depends
Added
deliveryas a required dependency. Method _create_delivery_line is on this moduleModel
sale.order: Refined the logic for applying general discounts. Added filters to prevent applying discounts todisplay_typelines (sections/notes) or products marked withbypass_general_discount.Model
sale.order.line:Optimized
_compute_discountXmethods by addingdisplay_typeandorder_id.general_discountto the@api.dependsdecorator to ensure correct reactivity.Simplified the logic by removing redundant loops and ensuring
display_typelines always default to a0.0discount.Added Test to check changes