Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions api_tests/notifications/test_notifications_db_transaction.py
Original file line number Diff line number Diff line change
@@ -1,12 +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 datetime import datetime
from osf.models import Notification, NotificationType, NotificationSubscription
from tests.utils import capture_notifications
from django.db import reset_queries, connection


@pytest.mark.django_db
Expand Down Expand Up @@ -47,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,
sent=datetime(1000, 1, 1)
).exists()
fake_sent=True
)
assert notifications.exists()
assert notifications.count() == 1
assert time_before < notifications.first().sent < time_after
24 changes: 12 additions & 12 deletions notifications/tasks.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -34,10 +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.sent = datetime(1000, 12, 31)
notification.save()
continue

rendered_notifications.append(rendered)
Expand Down Expand Up @@ -102,9 +100,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

Expand All @@ -123,10 +122,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
Expand Down Expand Up @@ -177,9 +176,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

Expand Down Expand Up @@ -211,10 +211,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 = {}
Expand Down Expand Up @@ -274,10 +274,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
Expand Down
2 changes: 1 addition & 1 deletion osf/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
38 changes: 38 additions & 0 deletions osf/migrations/0036_notification_refactor_post_release.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
# Generated by Django 4.2.17 on 2026-01-27 21:03

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.RemoveField(
model_name='notification',
name='seen',
),
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')},
),
]
2 changes: 2 additions & 0 deletions osf/models/email_task.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
15 changes: 7 additions & 8 deletions osf/models/notification.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ 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)

def send(
self,
Expand Down Expand Up @@ -56,14 +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'])

def mark_seen(self) -> None:
raise NotImplementedError('mark_seen must be implemented by subclasses.')
# self.seen = timezone.now()
# self.save(update_fields=['seen'])
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."""
Expand Down
6 changes: 4 additions & 2 deletions osf/models/notification_subscription.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions osf/models/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
20 changes: 10 additions & 10 deletions scripts/triggered_mails.py
Original file line number Diff line number Diff line change
Expand Up @@ -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) |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the new check only checks the user field, we need a script to migrate all sent ones to the user model before we clean them up.

Q(no_login_email_last_sent__lt=now - settings.NO_LOGIN_WAIT_TIME)
)


@celery_app.task(name='scripts.triggered_no_login_email')
Expand Down Expand Up @@ -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')
Expand Down
12 changes: 3 additions & 9 deletions tests/test_triggered_mails.py
Original file line number Diff line number Diff line change
Expand Up @@ -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():
Expand Down Expand Up @@ -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
Expand Down