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 84a825088f2..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 @@ -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 @@ -102,9 +103,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 +125,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 +179,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 @@ -211,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 = {} @@ -274,10 +277,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/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/migrations/0036_notification_fake_sent_and_more.py b/osf/migrations/0036_notification_fake_sent_and_more.py new file mode 100644 index 00000000000..fa9de320228 --- /dev/null +++ b/osf/migrations/0036_notification_fake_sent_and_more.py @@ -0,0 +1,34 @@ +# 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): + + 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.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', + 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')}, + ), + ] diff --git a/osf/models/email_task.py b/osf/models/email_task.py index f89f2285e5c..12def4c8c12 100644 --- a/osf/models/email_task.py +++ b/osf/models/email_task.py @@ -9,6 +9,8 @@ class EmailTask(models.Model): ('SUCCESS', 'Success'), ('FAILURE', 'Failure'), ('RETRY', 'Retry'), + ('PARTIAL_SUCCESS', 'Partial Success'), + ('AUTO_FIXED', 'Auto Fixed'), ) task_id = models.CharField(max_length=255, unique=True) 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..1e38f4cbdff 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 @@ -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, @@ -126,7 +127,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 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