From 3ea976461002bbbd6d4add3377e9f6d1c9d17103 Mon Sep 17 00:00:00 2001 From: Ostap-Zherebetskyi Date: Wed, 8 Jul 2026 20:31:24 +0300 Subject: [PATCH 01/25] [ENG-11583] Create Blank Notification Template (#11802) * Add blank notification type and template * Respond to CR --------- Co-authored-by: Longze Chen --- notifications.yaml | 7 +++++++ osf/models/notification_type.py | 1 + website/templates/blank.html.mako | 5 +++++ 3 files changed, 13 insertions(+) create mode 100644 website/templates/blank.html.mako diff --git a/notifications.yaml b/notifications.yaml index e3b7b286a30..6161abd9fc6 100644 --- a/notifications.yaml +++ b/notifications.yaml @@ -801,3 +801,10 @@ notification_types: object_content_type_model_name: abstractnode template: 'website/templates/empty.html.mako' tests: [] + + - name: blank + subject: 'PLACEHOLDER FOR EMAIL SUBJECT' + __docs__: ... + object_content_type_model_name: osfuser + template: 'website/templates/blank.html.mako' + tests: [] diff --git a/osf/models/notification_type.py b/osf/models/notification_type.py index f8162a08bce..9a6629e22e2 100644 --- a/osf/models/notification_type.py +++ b/osf/models/notification_type.py @@ -14,6 +14,7 @@ def get_default_frequency_choices(): class NotificationTypeEnum(str, Enum): EMPTY = 'empty' + BLANK = 'blank' # Desk notifications REVIEWS_SUBMISSION_STATUS = 'reviews_submission_status' ADDONS_BOA_JOB_FAILURE = 'addon_boa_job_failure' diff --git a/website/templates/blank.html.mako b/website/templates/blank.html.mako new file mode 100644 index 00000000000..f41301c1d1c --- /dev/null +++ b/website/templates/blank.html.mako @@ -0,0 +1,5 @@ +<%inherit file="notify_base.mako" /> + +<%def name="content()"> + + From aae133d4fe55ec1e27fe510f06bdb7f67fa2099a Mon Sep 17 00:00:00 2001 From: Ostap-Zherebetskyi Date: Thu, 9 Jul 2026 20:57:34 +0300 Subject: [PATCH 02/25] [ENG-11605][ENG-11695][ENG-11696][ENG-11697][ENG-11605][ENG-11698] ENTER - Part 1 (#11805) * Implement notification campaign models and email handling - Add NotificationCampaign and NotificationCampaignRecipient models. - Introduce notification campaign status management. - Update OSFUser model to track received notification campaigns. - Enhance email templates for notification campaigns. - Create migration for new models and relationships. * Remove notification settings table from blank email template * Code clean-up and fixes --------- Co-authored-by: Longze Chen --- admin/notifications/views.py | 11 +- .../notification_type_preview.html | 1 + osf/email/notification_campaign.py | 221 ++++++++++++++++++ ..._notificationcampaignrecipient_and_more.py | 53 +++++ osf/models/notification_campaign.py | 101 ++++++++ osf/models/user.py | 6 + website/settings/defaults.py | 2 + website/templates/blank.html.mako | 63 ++++- website/templates/notify_base.mako | 4 +- 9 files changed, 454 insertions(+), 8 deletions(-) create mode 100644 osf/email/notification_campaign.py create mode 100644 osf/migrations/0045_notificationcampaign_notificationcampaignrecipient_and_more.py create mode 100644 osf/models/notification_campaign.py diff --git a/admin/notifications/views.py b/admin/notifications/views.py index e1c55c05f47..e307b60e8b6 100644 --- a/admin/notifications/views.py +++ b/admin/notifications/views.py @@ -11,6 +11,7 @@ from mako.lexer import Lexer from mako.parsetree import ControlLine import re +from string import Formatter def delete_selected_notifications(selected_ids): NotificationSubscription.objects.filter(id__in=selected_ids).delete() @@ -82,7 +83,7 @@ def generate_mock_json(structure, list_name=None): return result -def build_safe_context(template: str) -> dict: +def build_safe_context(template: str, subject: str) -> dict: templatenode = Lexer(text=template).parse() identifiers_location = [] for node in templatenode.get_children(): @@ -103,6 +104,9 @@ def build_safe_context(template: str) -> dict: mock_json = generate_mock_json(identifier_structure) context = {identifier: f'mock_{identifier}' for identifier in flatten_identifiers if identifier not in TEMPLATE_IDENTIFIER_BLACKLIST} context.update(mock_json) + + # subject + context.update({key: key for _, key, _, _ in Formatter().parse(subject) if key}) return context class NotificationsList(PermissionRequiredMixin, ListView): @@ -282,12 +286,12 @@ def get_context_data(self, *args, **kwargs): return kwargs else: if notification_type.is_digest_type: - inner_context = build_safe_context(notification_type.template) + inner_context = build_safe_context(notification_type.template, notification_type.subject) inner_template = _render_email_html(notification_type, ctx=inner_context, return_original_error=True) safe_context = {'notifications': [inner_template]} return_context = inner_context else: - safe_context = build_safe_context(notification_type.template) + safe_context = build_safe_context(notification_type.template, notification_type.subject) return_context = safe_context if notification_type.is_digest_type: @@ -300,6 +304,7 @@ def get_context_data(self, *args, **kwargs): except Exception as e: kwargs['rendered_template'] = f"Error rendering template: {str(e)}" + kwargs['rendered_subject'] = notification_type.subject.format(**safe_context) kwargs['context'] = json.dumps(return_context, indent=4) return kwargs diff --git a/admin/templates/notifications/notification_type_preview.html b/admin/templates/notifications/notification_type_preview.html index e9aefbe3284..14c9d507c3a 100644 --- a/admin/templates/notifications/notification_type_preview.html +++ b/admin/templates/notifications/notification_type_preview.html @@ -1,5 +1,6 @@

Notification Template Preview

Rendered Template

+

Subject: {{ rendered_subject|safe }}

diff --git a/osf/email/notification_campaign.py b/osf/email/notification_campaign.py new file mode 100644 index 00000000000..322bf82e13b --- /dev/null +++ b/osf/email/notification_campaign.py @@ -0,0 +1,221 @@ +import logging +from osf.models import NotificationType, NotificationTypeEnum, OSFUser, UserActivityCounter +from django.db.models import Q +from django.db.models import OuterRef, Subquery, Exists, F +from django.db.models.functions import Coalesce +from framework.celery_tasks import app as celery_app +from celery import chord +from django.utils import timezone +from osf.models.notification_campaign import NotificationCampaign, NotificationCampaignRecipient, NotificationCampaignStatus, NotificationCampaignRecipientStatus + +logger = logging.getLogger(__name__) + + +counter_subquery = ( + UserActivityCounter.objects + .filter(_id=OuterRef('guids___id')) + .values('total')[:1] +) + + +def get_filtered_batches(filters, batch_size=1000, campaign_id=None): + already_sent_subquery = NotificationCampaignRecipient.objects.filter( + campaign_id=campaign_id, + user_id=OuterRef('pk'), + ) + + qs = ( + OSFUser.objects + .annotate(already_sent=Exists(already_sent_subquery)) + .filter(already_sent=False) + .filter(**filters) + .annotate(activity_total=Coalesce(Subquery(counter_subquery), 0)) + .order_by('-activity_total', '-date_registered', '-id') + ) + + last_total = None + last_date = None + last_id = None + + while True: + batch_qs = qs + + if last_total is not None: + batch_qs = batch_qs.filter( + Q(activity_total__lt=last_total) | + Q(activity_total=last_total, date_registered__lt=last_date) | + Q(activity_total=last_total, date_registered=last_date, id__lt=last_id) + ) + + batch = batch_qs[:batch_size] + + if not batch: + break + + rows = list(batch.values_list('id', 'activity_total', 'date_registered')) + + if not rows: + break + + batch_ids = [r[0] for r in rows] + + last_id, last_total, last_date = ( + rows[-1][0], + rows[-1][1], + rows[-1][2], + ) + + yield batch_ids + + +FILTER_PRESETS = { + 'all': {}, + 'active': {'is_active': True}, + 'internal': {'is_active': True, 'is_staff': True, 'username__endswith': '@cos.io'}, +} + +@celery_app.task(name='email.process_campaign_retry') +def process_campaign_retry(*args, **kwargs): + + campaign_id = kwargs.get('campaign_id') + campaign = NotificationCampaign.objects.get(id=campaign_id) + failed_recipients = NotificationCampaignRecipient.objects.filter(campaign=campaign, status=NotificationCampaignRecipientStatus.FAILED) + max_retries = campaign.metadata.get('execution', {}).get('max_retries', 3) + batch_size = campaign.metadata.get('execution', {}).get('batch_size', 1000) + failed_recipients_count = failed_recipients.count() + if not failed_recipients_count: + campaign.status = NotificationCampaignStatus.COMPLETED + campaign.completed_at = timezone.now() + campaign.failed_count = 0 + campaign.save(update_fields=['status', 'completed_at', 'failed_count']) + return + + if campaign.retries < max_retries: + logger.info(f'Retrying {failed_recipients_count} failed recipients for campaign {campaign_id}') + filters = {'id__in': failed_recipients.values_list('user_id', flat=True)} + tasks = [] + for batch in get_filtered_batches(filters=filters, batch_size=batch_size, campaign_id=campaign_id): + tasks.append( + send_campaign_batch.s( + notification_type_name=campaign.notification_type.name, + recipients_ids=batch, + context=campaign.metadata.get('context', {}), + campaign_id=campaign_id, + ) + ) + chord(tasks)( + process_campaign_retry.s(campaign_id=campaign_id) + ) + campaign.retries += 1 + campaign.save(update_fields=['retries']) + else: + campaign.failed_count = failed_recipients_count + campaign.status = NotificationCampaignStatus.PARTIALLY_COMPLETED + campaign.completed_at = timezone.now() + campaign.save(update_fields=['status', 'completed_at', 'failed_count']) + + +@celery_app.task(name='email.start_notification_campaign') +def start_notification_campaign(campaign_id): + campaign = NotificationCampaign.objects.get(id=campaign_id) + filters = campaign.metadata.get('filters', {}) + context = campaign.metadata.get('context', {}) + notification_type_name = campaign.notification_type.name + + if hasattr(NotificationTypeEnum, notification_type_name): + del getattr(NotificationTypeEnum, notification_type_name).instance + + if predefined_filter_name := filters.get('predefined'): + filters = FILTER_PRESETS.get(predefined_filter_name, {}) + + tasks = [] + total_recipients = 0 + batch_size = campaign.metadata.get('execution', {}).get('batch_size', 1000) + for batch in get_filtered_batches(filters=filters, batch_size=batch_size): + tasks.append( + send_campaign_batch.s( + notification_type_name=notification_type_name, + recipients_ids=batch, + context=context, + campaign_id=campaign_id, + ) + ) + total_recipients += len(batch) + + campaign.recipient_count = total_recipients + campaign.save(update_fields=['recipient_count']) + + chord(tasks)( + process_campaign_retry.s(campaign_id=campaign_id) + ) + + +@celery_app.task(name='email.send_campaign_batch', ignore_result=False) +def send_campaign_batch(context, recipients_ids, notification_type_name='blank', campaign_id=None): + campaign = NotificationCampaign.objects.get(id=campaign_id) + if campaign.status == NotificationCampaignStatus.CANCELLED: + logger.warning(f"Campaign {campaign_id} was cancelled") + return + if hasattr(NotificationTypeEnum, notification_type_name): + notification_type = getattr(NotificationTypeEnum, notification_type_name).instance + else: + notification_type = NotificationType.objects.filter( + name=notification_type_name + ).first() # TODO cache + + if notification_type is None: + return + + recipients_qs = OSFUser.objects.filter(id__in=recipients_ids) + recipient_records = { + 'to_create': [], + 'to_update': [], + } + existing = { + r.user_id: r + for r in NotificationCampaignRecipient.objects.filter( + campaign_id=campaign_id, + user_id__in=recipients_ids, + ) + } + success_count = 0 + failure_count = 0 + + for recipient in recipients_qs: + recipient_record = existing.get(recipient.id) + + if recipient_record is None: + recipient_record = NotificationCampaignRecipient( + campaign_id=campaign_id, + user=recipient, + ) + operation = 'to_create' + else: + operation = 'to_update' + + try: + notification_type.emit( + user=recipient, + event_context=context, + save=False, # Too many write operations + ) + + recipient_record.status = NotificationCampaignRecipientStatus.SENT + recipient_record.error_message = None + recipient_records[operation].append(recipient_record) + success_count += 1 + + except Exception as exc: + logger.error(exc) # TODO update error + recipient_record.status = NotificationCampaignRecipientStatus.FAILED + recipient_record.error_message = str(exc) + recipient_records[operation].append(recipient_record) + + failure_count += 1 + pass + + NotificationCampaignRecipient.objects.bulk_create(recipient_records['to_create']) + NotificationCampaignRecipient.objects.bulk_update(recipient_records['to_update'], ['status', 'error_message']) + + NotificationCampaign.objects.filter(pk=campaign_id).update(sent_count=F('sent_count') + success_count, failed_count=F('failed_count') + failure_count) + logger.info('Batch finished') # TODO: add/update logs diff --git a/osf/migrations/0045_notificationcampaign_notificationcampaignrecipient_and_more.py b/osf/migrations/0045_notificationcampaign_notificationcampaignrecipient_and_more.py new file mode 100644 index 00000000000..886ff3b29e9 --- /dev/null +++ b/osf/migrations/0045_notificationcampaign_notificationcampaignrecipient_and_more.py @@ -0,0 +1,53 @@ +# Generated by Django 4.2.26 on 2026-07-09 11:38 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('osf', '0044_notification_scheduled'), + ] + + operations = [ + migrations.CreateModel( + name='NotificationCampaign', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('created_at', models.DateTimeField(auto_now_add=True)), + ('updated_at', models.DateTimeField(auto_now=True)), + ('name', models.CharField(max_length=255)), + ('status', models.CharField(choices=[('created', 'Created'), ('running', 'Running'), ('completed', 'Completed'), ('partially_completed', 'Partially Completed'), ('failed', 'Failed'), ('cancelled', 'Cancelled'), ('ended', 'Ended')], default='created', max_length=20)), + ('started_at', models.DateTimeField(blank=True, null=True)), + ('completed_at', models.DateTimeField(blank=True, null=True)), + ('metadata', models.JSONField(blank=True, default=dict)), + ('recipient_count', models.PositiveIntegerField(default=0)), + ('sent_count', models.PositiveIntegerField(default=0)), + ('failed_count', models.PositiveIntegerField(default=0)), + ('retries', models.PositiveIntegerField(default=0)), + ('created_by', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL)), + ('notification_type', models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, to='osf.notificationtype')), + ], + ), + migrations.CreateModel( + name='NotificationCampaignRecipient', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('updated_at', models.DateTimeField(auto_now=True)), + ('status', models.CharField(choices=[('pending', 'Pending'), ('sent', 'Sent'), ('failed', 'Failed')], db_index=True, default='pending', max_length=20)), + ('error_message', models.TextField(blank=True, null=True)), + ('campaign', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='recipients', to='osf.notificationcampaign')), + ('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)), + ], + options={ + 'unique_together': {('campaign', 'user')}, + }, + ), + migrations.AddField( + model_name='osfuser', + name='received_notification_campaigns', + field=models.ManyToManyField(through='osf.NotificationCampaignRecipient', to='osf.notificationcampaign'), + ), + ] diff --git a/osf/models/notification_campaign.py b/osf/models/notification_campaign.py new file mode 100644 index 00000000000..8c5aa22a3ab --- /dev/null +++ b/osf/models/notification_campaign.py @@ -0,0 +1,101 @@ +from django.db import models +from django.utils import timezone + + +class NotificationCampaignStatus(models.TextChoices): + CREATED = 'created', 'Created' + RUNNING = 'running', 'Running' + COMPLETED = 'completed', 'Completed' + PARTIALLY_COMPLETED = 'partially_completed', 'Partially Completed' + FAILED = 'failed', 'Failed' + CANCELLED = 'cancelled', 'Cancelled' + ENDED = 'ended', 'Ended' + +class NotificationCampaignRecipientStatus(models.TextChoices): + PENDING = 'pending', 'Pending' + SENT = 'sent', 'Sent' + FAILED = 'failed', 'Failed' + + +class NotificationCampaign(models.Model): + created_at = models.DateTimeField(auto_now_add=True) + updated_at = models.DateTimeField(auto_now=True) + + name = models.CharField(max_length=255) + + notification_type = models.ForeignKey( + 'NotificationType', + on_delete=models.PROTECT, + ) + created_by = models.ForeignKey( + 'OSFUser', + null=True, + on_delete=models.SET_NULL, + ) + + status = models.CharField( + max_length=20, + choices=NotificationCampaignStatus.choices, + default=NotificationCampaignStatus.CREATED, + ) + + started_at = models.DateTimeField(null=True, blank=True) + completed_at = models.DateTimeField(null=True, blank=True) + + metadata = models.JSONField(default=dict, blank=True) + + # metadata structure: + # { + # "filters": { + # ... + # }, + # "context": { + # ... + # }, + # "execution": { + # "batch_size": , + # "max_retries": , + # }, + # "template": , + # } + + recipient_count = models.PositiveIntegerField(default=0) + sent_count = models.PositiveIntegerField(default=0) + failed_count = models.PositiveIntegerField(default=0) + retries = models.PositiveIntegerField(default=0) + + def start(self): + from osf.email.notification_campaign import start_notification_campaign + self.status = NotificationCampaignStatus.RUNNING + self.started_at = timezone.now() + + self.sent_count = 0 + self.failed_count = 0 + self.recipient_count = 0 + self.retries = 0 + self.metadata.update({'template': self.notification_type.template}) + self.save() + start_notification_campaign.delay(campaign_id=self.id) + + +class NotificationCampaignRecipient(models.Model): + campaign = models.ForeignKey( + NotificationCampaign, + on_delete=models.CASCADE, + related_name='recipients', + ) + user = models.ForeignKey( + 'OSFUser', + on_delete=models.CASCADE, + ) + updated_at = models.DateTimeField(auto_now=True) + status = models.CharField( + max_length=20, + choices=NotificationCampaignRecipientStatus.choices, + default=NotificationCampaignRecipientStatus.PENDING, + db_index=True, + ) + error_message = models.TextField(null=True, blank=True) + + class Meta: + unique_together = ('campaign', 'user') diff --git a/osf/models/user.py b/osf/models/user.py index 5739813cd48..b1e95010278 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -58,6 +58,7 @@ from .session import UserSessionMap from .tag import Tag from .validators import validate_email, validate_social, validate_history_item, has_domain_in_user_fields_for_names +from .notification_campaign import NotificationCampaign, NotificationCampaignRecipient from osf.utils.datetime_aware_jsonfield import DateTimeAwareJSONField from osf.utils.fields import NonNaiveDateTimeField, LowercaseEmailField, ensure_str from osf.utils.names import impute_names @@ -398,6 +399,11 @@ class OSFUser(DirtyFieldsMixin, GuidMixin, BaseModel, AbstractBaseUser, Permissi notifications_configured = DateTimeAwareJSONField(default=dict, blank=True) + received_notification_campaigns = models.ManyToManyField( + NotificationCampaign, + through=NotificationCampaignRecipient, + ) + # The time at which the user agreed to our updated ToS and Privacy Policy (GDPR, 25 May 2018) accepted_terms_of_service = NonNaiveDateTimeField(null=True, blank=True) diff --git a/website/settings/defaults.py b/website/settings/defaults.py index dfc78bb07ab..f87c293f245 100644 --- a/website/settings/defaults.py +++ b/website/settings/defaults.py @@ -483,6 +483,7 @@ class CeleryConfig: 'api.share.utils', 'scripts.check_manual_restart_approval', 'scripts.enhanced_stuck_registration_audit', + 'osf.email.notification_campaign', } background_migration_modules = { @@ -604,6 +605,7 @@ class CeleryConfig: 'scripts.disable_removed_beat_tasks', 'osf.management.commands.delete_withdrawn_or_failed_registration_files', 'osf.management.commands.migrate_osfmetrics_fix_6to8', + 'osf.email.notification_campaign', ) # Modules that need metrics and release requirements diff --git a/website/templates/blank.html.mako b/website/templates/blank.html.mako index f41301c1d1c..ced2e33c787 100644 --- a/website/templates/blank.html.mako +++ b/website/templates/blank.html.mako @@ -1,5 +1,62 @@ -<%inherit file="notify_base.mako" /> + + + + + + + -<%def name="content()"> +<%page args=" + domain='', + osf_contact_email='support@osf.io' +"/> - + + + + + + + + + + + + + + + +
+ OSF +
+ + + + +
+
+ + + + +
+

+ Copyright © 2026 + Center For Open Science, All rights reserved. | + + Privacy Policy + +

+

+ Questions? + ${osf_contact_email} +

+
+
+ + diff --git a/website/templates/notify_base.mako b/website/templates/notify_base.mako index 816ae3fbcf0..fde50148660 100644 --- a/website/templates/notify_base.mako +++ b/website/templates/notify_base.mako @@ -23,7 +23,7 @@ node_absolute_url=None, notification_settings_url=None, osf_contact_email='support@osf.io', - year=2025 + year=2026 "/>

- Copyright © 2025 + Copyright © 2026 Center For Open Science, All rights reserved. | Privacy Policy

From d1048ecdbfdd447aac74e909ac3adf9b397d0773 Mon Sep 17 00:00:00 2001 From: Ostap-Zherebetskyi Date: Fri, 10 Jul 2026 18:19:19 +0300 Subject: [PATCH 03/25] [ENG-11698] Add bulk email sending with SendGrid to send_campaign_batch (#11806) --- osf/email/notification_campaign.py | 63 ++++++++++++++++-------------- 1 file changed, 34 insertions(+), 29 deletions(-) diff --git a/osf/email/notification_campaign.py b/osf/email/notification_campaign.py index 322bf82e13b..941c02a88a8 100644 --- a/osf/email/notification_campaign.py +++ b/osf/email/notification_campaign.py @@ -7,6 +7,7 @@ from celery import chord from django.utils import timezone from osf.models.notification_campaign import NotificationCampaign, NotificationCampaignRecipient, NotificationCampaignStatus, NotificationCampaignRecipientStatus +from osf.email import send_email_with_send_grid logger = logging.getLogger(__name__) @@ -180,39 +181,43 @@ def send_campaign_batch(context, recipients_ids, notification_type_name='blank', } success_count = 0 failure_count = 0 + if campaign.metadata.get('sendgrid_bulk', False): + recipient_emails = list(recipients_qs.values_list('username', flat=True)) + send_email_with_send_grid(to_addr=recipient_emails, notification_type=notification_type, context=context) + success_count = len(recipient_emails) + else: + for recipient in recipients_qs: + recipient_record = existing.get(recipient.id) - for recipient in recipients_qs: - recipient_record = existing.get(recipient.id) - - if recipient_record is None: - recipient_record = NotificationCampaignRecipient( - campaign_id=campaign_id, - user=recipient, - ) - operation = 'to_create' - else: - operation = 'to_update' - - try: - notification_type.emit( - user=recipient, - event_context=context, - save=False, # Too many write operations - ) + if recipient_record is None: + recipient_record = NotificationCampaignRecipient( + campaign_id=campaign_id, + user=recipient, + ) + operation = 'to_create' + else: + operation = 'to_update' + + try: + notification_type.emit( + user=recipient, + event_context=context, + save=False, # Too many write operations + ) - recipient_record.status = NotificationCampaignRecipientStatus.SENT - recipient_record.error_message = None - recipient_records[operation].append(recipient_record) - success_count += 1 + recipient_record.status = NotificationCampaignRecipientStatus.SENT + recipient_record.error_message = None + recipient_records[operation].append(recipient_record) + success_count += 1 - except Exception as exc: - logger.error(exc) # TODO update error - recipient_record.status = NotificationCampaignRecipientStatus.FAILED - recipient_record.error_message = str(exc) - recipient_records[operation].append(recipient_record) + except Exception as exc: + logger.error(exc) # TODO update error + recipient_record.status = NotificationCampaignRecipientStatus.FAILED + recipient_record.error_message = str(exc) + recipient_records[operation].append(recipient_record) - failure_count += 1 - pass + failure_count += 1 + pass NotificationCampaignRecipient.objects.bulk_create(recipient_records['to_create']) NotificationCampaignRecipient.objects.bulk_update(recipient_records['to_update'], ['status', 'error_message']) From fd6f7783ed7321c5a89329cbfbc5f4f31771b0e9 Mon Sep 17 00:00:00 2001 From: Ostap-Zherebetskyi Date: Wed, 15 Jul 2026 18:35:24 +0300 Subject: [PATCH 04/25] [ENG-11702][ENG-11694][ENG-11699][ENG-11703] Notification Campaign Admin Page (#11808) * Add Notification Campaign management features including forms, views, and templates * Add support for restarting failed notification campaigns and update progress metrics * Prevent updating recipient count on campaign restart --- admin/notifications/forms.py | 42 +- admin/notifications/urls.py | 5 + admin/notifications/views.py | 285 +++++++++++- admin/templates/base.html | 5 +- .../notification_campaigns_detail.html | 300 ++++++++++++ .../notification_campaigns_list.html | 46 ++ .../notification_campaing_create.html | 427 ++++++++++++++++++ osf/email/notification_campaign.py | 73 ++- osf/models/__init__.py | 1 + osf/models/notification_campaign.py | 10 +- 10 files changed, 1158 insertions(+), 36 deletions(-) create mode 100644 admin/templates/notifications/notification_campaigns_detail.html create mode 100644 admin/templates/notifications/notification_campaigns_list.html create mode 100644 admin/templates/notifications/notification_campaing_create.html diff --git a/admin/notifications/forms.py b/admin/notifications/forms.py index 946754415bb..98ad5c467a2 100644 --- a/admin/notifications/forms.py +++ b/admin/notifications/forms.py @@ -1,8 +1,48 @@ from django import forms -from osf.models import NotificationType +from osf.models import NotificationType, NotificationCampaign +import json class NotificationTypeForm(forms.ModelForm): class Meta: model = NotificationType fields = '__all__' + + +class NotificationCampaignCreateForm(forms.ModelForm): + context = forms.CharField( + required=False, + widget=forms.Textarea(attrs={'rows': 8}), + initial='{}', + ) + + filters = forms.CharField( + required=False, + widget=forms.HiddenInput(), + initial='{}', + ) + + batch_size = forms.IntegerField( + min_value=1, + initial=1000, + ) + + max_retries = forms.IntegerField( + min_value=0, + initial=3, + ) + + class Meta: + model = NotificationCampaign + fields = ( + 'name', + 'notification_type', + ) + + def clean_context(self): + value = self.cleaned_data['context'] or '{}' + return json.loads(value) + + def clean_filters(self): + value = self.cleaned_data['filters'] or '{}' + return json.loads(value) diff --git a/admin/notifications/urls.py b/admin/notifications/urls.py index 236059a577e..a05e60ee284 100644 --- a/admin/notifications/urls.py +++ b/admin/notifications/urls.py @@ -11,4 +11,9 @@ re_path(r'types_preview/(?P\d+)/$', views.NotificationTypePreview.as_view(), name='types_preview'), re_path(r'subscriptions/$', views.NotificationSubscriptionsList.as_view(), name='subscriptions_list'), re_path(r'email_tasks/$', views.EmailTasksList.as_view(), name='email_tasks_list'), + re_path(r'notification_campaigns_list/$', views.NotificationCampaignsList.as_view(), name='notification_campaigns_list'), + re_path(r'notification_campaigns_detail/(?P\d+)/$', views.NotificationCampaignDetail.as_view(), name='notification_campaigns_detail'), + re_path(r'notification_campaigns_create/$', views.NotificationCampaignCreateView.as_view(), name='notification_campaigns_create'), + re_path(r'notification_campaigns_recipients_preview/$', views.NotificationCampaignsRecipientsPreview.as_view(), name='notification_campaigns_recipients_preview'), + re_path(r'notification_campaigns_start/(?P\d+)/$', views.StartNotificationCampaign.as_view(), name='notification_campaigns_start'), ] diff --git a/admin/notifications/views.py b/admin/notifications/views.py index e307b60e8b6..0627063259e 100644 --- a/admin/notifications/views.py +++ b/admin/notifications/views.py @@ -1,17 +1,23 @@ +import re +import json +from collections import defaultdict from django.urls import reverse_lazy from django.db.models import Q -from osf.models import NotificationSubscription, NotificationType, Notification, EmailTask -from django.views.generic import ListView, DetailView, UpdateView +from django.db import models +from django.shortcuts import get_object_or_404, redirect +from django.views.generic import ListView, DetailView, UpdateView, CreateView, View +from django.contrib import messages from django.contrib.auth.mixins import PermissionRequiredMixin +from osf.models import NotificationSubscription, NotificationType, Notification, EmailTask, NotificationCampaign, OSFUser +from osf.models.notification_campaign import NotificationCampaignStatus from django.forms.models import model_to_dict -from .forms import NotificationTypeForm -from osf.email import _render_email_html -import json -from collections import defaultdict +from .forms import NotificationTypeForm, NotificationCampaignCreateForm from mako.lexer import Lexer from mako.parsetree import ControlLine -import re from string import Formatter +from osf.email import _render_email_html +from osf.email.notification_campaign import FILTER_PRESETS, filter_users + def delete_selected_notifications(selected_ids): NotificationSubscription.objects.filter(id__in=selected_ids).delete() @@ -332,3 +338,268 @@ class NotificationTypeChangeForm(PermissionRequiredMixin, UpdateView): def get_success_url(self, *args, **kwargs): return reverse_lazy('notifications:type_display', kwargs={'pk': self.kwargs.get('pk')}) + + +class NotificationCampaignsList(PermissionRequiredMixin, ListView): + paginate_by = 25 + template_name = 'notifications/notification_campaigns_list.html' + ordering = 'name' + permission_required = 'osf.view_notificationcampaign' + raise_exception = True + model = NotificationCampaign + + def get_queryset(self): + qs = NotificationCampaign.objects.all().order_by(self.ordering) + q = self.request.GET.get('q') + if q: + qs = qs.filter( + Q(name__icontains=q) | + Q(status__icontains=q) | + Q(notification_type__name__icontains=q) + ) + return qs + + def get_context_data(self, **kwargs): + context = super().get_context_data(**kwargs) + q = self.request.GET.get('q', '') + context['q'] = q + # append search param to pagination links + if q: + context['extra_query_params'] = f"&q={q}" + else: + context['extra_query_params'] = '' + + context['notification_campaigns'] = context['object_list'] + context['page'] = context['page_obj'] + return context + + +class NotificationCampaignDetail(PermissionRequiredMixin, DetailView): + model = NotificationCampaign + template_name = 'notifications/notification_campaigns_detail.html' + permission_required = 'osf.change_notificationcampaign' + raise_exception = True + + def get_object(self, queryset=None): + return NotificationCampaign.objects.get(id=self.kwargs.get('pk')) + + def get_context_data(self, *args, **kwargs): + notification_campaign = self.get_object() + metadata = notification_campaign.metadata or {} + + context = { + 'notification_campaign': notification_campaign, + 'display_fields': [ + ('Name', notification_campaign.name), + ('Notification Type', notification_campaign.notification_type), + ('Created By', notification_campaign.created_by), + ('Status', notification_campaign.get_status_display()), + ('Recipients', notification_campaign.recipient_count), + ('Sent', notification_campaign.sent_count), + ('Failed', notification_campaign.failed_count), + ('Retries', notification_campaign.retries), + ('Created', notification_campaign.created_at), + ('Started', notification_campaign.started_at), + ('Completed', notification_campaign.completed_at), + ], + 'template': notification_campaign.notification_type.template, + 'metadata': metadata, + 'filters_json': json.dumps(notification_campaign.metadata['filters']), + 'sent_filters_json': json.dumps({ + 'manual': [ + {'field': 'notificationcampaignrecipient', 'value': notification_campaign.id, 'lookup': 'campaign'}, + {'field': 'notificationcampaignrecipient', 'value': 'sent', 'lookup': 'status'} + ] + }), + 'failed_filters_json': json.dumps({ + 'manual': [ + {'field': 'notificationcampaignrecipient', 'value': notification_campaign.id, 'lookup': 'campaign'}, + {'field': 'notificationcampaignrecipient', 'value': 'failed', 'lookup': 'status'} + ] + }), + 'other_metadata': { + k: v + for k, v in metadata.items() + if k not in {'filters', 'context', 'execution', 'template'} + }, + } + + if notification_campaign.status == NotificationCampaignStatus.RUNNING: + context.update({ + 'sent_percent': notification_campaign.sent_count * 100 / notification_campaign.recipient_count if notification_campaign.recipient_count else 0, + 'failed_percent': notification_campaign.failed_count * 100 / notification_campaign.recipient_count if notification_campaign.recipient_count else 0, + }) + + return context + + +LOOKUPS = { + models.CharField: { + 'exact': 'Equals', + 'iexact': 'Equals (case insensitive)', + 'contains': 'Contains', + 'icontains': 'Contains (case insensitive)', + 'startswith': 'Starts with', + 'istartswith': 'Starts with (case insensitive)', + 'endswith': 'Ends with', + 'iendswith': 'Ends with (case insensitive)', + 'in': 'In', + 'isnull': 'Is empty', + }, + models.TextField: { + 'exact': 'Equals', + 'iexact': 'Equals (case insensitive)', + 'contains': 'Contains', + 'icontains': 'Contains (case insensitive)', + 'startswith': 'Starts with', + 'istartswith': 'Starts with (case insensitive)', + 'endswith': 'Ends with', + 'iendswith': 'Ends with (case insensitive)', + 'isnull': 'Is empty', + }, + models.IntegerField: { + 'exact': 'Equals', + 'gt': 'Greater than', + 'gte': 'Greater than or equal to', + 'lt': 'Less than', + 'lte': 'Less than or equal to', + 'in': 'In', + 'isnull': 'Is empty', + }, + models.DateField: { + 'exact': 'On', + 'gt': 'After', + 'gte': 'On or after', + 'lt': 'Before', + 'lte': 'On or before', + 'isnull': 'Is empty', + }, + models.DateTimeField: { + 'exact': 'On', + 'gt': 'After', + 'gte': 'On or after', + 'lt': 'Before', + 'lte': 'On or before', + 'isnull': 'Is empty', + }, + models.BooleanField: { + 'exact': 'Is', + }, +} + + +class NotificationCampaignCreateView(CreateView): + model = NotificationCampaign + form_class = NotificationCampaignCreateForm + template_name = 'notifications/notification_campaing_create.html' + allowed_filters = [ + 'is_active', + 'is_staff', + 'username', + 'last_login', + ] + + def form_valid(self, form): + form.instance.created_by = self.request.user + + form.instance.metadata = { + 'filters': form.cleaned_data['filters'], + 'context': form.cleaned_data['context'], + 'execution': { + 'batch_size': form.cleaned_data['batch_size'], + 'max_retries': form.cleaned_data['max_retries'], + }, + } + + response = super().form_valid(form) + + messages.success( + self.request, + 'Notification campaign created successfully.', + ) + + return response + + def get_success_url(self): + return reverse_lazy( + 'notifications:notification_campaigns_detail', + kwargs={'pk': self.object.pk}, + ) + + def get_context_data(self, **kwargs): + context = super().get_context_data(**kwargs) + context['notification_types'] = NotificationType.objects.order_by('name') + + filter_fields = {} + for field in [f for f in OSFUser._meta.get_fields() if f.name in self.allowed_filters]: + if not field.concrete: + continue + if type(field) not in LOOKUPS.keys(): + continue + filter_fields[field.name] = { + 'label': field.verbose_name, + 'type': field.get_internal_type().lower(), + 'lookups': LOOKUPS.get(type(field), {}) + } + context['filter_fields'] = filter_fields + context['filters'] = [] + context['predefined_filters'] = FILTER_PRESETS.keys() + return context + + +class NotificationCampaignsRecipientsPreview(PermissionRequiredMixin, ListView): + template_name = 'users/list.html' + permission_required = 'osf.view_osfuser' + raise_exception = True + paginate_by = 25 + + def get_queryset(self): + filters = {} + raw_filters = self.request.GET.get('filters', None) + if raw_filters: + json_filters = json.loads(raw_filters) + if predefined := json_filters.get('predefined'): + filters = FILTER_PRESETS.get(predefined, {}) + else: + filters = { + f'{item["field"]}__{item["lookup"]}': item['value'] + for item in json_filters.get('manual', []) + } + + return filter_users(filters) + + def get_context_data(self, **kwargs): + users = self.get_queryset() + + page_size = self.get_paginate_by(users) + paginator, page, query_set, is_paginated = self.paginate_queryset( + users, + page_size, + ) + # append search param to pagination links + kwargs.update({'extra_query_params': f'&filters={self.request.GET.get("filters")}'}) + return super().get_context_data( + **kwargs, + page=page, + users=query_set, + paginator=paginator, + is_paginated=is_paginated, + ) + +class StartNotificationCampaign(PermissionRequiredMixin, View): + permission_required = 'osf.change_notificationtype' + + def post(self, request, *args, **kwargs): + notification_campaign = get_object_or_404( + NotificationCampaign, + pk=kwargs['pk'], + ) + + restart_failed = request.GET.get('restart_failed') == 'true' + + notification_campaign.start(restart_failed=restart_failed) + + return redirect( + 'notifications:notification_campaigns_detail', + pk=notification_campaign.pk, + ) diff --git a/admin/templates/base.html b/admin/templates/base.html index 5f645ffe267..4554c0ff1b5 100644 --- a/admin/templates/base.html +++ b/admin/templates/base.html @@ -289,7 +289,7 @@ {% endif %} {% endif %} - {% if perms.osf.view_notification or perms.osf.view_notificationtype or perms.osf.view_notificationsubscription %} + {% if perms.osf.view_notification or perms.osf.view_notificationtype or perms.osf.view_notificationsubscription or perms.osf.view_notificationcampaign %}
  • Notifications
  • @@ -307,6 +307,9 @@ {% if perms.osf.view_emailtask %}
  • Email Tasks
  • {% endif %} + {% if perms.osf.view_notificationcampaign %} +
  • Notification Campaigns
  • + {% endif %} diff --git a/admin/templates/notifications/notification_campaigns_detail.html b/admin/templates/notifications/notification_campaigns_detail.html new file mode 100644 index 00000000000..fd520e77f79 --- /dev/null +++ b/admin/templates/notifications/notification_campaigns_detail.html @@ -0,0 +1,300 @@ +{% extends "base.html" %} +{% load static %} +{% load render_bundle from webpack_loader %} + +{% block title %} + {{ notification_campaign.name }} +{% endblock title %} + +{% block content %} +
    +
    +
    +

    {{ notification_campaign.name }}

    +

    + Campaign #{{ notification_campaign.id }} +

    +
    +
    + {% if notification_campaign.status == 'running' %} +

    Progress

    + +
    +
    + {{ notification_campaign.sent_count }} +
    +
    + {{ notification_campaign.failed_count }} +
    +
    +

    + {{ notification_campaign.sent_count|add:notification_campaign.failed_count }}/{{ notification_campaign.recipient_count }} processed +

    + {% endif %} +
    + {% csrf_token %} + +
    + + +
    +
    +

    General

    + + {% for field, value in display_fields %} + + + + {% if field == 'Sent' and value != 0 %} + + + {% elif field == 'Failed' and value != 0 %} + + + {% else %} + + + + {% endif %} + + {% endfor %} +
    {{ field }}{{ value|safe }} + + Preview Recipients + + + + Preview Recipients + + +
    + {% csrf_token %} + +
    +
    +
    +
    + + + {% if metadata.filters %} +
    +
    +

    Recipient Filters

    + + {% if not "predefined" in metadata.filters %} + + + + + + + + + + + {% for filter in metadata.filters.manual %} + + + + + + {% empty %} + + + + {% endfor %} + +
    FieldLookupValue
    {{ filter.field }}{{ filter.lookup }}{{ filter.value }}
    + No filters configured. +
    + + {% elif "predefined" in metadata.filters %} + + + + + + +
    Predefined Filter{{ metadata.filters.predefined }}
    + + {% endif %} + + + Preview Recipients + + +
    +
    + {% endif %} + + + {% if metadata.context %} +
    +
    +

    Context

    + + {% for key, value in metadata.context.items %} + + + + + {% endfor %} +
    {{ key }}
    {{ value }}
    +
    +
    + {% endif %} + + + {% if metadata.execution %} +
    +
    +

    Execution

    + + {% for key, value in metadata.execution.items %} + + + + + {% endfor %} +
    {{ key }}{{ value }}
    +
    +
    + {% endif %} + + {% if template or metadata.template %} +
    +
    +

    Email Template

    + + + +
    + + {% if template %} +
    +
    {{ template }}
    +
    + {% endif %} + + {% if metadata.template %} +
    +
    {{ metadata.template }}
    +
    + {% endif %} + +
    +
    +
    + {% endif %} + + + {% if other_metadata %} +
    +
    +

    Additional Metadata

    + + {% for key, value in other_metadata.items %} + + + + + {% endfor %} +
    {{ key }}
    {{ value }}
    +
    +
    + {% endif %} + +
    +{% endblock content %} + +{% block bottom_js %} + +{% endblock %} diff --git a/admin/templates/notifications/notification_campaigns_list.html b/admin/templates/notifications/notification_campaigns_list.html new file mode 100644 index 00000000000..494e4584045 --- /dev/null +++ b/admin/templates/notifications/notification_campaigns_list.html @@ -0,0 +1,46 @@ +{% extends "base.html" %} +{% load render_bundle from webpack_loader %} +{% load static %} +{% block title %} + List of Notification Types +{% endblock title %} +{% block content %} +

    List of Notification Campaigns

    +
    +
    + +
    +
    + + {% include "util/pagination.html" with items=page status=status %} +
    + + +
    + + + + + + + + + + + + {% for notification_capmaign in notification_campaigns %} + + + + + + + + + {% endfor %} + +
    NameNotification TypeStatusStarted atCompleted at
    {{ notification_capmaign.name }}{{ notification_capmaign.notification_type.name }}{{ notification_capmaign.status }}{{ notification_capmaign.started_at }}{{ notification_capmaign.completed_at }}
    + +{% endblock content %} diff --git a/admin/templates/notifications/notification_campaing_create.html b/admin/templates/notifications/notification_campaing_create.html new file mode 100644 index 00000000000..bc274bdf4cc --- /dev/null +++ b/admin/templates/notifications/notification_campaing_create.html @@ -0,0 +1,427 @@ +{% extends "base.html" %} +{% load static %} +{% load render_bundle from webpack_loader %} + +{% block title %} + Create Notification Campaign +{% endblock title %} + +{% block content %} +
    + +
    +
    +

    Create Notification Campaign

    +
    +
    + +
    + {% csrf_token %} + + +
    +
    +

    General

    + + + + + + + + + + +
    Campaign Name + +
    Notification Type + +
    +
    +
    + + +
    +
    +

    Recipient Filters

    + +
    + + + +
    + +
    + +
    + + + +
    + + + + + + {{ filter_fields|json_script:"filter-fields" }} + {{ filters|json_script:"initial-filters" }} + +
    +
    + +
    +
    +

    Template Context (JSON)

    + + +
    +
    + + +
    +
    +

    Execution

    + + + + + + + + + + + +
    Batch Size + +
    Max Retries + +
    +
    +
    + + +
    +
    + +
    +
    + +
    + + + +
    +
    + +
    +{% endblock content %} + +{% block bottom_js %} + +{% endblock %} diff --git a/osf/email/notification_campaign.py b/osf/email/notification_campaign.py index 941c02a88a8..e88c41cc01b 100644 --- a/osf/email/notification_campaign.py +++ b/osf/email/notification_campaign.py @@ -1,7 +1,6 @@ import logging -from osf.models import NotificationType, NotificationTypeEnum, OSFUser, UserActivityCounter -from django.db.models import Q -from django.db.models import OuterRef, Subquery, Exists, F +from osf.models import NotificationType, NotificationTypeEnum, OSFUser, UserActivityCounter, Email +from django.db.models import OuterRef, Subquery, Exists, F, Q, Case, When, CharField from django.db.models.functions import Coalesce from framework.celery_tasks import app as celery_app from celery import chord @@ -12,27 +11,45 @@ logger = logging.getLogger(__name__) +first_email_subquery = ( + Email.objects + .filter(user=OuterRef('pk')) + .values('address')[:1] +) + + counter_subquery = ( UserActivityCounter.objects .filter(_id=OuterRef('guids___id')) .values('total')[:1] ) +def filter_users(filters, campaign_id=None, restart_failed=False): + qs = OSFUser.objects.all() + if campaign_id: + if restart_failed: + already_sent_subquery = NotificationCampaignRecipient.objects.filter( + campaign_id=campaign_id, + user_id=OuterRef('pk'), + status__in=['sent', 'pending'] + ) + else: + already_sent_subquery = NotificationCampaignRecipient.objects.filter( + campaign_id=campaign_id, + user_id=OuterRef('pk'), + ) -def get_filtered_batches(filters, batch_size=1000, campaign_id=None): - already_sent_subquery = NotificationCampaignRecipient.objects.filter( - campaign_id=campaign_id, - user_id=OuterRef('pk'), - ) + qs = OSFUser.objects.annotate(already_sent=Exists(already_sent_subquery)).filter(already_sent=False) - qs = ( - OSFUser.objects - .annotate(already_sent=Exists(already_sent_subquery)) - .filter(already_sent=False) - .filter(**filters) - .annotate(activity_total=Coalesce(Subquery(counter_subquery), 0)) - .order_by('-activity_total', '-date_registered', '-id') - ) + qs = qs.filter(**filters) + + return qs + + +def get_filtered_batches(filters, batch_size=1000, campaign_id=None, restart_failed=False): + qs = filter_users(filters, campaign_id, restart_failed=restart_failed) + + qs = qs.annotate(activity_total=Coalesce(Subquery(counter_subquery), 0)).order_by('-activity_total', '-date_registered', '-id') last_total = None last_date = None @@ -117,7 +134,7 @@ def process_campaign_retry(*args, **kwargs): @celery_app.task(name='email.start_notification_campaign') -def start_notification_campaign(campaign_id): +def start_notification_campaign(campaign_id, restart_failed=False): campaign = NotificationCampaign.objects.get(id=campaign_id) filters = campaign.metadata.get('filters', {}) context = campaign.metadata.get('context', {}) @@ -128,11 +145,16 @@ def start_notification_campaign(campaign_id): if predefined_filter_name := filters.get('predefined'): filters = FILTER_PRESETS.get(predefined_filter_name, {}) + else: + filters = { + f'{item["field"]}__{item["lookup"]}': item['value'] + for item in filters.get('manual', []) + } tasks = [] total_recipients = 0 batch_size = campaign.metadata.get('execution', {}).get('batch_size', 1000) - for batch in get_filtered_batches(filters=filters, batch_size=batch_size): + for batch in get_filtered_batches(filters=filters, batch_size=batch_size, campaign_id=campaign_id, restart_failed=restart_failed): tasks.append( send_campaign_batch.s( notification_type_name=notification_type_name, @@ -142,9 +164,9 @@ def start_notification_campaign(campaign_id): ) ) total_recipients += len(batch) - - campaign.recipient_count = total_recipients - campaign.save(update_fields=['recipient_count']) + if not restart_failed: + campaign.recipient_count = total_recipients + campaign.save(update_fields=['recipient_count']) chord(tasks)( process_campaign_retry.s(campaign_id=campaign_id) @@ -182,7 +204,14 @@ def send_campaign_batch(context, recipients_ids, notification_type_name='blank', success_count = 0 failure_count = 0 if campaign.metadata.get('sendgrid_bulk', False): - recipient_emails = list(recipients_qs.values_list('username', flat=True)) + recipients_qs_annotated = recipients_qs.annotate( + recipient_address=Case( + When(username__contains='@', then='username'), + default=Subquery(first_email_subquery), + output_field=CharField(), + ) + ) + recipient_emails = list(recipients_qs_annotated.values_list('recipient_address', flat=True)) send_email_with_send_grid(to_addr=recipient_emails, notification_type=notification_type, context=context) success_count = len(recipient_emails) else: diff --git a/osf/models/__init__.py b/osf/models/__init__.py index 918ca9aa009..90284dd0d2e 100644 --- a/osf/models/__init__.py +++ b/osf/models/__init__.py @@ -67,6 +67,7 @@ from .notification_subscription import NotificationSubscription from .notification_type import NotificationType, NotificationTypeEnum from .notification import Notification +from .notification_campaign import NotificationCampaign, NotificationCampaignRecipient from .oauth import ( ApiOAuth2Application, diff --git a/osf/models/notification_campaign.py b/osf/models/notification_campaign.py index 8c5aa22a3ab..9cce5c3b966 100644 --- a/osf/models/notification_campaign.py +++ b/osf/models/notification_campaign.py @@ -64,18 +64,18 @@ class NotificationCampaign(models.Model): failed_count = models.PositiveIntegerField(default=0) retries = models.PositiveIntegerField(default=0) - def start(self): + def start(self, restart_failed=False): from osf.email.notification_campaign import start_notification_campaign self.status = NotificationCampaignStatus.RUNNING self.started_at = timezone.now() - - self.sent_count = 0 + if not restart_failed: + self.recipient_count = 0 + self.sent_count = 0 self.failed_count = 0 - self.recipient_count = 0 self.retries = 0 self.metadata.update({'template': self.notification_type.template}) self.save() - start_notification_campaign.delay(campaign_id=self.id) + start_notification_campaign.delay(campaign_id=self.id, restart_failed=restart_failed) class NotificationCampaignRecipient(models.Model): From 22b2d027e0582a9f66040072e7ce976d27e4f956 Mon Sep 17 00:00:00 2001 From: Ostap-Zherebetskyi Date: Thu, 16 Jul 2026 18:23:30 +0300 Subject: [PATCH 05/25] [ENG-11762] Refactor filter handling in notification campaign (#11813) --- admin/notifications/views.py | 9 +++++---- .../notifications/notification_campaigns_detail.html | 5 +++-- osf/email/notification_campaign.py | 12 +++++++----- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/admin/notifications/views.py b/admin/notifications/views.py index 0627063259e..6815d77534c 100644 --- a/admin/notifications/views.py +++ b/admin/notifications/views.py @@ -561,10 +561,11 @@ def get_queryset(self): if predefined := json_filters.get('predefined'): filters = FILTER_PRESETS.get(predefined, {}) else: - filters = { - f'{item["field"]}__{item["lookup"]}': item['value'] - for item in json_filters.get('manual', []) - } + for item in json_filters.get('manual', []): + if item['lookup'] != 'in': + filters[f'{item["field"]}__{item["lookup"]}'] = item['value'] + else: + filters[f'{item["field"]}__{item["lookup"]}'] = [value.strip() for value in item['value'].split(',')] return filter_users(filters) diff --git a/admin/templates/notifications/notification_campaigns_detail.html b/admin/templates/notifications/notification_campaigns_detail.html index fd520e77f79..12590cf1a70 100644 --- a/admin/templates/notifications/notification_campaigns_detail.html +++ b/admin/templates/notifications/notification_campaigns_detail.html @@ -40,7 +40,7 @@

    Progress

    style="display:inline;" > {% csrf_token %} - @@ -74,6 +74,7 @@

    General

    +
    General style="display:inline;" > {% csrf_token %} -
    diff --git a/osf/email/notification_campaign.py b/osf/email/notification_campaign.py index e88c41cc01b..31a041a5642 100644 --- a/osf/email/notification_campaign.py +++ b/osf/email/notification_campaign.py @@ -146,11 +146,13 @@ def start_notification_campaign(campaign_id, restart_failed=False): if predefined_filter_name := filters.get('predefined'): filters = FILTER_PRESETS.get(predefined_filter_name, {}) else: - filters = { - f'{item["field"]}__{item["lookup"]}': item['value'] - for item in filters.get('manual', []) - } - + manual_filters = {} + for item in filters.get('manual', []): + if item['lookup'] != 'in': + manual_filters[f'{item["field"]}__{item["lookup"]}'] = item['value'] + else: + manual_filters[f'{item["field"]}__{item["lookup"]}'] = [value.strip() for value in item['value'].split(',')] + filters = manual_filters tasks = [] total_recipients = 0 batch_size = campaign.metadata.get('execution', {}).get('batch_size', 1000) From 3e5d7c18c035e92309f1022b95477c6ea5cf5d61 Mon Sep 17 00:00:00 2001 From: Ostap-Zherebetskyi Date: Sat, 18 Jul 2026 00:11:37 +0300 Subject: [PATCH 06/25] [ENG-11768] Update choices, add new field, check timeout and improve logging (#11814) * Add developer reminder status and update notification campaign recipient status choices * Handle campaign failure status and log execution time window in send_campaign_batch * Add Sentry logging for campaign retries and execution time window * Improve campaign logging --------- Co-authored-by: Longze Chen --- osf/email/notification_campaign.py | 20 ++++++++++++++++++- ...ient_and_more.py => 0045_project_enter.py} | 5 +++-- osf/models/notification_campaign.py | 4 ++++ 3 files changed, 26 insertions(+), 3 deletions(-) rename osf/migrations/{0045_notificationcampaign_notificationcampaignrecipient_and_more.py => 0045_project_enter.py} (90%) diff --git a/osf/email/notification_campaign.py b/osf/email/notification_campaign.py index 31a041a5642..e4d4aa81406 100644 --- a/osf/email/notification_campaign.py +++ b/osf/email/notification_campaign.py @@ -5,8 +5,10 @@ from framework.celery_tasks import app as celery_app from celery import chord from django.utils import timezone +from datetime import timedelta from osf.models.notification_campaign import NotificationCampaign, NotificationCampaignRecipient, NotificationCampaignStatus, NotificationCampaignRecipientStatus from osf.email import send_email_with_send_grid +from framework import sentry logger = logging.getLogger(__name__) @@ -109,7 +111,9 @@ def process_campaign_retry(*args, **kwargs): return if campaign.retries < max_retries: - logger.info(f'Retrying {failed_recipients_count} failed recipients for campaign {campaign_id}') + message = f'[Notification Campaign] Retrying {failed_recipients_count} failed recipients for campaign {campaign_id}' + logger.info(message) + sentry.log_message(message) filters = {'id__in': failed_recipients.values_list('user_id', flat=True)} tasks = [] for batch in get_filtered_batches(filters=filters, batch_size=batch_size, campaign_id=campaign_id): @@ -189,8 +193,20 @@ def send_campaign_batch(context, recipients_ids, notification_type_name='blank', ).first() # TODO cache if notification_type is None: + if campaign.status != NotificationCampaignStatus.FAILED: + campaign.status = NotificationCampaignStatus.FAILED + campaign.save() return + execution_time_window = campaign.metadata.get('execution', {}).get('time_window', 8) + if campaign.started_at < timezone.now() - timedelta(hours=execution_time_window): + if not campaign.developer_reminder_sent: + message = f'[Notification Campaign] Campaign {campaign_id} exceeded its execution time window ({execution_time_window}h).' + logger.warning(message) + sentry.log_message(message) + campaign.developer_reminder_sent = True + campaign.save() + recipients_qs = OSFUser.objects.filter(id__in=recipients_ids) recipient_records = { 'to_create': [], @@ -243,6 +259,8 @@ def send_campaign_batch(context, recipients_ids, notification_type_name='blank', except Exception as exc: logger.error(exc) # TODO update error + sentry.log_exception(exc) # TODO update error + recipient_record.status = NotificationCampaignRecipientStatus.FAILED recipient_record.error_message = str(exc) recipient_records[operation].append(recipient_record) diff --git a/osf/migrations/0045_notificationcampaign_notificationcampaignrecipient_and_more.py b/osf/migrations/0045_project_enter.py similarity index 90% rename from osf/migrations/0045_notificationcampaign_notificationcampaignrecipient_and_more.py rename to osf/migrations/0045_project_enter.py index 886ff3b29e9..0c42cba25c7 100644 --- a/osf/migrations/0045_notificationcampaign_notificationcampaignrecipient_and_more.py +++ b/osf/migrations/0045_project_enter.py @@ -1,4 +1,4 @@ -# Generated by Django 4.2.26 on 2026-07-09 11:38 +# Generated by Django 4.2.26 on 2026-07-17 11:20 from django.conf import settings from django.db import migrations, models @@ -27,6 +27,7 @@ class Migration(migrations.Migration): ('sent_count', models.PositiveIntegerField(default=0)), ('failed_count', models.PositiveIntegerField(default=0)), ('retries', models.PositiveIntegerField(default=0)), + ('developer_reminder_sent', models.BooleanField(default=False)), ('created_by', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL)), ('notification_type', models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, to='osf.notificationtype')), ], @@ -36,7 +37,7 @@ class Migration(migrations.Migration): fields=[ ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), ('updated_at', models.DateTimeField(auto_now=True)), - ('status', models.CharField(choices=[('pending', 'Pending'), ('sent', 'Sent'), ('failed', 'Failed')], db_index=True, default='pending', max_length=20)), + ('status', models.CharField(choices=[('pending', 'Pending'), ('sent', 'Sent'), ('failed', 'Failed'), ('skipped', 'Skipped'), ('postponed', 'Postponed')], db_index=True, default='pending', max_length=20)), ('error_message', models.TextField(blank=True, null=True)), ('campaign', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='recipients', to='osf.notificationcampaign')), ('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)), diff --git a/osf/models/notification_campaign.py b/osf/models/notification_campaign.py index 9cce5c3b966..84e635e2368 100644 --- a/osf/models/notification_campaign.py +++ b/osf/models/notification_campaign.py @@ -15,6 +15,8 @@ class NotificationCampaignRecipientStatus(models.TextChoices): PENDING = 'pending', 'Pending' SENT = 'sent', 'Sent' FAILED = 'failed', 'Failed' + SKIPPED = 'skipped', 'Skipped' + POSTPONED = 'postponed', 'Postponed' class NotificationCampaign(models.Model): @@ -64,6 +66,8 @@ class NotificationCampaign(models.Model): failed_count = models.PositiveIntegerField(default=0) retries = models.PositiveIntegerField(default=0) + developer_reminder_sent = models.BooleanField(default=False) + def start(self, restart_failed=False): from osf.email.notification_campaign import start_notification_campaign self.status = NotificationCampaignStatus.RUNNING From 8c770b2d8e4fc5e914f272b49d109bb65e061337 Mon Sep 17 00:00:00 2001 From: antkryt Date: Tue, 21 Jul 2026 17:56:39 +0300 Subject: [PATCH 07/25] [ENG-11764] Filter, order, skip user by activity, registration date and spam status (#11815) * sort recipients by activity priority * simplify filter; send campaign email tasks in 3 phases * add global values to defaults and admin campaign metadata fields --- admin/notifications/forms.py | 11 +- admin/notifications/views.py | 1 + .../notification_campaing_create.html | 20 ++- osf/email/notification_campaign.py | 129 +++++++++++++----- osf/models/notification_campaign.py | 1 + website/settings/defaults.py | 5 + 6 files changed, 132 insertions(+), 35 deletions(-) diff --git a/admin/notifications/forms.py b/admin/notifications/forms.py index 98ad5c467a2..2e8f6b96029 100644 --- a/admin/notifications/forms.py +++ b/admin/notifications/forms.py @@ -1,5 +1,6 @@ from django import forms from osf.models import NotificationType, NotificationCampaign +from website import settings import json @@ -24,12 +25,18 @@ class NotificationCampaignCreateForm(forms.ModelForm): batch_size = forms.IntegerField( min_value=1, - initial=1000, + initial=settings.DEFAULT_CAMPAIGN_BATCH_SIZE, ) max_retries = forms.IntegerField( min_value=0, - initial=3, + initial=settings.DEFAULT_CAMPAIGN_MAX_RETRIES, + ) + + activity_threshold = forms.IntegerField( + min_value=0, + initial=settings.DEFAULT_CAMPAIGN_ACTIVITY_THRESHOLD, + help_text='Non-spam users at or above this activity total are sent in the high-activity phase.', ) class Meta: diff --git a/admin/notifications/views.py b/admin/notifications/views.py index 6815d77534c..9f58d9be79e 100644 --- a/admin/notifications/views.py +++ b/admin/notifications/views.py @@ -508,6 +508,7 @@ def form_valid(self, form): 'execution': { 'batch_size': form.cleaned_data['batch_size'], 'max_retries': form.cleaned_data['max_retries'], + 'activity_threshold': form.cleaned_data['activity_threshold'], }, } diff --git a/admin/templates/notifications/notification_campaing_create.html b/admin/templates/notifications/notification_campaing_create.html index bc274bdf4cc..9b092673351 100644 --- a/admin/templates/notifications/notification_campaing_create.html +++ b/admin/templates/notifications/notification_campaing_create.html @@ -152,7 +152,7 @@

    Execution

    class="form-control" type="number" name="batch_size" - value="1000" + value="{{ form.batch_size.initial }}" min="1" > @@ -165,11 +165,27 @@

    Execution

    class="form-control" type="number" name="max_retries" - value="3" + value="{{ form.max_retries.initial }}" min="0" > + + + Activity Threshold + + +

    + Non-spam users at or above this activity total are sent first +

    + + diff --git a/osf/email/notification_campaign.py b/osf/email/notification_campaign.py index e4d4aa81406..40695761687 100644 --- a/osf/email/notification_campaign.py +++ b/osf/email/notification_campaign.py @@ -1,14 +1,17 @@ import logging + from osf.models import NotificationType, NotificationTypeEnum, OSFUser, UserActivityCounter, Email +from osf.models.spam import SpamStatus from django.db.models import OuterRef, Subquery, Exists, F, Q, Case, When, CharField from django.db.models.functions import Coalesce from framework.celery_tasks import app as celery_app -from celery import chord +from celery import chord, group, chain from django.utils import timezone from datetime import timedelta from osf.models.notification_campaign import NotificationCampaign, NotificationCampaignRecipient, NotificationCampaignStatus, NotificationCampaignRecipientStatus from osf.email import send_email_with_send_grid from framework import sentry +from website import settings logger = logging.getLogger(__name__) @@ -48,10 +51,27 @@ def filter_users(filters, campaign_id=None, restart_failed=False): return qs -def get_filtered_batches(filters, batch_size=1000, campaign_id=None, restart_failed=False): +def get_filtered_batches( + filters, + batch_size=settings.DEFAULT_CAMPAIGN_BATCH_SIZE, + campaign_id=None, + restart_failed=False, + min_activity=None, + max_activity=None, + exclude_spam=False, +): qs = filter_users(filters, campaign_id, restart_failed=restart_failed) + if exclude_spam: + qs = qs.exclude(spam_status=SpamStatus.SPAM) + + qs = qs.annotate(activity_total=Coalesce(Subquery(counter_subquery), 0)) + + if min_activity is not None: + qs = qs.filter(activity_total__gte=min_activity) + if max_activity is not None: + qs = qs.filter(activity_total__lt=max_activity) - qs = qs.annotate(activity_total=Coalesce(Subquery(counter_subquery), 0)).order_by('-activity_total', '-date_registered', '-id') + qs = qs.order_by('-activity_total', '-date_registered', '-id') last_total = None last_date = None @@ -68,24 +88,48 @@ def get_filtered_batches(filters, batch_size=1000, campaign_id=None, restart_fai ) batch = batch_qs[:batch_size] - - if not batch: - break - rows = list(batch.values_list('id', 'activity_total', 'date_registered')) if not rows: break batch_ids = [r[0] for r in rows] + last_id, last_total, last_date = rows[-1] + + yield batch_ids + - last_id, last_total, last_date = ( - rows[-1][0], - rows[-1][1], - rows[-1][2], +def build_campaign_group( + filters, + batch_size=settings.DEFAULT_CAMPAIGN_BATCH_SIZE, + campaign_id=None, + restart_failed=False, + min_activity=None, + max_activity=None, + exclude_spam=True, + **send_kwargs, +): + tasks = [] + total_recipients = 0 + for batch in get_filtered_batches( + filters, + batch_size=batch_size, + campaign_id=campaign_id, + restart_failed=restart_failed, + min_activity=min_activity, + max_activity=max_activity, + exclude_spam=exclude_spam, + ): + tasks.append( + send_campaign_batch.si( + recipients_ids=batch, + campaign_id=campaign_id, + **send_kwargs, + ) ) + total_recipients += len(batch) - yield batch_ids + return group(tasks), total_recipients FILTER_PRESETS = { @@ -96,12 +140,11 @@ def get_filtered_batches(filters, batch_size=1000, campaign_id=None, restart_fai @celery_app.task(name='email.process_campaign_retry') def process_campaign_retry(*args, **kwargs): - campaign_id = kwargs.get('campaign_id') campaign = NotificationCampaign.objects.get(id=campaign_id) failed_recipients = NotificationCampaignRecipient.objects.filter(campaign=campaign, status=NotificationCampaignRecipientStatus.FAILED) - max_retries = campaign.metadata.get('execution', {}).get('max_retries', 3) - batch_size = campaign.metadata.get('execution', {}).get('batch_size', 1000) + max_retries = campaign.metadata.get('execution', {}).get('max_retries', settings.DEFAULT_CAMPAIGN_MAX_RETRIES) + batch_size = campaign.metadata.get('execution', {}).get('batch_size', settings.DEFAULT_CAMPAIGN_BATCH_SIZE) failed_recipients_count = failed_recipients.count() if not failed_recipients_count: campaign.status = NotificationCampaignStatus.COMPLETED @@ -157,26 +200,50 @@ def start_notification_campaign(campaign_id, restart_failed=False): else: manual_filters[f'{item["field"]}__{item["lookup"]}'] = [value.strip() for value in item['value'].split(',')] filters = manual_filters - tasks = [] - total_recipients = 0 - batch_size = campaign.metadata.get('execution', {}).get('batch_size', 1000) - for batch in get_filtered_batches(filters=filters, batch_size=batch_size, campaign_id=campaign_id, restart_failed=restart_failed): - tasks.append( - send_campaign_batch.s( - notification_type_name=notification_type_name, - recipients_ids=batch, - context=context, - campaign_id=campaign_id, - ) - ) - total_recipients += len(batch) + + execution = campaign.metadata.get('execution', {}) + batch_size = execution.get('batch_size', settings.DEFAULT_CAMPAIGN_BATCH_SIZE) + activity_threshold = execution.get('activity_threshold', settings.DEFAULT_CAMPAIGN_ACTIVITY_THRESHOLD) + batch_task_kwargs = dict( + batch_size=batch_size, + campaign_id=campaign_id, + restart_failed=restart_failed, + notification_type_name=notification_type_name, + context=context, + ) + + # Phase 1: non-spam users at/above activity threshold + high_activity_tasks, high_activity_count = build_campaign_group( + filters=filters, + **batch_task_kwargs, + min_activity=activity_threshold, + ) + + # Phase 2: non-spam users below threshold (includes zero activity) + low_activity_tasks, low_activity_count = build_campaign_group( + filters=filters, + **batch_task_kwargs, + max_activity=activity_threshold, + ) + + # Phase 3: confirmed spam (scheduled only after non-spam phases finish) + spam_users_tasks, spam_users_count = build_campaign_group( + filters={**filters, 'spam_status': SpamStatus.SPAM}, + **batch_task_kwargs, + exclude_spam=False, + ) + + total_recipients = high_activity_count + low_activity_count + spam_users_count if not restart_failed: campaign.recipient_count = total_recipients campaign.save(update_fields=['recipient_count']) - chord(tasks)( - process_campaign_retry.s(campaign_id=campaign_id) - ) + chain( + high_activity_tasks, + low_activity_tasks, + spam_users_tasks, + process_campaign_retry.si(campaign_id=campaign_id) + ).apply_async() @celery_app.task(name='email.send_campaign_batch', ignore_result=False) diff --git a/osf/models/notification_campaign.py b/osf/models/notification_campaign.py index 84e635e2368..89597638b25 100644 --- a/osf/models/notification_campaign.py +++ b/osf/models/notification_campaign.py @@ -57,6 +57,7 @@ class NotificationCampaign(models.Model): # "execution": { # "batch_size": , # "max_retries": , + # "activity_threshold": , # }, # "template": , # } diff --git a/website/settings/defaults.py b/website/settings/defaults.py index f87c293f245..32bddc153af 100644 --- a/website/settings/defaults.py +++ b/website/settings/defaults.py @@ -191,6 +191,11 @@ def parent_dir(path): NOTIFICATIONS_CLEANUP_AGE = timedelta(weeks=12) # 3 months to clean up old notifications and email tasks NOTIFICATIONS_CLEANUP_BATCH_SIZE = 10000 # Batch size for notifications and email tasks cleanup +# Notification campaign execution defaults (overridable per campaign in admin metadata) +DEFAULT_CAMPAIGN_ACTIVITY_THRESHOLD = 3 # Users at/above this activity total are scheduled in the high-activity phase +DEFAULT_CAMPAIGN_BATCH_SIZE = 1000 +DEFAULT_CAMPAIGN_MAX_RETRIES = 3 + # Configuration for "We miss you at OSF" email (`NotificationTypeEnum.USER_NO_LOGIN`) # Note: 1) we can gradually increase `MAX_DAILY_NO_LOGIN_EMAILS` to 10000, 100000, etc. or set it to `None` after we # have verified that users are not spammed by this email after NR release. 2) If we want to clean up database for those From 4fd95a033c44470e7e2fba4002f0adaf00596162 Mon Sep 17 00:00:00 2001 From: Ostap-Zherebetskyi Date: Tue, 21 Jul 2026 18:00:42 +0300 Subject: [PATCH 08/25] [ENG-11769] Redo recipients page (#11821) * Add recipients list and preview views for notification campaigns --- admin/notifications/urls.py | 1 + admin/notifications/views.py | 50 ++++++++++++++++-- .../notification_campaigns_detail.html | 4 +- ...notification_campaing_recipients_list.html | 52 +++++++++++++++++++ ...ification_campaing_recipients_preview.html | 52 +++++++++++++++++++ 5 files changed, 153 insertions(+), 6 deletions(-) create mode 100644 admin/templates/notifications/notification_campaing_recipients_list.html create mode 100644 admin/templates/notifications/notification_campaing_recipients_preview.html diff --git a/admin/notifications/urls.py b/admin/notifications/urls.py index a05e60ee284..c9c03343fd8 100644 --- a/admin/notifications/urls.py +++ b/admin/notifications/urls.py @@ -15,5 +15,6 @@ re_path(r'notification_campaigns_detail/(?P\d+)/$', views.NotificationCampaignDetail.as_view(), name='notification_campaigns_detail'), re_path(r'notification_campaigns_create/$', views.NotificationCampaignCreateView.as_view(), name='notification_campaigns_create'), re_path(r'notification_campaigns_recipients_preview/$', views.NotificationCampaignsRecipientsPreview.as_view(), name='notification_campaigns_recipients_preview'), + re_path(r'notification_campaigns_recipients_list/$', views.NotificationCampaignsRecipientsView.as_view(), name='notification_campaigns_recipients_list'), re_path(r'notification_campaigns_start/(?P\d+)/$', views.StartNotificationCampaign.as_view(), name='notification_campaigns_start'), ] diff --git a/admin/notifications/views.py b/admin/notifications/views.py index 9f58d9be79e..f31e336f2e1 100644 --- a/admin/notifications/views.py +++ b/admin/notifications/views.py @@ -2,13 +2,13 @@ import json from collections import defaultdict from django.urls import reverse_lazy -from django.db.models import Q +from django.db.models import Q, F from django.db import models from django.shortcuts import get_object_or_404, redirect from django.views.generic import ListView, DetailView, UpdateView, CreateView, View from django.contrib import messages from django.contrib.auth.mixins import PermissionRequiredMixin -from osf.models import NotificationSubscription, NotificationType, Notification, EmailTask, NotificationCampaign, OSFUser +from osf.models import NotificationSubscription, NotificationType, Notification, EmailTask, NotificationCampaign, OSFUser, NotificationCampaignRecipient from osf.models.notification_campaign import NotificationCampaignStatus from django.forms.models import model_to_dict from .forms import NotificationTypeForm, NotificationCampaignCreateForm @@ -549,7 +549,7 @@ def get_context_data(self, **kwargs): class NotificationCampaignsRecipientsPreview(PermissionRequiredMixin, ListView): - template_name = 'users/list.html' + template_name = 'notifications/notification_campaing_recipients_preview.html' permission_required = 'osf.view_osfuser' raise_exception = True paginate_by = 25 @@ -568,7 +568,10 @@ def get_queryset(self): else: filters[f'{item["field"]}__{item["lookup"]}'] = [value.strip() for value in item['value'].split(',')] - return filter_users(filters) + qs = filter_users(filters) + return qs.annotate( + guid=F('guids___id') + ) def get_context_data(self, **kwargs): users = self.get_queryset() @@ -588,6 +591,45 @@ def get_context_data(self, **kwargs): is_paginated=is_paginated, ) +class NotificationCampaignsRecipientsView(PermissionRequiredMixin, ListView): + template_name = 'notifications/notification_campaing_recipients_list.html' + permission_required = 'osf.view_osfuser' + raise_exception = True + paginate_by = 25 + + def get_queryset(self): + status = self.request.GET.get('notification_status', None) + campaign_id = self.request.GET.get('campaign_id', None) + if not campaign_id: + return NotificationCampaignRecipient.objects.none() + query = {'campaign_id': campaign_id} + if status: + query['status'] = status + + qs = NotificationCampaignRecipient.objects.filter(**query) + + return qs.annotate( + guid=F('user__guids___id') + ) + + def get_context_data(self, **kwargs): + users = self.get_queryset() + + page_size = self.get_paginate_by(users) + paginator, page, query_set, is_paginated = self.paginate_queryset( + users, + page_size, + ) + # append search param to pagination links + kwargs.update({'extra_query_params': f'¬ification_status={self.request.GET.get("notification_status")}&campaign_id={self.request.GET.get('campaign_id')}'}) + return super().get_context_data( + **kwargs, + page=page, + query_set=query_set, + paginator=paginator, + is_paginated=is_paginated, + ) + class StartNotificationCampaign(PermissionRequiredMixin, View): permission_required = 'osf.change_notificationtype' diff --git a/admin/templates/notifications/notification_campaigns_detail.html b/admin/templates/notifications/notification_campaigns_detail.html index 12590cf1a70..1710209eea0 100644 --- a/admin/templates/notifications/notification_campaigns_detail.html +++ b/admin/templates/notifications/notification_campaigns_detail.html @@ -57,7 +57,7 @@

    General

    {% if field == 'Sent' and value != 0 %} Preview Recipients @@ -67,7 +67,7 @@

    General

    {% elif field == 'Failed' and value != 0 %}
    Preview Recipients diff --git a/admin/templates/notifications/notification_campaing_recipients_list.html b/admin/templates/notifications/notification_campaing_recipients_list.html new file mode 100644 index 00000000000..03e519b0235 --- /dev/null +++ b/admin/templates/notifications/notification_campaing_recipients_list.html @@ -0,0 +1,52 @@ +{% extends 'base.html' %} +{% load static %} +{% block title %} +User Search Results +{% endblock title %} +{% block content %} + {% load node_extras %} + {% include "util/pagination.html" with items=page status=status %} + {% if perms.osf.mark_spam %} +
    + {% csrf_token %} + {% endif %} + + + + + + + + + + + + {% for record in query_set %} + + + + + + + + {% endfor %} + +
    GUIDUsernameStatusErrorUpdated at
    + + {{ record.guid }} + + + {{record.user.username}} + + {{ record.status }} + + {{ record.error_message }} + + {{ record.updated_at }} +
    +
    + + {% if not query_set|length %} +

    No results found

    + {% endif %} +{% endblock content %} diff --git a/admin/templates/notifications/notification_campaing_recipients_preview.html b/admin/templates/notifications/notification_campaing_recipients_preview.html new file mode 100644 index 00000000000..62bb9dd763c --- /dev/null +++ b/admin/templates/notifications/notification_campaing_recipients_preview.html @@ -0,0 +1,52 @@ +{% extends 'base.html' %} +{% load static %} +{% block title %} +User Search Results +{% endblock title %} +{% block content %} + {% load node_extras %} + {% include "util/pagination.html" with items=page status=status %} + {% if perms.osf.mark_spam %} +
    + {% csrf_token %} + {% endif %} + + + + + + + + + + + + {% for user in users %} + + + + + + + + {% endfor %} + +
    GUIDUsernameFullnameDate confirmedDate disabled
    + + {{ user.guid }} + + + {{user.username}} + + {{ user.fullname }} + + {{ user.is_confirmed }} + + {{ user.is_disabled }} +
    +
    + + {% if not users|length %} +

    No results found

    + {% endif %} +{% endblock content %} From d2750498e65b185e9292a781a0c6187d438f0df2 Mon Sep 17 00:00:00 2001 From: Ostap-Zherebetskyi Date: Thu, 23 Jul 2026 21:38:15 +0300 Subject: [PATCH 09/25] [ENG-11791][ENG-11793] Update model to pre-calculate and sort recipients; rework campaign workflow (#11825) * Refactor notification campaign logic: create recipients in batches, add run_id to models, and improve filtering by activity score * Update notification campaign logic * Update sendgrid bulk * Handle email sending errors in send_campaign_batch: log exceptions and update recipient statuses * Improve error logging --------- Co-authored-by: antkryt --- admin/notifications/views.py | 4 +- osf/email/notification_campaign.py | 265 ++++++++++++--------------- osf/migrations/0045_project_enter.py | 18 +- osf/models/notification_campaign.py | 30 +++ 4 files changed, 169 insertions(+), 148 deletions(-) diff --git a/admin/notifications/views.py b/admin/notifications/views.py index f31e336f2e1..efbff738ff8 100644 --- a/admin/notifications/views.py +++ b/admin/notifications/views.py @@ -16,7 +16,7 @@ from mako.parsetree import ControlLine from string import Formatter from osf.email import _render_email_html -from osf.email.notification_campaign import FILTER_PRESETS, filter_users +from osf.email.notification_campaign import FILTER_PRESETS def delete_selected_notifications(selected_ids): @@ -568,7 +568,7 @@ def get_queryset(self): else: filters[f'{item["field"]}__{item["lookup"]}'] = [value.strip() for value in item['value'].split(',')] - qs = filter_users(filters) + qs = OSFUser.objects.filter(**filters) return qs.annotate( guid=F('guids___id') ) diff --git a/osf/email/notification_campaign.py b/osf/email/notification_campaign.py index 40695761687..c2d242a59d7 100644 --- a/osf/email/notification_campaign.py +++ b/osf/email/notification_campaign.py @@ -2,23 +2,25 @@ from osf.models import NotificationType, NotificationTypeEnum, OSFUser, UserActivityCounter, Email from osf.models.spam import SpamStatus -from django.db.models import OuterRef, Subquery, Exists, F, Q, Case, When, CharField +from django.db.models import OuterRef, Subquery, F, Case, When, CharField from django.db.models.functions import Coalesce from framework.celery_tasks import app as celery_app -from celery import chord, group, chain +from celery import group, chain from django.utils import timezone from datetime import timedelta from osf.models.notification_campaign import NotificationCampaign, NotificationCampaignRecipient, NotificationCampaignStatus, NotificationCampaignRecipientStatus from osf.email import send_email_with_send_grid from framework import sentry from website import settings +from itertools import batched logger = logging.getLogger(__name__) +BULK_CREATE_SIZE = 5000 first_email_subquery = ( Email.objects - .filter(user=OuterRef('pk')) + .filter(user=OuterRef('user_id')) .values('address')[:1] ) @@ -29,96 +31,80 @@ .values('total')[:1] ) -def filter_users(filters, campaign_id=None, restart_failed=False): - qs = OSFUser.objects.all() - if campaign_id: - if restart_failed: - already_sent_subquery = NotificationCampaignRecipient.objects.filter( - campaign_id=campaign_id, - user_id=OuterRef('pk'), - status__in=['sent', 'pending'] - ) - else: - already_sent_subquery = NotificationCampaignRecipient.objects.filter( +def create_campaign_recipients(filters, campaign_id): + qs = ( + OSFUser.objects + .filter(**filters) + .annotate(activity_score=Coalesce(Subquery(counter_subquery), 0)) + .values_list( + 'id', + 'activity_score', + ) + ) + + for rows in batched(qs.iterator(chunk_size=BULK_CREATE_SIZE), BULK_CREATE_SIZE): + NotificationCampaignRecipient.objects.bulk_create( + NotificationCampaignRecipient( campaign_id=campaign_id, - user_id=OuterRef('pk'), + user_id=user_id, + activity_score=activity_score, ) - - qs = OSFUser.objects.annotate(already_sent=Exists(already_sent_subquery)).filter(already_sent=False) - - qs = qs.filter(**filters) - - return qs + for user_id, activity_score in rows + ) -def get_filtered_batches( - filters, - batch_size=settings.DEFAULT_CAMPAIGN_BATCH_SIZE, - campaign_id=None, +def get_campaign_recipient_batches( + campaign_id, + batch_size, restart_failed=False, min_activity=None, max_activity=None, - exclude_spam=False, + spam=None, ): - qs = filter_users(filters, campaign_id, restart_failed=restart_failed) - if exclude_spam: - qs = qs.exclude(spam_status=SpamStatus.SPAM) + qs = NotificationCampaignRecipient.objects.filter( + campaign_id=campaign_id, + ) - qs = qs.annotate(activity_total=Coalesce(Subquery(counter_subquery), 0)) + if restart_failed: + qs = qs.filter(status=NotificationCampaignRecipientStatus.FAILED) + else: + qs = qs.filter(status=NotificationCampaignRecipientStatus.PENDING) + # Minimum and maximum activity are mutually exclusive and use the same threshold. if min_activity is not None: - qs = qs.filter(activity_total__gte=min_activity) - if max_activity is not None: - qs = qs.filter(activity_total__lt=max_activity) - - qs = qs.order_by('-activity_total', '-date_registered', '-id') - - last_total = None - last_date = None - last_id = None - - while True: - batch_qs = qs + qs = qs.filter(activity_score__gte=min_activity) - if last_total is not None: - batch_qs = batch_qs.filter( - Q(activity_total__lt=last_total) | - Q(activity_total=last_total, date_registered__lt=last_date) | - Q(activity_total=last_total, date_registered=last_date, id__lt=last_id) - ) - - batch = batch_qs[:batch_size] - rows = list(batch.values_list('id', 'activity_total', 'date_registered')) - - if not rows: - break - - batch_ids = [r[0] for r in rows] - last_id, last_total, last_date = rows[-1] + if max_activity is not None: + qs = qs.filter(activity_score__lt=max_activity) - yield batch_ids + if spam is True: + qs = qs.filter(user__spam_status=SpamStatus.SPAM) + elif spam is False: + qs = qs.exclude(user__spam_status=SpamStatus.SPAM) + yield from batched( + qs.values_list('id', flat=True).iterator(chunk_size=batch_size), + batch_size, + ) def build_campaign_group( - filters, - batch_size=settings.DEFAULT_CAMPAIGN_BATCH_SIZE, - campaign_id=None, + campaign_id, + batch_size, restart_failed=False, min_activity=None, max_activity=None, - exclude_spam=True, + spam=None, **send_kwargs, ): tasks = [] - total_recipients = 0 - for batch in get_filtered_batches( - filters, - batch_size=batch_size, + + for batch in get_campaign_recipient_batches( campaign_id=campaign_id, + batch_size=batch_size, restart_failed=restart_failed, min_activity=min_activity, max_activity=max_activity, - exclude_spam=exclude_spam, + spam=spam, ): tasks.append( send_campaign_batch.si( @@ -127,9 +113,8 @@ def build_campaign_group( **send_kwargs, ) ) - total_recipients += len(batch) - return group(tasks), total_recipients + return group(tasks) FILTER_PRESETS = { @@ -157,20 +142,20 @@ def process_campaign_retry(*args, **kwargs): message = f'[Notification Campaign] Retrying {failed_recipients_count} failed recipients for campaign {campaign_id}' logger.info(message) sentry.log_message(message) - filters = {'id__in': failed_recipients.values_list('user_id', flat=True)} - tasks = [] - for batch in get_filtered_batches(filters=filters, batch_size=batch_size, campaign_id=campaign_id): - tasks.append( - send_campaign_batch.s( - notification_type_name=campaign.notification_type.name, - recipients_ids=batch, - context=campaign.metadata.get('context', {}), - campaign_id=campaign_id, - ) - ) - chord(tasks)( - process_campaign_retry.s(campaign_id=campaign_id) + + tasks = build_campaign_group( + batch_size=batch_size, + campaign_id=campaign_id, + restart_failed=True, + notification_type_name=campaign.notification_type.name, + context=campaign.metadata.get('context', {}), + run_id=campaign.run_id ) + + chain( + tasks, + process_campaign_retry.si(campaign_id=campaign_id) + ).apply_async() campaign.retries += 1 campaign.save(update_fields=['retries']) else: @@ -201,6 +186,9 @@ def start_notification_campaign(campaign_id, restart_failed=False): manual_filters[f'{item["field"]}__{item["lookup"]}'] = [value.strip() for value in item['value'].split(',')] filters = manual_filters + if not restart_failed: + create_campaign_recipients(filters=filters, campaign_id=campaign_id) + execution = campaign.metadata.get('execution', {}) batch_size = execution.get('batch_size', settings.DEFAULT_CAMPAIGN_BATCH_SIZE) activity_threshold = execution.get('activity_threshold', settings.DEFAULT_CAMPAIGN_ACTIVITY_THRESHOLD) @@ -210,45 +198,41 @@ def start_notification_campaign(campaign_id, restart_failed=False): restart_failed=restart_failed, notification_type_name=notification_type_name, context=context, + run_id=campaign.run_id ) - # Phase 1: non-spam users at/above activity threshold - high_activity_tasks, high_activity_count = build_campaign_group( - filters=filters, - **batch_task_kwargs, + workflow = [] + high_activity_tasks = build_campaign_group( min_activity=activity_threshold, + spam=False, + **batch_task_kwargs ) + if high_activity_tasks: + workflow.append(high_activity_tasks) - # Phase 2: non-spam users below threshold (includes zero activity) - low_activity_tasks, low_activity_count = build_campaign_group( - filters=filters, - **batch_task_kwargs, + low_activity_tasks = build_campaign_group( max_activity=activity_threshold, + spam=False, + **batch_task_kwargs ) + if low_activity_tasks: + workflow.append(low_activity_tasks) - # Phase 3: confirmed spam (scheduled only after non-spam phases finish) - spam_users_tasks, spam_users_count = build_campaign_group( - filters={**filters, 'spam_status': SpamStatus.SPAM}, - **batch_task_kwargs, - exclude_spam=False, + spam_users_tasks = build_campaign_group( + spam=True, + **batch_task_kwargs ) + if spam_users_tasks: + workflow.append(spam_users_tasks) - total_recipients = high_activity_count + low_activity_count + spam_users_count - if not restart_failed: - campaign.recipient_count = total_recipients - campaign.save(update_fields=['recipient_count']) - - chain( - high_activity_tasks, - low_activity_tasks, - spam_users_tasks, - process_campaign_retry.si(campaign_id=campaign_id) - ).apply_async() + chain(*workflow, process_campaign_retry.si(campaign_id=campaign_id)).apply_async() @celery_app.task(name='email.send_campaign_batch', ignore_result=False) -def send_campaign_batch(context, recipients_ids, notification_type_name='blank', campaign_id=None): +def send_campaign_batch(context, recipients_ids, notification_type_name='blank', campaign_id=None, run_id=None): campaign = NotificationCampaign.objects.get(id=campaign_id) + if campaign.run_id != run_id: + return if campaign.status == NotificationCampaignStatus.CANCELLED: logger.warning(f"Campaign {campaign_id} was cancelled") return @@ -274,69 +258,62 @@ def send_campaign_batch(context, recipients_ids, notification_type_name='blank', campaign.developer_reminder_sent = True campaign.save() - recipients_qs = OSFUser.objects.filter(id__in=recipients_ids) - recipient_records = { - 'to_create': [], - 'to_update': [], - } - existing = { - r.user_id: r - for r in NotificationCampaignRecipient.objects.filter( - campaign_id=campaign_id, - user_id__in=recipients_ids, - ) - } + recipients_qs = NotificationCampaignRecipient.objects.filter(id__in=recipients_ids).select_related('user') + recipient_records = [] success_count = 0 failure_count = 0 if campaign.metadata.get('sendgrid_bulk', False): recipients_qs_annotated = recipients_qs.annotate( recipient_address=Case( - When(username__contains='@', then='username'), + When(user__username__contains='@', then='user_id'), default=Subquery(first_email_subquery), output_field=CharField(), ) ) - recipient_emails = list(recipients_qs_annotated.values_list('recipient_address', flat=True)) - send_email_with_send_grid(to_addr=recipient_emails, notification_type=notification_type, context=context) + valid_emails_qs = recipients_qs_annotated.exclude(recipient_address__isnull=True) + invalid_emails_qs = recipients_qs_annotated.filter(recipient_address__isnull=True) + recipient_emails = list(valid_emails_qs.values_list('recipient_address', flat=True)) success_count = len(recipient_emails) + try: + send_email_with_send_grid(to_addr=recipient_emails, notification_type=notification_type, context=context) + except Exception as exc: + message = f'[Notification Campaign] Campaign {campaign_id} sendgrid bulk request failed. {str(exc)}' + logger.error(message) + sentry.log_exception(message) + + valid_emails_qs.update(status=NotificationCampaignRecipientStatus.FAILED, error_message=str(exc)) + failure_count += success_count + success_count = 0 + pass + invalid_emails_qs.update(status=NotificationCampaignRecipientStatus.SKIPPED, error_message='Invalid email address') + else: for recipient in recipients_qs: - recipient_record = existing.get(recipient.id) - - if recipient_record is None: - recipient_record = NotificationCampaignRecipient( - campaign_id=campaign_id, - user=recipient, - ) - operation = 'to_create' - else: - operation = 'to_update' - try: notification_type.emit( - user=recipient, + user=recipient.user, event_context=context, save=False, # Too many write operations ) - recipient_record.status = NotificationCampaignRecipientStatus.SENT - recipient_record.error_message = None - recipient_records[operation].append(recipient_record) + recipient.status = NotificationCampaignRecipientStatus.SENT + recipient.error_message = None + recipient_records.append(recipient) success_count += 1 except Exception as exc: - logger.error(exc) # TODO update error - sentry.log_exception(exc) # TODO update error + message = f'[Notification Campaign] Campaign {campaign_id} sendgrid request failed for user {recipient.user.username}. {str(exc)}' + logger.error(message) + sentry.log_exception(message) - recipient_record.status = NotificationCampaignRecipientStatus.FAILED - recipient_record.error_message = str(exc) - recipient_records[operation].append(recipient_record) + recipient.status = NotificationCampaignRecipientStatus.FAILED + recipient.error_message = str(exc) + recipient_records.append(recipient) failure_count += 1 pass - NotificationCampaignRecipient.objects.bulk_create(recipient_records['to_create']) - NotificationCampaignRecipient.objects.bulk_update(recipient_records['to_update'], ['status', 'error_message']) + NotificationCampaignRecipient.objects.bulk_update(recipient_records, ['status', 'error_message']) NotificationCampaign.objects.filter(pk=campaign_id).update(sent_count=F('sent_count') + success_count, failed_count=F('failed_count') + failure_count) logger.info('Batch finished') # TODO: add/update logs diff --git a/osf/migrations/0045_project_enter.py b/osf/migrations/0045_project_enter.py index 0c42cba25c7..f9c3f7092b4 100644 --- a/osf/migrations/0045_project_enter.py +++ b/osf/migrations/0045_project_enter.py @@ -1,4 +1,4 @@ -# Generated by Django 4.2.26 on 2026-07-17 11:20 +# Generated by Django 4.2.26 on 2026-07-23 09:30 from django.conf import settings from django.db import migrations, models @@ -18,6 +18,7 @@ class Migration(migrations.Migration): ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), ('created_at', models.DateTimeField(auto_now_add=True)), ('updated_at', models.DateTimeField(auto_now=True)), + ('run_id', models.UUIDField(blank=True, null=True, unique=True)), ('name', models.CharField(max_length=255)), ('status', models.CharField(choices=[('created', 'Created'), ('running', 'Running'), ('completed', 'Completed'), ('partially_completed', 'Partially Completed'), ('failed', 'Failed'), ('cancelled', 'Cancelled'), ('ended', 'Ended')], default='created', max_length=20)), ('started_at', models.DateTimeField(blank=True, null=True)), @@ -39,11 +40,12 @@ class Migration(migrations.Migration): ('updated_at', models.DateTimeField(auto_now=True)), ('status', models.CharField(choices=[('pending', 'Pending'), ('sent', 'Sent'), ('failed', 'Failed'), ('skipped', 'Skipped'), ('postponed', 'Postponed')], db_index=True, default='pending', max_length=20)), ('error_message', models.TextField(blank=True, null=True)), + ('activity_score', models.IntegerField(default=0)), ('campaign', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='recipients', to='osf.notificationcampaign')), ('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)), ], options={ - 'unique_together': {('campaign', 'user')}, + 'ordering': ['-activity_score', 'user__date_registered', 'user_id'], }, ), migrations.AddField( @@ -51,4 +53,16 @@ class Migration(migrations.Migration): name='received_notification_campaigns', field=models.ManyToManyField(through='osf.NotificationCampaignRecipient', to='osf.notificationcampaign'), ), + migrations.AddIndex( + model_name='notificationcampaignrecipient', + index=models.Index(fields=['campaign', '-activity_score', 'user'], name='campaign_order_idx'), + ), + migrations.AddIndex( + model_name='notificationcampaignrecipient', + index=models.Index(fields=['campaign', 'status', '-activity_score', 'user'], name='campaign_status_order_idx'), + ), + migrations.AlterUniqueTogether( + name='notificationcampaignrecipient', + unique_together={('campaign', 'user')}, + ), ] diff --git a/osf/models/notification_campaign.py b/osf/models/notification_campaign.py index 89597638b25..995e179e96e 100644 --- a/osf/models/notification_campaign.py +++ b/osf/models/notification_campaign.py @@ -1,5 +1,6 @@ from django.db import models from django.utils import timezone +import uuid class NotificationCampaignStatus(models.TextChoices): @@ -22,6 +23,7 @@ class NotificationCampaignRecipientStatus(models.TextChoices): class NotificationCampaign(models.Model): created_at = models.DateTimeField(auto_now_add=True) updated_at = models.DateTimeField(auto_now=True) + run_id = models.UUIDField(null=True, blank=True, unique=True) name = models.CharField(max_length=255) @@ -73,6 +75,7 @@ def start(self, restart_failed=False): from osf.email.notification_campaign import start_notification_campaign self.status = NotificationCampaignStatus.RUNNING self.started_at = timezone.now() + self.run_id = uuid.uuid4() if not restart_failed: self.recipient_count = 0 self.sent_count = 0 @@ -102,5 +105,32 @@ class NotificationCampaignRecipient(models.Model): ) error_message = models.TextField(null=True, blank=True) + activity_score = models.IntegerField(default=0) + class Meta: unique_together = ('campaign', 'user') + ordering = [ + '-activity_score', + 'user__date_registered', + 'user_id', + ] + + indexes = [ + models.Index( + fields=[ + 'campaign', + '-activity_score', + 'user', + ], + name='campaign_order_idx', + ), + models.Index( + fields=[ + 'campaign', + 'status', + '-activity_score', + 'user', + ], + name='campaign_status_order_idx', + ), + ] From faddad1278db25b69e3522fc5b0bb29e51390d28 Mon Sep 17 00:00:00 2001 From: antkryt Date: Mon, 27 Jul 2026 16:35:17 +0300 Subject: [PATCH 10/25] [ENG-11796] Unit tests on ordering/filtering user (#11829) * unit tests on ordering and filtering * fix recipients ordering --- osf/migrations/0045_project_enter.py | 2 +- osf/models/notification_campaign.py | 2 +- osf_tests/test_notification_campaign.py | 210 ++++++++++++++++++++++++ 3 files changed, 212 insertions(+), 2 deletions(-) create mode 100644 osf_tests/test_notification_campaign.py diff --git a/osf/migrations/0045_project_enter.py b/osf/migrations/0045_project_enter.py index f9c3f7092b4..77784fa79d7 100644 --- a/osf/migrations/0045_project_enter.py +++ b/osf/migrations/0045_project_enter.py @@ -45,7 +45,7 @@ class Migration(migrations.Migration): ('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)), ], options={ - 'ordering': ['-activity_score', 'user__date_registered', 'user_id'], + 'ordering': ['-activity_score', '-user__date_registered', 'user_id'], }, ), migrations.AddField( diff --git a/osf/models/notification_campaign.py b/osf/models/notification_campaign.py index 995e179e96e..759a50956a3 100644 --- a/osf/models/notification_campaign.py +++ b/osf/models/notification_campaign.py @@ -111,7 +111,7 @@ class Meta: unique_together = ('campaign', 'user') ordering = [ '-activity_score', - 'user__date_registered', + '-user__date_registered', 'user_id', ] diff --git a/osf_tests/test_notification_campaign.py b/osf_tests/test_notification_campaign.py new file mode 100644 index 00000000000..0236a6cbaf7 --- /dev/null +++ b/osf_tests/test_notification_campaign.py @@ -0,0 +1,210 @@ +import pytest +from datetime import timedelta + +from django.utils import timezone + +from osf.email.notification_campaign import ( + create_campaign_recipients, + get_campaign_recipient_batches, +) +from osf.models import NotificationType, UserActivityCounter +from osf.models.notification_campaign import ( + NotificationCampaign, + NotificationCampaignRecipient, +) +from osf.models.spam import SpamStatus +from osf_tests.factories import UserFactory + +pytestmark = pytest.mark.django_db + + +@pytest.fixture +def notification_type(): + notification_type, _ = NotificationType.objects.get_or_create(name='blank') + return notification_type + + +@pytest.fixture +def campaign(notification_type): + return NotificationCampaign.objects.create( + name='Test campaign', + notification_type=notification_type, + metadata={'execution': {'activity_threshold': 100, 'batch_size': 2}}, + ) + + +def _set_activity(user, total): + UserActivityCounter.objects.update_or_create( + _id=user._id, + defaults={'total': total, 'action': {}, 'date': {}}, + ) + + +def _recipient_user_ids(campaign_id, **batch_kwargs): + """Flatten all batches into an list of user ids and preserve order""" + user_ids = [] + for batch in get_campaign_recipient_batches(campaign_id=campaign_id, batch_size=1000, **batch_kwargs): + recipients = NotificationCampaignRecipient.objects.filter(id__in=batch) + by_id = {r.id: r.user_id for r in recipients} + user_ids.extend(by_id[recipient_id] for recipient_id in batch) + return user_ids + + +def _recipient_scores(campaign_id, **batch_kwargs): + """Flatten all batches into an list of activity scores and preserve order""" + scores = [] + for batch in get_campaign_recipient_batches(campaign_id=campaign_id, batch_size=1000, **batch_kwargs): + by_id = { + r.id: r.activity_score + for r in NotificationCampaignRecipient.objects.filter(id__in=batch) + } + scores.extend(by_id[recipient_id] for recipient_id in batch) + return scores + + +class TestCreateCampaignRecipients: + + def test_creates_recipients_with_activity_scores(self, campaign): + high = UserFactory() + low = UserFactory() + zero = UserFactory() + _set_activity(high, 500) + _set_activity(low, 50) + + create_campaign_recipients( + filters={'id__in': [high.id, low.id, zero.id]}, + campaign_id=campaign.id, + ) + + recipients = { + r.user_id: r.activity_score + for r in NotificationCampaignRecipient.objects.filter(campaign=campaign) + } + assert recipients[high.id] == 500 + assert recipients[low.id] == 50 + assert recipients[zero.id] == 0 + assert NotificationCampaignRecipient.objects.filter(campaign=campaign).count() == 3 + + def test_respects_user_filters(self, campaign): + included = UserFactory(is_staff=True) + excluded = UserFactory(is_staff=False) + _set_activity(included, 10) + _set_activity(excluded, 10) + + create_campaign_recipients( + filters={'id__in': [included.id, excluded.id], 'is_staff': True}, + campaign_id=campaign.id, + ) + + user_ids = set( + NotificationCampaignRecipient.objects.filter(campaign=campaign).values_list('user_id', flat=True) + ) + assert user_ids == {included.id} + + def test_ordered_by_activity_score_descending(self, campaign): + older = UserFactory() + newer = UserFactory() + mid = UserFactory() + older.date_registered = timezone.now() - timedelta(days=3) + mid.date_registered = timezone.now() - timedelta(days=2) + newer.date_registered = timezone.now() - timedelta(days=1) + older.save(update_fields=['date_registered']) + mid.save(update_fields=['date_registered']) + newer.save(update_fields=['date_registered']) + + _set_activity(older, 10) + _set_activity(mid, 50) + _set_activity(newer, 50) + + create_campaign_recipients( + filters={'id__in': [older.id, newer.id, mid.id]}, + campaign_id=campaign.id, + ) + + scores = _recipient_scores(campaign.id) + assert scores == sorted(scores, reverse=True) + user_ids = _recipient_user_ids(campaign.id) + assert user_ids == [newer.id, mid.id, older.id] + + +class TestGetCampaignRecipientBatches: + + @pytest.fixture + def users_and_recipients(self, campaign): + threshold = 100 + high = UserFactory() + low = UserFactory() + zero = UserFactory() + flagged = UserFactory() + spam = UserFactory() + spam.spam_status = SpamStatus.SPAM + spam.save() + flagged.spam_status = SpamStatus.FLAGGED + flagged.save() + + _set_activity(high, 250) + _set_activity(low, 40) + _set_activity(flagged, 300) + _set_activity(spam, 900) + + create_campaign_recipients( + filters={'id__in': [high.id, low.id, zero.id, flagged.id, spam.id]}, + campaign_id=campaign.id, + ) + return { + 'threshold': threshold, + 'high': high, + 'low': low, + 'zero': zero, + 'flagged': flagged, + 'spam': spam, + } + + def test_high_activity_non_spam(self, campaign, users_and_recipients): + data = users_and_recipients + user_ids = _recipient_user_ids( + campaign.id, + min_activity=data['threshold'], + spam=False, + ) + assert set(user_ids) == {data['high'].id, data['flagged'].id} + + def test_low_activity_non_spam_includes_zero(self, campaign, users_and_recipients): + data = users_and_recipients + user_ids = _recipient_user_ids( + campaign.id, + max_activity=data['threshold'], + spam=False, + ) + assert set(user_ids) == {data['low'].id, data['zero'].id} + + def test_spam_only(self, campaign, users_and_recipients): + data = users_and_recipients + user_ids = _recipient_user_ids(campaign.id, spam=True) + assert user_ids == [data['spam'].id] + + def test_phases_cover_all_pending_recipients(self, campaign, users_and_recipients): + data = users_and_recipients + threshold = data['threshold'] + all_ids = ( + set(_recipient_user_ids(campaign.id, min_activity=threshold, spam=False)) + | set(_recipient_user_ids(campaign.id, max_activity=threshold, spam=False)) + | set(_recipient_user_ids(campaign.id, spam=True)) + ) + expected = {data['high'].id, data['low'].id, data['zero'].id, data['flagged'].id, data['spam'].id} + assert all_ids == expected + + def test_batches_respect_batch_size(self, campaign): + users = [UserFactory() for _ in range(5)] + for i, user in enumerate(users): + _set_activity(user, (i + 1) * 10) + + create_campaign_recipients( + filters={'id__in': [u.id for u in users]}, + campaign_id=campaign.id, + ) + + batches = list(get_campaign_recipient_batches(campaign_id=campaign.id, batch_size=2)) + assert [len(batch) for batch in batches] == [2, 2, 1] + flat = {recipient_id for batch in batches for recipient_id in batch} + assert len(flat) == 5 From a70d6eee186ef6cf0be1f84aa3c78e232d19490f Mon Sep 17 00:00:00 2001 From: Ostap-Zherebetskyi Date: Mon, 27 Jul 2026 16:35:55 +0300 Subject: [PATCH 11/25] [ENG-11763] Add validation for creating/starting campagin (#11828) * Add sendgrid_bulk field to NotificationCampaignCreateForm and handle errors in context and filters; update notification campaign views and templates for improved messaging and recipient handling * Add sendgrid_bulk field to NotificationCampaignCreateForm and update notification campaign processing logic * Fix campaign recipient query to filter only by FAILED status when restarting failed campaigns * Refactor notification campaign recipient status handling and improve message display in templates --- admin/notifications/forms.py | 15 ++- admin/notifications/views.py | 26 ++++- .../notification_campaigns_detail.html | 26 ++++- .../notification_campaing_create.html | 31 ++++- osf/email/notification_campaign.py | 108 +++++++++++------- osf/models/notification_campaign.py | 11 +- 6 files changed, 164 insertions(+), 53 deletions(-) diff --git a/admin/notifications/forms.py b/admin/notifications/forms.py index 2e8f6b96029..9a5ef522314 100644 --- a/admin/notifications/forms.py +++ b/admin/notifications/forms.py @@ -39,6 +39,11 @@ class NotificationCampaignCreateForm(forms.ModelForm): help_text='Non-spam users at or above this activity total are sent in the high-activity phase.', ) + sendgrid_bulk = forms.BooleanField( + required=False, + initial=False, + ) + class Meta: model = NotificationCampaign fields = ( @@ -48,8 +53,14 @@ class Meta: def clean_context(self): value = self.cleaned_data['context'] or '{}' - return json.loads(value) + try: + return json.loads(value) + except Exception as e: + forms.ValidationError(e) def clean_filters(self): value = self.cleaned_data['filters'] or '{}' - return json.loads(value) + try: + return json.loads(value) + except Exception as e: + forms.ValidationError(e) diff --git a/admin/notifications/views.py b/admin/notifications/views.py index efbff738ff8..8d06c9477b1 100644 --- a/admin/notifications/views.py +++ b/admin/notifications/views.py @@ -9,7 +9,7 @@ from django.contrib import messages from django.contrib.auth.mixins import PermissionRequiredMixin from osf.models import NotificationSubscription, NotificationType, Notification, EmailTask, NotificationCampaign, OSFUser, NotificationCampaignRecipient -from osf.models.notification_campaign import NotificationCampaignStatus +from osf.models.notification_campaign import NotificationCampaignStatus, NotificationCampaignRecipientStatus from django.forms.models import model_to_dict from .forms import NotificationTypeForm, NotificationCampaignCreateForm from mako.lexer import Lexer @@ -17,6 +17,7 @@ from string import Formatter from osf.email import _render_email_html from osf.email.notification_campaign import FILTER_PRESETS +from website import settings def delete_selected_notifications(selected_ids): @@ -510,7 +511,16 @@ def form_valid(self, form): 'max_retries': form.cleaned_data['max_retries'], 'activity_threshold': form.cleaned_data['activity_threshold'], }, + 'sendgrid_bulk': form.cleaned_data.get('sendgrid_bulk', False), } + try: + _render_email_html(form.instance.notification_type, form.cleaned_data['context']) + except Exception as e: + form.add_error( + 'context', + f"Failed to render template: {e}", + ) + return self.form_invalid(form) response = super().form_valid(form) @@ -545,6 +555,7 @@ def get_context_data(self, **kwargs): context['filter_fields'] = filter_fields context['filters'] = [] context['predefined_filters'] = FILTER_PRESETS.keys() + context['default_context'] = json.dumps({'domain': settings.DOMAIN, 'osf_contact_email': settings.OSF_CONTACT_EMAIL}, indent=4) return context @@ -603,8 +614,10 @@ def get_queryset(self): if not campaign_id: return NotificationCampaignRecipient.objects.none() query = {'campaign_id': campaign_id} - if status: - query['status'] = status + if status == NotificationCampaignRecipientStatus.SENT: + query['status'] = NotificationCampaignRecipientStatus.SENT + elif status == NotificationCampaignRecipientStatus.FAILED: + query['status__in'] = [NotificationCampaignRecipientStatus.FAILED, NotificationCampaignRecipientStatus.SKIPPED] qs = NotificationCampaignRecipient.objects.filter(**query) @@ -639,6 +652,13 @@ def post(self, request, *args, **kwargs): pk=kwargs['pk'], ) + if NotificationCampaign.objects.filter(status=NotificationCampaignStatus.RUNNING).exists(): + messages.error(request, 'Another campaign already running') + return redirect( + 'notifications:notification_campaigns_detail', + pk=notification_campaign.pk, + ) + restart_failed = request.GET.get('restart_failed') == 'true' notification_campaign.start(restart_failed=restart_failed) diff --git a/admin/templates/notifications/notification_campaigns_detail.html b/admin/templates/notifications/notification_campaigns_detail.html index 1710209eea0..6dad23ceda2 100644 --- a/admin/templates/notifications/notification_campaigns_detail.html +++ b/admin/templates/notifications/notification_campaigns_detail.html @@ -7,6 +7,16 @@ {% endblock title %} {% block content %} +
    + {% if messages %} +
      + {% for message in messages %} + {{ message }} + + {% endfor %} +
    + {% endif %} +
    @@ -54,13 +64,23 @@

    General

    {{ field }} {{ value|safe }} - {% if field == 'Sent' and value != 0 %} + {% if field == 'Recipients' and value != 0 %} + +
    + View Recipients + + + + {% elif field == 'Sent' and value != 0 %} - Preview Recipients + View Recipients @@ -70,7 +90,7 @@

    General

    href="{% url 'notifications:notification_campaigns_recipients_list' %}?notification_status=failed&campaign_id={{ notification_campaign.id }}" class="btn btn-default" target="_blank"> - Preview Recipients + View Recipients diff --git a/admin/templates/notifications/notification_campaing_create.html b/admin/templates/notifications/notification_campaing_create.html index 9b092673351..b8b4dcb72a6 100644 --- a/admin/templates/notifications/notification_campaing_create.html +++ b/admin/templates/notifications/notification_campaing_create.html @@ -7,6 +7,23 @@ {% endblock title %} {% block content %} +
    + {% if messages %} +
      + {% for message in messages %} + {{ message }} + + {% endfor %} +
    + {% endif %} +
    +
    + {% if form.errors %} +
    + {{ form.errors }} +
    + {% endif %} +
    @@ -135,7 +152,7 @@

    Template Context (JSON)

    "project": "OSF", "deadline": "2026-08-01" }' - > + >{{ default_context }}
    @@ -186,6 +203,18 @@

    Execution

    + + + Sendgrid Bulk + + + +
    diff --git a/osf/email/notification_campaign.py b/osf/email/notification_campaign.py index c2d242a59d7..ef981797ea2 100644 --- a/osf/email/notification_campaign.py +++ b/osf/email/notification_campaign.py @@ -2,7 +2,7 @@ from osf.models import NotificationType, NotificationTypeEnum, OSFUser, UserActivityCounter, Email from osf.models.spam import SpamStatus -from django.db.models import OuterRef, Subquery, F, Case, When, CharField +from django.db.models import OuterRef, Subquery, F, Case, When, CharField, Count, Q from django.db.models.functions import Coalesce from framework.celery_tasks import app as celery_app from celery import group, chain @@ -131,38 +131,58 @@ def process_campaign_retry(*args, **kwargs): max_retries = campaign.metadata.get('execution', {}).get('max_retries', settings.DEFAULT_CAMPAIGN_MAX_RETRIES) batch_size = campaign.metadata.get('execution', {}).get('batch_size', settings.DEFAULT_CAMPAIGN_BATCH_SIZE) failed_recipients_count = failed_recipients.count() - if not failed_recipients_count: + if failed_recipients_count: + if campaign.retries < max_retries: + message = ( + f"[Notification Campaign] Retrying " + f"{failed_recipients_count} failed recipients for campaign {campaign_id}" + ) + logger.info(message) + sentry.log_message(message) + campaign.retries += 1 + campaign.save(update_fields=['retries']) + retry_group = build_campaign_group( + batch_size=batch_size, + campaign_id=campaign_id, + restart_failed=True, + notification_type_name=campaign.notification_type.name, + context=campaign.metadata.get('context', {}), + run_id=campaign.run_id, + ) + chain( + retry_group, + process_campaign_retry.si(campaign_id=campaign_id), + ).apply_async() + return + + campaign.status = NotificationCampaignStatus.PARTIALLY_COMPLETED + else: campaign.status = NotificationCampaignStatus.COMPLETED - campaign.completed_at = timezone.now() - campaign.failed_count = 0 - campaign.save(update_fields=['status', 'completed_at', 'failed_count']) - return - if campaign.retries < max_retries: - message = f'[Notification Campaign] Retrying {failed_recipients_count} failed recipients for campaign {campaign_id}' - logger.info(message) - sentry.log_message(message) - - tasks = build_campaign_group( - batch_size=batch_size, - campaign_id=campaign_id, - restart_failed=True, - notification_type_name=campaign.notification_type.name, - context=campaign.metadata.get('context', {}), - run_id=campaign.run_id - ) + stats = NotificationCampaignRecipient.objects.filter( + campaign_id=campaign_id + ).aggregate( + recipient_count=Count('id'), + sent_count=Count( + 'id', + filter=Q(status=NotificationCampaignRecipientStatus.SENT), + ), + failed_count=Count( + 'id', + filter=Q( + status__in=[ + NotificationCampaignRecipientStatus.FAILED, + NotificationCampaignRecipientStatus.SKIPPED, + ] + ), + ), + ) - chain( - tasks, - process_campaign_retry.si(campaign_id=campaign_id) - ).apply_async() - campaign.retries += 1 - campaign.save(update_fields=['retries']) - else: - campaign.failed_count = failed_recipients_count - campaign.status = NotificationCampaignStatus.PARTIALLY_COMPLETED - campaign.completed_at = timezone.now() - campaign.save(update_fields=['status', 'completed_at', 'failed_count']) + campaign.recipient_count = stats['recipient_count'] + campaign.sent_count = stats['sent_count'] + campaign.failed_count = stats['failed_count'] + campaign.completed_at = timezone.now() + campaign.save() @celery_app.task(name='email.start_notification_campaign') @@ -188,6 +208,8 @@ def start_notification_campaign(campaign_id, restart_failed=False): if not restart_failed: create_campaign_recipients(filters=filters, campaign_id=campaign_id) + campaign.recipient_count = NotificationCampaignRecipient.objects.filter(campaign_id=campaign_id).count() + campaign.save() execution = campaign.metadata.get('execution', {}) batch_size = execution.get('batch_size', settings.DEFAULT_CAMPAIGN_BATCH_SIZE) @@ -260,18 +282,21 @@ def send_campaign_batch(context, recipients_ids, notification_type_name='blank', recipients_qs = NotificationCampaignRecipient.objects.filter(id__in=recipients_ids).select_related('user') recipient_records = [] + recipients_qs_annotated = recipients_qs.annotate( + recipient_address=Case( + When(user__username__contains='@', then='user__username'), + default=Subquery(first_email_subquery), + output_field=CharField(), + ) + ) + valid_emails_qs = recipients_qs_annotated.exclude(recipient_address__isnull=True) + invalid_emails_qs = recipients_qs_annotated.filter(recipient_address__isnull=True) + invalid_emails_qs.update(status=NotificationCampaignRecipientStatus.SKIPPED, error_message='Invalid email address') + success_count = 0 - failure_count = 0 + failure_count = invalid_emails_qs.count() + if campaign.metadata.get('sendgrid_bulk', False): - recipients_qs_annotated = recipients_qs.annotate( - recipient_address=Case( - When(user__username__contains='@', then='user_id'), - default=Subquery(first_email_subquery), - output_field=CharField(), - ) - ) - valid_emails_qs = recipients_qs_annotated.exclude(recipient_address__isnull=True) - invalid_emails_qs = recipients_qs_annotated.filter(recipient_address__isnull=True) recipient_emails = list(valid_emails_qs.values_list('recipient_address', flat=True)) success_count = len(recipient_emails) try: @@ -285,10 +310,9 @@ def send_campaign_batch(context, recipients_ids, notification_type_name='blank', failure_count += success_count success_count = 0 pass - invalid_emails_qs.update(status=NotificationCampaignRecipientStatus.SKIPPED, error_message='Invalid email address') else: - for recipient in recipients_qs: + for recipient in valid_emails_qs: try: notification_type.emit( user=recipient.user, diff --git a/osf/models/notification_campaign.py b/osf/models/notification_campaign.py index 759a50956a3..4f5000327e2 100644 --- a/osf/models/notification_campaign.py +++ b/osf/models/notification_campaign.py @@ -1,4 +1,4 @@ -from django.db import models +from django.db import models, transaction from django.utils import timezone import uuid @@ -83,7 +83,14 @@ def start(self, restart_failed=False): self.retries = 0 self.metadata.update({'template': self.notification_type.template}) self.save() - start_notification_campaign.delay(campaign_id=self.id, restart_failed=restart_failed) + + # run start_notification_campaign after transaction + transaction.on_commit( + lambda: start_notification_campaign.delay( + campaign_id=self.id, + restart_failed=restart_failed, + ) + ) class NotificationCampaignRecipient(models.Model): From dcfabd2b38f41176938df9e6ddeee63aa2e4c1c8 Mon Sep 17 00:00:00 2001 From: Ostap-Zherebetskyi Date: Mon, 27 Jul 2026 18:53:54 +0300 Subject: [PATCH 12/25] [ENG-11792] Implement campaign restart in case of catastrophe (#11831) * Add functionality to restart stuck notification campaigns * Add support for restarting stuck notification campaigns --- admin/notifications/views.py | 17 +++++++------- .../notification_campaigns_detail.html | 22 ++++++++++++++++++- osf/email/notification_campaign.py | 19 +++++++++------- osf/models/notification_campaign.py | 5 +++-- 4 files changed, 44 insertions(+), 19 deletions(-) diff --git a/admin/notifications/views.py b/admin/notifications/views.py index 8d06c9477b1..cea8e298eb7 100644 --- a/admin/notifications/views.py +++ b/admin/notifications/views.py @@ -1,7 +1,9 @@ import re import json from collections import defaultdict +from datetime import timedelta from django.urls import reverse_lazy +from django.utils import timezone from django.db.models import Q, F from django.db import models from django.shortcuts import get_object_or_404, redirect @@ -402,6 +404,7 @@ def get_context_data(self, *args, **kwargs): ('Created', notification_campaign.created_at), ('Started', notification_campaign.started_at), ('Completed', notification_campaign.completed_at), + ('Updated at', notification_campaign.updated_at), ], 'template': notification_campaign.notification_type.template, 'metadata': metadata, @@ -423,14 +426,11 @@ def get_context_data(self, *args, **kwargs): for k, v in metadata.items() if k not in {'filters', 'context', 'execution', 'template'} }, + 'allow_restart_stuck': True if timezone.now() - notification_campaign.updated_at > timedelta(minutes=15) else False, + 'sent_percent': notification_campaign.sent_count * 100 / notification_campaign.recipient_count if notification_campaign.recipient_count else 0, + 'failed_percent': notification_campaign.failed_count * 100 / notification_campaign.recipient_count if notification_campaign.recipient_count else 0, } - if notification_campaign.status == NotificationCampaignStatus.RUNNING: - context.update({ - 'sent_percent': notification_campaign.sent_count * 100 / notification_campaign.recipient_count if notification_campaign.recipient_count else 0, - 'failed_percent': notification_campaign.failed_count * 100 / notification_campaign.recipient_count if notification_campaign.recipient_count else 0, - }) - return context @@ -652,7 +652,7 @@ def post(self, request, *args, **kwargs): pk=kwargs['pk'], ) - if NotificationCampaign.objects.filter(status=NotificationCampaignStatus.RUNNING).exists(): + if NotificationCampaign.objects.filter(status=NotificationCampaignStatus.RUNNING).exclude(id=notification_campaign.id).exists(): messages.error(request, 'Another campaign already running') return redirect( 'notifications:notification_campaigns_detail', @@ -660,8 +660,9 @@ def post(self, request, *args, **kwargs): ) restart_failed = request.GET.get('restart_failed') == 'true' + restart_stuck = request.GET.get('restart_stuck') == 'true' - notification_campaign.start(restart_failed=restart_failed) + notification_campaign.start(restart_failed=restart_failed, restart_stuck=restart_stuck) return redirect( 'notifications:notification_campaigns_detail', diff --git a/admin/templates/notifications/notification_campaigns_detail.html b/admin/templates/notifications/notification_campaigns_detail.html index 6dad23ceda2..113248e2067 100644 --- a/admin/templates/notifications/notification_campaigns_detail.html +++ b/admin/templates/notifications/notification_campaigns_detail.html @@ -26,7 +26,7 @@

    {{ notification_campaign.name }}

    - {% if notification_campaign.status == 'running' %} + {% if notification_campaign.status != 'created' %}

    Progress

    @@ -54,6 +54,18 @@

    Progress

    Start Campaign +
    + {% csrf_token %} + +
    @@ -317,5 +329,13 @@

    Additional Metadata

    }); } + const restartstuckForm = document.getElementById("restart-stuck-campaign-form"); + + if (restartstuckForm) { + restartstuckForm.addEventListener("submit", function (e) { + confirmCampaign(this, e); + }); + } + {% endblock %} diff --git a/osf/email/notification_campaign.py b/osf/email/notification_campaign.py index ef981797ea2..16b5bb9b595 100644 --- a/osf/email/notification_campaign.py +++ b/osf/email/notification_campaign.py @@ -44,12 +44,15 @@ def create_campaign_recipients(filters, campaign_id): for rows in batched(qs.iterator(chunk_size=BULK_CREATE_SIZE), BULK_CREATE_SIZE): NotificationCampaignRecipient.objects.bulk_create( - NotificationCampaignRecipient( - campaign_id=campaign_id, - user_id=user_id, - activity_score=activity_score, - ) - for user_id, activity_score in rows + [ + NotificationCampaignRecipient( + campaign_id=campaign_id, + user_id=user_id, + activity_score=activity_score, + ) + for user_id, activity_score in rows + ], + ignore_conflicts=True ) @@ -186,7 +189,7 @@ def process_campaign_retry(*args, **kwargs): @celery_app.task(name='email.start_notification_campaign') -def start_notification_campaign(campaign_id, restart_failed=False): +def start_notification_campaign(campaign_id, restart_failed=False, restart_stuck=False): campaign = NotificationCampaign.objects.get(id=campaign_id) filters = campaign.metadata.get('filters', {}) context = campaign.metadata.get('context', {}) @@ -206,7 +209,7 @@ def start_notification_campaign(campaign_id, restart_failed=False): manual_filters[f'{item["field"]}__{item["lookup"]}'] = [value.strip() for value in item['value'].split(',')] filters = manual_filters - if not restart_failed: + if not restart_failed and not restart_stuck: create_campaign_recipients(filters=filters, campaign_id=campaign_id) campaign.recipient_count = NotificationCampaignRecipient.objects.filter(campaign_id=campaign_id).count() campaign.save() diff --git a/osf/models/notification_campaign.py b/osf/models/notification_campaign.py index 4f5000327e2..b1822400964 100644 --- a/osf/models/notification_campaign.py +++ b/osf/models/notification_campaign.py @@ -71,12 +71,12 @@ class NotificationCampaign(models.Model): developer_reminder_sent = models.BooleanField(default=False) - def start(self, restart_failed=False): + def start(self, restart_failed=False, restart_stuck=False): from osf.email.notification_campaign import start_notification_campaign self.status = NotificationCampaignStatus.RUNNING self.started_at = timezone.now() self.run_id = uuid.uuid4() - if not restart_failed: + if not restart_failed and not restart_stuck: self.recipient_count = 0 self.sent_count = 0 self.failed_count = 0 @@ -89,6 +89,7 @@ def start(self, restart_failed=False): lambda: start_notification_campaign.delay( campaign_id=self.id, restart_failed=restart_failed, + restart_stuck=restart_stuck, ) ) From 0ee21d267d5f5bf05835c6c5f2b859eb239d3dbe Mon Sep 17 00:00:00 2001 From: antkryt Date: Wed, 29 Jul 2026 17:20:15 +0300 Subject: [PATCH 13/25] [ENG-11797] Unit tests on updated campaign workflow - Part 1 (#11832) * Add unit tests for notification campaign flow and admin (part 1) --- admin/notifications/forms.py | 4 +- admin_tests/notifications/test_campaigns.py | 303 +++++++++++++ osf/email/notification_campaign.py | 76 ++-- osf_tests/test_notification_campaign.py | 477 +++++++++++++++++++- 4 files changed, 817 insertions(+), 43 deletions(-) create mode 100644 admin_tests/notifications/test_campaigns.py diff --git a/admin/notifications/forms.py b/admin/notifications/forms.py index 9a5ef522314..4bce2e1c719 100644 --- a/admin/notifications/forms.py +++ b/admin/notifications/forms.py @@ -56,11 +56,11 @@ def clean_context(self): try: return json.loads(value) except Exception as e: - forms.ValidationError(e) + raise forms.ValidationError(e) def clean_filters(self): value = self.cleaned_data['filters'] or '{}' try: return json.loads(value) except Exception as e: - forms.ValidationError(e) + raise forms.ValidationError(e) diff --git a/admin_tests/notifications/test_campaigns.py b/admin_tests/notifications/test_campaigns.py new file mode 100644 index 00000000000..336cb35cbd4 --- /dev/null +++ b/admin_tests/notifications/test_campaigns.py @@ -0,0 +1,303 @@ +import json +import pytest +from unittest import mock + +from django.contrib.auth.models import Permission +from django.contrib.messages.storage.fallback import FallbackStorage +from django.core.exceptions import PermissionDenied +from django.test import RequestFactory +from django.urls import reverse + +from admin.notifications.forms import NotificationCampaignCreateForm +from admin.notifications.views import ( + NotificationCampaignCreateView, + NotificationCampaignDetail, + NotificationCampaignsList, + StartNotificationCampaign, +) +from admin_tests.utilities import setup_form_view +from osf.models import NotificationType +from osf.models.notification_campaign import ( + NotificationCampaign, +) +from osf_tests.factories import AuthUserFactory +from tests.base import AdminTestCase +from website import settings + + +def patch_messages(request): + setattr(request, 'session', 'session') + messages = FallbackStorage(request) + setattr(request, '_messages', messages) + + +def grant_permission(user, codename): + user.user_permissions.add(Permission.objects.get(codename=codename)) + for attr in ('_perm_cache', '_user_perm_cache', '_group_perm_cache'): + if hasattr(user, attr): + delattr(user, attr) + + +@pytest.fixture +def notification_type(): + notification_type, _ = NotificationType.objects.get_or_create( + name='blank', + defaults={'subject': 'Test', 'template': 'Hello {{ name }}'}, + ) + return notification_type + + +def _valid_form_data(notification_type, **overrides): + data = { + 'name': 'My Campaign', + 'notification_type': notification_type.id, + 'context': '{"greeting": "hi"}', + 'filters': json.dumps({'predefined': 'active'}), + 'batch_size': settings.DEFAULT_CAMPAIGN_BATCH_SIZE, + 'max_retries': settings.DEFAULT_CAMPAIGN_MAX_RETRIES, + 'activity_threshold': settings.DEFAULT_CAMPAIGN_ACTIVITY_THRESHOLD, + 'sendgrid_bulk': False, + } + data.update(overrides) + return data + + +class TestNotificationCampaignCreateForm: + + def test_valid_form_parses_context_and_filters(self, notification_type): + form = NotificationCampaignCreateForm(data=_valid_form_data(notification_type)) + assert form.is_valid() + assert form.cleaned_data['context'] == {'greeting': 'hi'} + assert form.cleaned_data['filters'] == {'predefined': 'active'} + 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['sendgrid_bulk'] is False + + def test_defaults_come_from_settings(self): + form = NotificationCampaignCreateForm() + 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['sendgrid_bulk'].initial is False + + def test_invalid_context_json(self, notification_type): + form = NotificationCampaignCreateForm( + data=_valid_form_data(notification_type, context='{not-json') + ) + assert not form.is_valid() + assert 'context' in form.errors + + def test_invalid_filters_json(self, notification_type): + form = NotificationCampaignCreateForm( + data=_valid_form_data(notification_type, filters='[1, 2,') + ) + assert not form.is_valid() + assert 'filters' in form.errors + + def test_empty_context_and_filters_default_to_empty_dict(self, notification_type): + form = NotificationCampaignCreateForm( + data=_valid_form_data(notification_type, context='', filters='') + ) + assert form.is_valid() + assert form.cleaned_data['context'] == {} + assert form.cleaned_data['filters'] == {} + + def test_batch_size_must_be_at_least_one(self, notification_type): + form = NotificationCampaignCreateForm( + data=_valid_form_data(notification_type, batch_size=0) + ) + assert not form.is_valid() + assert 'batch_size' in form.errors + + def test_max_retries_cannot_be_negative(self, notification_type): + form = NotificationCampaignCreateForm( + data=_valid_form_data(notification_type, max_retries=-1) + ) + assert not form.is_valid() + assert 'max_retries' in form.errors + + def test_activity_threshold_cannot_be_negative(self, notification_type): + form = NotificationCampaignCreateForm( + data=_valid_form_data(notification_type, activity_threshold=-1) + ) + assert not form.is_valid() + assert 'activity_threshold' in form.errors + + def test_name_is_required(self, notification_type): + form = NotificationCampaignCreateForm( + data=_valid_form_data(notification_type, name='') + ) + assert not form.is_valid() + assert 'name' in form.errors + + def test_notification_type_is_required(self): + form = NotificationCampaignCreateForm( + data={ + 'name': 'My Campaign', + 'context': '{}', + 'filters': '{}', + 'batch_size': 10, + 'max_retries': 1, + 'activity_threshold': 5, + } + ) + assert not form.is_valid() + assert 'notification_type' in form.errors + + def test_sendgrid_bulk_can_be_enabled(self, notification_type): + form = NotificationCampaignCreateForm( + data=_valid_form_data(notification_type, sendgrid_bulk=True) + ) + assert form.is_valid() + assert form.cleaned_data['sendgrid_bulk'] is True + + +class TestNotificationCampaignCreateView(AdminTestCase): + + def setUp(self): + super().setUp() + self.user = AuthUserFactory() + self.notification_type, _ = NotificationType.objects.get_or_create( + name='blank', + defaults={'subject': 'Test', 'template': 'Hello'}, + ) + + def test_form_valid_persists_execution_metadata(self): + request = RequestFactory().post( + reverse('notifications:notification_campaigns_create'), + data=_valid_form_data( + self.notification_type, + batch_size=25, + max_retries=4, + activity_threshold=77, + sendgrid_bulk=True, + ), + ) + request.user = self.user + patch_messages(request) + + form = NotificationCampaignCreateForm(data=request.POST) + assert form.is_valid() + + view = setup_form_view( + NotificationCampaignCreateView(), + request, + form, + ) + view.form_valid(form) + + campaign = NotificationCampaign.objects.get(name='My Campaign') + assert campaign.created_by == self.user + assert campaign.metadata['execution'] == { + 'batch_size': 25, + 'max_retries': 4, + 'activity_threshold': 77, + } + assert campaign.metadata['sendgrid_bulk'] is True + assert campaign.metadata['filters'] == {'predefined': 'active'} + assert campaign.metadata['context'] == {'greeting': 'hi'} + + @mock.patch('admin.notifications.views._render_email_html', side_effect=Exception('bad template')) + def test_form_valid_rejects_unrenderable_context(self, mock_render): + request = RequestFactory().post( + reverse('notifications:notification_campaigns_create'), + data=_valid_form_data(self.notification_type), + ) + request.user = self.user + patch_messages(request) + + form = NotificationCampaignCreateForm(data=request.POST) + assert form.is_valid() + + view = setup_form_view( + NotificationCampaignCreateView(), + request, + form, + ) + with mock.patch.object(view, 'form_invalid', return_value=mock.Mock(status_code=200)) as mock_invalid: + view.form_valid(form) + + mock_invalid.assert_called_once_with(form) + assert 'context' in form.errors + assert 'Failed to render template' in form.errors['context'][0] + assert not NotificationCampaign.objects.filter(name='My Campaign').exists() + + +class TestNotificationCampaignAdminPermissions(AdminTestCase): + + def setUp(self): + super().setUp() + self.user = AuthUserFactory() + self.notification_type, _ = NotificationType.objects.get_or_create( + name='blank', + defaults={'subject': 'Test', 'template': 'Hello'}, + ) + self.campaign = NotificationCampaign.objects.create( + name='Campaign', + notification_type=self.notification_type, + metadata={'filters': {}, 'context': {}, 'execution': {}}, + ) + + def test_list_requires_view_permission(self): + request = RequestFactory().get(reverse('notifications:notification_campaigns_list')) + request.user = self.user + + with self.assertRaises(PermissionDenied): + NotificationCampaignsList.as_view()(request) + + grant_permission(self.user, 'view_notificationcampaign') + response = NotificationCampaignsList.as_view()(request) + assert response.status_code == 200 + + def test_detail_requires_change_permission(self): + request = RequestFactory().get( + reverse('notifications:notification_campaigns_detail', kwargs={'pk': self.campaign.pk}) + ) + request.user = self.user + + with self.assertRaises(PermissionDenied): + NotificationCampaignDetail.as_view()(request, pk=self.campaign.pk) + + grant_permission(self.user, 'change_notificationcampaign') + response = NotificationCampaignDetail.as_view()(request, pk=self.campaign.pk) + assert response.status_code == 200 + + def test_start_requires_change_notificationtype_permission(self): + request = RequestFactory().post( + reverse('notifications:notification_campaigns_start', kwargs={'pk': self.campaign.pk}) + ) + request.user = self.user + + with self.assertRaises(PermissionDenied): + StartNotificationCampaign.as_view()(request, pk=self.campaign.pk) + + grant_permission(self.user, 'change_notificationtype') + with mock.patch.object(NotificationCampaign, 'start') as mock_start: + response = StartNotificationCampaign.as_view()(request, pk=self.campaign.pk) + assert response.status_code == 302 + mock_start.assert_called_once_with(restart_failed=False, restart_stuck=False) + + def test_start_rejects_when_another_campaign_is_running(self): + from osf.models.notification_campaign import NotificationCampaignStatus + + grant_permission(self.user, 'change_notificationtype') + NotificationCampaign.objects.create( + name='Already Running', + notification_type=self.notification_type, + status=NotificationCampaignStatus.RUNNING, + metadata={'filters': {}, 'context': {}, 'execution': {}}, + ) + request = RequestFactory().post( + reverse('notifications:notification_campaigns_start', kwargs={'pk': self.campaign.pk}) + ) + request.user = self.user + patch_messages(request) + + with mock.patch.object(NotificationCampaign, 'start') as mock_start: + response = StartNotificationCampaign.as_view()(request, pk=self.campaign.pk) + + assert response.status_code == 302 + mock_start.assert_not_called() + self.campaign.refresh_from_db() + assert self.campaign.status == NotificationCampaignStatus.CREATED diff --git a/osf/email/notification_campaign.py b/osf/email/notification_campaign.py index 16b5bb9b595..89853c8064e 100644 --- a/osf/email/notification_campaign.py +++ b/osf/email/notification_campaign.py @@ -2,7 +2,8 @@ from osf.models import NotificationType, NotificationTypeEnum, OSFUser, UserActivityCounter, Email from osf.models.spam import SpamStatus -from django.db.models import OuterRef, Subquery, F, Case, When, CharField, Count, Q +from django.db import transaction +from django.db.models import OuterRef, Subquery, Case, When, CharField, Count, Q from django.db.models.functions import Coalesce from framework.celery_tasks import app as celery_app from celery import group, chain @@ -17,6 +18,11 @@ logger = logging.getLogger(__name__) BULK_CREATE_SIZE = 5000 +FILTER_PRESETS = { + 'all': {}, + 'active': {'is_active': True}, + 'internal': {'is_active': True, 'is_staff': True, 'username__endswith': '@cos.io'}, +} first_email_subquery = ( Email.objects @@ -120,11 +126,25 @@ def build_campaign_group( return group(tasks) -FILTER_PRESETS = { - 'all': {}, - 'active': {'is_active': True}, - 'internal': {'is_active': True, 'is_staff': True, 'username__endswith': '@cos.io'}, -} +def get_campaign_recipient_stats(campaign_id): + return NotificationCampaignRecipient.objects.filter( + campaign_id=campaign_id + ).aggregate( + recipient_count=Count('id'), + sent_count=Count( + 'id', + filter=Q(status=NotificationCampaignRecipientStatus.SENT), + ), + failed_count=Count( + 'id', + filter=Q( + status__in=[ + NotificationCampaignRecipientStatus.FAILED, + NotificationCampaignRecipientStatus.SKIPPED, + ] + ), + ), + ) @celery_app.task(name='email.process_campaign_retry') def process_campaign_retry(*args, **kwargs): @@ -162,25 +182,7 @@ def process_campaign_retry(*args, **kwargs): else: campaign.status = NotificationCampaignStatus.COMPLETED - stats = NotificationCampaignRecipient.objects.filter( - campaign_id=campaign_id - ).aggregate( - recipient_count=Count('id'), - sent_count=Count( - 'id', - filter=Q(status=NotificationCampaignRecipientStatus.SENT), - ), - failed_count=Count( - 'id', - filter=Q( - status__in=[ - NotificationCampaignRecipientStatus.FAILED, - NotificationCampaignRecipientStatus.SKIPPED, - ] - ), - ), - ) - + stats = get_campaign_recipient_stats(campaign_id) campaign.recipient_count = stats['recipient_count'] campaign.sent_count = stats['sent_count'] campaign.failed_count = stats['failed_count'] @@ -296,23 +298,16 @@ def send_campaign_batch(context, recipients_ids, notification_type_name='blank', invalid_emails_qs = recipients_qs_annotated.filter(recipient_address__isnull=True) invalid_emails_qs.update(status=NotificationCampaignRecipientStatus.SKIPPED, error_message='Invalid email address') - success_count = 0 - failure_count = invalid_emails_qs.count() - if campaign.metadata.get('sendgrid_bulk', False): recipient_emails = list(valid_emails_qs.values_list('recipient_address', flat=True)) - success_count = len(recipient_emails) try: send_email_with_send_grid(to_addr=recipient_emails, notification_type=notification_type, context=context) + valid_emails_qs.update(status=NotificationCampaignRecipientStatus.SENT, error_message=None) except Exception as exc: message = f'[Notification Campaign] Campaign {campaign_id} sendgrid bulk request failed. {str(exc)}' logger.error(message) sentry.log_exception(message) - valid_emails_qs.update(status=NotificationCampaignRecipientStatus.FAILED, error_message=str(exc)) - failure_count += success_count - success_count = 0 - pass else: for recipient in valid_emails_qs: @@ -326,8 +321,6 @@ def send_campaign_batch(context, recipients_ids, notification_type_name='blank', recipient.status = NotificationCampaignRecipientStatus.SENT recipient.error_message = None recipient_records.append(recipient) - success_count += 1 - except Exception as exc: message = f'[Notification Campaign] Campaign {campaign_id} sendgrid request failed for user {recipient.user.username}. {str(exc)}' logger.error(message) @@ -337,10 +330,15 @@ def send_campaign_batch(context, recipients_ids, notification_type_name='blank', recipient.error_message = str(exc) recipient_records.append(recipient) - failure_count += 1 - pass + NotificationCampaignRecipient.objects.bulk_update(recipient_records, ['status', 'error_message']) - NotificationCampaignRecipient.objects.bulk_update(recipient_records, ['status', 'error_message']) + # Lock the campaign row so concurrent batches cannot + # overwrite counters with a stale aggregate snapshot + with transaction.atomic(): + notification_campaign = NotificationCampaign.objects.select_for_update().get(pk=campaign_id) + stats = get_campaign_recipient_stats(campaign_id) + notification_campaign.sent_count = stats['sent_count'] + notification_campaign.failed_count = stats['failed_count'] + notification_campaign.save(update_fields=['sent_count', 'failed_count']) - NotificationCampaign.objects.filter(pk=campaign_id).update(sent_count=F('sent_count') + success_count, failed_count=F('failed_count') + failure_count) logger.info('Batch finished') # TODO: add/update logs diff --git a/osf_tests/test_notification_campaign.py b/osf_tests/test_notification_campaign.py index 0236a6cbaf7..a0eb6f5e477 100644 --- a/osf_tests/test_notification_campaign.py +++ b/osf_tests/test_notification_campaign.py @@ -1,17 +1,26 @@ import pytest +import uuid from datetime import timedelta +from unittest import mock from django.utils import timezone from osf.email.notification_campaign import ( create_campaign_recipients, get_campaign_recipient_batches, + get_campaign_recipient_stats, + process_campaign_retry, + send_campaign_batch, + start_notification_campaign, ) -from osf.models import NotificationType, UserActivityCounter +from osf.models import UserActivityCounter from osf.models.notification_campaign import ( NotificationCampaign, NotificationCampaignRecipient, + NotificationCampaignRecipientStatus, + NotificationCampaignStatus, ) +from osf.models.notification_type import NotificationType from osf.models.spam import SpamStatus from osf_tests.factories import UserFactory @@ -29,7 +38,15 @@ def campaign(notification_type): return NotificationCampaign.objects.create( name='Test campaign', notification_type=notification_type, - metadata={'execution': {'activity_threshold': 100, 'batch_size': 2}}, + metadata={ + 'filters': {'manual': []}, + 'context': {}, + 'execution': { + 'activity_threshold': 100, + 'batch_size': 2, + 'max_retries': 2, + }, + }, ) @@ -127,6 +144,69 @@ def test_ordered_by_activity_score_descending(self, campaign): assert user_ids == [newer.id, mid.id, older.id] +class TestGetCampaignRecipientStats: + + def test_empty_campaign(self, campaign): + assert get_campaign_recipient_stats(campaign.id) == { + 'recipient_count': 0, + 'sent_count': 0, + 'failed_count': 0, + } + + def test_counts_sent_failed_and_skipped(self, campaign, notification_type): + sent = UserFactory() + failed = UserFactory() + skipped = UserFactory() + pending = UserFactory() + create_campaign_recipients( + filters={'id__in': [sent.id, failed.id, skipped.id, pending.id]}, + campaign_id=campaign.id, + ) + NotificationCampaignRecipient.objects.filter(campaign=campaign, user=sent).update( + status=NotificationCampaignRecipientStatus.SENT + ) + NotificationCampaignRecipient.objects.filter(campaign=campaign, user=failed).update( + status=NotificationCampaignRecipientStatus.FAILED + ) + NotificationCampaignRecipient.objects.filter(campaign=campaign, user=skipped).update( + status=NotificationCampaignRecipientStatus.SKIPPED + ) + + assert get_campaign_recipient_stats(campaign.id) == { + 'recipient_count': 4, + 'sent_count': 1, + 'failed_count': 2, # FAILED + SKIPPED + } + + def test_scopes_to_requested_campaign(self, campaign, notification_type): + other = NotificationCampaign.objects.create( + name='Other campaign', + notification_type=notification_type, + metadata={'filters': {}, 'context': {}, 'execution': {}}, + ) + user = UserFactory() + other_user = UserFactory() + create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=campaign.id) + create_campaign_recipients(filters={'id__in': [other_user.id]}, campaign_id=other.id) + NotificationCampaignRecipient.objects.filter(campaign=campaign).update( + status=NotificationCampaignRecipientStatus.SENT + ) + NotificationCampaignRecipient.objects.filter(campaign=other).update( + status=NotificationCampaignRecipientStatus.FAILED + ) + + assert get_campaign_recipient_stats(campaign.id) == { + 'recipient_count': 1, + 'sent_count': 1, + 'failed_count': 0, + } + assert get_campaign_recipient_stats(other.id) == { + 'recipient_count': 1, + 'sent_count': 0, + 'failed_count': 1, + } + + class TestGetCampaignRecipientBatches: @pytest.fixture @@ -208,3 +288,396 @@ def test_batches_respect_batch_size(self, campaign): assert [len(batch) for batch in batches] == [2, 2, 1] flat = {recipient_id for batch in batches for recipient_id in batch} assert len(flat) == 5 + + def test_restart_failed_only_returns_failed(self, campaign, users_and_recipients): + data = users_and_recipients + failed = NotificationCampaignRecipient.objects.get(campaign=campaign, user=data['high']) + failed.status = NotificationCampaignRecipientStatus.FAILED + failed.save(update_fields=['status']) + + pending_ids = _recipient_user_ids(campaign.id, spam=False, min_activity=data['threshold']) + assert data['high'].id not in pending_ids + + failed_ids = _recipient_user_ids(campaign.id, restart_failed=True) + assert failed_ids == [data['high'].id] + + def test_ignore_conflicts_on_duplicate_create(self, campaign): + user = UserFactory() + _set_activity(user, 10) + create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=campaign.id) + create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=campaign.id) + assert NotificationCampaignRecipient.objects.filter(campaign=campaign).count() == 1 + + +class TestNotificationCampaignStart: + + @mock.patch('osf.email.notification_campaign.start_notification_campaign.delay') + def test_campaign_start_sets_running_state(self, mock_delay, campaign): + with mock.patch( + 'osf.models.notification_campaign.transaction.on_commit', + side_effect=lambda callback: callback(), + ): + campaign.start() + + campaign.refresh_from_db() + assert campaign.status == NotificationCampaignStatus.RUNNING + assert campaign.run_id is not None + assert campaign.started_at is not None + assert campaign.retries == 0 + mock_delay.assert_called_once_with( + campaign_id=campaign.id, + restart_failed=False, + restart_stuck=False, + ) + + @mock.patch('osf.email.notification_campaign.start_notification_campaign.delay') + def test_campaign_start_restart_stuck_counts(self, mock_delay, campaign): + campaign.recipient_count = 10 + campaign.sent_count = 4 + campaign.failed_count = 4 + campaign.retries = 1 + campaign.save() + + with mock.patch( + 'osf.models.notification_campaign.transaction.on_commit', + side_effect=lambda callback: callback(), + ): + campaign.start(restart_stuck=True) + + campaign.refresh_from_db() + assert campaign.recipient_count == 10 + assert campaign.sent_count == 4 + assert campaign.failed_count == 0 + assert campaign.retries == 0 + mock_delay.assert_called_once_with( + campaign_id=campaign.id, + restart_failed=False, + restart_stuck=True, + ) + + @mock.patch('osf.email.notification_campaign.chain') + def test_start_creates_recipients_and_schedules_workflow(self, mock_chain, campaign): + high = UserFactory() + low = UserFactory() + spam = UserFactory() + spam.spam_status = SpamStatus.SPAM + spam.save() + _set_activity(high, 250) + _set_activity(low, 10) + + campaign.metadata['filters'] = { + 'manual': [{'field': 'id', 'lookup': 'in', 'value': f'{high.id},{low.id},{spam.id}'}], + } + campaign.run_id = uuid.uuid4() + campaign.save() + + mock_chain.return_value.apply_async = mock.Mock() + + start_notification_campaign(campaign.id) + + campaign.refresh_from_db() + recipients = { + r.user_id: r.activity_score + for r in NotificationCampaignRecipient.objects.filter(campaign=campaign) + } + assert recipients == {high.id: 250, low.id: 10, spam.id: 0} + assert campaign.recipient_count == 3 + mock_chain.assert_called_once() + mock_chain.return_value.apply_async.assert_called_once() + + @mock.patch('osf.email.notification_campaign.chain') + def test_start_restart_failed_does_not_recreate_recipients(self, mock_chain, campaign): + user = UserFactory() + _set_activity(user, 50) + create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=campaign.id) + recipient = NotificationCampaignRecipient.objects.get(campaign=campaign, user=user) + recipient.status = NotificationCampaignRecipientStatus.FAILED + recipient.save(update_fields=['status']) + + campaign.run_id = uuid.uuid4() + campaign.metadata['filters'] = { + 'manual': [{'field': 'id', 'lookup': 'in', 'value': str(user.id)}], + } + campaign.save() + mock_chain.return_value.apply_async = mock.Mock() + + start_notification_campaign(campaign.id, restart_failed=True) + + assert NotificationCampaignRecipient.objects.filter(campaign=campaign).count() == 1 + assert NotificationCampaignRecipient.objects.get(pk=recipient.pk).status == NotificationCampaignRecipientStatus.FAILED + + @mock.patch('osf.email.notification_campaign.chain') + def test_start_restart_stuck_does_not_recreate_recipients(self, mock_chain, campaign): + user = UserFactory() + create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=campaign.id) + campaign.run_id = uuid.uuid4() + campaign.recipient_count = 1 + campaign.save() + mock_chain.return_value.apply_async = mock.Mock() + + start_notification_campaign(campaign.id, restart_stuck=True) + + assert NotificationCampaignRecipient.objects.filter(campaign=campaign).count() == 1 + campaign.refresh_from_db() + assert campaign.recipient_count == 1 + + +class TestSendCampaignBatch: + + @pytest.fixture + def running_campaign(self, campaign): + campaign.run_id = uuid.uuid4() + campaign.started_at = timezone.now() + campaign.status = NotificationCampaignStatus.RUNNING + campaign.save() + return campaign + + @mock.patch.object(NotificationType, 'emit') + def test_send_campaign_batch_marks_recipients_sent(self, mock_emit, running_campaign): + user = UserFactory() + 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 + mock_emit.assert_called_once() + + @mock.patch.object(NotificationType, 'emit', side_effect=Exception('send failed')) + @mock.patch('osf.email.notification_campaign.sentry.log_exception') + def test_send_campaign_batch_marks_recipients_failed(self, mock_sentry, mock_emit, running_campaign): + user = UserFactory() + 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.FAILED + assert 'send failed' in recipient.error_message + assert running_campaign.failed_count == 1 + + def test_send_campaign_batch_skips_invalid_email_addresses(self, running_campaign): + user = UserFactory() + user.username = 'asd' + user.save(update_fields=['username']) + user.emails.all().delete() + + 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.SKIPPED + assert recipient.error_message == 'Invalid email address' + assert running_campaign.failed_count == 1 + assert running_campaign.sent_count == 0 + + @mock.patch('osf.email.notification_campaign.send_email_with_send_grid') + def test_send_campaign_batch_sendgrid_bulk_success(self, mock_sendgrid, running_campaign): + user = UserFactory() + running_campaign.metadata['sendgrid_bulk'] = True + running_campaign.save(update_fields=['metadata']) + 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() + mock_sendgrid.assert_called_once() + assert recipient.status == NotificationCampaignRecipientStatus.SENT + assert running_campaign.sent_count == 1 + assert running_campaign.failed_count == 0 + + @mock.patch( + 'osf.email.notification_campaign.send_email_with_send_grid', + side_effect=Exception('bulk failed'), + ) + @mock.patch('osf.email.notification_campaign.sentry.log_exception') + def test_send_campaign_batch_sendgrid_bulk_failure(self, mock_sentry, mock_sendgrid, running_campaign): + user = UserFactory() + running_campaign.metadata['sendgrid_bulk'] = True + running_campaign.save(update_fields=['metadata']) + 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.FAILED + assert running_campaign.failed_count == 1 + assert running_campaign.sent_count == 0 + + @mock.patch('osf.email.notification_campaign.sentry.log_message') + def test_send_campaign_batch_logs_when_time_window_exceeded(self, mock_sentry, running_campaign): + user = UserFactory() + running_campaign.started_at = timezone.now() - timedelta(hours=9) + running_campaign.metadata['execution']['time_window'] = 8 + running_campaign.save() + create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=running_campaign.id) + recipient = NotificationCampaignRecipient.objects.get(campaign=running_campaign, user=user) + + with mock.patch.object(NotificationType, 'emit'): + send_campaign_batch( + context={}, + recipients_ids=[recipient.id], + notification_type_name='blank', + campaign_id=running_campaign.id, + run_id=running_campaign.run_id, + ) + + running_campaign.refresh_from_db() + assert running_campaign.developer_reminder_sent is True + mock_sentry.assert_called_once() + + def test_send_campaign_batch_marks_failed_when_notification_type_missing(self, running_campaign): + user = UserFactory() + 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='does-not-exist', + campaign_id=running_campaign.id, + run_id=running_campaign.run_id, + ) + + running_campaign.refresh_from_db() + recipient.refresh_from_db() + assert running_campaign.status == NotificationCampaignStatus.FAILED + assert recipient.status == NotificationCampaignRecipientStatus.PENDING + + def test_send_campaign_batch_skips_stale_run_id(self, running_campaign): + user = UserFactory() + 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=uuid.uuid4(), + ) + + recipient.refresh_from_db() + assert recipient.status == NotificationCampaignRecipientStatus.PENDING + + def test_send_campaign_batch_skips_cancelled_campaign(self, running_campaign): + user = UserFactory() + running_campaign.status = NotificationCampaignStatus.CANCELLED + running_campaign.save(update_fields=['status']) + 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() + assert recipient.status == NotificationCampaignRecipientStatus.PENDING + + +class TestProcessCampaignRetry: + + def test_process_campaign_retry_marks_completed_and_aggregates_stats(self, campaign): + sent_user = UserFactory() + skipped_user = UserFactory() + create_campaign_recipients( + filters={'id__in': [sent_user.id, skipped_user.id]}, + campaign_id=campaign.id, + ) + NotificationCampaignRecipient.objects.filter(campaign=campaign, user=sent_user).update( + status=NotificationCampaignRecipientStatus.SENT + ) + NotificationCampaignRecipient.objects.filter(campaign=campaign, user=skipped_user).update( + status=NotificationCampaignRecipientStatus.SKIPPED + ) + + process_campaign_retry(campaign_id=campaign.id) + + campaign.refresh_from_db() + assert campaign.status == NotificationCampaignStatus.COMPLETED + assert campaign.recipient_count == 2 + assert campaign.sent_count == 1 + assert campaign.failed_count == 1 + assert campaign.completed_at is not None + + @mock.patch('osf.email.notification_campaign.chain') + def test_process_campaign_retry_retries_failed_recipients(self, mock_chain, campaign): + user = UserFactory() + create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=campaign.id) + recipient = NotificationCampaignRecipient.objects.get(campaign=campaign, user=user) + recipient.status = NotificationCampaignRecipientStatus.FAILED + recipient.save(update_fields=['status']) + campaign.run_id = uuid.uuid4() + campaign.retries = 0 + campaign.save() + mock_chain.return_value.apply_async = mock.Mock() + + process_campaign_retry(campaign_id=campaign.id) + + campaign.refresh_from_db() + assert campaign.retries == 1 + assert campaign.status != NotificationCampaignStatus.PARTIALLY_COMPLETED + mock_chain.assert_called_once() + + def test_process_campaign_retry_marks_partially_completed_after_max_retries(self, campaign): + user = UserFactory() + create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=campaign.id) + recipient = NotificationCampaignRecipient.objects.get(campaign=campaign, user=user) + recipient.status = NotificationCampaignRecipientStatus.FAILED + recipient.save(update_fields=['status']) + campaign.retries = 2 + campaign.save(update_fields=['retries']) + + process_campaign_retry(campaign_id=campaign.id) + + campaign.refresh_from_db() + assert campaign.status == NotificationCampaignStatus.PARTIALLY_COMPLETED + assert campaign.failed_count == 1 + assert campaign.recipient_count == 1 + assert campaign.completed_at is not None From 3337e4e0bd5c08399c8a0183cc750d5fbc6849fb Mon Sep 17 00:00:00 2001 From: Ostap-Zherebetskyi Date: Wed, 29 Jul 2026 18:48:36 +0300 Subject: [PATCH 14/25] [ENG-11820] Admin Campaign Page Clean-up / Polish + Optional Features (#11836) * Add time window field to NotificationCampaignCreateForm and update templates for campaign details and recipients * Add cancel functionality to NotificationCampaign and update UI for campaign management * Refactor process_campaign_retry to handle cancelled campaigns and improve retry logic * Add run_id check to process_campaign_retry to prevent incorrect retries * Fix unit tests --- admin/notifications/forms.py | 6 + admin/notifications/views.py | 79 +++++++++- .../notification_campaigns_detail.html | 149 ++++++++++++++++-- .../notification_campaigns_list.html | 22 +++ .../notification_campaing_create.html | 15 ++ ...notification_campaing_recipients_list.html | 4 + ...ification_campaing_recipients_preview.html | 4 + admin_tests/notifications/test_campaigns.py | 3 + osf/email/notification_campaign.py | 81 ++++++---- osf/models/notification_campaign.py | 6 + osf_tests/test_notification_campaign.py | 2 +- 11 files changed, 319 insertions(+), 52 deletions(-) diff --git a/admin/notifications/forms.py b/admin/notifications/forms.py index 4bce2e1c719..ec104ab7e1b 100644 --- a/admin/notifications/forms.py +++ b/admin/notifications/forms.py @@ -39,6 +39,12 @@ class NotificationCampaignCreateForm(forms.ModelForm): help_text='Non-spam users at or above this activity total are sent in the high-activity phase.', ) + time_window = forms.IntegerField( + min_value=1, + initial=8, + help_text='The time in hours before the developer reminder is sent.', + ) + sendgrid_bulk = forms.BooleanField( required=False, initial=False, diff --git a/admin/notifications/views.py b/admin/notifications/views.py index cea8e298eb7..2661376a142 100644 --- a/admin/notifications/views.py +++ b/admin/notifications/views.py @@ -4,7 +4,8 @@ from datetime import timedelta from django.urls import reverse_lazy from django.utils import timezone -from django.db.models import Q, F +from django.db.models import Q, F, Subquery +from django.db.models.functions import Coalesce from django.db import models from django.shortcuts import get_object_or_404, redirect from django.views.generic import ListView, DetailView, UpdateView, CreateView, View @@ -18,7 +19,7 @@ from mako.parsetree import ControlLine from string import Formatter from osf.email import _render_email_html -from osf.email.notification_campaign import FILTER_PRESETS +from osf.email.notification_campaign import FILTER_PRESETS, counter_subquery from website import settings @@ -374,6 +375,7 @@ def get_context_data(self, **kwargs): context['notification_campaigns'] = context['object_list'] context['page'] = context['page_obj'] + context['active_campaign'] = NotificationCampaign.objects.filter(status=NotificationCampaignStatus.RUNNING).first() return context @@ -396,6 +398,7 @@ def get_context_data(self, *args, **kwargs): ('Name', notification_campaign.name), ('Notification Type', notification_campaign.notification_type), ('Created By', notification_campaign.created_by), + ('Developer reminder ', 'Sent' if notification_campaign.developer_reminder_sent else ''), ('Status', notification_campaign.get_status_display()), ('Recipients', notification_campaign.recipient_count), ('Sent', notification_campaign.sent_count), @@ -427,10 +430,60 @@ def get_context_data(self, *args, **kwargs): if k not in {'filters', 'context', 'execution', 'template'} }, 'allow_restart_stuck': True if timezone.now() - notification_campaign.updated_at > timedelta(minutes=15) else False, - 'sent_percent': notification_campaign.sent_count * 100 / notification_campaign.recipient_count if notification_campaign.recipient_count else 0, - 'failed_percent': notification_campaign.failed_count * 100 / notification_campaign.recipient_count if notification_campaign.recipient_count else 0, } + if notification_campaign.status != NotificationCampaignStatus.CREATED: + processed = notification_campaign.sent_count + notification_campaign.failed_count + pending = max(notification_campaign.recipient_count - processed, 0) + + sent_percent = ( + notification_campaign.sent_count / notification_campaign.recipient_count * 100 + if notification_campaign.recipient_count else 0 + ) + failed_percent = ( + notification_campaign.failed_count / notification_campaign.recipient_count * 100 + if notification_campaign.recipient_count else 0 + ) + + pending_percent = max(100 - sent_percent - failed_percent, 0) + elapsed = None + speed = None + eta = None + estimated_finish = None + last_activity_ago = None + failure_rate = None + + if notification_campaign.started_at: + end_time = notification_campaign.completed_at or timezone.now() + elapsed = end_time - notification_campaign.started_at + elapsed_seconds = elapsed.total_seconds() + if processed > 0 and elapsed_seconds > 0: + speed = processed / elapsed_seconds + if pending: + eta = timedelta(seconds=int(pending / speed)) + estimated_finish = timezone.now() + eta + + if notification_campaign.updated_at: + last_activity_ago = timezone.now() - notification_campaign.updated_at + if processed: + failure_rate = notification_campaign.failed_count / processed * 100 + else: + failure_rate = 0 + + context.update({ + 'processed': processed, + 'pending': pending, + 'sent_percent': sent_percent, + 'failed_percent': failed_percent, + 'pending_percent': pending_percent, + 'elapsed': elapsed, + 'speed': speed, + 'eta': eta, + 'estimated_finish': estimated_finish, + 'last_activity_ago': last_activity_ago, + 'failure_rate': failure_rate, + }) + return context @@ -510,6 +563,7 @@ def form_valid(self, form): 'batch_size': form.cleaned_data['batch_size'], 'max_retries': form.cleaned_data['max_retries'], 'activity_threshold': form.cleaned_data['activity_threshold'], + 'time_window': form.cleaned_data['time_window'], }, 'sendgrid_bulk': form.cleaned_data.get('sendgrid_bulk', False), } @@ -580,9 +634,12 @@ def get_queryset(self): filters[f'{item["field"]}__{item["lookup"]}'] = [value.strip() for value in item['value'].split(',')] qs = OSFUser.objects.filter(**filters) - return qs.annotate( - guid=F('guids___id') - ) + qs = qs.annotate( + guid=F('guids___id'), + activity_score=Coalesce(Subquery(counter_subquery), 0) + ).order_by('-activity_score') + + return qs def get_context_data(self, **kwargs): users = self.get_queryset() @@ -659,6 +716,14 @@ def post(self, request, *args, **kwargs): pk=notification_campaign.pk, ) + cancel_campaign = request.GET.get('cancel') == 'true' + if cancel_campaign: + notification_campaign.cancel() + return redirect( + 'notifications:notification_campaigns_detail', + pk=notification_campaign.pk, + ) + restart_failed = request.GET.get('restart_failed') == 'true' restart_stuck = request.GET.get('restart_stuck') == 'true' diff --git a/admin/templates/notifications/notification_campaigns_detail.html b/admin/templates/notifications/notification_campaigns_detail.html index 113248e2067..079b24aa694 100644 --- a/admin/templates/notifications/notification_campaigns_detail.html +++ b/admin/templates/notifications/notification_campaigns_detail.html @@ -27,21 +27,132 @@

    {{ notification_campaign.name }}

    {% if notification_campaign.status != 'created' %} -

    Progress

    +
    + + Status: + {{ notification_campaign.get_status_display }} + + {% if elapsed %} +
    + Running for: {{ elapsed }} + {% endif %} + + {% if last_activity_ago %} +
    + Last activity: {{ last_activity_ago }} ago + {% endif %} +
    +
    -
    -
    - {{ notification_campaign.sent_count }} -
    -
    - {{ notification_campaign.failed_count }} -
    +
    + {{ notification_campaign.sent_count }}
    -

    - {{ notification_campaign.sent_count|add:notification_campaign.failed_count }}/{{ notification_campaign.recipient_count }} processed -

    + +
    + {{ notification_campaign.failed_count }} +
    + +
    + {{ pending }} +
    + +
    +

    Progress

    + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
    Processed{{ processed }} / {{ notification_campaign.recipient_count }}
    Pending{{ pending }}
    Success rate{{ sent_percent|floatformat:2 }}%
    Failure rate{{ failure_rate|floatformat:2 }}%
    Average speed + {% if speed %} + {{ speed|floatformat:2 }} recipients/sec + {% else %} + — + {% endif %} +
    Elapsed + {% if elapsed %} + {{ elapsed }} + {% else %} + — + {% endif %} +
    ETA + {% if eta %} + {{ eta }} + {% else %} + — + {% endif %} +
    Estimated completion + {% if estimated_finish %} + {{ estimated_finish }} + {% else %} + — + {% endif %} +
    +{% if allow_restart_stuck and notification_campaign.status == "running" %} +
    + Warning! + No activity has been detected for more than 15 minutes. + The campaign may be stuck and can be restarted. +
    +{% endif %} +{% if notification_campaign.developer_reminder_sent and notification_campaign.status == "running" %} +
    + Warning! + The campaign exceeded the expected timeframe ({{ metadata.execution.time_window }}h). A reminder was sent. +
    +{% endif %} {% endif %}
    Progress Restart stuck Campaign
    +
    + {% csrf_token %} + +
    diff --git a/admin/templates/notifications/notification_campaigns_list.html b/admin/templates/notifications/notification_campaigns_list.html index 494e4584045..03e3d05e457 100644 --- a/admin/templates/notifications/notification_campaigns_list.html +++ b/admin/templates/notifications/notification_campaigns_list.html @@ -6,6 +6,28 @@ {% endblock title %} {% block content %}

    List of Notification Campaigns

    + {% if active_campaign %} +

    Active Campaign

    + + + + + + + + + + + + + + + + + + +
    NameNotification TypeStatusStarted atCompleted at
    {{ active_campaign.name }}{{ active_campaign.notification_type.name }}{{ active_campaign.status }}{{ active_campaign.started_at }}{{ active_campaign.completed_at }}
    + {% endif %}
    diff --git a/admin/templates/notifications/notification_campaing_create.html b/admin/templates/notifications/notification_campaing_create.html index b8b4dcb72a6..b24f58cdda8 100644 --- a/admin/templates/notifications/notification_campaing_create.html +++ b/admin/templates/notifications/notification_campaing_create.html @@ -203,6 +203,21 @@

    Execution

    + + Time window + + +

    + The time in hours before the developer reminder is sent. +

    + + Sendgrid Bulk diff --git a/admin/templates/notifications/notification_campaing_recipients_list.html b/admin/templates/notifications/notification_campaing_recipients_list.html index 03e519b0235..9602e8c90e8 100644 --- a/admin/templates/notifications/notification_campaing_recipients_list.html +++ b/admin/templates/notifications/notification_campaing_recipients_list.html @@ -18,6 +18,7 @@ Status Error Updated at + Activity score @@ -40,6 +41,9 @@ {{ record.updated_at }} + + {{ record.activity_score }} + {% endfor %} diff --git a/admin/templates/notifications/notification_campaing_recipients_preview.html b/admin/templates/notifications/notification_campaing_recipients_preview.html index 62bb9dd763c..7d3cd7e111b 100644 --- a/admin/templates/notifications/notification_campaing_recipients_preview.html +++ b/admin/templates/notifications/notification_campaing_recipients_preview.html @@ -18,6 +18,7 @@ Fullname Date confirmed Date disabled + Activity score @@ -40,6 +41,9 @@ {{ user.is_disabled }} + + {{ user.activity_score }} + {% endfor %} diff --git a/admin_tests/notifications/test_campaigns.py b/admin_tests/notifications/test_campaigns.py index 336cb35cbd4..3007c9fef75 100644 --- a/admin_tests/notifications/test_campaigns.py +++ b/admin_tests/notifications/test_campaigns.py @@ -57,6 +57,7 @@ def _valid_form_data(notification_type, **overrides): 'max_retries': settings.DEFAULT_CAMPAIGN_MAX_RETRIES, 'activity_threshold': settings.DEFAULT_CAMPAIGN_ACTIVITY_THRESHOLD, 'sendgrid_bulk': False, + 'time_window': 8, } data.update(overrides) return data @@ -172,6 +173,7 @@ def test_form_valid_persists_execution_metadata(self): max_retries=4, activity_threshold=77, sendgrid_bulk=True, + time_window=8 ), ) request.user = self.user @@ -193,6 +195,7 @@ def test_form_valid_persists_execution_metadata(self): 'batch_size': 25, 'max_retries': 4, 'activity_threshold': 77, + 'time_window': 8 } assert campaign.metadata['sendgrid_bulk'] is True assert campaign.metadata['filters'] == {'predefined': 'active'} diff --git a/osf/email/notification_campaign.py b/osf/email/notification_campaign.py index 89853c8064e..5ac41d410e6 100644 --- a/osf/email/notification_campaign.py +++ b/osf/email/notification_campaign.py @@ -150,43 +150,62 @@ def get_campaign_recipient_stats(campaign_id): def process_campaign_retry(*args, **kwargs): campaign_id = kwargs.get('campaign_id') campaign = NotificationCampaign.objects.get(id=campaign_id) - failed_recipients = NotificationCampaignRecipient.objects.filter(campaign=campaign, status=NotificationCampaignRecipientStatus.FAILED) - max_retries = campaign.metadata.get('execution', {}).get('max_retries', settings.DEFAULT_CAMPAIGN_MAX_RETRIES) - batch_size = campaign.metadata.get('execution', {}).get('batch_size', settings.DEFAULT_CAMPAIGN_BATCH_SIZE) - failed_recipients_count = failed_recipients.count() - if failed_recipients_count: - if campaign.retries < max_retries: - message = ( - f"[Notification Campaign] Retrying " - f"{failed_recipients_count} failed recipients for campaign {campaign_id}" - ) - logger.info(message) - sentry.log_message(message) - campaign.retries += 1 - campaign.save(update_fields=['retries']) - retry_group = build_campaign_group( - batch_size=batch_size, - campaign_id=campaign_id, - restart_failed=True, - notification_type_name=campaign.notification_type.name, - context=campaign.metadata.get('context', {}), - run_id=campaign.run_id, - ) - chain( - retry_group, - process_campaign_retry.si(campaign_id=campaign_id), - ).apply_async() - return + if kwargs.get('run_id') != campaign.run_id: + return + + final_status = NotificationCampaignStatus.COMPLETED + + if campaign.status != NotificationCampaignStatus.CANCELLED: + failed_recipients = NotificationCampaignRecipient.objects.filter(campaign=campaign, status=NotificationCampaignRecipientStatus.FAILED) + max_retries = campaign.metadata.get('execution', {}).get('max_retries', settings.DEFAULT_CAMPAIGN_MAX_RETRIES) + batch_size = campaign.metadata.get('execution', {}).get('batch_size', settings.DEFAULT_CAMPAIGN_BATCH_SIZE) + failed_recipients_count = failed_recipients.count() + if failed_recipients_count: + if campaign.retries < max_retries: + message = ( + f"[Notification Campaign] Retrying " + f"{failed_recipients_count} failed recipients for campaign {campaign_id}" + ) + logger.info(message) + sentry.log_message(message) + campaign.retries += 1 + campaign.save(update_fields=['retries']) + retry_group = build_campaign_group( + batch_size=batch_size, + campaign_id=campaign_id, + restart_failed=True, + notification_type_name=campaign.notification_type.name, + context=campaign.metadata.get('context', {}), + run_id=campaign.run_id, + ) + chain( + retry_group, + process_campaign_retry.si(campaign_id=campaign_id, run_id=campaign.run_id), + ).apply_async() + return - campaign.status = NotificationCampaignStatus.PARTIALLY_COMPLETED + final_status = NotificationCampaignStatus.PARTIALLY_COMPLETED else: - campaign.status = NotificationCampaignStatus.COMPLETED + message = f'[Notification Campaign] Campaign {campaign_id} {campaign.name} was cancelled.' + logger.info(message) + sentry.log_message(message) + + # Refresh in case the campaign was cancelled while we were running. + campaign.refresh_from_db(fields=['status', 'completed_at']) + # Sync statistics regardless of status. stats = get_campaign_recipient_stats(campaign_id) campaign.recipient_count = stats['recipient_count'] campaign.sent_count = stats['sent_count'] campaign.failed_count = stats['failed_count'] - campaign.completed_at = timezone.now() + + if campaign.completed_at is None: + campaign.completed_at = timezone.now() + + # Don't overwrite CANCELLED. + if campaign.status != NotificationCampaignStatus.CANCELLED: + campaign.status = final_status + campaign.save() @@ -252,7 +271,7 @@ def start_notification_campaign(campaign_id, restart_failed=False, restart_stuck if spam_users_tasks: workflow.append(spam_users_tasks) - chain(*workflow, process_campaign_retry.si(campaign_id=campaign_id)).apply_async() + chain(*workflow, process_campaign_retry.si(campaign_id=campaign_id, run_id=campaign.run_id)).apply_async() @celery_app.task(name='email.send_campaign_batch', ignore_result=False) diff --git a/osf/models/notification_campaign.py b/osf/models/notification_campaign.py index b1822400964..4b03337f25e 100644 --- a/osf/models/notification_campaign.py +++ b/osf/models/notification_campaign.py @@ -71,6 +71,12 @@ class NotificationCampaign(models.Model): developer_reminder_sent = models.BooleanField(default=False) + def cancel(self): + self.status = NotificationCampaignStatus.CANCELLED + if self.completed_at is None: + self.completed_at = timezone.now() + self.save() + def start(self, restart_failed=False, restart_stuck=False): from osf.email.notification_campaign import start_notification_campaign self.status = NotificationCampaignStatus.RUNNING diff --git a/osf_tests/test_notification_campaign.py b/osf_tests/test_notification_campaign.py index a0eb6f5e477..750bafabbcb 100644 --- a/osf_tests/test_notification_campaign.py +++ b/osf_tests/test_notification_campaign.py @@ -658,7 +658,7 @@ def test_process_campaign_retry_retries_failed_recipients(self, mock_chain, camp campaign.save() mock_chain.return_value.apply_async = mock.Mock() - 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.retries == 1 From c93805b3e22e8fa944c78502db88ecad0b9b1986 Mon Sep 17 00:00:00 2001 From: Ostap-Zherebetskyi Date: Thu, 30 Jul 2026 17:58:37 +0300 Subject: [PATCH 15/25] [ENG-11847] Look into the permission issue (#11838) * Update notification campaign permissions in views and tests * fix test_start_rejects_when_another_campaign_is_running --- admin/notifications/views.py | 2 +- admin_tests/notifications/test_campaigns.py | 6 +++--- osf/migrations/__init__.py | 3 +++ 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/admin/notifications/views.py b/admin/notifications/views.py index 2661376a142..9a08a5759ba 100644 --- a/admin/notifications/views.py +++ b/admin/notifications/views.py @@ -701,7 +701,7 @@ def get_context_data(self, **kwargs): ) class StartNotificationCampaign(PermissionRequiredMixin, View): - permission_required = 'osf.change_notificationtype' + permission_required = 'osf.change_notificationcampaign' def post(self, request, *args, **kwargs): notification_campaign = get_object_or_404( diff --git a/admin_tests/notifications/test_campaigns.py b/admin_tests/notifications/test_campaigns.py index 3007c9fef75..b7df85af5ed 100644 --- a/admin_tests/notifications/test_campaigns.py +++ b/admin_tests/notifications/test_campaigns.py @@ -266,7 +266,7 @@ def test_detail_requires_change_permission(self): response = NotificationCampaignDetail.as_view()(request, pk=self.campaign.pk) assert response.status_code == 200 - def test_start_requires_change_notificationtype_permission(self): + def test_start_requires_change_notificationcampaign_permission(self): request = RequestFactory().post( reverse('notifications:notification_campaigns_start', kwargs={'pk': self.campaign.pk}) ) @@ -275,7 +275,7 @@ def test_start_requires_change_notificationtype_permission(self): with self.assertRaises(PermissionDenied): StartNotificationCampaign.as_view()(request, pk=self.campaign.pk) - grant_permission(self.user, 'change_notificationtype') + grant_permission(self.user, 'change_notificationcampaign') with mock.patch.object(NotificationCampaign, 'start') as mock_start: response = StartNotificationCampaign.as_view()(request, pk=self.campaign.pk) assert response.status_code == 302 @@ -284,7 +284,7 @@ def test_start_requires_change_notificationtype_permission(self): def test_start_rejects_when_another_campaign_is_running(self): from osf.models.notification_campaign import NotificationCampaignStatus - grant_permission(self.user, 'change_notificationtype') + grant_permission(self.user, 'change_notificationcampaign') NotificationCampaign.objects.create( name='Already Running', notification_type=self.notification_type, diff --git a/osf/migrations/__init__.py b/osf/migrations/__init__.py index 95bfd49a76d..344cf381f5a 100644 --- a/osf/migrations/__init__.py +++ b/osf/migrations/__init__.py @@ -65,6 +65,8 @@ def get_admin_read_permissions(): 'view_notificationtype', 'view_notificationsubscription', 'view_emailtask', + 'view_notificationcampaign', + 'view_notificationcampaignrecipient', ]) @@ -116,6 +118,7 @@ def get_admin_write_permissions(): 'delete_notificationsubscription', 'change_emailtask', 'delete_emailtask', + 'change_notificationcampaign', ]) From 8bec7ddc95276e4f4852a7131628facdf73db742 Mon Sep 17 00:00:00 2001 From: antkryt Date: Thu, 30 Jul 2026 18:35:04 +0300 Subject: [PATCH 16/25] [ENG-11797] Unit tests on updated campaign workflow - Part 2 (#11839) * Update notification campaign flow and admin unit tests --- admin_tests/notifications/test_campaigns.py | 46 +++++++++ osf_tests/test_notification_campaign.py | 106 +++++++++++++++++++- 2 files changed, 148 insertions(+), 4 deletions(-) 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 From 59be6e8c2312997e1738a8fbc06e1c346d236a02 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Thu, 30 Jul 2026 11:46:40 -0400 Subject: [PATCH 17/25] [ENG-11844] Create confirmed test user with fake activity points (#11837) * Create confirmed test user with fake activity points * Respond to CR with improvements --- .../commands/create_confirmed_test_users.py | 151 ++++++++++++++++++ 1 file changed, 151 insertions(+) create mode 100644 osf/management/commands/create_confirmed_test_users.py diff --git a/osf/management/commands/create_confirmed_test_users.py b/osf/management/commands/create_confirmed_test_users.py new file mode 100644 index 00000000000..436d8697af3 --- /dev/null +++ b/osf/management/commands/create_confirmed_test_users.py @@ -0,0 +1,151 @@ +import logging +import random + +from django.core.management.base import BaseCommand +from django.db import transaction +from django.utils import timezone + +from osf.models import OSFUser, UserActivityCounter +from website.app import setup_django +from website.security import random_string + +setup_django() + + +logger = logging.getLogger(__name__) + + +def generate_users(prefix, suffix, domain, total): + """Generate usernames paired with full names. + """ + return { + f'{prefix}+{str(i).zfill(4)}+{suffix}@{domain}': f'{prefix}{str(i).zfill(4)} {suffix}{str(i).zfill(4)}' + for i in range(1, total + 1) + } + + +def create_confirmed_test_users( + prefix, + suffix='enter', + domain='cos.io', + total=100, + password=None, + set_activity=True, + dry_run=False +): + """Create a given number of confirmed users, with generated usernames and full names. For each created user, + optionally creates a matching UserActivityCounter entry with a random total between 1 and 100. + """ + created_user_ids = [] + username_to_fullname = generate_users(prefix, suffix, domain, total) + + for raw_username, fullname in username_to_fullname.items(): + username = raw_username.lower().strip() + user_password = password or random_string(16) + if dry_run: + logger.info(f'Dry run: would create confirmed user "{username}" ({fullname}).') + continue + try: + user = OSFUser.create_confirmed( + username=username, + password=user_password, + fullname=fullname, + ) + except Exception as e: + logger.error(f'Failed to create confirmed user "{username}" ({fullname}): error={e}.') + continue + user.accepted_terms_of_service = timezone.now() + user.save() + logger.info(f'Created confirmed user "{username}" ({fullname})') + created_user_ids.append(user._id) + + if set_activity: + UserActivityCounter.objects.bulk_create( + [ + UserActivityCounter( + _id=_id, + action={}, + date={}, + total=random.randint(1, 100) + ) + for _id in created_user_ids + ], + ignore_conflicts=True, + ) + + logger.info(f'Done. Created {len(created_user_ids)} user(s); skipped {len(username_to_fullname) - len(created_user_ids)}.') + + +class Command(BaseCommand): + help = '''Create a given number of confirmed users, with generated usernames and full names. + + python3 manage.py create_confirmed_test_users --prefix longze --suffix enter --domain cos.io --total 100 + ''' + + def add_arguments(self, parser): + super().add_arguments(parser) + parser.add_argument( + '--prefix', + type=str, + required=True, + help='Prefix for generated usernames and full names', + ) + parser.add_argument( + '--suffix', + type=str, + default='enter', + help='Suffix for generated usernames and full names', + ) + parser.add_argument( + '--domain', + type=str, + default='cos.io', + help='Domain for generated usernames', + ) + parser.add_argument( + '--total', + type=int, + default=100, + help='Total number of users to create', + ) + parser.add_argument( + '--password', + type=str, + dest='password', + help='Password to set for every created user.', + ) + parser.add_argument( + '--no-activity', + action='store_true', + dest='no_activity', + help='Skip setting a random activity total for the created users', + ) + parser.add_argument( + '--dry', + action='store_true', + dest='dry_run', + help='Dry run; log what would happen without creating any users', + ) + + def handle(self, *args, **options): + prefix = options.get('prefix') + suffix = options.get('suffix') + domain = options.get('domain') + total = options.get('total') + password = options.get('password') + set_activity = not options.get('no_activity', False) + dry_run = options.get('dry_run', False) + + if dry_run: + logger.info('This is a dry run; no users will be created.') + + with transaction.atomic(): + create_confirmed_test_users( + prefix, + suffix=suffix, + domain=domain, + total=total, + password=password, + set_activity=set_activity, + dry_run=dry_run, + ) From de20f5b548b9cd27573c76a0865c8bd47ef7b302 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Thu, 30 Jul 2026 16:52:21 -0400 Subject: [PATCH 18/25] [ENG-11844] Create confirmed test user with fake activity points - Part 2 (#11840) * Update command to create more users with different status and options --- .../commands/create_confirmed_test_users.py | 47 +++++++++++++++---- 1 file changed, 39 insertions(+), 8 deletions(-) diff --git a/osf/management/commands/create_confirmed_test_users.py b/osf/management/commands/create_confirmed_test_users.py index 436d8697af3..99e1861fcd8 100644 --- a/osf/management/commands/create_confirmed_test_users.py +++ b/osf/management/commands/create_confirmed_test_users.py @@ -5,22 +5,23 @@ from django.db import transaction from django.utils import timezone -from osf.models import OSFUser, UserActivityCounter +from osf.models import OSFUser, SpamStatus, UserActivityCounter from website.app import setup_django from website.security import random_string setup_django() - logger = logging.getLogger(__name__) -def generate_users(prefix, suffix, domain, total): - """Generate usernames paired with full names. +def generate_users(prefix, suffix, domain, total, start=1): + """Generate usernames paired with full names: prefix+NNNN+suffix@domain """ + if start + total > 9999: + raise Exception(f'The max numbered user cannot be greater than 9999: start({start}) + total({total}) = {start + total}!') return { f'{prefix}+{str(i).zfill(4)}+{suffix}@{domain}': f'{prefix}{str(i).zfill(4)} {suffix}{str(i).zfill(4)}' - for i in range(1, total + 1) + for i in range(start, total + start) } @@ -29,15 +30,18 @@ def create_confirmed_test_users( suffix='enter', domain='cos.io', total=100, + start=1, password=None, set_activity=True, + no_email=False, + flagged=False, dry_run=False ): """Create a given number of confirmed users, with generated usernames and full names. For each created user, optionally creates a matching UserActivityCounter entry with a random total between 1 and 100. """ created_user_ids = [] - username_to_fullname = generate_users(prefix, suffix, domain, total) + username_to_fullname = generate_users(prefix, suffix, domain, total, start) for raw_username, fullname in username_to_fullname.items(): username = raw_username.lower().strip() @@ -55,10 +59,14 @@ def create_confirmed_test_users( logger.error(f'Failed to create confirmed user "{username}" ({fullname}): error={e}.') continue user.accepted_terms_of_service = timezone.now() + if no_email: + user.emails.all().delete() + user.username = user._id + if flagged: + user.spam_status = SpamStatus.FLAGGED user.save() logger.info(f'Created confirmed user "{username}" ({fullname})') created_user_ids.append(user._id) - if set_activity: UserActivityCounter.objects.bulk_create( [ @@ -72,7 +80,6 @@ def create_confirmed_test_users( ], ignore_conflicts=True, ) - logger.info(f'Done. Created {len(created_user_ids)} user(s); skipped {len(username_to_fullname) - len(created_user_ids)}.') @@ -108,6 +115,12 @@ def add_arguments(self, parser): default=100, help='Total number of users to create', ) + parser.add_argument( + '--start', + type=int, + default=1, + help='The starting number of the first user to create', + ) parser.add_argument( '--password', type=str, @@ -120,6 +133,18 @@ def add_arguments(self, parser): dest='no_activity', help='Skip setting a random activity total for the created users', ) + parser.add_argument( + '--no-email', + action='store_true', + dest='no_email', + help='Set username to guid and remove all emails', + ) + parser.add_argument( + '--flagged', + action='store_true', + dest='flagged', + help='Flag user (flagged spam but not confirmed)', + ) parser.add_argument( '--dry', action='store_true', @@ -132,8 +157,11 @@ def handle(self, *args, **options): suffix = options.get('suffix') domain = options.get('domain') total = options.get('total') + start = options.get('start') password = options.get('password') set_activity = not options.get('no_activity', False) + no_email = options.get('no_email', False) + flagged = options.get('flagged', False) dry_run = options.get('dry_run', False) if dry_run: @@ -145,7 +173,10 @@ def handle(self, *args, **options): suffix=suffix, domain=domain, total=total, + start=start, password=password, set_activity=set_activity, + no_email=no_email, + flagged=flagged, dry_run=dry_run, ) From 577be69a4de926b9095db509f2020b0e8dd22325 Mon Sep 17 00:00:00 2001 From: Ostap-Zherebetskyi Date: Fri, 31 Jul 2026 18:26:50 +0300 Subject: [PATCH 19/25] [ENG-11866][ENG-11524][ENG-11871] Refactor notification campaign filters to use a dynamic query builder + Bug fixes (#11842) * Refactor notification campaign filters to use a dynamic query builder and streamline filter handling in the UI * Refactor campaign recipient filters to use Q objects and dynamic query building * Enhance notification campaign creation UI with improved group structure and styling * Remove copy * Refactor initial state handling in filter mode to improve readability * Add manual filter validation and improve filter display in notification campaigns --- admin/notifications/views.py | 27 +- .../notifications/campaign_filter_group.html | 22 ++ .../notification_campaigns_detail.html | 39 ++- .../notification_campaing_create.html | 246 ++++++++++++------ osf/email/notification_campaign.py | 44 +++- osf_tests/test_notification_campaign.py | 62 ++--- 6 files changed, 293 insertions(+), 147 deletions(-) create mode 100644 admin/templates/notifications/campaign_filter_group.html diff --git a/admin/notifications/views.py b/admin/notifications/views.py index 9a08a5759ba..4a35545eab2 100644 --- a/admin/notifications/views.py +++ b/admin/notifications/views.py @@ -19,8 +19,9 @@ from mako.parsetree import ControlLine from string import Formatter from osf.email import _render_email_html -from osf.email.notification_campaign import FILTER_PRESETS, counter_subquery +from osf.email.notification_campaign import FILTER_PRESETS, counter_subquery, build_query from website import settings +from urllib.parse import urlencode def delete_selected_notifications(selected_ids): @@ -556,6 +557,14 @@ class NotificationCampaignCreateView(CreateView): def form_valid(self, form): form.instance.created_by = self.request.user + if 'manual' in form.cleaned_data['filters']: + if not form.cleaned_data['filters']['manual']['children']: + form.add_error( + 'filters', + 'Manual filters cannot be empty.' + ) + return self.form_invalid(form) + form.instance.metadata = { 'filters': form.cleaned_data['filters'], 'context': form.cleaned_data['context'], @@ -620,20 +629,16 @@ class NotificationCampaignsRecipientsPreview(PermissionRequiredMixin, ListView): paginate_by = 25 def get_queryset(self): - filters = {} + query = Q() raw_filters = self.request.GET.get('filters', None) if raw_filters: json_filters = json.loads(raw_filters) if predefined := json_filters.get('predefined'): - filters = FILTER_PRESETS.get(predefined, {}) + query = Q(**FILTER_PRESETS.get(predefined, {})) else: - for item in json_filters.get('manual', []): - if item['lookup'] != 'in': - filters[f'{item["field"]}__{item["lookup"]}'] = item['value'] - else: - filters[f'{item["field"]}__{item["lookup"]}'] = [value.strip() for value in item['value'].split(',')] + query = build_query(json_filters.get('manual')) - qs = OSFUser.objects.filter(**filters) + qs = OSFUser.objects.filter(query) qs = qs.annotate( guid=F('guids___id'), activity_score=Coalesce(Subquery(counter_subquery), 0) @@ -649,8 +654,10 @@ def get_context_data(self, **kwargs): users, page_size, ) + + filters = self.request.GET.get('filters') # append search param to pagination links - kwargs.update({'extra_query_params': f'&filters={self.request.GET.get("filters")}'}) + kwargs.update({'extra_query_params': f'&{urlencode({'filters': filters})}'}) return super().get_context_data( **kwargs, page=page, diff --git a/admin/templates/notifications/campaign_filter_group.html b/admin/templates/notifications/campaign_filter_group.html new file mode 100644 index 00000000000..b756b1467d8 --- /dev/null +++ b/admin/templates/notifications/campaign_filter_group.html @@ -0,0 +1,22 @@ +
    +
    + Match + {{ group.operator }} +
    + +
    + {% for child in group.children %} + {% if child.children %} + {% include "notifications/campaign_filter_group.html" with group=child is_root=False %} + {% else %} +
    + {{ child.field }} + {{ child.lookup }} + {{ child.value }} +
    + {% endif %} + {% empty %} +
    No filters configured.
    + {% endfor %} +
    +
    diff --git a/admin/templates/notifications/notification_campaigns_detail.html b/admin/templates/notifications/notification_campaigns_detail.html index 079b24aa694..5b259abfcfe 100644 --- a/admin/templates/notifications/notification_campaigns_detail.html +++ b/admin/templates/notifications/notification_campaigns_detail.html @@ -7,6 +7,14 @@ {% endblock title %} {% block content %} + +
    {% if messages %}
      @@ -261,30 +269,13 @@

      Recipient Filters

      {% if not "predefined" in metadata.filters %} - - - - - - - - - - {% for filter in metadata.filters.manual %} - - - - - - {% empty %} - - - - {% endfor %} - -
      FieldLookupValue
      {{ filter.field }}{{ filter.lookup }}{{ filter.value }}
      - No filters configured. -
      + {% if not "predefined" in metadata.filters %} + {% if metadata.filters.manual %} + {% include "notifications/campaign_filter_group.html" with group=metadata.filters.manual is_root=True %} + {% else %} +

      No filters configured.

      + {% endif %} + {% endif %} {% elif "predefined" in metadata.filters %} diff --git a/admin/templates/notifications/notification_campaing_create.html b/admin/templates/notifications/notification_campaing_create.html index b24f58cdda8..ad7346bd98d 100644 --- a/admin/templates/notifications/notification_campaing_create.html +++ b/admin/templates/notifications/notification_campaing_create.html @@ -7,6 +7,28 @@ {% endblock title %} {% block content %} + +
      {% if messages %}
        @@ -97,17 +119,7 @@

        Recipient Filters

      -
      - - -
      Execution const filterMode = document.getElementById("filter-mode"); const manualFilters = document.getElementById("manual-filters"); const predefinedFilter = document.getElementById("predefined-filter"); -const container = document.getElementById("filter-builder"); +const builder = document.getElementById("filter-builder"); const form = document.querySelector("form"); function updateMode() { @@ -299,67 +311,18 @@

      Execution

      updateMode(); if (initialState) { - filterMode.value = initialState.mode || "manual"; + filterMode.value = initialState.mode || "predefined"; updateMode(); if (initialState.mode === "manual") { - (initialState.filters || []).forEach(addFilter); + createGroup(builder, initialState); } else if (initialState.mode === "predefined") { document.getElementById("filter-row").value = initialState.filter; } } -document - .getElementById("add-filter") - .addEventListener("click", () => addFilter()); - -function addFilter(initial = null) { - - const row = document.createElement("div"); - row.className = "filter-row form-inline"; - row.style.marginBottom = "10px"; - - const field = document.createElement("select"); - field.className = "field form-control"; - - const operator = document.createElement("select"); - operator.className = "operator form-control"; - - const value = document.createElement("input"); - value.className = "value form-control"; - - const remove = document.createElement("button"); - remove.type = "button"; - remove.className = "btn btn-danger"; - remove.textContent = "✕"; - remove.onclick = () => row.remove(); - - Object.entries(filterConfig).forEach(([name, config]) => { - field.add(new Option(config.label, name)); - }); - - field.onchange = () => updateOperatorAndValue(row, field, operator, value); - - operator.onchange = () => { - updateValueName(field, operator, row.querySelector(".value")); - }; - - row.append(field, operator, value, remove); - container.appendChild(row); - - if (initial) { - field.value = initial.field; - } - - updateOperatorAndValue(row, field, operator, value); - - if (initial) { - operator.value = initial.lookup; - updateValueName(field, operator, row.querySelector(".value")); - row.querySelector(".value").value = initial.value; - } -} +createGroup(builder) function updateValueName(field, operator, value) { value.name = `${field.value}__${operator.value}`; @@ -415,6 +378,7 @@

      Execution

      updateValueName(field, operator, newValue); } + function buildFilters() { if (filterMode.value === "predefined") { @@ -423,17 +387,8 @@

      Execution

      }; } - const filters = []; - - document.querySelectorAll(".filter-row").forEach(row => { - filters.push({ - field: row.querySelector(".field").value, - lookup: row.querySelector(".operator").value, - value: row.querySelector(".value").value - }); - }); - - return {"manual": filters} + const root = builder.querySelector(":scope > .group"); + return {"manual": serializeGroup(root)} } function updateFiltersInput() { @@ -483,5 +438,148 @@

      Execution

      }); }); +function createGroup(parent, initialState = {}) { + const group = document.createElement("div"); + group.className = "group"; + + group.innerHTML = ` +
      +
      + Match + + + ${ + parent !== builder + ? `` + : "" + } +
      + + +
      + + + +
      +
      +
      + `; + + parent.appendChild(group); + + const children = group.querySelector(".children"); + + // Set operator + group.querySelector(".group-operator").value = + initialState.operator || "AND"; + + group.querySelector(".add-condition").onclick = () => + createCondition(children); + + group.querySelector(".add-group").onclick = () => + createGroup(children); + + group.querySelector(".remove-group")?.addEventListener("click", () => { + group.remove(); + }); + + // Restore children + (initialState.children || []).forEach(child => { + if ("children" in child) { + createGroup(children, child); + } else { + createCondition(children, child); + } + }); + + return group; +} + +function createCondition(parent, initialState = {}) { + const row = document.createElement("div"); + row.className = "condition"; + + row.innerHTML = ` + + + + + `; + + const field = row.querySelector(".field"); + const lookup = row.querySelector(".lookup"); + const value = row.querySelector(".value"); + + Object.entries(filterConfig).forEach(([name, config]) => { + field.add(new Option(config.label, name)); + }); + + field.addEventListener("change", () => { + updateOperatorAndValue(row, field, lookup, value); + }); + + lookup.addEventListener("change", () => { + updateValueName(field, lookup, value); + }); + + parent.appendChild(row); + + // Set field first so available lookups are populated + if (initialState.field) { + field.value = initialState.field; + } + + updateOperatorAndValue(row, field, lookup, value); + + if (initialState.lookup) { + lookup.value = initialState.lookup; + } + + if (initialState.value !== undefined) { + value.value = initialState.value; + } + + row.querySelector(".remove-condition").onclick = () => row.remove(); + + return row; +} + +function serializeGroup(group) { + + const result = { + operator: group.querySelector(".group-operator").value, + children: [] + }; + + group.querySelector(":scope > .children").childNodes.forEach(child => { + + if (child.classList.contains("condition")) { + + result.children.push({ + field: child.querySelector(".field").value, + lookup: child.querySelector(".lookup").value, + value: child.querySelector(".value").value + }); + + } else if (child.classList.contains("group")) { + + result.children.push( + serializeGroup(child) + ); + + } + + }); + + return result; +} + {% endblock %} diff --git a/osf/email/notification_campaign.py b/osf/email/notification_campaign.py index 5ac41d410e6..7d6759ec2f6 100644 --- a/osf/email/notification_campaign.py +++ b/osf/email/notification_campaign.py @@ -37,10 +37,42 @@ .values('total')[:1] ) + +def build_query(node): + """ + Convert a filter tree into a Django Q object. + """ + + if 'field' in node: + value = node['value'] + + if node['lookup'] == 'in': + value = [v.strip() for v in value.split(',')] + + return Q(**{ + f'{node["field"]}__{node["lookup"]}': value + }) + + operator = node.get('operator', 'AND') + children = node.get('children', []) + + if not children: + return Q() + + query = build_query(children[0]) + + for child in children[1:]: + if operator == 'AND': + query &= build_query(child) + else: + query |= build_query(child) + + return query + def create_campaign_recipients(filters, campaign_id): qs = ( OSFUser.objects - .filter(**filters) + .filter(filters) .annotate(activity_score=Coalesce(Subquery(counter_subquery), 0)) .values_list( 'id', @@ -220,15 +252,9 @@ def start_notification_campaign(campaign_id, restart_failed=False, restart_stuck del getattr(NotificationTypeEnum, notification_type_name).instance if predefined_filter_name := filters.get('predefined'): - filters = FILTER_PRESETS.get(predefined_filter_name, {}) + filters = Q(**FILTER_PRESETS.get(predefined_filter_name, {})) else: - manual_filters = {} - for item in filters.get('manual', []): - if item['lookup'] != 'in': - manual_filters[f'{item["field"]}__{item["lookup"]}'] = item['value'] - else: - manual_filters[f'{item["field"]}__{item["lookup"]}'] = [value.strip() for value in item['value'].split(',')] - filters = manual_filters + filters = build_query(filters.get('manual', [])) if not restart_failed and not restart_stuck: create_campaign_recipients(filters=filters, campaign_id=campaign_id) diff --git a/osf_tests/test_notification_campaign.py b/osf_tests/test_notification_campaign.py index 03318ab99fc..1c160b852e2 100644 --- a/osf_tests/test_notification_campaign.py +++ b/osf_tests/test_notification_campaign.py @@ -4,6 +4,7 @@ from unittest import mock from django.utils import timezone +from django.db.models import Q from osf.email.notification_campaign import ( create_campaign_recipients, @@ -12,6 +13,7 @@ process_campaign_retry, send_campaign_batch, start_notification_campaign, + build_query, ) from osf.models import UserActivityCounter from osf.models.notification_campaign import ( @@ -39,7 +41,7 @@ def campaign(notification_type): name='Test campaign', notification_type=notification_type, metadata={ - 'filters': {'manual': []}, + 'filters': {'manual': {'operator': 'AND', 'children': []}}, 'context': {}, 'execution': { 'activity_threshold': 100, @@ -89,7 +91,7 @@ def test_creates_recipients_with_activity_scores(self, campaign): _set_activity(low, 50) create_campaign_recipients( - filters={'id__in': [high.id, low.id, zero.id]}, + Q(**{'id__in': [high.id, low.id, zero.id]}), campaign_id=campaign.id, ) @@ -109,7 +111,7 @@ def test_respects_user_filters(self, campaign): _set_activity(excluded, 10) create_campaign_recipients( - filters={'id__in': [included.id, excluded.id], 'is_staff': True}, + build_query({'operator': 'AND', 'children': [{'field': 'id', 'lookup': 'in', 'value': f'{included.id}, {excluded.id}'}, {'field': 'is_staff', 'lookup': 'exact', 'value': True}]}), campaign_id=campaign.id, ) @@ -134,7 +136,7 @@ def test_ordered_by_activity_score_descending(self, campaign): _set_activity(newer, 50) create_campaign_recipients( - filters={'id__in': [older.id, newer.id, mid.id]}, + Q(**{'id__in': [older.id, newer.id, mid.id]}), campaign_id=campaign.id, ) @@ -159,7 +161,7 @@ def test_counts_sent_failed_and_skipped(self, campaign, notification_type): skipped = UserFactory() pending = UserFactory() create_campaign_recipients( - filters={'id__in': [sent.id, failed.id, skipped.id, pending.id]}, + Q(**{'id__in': [sent.id, failed.id, skipped.id, pending.id]}), campaign_id=campaign.id, ) NotificationCampaignRecipient.objects.filter(campaign=campaign, user=sent).update( @@ -186,8 +188,8 @@ def test_scopes_to_requested_campaign(self, campaign, notification_type): ) user = UserFactory() other_user = UserFactory() - create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=campaign.id) - create_campaign_recipients(filters={'id__in': [other_user.id]}, campaign_id=other.id) + create_campaign_recipients(build_query({'operator': 'AND', 'children': [{'field': 'id', 'lookup': 'in', 'value': f'{user.id}'}]}), campaign_id=campaign.id) + create_campaign_recipients(build_query({'operator': 'AND', 'children': [{'field': 'id', 'lookup': 'in', 'value': f'{other_user.id}'}]}), campaign_id=other.id) NotificationCampaignRecipient.objects.filter(campaign=campaign).update( status=NotificationCampaignRecipientStatus.SENT ) @@ -228,7 +230,7 @@ def users_and_recipients(self, campaign): _set_activity(spam, 900) create_campaign_recipients( - filters={'id__in': [high.id, low.id, zero.id, flagged.id, spam.id]}, + Q(**{'id__in': [high.id, low.id, zero.id, flagged.id, spam.id]}), campaign_id=campaign.id, ) return { @@ -280,7 +282,7 @@ def test_batches_respect_batch_size(self, campaign): _set_activity(user, (i + 1) * 10) create_campaign_recipients( - filters={'id__in': [u.id for u in users]}, + Q(**{'id__in': [u.id for u in users]}), campaign_id=campaign.id, ) @@ -304,8 +306,8 @@ def test_restart_failed_only_returns_failed(self, campaign, users_and_recipients def test_ignore_conflicts_on_duplicate_create(self, campaign): user = UserFactory() _set_activity(user, 10) - create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=campaign.id) - create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=campaign.id) + create_campaign_recipients(Q(**{'id__in': [user.id]}), campaign_id=campaign.id) + create_campaign_recipients(Q(**{'id__in': [user.id]}), campaign_id=campaign.id) assert NotificationCampaignRecipient.objects.filter(campaign=campaign).count() == 1 @@ -366,7 +368,7 @@ def test_start_creates_recipients_and_schedules_workflow(self, mock_chain, campa _set_activity(low, 10) campaign.metadata['filters'] = { - 'manual': [{'field': 'id', 'lookup': 'in', 'value': f'{high.id},{low.id},{spam.id}'}], + 'manual': {'operator': 'AND', 'children': [{'field': 'id', 'lookup': 'in', 'value': f'{high.id},{low.id},{spam.id}'}]}, } campaign.run_id = uuid.uuid4() campaign.save() @@ -389,14 +391,14 @@ def test_start_creates_recipients_and_schedules_workflow(self, mock_chain, campa def test_start_restart_failed_does_not_recreate_recipients(self, mock_chain, campaign): user = UserFactory() _set_activity(user, 50) - create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=campaign.id) + create_campaign_recipients(Q(**{'id__in': [user.id]}), campaign_id=campaign.id) recipient = NotificationCampaignRecipient.objects.get(campaign=campaign, user=user) recipient.status = NotificationCampaignRecipientStatus.FAILED recipient.save(update_fields=['status']) campaign.run_id = uuid.uuid4() campaign.metadata['filters'] = { - 'manual': [{'field': 'id', 'lookup': 'in', 'value': str(user.id)}], + 'manual': {'operator': 'AND', 'children': [{'field': 'id', 'lookup': 'in', 'value': str(user.id)}]}, } campaign.save() mock_chain.return_value.apply_async = mock.Mock() @@ -409,7 +411,7 @@ def test_start_restart_failed_does_not_recreate_recipients(self, mock_chain, cam @mock.patch('osf.email.notification_campaign.chain') def test_start_restart_stuck_does_not_recreate_recipients(self, mock_chain, campaign): user = UserFactory() - create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=campaign.id) + create_campaign_recipients(Q(**{'id__in': [user.id]}), campaign_id=campaign.id) campaign.run_id = uuid.uuid4() campaign.recipient_count = 1 campaign.save() @@ -435,7 +437,7 @@ def running_campaign(self, campaign): @mock.patch.object(NotificationType, 'emit') def test_send_campaign_batch_marks_recipients_sent(self, mock_emit, running_campaign): user = UserFactory() - create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=running_campaign.id) + create_campaign_recipients(Q(**{'id__in': [user.id]}), campaign_id=running_campaign.id) recipient = NotificationCampaignRecipient.objects.get(campaign=running_campaign, user=user) send_campaign_batch( @@ -456,7 +458,7 @@ def test_send_campaign_batch_marks_recipients_sent(self, mock_emit, running_camp @mock.patch('osf.email.notification_campaign.sentry.log_exception') def test_send_campaign_batch_marks_recipients_failed(self, mock_sentry, mock_emit, running_campaign): user = UserFactory() - create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=running_campaign.id) + create_campaign_recipients(Q(**{'id__in': [user.id]}), campaign_id=running_campaign.id) recipient = NotificationCampaignRecipient.objects.get(campaign=running_campaign, user=user) send_campaign_batch( @@ -479,7 +481,7 @@ def test_send_campaign_batch_skips_invalid_email_addresses(self, running_campaig user.save(update_fields=['username']) user.emails.all().delete() - create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=running_campaign.id) + create_campaign_recipients(Q(**{'id__in': [user.id]}), campaign_id=running_campaign.id) recipient = NotificationCampaignRecipient.objects.get(campaign=running_campaign, user=user) send_campaign_batch( @@ -502,7 +504,7 @@ def test_send_campaign_batch_sendgrid_bulk_success(self, mock_sendgrid, running_ user = UserFactory() running_campaign.metadata['sendgrid_bulk'] = True running_campaign.save(update_fields=['metadata']) - create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=running_campaign.id) + create_campaign_recipients(Q(**{'id__in': [user.id]}), campaign_id=running_campaign.id) recipient = NotificationCampaignRecipient.objects.get(campaign=running_campaign, user=user) send_campaign_batch( @@ -529,7 +531,7 @@ def test_send_campaign_batch_sendgrid_bulk_failure(self, mock_sentry, mock_sendg user = UserFactory() running_campaign.metadata['sendgrid_bulk'] = True running_campaign.save(update_fields=['metadata']) - create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=running_campaign.id) + create_campaign_recipients(Q(**{'id__in': [user.id]}), campaign_id=running_campaign.id) recipient = NotificationCampaignRecipient.objects.get(campaign=running_campaign, user=user) send_campaign_batch( @@ -552,7 +554,7 @@ def test_send_campaign_batch_logs_when_time_window_exceeded(self, mock_sentry, r running_campaign.started_at = timezone.now() - timedelta(hours=9) running_campaign.metadata['execution']['time_window'] = 8 running_campaign.save() - create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=running_campaign.id) + create_campaign_recipients(Q(**{'id__in': [user.id]}), campaign_id=running_campaign.id) recipient = NotificationCampaignRecipient.objects.get(campaign=running_campaign, user=user) with mock.patch.object(NotificationType, 'emit'): @@ -570,7 +572,7 @@ def test_send_campaign_batch_logs_when_time_window_exceeded(self, mock_sentry, r def test_send_campaign_batch_marks_failed_when_notification_type_missing(self, running_campaign): user = UserFactory() - create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=running_campaign.id) + create_campaign_recipients(Q(**{'id__in': [user.id]}), campaign_id=running_campaign.id) recipient = NotificationCampaignRecipient.objects.get(campaign=running_campaign, user=user) send_campaign_batch( @@ -588,7 +590,7 @@ def test_send_campaign_batch_marks_failed_when_notification_type_missing(self, r def test_send_campaign_batch_skips_stale_run_id(self, running_campaign): user = UserFactory() - create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=running_campaign.id) + create_campaign_recipients(Q(**{'id__in': [user.id]}), campaign_id=running_campaign.id) recipient = NotificationCampaignRecipient.objects.get(campaign=running_campaign, user=user) send_campaign_batch( @@ -606,7 +608,7 @@ def test_send_campaign_batch_skips_cancelled_campaign(self, running_campaign): user = UserFactory() running_campaign.status = NotificationCampaignStatus.CANCELLED running_campaign.save(update_fields=['status']) - create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=running_campaign.id) + create_campaign_recipients(Q(**{'id__in': [user.id]}), campaign_id=running_campaign.id) recipient = NotificationCampaignRecipient.objects.get(campaign=running_campaign, user=user) send_campaign_batch( @@ -628,7 +630,7 @@ def test_send_campaign_batch_uses_fallback_email_when_username_has_no_at(self, m user.emails.all().delete() user.emails.create(address='fallback@example.com') - create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=running_campaign.id) + create_campaign_recipients(Q(**{'id__in': [user.id]}), campaign_id=running_campaign.id) recipient = NotificationCampaignRecipient.objects.get(campaign=running_campaign, user=user) send_campaign_batch( @@ -676,7 +678,7 @@ def test_process_campaign_retry_marks_completed_and_aggregates_stats(self, campa sent_user = UserFactory() skipped_user = UserFactory() create_campaign_recipients( - filters={'id__in': [sent_user.id, skipped_user.id]}, + Q(**{'id__in': [sent_user.id, skipped_user.id]}), campaign_id=campaign.id, ) NotificationCampaignRecipient.objects.filter(campaign=campaign, user=sent_user).update( @@ -699,7 +701,7 @@ def test_process_campaign_retry_marks_completed_and_aggregates_stats(self, campa 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) + create_campaign_recipients(Q(**{'id__in': [user.id]}), campaign_id=campaign.id) NotificationCampaignRecipient.objects.filter(campaign=campaign).update( status=NotificationCampaignRecipientStatus.SENT ) @@ -719,7 +721,7 @@ def test_process_campaign_retry_keeps_cancelled_status_and_syncs_stats(self, moc sent_user = UserFactory() failed_user = UserFactory() create_campaign_recipients( - filters={'id__in': [sent_user.id, failed_user.id]}, + Q(**{'id__in': [sent_user.id, failed_user.id]}), campaign_id=campaign.id, ) NotificationCampaignRecipient.objects.filter(campaign=campaign, user=sent_user).update( @@ -745,7 +747,7 @@ def test_process_campaign_retry_keeps_cancelled_status_and_syncs_stats(self, moc @mock.patch('osf.email.notification_campaign.chain') def test_process_campaign_retry_retries_failed_recipients(self, mock_chain, campaign): user = UserFactory() - create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=campaign.id) + create_campaign_recipients(Q(**{'id__in': [user.id]}), campaign_id=campaign.id) recipient = NotificationCampaignRecipient.objects.get(campaign=campaign, user=user) recipient.status = NotificationCampaignRecipientStatus.FAILED recipient.save(update_fields=['status']) @@ -764,7 +766,7 @@ def test_process_campaign_retry_retries_failed_recipients(self, mock_chain, camp def test_process_campaign_retry_marks_partially_completed_after_max_retries(self, campaign): user = UserFactory() - create_campaign_recipients(filters={'id__in': [user.id]}, campaign_id=campaign.id) + create_campaign_recipients(Q(**{'id__in': [user.id]}), campaign_id=campaign.id) recipient = NotificationCampaignRecipient.objects.get(campaign=campaign, user=user) recipient.status = NotificationCampaignRecipientStatus.FAILED recipient.save(update_fields=['status']) From 64efdc412316df2b2ab1fbc4845bd4aea8bc84ca Mon Sep 17 00:00:00 2001 From: antkryt Date: Mon, 3 Aug 2026 17:07:06 +0300 Subject: [PATCH 20/25] [ENG-11881] Time window needs to be set in seconds (#11847) * Change notification campaign time window to be in seconds --- admin/notifications/forms.py | 4 ++-- .../notification_campaigns_detail.html | 2 +- .../notification_campaing_create.html | 2 +- admin_tests/notifications/test_campaigns.py | 14 +++++++------- osf/email/notification_campaign.py | 6 +++--- osf_tests/test_notification_campaign.py | 2 +- 6 files changed, 15 insertions(+), 15 deletions(-) diff --git a/admin/notifications/forms.py b/admin/notifications/forms.py index ec104ab7e1b..9b576a0d3e9 100644 --- a/admin/notifications/forms.py +++ b/admin/notifications/forms.py @@ -41,8 +41,8 @@ class NotificationCampaignCreateForm(forms.ModelForm): time_window = forms.IntegerField( min_value=1, - initial=8, - help_text='The time in hours before the developer reminder is sent.', + initial=8 * 60 * 60, # 8 hours + help_text='The time in seconds before the developer reminder is sent.', ) sendgrid_bulk = forms.BooleanField( diff --git a/admin/templates/notifications/notification_campaigns_detail.html b/admin/templates/notifications/notification_campaigns_detail.html index 5b259abfcfe..4a64edbd351 100644 --- a/admin/templates/notifications/notification_campaigns_detail.html +++ b/admin/templates/notifications/notification_campaigns_detail.html @@ -158,7 +158,7 @@

      Progress

      {% if notification_campaign.developer_reminder_sent and notification_campaign.status == "running" %}
      Warning! - The campaign exceeded the expected timeframe ({{ metadata.execution.time_window }}h). A reminder was sent. + The campaign exceeded the expected timeframe ({{ metadata.execution.time_window }}s). A reminder was sent.
      {% endif %} {% endif %} diff --git a/admin/templates/notifications/notification_campaing_create.html b/admin/templates/notifications/notification_campaing_create.html index ad7346bd98d..d02b5dfb11d 100644 --- a/admin/templates/notifications/notification_campaing_create.html +++ b/admin/templates/notifications/notification_campaing_create.html @@ -226,7 +226,7 @@

      Execution

      min="1" >

      - The time in hours before the developer reminder is sent. + The time in seconds before the developer reminder is sent.

      diff --git a/admin_tests/notifications/test_campaigns.py b/admin_tests/notifications/test_campaigns.py index 5b91e76ff27..b1a04d42bfe 100644 --- a/admin_tests/notifications/test_campaigns.py +++ b/admin_tests/notifications/test_campaigns.py @@ -59,7 +59,7 @@ def _valid_form_data(notification_type, **overrides): 'max_retries': settings.DEFAULT_CAMPAIGN_MAX_RETRIES, 'activity_threshold': settings.DEFAULT_CAMPAIGN_ACTIVITY_THRESHOLD, 'sendgrid_bulk': False, - 'time_window': 8, + 'time_window': 8 * 60 * 60, } data.update(overrides) return data @@ -75,7 +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['time_window'] == 8 * 60 * 60 assert form.cleaned_data['sendgrid_bulk'] is False def test_defaults_come_from_settings(self): @@ -83,7 +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['time_window'].initial == 8 * 60 * 60 assert form.fields['sendgrid_bulk'].initial is False def test_invalid_context_json(self, notification_type): @@ -138,10 +138,10 @@ def test_time_window_must_be_at_least_one(self, notification_type): def test_time_window_is_accepted(self, notification_type): form = NotificationCampaignCreateForm( - data=_valid_form_data(notification_type, time_window=6) + data=_valid_form_data(notification_type, time_window=3600) ) assert form.is_valid() - assert form.cleaned_data['time_window'] == 6 + assert form.cleaned_data['time_window'] == 3600 def test_name_is_required(self, notification_type): form = NotificationCampaignCreateForm( @@ -191,7 +191,7 @@ def test_form_valid_persists_execution_metadata(self): max_retries=4, activity_threshold=77, sendgrid_bulk=True, - time_window=8 + time_window=28800 ), ) request.user = self.user @@ -213,7 +213,7 @@ def test_form_valid_persists_execution_metadata(self): 'batch_size': 25, 'max_retries': 4, 'activity_threshold': 77, - 'time_window': 8 + 'time_window': 28800 } assert campaign.metadata['sendgrid_bulk'] is True assert campaign.metadata['filters'] == {'predefined': 'active'} diff --git a/osf/email/notification_campaign.py b/osf/email/notification_campaign.py index 7d6759ec2f6..0169466da61 100644 --- a/osf/email/notification_campaign.py +++ b/osf/email/notification_campaign.py @@ -321,10 +321,10 @@ def send_campaign_batch(context, recipients_ids, notification_type_name='blank', campaign.save() return - execution_time_window = campaign.metadata.get('execution', {}).get('time_window', 8) - if campaign.started_at < timezone.now() - timedelta(hours=execution_time_window): + execution_time_window = campaign.metadata.get('execution', {}).get('time_window', 8 * 60 * 60) + if campaign.started_at < timezone.now() - timedelta(seconds=execution_time_window): if not campaign.developer_reminder_sent: - message = f'[Notification Campaign] Campaign {campaign_id} exceeded its execution time window ({execution_time_window}h).' + message = f'[Notification Campaign] Campaign {campaign_id} exceeded its execution time window ({execution_time_window}s).' logger.warning(message) sentry.log_message(message) campaign.developer_reminder_sent = True diff --git a/osf_tests/test_notification_campaign.py b/osf_tests/test_notification_campaign.py index 1c160b852e2..e88ba9a64a4 100644 --- a/osf_tests/test_notification_campaign.py +++ b/osf_tests/test_notification_campaign.py @@ -551,7 +551,7 @@ def test_send_campaign_batch_sendgrid_bulk_failure(self, mock_sentry, mock_sendg @mock.patch('osf.email.notification_campaign.sentry.log_message') def test_send_campaign_batch_logs_when_time_window_exceeded(self, mock_sentry, running_campaign): user = UserFactory() - running_campaign.started_at = timezone.now() - timedelta(hours=9) + running_campaign.started_at = timezone.now() - timedelta(seconds=9) running_campaign.metadata['execution']['time_window'] = 8 running_campaign.save() create_campaign_recipients(Q(**{'id__in': [user.id]}), campaign_id=running_campaign.id) From 765b17dcb2b71b7df98edff74609e3ab14f36991 Mon Sep 17 00:00:00 2001 From: antkryt Date: Mon, 3 Aug 2026 17:09:56 +0300 Subject: [PATCH 21/25] [ENG-11882] Add a filter on username to include non-email usernames (#11848) * Additional user lookups for notification campaign --- admin/notifications/views.py | 8 +++ .../notification_campaing_create.html | 24 ++++++++ osf/email/notification_campaign.py | 14 ++++- osf_tests/test_notification_campaign.py | 57 ++++++++++++++++++- 4 files changed, 100 insertions(+), 3 deletions(-) diff --git a/admin/notifications/views.py b/admin/notifications/views.py index 4a35545eab2..b22b5dd8346 100644 --- a/admin/notifications/views.py +++ b/admin/notifications/views.py @@ -494,10 +494,14 @@ def get_context_data(self, *args, **kwargs): 'iexact': 'Equals (case insensitive)', 'contains': 'Contains', 'icontains': 'Contains (case insensitive)', + 'not_contains': 'Does not contain', + 'not_icontains': 'Does not contain (case insensitive)', 'startswith': 'Starts with', 'istartswith': 'Starts with (case insensitive)', 'endswith': 'Ends with', 'iendswith': 'Ends with (case insensitive)', + 'regex': 'Matches regex', + 'iregex': 'Matches regex (case insensitive)', 'in': 'In', 'isnull': 'Is empty', }, @@ -506,10 +510,14 @@ def get_context_data(self, *args, **kwargs): 'iexact': 'Equals (case insensitive)', 'contains': 'Contains', 'icontains': 'Contains (case insensitive)', + 'not_contains': 'Does not contain', + 'not_icontains': 'Does not contain (case insensitive)', 'startswith': 'Starts with', 'istartswith': 'Starts with (case insensitive)', 'endswith': 'Ends with', 'iendswith': 'Ends with (case insensitive)', + 'regex': 'Matches regex', + 'iregex': 'Matches regex (case insensitive)', 'isnull': 'Is empty', }, models.IntegerField: { diff --git a/admin/templates/notifications/notification_campaing_create.html b/admin/templates/notifications/notification_campaing_create.html index d02b5dfb11d..873fafb0635 100644 --- a/admin/templates/notifications/notification_campaing_create.html +++ b/admin/templates/notifications/notification_campaing_create.html @@ -12,6 +12,30 @@ padding-left: 24px; border-left: 2px solid #000000; margin-left: 8px; + display: flex; + flex-direction: column; + gap: 8px; +} +.group > .children > .condition { + display: flex; + flex-direction: row; + flex-wrap: nowrap; + align-items: center; + gap: 8px; +} +.group > .children > .condition .field, +.group > .children > .condition .lookup { + width: auto; + min-width: 140px; + flex: 0 1 auto; +} +.group > .children > .condition .value { + width: auto; + min-width: 160px; + flex: 1 1 auto; +} +.group > .children > .condition .remove-condition { + flex: 0 0 auto; } .group-toolbar { display: flex; diff --git a/osf/email/notification_campaign.py b/osf/email/notification_campaign.py index 0169466da61..b33a3963af2 100644 --- a/osf/email/notification_campaign.py +++ b/osf/email/notification_campaign.py @@ -45,12 +45,22 @@ def build_query(node): if 'field' in node: value = node['value'] + lookup = node['lookup'] + negated_lookups = { # not native Django field lookups + 'not_contains': 'contains', + 'not_icontains': 'icontains', + } - if node['lookup'] == 'in': + if lookup == 'in': value = [v.strip() for v in value.split(',')] + if lookup in negated_lookups: + return ~Q(**{ + f'{node["field"]}__{negated_lookups[lookup]}': value + }) + return Q(**{ - f'{node["field"]}__{node["lookup"]}': value + f'{node["field"]}__{lookup}': value }) operator = node.get('operator', 'AND') diff --git a/osf_tests/test_notification_campaign.py b/osf_tests/test_notification_campaign.py index e88ba9a64a4..a54e1f16fed 100644 --- a/osf_tests/test_notification_campaign.py +++ b/osf_tests/test_notification_campaign.py @@ -15,7 +15,7 @@ start_notification_campaign, build_query, ) -from osf.models import UserActivityCounter +from osf.models import UserActivityCounter, OSFUser from osf.models.notification_campaign import ( NotificationCampaign, NotificationCampaignRecipient, @@ -81,6 +81,61 @@ def _recipient_scores(campaign_id, **batch_kwargs): return scores +class TestBuildQuery: + + def test_not_contains_excludes_matching_usernames(self): + email_user = UserFactory(username='user@example.com') + plain_user = UserFactory() + plain_user.username = 'deleted user' + plain_user.save(update_fields=['username']) + + query = build_query({ + 'field': 'username', + 'lookup': 'not_contains', + 'value': '@', + }) + user_ids = set(OSFUser.objects.filter(query).values_list('id', flat=True)) + + assert plain_user.id in user_ids + assert email_user.id not in user_ids + + def test_regex_matches_usernames_without_at(self): + email_user = UserFactory(username='user@example.com') + plain_user = UserFactory() + plain_user.username = 'gdpr-deleted-id' + plain_user.save(update_fields=['username']) + + query = build_query({ + 'field': 'username', + 'lookup': 'regex', + 'value': r'^[^@]+$', + }) + user_ids = set(OSFUser.objects.filter(query).values_list('id', flat=True)) + + assert plain_user.id in user_ids + assert email_user.id not in user_ids + + def test_or_combines_usernames(self): + staging_user = UserFactory(username='tester@staging.example') + plain_user = UserFactory() + plain_user.username = 'uuid-style-name' + plain_user.save(update_fields=['username']) + other_email = UserFactory(username='other@elsewhere.example') + + query = build_query({ + 'operator': 'OR', + 'children': [ + {'field': 'username', 'lookup': 'endswith', 'value': '@staging.example'}, + {'field': 'username', 'lookup': 'not_contains', 'value': '@'}, + ], + }) + user_ids = set(OSFUser.objects.filter(query).values_list('id', flat=True)) + + assert staging_user.id in user_ids + assert plain_user.id in user_ids + assert other_email.id not in user_ids + + class TestCreateCampaignRecipients: def test_creates_recipients_with_activity_scores(self, campaign): From 0871cbebd88e4fccffb8a029521f5b84b1e77743 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Tue, 4 Aug 2026 08:59:58 -0400 Subject: [PATCH 22/25] [ENG-11891] Part 1: Add and normalize logs and sentry messages for campaign and batches (#11850) * Add and normalize logs and sentry messages for campaign and batches * Fix type in logs * Add time check and logging for each notification emit in batch --- admin/notifications/forms.py | 2 +- osf/email/notification_campaign.py | 68 +++++++++++++++++++++--------- website/settings/defaults.py | 5 +++ 3 files changed, 55 insertions(+), 20 deletions(-) diff --git a/admin/notifications/forms.py b/admin/notifications/forms.py index 9b576a0d3e9..050f064bf0f 100644 --- a/admin/notifications/forms.py +++ b/admin/notifications/forms.py @@ -41,7 +41,7 @@ class NotificationCampaignCreateForm(forms.ModelForm): time_window = forms.IntegerField( min_value=1, - initial=8 * 60 * 60, # 8 hours + initial=settings.DEFAULT_CAMPAIGN_WINDOW_TIME, help_text='The time in seconds before the developer reminder is sent.', ) diff --git a/osf/email/notification_campaign.py b/osf/email/notification_campaign.py index b33a3963af2..8b57fa20f1a 100644 --- a/osf/email/notification_campaign.py +++ b/osf/email/notification_campaign.py @@ -204,10 +204,9 @@ def process_campaign_retry(*args, **kwargs): failed_recipients_count = failed_recipients.count() if failed_recipients_count: if campaign.retries < max_retries: - message = ( - f"[Notification Campaign] Retrying " - f"{failed_recipients_count} failed recipients for campaign {campaign_id}" - ) + message = (f'[Notification Campaign #{campaign_id}] WARNING: ' + f'Retrying {failed_recipients_count} failed recipients, ' + f'previous retry attempts: {campaign.retries}/{max_retries}') logger.info(message) sentry.log_message(message) campaign.retries += 1 @@ -228,7 +227,7 @@ def process_campaign_retry(*args, **kwargs): final_status = NotificationCampaignStatus.PARTIALLY_COMPLETED else: - message = f'[Notification Campaign] Campaign {campaign_id} {campaign.name} was cancelled.' + message = f'[Notification Campaign #{campaign_id}] WARNING: Campaign {campaign.name} was cancelled.' logger.info(message) sentry.log_message(message) @@ -267,9 +266,18 @@ def start_notification_campaign(campaign_id, restart_failed=False, restart_stuck filters = build_query(filters.get('manual', [])) if not restart_failed and not restart_stuck: + recipients_creation_started_at = timezone.now() create_campaign_recipients(filters=filters, campaign_id=campaign_id) campaign.recipient_count = NotificationCampaignRecipient.objects.filter(campaign_id=campaign_id).count() campaign.save() + recipients_creation_finished_at = timezone.now() + recipients_creation_run_time = (recipients_creation_finished_at - recipients_creation_started_at).total_seconds() + message = (f'[Notification Campaign #{campaign_id}] INFO: ' + f'Recipients creation finished in {recipients_creation_run_time} seconds ' + f'(start={recipients_creation_started_at}, finish={recipients_creation_finished_at}) ' + f'for Campaign {campaign.name} (start={campaign.started_at}).') + logger.info(message) + sentry.log_message(message) execution = campaign.metadata.get('execution', {}) batch_size = execution.get('batch_size', settings.DEFAULT_CAMPAIGN_BATCH_SIZE) @@ -318,23 +326,27 @@ def send_campaign_batch(context, recipients_ids, notification_type_name='blank', if campaign.status == NotificationCampaignStatus.CANCELLED: logger.warning(f"Campaign {campaign_id} was cancelled") return + batch_started_at = timezone.now() if hasattr(NotificationTypeEnum, notification_type_name): notification_type = getattr(NotificationTypeEnum, notification_type_name).instance else: notification_type = NotificationType.objects.filter( name=notification_type_name ).first() # TODO cache - if notification_type is None: if campaign.status != NotificationCampaignStatus.FAILED: campaign.status = NotificationCampaignStatus.FAILED campaign.save() + message = f'[Notification Campaign #{campaign_id}] ERROR: Batch failed due to none notification_type (template)' + logger.error(message) + sentry.log_message(message) return - execution_time_window = campaign.metadata.get('execution', {}).get('time_window', 8 * 60 * 60) + execution_time_window = campaign.metadata.get('execution', {}).get('time_window', settings.DEFAULT_CAMPAIGN_WINDOW_TIME) if campaign.started_at < timezone.now() - timedelta(seconds=execution_time_window): if not campaign.developer_reminder_sent: - message = f'[Notification Campaign] Campaign {campaign_id} exceeded its execution time window ({execution_time_window}s).' + message = (f'[Notification Campaign #{campaign_id}] WARNING: ' + f'Campaign {campaign.name} exceeded its execution time window ({execution_time_window} seconds).') logger.warning(message) sentry.log_message(message) campaign.developer_reminder_sent = True @@ -354,41 +366,50 @@ def send_campaign_batch(context, recipients_ids, notification_type_name='blank', invalid_emails_qs.update(status=NotificationCampaignRecipientStatus.SKIPPED, error_message='Invalid email address') if campaign.metadata.get('sendgrid_bulk', False): + # NOTE: sendgrid bulk send feature has not been fully implemented and tested recipient_emails = list(valid_emails_qs.values_list('recipient_address', flat=True)) try: send_email_with_send_grid(to_addr=recipient_emails, notification_type=notification_type, context=context) valid_emails_qs.update(status=NotificationCampaignRecipientStatus.SENT, error_message=None) except Exception as exc: - message = f'[Notification Campaign] Campaign {campaign_id} sendgrid bulk request failed. {str(exc)}' + message = (f'[Notification Campaign #{campaign_id}] ERROR: ' + f'Campaign {campaign.name} sendgrid bulk request failed, error={str(exc)}') logger.error(message) - sentry.log_exception(message) + sentry.log_message(message) valid_emails_qs.update(status=NotificationCampaignRecipientStatus.FAILED, error_message=str(exc)) - else: for recipient in valid_emails_qs: + notification_started_at = timezone.now() try: notification_type.emit( user=recipient.user, event_context=context, save=False, # Too many write operations ) - recipient.status = NotificationCampaignRecipientStatus.SENT recipient.error_message = None recipient_records.append(recipient) except Exception as exc: - message = f'[Notification Campaign] Campaign {campaign_id} sendgrid request failed for user {recipient.user.username}. {str(exc)}' + message = (f'[Notification Campaign #{campaign_id}] ERROR:' + f'SendGrid request failed for user {recipient.user.username} ({recipient.user._id}),' + f'error={str(exc)}') logger.error(message) - sentry.log_exception(message) - + sentry.log_message(message) recipient.status = NotificationCampaignRecipientStatus.FAILED recipient.error_message = str(exc) recipient_records.append(recipient) - + notification_finished_at = timezone.now() + notification_sent_run_time = (notification_finished_at - notification_started_at).total_seconds() + if notification_sent_run_time > settings.ESTIMATED_PER_REQUEST_THRESHOLD: + message = (f'[Notification Campaign #{campaign_id}] WARNING: Slow Notification, ' + f'run_time(threshold)={notification_sent_run_time}({settings.ESTIMATED_PER_REQUEST_THRESHOLD}), ' + f'user={recipient.user.username}({recipient.user._id})' + f'campaign_name={campaign.name}') + logger.warning(message) + sentry.log_message(message) NotificationCampaignRecipient.objects.bulk_update(recipient_records, ['status', 'error_message']) - # Lock the campaign row so concurrent batches cannot - # overwrite counters with a stale aggregate snapshot + # Lock the campaign row so concurrent batches cannot overwrite counters with a stale aggregate snapshot with transaction.atomic(): notification_campaign = NotificationCampaign.objects.select_for_update().get(pk=campaign_id) stats = get_campaign_recipient_stats(campaign_id) @@ -396,4 +417,13 @@ def send_campaign_batch(context, recipients_ids, notification_type_name='blank', notification_campaign.failed_count = stats['failed_count'] notification_campaign.save(update_fields=['sent_count', 'failed_count']) - logger.info('Batch finished') # TODO: add/update logs + batch_finished_at = timezone.now() + batch_run_time = (batch_finished_at - batch_started_at).total_seconds() + if batch_run_time > settings.ESTIMATED_BATCH_RUN_TIME_THRESHOLD: + message = (f'[Notification Campaign #{campaign_id}] WARNING: Slow Batch, ' + f'run_time(threshold)={batch_run_time}({settings.ESTIMATED_BATCH_RUN_TIME_THRESHOLD}), ' + f'campaign_name={campaign.name}') + logger.warning(message) + sentry.log_message(message) + logger.info(f'[Notification Campaign #{campaign_id}] INFO: ' + f'Batch finished in {batch_run_time} seconds for campaign {campaign.name}') diff --git a/website/settings/defaults.py b/website/settings/defaults.py index 583b9ae6cbc..69be3828026 100644 --- a/website/settings/defaults.py +++ b/website/settings/defaults.py @@ -194,7 +194,12 @@ def parent_dir(path): # Notification campaign execution defaults (overridable per campaign in admin metadata) DEFAULT_CAMPAIGN_ACTIVITY_THRESHOLD = 3 # Users at/above this activity total are scheduled in the high-activity phase DEFAULT_CAMPAIGN_BATCH_SIZE = 1000 +DEFAULT_CAMPAIGN_WINDOW_TIME = 28800 # 8 hours DEFAULT_CAMPAIGN_MAX_RETRIES = 3 +# The following are rough estimates so we can log to sentry those batches and sendgrid quests which run longer than normal +ESTIMATED_EIGHT_HOUR_WINDOW_USERS = 600000 # 600K Users (where the actual number is around 500K) +ESTIMATED_BATCH_RUN_TIME_THRESHOLD = DEFAULT_CAMPAIGN_WINDOW_TIME / (ESTIMATED_EIGHT_HOUR_WINDOW_USERS / DEFAULT_CAMPAIGN_BATCH_SIZE) # By default, 48 seconds per batch of 1000 requests +ESTIMATED_PER_REQUEST_THRESHOLD = ESTIMATED_BATCH_RUN_TIME_THRESHOLD / DEFAULT_CAMPAIGN_BATCH_SIZE # By default, 0.048 seconds (21 requests / second) # Configuration for "We miss you at OSF" email (`NotificationTypeEnum.USER_NO_LOGIN`) # Note: 1) we can gradually increase `MAX_DAILY_NO_LOGIN_EMAILS` to 10000, 100000, etc. or set it to `None` after we From 54b1719c1c75964868ec23bd448c853b2ce5e1f4 Mon Sep 17 00:00:00 2001 From: antkryt Date: Tue, 4 Aug 2026 17:03:00 +0300 Subject: [PATCH 23/25] [ENG-11889] Sort Campaign list (#11853) * Sort campaigns list by created time --- admin/notifications/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/admin/notifications/views.py b/admin/notifications/views.py index b22b5dd8346..fc27b02417b 100644 --- a/admin/notifications/views.py +++ b/admin/notifications/views.py @@ -348,7 +348,7 @@ def get_success_url(self, *args, **kwargs): class NotificationCampaignsList(PermissionRequiredMixin, ListView): paginate_by = 25 template_name = 'notifications/notification_campaigns_list.html' - ordering = 'name' + ordering = '-created_at' permission_required = 'osf.view_notificationcampaign' raise_exception = True model = NotificationCampaign From cc987e6b57e26fb52c3bd9f1113802280340a078 Mon Sep 17 00:00:00 2001 From: antkryt Date: Tue, 4 Aug 2026 17:03:33 +0300 Subject: [PATCH 24/25] [ENG-11888] Passing set time window error doesn't persist (#11851) * Keep developer reminder even if campaign is finished --- .../templates/notifications/notification_campaigns_detail.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/admin/templates/notifications/notification_campaigns_detail.html b/admin/templates/notifications/notification_campaigns_detail.html index 4a64edbd351..1c75d1f382a 100644 --- a/admin/templates/notifications/notification_campaigns_detail.html +++ b/admin/templates/notifications/notification_campaigns_detail.html @@ -155,7 +155,7 @@

      Progress

      The campaign may be stuck and can be restarted.
      {% endif %} -{% if notification_campaign.developer_reminder_sent and notification_campaign.status == "running" %} +{% if notification_campaign.developer_reminder_sent %}
      Warning! The campaign exceeded the expected timeframe ({{ metadata.execution.time_window }}s). A reminder was sent. From eaebfbc8a5643fa5c9d9e3978fa7a3d650f5d214 Mon Sep 17 00:00:00 2001 From: antkryt Date: Tue, 4 Aug 2026 17:25:55 +0300 Subject: [PATCH 25/25] [ENG-11871] Enforce manual option requires filters (#11854) * Enforce manual filters values cannot be none or empty --- admin/notifications/forms.py | 24 ++++++- .../notification_campaing_create.html | 26 ++++++- admin_tests/notifications/test_campaigns.py | 69 +++++++++++++++++++ 3 files changed, 117 insertions(+), 2 deletions(-) diff --git a/admin/notifications/forms.py b/admin/notifications/forms.py index 050f064bf0f..94b571dbb65 100644 --- a/admin/notifications/forms.py +++ b/admin/notifications/forms.py @@ -67,6 +67,28 @@ def clean_context(self): def clean_filters(self): value = self.cleaned_data['filters'] or '{}' try: - return json.loads(value) + data = json.loads(value) except Exception as e: raise forms.ValidationError(e) + + if isinstance(data, dict) and 'manual' in data: + self._validate_filter_node(data['manual']) + + return data + + def _validate_filter_node(self, node): + if not isinstance(node, dict): + raise forms.ValidationError('Invalid filter structure.') + + if 'field' in node: + if not str(node.get('field') or '').strip(): + raise forms.ValidationError('Filter field cannot be empty.') + if not str(node.get('lookup') or '').strip(): + raise forms.ValidationError('Filter lookup cannot be empty.') + value = node.get('value') + if value is None or (isinstance(value, str) and not value.strip()): + raise forms.ValidationError('Filter value cannot be empty.') + return + + for child in node.get('children') or []: + self._validate_filter_node(child) diff --git a/admin/templates/notifications/notification_campaing_create.html b/admin/templates/notifications/notification_campaing_create.html index 873fafb0635..c4e09df6a9b 100644 --- a/admin/templates/notifications/notification_campaing_create.html +++ b/admin/templates/notifications/notification_campaing_create.html @@ -371,6 +371,7 @@

      Execution

      case "booleanfield": newValue = document.createElement("select"); newValue.className = "value form-control"; + newValue.required = true; newValue.add(new Option("True", "True")); newValue.add(new Option("False", "False")); @@ -379,6 +380,7 @@

      Execution

      default: newValue = document.createElement("input"); newValue.className = "value form-control"; + newValue.required = true; switch (config.type) { case "datetimefield": @@ -421,11 +423,17 @@

      Execution

      ); } -form.addEventListener("submit", function () { +form.addEventListener("submit", function (e) { updateFiltersInput(); + if (!validateManualFilterValues()) { + e.preventDefault(); + } }); document.getElementById("preview-recipients").addEventListener("click", function () { + if (!validateManualFilterValues()) { + return; + } const filters = JSON.stringify(buildFilters()); const url = `/notifications/notification_campaigns_recipients_preview/?filters=${ @@ -435,6 +443,22 @@

      Execution

      window.open(url, "_blank"); }); +function validateManualFilterValues() { + if (filterMode.value !== "manual") { + return true; + } + + const values = builder.querySelectorAll(".condition .value"); + for (const input of values) { + if (!String(input.value || "").trim()) { + input.focus(); + alert("Filter value cannot be empty."); + return false; + } + } + return true; +} + document.getElementById("render-preview").addEventListener("click", async () => { const mockData = document.getElementById("context")?.value; diff --git a/admin_tests/notifications/test_campaigns.py b/admin_tests/notifications/test_campaigns.py index b1a04d42bfe..a213975163c 100644 --- a/admin_tests/notifications/test_campaigns.py +++ b/admin_tests/notifications/test_campaigns.py @@ -108,6 +108,75 @@ def test_empty_context_and_filters_default_to_empty_dict(self, notification_type assert form.cleaned_data['context'] == {} assert form.cleaned_data['filters'] == {} + def test_manual_filter_value_cannot_be_empty(self, notification_type): + filters = json.dumps({ + 'manual': { + 'operator': 'AND', + 'children': [ + {'field': 'username', 'lookup': 'contains', 'value': ''}, + ], + }, + }) + form = NotificationCampaignCreateForm( + data=_valid_form_data(notification_type, filters=filters) + ) + assert not form.is_valid() + assert 'filters' in form.errors + assert 'Filter value cannot be empty.' in form.errors['filters'][0] + + def test_manual_filter_value_cannot_be_none(self, notification_type): + filters = json.dumps({ + 'manual': { + 'operator': 'AND', + 'children': [ + {'field': 'username', 'lookup': 'contains', 'value': None}, + ], + }, + }) + form = NotificationCampaignCreateForm( + data=_valid_form_data(notification_type, filters=filters) + ) + assert not form.is_valid() + assert 'filters' in form.errors + assert 'Filter value cannot be empty.' in form.errors['filters'][0] + + def test_manual_filter_field_cannot_be_empty(self, notification_type): + filters = json.dumps({ + 'manual': { + 'operator': 'AND', + 'children': [ + {'field': '', 'lookup': 'contains', 'value': '@'}, + ], + }, + }) + form = NotificationCampaignCreateForm( + data=_valid_form_data(notification_type, filters=filters) + ) + assert not form.is_valid() + assert 'filters' in form.errors + assert 'Filter field cannot be empty.' in form.errors['filters'][0] + + def test_nested_manual_filter_value_cannot_be_empty(self, notification_type): + filters = json.dumps({ + 'manual': { + 'operator': 'OR', + 'children': [ + {'field': 'username', 'lookup': 'endswith', 'value': '@cos.io'}, + { + 'operator': 'AND', + 'children': [ + {'field': 'username', 'lookup': 'not_contains', 'value': ' '}, + ], + }, + ], + }, + }) + form = NotificationCampaignCreateForm( + data=_valid_form_data(notification_type, filters=filters) + ) + assert not form.is_valid() + assert 'filters' in form.errors + def test_batch_size_must_be_at_least_one(self, notification_type): form = NotificationCampaignCreateForm( data=_valid_form_data(notification_type, batch_size=0)