From 8d2d41068d364f67c4da3eb68eb1f3c95e3c479c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20Lef=C3=A8vre?= Date: Thu, 14 Mar 2019 16:11:17 +0100 Subject: [PATCH 1/3] [IMP] mail: Delete tracking values when uninstalling module Purpose ======= mail.tracking.values should be deleted when the corresponding field is deleted (when the module which defined it is uninstalled). Firstly because we don't want useless data in db. Secondly because the groups associated with the field can no longer be checked if it was deleted. As we don't know to whom the value was restricted, the value should not be displayed anyway. Note: this case was fixed in saas-12.2 by b9e96b7 but the proper way to fix it is to delete the tracking values. Specification ============= Delete `mail.tracking.value` when the associated `ir.model.fields` is deleted. Two alternatives were considered: 1. Change the `field` field of `mail.tracking.value` from Char to a m2o to `ir.model.fields` which allows to use delete oncascade. This implies to modify existing code, but more importantly it adds database queries. 2. Override the unlink method of `ir.model.fields` to first unlink associated tracking values. The first method is probably cleaner but the second method was nonetheless chosen as we don't want to impact performance in the main tracking flow only to better support module uninstalls which happens rarely. Note: when a module is uninstalled, `ir.model.fields` are unlinked one by one (in a for loop). Tracking values are therefore also unlinked field by field. Batchifying field deletion would greatly reduce the amount of queries. --- addons/mail/models/__init__.py | 1 + addons/mail/models/ir_model.py | 28 ----------- addons/mail/models/ir_model_fields.py | 50 ++++++++++++++++++++ addons/mail/models/mail_tracking_value.py | 5 +- addons/test_mail/tests/test_message_track.py | 14 ++++-- 5 files changed, 62 insertions(+), 36 deletions(-) create mode 100644 addons/mail/models/ir_model_fields.py diff --git a/addons/mail/models/__init__.py b/addons/mail/models/__init__.py index d0e1d954e63..7df60f71277 100644 --- a/addons/mail/models/__init__.py +++ b/addons/mail/models/__init__.py @@ -27,4 +27,5 @@ from . import ir_attachment from . import ir_autovacuum from . import ir_http from . import ir_model +from . import ir_model_fields from . import ir_ui_view diff --git a/addons/mail/models/ir_model.py b/addons/mail/models/ir_model.py index 52efe400df9..8df1315ee4d 100644 --- a/addons/mail/models/ir_model.py +++ b/addons/mail/models/ir_model.py @@ -76,31 +76,3 @@ class IrModel(models.Model): parents = [parents] if isinstance(parents, str) else parents model_class._inherit = parents + ['mail.activity.mixin'] return model_class - - -class IrModelField(models.Model): - _inherit = 'ir.model.fields' - - tracking = fields.Integer( - string="Enable Ordered Tracking", - help="If set every modification done to this field is tracked in the chatter. Value is used to order tracking values.", - ) - - def _reflect_field_params(self, field): - """ Tracking value can be either a boolean enabling tracking mechanism - on field, either an integer giving the sequence. Default sequence is - set to 100. """ - vals = super(IrModelField, self)._reflect_field_params(field) - tracking = getattr(field, 'tracking', None) - if tracking is True: - tracking = 100 - elif tracking is False: - tracking = None - vals['tracking'] = tracking - return vals - - def _instanciate_attrs(self, field_data): - attrs = super(IrModelField, self)._instanciate_attrs(field_data) - if field_data.get('tracking'): - attrs['tracking'] = field_data['tracking'] - return attrs diff --git a/addons/mail/models/ir_model_fields.py b/addons/mail/models/ir_model_fields.py new file mode 100644 index 00000000000..c3a18a84aa4 --- /dev/null +++ b/addons/mail/models/ir_model_fields.py @@ -0,0 +1,50 @@ +# -*- coding: utf-8 -*- +# Part of Odoo. See LICENSE file for full copyright and licensing details. + +from odoo import fields, models + + +class IrModelField(models.Model): + _inherit = 'ir.model.fields' + + tracking = fields.Integer( + string="Enable Ordered Tracking", + help="If set every modification done to this field is tracked in the chatter. Value is used to order tracking values.", + ) + + def _reflect_field_params(self, field): + """ Tracking value can be either a boolean enabling tracking mechanism + on field, either an integer giving the sequence. Default sequence is + set to 100. """ + vals = super(IrModelField, self)._reflect_field_params(field) + tracking = getattr(field, 'tracking', None) + if tracking is True: + tracking = 100 + elif tracking is False: + tracking = None + vals['tracking'] = tracking + return vals + + def _instanciate_attrs(self, field_data): + attrs = super(IrModelField, self)._instanciate_attrs(field_data) + if 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.env.cr.execute(query, (tuple(self.ids),)) + return super(IrModelField, self).unlink() diff --git a/addons/mail/models/mail_tracking_value.py b/addons/mail/models/mail_tracking_value.py index 570757a6534..7e6ae21ae6c 100644 --- a/addons/mail/models/mail_tracking_value.py +++ b/addons/mail/models/mail_tracking_value.py @@ -3,7 +3,7 @@ from datetime import datetime -from odoo import api, fields, models, tools +from odoo import api, fields, models class MailTracking(models.Model): @@ -41,8 +41,7 @@ class MailTracking(models.Model): for tracking in self: model = self.env[tracking.mail_message_id.model] field = model._fields.get(tracking.field) - # field may not exist anymore if module has been uninstalled - tracking.groups = field.groups if field else 'base.group_system' + tracking.groups = field.groups @api.model def create_tracking_values(self, initial_value, new_value, col_name, col_info, tracking_sequence): diff --git a/addons/test_mail/tests/test_message_track.py b/addons/test_mail/tests/test_message_track.py index 7786da9f594..6de2b4373f0 100644 --- a/addons/test_mail/tests/test_message_track.py +++ b/addons/test_mail/tests/test_message_track.py @@ -168,8 +168,12 @@ class TestTracking(common.BaseFunctionalTest, common.MockEmails): self.assertFalse(msg_emp.get('tracking_values'), "should not have protected tracking values") self.assertTrue(msg_admin.get('tracking_values'), "should have protected tracking values") - def test_unknown_field(self): - self.record.sudo().write({'email_from': 'X'}) # create a tracking value - tracking = self.record.sudo().mapped('message_ids.tracking_value_ids')[0] - tracking.field = 'I do not exist' - self.assertEqual(tracking.groups, 'base.group_system') + def test_unlinked_field(self): + record_sudo = self.record.sudo() + record_sudo.write({'email_from': 'X'}) # create a tracking value + self.assertEqual(len(record_sudo.message_ids.tracking_value_ids), 1) + ir_model_field = self.env['ir.model.fields'].search([ + ('model', '=', 'mail.test.full'), + ('name', '=', 'email_from')]) + ir_model_field.with_context(_force_unlink=True).unlink() + self.assertEqual(len(record_sudo.message_ids.tracking_value_ids), 0) From fbf9b55c5b1af68999fccd5dd168e5eb67af4880 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20Lef=C3=A8vre?= Date: Fri, 15 Mar 2019 09:33:19 +0100 Subject: [PATCH 2/3] [IMP] mail: Rename groups field of mail.tracking.value This commit renames the `groups` field into `field_groups`. The previous name was impossible to grep as the word `groups` is everywhere in the code base. Also the new name better follows the implicit convention that fields related to the tracked field all start with `field_`. --- addons/mail/models/mail_message.py | 2 +- addons/mail/models/mail_tracking_value.py | 7 +++---- addons/mail/models/res_partner.py | 2 +- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/addons/mail/models/mail_message.py b/addons/mail/models/mail_message.py index e2ebb16c69e..ba3108d61f6 100644 --- a/addons/mail/models/mail_message.py +++ b/addons/mail/models/mail_message.py @@ -350,7 +350,7 @@ class Message(models.Model): message_to_tracking = dict() tracking_tree = dict.fromkeys(tracking_values.ids, False) for tracking in tracking_values: - groups = tracking.groups + groups = tracking.field_groups if not groups or self.user_has_groups(groups): message_to_tracking.setdefault(tracking.mail_message_id.id, list()).append(tracking.id) tracking_tree[tracking.id] = { diff --git a/addons/mail/models/mail_tracking_value.py b/addons/mail/models/mail_tracking_value.py index 7e6ae21ae6c..e99e2fec946 100644 --- a/addons/mail/models/mail_tracking_value.py +++ b/addons/mail/models/mail_tracking_value.py @@ -16,6 +16,7 @@ class MailTracking(models.Model): field = fields.Char('Changed Field', required=True, readonly=1) field_desc = fields.Char('Field Description', required=True, readonly=1) field_type = fields.Char('Field Type') + field_groups = fields.Char(compute='_compute_field_groups') old_value_integer = fields.Integer('Old Value Integer', readonly=1) old_value_float = fields.Float('Old Value Float', readonly=1) @@ -35,13 +36,11 @@ class MailTracking(models.Model): tracking_sequence = fields.Integer('Tracking field sequence', readonly=1, default=100, oldname='track_sequence') - groups = fields.Char(compute='_compute_groups') - - def _compute_groups(self): + def _compute_field_groups(self): for tracking in self: model = self.env[tracking.mail_message_id.model] field = model._fields.get(tracking.field) - tracking.groups = field.groups + tracking.field_groups = field.groups @api.model def create_tracking_values(self, initial_value, new_value, col_name, col_info, tracking_sequence): diff --git a/addons/mail/models/res_partner.py b/addons/mail/models/res_partner.py index d073195aa2b..beee5608d92 100644 --- a/addons/mail/models/res_partner.py +++ b/addons/mail/models/res_partner.py @@ -69,7 +69,7 @@ class Partner(models.Model): tracking = [] for tracking_value in self.env['mail.tracking.value'].sudo().search([('mail_message_id', '=', message.id)]): - groups = tracking_value.groups + groups = tracking_value.field_groups if not groups or self.user_has_groups(groups): tracking.append((tracking_value.field_desc, tracking_value.get_old_display_value()[0], From ad8e969323ef504f6f4a745ceeac6d0d2b19a91d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20Lef=C3=A8vre?= Date: Mon, 8 Apr 2019 17:24:01 +0200 Subject: [PATCH 3/3] [FIX] mail: Don't display message if only subtype Currently, if a message does not have any tracking value, any attachment and no body BUT has a subtype with a description, the message is not considered as empty. The message subtype should not influence its "emptyness". --- addons/mail/static/src/js/models/messages/abstract_message.js | 1 - 1 file changed, 1 deletion(-) diff --git a/addons/mail/static/src/js/models/messages/abstract_message.js b/addons/mail/static/src/js/models/messages/abstract_message.js index 111ebaa760e..6a7b02d6e94 100644 --- a/addons/mail/static/src/js/models/messages/abstract_message.js +++ b/addons/mail/static/src/js/models/messages/abstract_message.js @@ -255,7 +255,6 @@ var AbstractMessage = Class.extend({ */ isEmpty: function () { return !this.hasTrackingValues() && - !this.hasSubtypeDescription() && !this.hasAttachments() && !this.getBody(); },