brbu - Improving the audit trail feature

[AarnorDeDardaliel]

Current behavior before PR:

• No information about restrictions in audits
• Limited grouping, searching and filtering

Desired behavior after PR is merged:

• Audit trail is a default feature
• An option allows to enter a well defined restricted mode
• For Germany, the setting does not appear and is automatically set
• Allows grouping, searching and filtering by log message

PR Enterprise and upgrade :

• https://github.com/odoo/enterprise/pull/82053
• https://github.com/odoo/upgrade/pull/7493

Link to the task :

• https://www.odoo.com/odoo/project/967/tasks/4637051

[robodoo]

[malb-odoo]

Just did a quick look and see something that could be changed here 😄

[malb-odoo]

Quick check 😄

[william-andre]

I really don't get this part:

• what is that comment about "move lines"?
• that check on the company restrictive_audit_trail seems redundant with the check on the preview string?
• why _t instead of env._?
• can't you check the tracking_value_ids instead of that string?

[AarnorDeDardaliel]

Sorry I pushed this too early, the _t is a mistake.

The check is not redundant :

• The check on the string aims to check if the current message is about the setting toggling.
• The check on the restrictive_audit_trail verifies that we are in restricted mode
  We want to make sure that in restricted mode, the setting toggling log can't be deleted, even if the company hasn't issued any move line yet (we have to be sure since when the setting was toggled)

I don't see how tracking_value_ids can be used here ?

[william-andre]

I don't see how tracking_value_ids can be used here ?

Where does "Restrictive Audit Trail" come from? Isn't it from tracking_value_ids.field_id.field_description? Can't you just check the model of res_model and the fields modified if we are updating res.company?

[william-andre]

@robodoo r+

@AarnorDeDardaliel FYI I applied this patch since we need to merge today. Feel free to have a look at it, even after the merge; I might have missed something.
I tried to keep things simple and a small diff

diff --git a/addons/account/models/company.py b/addons/account/models/company.py
index 4fda31a1161b..8d575eb39d8a 100644
--- a/addons/account/models/company.py
+++ b/addons/account/models/company.py
@@ -256,8 +256,7 @@ class ResCompany(models.Model):
     force_restrictive_audit_trail = fields.Boolean(
         string='Force Audit Trail',
         compute='_compute_force_restrictive_audit_trail',
-        help="Force the restrictive audit trail mode, and hide the corresponding setting.",
-    )
+    )  # Force the restrictive audit trail mode, and hide the corresponding setting.",

     # Autopost Wizard
     autopost_bills = fields.Boolean(string='Auto-validate bills', default=True)
@@ -308,7 +307,7 @@ class ResCompany(models.Model):
     def _check_audit_trail_restriction(self):
         companies = self.filtered(lambda c: not c.restrictive_audit_trail and c.force_restrictive_audit_trail)
         if companies:
-            raise ValidationError(_("Can't disable restricted audit trail : forced by localization."))
+            raise ValidationError(_("Can't disable restricted audit trail: forced by localization."))

     @api.constrains("account_price_include")
     def _check_set_account_price_include(self):
diff --git a/addons/account/models/mail_message.py b/addons/account/models/mail_message.py
index b99e48409966..1da246e6a822 100644
--- a/addons/account/models/mail_message.py
+++ b/addons/account/models/mail_message.py
@@ -6,23 +6,23 @@ from odoo.fields import Domain
 bypass_token = object()
 DOMAINS = {
     'res.company':
-        lambda rec, operator, value: [('id', 'in', rec.env['account.move.line'].sudo()._search([
-            ('company_id.restrictive_audit_trail', '=', True)
-        ]).subselect('company_id'))],
+        lambda rec, operator, value: [('id', 'in', rec.env['account.move.line']._where_calc([
+            ('company_id.restrictive_audit_trail', operator, value),
+        ], active_test=False).subselect('company_id'))],
     'account.move':
         lambda rec, operator, value: [('company_id.restrictive_audit_trail', operator, value)],
     'account.account':
         lambda rec, operator, value: [('used', operator, value), ('company_ids.restrictive_audit_trail', operator, value)],
     'account.tax':
-        lambda rec, operator, value: [('id', 'in', rec.env['account.move.line'].sudo()._search([
+        lambda rec, operator, value: [('id', 'in', rec.env['account.move.line']._where_calc([
             ('tax_line_id', '!=', False),
-            ('company_id.restrictive_audit_trail', '=', True),
-        ]).subselect('tax_line_id'))],
+            ('company_id.restrictive_audit_trail', operator, value),
+        ], active_test=False).subselect('tax_line_id'))],
     'res.partner':
-        lambda rec, operator, value: [('id', 'in', rec.env['account.move.line'].sudo()._search([
+        lambda rec, operator, value: [('id', 'in', rec.env['account.move.line']._where_calc([
             ('partner_id', '!=', False),
-            ('company_id.restrictive_audit_trail', '=', True),
-        ]).subselect('partner_id'))],
+            ('company_id.restrictive_audit_trail', operator, value),
+        ], active_test=False).subselect('partner_id'))],
     }


