Skip to content

[ ENG-9762] Improve moderator and user files tests to properly reflect digest code #254

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Johnetordoff wants to merge 8 commits into
base: feature/notification-refactor-p2-s
Choose a base branch
from
Open

[ ENG-9762] Improve moderator and user files tests to properly reflect digest code #254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f65db33
improve moderator and user files tests to properly reflect digest cod…
Johnetordoff Nov 13, 2025
51f427c
add config changes for testing reasons
Johnetordoff Nov 13, 2025
7975051
fix up tests and digest logic
Johnetordoff Nov 13, 2025
c71ee93
fix tests and digest code queueing
Johnetordoff Nov 13, 2025
ba7e6ae
fix more tests for notification digest
Johnetordoff Nov 13, 2025
c4de04c
fix another notification digest test
Johnetordoff Nov 13, 2025
a40430d
change status to error message to preserve RETRY status
Johnetordoff Nov 14, 2025
fd2e388
update code to work better with subscription detail and FE
Johnetordoff Nov 14, 2025
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
42 changes: 38 additions & 4 deletions api/subscriptions/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from api.nodes.serializers import RegistrationProviderRelationshipField
from api.collections_providers.fields import CollectionProviderRelationshipField
from api.preprints.serializers import PreprintProviderRelationshipField
from osf.models import NotificationType, NotificationSubscription
from website.util import api_v2_url


Expand Down Expand Up @@ -38,10 +39,43 @@ def get_absolute_url(self, obj):

def update(self, instance, validated_data):
freq = validated_data.get('message_frequency')
if freq is None:
freq = validated_data.get('frequency')
instance.message_frequency = freq
instance.save()
if instance.name in (
NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS,
NotificationType.Type.PROVIDER_NEW_PENDING_WITHDRAW_REQUESTS,
):
NotificationSubscription.get_or_create( # Legacy subscription keeps global settings
user=instance.user,
notification_type=NotificationType.Type.REVIEWS_SUBMISSION_STATUS.instance,
defaults={
'message_frequency': freq,
},
)
NotificationSubscription.objects.filter(
user=instance.user,
notification_type__in=[
NotificationType.Type.REVIEWS_SUBMISSION_STATUS.instance,
NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS.instance,
],
).update(frequency=freq)
elif instance.name == NotificationType.Type.USER_FILE_UPDATED:
NotificationSubscription.objects.filter(
user=instance.user,
notification_type__in=[
NotificationType.Type.USER_FILE_UPDATED,
NotificationType.Type.ADDON_FILE_RENAMED.instance,
NotificationType.Type.ADDON_FILE_COPIED.instance,
NotificationType.Type.FILE_ADDED.instance,
NotificationType.Type.ADDON_FILE_MOVED.instance,
NotificationType.Type.FILE_REMOVED.instance,
NotificationType.Type.FILE_UPDATED.instance,
NotificationType.Type.FOLDER_CREATED.instance,
],
).update(frequency=freq)
else:
if freq is None:
freq = validated_data.get('frequency')
instance.message_frequency = freq
instance.save()
return instance


