New notification system: implementation (#10922)

* Add some comments to understand the scope of the changes * Add more comments about the plan * Delete old code We are not sending any kind of notification from the Django admin. We used to use this in the past but we are not using it anymore. I'm deleting it now to avoid re-implementing it using the new notification system. * Add more comments to organize the work of new notification system * Notification: initial modelling * Define `max_length=128` that's required for SQLite (tests) * Initial implementation for build notifications API endpoint * API endpoint to create build notifications * Use APIv2 for internal endpoint to call from builders * Use APIv2 to create notifications from the builder * Check project slug when attaching to a Build/Project instance * Add extra TODO comments * Initial migration from Exceptions to Messages * Disable `Notification.format_values` for now I'll come back to this later. * Define all the notifications * Messages for symlink exceptions * Simplify how `NOTIFICATION_MESSAGES` is built * Add `Notification.format_values` field This allows to store values required for this notification at render time * Notification with `format_values` passed when exception is raised * Use `get_display_icon` to show the proper icon in the serializer * Proper relation name * Initial work to replace site notifications * We are not going to create Notifications via APIv3 for now * Send `format_values` to exception * Small changes and comments added * Initial work to migrate an old SiteNotification to new system * Create a notification `registry` to register messages by application * Use a custom `models.Manager` to implement de-duplication Use `Notification.objects.add()` to create a notification with de-duplication capabilities. If the notification already exists, it updates all the fields with the new ones and set the `modified` datetime to now. This commit also migrates the "Your primary email address is not verified" to the new notification system, which it was when I implemented the de-duplication logic. * Small refactor to email notifications * Render build/project notifications from Django templates Use a small hack on build detail's page to refresh the page once the build is finished. This way, we don't have to re-implement the new notification system in the old Knockout javascript code and we focus ourselves only in the new dashboard. * Attach "build.commands" in beta notification * Initial migration pattern for YAML config errors * Pass default settings for notifications * Organization disabled notification * Email not verified notification * Project skipped notification * Missing file for NotificationQuerySet * Migrate `oauth` app notifications to the new system * Add small comment about notification migration * Rename import * Fix queryset when creating a notification via `.add()` * Remove `SiteNotification` completely * Rename file and remove `__init__` magic * Remove `django-message-extends` completely We are not going to use it anymore with the new notification system. * Note about user notifications in base template * Remove commented code for debugging * Typo (from review) * Migrate `ConfigError` to the new notification system * Implement refact `icon` and `icon_type` into `icon_classes` * Remove unused setting * Update config file test cases to match changes * Mark test that depend on search to be skipped * Cancel build test fixed * Update APIv3 test cases * Down to a few of tests failing * Raise exception properly by passing `message_id` * More tests updated/fixed * BuildUser/BuildAdd error * Instantiate `BuildDirector` on `before_start` It makes more sense to be there than under `execute()`, since it's a pre-requirement. It also helps when testing. * Update test case to pass * Update more tests related to notifications * Make usage of `.add` for deduplication * Move the link to the message itself. * Create message IDs property * Use IDs to avoid notifications * Update comments * Attach organization on retry * Test we are hitting APIv2 for notifications * Remove old comment * Remove temporary debugging link * Remove old comment * Update send build status celery tests * Explain how to show notifications attached to users * Better comment in template * Lint * Minor changes * Refactor `ValidationError` to raise a proper exception/notification * Update tests for config after refactor * Pass `user_notifications` to templates * More updates on config tests * Remove debugging code * Add context processor for user notifications * Don't retrieve notifications if anonymous user * Use proper exception * Apply suggestions from code review Co-authored-by: Anthony <[email protected]> * Remove old TODO messages * Remove `deprecated_` utils and notifications They are not useful anymore since it's not possible to build without a config file or using a config file v1. * Use the proper HTML/CSS for user notifications * Message for deploy key (required in commercial) * Migrate Docker errors to new system * Migrate more messages to the new notifications system * Typo * Fix error type * Remove showing errors from `build.error` We are using the `Notification`s attached to the object now. These old errors are migrated to the new system by a one-time script. * Typo in copy * Revert "Remove showing errors from `build.error`" This reverts commit d4951a8. We decided to keep this code around for now. See #10980 (comment) * Comment explaining the decision about `Build.error` HTML+KO code * Use `textwrap.dedent` + `.strip()` for all messages Closes #10987 * Avoid breaking at rendering time If the message ID is not found we return a generic message to avoid breaking the request and we log an internal error to fix the issue. * Apply feedback from review --------- Co-authored-by: Anthony <[email protected]>
readthedocs · Jan 4, 2024 · e778db5 · e778db5
1 parent 9c8fe6f
commit e778db5
Show file tree

Hide file tree

Showing 92 changed files with 2,746 additions and 2,140 deletions.
diff --git a/common b/common
diff --git a/readthedocs/api/v2/serializers.py b/readthedocs/api/v2/serializers.py
@@ -2,11 +2,15 @@
 
 
 from allauth.socialaccount.models import SocialAccount
+from django.core.exceptions import ObjectDoesNotExist
+from django.utils.translation import gettext as _
+from generic_relations.relations import GenericRelatedField
 from rest_framework import serializers
 
 from readthedocs.api.v2.utils import normalize_build_command
 from readthedocs.builds.models import Build, BuildCommandResult, Version
 from readthedocs.core.resolver import Resolver
+from readthedocs.notifications.models import Notification
 from readthedocs.oauth.models import RemoteOrganization, RemoteRepository
 from readthedocs.projects.models import Domain, Project
 
@@ -378,3 +382,53 @@ def get_username(self, obj):
             or obj.extra_data.get("login")
             # FIXME: which one is GitLab?
         )