@@ -134,17 +134,10 @@ class MailMessage(models.Model):
         return self._search_audit_log_related_record_id('res.partner', operator, value)

     def _compute_account_audit_log_restricted(self):
-        self['account_audit_log_restricted'] = False
-
-        for model, domain_factory in DOMAINS.items():
-            messages_of_related = self.filtered(lambda m: m.model == model and m.res_id)
-            if messages_of_related:
-                domain = domain_factory(self, '=', True)
-                related_recs = self.env[model].sudo().search([('id', 'in', messages_of_related.mapped('res_id'))] + domain)
-                recs_by_id = {record.id: record for record in related_recs}
-                for message in messages_of_related:
-                    if message.message_type == 'notification' and recs_by_id.get(message.res_id):
-                        message.account_audit_log_restricted = True
+        self.account_audit_log_restricted = False
+        if potentially_restricted := self.filtered(lambda r: r.model in DOMAINS):
+            restricted = self.search(Domain('id', 'in', potentially_restricted.ids) + self._search_account_audit_log_restricted('in', [True]))
+            restricted.account_audit_log_restricted = True

     def _search_account_audit_log_restricted(self, operator, value):
         if operator not in ('in', 'not in'):
@@ -158,11 +151,8 @@ class MailMessage(models.Model):
     def _compute_audit_log_related_record_id(self, model, fname):
         messages_of_related = self.filtered(lambda m: m.model == model and m.res_id)
         (self - messages_of_related)[fname] = False
