From e8878a8b4c6dcf7927b5ec0623646cf2f2be477d Mon Sep 17 00:00:00 2001 From: Florian Charlier Date: Fri, 1 Sep 2023 18:57:31 +0200 Subject: [PATCH] [IMP] {mass_mailing_,}sms: delay sms deletion Also impacts test_mail_sms. Purpose: let a cron do delete records to end the sending transaction sooner. Removing the foreign key between sms_sms and mailing_trace is necessary as traces may have to be updated due to delivery reports. This is why we do it here and not with notifications because only traces will trigger updates of a possibly massive number of records and repeated concurrent updates. We also replace the now obsolete "sms_sms_X" prefix as there are not many "sms_id" to distinguish from. Task-2560666 Part-of: odoo/odoo#133392 --- addons/mass_mailing_sms/controllers/main.py | 8 ++--- .../mass_mailing_sms/models/mailing_trace.py | 20 +++++++++---- addons/mass_mailing_sms/models/sms_sms.py | 5 +++- addons/mass_mailing_sms/tests/common.py | 6 ++-- .../views/mailing_trace_views.xml | 10 +++---- addons/sms/controllers/main.py | 2 +- addons/sms/models/mail_notification.py | 18 +++++++++-- addons/sms/models/mail_thread.py | 4 +-- addons/sms/models/sms_sms.py | 30 +++++++++++-------- addons/sms/tests/common.py | 4 --- addons/sms/views/sms_sms_views.xml | 4 ++- .../test_mail_sms/tests/test_sms_composer.py | 10 +++---- .../tests/test_sms_management.py | 4 +-- .../tests/test_sms_performance.py | 4 +-- addons/test_mail_sms/tests/test_sms_sms.py | 8 ++--- 15 files changed, 82 insertions(+), 55 deletions(-) diff --git a/addons/mass_mailing_sms/controllers/main.py b/addons/mass_mailing_sms/controllers/main.py index 35aee22dbd1..000b26f62d7 100644 --- a/addons/mass_mailing_sms/controllers/main.py +++ b/addons/mass_mailing_sms/controllers/main.py @@ -89,10 +89,10 @@ class MailingSMSController(http.Controller): 'unsubscribe_error': unsubscribe_error, }) - @http.route('/r//s/', type='http', auth="public") - def sms_short_link_redirect(self, code, sms_sms_id, **post): - if sms_sms_id: - trace_id = request.env['mailing.trace'].sudo().search([('sms_sms_id_int', '=', int(sms_sms_id))]).id + @http.route('/r//s/', type='http', auth="public") + def sms_short_link_redirect(self, code, sms_id_int, **post): + if sms_id_int: + trace_id = request.env['mailing.trace'].sudo().search([('sms_id_int', '=', int(sms_id_int))]).id else: trace_id = False diff --git a/addons/mass_mailing_sms/models/mailing_trace.py b/addons/mass_mailing_sms/models/mailing_trace.py index b884c71b402..f485c9b29ee 100644 --- a/addons/mass_mailing_sms/models/mailing_trace.py +++ b/addons/mass_mailing_sms/models/mailing_trace.py @@ -16,9 +16,9 @@ class MailingTrace(models.Model): trace_type = fields.Selection(selection_add=[ ('sms', 'SMS') ], ondelete={'sms': 'set default'}) - sms_sms_id = fields.Many2one('sms.sms', string='SMS', index='btree_not_null', ondelete='set null') - sms_sms_id_int = fields.Integer( - string='SMS ID (tech)', + sms_id = fields.Many2one('sms.sms', string='SMS', store=False, compute='_compute_sms_id') + sms_id_int = fields.Integer( + string='SMS ID', index='btree_not_null' # Integer because the related sms.sms can be deleted separately from its statistics. # However, the ID is needed for several action and controllers. @@ -46,11 +46,21 @@ class MailingTrace(models.Model): ('sms_rejected', 'Rejected'), ]) + @api.depends('sms_id_int', 'trace_type') + def _compute_sms_id(self): + self.sms_id = False + sms_traces = self.filtered(lambda t: t.trace_type == 'sms' and bool(t.sms_id_int)) + if not sms_traces: + return + existing_sms_ids = self.env['sms.sms'].sudo().search([ + ('id', 'in', sms_traces.mapped('sms_id_int')), ('to_delete', '!=', True) + ]).ids + for sms_trace in sms_traces.filtered(lambda n: n.sms_id_int in set(existing_sms_ids)): + sms_trace.sms_id = sms_trace.sms_id_int + @api.model_create_multi def create(self, values_list): for values in values_list: - if 'sms_sms_id' in values: - values['sms_sms_id_int'] = values['sms_sms_id'] if values.get('trace_type') == 'sms' and not values.get('sms_code'): values['sms_code'] = self._get_random_code() return super(MailingTrace, self).create(values_list) diff --git a/addons/mass_mailing_sms/models/sms_sms.py b/addons/mass_mailing_sms/models/sms_sms.py index 768554a3961..03ffe03d8d0 100644 --- a/addons/mass_mailing_sms/models/sms_sms.py +++ b/addons/mass_mailing_sms/models/sms_sms.py @@ -10,7 +10,10 @@ class SmsSms(models.Model): _inherit = ['sms.sms'] mailing_id = fields.Many2one('mailing.mailing', string='Mass Mailing') - mailing_trace_ids = fields.One2many('mailing.trace', 'sms_sms_id', string='Statistics') + # Linking to another field than the comodel id allows to use the ORM to create + # "linked" records (see _prepare_sms_values) without adding a foreign key. + # See commit message for why this is useful. + mailing_trace_ids = fields.One2many('mailing.trace', 'sms_id_int', string='Statistics') def _update_body_short_links(self): """ Override to tweak shortened URLs by adding statistics ids, allowing to diff --git a/addons/mass_mailing_sms/tests/common.py b/addons/mass_mailing_sms/tests/common.py index 292d2b22295..36502491a68 100644 --- a/addons/mass_mailing_sms/tests/common.py +++ b/addons/mass_mailing_sms/tests/common.py @@ -90,7 +90,7 @@ class MassSMSCase(SMSCase, MockLinkTracker): ) self.assertTrue(len(trace) == 1, 'SMS: found %s notification for number %s, (status: %s) (1 expected)' % (len(trace), number, status)) - self.assertTrue(bool(trace.sms_sms_id_int)) + self.assertTrue(bool(trace.sms_id_int)) if check_sms: if status in {'process', 'pending', 'sent'}: @@ -156,8 +156,8 @@ class MassSMSCase(SMSCase, MockLinkTracker): if '/r/' in url: # shortened link, like 'http://localhost:8069/r/LBG/s/53' parsed_url = werkzeug.urls.url_parse(url) path_items = parsed_url.path.split('/') - code, sms_sms_id = path_items[2], int(path_items[4]) - trace_id = self.env['mailing.trace'].sudo().search([('sms_sms_id_int', '=', sms_sms_id)]).id + code, sms_id_int = path_items[2], int(path_items[4]) + trace_id = self.env['mailing.trace'].sudo().search([('sms_id_int', '=', sms_id_int)]).id self.env['link.tracker.click'].sudo().add_click( code, diff --git a/addons/mass_mailing_sms/views/mailing_trace_views.xml b/addons/mass_mailing_sms/views/mailing_trace_views.xml index 44cdb1188e9..ca1dd9026c7 100644 --- a/addons/mass_mailing_sms/views/mailing_trace_views.xml +++ b/addons/mass_mailing_sms/views/mailing_trace_views.xml @@ -6,8 +6,8 @@ - - + + @@ -68,7 +68,7 @@ - - + - + diff --git a/addons/sms/controllers/main.py b/addons/sms/controllers/main.py index dfa96bd4e24..b4931ddc4c4 100644 --- a/addons/sms/controllers/main.py +++ b/addons/sms/controllers/main.py @@ -36,7 +36,7 @@ class SmsController(Controller): else: sms_trackers_sudo._action_update_from_provider_error(iap_status) all_uuids += uuids - http.request.env['sms.sms'].sudo().search([('uuid', 'in', all_uuids)]).unlink() + request.env['sms.sms'].sudo().search([('uuid', 'in', all_uuids), ('to_delete', '=', False)]).to_delete = True return 'OK' @staticmethod diff --git a/addons/sms/models/mail_notification.py b/addons/sms/models/mail_notification.py index fe9d5567051..e8e1aea2fbf 100644 --- a/addons/sms/models/mail_notification.py +++ b/addons/sms/models/mail_notification.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- # Part of Odoo. See LICENSE file for full copyright and licensing details. -from odoo import fields, models +from odoo import api, fields, models class MailNotification(models.Model): @@ -10,7 +10,9 @@ class MailNotification(models.Model): notification_type = fields.Selection(selection_add=[ ('sms', 'SMS') ], ondelete={'sms': 'cascade'}) - sms_id = fields.Many2one('sms.sms', string='SMS', index='btree_not_null', ondelete='set null') + sms_id_int = fields.Integer('SMS ID', index='btree_not_null') + # Used to give links on form view without foreign key. In most cases, you'd want to use sms_id_int or sms_tracker_ids.sms_uuid. + sms_id = fields.Many2one('sms.sms', string='SMS', store=False, compute='_compute_sms_id') sms_tracker_ids = fields.One2many('sms.tracker', 'mail_notification_id', string="SMS Trackers") sms_number = fields.Char('SMS Number') failure_type = fields.Selection(selection_add=[ @@ -28,3 +30,15 @@ class MailNotification(models.Model): ('sms_not_delivered', 'Not Delivered'), ('sms_rejected', 'Rejected'), ]) + + @api.depends('sms_id_int', 'notification_type') + def _compute_sms_id(self): + self.sms_id = False + sms_notifications = self.filtered(lambda n: n.notification_type == 'sms' and bool(n.sms_id_int)) + if not sms_notifications: + return + existing_sms_ids = self.env['sms.sms'].sudo().search([ + ('id', 'in', sms_notifications.mapped('sms_id_int')), ('to_delete', '!=', True) + ]).ids + for sms_notification in sms_notifications.filtered(lambda n: n.sms_id_int in set(existing_sms_ids)): + sms_notification.sms_id = sms_notification.sms_id_int diff --git a/addons/sms/models/mail_thread.py b/addons/sms/models/mail_thread.py index cd7a2c2f191..b1ab8a6916f 100644 --- a/addons/sms/models/mail_thread.py +++ b/addons/sms/models/mail_thread.py @@ -305,7 +305,7 @@ class MailThread(models.AbstractModel): 'res_partner_id': sms.partner_id.id, 'sms_number': sms.number, 'notification_type': 'sms', - 'sms_id': sms.id, + 'sms_id_int': sms.id, 'sms_tracker_ids': [Command.create({'sms_uuid': sms.uuid})] if sms.state == 'outgoing' else False, 'is_read': True, # discard Inbox notification 'notification_status': 'ready' if sms.state == 'outgoing' else 'exception', @@ -323,7 +323,7 @@ class MailThread(models.AbstractModel): notif.write({ 'notification_type': 'sms', 'notification_status': 'ready', - 'sms_id': sms.id, + 'sms_id_int': sms.id, 'sms_tracker_ids': [Command.create({'sms_uuid': sms.uuid})], 'sms_number': sms.number, }) diff --git a/addons/sms/models/sms_sms.py b/addons/sms/models/sms_sms.py index 7dbeb16da00..ca93fef8e43 100644 --- a/addons/sms/models/sms_sms.py +++ b/addons/sms/models/sms_sms.py @@ -63,6 +63,10 @@ class SmsSms(models.Model): ('sms_optout', 'Opted Out'), ], copy=False) sms_tracker_id = fields.Many2one('sms.tracker', string='SMS trackers', compute='_compute_sms_tracker_id') + to_delete = fields.Boolean( + 'Marked for deletion', default=False, + help='Will automatically be deleted, while notifications will not be deleted in any case.' + ) _sql_constraints = [ ('uuid_unique', 'unique(uuid)', 'UUID must be unique'), @@ -93,7 +97,7 @@ class SmsSms(models.Model): :param auto_commit: commit after each batch of SMS; :param raise_exception: raise if there is an issue contacting IAP; """ - self = self.filtered(lambda sms: sms.state == 'outgoing') + self = self.filtered(lambda sms: sms.state == 'outgoing' and not sms.to_delete) for batch_ids in self._split_batch(): self.browse(batch_ids)._send(unlink_failed=unlink_failed, unlink_sent=unlink_sent, raise_exception=raise_exception) # auto-commit if asked except in testing mode @@ -101,7 +105,7 @@ class SmsSms(models.Model): self._cr.commit() def resend_failed(self): - sms_to_send = self.filtered(lambda sms: sms.state == 'error') + sms_to_send = self.filtered(lambda sms: sms.state == 'error' and not sms.to_delete) sms_to_send.state = 'outgoing' notification_title = _('Warning') notification_type = 'danger' @@ -135,7 +139,7 @@ class SmsSms(models.Model): :param list ids: optional list of emails ids to send. If passed no search is performed, and these ids are used instead. """ - domain = [('state', '=', 'outgoing')] + domain = [('state', '=', 'outgoing'), ('to_delete', '!=', True)] filtered_ids = self.search(domain, limit=10000).ids # TDE note: arbitrary limit we might have to update if ids: @@ -177,30 +181,30 @@ class SmsSms(models.Model): results_uuids = [result['uuid'] for result in results] all_sms_sudo = self.env['sms.sms'].sudo().search([('uuid', 'in', results_uuids)]).with_context(sms_skip_msg_notification=True) - mail_message_ids_sudo = all_sms_sudo.mail_message_id for iap_state, results_group in tools.groupby(results, key=lambda result: result['state']): sms_sudo = all_sms_sudo.filtered(lambda s: s.uuid in {result['uuid'] for result in results_group}) if success_state := self.IAP_TO_SMS_STATE_SUCCESS.get(iap_state): sms_sudo.sms_tracker_id._action_update_from_sms_state(success_state) - if unlink_sent: - sms_sudo.unlink() - else: - sms_sudo.write({'state': success_state, 'failure_type': False}) + to_delete = {'to_delete': True} if unlink_sent else {} + sms_sudo.write({'state': success_state, 'failure_type': False, **to_delete}) else: failure_type = self.IAP_TO_SMS_FAILURE_TYPE.get(iap_state, 'unknown') if failure_type != 'unknown': sms_sudo.sms_tracker_id._action_update_from_sms_state('error', failure_type=failure_type) else: sms_sudo.sms_tracker_id._action_update_from_provider_error(iap_state) - if unlink_failed: - sms_sudo.unlink() - else: - sms_sudo.write({'state': 'error', 'failure_type': failure_type}) + to_delete = {'to_delete': True} if unlink_failed else {} + sms_sudo.write({'state': 'error', 'failure_type': failure_type, **to_delete}) - mail_message_ids_sudo._notify_message_notification_update() + all_sms_sudo.mail_message_id._notify_message_notification_update() def _update_sms_state_and_trackers(self, new_state, failure_type=None): """Update sms state update and related tracking records (notifications, traces).""" self.write({'state': new_state, 'failure_type': failure_type}) self.sms_tracker_id._action_update_from_sms_state(new_state, failure_type=failure_type) + + @api.autovacuum + def _gc_device(self): + self._cr.execute("DELETE FROM sms_sms WHERE to_delete = TRUE") + _logger.info("GC'd %d sms marked for deletion", self._cr.rowcount) diff --git a/addons/sms/tests/common.py b/addons/sms/tests/common.py index 11c28f16a4a..e86894d09de 100644 --- a/addons/sms/tests/common.py +++ b/addons/sms/tests/common.py @@ -138,10 +138,6 @@ class SMSCase(MockSMS): raise NotImplementedError() return sms - def assertNoSMS(self): - """ Check no sms persisted during mock. """ - self.assertEqual(len(self._new_sms.exists()), 0) - def assertSMSIapSent(self, numbers, content=None): """ Check sent SMS. Order is not checked. Each number should have received the same content. Useful to check batch sending. diff --git a/addons/sms/views/sms_sms_views.xml b/addons/sms/views/sms_sms_views.xml index 59b5c84447b..82cd23459cb 100644 --- a/addons/sms/views/sms_sms_views.xml +++ b/addons/sms/views/sms_sms_views.xml @@ -6,7 +6,8 @@
-