From 92447e360992126d30675ae370ef49c07f540411 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 23 Nov 2023 17:14:11 +0100 Subject: [PATCH 001/101] Add some comments to understand the scope of the changes --- readthedocs/builds/tasks.py | 2 ++ readthedocs/core/utils/contact.py | 3 +++ readthedocs/domains/notifications.py | 1 + readthedocs/domains/tasks.py | 1 + readthedocs/notifications/views.py | 3 ++- readthedocs/oauth/notifications.py | 6 ++++++ readthedocs/projects/admin.py | 1 + readthedocs/projects/notifications.py | 2 ++ readthedocs/projects/views/private.py | 1 + readthedocs/subscriptions/notifications.py | 3 +++ 10 files changed, 22 insertions(+), 1 deletion(-) diff --git a/readthedocs/builds/tasks.py b/readthedocs/builds/tasks.py index 008086738e7..c91f9a2e04a 100644 --- a/readthedocs/builds/tasks.py +++ b/readthedocs/builds/tasks.py @@ -470,6 +470,8 @@ def send_build_status(build_pk, commit, status): for user in users: # Send Site notification about Build status reporting failure # to all the users of the project. + # + # TODO: migrate this notification to the new system notification = GitBuildStatusFailureNotification( context_object=build.project, extra_context={"provider_name": provider_name}, diff --git a/readthedocs/core/utils/contact.py b/readthedocs/core/utils/contact.py index eccb022b569..9bbf3644b1d 100644 --- a/readthedocs/core/utils/contact.py +++ b/readthedocs/core/utils/contact.py @@ -60,6 +60,9 @@ def contact_users( email_txt_template = engine.get_template("core/email/common.txt") email_html_template = engine.get_template("core/email/common.html") + # TODO: migrate this notification to the new notifications system. + # This notification are currently tied to a particular user. + # However, in the new system they will be tied to a Project. class TempNotification(SiteNotification): if sticky_notification: success_level = message_constants.SUCCESS_PERSISTENT diff --git a/readthedocs/domains/notifications.py b/readthedocs/domains/notifications.py index ae787b0d2b8..e9320bbb8db 100644 --- a/readthedocs/domains/notifications.py +++ b/readthedocs/domains/notifications.py @@ -4,6 +4,7 @@ from readthedocs.notifications.constants import REQUIREMENT +# TODO: migrate this notification to the new system class PendingCustomDomainValidation(Notification): app_templates = "domains" context_object_name = "domain" diff --git a/readthedocs/domains/tasks.py b/readthedocs/domains/tasks.py index a8ad7d9495e..de870fa5b24 100644 --- a/readthedocs/domains/tasks.py +++ b/readthedocs/domains/tasks.py @@ -29,6 +29,7 @@ def email_pending_custom_domains(number_of_emails=3): ) for domain in queryset: for user in AdminPermission.admins(domain.project): + # TODO: migrate this to the new system notification = PendingCustomDomainValidation( context_object=domain, request=HttpRequest(), diff --git a/readthedocs/notifications/views.py b/readthedocs/notifications/views.py index 8b3cd23c725..94644fd207d 100644 --- a/readthedocs/notifications/views.py +++ b/readthedocs/notifications/views.py @@ -5,7 +5,8 @@ from .forms import SendNotificationForm - +# TODO: remove this view since we don't really use it from the admin. +# It doesn't really worth re-implementing it in the new notification system. class SendNotificationView(FormView): """ diff --git a/readthedocs/oauth/notifications.py b/readthedocs/oauth/notifications.py index 0246606885f..14721d4ac48 100644 --- a/readthedocs/oauth/notifications.py +++ b/readthedocs/oauth/notifications.py @@ -1,3 +1,9 @@ +# TODO: all the site notifications from this file should be re-implemented +# using the new notification system. +# +# These notifications will be attached to a Project. + + from django.urls import reverse from django.utils.translation import gettext_lazy as _ from messages_extends.constants import ERROR_PERSISTENT diff --git a/readthedocs/projects/admin.py b/readthedocs/projects/admin.py index ff5a25ebb59..a3df4acc8b0 100644 --- a/readthedocs/projects/admin.py +++ b/readthedocs/projects/admin.py @@ -49,6 +49,7 @@ def has_delete_permission(self, request, obj=None): return False +# TODO: remove this view since it's not really used class ProjectSendNotificationView(SendNotificationView): notification_classes = [ ResourceUsageNotification, diff --git a/readthedocs/projects/notifications.py b/readthedocs/projects/notifications.py index 81a5178dca9..8f5e4f95ce2 100644 --- a/readthedocs/projects/notifications.py +++ b/readthedocs/projects/notifications.py @@ -8,6 +8,7 @@ from readthedocs.notifications.constants import REQUIREMENT +# TODO: this notification can be removed, it's just an admin action we don't really use class ResourceUsageNotification(Notification): name = "resource_usage" context_object_name = "project" @@ -15,6 +16,7 @@ class ResourceUsageNotification(Notification): level = REQUIREMENT +# TODO: migrate this communication to the new system, attached to a User class EmailConfirmNotification(SiteNotification): failure_level = ERROR_PERSISTENT failure_message = _( diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index e5e9e63acef..132f984ad0a 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -122,6 +122,7 @@ def validate_primary_email(self, user): email_qs = user.emailaddress_set.filter(primary=True) email = email_qs.first() if not email or not email.verified: + # TODO: migrate this notification to the new system notification = EmailConfirmNotification(user=user, success=False) notification.send() diff --git a/readthedocs/subscriptions/notifications.py b/readthedocs/subscriptions/notifications.py index 0bbc52d538a..002ab2dccdb 100644 --- a/readthedocs/subscriptions/notifications.py +++ b/readthedocs/subscriptions/notifications.py @@ -1,3 +1,6 @@ +# TODO: all these notification should be migrated to the new notification system. +# Most / All of them will be attached to an Organization + """Organization level notifications.""" From a7cb5ad7e9bf927e00f97c245c0f056649d77c74 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 23 Nov 2023 17:31:36 +0100 Subject: [PATCH 002/101] Add more comments about the plan --- readthedocs/notifications/backends.py | 5 +++++ readthedocs/notifications/notification.py | 12 ++++++++++++ readthedocs/projects/models.py | 3 +++ 3 files changed, 20 insertions(+) diff --git a/readthedocs/notifications/backends.py b/readthedocs/notifications/backends.py index b496d040f48..f07d5b5d7d8 100644 --- a/readthedocs/notifications/backends.py +++ b/readthedocs/notifications/backends.py @@ -73,6 +73,11 @@ def send(self, notification): ) +# TODO: modify this backend to create a `Notification` object +# instead of calling `storage.add()`. +# +# Note that probably `request` argument can be deleted from here +# since it's not required. class SiteBackend(Backend): """ diff --git a/readthedocs/notifications/notification.py b/readthedocs/notifications/notification.py index 4c13569923f..169a0a435f5 100644 --- a/readthedocs/notifications/notification.py +++ b/readthedocs/notifications/notification.py @@ -15,6 +15,13 @@ log = structlog.get_logger(__name__) +# NOTE: this Notification class should be renamed to EmailNotification, +# since it won't share too much code with the SiteNotification anymore. +# +# We can extract the shared logic if we want, +# but I don't think there is going to be too much in common. +# SiteNotification won't use render from TXT/HTML files. +# class Notification: """ @@ -51,6 +58,8 @@ def get_subject(self): def get_context_data(self): context = { self.context_object_name: self.object, + # NOTE: I checked the notifications templates, and the "request" attribute is not used. + # We can remove this complexity. "request": self.request, "production_uri": "{scheme}://{host}".format( scheme="https", @@ -95,6 +104,9 @@ def send(self): :py:func:`send_notification`, taking care to limit which backends are avoided. """ + + # NOTE: the concept of "backend" can be removed because we won't have multiple backends. + # Just overriding the `send()` method will be enough and reduce the complexity. send_notification(self.request, self) diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 6a01a9aa57e..8ae0b4aca19 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -1570,6 +1570,9 @@ def processed_json(self): class Notification(TimeStampedModel): + + """WebHook / Email notification attached to a Project.""" + # TODO: Overridden from TimeStampedModel just to allow null values, # remove after deploy. created = CreationDateTimeField( From fb79264474330d54c9153fa67228f3b9eacf7959 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 23 Nov 2023 17:45:42 +0100 Subject: [PATCH 003/101] 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. --- readthedocs/notifications/forms.py | 40 --------- readthedocs/notifications/views.py | 121 -------------------------- readthedocs/projects/admin.py | 21 ----- readthedocs/projects/notifications.py | 11 +-- 4 files changed, 1 insertion(+), 192 deletions(-) delete mode 100644 readthedocs/notifications/forms.py delete mode 100644 readthedocs/notifications/views.py diff --git a/readthedocs/notifications/forms.py b/readthedocs/notifications/forms.py deleted file mode 100644 index b42f3699502..00000000000 --- a/readthedocs/notifications/forms.py +++ /dev/null @@ -1,40 +0,0 @@ -"""HTML forms for sending notifications.""" -from django import forms -from django.utils.translation import gettext_lazy as _ - - -class SendNotificationForm(forms.Form): - - """ - Send notification form. - - Used for sending a notification to a list of users from admin pages - - Fields: - - _selected_action - This is required for the admin intermediate form to submit - - source - Source notification class to use, referenced by name - - :param notification_classes: List of notification sources to display - :type notification_classes: list - """ - - _selected_action = forms.CharField(widget=forms.MultipleHiddenInput) - - source = forms.ChoiceField(label=_("Notification"), choices=[]) - - def __init__(self, *args, **kwargs): - self.notification_classes = kwargs.pop("notification_classes", []) - super().__init__(*args, **kwargs) - self.fields["source"].choices = [ - (cls.name, cls.name) for cls in self.notification_classes - ] - - def clean_source(self): - """Get the source class from the class name.""" - source = self.cleaned_data["source"] - classes = {cls.name: cls for cls in self.notification_classes} - return classes.get(source, None) diff --git a/readthedocs/notifications/views.py b/readthedocs/notifications/views.py deleted file mode 100644 index 94644fd207d..00000000000 --- a/readthedocs/notifications/views.py +++ /dev/null @@ -1,121 +0,0 @@ -"""Django views for the notifications app.""" -from django.contrib import messages -from django.http import HttpResponseRedirect -from django.views.generic import FormView - -from .forms import SendNotificationForm - -# TODO: remove this view since we don't really use it from the admin. -# It doesn't really worth re-implementing it in the new notification system. -class SendNotificationView(FormView): - - """ - Form view for sending notifications to users from admin pages. - - Accepts the following additional parameters: - - :param queryset: Queryset to use to determine the users to send emails to - :param action_name: Name of the action to pass to the form template, - determines the action to pass back to the admin view - :param notification_classes: List of :py:class:`Notification` classes to - display in the form - """ - - form_class = SendNotificationForm - template_name = "notifications/send_notification_form.html" - action_name = "send_email" - notification_classes = [] - - def get_form_kwargs(self): - """ - Override form kwargs based on input fields. - - The admin posts to this view initially, so detect the send button on - form post variables. Drop additional fields if we see the send button. - """ - kwargs = super().get_form_kwargs() - kwargs["notification_classes"] = self.notification_classes - if "send" not in self.request.POST: - kwargs.pop("data", None) - kwargs.pop("files", None) - return kwargs - - def get_initial(self): - """Add selected ids to initial form data.""" - initial = super().get_initial() - initial["_selected_action"] = self.request.POST.getlist("_selected_action") - return initial - - def form_valid(self, form): - """If form is valid, send notification to recipients.""" - count = 0 - notification_cls = form.cleaned_data["source"] - for obj in self.get_queryset().all(): - for recipient in self.get_object_recipients(obj): - notification = notification_cls( - context_object=obj, - request=self.request, - user=recipient, - ) - notification.send() - count += 1 - if count == 0: - self.message_user("No recipients to send to", level=messages.ERROR) - else: - self.message_user("Queued {} messages".format(count)) - return HttpResponseRedirect(self.request.get_full_path()) - - def get_object_recipients(self, obj): - """ - Iterate over queryset objects and return User objects. - - This allows for non-User querysets to pass back a list of Users to send - to. By default, assume we're working with :py:class:`User` objects and - just yield the single object. - - For example, this could be made to return project owners with:: - - for owner in AdminPermission.members(project): - yield owner - - :param obj: object from queryset, type is dependent on model class - :rtype: django.contrib.auth.models.User - """ - yield obj - - def get_queryset(self): - return self.kwargs.get("queryset") - - def get_context_data(self, **kwargs): - """Return queryset in context.""" - context = super().get_context_data(**kwargs) - recipients = [] - for obj in self.get_queryset().all(): - recipients.extend(self.get_object_recipients(obj)) - context["recipients"] = recipients - context["action_name"] = self.action_name - return context - - def message_user( - self, - message, - level=messages.INFO, - extra_tags="", - fail_silently=False, - ): - """ - Implementation of. - - :py:meth:`django.contrib.admin.options.ModelAdmin.message_user` - - Send message through messages framework - """ - # TODO generalize this or check if implementation in ModelAdmin is - # usable here - messages.add_message( - self.request, - level, - message, - extra_tags=extra_tags, - fail_silently=fail_silently, - ) diff --git a/readthedocs/projects/admin.py b/readthedocs/projects/admin.py index a3df4acc8b0..b2d22e98b06 100644 --- a/readthedocs/projects/admin.py +++ b/readthedocs/projects/admin.py @@ -10,7 +10,6 @@ from readthedocs.builds.models import Version from readthedocs.core.history import ExtraSimpleHistoryAdmin, set_change_reason from readthedocs.core.utils import trigger_build -from readthedocs.notifications.views import SendNotificationView from readthedocs.projects.tasks.search import reindex_version from readthedocs.redirects.models import Redirect @@ -28,7 +27,6 @@ WebHook, WebHookEvent, ) -from .notifications import ResourceUsageNotification from .tag_utils import import_tags from .tasks.utils import clean_project_resources @@ -49,17 +47,6 @@ def has_delete_permission(self, request, obj=None): return False -# TODO: remove this view since it's not really used -class ProjectSendNotificationView(SendNotificationView): - notification_classes = [ - ResourceUsageNotification, - ] - - def get_object_recipients(self, obj): - for owner in obj.users.all(): - yield owner - - class ProjectRelationshipInline(admin.TabularInline): """Project inline relationship view for :py:class:`ProjectAdmin`.""" @@ -260,7 +247,6 @@ class ProjectAdmin(ExtraSimpleHistoryAdmin): ) raw_id_fields = ("users", "main_language_project", "remote_repository") actions = [ - "send_owner_email", "ban_owner", "run_spam_rule_checks", "build_default_version", @@ -277,13 +263,6 @@ def matching_spam_rules(self, obj): def feature_flags(self, obj): return "\n".join([str(f.get_feature_display()) for f in obj.features]) - @admin.action(description="Notify project owners") - def send_owner_email(self, request, queryset): - view = ProjectSendNotificationView.as_view( - action_name="send_owner_email", - ) - return view(request, queryset=queryset) - def run_spam_rule_checks(self, request, queryset): """Run all the spam checks on this project.""" if "readthedocsext.spamfighting" not in settings.INSTALLED_APPS: diff --git a/readthedocs/projects/notifications.py b/readthedocs/projects/notifications.py index 8f5e4f95ce2..262ce12ea3f 100644 --- a/readthedocs/projects/notifications.py +++ b/readthedocs/projects/notifications.py @@ -4,16 +4,7 @@ from django.utils.translation import gettext_lazy as _ from messages_extends.constants import ERROR_PERSISTENT -from readthedocs.notifications import Notification, SiteNotification -from readthedocs.notifications.constants import REQUIREMENT - - -# TODO: this notification can be removed, it's just an admin action we don't really use -class ResourceUsageNotification(Notification): - name = "resource_usage" - context_object_name = "project" - subject = "Builds for {{ project.name }} are using too many resources" - level = REQUIREMENT +from readthedocs.notifications import SiteNotification # TODO: migrate this communication to the new system, attached to a User From 780e28b3eb387be4ba44201e629ec0432686d673 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 23 Nov 2023 18:00:35 +0100 Subject: [PATCH 004/101] Add more comments to organize the work of new notification system --- readthedocs/builds/tasks.py | 1 + readthedocs/notifications/constants.py | 2 ++ readthedocs/oauth/tasks.py | 5 +++++ readthedocs/rtd_tests/tests/test_notifications.py | 3 +++ readthedocs/subscriptions/tasks.py | 2 ++ 5 files changed, 13 insertions(+) diff --git a/readthedocs/builds/tasks.py b/readthedocs/builds/tasks.py index c91f9a2e04a..657ac84686c 100644 --- a/readthedocs/builds/tasks.py +++ b/readthedocs/builds/tasks.py @@ -494,6 +494,7 @@ def send_build_notifications(version_pk, build_pk, event): if not build: return + # NOTE: this class is used to send email/webhook notification on build success/failure sender = BuildNotificationSender( version=version, build=build, diff --git a/readthedocs/notifications/constants.py b/readthedocs/notifications/constants.py index 375702b0998..1e8b7d408c2 100644 --- a/readthedocs/notifications/constants.py +++ b/readthedocs/notifications/constants.py @@ -1,3 +1,5 @@ +# TODO: remove most of this file since it won't be used anymore. + """Notification constants.""" from messages_extends import constants as message_constants diff --git a/readthedocs/oauth/tasks.py b/readthedocs/oauth/tasks.py index a49bf924987..ffda3961b70 100644 --- a/readthedocs/oauth/tasks.py +++ b/readthedocs/oauth/tasks.py @@ -166,6 +166,8 @@ def attach_webhook(project_pk, user_pk, integration=None): if not project or not user: return False + # TODO: migrate this notification to the new system. + # I will be attached to a Project. project_notification = InvalidProjectWebhookNotification( context_object=project, user=user, @@ -189,6 +191,9 @@ def attach_webhook(project_pk, user_pk, integration=None): return None provider = allauth_registry.by_id(service.adapter.provider_id) + + # TODO: migrate this notification to the new system. + # I will be attached to a Project. notification = AttachWebhookNotification( context_object=provider, extra_context={"project": project}, diff --git a/readthedocs/rtd_tests/tests/test_notifications.py b/readthedocs/rtd_tests/tests/test_notifications.py index d6f648799a2..05933abf306 100644 --- a/readthedocs/rtd_tests/tests/test_notifications.py +++ b/readthedocs/rtd_tests/tests/test_notifications.py @@ -1,3 +1,6 @@ +# TODO: adapt/remove these tests because we won't have backends anymore. + + """Notification tests.""" diff --git a/readthedocs/subscriptions/tasks.py b/readthedocs/subscriptions/tasks.py index e843a5c09be..ce44ea8dc05 100644 --- a/readthedocs/subscriptions/tasks.py +++ b/readthedocs/subscriptions/tasks.py @@ -11,6 +11,8 @@ from readthedocs.core.utils import send_email from readthedocs.organizations.models import Organization from readthedocs.projects.models import Domain, Project + +# NOTE: these are Email notifications so they won't be migrated from readthedocs.subscriptions.notifications import ( OrganizationDisabledNotification, TrialEndingNotification, From 749622c2e989c9711827a85e382bfb25cc5ac01d Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 23 Nov 2023 18:38:23 +0100 Subject: [PATCH 005/101] Notification: initial modelling --- readthedocs/notifications/constants.py | 19 +++++ readthedocs/notifications/messages.py | 68 +++++++++++++++++ .../notifications/migrations/0001_initial.py | 75 +++++++++++++++++++ .../notifications/migrations/__init__.py | 0 readthedocs/notifications/models.py | 60 +++++++++++++++ 5 files changed, 222 insertions(+) create mode 100644 readthedocs/notifications/messages.py create mode 100644 readthedocs/notifications/migrations/0001_initial.py create mode 100644 readthedocs/notifications/migrations/__init__.py create mode 100644 readthedocs/notifications/models.py diff --git a/readthedocs/notifications/constants.py b/readthedocs/notifications/constants.py index 1e8b7d408c2..4c31db9902b 100644 --- a/readthedocs/notifications/constants.py +++ b/readthedocs/notifications/constants.py @@ -34,3 +34,22 @@ WARNING_NON_PERSISTENT, ERROR_NON_PERSISTENT, ) + + +# TODO: use Enum here for these ones +# Status +UNREAD = "unread" +READ = "read" +DISMISSED = "dissmissed" +CANCELLED = "cancelled" + +# Type +ERROR = "error" +WARNING = "warning" +INFO = "info" +NOTE = "note" +TIP = "tip" + +# Icons +SOLID = "solid" +DUOTONE = "duotone" diff --git a/readthedocs/notifications/messages.py b/readthedocs/notifications/messages.py new file mode 100644 index 00000000000..45bf0ae9e97 --- /dev/null +++ b/readthedocs/notifications/messages.py @@ -0,0 +1,68 @@ +from django.utils.translation import gettext_noop as _ + +from .constants import ERROR, SOLID, TIP, WARNING + + +class Message: + def __init__(self, header, body, type, icon=None, icon_style=SOLID): + self.header = header + self.body = body + self.type = type # (ERROR, WARNING, INFO, NOTE, TIP) + self.icon = icon + self.icon_style = icon_style # (SOLID, DUOTONE) + + 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" + + +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, + ), +} diff --git a/readthedocs/notifications/migrations/0001_initial.py b/readthedocs/notifications/migrations/0001_initial.py new file mode 100644 index 00000000000..e140044e1ff --- /dev/null +++ b/readthedocs/notifications/migrations/0001_initial.py @@ -0,0 +1,75 @@ +# Generated by Django 4.2.6 on 2023-11-23 17:26 + +import django.db.models.deletion +import django_extensions.db.fields +from django.db import migrations, models + + +class Migration(migrations.Migration): + initial = True + + dependencies = [ + ("contenttypes", "0002_remove_content_type_name"), + ] + + operations = [ + migrations.CreateModel( + name="Notification", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ( + "created", + django_extensions.db.fields.CreationDateTimeField( + auto_now_add=True, verbose_name="created" + ), + ), + ( + "modified", + django_extensions.db.fields.ModificationDateTimeField( + auto_now=True, verbose_name="modified" + ), + ), + ("message_id", models.CharField(max_length=128)), + ( + "state", + models.CharField( + choices=[ + ("unread", "unread"), + ("read", "read"), + ("dissmissed", "dissmissed"), + ("cancelled", "cancelled"), + ], + db_index=True, + default="unread", + ), + ), + ("dismissable", models.BooleanField(default=False)), + ( + "news", + models.BooleanField( + default=False, help_text="Show under bell icon" + ), + ), + ("attached_to_id", models.PositiveIntegerField()), + ( + "attached_to_content_type", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to="contenttypes.contenttype", + ), + ), + ], + options={ + "get_latest_by": "modified", + "abstract": False, + }, + ), + ] diff --git a/readthedocs/notifications/migrations/__init__.py b/readthedocs/notifications/migrations/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/readthedocs/notifications/models.py b/readthedocs/notifications/models.py new file mode 100644 index 00000000000..7660135704d --- /dev/null +++ b/readthedocs/notifications/models.py @@ -0,0 +1,60 @@ +import textwrap + +from django.contrib.contenttypes.fields import GenericForeignKey +from django.contrib.contenttypes.models import ContentType +from django.db import models +from django.utils.translation import gettext_noop as _ +from django_extensions.db.models import TimeStampedModel + +from .constants import CANCELLED, DISMISSED, READ, UNREAD +from .messages import NOTIFICATION_MESSAGES + + +class Notification(TimeStampedModel): + # Message identifier + message_id = 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 + # CANCELLED: removed automatically because the user has done the action required (e.g. paid the subscription) + state = models.CharField( + choices=[ + (UNREAD, UNREAD), + (READ, READ), + (DISMISSED, DISMISSED), + (CANCELLED, CANCELLED), + ], + default=UNREAD, + db_index=True, + ) + + # 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 + news = 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.message_id).format( + instance=self.attached_to, # Build, Project, Organization, User + ) + ) From 777796029fa65f2dc17277f10bab6d36049aa525 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 27 Nov 2023 10:53:50 +0100 Subject: [PATCH 006/101] Define `max_length=128` that's required for SQLite (tests) --- readthedocs/notifications/migrations/0001_initial.py | 1 + readthedocs/notifications/models.py | 1 + 2 files changed, 2 insertions(+) diff --git a/readthedocs/notifications/migrations/0001_initial.py b/readthedocs/notifications/migrations/0001_initial.py index e140044e1ff..5032e20caee 100644 --- a/readthedocs/notifications/migrations/0001_initial.py +++ b/readthedocs/notifications/migrations/0001_initial.py @@ -49,6 +49,7 @@ class Migration(migrations.Migration): ], db_index=True, default="unread", + max_length=128, ), ), ("dismissable", models.BooleanField(default=False)), diff --git a/readthedocs/notifications/models.py b/readthedocs/notifications/models.py index 7660135704d..8c8f87bb623 100644 --- a/readthedocs/notifications/models.py +++ b/readthedocs/notifications/models.py @@ -26,6 +26,7 @@ class Notification(TimeStampedModel): (CANCELLED, CANCELLED), ], default=UNREAD, + max_length=128, db_index=True, ) From 105e7c8b2b644eb1522243106442442854d62406 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 27 Nov 2023 13:04:25 +0100 Subject: [PATCH 007/101] Initial implementation for build notifications API endpoint --- readthedocs/api/v3/serializers.py | 76 +++++++++++++++++++++++++++ readthedocs/api/v3/urls.py | 12 ++++- readthedocs/api/v3/views.py | 29 ++++++++++ readthedocs/builds/models.py | 9 ++++ readthedocs/notifications/messages.py | 7 ++- readthedocs/notifications/models.py | 14 +++-- readthedocs/projects/models.py | 8 +++ 7 files changed, 144 insertions(+), 11 deletions(-) diff --git a/readthedocs/api/v3/serializers.py b/readthedocs/api/v3/serializers.py index cab929f874a..1814fa7fd15 100644 --- 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 ( @@ -59,10 +60,33 @@ class Meta: fields = [] +# TODO: decide whether or not include a `_links` field on the object +# +# This also includes adding `/api/v3/notifications/` 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( @@ -95,6 +119,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") @@ -189,6 +223,48 @@ def get_success(self, obj): return None +class NotificationMessageSerializer(serializers.Serializer): + id = serializers.SlugField() + header = serializers.CharField() + body = serializers.CharField() + type = serializers.CharField() + icon = serializers.CharField() + icon_style = serializers.CharField() + + class Meta: + fields = [ + "id", + "header", + "body", + "type", + "icon", + "icon_style", + ] + + +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/urls.py b/readthedocs/api/v3/urls.py index 310f1b00e88..f596f674237 100644 --- 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,21 @@ # 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"], ) +# allows /api/v3/projects/pip/builds/1053/notifications/ +builds.register( + r"notifications", + NotificationsViewSet, + basename="project-builds-notifications", + parents_query_lookups=["project__slug", "build__id"], +) + # 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 index 4df4d7a2b35..3196980dca1 100644 --- 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, diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index e782f52599f..ee5fed6bcca 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -7,6 +7,7 @@ import regex import structlog from django.conf import settings +from django.contrib.contenttypes.fields import GenericRelation from django.db import models from django.db.models import F from django.urls import reverse @@ -62,6 +63,7 @@ from readthedocs.builds.version_slug import VersionSlugField from readthedocs.config import LATEST_CONFIGURATION_VERSION from readthedocs.core.utils import extract_valid_attributes_for_model, trigger_build +from readthedocs.notifications.models import Notification from readthedocs.projects.constants import ( BITBUCKET_COMMIT_URL, BITBUCKET_URL, @@ -834,6 +836,13 @@ class Build(models.Model): blank=True, ) + notifications = GenericRelation( + Notification, + related_query_name="build", + content_type_field="attached_to_content_type", + object_id_field="attached_to_id", + ) + # Managers objects = BuildQuerySet.as_manager() # Only include BRANCH, TAG, UNKNOWN type Version builds. diff --git a/readthedocs/notifications/messages.py b/readthedocs/notifications/messages.py index 45bf0ae9e97..64051cb8681 100644 --- a/readthedocs/notifications/messages.py +++ b/readthedocs/notifications/messages.py @@ -4,7 +4,8 @@ class Message: - def __init__(self, header, body, type, icon=None, icon_style=SOLID): + def __init__(self, id, header, body, type, icon=None, icon_style=SOLID): + self.id = id self.header = header self.body = body self.type = type # (ERROR, WARNING, INFO, NOTE, TIP) @@ -23,6 +24,7 @@ def get_display_icon(self): NOTIFICATION_MESSAGES = { "generic-with-build-id": Message( + id="generic-with-build-id", header=_("Unknown problem"), # Note the message receives the instance it's attached to # and could be use it to inject related data @@ -37,6 +39,7 @@ def get_display_icon(self): type=ERROR, ), "build-os-required": Message( + id="build-os-required", header=_("Invalid configuration"), body=_( """ @@ -47,6 +50,7 @@ def get_display_icon(self): type=ERROR, ), "cancelled-by-user": Message( + id="cancelled-by-user", header=_("User action"), body=_( """ @@ -56,6 +60,7 @@ def get_display_icon(self): type=ERROR, ), "os-ubuntu-18.04-deprecated": Message( + id="os-ubuntu-18.04-deprecated", header=_("Deprecated OS selected"), body=_( """ diff --git a/readthedocs/notifications/models.py b/readthedocs/notifications/models.py index 8c8f87bb623..8e3c30ce0d8 100644 --- a/readthedocs/notifications/models.py +++ b/readthedocs/notifications/models.py @@ -36,7 +36,7 @@ class Notification(TimeStampedModel): # Show the notification under the bell icon for the user news = models.BooleanField(default=False, help_text=_("Show under bell icon")) - # Notification attached to + # Notification attached to Organization, Project, Build or User # # Uses ContentType for this. # https://docs.djangoproject.com/en/4.2/ref/contrib/contenttypes/#generic-relations @@ -45,13 +45,11 @@ class Notification(TimeStampedModel): 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 __str__(self): + return self.message_id + + def get_message(self): + return NOTIFICATION_MESSAGES.get(self.message_id) def get_display_message(self): return textwrap.dedent( diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 8ae0b4aca19..93d351a2af1 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -36,6 +36,7 @@ from readthedocs.core.utils import extract_valid_attributes_for_model, slugify from readthedocs.core.utils.url import unsafe_join_url_path from readthedocs.domains.querysets import DomainQueryset +from readthedocs.notifications.models import Notification as SiteNotification from readthedocs.projects import constants from readthedocs.projects.exceptions import ProjectConfigurationError from readthedocs.projects.managers import HTMLFileManager @@ -532,6 +533,13 @@ class Project(models.Model): blank=True, ) + notifications = GenericRelation( + SiteNotification, + related_query_name="build", + content_type_field="attached_to_content_type", + object_id_field="attached_to_id", + ) + # TODO: remove the following fields since they all are going to be ignored # by the application when we start requiring a ``.readthedocs.yaml`` file. # These fields are: From cbf0e5443d1ab54762fa073cd7dbd7c8402561ac Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 27 Nov 2023 13:33:01 +0100 Subject: [PATCH 008/101] API endpoint to create build notifications --- readthedocs/api/v3/serializers.py | 10 ++++++++++ readthedocs/api/v3/urls.py | 9 +++++++++ readthedocs/api/v3/views.py | 30 ++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+) diff --git a/readthedocs/api/v3/serializers.py b/readthedocs/api/v3/serializers.py index 1814fa7fd15..a09814dfe5f 100644 --- a/readthedocs/api/v3/serializers.py +++ b/readthedocs/api/v3/serializers.py @@ -242,6 +242,16 @@ class Meta: ] +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() diff --git a/readthedocs/api/v3/urls.py b/readthedocs/api/v3/urls.py index f596f674237..57c400c90b9 100644 --- a/readthedocs/api/v3/urls.py +++ b/readthedocs/api/v3/urls.py @@ -3,6 +3,7 @@ BuildsCreateViewSet, BuildsViewSet, EnvironmentVariablesViewSet, + NotificationsCreateViewSet, NotificationsViewSet, ProjectsViewSet, RedirectsViewSet, @@ -70,6 +71,14 @@ parents_query_lookups=["project__slug"], ) +# allows /api/v3/projects/pip/builds/1053/notifications/ +builds.register( + r"notifications", + NotificationsCreateViewSet, + basename="project-builds-notifications", + parents_query_lookups=["project__slug", "build__id"], +) + # allows /api/v3/projects/pip/builds/1053/notifications/ builds.register( r"notifications", diff --git a/readthedocs/api/v3/views.py b/readthedocs/api/v3/views.py index 3196980dca1..bd7ed20e3da 100644 --- a/readthedocs/api/v3/views.py +++ b/readthedocs/api/v3/views.py @@ -1,6 +1,7 @@ import django_filters.rest_framework as filters from django.contrib.contenttypes.models import ContentType from django.db.models import Exists, OuterRef +from django.shortcuts import get_object_or_404 from rest_flex_fields import is_expanded from rest_flex_fields.views import FlexFieldsMixin from rest_framework import status @@ -64,6 +65,7 @@ BuildCreateSerializer, BuildSerializer, EnvironmentVariableSerializer, + NotificationCreateSerializer, NotificationSerializer, OrganizationSerializer, ProjectCreateSerializer, @@ -410,6 +412,34 @@ def get_queryset(self): ) +class NotificationsCreateViewSet( + NotificationsViewSet, + CreateModelMixin, +): + def get_serializer_class(self): + if self.action == "create": + return NotificationCreateSerializer + + return super().get_serializer_class() + + def create(self, request, **kwargs): # pylint: disable=arguments-differ + # TODO: move this into `self._get_parent_build()` method + build_id = kwargs.get("parent_lookup_build__id") + build = get_object_or_404(Build, pk=build_id) + + # TODO: find a better way to create a Notification object from a serializer "automatically" + n = NotificationCreateSerializer(data=request.POST) + if n.is_valid(): + build.notifications.create(**n.data) + return Response(status=status.HTTP_204_NO_CONTENT) + + code = status.HTTP_400_BAD_REQUEST + return Response(status=code) + + # build = self._get_parent_build() + # notification = build.notifications.create() + + class RedirectsViewSet( APIv3Settings, NestedViewSetMixin, From 584c0554ec24a1b2917a59c7e57008d472ee1cf9 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 27 Nov 2023 17:16:41 +0100 Subject: [PATCH 009/101] Use APIv2 for internal endpoint to call from builders --- readthedocs/api/v2/serializers.py | 11 +++++++ readthedocs/api/v2/urls.py | 2 ++ readthedocs/api/v2/views/model_views.py | 41 +++++++++++++++++++++++++ readthedocs/api/v3/urls.py | 11 ++----- 4 files changed, 56 insertions(+), 9 deletions(-) diff --git a/readthedocs/api/v2/serializers.py b/readthedocs/api/v2/serializers.py index 4756eceb675..2ded9bbfd09 100644 --- a/readthedocs/api/v2/serializers.py +++ b/readthedocs/api/v2/serializers.py @@ -7,6 +7,7 @@ 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 +379,13 @@ def get_username(self, obj): or obj.extra_data.get("login") # FIXME: which one is GitLab? ) + + +# TODO: consider using a third party library. +# See https://github.com/LilyFoote/rest-framework-generic-relations +# and overwrite the `.to_internal_value()` of the serializers to allow writting. +# +class NotificationSerializer(serializers.ModelSerializer): + class Meta: + model = Notification + exclude = [] diff --git a/readthedocs/api/v2/urls.py b/readthedocs/api/v2/urls.py index 9739ba8569b..42b354d00be 100644 --- 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 index aafd209ae5c..24dba342823 100644 --- 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,45 @@ 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 | ReadOnlyPermission] + 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_id = serializer.validated_data["attached_to_id"] + attached_to_content_type = serializer.validated_data["attached_to_content_type"] + + # Limit the checks to Build objects only for now + # content_type = ContentType.objects.get(pk=attached_to_content_type) + if not attached_to_content_type.name == "build": + raise Http404() + + # Limit the permissions to create a notification on this object only if the API key + # is attached to the related project + build = attached_to_content_type.get_object_for_this_type(pk=attached_to_id) + build_api_key = self.request.build_api_key + if not build_api_key.project.slug == build.project.slug: + raise PermissionDenied() + + return super().perform_create(serializer) + + # def get_queryset_for_api_key(self, api_key): + # return self.model.objects.filter(build__project=api_key.project) + + class DomainViewSet(DisableListEndpoint, UserSelectViewSet): permission_classes = [ReadOnlyPermission] renderer_classes = (JSONRenderer,) diff --git a/readthedocs/api/v3/urls.py b/readthedocs/api/v3/urls.py index 57c400c90b9..bcf4f8be69f 100644 --- a/readthedocs/api/v3/urls.py +++ b/readthedocs/api/v3/urls.py @@ -3,7 +3,6 @@ BuildsCreateViewSet, BuildsViewSet, EnvironmentVariablesViewSet, - NotificationsCreateViewSet, NotificationsViewSet, ProjectsViewSet, RedirectsViewSet, @@ -71,14 +70,8 @@ parents_query_lookups=["project__slug"], ) -# allows /api/v3/projects/pip/builds/1053/notifications/ -builds.register( - r"notifications", - NotificationsCreateViewSet, - basename="project-builds-notifications", - parents_query_lookups=["project__slug", "build__id"], -) - +# NOTE: we are only listing notifications for now. +# The front-end will use this endpoint. # allows /api/v3/projects/pip/builds/1053/notifications/ builds.register( r"notifications", From 3118eea3f971ae1f540d5a3e4373f82ae6f2444e Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 28 Nov 2023 17:49:40 +0100 Subject: [PATCH 010/101] Use APIv2 to create notifications from the builder --- readthedocs/api/v2/serializers.py | 53 ++++++++++++++++++++++--- readthedocs/api/v2/views/model_views.py | 16 ++------ readthedocs/notifications/messages.py | 3 ++ readthedocs/settings/base.py | 1 + requirements/deploy.txt | 3 ++ requirements/docker.txt | 3 ++ requirements/pip.in | 1 + requirements/pip.txt | 3 ++ requirements/testing.txt | 3 ++ 9 files changed, 69 insertions(+), 17 deletions(-) diff --git a/readthedocs/api/v2/serializers.py b/readthedocs/api/v2/serializers.py index 2ded9bbfd09..560ecc440d2 100644 --- a/readthedocs/api/v2/serializers.py +++ b/readthedocs/api/v2/serializers.py @@ -2,6 +2,9 @@ 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 @@ -381,11 +384,51 @@ def get_username(self, obj): ) -# TODO: consider using a third party library. -# See https://github.com/LilyFoote/rest-framework-generic-relations -# and overwrite the `.to_internal_value()` of the serializers to allow writting. -# +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 "/". + # 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 = [] + exclude = ["attached_to_id", "attached_to_content_type"] diff --git a/readthedocs/api/v2/views/model_views.py b/readthedocs/api/v2/views/model_views.py index 24dba342823..96536501c42 100644 --- a/readthedocs/api/v2/views/model_views.py +++ b/readthedocs/api/v2/views/model_views.py @@ -381,26 +381,18 @@ class NotificationViewSet(DisableListEndpoint, CreateModelMixin, UserSelectViewS def perform_create(self, serializer): """Restrict creation to notifications attached to the project's builds from the api key.""" - attached_to_id = serializer.validated_data["attached_to_id"] - attached_to_content_type = serializer.validated_data["attached_to_content_type"] - - # Limit the checks to Build objects only for now - # content_type = ContentType.objects.get(pk=attached_to_content_type) - if not attached_to_content_type.name == "build": - raise Http404() + attached_to = serializer.validated_data["attached_to"] # Limit the permissions to create a notification on this object only if the API key # is attached to the related project - build = attached_to_content_type.get_object_for_this_type(pk=attached_to_id) + # + # TODO: get the Project object from any ``attached_to`` object build_api_key = self.request.build_api_key - if not build_api_key.project.slug == build.project.slug: + if not build_api_key.project.slug == attached_to.project.slug: raise PermissionDenied() return super().perform_create(serializer) - # def get_queryset_for_api_key(self, api_key): - # return self.model.objects.filter(build__project=api_key.project) - class DomainViewSet(DisableListEndpoint, UserSelectViewSet): permission_classes = [ReadOnlyPermission] diff --git a/readthedocs/notifications/messages.py b/readthedocs/notifications/messages.py index 64051cb8681..1238072c9fa 100644 --- a/readthedocs/notifications/messages.py +++ b/readthedocs/notifications/messages.py @@ -4,6 +4,9 @@ class Message: + # TODO: consider inheriting this class as BuildMessage, SubscriptionMessage, etc + GENERIC_WITH_MESSAGE_ID = "generic-with-message-id" + def __init__(self, id, header, body, type, icon=None, icon_style=SOLID): self.id = id self.header = header diff --git a/readthedocs/settings/base.py b/readthedocs/settings/base.py index 90bc763f4aa..7eacb783740 100644 --- a/readthedocs/settings/base.py +++ b/readthedocs/settings/base.py @@ -224,6 +224,7 @@ def INSTALLED_APPS(self): # noqa 'rest_framework', 'rest_framework.authtoken', "rest_framework_api_key", + "generic_relations", 'corsheaders', 'annoying', 'django_extensions', diff --git a/requirements/deploy.txt b/requirements/deploy.txt index 402ab51e88a..5bbb617fb9d 100644 --- a/requirements/deploy.txt +++ b/requirements/deploy.txt @@ -172,6 +172,7 @@ djangorestframework==3.14.0 # via # -r requirements/pip.txt # drf-extensions + # rest-framework-generic-relations djangorestframework-api-key==3.0.0 # via -r requirements/pip.txt djangorestframework-jsonp==1.0.2 @@ -332,6 +333,8 @@ requests-oauthlib==1.3.1 # django-allauth requests-toolbelt==1.0.0 # via -r requirements/pip.txt +rest-framework-generic-relations==2.1.0 + # via -r requirements/pip.txt s3transfer==0.7.0 # via # -r requirements/pip.txt diff --git a/requirements/docker.txt b/requirements/docker.txt index 4258906dd8b..ad872597a86 100644 --- a/requirements/docker.txt +++ b/requirements/docker.txt @@ -183,6 +183,7 @@ djangorestframework==3.14.0 # via # -r requirements/pip.txt # drf-extensions + # rest-framework-generic-relations djangorestframework-api-key==3.0.0 # via -r requirements/pip.txt djangorestframework-jsonp==1.0.2 @@ -365,6 +366,8 @@ requests-oauthlib==1.3.1 # django-allauth requests-toolbelt==1.0.0 # via -r requirements/pip.txt +rest-framework-generic-relations==2.1.0 + # via -r requirements/pip.txt rich==13.7.0 # via -r requirements/docker.in s3transfer==0.7.0 diff --git a/requirements/pip.in b/requirements/pip.in index fb7dc05cf6b..c01548edc43 100644 --- a/requirements/pip.in +++ b/requirements/pip.in @@ -17,6 +17,7 @@ django-simple-history==3.0.0 djangorestframework djangorestframework-api-key +rest-framework-generic-relations # Embed v1 still uses docutils. # Remove this dependency once we deprecate this API. diff --git a/requirements/pip.txt b/requirements/pip.txt index 1e1614cb51e..92da5f83fb7 100644 --- a/requirements/pip.txt +++ b/requirements/pip.txt @@ -129,6 +129,7 @@ djangorestframework==3.14.0 # via # -r requirements/pip.in # drf-extensions + # rest-framework-generic-relations djangorestframework-api-key==3.0.0 # via -r requirements/pip.in djangorestframework-jsonp==1.0.2 @@ -240,6 +241,8 @@ requests-oauthlib==1.3.1 # django-allauth requests-toolbelt==1.0.0 # via -r requirements/pip.in +rest-framework-generic-relations==2.1.0 + # via -r requirements/pip.in s3transfer==0.7.0 # via boto3 selectolax==0.3.17 diff --git a/requirements/testing.txt b/requirements/testing.txt index 6f3d47b0698..d71f6d4e586 100644 --- a/requirements/testing.txt +++ b/requirements/testing.txt @@ -177,6 +177,7 @@ djangorestframework==3.14.0 # via # -r requirements/pip.txt # drf-extensions + # rest-framework-generic-relations djangorestframework-api-key==3.0.0 # via -r requirements/pip.txt djangorestframework-jsonp==1.0.2 @@ -355,6 +356,8 @@ requests-oauthlib==1.3.1 # django-allauth requests-toolbelt==1.0.0 # via -r requirements/pip.txt +rest-framework-generic-relations==2.1.0 + # via -r requirements/pip.txt s3transfer==0.7.0 # via # -r requirements/pip.txt From 03ca9a405b8a1b4d8467ecae0b17d23b59cd0659 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 29 Nov 2023 10:07:31 +0100 Subject: [PATCH 011/101] Check project slug when attaching to a Build/Project instance --- readthedocs/api/v2/views/model_views.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/readthedocs/api/v2/views/model_views.py b/readthedocs/api/v2/views/model_views.py index 96536501c42..ee6df9f39cd 100644 --- a/readthedocs/api/v2/views/model_views.py +++ b/readthedocs/api/v2/views/model_views.py @@ -383,12 +383,16 @@ 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 + + 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 - # - # TODO: get the Project object from any ``attached_to`` object - build_api_key = self.request.build_api_key - if not build_api_key.project.slug == attached_to.project.slug: + if not project_slug or not build_api_key.project.slug == project_slug: raise PermissionDenied() return super().perform_create(serializer) From dd8fb312f96683c000d15668e3948c2f4f9bd1be Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 29 Nov 2023 10:08:01 +0100 Subject: [PATCH 012/101] Add extra TODO comments --- readthedocs/api/v3/urls.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/readthedocs/api/v3/urls.py b/readthedocs/api/v3/urls.py index bcf4f8be69f..528f576e0f1 100644 --- a/readthedocs/api/v3/urls.py +++ b/readthedocs/api/v3/urls.py @@ -70,7 +70,7 @@ parents_query_lookups=["project__slug"], ) -# NOTE: we are only listing notifications for now. +# 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( @@ -80,6 +80,11 @@ 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( From 514c8f9f27ecd19600f718d5fb228452f1196bcc Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 29 Nov 2023 15:01:32 +0100 Subject: [PATCH 013/101] Initial migration from Exceptions to Messages --- readthedocs/api/v3/serializers.py | 1 + readthedocs/builds/views.py | 4 +- .../core/tests/test_filesystem_utils.py | 3 +- readthedocs/core/utils/__init__.py | 1 + readthedocs/core/utils/filesystem.py | 4 +- readthedocs/doc_builder/backends/sphinx.py | 3 +- readthedocs/doc_builder/environments.py | 6 +- readthedocs/doc_builder/exceptions.py | 176 ++------- readthedocs/notifications/messages.py | 367 +++++++++++++++--- readthedocs/notifications/models.py | 22 +- readthedocs/projects/tasks/builds.py | 83 ++-- .../templates/builds/build_detail.html | 7 + readthedocs/vcs_support/base.py | 2 +- 13 files changed, 440 insertions(+), 239 deletions(-) diff --git a/readthedocs/api/v3/serializers.py b/readthedocs/api/v3/serializers.py index a09814dfe5f..6b1ecabe2e0 100644 --- a/readthedocs/api/v3/serializers.py +++ b/readthedocs/api/v3/serializers.py @@ -225,6 +225,7 @@ def get_success(self, obj): class NotificationMessageSerializer(serializers.Serializer): id = serializers.SlugField() + # TODO: how do we render these string formatted with the Notifcation instance from here? header = serializers.CharField() body = serializers.CharField() type = serializers.CharField() diff --git a/readthedocs/builds/views.py b/readthedocs/builds/views.py index beb4fcbbab3..2a39ee282fa 100644 --- a/readthedocs/builds/views.py +++ b/readthedocs/builds/views.py @@ -171,7 +171,9 @@ def get_context_data(self, **kwargs): build = self.get_object() - if build.error != BuildAppError.GENERIC_WITH_BUILD_ID.format(build_id=build.pk): + if not build.notifications.filter( + message_id=BuildAppError.GENERIC_WITH_BUILD_ID + ).exists(): # Do not suggest to open an issue if the error is not generic return context diff --git a/readthedocs/core/tests/test_filesystem_utils.py b/readthedocs/core/tests/test_filesystem_utils.py index 6afe31edd51..0f8e4f040e7 100644 --- a/readthedocs/core/tests/test_filesystem_utils.py +++ b/readthedocs/core/tests/test_filesystem_utils.py @@ -7,7 +7,6 @@ from readthedocs.core.utils.filesystem import safe_copytree, safe_open, safe_rmtree from readthedocs.doc_builder.exceptions import ( - FileTooLarge, SymlinkOutsideBasePath, UnsupportedSymlinkFileError, ) @@ -110,7 +109,7 @@ def test_open_large_file(self): file_a.write_bytes(b"0" * (1024 * 2)) with override_settings(DOCROOT=docroot_path): - with pytest.raises(FileTooLarge): + with pytest.raises(BuildUserError): safe_open(file_a, max_size_bytes=1024) def test_write_file(self): diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index f7b821f11b2..d7363240f1a 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -153,6 +153,7 @@ def prepare_build( # and the user will be alerted in the UI from the Error below. options["countdown"] = settings.RTD_BUILDS_RETRY_DELAY options["max_retries"] = settings.RTD_BUILDS_MAX_RETRIES + # TODO: adapt this code to create a Notification on Build here. build.error = BuildMaxConcurrencyError.message.format( limit=max_concurrent_builds, ) diff --git a/readthedocs/core/utils/filesystem.py b/readthedocs/core/utils/filesystem.py index d98f7deb90d..51dad92fcec 100644 --- a/readthedocs/core/utils/filesystem.py +++ b/readthedocs/core/utils/filesystem.py @@ -13,8 +13,8 @@ from django.core.exceptions import SuspiciousFileOperation from readthedocs.doc_builder.exceptions import ( + BuildUserError, FileIsNotRegularFile, - FileTooLarge, SymlinkOutsideBasePath, UnsupportedSymlinkFileError, ) @@ -90,7 +90,7 @@ def safe_open( file_size = resolved_path.stat().st_size if file_size > max_size_bytes: log.info("File is too large.", size_bytes=file_size) - raise FileTooLarge() + raise BuildUserError(BuildUserError.FILE_TOO_LARGE) if allow_symlinks and base_path: base_path = Path(base_path).absolute() diff --git a/readthedocs/doc_builder/backends/sphinx.py b/readthedocs/doc_builder/backends/sphinx.py index 35c7d7503fd..0dc0e43a795 100644 --- a/readthedocs/doc_builder/backends/sphinx.py +++ b/readthedocs/doc_builder/backends/sphinx.py @@ -18,7 +18,6 @@ from readthedocs.builds import utils as version_utils from readthedocs.builds.models import APIVersion from readthedocs.core.utils.filesystem import safe_open -from readthedocs.doc_builder.exceptions import PDFNotFound from readthedocs.projects.constants import OLD_LANGUAGES_CODE_MAPPING, PUBLIC from readthedocs.projects.exceptions import ProjectConfigurationError, UserFileNotFound from readthedocs.projects.models import Feature @@ -581,7 +580,7 @@ def _post_build(self): """Internal post build to cleanup PDF output directory and leave only one .pdf file.""" if not self.pdf_file_name: - raise PDFNotFound() + raise BuildUserError(BuildUserError.PDF_NOT_FOUND) # TODO: merge this with ePUB since it's pretty much the same temp_pdf_file = f"/tmp/{self.project.slug}-{self.version.slug}.pdf" diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index 609fc234abc..5cfaad9d5d2 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -31,7 +31,7 @@ DOCKER_VERSION, RTD_SKIP_BUILD_EXIT_CODE, ) -from .exceptions import BuildAppError, BuildUserError, BuildUserSkip +from .exceptions import BuildAppError, BuildUserError log = structlog.get_logger(__name__) @@ -527,13 +527,13 @@ def run_command_class( version_slug=self.version.slug if self.version else "", ) elif build_cmd.exit_code == RTD_SKIP_BUILD_EXIT_CODE: - raise BuildUserSkip() + raise BuildAppError(BuildAppError.SKIPPED_EXIT_CODE_183) else: # TODO: for now, this still outputs a generic error message # that is the same across all commands. We could improve this # with more granular error messages that vary by the command # being run. - raise BuildUserError() + raise BuildUserError(BuildUserError.GENERIC) return build_cmd diff --git a/readthedocs/doc_builder/exceptions.py b/readthedocs/doc_builder/exceptions.py index 9597fb9e7ce..ee082604eff 100644 --- a/readthedocs/doc_builder/exceptions.py +++ b/readthedocs/doc_builder/exceptions.py @@ -1,155 +1,70 @@ -"""Exceptions raised when building documentation.""" +""" +Exceptions raised when building documentation. -from django.utils.translation import gettext_noop +Exceptions defined here have different categories based on -from readthedocs.builds.constants import BUILD_STATE_CANCELLED -from readthedocs.projects.constants import BUILD_COMMANDS_OUTPUT_PATH_HTML + - responsibility (user or application) + - special cases (they need to be handled in a special way, e.g. concurrency limit reached) + - grouped by topic (e.g. MkDocs errors) +All these exception should only define the "message id" under one of these categories. +Then the header/body texts should be defined in ``readthedocs/notifications/messages.py``. +""" -class BuildBaseException(Exception): - message = None - status_code = None +from django.utils.translation import gettext_noop - def __init__(self, message=None, **kwargs): - self.status_code = ( - kwargs.pop( - "status_code", - None, - ) - or self.status_code - or 1 - ) - self.message = message or self.message or self.get_default_message() - super().__init__(message, **kwargs) - def get_default_message(self): - return self.message +class BuildBaseException(Exception): + + def __init__(self, message_id, **kwargs): + self.message_id = message_id + super().__init__("Build exception", **kwargs) class BuildAppError(BuildBaseException): - GENERIC_WITH_BUILD_ID = gettext_noop( - "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 ({build_id}).", - ) + GENERIC_WITH_BUILD_ID = "build:app:generic-with-build-id" + BUILDS_DISABLED = "build:app:project-builds-disabled" class BuildUserError(BuildBaseException): - GENERIC = gettext_noop( - "We encountered a problem with a command while building your project. " - "To resolve this error, double check your project configuration and installed " - "dependencies are correct and have not changed recently." - ) - BUILD_COMMANDS_WITHOUT_OUTPUT = gettext_noop( - f'No "{BUILD_COMMANDS_OUTPUT_PATH_HTML}" folder was created during this build.' - ) - BUILD_OUTPUT_IS_NOT_A_DIRECTORY = gettext_noop( - 'Build output directory for format "{artifact_type}" is not a directory.' - ) - BUILD_OUTPUT_HAS_0_FILES = gettext_noop( - 'Build output directory for format "{artifact_type}" does not contain any files. ' - "It seems the build process created the directory but did not save any file to it." - ) - BUILD_OUTPUT_HAS_MULTIPLE_FILES = gettext_noop( - 'Build output directory for format "{artifact_type}" contains multiple files ' - "and it is not currently supported. " - 'Please, remove all the files but the "{artifact_type}" you want to upload.' - ) - BUILD_OUTPUT_HTML_NO_INDEX_FILE = gettext_noop( - "Your documentation did not generate an 'index.html' at its root directory. " - "This is required for documentation serving at the root URL for this version." - ) - BUILD_OUTPUT_OLD_DIRECTORY_USED = gettext_noop( - "Some files were detected in an unsupported output path, '_build/html'. " - "Ensure your project is configured to use the output path " - "'$READTHEDOCS_OUTPUT/html' instead." - ) - NO_CONFIG_FILE_DEPRECATED = gettext_noop( - "The configuration file required to build documentation is missing from your project. " - "Add a configuration file to your project to make it build successfully. " - "Read more at https://docs.readthedocs.io/en/stable/config-file/v2.html" - ) - BUILD_IMAGE_CONFIG_KEY_DEPRECATED = gettext_noop( - 'The configuration key "build.image" is deprecated. ' - 'Use "build.os" instead to continue building your project. ' - "Read more at https://docs.readthedocs.io/en/stable/config-file/v2.html#build-os" - ) - BUILD_OS_REQUIRED = gettext_noop( - 'The configuration key "build.os" is required to build your documentation. ' - "Read more at https://docs.readthedocs.io/en/stable/config-file/v2.html#build-os" - ) - - -class BuildUserSkip(BuildUserError): - message = gettext_noop("This build was manually skipped using a command exit code.") - state = BUILD_STATE_CANCELLED - - -class ProjectBuildsSkippedError(BuildUserError): - message = gettext_noop("Builds for this project are temporarily disabled") - - -class YAMLParseError(BuildUserError): - GENERIC_WITH_PARSE_EXCEPTION = gettext_noop( - "Problem in your project's configuration. {exception}", - ) + GENERIC = "build:user:generic" + SKIPPED_EXIT_CODE_183 = "build:user:exit-code-183" + MAX_CONCURRENCY = "build:user:max-concurrency" + BUILD_COMMANDS_WITHOUT_OUTPUT = "build:user:output:no-html" + BUILD_OUTPUT_IS_NOT_A_DIRECTORY = "build:user:output:is-no-a-directory" + BUILD_OUTPUT_HAS_0_FILES = "build:user:output:has-0-files" + BUILD_OUTPUT_HAS_MULTIPLE_FILES = "build:user:output:has-multiple-files" + BUILD_OUTPUT_HTML_NO_INDEX_FILE = "build:user:output:html-no-index-file" + BUILD_OUTPUT_OLD_DIRECTORY_USED = "build:user:output:old-directory-used" + FILE_TOO_LARGE = "build:user:output:file-too-large" -class BuildMaxConcurrencyError(BuildUserError): - message = gettext_noop( - "Concurrency limit reached ({limit}), retrying in 5 minutes." - ) + NO_CONFIG_FILE_DEPRECATED = "build:user:config:no-config-file" + BUILD_IMAGE_CONFIG_KEY_DEPRECATED = "build:user:config:build-image-deprecated" + BUILD_OS_REQUIRED = "build:user:config:build-os-required" -class BuildCancelled(BuildUserError): - message = gettext_noop("Build cancelled by user.") - state = BUILD_STATE_CANCELLED +class BuildMaxConcurrencyError(BuildUserError): + LIMIT_REACHED = "build:user:concurrency-limit-reached" -class PDFNotFound(BuildUserError): - message = gettext_noop( - 'PDF file was not generated/found in "_readthedocs/pdf" output directory.' - ) +class BuildCancelled(BuildUserError): + CANCELLED_BY_USER = "build:user:cancelled" class MkDocsYAMLParseError(BuildUserError): - GENERIC_WITH_PARSE_EXCEPTION = gettext_noop( - "Problem parsing MkDocs YAML configuration. {exception}", - ) - - INVALID_DOCS_DIR_CONFIG = gettext_noop( - 'The "docs_dir" config from your MkDocs YAML config file has to be a ' - "string with relative or absolute path.", - ) - - INVALID_DOCS_DIR_PATH = gettext_noop( - 'The "docs_dir" config from your MkDocs YAML config file does not ' - "contain a valid path.", - ) - - INVALID_EXTRA_CONFIG = gettext_noop( - 'The "{config}" config from your MkDocs YAML config file has to be a ' - "list of relative paths.", - ) - - EMPTY_CONFIG = gettext_noop( - "Please make sure the MkDocs YAML configuration file is not empty.", - ) - NOT_FOUND = gettext_noop( - "A configuration file was not found. " - 'Make sure you have a "mkdocs.yml" file in your repository.', - ) - - CONFIG_NOT_DICT = gettext_noop( - "Your MkDocs YAML config file is incorrect. " - "Please follow the user guide https://www.mkdocs.org/user-guide/configuration/ " - "to configure the file properly.", - ) + GENERIC_WITH_PARSE_EXCEPTION = "build:user:mkdocs:yaml-parse" + INVALID_DOCS_DIR_CONFIG = "build:user:mkdocs:invalid-dir-config" + INVALID_DOCS_DIR_PATH = "build:user:mkdocs:invalid-dir-path" + INVALID_EXTRA_CONFIG = "build:user:mkdocs:invalid-extra-config" + EMPTY_CONFIG = "build:user:mkdocs:empty-config" + NOT_FOUND = "build:user:mkdocs:config-not-found" + CONFIG_NOT_DICT = "build:user:mkdocs:invalid-yaml" # TODO: improve messages for symlink errors with a more detailed error and include the `filepath`. class UnsupportedSymlinkFileError(BuildUserError): + SYMLINK_USED = "build:user:symlink:used" message = gettext_noop("Symlinks are not fully supported") @@ -159,10 +74,3 @@ class FileIsNotRegularFile(UnsupportedSymlinkFileError): class SymlinkOutsideBasePath(UnsupportedSymlinkFileError): pass - - -class FileTooLarge(BuildUserError): - message = gettext_noop( - "A file from your build process is too large to be processed by Read the Docs. " - "Please ensure no files generated are larger than 1GB." - ) diff --git a/readthedocs/notifications/messages.py b/readthedocs/notifications/messages.py index 1238072c9fa..599d966db17 100644 --- a/readthedocs/notifications/messages.py +++ b/readthedocs/notifications/messages.py @@ -1,12 +1,18 @@ from django.utils.translation import gettext_noop as _ -from .constants import ERROR, SOLID, TIP, WARNING +from readthedocs.doc_builder.exceptions import ( + BuildAppError, + BuildCancelled, + BuildMaxConcurrencyError, + BuildUserError, + MkDocsYAMLParseError, +) +from readthedocs.projects.constants import BUILD_COMMANDS_OUTPUT_PATH_HTML +from .constants import ERROR, INFO, SOLID, WARNING -class Message: - # TODO: consider inheriting this class as BuildMessage, SubscriptionMessage, etc - GENERIC_WITH_MESSAGE_ID = "generic-with-message-id" +class Message: def __init__(self, id, header, body, type, icon=None, icon_style=SOLID): self.id = id self.header = header @@ -25,52 +31,309 @@ def get_display_icon(self): return "fa-triangle-exclamation" -NOTIFICATION_MESSAGES = { - "generic-with-build-id": Message( - id="generic-with-build-id", - 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( - id="build-os-required", - header=_("Invalid configuration"), - body=_( - """ - The configuration key "build.os" is required to build your documentation. - Read more. - """ - ), - type=ERROR, - ), - "cancelled-by-user": Message( - id="cancelled-by-user", - header=_("User action"), - body=_( - """ - Build cancelled by the user. - """ - ), - type=ERROR, - ), - "os-ubuntu-18.04-deprecated": Message( - id="os-ubuntu-18.04-deprecated", - 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, - ), +CONFIG_YAML_MESSAGES = { + message.id: message + for message in [ + Message( + id=f"build:config-yaml:generic", + header=_("Error while parsing configuration file."), + body=_("Take a look at your YAML file looking for syntax errors."), + type=ERROR, + ) + ] +} + +# TODO: review the copy of these notifications/messages on PR review and adapt them. +# Most of them are copied from what we had in `readthedocs.doc_builder.exceptions` +# and slightly adapted to have a header and a better body. +BUILD_MESSAGES = { + message.id: message + for message in [ + Message( + id=BuildAppError.GENERIC_WITH_BUILD_ID, + 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, + ), + Message( + id=BuildUserError.GENERIC, + header=_("Unknown problem"), + body=_( + """ + We encountered a problem with a command while building your project. + To resolve this error, double check your project configuration and installed + dependencies are correct and have not changed recently. + """ + ), + type=ERROR, + ), + Message( + id=BuildMaxConcurrencyError.LIMIT_REACHED, + header=_("Maximum concurrency limit reached."), + # TODO: how we are going to format variables (e.g. ``limit`` here)? + # The variables are passed when it's instantiated. + # However, we need to render the notification from the front-end in a different moment. + # Do we want to store the key/values in the database and use them to render the message? + body=_( + """ + Concurrency limit reached ({limit}), retrying in 5 minutes. + """ + ), + type=INFO, + ), + Message( + id=BuildCancelled.CANCELLED_BY_USER, + header=_("Build cancelled manually."), + body=_( + """ + The user has cancel this build. + """ + ), + type=INFO, + ), + Message( + id=BuildUserError.SKIPPED_EXIT_CODE_183, + header=_("Build skipped manually."), + body=_( + """ + One of the commands exited with code 183 + and the build was skipped. + """ + ), + type=INFO, + ), + Message( + id=BuildAppError.BUILDS_DISABLED, + header=_("Builds are temporary disabled for this project."), + body=_( + """ + This is due to an excess usage of our resources. + Please, contact our support team if you think this is a mistake + and builds should be re-enabled. + """ + ), + type=ERROR, + ), + Message( + id=BuildUserError.MAX_CONCURRENCY, + header=_("Concurrency limit reached"), + body=_( + """ + Your project/organization/user is currently building the maximum concurrency builds allowed ({limit}). + It will automatically retried in 5 minutes. + """ + ), + type=WARNING, + ), + Message( + id=BuildUserError.BUILD_COMMANDS_WITHOUT_OUTPUT, + header=_("No HTML content found"), + body=_( + f""" + No "{BUILD_COMMANDS_OUTPUT_PATH_HTML}" folder was created during this build. + """ + ), + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_OUTPUT_IS_NOT_A_DIRECTORY, + header=_("Build output directory is not a directory"), + body=_( + """ + Build output directory for format "{artifact_type}" is not a directory. + """ + ), + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_OUTPUT_HAS_0_FILES, + header=_("Build output directory doesn't contain any file"), + body=_( + """ + Build output directory for format "{artifact_type}" does not contain any files. + It seems the build process created the directory but did not save any file to it. + """ + ), + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_OUTPUT_HAS_MULTIPLE_FILES, + header=_("Build output directory contains multiple files"), + body=_( + """ + Build output directory for format "{artifact_type}" contains multiple files + and it is not currently supported. + Please, remove all the files but the "{artifact_type}" you want to upload. + """ + ), + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_OUTPUT_HTML_NO_INDEX_FILE, + header=_("Index file is not present in HTML output directory"), + body=_( + """ + Your documentation did not generate an 'index.html' at its root directory. + This is required for documentation serving at the root URL for this version. + """ + ), + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_OUTPUT_OLD_DIRECTORY_USED, + header=_("Your project is outputing files in an old directory"), + body=_( + """ + Some files were detected in an unsupported output path, '_build/html'. + Ensure your project is configured to use the output path + '$READTHEDOCS_OUTPUT/html' instead. + """ + ), + type=ERROR, + ), + Message( + id=BuildUserError.NO_CONFIG_FILE_DEPRECATED, + header=_("Your project doesn't have a .readthedocs.yaml file"), + body=_( + """ + The configuration file required to build documentation is missing from your project. + Add a configuration file to your project to make it build successfully. + Read more at https://docs.readthedocs.io/en/stable/config-file/v2.html + """ + ), + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_IMAGE_CONFIG_KEY_DEPRECATED, + header=_("Configuration key build.image is deprecated"), + body=_( + """ + The configuration key "build.image" is deprecated. + Use "build.os" instead to continue building your project. + Read more at https://docs.readthedocs.io/en/stable/config-file/v2.html#build-os + """ + ), + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_OS_REQUIRED, + header=_("Configuration key build.os is required"), + body=_( + """ + The configuration key "build.os" is required to build your documentation. + Read more at https://docs.readthedocs.io/en/stable/config-file/v2.html#build-os + """ + ), + type=ERROR, + ), + Message( + id=BuildUserError.FILE_TOO_LARGE, + header=_("There is at least one file size that exceeds the limits"), + body=_( + """ + A file from your build process is too large to be processed by Read the Docs. + Please ensure no files generated are larger than 1GB. + """ + ), + type=ERROR, + ), + Message( + id=BuildUserError.FILE_TOO_LARGE, + header=_("There is no PDF file in output directory"), + body=_( + f""" + PDF file was not generated/found in "{BUILD_COMMANDS_OUTPUT_PATH_HTML}/pdf" output directory. + """ + ), + type=ERROR, + ), + # TODO: it probably makes sense to split the notification object here. + Message( + id=MkDocsYAMLParseError.GENERIC_WITH_PARSE_EXCEPTION, + header=_(""), + body=_( + """ + Problem parsing MkDocs YAML configuration. {exception} + """ + ), + type=ERROR, + ), + Message( + id=MkDocsYAMLParseError.INVALID_DOCS_DIR_CONFIG, + header=_(""), + body=_( + """ + The "docs_dir" config from your MkDocs YAML config file has to be a + string with relative or absolute path. + """ + ), + type=ERROR, + ), + Message( + id=MkDocsYAMLParseError.INVALID_DOCS_DIR_PATH, + header=_(""), + body=_( + """ + """ + ), + type=ERROR, + ), + Message( + id=MkDocsYAMLParseError.INVALID_EXTRA_CONFIG, + header=_(""), + body=_( + """ + The "{config}" config from your MkDocs YAML config file has to be a + list of relative paths. + """ + ), + type=ERROR, + ), + Message( + id=MkDocsYAMLParseError.EMPTY_CONFIG, + header=_(""), + body=_( + """ + Please make sure the MkDocs YAML configuration file is not empty. + """ + ), + type=ERROR, + ), + Message( + id=MkDocsYAMLParseError.NOT_FOUND, + header=_(""), + body=_( + """ + A configuration file was not found. + Make sure you have a "mkdocs.yml" file in your repository. + """ + ), + type=ERROR, + ), + Message( + id=MkDocsYAMLParseError.CONFIG_NOT_DICT, + header=_(""), + body=_( + """ + Your MkDocs YAML config file is incorrect. + Please follow the user guide https://www.mkdocs.org/user-guide/configuration/ + to configure the file properly. + """ + ), + type=ERROR, + ), + ] } + +NOTIFICATION_MESSAGES = {} +NOTIFICATION_MESSAGES.update(CONFIG_YAML_MESSAGES) +NOTIFICATION_MESSAGES.update(BUILD_MESSAGES) diff --git a/readthedocs/notifications/models.py b/readthedocs/notifications/models.py index 8e3c30ce0d8..c2515b6f44e 100644 --- a/readthedocs/notifications/models.py +++ b/readthedocs/notifications/models.py @@ -45,15 +45,33 @@ class Notification(TimeStampedModel): attached_to_id = models.PositiveIntegerField() attached_to = GenericForeignKey("attached_to_content_type", "attached_to_id") + # Store values known at creation time that are required to render the final message + format_values = models.JSONField(null=True, blank=True) + def __str__(self): return self.message_id def get_message(self): return NOTIFICATION_MESSAGES.get(self.message_id) - def get_display_message(self): + def get_header(self): + return NOTIFICATION_MESSAGES.get(self.message_id).header + + def get_display_header(self): + return textwrap.dedent( + NOTIFICATION_MESSAGES.get(self.message_id).header.format( + instance=self.attached_to, # Build, Project, Organization, User + **self.format_values, + ) + ) + + def get_body(self): + return NOTIFICATION_MESSAGES.get(self.message_id).body + + def get_display_body(self): return textwrap.dedent( - NOTIFICATION_MESSAGES.get(self.message_id).format( + NOTIFICATION_MESSAGES.get(self.message_id).body.format( instance=self.attached_to, # Build, Project, Organization, User + **self.format_values, ) ) diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index 19a0e508a85..439ca572498 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/readthedocs/projects/tasks/builds.py @@ -49,10 +49,7 @@ BuildCancelled, BuildMaxConcurrencyError, BuildUserError, - BuildUserSkip, MkDocsYAMLParseError, - ProjectBuildsSkippedError, - YAMLParseError, ) from readthedocs.projects.models import Feature from readthedocs.storage import build_media_storage @@ -292,12 +289,9 @@ class UpdateDocsTask(SyncRepositoryMixin, Task): # All exceptions generated by a user miss-configuration should be listed # here. Actually, every subclass of ``BuildUserError``. throws = ( - ProjectBuildsSkippedError, ConfigError, - YAMLParseError, BuildCancelled, BuildUserError, - BuildUserSkip, RepositoryError, MkDocsYAMLParseError, ProjectConfigurationError, @@ -306,9 +300,11 @@ class UpdateDocsTask(SyncRepositoryMixin, Task): # Do not send notifications on failure builds for these exceptions. exceptions_without_notifications = ( BuildCancelled, - BuildMaxConcurrencyError, - BuildUserSkip, - ProjectBuildsSkippedError, + # BuildUserError.MAX_CONCURRENCY, + BuildUserError.SKIPPED_EXIT_CODE_183, + # TODO: use BuildAppError.BUILDS_DISABLED (message_id's) here instead of specific exceptions. + # There is no need to have multiple different exceptions when only the message changes. + BuildAppError.BUILDS_DISABLED, ) # Do not send external build status on failure builds for these exceptions. @@ -339,7 +335,7 @@ def sigint_received(*args, **kwargs): log.warning('Ignoring cancelling the build at "Uploading" state.') return - raise BuildCancelled + raise BuildCancelled(BuildCancelled.CANCELLED_BY_USER) # Do not send the SIGTERM signal to children (pip is automatically killed when # receives SIGTERM and make the build to fail one command and stop build) @@ -373,16 +369,15 @@ def _check_concurrency_limit(self): log.info("Concurrency limit reached, retrying task.") self.retry( exc=BuildMaxConcurrencyError( - BuildMaxConcurrencyError.message.format( - limit=max_concurrent_builds, - ) + BuildMaxConcurrencyError.LIMIT_REACHED, + limit=max_concurrent_builds, ) ) def _check_project_disabled(self): if self.data.project.skip: log.warning('Project build skipped.') - raise ProjectBuildsSkippedError + raise BuildAppError(BuildAppError.BUILDS_DISABLED) def before_start(self, task_id, args, kwargs): # Create the object to store all the task-related data @@ -478,43 +473,51 @@ def on_failure(self, exc, task_id, args, kwargs, einfo): # TODO: handle this `ConfigError` as a `BuildUserError` in the # following block if isinstance(exc, ConfigError): - self.data.build['error'] = str( - YAMLParseError( - YAMLParseError.GENERIC_WITH_PARSE_EXCEPTION.format( - exception=str(exc), - ), - ), - ) + message_id = "build:config-yaml:generic" + # Known errors in our application code (e.g. we couldn't connect to # Docker API). Report a generic message to the user. elif isinstance(exc, BuildAppError): - self.data.build['error'] = BuildAppError.GENERIC_WITH_BUILD_ID.format( - build_id=self.data.build['id'], - ) + message_id = "build:app:generic-with-build-id" + # Known errors in the user's project (e.g. invalid config file, invalid # repository, command failed, etc). Report the error back to the user - # using the `message` and `state` attributes from the exception itself. - # Otherwise, use a generic message and default state. + # by creating a notification attached to the build + # Otherwise, use a notification with a generic message. elif isinstance(exc, BuildUserError): - if hasattr(exc, 'message') and exc.message is not None: - self.data.build['error'] = exc.message + # TODO: we are using APIv2 here :/ + # We just can't migrate everything to APIv3 -- that would be a bigger project + + if hasattr(exc, "message_id") and exc.message_id is not None: + message_id = exc.message_id else: - self.data.build['error'] = BuildUserError.GENERIC + message_id = "build:user:generic" - if hasattr(exc, "state"): - self.data.build["state"] = exc.state else: # We don't know what happened in the build. Log the exception and - # report a generic message to the user. + # report a generic notification to the user. # Note we are using `log.error(exc_info=...)` instead of `log.exception` # because this is not executed inside a try/except block. log.error("Build failed with unhandled exception.", exc_info=exc) - self.data.build["error"] = BuildAppError.GENERIC_WITH_BUILD_ID.format( - build_id=self.data.build["id"], - ) + message_id = "build:app:generic-with-build-id" + + # TODO: finish sending "format_values" via the API + exc.kwargs if hasattr(exc, "kwargs") else None + + # POST the notification via the APIv2 + self.data.api_client.notifications.post( + { + "attached_to": f'build/{self.data.build["id"]}', + "message_id": message_id, + "state": "unread", # Optional + "dismissable": False, + "news": False, + # "format_values": format_values, + } + ) # Send notifications for unhandled errors - if not isinstance(exc, self.exceptions_without_notifications): + if message_id not in self.exceptions_without_notifications: self.send_notifications( self.data.version_pk, self.data.build['id'], @@ -535,7 +538,7 @@ def on_failure(self, exc, task_id, args, kwargs, einfo): version_type = self.data.version.type status = BUILD_STATUS_FAILURE - if isinstance(exc, BuildUserSkip): + if message_id == BuildUserError.SKIPPED_EXIT_CODE_183: # The build was skipped by returning the magic exit code, # marked as CANCELLED, but communicated to GitHub as successful. # This is because the PR has to be available for merging when the build @@ -594,9 +597,8 @@ def get_valid_artifact_types(self): output_format=artifact_type, ) raise BuildUserError( - BuildUserError.BUILD_OUTPUT_IS_NOT_A_DIRECTORY.format( - artifact_type=artifact_type - ) + BuildUserError.BUILD_OUTPUT_IS_NOT_A_DIRECTORY, + artifact_type=artifact_type, ) # Check if there are multiple files on artifact directories. @@ -694,6 +696,7 @@ def on_retry(self, exc, task_id, args, kwargs, einfo): project_slug=self.data.project.slug, version_slug=self.data.version.slug, ) + # TODO: update this code to add the notification to the Build instance self.data.build['error'] = exc.message self.update_build(state=BUILD_STATE_TRIGGERED) diff --git a/readthedocs/templates/builds/build_detail.html b/readthedocs/templates/builds/build_detail.html index 540c8806e54..ac8c4aaac0e 100644 --- a/readthedocs/templates/builds/build_detail.html +++ b/readthedocs/templates/builds/build_detail.html @@ -149,6 +149,13 @@ + {# Temporary link just for quick access until we implement the font-end in ext-theme #} +
+ See notifications for this Build +
+
+ + {% if request.user|is_admin:project %} {% if not build.success and "setup.py install" in build.commands.last.output %}
diff --git a/readthedocs/vcs_support/base.py b/readthedocs/vcs_support/base.py index 3304fefef24..df0bc82896a 100644 --- a/readthedocs/vcs_support/base.py +++ b/readthedocs/vcs_support/base.py @@ -106,7 +106,7 @@ def run(self, *cmd, **kwargs): # Catch ``BuildCancelled`` here and re raise it. Otherwise, if we # raise a ``RepositoryError`` then the ``on_failure`` method from # Celery won't treat this problem as a ``BuildCancelled`` issue. - raise BuildCancelled from exc + raise BuildCancelled(BuildCancelled.CANCELLED_BY_USER) from exc except BuildUserError as exc: # Re raise as RepositoryError to handle it properly from outside if hasattr(exc, "message"): From e9d0c02fc0421a08515486ec0382c9fdf1ce7574 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 29 Nov 2023 16:17:07 +0100 Subject: [PATCH 014/101] Disable `Notification.format_values` for now I'll come back to this later. --- readthedocs/notifications/models.py | 7 ++++--- readthedocs/projects/tasks/builds.py | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/readthedocs/notifications/models.py b/readthedocs/notifications/models.py index c2515b6f44e..ac128ae3cc1 100644 --- a/readthedocs/notifications/models.py +++ b/readthedocs/notifications/models.py @@ -45,8 +45,9 @@ class Notification(TimeStampedModel): attached_to_id = models.PositiveIntegerField() attached_to = GenericForeignKey("attached_to_content_type", "attached_to_id") + # TODO: create a migration for this field # Store values known at creation time that are required to render the final message - format_values = models.JSONField(null=True, blank=True) + # format_values = models.JSONField(null=True, blank=True) def __str__(self): return self.message_id @@ -61,7 +62,7 @@ def get_display_header(self): return textwrap.dedent( NOTIFICATION_MESSAGES.get(self.message_id).header.format( instance=self.attached_to, # Build, Project, Organization, User - **self.format_values, + # **self.format_values, ) ) @@ -72,6 +73,6 @@ def get_display_body(self): return textwrap.dedent( NOTIFICATION_MESSAGES.get(self.message_id).body.format( instance=self.attached_to, # Build, Project, Organization, User - **self.format_values, + # **self.format_values, ) ) diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index 439ca572498..590d7f57044 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/readthedocs/projects/tasks/builds.py @@ -502,7 +502,7 @@ def on_failure(self, exc, task_id, args, kwargs, einfo): message_id = "build:app:generic-with-build-id" # TODO: finish sending "format_values" via the API - exc.kwargs if hasattr(exc, "kwargs") else None + # format_values = exc.kwargs if hasattr(exc, "kwargs") else None # POST the notification via the APIv2 self.data.api_client.notifications.post( From 0c01d76b55f67f1317a38e271ce7fa05c22d0457 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 29 Nov 2023 16:17:49 +0100 Subject: [PATCH 015/101] Define all the notifications --- readthedocs/notifications/messages.py | 493 +++++++++++++------------- 1 file changed, 241 insertions(+), 252 deletions(-) diff --git a/readthedocs/notifications/messages.py b/readthedocs/notifications/messages.py index 599d966db17..1bcad9f0bac 100644 --- a/readthedocs/notifications/messages.py +++ b/readthedocs/notifications/messages.py @@ -31,309 +31,298 @@ def get_display_icon(self): return "fa-triangle-exclamation" -CONFIG_YAML_MESSAGES = { - message.id: message - for message in [ - Message( - id=f"build:config-yaml:generic", - header=_("Error while parsing configuration file."), - body=_("Take a look at your YAML file looking for syntax errors."), - type=ERROR, - ) - ] -} +CONFIG_YAML_MESSAGES = [] # TODO: review the copy of these notifications/messages on PR review and adapt them. # Most of them are copied from what we had in `readthedocs.doc_builder.exceptions` # and slightly adapted to have a header and a better body. -BUILD_MESSAGES = { - message.id: message - for message in [ - Message( - id=BuildAppError.GENERIC_WITH_BUILD_ID, - header=_("Unknown problem"), - # Note the message receives the instance it's attached to - # and could be use it to inject related data - body=_( +BUILD_MESSAGES = [ + Message( + id=BuildAppError.GENERIC_WITH_BUILD_ID, + 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}). """ - 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, ), - Message( - id=BuildUserError.GENERIC, - header=_("Unknown problem"), - body=_( + type=ERROR, + ), + Message( + id=BuildUserError.GENERIC, + header=_("Unknown problem"), + body=_( + """ + We encountered a problem with a command while building your project. + To resolve this error, double check your project configuration and installed + dependencies are correct and have not changed recently. """ - We encountered a problem with a command while building your project. - To resolve this error, double check your project configuration and installed - dependencies are correct and have not changed recently. - """ - ), - type=ERROR, ), - Message( - id=BuildMaxConcurrencyError.LIMIT_REACHED, - header=_("Maximum concurrency limit reached."), - # TODO: how we are going to format variables (e.g. ``limit`` here)? - # The variables are passed when it's instantiated. - # However, we need to render the notification from the front-end in a different moment. - # Do we want to store the key/values in the database and use them to render the message? - body=_( + type=ERROR, + ), + Message( + id=BuildMaxConcurrencyError.LIMIT_REACHED, + header=_("Maximum concurrency limit reached."), + # TODO: how we are going to format variables (e.g. ``limit`` here)? + # The variables are passed when it's instantiated. + # However, we need to render the notification from the front-end in a different moment. + # Do we want to store the key/values in the database and use them to render the message? + body=_( + """ + Concurrency limit reached ({limit}), retrying in 5 minutes. """ - Concurrency limit reached ({limit}), retrying in 5 minutes. - """ - ), - type=INFO, ), - Message( - id=BuildCancelled.CANCELLED_BY_USER, - header=_("Build cancelled manually."), - body=_( + type=INFO, + ), + Message( + id=BuildCancelled.CANCELLED_BY_USER, + header=_("Build cancelled manually."), + body=_( + """ + The user has cancel this build. """ - The user has cancel this build. - """ - ), - type=INFO, ), - Message( - id=BuildUserError.SKIPPED_EXIT_CODE_183, - header=_("Build skipped manually."), - body=_( + type=INFO, + ), + Message( + id=BuildUserError.SKIPPED_EXIT_CODE_183, + header=_("Build skipped manually."), + body=_( + """ + One of the commands exited with code 183 + and the build was skipped. """ - One of the commands exited with code 183 - and the build was skipped. - """ - ), - type=INFO, ), - Message( - id=BuildAppError.BUILDS_DISABLED, - header=_("Builds are temporary disabled for this project."), - body=_( + type=INFO, + ), + Message( + id=BuildAppError.BUILDS_DISABLED, + header=_("Builds are temporary disabled for this project."), + body=_( + """ + This is due to an excess usage of our resources. + Please, contact our support team if you think this is a mistake + and builds should be re-enabled. """ - This is due to an excess usage of our resources. - Please, contact our support team if you think this is a mistake - and builds should be re-enabled. - """ - ), - type=ERROR, ), - Message( - id=BuildUserError.MAX_CONCURRENCY, - header=_("Concurrency limit reached"), - body=_( + type=ERROR, + ), + Message( + id=BuildUserError.MAX_CONCURRENCY, + header=_("Concurrency limit reached"), + body=_( + """ + Your project/organization/user is currently building the maximum concurrency builds allowed ({limit}). + It will automatically retried in 5 minutes. """ - Your project/organization/user is currently building the maximum concurrency builds allowed ({limit}). - It will automatically retried in 5 minutes. - """ - ), - type=WARNING, ), - Message( - id=BuildUserError.BUILD_COMMANDS_WITHOUT_OUTPUT, - header=_("No HTML content found"), - body=_( - f""" - No "{BUILD_COMMANDS_OUTPUT_PATH_HTML}" folder was created during this build. - """ - ), - type=ERROR, + type=WARNING, + ), + Message( + id=BuildUserError.BUILD_COMMANDS_WITHOUT_OUTPUT, + header=_("No HTML content found"), + body=_( + f""" + No "{BUILD_COMMANDS_OUTPUT_PATH_HTML}" folder was created during this build. + """ ), - Message( - id=BuildUserError.BUILD_OUTPUT_IS_NOT_A_DIRECTORY, - header=_("Build output directory is not a directory"), - body=_( + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_OUTPUT_IS_NOT_A_DIRECTORY, + header=_("Build output directory is not a directory"), + body=_( + """ + Build output directory for format "{artifact_type}" is not a directory. """ - Build output directory for format "{artifact_type}" is not a directory. - """ - ), - type=ERROR, ), - Message( - id=BuildUserError.BUILD_OUTPUT_HAS_0_FILES, - header=_("Build output directory doesn't contain any file"), - body=_( + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_OUTPUT_HAS_0_FILES, + header=_("Build output directory doesn't contain any file"), + body=_( + """ + Build output directory for format "{artifact_type}" does not contain any files. + It seems the build process created the directory but did not save any file to it. """ - Build output directory for format "{artifact_type}" does not contain any files. - It seems the build process created the directory but did not save any file to it. - """ - ), - type=ERROR, ), - Message( - id=BuildUserError.BUILD_OUTPUT_HAS_MULTIPLE_FILES, - header=_("Build output directory contains multiple files"), - body=_( + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_OUTPUT_HAS_MULTIPLE_FILES, + header=_("Build output directory contains multiple files"), + body=_( + """ + Build output directory for format "{artifact_type}" contains multiple files + and it is not currently supported. + Please, remove all the files but the "{artifact_type}" you want to upload. """ - Build output directory for format "{artifact_type}" contains multiple files - and it is not currently supported. - Please, remove all the files but the "{artifact_type}" you want to upload. - """ - ), - type=ERROR, ), - Message( - id=BuildUserError.BUILD_OUTPUT_HTML_NO_INDEX_FILE, - header=_("Index file is not present in HTML output directory"), - body=_( + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_OUTPUT_HTML_NO_INDEX_FILE, + header=_("Index file is not present in HTML output directory"), + body=_( + """ + Your documentation did not generate an 'index.html' at its root directory. + This is required for documentation serving at the root URL for this version. """ - Your documentation did not generate an 'index.html' at its root directory. - This is required for documentation serving at the root URL for this version. - """ - ), - type=ERROR, ), - Message( - id=BuildUserError.BUILD_OUTPUT_OLD_DIRECTORY_USED, - header=_("Your project is outputing files in an old directory"), - body=_( + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_OUTPUT_OLD_DIRECTORY_USED, + header=_("Your project is outputing files in an old directory"), + body=_( + """ + Some files were detected in an unsupported output path, '_build/html'. + Ensure your project is configured to use the output path + '$READTHEDOCS_OUTPUT/html' instead. """ - Some files were detected in an unsupported output path, '_build/html'. - Ensure your project is configured to use the output path - '$READTHEDOCS_OUTPUT/html' instead. - """ - ), - type=ERROR, ), - Message( - id=BuildUserError.NO_CONFIG_FILE_DEPRECATED, - header=_("Your project doesn't have a .readthedocs.yaml file"), - body=_( + type=ERROR, + ), + Message( + id=BuildUserError.NO_CONFIG_FILE_DEPRECATED, + header=_("Your project doesn't have a .readthedocs.yaml file"), + body=_( + """ + The configuration file required to build documentation is missing from your project. + Add a configuration file to your project to make it build successfully. + Read more at https://docs.readthedocs.io/en/stable/config-file/v2.html """ - The configuration file required to build documentation is missing from your project. - Add a configuration file to your project to make it build successfully. - Read more at https://docs.readthedocs.io/en/stable/config-file/v2.html - """ - ), - type=ERROR, ), - Message( - id=BuildUserError.BUILD_IMAGE_CONFIG_KEY_DEPRECATED, - header=_("Configuration key build.image is deprecated"), - body=_( + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_IMAGE_CONFIG_KEY_DEPRECATED, + header=_("Configuration key build.image is deprecated"), + body=_( + """ + The configuration key "build.image" is deprecated. + Use "build.os" instead to continue building your project. + Read more at https://docs.readthedocs.io/en/stable/config-file/v2.html#build-os """ - The configuration key "build.image" is deprecated. - Use "build.os" instead to continue building your project. - Read more at https://docs.readthedocs.io/en/stable/config-file/v2.html#build-os - """ - ), - type=ERROR, ), - Message( - id=BuildUserError.BUILD_OS_REQUIRED, - header=_("Configuration key build.os is required"), - body=_( + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_OS_REQUIRED, + header=_("Configuration key build.os is required"), + body=_( + """ + The configuration key "build.os" is required to build your documentation. + Read more at https://docs.readthedocs.io/en/stable/config-file/v2.html#build-os """ - The configuration key "build.os" is required to build your documentation. - Read more at https://docs.readthedocs.io/en/stable/config-file/v2.html#build-os - """ - ), - type=ERROR, ), - Message( - id=BuildUserError.FILE_TOO_LARGE, - header=_("There is at least one file size that exceeds the limits"), - body=_( + type=ERROR, + ), + Message( + id=BuildUserError.FILE_TOO_LARGE, + header=_("There is at least one file size that exceeds the limits"), + body=_( + """ + A file from your build process is too large to be processed by Read the Docs. + Please ensure no files generated are larger than 1GB. """ - A file from your build process is too large to be processed by Read the Docs. - Please ensure no files generated are larger than 1GB. - """ - ), - type=ERROR, ), - Message( - id=BuildUserError.FILE_TOO_LARGE, - header=_("There is no PDF file in output directory"), - body=_( - f""" - PDF file was not generated/found in "{BUILD_COMMANDS_OUTPUT_PATH_HTML}/pdf" output directory. - """ - ), - type=ERROR, + type=ERROR, + ), + Message( + id=BuildUserError.FILE_TOO_LARGE, + header=_("There is no PDF file in output directory"), + body=_( + f""" + PDF file was not generated/found in "{BUILD_COMMANDS_OUTPUT_PATH_HTML}/pdf" output directory. + """ ), - # TODO: it probably makes sense to split the notification object here. - Message( - id=MkDocsYAMLParseError.GENERIC_WITH_PARSE_EXCEPTION, - header=_(""), - body=_( + type=ERROR, + ), +] + +BUILD_MKDOCS_MESSAGES = [ + Message( + id=MkDocsYAMLParseError.GENERIC_WITH_PARSE_EXCEPTION, + header=_(""), + body=_( + """ + Problem parsing MkDocs YAML configuration. {exception} """ - Problem parsing MkDocs YAML configuration. {exception} - """ - ), - type=ERROR, ), - Message( - id=MkDocsYAMLParseError.INVALID_DOCS_DIR_CONFIG, - header=_(""), - body=_( + type=ERROR, + ), + Message( + id=MkDocsYAMLParseError.INVALID_DOCS_DIR_CONFIG, + header=_(""), + body=_( + """ + The "docs_dir" config from your MkDocs YAML config file has to be a + string with relative or absolute path. """ - The "docs_dir" config from your MkDocs YAML config file has to be a - string with relative or absolute path. - """ - ), - type=ERROR, ), - Message( - id=MkDocsYAMLParseError.INVALID_DOCS_DIR_PATH, - header=_(""), - body=_( + type=ERROR, + ), + Message( + id=MkDocsYAMLParseError.INVALID_DOCS_DIR_PATH, + header=_(""), + body=_( + """ """ - """ - ), - type=ERROR, ), - Message( - id=MkDocsYAMLParseError.INVALID_EXTRA_CONFIG, - header=_(""), - body=_( + type=ERROR, + ), + Message( + id=MkDocsYAMLParseError.INVALID_EXTRA_CONFIG, + header=_(""), + body=_( + """ + The "{config}" config from your MkDocs YAML config file has to be a + list of relative paths. """ - The "{config}" config from your MkDocs YAML config file has to be a - list of relative paths. - """ - ), - type=ERROR, ), - Message( - id=MkDocsYAMLParseError.EMPTY_CONFIG, - header=_(""), - body=_( + type=ERROR, + ), + Message( + id=MkDocsYAMLParseError.EMPTY_CONFIG, + header=_(""), + body=_( + """ + Please make sure the MkDocs YAML configuration file is not empty. """ - Please make sure the MkDocs YAML configuration file is not empty. - """ - ), - type=ERROR, ), - Message( - id=MkDocsYAMLParseError.NOT_FOUND, - header=_(""), - body=_( + type=ERROR, + ), + Message( + id=MkDocsYAMLParseError.NOT_FOUND, + header=_(""), + body=_( + """ + A configuration file was not found. + Make sure you have a "mkdocs.yml" file in your repository. """ - A configuration file was not found. - Make sure you have a "mkdocs.yml" file in your repository. - """ - ), - type=ERROR, ), - Message( - id=MkDocsYAMLParseError.CONFIG_NOT_DICT, - header=_(""), - body=_( + type=ERROR, + ), + Message( + id=MkDocsYAMLParseError.CONFIG_NOT_DICT, + header=_(""), + body=_( + """ + Your MkDocs YAML config file is incorrect. + Please follow the user guide https://www.mkdocs.org/user-guide/configuration/ + to configure the file properly. """ - Your MkDocs YAML config file is incorrect. - Please follow the user guide https://www.mkdocs.org/user-guide/configuration/ - to configure the file properly. - """ - ), - type=ERROR, ), - ] -} + type=ERROR, + ), +] NOTIFICATION_MESSAGES = {} -NOTIFICATION_MESSAGES.update(CONFIG_YAML_MESSAGES) -NOTIFICATION_MESSAGES.update(BUILD_MESSAGES) +for message in zip(CONFIG_YAML_MESSAGES, BUILD_MKDOCS_MESSAGES, BUILD_MESSAGES): + NOTIFICATION_MESSAGES[message.id] = message From 36de5f8fe22a5ab8963015f81241c01f0f4f9045 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 29 Nov 2023 16:33:33 +0100 Subject: [PATCH 016/101] Messages for symlink exceptions --- readthedocs/core/utils/filesystem.py | 8 ++++---- readthedocs/doc_builder/exceptions.py | 5 +++-- readthedocs/notifications/messages.py | 13 +++++++++++++ 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/readthedocs/core/utils/filesystem.py b/readthedocs/core/utils/filesystem.py index 51dad92fcec..fe63dc769b9 100644 --- a/readthedocs/core/utils/filesystem.py +++ b/readthedocs/core/utils/filesystem.py @@ -77,11 +77,11 @@ def safe_open( ) if path.exists() and not path.is_file(): - raise FileIsNotRegularFile() + raise FileIsNotRegularFile(FileIsNotRegularFile.SYMLINK_USED) if not allow_symlinks and path.is_symlink(): - log.info("Skipping file becuase it's a symlink.") - raise UnsupportedSymlinkFileError() + log.info("Skipping file because it's a symlink.") + raise UnsupportedSymlinkFileError(UnsupportedSymlinkFileError.SYMLINK_USED) # Expand symlinks. resolved_path = path.resolve() @@ -97,7 +97,7 @@ def safe_open( if not resolved_path.is_relative_to(base_path): # Trying to path traversal via a symlink, sneaky! log.info("Path traversal via symlink.") - raise SymlinkOutsideBasePath() + raise SymlinkOutsideBasePath(SymlinkOutsideBasePath.SYMLINK_USED) _assert_path_is_inside_docroot(resolved_path) diff --git a/readthedocs/doc_builder/exceptions.py b/readthedocs/doc_builder/exceptions.py index ee082604eff..d1c2bbb9daf 100644 --- a/readthedocs/doc_builder/exceptions.py +++ b/readthedocs/doc_builder/exceptions.py @@ -11,7 +11,6 @@ Then the header/body texts should be defined in ``readthedocs/notifications/messages.py``. """ -from django.utils.translation import gettext_noop class BuildBaseException(Exception): @@ -62,10 +61,12 @@ class MkDocsYAMLParseError(BuildUserError): CONFIG_NOT_DICT = "build:user:mkdocs:invalid-yaml" +# NOTE: there is no need to have three different error classes for this. +# We can merge all of them in one, always raise the same exception with different messages. +# # TODO: improve messages for symlink errors with a more detailed error and include the `filepath`. class UnsupportedSymlinkFileError(BuildUserError): SYMLINK_USED = "build:user:symlink:used" - message = gettext_noop("Symlinks are not fully supported") class FileIsNotRegularFile(UnsupportedSymlinkFileError): diff --git a/readthedocs/notifications/messages.py b/readthedocs/notifications/messages.py index 1bcad9f0bac..c6dd72542fd 100644 --- a/readthedocs/notifications/messages.py +++ b/readthedocs/notifications/messages.py @@ -6,6 +6,7 @@ BuildMaxConcurrencyError, BuildUserError, MkDocsYAMLParseError, + UnsupportedSymlinkFileError, ) from readthedocs.projects.constants import BUILD_COMMANDS_OUTPUT_PATH_HTML @@ -323,6 +324,18 @@ def get_display_icon(self): ), ] +SYMLINK_MESSAGES = [ + Message( + id=UnsupportedSymlinkFileError.SYMLINK_USED, + header=_(""), + body=_( + """ + Symlinks are not fully supported. + """ + ), + type=ERROR, + ), +] NOTIFICATION_MESSAGES = {} for message in zip(CONFIG_YAML_MESSAGES, BUILD_MKDOCS_MESSAGES, BUILD_MESSAGES): NOTIFICATION_MESSAGES[message.id] = message From e05592a8edc0831ef35ef45c87758b7860b3e930 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 29 Nov 2023 16:35:29 +0100 Subject: [PATCH 017/101] Simplify how `NOTIFICATION_MESSAGES` is built --- readthedocs/notifications/messages.py | 16 +++++++++++++++- readthedocs/projects/tasks/builds.py | 2 +- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/readthedocs/notifications/messages.py b/readthedocs/notifications/messages.py index c6dd72542fd..01ed99921ef 100644 --- a/readthedocs/notifications/messages.py +++ b/readthedocs/notifications/messages.py @@ -1,3 +1,5 @@ +import itertools + from django.utils.translation import gettext_noop as _ from readthedocs.doc_builder.exceptions import ( @@ -22,6 +24,12 @@ def __init__(self, id, header, body, type, icon=None, icon_style=SOLID): self.icon = icon self.icon_style = icon_style # (SOLID, DUOTONE) + def __repr__(self): + return f"" + + def __str__(self): + return f"Message: {self.id} | {self.header}" + def get_display_icon(self): if self.icon: return self.icon @@ -336,6 +344,12 @@ def get_display_icon(self): type=ERROR, ), ] + +# Merge all the notifications into one object NOTIFICATION_MESSAGES = {} -for message in zip(CONFIG_YAML_MESSAGES, BUILD_MKDOCS_MESSAGES, BUILD_MESSAGES): +for message in itertools.chain( + CONFIG_YAML_MESSAGES, + BUILD_MKDOCS_MESSAGES, + BUILD_MESSAGES, +): NOTIFICATION_MESSAGES[message.id] = message diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index 590d7f57044..d42036745f3 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/readthedocs/projects/tasks/builds.py @@ -491,7 +491,7 @@ def on_failure(self, exc, task_id, args, kwargs, einfo): if hasattr(exc, "message_id") and exc.message_id is not None: message_id = exc.message_id else: - message_id = "build:user:generic" + message_id = BuildUserError.GENERIC else: # We don't know what happened in the build. Log the exception and From 9f9fcfbb19e60777de9b15bacdcb3a74d6a17ee5 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 29 Nov 2023 16:58:15 +0100 Subject: [PATCH 018/101] Add `Notification.format_values` field This allows to store values required for this notification at render time --- readthedocs/api/v3/serializers.py | 4 +-- readthedocs/core/utils/__init__.py | 7 +++-- readthedocs/notifications/messages.py | 10 +++++++ .../0002_notification_format_values.py | 17 +++++++++++ readthedocs/notifications/models.py | 30 +++---------------- readthedocs/projects/tasks/builds.py | 4 +-- 6 files changed, 39 insertions(+), 33 deletions(-) create mode 100644 readthedocs/notifications/migrations/0002_notification_format_values.py diff --git a/readthedocs/api/v3/serializers.py b/readthedocs/api/v3/serializers.py index 6b1ecabe2e0..efe45ef3c0d 100644 --- a/readthedocs/api/v3/serializers.py +++ b/readthedocs/api/v3/serializers.py @@ -226,8 +226,8 @@ def get_success(self, obj): class NotificationMessageSerializer(serializers.Serializer): id = serializers.SlugField() # TODO: how do we render these string formatted with the Notifcation instance from here? - header = serializers.CharField() - body = serializers.CharField() + header = serializers.CharField(source="get_rendered_header") + body = serializers.CharField(source="get_rendered_body") type = serializers.CharField() icon = serializers.CharField() icon_style = serializers.CharField() diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index d7363240f1a..61e83998a37 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -154,10 +154,11 @@ def prepare_build( options["countdown"] = settings.RTD_BUILDS_RETRY_DELAY options["max_retries"] = settings.RTD_BUILDS_MAX_RETRIES # TODO: adapt this code to create a Notification on Build here. - build.error = BuildMaxConcurrencyError.message.format( - limit=max_concurrent_builds, + + build.notifications.create( + message_id=BuildMaxConcurrencyError.LIMIT_REACHED, + format_values={"limit": max_concurrent_builds}, ) - build.save() _, build_api_key = BuildAPIKey.objects.create_key(project=project) diff --git a/readthedocs/notifications/messages.py b/readthedocs/notifications/messages.py index 01ed99921ef..d107c33d8b5 100644 --- a/readthedocs/notifications/messages.py +++ b/readthedocs/notifications/messages.py @@ -23,6 +23,7 @@ def __init__(self, id, header, body, type, icon=None, icon_style=SOLID): self.type = type # (ERROR, WARNING, INFO, NOTE, TIP) self.icon = icon self.icon_style = icon_style # (SOLID, DUOTONE) + self.format_values = {} def __repr__(self): return f"" @@ -30,6 +31,9 @@ def __repr__(self): def __str__(self): return f"Message: {self.id} | {self.header}" + def set_format_values(self, format_values): + self.format_values = format_values or {} + def get_display_icon(self): if self.icon: return self.icon @@ -39,6 +43,12 @@ def get_display_icon(self): if self.type == WARNING: return "fa-triangle-exclamation" + def get_rendered_header(self): + return self.header.format(**self.format_values) + + def get_rendered_body(self): + return self.body.format(**self.format_values) + CONFIG_YAML_MESSAGES = [] diff --git a/readthedocs/notifications/migrations/0002_notification_format_values.py b/readthedocs/notifications/migrations/0002_notification_format_values.py new file mode 100644 index 00000000000..1bd97356bbc --- /dev/null +++ b/readthedocs/notifications/migrations/0002_notification_format_values.py @@ -0,0 +1,17 @@ +# Generated by Django 4.2.7 on 2023-11-29 15:44 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("notifications", "0001_initial"), + ] + + operations = [ + migrations.AddField( + model_name="notification", + name="format_values", + field=models.JSONField(blank=True, null=True), + ), + ] diff --git a/readthedocs/notifications/models.py b/readthedocs/notifications/models.py index ac128ae3cc1..0cbe2fa6bad 100644 --- a/readthedocs/notifications/models.py +++ b/readthedocs/notifications/models.py @@ -1,4 +1,3 @@ -import textwrap from django.contrib.contenttypes.fields import GenericForeignKey from django.contrib.contenttypes.models import ContentType @@ -45,34 +44,13 @@ class Notification(TimeStampedModel): attached_to_id = models.PositiveIntegerField() attached_to = GenericForeignKey("attached_to_content_type", "attached_to_id") - # TODO: create a migration for this field # Store values known at creation time that are required to render the final message - # format_values = models.JSONField(null=True, blank=True) + format_values = models.JSONField(null=True, blank=True) def __str__(self): return self.message_id def get_message(self): - return NOTIFICATION_MESSAGES.get(self.message_id) - - def get_header(self): - return NOTIFICATION_MESSAGES.get(self.message_id).header - - def get_display_header(self): - return textwrap.dedent( - NOTIFICATION_MESSAGES.get(self.message_id).header.format( - instance=self.attached_to, # Build, Project, Organization, User - # **self.format_values, - ) - ) - - def get_body(self): - return NOTIFICATION_MESSAGES.get(self.message_id).body - - def get_display_body(self): - return textwrap.dedent( - NOTIFICATION_MESSAGES.get(self.message_id).body.format( - instance=self.attached_to, # Build, Project, Organization, User - # **self.format_values, - ) - ) + message = NOTIFICATION_MESSAGES.get(self.message_id) + message.set_format_values(self.format_values) + return message diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index d42036745f3..49989f8253a 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/readthedocs/projects/tasks/builds.py @@ -502,7 +502,7 @@ def on_failure(self, exc, task_id, args, kwargs, einfo): message_id = "build:app:generic-with-build-id" # TODO: finish sending "format_values" via the API - # format_values = exc.kwargs if hasattr(exc, "kwargs") else None + format_values = exc.kwargs if hasattr(exc, "kwargs") else None # POST the notification via the APIv2 self.data.api_client.notifications.post( @@ -512,7 +512,7 @@ def on_failure(self, exc, task_id, args, kwargs, einfo): "state": "unread", # Optional "dismissable": False, "news": False, - # "format_values": format_values, + "format_values": format_values, } ) From b7600ce0ee6b1984cb0cf1e33e88e108b78eb07a Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 13 Dec 2023 13:01:31 +0100 Subject: [PATCH 019/101] Notification with `format_values` passed when exception is raised --- common | 2 +- readthedocs/doc_builder/director.py | 16 ++++++++++++++++ readthedocs/doc_builder/exceptions.py | 7 +++++-- readthedocs/notifications/messages.py | 17 +++++++++++++++++ readthedocs/notifications/models.py | 10 +++++++++- readthedocs/projects/tasks/builds.py | 4 ++-- 6 files changed, 50 insertions(+), 6 deletions(-) diff --git a/common b/common index f90e6a8ea11..9e0ca9f3e99 160000 --- a/common +++ b/common @@ -1 +1 @@ -Subproject commit f90e6a8ea112bcea12530de96d9ce3d2c5c7f303 +Subproject commit 9e0ca9f3e99a14d22ac804422094513a468fcf3a diff --git a/readthedocs/doc_builder/director.py b/readthedocs/doc_builder/director.py index fcbb4c47c2e..976696afd7f 100644 --- a/readthedocs/doc_builder/director.py +++ b/readthedocs/doc_builder/director.py @@ -110,6 +110,22 @@ def setup_vcs(self): # self.run_build_job("pre_checkout") self.checkout() + # FIXME: remove this example of how to raise and exception + notification from the builder. + # raise BuildUserError(BuildUserError.BUILD_OS_REQUIRED) + + # FIXME: remove this example of a notification that uses "instance" format values + # raise BuildAppError(BuildAppError.GENERIC_WITH_BUILD_ID) + + # FIXME: remove this example of a notification that uses extra format values + raise BuildUserError( + BuildUserError.TEST_FORMAT_VALUES, + format_values={ + "message": "My message", + "another": "Another text", + "number": 42, + }, + ) + # Output the path for the config file used. # This works as confirmation for us & the user about which file is used, # as well as the fact that *any* config file is used. diff --git a/readthedocs/doc_builder/exceptions.py b/readthedocs/doc_builder/exceptions.py index d1c2bbb9daf..e7ab79770fc 100644 --- a/readthedocs/doc_builder/exceptions.py +++ b/readthedocs/doc_builder/exceptions.py @@ -15,9 +15,10 @@ class BuildBaseException(Exception): - def __init__(self, message_id, **kwargs): + def __init__(self, message_id, format_values=None, **kwargs): self.message_id = message_id - super().__init__("Build exception", **kwargs) + self.format_values = format_values + super().__init__("Build user exception", **kwargs) class BuildAppError(BuildBaseException): @@ -42,6 +43,8 @@ class BuildUserError(BuildBaseException): BUILD_IMAGE_CONFIG_KEY_DEPRECATED = "build:user:config:build-image-deprecated" BUILD_OS_REQUIRED = "build:user:config:build-os-required" + TEST_FORMAT_VALUES = "build:user:test-format-values" + class BuildMaxConcurrencyError(BuildUserError): LIMIT_REACHED = "build:user:concurrency-limit-reached" diff --git a/readthedocs/notifications/messages.py b/readthedocs/notifications/messages.py index d107c33d8b5..d0d2b085d9d 100644 --- a/readthedocs/notifications/messages.py +++ b/readthedocs/notifications/messages.py @@ -263,6 +263,23 @@ def get_rendered_body(self): ), type=ERROR, ), + # TODO: remove this test for a message with format values + Message( + id=BuildUserError.TEST_FORMAT_VALUES, + header=_("Test format values. Build: {instance.pk}"), + # Note the message receives the instance it's attached to + # and could be use it to inject related data + body=_( + """ + Some of the values passed in "format_values": + + message: {message} + another: {another} + number: {number} + """ + ), + type=ERROR, + ), ] BUILD_MKDOCS_MESSAGES = [ diff --git a/readthedocs/notifications/models.py b/readthedocs/notifications/models.py index 0cbe2fa6bad..26d531c2cd6 100644 --- a/readthedocs/notifications/models.py +++ b/readthedocs/notifications/models.py @@ -52,5 +52,13 @@ def __str__(self): def get_message(self): message = NOTIFICATION_MESSAGES.get(self.message_id) - message.set_format_values(self.format_values) + + # Pass the instance attached to this notification + all_format_values = { + "instance": self.attached_to, + } + + # Pass the values stored in the database + all_format_values.update(self.format_values or {}) + message.set_format_values(all_format_values) return message diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index 49989f8253a..119993bae8e 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/readthedocs/projects/tasks/builds.py @@ -501,8 +501,8 @@ def on_failure(self, exc, task_id, args, kwargs, einfo): log.error("Build failed with unhandled exception.", exc_info=exc) message_id = "build:app:generic-with-build-id" - # TODO: finish sending "format_values" via the API - format_values = exc.kwargs if hasattr(exc, "kwargs") else None + # Grab the format values from the exception in case it contains + format_values = exc.format_values if hasattr(exc, "format_values") else None # POST the notification via the APIv2 self.data.api_client.notifications.post( From fa27fb1da582016a90ac8c8dd574d1a2f1ad8fd3 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 13 Dec 2023 13:04:38 +0100 Subject: [PATCH 020/101] Use `get_display_icon` to show the proper icon in the serializer --- readthedocs/api/v3/serializers.py | 3 +-- readthedocs/notifications/messages.py | 8 +++++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/readthedocs/api/v3/serializers.py b/readthedocs/api/v3/serializers.py index efe45ef3c0d..491265d0902 100644 --- a/readthedocs/api/v3/serializers.py +++ b/readthedocs/api/v3/serializers.py @@ -225,11 +225,10 @@ def get_success(self, obj): class NotificationMessageSerializer(serializers.Serializer): id = serializers.SlugField() - # TODO: how do we render these string formatted with the Notifcation instance from here? header = serializers.CharField(source="get_rendered_header") body = serializers.CharField(source="get_rendered_body") type = serializers.CharField() - icon = serializers.CharField() + icon = serializers.CharField(source="get_display_icon") icon_style = serializers.CharField() class Meta: diff --git a/readthedocs/notifications/messages.py b/readthedocs/notifications/messages.py index d0d2b085d9d..b73e7c1bf5d 100644 --- a/readthedocs/notifications/messages.py +++ b/readthedocs/notifications/messages.py @@ -12,7 +12,7 @@ ) from readthedocs.projects.constants import BUILD_COMMANDS_OUTPUT_PATH_HTML -from .constants import ERROR, INFO, SOLID, WARNING +from .constants import ERROR, INFO, NOTE, SOLID, TIP, WARNING class Message: @@ -42,6 +42,12 @@ def get_display_icon(self): return "fa-exclamation" if self.type == WARNING: return "fa-triangle-exclamation" + if self.type == INFO: + return "fa-????" + if self.type == NOTE: + return "fa-????" + if self.type == TIP: + return "fa-????" def get_rendered_header(self): return self.header.format(**self.format_values) From 1dc0f6ffe3173f7e82c96d5fd69b73c4f2c0a785 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 13 Dec 2023 13:05:12 +0100 Subject: [PATCH 021/101] Proper relation name --- readthedocs/projects/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 93d351a2af1..6206252dfff 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -535,7 +535,7 @@ class Project(models.Model): notifications = GenericRelation( SiteNotification, - related_query_name="build", + related_query_name="project", content_type_field="attached_to_content_type", object_id_field="attached_to_id", ) From 27425fd3ab1e079e45ed910fcee3d7460bad0b61 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 13 Dec 2023 13:07:58 +0100 Subject: [PATCH 022/101] Initial work to replace site notifications --- readthedocs/domains/tasks.py | 5 ++ .../pending_domain_configuration_site.html | 8 ---- readthedocs/notifications/messages.py | 47 +++++++++++++++++++ 3 files changed, 52 insertions(+), 8 deletions(-) delete mode 100644 readthedocs/domains/templates/domains/notifications/pending_domain_configuration_site.html diff --git a/readthedocs/domains/tasks.py b/readthedocs/domains/tasks.py index de870fa5b24..001d708617c 100644 --- a/readthedocs/domains/tasks.py +++ b/readthedocs/domains/tasks.py @@ -30,6 +30,11 @@ def email_pending_custom_domains(number_of_emails=3): for domain in queryset: for user in AdminPermission.admins(domain.project): # TODO: migrate this to the new system + # + # Here, we want to create only one notification attached to a Project, + # but send the email to all the users? + # + # or do we want to create a notification per-user? notification = PendingCustomDomainValidation( context_object=domain, request=HttpRequest(), diff --git a/readthedocs/domains/templates/domains/notifications/pending_domain_configuration_site.html b/readthedocs/domains/templates/domains/notifications/pending_domain_configuration_site.html deleted file mode 100644 index ce862872a90..00000000000 --- a/readthedocs/domains/templates/domains/notifications/pending_domain_configuration_site.html +++ /dev/null @@ -1,8 +0,0 @@ -{% url "projects_domains_edit" domain.project.slug domain.pk as domain_url %} -{% if domain.validation_process_expired %} - The validation period of your custom domain {{ domain.domain }} has ended. - Go to the domain page and click on "Save" to restart the process. -{% else %} - The configuration of your custom domain {{ domain.domain }} is pending. - Go to the domain page and follow the instructions to complete it. -{% endif %} diff --git a/readthedocs/notifications/messages.py b/readthedocs/notifications/messages.py index b73e7c1bf5d..8f3433c1302 100644 --- a/readthedocs/notifications/messages.py +++ b/readthedocs/notifications/messages.py @@ -378,11 +378,58 @@ def get_rendered_body(self): ), ] +# TODO: think a way to define these notifications from other apps. +# Maybe implement a "registry.add(messages)" that updates the available messages? +DOMAIN_MESSAGES = [ + Message( + id="project:domain:validation-pending", + header=_("Pending configuration of custom domain: {domain}"), + body=_( + """ + The configuration of your custom domain {domain} is pending. + Go to the domain page and follow the instructions to complete it. + """ + ), + type=INFO, + ), + # TODO: the custom domain expired notification requires a periodic task to + # remove the old notification and create a new one pointing to this + # ``message_id`` + Message( + id="project:domain:validation-expired", + header=_("Validation of custom domain expired: {domain}"), + body=_( + """ + The validation period of your custom domain {domain} has ended. + Go to the domain page and click on "Save" to restart the process. + """ + ), + type=INFO, + ), +] + +SUBSCRIPTION_MESSAGES = [] +USER_MESSAGES = [ + Message( + id="user:email:validation-pending", + header=_("You primary email is pending for validation."), + body=_( + """ + Your primary email address is not verified. + Please verify it here. + """ + ), + type=INFO, + ), +] + # Merge all the notifications into one object NOTIFICATION_MESSAGES = {} for message in itertools.chain( CONFIG_YAML_MESSAGES, BUILD_MKDOCS_MESSAGES, BUILD_MESSAGES, + SUBSCRIPTION_MESSAGES, + USER_MESSAGES, ): NOTIFICATION_MESSAGES[message.id] = message From 05e7e31fc99e3d79d2e04eef613c8d960368cc77 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 13 Dec 2023 13:16:06 +0100 Subject: [PATCH 023/101] We are not going to create Notifications via APIv3 for now --- readthedocs/api/v3/views.py | 30 ------------------------------ 1 file changed, 30 deletions(-) diff --git a/readthedocs/api/v3/views.py b/readthedocs/api/v3/views.py index bd7ed20e3da..3196980dca1 100644 --- a/readthedocs/api/v3/views.py +++ b/readthedocs/api/v3/views.py @@ -1,7 +1,6 @@ import django_filters.rest_framework as filters from django.contrib.contenttypes.models import ContentType from django.db.models import Exists, OuterRef -from django.shortcuts import get_object_or_404 from rest_flex_fields import is_expanded from rest_flex_fields.views import FlexFieldsMixin from rest_framework import status @@ -65,7 +64,6 @@ BuildCreateSerializer, BuildSerializer, EnvironmentVariableSerializer, - NotificationCreateSerializer, NotificationSerializer, OrganizationSerializer, ProjectCreateSerializer, @@ -412,34 +410,6 @@ def get_queryset(self): ) -class NotificationsCreateViewSet( - NotificationsViewSet, - CreateModelMixin, -): - def get_serializer_class(self): - if self.action == "create": - return NotificationCreateSerializer - - return super().get_serializer_class() - - def create(self, request, **kwargs): # pylint: disable=arguments-differ - # TODO: move this into `self._get_parent_build()` method - build_id = kwargs.get("parent_lookup_build__id") - build = get_object_or_404(Build, pk=build_id) - - # TODO: find a better way to create a Notification object from a serializer "automatically" - n = NotificationCreateSerializer(data=request.POST) - if n.is_valid(): - build.notifications.create(**n.data) - return Response(status=status.HTTP_204_NO_CONTENT) - - code = status.HTTP_400_BAD_REQUEST - return Response(status=code) - - # build = self._get_parent_build() - # notification = build.notifications.create() - - class RedirectsViewSet( APIv3Settings, NestedViewSetMixin, From fa11b1a689e1bb3ea9c8aa32ea8cabe3f735960e Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 13 Dec 2023 13:22:36 +0100 Subject: [PATCH 024/101] Send `format_values` to exception --- readthedocs/projects/tasks/builds.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index 119993bae8e..86028158cab 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/readthedocs/projects/tasks/builds.py @@ -370,7 +370,9 @@ def _check_concurrency_limit(self): self.retry( exc=BuildMaxConcurrencyError( BuildMaxConcurrencyError.LIMIT_REACHED, - limit=max_concurrent_builds, + format_values={ + "limit": max_concurrent_builds, + }, ) ) From 10d5c9c854bc07338cb69f68072e729262cfe8f1 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 13 Dec 2023 13:30:30 +0100 Subject: [PATCH 025/101] Small changes and comments added --- readthedocs/projects/tasks/builds.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index 86028158cab..f19d8b16b80 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/readthedocs/projects/tasks/builds.py @@ -475,12 +475,14 @@ def on_failure(self, exc, task_id, args, kwargs, einfo): # TODO: handle this `ConfigError` as a `BuildUserError` in the # following block if isinstance(exc, ConfigError): + # TODO: create ConfigError messages under + # "readthedocs.notification.messages.CONFIG_YAML_MESSAGES" message_id = "build:config-yaml:generic" # Known errors in our application code (e.g. we couldn't connect to # Docker API). Report a generic message to the user. elif isinstance(exc, BuildAppError): - message_id = "build:app:generic-with-build-id" + message_id = BuildAppError.GENERIC_WITH_BUILD_ID # Known errors in the user's project (e.g. invalid config file, invalid # repository, command failed, etc). Report the error back to the user @@ -501,7 +503,7 @@ def on_failure(self, exc, task_id, args, kwargs, einfo): # Note we are using `log.error(exc_info=...)` instead of `log.exception` # because this is not executed inside a try/except block. log.error("Build failed with unhandled exception.", exc_info=exc) - message_id = "build:app:generic-with-build-id" + message_id = BuildAppError.GENERIC_WITH_BUILD_ID # Grab the format values from the exception in case it contains format_values = exc.format_values if hasattr(exc, "format_values") else None @@ -600,7 +602,9 @@ def get_valid_artifact_types(self): ) raise BuildUserError( BuildUserError.BUILD_OUTPUT_IS_NOT_A_DIRECTORY, - artifact_type=artifact_type, + format_values={ + "artifact_type": artifact_type, + }, ) # Check if there are multiple files on artifact directories. @@ -699,6 +703,9 @@ def on_retry(self, exc, task_id, args, kwargs, einfo): version_slug=self.data.version.slug, ) # TODO: update this code to add the notification to the Build instance + # + # TODO: think about how to de-duplicate notifications. + # It may happens the build is retried multiple times. self.data.build['error'] = exc.message self.update_build(state=BUILD_STATE_TRIGGERED) From 4e0036561478931befdabe6fb1bccdbccea70e55 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 13 Dec 2023 16:18:57 +0100 Subject: [PATCH 026/101] Initial work to migrate an old SiteNotification to new system --- readthedocs/core/tests/test_contact.py | 7 +- readthedocs/core/utils/contact.py | 16 +- readthedocs/domains/notifications.py | 7 +- readthedocs/domains/tasks.py | 25 ++- readthedocs/notifications/__init__.py | 23 +-- readthedocs/notifications/backends.py | 118 ------------ readthedocs/notifications/notification.py | 76 ++++---- .../rtd_tests/tests/test_notifications.py | 179 +----------------- readthedocs/settings/base.py | 6 +- readthedocs/subscriptions/notifications.py | 59 +++--- 10 files changed, 107 insertions(+), 409 deletions(-) delete mode 100644 readthedocs/notifications/backends.py diff --git a/readthedocs/core/tests/test_contact.py b/readthedocs/core/tests/test_contact.py index becb6e8aa70..634556c0ec5 100644 --- a/readthedocs/core/tests/test_contact.py +++ b/readthedocs/core/tests/test_contact.py @@ -5,7 +5,8 @@ from django_dynamic_fixture import get from readthedocs.core.utils.contact import contact_users -from readthedocs.notifications.backends import SiteBackend + +# from readthedocs.notifications.backends import SiteBackend User = get_user_model() @@ -17,7 +18,7 @@ def setUp(self): self.user_three = get(User, username="test3", email="three@test.com") @mock.patch("readthedocs.core.utils.contact.send_mail") - @mock.patch.object(SiteBackend, "send") + # @mock.patch.object(SiteBackend, "send") def test_contact_users_dryrun(self, send_notification, send_mail): self.assertEqual(User.objects.all().count(), 3) resp = contact_users( @@ -49,7 +50,7 @@ def test_contact_users_dryrun(self, send_notification, send_mail): self.assertEqual(send_mail.call_count, 0) @mock.patch("readthedocs.core.utils.contact.send_mail") - @mock.patch.object(SiteBackend, "send") + # @mock.patch.object(SiteBackend, "send") def test_contact_users_not_dryrun(self, send_notification, send_mail): self.assertEqual(User.objects.all().count(), 3) resp = contact_users( diff --git a/readthedocs/core/utils/contact.py b/readthedocs/core/utils/contact.py index 9bbf3644b1d..c5aa422413f 100644 --- a/readthedocs/core/utils/contact.py +++ b/readthedocs/core/utils/contact.py @@ -4,10 +4,6 @@ from django.conf import settings from django.core.mail import send_mail from django.template import Context, Engine -from messages_extends import constants as message_constants - -from readthedocs.notifications import SiteNotification -from readthedocs.notifications.backends import SiteBackend log = structlog.get_logger(__name__) @@ -51,7 +47,9 @@ def contact_users( sent_notifications = set() failed_notifications = set() - backend = SiteBackend(request=None) + # TODO: migrate this to ``Notification.objects.create()`` + # + # backend = SiteBackend(request=None) engine = Engine.get_default() notification_template = engine.from_string(notification_content or "") @@ -63,10 +61,7 @@ def contact_users( # TODO: migrate this notification to the new notifications system. # This notification are currently tied to a particular user. # However, in the new system they will be tied to a Project. - class TempNotification(SiteNotification): - if sticky_notification: - success_level = message_constants.SUCCESS_PERSISTENT - + class TempNotification: def render(self, *args, **kwargs): return markdown.markdown( notification_template.render(Context(self.get_context_data())) @@ -83,12 +78,11 @@ def render(self, *args, **kwargs): if notification_content: notification = TempNotification( user=user, - success=True, extra_context=context, ) try: if not dryrun: - backend.send(notification) + notification.send() else: # Check we can render the notification with the context properly log.debug( diff --git a/readthedocs/domains/notifications.py b/readthedocs/domains/notifications.py index e9320bbb8db..5947ae9630a 100644 --- a/readthedocs/domains/notifications.py +++ b/readthedocs/domains/notifications.py @@ -1,13 +1,10 @@ """Notifications related to custom domains.""" -from readthedocs.notifications import Notification -from readthedocs.notifications.constants import REQUIREMENT +from readthedocs.notifications import EmailNotification -# TODO: migrate this notification to the new system -class PendingCustomDomainValidation(Notification): +class PendingCustomDomainValidation(EmailNotification): app_templates = "domains" context_object_name = "domain" name = "pending_domain_configuration" subject = "Pending configuration of custom domain {{ domain.domain }}" - level = REQUIREMENT diff --git a/readthedocs/domains/tasks.py b/readthedocs/domains/tasks.py index 001d708617c..812d43eabb6 100644 --- a/readthedocs/domains/tasks.py +++ b/readthedocs/domains/tasks.py @@ -1,11 +1,12 @@ """Tasks related to custom domains.""" from django.conf import settings -from django.http import HttpRequest +from django.urls import reverse from django.utils import timezone from readthedocs.core.permissions import AdminPermission from readthedocs.domains.notifications import PendingCustomDomainValidation +from readthedocs.notifications.models import Notification from readthedocs.projects.models import Domain from readthedocs.worker import app @@ -28,16 +29,24 @@ def email_pending_custom_domains(number_of_emails=3): validation_process_start__date__in=dates ) for domain in queryset: + # NOTE: this site notification was attach to every single user. + # The new behavior is to attach it to the project. + # + # We send an email notification to all the project's admins. + Notification.objects.create( + message_id="project:domain:validation-pending", + attached_to=domain.project, + format_values={ + "domain": domain.domain, + "domain_url": reverse( + "projects_domains_edit", args=[domain.project.slug, domain.pk] + ), + }, + ) + for user in AdminPermission.admins(domain.project): - # TODO: migrate this to the new system - # - # Here, we want to create only one notification attached to a Project, - # but send the email to all the users? - # - # or do we want to create a notification per-user? notification = PendingCustomDomainValidation( context_object=domain, - request=HttpRequest(), user=user, ) notification.send() diff --git a/readthedocs/notifications/__init__.py b/readthedocs/notifications/__init__.py index 7b4c69b05c6..ff7a12478f6 100644 --- a/readthedocs/notifications/__init__.py +++ b/readthedocs/notifications/__init__.py @@ -1,22 +1,7 @@ -""" -Extensions to Django messages to support notifications to users. - -Notifications are important communications to users that need to be as visible -as possible. We support different backends to make notifications visible in -different ways. For example, they might be e-mailed to users as well as -displayed on the site. - -This app builds on `django-messages-extends`_ to provide persistent messages -on the site. - -.. _`django-messages-extends`: https://github.com - /AliLozano/django-messages-extends/ -""" -from .notification import Notification, SiteNotification -from .backends import send_notification +"""Notification system to send email/site messages to users.""" +from .notification import EmailNotification, SiteNotification __all__ = ( - "Notification", - "SiteNotification", - "send_notification", + "EmailNotification", + "SiteNotification", # TODO: remove this ) diff --git a/readthedocs/notifications/backends.py b/readthedocs/notifications/backends.py deleted file mode 100644 index f07d5b5d7d8..00000000000 --- a/readthedocs/notifications/backends.py +++ /dev/null @@ -1,118 +0,0 @@ -""" -Pluggable backends for the delivery of notifications. - -Delivery of notifications to users depends on a list of backends configured in -Django settings. For example, they might be e-mailed to users as well as -displayed on the site. -""" - -from django.conf import settings -from django.http import HttpRequest -from django.utils.module_loading import import_string -from messages_extends.constants import INFO_PERSISTENT - -from readthedocs.core.utils import send_email - -from .constants import HTML, LEVEL_MAPPING, REQUIREMENT, TEXT - - -def send_notification(request, notification): - """ - Send notifications through all backends defined by settings. - - Backends should be listed in the settings ``NOTIFICATION_BACKENDS``, which - should be a list of class paths to be loaded, using the standard Django - string module loader. - """ - for cls_name in settings.NOTIFICATION_BACKENDS: - backend = import_string(cls_name)(request) - backend.send(notification) - - -class Backend: - def __init__(self, request): - self.request = request - - def send(self, notification): - pass - - -class EmailBackend(Backend): - - """ - Send templated notification emails through our standard email backend. - - The content body is first rendered from an on-disk template, then passed - into the standard email templates as a string. - - If the notification is set to ``send_email=False``, this backend will exit - early from :py:meth:`send`. - """ - - name = "email" - - def send(self, notification): - if not notification.send_email: - return - # FIXME: if the level is an ERROR an email is received and sometimes - # it's not necessary. This behavior should be clearly documented in the - # code - if notification.level >= REQUIREMENT: - template = notification.get_template_names( - backend_name=self.name, source_format=TEXT - ) - template_html = notification.get_template_names( - backend_name=self.name, source_format=HTML - ) - send_email( - recipient=notification.user.email, - subject=notification.get_subject(), - template=template, - template_html=template_html, - context=notification.get_context_data(), - ) - - -# TODO: modify this backend to create a `Notification` object -# instead of calling `storage.add()`. -# -# Note that probably `request` argument can be deleted from here -# since it's not required. -class SiteBackend(Backend): - - """ - Add messages through Django messages application. - - This uses persistent messageing levels provided by :py:mod:`message_extends` - and stores persistent messages in the database. - """ - - name = "site" - - def send(self, notification): - # Instead of calling the standard messages.add method, this instead - # manipulates the storage directly. This is because we don't have a - # request object and need to mock one out to fool the message storage - # into saving a message for a separate user. - cls_name = settings.MESSAGE_STORAGE - cls = import_string(cls_name) - req = HttpRequest() - setattr(req, "session", "") - storage = cls(req) - - # Use the method defined by the notification or map a simple level to a - # persistent one otherwise - if hasattr(notification, "get_message_level"): - level = notification.get_message_level() - else: - level = LEVEL_MAPPING.get(notification.level, INFO_PERSISTENT) - - storage.add( - level=level, - message=notification.render( - backend_name=self.name, - source_format=HTML, - ), - extra_tags=notification.extra_tags, - user=notification.user, - ) diff --git a/readthedocs/notifications/notification.py b/readthedocs/notifications/notification.py index 169a0a435f5..06d9430c0f7 100644 --- a/readthedocs/notifications/notification.py +++ b/readthedocs/notifications/notification.py @@ -8,48 +8,45 @@ from django.template.loader import render_to_string from readthedocs.core.context_processors import readthedocs_processor +from readthedocs.core.utils import send_email from . import constants -from .backends import send_notification log = structlog.get_logger(__name__) -# NOTE: this Notification class should be renamed to EmailNotification, -# since it won't share too much code with the SiteNotification anymore. +# TODO: remove this, it's just a quick test for QA from the console. # -# We can extract the shared logic if we want, -# but I don't think there is going to be too much in common. -# SiteNotification won't use render from TXT/HTML files. -# -class Notification: +# from readthedocs.domains.notifications import PendingCustomDomainValidation +# domain = Domain.objects.get(domain='docs.humitos.com') +# project = Project.objects.get(slug='test-builds') +# user = User.objects.get(username='admin') +# n = PendingCustomDomainValidation(context_object=domain, user=user) +# n.send() + + +class EmailNotification: """ An unsent notification linked to an object. This class provides an interface to construct notification messages by - rendering Django templates. The ``Notification`` itself is not expected - to be persisted by the backends. + rendering Django templates and send them via email. - Call .send() to send the notification. + Call .send() to send the email notification. """ name = None context_object_name = "object" app_templates = None - level = constants.INFO subject = None user = None - send_email = True extra_tags = "" - def __init__(self, context_object, request=None, user=None, extra_context=None): - self.object = context_object - self.request = request or HttpRequest() + def __init__(self, context_object, user, extra_context=None): + self.context_object = context_object self.extra_context = extra_context or {} self.user = user - if self.user is None: - self.user = request.user def get_subject(self): template = Template(self.subject) @@ -57,39 +54,35 @@ def get_subject(self): def get_context_data(self): context = { - self.context_object_name: self.object, - # NOTE: I checked the notifications templates, and the "request" attribute is not used. - # We can remove this complexity. - "request": self.request, + self.context_object_name: self.context_object, "production_uri": "{scheme}://{host}".format( scheme="https", host=settings.PRODUCTION_DOMAIN, ), } context.update(self.extra_context) - context.update(readthedocs_processor(self.request)) + context.update(readthedocs_processor(None)) return context - def get_template_names(self, backend_name, source_format=constants.HTML): + def get_template_names(self, source_format=constants.HTML): names = [] - if self.object and isinstance(self.object, models.Model): - meta = self.object._meta + if self.context_object and isinstance(self.context_object, models.Model): + meta = self.context_object._meta names.append( "{app}/notifications/{name}_{backend}.{source_format}".format( app=self.app_templates or meta.app_label, name=self.name or meta.model_name, - backend=backend_name, + backend="email", source_format=source_format, ), ) return names - raise AttributeError() + raise AttributeError("Template for this email not found.") - def render(self, backend_name, source_format=constants.HTML): + def render(self, source_format=constants.HTML): return render_to_string( template_name=self.get_template_names( - backend_name=backend_name, source_format=source_format, ), context=self.get_context_data(), @@ -97,20 +90,25 @@ def render(self, backend_name, source_format=constants.HTML): def send(self): """ - Trigger notification send through all notification backends. + Send templated notification emails through our standard email backend. - In order to limit which backends a notification will send out from, - override this method and duplicate the logic from - :py:func:`send_notification`, taking care to limit which backends are - avoided. + The content body is first rendered from an on-disk template, then passed + into the standard email templates as a string. """ - # NOTE: the concept of "backend" can be removed because we won't have multiple backends. - # Just overriding the `send()` method will be enough and reduce the complexity. - send_notification(self.request, self) + template = self.get_template_names(source_format=constants.TEXT) + template_html = self.get_template_names(source_format=constants.HTML) + send_email( + recipient=self.user.email, + subject=self.get_subject(), + template=template, + template_html=template_html, + context=self.get_context_data(), + ) -class SiteNotification(Notification): +# NOTE: this class is replaced by the new Notification model. +class SiteNotification: """ Simple notification to show *only* on site messages. diff --git a/readthedocs/rtd_tests/tests/test_notifications.py b/readthedocs/rtd_tests/tests/test_notifications.py index 05933abf306..aa1f388906c 100644 --- a/readthedocs/rtd_tests/tests/test_notifications.py +++ b/readthedocs/rtd_tests/tests/test_notifications.py @@ -1,43 +1,29 @@ # TODO: adapt/remove these tests because we won't have backends anymore. - - """Notification tests.""" from unittest import mock import django_dynamic_fixture as fixture -from allauth.account.models import EmailAddress -from django.contrib.auth.models import AnonymousUser, User +from django.contrib.auth.models import User from django.test import TestCase from django.test.utils import override_settings -from messages_extends.models import Message as PersistentMessage from readthedocs.builds.models import Build -from readthedocs.notifications import Notification, SiteNotification -from readthedocs.notifications.backends import EmailBackend, SiteBackend -from readthedocs.notifications.constants import ( - ERROR, - INFO_NON_PERSISTENT, - WARNING_NON_PERSISTENT, -) +from readthedocs.notifications import EmailNotification @override_settings( - NOTIFICATION_BACKENDS=[ - "readthedocs.notifications.backends.EmailBackend", - "readthedocs.notifications.backends.SiteBackend", - ], PRODUCTION_DOMAIN="readthedocs.org", SUPPORT_EMAIL="support@readthedocs.org", ) @mock.patch("readthedocs.notifications.notification.render_to_string") -@mock.patch.object(Notification, "send") +@mock.patch.object(EmailNotification, "send") class NotificationTests(TestCase): def test_notification_custom(self, send, render_to_string): render_to_string.return_value = "Test" - class TestNotification(Notification): + class TestNotification(EmailNotification): name = "foo" subject = "This is {{ foo.id }}" context_object_name = "foo" @@ -97,14 +83,13 @@ class TestNotification(Notification): name = "foo" subject = "This is {{ foo.id }}" context_object_name = "foo" - level = ERROR build = fixture.get(Build) req = mock.MagicMock() user = fixture.get(User) notify = TestNotification(context_object=build, request=req, user=user) - backend = EmailBackend(request=req) - backend.send(notify) + # backend = EmailBackend(request=req) + # backend.send(notify) send_email.assert_has_calls( [ @@ -117,155 +102,3 @@ class TestNotification(Notification): ), ] ) - - @mock.patch("readthedocs.notifications.notification.render_to_string") - def test_message_backend(self, render_to_string): - render_to_string.return_value = "Test" - - class TestNotification(Notification): - name = "foo" - subject = "This is {{ foo.id }}" - context_object_name = "foo" - - build = fixture.get(Build) - user = fixture.get(User) - req = mock.MagicMock() - notify = TestNotification(context_object=build, request=req, user=user) - backend = SiteBackend(request=req) - backend.send(notify) - - self.assertEqual(render_to_string.call_count, 1) - self.assertEqual(PersistentMessage.objects.count(), 1) - - message = PersistentMessage.objects.first() - self.assertEqual(message.user, user) - - @mock.patch("readthedocs.notifications.notification.render_to_string") - def test_message_anonymous_user(self, render_to_string): - """Anonymous user still throwns exception on persistent messages.""" - render_to_string.return_value = "Test" - - class TestNotification(Notification): - name = "foo" - subject = "This is {{ foo.id }}" - context_object_name = "foo" - - build = fixture.get(Build) - user = AnonymousUser() - req = mock.MagicMock() - notify = TestNotification(context_object=build, request=req, user=user) - backend = SiteBackend(request=req) - - self.assertEqual(PersistentMessage.objects.count(), 0) - - # We should never be adding persistent messages for anonymous users. - # Make sure message_extends still throws an exception here - with self.assertRaises(NotImplementedError): - backend.send(notify) - - self.assertEqual(render_to_string.call_count, 1) - self.assertEqual(PersistentMessage.objects.count(), 0) - - @mock.patch("readthedocs.notifications.backends.send_email") - def test_non_persistent_message(self, send_email): - class TestNotification(SiteNotification): - name = "foo" - success_message = "Test success message" - success_level = INFO_NON_PERSISTENT - - user = fixture.get(User) - # Setting the primary and verified email address of the user - email = fixture.get(EmailAddress, user=user, primary=True, verified=True) - - n = TestNotification(user, True) - backend = SiteBackend(request=None) - - self.assertEqual(PersistentMessage.objects.count(), 0) - backend.send(n) - # No email is sent for non persistent messages - send_email.assert_not_called() - self.assertEqual(PersistentMessage.objects.count(), 1) - self.assertEqual(PersistentMessage.objects.filter(read=False).count(), 1) - - self.client.force_login(user) - response = self.client.get("/dashboard/") - self.assertContains(response, "Test success message") - self.assertEqual(PersistentMessage.objects.count(), 1) - self.assertEqual(PersistentMessage.objects.filter(read=True).count(), 1) - - response = self.client.get("/dashboard/") - self.assertNotContains(response, "Test success message") - - -@override_settings( - PRODUCTION_DOMAIN="readthedocs.org", - SUPPORT_EMAIL="support@readthedocs.org", -) -class SiteNotificationTests(TestCase): - class TestSiteNotification(SiteNotification): - name = "foo" - success_message = "simple success message" - failure_message = { - 1: "simple failure message", - 2: "{{ object.name }} object name", - "three": "{{ object.name }} and {{ other.name }} render", - } - success_level = INFO_NON_PERSISTENT - failure_level = WARNING_NON_PERSISTENT - - def setUp(self): - self.user = fixture.get(User) - self.context = {"other": {"name": "other name"}} - self.n = self.TestSiteNotification( - self.user, - True, - context_object={"name": "object name"}, - extra_context=self.context, - ) - - def test_context_data(self): - context = { - "object": {"name": "object name"}, - "request": mock.ANY, - "production_uri": "https://readthedocs.org", - "other": {"name": "other name"}, - # readthedocs_processor context - "DASHBOARD_ANALYTICS_CODE": mock.ANY, - "DO_NOT_TRACK_ENABLED": mock.ANY, - "GLOBAL_ANALYTICS_CODE": mock.ANY, - "PRODUCTION_DOMAIN": "readthedocs.org", - "PUBLIC_DOMAIN": mock.ANY, - "PUBLIC_API_URL": mock.ANY, - "SITE_ROOT": mock.ANY, - "SUPPORT_EMAIL": "support@readthedocs.org", - "TEMPLATE_ROOT": mock.ANY, - "USE_PROMOS": mock.ANY, - "USE_ORGANIZATIONS": mock.ANY, - } - self.assertEqual(self.n.get_context_data(), context) - - def test_message_level(self): - self.n.success = True - self.assertEqual(self.n.get_message_level(), INFO_NON_PERSISTENT) - - self.n.success = False - self.assertEqual(self.n.get_message_level(), WARNING_NON_PERSISTENT) - - def test_message(self): - self.n.reason = 1 - self.assertEqual(self.n.get_message(True), "simple success message") - self.n.reason = "three" - self.assertEqual(self.n.get_message(True), "simple success message") - - self.n.reason = 1 - self.assertEqual(self.n.get_message(False), "simple failure message") - self.n.reason = 2 - self.assertEqual(self.n.get_message(False), "object name object name") - self.n.reason = "three" - self.assertEqual(self.n.get_message(False), "object name and other name render") - - # Invalid reason - self.n.reason = None - with mock.patch("readthedocs.notifications.notification.log") as mock_log: - self.assertEqual(self.n.get_message(False), "") - mock_log.error.assert_called_once() diff --git a/readthedocs/settings/base.py b/readthedocs/settings/base.py index 7eacb783740..bb8f41ecc0b 100644 --- a/readthedocs/settings/base.py +++ b/readthedocs/settings/base.py @@ -352,10 +352,8 @@ def MIDDLEWARE(self): MESSAGE_STORAGE = 'readthedocs.notifications.storages.FallbackUniqueStorage' - NOTIFICATION_BACKENDS = [ - 'readthedocs.notifications.backends.EmailBackend', - 'readthedocs.notifications.backends.SiteBackend', - ] + # TODO: remove this setting since it's not used anymore + NOTIFICATION_BACKENDS = [] # Paths SITE_ROOT = os.path.dirname( diff --git a/readthedocs/subscriptions/notifications.py b/readthedocs/subscriptions/notifications.py index 002ab2dccdb..41e11281572 100644 --- a/readthedocs/subscriptions/notifications.py +++ b/readthedocs/subscriptions/notifications.py @@ -4,16 +4,15 @@ """Organization level notifications.""" -from django.urls import reverse from djstripe import models as djstripe -from messages_extends.constants import WARNING_PERSISTENT -from readthedocs.notifications import Notification, SiteNotification -from readthedocs.notifications.constants import REQUIREMENT +from readthedocs.notifications import EmailNotification from readthedocs.organizations.models import Organization from readthedocs.subscriptions.constants import DISABLE_AFTER_DAYS +# NOTE: these notifications need to be re-written using the new system. +# However, the email notification should keep being working as-is. class SubscriptionNotificationMixin: """Force to read templates from the subscriptions app.""" @@ -21,14 +20,13 @@ class SubscriptionNotificationMixin: app_templates = "subscriptions" -class TrialEndingNotification(SubscriptionNotificationMixin, Notification): +class TrialEndingNotification(SubscriptionNotificationMixin, EmailNotification): """Trial is ending, nudge user towards subscribing.""" name = "trial_ending" context_object_name = "organization" subject = "Your trial is ending soon" - level = REQUIREMENT @staticmethod def for_subscriptions(): @@ -39,17 +37,18 @@ def for_subscriptions(): ) -class SubscriptionRequiredNotification(SubscriptionNotificationMixin, Notification): +class SubscriptionRequiredNotification( + SubscriptionNotificationMixin, EmailNotification +): """Trial has ended, push user into subscribing.""" name = "subscription_required" context_object_name = "organization" subject = "We hope you enjoyed your trial of Read the Docs!" - level = REQUIREMENT -class SubscriptionEndedNotification(SubscriptionNotificationMixin, Notification): +class SubscriptionEndedNotification(SubscriptionNotificationMixin, EmailNotification): """ Subscription has ended. @@ -61,10 +60,11 @@ class SubscriptionEndedNotification(SubscriptionNotificationMixin, Notification) name = "subscription_ended" context_object_name = "organization" subject = "Your subscription to Read the Docs has ended" - level = REQUIREMENT -class OrganizationDisabledNotification(SubscriptionNotificationMixin, Notification): +class OrganizationDisabledNotification( + SubscriptionNotificationMixin, EmailNotification +): """ Subscription has ended a month ago. @@ -77,7 +77,6 @@ class OrganizationDisabledNotification(SubscriptionNotificationMixin, Notificati name = "organization_disabled" context_object_name = "organization" subject = "Your Read the Docs organization will be disabled soon" - level = REQUIREMENT days_after_end = DISABLE_AFTER_DAYS @@ -90,20 +89,22 @@ def for_organizations(cls): return organizations -class OrganizationDisabledSiteNotification( - SubscriptionNotificationMixin, SiteNotification -): - success_message = 'The organization "{{ object.name }}" is currently disabled. You need to renew your subscription to keep using Read the Docs.' # noqa - success_level = WARNING_PERSISTENT - - def get_context_data(self): - context = super().get_context_data() - context.update( - { - "url": reverse( - "subscription_detail", - args=[self.object.slug], - ), - } - ) - return context +# TODO: migrate this to the new system +# +# class OrganizationDisabledSiteNotification( +# SubscriptionNotificationMixin, SiteNotification +# ): +# success_message = 'The organization "{{ object.name }}" is currently disabled. You need to renew your subscription to keep using Read the Docs.' # noqa +# success_level = WARNING_PERSISTENT + +# def get_context_data(self): +# context = super().get_context_data() +# context.update( +# { +# "url": reverse( +# "subscription_detail", +# args=[self.object.slug], +# ), +# } +# ) +# return context From 4e8d63d8f9ddb23f688e75e4edf815572241d5e0 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 13 Dec 2023 16:32:51 +0100 Subject: [PATCH 027/101] Create a notification `registry` to register messages by application --- readthedocs/domains/notifications.py | 34 +++++++++++++ readthedocs/notifications/messages.py | 70 +++++++++++---------------- readthedocs/notifications/models.py | 4 +- 3 files changed, 65 insertions(+), 43 deletions(-) diff --git a/readthedocs/domains/notifications.py b/readthedocs/domains/notifications.py index 5947ae9630a..e2ceb852570 100644 --- a/readthedocs/domains/notifications.py +++ b/readthedocs/domains/notifications.py @@ -1,6 +1,10 @@ """Notifications related to custom domains.""" +from django.utils.translation import gettext_noop as _ + from readthedocs.notifications import EmailNotification +from readthedocs.notifications.constants import INFO +from readthedocs.notifications.messages import Message, registry class PendingCustomDomainValidation(EmailNotification): @@ -8,3 +12,33 @@ class PendingCustomDomainValidation(EmailNotification): context_object_name = "domain" name = "pending_domain_configuration" subject = "Pending configuration of custom domain {{ domain.domain }}" + + +messages = [ + Message( + id="project:domain:validation-pending", + header=_("Pending configuration of custom domain: {domain}"), + body=_( + """ + The configuration of your custom domain {domain} is pending. + Go to the domain page and follow the instructions to complete it. + """ + ), + type=INFO, + ), + # TODO: the custom domain expired notification requires a periodic task to + # remove the old notification and create a new one pointing to this + # ``message_id`` + Message( + id="project:domain:validation-expired", + header=_("Validation of custom domain expired: {domain}"), + body=_( + """ + The validation period of your custom domain {domain} has ended. + Go to the domain page and click on "Save" to restart the process. + """ + ), + type=INFO, + ), +] +registry.add(messages) diff --git a/readthedocs/notifications/messages.py b/readthedocs/notifications/messages.py index 8f3433c1302..98dff640c59 100644 --- a/readthedocs/notifications/messages.py +++ b/readthedocs/notifications/messages.py @@ -1,4 +1,3 @@ -import itertools from django.utils.translation import gettext_noop as _ @@ -378,36 +377,6 @@ def get_rendered_body(self): ), ] -# TODO: think a way to define these notifications from other apps. -# Maybe implement a "registry.add(messages)" that updates the available messages? -DOMAIN_MESSAGES = [ - Message( - id="project:domain:validation-pending", - header=_("Pending configuration of custom domain: {domain}"), - body=_( - """ - The configuration of your custom domain {domain} is pending. - Go to the domain page and follow the instructions to complete it. - """ - ), - type=INFO, - ), - # TODO: the custom domain expired notification requires a periodic task to - # remove the old notification and create a new one pointing to this - # ``message_id`` - Message( - id="project:domain:validation-expired", - header=_("Validation of custom domain expired: {domain}"), - body=_( - """ - The validation period of your custom domain {domain} has ended. - Go to the domain page and click on "Save" to restart the process. - """ - ), - type=INFO, - ), -] - SUBSCRIPTION_MESSAGES = [] USER_MESSAGES = [ Message( @@ -423,13 +392,32 @@ def get_rendered_body(self): ), ] -# Merge all the notifications into one object -NOTIFICATION_MESSAGES = {} -for message in itertools.chain( - CONFIG_YAML_MESSAGES, - BUILD_MKDOCS_MESSAGES, - BUILD_MESSAGES, - SUBSCRIPTION_MESSAGES, - USER_MESSAGES, -): - NOTIFICATION_MESSAGES[message.id] = message + +class MessagesRegistry: + def __init__(self): + self.messages = {} + + def add(self, messages): + if not isinstance(messages, list): + if not isinstance(messages, Message): + raise ValueError( + "A message should be instance of Message or a list of Messages." + ) + + messages = [messages] + + for message in messages: + if message.id in messages: + raise ValueError("A message with the same 'id' is already registered.") + self.messages[message.id] = message + + def get(self, message_id): + return self.messages.get(message_id) + + +registry = MessagesRegistry() +registry.add(CONFIG_YAML_MESSAGES) +registry.add(BUILD_MKDOCS_MESSAGES) +registry.add(BUILD_MESSAGES) +registry.add(SUBSCRIPTION_MESSAGES) +registry.add(USER_MESSAGES) diff --git a/readthedocs/notifications/models.py b/readthedocs/notifications/models.py index 26d531c2cd6..b97f4ab4e95 100644 --- a/readthedocs/notifications/models.py +++ b/readthedocs/notifications/models.py @@ -6,7 +6,7 @@ from django_extensions.db.models import TimeStampedModel from .constants import CANCELLED, DISMISSED, READ, UNREAD -from .messages import NOTIFICATION_MESSAGES +from .messages import registry class Notification(TimeStampedModel): @@ -51,7 +51,7 @@ def __str__(self): return self.message_id def get_message(self): - message = NOTIFICATION_MESSAGES.get(self.message_id) + message = registry.get(self.message_id) # Pass the instance attached to this notification all_format_values = { From ff43348c1170ec7e864139128b59c991a9bb7e12 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 13 Dec 2023 17:10:24 +0100 Subject: [PATCH 028/101] 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. --- readthedocs/domains/tasks.py | 2 +- readthedocs/notifications/models.py | 5 +++++ readthedocs/projects/notifications.py | 21 --------------------- readthedocs/projects/views/private.py | 18 ++++++++++++------ 4 files changed, 18 insertions(+), 28 deletions(-) delete mode 100644 readthedocs/projects/notifications.py diff --git a/readthedocs/domains/tasks.py b/readthedocs/domains/tasks.py index 812d43eabb6..c419bc4c7a9 100644 --- a/readthedocs/domains/tasks.py +++ b/readthedocs/domains/tasks.py @@ -33,7 +33,7 @@ def email_pending_custom_domains(number_of_emails=3): # The new behavior is to attach it to the project. # # We send an email notification to all the project's admins. - Notification.objects.create( + Notification.objects.add( message_id="project:domain:validation-pending", attached_to=domain.project, format_values={ diff --git a/readthedocs/notifications/models.py b/readthedocs/notifications/models.py index b97f4ab4e95..4e3f06cff9f 100644 --- a/readthedocs/notifications/models.py +++ b/readthedocs/notifications/models.py @@ -7,6 +7,7 @@ from .constants import CANCELLED, DISMISSED, READ, UNREAD from .messages import registry +from .querysets import NotificationQuerySet class Notification(TimeStampedModel): @@ -47,6 +48,10 @@ class Notification(TimeStampedModel): # Store values known at creation time that are required to render the final message format_values = models.JSONField(null=True, blank=True) + # Use a custom manager with an ``.add()`` method that deduplicates + # notifications attached to the same object. + objects = NotificationQuerySet.as_manager() + def __str__(self): return self.message_id diff --git a/readthedocs/projects/notifications.py b/readthedocs/projects/notifications.py deleted file mode 100644 index 262ce12ea3f..00000000000 --- a/readthedocs/projects/notifications.py +++ /dev/null @@ -1,21 +0,0 @@ -"""Project notifications.""" - -from django.urls import reverse -from django.utils.translation import gettext_lazy as _ -from messages_extends.constants import ERROR_PERSISTENT - -from readthedocs.notifications import SiteNotification - - -# TODO: migrate this communication to the new system, attached to a User -class EmailConfirmNotification(SiteNotification): - failure_level = ERROR_PERSISTENT - failure_message = _( - "Your primary email address is not verified. " - 'Please verify it here.', - ) - - def get_context_data(self): - context = super().get_context_data() - context.update({"account_email_url": reverse("account_email")}) - return context diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 132f984ad0a..21963342570 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -39,8 +39,10 @@ ) from readthedocs.core.history import UpdateChangeReasonPostView from readthedocs.core.mixins import ListViewWithForm, PrivateViewMixin +from readthedocs.core.notifications import MESSAGE_EMAIL_VALIDATION_PENDING from readthedocs.integrations.models import HttpExchange, Integration from readthedocs.invitations.models import Invitation +from readthedocs.notifications.models import Notification from readthedocs.oauth.services import registry from readthedocs.oauth.tasks import attach_webhook from readthedocs.oauth.utils import update_webhook @@ -71,7 +73,6 @@ ProjectRelationship, WebHook, ) -from readthedocs.projects.notifications import EmailConfirmNotification from readthedocs.projects.tasks.utils import clean_project_resources from readthedocs.projects.utils import get_csv_file from readthedocs.projects.views.base import ProjectAdminMixin @@ -113,18 +114,23 @@ def get_context_data(self, **kwargs): def validate_primary_email(self, user): """ - Sends a persistent error notification. + Sends a dismissable site notification to this user. Checks if the user has a primary email or if the primary email - is verified or not. Sends a persistent error notification if + is verified or not. Sends a dismissable notification if either of the condition is False. """ email_qs = user.emailaddress_set.filter(primary=True) email = email_qs.first() if not email or not email.verified: - # TODO: migrate this notification to the new system - notification = EmailConfirmNotification(user=user, success=False) - notification.send() + Notification.objects.add( + attached_to=user, + message_id=MESSAGE_EMAIL_VALIDATION_PENDING, + dismissable=True, + format_values={ + "account_email_url": reverse("account_email"), + }, + ) def get_queryset(self): sort = self.request.GET.get('sort') From 15c851743bd7d3f6639b09cd937946a69b5d828d Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 13 Dec 2023 17:16:48 +0100 Subject: [PATCH 029/101] Small refactor to email notifications --- readthedocs/subscriptions/notifications.py | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/readthedocs/subscriptions/notifications.py b/readthedocs/subscriptions/notifications.py index 41e11281572..e6712f40c6a 100644 --- a/readthedocs/subscriptions/notifications.py +++ b/readthedocs/subscriptions/notifications.py @@ -1,6 +1,3 @@ -# TODO: all these notification should be migrated to the new notification system. -# Most / All of them will be attached to an Organization - """Organization level notifications.""" @@ -11,13 +8,12 @@ from readthedocs.subscriptions.constants import DISABLE_AFTER_DAYS -# NOTE: these notifications need to be re-written using the new system. -# However, the email notification should keep being working as-is. class SubscriptionNotificationMixin: """Force to read templates from the subscriptions app.""" app_templates = "subscriptions" + context_object_name = "organization" class TrialEndingNotification(SubscriptionNotificationMixin, EmailNotification): @@ -25,7 +21,6 @@ class TrialEndingNotification(SubscriptionNotificationMixin, EmailNotification): """Trial is ending, nudge user towards subscribing.""" name = "trial_ending" - context_object_name = "organization" subject = "Your trial is ending soon" @staticmethod @@ -44,7 +39,6 @@ class SubscriptionRequiredNotification( """Trial has ended, push user into subscribing.""" name = "subscription_required" - context_object_name = "organization" subject = "We hope you enjoyed your trial of Read the Docs!" @@ -58,7 +52,6 @@ class SubscriptionEndedNotification(SubscriptionNotificationMixin, EmailNotifica """ name = "subscription_ended" - context_object_name = "organization" subject = "Your subscription to Read the Docs has ended" @@ -75,7 +68,6 @@ class OrganizationDisabledNotification( """ name = "organization_disabled" - context_object_name = "organization" subject = "Your Read the Docs organization will be disabled soon" days_after_end = DISABLE_AFTER_DAYS From 82637ec06cf2d55cdab94c23d3414a494450ef1d Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 14 Dec 2023 11:05:36 +0100 Subject: [PATCH 030/101] 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. --- .../builds/static-src/builds/js/detail.js | 21 +++++++++-- readthedocs/builds/static/builds/js/detail.js | 2 +- .../templates/builds/build_detail.html | 35 ++++++++++++++----- .../templates/core/project_bar_base.html | 21 +++++------ 4 files changed, 56 insertions(+), 23 deletions(-) diff --git a/readthedocs/builds/static-src/builds/js/detail.js b/readthedocs/builds/static-src/builds/js/detail.js index 60ae8c2c6b5..3cb170a881b 100644 --- a/readthedocs/builds/static-src/builds/js/detail.js +++ b/readthedocs/builds/static-src/builds/js/detail.js @@ -28,6 +28,7 @@ function BuildCommand(data) { function BuildDetailView(instance) { var self = this; var instance = instance || {}; + var poll_api_counts = 0; /* Instance variables */ self.state = ko.observable(instance.state); @@ -63,10 +64,8 @@ function BuildDetailView(instance) { self.legacy_output(true); }; + function poll_api() { - if (self.finished()) { - return; - } $.getJSON('/api/v2/build/' + instance.id + '/', function (data) { self.state(data.state); self.state_display(data.state_display); @@ -77,6 +76,9 @@ function BuildDetailView(instance) { self.commit(data.commit); self.docs_url(data.docs_url); self.commit_url(data.commit_url); + + poll_api_counts = poll_api_counts + 1; + var n; for (n in data.commands) { var command = data.commands[n]; @@ -92,6 +94,19 @@ function BuildDetailView(instance) { } }); + if (self.finished()) { + // HACK: this is a small hack to avoid re-implementing the new notification system + // in the old dashboard with Knockout. + // The notifications are rendered properly via Django template in a static way. + // So, we refresh the page once the build has finished to make Django render the notifications. + // We use a check of 1 API poll for those builds that are already finished when opened. + // The new dashboard will implement the new notification system in a nicer way using APIv3. + if (poll_api_counts !== 1) { + location.reload(); + } + return; + } + setTimeout(poll_api, 2000); } diff --git a/readthedocs/builds/static/builds/js/detail.js b/readthedocs/builds/static/builds/js/detail.js index 8662e9f08e4..7cd3809b6ad 100644 --- a/readthedocs/builds/static/builds/js/detail.js +++ b/readthedocs/builds/static/builds/js/detail.js @@ -1 +1 @@ -require=function r(s,n,u){function i(t,e){if(!n[t]){if(!s[t]){var o="function"==typeof require&&require;if(!e&&o)return o(t,!0);if(c)return c(t,!0);throw(e=new Error("Cannot find module '"+t+"'")).code="MODULE_NOT_FOUND",e}o=n[t]={exports:{}},s[t][0].call(o.exports,function(e){return i(s[t][1][e]||e)},o,o.exports,r,s,n,u)}return n[t].exports}for(var c="function"==typeof require&&require,e=0;e

