[18.0][POC] base_exception: Rollback transaction triggering exception rule

[grindtildeath]

In case a implementation module, eg sale_exception, breaks a function override, ie does not call super on sale.order.action_confirm in case an exception rule applies, it is still possible that another module that is not in the implementation module's dependency, does modify existing the same object through the same function's override that is called before through MRO.

As the exception rule is only written in base_exception module, such modifications would be still be committed although the exception was triggered.

The only way to rollback everything that happened is to raise an exception that is going to rollback the transaction. However, we still want to commit the write of the exception rule and not to propagate the exception back to the webclient.

Since we cannot alter the env in the retrying function (that handles rollback), we do not have any other choice than to use a dedicated cursor in the dispatching function to process the request, since it could trigger the exception rule, and to use the original env to write the exception rule, and have the webclient being refreshed to display the exception.

This might break the latest feature to pop up the wizard, but since this is still a POC, we could fix that afterwards in case this POC is accepted.

[OCA-git-bot]

Hi @sebastienbeau, @hparfr,
some modules you are maintaining are being modified, check this out!

[SilvioC2C]

Beware:

1. in the old behavior, old rules were removed before new ones got added, while in here you do the opposite: is this a wanted change?
2. the command value should be 3 when removing rules, 4 when adding them, while in here you're always using 4: can you please switch to Command.[un]link(ID) instead to make it easier to read and debug (and also, possibly add tests)?

[grindtildeath]

Good catch, I did focus on the transaction handling, but both are valid points 👍

[hparfr]

So you want to use an Exception in base_exception ?! :D

Why not

[grindtildeath]

@hparfr I'm not sure if you understood the issue here 😕

This is actually a potential fix to all of these issues:

• [16]sale_exception: Subscription behavior when confirming an upsell sale-workflow#2667
• [16.0][FIX] sale_expection: don't compute loyalty program if there is… sale-workflow#3672
• [FIX] sale_exception: add a register_hook to prevent other action_con… sale-workflow#3767
• [18.0][FIX] sale_exception: Rollback confirmation upon exception sale-workflow#3885
• [18.0][FIX] sale_exception: handle exception rollback in action_confirm method sale-workflow#3952

It happened to some of our OE customers using module sale_subscription and sale_exception , when such conditions are verified:

• Have a sale exception on sale.order in case a field is not set
• Field is set on the subscription so you can confirm without any issue
• An upsell is created where field is not set
• When confirming the upsell order, action_confirm from sale_subscription module is called before action_confirm from the sale_exception module. Hence, the override in sale_exception will not call super, and will return an empty value, while the code from action_confirm in sale_subscription will still be executed, and therefore the subscription is still updated although the upsell order was not confirmed. When sale exception is lifted and the upsell order is confirmed, the subscription would have been updated twice with the products from the upsell order.

Linked to this, #3405 is actually a workaround to avoid applying the exception for records (eg upsell orders in the example I just provided).

[hparfr]

@hparfr I'm not sure if you understood the issue here 😕

No issue at all from my side. I find it "fun" to have have a true Exception in a module called base_exception.
It's a fine and logical design decision.

I read the code, but did not test it yet.

[simahawk]

this will work only over an HTTP request. What about crons or other types of operations? 🤔

[grindtildeath]

Actually crons are quite tricky to handle in this context since they use their own Runner, and even in case we store the exception.rule record through a subtransaction, raising a specific error linked to this base exception module would result in the cron failing multiple times. Still, I'll check if using a subtransaction to store the exception rule and and raising an error to rollback the transaction can be handled properly in the web client as this is what prevented continuing on this alternative solution.