[FIX] sale_exception: add a register_hook to prevent other action_con…

[lav-adhoc]

…firm being executed if there is an exception rule.

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 (version 16), because in the version 16 there is the same problem. We decided to apply the fix in version 18, since version 16 is a stable.
https://drive.google.com/file/d/1qeEYlVVqXb8MOJq7BybVe4EaYtiDcIPS/view

Before this PR, if a sale was not confirmed due to an exception, the loyalty program would still be applied.

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

[rousseldenis]

@lav-adhoc Could you explain again the use case you have and what actions that are not protected by transactions are concerned ?

[lav-adhoc]

@rousseldenis
The problem is the same that in 16
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 (version 16)
https://drive.google.com/file/d/1qeEYlVVqXb8MOJq7BybVe4EaYtiDcIPS/view

Before this PR, if a sale was not confirmed due to an exception, the loyalty program would still be applied.

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

[lav-adhoc]

@rousseldenis
Since these are modules that do not depend on each other, the action_confirm method will be executed first from the module that was installed earlier.

We don't consider the solution of creating bridge modules to be viable, as it would require developing an additional module for each combination of independent modules that perform a specific action (such as assigning points in sale_loyalty).

We believe this is the best solution, as it decouples the behavior from the installation order of the modules. It would also work for any case, not just loyalty.

Lastly, the tests are failing, but the issue is unrelated to this implementation and comes from other modules.

[rousseldenis]

Ok, I have a concern about the method used (like before).

IMHO, in fact, that's because your functional need is: if there are exceptions, keep them but don't add coupons.
That's why I recommend instead a glue module that will add an option on exception rules that say 'don't add coupon in case of this exception'.

Then, in _get_point_changes(), don't retrieve items if sale has exceptions that verify the preceding option.

What do you think ?

[maq-adhoc]

@rousseldenis , you're right that we could add a mechanism to prevent coupons from being created. Our primary concern, however, lies with the design of the module.

From our perspective, if an exception occurs, everything executed before the super() call should be rolled back. This means if sale_exception halts a method's flow, the Sale Order should remain unmodified. The current behavior is inconsistent with how Odoo typically handles exceptions.

We've explored various possibilities, such as creating a new exception type in the ORM that triggers a rollback and write exception data via another database pointer. However, all these solutions would require a significant refactor of the module.

While @lav-adhoc 's proposal is somewhat of a "hack," it currently appears to be the least invasive way to address this module's issue.

[lav-adhoc]

Hi @rousseldenis what do you think about @maq-adhoc 's comment?
We believe this is a good way to solve it without making major changes and ensuring that the problem with coupons does not occur with other methods.

[grindtildeath]

@lav-adhoc Thanks for the PR

I stumbled on the same issue while having sale_exception and OdooEE sale_subscription module installed, but I'm not sure if this "hack" is the right solution, because we can't rely on proper MRO between modules having no dependencies between each other.

IMO as @maq-adhoc mentioned, an exception must be raised in the action_confirm of this module to rollback anything that could happen in the action_confirm from another module. I tried to go this way in #3885 but there's still a UI issue and the test to fix 😞

Anyway, I would suggest to move forward and merge this PR for the time being until we can find a better solution, but could you please do the following:

• Rebase on 18.0 now that [18.0][IMP] sale_exception: sale_exception_show_popup configurable #3837 is merged
• Add roadmap entry explaining the issue
• Write a big comment on the register hook explaining the issue

Eventually you could mention my PR #3885 as a possible way to solve this, but as expressed, I guess the refactor will have to be more consequent 😓

EDIT: Actually, this didn't fix my issue with #3837 integrated into the patched function. I'll get back to it next week

[florian-dacosta]

Hello,
I also have this issue since always, did not attend because not so critical in my case, but you have a similar issue with https://github.com/OCA/sale-workflow/blob/16.0/sale_order_lot_generator
The lot is generated even if the sale has exception and is not confirmed.

I agree this is a general issue of this module and the glue module stuff for me is not good enough. When a module override the _action_confirm, it can only expects its action are either rollbacked in case of exception, either commit if the sale order is confirmed, but commited without the SO validation, can only leads to weird cases.

Between the multiple PR around this, this one seems to do the job. I would also be in favor on merging this until #3885 is completed.
Although I also think it deserves a comment in the _register_hook and also a new test to how the issue.