[IMP] mail: track fields based on ir_model_fields

The tracked field is now a relation to the corresponding ir.model.field.
This prevents potential privacy issues should the field be deleted or renamed.

closes odoo/odoo#39232

Taskid: 2088634
Signed-off-by: Yannick Tivisse (yti) <yti@odoo.com>

8 files changed, 52 insertions, 29 deletions

diff --git a/addons/mail/models/ir_model_fields.py b/addons/mail/models/ir_model_fields.py
index bf9ff3e2de2..e8650ce2d6d 100644
--- a/addons/mail/models/ir_model_fields.py
+++ b/addons/mail/models/ir_model_fields.py
@@ -30,22 +30,3 @@ class IrModelField(models.Model):
         if attrs and field_data.get('tracking'):
             attrs['tracking'] = field_data['tracking']
         return attrs
-
-    def unlink(self):
-        """
-        Delete 'mail.tracking.value's when a module is uninstalled
-        """
-        if self:
-            query = """
-                DELETE FROM mail_tracking_value
-                WHERE id IN (
-                    SELECT t.id
-                    FROM mail_tracking_value t
-                    INNER JOIN mail_message m ON (m.id = t.mail_message_id)
-                    INNER JOIN ir_model_fields f ON (t.field = f.name AND m.model = f.model)
-                    WHERE f.id IN %s
-                );
-            """
-            self.flush()
-            self.env.cr.execute(query, (tuple(self.ids),))
-        return super(IrModelField, self).unlink()
diff --git a/addons/mail/models/mail_thread.py b/addons/mail/models/mail_thread.py
index 499b0461b32..a18aa50727e 100644
--- a/addons/mail/models/mail_thread.py
+++ b/addons/mail/models/mail_thread.py
@@ -616,7 +616,7 @@ class MailThread(models.AbstractModel):
                                             getattr(self._fields[col_name], 'track_sequence', 100))  # backward compatibility with old parameter name
                 if tracking_sequence is True:
                     tracking_sequence = 100
-                tracking = self.env['mail.tracking.value'].create_tracking_values(initial_value, new_value, col_name, col_info, tracking_sequence)
+                tracking = self.env['mail.tracking.value'].create_tracking_values(initial_value, new_value, col_name, col_info, tracking_sequence, self._name)
                 if tracking:
                     tracking_value_ids.append([0, 0, tracking])
                 changes.add(col_name)
diff --git a/addons/mail/models/mail_tracking_value.py b/addons/mail/models/mail_tracking_value.py
index a35d10c9c0c..cc2572bc5bc 100644
--- a/addons/mail/models/mail_tracking_value.py
+++ b/addons/mail/models/mail_tracking_value.py
@@ -12,8 +12,7 @@ class MailTracking(models.Model):
     _rec_name = 'field'
     _order = 'tracking_sequence asc'
 
-    # TDE CLEANME: why not a m2o to ir model field ?
-    field = fields.Char('Changed Field', required=True, readonly=1)
+    field = fields.Many2one('ir.model.fields', required=True, readonly=1, ondelete='cascade')
     field_desc = fields.Char('Field Description', required=True, readonly=1)
     field_type = fields.Char('Field Type')
     field_groups = fields.Char(compute='_compute_field_groups')
@@ -39,13 +38,18 @@ class MailTracking(models.Model):
     def _compute_field_groups(self):
         for tracking in self:
             model = self.env[tracking.mail_message_id.model]
-            field = model._fields.get(tracking.field)
-            tracking.field_groups = field.groups if field else 'base.group_system'
+            field = model._fields.get(tracking.field.name)
+            tracking.field_groups = field.groups
 
     @api.model
-    def create_tracking_values(self, initial_value, new_value, col_name, col_info, tracking_sequence):
+    def create_tracking_values(self, initial_value, new_value, col_name, col_info, tracking_sequence, model_name):
         tracked = True
