[FIX] account: block deletion of used tax groups by smjo-odoo · Pull Request #245243 · odoo/odoo

smjo-odoo · 2026-01-22T13:11:49Z

Before this PR:

Group of taxes can be deleted by a user, even if they are used.

After this PR:

If a user tries to delete a group of taxes in use, a validation
error is raised.
Fixed a test case in POS and deactivated the tax instead of
deleting the tax.

Related PR: https://github.com/odoo/enterprise/pull/105174

task-5472834

robodoo · 2026-01-22T13:11:52Z

jco-odoo · 2026-02-27T14:39:50Z

addons/account/models/account_tax.py

            return ''
        return html2plaintext(self.description)

+    @api.ondelete(at_uninstall=False)


We have an ondelete="restrict" on tax_line_id. Would not an ondelete="restrict" on tax_ids (on account_move_line) do the same trick? Why this choice to pass through is_used?

Would not this avoid the enterprise PR as well?

Why this choice to pass through is_used?

@jco-odoo, the reason for using is_used is that the tax group might be used in different modules, like sale, PoS, purchase, etc. We have the _hook_compute_is_used method that computes the IDs of taxes used in other modules and returns a set of IDs of taxes from that input set that are used in transactions.

If we use ondelete='restrict' on tax_ids in account.move.line, It will only prevent the deletion in invoicing module and we would still be able to delete tax groups used in other modules.

If the is_used field can be used for preventing the modification of taxes. Why should we not use the same field to prevent the deletion of used tax groups ?

Would not this avoid the enterprise PR as well?

The enterprise PR would still be needed because in that particular test case, the goal is to create a bill with "no tax", and not to delete/unlink the tax. This is the reason why we used Command.clear() instead of unlink() on the tax_ids.

Hello @jco-odoo Do we have any update on this PR ?

@smjo-odoo I had not checked that far and that is_used seems to be in handy for it. I see that the ondelete='restrict' is also not on some other models, where we could have simply put it. (hr.expense, purchase.order.line, sale.order.line) But e.g. the method is not inherited for sale.order.line... Also, such changes are huge in 18.3 for still solving a corner case (although it is not 18 stable), but it can be a good idea to enforce it this way.

I see @aboo-odoo worked a lot on the is_used, so maybe he can already tell why it is not on sale order line or if it can be a good approach for it.

The initial idea behind is_used was to check if a tax was used to restrict/track modification of used taxes (restriction has been removed as per FP request). I don't recall, nor see any reason anywhere on why the is_used is not applied to SOs and SOLs - maybe this was said during a discussion with TSB but not written anywhere.

However, it seems that here you are making it so that any taxes (not just tax groups) cannot be deleted after it as been used. If this is the case, I'm pretty sure this should be further discussed and validated.

edit: is_used is quite heavy to compute but I think it's okay to use it to check whether or not a tax should or not be deleted (again - if it has been functionally agreed upon)

@jco-odoo Should we continue with the is_used approach ?

However, it seems that here you are making it so that any taxes (not just tax groups) cannot be deleted after it as been used. If this is the case, I'm pretty sure this should be further discussed and validated.

@aboo-odoo Would it be fine to use context(force_delete=True) to allow a forced deletion?
This way, we could bypass the blocking error when intentionally force-deleting a group of taxes, while still keeping the restriction in normal cases.

edit: is_used is quite heavy to compute but I think it's okay to use it to check whether or not a tax should or not be deleted (again - if it has been functionally agreed upon)

Especially since the SQL logic is flawed.

SELECT id FROM account_tax WHERE EXISTS( SELECT 1 FROM account_move_line_account_tax_rel AS line WHERE account_tax_id IN %s AND account_tax.id = line.account_tax_id )

Should be

SELECT id FROM account_tax WHERE EXISTS( SELECT 1 FROM account_move_line_account_tax_rel AS line WHERE account_tax.id = line.account_tax_id ) AND id IN %s

Same for all the similar queries of course.

william-andre

Using is_used is fine for me; it makes sense to protect the taxes against edition and deletion the same way.

addons/account/tests/test_account_tax.py

william-andre · 2026-03-13T11:09:59Z

addons/account/models/account_tax.py

            return ''
        return html2plaintext(self.description)

+    @api.ondelete(at_uninstall=False)


edit: is_used is quite heavy to compute but I think it's okay to use it to check whether or not a tax should or not be deleted (again - if it has been functionally agreed upon)

Especially since the SQL logic is flawed.

SELECT id FROM account_tax WHERE EXISTS( SELECT 1 FROM account_move_line_account_tax_rel AS line WHERE account_tax_id IN %s AND account_tax.id = line.account_tax_id )

Should be

