[16.0][FIX] sale_expection: don't compute loyalty program if there is…

[lav-adhoc]

… any exception

Before this PR, if a sale was not confirmed due to an exception, the loyalty program would still be applied. For example, if a coupon was used, a point would be added, and then upon confirming the sale, it would be added again.
The issue is in the sale_loyalty module, where the loyalty program is computed before the sale is confirmed in this line

This video demonstrates the issue:
https://drive.google.com/file/d/1qeEYlVVqXb8MOJq7BybVe4EaYtiDcIPS/view

[lef-adhoc]

@pedrobaeza Hi! What do you think of this pr?

[pedrobaeza]

I don't use this module, but doing self.env.cr.rollback() (or commit) is a total no-go.

[lav-adhoc]

@pedrobaeza Hi Pedro, how are you? Thanks for your answer. I understand that using rollback may not be a good practice, we’ve been analyzing other possibilities with the team.
Override detect_exceptions so that it returns a list of exceptions for sale. Open a new cursor, write the exceptions to the database, and raise an error with the text shown in a wizard.
What do you think about this? Do you have any other idea or possible solution in mind?

I’m mentioning other people I saw have been involved with this module.
@rousseldenis
Thank you in advance.

[pedrobaeza]

Check https://github.com/OCA/l10n-spain/blob/7818a061e22e774f93a48f1696f11e1cb1a34ab9/l10n_es_aeat_sii_oca/models/sii_mixin.py#L826-L841 for a possible technique opening a new cursor for writing the data you want (in this case, the exceptions), but letting regular Odoo cursor to follow its path, being rollbacked by the ORM.

[rousseldenis]

As @pedrobaeza said, a line alone with rollback is not the good way to do.

IMHO, the behavior is totally normal as the 'exceptions' are not technical errors but properties on sale orders that are 'anomalies' to be checked.

As I understand, as sale_loyalty module is not in the same call stack (as no dependency) as this, the coupons are applied even if super() is not called.

The solution should goes IMHO in a glue module between sale_loyalty and sale_exception modules.

Your rollback is too harsh and can have too much corner cases where changes should have been applied

[rousseldenis]

As said in base_exception, you can maybe add in the glue module a constraint if there are coupons.

https://github.com/OCA/server-tools/blob/16.0/base_exception/models/base_exception.py#L100C9-L100C25

[jjscarafia]

@rousseldenis I believe that on a method that is supposed to confirm and order (action_confirm), if it's cancelled by any module, it should be canceled by a raise to make a rollback.

They bring the use case of sale_loyalty, but I remember other use cases with other modules that extends action_confirm.

Do you agree?

Other cursor seems to be the good direction but the change is really bigger. Would it be ok for 16? better for 18 or 19?

[maq-adhoc]

@jjscarafia @rousseldenis , @lav-adhoc and I have been doing some tests to combine a new cursor and a raise that prevents a data commit. You can preview the changes here. https://github.com/OCA/sale-workflow/compare/16.0...adhoc-dev:sale-workflow:16.0-h-89839-lav2?expand=1

While it worked, we feel we are adding too many changes in a stable version.

Although the problem occurs with coupons, it's possible that new problems might also arise with payment integrations or electronic invoicing.

So, on one hand, I'd like your opinion on the other approach (does it make sense to implement it in version 16?).

And in newer versions, we can consider a refactoring of the base_exception module in which exceptions are a new type of raise exception with the characteristics we need, and that they write the exceptions with a new cursor.

[alexey-pelykh]

Review Summary

The underlying problem is valid and well-described: when sale_exception blocks sale confirmation, other modules (like sale_loyalty) may have already persisted side effects (e.g., applying coupon points) that should have been rolled back. However, the proposed fix using self.env.cr.rollback() introduces serious risks and is not the right approach.

I agree with the feedback already provided by @pedrobaeza and @rousseldenis.

Key Issues

1. self.env.cr.rollback() is a no-go in Odoo business logic. It breaks the ORM cache coherence -- the in-memory recordset state (caches, prefetch data, computed field values) becomes inconsistent with the database state after rollback. This can cause silent data corruption, ghost writes, or crashes in any subsequent ORM operation within the same request.

2. Unpredictable blast radius. The rollback undoes all uncommitted writes in the current transaction, not just the ones from sale_loyalty. Any other module that wrote data between the start of the request and this point (audit logs, sequence counters, state changes) will also be silently reverted. This is extremely hard to debug.

3. Re-writing exception_ids after rollback is fragile. The sale.write() after rollback operates on a potentially stale ORM environment. Fields that were computed or cached before rollback may produce incorrect values.

4. No tests. A change this impactful requires test coverage demonstrating both the original bug and the fix.

Recommended Direction

As discussed in the thread, the proper approaches for 16.0 would be:

• New cursor approach: Use self.pool.cursor() (or registry.cursor()) to persist the exception_ids in a separate transaction, then raise an exception to trigger a natural rollback of the main cursor. This is the pattern @pedrobaeza pointed to in l10n_es_aeat_sii_oca.
• Glue module approach: Create a sale_exception_loyalty bridge module that prevents loyalty program computation when exceptions exist (as @rousseldenis suggested).

For newer versions (18+), a refactoring of base_exception to use a raise-based mechanism (like UserError subclass) that naturally triggers ORM rollback would be the cleanest long-term solution.

---

Reviewer: CorporateHub (via automated OCA review)

[alexey-pelykh]

Critical: self.env.cr.rollback() must not be used in Odoo business methods.

This rolls back the entire current database transaction, not just the changes made by sale_loyalty. Any writes from other modules (audit trails, sequences, computed fields stored during this request cycle) will also be silently lost.

Additionally, after rollback(), the ORM caches (prefetch, field cache, computed values) are stale -- they still reflect the pre-rollback state. Subsequent ORM operations (including the sale.write() below) operate on an inconsistent view, which can cause silent data corruption or MissingError exceptions.

The recommended pattern is to use a new cursor (self.pool.cursor()) to persist the exception data in a separate transaction, then raise an exception to let the ORM naturally roll back the main transaction. See: https://github.com/OCA/l10n-spain/blob/7818a061/l10n_es_aeat_sii_oca/models/sii_mixin.py#L826-L841

[rousseldenis]

@lav-adhoc @lef-adhoc What's the status of this ?