[18.0][FIX] base_exception: Rollback transaction if we detect exceptions by grindtildeath · Pull Request #3590 · OCA/server-tools

grindtildeath · 2026-04-01T18:48:30Z

This aims to solve issues when modules not depending on whatever implementation of base_exception, override the same function that triggers the detection of exceptions.

Before this commit, any changes done in such overrides could end up being committed to the database if the MRO did execute such function before the function implementing base_exception that avoids to call super in case an exception is detected. (eg sale.order. action_confirm in sale_exception)

With this commit, in case there is any newly detected exception, or a record with exception that is not ignored, or a blocking exception linked to a record, exception changes will be committed in DB while a specific Exception type will be raised to rollback any changes done in the ongoing transaction. Such an exception will be handled in the UI to refresh the exception_ids field so that the user knows why the action was not completed.

Replaces and fixes:

Dependent PRs:

OCA-git-bot · 2026-04-01T18:48:38Z

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

ivantodorovich · 2026-04-01T19:09:46Z

+        raise_exception = False
+        # Write exceptions in a new transaction to be committed so that we can
+        #  rollback the ongoing one while keeping the exceptions stored
+        with Registry(self.env.cr.dbname).cursor() as new_cr:


Suggested change

with Registry(self.env.cr.dbname).cursor() as new_cr:

with self.env.registry.cursor() as new_cr:

no need to initialize a new registry

hparfr · 2026-04-02T07:04:59Z

ping @florian-dacosta

With OCA/server-tools#3590 changing the way exceptions are detected, the error raising doesn't allow to call _popup_exception as we used to, but we can use the new hook to have the popup displayed as it used to.

With OCA/server-tools#3590 changing the way exceptions are detected, the error raising doesn't allow to call `_popup_exception` as we used to, but we can use the new hook to have the popup displayed smoothly.

With OCA/server-tools#3590 changing the way exceptions are detected, the error raising doesn't allow to call _popup_exception as we used to, but we can use the new hook to have the popup displayed smoothly.

…ollback With OCA/server-tools#3590 changing the way exceptions are detected, the error raising doesn't allow to call _popup_exception as we used to, but we can use the new hook to have the popup displayed smoothly.

grindtildeath · 2026-04-06T16:21:58Z

ping @jok-adhoc @lav-adhoc @lef-adhoc @maq-adhoc @rousseldenis @florian-dacosta

After spending hours on this issue, IMO this solves the core issue we have with non dependent module having their changes done in action_confirm committed after an exception was detected since the transaction was not fully rolled back.

I then changed the way the exception popup can be displayed and it seems to work smoothly with no side effects and no need of glue modules.

Could you guys please have a look, eventually test this (with their dependent PRs) and let me know what do you think? 🙏

rousseldenis

LGTM.

Great! @grindtildeath Thanks for this, this seems indeed cleaner to solve the historical issue we have with this.

florian-dacosta · 2026-04-08T12:26:15Z

Hello @grindtildeath Thanks for this PR.
Code review looks fine. There is a pre-commit error, I had this recently and did change the name of the file by .esm.js to solve it. (Not sure it is the right solution).

But I did a test and it is not working as expected. On a demo database with only sale_management and sale_exception installed, I did add the following line in the method action_confirm of sale_management (before the super) : self.write({"client_order_ref": "TEST"}).

sale-workflow.webm

As you can see in the video, after clicking on the confirm button of the sale order, I have the new exception and the client order ref field did not change, all good on this part.
But, I did not have any window with the exceptions + all button are disabled. (including the little arrow to go to partner form for instance, not only the header buttons).

Can you check this issue please ?

grindtildeath · 2026-04-08T17:17:47Z

@florian-dacosta Thanks for your feedback.

I reworked the JS part to make it cleaner and simpler, it seems to solve your issue and works seamlessly on my side. Would be good if you can test again.

ivantodorovich

CI fails due to pre-commit

florian-dacosta

@grindtildeath
It seems to work pretty well now, but there is still an issue.
I test with sale_exception, and if I run the cron to test the exceptions on draft order, manually (with button method_direct_trigger), I have this error
AttributeError: The method 'ir.cron.action_popup_exceptions' does not exist

This is because the button on ir.cron do trigger the BaseExceptionError.

grindtildeath · 2026-04-13T18:39:22Z

@florian-dacosta Tanks for the feedback, must be fixed now

florian-dacosta · 2026-04-14T08:02:30Z

