Skip to content
Open
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
2 changes: 1 addition & 1 deletion api/providers/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,7 @@ def update(self, instance, validated_data):
return instance

try:
provider.remove_from_group(instance, instance.permission_group, unsubscribe=False)
provider.remove_from_group(instance, instance.permission_group, unsubscribe=True)
except ValueError as e:
raise ValidationError(str(e))
provider.add_to_group(instance, perm_group)
Expand Down
3 changes: 1 addition & 2 deletions api_tests/notifications/test_notifications_db_transaction.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
19 changes: 11 additions & 8 deletions notifications/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,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 @@ -118,10 +119,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 @@ -172,9 +173,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 @@ -205,10 +207,11 @@ def send_moderator_email_task(self, user_id, notification_ids, provider_content_
if current_moderators is None or not current_moderators.user_set.filter(id=user.id).exists():
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} - skipping email")
email_task.status = 'FAILURE'
log_message(f"User is not a moderator for provider {provider._id} - notifications will not be sent.")
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=timezone.now(), fake_sent=True)
return

additional_context = {}
Expand Down Expand Up @@ -267,10 +270,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
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
from django.core.management.base import BaseCommand
from django.db import transaction
from django.db.models import OuterRef, Exists

from osf.models import NotificationSubscription


class Command(BaseCommand):
help = (
'Remove duplicate NotificationSubscription records, keeping only '
'the highest-id record per (user, content_type, object_id, notification_type).'
)

def add_arguments(self, parser):
parser.add_argument(
'--dry',
action='store_true',
help='Show how many rows would be deleted without deleting anything.',
)

def handle(self, *args, **options):
self.stdout.write('Finding duplicate NotificationSubscription records…')

to_remove = NotificationSubscription.objects.filter(
Exists(
NotificationSubscription.objects.filter(
user_id=OuterRef('user_id'),
content_type_id=OuterRef('content_type_id'),
object_id=OuterRef('object_id'),
notification_type_id=OuterRef('notification_type_id'),
_is_digest=OuterRef('_is_digest'),
id__gt=OuterRef('id'), # keep most recent record
)
)
)

count = to_remove.count()
self.stdout.write(f"Duplicates to remove: {count}")

if options['dry']:
self.stdout.write(
self.style.WARNING('Dry run enabled — no records were deleted.')
)
return

if count == 0:
self.stdout.write(self.style.SUCCESS('No duplicates found.'))
return

with transaction.atomic():
deleted, _ = to_remove.delete()

self.stdout.write(
self.style.SUCCESS(f"Successfully removed {deleted} duplicate records.")
)
Original file line number Diff line number Diff line change
@@ -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')},
),
]
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
1 change: 1 addition & 0 deletions osf/models/mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -1087,6 +1087,7 @@ def add_to_group(self, user, group):
content_type=ContentType.objects.get_for_model(self, for_concrete_model=False),
object_id=self.id,
notification_type=subscription.instance,
message_frequency='instantly',
_is_digest=True
)

Expand Down
1 change: 1 addition & 0 deletions osf/models/notification.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
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
6 changes: 4 additions & 2 deletions scripts/populate_notification_subscriptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,9 @@ def populate_notification_subscriptions():
content_type=user_ct,
object_id=user.id,
defaults={
'message_frequency': 'none',
},
'_is_digest': True,
'message_frequency': 'none',
},
)
if is_created:
created += 1
Expand All @@ -96,6 +97,7 @@ def populate_notification_subscriptions():
content_type=node_ct,
object_id=node.id,
defaults={
'_is_digest': True,
'message_frequency': 'none',
},
)
Expand Down
Loading