[REF] mail: remove 'sequence' on tracking value, use insert order

PURPOSE

Simplify 'mail.tracking.value' model and code. Remove unnecessary fields and
computation. Make code easier to handle and more batch-enabled.

SPECIFICATIONS

Remove 'tracking_sequence' on tracking values in DB. Consider insertion order
should be done accordingly and display them based on ID DESC. Sequence is now
used only for order records to insert in DB. Feature is still the same (order
based on sequence) but without having to store the sequence itself. It was
never updated anyway.

Task-3345979 (Mail: Simplify tracking model)

Part-of: odoo/odoo#124182

addons/mail/models/mail_tracking_value.py

@@ -10,7 +10,7 @@ class MailTracking(models.Model):
     _name = 'mail.tracking.value'
     _description = 'Mail Tracking Value'
     _rec_name = 'field'
-    _order = 'tracking_sequence asc'
+    _order = 'id DESC'
 
     field = fields.Many2one('ir.model.fields', required=True, readonly=True, index=True, ondelete='cascade')
     field_desc = fields.Char('Field Description', required=True, readonly=True)
@@ -36,8 +36,6 @@ class MailTracking(models.Model):
 
     mail_message_id = fields.Many2one('mail.message', 'Message ID', required=True, index=True, ondelete='cascade')
 
-    tracking_sequence = fields.Integer('Tracking field sequence', readonly=True, default=100)
-
     @api.depends('mail_message_id', 'field')
     def _compute_field_groups(self):
         for tracking in self:
@@ -46,7 +44,7 @@ class MailTracking(models.Model):
             tracking.field_groups = field.groups if field else 'base.group_system'
 
     @api.model
-    def _create_tracking_values(self, initial_value, new_value, col_name, col_info, tracking_sequence, record):
+    def _create_tracking_values(self, initial_value, new_value, col_name, col_info, record):
         """ Prepare values to create a mail.tracking.value. It prepares old and
         new value according to the field type.
 
@@ -56,8 +54,6 @@ class MailTracking(models.Model):
           date, datetime, ...;
         :param str col_name: technical field name, column name (e.g. 'user_id);
         :param dict col_info: result of fields_get(col_name);
-        :param int tracking_sequence: sequence used for ordering tracking
-          value display;
         :param <record> record: record on which tracking is performed, used for
           related computation e.g. finding currency of monetary fields;
 
@@ -67,7 +63,7 @@ class MailTracking(models.Model):
         if not field:
             raise ValueError(f'Unknown field {col_name} on model {record._name}')
 
-        values = {'field': field.id, 'field_desc': col_info['string'], 'field_type': col_info['type'], 'tracking_sequence': tracking_sequence}
+        values = {'field': field.id, 'field_desc': col_info['string'], 'field_type': col_info['type']}
 
         if col_info['type'] in {'integer', 'float', 'char', 'text', 'datetime', 'monetary'}:
             values.update({
@@ -110,11 +106,21 @@ class MailTracking(models.Model):
 
     def _tracking_value_format(self):
         """ Return structure and formatted data structure to be used by chatter
-        to display tracking values.
+        to display tracking values. Order it according to asked display, aka
+        ascending sequence (and field name).
 
         :return list: for each tracking value in self, their formatted display
           values given as a dict;
         """
+        if not self:
+            return []
+        field_models = self.field.mapped('model')
+        if len(set(field_models)) != 1:
+            raise ValueError('All tracking value should belong to the same model.')
+        TrackedModel = self.env[field_models[0]]
+        tracked_fields = TrackedModel.fields_get(self.field.mapped('name'), attributes={'string', 'type'})
+        fields_sequence_map = dict(TrackedModel._mail_track_order_fields(tracked_fields))
+
         formatted = []
         for tracking in self:
             formatted.append({
@@ -131,6 +137,10 @@ class MailTracking(models.Model):
                     'value': tracking._format_display_value(new=False)[0],
                 },
             })
+        formatted.sort(
+            key=lambda info: (fields_sequence_map[info['fieldName']], info['fieldName']),
+            reverse=False,
+        )
         return formatted
 
     def _format_display_value(self, new=True):

addons/mail/models/models.py

@@ -81,39 +81,60 @@ class BaseModel(models.AbstractModel):
           fields;
 
         :return: a tuple (changes, tracking_value_ids) where
-          changes: set of updated column names;
+          changes: set of updated column names; contains onchange tracked fields
+          that changed;
           tracking_value_ids: a list of ORM (0, 0, values) commands to create
           ``mail.tracking.value`` records;
 
         Override this method on a specific model to implement model-specific
         behavior. Also consider inheriting from ``mail.thread``. """
         self.ensure_one()
-        changes = set()  # contains onchange tracked fields that changed
+        updated = set()
         tracking_value_ids = []
 
-        # generate tracked_values data structure: {'col_name': {col_info, new_value, old_value}}
-        for col_name, col_info in tracked_fields.items():
+        fields_track_info = self._mail_track_order_fields(tracked_fields)
+        for col_name, _sequence in fields_track_info:
             if col_name not in initial_values:
                 continue
-            initial_value = initial_values[col_name]
-            new_value = self[col_name]
+            initial_value, new_value = initial_values[col_name], self[col_name]
+            if new_value == initial_value or (not new_value and not initial_value):  # because browse null != False
+                continue
 
