From a0d9b1216ae142eb518a05f66ce659bf0f85cca7 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 24 Jan 2024 10:44:58 +0100 Subject: [PATCH] Notification: cancel notifications automatically (#11048) * Notification: cancel notifications automatically Closes #10981 * Notifications: cancel `Project.skip` and `Organization.disabled` Attach Django signals to `Organization` and `Project` to handle these two cases. * Notifications: move check from each request to signal * Notifications: use Django signals for `user_email_verified` --- .../tests/responses/notifications-list.json | 32 ++-- .../api/v3/tests/test_notifications.py | 29 ++++ readthedocs/core/context_processors.py | 7 +- readthedocs/notifications/apps.py | 3 + readthedocs/notifications/querysets.py | 40 ++++- readthedocs/notifications/signals.py | 68 ++++++++ .../notifications/tests/test_querysets.py | 151 ++++++++++++++++++ .../notifications/tests/test_signals.py | 91 +++++++++++ 8 files changed, 396 insertions(+), 25 deletions(-) create mode 100644 readthedocs/notifications/signals.py create mode 100644 readthedocs/notifications/tests/test_querysets.py create mode 100644 readthedocs/notifications/tests/test_signals.py diff --git a/readthedocs/api/v3/tests/responses/notifications-list.json b/readthedocs/api/v3/tests/responses/notifications-list.json index 4e977849d7d..331a66e81eb 100644 --- a/readthedocs/api/v3/tests/responses/notifications-list.json +++ b/readthedocs/api/v3/tests/responses/notifications-list.json @@ -5,18 +5,18 @@ "results": [ { "_links": { - "_self": "https://readthedocs.org/api/v3/users/testuser/notifications/4/" + "_self": "https://readthedocs.org/api/v3/organizations/organization/notifications/1/" }, - "attached_to_content_type": "user", + "attached_to_content_type": "organization", "attached_to_id": 1, "dismissable": false, - "id": 4, + "id": 1, "message": { - "body": "Your primary email address is not verified.\nPlease verify it here.", - "header": "Email address not verified", - "icon_classes": "fas fa-circle-exclamation", - "id": "core:email:validation-pending", - "type": "warning" + "body": "The organization \"organization\" is currently disabled. You need to renew your subscription to keep using Read the Docs", + "header": "Your organization has been disabled", + "icon_classes": "fas fa-circle-info", + "id": "organization:disabled", + "type": "info" }, "news": false, "state": "unread" @@ -41,18 +41,18 @@ }, { "_links": { - "_self": "https://readthedocs.org/api/v3/organizations/organization/notifications/1/" + "_self": "https://readthedocs.org/api/v3/users/testuser/notifications/4/" }, - "attached_to_content_type": "organization", + "attached_to_content_type": "user", "attached_to_id": 1, "dismissable": false, - "id": 1, + "id": 4, "message": { - "body": "The organization \"organization\" is currently disabled. You need to renew your subscription to keep using Read the Docs", - "header": "Your organization has been disabled", - "icon_classes": "fas fa-circle-info", - "id": "organization:disabled", - "type": "info" + "body": "Your primary email address is not verified.\nPlease verify it here.", + "header": "Email address not verified", + "icon_classes": "fas fa-circle-exclamation", + "id": "core:email:validation-pending", + "type": "warning" }, "news": false, "state": "unread" diff --git a/readthedocs/api/v3/tests/test_notifications.py b/readthedocs/api/v3/tests/test_notifications.py index 1224fca198b..045a777998e 100644 --- a/readthedocs/api/v3/tests/test_notifications.py +++ b/readthedocs/api/v3/tests/test_notifications.py @@ -1,6 +1,11 @@ +import django_dynamic_fixture as fixture +from django.contrib.contenttypes.models import ContentType from django.test import override_settings from django.urls import reverse +from readthedocs.notifications.constants import CANCELLED, DISMISSED +from readthedocs.notifications.models import Notification +from readthedocs.projects.notifications import MESSAGE_PROJECT_SKIP_BUILDS from readthedocs.subscriptions.constants import TYPE_CONCURRENT_BUILDS from readthedocs.subscriptions.products import RTDProductFeature @@ -30,6 +35,30 @@ def test_notifications_list(self): self._get_response_dict("notifications-list"), ) + # Adding a CANCELLED/DISMISSED notification won't be returned on this endpoint + fixture.get( + Notification, + attached_to_content_type=ContentType.objects.get_for_model(self.project), + attached_to_id=self.project.id, + message_id=MESSAGE_PROJECT_SKIP_BUILDS, + state=CANCELLED, + ) + + fixture.get( + Notification, + attached_to_content_type=ContentType.objects.get_for_model(self.project), + attached_to_id=self.project.id, + message_id=MESSAGE_PROJECT_SKIP_BUILDS, + state=DISMISSED, + ) + + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertDictEqual( + response.json(), + self._get_response_dict("notifications-list"), + ) + def test_notifications_list_post(self): url = reverse("notifications-list") diff --git a/readthedocs/core/context_processors.py b/readthedocs/core/context_processors.py index 99b75fccaf2..66223eadaf0 100644 --- a/readthedocs/core/context_processors.py +++ b/readthedocs/core/context_processors.py @@ -1,7 +1,6 @@ """Template context processors for core app.""" from django.conf import settings -from django.contrib.contenttypes.models import ContentType def readthedocs_processor(request): @@ -45,11 +44,7 @@ def user_notifications(request): user_notifications = Notification.objects.none() if request.user.is_authenticated: - content_type = ContentType.objects.get_for_model(request.user) - user_notifications = Notification.objects.filter( - attached_to_content_type=content_type, - attached_to_id=request.user.pk, - ) + user_notifications = Notification.objects.for_user(request.user) return { "user_notifications": user_notifications, diff --git a/readthedocs/notifications/apps.py b/readthedocs/notifications/apps.py index 7a98efee250..50c2fc24b86 100644 --- a/readthedocs/notifications/apps.py +++ b/readthedocs/notifications/apps.py @@ -5,3 +5,6 @@ class NotificationsAppConfig(AppConfig): name = "readthedocs.notifications" verbose_name = "Notifications" + + def ready(self): + import readthedocs.notifications.signals # noqa diff --git a/readthedocs/notifications/querysets.py b/readthedocs/notifications/querysets.py index 3c569427537..8c939be5c1b 100644 --- a/readthedocs/notifications/querysets.py +++ b/readthedocs/notifications/querysets.py @@ -5,6 +5,8 @@ from readthedocs.core.permissions import AdminPermission +from .constants import CANCELLED, READ, UNREAD + class NotificationQuerySet(models.QuerySet): def add(self, *args, **kwargs): @@ -12,7 +14,7 @@ def add(self, *args, **kwargs): Create a notification without duplicating it. If a notification with the same ``message_id`` is already attached to the object, - its ``modified_at`` timestamp is updated. + its ``modified_at`` timestamp and ``state`` is updated. Otherwise, a new notification object is created for this object. """ @@ -25,17 +27,45 @@ def add(self, *args, **kwargs): attached_to_content_type=content_type, attached_to_id=attached_to.id, message_id=message_id, + # Update only ``READ`` and ``UNREAD`` notifications because we want + # to keep track of ``DISMISSED`` and ``CANCELLED`` ones. + state__in=(UNREAD, READ), ).first() if notification: self.filter(pk=notification.pk).update( - *args, modified=timezone.now(), **kwargs + *args, + modified=timezone.now(), + state=UNREAD, + **kwargs, ) notification.refresh_from_db() return notification return super().create(*args, attached_to=attached_to, **kwargs) + def cancel(self, message_id, attached_to): + """ + Cancel an on-going notification because the underlying state has changed. + + When a notification is not valid anymore because the user has made the + required action (e.g. paid an unpaid subscription) we use this method + to mark those notifications as ``CANCELLED``. + + It only cancels notifications that are ``UNREAD`` or ``READ``. + """ + content_type = ContentType.objects.get_for_model(attached_to) + + self.filter( + attached_to_content_type=content_type, + attached_to_id=attached_to.id, + message_id=message_id, + state__in=(UNREAD, READ), + ).update( + state=CANCELLED, + modified=timezone.now(), + ) + def for_user(self, user): """ Retrieve all notifications related to a particular user. @@ -45,6 +75,8 @@ def for_user(self, user): - are attached to an ``Organization`` where the user is owner - are attached to a ``Project`` where the user is admin - are attacehd to the ``User`` themselves + + It only returns notifications that are ``READ`` or ``UNREAD``. """ # Need to be here due to circular imports from readthedocs.organizations.models import Organization @@ -76,4 +108,6 @@ def for_user(self, user): # Return all the notifications related to this user attached to: # User, Project and Organization models where the user is admin. - return user_notifications | project_notifications | organization_notifications + return ( + user_notifications | project_notifications | organization_notifications + ).filter(state__in=(UNREAD, READ)) diff --git a/readthedocs/notifications/signals.py b/readthedocs/notifications/signals.py new file mode 100644 index 00000000000..313b9cfd90b --- /dev/null +++ b/readthedocs/notifications/signals.py @@ -0,0 +1,68 @@ +"""Custom Django signals related to notifications.""" + +import structlog +from allauth.account.models import EmailAddress +from django.db.models.signals import post_save +from django.dispatch import receiver +from django.urls import reverse + +from readthedocs.core.notifications import MESSAGE_EMAIL_VALIDATION_PENDING +from readthedocs.notifications.models import Notification +from readthedocs.organizations.models import Organization +from readthedocs.projects.models import Project +from readthedocs.projects.notifications import MESSAGE_PROJECT_SKIP_BUILDS +from readthedocs.subscriptions.notifications import MESSAGE_ORGANIZATION_DISABLED + +log = structlog.get_logger(__name__) + + +@receiver(post_save, sender=Project) +def project_skip_builds(instance, *args, **kwargs): + """Check if the project is ``skip`` and add/cancel the notification.""" + if instance.skip: + Notification.objects.add( + message_id=MESSAGE_PROJECT_SKIP_BUILDS, + attached_to=instance, + dismissable=False, + ) + else: + Notification.objects.cancel( + message_id=MESSAGE_PROJECT_SKIP_BUILDS, + attached_to=instance, + ) + + +@receiver(post_save, sender=Organization) +def organization_disabled(instance, *args, **kwargs): + """Check if the organization is ``disabled`` and add/cancel the notification.""" + if instance.disabled: + Notification.objects.add( + message_id=MESSAGE_ORGANIZATION_DISABLED, + attached_to=instance, + dismissable=False, + ) + else: + Notification.objects.cancel( + message_id=MESSAGE_ORGANIZATION_DISABLED, + attached_to=instance, + ) + + +@receiver(post_save, sender=EmailAddress) +def user_email_verified(instance, *args, **kwargs): + """Check if the primary email is validated and cancel the notification.""" + if instance.primary: + if instance.verified: + Notification.objects.cancel( + attached_to=instance.user, + message_id=MESSAGE_EMAIL_VALIDATION_PENDING, + ) + else: + Notification.objects.add( + attached_to=instance.user, + message_id=MESSAGE_EMAIL_VALIDATION_PENDING, + dismissable=True, + format_values={ + "account_email_url": reverse("account_email"), + }, + ) diff --git a/readthedocs/notifications/tests/test_querysets.py b/readthedocs/notifications/tests/test_querysets.py new file mode 100644 index 00000000000..1b5e0265984 --- /dev/null +++ b/readthedocs/notifications/tests/test_querysets.py @@ -0,0 +1,151 @@ +import django_dynamic_fixture as fixture +import pytest +from django.contrib.auth.models import User +from django.contrib.contenttypes.models import ContentType + +from readthedocs.core.notifications import MESSAGE_EMAIL_VALIDATION_PENDING +from readthedocs.notifications.constants import CANCELLED, DISMISSED, READ, UNREAD +from readthedocs.notifications.models import Notification + + +@pytest.mark.django_db +class TestNotificationQuerySet: + def test_add(self): + user = fixture.get( + User, + ) + + # There is any notification attached to this user + assert ( + Notification.objects.filter( + attached_to_content_type=ContentType.objects.get_for_model(User), + attached_to_id=user.id, + ).count() + == 0 + ) + + Notification.objects.add( + attached_to=user, + message_id=MESSAGE_EMAIL_VALIDATION_PENDING, + ) + + # There is 1 notification attached to this user + assert ( + Notification.objects.filter( + attached_to_content_type=ContentType.objects.get_for_model(User), + attached_to_id=user.id, + ).count() + == 1 + ) + + old_notification = Notification.objects.first() + old_notification.state = READ + old_notification.save() + + # Add the same notification again + Notification.objects.add( + attached_to=user, + message_id=MESSAGE_EMAIL_VALIDATION_PENDING, + ) + + # Notification is not duplicated, but timestamp and state is updated + assert ( + Notification.objects.filter( + attached_to_content_type=ContentType.objects.get_for_model(User), + attached_to_id=user.id, + ).count() + == 1 + ) + + new_notification = Notification.objects.first() + assert old_notification.pk == new_notification.pk + assert old_notification.modified < new_notification.modified + assert old_notification.state == READ + assert new_notification.state == UNREAD + + # Add another notification + Notification.objects.add( + attached_to=user, + message_id="user:another:notification", + ) + + # Notification is added + assert ( + Notification.objects.filter( + attached_to_content_type=ContentType.objects.get_for_model(User), + attached_to_id=user.id, + ).count() + == 2 + ) + + def test_cancel(self): + user = fixture.get(User) + + Notification.objects.add( + attached_to=user, + message_id=MESSAGE_EMAIL_VALIDATION_PENDING, + ) + + # There is one UNREAD notification attached to this user + assert ( + Notification.objects.filter( + attached_to_content_type=ContentType.objects.get_for_model(User), + attached_to_id=user.id, + state=UNREAD, + ).count() + == 1 + ) + + Notification.objects.cancel( + attached_to=user, + message_id=MESSAGE_EMAIL_VALIDATION_PENDING, + ) + + # There is none UNREAD notification attached to this user + assert ( + Notification.objects.filter( + attached_to_content_type=ContentType.objects.get_for_model(User), + attached_to_id=user.id, + state=UNREAD, + ).count() + == 0 + ) + + # There is one CANCELLED notification attached to this user + assert ( + Notification.objects.filter( + attached_to_content_type=ContentType.objects.get_for_model(User), + attached_to_id=user.id, + state=CANCELLED, + ).count() + == 1 + ) + + def test_for_user(self): + user = fixture.get(User) + + Notification.objects.add( + attached_to=user, + message_id="user:notification:read", + state=READ, + ) + Notification.objects.add( + attached_to=user, + message_id="user:notification:unread", + state=UNREAD, + ) + Notification.objects.add( + attached_to=user, + message_id="user:notification:dismissed", + state=DISMISSED, + ) + Notification.objects.add( + attached_to=user, + message_id="user:notification:cancelled", + state=CANCELLED, + ) + + assert [n.message_id for n in Notification.objects.for_user(user)] == [ + "user:notification:read", + "user:notification:unread", + ] diff --git a/readthedocs/notifications/tests/test_signals.py b/readthedocs/notifications/tests/test_signals.py new file mode 100644 index 00000000000..52df285e8cf --- /dev/null +++ b/readthedocs/notifications/tests/test_signals.py @@ -0,0 +1,91 @@ +import django_dynamic_fixture as fixture +from allauth.account.models import EmailAddress +from django.contrib.auth.models import User +from django.contrib.contenttypes.models import ContentType +from django.test import TestCase + +from readthedocs.core.notifications import MESSAGE_EMAIL_VALIDATION_PENDING +from readthedocs.notifications.constants import CANCELLED, UNREAD +from readthedocs.notifications.models import Notification +from readthedocs.organizations.models import Organization +from readthedocs.projects.models import Project +from readthedocs.projects.notifications import MESSAGE_PROJECT_SKIP_BUILDS +from readthedocs.subscriptions.notifications import MESSAGE_ORGANIZATION_DISABLED + + +class TestSignals(TestCase): + def test_project_skip_builds(self): + project = fixture.get(Project) + + self.assertEqual(project.notifications.count(), 0) + project.skip = True + project.save() + + self.assertEqual(project.notifications.count(), 1) + + notification = project.notifications.first() + self.assertEqual(notification.message_id, MESSAGE_PROJECT_SKIP_BUILDS) + self.assertEqual(notification.state, UNREAD) + + project.skip = False + project.save() + + notification.refresh_from_db() + self.assertEqual(project.notifications.count(), 1) + self.assertEqual(notification.message_id, MESSAGE_PROJECT_SKIP_BUILDS) + self.assertEqual(notification.state, CANCELLED) + + def test_organization_disabled(self): + organization = fixture.get(Organization) + + self.assertEqual(organization.notifications.count(), 0) + organization.disabled = True + organization.save() + + self.assertEqual(organization.notifications.count(), 1) + + notification = organization.notifications.first() + self.assertEqual(notification.message_id, MESSAGE_ORGANIZATION_DISABLED) + self.assertEqual(notification.state, UNREAD) + + organization.disabled = False + organization.save() + + notification.refresh_from_db() + self.assertEqual(organization.notifications.count(), 1) + self.assertEqual(notification.message_id, MESSAGE_ORGANIZATION_DISABLED) + self.assertEqual(notification.state, CANCELLED) + + def test_user_email_verified(self): + user = fixture.get(User) + emailaddress = fixture.get( + EmailAddress, + user=user, + primary=True, + verified=False, + ) + + self.assertEqual(Notification.objects.for_user(user).count(), 1) + + notification = Notification.objects.for_user(user).first() + self.assertEqual(notification.message_id, MESSAGE_EMAIL_VALIDATION_PENDING) + self.assertEqual(notification.state, UNREAD) + + emailaddress.verified = True + emailaddress.save() + + notification.refresh_from_db() + + # 0 notifications to show to the user (none UNREAD) + self.assertEqual(Notification.objects.for_user(user).count(), 0) + + # 1 notification exists attached to this user, tho + self.assertEqual( + Notification.objects.filter( + attached_to_content_type=ContentType.objects.get_for_model(user), + attached_to_id=user.id, + ).count(), + 1, + ) + self.assertEqual(notification.message_id, MESSAGE_EMAIL_VALIDATION_PENDING) + self.assertEqual(notification.state, CANCELLED)