From 3c63e556663376b48694eb16424e626b6976e211 Mon Sep 17 00:00:00 2001 From: Paul McLanahan Date: Tue, 28 Oct 2014 15:56:02 -0400 Subject: [PATCH] Bug 1090432: Retry when we see "No valid subscriber" error. Bug 1090007: Move sending triggerd emails to a task. --- news/tasks.py | 13 ++++--- news/tests/test_confirm.py | 12 +++---- news/tests/test_send_confirm_notice.py | 48 +++++++++++++------------- news/tests/test_send_welcomes.py | 12 +++---- news/tests/test_tasks.py | 6 ++-- news/tests/test_update_user.py | 16 ++++----- news/tests/test_users.py | 16 ++++----- 7 files changed, 61 insertions(+), 62 deletions(-) diff --git a/news/tasks.py b/news/tasks.py index 8af8070..12310b1 100644 --- a/news/tasks.py +++ b/news/tasks.py @@ -304,7 +304,7 @@ def update_fxa_info(email, lang, fxa_id, source_url=None): apply_updates(settings.EXACTTARGET_DATA, record) welcome = mogrify_message_id(FXACCOUNT_WELCOME, lang, welcome_format) - send_message(welcome, email, token, welcome_format) + send_message.delay(welcome, email, token, welcome_format) @et_task @@ -367,7 +367,7 @@ def update_get_involved(interest_id, lang, name, email, country, email_format, 'INTEREST': interest_id, }) welcome_id = mogrify_message_id(interest.welcome_id, lang, email_format) - send_message(welcome_id, email, token, email_format) + send_message.delay(welcome_id, email, token, email_format) interest.notify_stewards(name, email, lang, message) if to_subscribe: @@ -580,6 +580,7 @@ def apply_updates(target_et, record): et.data_ext().add_record(target_et, record.keys(), record.values()) +@et_task def send_message(message_id, email, token, format): """ Ask ET to send a message. @@ -615,8 +616,6 @@ def send_message(message_id, email, token, format): # remember it's a bad message ID so we don't try again during this process. BAD_MESSAGE_ID_CACHE.set(message_id, True) return - elif 'There are no valid subscribers.' in e.message: - raise BasketError("ET says: there are no valid subscribers.") # we should retry raise @@ -669,7 +668,7 @@ def send_confirm_notice(email, token, lang, format, newsletter_slugs): welcome = CONFIRMATION_MESSAGE welcome = mogrify_message_id(welcome, lang, format) - send_message(welcome, email, token, format) + send_message.delay(welcome, email, token, format) def send_welcomes(user_data, newsletter_slugs, format): @@ -717,7 +716,7 @@ def send_welcomes(user_data, newsletter_slugs, format): for welcome in welcomes_to_send: log.info("Sending welcome %s to user %s %s" % (welcome, user_data['email'], user_data['token'])) - send_message(welcome, user_data['email'], user_data['token'], + send_message.delay(welcome, user_data['email'], user_data['token'], format) @@ -828,4 +827,4 @@ def send_recovery_message_task(email): format = user_data.get('format', 'H') or 'H' message_id = mogrify_message_id(RECOVERY_MESSAGE_ID, lang, format) - send_message(message_id, email, user_data['token'], format) + send_message.delay(message_id, email, user_data['token'], format) diff --git a/news/tests/test_confirm.py b/news/tests/test_confirm.py index 25a5eb8..36153af 100644 --- a/news/tests/test_confirm.py +++ b/news/tests/test_confirm.py @@ -404,7 +404,7 @@ def test_text_welcome(self, apply_updates, send_message): } confirm_user(token, user_data) expected_welcome = "%s_%s_%s" % (lang, welcome, format) - send_message.assert_called_with(expected_welcome, email, token, format) + send_message.delay.assert_called_with(expected_welcome, email, token, format) @patch('news.tasks.send_message') @patch('news.tasks.apply_updates') @@ -434,7 +434,7 @@ def test_html_welcome(self, apply_updates, send_message): confirm_user(token, user_data) # Lang code is lowercased. And we don't append anything for HTML. expected_welcome = "%s_%s" % (lang.lower(), welcome) - send_message.assert_called_with(expected_welcome, email, token, format) + send_message.delay.assert_called_with(expected_welcome, email, token, format) @patch('news.tasks.send_message') @patch('news.tasks.apply_updates') @@ -465,7 +465,7 @@ def test_bad_lang_welcome(self, apply_updates, send_message): confirm_user(token, user_data) # They're getting English. And we don't append anything for HTML. expected_welcome = "en_%s" % welcome - send_message.assert_called_with(expected_welcome, email, token, format) + send_message.delay.assert_called_with(expected_welcome, email, token, format) @patch('news.tasks.send_message') @patch('news.tasks.apply_updates') @@ -495,7 +495,7 @@ def test_long_lang_welcome(self, apply_updates, send_message): confirm_user(token, user_data) # They're getting pt. And we don't append anything for HTML. expected_welcome = "pt_%s" % welcome - send_message.assert_called_with(expected_welcome, email, token, format) + send_message.delay.assert_called_with(expected_welcome, email, token, format) @patch('news.tasks.send_message') @patch('news.tasks.apply_updates') @@ -525,7 +525,7 @@ def test_other_long_lang_welcome(self, apply_updates, send_message): confirm_user(token, user_data) # They're getting pt. And we don't append anything for HTML. expected_welcome = "pt_%s" % welcome - send_message.assert_called_with(expected_welcome, email, token, format) + send_message.delay.assert_called_with(expected_welcome, email, token, format) @patch('news.tasks.send_message') @patch('news.tasks.apply_updates') @@ -554,4 +554,4 @@ def test_one_lang_welcome(self, apply_updates, send_message): } confirm_user(token, user_data) expected_welcome = 'en_' + welcome - send_message.assert_called_with(expected_welcome, email, token, format) + send_message.delay.assert_called_with(expected_welcome, email, token, format) diff --git a/news/tests/test_send_confirm_notice.py b/news/tests/test_send_confirm_notice.py index b056d9b..aa167e3 100644 --- a/news/tests/test_send_confirm_notice.py +++ b/news/tests/test_send_confirm_notice.py @@ -31,28 +31,28 @@ def test_default_confirm_notice_H(self, send_message): """Test newsletter that has no explicit confirm""" send_confirm_notice(self.email, self.token, "en", "H", ['slug2']) expected_message = mogrify_message_id(CONFIRMATION_MESSAGE, 'en', 'H') - send_message.assert_called_with(expected_message, - self.email, - self.token, - 'H') + send_message.delay.assert_called_with(expected_message, + self.email, + self.token, + 'H') def test_default_confirm_notice_T(self, send_message): """Test newsletter that has no explicit confirm""" send_confirm_notice(self.email, self.token, "es", "T", ['slug2']) expected_message = mogrify_message_id(CONFIRMATION_MESSAGE, 'es', 'T') - send_message.assert_called_with(expected_message, - self.email, - self.token, - 'T') + send_message.delay.assert_called_with(expected_message, + self.email, + self.token, + 'T') def test_default_confirm_notice_short_for_long(self, send_message): """Test using short form of lang that's in the newsletter list as a long lang""" send_confirm_notice(self.email, self.token, "rr", "H", ['slug2']) expected_message = mogrify_message_id(CONFIRMATION_MESSAGE, 'rr', 'H') - send_message.assert_called_with(expected_message, - self.email, - self.token, - 'H') + send_message.delay.assert_called_with(expected_message, + self.email, + self.token, + 'H') ### known long lang @@ -60,19 +60,19 @@ def test_default_confirm_notice_long_lang_H(self, send_message): """Test newsletter that has no explicit confirm with long lang code""" send_confirm_notice(self.email, self.token, "es-ES", "H", ['slug2']) expected_message = mogrify_message_id(CONFIRMATION_MESSAGE, 'es', 'H') - send_message.assert_called_with(expected_message, - self.email, - self.token, - 'H') + send_message.delay.assert_called_with(expected_message, + self.email, + self.token, + 'H') def test_default_confirm_notice_long_lang_T(self, send_message): """Test newsletter that has no explicit confirm with long lang code""" send_confirm_notice(self.email, self.token, "es-ES", "T", ['slug2']) expected_message = mogrify_message_id(CONFIRMATION_MESSAGE, 'es', 'T') - send_message.assert_called_with(expected_message, - self.email, - self.token, - 'T') + send_message.delay.assert_called_with(expected_message, + self.email, + self.token, + 'T') ### bad lang @@ -97,7 +97,7 @@ def test_explicit_confirm_notice_H(self, send_message): """Test newsletter that has explicit confirm message""" send_confirm_notice(self.email, self.token, "en", "H", ['slug1']) expected_message = 'en_confirm1' - send_message.assert_called_with(expected_message, + send_message.delay.assert_called_with(expected_message, self.email, self.token, 'H') @@ -106,7 +106,7 @@ def test_explicit_confirm_notice_T(self, send_message): """Test newsletter that has explicit confirm message""" send_confirm_notice(self.email, self.token, "en", "T", ['slug1']) expected_message = 'en_confirm1_T' - send_message.assert_called_with(expected_message, + send_message.delay.assert_called_with(expected_message, self.email, self.token, 'T') @@ -117,7 +117,7 @@ def test_explicit_confirm_notice_long_lang_H(self, send_message): """Test newsletter that has explicit confirm message with long lang""" send_confirm_notice(self.email, self.token, "en-US", "H", ['slug1']) expected_message = 'en_confirm1' - send_message.assert_called_with(expected_message, + send_message.delay.assert_called_with(expected_message, self.email, self.token, 'H') @@ -126,7 +126,7 @@ def test_explicit_confirm_notice_long_lang_T(self, send_message): """Test newsletter that has explicit confirm message with long lang""" send_confirm_notice(self.email, self.token, "en-US", "T", ['slug1']) expected_message = 'en_confirm1_T' - send_message.assert_called_with(expected_message, + send_message.delay.assert_called_with(expected_message, self.email, self.token, 'T') diff --git a/news/tests/test_send_welcomes.py b/news/tests/test_send_welcomes.py index 19a6b34..98a97be 100644 --- a/news/tests/test_send_welcomes.py +++ b/news/tests/test_send_welcomes.py @@ -86,7 +86,7 @@ def test_text_welcome(self, apply_updates, send_message): } confirm_user(token, user_data) expected_welcome = "%s_%s_%s" % (lang, welcome, format) - send_message.assert_called_with(expected_welcome, email, token, format) + send_message.delay.assert_called_with(expected_welcome, email, token, format) @patch('news.tasks.send_message') @patch('news.tasks.apply_updates') @@ -116,7 +116,7 @@ def test_html_welcome(self, apply_updates, send_message): confirm_user(token, user_data) # Lang code is lowercased. And we don't append anything for HTML. expected_welcome = "%s_%s" % (lang.lower(), welcome) - send_message.assert_called_with(expected_welcome, email, token, format) + send_message.delay.assert_called_with(expected_welcome, email, token, format) @patch('news.tasks.send_message') @patch('news.tasks.apply_updates') @@ -147,7 +147,7 @@ def test_bad_lang_welcome(self, apply_updates, send_message): confirm_user(token, user_data) # They're getting English. And we don't append anything for HTML. expected_welcome = "en_%s" % welcome - send_message.assert_called_with(expected_welcome, email, token, format) + send_message.delay.assert_called_with(expected_welcome, email, token, format) @patch('news.tasks.send_message') @patch('news.tasks.apply_updates') @@ -177,7 +177,7 @@ def test_long_lang_welcome(self, apply_updates, send_message): confirm_user(token, user_data) # They're getting pt. And we don't append anything for HTML. expected_welcome = "pt_%s" % welcome - send_message.assert_called_with(expected_welcome, email, token, format) + send_message.delay.assert_called_with(expected_welcome, email, token, format) @patch('news.tasks.send_message') @patch('news.tasks.apply_updates') @@ -207,7 +207,7 @@ def test_other_long_lang_welcome(self, apply_updates, send_message): confirm_user(token, user_data) # They're getting pt. And we don't append anything for HTML. expected_welcome = "pt_%s" % welcome - send_message.assert_called_with(expected_welcome, email, token, format) + send_message.delay.assert_called_with(expected_welcome, email, token, format) @patch('news.tasks.send_message') @patch('news.tasks.apply_updates') @@ -236,4 +236,4 @@ def test_one_lang_welcome(self, apply_updates, send_message): } confirm_user(token, user_data) expected_welcome = 'en_' + welcome - send_message.assert_called_with(expected_welcome, email, token, format) + send_message.delay.assert_called_with(expected_welcome, email, token, format) diff --git a/news/tests/test_tasks.py b/news/tests/test_tasks.py index 8a10194..3c71a33 100644 --- a/news/tests/test_tasks.py +++ b/news/tests/test_tasks.py @@ -108,7 +108,7 @@ def test_email_only_in_et(self, mock_look_for_user, mock_send): subscriber = Subscriber.objects.get(email=self.email) self.assertEqual('USERTOKEN', subscriber.token) message_id = mogrify_message_id(RECOVERY_MESSAGE_ID, lang, format) - mock_send.assert_called_with(message_id, self.email, + mock_send.delay.assert_called_with(message_id, self.email, subscriber.token, format) def test_email_only_in_basket(self, mock_look_for_user, mock_send): @@ -136,8 +136,8 @@ def test_email_in_both(self, mock_look_for_user, mock_send): } send_recovery_message_task(self.email) message_id = mogrify_message_id(RECOVERY_MESSAGE_ID, lang, format) - mock_send.assert_called_with(message_id, self.email, - subscriber.token, format) + mock_send.delay.assert_called_with(message_id, self.email, + subscriber.token, format) class UpdateUserTests(TestCase): diff --git a/news/tests/test_update_user.py b/news/tests/test_update_user.py index 26d1072..1f016d6 100644 --- a/news/tests/test_update_user.py +++ b/news/tests/test_update_user.py @@ -78,8 +78,8 @@ def test_update_send_newsletter_welcome(self, get_user_data, send_message, self.assertEqual(UU_ALREADY_CONFIRMED, rc) apply_updates.assert_called() # The welcome should have been sent - send_message.assert_called() - send_message.assert_called_with('en_' + welcome_id, self.sub.email, + send_message.delay.assert_called() + send_message.delay.assert_called_with('en_' + welcome_id, self.sub.email, self.sub.token, 'H') @patch('news.tasks.apply_updates') @@ -258,8 +258,8 @@ def test_update_send_newsletters_welcome(self, get_user_data, api_call_type=SUBSCRIBE, optin=True) self.assertEqual(UU_EXEMPT_NEW, rc) - self.assertEqual(2, send_message.call_count) - calls_args = [x[0] for x in send_message.call_args_list] + self.assertEqual(2, send_message.delay.call_count) + calls_args = [x[0] for x in send_message.delay.call_args_list] self.assertIn(('en_WELCOME1', self.sub.email, self.sub.token, 'H'), calls_args) self.assertIn(('en_WELCOME2', self.sub.email, self.sub.token, 'H'), @@ -294,7 +294,7 @@ def test_update_user_works_with_no_welcome(self, get_user_data, api_call_type=SUBSCRIBE, optin=True) self.assertEqual(UU_ALREADY_CONFIRMED, rc) apply_updates.assert_called() - send_message.assert_called() + send_message.delay.assert_called() @patch('news.tasks.apply_updates') @patch('news.tasks.send_message') @@ -325,7 +325,7 @@ def test_update_user_works_with_no_lang(self, get_user_data, api_call_type=SUBSCRIBE, optin=True) self.assertEqual(UU_EXEMPT_NEW, rc) apply_updates.assert_called() - send_message.assert_called_with(u'en_Welcome_T', + send_message.delay.assert_called_with(u'en_Welcome_T', self.sub.email, self.sub.token, 'T') @@ -366,8 +366,8 @@ def test_ffos_welcome(self, get_user_data, send_message, apply_updates): api_call_type=SUBSCRIBE, optin=True) self.assertEqual(UU_EXEMPT_NEW, rc) - self.assertEqual(1, send_message.call_count) - calls_args = [x[0] for x in send_message.call_args_list] + self.assertEqual(1, send_message.delay.call_count) + calls_args = [x[0] for x in send_message.delay.call_args_list] self.assertIn(('en_FFOS_WELCOME', self.sub.email, self.sub.token, 'H'), calls_args) self.assertNotIn(('en_FF&Y_WELCOME', self.sub.email, diff --git a/news/tests/test_users.py b/news/tests/test_users.py index 6acc114..4608d74 100644 --- a/news/tests/test_users.py +++ b/news/tests/test_users.py @@ -50,7 +50,7 @@ def test_new_user(self): 'MODIFIED_DATE_': ANY, }) self.get_external_user_data.assert_called_with(email=email) - self.send_message.assert_called_with('de_{0}'.format(tasks.FXACCOUNT_WELCOME), + self.send_message.delay.assert_called_with('de_{0}'.format(tasks.FXACCOUNT_WELCOME), email, sub.token, 'H') def test_user_in_et_not_basket(self): @@ -73,7 +73,7 @@ def test_user_in_et_not_basket(self): 'MODIFIED_DATE_': ANY, }) self.get_external_user_data.assert_called_with(email=email) - self.send_message.assert_called_with('de_{0}'.format(tasks.FXACCOUNT_WELCOME), + self.send_message.delay.assert_called_with('de_{0}'.format(tasks.FXACCOUNT_WELCOME), email, sub.token, 'H') def test_existing_user_not_in_basket(self): @@ -100,7 +100,7 @@ def test_existing_user_not_in_basket(self): 'FXA_LANGUAGE_ISO2': 'de', }) self.get_external_user_data.assert_called_with(email=email) - self.send_message.assert_called_with('de_{0}'.format(tasks.FXACCOUNT_WELCOME), + self.send_message.delay.assert_called_with('de_{0}'.format(tasks.FXACCOUNT_WELCOME), email, sub.token, '') def test_existing_user(self): @@ -125,7 +125,7 @@ def test_existing_user(self): 'MODIFIED_DATE_': ANY, 'FXA_LANGUAGE_ISO2': 'de', }) - self.send_message.assert_called_with('de_{0}_T'.format(tasks.FXACCOUNT_WELCOME), + self.send_message.delay.assert_called_with('de_{0}_T'.format(tasks.FXACCOUNT_WELCOME), email, sub.token, 'T') @@ -185,7 +185,7 @@ def test_new_user_interested(self): 'INTEREST': 'bowling', }), ]) - self.send_message.assert_called_with('en_welcome_bowling_T', email, token, 'T') + self.send_message.delay.assert_called_with('en_welcome_bowling_T', email, token, 'T') self.send_welcomes.assert_called_once_with({ 'email': email, 'token': token, @@ -232,7 +232,7 @@ def test_existing_user_interested(self): 'INTEREST': 'bowling', }), ]) - self.send_message.assert_called_with('en_welcome_bowling_T', email, token, 'T') + self.send_message.delay.assert_called_with('en_welcome_bowling_T', email, token, 'T') # not called because 'about-mozilla' already in newsletters self.assertFalse(self.send_welcomes.called) self.interest.notify_stewards.assert_called_with('Walter', email, 'en', @@ -271,7 +271,7 @@ def test_existing_user_interested_no_newsletter(self): 'INTEREST': 'bowling', }), ]) - self.send_message.assert_called_with('en_welcome_bowling_T', email, token, 'T') + self.send_message.delay.assert_called_with('en_welcome_bowling_T', email, token, 'T') self.send_welcomes.assert_called_once_with(self.get_user_data.return_value, ['about-mozilla'], 'T') self.interest.notify_stewards.assert_called_with('Walter', email, 'en', @@ -300,7 +300,7 @@ def test_new_user_interested_no_sub(self): 'INTEREST': 'bowling', }), ]) - self.send_message.assert_called_with('en_welcome_bowling', email, token, 'H') + self.send_message.delay.assert_called_with('en_welcome_bowling', email, token, 'H') self.assertFalse(self.send_welcomes.called) self.interest.notify_stewards.assert_called_with('Walter', email, 'en', 'It really tied the room together.')