From e86617fd93f1ddcfceecf2d55d468dd10b92a4bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 24 Jan 2024 16:19:03 +0100 Subject: [PATCH] [FIX] mail, sms: respect scheduled_date for notification When a 'scheduled_date' is given to posting API notifications are delayed. They are send using a cron running on a schedule model. However SMS are not respecting this parameter. This is now fixed. We also use sql.now() instead of datetime.now() when checking notification delay. This leads to values that are consistent through the transaction and avoid non deterministic behavior. This leads to fixing a global mock of "cr.now" that has unexpected side effects in composer tests. Indeed now that the scheduled notification checks cursor now instead of datetime now this global mock leads to some notification not being sent. We now mock cr.now() only for creating records, allowing to effectively test templates using create_date for dynamic scheduled date computation. closes odoo/odoo#150911 Related: odoo/enterprise#55052 Signed-off-by: Thibault Delavallee (tde) --- addons/mail/models/mail_thread.py | 17 +++----- addons/mail/tests/common.py | 14 ++++++ addons/sms/models/mail_thread.py | 4 +- addons/test_mail/tests/test_mail_composer.py | 31 ++++++------- addons/test_mail/tests/test_message_post.py | 10 ++--- addons/test_mail_sms/tests/test_sms_post.py | 46 +++++++++++++++++++- 6 files changed, 89 insertions(+), 33 deletions(-) diff --git a/addons/mail/models/mail_thread.py b/addons/mail/models/mail_thread.py index 18ad7864f7d..180d6667c71 100644 --- a/addons/mail/models/mail_thread.py +++ b/addons/mail/models/mail_thread.py @@ -2948,7 +2948,7 @@ class MailThread(models.AbstractModel): } @api.model - def _is_notification_scheduled(self, notify_cheduled_date): + def _is_notification_scheduled(self, notify_scheduled_date): """ Helper to check if notification are about to be scheduled. Eases overrides. @@ -2959,12 +2959,10 @@ class MailThread(models.AbstractModel): :return bool: True if a valid datetime has been found and is in the future; False otherwise. """ - if notify_cheduled_date: - parsed_datetime = self.env['mail.mail']._parse_scheduled_datetime(notify_cheduled_date) - notify_cheduled_date = parsed_datetime.replace(tzinfo=None) if parsed_datetime else False - return ( - notify_cheduled_date and notify_cheduled_date > datetime.datetime.utcnow() - ) + if notify_scheduled_date: + parsed_datetime = self.env['mail.mail']._parse_scheduled_datetime(notify_scheduled_date) + notify_scheduled_date = parsed_datetime.replace(tzinfo=None) if parsed_datetime else False + return notify_scheduled_date if notify_scheduled_date and notify_scheduled_date > self.env.cr.now() else False def _raise_for_invalid_parameters(self, parameter_names, forbidden_names=None, restricting_names=None): """ Helper to warn about invalid parameters (or fields). @@ -3067,11 +3065,8 @@ class MailThread(models.AbstractModel): return recipients_data # if scheduled for later: add in queue instead of generating notifications - scheduled_date = kwargs.pop('scheduled_date', None) + scheduled_date = self._is_notification_scheduled(kwargs.pop('scheduled_date', None)) if scheduled_date: - parsed_datetime = self.env['mail.mail']._parse_scheduled_datetime(scheduled_date) - scheduled_date = parsed_datetime.replace(tzinfo=None) if parsed_datetime else False - if scheduled_date and scheduled_date > datetime.datetime.utcnow(): # send the message notifications at the scheduled date self.env['mail.message.schedule'].sudo().create({ 'scheduled_datetime': scheduled_date, diff --git a/addons/mail/tests/common.py b/addons/mail/tests/common.py index 0221b80a37d..87e77f286bd 100644 --- a/addons/mail/tests/common.py +++ b/addons/mail/tests/common.py @@ -9,6 +9,7 @@ import time from ast import literal_eval from collections import defaultdict from contextlib import contextmanager +from freezegun import freeze_time from functools import partial from lxml import html from unittest.mock import patch @@ -44,6 +45,19 @@ class MockEmail(common.BaseCase, MockSmtplibCase): super(MockEmail, cls).setUpClass() cls._mc_enabled = False + # ------------------------------------------------------------ + # UTILITY MOCKS + # ------------------------------------------------------------ + + @contextmanager + def mock_datetime_and_now(self, mock_dt): + """ Used when synchronization date (using env.cr.now()) is important + in addition to standard datetime mocks. Used mainly to detect sync + issues. """ + with freeze_time(mock_dt), \ + patch.object(self.env.cr, 'now', lambda: mock_dt): + yield + # ------------------------------------------------------------ # GATEWAY MOCK # ------------------------------------------------------------ diff --git a/addons/sms/models/mail_thread.py b/addons/sms/models/mail_thread.py index b1ab8a6916f..6ac839e5201 100644 --- a/addons/sms/models/mail_thread.py +++ b/addons/sms/models/mail_thread.py @@ -208,8 +208,10 @@ class MailThread(models.AbstractModel): ) def _notify_thread(self, message, msg_vals=False, **kwargs): + scheduled_date = self._is_notification_scheduled(kwargs.get('scheduled_date')) recipients_data = super(MailThread, self)._notify_thread(message, msg_vals=msg_vals, **kwargs) - self._notify_thread_by_sms(message, recipients_data, msg_vals=msg_vals, **kwargs) + if not scheduled_date: + self._notify_thread_by_sms(message, recipients_data, msg_vals=msg_vals, **kwargs) return recipients_data def _notify_thread_by_sms(self, message, recipients_data, msg_vals=False, diff --git a/addons/test_mail/tests/test_mail_composer.py b/addons/test_mail/tests/test_mail_composer.py index 92a3f198e74..a88fff8e8c2 100644 --- a/addons/test_mail/tests/test_mail_composer.py +++ b/addons/test_mail/tests/test_mail_composer.py @@ -5,7 +5,6 @@ import base64 from ast import literal_eval from datetime import timedelta -from freezegun import freeze_time from itertools import chain, product from unittest.mock import DEFAULT, patch @@ -32,7 +31,6 @@ class TestMailComposer(MailCommon, TestRecipients): # force 'now' to ease test about schedulers cls.reference_now = FieldDatetime.from_string('2022-12-24 12:00:00') - cls.env.cr._now = cls.reference_now # force create_date to check schedulers # ensure employee can create partners, necessary for templates cls.user_employee.write({ @@ -56,15 +54,16 @@ class TestMailComposer(MailCommon, TestRecipients): ) cls.env.ref('mail.group_mail_template_editor').users -= cls.user_rendering_restricted - cls.test_record = cls.env['mail.test.ticket.mc'].with_context(cls._test_context).create({ - 'name': 'TestRecord', - 'customer_id': cls.partner_1.id, - 'user_id': cls.user_employee_2.id, - }) - cls.test_records, cls.test_partners = cls._create_records_for_batch( - 'mail.test.ticket.mc', 2, - additional_values={'user_id': cls.user_employee_2.id}, - ) + with cls.mock_datetime_and_now(cls, cls.reference_now): + cls.test_record = cls.env['mail.test.ticket.mc'].with_context(cls._test_context).create({ + 'name': 'TestRecord', + 'customer_id': cls.partner_1.id, + 'user_id': cls.user_employee_2.id, + }) + cls.test_records, cls.test_partners = cls._create_records_for_batch( + 'mail.test.ticket.mc', 2, + additional_values={'user_id': cls.user_employee_2.id}, + ) cls.test_report, cls.test_report_2, cls.test_report_3 = cls.env['ir.actions.report'].create([ { @@ -1679,7 +1678,7 @@ class TestComposerResultsComment(TestMailComposer, CronMixinCase): schedule_cron_id = self.env.ref('mail.ir_cron_send_scheduled_message').id with self.mock_mail_gateway(mail_unlink_sent=False), \ self.mock_mail_app(), \ - freeze_time(self.reference_now), \ + self.mock_datetime_and_now(self.reference_now), \ self.capture_triggers(schedule_cron_id) as capt: composer._action_send_mail() @@ -1748,7 +1747,7 @@ class TestComposerResultsComment(TestMailComposer, CronMixinCase): # Send the scheduled message from the CRON with self.mock_mail_gateway(mail_unlink_sent=False), \ self.mock_mail_app(), \ - freeze_time(self.reference_now + timedelta(days=3)): + self.mock_datetime_and_now(self.reference_now + timedelta(days=3)): self.env['mail.message.schedule'].sudo()._send_notifications_cron() # monorecord: force_send notifications @@ -2191,6 +2190,7 @@ class TestComposerResultsCommentStatus(TestMailComposer): cls.template.write({ 'auto_delete': False, 'model_id': cls.env['ir.model']._get_id(cls.test_records._name), + 'scheduled_date': False, }) def test_assert_initial_data(self): @@ -2591,7 +2591,7 @@ class TestComposerResultsMass(TestMailComposer): self.assertEqual(composer.email_from, self.template.email_from) with self.mock_mail_gateway(mail_unlink_sent=False), \ - freeze_time(self.reference_now): + self.mock_datetime_and_now(self.reference_now): composer._action_send_mail() # partners created from raw emails @@ -2616,7 +2616,7 @@ class TestComposerResultsMass(TestMailComposer): [self.reference_now + timedelta(days=2)] * 2) # simulate cron queue at right time for sending - with freeze_time(self.reference_now + timedelta(days=2)): + with self.mock_datetime_and_now(self.reference_now + timedelta(days=2)): self.env['mail.mail'].sudo().process_email_queue() # everything should be sent now @@ -3236,6 +3236,7 @@ class TestComposerResultsMassStatus(TestMailComposer): ]) cls.template.write({ 'model_id': cls.env['ir.model']._get_id(cls.test_records._name), + 'scheduled_date': False, }) def test_assert_initial_data(self): diff --git a/addons/test_mail/tests/test_message_post.py b/addons/test_mail/tests/test_message_post.py index 006bdce2d95..edebb89034a 100644 --- a/addons/test_mail/tests/test_message_post.py +++ b/addons/test_mail/tests/test_message_post.py @@ -1056,7 +1056,7 @@ class TestMessagePost(TestMessagePostCommon, CronMixinCase): test_record = self.test_record.with_env(self.env) test_record.message_subscribe((self.partner_1 | self.partner_admin).ids) - with freeze_time(now), \ + with self.mock_datetime_and_now(now), \ self.assertMsgWithoutNotifications(), \ self.capture_triggers(cron_id) as capt: msg = test_record.message_post( @@ -1076,12 +1076,12 @@ class TestMessagePost(TestMessagePostCommon, CronMixinCase): self.assertEqual(schedules.scheduled_datetime, scheduled_datetime) # trigger cron now -> should not sent as in future - with freeze_time(now): + with self.mock_datetime_and_now(now): self.env['mail.message.schedule'].sudo()._send_notifications_cron() self.assertTrue(schedules.exists(), msg='Should not have sent the message') # Send the scheduled message from the cron at right date - with freeze_time(now + timedelta(days=5)), self.mock_mail_gateway(mail_unlink_sent=True): + with self.mock_datetime_and_now(now + timedelta(days=5)), self.mock_mail_gateway(mail_unlink_sent=True): self.env['mail.message.schedule'].sudo()._send_notifications_cron() self.assertFalse(schedules.exists(), msg='Should have sent the message') # check notifications have been sent @@ -1099,11 +1099,11 @@ class TestMessagePost(TestMessagePostCommon, CronMixinCase): }) # Send the scheduled message from the CRON - with freeze_time(now + timedelta(days=5)), self.assertNoNotifications(): + with self.mock_datetime_and_now(now + timedelta(days=5)), self.assertNoNotifications(): self.env['mail.message.schedule'].sudo()._send_notifications_cron() # schedule in the past = send when posting - with freeze_time(now), \ + with self.mock_datetime_and_now(now), \ self.mock_mail_gateway(mail_unlink_sent=False), \ self.capture_triggers(cron_id) as capt: msg = test_record.message_post( diff --git a/addons/test_mail_sms/tests/test_sms_post.py b/addons/test_mail_sms/tests/test_sms_post.py index 32c7987dfc9..c8f9daeeb0e 100644 --- a/addons/test_mail_sms/tests/test_sms_post.py +++ b/addons/test_mail_sms/tests/test_sms_post.py @@ -1,11 +1,16 @@ # -*- coding: utf-8 -*- # Part of Odoo. See LICENSE file for full copyright and licensing details. +from datetime import datetime, timedelta + +from odoo.addons.base.tests.test_ir_cron import CronMixinCase from odoo.addons.sms.tests.common import SMSCommon from odoo.addons.test_mail_sms.tests.common import TestSMSRecipients +from odoo.tests import tagged -class TestSMSPost(SMSCommon, TestSMSRecipients): +@tagged('sms_post') +class TestSMSPost(SMSCommon, TestSMSRecipients, CronMixinCase): """ TODO * add tests for new mail.message and mail.thread fields; @@ -209,6 +214,44 @@ class TestSMSPost(SMSCommon, TestSMSRecipients): self.assertSMSNotification([{'partner': self.partner_1}, {'number': self.random_numbers_san[0]}], self._test_body, messages) + def test_message_sms_schedule(self): + """ Test delaying notifications through scheduled_date usage """ + cron_id = self.env.ref('mail.ir_cron_send_scheduled_message').id + now = datetime.utcnow().replace(second=0, microsecond=0) + scheduled_datetime = now + timedelta(days=5) + + with self.mock_datetime_and_now(now), \ + self.with_user('employee'), \ + self.capture_triggers(cron_id) as capt, \ + self.mockSMSGateway(): + test_record = self.env['mail.test.sms'].browse(self.test_record.id) + messages = test_record._message_sms( + 'Testing Scheduled Notifications', + partner_ids=self.partner_1.ids, + scheduled_date=scheduled_datetime, + ) + + self.assertEqual(capt.records.call_at, scheduled_datetime, + msg='Should have created a cron trigger for the scheduled sending') + self.assertFalse(self._new_sms) + self.assertFalse(self._sms) + + schedules = self.env['mail.message.schedule'].sudo().search([('mail_message_id', '=', messages.id)]) + self.assertEqual(len(schedules), 1, msg='Should have scheduled the message') + self.assertEqual(schedules.scheduled_datetime, scheduled_datetime) + + # trigger cron now -> should not sent as in future + with self.mock_datetime_and_now(now): + self.env['mail.message.schedule'].sudo()._send_notifications_cron() + self.assertTrue(schedules.exists(), msg='Should not have sent the message') + + # Send the scheduled message from the cron at right date + with self.mock_datetime_and_now(now + timedelta(days=5)), self.mockSMSGateway(): + self.env['mail.message.schedule'].sudo()._send_notifications_cron() + self.assertFalse(schedules.exists(), msg='Should have sent the message') + # check notifications have been sent + self.assertSMSNotification([{'partner': self.partner_1}], 'Testing Scheduled Notifications', messages) + def test_message_sms_with_template(self): sms_template = self.env['sms.template'].create({ 'name': 'Test Template', @@ -252,6 +295,7 @@ class TestSMSPost(SMSCommon, TestSMSRecipients): self.assertSMSNotification([{'partner': self.partner_1, 'number': self.test_numbers_san[1]}], 'Dear %s this is an SMS.' % self.test_record.display_name, messages) +@tagged('sms_post') class TestSMSPostException(SMSCommon, TestSMSRecipients): @classmethod