-            if new_value != initial_value and (new_value or initial_value):  # because browse null != False
-                tracking_sequence = getattr(self._fields[col_name], 'tracking',
-                                            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(
+            updated.add(col_name)
+            tracking_value_ids.append(
+                [0, 0, self.env['mail.tracking.value']._create_tracking_values(
                     initial_value, new_value,
-                    col_name, col_info,
-                    tracking_sequence,
+                    col_name, tracked_fields[col_name],
                     self
-                )
-                if tracking:
-                    tracking_value_ids.append([0, 0, tracking])
-                changes.add(col_name)
+                )])
 
-        return changes, tracking_value_ids
+        return updated, tracking_value_ids
+
+    def _mail_track_order_fields(self, tracked_fields):
+        """ Order tracking, based on sequence found on field definition. When
+        having several identical sequences, field name is used. """
+        fields_track_info = [
+            (col_name, self._mail_track_get_field_sequence(col_name))
+            for col_name in tracked_fields.keys()
+        ]
+        # sorting: sequence ASC, name ASC (higher sequence -> displayed last, then
+        # order by name). Model order being id DESC (aka: first insert -> last
+        # displayed) insert should be done by descending sequence then descending
+        # name.
+        fields_track_info.sort(key=lambda item: (item[1], item[0]), reverse=True)
+        return fields_track_info
+
+    def _mail_track_get_field_sequence(self, fname):
+        """ Find tracking sequence of a given field, given their name. Current
+        parameter 'tracking' should be an integer, but attributes with True
+        are still supported; old naming 'track_sequence' also. """
+        sequence = getattr(
+            self._fields[fname], 'tracking',
+            getattr(self._fields[fname], 'track_sequence', 100)
+        )
+        if sequence is True:
+            sequence = 100
+        return sequence
 
     def _message_get_default_recipients(self):
         """ Generic implementation for finding default recipient to mail on

addons/mail/views/mail_tracking_value_views.xml

@@ -34,7 +34,6 @@
                             <field name="field"/>
                             <field name="field_desc"/>
                             <field name="field_type"/>
-                            <field name="tracking_sequence"/>
                         </group>
                         <group name="values">
                             <group name="old_values" string="Old values">

addons/test_mail/tests/test_message_track.py

@@ -647,7 +647,6 @@ class TestTrackingInternals(MailCommon):
             self.env['mail.tracking.value']._create_tracking_values(
                 '', 'Test',
                 'not_existing_field', {'string': 'Test', 'type': 'char'},
-                0,
                 test_record,
             )
 
@@ -656,30 +655,70 @@ class TestTrackingInternals(MailCommon):
             self.env['mail.tracking.value']._create_tracking_values(
                 '', '<p>Html</p>',
                 'html_field', {'string': 'HTML', 'type': 'html'},
-                0,
                 test_record,
             )
 
     @users('employee')
     def test_track_sequence(self):
-        """ Update some tracked fields and check that the mail.tracking.value are ordered according to their tracking_sequence"""
+        """ Update some tracked fields and check that the mail.tracking.value
+        are ordered according to their tracking_sequence """
         record = self.record.with_env(self.env)
         self.assertEqual(len(record.message_ids), 1)
+        # order: user_id -> 1, customer_id -> 2, container_id -> True -> 100, email_from -> True -> 100
+        ordered_fnames = ['user_id', 'customer_id', 'container_id', 'email_from']
+
+        # Update tracked fields, should generate tracking values correctly ordered
         record.write({
-            'name': 'Zboub',
+            'container_id': self.env['mail.test.container'].with_context(mail_create_nosubscribe=True).create({'name': 'Container'}).id,
             'customer_id': self.user_admin.partner_id.id,
+            'email_from': 'new.from@test.example.com',
+            'name': 'Zboub',
             'user_id': self.user_admin.id,
-            'container_id': self.env['mail.test.container'].with_context(mail_create_nosubscribe=True).create({'name': 'Container'}).id
         })
         self.flush_tracking()
         self.assertEqual(len(record.message_ids), 2, 'should have 1 new tracking message')
-
         tracking_values = self.env['mail.tracking.value'].sudo().search(
             [('mail_message_id', '=', record.message_ids[0].id)]
         )
-        self.assertEqual(tracking_values[0].tracking_sequence, 1)
-        self.assertEqual(tracking_values[1].tracking_sequence, 2)
-        self.assertEqual(tracking_values[2].tracking_sequence, 100)
+        self.assertEqual(
+            tracking_values.field.mapped('name'),
+            ordered_fnames,
+            'Track: order, based on ID DESC, should follow tracking sequence (or name) on field'
+        )
+
+        # Manually create trackings, format should be the fallback to reorder them
+        new_msg = record.message_post(
+            body='Manual Hack of tracking',
+            subtype_xmlid='mail.mt_note',
+        )
+        custom_order_fnames = ['container_id', 'customer_id', 'email_from', 'user_id']
+        field_ids = [
+            self.env['ir.model.fields']._get(record._name, fname).id
+            for fname in custom_order_fnames
+        ]
+        self.env['mail.tracking.value'].sudo().create([
+            {
+                'field': field_id,
+                'mail_message_id': new_msg.id,
+                'old_value_char': 'unimportant',
+                'new_value_char': 'unimportant',
+            }
+            for field_id in field_ids
+        ])
+        tracking_values = self.env['mail.tracking.value'].sudo().search(
+            [('mail_message_id', '=', record.message_ids[0].id)]
+        )
+        self.assertEqual(
+            tracking_values.field.mapped('name'),
+            list(reversed(custom_order_fnames)),
+            'Tracking model: order, based on ID DESC, following reverted insertion'
+        )
+        tracking_formatted = tracking_values._tracking_value_format()
+        self.assertEqual(
+            [t['fieldName'] for t in tracking_formatted],
+            ordered_fnames,
+            'Track: formatted order is correctly based on field sequence definition'
+        )
 
     @users('employee')
     def test_unlinked_field(self):