From b4cb36ed8083539cace6435674a69f78e9131d70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 7 Jun 2023 17:41:45 +0200 Subject: [PATCH] [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 | 26 +++++--- addons/mail/models/models.py | 59 +++++++++++++------ .../mail/views/mail_tracking_value_views.xml | 1 - addons/test_mail/tests/test_message_track.py | 57 +++++++++++++++--- 4 files changed, 106 insertions(+), 37 deletions(-) diff --git a/addons/mail/models/mail_tracking_value.py b/addons/mail/models/mail_tracking_value.py index 2b664e16f87..17dbc569363 100644 --- a/addons/mail/models/mail_tracking_value.py +++ b/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 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): diff --git a/addons/mail/models/models.py b/addons/mail/models/models.py index 1b38fcc2693..fe6864fa3a6 100644 --- a/addons/mail/models/models.py +++ b/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 diff --git a/addons/mail/views/mail_tracking_value_views.xml b/addons/mail/views/mail_tracking_value_views.xml index 793023d1092..1d6cb2769cc 100644 --- a/addons/mail/views/mail_tracking_value_views.xml +++ b/addons/mail/views/mail_tracking_value_views.xml @@ -34,7 +34,6 @@ - diff --git a/addons/test_mail/tests/test_message_track.py b/addons/test_mail/tests/test_message_track.py index c017f735df8..783d70b4497 100644 --- a/addons/test_mail/tests/test_message_track.py +++ b/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( '', '

Html

', '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):