-        if messages_of_related:
-            related_recs = self.env[model].sudo().search([('id', 'in', messages_of_related.mapped('res_id'))])
-            recs_by_id = {record.id: record for record in related_recs}
-            for message in messages_of_related:
-                message[fname] = recs_by_id.get(message.res_id, False)
+        for message in messages_of_related:
+            message[fname] = message.res_id

     def _search_audit_log_related_record_id(self, model, operator, value):
         if (
@@ -187,24 +177,10 @@ class MailMessage(models.Model):
     def _except_audit_log(self):
         if self.env.context.get('bypass_audit') is bypass_token:
             return
-        to_check = self
-        partner_message = self.filtered(lambda m: m.account_audit_log_partner_id)
-        if partner_message:
-            # The audit trail uses the cheaper check on `customer_rank`, but that field could be set
-            # without actually having an invoice linked (i.e. creation of the contact through the
-            # Invoicing/Customers menu)
-            has_related_move = self.env['account.move'].sudo().search_count([
-                ('partner_id', 'in', partner_message.account_audit_log_partner_id.ids),
-            ], limit=1)
-            if not has_related_move:
-                to_check -= partner_message
-        for message in to_check:
+        for message in self:
             if message.account_audit_log_move_id and not message.account_audit_log_move_id.posted_before:
                 continue
-            # When a company has no move line yet, they can delete every log, as none is linked to a journal Item
-            # But we always want to know when was the last time the setting was set to True, so we need an additional check
-            setting_toggling = message.account_audit_log_company_id and self.env._("Restrictive Audit Trail") in message.account_audit_log_preview
-            if message.account_audit_log_restricted or (setting_toggling and message.account_audit_log_company_id.restrictive_audit_trail):
+            if message.account_audit_log_restricted:
                 raise UserError(self.env._("You cannot remove parts of a restricted audit trail. Archive the record instead."))

     def write(self, vals):
diff --git a/addons/account/tests/test_audit_trail.py b/addons/account/tests/test_audit_trail.py
index 3f2ad7d4719d..394425d3ed4c 100644
--- a/addons/account/tests/test_audit_trail.py
+++ b/addons/account/tests/test_audit_trail.py
@@ -53,21 +53,21 @@ class TestAuditTrail(AccountTestInvoicingCommon):
         self.env.company.restrictive_audit_trail = True
         self.move.action_post()
         self.move.button_draft()
-        with self.assertRaisesRegex(UserError, "You cannot remove parts of a restricted audit trail. Archive the record instead."):
+        with self.assertRaisesRegex(UserError, "remove parts of a restricted audit trail"):
             self.move.unlink()

     def test_cant_unlink_message(self):
         self.env.company.restrictive_audit_trail = True
         self.move.action_post()
         audit_trail = self.get_trail(self.move)
-        with self.assertRaisesRegex(UserError, "You cannot remove parts of a restricted audit trail. Archive the record instead."):
+        with self.assertRaisesRegex(UserError, "remove parts of a restricted audit trail"):
             audit_trail.unlink()

     def test_cant_unown_message(self):
         self.env.company.restrictive_audit_trail = True
         self.move.action_post()
         audit_trail = self.get_trail(self.move)
-        with self.assertRaisesRegex(UserError, "You cannot remove parts of a restricted audit trail. Archive the record instead."):
+        with self.assertRaisesRegex(UserError, "remove parts of a restricted audit trail"):
             audit_trail.res_id = 0

     def test_cant_unlink_tracking_value(self):
@@ -78,7 +78,7 @@ class TestAuditTrail(AccountTestInvoicingCommon):
         audit_trail = self.get_trail(self.move)
         trackings = audit_trail.tracking_value_ids.sudo()
         self.assertTrue(trackings)
-        with self.assertRaisesRegex(UserError, "You cannot remove parts of a restricted audit trail. Archive the record instead."):
+        with self.assertRaisesRegex(UserError, "remove parts of a restricted audit trail"):
             trackings.unlink()

     def test_content(self):
diff --git a/addons/l10n_de/tests/test_audit_trail.py b/addons/l10n_de/tests/test_audit_trail.py
index 909588e2b78e..cafd364d1b6e 100644
--- a/addons/l10n_de/tests/test_audit_trail.py
+++ b/addons/l10n_de/tests/test_audit_trail.py
@@ -35,7 +35,7 @@ class TestAuditTrailDE(AccountTestInvoicingHttpCommon):
         self.assertEqual(self.env.company.country_id.code, 'DE')
         self.assertEqual(self.env.company.account_fiscal_country_id.code, 'DE')
         self.assertTrue(self.env.company.restrictive_audit_trail)
-        with self.assertRaisesRegex(UserError, "Can't disable restricted audit trail : forced by localization."):
+        with self.assertRaisesRegex(UserError, "Can't disable restricted audit trail: forced by localization."):
             self.env.company.restrictive_audit_trail = False

     def test_audit_trail_de(self):
@@ -59,7 +59,7 @@ class TestAuditTrailDE(AccountTestInvoicingHttpCommon):
         first_attachment.unlink()
         self.assertTrue(first_attachment.exists())
         # But we cannot entirely remove it
-        with self.assertRaisesRegex(UserError, "You cannot remove parts of a restricted audit trail."):
+        with self.assertRaisesRegex(UserError, "remove parts of a restricted audit trail."):
             first_attachment.unlink()

         # Print a second time the invoice, it generates a new attachment
@@ -103,11 +103,11 @@ class TestAuditTrailDE(AccountTestInvoicingHttpCommon):
         self._send_and_print(invoice)
         attachment = invoice.message_main_attachment_id

-        with self.assertRaisesRegex(UserError, "You cannot remove parts of a restricted audit trail."):
+        with self.assertRaisesRegex(UserError, "remove parts of a restricted audit trail."):
             attachment.write({
                 'res_id': self.env.user.id,
                 'res_model': self.env.user._name,
             })

-        with self.assertRaisesRegex(UserError, "You cannot remove parts of a restricted audit trail."):
+        with self.assertRaisesRegex(UserError, "remove parts of a restricted audit trail."):
             attachment.datas = b'new data'

[william-andre]

@robodoo r+

[robodoo]

@AarnorDeDardaliel @william-andre unable to stage: merge conflict

[william-andre]

@robodoo r+

[robodoo]

@AarnorDeDardaliel @william-andre 'ci/runbot' failed on this reviewed PR.

[william-andre]

@robodoo priority