[16.0][FIX] sale_discount_display_amount: fix onchange peformance

[lmignon]

Before this change, when the onchange was triggered on the sale order following the update of value on a line, odoo replied by marking all the lines as dirty, which was not necessary. This was causing a performance issue and unpredictable behavior since the following call to onchange will include into the list of dirty lines all the lines even if they were not modified.

This issue was initiated by a wrong declaration/implementation of the compute method for the fields discount_total and price_total_no_discount. The computation of these fields was initially done by the native _compute_amount method of the sale order line. But in fact, it depends on the price_total and discount fields of the sale order line. As a result, into the onchange mechanism, when these fields were recomputed, the system wrongly marked all the lines as dirty since the value of a field that was not marked as a dependency of the compute method and not provided into the snapshot info of lines from the UI was changed into the onchange process.

By declaring the fields discount_total and price_total_no_discount as dependency of the fields price_total we ensure that the onchange mechanism will pre-compute the value into the initial snapshot of the lines (since the value is not provided into the information of the lines but some values changed in the lines required to recompute this value) and will not detect any changes at the end of the onchange process when comparing the initial and final snapshot of the lines.

Prior to this change, the price_total was read from the database and when the recompute of the discount_total and price_total_no_discount was done, the system detected a change int the value of the field price_total that was not expected since this field was a declared dependency of these two fields. As a result, the system marked all the lines as dirty...

[alexey-pelykh]

Good work addressing the onchange performance issue. The root cause analysis is correct: having discount_total and price_total_no_discount computed inside _compute_amount without declaring price_total as a dependency caused the ORM snapshot mechanism to mark all lines as dirty during onchange.

The approach of introducing a dedicated _compute_discount_total method with proper dependencies is sound. A few observations below.

Observations:

1. _has_discount hook method is a nice extension point -- using currency_id.is_zero() for the discount check (instead of raw if not line.discount) is more robust and provides a clean override point for modules like sale_triple_discount.

2. float_compare guard before assignment -- this is a pragmatic workaround for the unlink/cache issue described in the regression test. However, the comment on _compute_discount_total mentions "TODO in V19" to consolidate the logic, which is reasonable.

3. _compute_amount override removed -- this means the module no longer calls super()._compute_amount(). Since _compute_discount_total is now a separate compute method triggered by its own @api.depends, this should be fine -- the native _compute_amount still runs independently. Worth confirming there are no other modules in the dependency chain that expect _update_discount_display_fields to be called from within _compute_amount.

4. Dependency list on _compute_discount_total includes price_total -- this is the key fix. By declaring price_total as a dependency, the ORM pre-computes its value in the initial onchange snapshot, so no spurious dirty detection occurs.

5. Test migration from TransactionCase to BaseCommon -- good modernization. The setUpClass pattern avoids redundant record creation across tests.

6. Minor: typos in PR description -- "implemantation" (implementation), "ochange" (onchange), "snappshot" (snapshot). Not blocking, but worth fixing for commit history readability.

7. Minor: the _update_discount_display_fields method still uses line.update({...}) pattern in the else branch -- actually looking more carefully, I see it was changed to direct assignment with the float_compare guard. Good.

8. Question: order_id.partner_shipping_id in @api.depends -- this field is used in tax_id.compute_all() for fiscal position resolution. Including it as a dependency is correct since a shipping address change could affect tax computation and therefore the discount amounts.

[alexey-pelykh]

The float_compare guard is a pragmatic solution to prevent unnecessary cache invalidation (and the unlink regression), but it does add some complexity. A simpler alternative would be to check once at the top of the loop whether any recomputation is needed at all -- though the current approach is more precise since it checks each field independently.

Also, note that the currency variable is fetched from line.order_id.currency_id but the _has_discount method falls back to self.env.company.currency_id when currency_id is falsy. The rounding precision used in float_compare here could theoretically differ from the one used in _has_discount if order_id.currency_id is empty. This is unlikely in practice (a sale order should always have a currency), but worth being aware of for consistency.

[alexey-pelykh]

The @api.depends list is comprehensive. One consideration: order_id.currency_id and order_id.partner_shipping_id are relational traversals. In Odoo 16, these work correctly as compute dependencies, but they do cause the compute to trigger on any change to the related order's currency or shipping partner, even if the net effect on the discount is zero. This is acceptable behavior since the float_compare guard will prevent unnecessary writes in that case.

[alexey-pelykh]

Good regression test for the unlink scenario. The comment could be slightly clearer -- consider: "Regression test: compute_discount_total must not write unchanged values, otherwise unlink raises a cache error when flushing triggers recomputation on records being deleted."

[alexey-pelykh]

This test exercises the fallback path in _has_discount where currency_id is empty and the company currency is used. Good edge case coverage.

Minor: setting self.so_line.currency_id = self.env['res.currency'] (empty recordset) is a somewhat unusual test setup since currency_id on a sale order line is typically a related field from the order. This works for unit-testing _has_discount in isolation, but in a real scenario the currency would come from the order. Consider adding a brief comment noting this is intentionally testing the defensive fallback path.

[lmignon]

@alexey-pelykh Thank you for your deep review. All your comments are addressed (at least I Hope so) . I put the requested changes into the right commits and fixed typos into the first commit message.

[alexey-pelykh]

Re-reviewed after updates. All previous feedback has been addressed:

• Regression test comment clarified
• Empty currency test documents the intentional defensive fallback testing
• Currency fallback is consistent between _has_discount and _update_discount_display_fields
• _has_discount hook is well-documented as an extension point

The approach is solid: dedicated _compute_discount_total with proper price_total dependency fixes the onchange snapshot issue, and the float_compare guard prevents unnecessary writes that would trigger cache errors on unlink.

Minor: commit title still has "peformance" (missing 'r') and body has "mechanim" (missing 's') -- non-blocking, can be fixed at merge time by maintainer if desired.

[OCA-git-bot]

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