SELECT id FROM account_tax WHERE EXISTS( SELECT 1 FROM account_move_line_account_tax_rel AS line WHERE account_tax.id = line.account_tax_id ) AND id IN %s

Same for all the similar queries of course.

jco-odoo · 2026-03-16T09:06:09Z

@william-andre @smjo-odoo I think the _is_used makes sense too, but I want to confirm with PO as it changes the behaviour in stable.

jco-odoo · 2026-03-17T13:24:59Z

addons/account/models/account_tax.py

-                tuple(self.ids),
+                        WHERE account_tax.id = line.account_tax_id
+                    ) AND id IN %s
+                    """, tuple(self.ids),


The simplest would be to not use _is_used but only put this one in a separate method and call it. In principle, the tax is mainly important for accounting (in principle you would still have the tax tags but it is more difficult to track where they come from then) and if it gets deleted on expense, ... it is less of a problem. (discussed with PO) If we would keep the _is_used, we would need a very precise error message to tell where the user would need to go to delete the tax. Doing this is also better in stable that the ondelete restrict as we would need an update of the module for that. (so only the tax_ids on account.move.line but use this query instead of the ondelete restrict)

You can simply add smart buttons on the tax to jump to the different related models.

In the meantime, this fix is alright; even if it could be improved.

That would be a lot of buttons just to tell where it is used...

You're right, but I don't think that it matters that much...

addons/account/models/account_tax.py

william-andre · 2026-03-17T15:13:28Z

addons/account/models/account_tax.py

-                tuple(self.ids),
+                        WHERE account_tax.id = line.account_tax_id
+                    ) AND id IN %s
+                    """, tuple(self.ids),


You can simply add smart buttons on the tax to jump to the different related models.

In the meantime, this fix is alright; even if it could be improved.

Before this commit: - Group of taxes can be deleted by a user, even if they are used. After this commit: - If a user tries to delete a group of taxes in use, a validation error is raised. - Fixed a test case in POS and deactivated the tax instead of deleting the tax. Task: 5472834

Problem: - Tax usage queries filter inside subqueries (`account_tax_id IN %s`), causing unnecessary scans of the account_tax table. Solution: - moved `id IN %s` to outer query to restrict records earlier and reduce unnecessary scanning of account_tax table. task-5472834

jco-odoo · 2026-03-18T10:43:56Z

@robodoo r+ rebase-ff

robodoo · 2026-03-18T10:43:58Z

Merge method set to rebase and fast-forward.

smjo-odoo force-pushed the saas-18.3-prevent-tax-grp-deletion-if-used-smjo branch from f751df1 to ba2d5cf Compare January 28, 2026 05:42

This comment was marked as resolved.

Sign in to view

smjo-odoo force-pushed the saas-18.3-prevent-tax-grp-deletion-if-used-smjo branch from ba2d5cf to cf820cc Compare February 3, 2026 04:59

zepa-odoo approved these changes Feb 3, 2026

View reviewed changes

smjo-odoo marked this pull request as ready for review February 16, 2026 06:27

C3POdoo requested review from a team, manv-afk and yosuanicolaus and removed request for a team February 16, 2026 06:30

yosuanicolaus approved these changes Feb 27, 2026

View reviewed changes

yosuanicolaus requested a review from jco-odoo February 27, 2026 14:14

jco-odoo reviewed Feb 27, 2026

View reviewed changes

william-andre reviewed Mar 13, 2026

View reviewed changes

smjo-odoo force-pushed the saas-18.3-prevent-tax-grp-deletion-if-used-smjo branch from cf820cc to bad65bb Compare March 17, 2026 10:17

C3POdoo requested review from a team and ajf-odoo and removed request for a team March 17, 2026 10:21

jco-odoo reviewed Mar 17, 2026

View reviewed changes

william-andre reviewed Mar 17, 2026

View reviewed changes

smjo-odoo added 2 commits March 18, 2026 11:01

smjo-odoo force-pushed the saas-18.3-prevent-tax-grp-deletion-if-used-smjo branch from bad65bb to f05ac78 Compare March 18, 2026 05:32

Conversation

smjo-odoo commented Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robodoo commented Jan 22, 2026

Uh oh!

This comment was marked as resolved.

Uh oh!

jco-odoo Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smjo-odoo Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

aboo-odoo Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

william-andre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jco-odoo commented Mar 16, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jco-odoo commented Mar 18, 2026

Uh oh!

robodoo commented Mar 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

smjo-odoo commented Jan 22, 2026 •

edited

Loading

jco-odoo Feb 27, 2026 •

edited

Loading

smjo-odoo Mar 2, 2026 •

edited

Loading

aboo-odoo Mar 11, 2026 •

edited

Loading