[FIX] sale_discount_display_amount: reset discount_subtotal when discount is removed

[Jay2163]

Problem

When a discount is applied on all order lines using the sale.order.discount wizard, the discount_subtotal field is updated correctly.
However, when the discount is reset to 0, the field retains its previous value instead of being reset.

This leads to inconsistent information being displayed on the sales order.

Solution

Ensure that discount_subtotal is recalculated and reset to 0 when no discount is applied.
This aligns the displayed subtotal with the actual discount state.

Impact

Restores the expected behavior of discount computation on sales orders, ensuring discount_subtotal always reflects the correct discount value.

Fixes issue #3849

[rousseldenis]

@Jay2163 Thanks for this. Could you add a test for that case ?

[Jay2163]

Sure, I’ll add a test for this case and update the PR.

[rousseldenis]

@etobella Did you really forced push to 18.0 branch?

[etobella]

yes, and I rolled everything back. 😭

I usually use https urls for OCA to avoid this, but vscode was able to bypass that by using my user and password. I was able to return to the original point. I was able to disallow that for the future

a really stressfull moment, as that shouldn't be merged without your approval (as happened in the end)

[etobella]

Just to clarify everything, the history was:

1. I use git-aggregate and I had a PR ([18.0] [MIG] sale_invoice_split_payment: Migration to 18.0 #3903) and branch16.
2. Accidentally I made a push after making a git-aggregate and the named branch was 16.0 (I wanted to make a push to the PR branch). Making a push on 16.0 branch with the code of the PR. The PR was ready to be merged, but there were not enough approvals.
3. After noticing it, I made a manual push force on 16 to restore the history before this merge.

It happened for some issues happening at the same time:

• using terminal on vscode, that added my credentials without notifying me
• Not noticing the branch name
• Configuring git-aggregate with a target on OCA

Sorry for the issue. The good thing is that I was able to go back to the original point fastly.

[rousseldenis]

Just to clarify everything, the history was:

1. I use git-aggregate and I had a PR ([18.0] [MIG] sale_invoice_split_payment: Migration to 18.0 #3903) and branch16.
2. Accidentally I made a push after making a git-aggregate and the named branch was 16.0 (I wanted to make a push to the PR branch). Making a push on 16.0 branch with the code of the PR. The PR was ready to be merged, but there were not enough approvals.
3. After noticing it, I made a manual push force on 16 to restore the history before this merge.

It happened for some issues happening at the same time:

• using terminal on vscode, that added my credentials without notifying me
• Not noticing the branch name
• Configuring git-aggregate with a target on OCA

Sorry for the issue. The good thing is that I was able to go back to the original point fastly.

Ok, great. Do you use a '18.0' target branch name on git-aggregate config ? It's always better to use a specific branch name.

[etobella]

i discovered the issue these days. I am changing all my configs. obviously it was a wrong configuration from my side

[alexey-pelykh]

Thanks for looking into this, @Jay2163. The bug diagnosis is correct -- when discount == 0, the continue skips line.update(...), leaving discount_subtotal stale because it is never reset (unlike the other three fields initialized at the top of the loop).

However, the current fix has two issues that should be addressed before merging:

1. Unnecessary computation for zero-discount lines

Removing continue means every line with discount == 0 now runs the full tax_id.compute_all(...) at undiscounted price, only to produce values identical to what was already assigned in the if block. This is wasteful.

A simpler and more targeted fix: add line.discount_subtotal = 0 to the initialization block at lines 36-38 (alongside the existing resets for price_subtotal_no_discount, price_total_no_discount, and discount_total), keeping the continue in place. This fixes the root cause (missing initialization) rather than working around it (removing the early-exit).

Suggested change:

        for line in self:
            line.price_subtotal_no_discount = 0
            line.price_total_no_discount = 0
            line.discount_total = 0
            line.discount_subtotal = 0  # <-- add this
            if not line.discount:
                line.price_subtotal_no_discount = line.price_subtotal
                line.price_total_no_discount = line.price_total
                continue

2. Missing test (as noted by @rousseldenis)

A regression test is needed: set discount on a line, verify discount_subtotal != 0, then reset discount to 0 and assert discount_subtotal == 0. The existing test_sale_discount_value test only checks with discount applied, and test_sale_without_discount_value only checks a line that never had a discount. Neither covers the apply-then-remove scenario described in issue #3849.

[alexey-pelykh]

The root cause is that discount_subtotal is not initialized to 0 at the top of the loop (lines 36-38 reset the other three fields but omit this one). Removing continue works around the bug but forces every zero-discount line through the full tax_id.compute_all() computation unnecessarily.

Preferred fix: keep continue, add line.discount_subtotal = 0 to the initialization block at line 38:

    line.discount_total = 0
    line.discount_subtotal = 0  # add this line

This directly addresses the missing reset and preserves the early-exit optimization.

[Jay2163]

Thanks for the review @alexey-pelykh.

I’ll check the suggested changes and update the code accordingly:

Restore the continue

Add line.discount_subtotal = 0 to the initialization block

Add the missing regression test for the apply-then-remove scenario

I’ll push the updated changes shortly.

[rousseldenis]

@Jay2163