diff --git a/api/providers/serializers.py b/api/providers/serializers.py index 81ba5f74b5c..ffc9097e2e3 100644 --- a/api/providers/serializers.py +++ b/api/providers/serializers.py @@ -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) 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 0bf894be856..17baef516c9 100644 --- a/notifications/tasks.py +++ b/notifications/tasks.py @@ -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 @@ -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 @@ -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 @@ -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 = {} @@ -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 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/management/commands/remove_duplicate_notification_subscriptions.py b/osf/management/commands/remove_duplicate_notification_subscriptions.py new file mode 100644 index 00000000000..1897ec65efe --- /dev/null +++ b/osf/management/commands/remove_duplicate_notification_subscriptions.py @@ -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.") + ) 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')}, + ), + ] 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/mixins.py b/osf/models/mixins.py index 035c0430b7c..2604b7d32cb 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -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 ) 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/scripts/populate_notification_subscriptions.py b/scripts/populate_notification_subscriptions.py index f9da312f6e4..557b9f2a47d 100644 --- a/scripts/populate_notification_subscriptions.py +++ b/scripts/populate_notification_subscriptions.py @@ -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 @@ -96,6 +97,7 @@ def populate_notification_subscriptions(): content_type=node_ct, object_id=node.id, defaults={ + '_is_digest': True, 'message_frequency': 'none', }, )