-        values = {'field': col_name, 'field_desc': col_info['string'], 'field_type': col_info['type'], 'tracking_sequence': tracking_sequence}
+
+        field = self.env['ir.model.fields']._get(model_name, col_name)
+        if not field:
+            return
+
+        values = {'field': field.id, 'field_desc': col_info['string'], 'field_type': col_info['type'], 'tracking_sequence': tracking_sequence}
 
         if col_info['type'] in ['integer', 'float', 'char', 'text', 'datetime', 'monetary']:
             values.update({
diff --git a/addons/test_mail/models/test_mail_models.py b/addons/test_mail/models/test_mail_models.py
index e9c0eaa81f3..e84c6d3ee5e 100644
--- a/addons/test_mail/models/test_mail_models.py
+++ b/addons/test_mail/models/test_mail_models.py
@@ -159,3 +159,14 @@ class MailMultiCompany(models.Model):
 
     name = fields.Char()
     company_id = fields.Many2one('res.company')
+
+
+class MailTrackingModel(models.Model):
+    _description = 'Test Tracking Model'
+    _name = 'mail.test.tracking'
+    _inherit = ['mail.thread']
+
+    name = fields.Char(required=True, tracking=True)
+    field_0 = fields.Char(tracking=True)
+    field_1 = fields.Char(tracking=True)
+    field_2 = fields.Char(tracking=True)
diff --git a/addons/test_mail/security/ir.model.access.csv b/addons/test_mail/security/ir.model.access.csv
index 90b56e67703..2753e2fbd72 100644
--- a/addons/test_mail/security/ir.model.access.csv
+++ b/addons/test_mail/security/ir.model.access.csv
@@ -16,3 +16,4 @@ access_mail_test_cc_portal,mail.test.cc.portal,model_mail_test_cc,base.group_por
 access_mail_test_cc_user,mail.test.cc.user,model_mail_test_cc,base.group_user,1,1,1,1
 access_mail_test_multi_company_user,mail.test.multi.company.user,model_mail_test_multi_company,base.group_user,1,1,1,1
 access_mail_test_multi_company_portal,mail.test.multi.company.portal,model_mail_test_multi_company,base.group_portal,1,0,0,0
+access_mail_test_tracking_user,mail.test.tracking,model_mail_test_tracking,base.group_user,1,1,1,1
diff --git a/addons/test_mail/tests/common.py b/addons/test_mail/tests/common.py
index d73a091b7b2..bda876d61ef 100644
--- a/addons/test_mail/tests/common.py
+++ b/addons/test_mail/tests/common.py
@@ -176,7 +176,7 @@ class BaseFunctionalTest(common.SavepointCase):
     def assertTracking(self, message, data):
         tracking_values = message.sudo().tracking_value_ids
         for field_name, value_type, old_value, new_value in data:
-            tracking = tracking_values.filtered(lambda track: track.field == field_name)
+            tracking = tracking_values.filtered(lambda track: track.field.name == field_name)
             self.assertEqual(len(tracking), 1)
             if value_type in ('char', 'integer'):
                 self.assertEqual(tracking.old_value_char, old_value)
diff --git a/addons/test_mail/tests/test_message_track.py b/addons/test_mail/tests/test_message_track.py
index c4482be9e9d..05f375e3b3b 100644
--- a/addons/test_mail/tests/test_message_track.py
+++ b/addons/test_mail/tests/test_message_track.py
@@ -162,6 +162,7 @@ class TestTracking(common.BaseFunctionalTest, common.MockEmails):
     def test_unlinked_field(self):
         record_sudo = self.record.sudo()
         record_sudo.write({'email_from': 'X'})  # create a tracking value
+        record_sudo.flush()
         self.assertEqual(len(record_sudo.message_ids.tracking_value_ids), 1)
         ir_model_field = self.env['ir.model.fields'].search([
             ('model', '=', 'mail.test.full'),
diff --git a/addons/test_mail/tests/test_performance.py b/addons/test_mail/tests/test_performance.py
index c26fe1a81f4..d4a32557eff 100644
--- a/addons/test_mail/tests/test_performance.py
+++ b/addons/test_mail/tests/test_performance.py
@@ -609,7 +609,7 @@ class TestMailComplexPerformance(BaseMailPerformance):
         rec1 = rec.with_context(active_test=False)      # to see inactive records
         self.assertEqual(rec1.message_partner_ids, self.user_portal.partner_id | self.env.user.partner_id)
         self.assertEqual(len(rec1.message_ids), 1)
-        with self.assertQueryCount(__system__=86, emp=87):
+        with self.assertQueryCount(__system__=87, emp=88):
             rec.write({
                 'name': 'Test2',
                 'umbrella_id': self.umbrella.id,
@@ -646,13 +646,14 @@ class TestMailComplexPerformance(BaseMailPerformance):
         rec1 = rec.with_context(active_test=False)      # to see inactive records
         self.assertEqual(rec1.message_partner_ids, self.user_portal.partner_id | self.env.user.partner_id)
 
-        with self.assertQueryCount(__system__=94, emp=95):
+        with self.assertQueryCount(__system__=95, emp=96):
             rec.write({
                 'name': 'Test2',
                 'umbrella_id': umbrella_id,
                 'customer_id': customer_id,
                 })
 
+
         self.assertEqual(rec1.message_partner_ids, self.partners | self.env.user.partner_id | self.user_portal.partner_id)
         # write tracking message
         self.assertEqual(rec1.message_ids[0].subtype_id, self.env.ref('test_mail.st_mail_test_full_umbrella_upd'))
@@ -837,3 +838,27 @@ class TestMailHeavyPerformancePost(BaseMailPerformance):
         self.assertEqual(self.attachements.mapped('res_model'), [record._name for i in range(3)])
         self.assertEqual(self.attachements.mapped('res_id'), [record.id for i in range(3)])
         # self.assertEqual(record.message_ids[0].notified_partner_ids, [])
+
+
+@tagged('mail_performance')
+class TestTrackingPerformance(BaseMailPerformance):
+
+    @users('__system__', 'demo')
+    @warmup
+    def test_tracking_multiple_fields(self):
+        """ Check that tracking multiple fields is scalable """
+
+        record = self.env['mail.test.tracking'].create({'name': 'Zizizatestname'})
+        with self.assertQueryCount(__system__=3, demo=3):
+            record.write({'name': 'Zizizanewtestname'})
+            record.flush()
+
+        with self.assertQueryCount(__system__=5, demo=5):
+            record.write({'field_%s' % (i): 'Tracked Char Fields %s' % (i) for i in range(3)})
+            record.flush()
+
+        with self.assertQueryCount(__system__=10, demo=10):
+            record.write({'field_%s' % (i): 'Field Without Cache %s' % (i) for i in range(3)})
+            record.flush()
+            record.write({'field_%s' % (i): 'Field With Cache %s' % (i) for i in range(3)})
+            record.flush()