From feb510ae9347b18656b7403ed3d6b937c2cd9027 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Tue, 31 Aug 2021 14:03:13 +0000 Subject: [PATCH 1/3] [IMP] sms: improve synchronization of SMS actions with notifications Improve cancel / set outgoing actions on sms model. This notably allows to synchronize notifications when hitting action buttons on sms form view. Also add an action to set as error. This will be used when having IAP feedback on SMS sending. Task-2634957 Prepares Task-2535005 (SMS view pimp) and Task-2560666 (IAP feedback) PR odoo/odoo#75798 --- addons/sms/models/sms_sms.py | 43 +++++++-- addons/sms/views/sms_sms_views.xml | 4 +- .../tests/test_sms_management.py | 87 ++++++++++++++++++- 3 files changed, 124 insertions(+), 10 deletions(-) diff --git a/addons/sms/models/sms_sms.py b/addons/sms/models/sms_sms.py index e9b4c900838..349ecee2777 100644 --- a/addons/sms/models/sms_sms.py +++ b/addons/sms/models/sms_sms.py @@ -45,6 +45,43 @@ class SmsSms(models.Model): ('sms_optout', 'Opted Out'), ], copy=False) + def action_set_canceled(self): + self.state = 'canceled' + notifications = self.env['mail.notification'].sudo().search([ + ('sms_id', 'in', self.ids), + # sent is sent -> cannot reset + ('notification_status', 'not in', ['canceled', 'sent']), + ]) + if notifications: + notifications.write({'notification_status': 'canceled'}) + if not self._context.get('sms_skip_msg_notification', False): + notifications.mail_message_id._notify_message_notification_update() + + def action_set_error(self, failure_type): + self.state = 'error' + self.failure_type = failure_type + notifications = self.env['mail.notification'].sudo().search([ + ('sms_id', 'in', self.ids), + # sent can be set to error due to IAP feedback + ('notification_status', '!=', 'exception'), + ]) + if notifications: + notifications.write({'notification_status': 'exception', 'failure_type': failure_type}) + if not self._context.get('sms_skip_msg_notification', False): + notifications.mail_message_id._notify_message_notification_update() + + def action_set_outgoing(self): + self.state = 'outgoing' + notifications = self.env['mail.notification'].sudo().search([ + ('sms_id', 'in', self.ids), + # sent is sent -> cannot reset + ('notification_status', 'not in', ['ready', 'sent']), + ]) + if notifications: + notifications.write({'notification_status': 'ready', 'failure_type': False}) + if not self._context.get('sms_skip_msg_notification', False): + notifications.mail_message_id._notify_message_notification_update() + def send(self, delete_all=False, auto_commit=False, raise_exception=False): """ Main API method to send SMS. @@ -59,9 +96,6 @@ class SmsSms(models.Model): if auto_commit is True and not getattr(threading.currentThread(), 'testing', False): self._cr.commit() - def set_outgoing(self): - self.state = 'outgoing' - def resend_failed(self): sms_to_send = self.filtered(lambda sms: sms.state == 'error') notification_title = _('Warning') @@ -88,9 +122,6 @@ class SmsSms(models.Model): } } - def cancel(self): - self.state = 'canceled' - @api.model def _process_queue(self, ids=None): """ Send immediately queued messages, committing after each message is sent. diff --git a/addons/sms/views/sms_sms_views.xml b/addons/sms/views/sms_sms_views.xml index 19f4811d15f..4229eb3896e 100644 --- a/addons/sms/views/sms_sms_views.xml +++ b/addons/sms/views/sms_sms_views.xml @@ -7,8 +7,8 @@
diff --git a/addons/test_mail_full/tests/test_sms_management.py b/addons/test_mail_full/tests/test_sms_management.py index 9a745ff5cbd..af7602d62a8 100644 --- a/addons/test_mail_full/tests/test_sms_management.py +++ b/addons/test_mail_full/tests/test_sms_management.py @@ -2,36 +2,116 @@ # Part of Odoo. See LICENSE file for full copyright and licensing details. from odoo.addons.test_mail_full.tests.common import TestMailFullCommon, TestMailFullRecipients +from odoo.tests import tagged +from odoo.tools import mute_logger -class TestSMSWizards(TestMailFullCommon, TestMailFullRecipients): +class TestSMSActionsCommon(TestMailFullCommon, TestMailFullRecipients): @classmethod def setUpClass(cls): - super(TestSMSWizards, cls).setUpClass() + super(TestSMSActionsCommon, cls).setUpClass() cls.test_record = cls.env['mail.test.sms'].with_context(**cls._test_context).create({ 'name': 'Test', 'customer_id': cls.partner_1.id, }) cls.test_record = cls._reset_mail_context(cls.test_record) cls.msg = cls.test_record.message_post(body='TEST BODY', author_id=cls.partner_employee.id) + cls.sms_p1 = cls.env['sms.sms'].create({ + 'body': 'TEST BODY', + 'failure_type': 'sms_number_format', + 'mail_message_id': cls.msg.id, + 'number': cls.partner_1.mobile, + 'partner_id': cls.partner_1.id, + 'state': 'error', + }) cls.notif_p1 = cls.env['mail.notification'].create({ 'mail_message_id': cls.msg.id, 'res_partner_id': cls.partner_1.id, + 'sms_id': cls.sms_p1.id, 'sms_number': cls.partner_1.mobile, 'notification_type': 'sms', 'notification_status': 'exception', 'failure_type': 'sms_number_format', }) + cls.sms_p2 = cls.env['sms.sms'].create({ + 'body': 'TEST BODY', + 'failure_type': 'sms_credit', + 'mail_message_id': cls.msg.id, + 'number': cls.partner_2.mobile, + 'partner_id': cls.partner_2.id, + 'state': 'error', + }) cls.notif_p2 = cls.env['mail.notification'].create({ 'mail_message_id': cls.msg.id, 'res_partner_id': cls.partner_2.id, + 'sms_id': cls.sms_p2.id, 'sms_number': cls.partner_2.mobile, 'notification_type': 'sms', 'notification_status': 'exception', 'failure_type': 'sms_credit', }) + +@tagged('sms_management') +class TestSMSActions(TestSMSActionsCommon): + + def test_sms_set_cancel(self): + self._reset_bus() + self.sms_p1.action_set_canceled() + self.assertEqual(self.sms_p1.state, 'canceled') + + self.assertMessageBusNotifications(self.msg) + self.assertSMSNotification([ + {'partner': self.partner_1, 'number': self.notif_p1.sms_number, 'state': 'canceled', 'failure_type': 'sms_number_format'}, + {'partner': self.partner_2, 'number': self.notif_p2.sms_number, 'state': 'exception', 'failure_type': 'sms_credit'} + ], 'TEST BODY', self.msg, check_sms=False) # do not check new sms as they already exist + + self._reset_bus() + self.sms_p2.with_context(sms_skip_msg_notification=True).action_set_canceled() + self.assertEqual(self.sms_p2.state, 'canceled') + + self.assertEqual(self.env['bus.bus'].search([]), self.env['bus.bus'], 'SMS: no bus notifications unless asked') + self.assertSMSNotification([ + {'partner': self.partner_1, 'number': self.notif_p1.sms_number, 'state': 'canceled', 'failure_type': 'sms_number_format'}, + {'partner': self.partner_2, 'number': self.notif_p2.sms_number, 'state': 'canceled', 'failure_type': 'sms_credit'} + ], 'TEST BODY', self.msg, check_sms=False) # do not check new sms as they already exist + + + def test_sms_set_error(self): + self._reset_bus() + (self.sms_p1 + self.sms_p2).with_context(sms_skip_msg_notification=True).action_set_canceled() + self.assertEqual(self.sms_p1.state, 'canceled') + self.assertEqual(self.sms_p2.state, 'canceled') + self.assertEqual(self.env['bus.bus'].search([]), self.env['bus.bus'], 'SMS: no bus notifications unless asked') + + (self.sms_p1 + self.sms_p2).action_set_error('sms_server') + self.assertEqual(self.sms_p1.state, 'error') + self.assertEqual(self.sms_p2.state, 'error') + + self.assertMessageBusNotifications(self.msg) + self.assertSMSNotification([ + {'partner': self.partner_1, 'number': self.notif_p1.sms_number, 'state': 'exception', 'failure_type': 'sms_server'}, + {'partner': self.partner_2, 'number': self.notif_p2.sms_number, 'state': 'exception', 'failure_type': 'sms_server'} + ], 'TEST BODY', self.msg, check_sms=False) # do not check new sms as they already exist + + def test_sms_set_outgoing(self): + self._reset_bus() + (self.sms_p1 + self.sms_p2).action_set_outgoing() + self.assertEqual(self.sms_p1.state, 'outgoing') + self.assertEqual(self.sms_p2.state, 'outgoing') + + self.assertMessageBusNotifications(self.msg) + self.assertSMSNotification([ + {'partner': self.partner_1, 'number': self.notif_p1.sms_number, 'state': 'ready'}, + {'partner': self.partner_2, 'number': self.notif_p2.sms_number, 'state': 'ready'} + ], 'TEST BODY', self.msg, check_sms=False) # do not check new sms as they already exist + + +@tagged('sms_management') +class TestSMSWizards(TestSMSActionsCommon): + + @mute_logger('odoo.addons.sms.models.sms_sms') def test_sms_resend(self): self._reset_bus() @@ -47,6 +127,7 @@ class TestSMSWizards(TestMailFullCommon, TestMailFullRecipients): ], 'TEST BODY', self.msg, check_sms=True) self.assertMessageBusNotifications(self.msg) + @mute_logger('odoo.addons.sms.models.sms_sms') def test_sms_resend_update_number(self): self._reset_bus() @@ -76,6 +157,7 @@ class TestSMSWizards(TestMailFullCommon, TestMailFullRecipients): ], 'TEST BODY', self.msg, check_sms=False) self.assertMessageBusNotifications(self.msg) + @mute_logger('odoo.addons.sms.models.sms_sms') def test_sms_resend_internals(self): self._reset_bus() self.assertSMSNotification([ @@ -91,6 +173,7 @@ class TestSMSWizards(TestMailFullCommon, TestMailFullRecipients): with self.mockSMSGateway(): wizard.action_resend() + @mute_logger('odoo.addons.sms.models.sms_sms') def test_sms_resend_w_cancel(self): self._reset_bus() From e86268222b642d4d5b46c461ca4fb90d044805da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Tue, 31 Aug 2021 14:40:47 +0000 Subject: [PATCH 2/3] [FIX] sms: fix mock when sending sms Only SMS not in error should be considered as sent instead of being unlinked. This has no effect on current tests but will be used in future cleaning. Task-2634957 Prepares Task-2535005 (SMS view pimp) and Task-2560666 (IAP feedback) PR odoo/odoo#75798 --- addons/sms/tests/common.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/addons/sms/tests/common.py b/addons/sms/tests/common.py index b454796e81c..9ad3c3c3efc 100644 --- a/addons/sms/tests/common.py +++ b/addons/sms/tests/common.py @@ -65,7 +65,7 @@ class MockSMS(common.BaseCase): return sms_unlink_origin(records, *args, **kwargs) # hack: instead of unlink, update state to sent for tests else: - records.filtered(lambda sms: sms.id in self._new_sms.ids).state = 'sent' + records.filtered(lambda sms: sms.id in self._new_sms.ids and sms.state != 'error').state = 'sent' return True try: From 1b3fca70f788bd81caed921434aa1c8c54420163 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Tue, 31 Aug 2021 15:12:10 +0000 Subject: [PATCH 3/3] [IMP] sms: allow to keep sent sms When calling send on SMS, sent SMS are unlinked. If a ``delete_all`` parameter is true, failed SMS are also unlinked. In this commit we allow to control that behavior with two boolean, one for sent SMS and one for failed SMS. This allows to ask to keep sent SMS and update their status accordingly. More control is necessary notably to enable IAP feedback on sent SMS as an error could happen after considering it as sent. Some flows will have to be updated to decide whether this status is necessary and if SMS are kept. This commit prepares ground for that feature by already improving methods and API. Task-2634957 Prepares Task-2535005 (SMS view pimp) and Task-2560666 (IAP feedback) PR odoo/odoo#75798 --- addons/mass_mailing_sms/models/sms_sms.py | 7 ++-- addons/sms/models/sms_sms.py | 36 +++++++++++++-------- addons/sms/tests/common.py | 11 +++---- addons/test_mail_full/tests/test_sms_sms.py | 4 +-- 4 files changed, 34 insertions(+), 24 deletions(-) diff --git a/addons/mass_mailing_sms/models/sms_sms.py b/addons/mass_mailing_sms/models/sms_sms.py index 504e4417924..298801f3c0e 100644 --- a/addons/mass_mailing_sms/models/sms_sms.py +++ b/addons/mass_mailing_sms/models/sms_sms.py @@ -28,7 +28,7 @@ class SmsSms(models.Model): res[sms.id] = body return res - def _postprocess_iap_sent_sms(self, iap_results, failure_reason=None, delete_all=False): + def _postprocess_iap_sent_sms(self, iap_results, failure_reason=None, unlink_failed=False, unlink_sent=True): all_sms_ids = [item['res_id'] for item in iap_results] if any(sms.mailing_id for sms in self.env['sms.sms'].sudo().browse(all_sms_ids)): for state in self.IAP_TO_SMS_STATE.keys(): @@ -40,4 +40,7 @@ class SmsSms(models.Model): traces.set_sent() elif traces: traces.set_failed(failure_type=self.IAP_TO_SMS_STATE[state]) - return super(SmsSms, self)._postprocess_iap_sent_sms(iap_results, failure_reason=failure_reason, delete_all=delete_all) + return super(SmsSms, self)._postprocess_iap_sent_sms( + iap_results, failure_reason=failure_reason, + unlink_failed=unlink_failed, unlink_sent=unlink_sent + ) diff --git a/addons/sms/models/sms_sms.py b/addons/sms/models/sms_sms.py index 349ecee2777..e0907d93d4a 100644 --- a/addons/sms/models/sms_sms.py +++ b/addons/sms/models/sms_sms.py @@ -82,16 +82,16 @@ class SmsSms(models.Model): if not self._context.get('sms_skip_msg_notification', False): notifications.mail_message_id._notify_message_notification_update() - def send(self, delete_all=False, auto_commit=False, raise_exception=False): + def send(self, unlink_failed=False, unlink_sent=True, auto_commit=False, raise_exception=False): """ Main API method to send SMS. - :param delete_all: delete all SMS (sent or not); otherwise delete only - sent SMS; + :param unlink_failed: unlink failed SMS after IAP feedback; + :param unlink_sent: unlink sent SMS after IAP feedback; :param auto_commit: commit after each batch of SMS; :param raise_exception: raise if there is an issue contacting IAP; """ for batch_ids in self._split_batch(): - self.browse(batch_ids)._send(delete_all=delete_all, raise_exception=raise_exception) + 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 if auto_commit is True and not getattr(threading.currentThread(), 'testing', False): self._cr.commit() @@ -143,7 +143,7 @@ class SmsSms(models.Model): try: # auto-commit except in testing mode auto_commit = not getattr(threading.currentThread(), 'testing', False) - res = self.browse(ids).send(delete_all=False, auto_commit=auto_commit, raise_exception=False) + res = self.browse(ids).send(unlink_failed=False, unlink_sent=True, auto_commit=auto_commit, raise_exception=False) except Exception: _logger.exception("Failed processing SMS queue") return res @@ -153,7 +153,7 @@ class SmsSms(models.Model): for sms_batch in tools.split_every(batch_size, self.ids): yield sms_batch - def _send(self, delete_all=False, raise_exception=False): + def _send(self, unlink_failed=False, unlink_sent=True, raise_exception=False): """ This method tries to send SMS after checking the number (presence and formatting). """ iap_data = [{ @@ -168,25 +168,33 @@ class SmsSms(models.Model): _logger.info('Sent batch %s SMS: %s: failed with exception %s', len(self.ids), self.ids, e) if raise_exception: raise - self._postprocess_iap_sent_sms([{'res_id': sms.id, 'state': 'server_error'} for sms in self], delete_all=delete_all) + self._postprocess_iap_sent_sms( + [{'res_id': sms.id, 'state': 'server_error'} for sms in self], + unlink_failed=unlink_failed, unlink_sent=unlink_sent) else: _logger.info('Send batch %s SMS: %s: gave %s', len(self.ids), self.ids, iap_results) - self._postprocess_iap_sent_sms(iap_results, delete_all=delete_all) + self._postprocess_iap_sent_sms(iap_results, unlink_failed=unlink_failed, unlink_sent=unlink_sent) - def _postprocess_iap_sent_sms(self, iap_results, failure_reason=None, delete_all=False): - if delete_all: - todelete_sms_ids = [item['res_id'] for item in iap_results] - else: - todelete_sms_ids = [item['res_id'] for item in iap_results if item['state'] == 'success'] + def _postprocess_iap_sent_sms(self, iap_results, failure_reason=None, unlink_failed=False, unlink_sent=True): + todelete_sms_ids = [] + if unlink_failed: + todelete_sms_ids += [item['res_id'] for item in iap_results if item['state'] != 'success'] + if unlink_sent: + todelete_sms_ids += [item['res_id'] for item in iap_results if item['state'] == 'success'] for state in self.IAP_TO_SMS_STATE.keys(): sms_ids = [item['res_id'] for item in iap_results if item['state'] == state] if sms_ids: - if state != 'success' and not delete_all: + if state != 'success' and not unlink_failed: self.env['sms.sms'].sudo().browse(sms_ids).write({ 'state': 'error', 'failure_type': self.IAP_TO_SMS_STATE[state], }) + if state == 'success' and not unlink_sent: + self.env['sms.sms'].sudo().browse(sms_ids).write({ + 'state': 'sent', + 'failure_type': False, + }) notifications = self.env['mail.notification'].sudo().search([ ('notification_type', '=', 'sms'), ('sms_id', 'in', sms_ids), diff --git a/addons/sms/tests/common.py b/addons/sms/tests/common.py index 9ad3c3c3efc..ed1351ebae5 100644 --- a/addons/sms/tests/common.py +++ b/addons/sms/tests/common.py @@ -21,7 +21,7 @@ class MockSMS(common.BaseCase): def mockSMSGateway(self, sms_allow_unlink=False, sim_error=None, nbr_t_error=None): self._clear_sms_sent() sms_create_origin = SmsSms.create - sms_unlink_origin = SmsSms.unlink + sms_send_origin = SmsSms._send def _contact_iap(local_endpoint, params): # mock single sms sending @@ -60,18 +60,17 @@ class MockSMS(common.BaseCase): self._new_sms += res.sudo() return res - def _sms_sms_unlink(records, *args, **kwargs): + def _sms_sms_send(records, unlink_failed=False, unlink_sent=True, raise_exception=False): if sms_allow_unlink: - return sms_unlink_origin(records, *args, **kwargs) - # hack: instead of unlink, update state to sent for tests + return sms_send_origin(records, unlink_failed=unlink_failed, unlink_sent=unlink_sent, raise_exception=raise_exception) else: - records.filtered(lambda sms: sms.id in self._new_sms.ids and sms.state != 'error').state = 'sent' + return sms_send_origin(records, unlink_failed=False, unlink_sent=False, raise_exception=raise_exception) return True try: with patch.object(SmsApi, '_contact_iap', side_effect=_contact_iap), \ patch.object(SmsSms, 'create', autospec=True, wraps=SmsSms, side_effect=_sms_sms_create), \ - patch.object(SmsSms, 'unlink', autospec=True, wraps=SmsSms, side_effect=_sms_sms_unlink): + patch.object(SmsSms, '_send', autospec=True, wraps=SmsSms, side_effect=_sms_sms_send): yield finally: pass diff --git a/addons/test_mail_full/tests/test_sms_sms.py b/addons/test_mail_full/tests/test_sms_sms.py index 2c009a47da6..f79e0556750 100644 --- a/addons/test_mail_full/tests/test_sms_sms.py +++ b/addons/test_mail_full/tests/test_sms_sms.py @@ -131,7 +131,7 @@ class TestSMSPost(TestMailFullCommon, LinkTrackerMock): def test_sms_send_batch_size(self): self.count = 0 - def _send(sms_self, delete_all=False, raise_exception=False): + def _send(sms_self, unlink_failed=False, unlink_sent=True, raise_exception=False): self.count += 1 return DEFAULT @@ -147,7 +147,7 @@ class TestSMSPost(TestMailFullCommon, LinkTrackerMock): def test_sms_send_delete_all(self): with self.mockSMSGateway(sms_allow_unlink=True, sim_error='jsonrpc_exception'): - self.env['sms.sms'].browse(self.sms_all.ids).send(delete_all=True, raise_exception=False) + self.env['sms.sms'].browse(self.sms_all.ids).send(unlink_failed=True, unlink_sent=True, raise_exception=False) self.assertFalse(len(self.sms_all.exists())) def test_sms_send_raise(self):