[16.0][FIX] sale_expection: don't compute loyalty program if there is… #3672
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,11 @@ def sale_check_exception(self): | |
|
|
||
| def action_confirm(self): | ||
| if self.detect_exceptions(): | ||
| exception_map = {sale.id: sale.exception_ids.ids for sale in self} | ||
| self.env.cr.rollback() | ||
|
There was a problem hiding this comment. Critical: This rolls back the entire current database transaction, not just the changes made by Additionally, after The recommended pattern is to use a new cursor ( |
||
| for sale in self: | ||
| exception_ids = exception_map.get(sale.id, []) | ||
| sale.write({"exception_ids": [(6, 0, exception_ids)]}) | ||
| return self._popup_exceptions() | ||
| return super().action_confirm() | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @pedrobaeza said, a line alone with
rollbackis 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_loyaltymodule is not in the same call stack (as no dependency) as this, the coupons are applied even ifsuper()is not called.The solution should goes IMHO in a glue module between
sale_loyaltyandsale_exceptionmodules.Your rollback is too harsh and can have too much corner cases where changes should have been applied
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.