From 11289fd128827587f6ed3fc5beed28f461c0e039 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 7 Jun 2023 18:19:50 +0200 Subject: [PATCH] [REF] mail: cleanup 'mail.tracking.value' model code PURPOSE Simplify 'mail.tracking.value' model and code. Remove unnecessary fields and computation. Make code easier to handle and more batch-enabled. SPECIFICATIONS SPEC 1: prepare all tracking values at same code place Delegate all computation of tracking values into the 'create_tracking_values' method. Curently part of it (currency field) is done in the caller. Better split code per feature. Method is also made private, as it is not required to expose it. SPEC 2: cleanup tracking value formatting methods and calls Code used to display tracking value can be simplified: remove unnecessary wrappers, make code easier to read, avoid composition of field name but use a mapping instead (easier to grep 'old_value_char' when it is effectively used). Ensure code always calls '_tracking_value_format' to ease future improvements and have all formatting code being batch-enabled. SPEC 3: simplify Chatter formatted value structure 'fieldType' is currently added in old and new values. As it is a field property it can be moved higher in the formatting result to be included only once. Also add 'fieldName', the column name, to the formatted results as it will soon help various tool methods. Moreover it makes sense to have the source of the tracking as the real column name, in addition to its string and type. Task-3345979 (Mail: Simplify tracking model) Part-of: odoo/odoo#124182 --- addons/l10n_in/models/mail_message.py | 15 ++- addons/mail/__manifest__.py | 2 +- addons/mail/models/mail_message.py | 4 +- addons/mail/models/mail_thread.py | 23 ++-- addons/mail/models/mail_tracking_value.py | 124 +++++++++++------- addons/mail/models/models.py | 22 ++-- .../mail/static/src/core/web/message_patch.js | 14 +- .../static/src/core/web/message_patch.xml | 4 +- .../mock_server/models/mail_tracking_value.js | 33 +++-- addons/mail/tests/test_res_partner.py | 4 +- ...iews.xml => mail_tracking_value_views.xml} | 0 addons/test_mail/tests/test_message_track.py | 4 +- 12 files changed, 150 insertions(+), 99 deletions(-) rename addons/mail/views/{mail_tracking_views.xml => mail_tracking_value_views.xml} (100%) diff --git a/addons/l10n_in/models/mail_message.py b/addons/l10n_in/models/mail_message.py index c85dd769062..7739102468d 100644 --- a/addons/l10n_in/models/mail_message.py +++ b/addons/l10n_in/models/mail_message.py @@ -23,14 +23,21 @@ class Message(models.Model): elif not title and message.subtype_id and not message.subtype_id.internal: title = message.subtype_id.display_name audit_log_preview = Markup("
%s
") % (title) - for value in tracking_value_ids: + trackings = [ + ( + fmt_vals['changedField'], + fmt_vals['oldValue']['value'], + fmt_vals['newValue']['value'], + ) for fmt_vals in tracking_value_ids._tracking_value_format() + ] + for field_desc, old_value, new_value in trackings: audit_log_preview += Markup( "
  • %(old_value)s %(new_value)s (%(field)s)
    • " ) % { - 'old_value': value._get_old_display_value()[0] or _("None"), - 'new_value': value._get_new_display_value()[0] or _("None"), + 'old_value': old_value, + 'new_value': new_value, 'title': _("Changed"), - 'field': value.field.field_description, + 'field': field_desc, } message.l10n_in_audit_log_preview = audit_log_preview diff --git a/addons/mail/__manifest__.py b/addons/mail/__manifest__.py index 2d3d7a8ff24..fbecf4c8276 100644 --- a/addons/mail/__manifest__.py +++ b/addons/mail/__manifest__.py @@ -71,7 +71,7 @@ For more specific needs, you may also assign custom-defined actions 'wizard/mail_template_reset_views.xml', 'views/fetchmail_views.xml', 'views/mail_message_subtype_views.xml', - 'views/mail_tracking_views.xml', + 'views/mail_tracking_value_views.xml', 'views/mail_notification_views.xml', 'views/mail_message_views.xml', 'views/mail_message_schedule_views.xml', diff --git a/addons/mail/models/mail_message.py b/addons/mail/models/mail_message.py index 4074b36fe82..2160ca586d1 100644 --- a/addons/mail/models/mail_message.py +++ b/addons/mail/models/mail_message.py @@ -989,14 +989,14 @@ class Message(models.Model): { 'changedField': "Customer", 'id': 2965, + 'fieldName': 'partner_id', + 'fieldType': 'char', 'newValue': { 'currencyId': "", - 'fieldType': 'char', 'value': "Axelor", ], 'oldValue': { 'currencyId': "", - 'fieldType': 'char', 'value': "", ], } diff --git a/addons/mail/models/mail_thread.py b/addons/mail/models/mail_thread.py index 4c9eaddb309..17922dde265 100644 --- a/addons/mail/models/mail_thread.py +++ b/addons/mail/models/mail_thread.py @@ -3333,15 +3333,22 @@ class MailThread(models.AbstractModel): ) record_name = msg_vals.get('record_name') if 'record_name' in msg_vals else message.record_name - # tracking + # tracking: in case of missing value, perform search (skip only if sure we don't have any) + check_tracking = msg_vals.get('tracking_value_ids', True) if msg_vals else bool(self) tracking = [] - if msg_vals.get('tracking_value_ids', True) if msg_vals else bool(self): # could be tracking - for tracking_value in self.env['mail.tracking.value'].sudo().search([('mail_message_id', '=', message.id)]): - groups = tracking_value.field_groups - if not groups or self.env.is_superuser() or self.user_has_groups(groups): - tracking.append((tracking_value.field_desc, - tracking_value._get_old_display_value()[0], - tracking_value._get_new_display_value()[0])) + if check_tracking: + tracking_values = self.env['mail.tracking.value'].sudo().search( + [('mail_message_id', '=', message.id)] + ).filtered( + lambda track: not track.field_groups or self.env.is_superuser() or self.user_has_groups(track.field_groups) + ) + tracking = [ + ( + fmt_vals['changedField'], + fmt_vals['oldValue']['value'], + fmt_vals['newValue']['value'], + ) for fmt_vals in tracking_values._tracking_value_format() + ] subtype_id = msg_vals.get('subtype_id') if msg_vals and 'subtype_id' in msg_vals else message.subtype_id.id is_discussion = subtype_id == self.env['ir.model.data']._xmlid_to_res_id('mail.mt_comment') diff --git a/addons/mail/models/mail_tracking_value.py b/addons/mail/models/mail_tracking_value.py index 0a157f81dab..37f7341dd32 100644 --- a/addons/mail/models/mail_tracking_value.py +++ b/addons/mail/models/mail_tracking_value.py @@ -46,10 +46,26 @@ 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, model_name): + def _create_tracking_values(self, initial_value, new_value, col_name, col_info, tracking_sequence, record): + """ Prepare values to create a mail.tracking.value. It prepares old and + new value according to the field type. + + :param initial_value: field value before the change, could be text, int, + date, datetime, ...; + :param new_value: field value after the change, could be text, int, + 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; + + :return: a dict values valid for 'mail.tracking.value' creation; + """ tracked = True - field = self.env['ir.model.fields']._get(model_name, col_name) + field = self.env['ir.model.fields']._get(record._name, col_name) if not field: return @@ -57,9 +73,11 @@ class MailTracking(models.Model): if col_info['type'] in ['integer', 'float', 'char', 'text', 'datetime', 'monetary']: values.update({ - 'old_value_%s' % col_info['type']: initial_value, - 'new_value_%s' % col_info['type']: new_value + f'old_value_{col_info["type"]}': initial_value, + f'new_value_{col_info["type"]}': new_value }) + if col_info['type'] == 'monetary': + values['currency_id'] = record[col_info['currency_field']].id elif col_info['type'] == 'date': values.update({ 'old_value_datetime': initial_value and fields.Datetime.to_string(datetime.combine(fields.Date.from_string(initial_value), datetime.min.time())) or False, @@ -95,50 +113,66 @@ class MailTracking(models.Model): return {} def _tracking_value_format(self): - tracking_values = [{ - 'changedField': tracking.field_desc, - 'id': tracking.id, - 'newValue': { - 'currencyId': tracking.currency_id.id, - 'fieldType': tracking.field_type, - 'value': tracking._get_new_display_value()[0], - }, - 'oldValue': { - 'currencyId': tracking.currency_id.id, - 'fieldType': tracking.field_type, - 'value': tracking._get_old_display_value()[0], - }, - } for tracking in self] - return tracking_values + """ Return structure and formatted data structure to be used by chatter + to display tracking values. + + :return list: for each tracking value in self, their formatted display + values given as a dict; + """ + formatted = [] + for tracking in self: + formatted.append({ + 'changedField': tracking.field_desc, + 'id': tracking.id, + 'fieldName': tracking.field.name, + 'fieldType': tracking.field_type, + 'newValue': { + 'currencyId': tracking.currency_id.id, + 'value': tracking._format_display_value(new=True)[0], + }, + 'oldValue': { + 'currencyId': tracking.currency_id.id, + 'value': tracking._format_display_value(new=False)[0], + }, + }) + return formatted + + def _format_display_value(self, new=True): + """ Format value of 'mail.tracking.value', according to the field type. + + :param bool new: if True, display the 'new' value. Otherwise display + the 'old' one. + """ + field_mapping = { + 'boolean': ('old_value_integer', 'new_value_integer'), + 'date': ('old_value_datetime', 'new_value_datetime'), + 'datetime': ('old_value_datetime', 'new_value_datetime'), + 'char': ('old_value_char', 'new_value_char'), + 'float': ('old_value_float', 'new_value_float'), + 'integer': ('old_value_integer', 'new_value_integer'), + 'monetary': ('old_value_monetary', 'new_value_monetary'), + 'text': ('old_value_text', 'new_value_text'), + } - def _get_display_value(self, prefix): - assert prefix in ('new', 'old') result = [] for record in self: - if record.field_type in ['integer', 'float', 'char', 'text', 'monetary']: - result.append(record[f'{prefix}_value_{record.field_type}']) - elif record.field_type == 'datetime': - if record[f'{prefix}_value_datetime']: - new_datetime = record[f'{prefix}_value_datetime'] - result.append(f'{new_datetime}Z') + ftype = record.field_type + value_fname = field_mapping.get( + ftype, ('old_value_char', 'new_value_char') + )[bool(new)] + value = record[value_fname] + + if ftype in {'integer', 'float', 'char', 'text', 'monetary'}: + result.append(value) + elif ftype in {'date', 'datetime'}: + if not record[value_fname]: + result.append(value) + elif ftype == 'date': + result.append(fields.Date.to_string(value)) else: - result.append(record[f'{prefix}_value_datetime']) - elif record.field_type == 'date': - if record[f'{prefix}_value_datetime']: - new_date = record[f'{prefix}_value_datetime'] - result.append(fields.Date.to_string(new_date)) - else: - result.append(record[f'{prefix}_value_datetime']) - elif record.field_type == 'boolean': - result.append(bool(record[f'{prefix}_value_integer'])) + result.append(f'{value}Z') + elif ftype == 'boolean': + result.append(bool(value)) else: - result.append(record[f'{prefix}_value_char']) + result.append(value) return result - - def _get_old_display_value(self): - # grep : # old_value_integer | old_value_datetime | old_value_char - return self._get_display_value('old') - - def _get_new_display_value(self): - # grep : # new_value_integer | new_value_datetime | new_value_char - return self._get_display_value('new') diff --git a/addons/mail/models/models.py b/addons/mail/models/models.py index 3872b814057..1b38fcc2693 100644 --- a/addons/mail/models/models.py +++ b/addons/mail/models/models.py @@ -71,13 +71,14 @@ class BaseModel(models.AbstractModel): # GENERIC MAIL FEATURES # ------------------------------------------------------------ - def _mail_track(self, tracked_fields, initial): + def _mail_track(self, tracked_fields, initial_values): """ For a given record, fields to check (tuple column name, column info) and initial values, return a valid command to create tracking values. - :param tracked_fields: fields_get of updated fields on which tracking - is checked and performed; - :param initial: dict of initial values for each updated fields; + :param dict tracked_fields: fields_get of updated fields on which + tracking is checked and performed; + :param dict initial_values: dict of initial values for each updated + fields; :return: a tuple (changes, tracking_value_ids) where changes: set of updated column names; @@ -92,9 +93,9 @@ class BaseModel(models.AbstractModel): # generate tracked_values data structure: {'col_name': {col_info, new_value, old_value}} for col_name, col_info in tracked_fields.items(): - if col_name not in initial: + if col_name not in initial_values: continue - initial_value = initial[col_name] + initial_value = initial_values[col_name] new_value = self[col_name] if new_value != initial_value and (new_value or initial_value): # because browse null != False @@ -102,10 +103,13 @@ class BaseModel(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, self._name) + tracking = self.env['mail.tracking.value']._create_tracking_values( + initial_value, new_value, + col_name, col_info, + tracking_sequence, + self + ) if tracking: - if tracking['field_type'] == 'monetary': - tracking['currency_id'] = self[col_info['currency_field']].id tracking_value_ids.append([0, 0, tracking]) changes.add(col_name) diff --git a/addons/mail/static/src/core/web/message_patch.js b/addons/mail/static/src/core/web/message_patch.js index b7dfd959f26..9ed40b2c99b 100644 --- a/addons/mail/static/src/core/web/message_patch.js +++ b/addons/mail/static/src/core/web/message_patch.js @@ -54,14 +54,8 @@ patch(Message.prototype, { /** * @returns {string} */ - formatTracking(trackingValue) { - /** - * Maps tracked field type to a JS formatter. Tracking values are - * not always stored in the same field type as their origin type. - * Field types that are not listed here are not supported by - * tracking in Python. Also see `create_tracking_values` in Python. - */ - switch (trackingValue.fieldType) { + formatTracking(trackingType, trackingValue) { + switch (trackingType) { case "boolean": return trackingValue.value ? _t("Yes") : _t("No"); /** @@ -106,8 +100,8 @@ patch(Message.prototype, { /** * @returns {string} */ - formatTrackingOrNone(trackingValue) { - const formattedValue = this.formatTracking(trackingValue); + formatTrackingOrNone(trackingType, trackingValue) { + const formattedValue = this.formatTracking(trackingType, trackingValue); return formattedValue || _t("None"); }, }); diff --git a/addons/mail/static/src/core/web/message_patch.xml b/addons/mail/static/src/core/web/message_patch.xml index ab8ddbe9c10..69e68127938 100644 --- a/addons/mail/static/src/core/web/message_patch.xml +++ b/addons/mail/static/src/core/web/message_patch.xml @@ -13,9 +13,9 @@