+
+
+class NotificationAttachedToRelatedField(serializers.RelatedField):
+
+    """
+    Attached to related field for Notifications.
+
+    Used together with ``rest-framework-generic-relations`` to accept multiple object types on ``attached_to``.
+
+    See https://github.com/LilyFoote/rest-framework-generic-relations
+    """
+
+    default_error_messages = {
+        "required": _("This field is required."),
+        "does_not_exist": _("Object does not exist."),
+        "incorrect_type": _(
+            "Incorrect type. Expected URL string, received {data_type}."
+        ),
+    }
+
+    def to_representation(self, value):
+        return f"{self.queryset.model._meta.model_name}/{value.pk}"
+
+    def to_internal_value(self, data):
+        # TODO: handle exceptions
+        model, pk = data.strip("/").split("/")
+        if self.queryset.model._meta.model_name != model:
+            self.fail("incorrect_type")
+
+        try:
+            return self.queryset.get(pk=pk)
+        except (ObjectDoesNotExist, ValueError, TypeError):
+            self.fail("does_not_exist")
+
+
+class NotificationSerializer(serializers.ModelSerializer):
+    # Accept different object types (Project, Build, User, etc) depending on what the notification is attached to.
+    # The client has to send a value like "<model>/<pk>".
+    # Example: "build/3522" will attach the notification to the Build object with id 3522
+    attached_to = GenericRelatedField(
+        {
+            Build: NotificationAttachedToRelatedField(queryset=Build.objects.all()),
+            Project: NotificationAttachedToRelatedField(queryset=Project.objects.all()),
+        },
+        required=True,
+    )
+
+    class Meta:
+        model = Notification
+        exclude = ["attached_to_id", "attached_to_content_type"]
diff --git a/readthedocs/api/v2/urls.py b/readthedocs/api/v2/urls.py
@@ -12,6 +12,7 @@
     BuildCommandViewSet,
     BuildViewSet,
     DomainViewSet,
+    NotificationViewSet,
     ProjectViewSet,
     RemoteOrganizationViewSet,
     RemoteRepositoryViewSet,
@@ -25,6 +26,7 @@
 router.register(r"version", VersionViewSet, basename="version")
 router.register(r"project", ProjectViewSet, basename="project")
 router.register(r"domain", DomainViewSet, basename="domain")
