From 0c901213db0a605fc79f5706e50d6ce2cd861149 Mon Sep 17 00:00:00 2001 From: Anthony Johnson Date: Mon, 5 Feb 2024 12:58:27 -0800 Subject: [PATCH 1/3] Some fixes for notifications This includes a few fixes and small restructuring to the notification/message classes. - Centralize message lookup and format string population on the registry model instead of the Notification model. This change will make more sense with my next PR - Use `copy()` when setting the format values, as to ensure thread safety, these shouldn't be set on the static instance class in the registry. - Add a base class for notifications, for messages/notifications that fall outside doc building. --- readthedocs/doc_builder/exceptions.py | 12 +++++------- readthedocs/notifications/exceptions.py | 18 ++++++++++++++++++ readthedocs/notifications/messages.py | 16 ++++++++++++++-- readthedocs/notifications/models.py | 19 ++++++------------- 4 files changed, 43 insertions(+), 22 deletions(-) create mode 100644 readthedocs/notifications/exceptions.py diff --git a/readthedocs/doc_builder/exceptions.py b/readthedocs/doc_builder/exceptions.py index 0e67422a6b4..cb7cd4a5dbf 100644 --- a/readthedocs/doc_builder/exceptions.py +++ b/readthedocs/doc_builder/exceptions.py @@ -11,20 +11,18 @@ Then the header/body texts should be defined in ``readthedocs/notifications/messages.py``. """ +from django.utils.translation import gettext_lazy as _ +from readthedocs.notifications.exceptions import NotificationBaseException -class BuildBaseException(Exception): - default_message = "Build user exception" +class BuildBaseException(NotificationBaseException): - def __init__(self, message_id, format_values=None, **kwargs): - self.message_id = message_id - self.format_values = format_values - super().__init__(self.default_message, **kwargs) + default_message = _("Build user exception") class BuildAppError(BuildBaseException): - default_message = "Build app exception" + default_message = _("Build application exception") GENERIC_WITH_BUILD_ID = "build:app:generic-with-build-id" BUILDS_DISABLED = "build:app:project-builds-disabled" diff --git a/readthedocs/notifications/exceptions.py b/readthedocs/notifications/exceptions.py new file mode 100644 index 00000000000..be9b7833eea --- /dev/null +++ b/readthedocs/notifications/exceptions.py @@ -0,0 +1,18 @@ +from django.utils.translation import gettext_lazy as _ + + +class NotificationBaseException(Exception): + + """ + The base exception class for notification and messages. + + This provides the additional ``message_id`` and ``format_values`` attributes + that are used for message display and registry lookup. + """ + + default_message = _("Undefined error") + + def __init__(self, message_id, format_values=None, **kwargs): + self.message_id = message_id + self.format_values = format_values + super().__init__(self.default_message, **kwargs) diff --git a/readthedocs/notifications/messages.py b/readthedocs/notifications/messages.py index 28c43b453ff..e1238d0564c 100644 --- a/readthedocs/notifications/messages.py +++ b/readthedocs/notifications/messages.py @@ -1,9 +1,11 @@ +import copy import textwrap import structlog from django.template import Context, Template from django.utils.translation import gettext_noop as _ +from readthedocs.core.context_processors import readthedocs_processor from readthedocs.doc_builder.exceptions import ( BuildAppError, BuildCancelled, @@ -514,8 +516,18 @@ def add(self, 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) + def get(self, message_id, format_values={}): + # Copy to avoid setting format values on the static instance of the + # message inside the registry, set on a per-request instance instead. + message = copy.copy(self.messages.get(message_id)) + + if message is not None: + # Always include global variables, override with provided values + all_format_values = readthedocs_processor(None) + all_format_values.update(format_values or {}) + message.set_format_values(all_format_values) + + return message registry = MessagesRegistry() diff --git a/readthedocs/notifications/models.py b/readthedocs/notifications/models.py index 0d1e8942ba2..f583e0a99f8 100644 --- a/readthedocs/notifications/models.py +++ b/readthedocs/notifications/models.py @@ -7,7 +7,6 @@ from django.utils.translation import gettext_noop as _ from django_extensions.db.models import TimeStampedModel -from readthedocs.core.context_processors import readthedocs_processor from .constants import CANCELLED, DISMISSED, READ, UNREAD, WARNING from .messages import Message, registry @@ -67,7 +66,12 @@ def __str__(self): return self.message_id def get_message(self): - message = registry.get(self.message_id) + # Pass the instance attached to this notification + format_values = { + "instance": self.attached_to, + } + + message = registry.get(self.message_id, format_values=format_values) if message is None: # Log the error and return an unknown error to avoid breaking the response. log.error( @@ -88,15 +92,4 @@ def get_message(self): type=WARNING, ) - # Pass the instance attached to this notification - all_format_values = { - "instance": self.attached_to, - } - - # Always include global variables - all_format_values.update(readthedocs_processor(None)) - - # Pass the values stored in the database - all_format_values.update(self.format_values or {}) - message.set_format_values(all_format_values) return message From b133c136b0e92e4b834f3d64c5924fa7ebf2d300 Mon Sep 17 00:00:00 2001 From: Anthony Johnson Date: Mon, 5 Feb 2024 13:06:42 -0800 Subject: [PATCH 2/3] Fix argument type --- readthedocs/notifications/messages.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/notifications/messages.py b/readthedocs/notifications/messages.py index e1238d0564c..ed963a52831 100644 --- a/readthedocs/notifications/messages.py +++ b/readthedocs/notifications/messages.py @@ -516,7 +516,7 @@ def add(self, messages): raise ValueError("A message with the same 'id' is already registered.") self.messages[message.id] = message - def get(self, message_id, format_values={}): + def get(self, message_id, format_values=None): # Copy to avoid setting format values on the static instance of the # message inside the registry, set on a per-request instance instead. message = copy.copy(self.messages.get(message_id)) From eb95a2059c9284bcf5c1c38e17cc0940dc3e689a Mon Sep 17 00:00:00 2001 From: Anthony Johnson Date: Mon, 5 Feb 2024 13:12:45 -0800 Subject: [PATCH 3/3] Initial example of using notification system for view-specific errors I found myself rebuilding these messages in template code and figured it would be a good excuse to further consolidate the patterns we're using. In short, these messages are emitted at view time and are not saved to the database. The view time logic is similar as notifications however. A few questions stand for me: - I like centering all of our notifications/errors around exceptions, as it retains an opportunity to emit exceptions from wherever we might want to move some of this logic. Arguably, this logic should not exist on a single view and could be moved on to modeling/queryset classes. - I don't have a strong opinion on whether it's important for these messages to exist in the registry. I see arguments for both, but I chose to use the registry so that we can consolidate logic there -- format values and message lookup, etc. I'm looking for input on these patterns. On the template side, the display is now just a simple `if` and template logic similar to the current notification display, using the header/body render methods. --- readthedocs/projects/exceptions.py | 18 +++++++++ readthedocs/projects/notifications.py | 45 +++++++++++++++++++++ readthedocs/projects/views/private.py | 57 +++++++++++++++++++++++++-- 3 files changed, 117 insertions(+), 3 deletions(-) diff --git a/readthedocs/projects/exceptions.py b/readthedocs/projects/exceptions.py index 724129b195f..541d8b1f35f 100644 --- a/readthedocs/projects/exceptions.py +++ b/readthedocs/projects/exceptions.py @@ -2,6 +2,7 @@ from readthedocs.doc_builder.exceptions import BuildAppError, BuildUserError +from readthedocs.notifications.exceptions import NotificationBaseException class ProjectConfigurationError(BuildUserError): @@ -32,3 +33,20 @@ class SyncRepositoryLocked(BuildAppError): """Error risen when there is another sync_repository_task already running.""" REPOSITORY_LOCKED = "project:repository:locked" + + +class ProjectAutomaticCreationDisallowed(NotificationBaseException): + + """Used for raising messages in the project automatic create form view""" + + NO_CONNECTED_ACCOUNT = "project:create:automatic:no-connected-account" + SSO_ENABLED = "project:create:automatic:sso-enabled" + INADEQUATE_PERMISSIONS = "project:create:automatic:inadequate-permissions" + + +class ProjectManualCreationDisallowed(NotificationBaseException): + + """Used for raising messages in the project manual create form view""" + + SSO_ENABLED = "project:create:manual:sso-enabled" + INADEQUATE_PERMISSIONS = "project:create:manual:inadequate-permissions" diff --git a/readthedocs/projects/notifications.py b/readthedocs/projects/notifications.py index e45d78ea998..ab3cde75d52 100644 --- a/readthedocs/projects/notifications.py +++ b/readthedocs/projects/notifications.py @@ -6,7 +6,9 @@ from readthedocs.notifications.constants import ERROR, INFO, WARNING from readthedocs.notifications.messages import Message, registry from readthedocs.projects.exceptions import ( + ProjectAutomaticCreationDisallowed, ProjectConfigurationError, + ProjectManualCreationDisallowed, RepositoryError, SyncRepositoryLocked, UserFileNotFound, @@ -145,5 +147,48 @@ ), type=WARNING, ), + # The following messages are added to the registry but are only used + # directly by the project creation view template. These could be split out + # directly for use by import, instead of reusing message id lookup. + Message( + id=ProjectAutomaticCreationDisallowed.NO_CONNECTED_ACCOUNT, + header=_("No connected services found"), + body=_( + textwrap.dedent( + # Translators: "connected service" refers to the user setting page for "Connected Services" + """ + You must first add a connected service to your account + to enable automatic configuration of repositories. + """ + ).strip(), + ), + type=WARNING, + ), + Message( + id=ProjectAutomaticCreationDisallowed.SSO_ENABLED, + header=_("Organization single sign-on enabled"), + body=_( + textwrap.dedent( + """ + Only organization owners may create new projects when single + sign-on is enabled. + """ + ).strip(), + ), + type=WARNING, + ), + Message( + id=ProjectManualCreationDisallowed.SSO_ENABLED, + header=_("Organization single sign-on enabled"), + body=_( + textwrap.dedent( + """ + Projects cannot be manually configured when single sign-on is + enabled. + """ + ).strip(), + ), + type=WARNING, + ), ] registry.add(messages) diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 4068063a937..8ec5ffe9fab 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -42,10 +42,15 @@ 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.messages import registry as messages_registry 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 +from readthedocs.projects.exceptions import ( + ProjectAutomaticCreationDisallowed, + ProjectManualCreationDisallowed, +) from readthedocs.projects.filters import ProjectListFilterSet from readthedocs.projects.forms import ( AddonsConfigForm, @@ -398,9 +403,55 @@ def post(self, request, *args, **kwargs): def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) context['view_csrf_token'] = get_token(self.request) - context['has_connected_accounts'] = SocialAccount.objects.filter( - user=self.request.user, - ).exists() + + if settings.RTD_EXT_THEME_ENABLED: + error_import_manually = None + error_import_automatically = None + + has_connected_account = SocialAccount.objects.filter( + user=self.request.user, + ).exists() + + # First check ability for automatic project creation + # NOTE that try catch is used here, but isn't super useful yet. But + # if it makes more sense to house this logic outside this view, + # somewhere central to organization/user modeling, then we don't + # have to change any code here. + try: + if has_connected_account: + raise ProjectAutomaticCreationDisallowed( + message_id=ProjectAutomaticCreationDisallowed.NO_CONNECTED_ACCOUNT, + format_values={ + "url": reverse("socialaccount_connections"), + }, + ) + # TODO if organization sso enabled and user is not an owner + if True: + raise ProjectAutomaticCreationDisallowed( + message_id=ProjectAutomaticCreationDisallowed.SSO_ENABLED, + ) + except ProjectAutomaticCreationDisallowed as exc: + error_import_automatically = messages_registry.get( + exc.message_id, + format_values=exc.format_values, + ) + + # Next check ability for manual project creation + try: + # TODO if organization sso enabled + if True: + raise ProjectManualCreationDisallowed( + ProjectManualCreationDisallowed.SSO_ENABLED + ) + except ProjectManualCreationDisallowed as exc: + error_import_manually = messages_registry.get( + exc.message_id, + format_values=exc.format_values, + ) + + context["error_import_automatically"] = error_import_automatically + context["error_import_manually"] = error_import_manually + return context