From dac2582fe447b998eaea324243146bc9f70862b3 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 6 Nov 2023 12:39:57 +0100 Subject: [PATCH 01/14] Design doc: new notification system Initial design document to discuss the implementation of a new notification system. --- docs/dev/design/new-notifications-system.rst | 445 +++++++++++++++++++ 1 file changed, 445 insertions(+) create mode 100644 docs/dev/design/new-notifications-system.rst diff --git a/docs/dev/design/new-notifications-system.rst b/docs/dev/design/new-notifications-system.rst new file mode 100644 index 00000000000..c970de7a9af --- /dev/null +++ b/docs/dev/design/new-notifications-system.rst @@ -0,0 +1,445 @@ +Notification system: a new approach after lot of discussions +============================================================ + +Notifications have been a recurrent topic in the last years. +We have talked about different problems and solution's approaches during these years. +However, due to the complexity of the change, and without having a clear path, it has been hard to prioritize. + +We've written a lot about the problems and potential solutions for the current notification system. +This is a non-complete list of them just for reference: + +* https://github.com/readthedocs/readthedocs.org/issues/4226 +* https://github.com/readthedocs/readthedocs.org/issues/3399 +* https://github.com/readthedocs/meta/discussions/126 +* https://github.com/readthedocs/readthedocs.org/issues/5909 +* https://github.com/readthedocs/readthedocs.org/issues/9110 +* https://github.com/readthedocs/readthedocs.org/issues/9279 +* https://github.com/readthedocs/readthedocs.org/issues/4235 + +At the offsite in Portland, Anthony and myself were able to talk deeply about this and wrote a bunch of thoughts in a Google Doc. +We had pretty similar ideas and we thought we were solving most of the problems we identified already. + +I read all of these issues and all the discussions I found and wrote this document that summarizes my proposal: +*create a new notification system that we can customize and expand as we need in the future*: + +* A Django model to store the notifications' data +* API endpoints to retrieve the notifications for a particular resource (User, Build, Project, Organization) +* Frontend code to display them (*outside the scope of this document*) + + +Goals +----- + +* Keep raising exceptions for errors from the build process +* Ability to add non-error notifications from the build process +* Add extra metadata associated to the notification: icon, color, header, body, etc +* Support different types of notifications (e.g. error, warning, note, tip) +* Re-use the new notification system for product updates (e.g. new features, deprecated config keys) +* Message content lives on Python classes that can be translated and formatted with objects (e.g. Build, Project) +* Message could have richer content (e.g. HTML code) to generate links and emphasis +* Notifications have trackable state (e.g. unread (default)=never shown, read=shown, dismissed=don't show again) +* An object (e.g. Build, Organization) can have more than 1 notification attached +* Remove hardcoded notifications from the templates +* Notifications can be attached to Project, Organization, Build and User models +* Specific notifications can be shown under the user's bell icon +* Easy way to cleanup notification on status changes (e.g. subscription failure notification is auto-deleted after CC updated) +* Notifications attached to Organization/Project dissappear for all the users once they are dismissed by anyone + + +Non-goals +--------- + +* Create new Build "state" or "status" +* Implement the new notification in the old dashboard +* Define front-end code implementation + + +Small notes and other considerations +------------------------------------ + +* Django message system is not enough for this purpose. +* Use a new model to store all the required data (expandable in the future) +* How do we handle translations? + We should use _("This is the message shown to the user") in Python code and return the proper translation when they are read. +* Reduce complexity on ``Build`` object (remove ``Build.status`` and ``Build.error`` fields among others). +* Since the ``Build`` object could have more than 1 notification, when showing them, we will sort them by importane: errors, warnings, note, tip. +* In case we need a pretty specific order, we can add an extra field for that, but it adds unnecessary complexity at this point. +* For those notifications that are attached to the ``Project`` or ``Organization``, should it be shown to all the members even if they don't have admin permissions? + If yes, this is good because all of them will be notified but only some of them will be able to take an action. + If no, non-admin users won't see the notification and won't be able to communicate this to the admins +* Notification could be attached to a ``BuildCommand`` in case we want to display a specific message on a command itself. + We don't know how useful this will be, but it's something we can consider in the future. +* Notification preferences: what kind of notifications I want to see in my own bell icon? + - Build errors + - Build tips + - Product updates + - Blog post news + - Organization updates + - Project updates + + +Implementation ideas +-------------------- + +This section shows all the classes and models involved for the notification system as well as some already known use-cases. + +.. note:: Accessing the database from the build process + + Builders doesn't have access to the database due to security reasons. + We had solved this limitation by creating an API endpoint the builder hits once they need to interact with the databse to get a ``Project``, ``Version`` and ``Build`` resources, create a ``BuildCommand`` resource, etc. + + Besides, the build process is capable to trigger Celery tasks that are useful for managing more complex logic + that also require accessing from and writing to the database. + + Currently, ``readthedocs.doc_builder.director.Director`` and + ``readthedocs.doc_builder.environments.DockerBuildEnvironment`` + have access to the API client and can use it to create the ``Notification`` resources. + + I plan to use the same pattern to create ``Notification`` resources by hitting the API from the director or the build environment. + In case we require hitting the API from other places, we will need to pass the API client instance to those other classes as well. + + + +``Message`` class definition +~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +This class encapsulates the content of the notification (e.g. header, body, icon, etc) --the message it's shown to the uer--, +and some helper logic to return in the API response. + +.. code-block:: python + + class Message: + def __init__(self): + header = str + body = str + icon = str + color = str + type = str(ERROR, WARINIG, NOTE, TIP) + + def get_display_icon(self): + if self.icon: + return self.icon + + if self.type == ERROR: + return "fa-exclamation" + if self.type == WARNING: + return "fa-triangle-exclamation" + + def get_display_color(self): + if self.color: + return color + + if self.type == ERROR: + return "red" + if self.type == WARNING: + return "yellow" + + + +Definition of notifications to display to users +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +This constant defines all the possible notifications to be displayed to the user. +Each notification has to be defined here using the ``Message`` class previously defined. + +.. code-block:: python + + NOTIFICATION_MESSAGES = { + "generic-with-build-id": Message( + header=_("Unknown problem"), + # Note the message receives the instance it's attached to + # and could be use it to inject related data + body=_( + """ + There was a problem with Read the Docs while building your documentation. + Please try again later. + If this problem persists, + report this error to us with your build id ({instance[pk]}). + """, + type=ERROR, + ), + ), + "build-os-required": Message( + header=_("Invalid configuration"), + body=_( + """ + The configuration key "build.os" is required to build your documentation. + Read more. + """, + type=ERROR, + ), + ), + "cancelled-by-user": Message( + header=_("User action"), + body=_( + """ + Build cancelled by the user. + """, + type=ERROR, + ), + ), + "os-ubuntu-18.04-deprecated": Message( + header=_("Deprecated OS selected"), + body=_( + """ + Ubuntu 18.04 is deprecated and will be removed soon. + Update your .readthedocs.yaml to use a newer image. + """, + type=TIP, + ), + ), + } + + + +``Notification`` model definition +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +This class is the representation of a notification attached to an resource (e.g. User, Build, etc) in the database. +It contains an identifier (``slug``) pointing to one of the messages defined in the previous section (key in constant ``NOTIFICATION_MESSAGES``). + +.. code-block:: python + + import textwrap + from django.utils.translation import gettext_noop as _ + + + class Notification(TimeStampedModel): + # Message identifier + slug = models.CharField(max_length=128) + + # UNREAD: the notification was not shown to the user + # READ: the notifiation was shown + # DISMISSED: the notification was shown and the user dismissed it + state = models.CharField(choices=[UNREAD, READ, DISMISSED], default=UNREAD) + + # Makes the notification imposible to dismiss (useful for Build notifications) + dismissable = models.BooleanField(default=False) + + # Show the notification under the bell icon for the user + bell = models.BooleanField(default=False, help_text="Show under bell icon") + + # Notification attached to + # + # Uses ContentType for this. + # https://docs.djangoproject.com/en/4.2/ref/contrib/contenttypes/#generic-relations + # + attached_to_content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE) + attached_to_id = models.PositiveIntegerField() + attached_to = GenericForeignKey("attached_to_content_type", "attached_to_id") + + # If we don't want to use ContentType, we could define all the potential models + # the notification could be attached to + # + # organization = models.ForeignKey(Organization, null=True, blank=True, default=None) + # project = models.ForeignKey(Project, null=True, blank=True, default=None) + # build = models.ForeignKey(Build, null=True, blank=True, default=None) + # user = models.ForeignKey(User, null=True, blank=True, default=None) + + def get_display_message(self): + return textwrap.dedent( + NOTIFICATION_MESSAGES.get(self.slug).format( + instance=self.attached_to, # Build, Project, Organization, User + ) + ) + + +Attach error ``Notification`` during the build process +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +During the build, we will keep raising exceptions to both things: + +- stop the build process immediately +- communicate back to the ``doc_builder.director.Director`` class the build failed. + +The director is the one in charge of creating the error ``Notification``, +in a similar way it currently works now. +The only difference is that instead of saving the error under ``Build.error`` as it currently works now, +it will create a ``Notification`` object and attach it to the particular ``Build``. +Note the director does not have access to the DB, so it will need to create/associate the object via an API endpoint/Celery task. + +Example of how the exception ``BuildCancelled`` creates an error ``Notification``: + + +.. code-block:: python + + class UpdateDocsTask(...): + def on_failure(self): + self.data.api_client.build(self.data.build["id"]).notifications.post( + { + "slug": "cancelled-by-user", + # Override default fields if required + "type": WARNING, + } + ) + + + +Attach non-error ``Notification`` during the build process +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +During the build, we will be able attach non-error notifications with the following pattern: + +- check something in particular (e.g. using a deprecated key in ``readthedocs.yaml``) +- create a non-error ``Notification`` and attach it to the particular ``Build`` object + +.. code-block:: python + + class DockerBuildEnvironment(...): + def check_deprecated_os_image(self): + if self.config.build.os == "ubuntu-18.04": + self.api_client.build(self.data.build["id"]).notifications.post( + { + "slug": "os-ubuntu-18.04-deprecated", + } + ) + + + +Show a ``Notification`` under the user's bell icon +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +If we want to show a notification on a user's profile, +we can create the notification as follows, +maybe from a simple script ran in the Django shell's console +after publishing a blog post: + + +.. code-block:: python + + users_to_show_notification = User.objects.filter(...) + + for user in users_to_show_notification: + Notification.objects.create( + slug="blog-post-beta-addons", + dismissable=True, + bell=True, + attached_to=User, + attached_to_id=user.id, + ) + + +Remove notification on status change +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +When we show a notification for an unpaid subscription, +we want to remove it once the user has updated and paid the subscription. +We can do this with the following code: + + +.. code-block:: python + + @handler("customer.subscription.updated", "customer.subscription.deleted") + def subscription_updated_event(event): + if subscription.status == ACTIVE: + organization = Organization.objects.get(slug="read-the-docs") + + Notification.objects.filter( + slug="subscription-update-your-cc-details", + state__in=[UNREAD, READ], + attached_to=Organization, + attached_to_id=organization.id, + ).update(state=DISMISSED) + + + +API definition +-------------- + +I will follows the same pattern we have on APIv3 that uses nested endpoints. +This means that we will add a ``/notifications/`` postfix to most of the resource endpoints +where we want to be able to attach/list notifications. + +Notifications list +++++++++++++++++++ + +.. http:get:: /api/v3/users/(str:user_username)/notifications/ + + Retrieve a list of all the notifications for this user. + +.. http:get:: /api/v3/projects/(str:project_slug)/notifications/ + + Retrieve a list of all the notifications for this project. + +.. http:get:: /api/v3/organizations/(str:organization_slug)/notifications/ + + Retrieve a list of all the notifications for this organization. + +.. http:get:: /api/v3/projects/(str:project_slug)/builds/(int:build_id)/notifications/ + + Retrieve a list of all the notifications for this build. + + **Example response**: + + .. sourcecode:: json + + { + "count": 25, + "next": "/api/v3/projects/pip/builds/12345/notifications/?unread=true&limit=10&offset=10", + "previous": null, + "results": [ + { + "slug": "cancelled-by-user", + "state": "unread", + "dismissable": false, + "bell": false, + "attached_to": "build", + "message": { + "header": "User action", + "body": "Build cancelled by the user.", + "type": "error", + "icon": "fa-exclamation", + "color": "red" + } + } + ] + } + + :query boolean unread: return only unread notifications + + +Notification create ++++++++++++++++++++ + + +.. http:post:: /api/v3/projects/(str:project_slug)/builds/(int:build_id)/notifications/ + + Create a notification for the resource. + In this example, for a ``Build`` resource. + + **Example request**: + + .. sourcecode:: json + + { + "slug": "cancelled-by-user", + "type": "error", + "state": "unread", + "dismissable": false, + "bell": false, + } + + +Backward compatibility +---------------------- + +It's not strickly required, but if we want, we could extract the current notification logic from: + +* Django templates + + * "Don't want ``setup.py`` called?" + * ``build.image`` config key is deprecated + * Configuration file is required + * ``build.commands`` is a beta feature + +* ``Build.error`` fields + + * Build cancelled by user + * Unknown exception + * ``build.os`` is not found + * No config file + * No checkout revision + * Failed when cloning the repository + * etc + +and iterate over all the ``Build`` objects to create a ``Notification`` object for each of them. From 500aff89cece624929f6878db26cab29716d1c05 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 6 Nov 2023 12:44:09 +0100 Subject: [PATCH 02/14] Update Sphinx titles --- docs/dev/design/new-notifications-system.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/dev/design/new-notifications-system.rst b/docs/dev/design/new-notifications-system.rst index c970de7a9af..4950b43efe8 100644 --- a/docs/dev/design/new-notifications-system.rst +++ b/docs/dev/design/new-notifications-system.rst @@ -351,7 +351,7 @@ This means that we will add a ``/notifications/`` postfix to most of the resourc where we want to be able to attach/list notifications. Notifications list -++++++++++++++++++ +~~~~~~~~~~~~~~~~~~ .. http:get:: /api/v3/users/(str:user_username)/notifications/ @@ -399,7 +399,7 @@ Notifications list Notification create -+++++++++++++++++++ +~~~~~~~~~~~~~~~~~~~ .. http:post:: /api/v3/projects/(str:project_slug)/builds/(int:build_id)/notifications/ From eabe034f5d5f468eb9a0eebeaaad4635b8f7a795 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 6 Nov 2023 13:51:09 +0100 Subject: [PATCH 03/14] Don't consider email/webhook notifications --- docs/dev/design/new-notifications-system.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/dev/design/new-notifications-system.rst b/docs/dev/design/new-notifications-system.rst index 4950b43efe8..9e275c875cb 100644 --- a/docs/dev/design/new-notifications-system.rst +++ b/docs/dev/design/new-notifications-system.rst @@ -52,6 +52,7 @@ Non-goals * Create new Build "state" or "status" * Implement the new notification in the old dashboard * Define front-end code implementation +* Replace email or webhook notifications Small notes and other considerations From 638cf65ea40ea19231b7b08ec109f9b02f09f7da Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 6 Nov 2023 13:51:21 +0100 Subject: [PATCH 04/14] Clarify non-goal --- docs/dev/design/new-notifications-system.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/dev/design/new-notifications-system.rst b/docs/dev/design/new-notifications-system.rst index 9e275c875cb..bffe02318f6 100644 --- a/docs/dev/design/new-notifications-system.rst +++ b/docs/dev/design/new-notifications-system.rst @@ -49,7 +49,7 @@ Goals Non-goals --------- -* Create new Build "state" or "status" +* Create new Build "state" or "status" option for these fields * Implement the new notification in the old dashboard * Define front-end code implementation * Replace email or webhook notifications From 2886fc396b58f91ca4e37d5ead62467650c40ebe Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 6 Nov 2023 15:17:06 +0100 Subject: [PATCH 05/14] Add some queryset filters to the API --- docs/dev/design/new-notifications-system.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/dev/design/new-notifications-system.rst b/docs/dev/design/new-notifications-system.rst index bffe02318f6..2dfc0387ad7 100644 --- a/docs/dev/design/new-notifications-system.rst +++ b/docs/dev/design/new-notifications-system.rst @@ -376,7 +376,7 @@ Notifications list { "count": 25, - "next": "/api/v3/projects/pip/builds/12345/notifications/?unread=true&limit=10&offset=10", + "next": "/api/v3/projects/pip/builds/12345/notifications/?unread=true&sort=type&limit=10&offset=10", "previous": null, "results": [ { @@ -397,6 +397,8 @@ Notifications list } :query boolean unread: return only unread notifications + :query string type: filter notifications by type (``error``, ``note``, ``tip``) + :query string sort: sort the notifications (``type``, ``date`` (default)) Notification create From e1a2bb36c84a5284b49ad5b63e06cfb436bf2af6 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 6 Nov 2023 15:18:50 +0100 Subject: [PATCH 06/14] Add `db_index` --- docs/dev/design/new-notifications-system.rst | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/docs/dev/design/new-notifications-system.rst b/docs/dev/design/new-notifications-system.rst index 2dfc0387ad7..a5566a24efb 100644 --- a/docs/dev/design/new-notifications-system.rst +++ b/docs/dev/design/new-notifications-system.rst @@ -212,7 +212,11 @@ It contains an identifier (``slug``) pointing to one of the messages defined in # UNREAD: the notification was not shown to the user # READ: the notifiation was shown # DISMISSED: the notification was shown and the user dismissed it - state = models.CharField(choices=[UNREAD, READ, DISMISSED], default=UNREAD) + state = models.CharField( + choices=[UNREAD, READ, DISMISSED], + default=UNREAD, + db_index=True, + ) # Makes the notification imposible to dismiss (useful for Build notifications) dismissable = models.BooleanField(default=False) From 621d0fb8eb57f0624d2d21f5894cd82f4802d60e Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 6 Nov 2023 15:20:17 +0100 Subject: [PATCH 07/14] Add note about notification create endpoint --- docs/dev/design/new-notifications-system.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/dev/design/new-notifications-system.rst b/docs/dev/design/new-notifications-system.rst index a5566a24efb..a8acfda9be7 100644 --- a/docs/dev/design/new-notifications-system.rst +++ b/docs/dev/design/new-notifications-system.rst @@ -427,6 +427,12 @@ Notification create } +.. note:: + + Similar API endpoints will be created for each of the resources + we want to attach a ``Notification`` (e.g. ``User``, ``Organization``, etc) + + Backward compatibility ---------------------- From 776d09f72482cbc0339a9d6db11ede8d68574ebd Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 6 Nov 2023 15:22:36 +0100 Subject: [PATCH 08/14] Sphinx style --- docs/dev/design/new-notifications-system.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/dev/design/new-notifications-system.rst b/docs/dev/design/new-notifications-system.rst index a8acfda9be7..a5f7811bbef 100644 --- a/docs/dev/design/new-notifications-system.rst +++ b/docs/dev/design/new-notifications-system.rst @@ -61,7 +61,7 @@ Small notes and other considerations * Django message system is not enough for this purpose. * Use a new model to store all the required data (expandable in the future) * How do we handle translations? - We should use _("This is the message shown to the user") in Python code and return the proper translation when they are read. + We should use ``_("This is the message shown to the user")`` in Python code and return the proper translation when they are read. * Reduce complexity on ``Build`` object (remove ``Build.status`` and ``Build.error`` fields among others). * Since the ``Build`` object could have more than 1 notification, when showing them, we will sort them by importane: errors, warnings, note, tip. * In case we need a pretty specific order, we can add an extra field for that, but it adds unnecessary complexity at this point. @@ -71,6 +71,7 @@ Small notes and other considerations * Notification could be attached to a ``BuildCommand`` in case we want to display a specific message on a command itself. We don't know how useful this will be, but it's something we can consider in the future. * Notification preferences: what kind of notifications I want to see in my own bell icon? + - Build errors - Build tips - Product updates From 3ff602be9c4b46fe76f58e98f5b39b3d42ddc8a5 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 22 Nov 2023 11:17:59 +0100 Subject: [PATCH 09/14] Add `Message.icon_style` (solid, duotone) --- docs/dev/design/new-notifications-system.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/dev/design/new-notifications-system.rst b/docs/dev/design/new-notifications-system.rst index a5f7811bbef..ae7a0b39bbc 100644 --- a/docs/dev/design/new-notifications-system.rst +++ b/docs/dev/design/new-notifications-system.rst @@ -115,6 +115,7 @@ and some helper logic to return in the API response. header = str body = str icon = str + icon_style = str(SOLID, DUOTONE) color = str type = str(ERROR, WARINIG, NOTE, TIP) @@ -395,6 +396,7 @@ Notifications list "body": "Build cancelled by the user.", "type": "error", "icon": "fa-exclamation", + "icon_style": "duotone", "color": "red" } } From 404615ab6639db8a647c5f369ef83f42e3f95158 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 22 Nov 2023 11:23:17 +0100 Subject: [PATCH 10/14] Use `news` instead of `bell` --- docs/dev/design/new-notifications-system.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/dev/design/new-notifications-system.rst b/docs/dev/design/new-notifications-system.rst index ae7a0b39bbc..b53ddc93861 100644 --- a/docs/dev/design/new-notifications-system.rst +++ b/docs/dev/design/new-notifications-system.rst @@ -224,7 +224,7 @@ It contains an identifier (``slug``) pointing to one of the messages defined in dismissable = models.BooleanField(default=False) # Show the notification under the bell icon for the user - bell = models.BooleanField(default=False, help_text="Show under bell icon") + news = models.BooleanField(default=False, help_text="Show under bell icon") # Notification attached to # @@ -320,7 +320,7 @@ after publishing a blog post: Notification.objects.create( slug="blog-post-beta-addons", dismissable=True, - bell=True, + news=True, attached_to=User, attached_to_id=user.id, ) @@ -389,7 +389,7 @@ Notifications list "slug": "cancelled-by-user", "state": "unread", "dismissable": false, - "bell": false, + "news": false, "attached_to": "build", "message": { "header": "User action", @@ -426,7 +426,7 @@ Notification create "type": "error", "state": "unread", "dismissable": false, - "bell": false, + "news": false, } From 66e08b36da58bb2de36c605500cfb54fd8d10db1 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 22 Nov 2023 11:24:47 +0100 Subject: [PATCH 11/14] Remove `color` field since it's based on `type` --- docs/dev/design/new-notifications-system.rst | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/docs/dev/design/new-notifications-system.rst b/docs/dev/design/new-notifications-system.rst index b53ddc93861..f490d1ccca6 100644 --- a/docs/dev/design/new-notifications-system.rst +++ b/docs/dev/design/new-notifications-system.rst @@ -32,7 +32,7 @@ Goals * Keep raising exceptions for errors from the build process * Ability to add non-error notifications from the build process -* Add extra metadata associated to the notification: icon, color, header, body, etc +* Add extra metadata associated to the notification: icon, header, body, etc * Support different types of notifications (e.g. error, warning, note, tip) * Re-use the new notification system for product updates (e.g. new features, deprecated config keys) * Message content lives on Python classes that can be translated and formatted with objects (e.g. Build, Project) @@ -116,7 +116,6 @@ and some helper logic to return in the API response. body = str icon = str icon_style = str(SOLID, DUOTONE) - color = str type = str(ERROR, WARINIG, NOTE, TIP) def get_display_icon(self): @@ -128,16 +127,6 @@ and some helper logic to return in the API response. if self.type == WARNING: return "fa-triangle-exclamation" - def get_display_color(self): - if self.color: - return color - - if self.type == ERROR: - return "red" - if self.type == WARNING: - return "yellow" - - Definition of notifications to display to users ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -397,7 +386,6 @@ Notifications list "type": "error", "icon": "fa-exclamation", "icon_style": "duotone", - "color": "red" } } ] From 269605532bf2e548b0cbe6269e505716b2250b33 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 22 Nov 2023 11:31:58 +0100 Subject: [PATCH 12/14] Use `message_id` instead of `slug` --- docs/dev/design/new-notifications-system.rst | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/docs/dev/design/new-notifications-system.rst b/docs/dev/design/new-notifications-system.rst index f490d1ccca6..346a6b5afbd 100644 --- a/docs/dev/design/new-notifications-system.rst +++ b/docs/dev/design/new-notifications-system.rst @@ -188,7 +188,7 @@ Each notification has to be defined here using the ``Message`` class previously ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ This class is the representation of a notification attached to an resource (e.g. User, Build, etc) in the database. -It contains an identifier (``slug``) pointing to one of the messages defined in the previous section (key in constant ``NOTIFICATION_MESSAGES``). +It contains an identifier (``message_id``) pointing to one of the messages defined in the previous section (key in constant ``NOTIFICATION_MESSAGES``). .. code-block:: python @@ -198,7 +198,7 @@ It contains an identifier (``slug``) pointing to one of the messages defined in class Notification(TimeStampedModel): # Message identifier - slug = models.CharField(max_length=128) + message_id = models.CharField(max_length=128) # UNREAD: the notification was not shown to the user # READ: the notifiation was shown @@ -234,7 +234,7 @@ It contains an identifier (``slug``) pointing to one of the messages defined in def get_display_message(self): return textwrap.dedent( - NOTIFICATION_MESSAGES.get(self.slug).format( + NOTIFICATION_MESSAGES.get(self.message_id).format( instance=self.attached_to, # Build, Project, Organization, User ) ) @@ -263,7 +263,7 @@ Example of how the exception ``BuildCancelled`` creates an error ``Notification` def on_failure(self): self.data.api_client.build(self.data.build["id"]).notifications.post( { - "slug": "cancelled-by-user", + "message_id": "cancelled-by-user", # Override default fields if required "type": WARNING, } @@ -286,7 +286,7 @@ During the build, we will be able attach non-error notifications with the follow if self.config.build.os == "ubuntu-18.04": self.api_client.build(self.data.build["id"]).notifications.post( { - "slug": "os-ubuntu-18.04-deprecated", + "message_id": "os-ubuntu-18.04-deprecated", } ) @@ -307,7 +307,7 @@ after publishing a blog post: for user in users_to_show_notification: Notification.objects.create( - slug="blog-post-beta-addons", + message_id="blog-post-beta-addons", dismissable=True, news=True, attached_to=User, @@ -331,7 +331,7 @@ We can do this with the following code: organization = Organization.objects.get(slug="read-the-docs") Notification.objects.filter( - slug="subscription-update-your-cc-details", + message_id="subscription-update-your-cc-details", state__in=[UNREAD, READ], attached_to=Organization, attached_to_id=organization.id, @@ -375,7 +375,7 @@ Notifications list "previous": null, "results": [ { - "slug": "cancelled-by-user", + "message_id": "cancelled-by-user", "state": "unread", "dismissable": false, "news": false, @@ -410,7 +410,7 @@ Notification create .. sourcecode:: json { - "slug": "cancelled-by-user", + "message_id": "cancelled-by-user", "type": "error", "state": "unread", "dismissable": false, From e3773728d8fe977f8ac70139d2765adb9e2752db Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 22 Nov 2023 11:34:48 +0100 Subject: [PATCH 13/14] Use `CANCELLED` for `state` when auto-removed --- docs/dev/design/new-notifications-system.rst | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/dev/design/new-notifications-system.rst b/docs/dev/design/new-notifications-system.rst index 346a6b5afbd..a00c0ba5a6d 100644 --- a/docs/dev/design/new-notifications-system.rst +++ b/docs/dev/design/new-notifications-system.rst @@ -37,7 +37,7 @@ Goals * Re-use the new notification system for product updates (e.g. new features, deprecated config keys) * Message content lives on Python classes that can be translated and formatted with objects (e.g. Build, Project) * Message could have richer content (e.g. HTML code) to generate links and emphasis -* Notifications have trackable state (e.g. unread (default)=never shown, read=shown, dismissed=don't show again) +* Notifications have trackable state (e.g. unread (default)=never shown, read=shown, dismissed=don't show again, cancelled=auto-removed after user action) * An object (e.g. Build, Organization) can have more than 1 notification attached * Remove hardcoded notifications from the templates * Notifications can be attached to Project, Organization, Build and User models @@ -203,8 +203,9 @@ It contains an identifier (``message_id``) pointing to one of the messages defin # UNREAD: the notification was not shown to the user # READ: the notifiation was shown # DISMISSED: the notification was shown and the user dismissed it + # CANCELLED: removed automatically because the user has done the action required (e.g. paid the subscription) state = models.CharField( - choices=[UNREAD, READ, DISMISSED], + choices=[UNREAD, READ, DISMISSED, CANCELLED], default=UNREAD, db_index=True, ) @@ -335,7 +336,7 @@ We can do this with the following code: state__in=[UNREAD, READ], attached_to=Organization, attached_to_id=organization.id, - ).update(state=DISMISSED) + ).update(state=CANCELLED) From 0bca7d9abc83de9ee98049dbb3e020f5b4caa2ba Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 22 Nov 2023 11:52:42 +0100 Subject: [PATCH 14/14] Expand backward compatibility --- docs/dev/design/new-notifications-system.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/dev/design/new-notifications-system.rst b/docs/dev/design/new-notifications-system.rst index a00c0ba5a6d..d62f190016c 100644 --- a/docs/dev/design/new-notifications-system.rst +++ b/docs/dev/design/new-notifications-system.rst @@ -448,3 +448,9 @@ It's not strickly required, but if we want, we could extract the current notific * etc and iterate over all the ``Build`` objects to create a ``Notification`` object for each of them. + +I'm not planning to implement the "new notification system" in the old templates. +It doesn't make sense to spend time in them since we are deprecating them. + +Old builds will keep using the current notification approach based on ``build.error`` field. +New builds won't have ``build.error`` anymore and they will use the new notification system on ext-theme.