diff --git a/kobo/apps/mass_emails/admin.py b/kobo/apps/mass_emails/admin.py index 0b5feee1e9..f044d06736 100644 --- a/kobo/apps/mass_emails/admin.py +++ b/kobo/apps/mass_emails/admin.py @@ -10,28 +10,24 @@ class MassEmailConfigAdmin(admin.ModelAdmin): fields = ('name', 'subject', 'template', 'query', 'frequency', 'live') actions = ['enqueue_mass_emails'] + def get_readonly_fields(self, request, obj=None): + if obj and obj.type == EmailType.ONE_TIME: + if MassEmailRecord.objects.filter( + email_job__email_config__id=obj.id, status=EmailStatus.ENQUEUED + ).exists(): + return ('live',) + return () + @admin.action(description='Add to daily send queue') def enqueue_mass_emails(self, request, queryset): for config in queryset: if config.type == EmailType.ONE_TIME: - if ( - MassEmailRecord.objects.filter(email_job__email_config=config) - .exclude(status=EmailStatus.FAILED) - .exists() - ): + if config.live: self.message_user( request, - f'Emails for {config.name} have already been sent or enqueued. ' - f'Cannot send a one-time email twice.', + f'Emails for {config.name} have already been scheduled', level=messages.ERROR, ) - elif config.live: - self.message_user( - request, - f'Emails for {config.name} have already been scheduled for' - f' tomorrow', - level=messages.SUCCESS, - ) else: config.live = True config.save() diff --git a/kobo/apps/mass_emails/tasks.py b/kobo/apps/mass_emails/tasks.py index 0699201a9b..e853fe36e6 100644 --- a/kobo/apps/mass_emails/tasks.py +++ b/kobo/apps/mass_emails/tasks.py @@ -316,6 +316,17 @@ def send_emails(): return sender = MassEmailSender() sender.send_day_emails() + finished_one_offs = ( + MassEmailConfig.objects.filter(frequency=-1, live=True) + .values('id') + .annotate( + enqueued_count=Count( + 'pk', filter=Q(jobs__records__status=EmailStatus.ENQUEUED) + ) + ) + .filter(enqueued_count=0) + ).values_list('pk', flat=True) + MassEmailConfig.objects.filter(pk__in=finished_one_offs).update(live=False) def get_users_for_config(email_config): @@ -374,14 +385,8 @@ def generate_mass_email_user_lists(): email_job__email_config=email_config, ) - # Skip processing for one time emails that have been sent or enqueued - if email_config.frequency == -1 and email_records.filter( - status__in=[EmailStatus.ENQUEUED, EmailStatus.SENT] - ).exists(): - processed_configs.add(email_config.id) - - # Skip processing if there are pending enqueued records - elif email_records.filter(status=EmailStatus.ENQUEUED).exists(): + # Skip processing emails that have already been enqueued + if email_records.filter(status=EmailStatus.ENQUEUED).exists(): logging.info( f'Skipping email config {email_config.id} as it already has ' f'enqueued records.' diff --git a/kobo/apps/mass_emails/tests/admin/test_add_to_send.py b/kobo/apps/mass_emails/tests/admin/test_add_to_send.py index e15240071b..34e2b1a34c 100644 --- a/kobo/apps/mass_emails/tests/admin/test_add_to_send.py +++ b/kobo/apps/mass_emails/tests/admin/test_add_to_send.py @@ -30,56 +30,26 @@ def _add_to_send(self, email_config): return request @data( - # frequency, is_live, has_records, expect_live, expected_message_template + # frequency, is_live, expected_message_template ( 1, False, - False, - True, 'Emails for {config} have been added to the daily send', ), - (1, False, True, True, 'Emails for {config} have been added to the daily send'), - ( - 1, - True, - False, - True, - 'Emails for {config} are already part of the daily send', - ), - (1, True, True, True, 'Emails for {config} are already part of the daily send'), + (1, True, 'Emails for {config} are already part of the daily send'), ( -1, False, - False, - True, 'Emails for {config} have been scheduled for tomorrow', ), ( -1, - False, - True, - False, - 'Emails for {config} have already been sent or enqueued. Cannot send a one-time email twice.', # noqa - ), - ( - -1, - True, - False, - True, - 'Emails for {config} have already been scheduled for tomorrow', - ), - ( - -1, - True, - True, True, - 'Emails for {config} have already been sent or enqueued. Cannot send a one-time email twice.', # noqa + 'Emails for {config} have already been scheduled', ), ) @unpack - def test_admin_action( - self, frequency, is_live, has_records, expect_live, expected_message - ): + def test_admin_action(self, frequency, is_live, expected_message): """ Test that selecting 'Add to daily send queue' in the admin sets the config to live or delivers an appropriate error @@ -93,46 +63,29 @@ def test_admin_action( live=is_live, ) - if has_records: - user = User.objects.get(username='someuser') - job = MassEmailJob.objects.create(email_config=email_config) - MassEmailRecord.objects.create( - email_job=job, user=user, status=EmailStatus.ENQUEUED - ) - # Attempt to enqueue the records via the admin action request = self._add_to_send(email_config) expected_message = expected_message.format(config=email_config.name) messages_list = [m.message for m in get_messages(request)] assert expected_message in messages_list - assert email_config.live == expect_live + assert email_config.live - def test_admin_action_ignores_failed_records_for_one_time_sends(self): - """ - Test that selecting 'Add to daily send queue' allows re-adding a one-time send - if previous records all failed - """ + def test_live_disabled_when_oneoff_is_enqueued(self): email_config = MassEmailConfig.objects.create( name='Test Config', subject='Test Subject', template='Test Template', query='users_inactive_for_365_days', frequency=-1, - live=False, + live=True, ) - - user = User.objects.get(username='someuser') + admin_instance = MassEmailConfigAdmin(MassEmailConfig, site) + request = self.client.request().wsgi_request job = MassEmailJob.objects.create(email_config=email_config) MassEmailRecord.objects.create( - email_job=job, user=user, status=EmailStatus.FAILED + email_job=job, user=User.objects.get(pk=1), status=EmailStatus.ENQUEUED ) - request = self._add_to_send(email_config) - - messages_list = [m.message for m in get_messages(request)] - - expected_message = ( - f'Emails for {email_config.name} have been scheduled for tomorrow' - ) - assert expected_message in messages_list - assert email_config.live + readonly_fields = admin_instance.get_readonly_fields(request, email_config) + assert len(readonly_fields) == 1 + assert readonly_fields[0] == 'live' diff --git a/kobo/apps/mass_emails/tests/test_celery_tasks.py b/kobo/apps/mass_emails/tests/test_celery_tasks.py index 80f02b5bb6..2a5ad7b467 100644 --- a/kobo/apps/mass_emails/tests/test_celery_tasks.py +++ b/kobo/apps/mass_emails/tests/test_celery_tasks.py @@ -311,48 +311,11 @@ def test_one_time_emails_deprioritized( @ddt class GenerateDailyEmailUserListTaskTestCase(BaseMassEmailsTestCase): - @data( - (EmailStatus.ENQUEUED, 1), - (EmailStatus.SENT, 0), - (EmailStatus.FAILED, 2), - ) - @unpack - def test_one_time_email_with_existing_records(self, status, enqueued_count): - """ - Test one-time email configs (frequency=-1) behave correctly - - - If status is `ENQUEUED` or `SENT`, no new records should be created. - - If status is `FAILED`, new records should be created. - """ - email_config = self._create_email_config('Test') - self._create_email_record(self.user1, email_config, status) - - self.assertNotIn(email_config.id, cache.get(self.cache_key, set())) - generate_mass_email_user_lists() - records = MassEmailRecord.objects.filter( - email_job__email_config=email_config, status=EmailStatus.ENQUEUED - ) - self.assertEqual(records.count(), enqueued_count) - self.assertIn(email_config.id, cache.get(self.cache_key)) - - def test_one_time_email_with_no_existing_records(self): - """ - Test that new records are created when there are no existing records - """ - email_config = self._create_email_config('Test') - self.assertNotIn(email_config.id, cache.get(self.cache_key, set())) - generate_mass_email_user_lists() - - new_records = MassEmailRecord.objects.filter( - email_job__email_config=email_config, status=EmailStatus.ENQUEUED - ) - self.assertEqual(new_records.count(), 2) - self.assertIn(email_config.id, cache.get(self.cache_key)) @data( - (EmailStatus.ENQUEUED, 1, 1), - (EmailStatus.SENT, 1, 2), - (EmailStatus.FAILED, 1, 2), + (EmailStatus.ENQUEUED, -1, 1), + (EmailStatus.SENT, -1, 2), + (EmailStatus.FAILED, -1, 2), (EmailStatus.ENQUEUED, 2, 1), (EmailStatus.SENT, 2, 2), (EmailStatus.FAILED, 2, 2), @@ -360,7 +323,7 @@ def test_one_time_email_with_no_existing_records(self): @unpack def test_recurring_email_scheduling(self, status, frequency, enqueued_count): """ - Test that recurring email configs (frequency > 0) behave correctly + Test we don't enqueue records if there are already pending ones """ email_config = self._create_email_config( 'Test', frequency=frequency diff --git a/kobo/settings/base.py b/kobo/settings/base.py index e5ede08223..99846a4e51 100644 --- a/kobo/settings/base.py +++ b/kobo/settings/base.py @@ -1419,7 +1419,7 @@ def dj_stripe_request_callback_method(): if os.environ.get('EMAIL_USE_TLS'): EMAIL_USE_TLS = os.environ.get('EMAIL_USE_TLS') -MAX_MASS_EMAILS_PER_DAY = 1000 +MAX_MASS_EMAILS_PER_DAY = 1 MASS_EMAIL_THROTTLE_PER_SECOND = 40 MASS_EMAIL_SLEEP_SECONDS = 1 # change the interval between "daily" email sends for testing. this will set both