From 3cc931ef582fdb762f8322f3d09edfc4c5138782 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 15 Jun 2022 07:54:53 +0000 Subject: [PATCH 01/10] [IMP] mail: add helper to find customer partner on thread Purpose is to use it notably in Voip where we link activities / phonecalls to partners when possible. Future commits will use it more often in thread methods. Method to have the primary email is moved with that helper method in a sub section, in order to have "model" helpers in the same section. Its code is also simplified and fixed (return missing). Its name is made coherent with other naming in the file. Task-2884301 (Voip activity synchronization simplification) Part-of: odoo/odoo#93682 --- addons/mail/models/mail_thread.py | 52 ++++++++++++++++++++++--------- addons/mail/models/res_partner.py | 3 ++ 2 files changed, 40 insertions(+), 15 deletions(-) diff --git a/addons/mail/models/mail_thread.py b/addons/mail/models/mail_thread.py index 7df8b71d74e..efdeae20be8 100644 --- a/addons/mail/models/mail_thread.py +++ b/addons/mail/models/mail_thread.py @@ -450,6 +450,39 @@ class MailThread(models.AbstractModel): if any(message.message_type != 'comment' for message in messages): raise exceptions.UserError(_("Only messages type comment can have their content updated")) + # ------------------------------------------------------------ + # FIELDS HELPERS + # ------------------------------------------------------------ + + @api.model + def _mail_get_partner_fields(self): + """ This method returns the fields to use to find the contact to link + when sending emails or notifications. Having partner is not always + necessary but gives more flexibility to notifications management. """ + return [fname for fname in ('partner_id', 'partner_ids') + if fname in self] + + def _mail_get_partners(self): + """ Give the default partners associated to customers. + + :return dict: for each record ID, a res.partner recordsets being default + customers to contact; + """ + partner_fields = self._mail_get_partner_fields() + return dict( + (record.id, self.env['res.partner'].union(*[self[fname] for fname in partner_fields])) + for record in self + ) + + @api.model + def _mail_get_primary_email_field(self): + """ Check if the "_primary_email" model attribute is correctly set and + matches an existing field, and return it. Otherwise return None. """ + primary_email = getattr(self, '_primary_email', None) + if primary_email and primary_email in self._fields: + return primary_email + return None + # ------------------------------------------------------ # TRACKING / LOG # ------------------------------------------------------ @@ -816,24 +849,13 @@ class MailThread(models.AbstractModel): for model in [bl_model for bl_model in bl_models if bl_model.model in self.env]: # transient test mode self.env[model.model].sudo().search([('message_bounce', '>', 0), ('email_normalized', '=', valid_email)])._message_reset_bounce(valid_email) - @api.model - def _get_primary_email_field(self): - """Check if the "_primary_email" model attribute is correctly set. - - If it is correctly set, return the _primary_email field name - Otherwise return None - """ - primary_email = getattr(self, '_primary_email', None) - if primary_email and primary_email in self._fields: - return primary_email - @api.model def _detect_loop_sender_domain(self, email_from_normalized): """Return the domain to be used to detect duplicated records created by alias. :param email_from_normalized: FROM of the incoming email, normalized """ - primary_email = self._get_primary_email_field() + primary_email = self._mail_get_primary_email_field() if primary_email: return [(primary_email, 'ilike', email_from_normalized)] @@ -1233,12 +1255,12 @@ class MailThread(models.AbstractModel): data = {} if isinstance(custom_values, dict): data = custom_values.copy() - fields = self.fields_get() + model_fields = self.fields_get() name_field = self._rec_name or 'name' - if name_field in fields and not data.get('name'): + if name_field in model_fields and not data.get('name'): data[name_field] = msg_dict.get('subject', '') - primary_email = self._get_primary_email_field() + primary_email = self._mail_get_primary_email_field() if primary_email and msg_dict.get('email_from'): data[primary_email] = msg_dict['email_from'] diff --git a/addons/mail/models/res_partner.py b/addons/mail/models/res_partner.py index c01f17a1d47..97a103df155 100644 --- a/addons/mail/models/res_partner.py +++ b/addons/mail/models/res_partner.py @@ -55,6 +55,9 @@ class Partner(models.Model): # MESSAGING # ------------------------------------------------------------ + def _mail_get_partners(self): + return dict((partner.id, partner) for partner in self) + def _message_get_suggested_recipients(self): recipients = super(Partner, self)._message_get_suggested_recipients() for partner in self: From 6bea3c7605cf248f71a46d20bf2503cda3430738 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Fri, 19 Aug 2022 12:02:31 +0000 Subject: [PATCH 02/10] [IMP] mail: return generated records in mail composer We now return generated records when calling ``_action_send_mail`` on mail composer. This allows to have access to generated emails (in mass mode) or posted messages (in comment mode). Otherwise we have to access sub-fields like activity_ids / message_ids, which may be inefficient or generate extra ACLs checks. Also replace some |= with += when usage of OrderedSet is not necessary. To keep ordering, ordered sets are used when doing an or between record sets. However in some cases we know we are simply adding records not already in a previous record set, meaning we can just use +. Task-2883589 (Activity performance and cleaning) Part-of: odoo/odoo#93682 --- addons/mail/models/mail_activity.py | 2 +- addons/mail/models/mail_activity_mixin.py | 2 +- addons/mail/models/mail_message.py | 2 +- addons/mail/models/mail_thread.py | 4 ++-- addons/mail/wizard/mail_compose_message.py | 16 +++++++++++++--- 5 files changed, 18 insertions(+), 8 deletions(-) diff --git a/addons/mail/models/mail_activity.py b/addons/mail/models/mail_activity.py index 63a86c7bbdb..5e5741cf6e6 100644 --- a/addons/mail/models/mail_activity.py +++ b/addons/mail/models/mail_activity.py @@ -557,7 +557,7 @@ class MailActivity(models.Model): 'res_model': activity_message._name, }) activity_message.attachment_ids = message_attachments - messages |= activity_message + messages += activity_message next_activities = self.env['mail.activity'].create(next_activities_values) self.unlink() # will unlink activity, dont access `self` after that diff --git a/addons/mail/models/mail_activity_mixin.py b/addons/mail/models/mail_activity_mixin.py index 9528886ace5..d29c482b7fc 100644 --- a/addons/mail/models/mail_activity_mixin.py +++ b/addons/mail/models/mail_activity_mixin.py @@ -443,7 +443,7 @@ class MailActivityMixin(models.AbstractModel): for record in self: render_context['object'] = record note = self.env['ir.qweb']._render(view_ref, render_context, minimal_qcontext=True, raise_if_not_found=False) - activities |= record.activity_schedule(act_type_xmlid=act_type_xmlid, date_deadline=date_deadline, summary=summary, note=note, **act_values) + activities += record.activity_schedule(act_type_xmlid=act_type_xmlid, date_deadline=date_deadline, summary=summary, note=note, **act_values) return activities def activity_reschedule(self, act_type_xmlids, user_id=None, date_deadline=None, new_user_id=None): diff --git a/addons/mail/models/mail_message.py b/addons/mail/models/mail_message.py index a8bb8e81e6c..617b22308d8 100644 --- a/addons/mail/models/mail_message.py +++ b/addons/mail/models/mail_message.py @@ -997,7 +997,7 @@ class Message(models.Model): except AccessError: continue else: - messages |= message + messages += message messages_per_partner = defaultdict(lambda: self.env['mail.message']) for message in messages: if not self.env.user._is_public(): diff --git a/addons/mail/models/mail_thread.py b/addons/mail/models/mail_thread.py index efdeae20be8..43984c8b2df 100644 --- a/addons/mail/models/mail_thread.py +++ b/addons/mail/models/mail_thread.py @@ -2045,7 +2045,7 @@ class MailThread(models.AbstractModel): def message_post_with_view(self, views_or_xmlid, **kwargs): """ Helper method to send a mail / post a message using a view_id """ - self._message_compose_with_view(views_or_xmlid, **kwargs) + return self._message_compose_with_view(views_or_xmlid, **kwargs) def message_post_with_template(self, template_id, email_layout_xmlid=None, auto_commit=False, **kwargs): """ Helper method to send a mail with a template @@ -2518,7 +2518,7 @@ class MailThread(models.AbstractModel): 'is_read': True, # discard Inbox notification 'notification_status': 'ready', } for recipient_id in tocreate_recipient_ids] - emails |= new_email + emails += new_email if notif_create_values: SafeNotification.create(notif_create_values) diff --git a/addons/mail/wizard/mail_compose_message.py b/addons/mail/wizard/mail_compose_message.py index 4229b0f0c0c..2be52e4ac1d 100644 --- a/addons/mail/wizard/mail_compose_message.py +++ b/addons/mail/wizard/mail_compose_message.py @@ -240,7 +240,13 @@ class MailComposer(models.TransientModel): def _action_send_mail(self, auto_commit=False): """ Process the wizard content and proceed with sending the related - email(s), rendering any template patterns on the fly if needed. """ + email(s), rendering any template patterns on the fly if needed. + + :return tuple: ( + result_mails_su: in mass mode, sent emails (as sudo), + result_messages: in comment mode, posted messages + ) + """ # Several custom layouts make use of the model description at rendering, e.g. in the # 'View ' button. Some models are used for different business concepts, such as # 'purchase.order' which is used for a RFQ and and PO. To avoid confusion, we must use a @@ -248,6 +254,7 @@ class MailComposer(models.TransientModel): # Therefore, we can set the description in the context from the beginning to avoid falling # back on the regular display_name retrieved in ``_notify_by_email_prepare_rendering_context()``. model_description = self._context.get('model_description') + result_mails_su, result_messages = self.env['mail.mail'].sudo(), self.env['mail.message'] for wizard in self: # Duplicate attachments linked to the email.template. @@ -299,7 +306,7 @@ class MailComposer(models.TransientModel): all_mail_values = wizard.get_mail_values(res_ids) for res_id, mail_values in all_mail_values.items(): if wizard.composition_mode == 'mass_mail': - batch_mails_sudo |= self.env['mail.mail'].sudo().create(mail_values) + batch_mails_sudo += self.env['mail.mail'].sudo().create(mail_values) else: post_params = dict( message_type=wizard.message_type, @@ -317,11 +324,14 @@ class MailComposer(models.TransientModel): # if message_notify returns an empty record set, no recipients where found. raise UserError(_("No recipient found.")) else: - ActiveModel.browse(res_id).message_post(**post_params) + result_messages += ActiveModel.browse(res_id).message_post(**post_params) + result_mails_su += batch_mails_sudo if wizard.composition_mode == 'mass_mail': batch_mails_sudo.send(auto_commit=auto_commit) + return result_mails_su, result_messages + def action_save_as_template(self): """ hit save as template button: current form value will be a new template attached to the current document. """ From 3aa938cc056e02133b1748fca5295306f9cbfcd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 15 Jun 2022 11:28:10 +0000 Subject: [PATCH 03/10] [IMP] mail: add helpers to order/group activities Purpose is to add helpers on activity model, notably to ease batching based on models. As activities are linked to records using a res_model / res_id pair most code is not optimized (checking records one by one). Purpose of this tool is to ease doing computation by batch of models. Task-2883589 (Activity performance and cleaning) Part-of: odoo/odoo#93682 --- addons/mail/models/mail_activity.py | 22 ++++++++++++++++++++++ addons/mail/models/mail_thread.py | 2 +- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/addons/mail/models/mail_activity.py b/addons/mail/models/mail_activity.py index 5e5741cf6e6..4d2795cbe75 100644 --- a/addons/mail/models/mail_activity.py +++ b/addons/mail/models/mail_activity.py @@ -635,6 +635,28 @@ class MailActivity(models.Model): # TOOLS # ---------------------------------------------------------------------- + def _classify_by_model(self): + """ To ease batch computation of various activities related methods they + are classified by model. Activities not linked to a valid record through + res_model / res_id are ignored. + + :return dict: for each model having at least one activity in self, have + a sub-dict containing + * activities: activities related to that model; + * record IDs: record linked to the activities of that model, in same + order; + """ + data_by_model = {} + for activity in self.filtered(lambda act: act.res_model and act.res_id): + if activity.res_model not in data_by_model: + data_by_model[activity.res_model] = { + 'activities': self.env['mail.activity'], + 'record_ids': [], + } + data_by_model[activity.res_model]['activities'] += activity + data_by_model[activity.res_model]['record_ids'].append(activity.res_id) + return data_by_model + def _prepare_next_activity_values(self): """ Prepare the next activity values based on the current activity record and applies _onchange methods :returns a dict of values for the new activity diff --git a/addons/mail/models/mail_thread.py b/addons/mail/models/mail_thread.py index 43984c8b2df..5e21146141d 100644 --- a/addons/mail/models/mail_thread.py +++ b/addons/mail/models/mail_thread.py @@ -470,7 +470,7 @@ class MailThread(models.AbstractModel): """ partner_fields = self._mail_get_partner_fields() return dict( - (record.id, self.env['res.partner'].union(*[self[fname] for fname in partner_fields])) + (record.id, self.env['res.partner'].union(*[record[fname] for fname in partner_fields])) for record in self ) From 0a380fec7789b346cd0a1a338a57318e248f495f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Tue, 14 Jun 2022 13:37:33 +0000 Subject: [PATCH 04/10] [PRF] mail: create activities in batch in activity mixin Improve performance by batching activities creation through the mixin. This helps saving some queries. Task-2883589 (Activity performance and cleaning) Part-of: odoo/odoo#93682 --- addons/mail/models/mail_activity_mixin.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/addons/mail/models/mail_activity_mixin.py b/addons/mail/models/mail_activity_mixin.py index d29c482b7fc..e8867d536f8 100644 --- a/addons/mail/models/mail_activity_mixin.py +++ b/addons/mail/models/mail_activity_mixin.py @@ -407,7 +407,7 @@ class MailActivityMixin(models.AbstractModel): activity_type = activity_type_id and self.env['mail.activity.type'].sudo().browse(activity_type_id) model_id = self.env['ir.model']._get(self._name).id - activities = self.env['mail.activity'] + create_vals_list = [] for record in self: create_vals = { 'activity_type_id': activity_type and activity_type.id, @@ -421,8 +421,8 @@ class MailActivityMixin(models.AbstractModel): create_vals.update(act_values) if not create_vals.get('user_id'): create_vals['user_id'] = activity_type.default_user_id.id or self.env.uid - activities |= self.env['mail.activity'].create(create_vals) - return activities + create_vals_list.append(create_vals) + return self.env['mail.activity'].create(create_vals_list) def _activity_schedule_with_view(self, act_type_xmlid='', date_deadline=None, summary='', views_or_xmlid='', render_context=None, **act_values): """ Helper method: Schedule an activity on each record of the current record set. From 9aa934e68350a4c5961f62a0ead571207e06c438 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 15 Jun 2022 11:45:21 +0000 Subject: [PATCH 05/10] [PRF] mail: send activity bus notifications in batch No need to send bus notifications one by one, we may use sendmany. Various activities crud methods send notifications, better group them when possible. Task-2883589 (Activity performance and cleaning) Part-of: odoo/odoo#93682 --- addons/mail/models/mail_activity.py | 37 ++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/addons/mail/models/mail_activity.py b/addons/mail/models/mail_activity.py index 4d2795cbe75..3157055c3f5 100644 --- a/addons/mail/models/mail_activity.py +++ b/addons/mail/models/mail_activity.py @@ -269,6 +269,7 @@ class MailActivity(models.Model): @api.model_create_multi def create(self, vals_list): activities = super(MailActivity, self).create(vals_list) + for activity in activities: need_sudo = False try: # in multicompany, reading the partner might break @@ -290,8 +291,13 @@ class MailActivity(models.Model): activity.action_notify() self.env[activity.res_model].browse(activity.res_id).message_subscribe(partner_ids=[partner_id]) - if activity.date_deadline <= fields.Date.today(): - self.env['bus.bus']._sendone(activity.user_id.partner_id, 'mail.activity/updated', {'activity_created': True}) + + todo_activities = activities.filtered(lambda act: act.date_deadline <= fields.Date.today()) + if todo_activities: + self.env['bus.bus']._sendmany([ + (activity.user_id.partner_id, 'mail.activity/updated', {'activity_created': True}) + for activity in todo_activities + ]) return activities def read(self, fields=None, load='_classic_read'): @@ -319,18 +325,27 @@ class MailActivity(models.Model): user_changes.action_notify() for activity in user_changes: self.env[activity.res_model].browse(activity.res_id).message_subscribe(partner_ids=[activity.user_id.partner_id.id]) - if activity.date_deadline <= fields.Date.today(): - self.env['bus.bus']._sendone(activity.user_id.partner_id, 'mail.activity/updated', {'activity_created': True}) - for activity in user_changes: - if activity.date_deadline <= fields.Date.today(): - for partner in pre_responsibles: - self.env['bus.bus']._sendone(partner, 'mail.activity/updated', {'activity_deleted': True}) + + # send bus notifications + todo_activities = user_changes.filtered(lambda act: act.date_deadline <= fields.Date.today()) + if todo_activities: + self.env['bus.bus']._sendmany([ + [partner, 'mail.activity/updated', {'activity_created': True}] + for partner in todo_activities.user_id.partner_id + ]) + self.env['bus.bus']._sendmany([ + [partner, 'mail.activity/updated', {'activity_deleted': True}] + for partner in pre_responsibles + ]) return res def unlink(self): - for activity in self: - if activity.date_deadline <= fields.Date.today(): - self.env['bus.bus']._sendone(activity.user_id.partner_id, 'mail.activity/updated', {'activity_deleted': True}) + todo_activities = self.filtered(lambda act: act.date_deadline <= fields.Date.today()) + if todo_activities: + self.env['bus.bus']._sendmany([ + [partner, 'mail.activity/updated', {'activity_deleted': True}] + for partner in todo_activities.user_id.partner_id + ]) return super(MailActivity, self).unlink() @api.model From 115f5663f159f0bac71174667c812b5b65a6c669 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 15 Jun 2022 11:48:03 +0000 Subject: [PATCH 06/10] [PRF] mail, note: improve default activity type computation and remove useless ref Change some ``ref`` to low level method ``_xmlid_to_res_id``. The latter one does not call exists, which allows to gain one query that we do not need here. Default methods on activity / activity mixin to find the default activity type (todo, model-based, generic) are cleaned. Notably code was duplicated between mixin and activity, it now always call the same code. Remove an unnecessary ref, probably a leftover of new rendering methods on Qweb. Remove an unnecessary sudo. Task-2883589 (Activity performance and cleaning) Part-of: odoo/odoo#93682 --- addons/mail/models/mail_activity.py | 25 +++++++++++++---------- addons/mail/models/mail_activity_mixin.py | 14 +++++++------ addons/note/controllers/note.py | 2 +- 3 files changed, 23 insertions(+), 18 deletions(-) diff --git a/addons/mail/models/mail_activity.py b/addons/mail/models/mail_activity.py index 3157055c3f5..e9c86c05d08 100644 --- a/addons/mail/models/mail_activity.py +++ b/addons/mail/models/mail_activity.py @@ -31,21 +31,25 @@ class MailActivity(models.Model): return res @api.model - def _default_activity_type_id(self): - ActivityType = self.env["mail.activity.type"] - activity_type_todo = self.env.ref('mail.mail_activity_data_todo', raise_if_not_found=False) + def _default_activity_type(self): default_vals = self.default_get(['res_model_id', 'res_model']) if not default_vals.get('res_model_id'): - return ActivityType - current_model_id = default_vals['res_model_id'] - current_model = self.env["ir.model"].sudo().browse(current_model_id) + return False + + current_model = self.env["ir.model"].sudo().browse(default_vals['res_model_id']).model + return self._default_activity_type_for_model(current_model) + + @api.model + def _default_activity_type_for_model(self, model): + todo_id = self.env['ir.model.data']._xmlid_to_res_id('mail.mail_activity_data_todo', raise_if_not_found=False) + activity_type_todo = self.env['mail.activity.type'].browse(todo_id) if todo_id else self.env['mail.activity.type'] if activity_type_todo and activity_type_todo.active and \ - (activity_type_todo.res_model == current_model.model or not activity_type_todo.res_model): + (activity_type_todo.res_model == model or not activity_type_todo.res_model): return activity_type_todo - activity_type_model = ActivityType.search([('res_model', '=', current_model.model)], limit=1) + activity_type_model = self.env['mail.activity.type'].search([('res_model', '=', model)], limit=1) if activity_type_model: return activity_type_model - activity_type_generic = ActivityType.search([('res_model', '=', False)], limit=1) + activity_type_generic = self.env['mail.activity.type'].search([('res_model', '=', False)], limit=1) return activity_type_generic # owner @@ -63,7 +67,7 @@ class MailActivity(models.Model): activity_type_id = fields.Many2one( 'mail.activity.type', string='Activity Type', domain="['|', ('res_model', '=', False), ('res_model', '=', res_model)]", ondelete='restrict', - default=_default_activity_type_id) + default=_default_activity_type) activity_category = fields.Selection(related='activity_type_id.category', readonly=True) activity_decoration = fields.Selection(related='activity_type_id.decoration_type', readonly=True) icon = fields.Char('Icon', related='activity_type_id.icon', readonly=True) @@ -450,7 +454,6 @@ class MailActivity(models.Model): def action_notify(self): if not self: return - body_template = self.env.ref('mail.message_activity_assigned') for activity in self: if activity.user_id.lang: # Send the notification in the assigned user's language diff --git a/addons/mail/models/mail_activity_mixin.py b/addons/mail/models/mail_activity_mixin.py index e8867d536f8..3c0542b110a 100644 --- a/addons/mail/models/mail_activity_mixin.py +++ b/addons/mail/models/mail_activity_mixin.py @@ -45,9 +45,7 @@ class MailActivityMixin(models.AbstractModel): Can be overriden to specify the default activity type of a model. It is only called in in activity_schedule() for now. """ - return self.env.ref('mail.mail_activity_data_todo', raise_if_not_found=False) \ - or self.env['mail.activity.type'].search([('res_model', '=', self._name)], limit=1) \ - or self.env['mail.activity.type'].search([('res_model', '=', False)], limit=1) + return self.env['mail.activity']._default_activity_type_for_model(self._name) activity_ids = fields.One2many( 'mail.activity', 'res_id', 'Activities', @@ -401,16 +399,20 @@ class MailActivityMixin(models.AbstractModel): if isinstance(date_deadline, datetime): _logger.warning("Scheduled deadline should be a date (got %s)", date_deadline) if act_type_xmlid: - activity_type = self.env.ref(act_type_xmlid, raise_if_not_found=False) or self._default_activity_type() + activity_type_id = self.env['ir.model.data']._xmlid_to_res_id(act_type_xmlid, raise_if_not_found=False) + if activity_type_id: + activity_type = self.env['mail.activity.type'].browse(activity_type_id) + else: + activity_type = self._default_activity_type() else: activity_type_id = act_values.get('activity_type_id', False) - activity_type = activity_type_id and self.env['mail.activity.type'].sudo().browse(activity_type_id) + activity_type = self.env['mail.activity.type'].browse(activity_type_id) if activity_type_id else self.env['mail.activity.type'] model_id = self.env['ir.model']._get(self._name).id create_vals_list = [] for record in self: create_vals = { - 'activity_type_id': activity_type and activity_type.id, + 'activity_type_id': activity_type.id, 'summary': summary or activity_type.summary, 'automated': True, 'note': note or activity_type.default_note, diff --git a/addons/note/controllers/note.py b/addons/note/controllers/note.py index 6cafea730cf..24db9d49e96 100644 --- a/addons/note/controllers/note.py +++ b/addons/note/controllers/note.py @@ -13,7 +13,7 @@ class NoteController(http.Controller): note = request.env['note.note'].create({'memo': note}) if date_deadline: note.activity_schedule( - activity_type_id=activity_type_id or request.env['mail.activity.type'].sudo().search([('category', '=', 'reminder')], limit=1).id, + activity_type_id=activity_type_id or request.env['mail.activity.type'].search([('category', '=', 'reminder')], limit=1).id, note=note.memo, date_deadline=date_deadline ) From 17b8d7062c235b37601e6bb9afbcb44cb47222f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 15 Jun 2022 12:46:47 +0000 Subject: [PATCH 07/10] [PRF] mail: improve batch creation of activities Purpose of this commit is to improve batch creation of activities by trying to group checks by user and/or model when possible. Notably * subscribe partners in batch when several activities are assigned to the same user (use case: batch assign on leads, users may be assigned on several records in a single udpate); * check in batch access on partners; Task-2883589 (Activity performance and cleaning) Part-of: odoo/odoo#93682 --- addons/mail/models/mail_activity.py | 51 ++++++++++++++++++----------- 1 file changed, 32 insertions(+), 19 deletions(-) diff --git a/addons/mail/models/mail_activity.py b/addons/mail/models/mail_activity.py index e9c86c05d08..d0488b30088 100644 --- a/addons/mail/models/mail_activity.py +++ b/addons/mail/models/mail_activity.py @@ -274,28 +274,41 @@ class MailActivity(models.Model): def create(self, vals_list): activities = super(MailActivity, self).create(vals_list) - for activity in activities: - need_sudo = False - try: # in multicompany, reading the partner might break - partner_id = activity.user_id.partner_id.id - except exceptions.AccessError: - need_sudo = True - partner_id = activity.user_id.sudo().partner_id.id + # find partners related to responsible users, separate readable from unreadable + if any(user != self.env.user for user in activities.user_id): + user_partners = activities.user_id.partner_id + readable_user_partners = user_partners._filter_access_rules_python('read') + else: + readable_user_partners = self.env.user.partner_id - # send a notification to assigned user; in case of manually done activity also check - # target has rights on document otherwise we prevent its creation. Automated activities - # are checked since they are integrated into business flows that should not crash. - if activity.user_id != self.env.user: - if not activity.automated: - activity._check_access_assignation() - if not self.env.context.get('mail_activity_quick_update', False): - if need_sudo: - activity.sudo().action_notify() - else: - activity.action_notify() + # when creating activities for other: send a notification to assigned user; + # in case of manually done activity also check target has rights on document + # otherwise we prevent its creation. Automated activities are checked since + # they are integrated into business flows that should not crash. + if self.env.context.get('mail_activity_quick_update'): + activities_to_notify = self.env['mail.activity'] + else: + activities_to_notify = activities.filtered(lambda act: act.user_id != self.env.user) + activities_to_notify.filtered(lambda act: not act.automated)._check_access_assignation() + if activities_to_notify: + to_sudo = activities_to_notify.filtered(lambda act: act.user_id.partner_id not in readable_user_partners) + other = activities_to_notify - to_sudo + to_sudo.sudo().action_notify() + other.action_notify() - self.env[activity.res_model].browse(activity.res_id).message_subscribe(partner_ids=[partner_id]) + # subscribe (batch by model and user to speedup) + for model, activity_data in activities._classify_by_model().items(): + per_user = dict() + for activity in activity_data['activities'].filtered(lambda act: act.user_id): + if activity.user_id not in per_user: + per_user[activity.user_id] = [activity.res_id] + else: + per_user[activity.user_id].append(activity.res_id) + for user, res_ids in per_user.items(): + pids = user.partner_id.ids if user.partner_id in readable_user_partners else user.sudo().partner_id.ids + self.env[model].browse(res_ids).message_subscribe(partner_ids=pids) + # send notifications about activity creation todo_activities = activities.filtered(lambda act: act.date_deadline <= fields.Date.today()) if todo_activities: self.env['bus.bus']._sendmany([ From 33d7ea6fb8198f741cc13765b2f60c820f54ae66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Fri, 19 Aug 2022 12:34:20 +0000 Subject: [PATCH 08/10] [PRF] mail: improve performance of closing activities MailActivity model has an ``_action_done`` method that is quite heavily used and overridden. It notably posts messages, create next activities but also has some specific behavior depending on category. Purpose of this commit is to improve code of this method to avoid useless queries, improve performance, and rethink the computation with batch in mind. Task-2883589 (Activity performance and cleaning) Part-of: odoo/odoo#93682 --- addons/calendar/models/mail_activity.py | 6 +-- addons/mail/models/mail_activity.py | 66 +++++++++++++------------ 2 files changed, 37 insertions(+), 35 deletions(-) diff --git a/addons/calendar/models/mail_activity.py b/addons/calendar/models/mail_activity.py index 4ceed29cc11..cd921cda669 100644 --- a/addons/calendar/models/mail_activity.py +++ b/addons/calendar/models/mail_activity.py @@ -24,17 +24,15 @@ class MailActivity(models.Model): return action def _action_done(self, feedback=False, attachment_ids=False): - events = self.mapped('calendar_event_id') - messages, activities = super(MailActivity, self)._action_done(feedback=feedback, attachment_ids=attachment_ids) if feedback: - for event in events: + for event in self.calendar_event_id: description = event.description description = '%s
%s' % ( description if not tools.is_html_empty(description) else '', _('Feedback: %(feedback)s', feedback=tools.plaintext2html(feedback)) if feedback else '', ) event.write({'description': description}) - return messages, activities + return super(MailActivity, self)._action_done(feedback=feedback, attachment_ids=attachment_ids) def unlink_w_meeting(self): events = self.mapped('calendar_event_id') diff --git a/addons/mail/models/mail_activity.py b/addons/mail/models/mail_activity.py index d0488b30088..f5904c772a3 100644 --- a/addons/mail/models/mail_activity.py +++ b/addons/mail/models/mail_activity.py @@ -557,40 +557,44 @@ class MailActivity(models.Model): activity_id = attachment['res_id'] activity_attachments[activity_id].append(attachment['id']) - for activity in self: - # extract value to generate next activities - if activity.chaining_type == 'trigger': - vals = activity.with_context(activity_previous_deadline=activity.date_deadline)._prepare_next_activity_values() - next_activities_values.append(vals) + for model, activity_data in self._classify_by_model().items(): + records = self.env[model].browse(activity_data['record_ids']) + for record, activity in zip(records, activity_data['activities']): + # extract value to generate next activities + if activity.chaining_type == 'trigger': + vals = activity.with_context(activity_previous_deadline=activity.date_deadline)._prepare_next_activity_values() + next_activities_values.append(vals) - # post message on activity, before deleting it - record = self.env[activity.res_model].browse(activity.res_id) - record.message_post_with_view( - 'mail.message_activity_done', - values={ - 'activity': activity, - 'feedback': feedback, - 'display_assignee': activity.user_id != self.env.user - }, - subtype_id=self.env['ir.model.data']._xmlid_to_res_id('mail.mt_activities'), - mail_activity_type_id=activity.activity_type_id.id, - attachment_ids=[Command.link(attachment_id) for attachment_id in attachment_ids] if attachment_ids else [], - ) + # post message on activity, before deleting it + _, activity_message = record.message_post_with_view( + 'mail.message_activity_done', + values={ + 'activity': activity, + 'feedback': feedback, + 'display_assignee': activity.user_id != self.env.user + }, + subtype_id=self.env['ir.model.data']._xmlid_to_res_id('mail.mt_activities'), + mail_activity_type_id=activity.activity_type_id.id, + attachment_ids=[Command.link(attachment_id) for attachment_id in attachment_ids] if attachment_ids else [], + ) - # Moving the attachments in the message - # TODO: Fix void res_id on attachment when you create an activity with an image - # directly, see route /web_editor/attachment/add - activity_message = record.message_ids[0] - message_attachments = self.env['ir.attachment'].browse(activity_attachments[activity.id]) - if message_attachments: - message_attachments.write({ - 'res_id': activity_message.id, - 'res_model': activity_message._name, - }) - activity_message.attachment_ids = message_attachments - messages += activity_message + # Moving the attachments in the message + # TODO: Fix void res_id on attachment when you create an activity with an image + # directly, see route /web_editor/attachment/add + if activity_attachments[activity.id]: + message_attachments = self.env['ir.attachment'].browse(activity_attachments[activity.id]) + if message_attachments: + message_attachments.write({ + 'res_id': activity_message.id, + 'res_model': activity_message._name, + }) + activity_message.attachment_ids = message_attachments + messages += activity_message + + next_activities = self.env['mail.activity'] + if next_activities_values: + next_activities = self.env['mail.activity'].create(next_activities_values) - next_activities = self.env['mail.activity'].create(next_activities_values) self.unlink() # will unlink activity, dont access `self` after that return messages, next_activities From 846d66ae9d97e7040e9936367a3078c3ea7640f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Thu, 25 Aug 2022 15:37:55 +0000 Subject: [PATCH 09/10] [PRF] mail: improve check assignment by doing it in batch When assigning activities to people we check target user has the rights to access to document. Otherwise they end up with notifications and activities for records they can't see. This code is currently a loop doing browse and access checks one record at a time. We improve it by grouping activities by related record model and target user, allowing to batch some calls. Note that this somehow reverts odoo/odoo@c38a0d67680818dbb2e3334106e183aa307f5734 and odoo/odoo@a98c15f44358d6342632a8bd461fb476461f35ae as normally odoo/odoo@16351e48792bd35c6802646e5da38042709a785b should be sufficient to handle MC use cases. Lack of clear unit tests for those corner cases make it difficult to clearly reproduce issue anyway. Task-2883589 (Activity performance and cleaning) Part-of: odoo/odoo#93682 --- addons/mail/models/mail_activity.py | 39 ++++++++++++++++------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/addons/mail/models/mail_activity.py b/addons/mail/models/mail_activity.py index f5904c772a3..8be9c6abf71 100644 --- a/addons/mail/models/mail_activity.py +++ b/addons/mail/models/mail_activity.py @@ -244,27 +244,32 @@ class MailActivity(models.Model): is to allow assigned user to handle their activities. For that purpose assigned user should be able to at least read the document. We therefore raise an UserError if the assigned user has no access to the document. """ - for activity in self: - model = self.env[activity.res_model].with_user(activity.user_id).with_context(allowed_company_ids=activity.user_id.company_ids.ids) - try: - model.check_access_rights('read') - except exceptions.AccessError: - raise exceptions.UserError( - _('Assigned user %s has no access to the document and is not able to handle this activity.', - activity.user_id.display_name)) - else: + for model, activity_data in self._classify_by_model().items(): + # group activities / user, in order to batch the check of ACLs + per_user = dict() + for activity in activity_data['activities'].filtered(lambda act: act.user_id): + if activity.user_id not in per_user: + per_user[activity.user_id] = activity + else: + per_user[activity.user_id] += activity + for user, activities in per_user.items(): + RecordModel = self.env[model].with_user(user).with_context( + allowed_company_ids=user.company_ids.ids + ) try: - target_user = activity.user_id - target_record = self.env[activity.res_model].browse(activity.res_id) - if hasattr(target_record, 'company_id') and ( - target_record.company_id != target_user.company_id and ( - len(target_user.sudo().company_ids) > 1)): - return # in that case we skip the check, assuming it would fail because of the company - model.browse(activity.res_id).check_access_rule('read') + RecordModel.check_access_rights('read') except exceptions.AccessError: raise exceptions.UserError( _('Assigned user %s has no access to the document and is not able to handle this activity.', - activity.user_id.display_name)) + user.display_name)) + else: + try: + target_records = self.env[model].browse(activities.mapped('res_id')) + target_records.check_access_rule('read') + except exceptions.AccessError: + raise exceptions.UserError( + _('Assigned user %s has no access to the document and is not able to handle this activity.', + user.display_name)) # ------------------------------------------------------ # ORM overrides From 8f5943269f519592b7eea174ffda0ebaf6e02a2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Fri, 19 Aug 2022 12:34:54 +0000 Subject: [PATCH 10/10] [PRF] test_mail: update counters Update counters after all previous performance commits. It is done as a single update due to frequent conflicts and difficulty to keep updated counters for each commit separately. Task-2883589 (Activity performance and cleaning) Part-of: odoo/odoo#93682 --- addons/test_event_full/tests/test_performance.py | 4 ++-- addons/test_mail/tests/test_performance.py | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/addons/test_event_full/tests/test_performance.py b/addons/test_event_full/tests/test_performance.py index b1b100ed1e3..6ac46586bd5 100644 --- a/addons/test_event_full/tests/test_performance.py +++ b/addons/test_event_full/tests/test_performance.py @@ -301,7 +301,7 @@ class TestRegistrationPerformance(EventPerformanceCase): """ Test a single registration creation using Form """ event = self.env['event.event'].browse(self.test_event.ids) - with freeze_time(self.reference_now), self.assertQueryCount(event_user=202): # tef 185 / com 190 + with freeze_time(self.reference_now), self.assertQueryCount(event_user=202): # tef 185 / com 189 self.env.cr._now = self.reference_now # force create_date to check schedulers with Form(self.env['event.registration']) as reg_form: reg_form.event_id = event @@ -317,7 +317,7 @@ class TestRegistrationPerformance(EventPerformanceCase): """ Test a single registration creation using Form """ event = self.env['event.event'].browse(self.test_event.ids) - with freeze_time(self.reference_now), self.assertQueryCount(event_user=209): # tef 190 / com 195 + with freeze_time(self.reference_now), self.assertQueryCount(event_user=209): # tef 190 / com 194 self.env.cr._now = self.reference_now # force create_date to check schedulers with Form(self.env['event.registration']) as reg_form: reg_form.event_id = event diff --git a/addons/test_mail/tests/test_performance.py b/addons/test_mail/tests/test_performance.py index 8be8bc41d19..dd79765cef8 100644 --- a/addons/test_mail/tests/test_performance.py +++ b/addons/test_mail/tests/test_performance.py @@ -243,7 +243,7 @@ class TestMailAPIPerformance(BaseMailPerformance): # voip module read activity_type during create leading to one less query in enterprise on action_feedback _category = activity.activity_type_id.category - with self.assertQueryCount(__system__=17, employee=19): # com+tm 16/18 + with self.assertQueryCount(__system__=14, employee=15): # com+tm 14/15 activity.action_feedback(feedback='Zizisse Done !') @users('__system__', 'employee') @@ -252,7 +252,7 @@ class TestMailAPIPerformance(BaseMailPerformance): def test_adv_activity_mixin(self): record = self.env['mail.test.activity'].create({'name': 'Test'}) - with self.assertQueryCount(__system__=8, employee=8): + with self.assertQueryCount(__system__=7, employee=7): activity = record.action_start('Test Start') # read activity_type to normalize cache between enterprise and community # voip module read activity_type during create leading to one less query in enterprise on action_close @@ -260,7 +260,7 @@ class TestMailAPIPerformance(BaseMailPerformance): record.write({'name': 'Dupe write'}) - with self.assertQueryCount(__system__=18, employee=21): # com+tm 17/20 + with self.assertQueryCount(__system__=15, employee=17): # com+tm 15/17 record.action_close('Dupe feedback') self.assertEqual(record.activity_ids, self.env['mail.activity']) @@ -278,7 +278,7 @@ class TestMailAPIPerformance(BaseMailPerformance): for values in self.test_attachments_vals ]) - with self.assertQueryCount(__system__=8, employee=8): + with self.assertQueryCount(__system__=7, employee=7): activity = record.action_start('Test Start') #read activity_type to normalize cache between enterprise and community #voip module read activity_type during create leading to one less query in enterprise on action_close @@ -286,7 +286,7 @@ class TestMailAPIPerformance(BaseMailPerformance): record.write({'name': 'Dupe write'}) - with self.assertQueryCount(__system__=21, employee=25): # com+tm 19/23 + with self.assertQueryCount(__system__=18, employee=21): # com+tm 17/20 record.action_close('Dupe feedback', attachment_ids=attachments.ids) # notifications