From f0b70565ef8e2696553d56e8430262d20f43c5cd Mon Sep 17 00:00:00 2001 From: Ostap Zherebetskyi Date: Thu, 8 Jan 2026 13:01:55 +0200 Subject: [PATCH 01/11] Add fake_sent field to Notification model and update notification creation logic --- osf/admin.py | 2 +- osf/models/notification.py | 1 + osf/models/notification_subscription.py | 5 +++-- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/osf/admin.py b/osf/admin.py index 2f5698b2aca..d9fed50b7ff 100644 --- a/osf/admin.py +++ b/osf/admin.py @@ -367,7 +367,7 @@ class EmailTaskAdmin(admin.ModelAdmin): @admin.register(Notification) class NotificationAdmin(admin.ModelAdmin): - list_display = ('user', 'notification_type_name', 'sent', 'seen') + list_display = ('user', 'notification_type_name', 'sent', 'fake_sent') list_filter = ('sent',) search_fields = ('subscription__notification_type__name', 'subscription__user__username') list_per_page = 50 diff --git a/osf/models/notification.py b/osf/models/notification.py index aab9f2b0f0e..d9174cad8ef 100644 --- a/osf/models/notification.py +++ b/osf/models/notification.py @@ -18,6 +18,7 @@ class Notification(models.Model): sent = models.DateTimeField(null=True, blank=True) seen = models.DateTimeField(null=True, blank=True) created = models.DateTimeField(auto_now_add=True) + fake_sent = models.BooleanField(default=False) def send( self, diff --git a/osf/models/notification_subscription.py b/osf/models/notification_subscription.py index 6a4a27533b5..25910d9dee7 100644 --- a/osf/models/notification_subscription.py +++ b/osf/models/notification_subscription.py @@ -1,5 +1,5 @@ import logging -from datetime import datetime +from django.utils import timezone from django.db import models from django.contrib.contenttypes.fields import GenericForeignKey from django.contrib.contenttypes.models import ContentType @@ -126,7 +126,8 @@ def emit( Notification.objects.create( subscription=self, event_context=event_context, - sent=None if self.message_frequency != 'none' else datetime(1000, 1, 1), + sent=timezone.now() if self.message_frequency == 'none' else None, + fake_sent=True if self.message_frequency == 'none' else False, ) @property From ce1483829e1148903ec3ff3565ab2e0a430fb843 Mon Sep 17 00:00:00 2001 From: Ostap Zherebetskyi Date: Thu, 8 Jan 2026 13:03:04 +0200 Subject: [PATCH 02/11] add unique_together constraint --- osf/models/notification_subscription.py | 1 + 1 file changed, 1 insertion(+) diff --git a/osf/models/notification_subscription.py b/osf/models/notification_subscription.py index 25910d9dee7..1e38f4cbdff 100644 --- a/osf/models/notification_subscription.py +++ b/osf/models/notification_subscription.py @@ -59,6 +59,7 @@ class Meta: verbose_name = 'Notification Subscription' verbose_name_plural = 'Notification Subscriptions' db_table = 'osf_notificationsubscription_v2' + unique_together = ('notification_type', 'user', 'content_type', 'object_id', '_is_digest') def emit( self, From 0e3f9bcbebea1cef2537e2450173728a49a4f41f Mon Sep 17 00:00:00 2001 From: Ostap Zherebetskyi Date: Thu, 8 Jan 2026 13:13:21 +0200 Subject: [PATCH 03/11] Add 'PARTIAL_SUCCESS' status to EmailTask model and update email task handling logic --- notifications/tasks.py | 14 ++++++++------ osf/models/email_task.py | 1 + 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/notifications/tasks.py b/notifications/tasks.py index 84a825088f2..c9cf6beb06f 100644 --- a/notifications/tasks.py +++ b/notifications/tasks.py @@ -102,9 +102,10 @@ def send_user_email_task(self, user_id, notification_ids, **kwargs): notifications_qs = notifications_qs.exclude(id__in=failed_notifications) if not rendered_notifications: + email_task.status = 'SUCCESS' if email_task.error_message: logger.error(f'Partial success for send_user_email_task for user {user_id}. Task id: {self.request.id}. Errors: {email_task.error_message}') - email_task.status = 'SUCCESS' + email_task.status = 'PARTIAL_SUCCESS' email_task.save() return @@ -123,10 +124,10 @@ def send_user_email_task(self, user_id, notification_ids, **kwargs): notifications_qs.update(sent=timezone.now()) email_task.status = 'SUCCESS' - email_task.save() - if email_task.error_message: logger.error(f'Partial success for send_user_email_task for user {user_id}. Task id: {self.request.id}. Errors: {email_task.error_message}') + email_task.status = 'PARTIAL_SUCCESS' + email_task.save() except Exception as e: retry_count = self.request.retries @@ -177,9 +178,10 @@ def send_moderator_email_task(self, user_id, notification_ids, provider_content_ notifications_qs = notifications_qs.exclude(id__in=failed_notifications) if not rendered_notifications: + email_task.status = 'SUCCESS' if email_task.error_message: logger.error(f'Partial success for send_moderator_email_task for user {user_id}. Task id: {self.request.id}. Errors: {email_task.error_message}') - email_task.status = 'SUCCESS' + email_task.status = 'PARTIAL_SUCCESS' email_task.save() return @@ -274,10 +276,10 @@ def send_moderator_email_task(self, user_id, notification_ids, provider_content_ notifications_qs.update(sent=timezone.now()) email_task.status = 'SUCCESS' - email_task.save() - if email_task.error_message: logger.error(f'Partial success for send_moderator_email_task for user {user_id}. Task id: {self.request.id}. Errors: {email_task.error_message}') + email_task.status = 'PARTIAL_SUCCESS' + email_task.save() except Exception as e: retry_count = self.request.retries diff --git a/osf/models/email_task.py b/osf/models/email_task.py index f89f2285e5c..e7f52fbaf18 100644 --- a/osf/models/email_task.py +++ b/osf/models/email_task.py @@ -9,6 +9,7 @@ class EmailTask(models.Model): ('SUCCESS', 'Success'), ('FAILURE', 'Failure'), ('RETRY', 'Retry'), + ('PARTIAL_SUCCESS', 'Partial Success'), ) task_id = models.CharField(max_length=255, unique=True) From e90bb1c656343e99766f2e9d80962b3651defbd9 Mon Sep 17 00:00:00 2001 From: Ostap Zherebetskyi Date: Thu, 8 Jan 2026 15:02:16 +0200 Subject: [PATCH 04/11] NR migration [ENG-10040, ENG-10025, ENG-9854] --- ...ke_sent_alter_emailtask_status_and_more.py | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 osf/migrations/0036_notification_fake_sent_alter_emailtask_status_and_more.py diff --git a/osf/migrations/0036_notification_fake_sent_alter_emailtask_status_and_more.py b/osf/migrations/0036_notification_fake_sent_alter_emailtask_status_and_more.py new file mode 100644 index 00000000000..2b9fc8f1b1c --- /dev/null +++ b/osf/migrations/0036_notification_fake_sent_alter_emailtask_status_and_more.py @@ -0,0 +1,28 @@ +# Generated by Django 4.2.15 on 2026-01-08 12:57 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('contenttypes', '0002_remove_content_type_name'), + ('osf', '0035_merge_20251215_1451'), + ] + + operations = [ + migrations.AddField( + model_name='notification', + name='fake_sent', + field=models.BooleanField(default=False), + ), + migrations.AlterField( + model_name='emailtask', + name='status', + field=models.CharField(choices=[('PENDING', 'Pending'), ('NO_USER_FOUND', 'No User Found'), ('USER_DISABLED', 'User Disabled'), ('STARTED', 'Started'), ('SUCCESS', 'Success'), ('FAILURE', 'Failure'), ('RETRY', 'Retry'), ('PARTIAL_SUCCESS', 'Partial Success'), ('AUTO_FIXED', 'Auto Fixed')], default='PENDING', max_length=20), + ), + migrations.AlterUniqueTogether( + name='notificationsubscription', + unique_together={('notification_type', 'user', 'content_type', 'object_id', '_is_digest')}, + ), + ] From 08e1ea25e3911762ea82fa8d082b4efb4916ab9a Mon Sep 17 00:00:00 2001 From: Ostap Zherebetskyi Date: Thu, 8 Jan 2026 15:49:26 +0200 Subject: [PATCH 05/11] Remove subscription if notifications.tasks.send_moderator_email_task fails with permission error --- .../notifications/test_notifications_db_transaction.py | 3 +-- notifications/tasks.py | 7 ++++--- osf/models/email_task.py | 1 + 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/api_tests/notifications/test_notifications_db_transaction.py b/api_tests/notifications/test_notifications_db_transaction.py index dc09dd46487..0deb302477c 100644 --- a/api_tests/notifications/test_notifications_db_transaction.py +++ b/api_tests/notifications/test_notifications_db_transaction.py @@ -3,7 +3,6 @@ AuthUserFactory, NotificationTypeFactory ) -from datetime import datetime from osf.models import Notification, NotificationType, NotificationSubscription from tests.utils import capture_notifications from django.db import reset_queries, connection @@ -54,5 +53,5 @@ def test_emit_frequency_none(self, user_one, test_notification_type): ) assert Notification.objects.filter( subscription__notification_type=test_notification_type, - sent=datetime(1000, 1, 1) + fake_sent=True ).exists() diff --git a/notifications/tasks.py b/notifications/tasks.py index c9cf6beb06f..ac00597b640 100644 --- a/notifications/tasks.py +++ b/notifications/tasks.py @@ -36,7 +36,8 @@ def safe_render_notification(notifications, email_task): # Mark notifications that failed to render as fake sent # Use 1000/12/31 to distinguish itself from another type of fake sent 1000/1/1 log_message(f'Error rendering notification, mark as fake sent: [notification_id={notification.id}]') - notification.sent = datetime(1000, 12, 31) + notification.mark_sent() + notification.fake_sent = True notification.save() continue @@ -213,10 +214,10 @@ def send_moderator_email_task(self, user_id, notification_ids, provider_content_ current_admins = provider.get_group('admin') if current_admins is None or not current_admins.user_set.filter(id=user.id).exists(): log_message(f"User is not a moderator for provider {provider._id} - notifications will be marked as sent.") - email_task.status = 'FAILURE' + email_task.status = 'AUTO_FIXED' email_task.error_message = f'User is not a moderator for provider {provider._id}' email_task.save() - notifications_qs.update(sent=datetime(1000, 1, 1)) + notifications_qs.update(sent=timezone.now(), fake_sent=True) return additional_context = {} diff --git a/osf/models/email_task.py b/osf/models/email_task.py index e7f52fbaf18..12def4c8c12 100644 --- a/osf/models/email_task.py +++ b/osf/models/email_task.py @@ -10,6 +10,7 @@ class EmailTask(models.Model): ('FAILURE', 'Failure'), ('RETRY', 'Retry'), ('PARTIAL_SUCCESS', 'Partial Success'), + ('AUTO_FIXED', 'Auto Fixed'), ) task_id = models.CharField(max_length=255, unique=True) From ab06f57f4d1a9e68fa26192e84879f64e3305bae Mon Sep 17 00:00:00 2001 From: Ostap-Zherebetskyi Date: Mon, 19 Jan 2026 13:52:28 +0200 Subject: [PATCH 06/11] Apply suggestion from @Ostap-Zherebetskyi remove datetime remove datetime --- notifications/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notifications/tasks.py b/notifications/tasks.py index ac00597b640..fac028090f5 100644 --- a/notifications/tasks.py +++ b/notifications/tasks.py @@ -1,6 +1,6 @@ import itertools from calendar import monthrange -from datetime import date, datetime +from datetime import date from django.contrib.contenttypes.models import ContentType from django.db import connection from django.utils import timezone From c0f944fa67fa8f71e2b574b36e27f1ece948d3ae Mon Sep 17 00:00:00 2001 From: Ostap Zherebetskyi Date: Thu, 22 Jan 2026 13:34:01 +0200 Subject: [PATCH 07/11] Add 'no_login_email_last_sent' field to OSFUser and update email task logic --- ...> 0036_notification_fake_sent_and_more.py} | 8 +++++++- osf/models/user.py | 1 + scripts/triggered_mails.py | 20 +++++++++---------- tests/test_triggered_mails.py | 12 +++-------- 4 files changed, 21 insertions(+), 20 deletions(-) rename osf/migrations/{0036_notification_fake_sent_alter_emailtask_status_and_more.py => 0036_notification_fake_sent_and_more.py} (79%) diff --git a/osf/migrations/0036_notification_fake_sent_alter_emailtask_status_and_more.py b/osf/migrations/0036_notification_fake_sent_and_more.py similarity index 79% rename from osf/migrations/0036_notification_fake_sent_alter_emailtask_status_and_more.py rename to osf/migrations/0036_notification_fake_sent_and_more.py index 2b9fc8f1b1c..fa9de320228 100644 --- a/osf/migrations/0036_notification_fake_sent_alter_emailtask_status_and_more.py +++ b/osf/migrations/0036_notification_fake_sent_and_more.py @@ -1,6 +1,7 @@ -# Generated by Django 4.2.15 on 2026-01-08 12:57 +# Generated by Django 4.2.15 on 2026-01-22 10:29 from django.db import migrations, models +import osf.utils.fields class Migration(migrations.Migration): @@ -16,6 +17,11 @@ class Migration(migrations.Migration): name='fake_sent', field=models.BooleanField(default=False), ), + migrations.AddField( + model_name='osfuser', + name='no_login_email_last_sent', + field=osf.utils.fields.NonNaiveDateTimeField(blank=True, null=True), + ), migrations.AlterField( model_name='emailtask', name='status', diff --git a/osf/models/user.py b/osf/models/user.py index a8cbf66d5b3..8b8c0ede502 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -249,6 +249,7 @@ class OSFUser(DirtyFieldsMixin, GuidMixin, BaseModel, AbstractBaseUser, Permissi # } email_last_sent = NonNaiveDateTimeField(null=True, blank=True) + no_login_email_last_sent = NonNaiveDateTimeField(null=True, blank=True) change_password_last_attempt = NonNaiveDateTimeField(null=True, blank=True) # Logs number of times user attempted to change their password where their # old password was invalid diff --git a/scripts/triggered_mails.py b/scripts/triggered_mails.py index 4b56d12c5df..8d06245f9e6 100644 --- a/scripts/triggered_mails.py +++ b/scripts/triggered_mails.py @@ -60,31 +60,29 @@ def find_inactive_users_without_enqueued_or_sent_no_login(): Match your original inactivity rules, but exclude users who already have a no_login EmailTask either pending, started, retrying, or already sent successfully. """ + now = timezone.now() - # Subquery: Is there already a not-yet-failed/aborted EmailTask for this user with our prefix? - existing_no_login = EmailTask.objects.filter( - user_id=OuterRef('pk'), - task_id__startswith=NO_LOGIN_PREFIX, - status__in=['PENDING', 'STARTED', 'RETRY', 'SUCCESS'], - ) cutoff_query = Q(date_last_login__gte=settings.NO_LOGIN_EMAIL_CUTOFF - settings.NO_LOGIN_WAIT_TIME) if settings.NO_LOGIN_EMAIL_CUTOFF else Q() base_q = OSFUser.objects.filter( cutoff_query, is_active=True, ).filter( Q( - date_last_login__lt=timezone.now() - settings.NO_LOGIN_WAIT_TIME, + date_last_login__lt=now - settings.NO_LOGIN_WAIT_TIME, # NOT tagged osf4m ) & ~Q(tags__name='osf4m') | Q( - date_last_login__lt=timezone.now() - settings.NO_LOGIN_OSF4M_WAIT_TIME, + date_last_login__lt=now - settings.NO_LOGIN_OSF4M_WAIT_TIME, tags__name='osf4m' ) ).distinct() - # Exclude users who already have a task for this email type - return base_q.annotate(_has_task=Exists(existing_no_login)).filter(_has_task=False) + # Exclude users who have already received a no-login email recently + return base_q.filter( + Q(no_login_email_last_sent__isnull=True) | + Q(no_login_email_last_sent__lt=now - settings.NO_LOGIN_WAIT_TIME) + ) @celery_app.task(name='scripts.triggered_no_login_email') @@ -133,6 +131,8 @@ def send_no_login_email(email_task_id: int): ) email_task.status = 'SUCCESS' email_task.save() + user.no_login_email_last_sent = timezone.now() + user.save() except Exception as exc: # noqa: BLE001 logger.exception(f'EmailTask {email_task.id}: error while sending') diff --git a/tests/test_triggered_mails.py b/tests/test_triggered_mails.py index c482338ccff..946c6a5e84b 100644 --- a/tests/test_triggered_mails.py +++ b/tests/test_triggered_mails.py @@ -22,7 +22,7 @@ def _inactive_time(): """Make a timestamp that is definitely 'inactive' regardless of threshold settings.""" # Very conservative: 12 weeks ago - return timezone.now() - timedelta(weeks=12) + return timezone.now() - timedelta(weeks=52) def _recent_time(): @@ -114,21 +114,15 @@ def test_finder_returns_two_inactive_when_none_queued(self): assert ids == {u1.id, u2.id} def test_finder_excludes_users_with_existing_task(self): - """Inactive users but one already has a no_login EmailTask -> excluded.""" + """Inactive users but one already has a no_login_email_last_sent -> excluded.""" u1 = UserFactory(fullname='Jalen Hurts') u2 = UserFactory(fullname='Jason Kelece') u1.date_last_login = _inactive_time() u2.date_last_login = _inactive_time() + u2.no_login_email_last_sent = timezone.now() u1.save() u2.save() - # Pretend u2 already had this email flow (SUCCESS qualifies for exclusion) - EmailTask.objects.create( - task_id=f"{NO_LOGIN_PREFIX}existing-success", - user=u2, - status='SUCCESS', - ) - users = list(find_inactive_users_without_enqueued_or_sent_no_login()) ids = {u.id for u in users} assert ids == {u1.id} # u2 excluded because of existing task From cd800369c47c1f563418cb1b15e84fab562881ad Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Tue, 27 Jan 2026 15:35:11 -0500 Subject: [PATCH 08/11] Rename migration name for NR post-release --- ...ent_and_more.py => 0036_notification_refactor_post_release.py} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename osf/migrations/{0036_notification_fake_sent_and_more.py => 0036_notification_refactor_post_release.py} (100%) diff --git a/osf/migrations/0036_notification_fake_sent_and_more.py b/osf/migrations/0036_notification_refactor_post_release.py similarity index 100% rename from osf/migrations/0036_notification_fake_sent_and_more.py rename to osf/migrations/0036_notification_refactor_post_release.py From 6bc138c188826ed2428fcae25b15530e0cfda7e5 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Tue, 27 Jan 2026 15:57:34 -0500 Subject: [PATCH 09/11] Improve unit test: test_emit_frequency_none --- .../test_notifications_db_transaction.py | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/api_tests/notifications/test_notifications_db_transaction.py b/api_tests/notifications/test_notifications_db_transaction.py index 0deb302477c..2d2d7365494 100644 --- a/api_tests/notifications/test_notifications_db_transaction.py +++ b/api_tests/notifications/test_notifications_db_transaction.py @@ -1,11 +1,14 @@ +from django.db import reset_queries, connection +from django.utils import timezone + import pytest + +from osf.models import Notification, NotificationType, NotificationSubscription from osf_tests.factories import ( AuthUserFactory, NotificationTypeFactory ) -from osf.models import Notification, NotificationType, NotificationSubscription from tests.utils import capture_notifications -from django.db import reset_queries, connection @pytest.mark.django_db @@ -46,12 +49,21 @@ def test_emit_without_saving(self, user_one, test_notification_type): ).exists() def test_emit_frequency_none(self, user_one, test_notification_type): + assert not Notification.objects.filter( + subscription__notification_type=test_notification_type, + fake_sent=True + ).exists() + time_before = timezone.now() test_notification_type.emit( user=user_one, event_context={'notifications': 'test template for Test notification'}, message_frequency='none' ) - assert Notification.objects.filter( + time_after = timezone.now() + notifications = Notification.objects.filter( subscription__notification_type=test_notification_type, fake_sent=True - ).exists() + ) + assert notifications.exists() + assert notifications.count() == 1 + assert time_before < notifications.first().sent < time_after From 09a570d6a5de7409ef742b65f56816015b2c2778 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Tue, 27 Jan 2026 16:05:33 -0500 Subject: [PATCH 10/11] Remove `seen` from `Notification` and re-make migrations --- osf/migrations/0036_notification_refactor_post_release.py | 6 +++++- osf/models/notification.py | 6 ------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/osf/migrations/0036_notification_refactor_post_release.py b/osf/migrations/0036_notification_refactor_post_release.py index fa9de320228..d65fe5b82bb 100644 --- a/osf/migrations/0036_notification_refactor_post_release.py +++ b/osf/migrations/0036_notification_refactor_post_release.py @@ -1,4 +1,4 @@ -# Generated by Django 4.2.15 on 2026-01-22 10:29 +# Generated by Django 4.2.17 on 2026-01-27 21:03 from django.db import migrations, models import osf.utils.fields @@ -12,6 +12,10 @@ class Migration(migrations.Migration): ] operations = [ + migrations.RemoveField( + model_name='notification', + name='seen', + ), migrations.AddField( model_name='notification', name='fake_sent', diff --git a/osf/models/notification.py b/osf/models/notification.py index d9174cad8ef..cf62bf297bd 100644 --- a/osf/models/notification.py +++ b/osf/models/notification.py @@ -16,7 +16,6 @@ class Notification(models.Model): ) event_context: dict = models.JSONField() sent = models.DateTimeField(null=True, blank=True) - seen = models.DateTimeField(null=True, blank=True) created = models.DateTimeField(auto_now_add=True) fake_sent = models.BooleanField(default=False) @@ -61,11 +60,6 @@ def mark_sent(self) -> None: self.sent = timezone.now() self.save(update_fields=['sent']) - def mark_seen(self) -> None: - raise NotImplementedError('mark_seen must be implemented by subclasses.') - # self.seen = timezone.now() - # self.save(update_fields=['seen']) - def render(self) -> str: """Render the notification message using the event context.""" notification_type = self.subscription.notification_type From c5fa8bca67f6f82fd031cb7d9222f3a1e9ece938 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Tue, 27 Jan 2026 16:25:59 -0500 Subject: [PATCH 11/11] `mark_sent()` now handles `fake_sent=True`, and only `save()` once --- notifications/tasks.py | 5 +---- osf/models/notification.py | 8 ++++++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/notifications/tasks.py b/notifications/tasks.py index fac028090f5..7298eb71246 100644 --- a/notifications/tasks.py +++ b/notifications/tasks.py @@ -34,11 +34,8 @@ def safe_render_notification(notifications, email_task): email_task.save() failed_notifications.append(notification.id) # Mark notifications that failed to render as fake sent - # Use 1000/12/31 to distinguish itself from another type of fake sent 1000/1/1 + notification.mark_sent(fake_sent=True) log_message(f'Error rendering notification, mark as fake sent: [notification_id={notification.id}]') - notification.mark_sent() - notification.fake_sent = True - notification.save() continue rendered_notifications.append(rendered) diff --git a/osf/models/notification.py b/osf/models/notification.py index cf62bf297bd..533a05a4e97 100644 --- a/osf/models/notification.py +++ b/osf/models/notification.py @@ -56,9 +56,13 @@ def send( if save: self.mark_sent() - def mark_sent(self) -> None: + def mark_sent(self, fake_sent=False) -> None: + update_fields = ['sent'] self.sent = timezone.now() - self.save(update_fields=['sent']) + if fake_sent: + update_fields.append('fake_sent') + self.fake_sent = True + self.save(update_fields=update_fields) def render(self) -> str: """Render the notification message using the event context."""