@@ -169,6 +170,12 @@

{% endif %} + {% comment %} + NOTE: I'm not migrating "deprecated build image used" nor "deprecated config used" + since these notifications are not useful anymore. + The YAML config file is required now and it's not possible to use `build.image` anymore. + We should probably remove these blocks completely now. + {# This message is not dynamic and only appears when loading the page after the build has finished #} {% if build.finished and build.deprecated_build_image_used %}
@@ -194,19 +201,19 @@

{% endif %} + {% endcomment %} {% endif %} - {% if build.finished and build.config.build.commands %} -
-

- {% blocktrans trimmed with config_file_link="https://docs.readthedocs.io/page/build-customization.html#override-the-build-process" %} - The "build.commands" feature is in beta, and could have backwards incompatible changes while in beta. - Read more at our documentation to find out its limitations and potential issues. - {% endblocktrans %} -

-
+ {# Show non error notifications as "build-ideas" #} + {% for notification in build.notifications.all %} + {% if notification.get_message.type != "error" %} +

+ {{ notification.get_message.get_rendered_body|safe }} +

{% endif %} + {% endfor %} + {% if build.output %} {# If we have build output, this is an old build #} @@ -240,6 +247,16 @@

{% trans "Build Errors" %}

{% else %} {# Show new command output if lacking build.output #} + {# Show error notifications as "build-error" #} + {% for notification in build.notifications.all %} + {% if notification.get_message.type == "error" %} +
+

{{ notification.get_message.type|title }}

+

{{ notification.get_message.get_rendered_body|safe }}

+
+ {% endif %} + {% endfor %} +