Thanks for the fix @grindtildeath
It seems to work quite well now. The last thing that bother me a little, is that an exception is raised even when not needed.
Indeed, when you confirm a sale order, you want the exception to revert everything, but when it is the cron that "test exceptions on draft orders", you don"'t need to revert anything the raise is not needed.

It does not seem to be a big problem since the exceptions are commited anyway, but
It still gives a warning : 2026-04-14 07:44:57,926 31 WARNING db odoo.http: Exceptions detected on model: sale.order
while it is a "false alarm" since it is totally normal to set/remove exeption with the cron.

Also, I guess we may have situation with the inverse issue we try to fix here.
Imagine you override some test_all_draft_orders on sale order for instance, and you expect your code to change some stuff after the update of all exceptions, it won't happen because of the raise, while you would not expect a raise since the method detect_exceptions succeed in doing what it has been designed for...
overriding detect_exceptions directly could also lead to this.
We want to raise only because detect_exceptions is called during the action_confirm, but not necessarly for other cases...

I am not blocking anything as for now, the gain seems better than the potential loss, but I think it may be an issue.
What do you think ?

simahawk

LGTM. As for version bump I would go for major.

simahawk · 2026-04-17T07:17:29Z

+                error.exceptionName ===
+                "odoo.addons.base_exception.exceptions.BaseExceptionError"
+            ) {
+                if (error.data.message.split(":")[1].trim() === params.model) {


If I get it right when looking at If you register the exception into any of these registries, you won't need this patch:

const errorHandlerRegistry = registry.category("error_handlers"); const errorDialogRegistry = registry.category("error_dialogs"); const errorNotificationRegistry = registry.category("error_notifications");

The second one is probably the right one.

Not a blocker but maybe we can add a TODO here 😉

Done in fixup commit, but had to use errorHandlerRegistry

simahawk · 2026-04-17T07:25:13Z

@florian-dacosta If I get your explanation right, it sounds like a side effect that is not fully in control.

To allow devs to still store some information before the exception is raised we could add an hook and pass to it the new env. This way you can control it in a very explicit way.

Not a blocker. maybe we can add a TODO in the roadmap.

florian-dacosta · 2026-04-17T07:47:33Z

@florian-dacosta If I get your explanation right, it sounds like a side effect that is not fully in control.

To allow devs to still store some information before the exception is raised we could add an hook and pass to it the new env. This way you can control it in a very explicit way.

Not a blocker. maybe we can add a TODO in the roadmap.

I was more thinking on a context to get rid of the raise.
So, by default (no context), the error is raise and the rollback is done (required when the detect exceptions is part of the action_confirm for instance.
And for a method like sale.order.test_all_draft_orders which only goal is to check exceptions, you know the raise is not necessary, you could call it with the special context.

Well anyway, not a blocker like I said, maybe it is anticipating an issue we won't ever have. I have no usecase in mind, just thought about the possible side effects.

With OCA/server-tools#3590 changing the way exceptions are detected, the error raising doesn't allow to call `_popup_exception` as we used to, but we can use the new hook to have the popup displayed smoothly.

hparfr · 2026-05-04T06:53:30Z

@grindtildeath may you please rebase with autosquash

With OCA/server-tools#3590 changing the way exceptions are detected, the error raising doesn't allow to call _popup_exception as we used to, but we can use the new hook to have the popup displayed smoothly.

…ollback With OCA/server-tools#3590 changing the way exceptions are detected, the error raising doesn't allow to call _popup_exception as we used to, but we can use the new hook to have the popup displayed smoothly.

With OCA/server-tools#3590 changing the way exceptions are detected, the error raising doesn't allow to call `_popup_exception` as we used to, but we can use the new hook to have the popup displayed smoothly.

With OCA/server-tools#3590 changing the way exceptions are detected and rollbacked, tests need to be adapted accordingly.

vvrossem · 2026-05-28T13:44:58Z

+                else self.env
+            )
+            for rule_id, records in rules_to_remove.items():
+                records.with_env(new_env).write({"exception_ids": [(3, rule_id)]})


Suggested change

records.with_env(new_env).write({"exception_ids": [(3, rule_id)]})

records.with_env(new_env).write({"exception_ids": [Command.unlink(rule_id)]})

vvrossem · 2026-05-28T13:45:15Z

+            for rule_id, records in rules_to_remove.items():
+                records.with_env(new_env).write({"exception_ids": [(3, rule_id)]})
+            for rule_id, records in rules_to_add.items():
+                records.with_env(new_env).write({"exception_ids": [(4, rule_id)]})


Suggested change

records.with_env(new_env).write({"exception_ids": [(4, rule_id)]})

records.with_env(new_env).write({"exception_ids": [Command.link(rule_id)]})

vvrossem · 2026-05-28T13:45:49Z

 import logging
 from collections import defaultdict

 from odoo import _, api, models


Suggested change

from odoo import _, api, models

from odoo import _, api, models, Command

This aims to solve issues when modules not depending on whatever implementation of base_exception, override the same function that triggers the detection of exceptions. Before this commit, any changes done in such overrides could end up being committed to the database if the MRO did execute such function before the function implementing base_exception that avoids to call super in case an exception is detected. (eg sale.order. action_confirm in sale_exception) With this commit, in case there is any newly detected exception, or a record with exception that is not ignored, or a blocking exception linked to a record, exception changes will be committed in DB while a specific Exception type will be raised to rollback any changes done in the ongoing transaction. Such an exception will be handled in the UI to refresh the exception_ids field so that the user knows why the action was not completed.

With the previous change raising an exception to rollback a transaction, the function `_popup_exceptions` could not be called anymore while the error was raised. Instead, we provide now a hook `_must_popup_exception` that can be redefined by model and will be called when `action_popup_exception` is called by the webclient, to smoothly refresh the page and display the popup exception wizard.

Simplify the JS layer by leveraging client actions. Return a custom client action whenever the BaseExceptionError is catched on RPC call and handle everything through the call to action_popup_exceptions: - In case exception should not pop up, return a soft reload client action instead of handling the refresh manually in JS - In case exception should pop up, return the wizard action, but do a soft reload first to display the exception_ids block before execution the wizard opening action.

divad1196 · 2026-05-29T06:30:43Z

            else:
                rec.exceptions_summary = False

+    def _must_popup_exception(self):


_must_popup_exception -> _should_popup_exception ?

must express a necessity to enforce, not just the intent to show it.

divad1196 · 2026-05-29T06:44:10Z

+        #  rollback the ongoing one while keeping the exceptions stored
+        with self.env.registry.cursor() as new_cr:
+            new_env = (
+                Environment(new_cr, self.env.uid, self.env.context)


self.env(cr=cr) instead ?

We need to call Environment here so that we can patch it in mock_base_exception_method_env

I think you can use self.env(cr=new_cr) by mocking Environment.__call__(). Though I think it's fine as it is.

divad1196 · 2026-05-29T06:52:48Z

+        }
+
+    def _must_raise_exception_after_detection(self):
+        return not all(


Can we decompress the expression like this

main_records = self._get_main_records().filtered("exception_ids") return not all( main_records.mapped("ignore_exception") ) or any(rule.is_blocking for rule in records.exception_ids)

or

main_records = self._get_main_records().filtered("exception_ids") ignore_exception = all( main_records.mapped("ignore_exception") ) blocking_exception = any(rule.is_blocking for rule in records.exception_ids) return not ignore_exception or blocking_exception

divad1196 · 2026-05-29T07:02:25Z

+            new_env = (
+                Environment(new_cr, self.env.uid, self.env.context)
+                if not test_mode
+                else self.env


if we use self.env, this means we cannot test to rollback?

Not sure to understand your concern here 🤨

divad1196 · 2026-05-29T07:09:47Z

+            # base.exception.method.detect_exceptions, a second savepoint will be
+            # created using new_cr, and an odoo.sql_db.Savepoint object will be stored
+            # on new_cr._savepoint for this second savepoint.
+            # As the with block of assertRaises is exited a rollback to the first


Explanation a bit hard to follow IMO. I think that bullet points would help, what do you think?

Enter with assertRaises => Create SavePoint1

Write using new_cr => Create SavePoint2 (after SavePoint1)

Exit with assertRaises => Rollback to SavePoint1 => invalidate Savepoint2

=> We need to discard SavePoint2 to avoid savepoint does not exist error.
This happens because new_cr and self.env.cr use the same psycopg2 cursor object behind the scene

What's still not clear to me is when the write on new_cr happens

What's still not clear to me is when the write on new_cr happens

It happens in base.exception.method.detect_exceptions as explained 😬

I added some numbers to explicit each action to avoid rewriting everything, hope it's fine.

…ions

SilvioC2C · 2026-05-29T14:32:47Z

+            self.assertFalse(self.po.exception_ids)
+            self.assertTrue(self.po.with_env(new_env).exception_ids)


For the sake of maintainability, would you please write a comment explaining why we expect self.po.exception_ids to have different values according to the chosen Environment?

ivantodorovich approved these changes Apr 1, 2026

View reviewed changes

sebastienbeau requested review from hparfr and sebastienbeau April 1, 2026 23:09

sebastienbeau assigned sebastienbeau and unassigned sebastienbeau Apr 1, 2026

grindtildeath force-pushed the 18.0-fix-base_exception_rollback_transaction_fix_ui branch 2 times, most recently from 4ea380d to 61eed8a Compare April 2, 2026 15:10

ivantodorovich approved these changes Apr 6, 2026

View reviewed changes

grindtildeath mentioned this pull request Apr 6, 2026

[18.0][IMP] sale_exception: Use hook to popup exception after rollback OCA/sale-workflow#4255

Open

1 task

grindtildeath mentioned this pull request Apr 6, 2026

[18.0][IMP] purchase_exception: Use hook to popup exception after rollback OCA/purchase-workflow#3012

Open

1 task

rousseldenis approved these changes Apr 7, 2026

View reviewed changes

OCA-git-bot added the approved label Apr 7, 2026

ivantodorovich approved these changes Apr 9, 2026

View reviewed changes

florian-dacosta suggested changes Apr 13, 2026

View reviewed changes

OCA-git-bot added series:18.0 mod:base_exception Module base_exception and removed approved labels Apr 13, 2026

simahawk approved these changes Apr 17, 2026

View reviewed changes

florian-dacosta approved these changes Apr 17, 2026

View reviewed changes

OCA-git-bot added the approved label Apr 17, 2026

grindtildeath force-pushed the 18.0-fix-base_exception_rollback_transaction_fix_ui branch from a38528c to f1b8586 Compare April 28, 2026 15:36

vvrossem approved these changes May 28, 2026

View reviewed changes

grindtildeath force-pushed the 18.0-fix-base_exception_rollback_transaction_fix_ui branch from c3369a1 to a581627 Compare May 28, 2026 17:59

grindtildeath added 6 commits May 28, 2026 20:05

[TEST] base_exception: Test exception rollbacking

90d0cc3

Make test decorator available for import in other modules

90dc4be

Avoid using new env for unrelated tests

8eedb1e

grindtildeath force-pushed the 18.0-fix-base_exception_rollback_transaction_fix_ui branch from a581627 to 8eedb1e Compare May 28, 2026 18:05

divad1196 approved these changes May 29, 2026

View reviewed changes

grindtildeath added 4 commits May 29, 2026 16:09

fixup! [FIX] base_exception: Rollback transaction if we detect except…

0d982fa

…ions

fixup! Avoid using new env for unrelated tests

35b92f6

fixup! [TEST] base_exception: Test exception rollbacking

995b06f

fixup! [TEST] base_exception: Test exception rollbacking

1c461ce

SilvioC2C approved these changes May 29, 2026

View reviewed changes

	with Registry(self.env.cr.dbname).cursor() as new_cr:
	with self.env.registry.cursor() as new_cr:

	records.with_env(new_env).write({"exception_ids": [(3, rule_id)]})
	records.with_env(new_env).write({"exception_ids": [Command.unlink(rule_id)]})

	from odoo import _, api, models
	from odoo import _, api, models, Command

		self.assertFalse(self.po.exception_ids)
		self.assertTrue(self.po.with_env(new_env).exception_ids)

Uh oh!

Conversation

grindtildeath commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OCA-git-bot commented Apr 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hparfr commented Apr 2, 2026

Uh oh!

grindtildeath commented Apr 6, 2026

Uh oh!

rousseldenis left a comment

Choose a reason for hiding this comment

Uh oh!

florian-dacosta commented Apr 8, 2026

Uh oh!

grindtildeath commented Apr 8, 2026

Uh oh!

ivantodorovich left a comment

Choose a reason for hiding this comment

Uh oh!

florian-dacosta left a comment

Choose a reason for hiding this comment

Uh oh!

grindtildeath commented Apr 13, 2026

Uh oh!

florian-dacosta commented Apr 14, 2026

Uh oh!

simahawk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simahawk commented Apr 17, 2026

Uh oh!

florian-dacosta commented Apr 17, 2026

Uh oh!

hparfr commented May 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

grindtildeath commented Apr 1, 2026 •

edited

Loading