diff --git a/admin_tests/notifications/test_campaigns.py b/admin_tests/notifications/test_campaigns.py index b7df85af5ed..5b91e76ff27 100644 --- a/admin_tests/notifications/test_campaigns.py +++ b/admin_tests/notifications/test_campaigns.py @@ -7,6 +7,8 @@ from django.core.exceptions import PermissionDenied from django.test import RequestFactory from django.urls import reverse +from django.utils import timezone +from datetime import timedelta from admin.notifications.forms import NotificationCampaignCreateForm from admin.notifications.views import ( @@ -73,6 +75,7 @@ def test_valid_form_parses_context_and_filters(self, notification_type): assert form.cleaned_data['batch_size'] == settings.DEFAULT_CAMPAIGN_BATCH_SIZE assert form.cleaned_data['max_retries'] == settings.DEFAULT_CAMPAIGN_MAX_RETRIES assert form.cleaned_data['activity_threshold'] == settings.DEFAULT_CAMPAIGN_ACTIVITY_THRESHOLD + assert form.cleaned_data['time_window'] == 8 assert form.cleaned_data['sendgrid_bulk'] is False def test_defaults_come_from_settings(self): @@ -80,6 +83,7 @@ def test_defaults_come_from_settings(self): assert form.fields['batch_size'].initial == settings.DEFAULT_CAMPAIGN_BATCH_SIZE assert form.fields['max_retries'].initial == settings.DEFAULT_CAMPAIGN_MAX_RETRIES assert form.fields['activity_threshold'].initial == settings.DEFAULT_CAMPAIGN_ACTIVITY_THRESHOLD + assert form.fields['time_window'].initial == 8 assert form.fields['sendgrid_bulk'].initial is False def test_invalid_context_json(self, notification_type): @@ -125,6 +129,20 @@ def test_activity_threshold_cannot_be_negative(self, notification_type): assert not form.is_valid() assert 'activity_threshold' in form.errors + def test_time_window_must_be_at_least_one(self, notification_type): + form = NotificationCampaignCreateForm( + data=_valid_form_data(notification_type, time_window=0) + ) + assert not form.is_valid() + assert 'time_window' in form.errors + + def test_time_window_is_accepted(self, notification_type): + form = NotificationCampaignCreateForm( + data=_valid_form_data(notification_type, time_window=6) + ) + assert form.is_valid() + assert form.cleaned_data['time_window'] == 6 + def test_name_is_required(self, notification_type): form = NotificationCampaignCreateForm( data=_valid_form_data(notification_type, name='') @@ -266,7 +284,35 @@ def test_detail_requires_change_permission(self): response = NotificationCampaignDetail.as_view()(request, pk=self.campaign.pk) assert response.status_code == 200 + def test_detail_allow_restart_stuck_false_when_recently_updated(self): + grant_permission(self.user, 'change_notificationcampaign') + request = RequestFactory().get( + reverse('notifications:notification_campaigns_detail', kwargs={'pk': self.campaign.pk}) + ) + request.user = self.user + + response = NotificationCampaignDetail.as_view()(request, pk=self.campaign.pk) + + assert response.status_code == 200 + assert response.context_data['allow_restart_stuck'] is False + + def test_detail_allow_restart_stuck_true_when_updated_long_time_ago(self): + grant_permission(self.user, 'change_notificationcampaign') + NotificationCampaign.objects.filter(pk=self.campaign.pk).update( + updated_at=timezone.now() - timedelta(minutes=16), + ) + request = RequestFactory().get( + reverse('notifications:notification_campaigns_detail', kwargs={'pk': self.campaign.pk}) + ) + request.user = self.user + + response = NotificationCampaignDetail.as_view()(request, pk=self.campaign.pk) + + assert response.status_code == 200 + assert response.context_data['allow_restart_stuck'] is True + def test_start_requires_change_notificationcampaign_permission(self): + request = RequestFactory().post( reverse('notifications:notification_campaigns_start', kwargs={'pk': self.campaign.pk}) ) diff --git a/osf_tests/test_notification_campaign.py b/osf_tests/test_notification_campaign.py index 750bafabbcb..03318ab99fc 100644 --- a/osf_tests/test_notification_campaign.py +++ b/osf_tests/test_notification_campaign.py @@ -620,6 +620,55 @@ def test_send_campaign_batch_skips_cancelled_campaign(self, running_campaign): recipient.refresh_from_db() assert recipient.status == NotificationCampaignRecipientStatus.PENDING + @mock.patch.object(NotificationType, 'emit') + def test_send_campaign_batch_uses_fallback_email_when_username_has_no_at(self, mock_emit, running_campaign): + user = UserFactory() + user.username = 'invalid' + user.save(update_fields=['username']) + user.emails.all().delete() + user.emails.create(address='fallback@example.com') + + create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=running_campaign.id) + recipient = NotificationCampaignRecipient.objects.get(campaign=running_campaign, user=user) + + send_campaign_batch( + context={}, + recipients_ids=[recipient.id], + notification_type_name='blank', + campaign_id=running_campaign.id, + run_id=running_campaign.run_id, + ) + + recipient.refresh_from_db() + running_campaign.refresh_from_db() + assert recipient.status == NotificationCampaignRecipientStatus.SENT + assert running_campaign.sent_count == 1 + assert running_campaign.failed_count == 0 + mock_emit.assert_called_once() + + +class TestNotificationCampaignCancel: + + def test_cancel_sets_cancelled_status_and_completed_at(self, campaign): + assert campaign.completed_at is None + + campaign.cancel() + + campaign.refresh_from_db() + assert campaign.status == NotificationCampaignStatus.CANCELLED + assert campaign.completed_at is not None + + def test_cancel_does_not_overwrite_existing_completed_at(self, campaign): + completed_at = timezone.now() - timedelta(hours=1) + campaign.completed_at = completed_at + campaign.save(update_fields=['completed_at']) + + campaign.cancel() + + campaign.refresh_from_db() + assert campaign.status == NotificationCampaignStatus.CANCELLED + assert campaign.completed_at == completed_at + class TestProcessCampaignRetry: @@ -636,8 +685,10 @@ def test_process_campaign_retry_marks_completed_and_aggregates_stats(self, campa NotificationCampaignRecipient.objects.filter(campaign=campaign, user=skipped_user).update( status=NotificationCampaignRecipientStatus.SKIPPED ) + campaign.run_id = uuid.uuid4() + campaign.save(update_fields=['run_id']) - process_campaign_retry(campaign_id=campaign.id) + process_campaign_retry(campaign_id=campaign.id, run_id=campaign.run_id) campaign.refresh_from_db() assert campaign.status == NotificationCampaignStatus.COMPLETED @@ -646,6 +697,51 @@ def test_process_campaign_retry_marks_completed_and_aggregates_stats(self, campa assert campaign.failed_count == 1 assert campaign.completed_at is not None + def test_process_campaign_retry_skips_stale_run_id(self, campaign): + user = UserFactory() + create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=campaign.id) + NotificationCampaignRecipient.objects.filter(campaign=campaign).update( + status=NotificationCampaignRecipientStatus.SENT + ) + campaign.run_id = uuid.uuid4() + campaign.status = NotificationCampaignStatus.RUNNING + campaign.save() + + process_campaign_retry(campaign_id=campaign.id, run_id=uuid.uuid4()) + + campaign.refresh_from_db() + assert campaign.status == NotificationCampaignStatus.RUNNING + assert campaign.completed_at is None + assert campaign.sent_count == 0 + + @mock.patch('osf.email.notification_campaign.sentry.log_message') + def test_process_campaign_retry_keeps_cancelled_status_and_syncs_stats(self, mock_sentry, campaign): + sent_user = UserFactory() + failed_user = UserFactory() + create_campaign_recipients( + filters={'id__in': [sent_user.id, failed_user.id]}, + campaign_id=campaign.id, + ) + NotificationCampaignRecipient.objects.filter(campaign=campaign, user=sent_user).update( + status=NotificationCampaignRecipientStatus.SENT + ) + NotificationCampaignRecipient.objects.filter(campaign=campaign, user=failed_user).update( + status=NotificationCampaignRecipientStatus.FAILED + ) + campaign.run_id = uuid.uuid4() + campaign.status = NotificationCampaignStatus.CANCELLED + campaign.save() + + process_campaign_retry(campaign_id=campaign.id, run_id=campaign.run_id) + + campaign.refresh_from_db() + assert campaign.status == NotificationCampaignStatus.CANCELLED + assert campaign.recipient_count == 2 + assert campaign.sent_count == 1 + assert campaign.failed_count == 1 + assert campaign.completed_at is not None + mock_sentry.assert_called_once() + @mock.patch('osf.email.notification_campaign.chain') def test_process_campaign_retry_retries_failed_recipients(self, mock_chain, campaign): user = UserFactory() @@ -655,6 +751,7 @@ def test_process_campaign_retry_retries_failed_recipients(self, mock_chain, camp recipient.save(update_fields=['status']) campaign.run_id = uuid.uuid4() campaign.retries = 0 + campaign.status = NotificationCampaignStatus.RUNNING campaign.save() mock_chain.return_value.apply_async = mock.Mock() @@ -662,7 +759,7 @@ def test_process_campaign_retry_retries_failed_recipients(self, mock_chain, camp campaign.refresh_from_db() assert campaign.retries == 1 - assert campaign.status != NotificationCampaignStatus.PARTIALLY_COMPLETED + assert campaign.status == NotificationCampaignStatus.RUNNING mock_chain.assert_called_once() def test_process_campaign_retry_marks_partially_completed_after_max_retries(self, campaign): @@ -671,10 +768,11 @@ def test_process_campaign_retry_marks_partially_completed_after_max_retries(self recipient = NotificationCampaignRecipient.objects.get(campaign=campaign, user=user) recipient.status = NotificationCampaignRecipientStatus.FAILED recipient.save(update_fields=['status']) + campaign.run_id = uuid.uuid4() campaign.retries = 2 - campaign.save(update_fields=['retries']) + campaign.save() - process_campaign_retry(campaign_id=campaign.id) + process_campaign_retry(campaign_id=campaign.id, run_id=campaign.run_id) campaign.refresh_from_db() assert campaign.status == NotificationCampaignStatus.PARTIALLY_COMPLETED