From 5c8432f4679af3eaa986a7c1cc62315c3807e0f6 Mon Sep 17 00:00:00 2001 From: Wes Okes Date: Thu, 11 Feb 2021 15:28:38 -0500 Subject: [PATCH 1/5] fix unique constraint error from duplicate entities --- entity_emailer/models.py | 16 ++++++++---- entity_emailer/tests/interface_tests.py | 34 +++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/entity_emailer/models.py b/entity_emailer/models.py index c61a593..e46c4ba 100644 --- a/entity_emailer/models.py +++ b/entity_emailer/models.py @@ -47,14 +47,20 @@ def create_emails(self, email_params_list): # Build list of recipient through relationships to create recipients_to_create = [] + + # Keep track of unique pairs to avoid unique constraint on through relationship + email_entity_pairs = set() for i, recipient_entities in enumerate(recipient_entities_per_email): for recipient_entity in recipient_entities: - recipients_to_create.append( - Email.recipients.through( - email_id=emails[i].id, - entity_id=recipient_entity.id, + if (emails[i].id, recipient_entity.id) not in email_entity_pairs: + email_entity_pairs.add((emails[i].id, recipient_entity.id)) + + recipients_to_create.append( + Email.recipients.through( + email_id=emails[i].id, + entity_id=recipient_entity.id, + ) ) - ) # Bulk create the recipient relationships Email.recipients.through.objects.bulk_create(recipients_to_create) diff --git a/entity_emailer/tests/interface_tests.py b/entity_emailer/tests/interface_tests.py index 79a4ff2..733e9d4 100644 --- a/entity_emailer/tests/interface_tests.py +++ b/entity_emailer/tests/interface_tests.py @@ -296,10 +296,15 @@ def test_multiple_events_only_following_false(self): @freeze_time('2013-1-2') def test_bulk_multiple_events_only_following_false(self): + """ + Handles bulk creating events and tests the unique constraint of the duplicated subscription which would cause + a bulk create error if it wasn't handled + """ source = G(Source) e = G(Entity) other_e = G(Entity) + G(Subscription, entity=e, source=source, medium=self.email_medium, only_following=False) G(Subscription, entity=e, source=source, medium=self.email_medium, only_following=False) G(Subscription, entity=other_e, source=source, medium=self.email_medium, only_following=False) email_context = { @@ -342,6 +347,35 @@ def test_multiple_events_only_following_true(self): self.assertEquals(email.subject, '') self.assertEquals(email.scheduled, datetime(2013, 1, 2)) + @freeze_time('2013-1-2') + def test_bulk_multiple_events_only_following_true(self): + """ + Handles bulk creating events and tests the unique constraint of the duplicated subscription which would cause + a bulk create error if it wasn't handled + """ + source = G(Source) + e = G(Entity) + other_e = G(Entity) + + G(Subscription, entity=e, source=source, medium=self.email_medium, only_following=True) + G(Subscription, entity=e, source=source, medium=self.email_medium, only_following=True) + G(Subscription, entity=other_e, source=source, medium=self.email_medium, only_following=True) + email_context = { + 'entity_emailer_template': 'template', + 'entity_emailer_subject': 'hi', + } + G(Event, source=source, context=email_context) + event = G(Event, source=source, context=email_context) + G(EventActor, event=event, entity=e) + + EntityEmailerInterface.bulk_convert_events_to_emails() + + email = Email.objects.get() + self.assertEquals(set(email.recipients.all()), set([e])) + self.assertEquals(email.event.context, email_context) + self.assertEquals(email.subject, '') + self.assertEquals(email.scheduled, datetime(2013, 1, 2)) + @freeze_time('2014-01-05') class SendUnsentScheduledEmailsTest(TestCase): From a4dba0d801a346e07af8f02be20e17b3a392677b Mon Sep 17 00:00:00 2001 From: Wes Okes Date: Thu, 11 Feb 2021 15:30:10 -0500 Subject: [PATCH 2/5] notes --- entity_emailer/version.py | 2 +- release_notes.rst | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/entity_emailer/version.py b/entity_emailer/version.py index afced14..3f39079 100644 --- a/entity_emailer/version.py +++ b/entity_emailer/version.py @@ -1 +1 @@ -__version__ = '2.0.0' +__version__ = '2.0.1' diff --git a/release_notes.rst b/release_notes.rst index 30d5525..79c7a7f 100644 --- a/release_notes.rst +++ b/release_notes.rst @@ -1,6 +1,10 @@ Release Notes ============= +v2.0.1 +------ +* Fix unique constraint when bulk creating emails + v2.0.0 ------ * Added bulk interface for converting to emails From ead0425d8937287a27e80e9064fb06c99a214ba6 Mon Sep 17 00:00:00 2001 From: Wes Okes Date: Thu, 11 Feb 2021 15:31:01 -0500 Subject: [PATCH 3/5] version --- entity_emailer/version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/entity_emailer/version.py b/entity_emailer/version.py index 3f39079..668c344 100644 --- a/entity_emailer/version.py +++ b/entity_emailer/version.py @@ -1 +1 @@ -__version__ = '2.0.1' +__version__ = '2.0.2' From 8c573a5e2bfc71f09f37cd027c6d47301c830a9f Mon Sep 17 00:00:00 2001 From: Ryan Bales Date: Thu, 4 Mar 2021 13:53:53 -0500 Subject: [PATCH 4/5] send messages retry failure handling --- entity_emailer/interface.py | 21 +++-- .../migrations/0004_email_num_tries.py | 18 ++++ entity_emailer/models.py | 3 + entity_emailer/tests/interface_tests.py | 89 +++++++++++-------- requirements/requirements.txt | 1 + settings.py | 1 + 6 files changed, 88 insertions(+), 45 deletions(-) create mode 100644 entity_emailer/migrations/0004_email_num_tries.py diff --git a/entity_emailer/interface.py b/entity_emailer/interface.py index 07b58ec..6afde87 100644 --- a/entity_emailer/interface.py +++ b/entity_emailer/interface.py @@ -1,15 +1,16 @@ +from datetime import datetime import json import sys import traceback -from datetime import datetime -from django.db import transaction +from ambition_utils.transaction import durable +from django.conf import settings from django.core import mail +from django.db import transaction from entity_event import context_loader from entity_emailer.models import Email from entity_emailer.signals import pre_send, email_exception - from entity_emailer.utils import get_medium, get_from_email_address, get_subscribed_email_addresses, \ create_email_message, extract_email_subject_from_html_content @@ -20,6 +21,7 @@ class EntityEmailerInterface(object): """ @classmethod + @durable def send_unsent_scheduled_emails(cls): """ Send out any scheduled emails that are unsent @@ -30,7 +32,8 @@ def send_unsent_scheduled_emails(cls): email_medium = get_medium() to_send = Email.objects.filter( scheduled__lte=current_time, - sent__isnull=True + sent__isnull=True, + num_tries__lt=settings.ENTITY_EMAILER_MAX_SEND_MESSAGE_TRIES ).select_related( 'event__source' ).prefetch_related( @@ -95,12 +98,13 @@ def send_unsent_scheduled_emails(cls): try: # Send mail connection.send_messages([email.get('message')]) + # Update the email model sent value + email_model = email.get('model') + email_model.sent = current_time + email_model.save(update_fields=['sent']) except Exception as e: cls.save_email_exception(email.get('model'), e) - # Update the emails as sent - to_send.update(sent=current_time) - @staticmethod def convert_events_to_emails(): """ @@ -163,7 +167,8 @@ def save_email_exception(cls, email, e): exception_message += ': {}'.format(json.dumps(e.to_dict())) email.exception = exception_message - email.save(update_fields=['exception']) + email.num_tries += 1 + email.save(update_fields=['exception', 'num_tries']) # Fire the email exception event email_exception.send( diff --git a/entity_emailer/migrations/0004_email_num_tries.py b/entity_emailer/migrations/0004_email_num_tries.py new file mode 100644 index 0000000..1d693d6 --- /dev/null +++ b/entity_emailer/migrations/0004_email_num_tries.py @@ -0,0 +1,18 @@ +# Generated by Django 2.2.13 on 2021-03-02 21:05 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('entity_emailer', '0003_email_exception'), + ] + + operations = [ + migrations.AddField( + model_name='email', + name='num_tries', + field=models.IntegerField(default=0), + ), + ] diff --git a/entity_emailer/models.py b/entity_emailer/models.py index e46c4ba..a3fee91 100644 --- a/entity_emailer/models.py +++ b/entity_emailer/models.py @@ -105,6 +105,9 @@ class Email(models.Model): # The time that the email was actually sent, or None if the email is still waiting to be sent sent = models.DateTimeField(null=True, default=None) + # Number of attempts to send this email message, used only in retry logic + num_tries = models.IntegerField(default=0) + # Any exception that occurred when attempting to send the email last exception = models.TextField(default=None, null=True) diff --git a/entity_emailer/tests/interface_tests.py b/entity_emailer/tests/interface_tests.py index c44d628..4b6877b 100644 --- a/entity_emailer/tests/interface_tests.py +++ b/entity_emailer/tests/interface_tests.py @@ -5,6 +5,7 @@ from django.core import mail from django.core.mail import EmailMultiAlternatives from django.core.management import call_command +from django.db import utils from django.test import TestCase, SimpleTestCase from django.test.utils import override_settings from django_dynamic_fixture import G @@ -378,41 +379,13 @@ def test_multiple_events_only_following_true(self): self.assertEquals(email.subject, '') self.assertEquals(email.scheduled, datetime(2013, 1, 2)) - @freeze_time('2013-1-2') - def test_bulk_multiple_events_only_following_true(self): - """ - Handles bulk creating events and tests the unique constraint of the duplicated subscription which would cause - a bulk create error if it wasn't handled - """ - source = G(Source) - e = G(Entity) - other_e = G(Entity) - - G(Subscription, entity=e, source=source, medium=self.email_medium, only_following=True) - G(Subscription, entity=e, source=source, medium=self.email_medium, only_following=True) - G(Subscription, entity=other_e, source=source, medium=self.email_medium, only_following=True) - email_context = { - 'entity_emailer_template': 'template', - 'entity_emailer_subject': 'hi', - } - G(Event, source=source, context=email_context) - event = G(Event, source=source, context=email_context) - G(EventActor, event=event, entity=e) - - EntityEmailerInterface.bulk_convert_events_to_emails() - - email = Email.objects.get() - self.assertEquals(set(email.recipients.all()), set([e])) - self.assertEquals(email.event.context, email_context) - self.assertEquals(email.subject, '') - self.assertEquals(email.scheduled, datetime(2013, 1, 2)) - @freeze_time('2014-01-05') class SendUnsentScheduledEmailsTest(TestCase): def setUp(self): G(Medium, name='email') + @override_settings(DISABLE_DURABILITY_CHECKING=True) @patch('entity_emailer.interface.get_subscribed_email_addresses') @patch.object(Event, 'render', spec_set=True) def test_sends_all_scheduled_emails_no_email_addresses(self, render_mock, address_mock): @@ -423,6 +396,7 @@ def test_sends_all_scheduled_emails_no_email_addresses(self, render_mock, addres EntityEmailerInterface.send_unsent_scheduled_emails() self.assertEqual(len(mail.outbox), 0) + @override_settings(DISABLE_DURABILITY_CHECKING=True) @patch('entity_emailer.interface.get_subscribed_email_addresses') @patch.object(Event, 'render', spec_set=True) def test_sends_all_scheduled_emails(self, render_mock, address_mock): @@ -436,6 +410,7 @@ def test_sends_all_scheduled_emails(self, render_mock, address_mock): self.assertEqual(2, mock_connection.return_value.__enter__.return_value.send_messages.call_count) + @override_settings(DISABLE_DURABILITY_CHECKING=True) @patch('entity_emailer.interface.pre_send') @patch('entity_emailer.interface.get_subscribed_email_addresses') @patch.object(Event, 'render', spec_set=True) @@ -468,6 +443,7 @@ def test_send_signals(self, render_mock, address_mock, mock_pre_send): }) self.assertIsInstance(kwargs['message'], EmailMultiAlternatives) + @override_settings(DISABLE_DURABILITY_CHECKING=True) @patch('entity_emailer.interface.get_subscribed_email_addresses') @patch.object(Event, 'render', spec_set=True) def test_sends_email_with_specified_from_address(self, render_mock, address_mock): @@ -482,6 +458,7 @@ def test_sends_email_with_specified_from_address(self, render_mock, address_mock args = mock_connection.return_value.__enter__.return_value.send_messages.call_args self.assertEqual(args[0][0][0].from_email, from_address) + @override_settings(DISABLE_DURABILITY_CHECKING=True) @patch('entity_emailer.interface.get_subscribed_email_addresses') @patch.object(Event, 'render', spec_set=True) def test_sends_no_future_emails(self, render_mock, address_mock): @@ -491,6 +468,7 @@ def test_sends_no_future_emails(self, render_mock, address_mock): EntityEmailerInterface.send_unsent_scheduled_emails() self.assertEqual(len(mail.outbox), 0) + @override_settings(DISABLE_DURABILITY_CHECKING=True) @patch('entity_emailer.interface.get_subscribed_email_addresses') @patch.object(Event, 'render', spec_set=True) def test_sends_no_sent_emails(self, render_mock, address_mock): @@ -500,6 +478,7 @@ def test_sends_no_sent_emails(self, render_mock, address_mock): EntityEmailerInterface.send_unsent_scheduled_emails() self.assertEqual(len(mail.outbox), 0) + @override_settings(DISABLE_DURABILITY_CHECKING=True) @patch('entity_emailer.interface.get_subscribed_email_addresses') @patch.object(Event, 'render', spec_set=True) def test_updates_times(self, render_mock, address_mock): @@ -510,6 +489,7 @@ def test_updates_times(self, render_mock, address_mock): sent_email = Email.objects.filter(sent__isnull=False) self.assertEqual(sent_email.count(), 1) + @override_settings(DISABLE_DURABILITY_CHECKING=True) @patch('entity_emailer.interface.email_exception') @patch('entity_emailer.interface.get_subscribed_email_addresses') @patch.object(Event, 'render', spec_set=True) @@ -535,19 +515,26 @@ def test_exceptions(self, render_mock, address_mock, mock_email_exception): with patch(settings.EMAIL_BACKEND) as mock_connection: EntityEmailerInterface.send_unsent_scheduled_emails() - # Assert that both emails were marked as sent - self.assertEqual(Email.objects.filter(sent__isnull=False).count(), 2) + # Assert that only one email was marked as sent + self.assertEqual(Email.objects.filter(sent__isnull=False).count(), 1) # Assert that only one email is actually sent through backend self.assertEquals(1, mock_connection.call_count) - # Assert that one email raised an exception - exception_email = Email.objects.get(sent__isnull=False, exception__isnull=False) + # Assert that one email raised an exception and that the tries count was updated + exception_email = Email.objects.get(exception__isnull=False, num_tries=1) self.assertIsNotNone(exception_email) self.assertTrue('Exception: test' in exception_email.exception) + def test_send_unsent_emails_is_durable(self): + """ + Verify that the process to send unsent emails is durable and will not be rolled-back on exit from any caller + """ + self.assertRaises(utils.ProgrammingError, EntityEmailerInterface.send_unsent_scheduled_emails) + + @override_settings(DISABLE_DURABILITY_CHECKING=True, ENTITY_EMAILER_MAX_SEND_MESSAGE_TRIES=2) @patch.object(Event, 'render', spec_set=True) @patch('entity_emailer.interface.get_subscribed_email_addresses') - def test_send_exceptions(self, mock_get_subscribed_addresses, mock_render): + def test_send_exceptions_and_retry(self, mock_get_subscribed_addresses, mock_render): """ Verifies that when a single email raises an exception from within the backend, the batch is still updated as sent and the failed email is saved with the exception @@ -574,16 +561,44 @@ def to_dict(self): EntityEmailerInterface.send_unsent_scheduled_emails() - # Verify that both emails are marked as sent - self.assertEquals(2, Email.objects.filter(sent__isnull=False).count()) + # Verify that only one email is marked as sent + self.assertEquals(1, Email.objects.filter(sent__isnull=False).count()) # Verify that the failed email was saved with the exception - actual_failed_email = Email.objects.get(sent__isnull=False, exception__isnull=False) + actual_failed_email = Email.objects.get(exception__isnull=False, num_tries=1) self.assertEquals(failed_email.id, actual_failed_email.id) self.assertEquals( 'test: {}'.format(json.dumps({'message': 'test'})), actual_failed_email.exception ) + # Verify that a subsequent attempt to send unscheduled emails will retry the failed email + with patch(settings.EMAIL_BACKEND) as mock_connection: + # Mock side effect for sending email + mock_connection.return_value.__enter__.return_value.send_messages.side_effect = ( + TestEmailSendMessageException('test') + ) + + EntityEmailerInterface.send_unsent_scheduled_emails() + + # Verify only called once, with the failed email + self.assertEquals(1, mock_connection.return_value.__enter__.return_value.send_messages.call_count) + + # Verify num tries updated + actual_failed_email = Email.objects.get(exception__isnull=False, num_tries=2) + self.assertEquals(failed_email.id, actual_failed_email.id) + + # Verify that a subsequent attempt to send unscheduled emails will find no emails to send + with patch(settings.EMAIL_BACKEND) as mock_connection: + + EntityEmailerInterface.send_unsent_scheduled_emails() + + # Verify only called once, with the failed email + self.assertEquals(0, mock_connection.return_value.__enter__.return_value.send_messages.call_count) + + # Verify num tries not updated + actual_failed_email = Email.objects.get(exception__isnull=False, num_tries=2) + self.assertEquals(failed_email.id, actual_failed_email.id) + class CreateEmailObjectTest(TestCase): def test_no_html(self): diff --git a/requirements/requirements.txt b/requirements/requirements.txt index c9f24ba..4a4a735 100644 --- a/requirements/requirements.txt +++ b/requirements/requirements.txt @@ -4,5 +4,6 @@ django-db-mutex>=2.0.0 django-entity>=5.0.0 django-entity-event>=2.0.0 ambition-django-uuidfield>=0.5.0 +ambition-utils>=2.2.0 beautifulsoup4>=4.3.2 diff --git a/settings.py b/settings.py index b57f97c..18ffa74 100644 --- a/settings.py +++ b/settings.py @@ -35,6 +35,7 @@ def configure_settings(): raise RuntimeError('Unsupported test DB {0}'.format(test_db)) settings.configure( + ENTITY_EMAILER_MAX_SEND_MESSAGE_TRIES=3, DATABASES={ 'default': db_config, }, From 7fb89f75ff893900360d15ca5fef38a512d1751f Mon Sep 17 00:00:00 2001 From: Ryan Bales Date: Fri, 5 Mar 2021 09:01:41 -0500 Subject: [PATCH 5/5] Release 2.1.0 --- entity_emailer/version.py | 2 +- release_notes.rst | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/entity_emailer/version.py b/entity_emailer/version.py index f593cd5..a33997d 100644 --- a/entity_emailer/version.py +++ b/entity_emailer/version.py @@ -1 +1 @@ -__version__ = '2.0.4' +__version__ = '2.1.0' diff --git a/release_notes.rst b/release_notes.rst index c0fbdb5..e81de7a 100644 --- a/release_notes.rst +++ b/release_notes.rst @@ -1,6 +1,10 @@ Release Notes ============= +v2.1.0 +------ +* More durable handling and retry logic for failures in bulk send unsent emails process + v2.0.4 ------ * Bugfix for updated interface