+router.register(r"notifications", NotificationViewSet, basename="notifications")
 router.register(
     r"remote/org",
     RemoteOrganizationViewSet,

diff --git a/readthedocs/api/v2/views/model_views.py b/readthedocs/api/v2/views/model_views.py
@@ -18,6 +18,7 @@
 from readthedocs.api.v2.utils import normalize_build_command
 from readthedocs.builds.constants import INTERNAL
 from readthedocs.builds.models import Build, BuildCommandResult, Version
+from readthedocs.notifications.models import Notification
 from readthedocs.oauth.models import RemoteOrganization, RemoteRepository
 from readthedocs.oauth.services import registry
 from readthedocs.projects.models import Domain, Project
@@ -29,6 +30,7 @@
     BuildCommandSerializer,
     BuildSerializer,
     DomainSerializer,
+    NotificationSerializer,
     ProjectAdminSerializer,
     ProjectSerializer,
     RemoteOrganizationSerializer,
@@ -361,6 +363,42 @@ def get_queryset_for_api_key(self, api_key):
         return self.model.objects.filter(build__project=api_key.project)
 
 
+class NotificationViewSet(DisableListEndpoint, CreateModelMixin, UserSelectViewSet):
+
+    """
+    Create a notification attached to an object (User, Project, Build, Organization).
+
+    This endpoint is currently used only internally by the builder.
+    Notifications are attached to `Build` objects only when using this endpoint.
+    This limitation will change in the future when re-implementing this on APIv3 if neeed.
+    """
+
+    parser_classes = [JSONParser, MultiPartParser]
+    permission_classes = [HasBuildAPIKey]
+    renderer_classes = (JSONRenderer,)
+    serializer_class = NotificationSerializer
+    model = Notification
+
+    def perform_create(self, serializer):
+        """Restrict creation to notifications attached to the project's builds from the api key."""
+        attached_to = serializer.validated_data["attached_to"]
+
+        build_api_key = self.request.build_api_key
+
+        project_slug = None
+        if isinstance(attached_to, Build):
+            project_slug = attached_to.project.slug
+        elif isinstance(attached_to, Project):
+            project_slug = attached_to.slug
+
+        # Limit the permissions to create a notification on this object only if the API key
+        # is attached to the related project
+        if not project_slug or build_api_key.project.slug != project_slug:
+            raise PermissionDenied()
+
+        return super().perform_create(serializer)
+
+
 class DomainViewSet(DisableListEndpoint, UserSelectViewSet):
     permission_classes = [ReadOnlyPermission]
     renderer_classes = (JSONRenderer,)

diff --git a/readthedocs/api/v3/serializers.py b/readthedocs/api/v3/serializers.py
@@ -15,6 +15,7 @@
 from readthedocs.core.resolver import Resolver
 from readthedocs.core.utils import slugify
 from readthedocs.core.utils.extend import SettingsOverrideObject
+from readthedocs.notifications.models import Notification
 from readthedocs.oauth.models import RemoteOrganization, RemoteRepository
 from readthedocs.organizations.models import Organization, Team
 from readthedocs.projects.constants import (
@@ -60,10 +61,33 @@ class Meta:
         fields = []
 
 
+# TODO: decide whether or not include a `_links` field on the object
+#
+# This also includes adding `/api/v3/notifications/<pk>` endpoint,
+# which I'm not sure it's useful at this point.
+#
+# class NotificationLinksSerializer(BaseLinksSerializer):
+#     _self = serializers.SerializerMethodField()
+#     attached_to = serializers.SerializerMethodField()
+
+#     def get__self(self, obj):
+#         path = reverse(
+#             "notifications-detail",
+#             kwargs={
+#                 "pk": obj.pk,
+#             },
+#         )
+#         return self._absolute_url(path)
+
+#     def get_attached_to(self, obj):
+#         return None
+
+
 class BuildLinksSerializer(BaseLinksSerializer):
     _self = serializers.SerializerMethodField()
     version = serializers.SerializerMethodField()
     project = serializers.SerializerMethodField()
+    notifications = serializers.SerializerMethodField()
 
     def get__self(self, obj):
         path = reverse(
@@ -96,6 +120,16 @@ def get_project(self, obj):
         )
         return self._absolute_url(path)
 
+    def get_notifications(self, obj):
+        path = reverse(
+            "project-builds-notifications-list",
+            kwargs={
+                "parent_lookup_project__slug": obj.project.slug,
+                "parent_lookup_build__id": obj.pk,
+            },
+        )
+        return self._absolute_url(path)
+
 
 class BuildURLsSerializer(BaseLinksSerializer, serializers.Serializer):
     build = serializers.URLField(source="get_full_url")
@@ -190,6 +224,57 @@ def get_success(self, obj):
         return None
 
 
+class NotificationMessageSerializer(serializers.Serializer):
+    id = serializers.SlugField()
+    header = serializers.CharField(source="get_rendered_header")
+    body = serializers.CharField(source="get_rendered_body")
+    type = serializers.CharField()
+    icon_classes = serializers.CharField(source="get_display_icon_classes")
+
+    class Meta:
+        fields = [
+            "id",
+            "header",
+            "body",
+            "type",
+            "icon_classes",
+        ]
+
+
+class NotificationCreateSerializer(serializers.ModelSerializer):
+    class Meta:
+        model = Notification
+        fields = [
+            "message_id",
+            "dismissable",
+            "news",
+            "state",
+        ]
+
+
+class NotificationSerializer(serializers.ModelSerializer):
+    message = NotificationMessageSerializer(source="get_message")
+    attached_to_content_type = serializers.SerializerMethodField()
+    # TODO: review these fields
+    # _links = BuildLinksSerializer(source="*")
+    # urls = BuildURLsSerializer(source="*")
+
+    class Meta:
+        model = Notification
+        fields = [
+            "id",
+            "state",
+            "dismissable",
+            "news",
+            "attached_to_content_type",
+            "attached_to_id",
+            "message",
+        ]
+
+    def get_attached_to_content_type(self, obj):
+        return obj.attached_to_content_type.name
+
+
 class VersionLinksSerializer(BaseLinksSerializer):
     _self = serializers.SerializerMethodField()
     builds = serializers.SerializerMethodField()

diff --git a/readthedocs/api/v3/tests/mixins.py b/readthedocs/api/v3/tests/mixins.py
@@ -26,6 +26,7 @@
 )
 class APIEndpointMixin(TestCase):
     fixtures = []
+    maxDiff = None  # So we get an actual diff when it fails
 
     def setUp(self):
         self.created = make_aware(datetime.datetime(2019, 4, 29, 10, 0, 0))

diff --git a/readthedocs/api/v3/tests/responses/projects-builds-detail.json b/readthedocs/api/v3/tests/responses/projects-builds-detail.json
@@ -8,7 +8,8 @@
     "_links": {
         "_self": "https://readthedocs.org/api/v3/projects/project/builds/1/",
         "project": "https://readthedocs.org/api/v3/projects/project/",
-        "version": "https://readthedocs.org/api/v3/projects/project/versions/v1.0/"
+        "version": "https://readthedocs.org/api/v3/projects/project/versions/v1.0/",
+        "notifications": "https://readthedocs.org/api/v3/projects/project/builds/1/notifications/"
     },
     "urls": {
         "build": "https://readthedocs.org/projects/project/builds/1/",

diff --git a/readthedocs/api/v3/tests/responses/projects-detail.json b/readthedocs/api/v3/tests/responses/projects-detail.json
@@ -23,6 +23,7 @@
                 "id": 1,
                 "_links": {
                     "_self": "https://readthedocs.org/api/v3/projects/project/builds/1/",
+                    "notifications": "https://readthedocs.org/api/v3/projects/project/builds/1/notifications/",
                     "project": "https://readthedocs.org/api/v3/projects/project/",
                     "version": "https://readthedocs.org/api/v3/projects/project/versions/v1.0/"
                 },

diff --git a/readthedocs/api/v3/tests/responses/projects-versions-builds-list_POST.json b/readthedocs/api/v3/tests/responses/projects-versions-builds-list_POST.json
@@ -8,6 +8,7 @@
         "id": 2,
         "_links": {
             "_self": "https://readthedocs.org/api/v3/projects/project/builds/2/",
+            "notifications": "https://readthedocs.org/api/v3/projects/project/builds/2/notifications/",
             "project": "https://readthedocs.org/api/v3/projects/project/",
             "version": "https://readthedocs.org/api/v3/projects/project/versions/v1.0/"
         },

diff --git a/readthedocs/api/v3/urls.py b/readthedocs/api/v3/urls.py
@@ -3,6 +3,7 @@
     BuildsCreateViewSet,
     BuildsViewSet,
     EnvironmentVariablesViewSet,
+    NotificationsViewSet,
     ProjectsViewSet,
     RedirectsViewSet,
     RemoteOrganizationViewSet,
@@ -12,7 +13,6 @@
     VersionsViewSet,
 )
 
-
 router = DefaultRouterWithNesting()
 
 # allows /api/v3/projects/
@@ -63,13 +63,28 @@
 
 # allows /api/v3/projects/pip/builds/
 # allows /api/v3/projects/pip/builds/1053/
-projects.register(
+builds = projects.register(
     r"builds",
     BuildsViewSet,
     basename="projects-builds",
     parents_query_lookups=["project__slug"],
 )
 
+# NOTE: we are only listing notifications on APIv3 for now.
+# The front-end will use this endpoint.
+# allows /api/v3/projects/pip/builds/1053/notifications/
+builds.register(
+    r"notifications",
+    NotificationsViewSet,
+    basename="project-builds-notifications",
+    parents_query_lookups=["project__slug", "build__id"],
+)
+
+# TODO: create an APIv3 endpoint to PATCH Build/Project notifications.
+# This way the front-end can mark them as READ/DISMISSED.
+#
+# TODO: create an APIv3 endpoint to list notifications for Projects.
+
 # allows /api/v3/projects/pip/redirects/
 # allows /api/v3/projects/pip/redirects/1053/
 projects.register(

diff --git a/readthedocs/api/v3/views.py b/readthedocs/api/v3/views.py
@@ -1,4 +1,5 @@
 import django_filters.rest_framework as filters
+from django.contrib.contenttypes.models import ContentType
 from django.db.models import Exists, OuterRef
 from rest_flex_fields import is_expanded
 from rest_flex_fields.views import FlexFieldsMixin
@@ -23,6 +24,7 @@
 from readthedocs.builds.models import Build, Version
 from readthedocs.core.utils import trigger_build
 from readthedocs.core.utils.extend import SettingsOverrideObject
+from readthedocs.notifications.models import Notification
 from readthedocs.oauth.models import (
     RemoteOrganization,
     RemoteRepository,
@@ -62,6 +64,7 @@
     BuildCreateSerializer,
     BuildSerializer,
     EnvironmentVariableSerializer,
+    NotificationSerializer,
     OrganizationSerializer,
     ProjectCreateSerializer,
     ProjectSerializer,
@@ -381,6 +384,32 @@ def create(self, request, **kwargs):  # pylint: disable=arguments-differ
         return Response(data=data, status=code)
 
 
+class NotificationsViewSet(
+    APIv3Settings,
+    NestedViewSetMixin,
+    ProjectQuerySetMixin,
+    FlexFieldsMixin,
+    ReadOnlyModelViewSet,
+):
+    model = Notification
+    lookup_field = "pk"
+    lookup_url_kwarg = "notification_pk"
+    serializer_class = NotificationSerializer
+    queryset = Notification.objects.all()
+    # filterset_class = BuildFilter
+
+    # http://chibisov.github.io/drf-extensions/docs/#usage-with-generic-relations
+    def get_queryset(self):
+        queryset = self.queryset.filter(
+            attached_to_content_type=ContentType.objects.get_for_model(Build)
+        )
+
+        # TODO: make sure if this particular filter should be applied here or somewhere else.
+        return queryset.filter(
+            attached_to_id=self.kwargs.get("parent_lookup_build__id")
+        )
+
+
 class RedirectsViewSet(
     APIv3Settings,
     NestedViewSetMixin,