Expand Down
93 changes: 52 additions & 41 deletions api_tests/notifications/test_notification_digest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,16 +113,14 @@ def test_send_moderator_email_task_registration_provider_admin(self):
user = AuthUserFactory(fullname='Admin User')
reg_provider = RegistrationProviderFactory(_id='abc123')
reg = RegistrationFactory(provider=reg_provider)
admin_group = reg_provider.get_group('admin')
admin_group.user_set.add(user)
notification_type = NotificationType.objects.get(name=NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS)
notification = Notification.objects.create(
subscription=add_notification_subscription(
user,
notification_type,
'daily',
subscribed_object=reg
),
reg_provider.add_to_group(user, 'admin')
reg_provider.add_to_group(user, 'moderator')
add_notification_subscription(
user,
NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS.instance,
'daily',
subscribed_object=reg
).emit(
event_context={
'profile_image_url': 'http://example.com/profile.png',
'is_request_email': False,
Expand All @@ -132,8 +130,8 @@ def test_send_moderator_email_task_registration_provider_admin(self):
'requester_fullname': '<NAME>',
'localized_timestamp': 'test timestamp',
},
sent=None,
)
notification = Notification.objects.get()
notification_ids = [notification.id]
with capture_notifications() as notifications:
send_moderator_email_task.apply(args=(user._id, notification_ids)).get()
Expand Down Expand Up @@ -172,28 +170,45 @@ def test_send_moderator_email_task_user_not_found(self):

def test_get_users_emails(self):
user = AuthUserFactory()
notification_type = NotificationType.objects.get(name=NotificationType.Type.USER_DIGEST)
notification1 = Notification.objects.create(
subscription=add_notification_subscription(user, notification_type, 'daily'),
sent=None,
event_context={},
add_notification_subscription(
user,
NotificationType.Type.FILE_UPDATED,
'daily'
).emit(
event_context={
'source_path': '/',
'requester_fullname': '<NAME>',
'source_node_title': 'test title',
'source_addon': 'test addon',
'destination_addon': 'what?',
'logo': 'test logo',
'requester_contributor_names': ['<NAME>'],
'action': 'test action',
'osf_logo': 'test logo',
'osf_logo_list': 'osf_logo_list',
'profile_image_url': 'http://example.com/profile.png',
'destination_node_parent_node_title': 'test parent node title',
'destination_node_title': 'test node title',
'nessage': 'test message',
'localized_timestamp': 'test timestamp',
},
)
res = list(get_users_emails('daily'))
assert len(res) == 1
user_info = res[0]
assert user_info['user_id'] == user._id
assert any(msg['notification_id'] == notification1.id for msg in user_info['info'])
notification = Notification.objects.get()
assert any(msg['notification_id'] == notification.id for msg in user_info['info'])

def test_get_moderators_emails(self):
user = AuthUserFactory()
provider = RegistrationProviderFactory()
reg = RegistrationFactory(provider=provider)
notification_type = NotificationType.objects.get(name=NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS)
subscription = add_notification_subscription(user, notification_type, 'daily', subscribed_object=reg)
Notification.objects.create(
subscription=subscription,
event_context={},
sent=None
subscription = add_notification_subscription(
user,
NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS.instance,
'daily',
subscribed_object=reg
)
res = list(get_moderators_emails('daily'))
assert len(res) >= 1
Expand All @@ -204,20 +219,11 @@ def test_get_moderators_emails(self):

def test_send_users_digest_email_end_to_end(self):
user = AuthUserFactory()
notification_type = NotificationType.objects.get(name=NotificationType.Type.USER_FILE_UPDATED)
subscription_type = add_notification_subscription(
add_notification_subscription(
user,
notification_type,
NotificationType.Type.FILE_UPDATED.instance,
'daily',
subscription=add_notification_subscription(
user,
NotificationType.objects.get(name=NotificationType.Type.FILE_UPDATED),
'daily'
)
)

Notification.objects.create(
subscription=subscription_type,
).emit(
event_context={
'source_path': '/',
'requester_fullname': '<NAME>',
Expand All @@ -232,13 +238,13 @@ def test_send_users_digest_email_end_to_end(self):
'profile_image_url': 'http://example.com/profile.png',
'destination_node_parent_node_title': 'test parent node title',
'destination_node_title': 'test node title',
'nessage': 'test message',
'message': 'test message',
'localized_timestamp': 'test timestamp',
},
)
user.save()
with capture_notifications() as notifications:
send_users_digest_email.delay()
send_users_digest_email.apply(kwargs={'dry_run': False}).get()
assert len(notifications['emits']) == 1
assert notifications['emits'][0]['type'] == NotificationType.Type.USER_DIGEST
email_task = EmailTask.objects.get(user_id=user.id)
Expand All @@ -248,10 +254,15 @@ def test_send_moderators_digest_email_end_to_end(self):
user = AuthUserFactory()
provider = RegistrationProviderFactory()
reg = RegistrationFactory(provider=provider)
notification_type = NotificationType.objects.get(name=NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS)
Notification.objects.create(
subscription=add_notification_subscription(user, notification_type, 'daily', subscribed_object=reg),
sent=None,
provider.add_to_group(user, 'admin')
provider.add_to_group(user, 'moderator')

add_notification_subscription(
user,
NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS.instance,
'daily',
subscribed_object=reg
).emit(
event_context={
'submitter_fullname': 'submitter_fullname',
'requester_fullname': 'requester_fullname',
Expand Down
28 changes: 20 additions & 8 deletions notifications/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ def send_user_email_task(self, user_id, notification_ids, **kwargs):
NotificationType.Type.USER_DIGEST.instance.emit(
user=user,
event_context=event_context,
message_frequency='instantly'
)

notifications_qs.update(sent=timezone.now())
Expand Down Expand Up @@ -118,21 +119,26 @@ def send_moderator_email_task(self, user_id, notification_ids, **kwargs):

if subscribed_object is None:
log_message(f"subscribed_object fpr {subscribed_object} does not exist")
email_task.status = 'OBJECT NOT FOUND'
email_task.error_message = 'subscribed object not found'
email_task.status = 'FAILURE'
email_task.save()
return

provider = getattr(subscribed_object, 'provider', None)
if provider is None:
log_message(f"subscribed_object fpr {subscribed_object} does not exist")
email_task.status = 'PROVIDER NOT FOUND'
log_message(f"provider for subscribed_object {subscribed_object} does not exist")
email_task.error_message = 'provider not found'
email_task.status = 'FAILURE'
email_task.save()

@Ostap-Zherebetskyi Ostap-Zherebetskyi Nov 14, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It looks like this is the place for the email_task.error_message

return

current_moderators = provider.get_group('moderator')
if current_moderators is None or not current_moderators.user_set.filter(id=user.id).exists():
log_message(f"User is not a moderator for provider {provider._id} - skipping email")
email_task.status = 'NOT MODERATOR'
email_task.error_message = 'NOT MODERATOR'
email_task.status = 'FAILURE'
email_task.save()
return

additional_context = {}
if isinstance(provider, RegistrationProvider):
Expand Down Expand Up @@ -177,6 +183,7 @@ def send_moderator_email_task(self, user_id, notification_ids, **kwargs):
user=user,
subscribed_object=subscribed_object,
event_context=event_context,
message_frequency='instantly'
)

notifications_qs.update(sent=timezone.now())
Expand Down Expand Up @@ -254,7 +261,7 @@ def get_moderators_emails(message_freq: str):
)
GROUP BY osf_guid._id, (n.event_context ->> 'provider_id')
ORDER BY osf_guid._id ASC
"""
"""

with connection.cursor() as cursor:
cursor.execute(sql,
Expand Down Expand Up @@ -287,7 +294,7 @@ def get_users_emails(message_freq):
LEFT JOIN osf_guid ON ns.user_id = osf_guid.object_id
WHERE n.sent IS NULL
AND ns.message_frequency = %s
AND nt.name NOT IN (%s, %s)
AND nt.name IN (%s, %s, %s, %s, %s, %s, %s)
AND osf_guid.content_type_id = (
SELECT id FROM django_content_type WHERE model = 'osfuser'
)
Expand All @@ -299,8 +306,13 @@ def get_users_emails(message_freq):
cursor.execute(sql,
[
message_freq,
NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS.value,
NotificationType.Type.PROVIDER_NEW_PENDING_WITHDRAW_REQUESTS.value
NotificationType.Type.ADDON_FILE_RENAMED,
NotificationType.Type.ADDON_FILE_COPIED,
NotificationType.Type.FILE_ADDED,
NotificationType.Type.ADDON_FILE_MOVED,
NotificationType.Type.FILE_REMOVED,
NotificationType.Type.FILE_UPDATED,
NotificationType.Type.FOLDER_CREATED,
]
)
return itertools.chain.from_iterable(cursor.fetchall())
Expand Down
Loading
Loading