diff --git a/common b/common index cadcca43d33..9e0ca9f3e99 160000 --- a/common +++ b/common @@ -1 +1 @@ -Subproject commit cadcca43d33cd9211810873632136bed098970c0 +Subproject commit 9e0ca9f3e99a14d22ac804422094513a468fcf3a diff --git a/readthedocs/api/v2/serializers.py b/readthedocs/api/v2/serializers.py index 4756eceb675..560ecc440d2 100644 --- a/readthedocs/api/v2/serializers.py +++ b/readthedocs/api/v2/serializers.py @@ -2,11 +2,15 @@ from allauth.socialaccount.models import SocialAccount +from django.core.exceptions import ObjectDoesNotExist +from django.utils.translation import gettext as _ +from generic_relations.relations import GenericRelatedField from rest_framework import serializers from readthedocs.api.v2.utils import normalize_build_command from readthedocs.builds.models import Build, BuildCommandResult, Version from readthedocs.core.resolver import Resolver +from readthedocs.notifications.models import Notification from readthedocs.oauth.models import RemoteOrganization, RemoteRepository from readthedocs.projects.models import Domain, Project @@ -378,3 +382,53 @@ def get_username(self, obj): or obj.extra_data.get("login") # FIXME: which one is GitLab? ) + + +class NotificationAttachedToRelatedField(serializers.RelatedField): + + """ + Attached to related field for Notifications. + + Used together with ``rest-framework-generic-relations`` to accept multiple object types on ``attached_to``. + + See https://github.com/LilyFoote/rest-framework-generic-relations + """ + + default_error_messages = { + "required": _("This field is required."), + "does_not_exist": _("Object does not exist."), + "incorrect_type": _( + "Incorrect type. Expected URL string, received {data_type}." + ), + } + + def to_representation(self, value): + return f"{self.queryset.model._meta.model_name}/{value.pk}" + + def to_internal_value(self, data): + # TODO: handle exceptions + model, pk = data.strip("/").split("/") + if self.queryset.model._meta.model_name != model: + self.fail("incorrect_type") + + try: + return self.queryset.get(pk=pk) + except (ObjectDoesNotExist, ValueError, TypeError): + self.fail("does_not_exist") + + +class NotificationSerializer(serializers.ModelSerializer): + # Accept different object types (Project, Build, User, etc) depending on what the notification is attached to. + # The client has to send a value like "/". + # Example: "build/3522" will attach the notification to the Build object with id 3522 + attached_to = GenericRelatedField( + { + Build: NotificationAttachedToRelatedField(queryset=Build.objects.all()), + Project: NotificationAttachedToRelatedField(queryset=Project.objects.all()), + }, + required=True, + ) + + class Meta: + model = Notification + exclude = ["attached_to_id", "attached_to_content_type"] diff --git a/readthedocs/api/v2/urls.py b/readthedocs/api/v2/urls.py 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..8498cd26af2 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,42 @@ def get_queryset_for_api_key(self, api_key): return self.model.objects.filter(build__project=api_key.project) +class NotificationViewSet(DisableListEndpoint, CreateModelMixin, UserSelectViewSet): + + """ + Create a notification attached to an object (User, Project, Build, Organization). + + This endpoint is currently used only internally by the builder. + Notifications are attached to `Build` objects only when using this endpoint. + This limitation will change in the future when re-implementing this on APIv3 if neeed. + """ + + parser_classes = [JSONParser, MultiPartParser] + permission_classes = [HasBuildAPIKey] + renderer_classes = (JSONRenderer,) + serializer_class = NotificationSerializer + model = Notification + + def perform_create(self, serializer): + """Restrict creation to notifications attached to the project's builds from the api key.""" + attached_to = serializer.validated_data["attached_to"] + + build_api_key = self.request.build_api_key + + project_slug = None + if isinstance(attached_to, Build): + project_slug = attached_to.project.slug + elif isinstance(attached_to, Project): + project_slug = attached_to.slug + + # Limit the permissions to create a notification on this object only if the API key + # is attached to the related project + if not project_slug or build_api_key.project.slug != project_slug: + raise PermissionDenied() + + return super().perform_create(serializer) + + class DomainViewSet(DisableListEndpoint, UserSelectViewSet): permission_classes = [ReadOnlyPermission] renderer_classes = (JSONRenderer,) diff --git a/readthedocs/api/v3/serializers.py b/readthedocs/api/v3/serializers.py index e0c9237388d..21b1c30eced 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 ( @@ -60,10 +61,33 @@ class Meta: fields = [] +# TODO: decide whether or not include a `_links` field on the object +# +# This also includes adding `/api/v3/notifications/` endpoint, +# which I'm not sure it's useful at this point. +# +# class NotificationLinksSerializer(BaseLinksSerializer): +# _self = serializers.SerializerMethodField() +# attached_to = serializers.SerializerMethodField() + +# def get__self(self, obj): +# path = reverse( +# "notifications-detail", +# kwargs={ +# "pk": obj.pk, +# }, +# ) +# return self._absolute_url(path) + +# def get_attached_to(self, obj): +# return None + + class BuildLinksSerializer(BaseLinksSerializer): _self = serializers.SerializerMethodField() version = serializers.SerializerMethodField() project = serializers.SerializerMethodField() + notifications = serializers.SerializerMethodField() def get__self(self, obj): path = reverse( @@ -96,6 +120,16 @@ def get_project(self, obj): ) return self._absolute_url(path) + def get_notifications(self, obj): + path = reverse( + "project-builds-notifications-list", + kwargs={ + "parent_lookup_project__slug": obj.project.slug, + "parent_lookup_build__id": obj.pk, + }, + ) + return self._absolute_url(path) + class BuildURLsSerializer(BaseLinksSerializer, serializers.Serializer): build = serializers.URLField(source="get_full_url") @@ -190,6 +224,57 @@ def get_success(self, obj): return None +class NotificationMessageSerializer(serializers.Serializer): + id = serializers.SlugField() + header = serializers.CharField(source="get_rendered_header") + body = serializers.CharField(source="get_rendered_body") + type = serializers.CharField() + icon_classes = serializers.CharField(source="get_display_icon_classes") + + class Meta: + fields = [ + "id", + "header", + "body", + "type", + "icon_classes", + ] + + +class NotificationCreateSerializer(serializers.ModelSerializer): + class Meta: + model = Notification + fields = [ + "message_id", + "dismissable", + "news", + "state", + ] + + +class NotificationSerializer(serializers.ModelSerializer): + message = NotificationMessageSerializer(source="get_message") + attached_to_content_type = serializers.SerializerMethodField() + # TODO: review these fields + # _links = BuildLinksSerializer(source="*") + # urls = BuildURLsSerializer(source="*") + + class Meta: + model = Notification + fields = [ + "id", + "state", + "dismissable", + "news", + "attached_to_content_type", + "attached_to_id", + "message", + ] + + def get_attached_to_content_type(self, obj): + return obj.attached_to_content_type.name + + class VersionLinksSerializer(BaseLinksSerializer): _self = serializers.SerializerMethodField() builds = serializers.SerializerMethodField() diff --git a/readthedocs/api/v3/tests/mixins.py b/readthedocs/api/v3/tests/mixins.py index 8dfb7cf1b7d..3736cf30a32 100644 --- a/readthedocs/api/v3/tests/mixins.py +++ b/readthedocs/api/v3/tests/mixins.py @@ -26,6 +26,7 @@ ) class APIEndpointMixin(TestCase): fixtures = [] + maxDiff = None # So we get an actual diff when it fails def setUp(self): self.created = make_aware(datetime.datetime(2019, 4, 29, 10, 0, 0)) diff --git a/readthedocs/api/v3/tests/responses/projects-builds-detail.json b/readthedocs/api/v3/tests/responses/projects-builds-detail.json index 7f3b319f6e9..996bcef6e61 100644 --- a/readthedocs/api/v3/tests/responses/projects-builds-detail.json +++ b/readthedocs/api/v3/tests/responses/projects-builds-detail.json @@ -8,7 +8,8 @@ "_links": { "_self": "https://readthedocs.org/api/v3/projects/project/builds/1/", "project": "https://readthedocs.org/api/v3/projects/project/", - "version": "https://readthedocs.org/api/v3/projects/project/versions/v1.0/" + "version": "https://readthedocs.org/api/v3/projects/project/versions/v1.0/", + "notifications": "https://readthedocs.org/api/v3/projects/project/builds/1/notifications/" }, "urls": { "build": "https://readthedocs.org/projects/project/builds/1/", diff --git a/readthedocs/api/v3/tests/responses/projects-detail.json b/readthedocs/api/v3/tests/responses/projects-detail.json index 1b21a984bd2..a5266b94564 100644 --- a/readthedocs/api/v3/tests/responses/projects-detail.json +++ b/readthedocs/api/v3/tests/responses/projects-detail.json @@ -23,6 +23,7 @@ "id": 1, "_links": { "_self": "https://readthedocs.org/api/v3/projects/project/builds/1/", + "notifications": "https://readthedocs.org/api/v3/projects/project/builds/1/notifications/", "project": "https://readthedocs.org/api/v3/projects/project/", "version": "https://readthedocs.org/api/v3/projects/project/versions/v1.0/" }, diff --git a/readthedocs/api/v3/tests/responses/projects-versions-builds-list_POST.json b/readthedocs/api/v3/tests/responses/projects-versions-builds-list_POST.json index 987d72de209..18f392601a5 100644 --- a/readthedocs/api/v3/tests/responses/projects-versions-builds-list_POST.json +++ b/readthedocs/api/v3/tests/responses/projects-versions-builds-list_POST.json @@ -8,6 +8,7 @@ "id": 2, "_links": { "_self": "https://readthedocs.org/api/v3/projects/project/builds/2/", + "notifications": "https://readthedocs.org/api/v3/projects/project/builds/2/notifications/", "project": "https://readthedocs.org/api/v3/projects/project/", "version": "https://readthedocs.org/api/v3/projects/project/versions/v1.0/" }, diff --git a/readthedocs/api/v3/urls.py b/readthedocs/api/v3/urls.py index 310f1b00e88..528f576e0f1 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,28 @@ # allows /api/v3/projects/pip/builds/ # allows /api/v3/projects/pip/builds/1053/ -projects.register( +builds = projects.register( r"builds", BuildsViewSet, basename="projects-builds", parents_query_lookups=["project__slug"], ) +# NOTE: we are only listing notifications on APIv3 for now. +# The front-end will use this endpoint. +# allows /api/v3/projects/pip/builds/1053/notifications/ +builds.register( + r"notifications", + NotificationsViewSet, + basename="project-builds-notifications", + parents_query_lookups=["project__slug", "build__id"], +) + +# TODO: create an APIv3 endpoint to PATCH Build/Project notifications. +# This way the front-end can mark them as READ/DISMISSED. +# +# TODO: create an APIv3 endpoint to list notifications for Projects. + # allows /api/v3/projects/pip/redirects/ # allows /api/v3/projects/pip/redirects/1053/ projects.register( diff --git a/readthedocs/api/v3/views.py b/readthedocs/api/v3/views.py 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 82646cf5532..32f6b70afff 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.urls import reverse from django.utils import timezone @@ -58,8 +59,8 @@ get_vcs_url, ) 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, @@ -833,6 +834,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. @@ -1095,36 +1103,6 @@ def can_rebuild(self): def external_version_name(self): return external_version_name(self) - def deprecated_config_used(self): - """ - Check whether this particular build is using a deprecated config file. - - When using v1 or not having a config file at all, it returns ``True``. - Returns ``False`` only when it has a config file and it is using v2. - - Note we are using this to communicate deprecation of v1 file and not using a config file. - See https://github.com/readthedocs/readthedocs.org/issues/10342 - """ - if not self.config: - return True - - return int(self.config.get("version", "1")) != LATEST_CONFIGURATION_VERSION - - def deprecated_build_image_used(self): - """ - Check whether this particular build is using the deprecated "build.image" config. - - Note we are using this to communicate deprecation of "build.image". - See https://github.com/readthedocs/meta/discussions/48 - """ - if not self.config: - # Don't notify users without a config file. - # We hope they will migrate to `build.os` in the process of adding a `.readthedocs.yaml` - return False - - build_config_key = self.config.get("build", {}) - return "image" in build_config_key - def reset(self): """ Reset the build so it can be re-used when re-trying. 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{directory}. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ConfigError.KEY_NOT_SUPPORTED_IN_VERSION, + header=_("Configuration key not supported in this version"), + body=_( + textwrap.dedent( + """ + The {key} configuration option is not supported in this version. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ConfigError.PYTHON_SYSTEM_PACKAGES_REMOVED, + header=_("Invalid configuration key"), + body=_( + textwrap.dedent( + """ + The configuration key python.system_packages has been deprecated and removed. + Refer to https://blog.readthedocs.com/drop-support-system-packages/ to read more + about this change and how to upgrade your config file." + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ConfigError.PYTHON_USE_SYSTEM_SITE_PACKAGES_REMOVED, + header=_("Invalid configuration key"), + body=_( + textwrap.dedent( + """ + The configuration key python.use_system_site_packages has been deprecated and removed. + Refer to https://blog.readthedocs.com/drop-support-system-packages/ to read more + about this change and how to upgrade your config file." + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ConfigError.INVALID_VERSION, + header=_("Invalid configuration version"), + body=_( + textwrap.dedent( + """ + Invalid version of the configuration file. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ConfigError.GENERIC_INVALID_CONFIG_KEY, + header=_("Invalid configuration option"), + body=_( + textwrap.dedent( + """ + Invalid configuration option: {key}. + + Read the Docs configuration file: {source_file}. + + {error_message} + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ConfigError.NOT_BUILD_TOOLS_OR_COMMANDS, + header=_("Invalid configuration option: build"), + body=_( + textwrap.dedent( + """ + At least one item should be provided in "tools" or "commands". + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ConfigError.BUILD_JOBS_AND_COMMANDS, + header=_("Invalid configuration option"), + body=_( + textwrap.dedent( + """ + The keys build.jobs and build.commands can't be used together. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ConfigError.APT_INVALID_PACKAGE_NAME_PREFIX, + header=_("Invalid APT package name"), + body=_( + textwrap.dedent( + """ + The name of the packages (e.g. {package}) can't start with {prefix} + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ConfigError.APT_INVALID_PACKAGE_NAME, + header=_("Invalid APT package name"), + body=_( + textwrap.dedent( + """ + The name of the package {pacakge} is invalid. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ConfigError.USE_PIP_FOR_EXTRA_REQUIREMENTS, + header=_("Invalid Python install method"), + body=_( + textwrap.dedent( + """ + You need to install your project with pip to use extra_requirements. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ConfigError.PIP_PATH_OR_REQUIREMENT_REQUIRED, + header=_("Invalid configuration key"), + body=_( + textwrap.dedent( + """ + path or requirements key is required for python.install. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ConfigError.SPHINX_MKDOCS_CONFIG_TOGETHER, + header=_("Invalid configuration key"), + body=_( + textwrap.dedent( + """ + You can not have sphinx and mkdocs keys at the same time. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ConfigError.SUBMODULES_INCLUDE_EXCLUDE_TOGETHER, + header=_("Invalid configuration key"), + body=_( + textwrap.dedent( + """ + You can not have exclude and include submodules at the same time. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ConfigError.INVALID_KEY_NAME, + header=_("Invalid configuration key: {key}"), + body=_( + textwrap.dedent( + """ + Make sure the key name {key} is correct. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ConfigError.SYNTAX_INVALID, + header=_("Invalid syntax in configuration file"), + body=_( + textwrap.dedent( + """ + Error while parsing {filename}. + + {error_message} + """ + ).strip(), + ), + type=ERROR, + ), +] +registry.add(messages) + +# Validation errors +messages = [ + Message( + id=ConfigValidationError.INVALID_BOOL, + header=_("Config file validation error"), + body=_( + textwrap.dedent( + """ + Config validation error in {key}. + Expected one of (0, 1, true, false), got {value}. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ConfigValidationError.INVALID_CHOICE, + header=_("Config file validation error"), + body=_( + textwrap.dedent( + """ + Config validation error in {key}. + Expected one of ({choices}), got {value}. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ConfigValidationError.INVALID_DICT, + header=_("Config file validation error"), + body=_( + textwrap.dedent( + """ + Config validation error in {key}. + Expected a dictionary, got {value}. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ConfigValidationError.INVALID_PATH, + header=_("Config file validation error"), + body=_( + textwrap.dedent( + """ + Config validation error in {key}. + The path {value} does not exist. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ConfigValidationError.INVALID_PATH_PATTERN, + header=_("Config file validation error"), + body=_( + textwrap.dedent( + """ + Config validation error in {key}. + The path {value} is not a valid path pattern. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ConfigValidationError.INVALID_STRING, + header=_("Config file validation error"), + body=_( + textwrap.dedent( + """ + Config validation error in {key}. + Expected a string, got {value}. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ConfigValidationError.INVALID_LIST, + header=_("Config file validation error"), + body=_( + textwrap.dedent( + """ + Config validation error in {key}. + Expected a list, got {value}. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ConfigValidationError.VALUE_NOT_FOUND, + header=_("Config file validation error"), + body=_( + textwrap.dedent( + """ + Config validation error in {key}. + Value {value} not found. + """ + ).strip(), + ), + type=ERROR, + ), +] +registry.add(messages) diff --git a/readthedocs/config/tests/test_config.py b/readthedocs/config/tests/test_config.py index 8e73b6d8a9a..c5963bf7ef4 100644 --- a/readthedocs/config/tests/test_config.py +++ b/readthedocs/config/tests/test_config.py @@ -8,32 +8,15 @@ from django.test import override_settings from pytest import raises -from readthedocs.config import ( - ALL, - PIP, - SETUPTOOLS, - BuildConfigV2, - ConfigError, - DefaultConfigFileNotFound, - InvalidConfig, - load, -) -from readthedocs.config.config import ( - CONFIG_FILE_REQUIRED, - CONFIG_FILENAME_REGEX, - CONFIG_REQUIRED, - CONFIG_SYNTAX_INVALID, - INVALID_KEY, - INVALID_NAME, - VERSION_INVALID, -) +from readthedocs.config import ALL, PIP, SETUPTOOLS, BuildConfigV2, load +from readthedocs.config.config import CONFIG_FILENAME_REGEX +from readthedocs.config.exceptions import ConfigError, ConfigValidationError from readthedocs.config.models import ( BuildJobs, BuildWithOs, PythonInstall, PythonInstallRequirements, ) -from readthedocs.config.validation import VALUE_NOT_FOUND, ValidationError from .utils import apply_fs @@ -73,11 +56,10 @@ def get_build_config(config, source_file="readthedocs.yml", validate=False): def test_load_no_config_file(tmpdir, files): apply_fs(tmpdir, files) base = str(tmpdir) - with raises(DefaultConfigFileNotFound) as e: + with raises(ConfigError) as e: with override_settings(DOCROOT=tmpdir): load(base, {}) - assert e.value.code == CONFIG_FILE_REQUIRED - + assert e.value.message_id == ConfigError.DEFAULT_PATH_NOT_FOUND def test_load_empty_config_file(tmpdir): apply_fs( @@ -124,7 +106,7 @@ def test_load_unknow_version(tmpdir): with raises(ConfigError) as excinfo: with override_settings(DOCROOT=tmpdir): load(base, {}) - assert excinfo.value.code == VERSION_INVALID + assert excinfo.value.message_id == ConfigError.INVALID_VERSION def test_load_raise_exception_invalid_syntax(tmpdir): @@ -146,7 +128,7 @@ def test_load_raise_exception_invalid_syntax(tmpdir): with raises(ConfigError) as excinfo: with override_settings(DOCROOT=tmpdir): load(base, {}) - assert excinfo.value.code == CONFIG_SYNTAX_INVALID + assert excinfo.value.message_id == ConfigError.SYNTAX_INVALID def test_load_non_default_filename(tmpdir): @@ -232,14 +214,6 @@ def test_version(self): build = get_build_config({}) assert build.version == "2" - def test_correct_error_when_source_is_dir(self, tmpdir): - build = get_build_config({}, source_file=str(tmpdir)) - with raises(InvalidConfig) as excinfo: - build.error(key="key", message="Message", code="code") - # We don't have any extra information about - # the source_file. - assert str(excinfo.value) == 'Invalid configuration option "key": Message' - def test_formats_check_valid(self): build = get_build_config({"formats": ["htmlzip", "pdf", "epub"]}) build.validate() @@ -248,17 +222,19 @@ def test_formats_check_valid(self): @pytest.mark.parametrize("value", [3, "invalid", {"other": "value"}]) def test_formats_check_invalid_value(self, value): build = get_build_config({"formats": value}) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "formats" + assert excinfo.value.message_id == ConfigValidationError.INVALID_LIST + assert excinfo.value.format_values.get("key") == "formats" def test_formats_check_invalid_type(self): build = get_build_config( {"formats": ["htmlzip", "invalid", "epub"]}, ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "formats" + assert excinfo.value.message_id == ConfigValidationError.INVALID_CHOICE + assert excinfo.value.format_values.get("key") == "formats" def test_formats_default_value(self): build = get_build_config({}) @@ -309,36 +285,41 @@ def test_conda_check_valid(self, tmpdir): @pytest.mark.parametrize("value", [3, [], "invalid"]) def test_conda_check_invalid_value(self, value): build = get_build_config({"conda": value}) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "conda" + assert excinfo.value.message_id == ConfigValidationError.INVALID_DICT + assert excinfo.value.format_values.get("key") == "conda" @pytest.mark.parametrize("value", [3, [], {}]) def test_conda_check_invalid_file_value(self, value): build = get_build_config({"conda": {"file": value}}) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "conda.environment" + assert excinfo.value.message_id == ConfigValidationError.VALUE_NOT_FOUND + assert excinfo.value.format_values.get("key") == "conda.environment" def test_conda_check_file_required(self): build = get_build_config({"conda": {"no-file": "other"}}) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "conda.environment" + assert excinfo.value.message_id == ConfigValidationError.VALUE_NOT_FOUND + assert excinfo.value.format_values.get("key") == "conda.environment" @pytest.mark.parametrize("value", [3, [], "invalid"]) def test_build_check_invalid_type(self, value): build = get_build_config({"build": value}) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "build" + assert excinfo.value.message_id == ConfigValidationError.INVALID_DICT + assert excinfo.value.format_values.get("key") == "build" @pytest.mark.parametrize("value", [3, [], {}]) def test_build_image_check_invalid_type(self, value): build = get_build_config({"build": {"image": value}}) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "build.os" + assert excinfo.value.message_id == ConfigValidationError.VALUE_NOT_FOUND + assert excinfo.value.format_values.get("key") == "build.os" @pytest.mark.parametrize("value", ["", None, "latest"]) def test_new_build_config_invalid_os(self, value): @@ -350,9 +331,10 @@ def test_new_build_config_invalid_os(self, value): }, }, ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "build.os" + assert excinfo.value.message_id == ConfigValidationError.INVALID_CHOICE + assert excinfo.value.format_values.get("key") == "build.os" @pytest.mark.parametrize( "value", ["", None, "python", ["python", "nodejs"], {}, {"cobol": "99"}] @@ -366,9 +348,17 @@ def test_new_build_config_invalid_tools(self, value): }, }, ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "build.tools" + + # TODO: split this test to check specific errors now we have better messages + assert excinfo.value.message_id in ( + ConfigError.NOT_BUILD_TOOLS_OR_COMMANDS, + ConfigValidationError.INVALID_DICT, + ConfigValidationError.VALUE_NOT_FOUND, + ConfigValidationError.INVALID_CHOICE, + ) + assert excinfo.value.format_values.get("key") in ("build.tools", "build") def test_new_build_config_invalid_tools_version(self): build = get_build_config( @@ -379,9 +369,10 @@ def test_new_build_config_invalid_tools_version(self): }, }, ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "build.tools.python" + assert excinfo.value.message_id == ConfigValidationError.INVALID_CHOICE + assert excinfo.value.format_values.get("key") == "build.tools.python" def test_new_build_config(self): build = get_build_config( @@ -410,9 +401,10 @@ def test_new_build_config_conflict_with_build_image(self): }, }, ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "build.image" + assert excinfo.value.message_id == ConfigError.INVALID_KEY_NAME + assert excinfo.value.format_values.get("key") == "build.image" def test_new_build_config_conflict_with_build_python_version(self): build = get_build_config( @@ -424,9 +416,10 @@ def test_new_build_config_conflict_with_build_python_version(self): "python": {"version": "3.8"}, }, ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "python.version" + assert excinfo.value.message_id == ConfigError.INVALID_KEY_NAME + assert excinfo.value.format_values.get("key") == "python.version" def test_commands_build_config_tools_and_commands_valid(self): """ @@ -459,9 +452,10 @@ def test_build_jobs_without_build_os_is_invalid(self): }, }, ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "build.os" + assert excinfo.value.message_id == ConfigValidationError.VALUE_NOT_FOUND + assert excinfo.value.format_values.get("key") == "build.os" def test_commands_build_config_invalid_command(self): build = get_build_config( @@ -473,9 +467,10 @@ def test_commands_build_config_invalid_command(self): }, }, ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "build.commands" + assert excinfo.value.message_id == ConfigValidationError.INVALID_LIST + assert excinfo.value.format_values.get("key") == "build.commands" def test_commands_build_config_invalid_no_os(self): build = get_build_config( @@ -485,9 +480,10 @@ def test_commands_build_config_invalid_no_os(self): }, }, ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "build.os" + assert excinfo.value.message_id == ConfigValidationError.VALUE_NOT_FOUND + assert excinfo.value.format_values.get("key") == "build.os" def test_commands_build_config_valid(self): """It's valid to build with just build.os and build.commands.""" @@ -516,9 +512,10 @@ def test_jobs_build_config_invalid_jobs(self, value): }, }, ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "build.jobs" + assert excinfo.value.message_id == ConfigValidationError.INVALID_CHOICE + assert excinfo.value.format_values.get("key") == "build.jobs" @pytest.mark.parametrize("value", ["", None, "echo 123", 42]) def test_jobs_build_config_invalid_job_commands(self, value): @@ -533,9 +530,10 @@ def test_jobs_build_config_invalid_job_commands(self, value): }, }, ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "build.jobs.pre_install" + assert excinfo.value.message_id == ConfigValidationError.INVALID_LIST + assert excinfo.value.format_values.get("key") == "build.jobs.pre_install" def test_jobs_build_config(self): build = get_build_config( @@ -622,9 +620,10 @@ def test_build_apt_packages_invalid_type(self, value): } } ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "build.apt_packages" + assert excinfo.value.message_id == ConfigValidationError.INVALID_LIST + assert excinfo.value.format_values.get("key") == "build.apt_packages" @pytest.mark.parametrize( "error_index, value", @@ -662,24 +661,32 @@ def test_build_apt_packages_invalid_value(self, error_index, value): } } ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == f"build.apt_packages.{error_index}" - assert excinfo.value.code == INVALID_NAME + assert excinfo.value.message_id in ( + ConfigError.APT_INVALID_PACKAGE_NAME, + ConfigError.APT_INVALID_PACKAGE_NAME_PREFIX, + ) + assert ( + excinfo.value.format_values.get("key") + == f"build.apt_packages.{error_index}" + ) @pytest.mark.parametrize("value", [3, [], "invalid"]) def test_python_check_invalid_types(self, value): build = get_build_config({"python": value}) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "python" + assert excinfo.value.message_id == ConfigValidationError.INVALID_DICT + assert excinfo.value.format_values.get("key") == "python" @pytest.mark.parametrize("value", [[], {}, "3", "3.10"]) def test_python_version_check_invalid_types(self, value): build = get_build_config({"python": {"version": value}}) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "python.version" + assert excinfo.value.message_id == ConfigError.INVALID_KEY_NAME + assert excinfo.value.format_values.get("key") == "python.version" def test_python_install_default_value(self): build = get_build_config({}) @@ -723,9 +730,10 @@ def test_python_install_method_check_invalid(self, value, tmpdir): }, source_file=str(tmpdir.join('readthedocs.yml')), ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "python.install.0.method" + assert excinfo.value.message_id == ConfigValidationError.INVALID_CHOICE + assert excinfo.value.format_values.get("key") == "python.install.0.method" def test_python_install_requirements_check_valid(self, tmpdir): apply_fs(tmpdir, {"requirements.txt": ""}) @@ -757,9 +765,10 @@ def test_python_install_requirements_does_not_allow_null(self, tmpdir): }, source_file=str(tmpdir.join('readthedocs.yml')), ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "python.install.0.requirements" + assert excinfo.value.message_id == ConfigValidationError.INVALID_STRING + assert excinfo.value.format_values.get("key") == "python.install.0.requirements" def test_python_install_requirements_error_msg(self, tmpdir): build = get_build_config( @@ -775,13 +784,12 @@ def test_python_install_requirements_error_msg(self, tmpdir): }, source_file=str(tmpdir.join('readthedocs.yml')), ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert ( - str(excinfo.value) - == 'Invalid configuration option "python.install[0].requirements": expected string' - ) + assert str(excinfo.value) == "Build user exception" + # assert registry.get() + # == 'Invalid configuration option "python.install[0].requirements": expected string' def test_python_install_requirements_does_not_allow_empty_string(self, tmpdir): build = get_build_config( @@ -797,9 +805,10 @@ def test_python_install_requirements_does_not_allow_empty_string(self, tmpdir): }, source_file=str(tmpdir.join('readthedocs.yml')), ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "python.install.0.requirements" + assert excinfo.value.message_id == ConfigValidationError.INVALID_PATH + assert excinfo.value.format_values.get("key") == "python.install.0.requirements" @pytest.mark.parametrize("value", [3, [], {}]) def test_python_install_requirements_check_invalid_types(self, value, tmpdir): @@ -816,9 +825,10 @@ def test_python_install_requirements_check_invalid_types(self, value, tmpdir): }, source_file=str(tmpdir.join('readthedocs.yml')), ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "python.install.0.requirements" + assert excinfo.value.message_id == ConfigValidationError.INVALID_STRING + assert excinfo.value.format_values.get("key") == "python.install.0.requirements" def test_python_install_path_is_required(self, tmpdir): build = get_build_config( @@ -833,10 +843,10 @@ def test_python_install_path_is_required(self, tmpdir): }, source_file=str(tmpdir.join('readthedocs.yml')), ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "python.install.0" - assert excinfo.value.code == CONFIG_REQUIRED + assert excinfo.value.message_id == ConfigError.PIP_PATH_OR_REQUIREMENT_REQUIRED + assert excinfo.value.format_values.get("key") == "python.install.0" def test_python_install_pip_check_valid(self, tmpdir): build = get_build_config( @@ -895,9 +905,10 @@ def test_python_install_check_invalid_type(self, value): build = get_build_config( {"python": {"install": value}}, ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "python.install" + assert excinfo.value.message_id == ConfigValidationError.INVALID_LIST + assert excinfo.value.format_values.get("key") == "python.install" def test_python_install_extra_requirements_and_pip(self, tmpdir): build = get_build_config( @@ -934,9 +945,9 @@ def test_python_install_extra_requirements_and_setuptools(self, tmpdir): }, source_file=str(tmpdir.join('readthedocs.yml')), ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "python.install.0.extra_requirements" + assert excinfo.value.message_id == ConfigError.USE_PIP_FOR_EXTRA_REQUIREMENTS @pytest.mark.parametrize("value", [2, "invalid", {}, "", None]) def test_python_install_extra_requirements_check_type(self, value, tmpdir): @@ -954,9 +965,13 @@ def test_python_install_extra_requirements_check_type(self, value, tmpdir): }, source_file=str(tmpdir.join('readthedocs.yml')), ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "python.install.0.extra_requirements" + assert excinfo.value.message_id == ConfigValidationError.INVALID_LIST + assert ( + excinfo.value.format_values.get("key") + == "python.install.0.extra_requirements" + ) def test_python_install_extra_requirements_allow_empty(self, tmpdir): build = get_build_config( @@ -1024,9 +1039,10 @@ def test_python_install_several_respects_order(self, tmpdir): @pytest.mark.parametrize("value", [[], True, 0, "invalid"]) def test_sphinx_validate_type(self, value): build = get_build_config({"sphinx": value}) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "sphinx" + assert excinfo.value.message_id == ConfigValidationError.INVALID_DICT + assert excinfo.value.format_values.get("key") == "sphinx" def test_sphinx_is_default_doc_type(self): build = get_build_config({}) @@ -1055,9 +1071,10 @@ def test_sphinx_builder_check_valid(self, value, expected): @pytest.mark.parametrize("value", [[], True, 0, "invalid"]) def test_sphinx_builder_check_invalid(self, value): build = get_build_config({"sphinx": {"builder": value}}) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "sphinx.builder" + assert excinfo.value.message_id == ConfigValidationError.INVALID_CHOICE + assert excinfo.value.format_values.get("key") == "sphinx.builder" def test_sphinx_builder_default(self): build = get_build_config({}) @@ -1089,9 +1106,9 @@ def test_sphinx_cant_be_used_with_mkdocs(self, tmpdir): }, source_file=str(tmpdir.join("readthedocs.yml")), ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "." + assert excinfo.value.message_id == ConfigError.SPHINX_MKDOCS_CONFIG_TOGETHER def test_sphinx_configuration_allow_null(self): build = get_build_config( @@ -1110,9 +1127,10 @@ def test_sphinx_configuration_validate_type(self, value): build = get_build_config( {"sphinx": {"configuration": value}}, ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "sphinx.configuration" + assert excinfo.value.message_id == ConfigValidationError.INVALID_STRING + assert excinfo.value.format_values.get("key") == "sphinx.configuration" @pytest.mark.parametrize("value", [True, False]) def test_sphinx_fail_on_warning_check_valid(self, value): @@ -1123,9 +1141,10 @@ def test_sphinx_fail_on_warning_check_valid(self, value): @pytest.mark.parametrize("value", [[], "invalid", 5]) def test_sphinx_fail_on_warning_check_invalid(self, value): build = get_build_config({"sphinx": {"fail_on_warning": value}}) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "sphinx.fail_on_warning" + assert excinfo.value.message_id == ConfigValidationError.INVALID_BOOL + assert excinfo.value.format_values.get("key") == "sphinx.fail_on_warning" def test_sphinx_fail_on_warning_check_default(self): build = get_build_config({}) @@ -1135,9 +1154,10 @@ def test_sphinx_fail_on_warning_check_default(self): @pytest.mark.parametrize("value", [[], True, 0, "invalid"]) def test_mkdocs_validate_type(self, value): build = get_build_config({"mkdocs": value}) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "mkdocs" + assert excinfo.value.message_id == ConfigValidationError.INVALID_DICT + assert excinfo.value.format_values.get("key") == "mkdocs" def test_mkdocs_default(self): build = get_build_config({}) @@ -1174,9 +1194,10 @@ def test_mkdocs_configuration_validate_type(self, value): build = get_build_config( {"mkdocs": {"configuration": value}}, ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "mkdocs.configuration" + assert excinfo.value.message_id == ConfigValidationError.INVALID_STRING + assert excinfo.value.format_values.get("key") == "mkdocs.configuration" @pytest.mark.parametrize("value", [True, False]) def test_mkdocs_fail_on_warning_check_valid(self, value): @@ -1191,9 +1212,10 @@ def test_mkdocs_fail_on_warning_check_invalid(self, value): build = get_build_config( {"mkdocs": {"fail_on_warning": value}}, ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "mkdocs.fail_on_warning" + assert excinfo.value.message_id == ConfigValidationError.INVALID_BOOL + assert excinfo.value.format_values.get("key") == "mkdocs.fail_on_warning" def test_mkdocs_fail_on_warning_check_default(self): build = get_build_config( @@ -1212,9 +1234,10 @@ def test_submodule_defaults(self): @pytest.mark.parametrize("value", [[], "invalid", 0]) def test_submodules_check_invalid_type(self, value): build = get_build_config({"submodules": value}) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "submodules" + assert excinfo.value.message_id == ConfigValidationError.INVALID_DICT + assert excinfo.value.format_values.get("key") == "submodules" def test_submodules_include_check_valid(self): build = get_build_config( @@ -1238,9 +1261,10 @@ def test_submodules_include_check_invalid(self, value): }, } ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "submodules.include" + assert excinfo.value.message_id == ConfigValidationError.INVALID_LIST + assert excinfo.value.format_values.get("key") == "submodules.include" def test_submodules_include_allows_all_keyword(self): build = get_build_config( @@ -1277,9 +1301,10 @@ def test_submodules_exclude_check_invalid(self, value): }, } ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "submodules.exclude" + assert excinfo.value.message_id == ConfigValidationError.INVALID_LIST + assert excinfo.value.format_values.get("key") == "submodules.exclude" def test_submodules_exclude_allows_all_keyword(self): build = get_build_config( @@ -1303,9 +1328,11 @@ def test_submodules_cant_exclude_and_include(self): }, } ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "submodules" + assert ( + excinfo.value.message_id == ConfigError.SUBMODULES_INCLUDE_EXCLUDE_TOGETHER + ) def test_submodules_can_exclude_include_be_empty(self): build = get_build_config( @@ -1346,9 +1373,10 @@ def test_submodules_recursive_check_invalid(self, value): }, } ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "submodules.recursive" + assert excinfo.value.message_id == ConfigValidationError.INVALID_BOOL + assert excinfo.value.format_values.get("key") == "submodules.recursive" def test_submodules_recursive_explicit_default(self): build = get_build_config( @@ -1384,9 +1412,10 @@ def test_search_invalid_type(self, value): "search": value, } ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "search" + assert excinfo.value.message_id == ConfigValidationError.INVALID_DICT + assert excinfo.value.format_values.get("key") == "search" @pytest.mark.parametrize( 'value', @@ -1413,9 +1442,18 @@ def test_search_ranking_invalid_type(self, value): "search": {"ranking": value}, } ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "search.ranking" + + # TODO: these test should be split to validate the exact ``message_id`` + assert excinfo.value.message_id in ( + ConfigValidationError.INVALID_DICT, + ConfigValidationError.INVALID_CHOICE, + ConfigValidationError.INVALID_PATH_PATTERN, + ConfigValidationError.INVALID_STRING, + ) + + assert excinfo.value.format_values.get("key") == "search.ranking" @pytest.mark.parametrize("value", list(range(-10, 10 + 1))) def test_search_valid_ranking(self, value): @@ -1476,9 +1514,13 @@ def test_search_ignore_invalid_type(self, value): "search": {"ignore": value}, } ) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == "search.ignore" + assert excinfo.value.message_id in ( + ConfigValidationError.INVALID_LIST, + ConfigValidationError.INVALID_STRING, + ) + assert excinfo.value.format_values.get("key") == "search.ignore" @pytest.mark.parametrize( "path, expected", @@ -1549,10 +1591,13 @@ def test_search_ignore_valid_type(self, path, expected): ) def test_strict_validation(self, value, key): build = get_build_config(value) - with raises(InvalidConfig) as excinfo: + with raises(ConfigError) as excinfo: build.validate() - assert excinfo.value.key == key - assert excinfo.value.code == INVALID_KEY + assert excinfo.value.message_id in ( + ConfigError.INVALID_KEY_NAME, + ConfigValidationError.INVALID_BOOL, + ) + assert excinfo.value.format_values.get("key") == key @pytest.mark.parametrize( 'value,expected', [ @@ -1596,10 +1641,10 @@ def test_pop_config_default(self): def test_pop_config_raise_exception(self): build = get_build_config({}) - with raises(ValidationError) as excinfo: + with raises(ConfigValidationError) as excinfo: build.pop_config("build.invalid", raise_ex=True) - assert excinfo.value.value == "invalid" - assert excinfo.value.code == VALUE_NOT_FOUND + assert excinfo.value.format_values.get("value") == "invalid" + assert excinfo.value.message_id == ConfigValidationError.VALUE_NOT_FOUND def test_as_dict_new_build_config(self, tmpdir): build = get_build_config( diff --git a/readthedocs/config/tests/test_validation.py b/readthedocs/config/tests/test_validation.py index 3a3426ce9ee..c3585b180f2 100644 --- a/readthedocs/config/tests/test_validation.py +++ b/readthedocs/config/tests/test_validation.py @@ -1,11 +1,7 @@ from pytest import raises +from readthedocs.config.exceptions import ConfigValidationError from readthedocs.config.validation import ( - INVALID_BOOL, - INVALID_CHOICE, - INVALID_LIST, - INVALID_STRING, - ValidationError, validate_bool, validate_choice, validate_list, @@ -28,9 +24,9 @@ def test_it_accepts_1(self): assert validate_bool(1) is True def test_it_fails_on_string(self): - with raises(ValidationError) as excinfo: + with raises(ConfigValidationError) as excinfo: validate_bool("random string") - assert excinfo.value.code == INVALID_BOOL + assert excinfo.value.message_id == ConfigValidationError.INVALID_BOOL class TestValidateChoice: @@ -38,14 +34,14 @@ def test_it_accepts_valid_choice(self): result = validate_choice("choice", ("choice", "another_choice")) assert result == "choice" - with raises(ValidationError) as excinfo: + with raises(ConfigValidationError) as excinfo: validate_choice("c", "abc") - assert excinfo.value.code == INVALID_LIST + assert excinfo.value.message_id == ConfigValidationError.INVALID_LIST def test_it_rejects_invalid_choice(self): - with raises(ValidationError) as excinfo: + with raises(ConfigValidationError) as excinfo: validate_choice("not-a-choice", ("choice", "another_choice")) - assert excinfo.value.code == INVALID_CHOICE + assert excinfo.value.message_id == ConfigValidationError.INVALID_CHOICE class TestValidateList: @@ -62,14 +58,14 @@ def iterator(): result = validate_list(iterator()) assert result == ["choice"] - with raises(ValidationError) as excinfo: + with raises(ConfigValidationError) as excinfo: validate_choice("c", "abc") - assert excinfo.value.code == INVALID_LIST + assert excinfo.value.message_id == ConfigValidationError.INVALID_LIST def test_it_rejects_string_types(self): - with raises(ValidationError) as excinfo: + with raises(ConfigValidationError) as excinfo: validate_list("choice") - assert excinfo.value.code == INVALID_LIST + assert excinfo.value.message_id == ConfigValidationError.INVALID_LIST class TestValidatePath: @@ -91,9 +87,9 @@ def test_it_returns_relative_path(self, tmpdir): assert path == "a directory" def test_it_only_accepts_strings(self): - with raises(ValidationError) as excinfo: + with raises(ConfigValidationError) as excinfo: validate_path(None, "") - assert excinfo.value.code == INVALID_STRING + assert excinfo.value.message_id == ConfigValidationError.INVALID_STRING class TestValidateString: @@ -106,11 +102,11 @@ def test_it_accepts_nonunicode(self): assert isinstance(result, str) def test_it_rejects_float(self): - with raises(ValidationError) as excinfo: + with raises(ConfigValidationError) as excinfo: validate_string(123.456) - assert excinfo.value.code == INVALID_STRING + assert excinfo.value.message_id == ConfigValidationError.INVALID_STRING def test_it_rejects_none(self): - with raises(ValidationError) as excinfo: + with raises(ConfigValidationError) as excinfo: validate_string(None) - assert excinfo.value.code == INVALID_STRING + assert excinfo.value.message_id == ConfigValidationError.INVALID_STRING diff --git a/readthedocs/config/validation.py b/readthedocs/config/validation.py index 0b8bb894efd..fd08955f0f3 100644 --- a/readthedocs/config/validation.py +++ b/readthedocs/config/validation.py @@ -2,66 +2,47 @@ import os -INVALID_BOOL = "invalid-bool" -INVALID_CHOICE = "invalid-choice" -INVALID_LIST = "invalid-list" -INVALID_DICT = "invalid-dictionary" -INVALID_PATH = "invalid-path" -INVALID_PATH_PATTERN = "invalid-path-pattern" -INVALID_STRING = "invalid-string" -VALUE_NOT_FOUND = "value-not-found" - - -class ValidationError(Exception): - - """Base error for validations.""" - - messages = { - INVALID_BOOL: "expected one of (0, 1, true, false), got {value}", - INVALID_CHOICE: "expected one of ({choices}), got {value}", - INVALID_DICT: "{value} is not a dictionary", - INVALID_PATH: "path {value} does not exist", - INVALID_PATH_PATTERN: "{value} isn't a valid path pattern", - INVALID_STRING: "expected string", - INVALID_LIST: "expected list", - VALUE_NOT_FOUND: "{value} not found", - } - - def __init__(self, value, code, format_kwargs=None): - self.value = value - self.code = code - defaults = { - "value": value, - } - if format_kwargs is not None: - defaults.update(format_kwargs) - message = self.messages[code].format(**defaults) - super().__init__(message) +from .exceptions import ConfigValidationError def validate_list(value): """Check if ``value`` is an iterable.""" if isinstance(value, (dict, str)): - raise ValidationError(value, INVALID_LIST) + raise ConfigValidationError( + message_id=ConfigValidationError.INVALID_LIST, + format_values={ + "value": value, + }, + ) if not hasattr(value, "__iter__"): - raise ValidationError(value, INVALID_LIST) + raise ConfigValidationError( + message_id=ConfigValidationError.INVALID_LIST, + format_values={ + "value": value, + }, + ) return list(value) def validate_dict(value): """Check if ``value`` is a dictionary.""" if not isinstance(value, dict): - raise ValidationError(value, INVALID_DICT) + raise ConfigValidationError( + message_id=ConfigValidationError.INVALID_DICT, + format_values={ + "value": value, + }, + ) def validate_choice(value, choices): """Check that ``value`` is in ``choices``.""" choices = validate_list(choices) if value not in choices: - raise ValidationError( - value, - INVALID_CHOICE, - { + raise ConfigValidationError( + message_id=ConfigValidationError.INVALID_CHOICE, + format_values={ + "value": value, "choices": ", ".join(map(str, choices)), }, ) @@ -71,7 +52,12 @@ def validate_choice(value, choices): def validate_bool(value): """Check that ``value`` is an boolean value.""" if value not in (0, 1, False, True): - raise ValidationError(value, INVALID_BOOL) + raise ConfigValidationError( + message_id=ConfigValidationError.INVALID_BOOL, + format_values={ + "value": value, + }, + ) return bool(value) @@ -79,7 +65,12 @@ def validate_path(value, base_path): """Check that ``value`` is a valid path name and normamlize it.""" string_value = validate_string(value) if not string_value: - raise ValidationError(value, INVALID_PATH) + raise ConfigValidationError( + message_id=ConfigValidationError.INVALID_PATH, + format_values={ + "value": value, + }, + ) full_path = os.path.join(base_path, string_value) rel_path = os.path.relpath(full_path, base_path) return rel_path @@ -98,16 +89,31 @@ def validate_path_pattern(value): path = "/" + path.lstrip("/") path = os.path.normpath(path) if not os.path.isabs(path): - raise ValidationError(value, INVALID_PATH_PATTERN) + raise ConfigValidationError( + message_id=ConfigValidationError.INVALID_PATH_PATTERN, + format_values={ + "value": value, + }, + ) # Remove ``/`` from the path once is validated. path = path.lstrip("/") if not path: - raise ValidationError(value, INVALID_PATH_PATTERN) + raise ConfigValidationError( + message_id=ConfigValidationError.INVALID_PATH_PATTERN, + format_values={ + "value": value, + }, + ) return path def validate_string(value): """Check that ``value`` is a string type.""" if not isinstance(value, str): - raise ValidationError(value, INVALID_STRING) + raise ConfigValidationError( + message_id=ConfigValidationError.INVALID_STRING, + format_values={ + "value": value, + }, + ) return str(value) diff --git a/readthedocs/core/admin.py b/readthedocs/core/admin.py index c01c00d4e4d..e488758d093 100644 --- a/readthedocs/core/admin.py +++ b/readthedocs/core/admin.py @@ -10,8 +10,6 @@ from django.utils.html import format_html from django.utils.safestring import mark_safe from django.utils.translation import gettext_lazy as _ -from messages_extends.admin import MessageAdmin -from messages_extends.models import Message from rest_framework.authtoken.admin import TokenAdmin from readthedocs.core.history import ExtraSimpleHistoryAdmin @@ -127,30 +125,5 @@ class UserProfileAdmin(ExtraSimpleHistoryAdmin): raw_id_fields = ("user",) -class MessageAdminExtra(MessageAdmin): - list_display = [ - "user", - "organizations", - "message", - "created", - "read", - ] - list_filter = [ - "read", - ] - search_fields = [ - "user__username", - "message", - "user__organizationowner__organization__slug", - ] - - def organizations(self, obj): - return ", ".join( - organization.slug for organization in obj.user.owner_organizations.all() - ) - - admin.site.unregister(User) admin.site.register(User, UserAdminExtra) -admin.site.unregister(Message) -admin.site.register(Message, MessageAdminExtra) diff --git a/readthedocs/core/context_processors.py b/readthedocs/core/context_processors.py index c7c8aa2b6df..99b75fccaf2 100644 --- a/readthedocs/core/context_processors.py +++ b/readthedocs/core/context_processors.py @@ -1,9 +1,19 @@ """Template context processors for core app.""" from django.conf import settings +from django.contrib.contenttypes.models import ContentType def readthedocs_processor(request): + """ + Context processor to include global settings to templates. + + Note that we are not using the ``request`` object at all here. + It's preferable to keep it like that since it's used from places where there is no request. + + If you need to add something that depends on the request, + create a new context processor. + """ exports = { "PUBLIC_DOMAIN": settings.PUBLIC_DOMAIN, "PRODUCTION_DOMAIN": settings.PRODUCTION_DOMAIN, @@ -18,3 +28,29 @@ def readthedocs_processor(request): "PUBLIC_API_URL": settings.PUBLIC_API_URL, } return exports + + +def user_notifications(request): + """ + Context processor to include user's notification to templates. + + We can't use ``request.user.notifications.all()`` because we are not using a ``CustomUser``. + If we want to go that route, we should define a ``CustomUser`` and define a ``GenericRelation``. + + See https://docs.djangoproject.com/en/4.2/ref/settings/#std-setting-AUTH_USER_MODEL + """ + + # Import here due to circular import + from readthedocs.notifications.models import Notification + + user_notifications = Notification.objects.none() + if request.user.is_authenticated: + content_type = ContentType.objects.get_for_model(request.user) + user_notifications = Notification.objects.filter( + attached_to_content_type=content_type, + attached_to_id=request.user.pk, + ) + + return { + "user_notifications": user_notifications, + } diff --git a/readthedocs/core/notifications.py b/readthedocs/core/notifications.py new file mode 100644 index 00000000000..f38e37264c5 --- /dev/null +++ b/readthedocs/core/notifications.py @@ -0,0 +1,28 @@ +"""Read the Docs notifications.""" + +import textwrap + +from django.utils.translation import gettext_lazy as _ + +from readthedocs.notifications.constants import WARNING +from readthedocs.notifications.messages import Message, registry + +MESSAGE_EMAIL_VALIDATION_PENDING = "core:email:validation-pending" +messages = [ + Message( + id=MESSAGE_EMAIL_VALIDATION_PENDING, + header=_("Email address not verified"), + body=_( + textwrap.dedent( + """ + Your primary email address is not verified. + Please verify it here. + """ + ).strip(), + ), + type=WARNING, + ), +] + + +registry.add(messages) diff --git a/readthedocs/core/tasks.py b/readthedocs/core/tasks.py index ce4e046f608..157d7c91822 100644 --- a/readthedocs/core/tasks.py +++ b/readthedocs/core/tasks.py @@ -6,8 +6,6 @@ import structlog from django.conf import settings from django.core.mail import EmailMultiAlternatives -from django.utils import timezone -from messages_extends.models import Message as PersistentMessage from readthedocs.worker import app @@ -51,16 +49,6 @@ def send_email_task( msg.send() -@app.task(queue="web") -def clear_persistent_messages(): - # Delete all expired message_extend's messages - log.info("Deleting all expired message_extend's messages") - expired_messages = PersistentMessage.objects.filter( - expires__lt=timezone.now(), - ) - expired_messages.delete() - - @app.task(queue="web") def cleanup_pidbox_keys(): """ diff --git a/readthedocs/core/tests/test_contact.py b/readthedocs/core/tests/test_contact.py index becb6e8aa70..8b4b6462db1 100644 --- a/readthedocs/core/tests/test_contact.py +++ b/readthedocs/core/tests/test_contact.py @@ -5,7 +5,6 @@ from django_dynamic_fixture import get from readthedocs.core.utils.contact import contact_users -from readthedocs.notifications.backends import SiteBackend User = get_user_model() @@ -17,14 +16,12 @@ 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") - def test_contact_users_dryrun(self, send_notification, send_mail): + def test_contact_users_dryrun(self, send_mail): self.assertEqual(User.objects.all().count(), 3) resp = contact_users( users=User.objects.all(), email_subject="Subject", email_content="Content", - notification_content="Notification", dryrun=True, ) self.assertEqual( @@ -34,29 +31,18 @@ def test_contact_users_dryrun(self, send_notification, send_mail): "sent": {"one@test.com", "two@test.com", "three@test.com"}, "failed": set(), }, - "notification": { - "sent": { - self.user.username, - self.user_two.username, - self.user_three.username, - }, - "failed": set(), - }, }, ) - self.assertEqual(send_notification.call_count, 0) self.assertEqual(send_mail.call_count, 0) @mock.patch("readthedocs.core.utils.contact.send_mail") - @mock.patch.object(SiteBackend, "send") - def test_contact_users_not_dryrun(self, send_notification, send_mail): + def test_contact_users_not_dryrun(self, send_mail): self.assertEqual(User.objects.all().count(), 3) resp = contact_users( users=User.objects.all(), email_subject="Subject", email_content="Content", - notification_content="Notification", dryrun=False, ) self.assertEqual( @@ -66,16 +52,7 @@ def test_contact_users_not_dryrun(self, send_notification, send_mail): "sent": {"one@test.com", "two@test.com", "three@test.com"}, "failed": set(), }, - "notification": { - "sent": { - self.user.username, - self.user_two.username, - self.user_three.username, - }, - "failed": set(), - }, }, ) - self.assertEqual(send_notification.call_count, 3) self.assertEqual(send_mail.call_count, 3) diff --git a/readthedocs/core/tests/test_filesystem_utils.py b/readthedocs/core/tests/test_filesystem_utils.py index 6afe31edd51..6c744a975f8 100644 --- a/readthedocs/core/tests/test_filesystem_utils.py +++ b/readthedocs/core/tests/test_filesystem_utils.py @@ -7,7 +7,7 @@ from readthedocs.core.utils.filesystem import safe_copytree, safe_open, safe_rmtree from readthedocs.doc_builder.exceptions import ( - FileTooLarge, + BuildUserError, SymlinkOutsideBasePath, UnsupportedSymlinkFileError, ) @@ -110,7 +110,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..7e30678fd63 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -18,6 +18,7 @@ EXTERNAL, ) from readthedocs.doc_builder.exceptions import BuildCancelled, BuildMaxConcurrencyError +from readthedocs.notifications.models import Notification from readthedocs.worker import app log = structlog.get_logger(__name__) @@ -153,10 +154,13 @@ 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 - build.error = BuildMaxConcurrencyError.message.format( - limit=max_concurrent_builds, + + Notification.objects.add( + message_id=BuildMaxConcurrencyError.LIMIT_REACHED, + attached_to=build, + dismissable=False, + format_values={"limit": max_concurrent_builds}, ) - build.save() _, build_api_key = BuildAPIKey.objects.create_key(project=project) @@ -244,7 +248,14 @@ def cancel_build(build): terminate = False build.state = BUILD_STATE_CANCELLED build.success = False - build.error = BuildCancelled.message + + # Add a notification for this build + Notification.objects.add( + message_id=BuildCancelled.CANCELLED_BY_USER, + attached_to=build, + dismissable=False, + ) + build.length = 0 build.save() else: diff --git a/readthedocs/core/utils/contact.py b/readthedocs/core/utils/contact.py index eccb022b569..3fd20c4a5f1 100644 --- a/readthedocs/core/utils/contact.py +++ b/readthedocs/core/utils/contact.py @@ -4,71 +4,53 @@ 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__) +# TODO: re-implement sending notifications to users. +# This needs more thinking because the notifications where "Generic", +# and now we need to register a ``Message`` with its ID. def contact_users( users, email_subject=None, email_content=None, from_email=None, - notification_content=None, - sticky_notification=False, context_function=None, dryrun=True, ): """ - Send an email or a sticky notification to a list of users. + Send an email to a list of users. :param users: Queryset of Users. :param string email_subject: Email subject :param string email_content: Email content (markdown) :param string from_email: Email to sent from (Test Support ) - :param string notification_content: Content for the sticky notification (markdown) :param context_function: A callable that will receive an user - and return a dict of additional context to be used in the email/notification content - :param bool dryrun: If `True` don't sent the email or notification, just logs the content + and return a dict of additional context to be used in the email content + :param bool dryrun: If `True` don't sent the email, just logs the content - The `email_content` and `notification_content` contents will be rendered using - a template with the following context:: + The `email_content` contents will be rendered using a template with the following context:: { 'user': , 'production_uri': https://readthedocs.org, } - :returns: A dictionary with a list of sent/failed emails/notifications. + :returns: A dictionary with a list of sent/failed emails. """ from_email = from_email or settings.DEFAULT_FROM_EMAIL context_function = context_function or (lambda user: {}) sent_emails = set() failed_emails = set() - sent_notifications = set() - failed_notifications = set() - - backend = SiteBackend(request=None) engine = Engine.get_default() - notification_template = engine.from_string(notification_content or "") email_template = engine.from_string(email_content or "") email_txt_template = engine.get_template("core/email/common.txt") email_html_template = engine.get_template("core/email/common.html") - class TempNotification(SiteNotification): - if sticky_notification: - success_level = message_constants.SUCCESS_PERSISTENT - - def render(self, *args, **kwargs): - return markdown.markdown( - notification_template.render(Context(self.get_context_data())) - ) - total = users.count() for count, user in enumerate(users.iterator(), start=1): context = { @@ -77,35 +59,6 @@ def render(self, *args, **kwargs): } context.update(context_function(user)) - if notification_content: - notification = TempNotification( - user=user, - success=True, - extra_context=context, - ) - try: - if not dryrun: - backend.send(notification) - else: - # Check we can render the notification with the context properly - log.debug( - "Rendered notification.", - notification=markdown.markdown( - notification_template.render(Context(context)) - ), - ) - except Exception: - log.exception("Notification failed to send") - failed_notifications.add(user.username) - else: - log.info( - "Successfully sent notification.", - user_username=user.username, - count=count, - total=total, - ) - sent_notifications.add(user.username) - if email_subject: emails = list( user.emailaddress_set.filter(verified=True) @@ -148,8 +101,4 @@ def render(self, *args, **kwargs): "sent": sent_emails, "failed": failed_emails, }, - "notification": { - "sent": sent_notifications, - "failed": failed_emails, - }, } diff --git a/readthedocs/core/utils/filesystem.py b/readthedocs/core/utils/filesystem.py index d98f7deb90d..fe63dc769b9 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, ) @@ -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() @@ -90,14 +90,14 @@ 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() 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/backends/mkdocs.py b/readthedocs/doc_builder/backends/mkdocs.py index 7f36b49eafd..6fef3009818 100644 --- a/readthedocs/doc_builder/backends/mkdocs.py +++ b/readthedocs/doc_builder/backends/mkdocs.py @@ -102,7 +102,10 @@ def load_yaml_config(self): ) if not result: raise UserFileNotFound( - UserFileNotFound.FILE_NOT_FOUND.format(self.yaml_file) + message_id=UserFileNotFound.FILE_NOT_FOUND, + format_values={ + "filename": self.yaml_file, + }, ) config = yaml_load_safely(result) @@ -124,8 +127,7 @@ def load_yaml_config(self): mark.column + 1, ) raise MkDocsYAMLParseError( - 'Your mkdocs.yml could not be loaded, ' - 'possibly due to a syntax error{note}'.format(note=note), + MkDocsYAMLParseError.SYNTAX_ERROR, ) from exc def append_conf(self): diff --git a/readthedocs/doc_builder/backends/sphinx.py b/readthedocs/doc_builder/backends/sphinx.py index 35c7d7503fd..b4d2ddc6988 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 @@ -502,7 +501,7 @@ def build(self): tex_files = glob(os.path.join(self.absolute_host_output_dir, "*.tex")) if not tex_files: - raise BuildUserError("No TeX files were found.") + raise BuildUserError(message_id=BuildUserError.TEX_FILE_NOT_FOUND) # Run LaTeX -> PDF conversions success = self._build_latexmk(self.project_path) @@ -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/director.py b/readthedocs/doc_builder/director.py index fcbb4c47c2e..b8b9aaa2a04 100644 --- a/readthedocs/doc_builder/director.py +++ b/readthedocs/doc_builder/director.py @@ -443,6 +443,11 @@ def check_old_output_directory(self): def run_build_commands(self): """Runs each build command in the build environment.""" + + self.attach_notification( + message_id=BuildUserError.BUILD_COMMANDS_IN_BETA, + ) + reshim_commands = ( {"pip", "install"}, {"conda", "create"}, @@ -767,3 +772,27 @@ def store_readthedocs_build_yaml(self): # It will be saved when the API is hit. # This data will be used by the `/_/readthedocs-config.json` API endpoint. self.data.version.build_data = data + + def attach_notification( + self, + message_id, + format_values=None, + state="unread", + dismissable=False, + news=False, + ): + """Attach a notification to build in progress using the APIv2.""" + + format_values = format_values or {} + # NOTE: we are using APIv2 here because it uses BuildAPIKey authentication, + # which is not currently supported by APIv3. + self.data.api_client.notifications.post( + { + "attached_to": f'build/{self.data.build["id"]}', + "message_id": message_id, + "state": state, # Optional + "dismissable": dismissable, + "news": news, + "format_values": format_values, + } + ) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index 609fc234abc..9950498f315 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 @@ -776,22 +776,17 @@ def raise_container_error(self, state): """ if state is not None and state.get("Running") is False: if state.get("ExitCode") == DOCKER_TIMEOUT_EXIT_CODE: - raise BuildUserError( - _("Build exited due to time out"), - ) + raise BuildUserError(message_id=BuildUserError.BUILD_TIME_OUT) if state.get("OOMKilled", False): - raise BuildUserError( - _("Build exited due to excessive memory consumption"), - ) + raise BuildUserError(message_id=BuildUserError.BUILD_EXCESSIVE_MEMORY) if state.get("Error"): raise BuildAppError( - ( - _("Build exited due to unknown error: {0}").format( - state.get("Error") - ), - ) + message_id=BuildAppError.BUILD_DOCKER_UNKNOWN_ERROR, + format_values={ + "message": state.get("Error"), + }, ) def create_container(self): diff --git a/readthedocs/doc_builder/exceptions.py b/readthedocs/doc_builder/exceptions.py index 9597fb9e7ce..b13e9ba2b74 100644 --- a/readthedocs/doc_builder/exceptions.py +++ b/readthedocs/doc_builder/exceptions.py @@ -1,156 +1,86 @@ -"""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 + + - 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``. +""" -from readthedocs.builds.constants import BUILD_STATE_CANCELLED -from readthedocs.projects.constants import BUILD_COMMANDS_OUTPUT_PATH_HTML class BuildBaseException(Exception): - message = None - status_code = None - 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) + default_message = "Build user exception" - def get_default_message(self): - return self.message + 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) 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}).", - ) + default_message = "Build app exception" + GENERIC_WITH_BUILD_ID = "build:app:generic-with-build-id" + BUILDS_DISABLED = "build:app:project-builds-disabled" + BUILD_DOCKER_UNKNOWN_ERROR = "build:app:docker:unknown-error" + BUILD_TERMINATED_DUE_INACTIVITY = "build:app:terminated-due-inactivity" -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}", - ) +class BuildUserError(BuildBaseException): + GENERIC = "build:user:generic" + SKIPPED_EXIT_CODE_183 = "build:user:exit-code-183" + MAX_CONCURRENCY = "build:user:max-concurrency" -class BuildMaxConcurrencyError(BuildUserError): - message = gettext_noop( - "Concurrency limit reached ({limit}), retrying in 5 minutes." - ) + 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_NO_PDF_FILES = "build:user:output:has-no-pdf-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" + TEX_FILE_NOT_FOUND = "build:user:tex-file-not-found" + 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 + BUILD_COMMANDS_IN_BETA = "build:user:build-commands-config-key-in-beta" + BUILD_TIME_OUT = "build:user:time-out" + BUILD_EXCESSIVE_MEMORY = "build:user:excessive-memory" -class PDFNotFound(BuildUserError): - message = gettext_noop( - 'PDF file was not generated/found in "_readthedocs/pdf" output directory.' - ) +class BuildMaxConcurrencyError(BuildUserError): + LIMIT_REACHED = "build:user:concurrency-limit-reached" -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.", - ) +class BuildCancelled(BuildUserError): + CANCELLED_BY_USER = "build:user:cancelled" +class MkDocsYAMLParseError(BuildUserError): + 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" + SYNTAX_ERROR = "build:user:mkdocs:syntax-error" + + +# 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): - message = gettext_noop("Symlinks are not fully supported") + SYMLINK_USED = "build:user:symlink:used" class FileIsNotRegularFile(UnsupportedSymlinkFileError): @@ -159,10 +89,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/domains/notifications.py b/readthedocs/domains/notifications.py index ae787b0d2b8..57a3694e9d6 100644 --- a/readthedocs/domains/notifications.py +++ b/readthedocs/domains/notifications.py @@ -1,12 +1,51 @@ """Notifications related to custom domains.""" +import textwrap -from readthedocs.notifications import Notification -from readthedocs.notifications.constants import REQUIREMENT +from django.utils.translation import gettext_noop as _ +from readthedocs.notifications.constants import INFO +from readthedocs.notifications.email import EmailNotification +from readthedocs.notifications.messages import Message, registry -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 + + +MESSAGE_DOMAIN_VALIDATION_PENDING = "project:domain:validation-pending" +MESSAGE_DOMAIN_VALIDATION_EXPIRED = "project:domain:validation-expired" +messages = [ + Message( + id=MESSAGE_DOMAIN_VALIDATION_PENDING, + header=_("Pending configuration of custom domain: {domain}"), + body=_( + textwrap.dedent( + """ + The configuration of your custom domain {domain} is pending. + Go to the domain page and follow the instructions to complete it. + """ + ).strip(), + ), + 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=MESSAGE_DOMAIN_VALIDATION_EXPIRED, + header=_("Validation of custom domain expired: {domain}"), + body=_( + textwrap.dedent( + """ + The validation period of your custom domain {domain} has ended. + Go to the domain page and click on "Save" to restart the process. + """ + ).strip(), + ), + type=INFO, + ), +] +registry.add(messages) diff --git a/readthedocs/domains/tasks.py b/readthedocs/domains/tasks.py index a8ad7d9495e..2a12bd0956c 100644 --- a/readthedocs/domains/tasks.py +++ b/readthedocs/domains/tasks.py @@ -1,11 +1,15 @@ """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.domains.notifications import ( + MESSAGE_DOMAIN_VALIDATION_PENDING, + PendingCustomDomainValidation, +) +from readthedocs.notifications.models import Notification from readthedocs.projects.models import Domain from readthedocs.worker import app @@ -28,10 +32,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.add( + message_id=MESSAGE_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): notification = PendingCustomDomainValidation( context_object=domain, - request=HttpRequest(), user=user, ) notification.send() 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/domains/tests/test_tasks.py b/readthedocs/domains/tests/test_tasks.py index 8e3ba95b0dd..8e1b0b47da9 100644 --- a/readthedocs/domains/tests/test_tasks.py +++ b/readthedocs/domains/tests/test_tasks.py @@ -72,7 +72,7 @@ def setUp(self): ) self.domain_expired.save() - @mock.patch("readthedocs.notifications.backends.send_email") + @mock.patch("readthedocs.notifications.email.send_email") def test_email_pending_emails(self, send_email): subject = "Pending configuration of custom domain" email_pending_custom_domains.delay(number_of_emails=3) @@ -88,7 +88,7 @@ def test_email_pending_emails(self, send_email): self.assertTrue(kwargs["subject"].startswith(subject)) self.assertIn(self.domain_recently_expired.domain, kwargs["subject"]) - @mock.patch("readthedocs.notifications.backends.send_email") + @mock.patch("readthedocs.notifications.email.send_email") def test_dont_send_email_on_given_days(self, send_email): now = timezone.now() days = [5, 8, 14, 16, 29, 31] @@ -98,7 +98,7 @@ def test_dont_send_email_on_given_days(self, send_email): email_pending_custom_domains.delay(number_of_emails=3) send_email.assert_not_called() - @mock.patch("readthedocs.notifications.backends.send_email") + @mock.patch("readthedocs.notifications.email.send_email") def test_send_email_on_given_days(self, send_email): now = timezone.now() days = [7, 15, 30] diff --git a/readthedocs/notifications/__init__.py b/readthedocs/notifications/__init__.py deleted file mode 100644 index 7b4c69b05c6..00000000000 --- a/readthedocs/notifications/__init__.py +++ /dev/null @@ -1,22 +0,0 @@ -""" -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 - -__all__ = ( - "Notification", - "SiteNotification", - "send_notification", -) diff --git a/readthedocs/notifications/backends.py b/readthedocs/notifications/backends.py deleted file mode 100644 index b496d040f48..00000000000 --- a/readthedocs/notifications/backends.py +++ /dev/null @@ -1,113 +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(), - ) - - -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/constants.py b/readthedocs/notifications/constants.py index 375702b0998..84a3f3af1e1 100644 --- a/readthedocs/notifications/constants.py +++ b/readthedocs/notifications/constants.py @@ -1,7 +1,5 @@ """Notification constants.""" -from messages_extends import constants as message_constants - TEXT = "txt" HTML = "html" @@ -10,25 +8,16 @@ REQUIREMENT = 2 ERROR = 3 -LEVEL_MAPPING = { - INFO: message_constants.INFO_PERSISTENT, - WARNING: message_constants.WARNING_PERSISTENT, - REQUIREMENT: message_constants.WARNING_PERSISTENT, - ERROR: message_constants.ERROR_PERSISTENT, -} - -# Message levels to save the message into the database and mark as read -# immediately after retrieved (one-time shown message) -DEBUG_NON_PERSISTENT = 100 -INFO_NON_PERSISTENT = 101 -SUCCESS_NON_PERSISTENT = 102 -WARNING_NON_PERSISTENT = 103 -ERROR_NON_PERSISTENT = 104 +# TODO: use Enum here for these ones +# Status +UNREAD = "unread" +READ = "read" +DISMISSED = "dismissed" +CANCELLED = "cancelled" -NON_PERSISTENT_MESSAGE_LEVELS = ( - DEBUG_NON_PERSISTENT, - INFO_NON_PERSISTENT, - SUCCESS_NON_PERSISTENT, - WARNING_NON_PERSISTENT, - ERROR_NON_PERSISTENT, -) +# Type +ERROR = "error" +WARNING = "warning" +INFO = "info" +NOTE = "note" +TIP = "tip" diff --git a/readthedocs/notifications/email.py b/readthedocs/notifications/email.py new file mode 100644 index 00000000000..c4fa36cf230 --- /dev/null +++ b/readthedocs/notifications/email.py @@ -0,0 +1,96 @@ +"""Email notifications.""" + +import structlog +from django.conf import settings +from django.db import models +from django.template import Context, Template +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 + +log = structlog.get_logger(__name__) + + +class EmailNotification: + + """ + An unsent notification linked to an object. + + This class provides an interface to construct notification messages by + rendering Django templates and send them via email. + + Call .send() to send the email notification. + """ + + name = None + context_object_name = "object" + app_templates = None + subject = None + user = None + extra_tags = "" + + def __init__(self, context_object, user, extra_context=None): + self.context_object = context_object + self.extra_context = extra_context or {} + self.user = user + + def get_subject(self): + template = Template(self.subject) + return template.render(context=Context(self.get_context_data())) + + def get_context_data(self): + context = { + 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(None)) + return context + + def get_template_names(self, source_format=constants.HTML): + names = [] + 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="email", + source_format=source_format, + ), + ) + return names + + raise AttributeError("Template for this email not found.") + + def render(self, source_format=constants.HTML): + return render_to_string( + template_name=self.get_template_names( + source_format=source_format, + ), + context=self.get_context_data(), + ) + + def send(self): + """ + 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. + """ + + 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(), + ) 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/messages.py b/readthedocs/notifications/messages.py new file mode 100644 index 00000000000..2f0d01a5bbe --- /dev/null +++ b/readthedocs/notifications/messages.py @@ -0,0 +1,513 @@ +import textwrap + +from django.utils.translation import gettext_noop as _ + +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, NOTE, TIP, WARNING + + +class Message: + def __init__(self, id, header, body, type, icon_classes=None): + self.id = id + self.header = header + self.body = body + self.type = type # (ERROR, WARNING, INFO, NOTE, TIP) + self.icon_classes = icon_classes + self.format_values = {} + + def __repr__(self): + return f"" + + 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_classes(self): + if self.icon_classes: + return self.icon_classes + + # Default classes that apply to all the notifications + classes = [ + "fas", + ] + + if self.type == ERROR: + classes.append("fa-circle-xmark") + if self.type == WARNING: + classes.append("fa-circle-exclamation") + if self.type == INFO: + classes.append("fa-circle-info") + if self.type == NOTE: + classes.append("fa-circle-info") + if self.type == TIP: + classes.append("fa-circle-info") + + return " ".join(classes) + + def get_rendered_header(self): + return self.header.format(**self.format_values) + + def get_rendered_body(self): + return self.body.format(**self.format_values) + + +# 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=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=_( + textwrap.dedent( + """ + 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}). + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=BuildAppError.BUILD_TERMINATED_DUE_INACTIVITY, + header=_("Build terminated due inactivity"), + body=_( + textwrap.dedent( + """ + This build was terminated due to inactivity. + If you continue to encounter this error, + file a support request and reference this build id ({instance.pk}). + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=BuildUserError.GENERIC, + header=_("Unknown problem"), + body=_( + textwrap.dedent( + """ + 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. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=BuildMaxConcurrencyError.LIMIT_REACHED, + header=_("Maximum concurrency limit reached."), + body=_( + textwrap.dedent( + """ + Concurrency limit reached ({limit}), retrying in 5 minutes. + """ + ).strip(), + ), + type=INFO, + ), + Message( + id=BuildCancelled.CANCELLED_BY_USER, + header=_("Build cancelled manually."), + body=_( + textwrap.dedent( + """ + The user has cancelled this build. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=BuildUserError.SKIPPED_EXIT_CODE_183, + header=_("Build skipped manually."), + body=_( + textwrap.dedent( + """ + This build was skipped because + one of the commands exited with code 183 + """ + ).strip(), + ), + type=INFO, + ), + Message( + id=BuildUserError.BUILD_TIME_OUT, + header=_("Build exited due to time out."), + body=_( + textwrap.dedent( + """ + Build exited due to time out. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_EXCESSIVE_MEMORY, + header=_("Build exited due to excessive memory consumption."), + body=_( + textwrap.dedent( + """ + Build exited due to excessive memory consumption. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=BuildAppError.BUILD_DOCKER_UNKNOWN_ERROR, + header=_("Build exited due to unknown error."), + body=_( + textwrap.dedent( + """ + Build exited due to unknown error: {message} + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=BuildAppError.BUILDS_DISABLED, + header=_("Builds are temporary disabled for this project."), + body=_( + textwrap.dedent( + """ + This is due to excessive usage of our resources. + Please, contact our support team if you think this is a mistake + and builds should be re-enabled. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=BuildUserError.MAX_CONCURRENCY, + header=_("Concurrency limit reached"), + body=_( + textwrap.dedent( + """ + Your project, organization, or user is currently building the maximum concurrency builds allowed ({limit}). + It will automatically retry in 5 minutes. + """ + ).strip(), + ), + type=WARNING, + ), + Message( + id=BuildUserError.BUILD_COMMANDS_WITHOUT_OUTPUT, + header=_("No HTML content found"), + body=_( + textwrap.dedent( + f""" + No "{BUILD_COMMANDS_OUTPUT_PATH_HTML}" folder was created during this build. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_OUTPUT_IS_NOT_A_DIRECTORY, + header=_("Build output directory is not a directory"), + body=_( + textwrap.dedent( + """ + Build output directory for format "{artifact_type}" is not a directory. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_OUTPUT_HAS_0_FILES, + header=_("Build output directory doesn't contain any file"), + body=_( + textwrap.dedent( + """ + 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. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_OUTPUT_HAS_MULTIPLE_FILES, + header=_("Build output directory contains multiple files"), + body=_( + textwrap.dedent( + """ + 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. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_OUTPUT_HTML_NO_INDEX_FILE, + header=_("Index file is not present in HTML output directory"), + body=_( + textwrap.dedent( + """ + 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. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_OUTPUT_OLD_DIRECTORY_USED, + header=_("Your project is outputing files in an old directory"), + body=_( + textwrap.dedent( + """ + 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. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=BuildUserError.NO_CONFIG_FILE_DEPRECATED, + header=_("Your project doesn't have a .readthedocs.yaml file"), + body=_( + textwrap.dedent( + """ + 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 + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_IMAGE_CONFIG_KEY_DEPRECATED, + header=_("Configuration key build.image is deprecated"), + body=_( + textwrap.dedent( + """ + 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 + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_OS_REQUIRED, + header=_("Configuration key build.os is required"), + body=_( + textwrap.dedent( + """ + 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 + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=BuildUserError.FILE_TOO_LARGE, + header=_("There is at least one file that exceeds the size limit"), + body=_( + textwrap.dedent( + """ + A file from your build process is too large to be processed by Read the Docs. + Please ensure no generated files are larger than 1GB. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_OUTPUT_HAS_NO_PDF_FILES, + header=_("There is no PDF file in output directory"), + body=_( + textwrap.dedent( + f""" + PDF file was not generated/found in "{BUILD_COMMANDS_OUTPUT_PATH_HTML}/pdf" output directory. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=BuildUserError.BUILD_COMMANDS_IN_BETA, + header=_("Config key build.commands is in beta"), + body=_( + textwrap.dedent( + """ + 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. + """ + ).strip(), + ), + type=INFO, + ), + Message( + id=BuildUserError.TEX_FILE_NOT_FOUND, + header=_("No TeX files were found"), + body=_( + textwrap.dedent( + """ + No TeX files were found. + """ + ).strip(), + ), + type=ERROR, + ), +] + +BUILD_MKDOCS_MESSAGES = [ + Message( + id=MkDocsYAMLParseError.GENERIC_WITH_PARSE_EXCEPTION, + header=_(""), + body=_( + textwrap.dedent( + """ + Problem parsing MkDocs YAML configuration. {exception} + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=MkDocsYAMLParseError.INVALID_DOCS_DIR_CONFIG, + header=_(""), + body=_( + textwrap.dedent( + """ + The "docs_dir" config from your MkDocs YAML config file has to be a + string with relative or absolute path. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=MkDocsYAMLParseError.INVALID_DOCS_DIR_PATH, + header=_(""), + body=_( + textwrap.dedent( + """ + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=MkDocsYAMLParseError.INVALID_EXTRA_CONFIG, + header=_(""), + body=_( + textwrap.dedent( + """ + The "{config}" config from your MkDocs YAML config file has to be a + list of relative paths. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=MkDocsYAMLParseError.EMPTY_CONFIG, + header=_(""), + body=_( + textwrap.dedent( + """ + Please make sure the MkDocs YAML configuration file is not empty. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=MkDocsYAMLParseError.NOT_FOUND, + header=_(""), + body=_( + textwrap.dedent( + """ + A configuration file was not found. + Make sure you have a "mkdocs.yml" file in your repository. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=MkDocsYAMLParseError.CONFIG_NOT_DICT, + header=_(""), + body=_( + textwrap.dedent( + """ + Your MkDocs YAML config file is incorrect. + Please follow the user guide https://www.mkdocs.org/user-guide/configuration/ + to configure the file properly. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=MkDocsYAMLParseError.SYNTAX_ERROR, + header=_("Syntax error in mkdocs.yml"), + body=_( + textwrap.dedent( + """ + Your mkdocs.yml could not be loaded, + possibly due to a syntax error. + """ + ).strip(), + ), + type=ERROR, + ), +] + + +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(BUILD_MKDOCS_MESSAGES) +registry.add(BUILD_MESSAGES) diff --git a/readthedocs/notifications/migrations/0001_initial.py b/readthedocs/notifications/migrations/0001_initial.py new file mode 100644 index 00000000000..2b4dc2e142c --- /dev/null +++ b/readthedocs/notifications/migrations/0001_initial.py @@ -0,0 +1,76 @@ +# 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"), + ("dismissed", "dismissed"), + ("cancelled", "cancelled"), + ], + db_index=True, + default="unread", + max_length=128, + ), + ), + ("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/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/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..7db34818f24 --- /dev/null +++ b/readthedocs/notifications/models.py @@ -0,0 +1,97 @@ +import textwrap + +import structlog +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 readthedocs.core.context_processors import readthedocs_processor + +from .constants import CANCELLED, DISMISSED, READ, UNREAD, WARNING +from .messages import Message, registry +from .querysets import NotificationQuerySet + +log = structlog.get_logger(__name__) + + +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, + max_length=128, + 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 Organization, Project, Build or User + # + # 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") + + # 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 + + def get_message(self): + message = registry.get(self.message_id) + if message is None: + # Log the error and return an unknown error to avoid breaking the response. + log.error( + "The message ID retrieved is not in our registry anymore.", + message_id=self.message_id, + ) + return Message( + id="unknown-message", + header=_("Unknown message"), + body=_( + textwrap.dedent( + """ + It seems it was an old message, + and is not in our registry anymore. + """ + ).strip(), + ), + 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 diff --git a/readthedocs/notifications/notification.py b/readthedocs/notifications/notification.py deleted file mode 100644 index 4c13569923f..00000000000 --- a/readthedocs/notifications/notification.py +++ /dev/null @@ -1,185 +0,0 @@ -"""Support for templating of notifications.""" - -import structlog -from django.conf import settings -from django.db import models -from django.http import HttpRequest -from django.template import Context, Template -from django.template.loader import render_to_string - -from readthedocs.core.context_processors import readthedocs_processor - -from . import constants -from .backends import send_notification - -log = structlog.get_logger(__name__) - - -class Notification: - - """ - 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. - - Call .send() to send the 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() - 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) - return template.render(context=Context(self.get_context_data())) - - def get_context_data(self): - context = { - self.context_object_name: self.object, - "request": self.request, - "production_uri": "{scheme}://{host}".format( - scheme="https", - host=settings.PRODUCTION_DOMAIN, - ), - } - context.update(self.extra_context) - context.update(readthedocs_processor(self.request)) - return context - - def get_template_names(self, backend_name, source_format=constants.HTML): - names = [] - if self.object and isinstance(self.object, models.Model): - meta = self.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, - source_format=source_format, - ), - ) - return names - - raise AttributeError() - - def render(self, backend_name, 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(), - ) - - def send(self): - """ - Trigger notification send through all notification backends. - - 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. - """ - send_notification(self.request, self) - - -class SiteNotification(Notification): - - """ - Simple notification to show *only* on site messages. - - ``success_message`` and ``failure_message`` can be a simple string or a - dictionary with different messages depending on the reason of the failure / - success. The message is selected by using ``reason`` to get the proper - value. - - The notification is tied to the ``user`` and it could be sticky, persistent - or normal --this depends on the ``success_level`` and ``failure_level``. - - .. note:: - - ``send_email`` is forced to False to not send accidental emails when - only a simple site notification is needed. - """ - - send_email = False - - success_message = None - failure_message = None - - success_level = constants.SUCCESS_NON_PERSISTENT - failure_level = constants.ERROR_NON_PERSISTENT - - def __init__( - self, - user, - success, - reason=None, - context_object=None, - request=None, - extra_context=None, - ): - self.object = context_object - - self.user = user or request.user - # Fake the request in case the notification is instantiated from a place - # without access to the request object (Celery task, for example) - self.request = request or HttpRequest() - self.request.user = user - - self.success = success - self.reason = reason - super().__init__(context_object, request, user, extra_context) - - def get_message_level(self): - if self.success: - return self.success_level - return self.failure_level - - def get_message(self, success): - if success: - message = self.success_message - else: - message = self.failure_message - - msg = "" # default message in case of error - if isinstance(message, dict): - if self.reason: - if self.reason in message.keys(): - msg = message.get(self.reason) - else: - # log the error but not crash - log.error( - "Notification has no key for messages", - notification=self.__class__.__name__, - key=self.reason, - message="success" if self.success else "failure", - ) - else: - log.error( - "{message} is a dictionary but no reason was provided", - notification=self.__class__.__name__, - message="success" if self.success else "failure", - ) - else: - msg = message - - return Template(msg).render(context=Context(self.get_context_data())) - - def render(self, *args, **kwargs): - return self.get_message(self.success) diff --git a/readthedocs/notifications/querysets.py b/readthedocs/notifications/querysets.py new file mode 100644 index 00000000000..787ecf17197 --- /dev/null +++ b/readthedocs/notifications/querysets.py @@ -0,0 +1,34 @@ +from django.contrib.contenttypes.models import ContentType +from django.db import models +from django.utils import timezone + + +class NotificationQuerySet(models.QuerySet): + def add(self, *args, **kwargs): + """ + Create a notification without duplicating it. + + If a notification with the same ``message_id`` is already attached to the object, + its ``modified_at`` timestamp is updated. + + Otherwise, a new notification object is created for this object. + """ + + message_id = kwargs.get("message_id") + attached_to = kwargs.pop("attached_to") + + content_type = ContentType.objects.get_for_model(attached_to) + notification = self.filter( + attached_to_content_type=content_type, + attached_to_id=attached_to.id, + message_id=message_id, + ).first() + + if notification: + self.filter(pk=notification.pk).update( + *args, modified=timezone.now(), **kwargs + ) + notification.refresh_from_db() + return notification + + return super().create(*args, attached_to=attached_to, **kwargs) diff --git a/readthedocs/notifications/storages.py b/readthedocs/notifications/storages.py deleted file mode 100644 index ce9e58aa346..00000000000 --- a/readthedocs/notifications/storages.py +++ /dev/null @@ -1,159 +0,0 @@ -"""Customised storage for notifications.""" - -from django.contrib.messages.storage.base import Message -from django.db.models import Q -from django.utils.safestring import mark_safe -from messages_extends.constants import PERSISTENT_MESSAGE_LEVELS -from messages_extends.models import Message as PersistentMessage -from messages_extends.storages import FallbackStorage, PersistentStorage - -from .constants import NON_PERSISTENT_MESSAGE_LEVELS - -try: - from django.utils import timezone -except ImportError: - from datetime import datetime as timezone - - -class FallbackUniqueStorage(FallbackStorage): - - """ - Persistent message fallback storage, but only stores unique notifications. - - This loops through all backends to find messages to store, but will skip - this step if the message already exists for the user in the database. - - Deduplication is important here, as a persistent message may ask a user to - perform a specific action, such as change a build option. Duplicated - messages would lead to confusing UX, where a duplicate message may not be - dismissed when the prescribed action is taken. Instead of detecting - duplication while triggering the message, we handle this at the storage - level. - - This class also assumes that notifications set as persistent messages are - more likely to have HTML that should be marked safe. If the level matches a - persistent message level, mark the message text as safe so that we can - render the text in templates. - """ - - storages_names = ( - "readthedocs.notifications.storages.NonPersistentStorage", - "messages_extends.storages.StickyStorage", - "messages_extends.storages.PersistentStorage", - "django.contrib.messages.storage.cookie.CookieStorage", - "django.contrib.messages.storage.session.SessionStorage", - ) - - def _get(self, *args, **kwargs): - # The database backend for persistent messages doesn't support setting - # messages with ``mark_safe``, therefore, we need to do it broadly here. - messages, all_ret = super()._get(self, *args, **kwargs) - - safe_messages = [] - for message in messages: - # Handle all message types, if the message is persistent, take - # special action. As the default message handler, this will also - # process ephemeral messages - if ( - message.level - in PERSISTENT_MESSAGE_LEVELS + NON_PERSISTENT_MESSAGE_LEVELS - ): - message_pk = message.pk - message = Message( - message.level, - mark_safe(message.message), - message.extra_tags, - ) - message.pk = message_pk - safe_messages.append(message) - return safe_messages, all_ret - - def add(self, level, message, extra_tags="", *args, **kwargs): # noqa - user = kwargs.get("user") or self.request.user - if not user.is_anonymous: - persist_messages = PersistentMessage.objects.filter( - message=message, - user=user, - read=False, - ) - if persist_messages.exists(): - return - super().add(level, message, extra_tags, *args, **kwargs) - - -class NonPersistentStorage(PersistentStorage): - - """ - Save one time (non-pesistent) messages in the database. - - Messages are saved into the database. ``user`` object is mandatory but - ``request`` is needed. - """ - - # Non-persistent level numbers start at 100 and it's used to check if it's - # an actionable message or not - level = 100 - - def _message_queryset(self, include_read=False): - """Return a queryset of non persistent messages for the request user.""" - expire = timezone.now() - - qs = ( - PersistentMessage.objects.filter(user=self.get_user()) - .filter(Q(expires=None) | Q(expires__gt=expire)) - .filter(level__in=NON_PERSISTENT_MESSAGE_LEVELS) - ) - if not include_read: - qs = qs.exclude(read=True) - - # We need to save the objects returned by the query before updating it, - # otherwise they are marked as ``read`` and not returned when executed - result = list(qs) - - # Mark non-persistent messages as read when show them - qs.update(read=True) - - return result - - # These methods (_store, process_message) are copied from - # https://github.com/AliLozano/django-messages-extends/blob/master/messages_extends/storages.py - # and replaced PERSISTENT_MESSAGE_LEVELS by NON_PERSISTENT_MESSAGE_LEVELS - - def _store(self, messages, response, *args, **kwargs): - # There are already saved. - return [ - message - for message in messages - if message.level not in NON_PERSISTENT_MESSAGE_LEVELS - ] - - def process_message(self, message, *args, **kwargs): - """ - Convert messages to models and save them. - - If its level is into non-persistent levels, convert the message to - models and save it - """ - if message.level not in NON_PERSISTENT_MESSAGE_LEVELS: - return message - - user = kwargs.get("user") or self.get_user() - - try: - anonymous = user.is_anonymous - except TypeError: - anonymous = user.is_anonymous - if anonymous: - raise NotImplementedError( - "Persistent message levels cannot be used for anonymous users.", - ) - message_persistent = PersistentMessage() - message_persistent.level = message.level - message_persistent.message = message.message - message_persistent.extra_tags = message.extra_tags - message_persistent.user = user - - if "expires" in kwargs: - message_persistent.expires = kwargs["expires"] - message_persistent.save() - return None diff --git a/readthedocs/notifications/urls.py b/readthedocs/notifications/urls.py deleted file mode 100644 index 8659fc17b19..00000000000 --- a/readthedocs/notifications/urls.py +++ /dev/null @@ -1,17 +0,0 @@ -"""Renames for messages_extends URLs.""" - -from django.urls import path -from messages_extends.views import message_mark_all_read, message_mark_read - -urlpatterns = [ - path( - "dismiss//", - message_mark_read, - name="message_mark_read", - ), - path( - "dismiss/all/", - message_mark_all_read, - name="message_mark_all_read", - ), -] diff --git a/readthedocs/notifications/views.py b/readthedocs/notifications/views.py deleted file mode 100644 index 8b3cd23c725..00000000000 --- a/readthedocs/notifications/views.py +++ /dev/null @@ -1,120 +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 - - -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/oauth/notifications.py b/readthedocs/oauth/notifications.py index 0246606885f..257c8cfd39f 100644 --- a/readthedocs/oauth/notifications.py +++ b/readthedocs/oauth/notifications.py @@ -1,79 +1,98 @@ -from django.urls import reverse -from django.utils.translation import gettext_lazy as _ -from messages_extends.constants import ERROR_PERSISTENT +"""Notifications related to OAuth.""" + +import textwrap -from readthedocs.notifications import SiteNotification -from readthedocs.notifications.constants import ERROR_NON_PERSISTENT +from django.utils.translation import gettext_lazy as _ +from readthedocs.notifications.constants import ERROR, INFO +from readthedocs.notifications.messages import Message, registry -class AttachWebhookNotification(SiteNotification): - # Fail reasons - NO_PERMISSIONS = "no_permissions" - NO_ACCOUNTS = "no_accounts" +MESSAGE_OAUTH_WEBHOOK_NO_PERMISSIONS = "oauth:webhook:no-permissions" +MESSAGE_OAUTH_WEBHOOK_NO_ACCOUNT = "oauth:webhook:no-account" +MESSAGE_OAUTH_WEBHOOK_INVALID = "oauth:webhook:invalid" +MESSAGE_OAUTH_BUILD_STATUS_FAILURE = "oauth:status:send-failed" +MESSAGE_OAUTH_DEPLOY_KEY_ATTACHED_SUCCESSFULY = "oauth:deploy-key:attached-successfully" +MESSAGE_OAUTH_DEPLOY_KEY_ATTACHED_FAILED = "oauth:deploy-key:attached-failed" - context_object_name = "provider" - success_message = _("Webhook successfully added.") - failure_message = { - NO_PERMISSIONS: _( - 'Could not add webhook for {{ project.name }}. Make sure you have the correct {{ provider.name }} permissions.', # noqa +messages = [ + Message( + id=MESSAGE_OAUTH_WEBHOOK_NO_ACCOUNT, + header=_("Unable to attach webhook to this project"), + body=_( + textwrap.dedent( + """ + Could not add webhook for {instance.name}. + Please connect your {provider_name} account. + """ + ).strip(), ), - NO_ACCOUNTS: _( - 'Could not add webhook for {{ project.name }}. Please connect your {{ provider.name }} account.', # noqa + type=ERROR, + ), + Message( + id=MESSAGE_OAUTH_WEBHOOK_NO_PERMISSIONS, + header=_("Unable to attach webhook to this project"), + body=_( + textwrap.dedent( + """ + Could not add webhook for {instance.name}. + Make sure you have the correct {provider_name} permissions. + """ + ).strip(), ), - } - - def get_context_data(self): - context = super().get_context_data() - project = self.extra_context.get("project") - context.update( - { - "url_connect_account": reverse( - "projects_integrations", - args=[project.slug], - ), - "url_docs_webhook": "http://docs.readthedocs.io/en/latest/webhooks.html", # noqa - } - ) - return context - - -class InvalidProjectWebhookNotification(SiteNotification): - context_object_name = "project" - failure_level = ERROR_PERSISTENT - failure_message = _( - "The project {{ project.name }} doesn't have a valid webhook set up, " - "commits won't trigger new builds for this project. " - "See the project integrations for more information.", - ) # noqa - - def get_context_data(self): - context = super().get_context_data() - context.update( - { - "url_integrations": reverse( - "projects_integrations", - args=[self.object.slug], - ), - } - ) - return context - - -class GitBuildStatusFailureNotification(SiteNotification): - context_object_name = "project" - failure_level = ERROR_NON_PERSISTENT - failure_message = _( - "Could not send {{ provider_name }} build status report for {{ project.name }}. " - "Make sure you have the correct {{ provider_name }} repository permissions and " - 'your {{ provider_name }} account ' - "is connected to Read the Docs." - ) # noqa - - def get_context_data(self): - context = super().get_context_data() - context.update( - { - "url_connect_account": reverse("socialaccount_connections"), - } - ) - return context + type=ERROR, + ), + Message( + id=MESSAGE_OAUTH_WEBHOOK_INVALID, + header=_("The project doesn't have a valid webhook set up"), + body=_( + textwrap.dedent( + """ + The project {instance.name} doesn't have a valid webhook set up, + commits won't trigger new builds for this project. + See the project integrations for more information. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=MESSAGE_OAUTH_BUILD_STATUS_FAILURE, + header=_("{provier_name} build status report failed"), + body=_( + textwrap.dedent( + """ + Could not send {provider_name} build status report for {instance.name}. + Make sure you have the correct {provider_name} repository permissions and + your {provider_name} account + is connected to Read the Docs. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=MESSAGE_OAUTH_DEPLOY_KEY_ATTACHED_SUCCESSFULY, + header=_("Deploy key added successfully"), + body=_( + textwrap.dedent( + """ + Successfully added deploy key to {provider_name} project. + """ + ).strip(), + ), + type=INFO, + ), + Message( + id=MESSAGE_OAUTH_DEPLOY_KEY_ATTACHED_FAILED, + header=_("Failed to add deploy key to project"), + body=_( + textwrap.dedent( + """ + Failed to add deploy key to {provider_name} project, ensure you have the correct permissions and try importing again. + """ + ).strip(), + ), + type=ERROR, + ), +] +registry.add(messages) diff --git a/readthedocs/oauth/tasks.py b/readthedocs/oauth/tasks.py index a49bf924987..be7d83c00b3 100644 --- a/readthedocs/oauth/tasks.py +++ b/readthedocs/oauth/tasks.py @@ -6,13 +6,16 @@ from allauth.socialaccount.providers import registry as allauth_registry from django.contrib.auth.models import User from django.db.models.functions import ExtractIsoWeekDay +from django.urls import reverse from django.utils import timezone from readthedocs.core.permissions import AdminPermission from readthedocs.core.utils.tasks import PublicTask, user_id_matches_or_superuser +from readthedocs.notifications.models import Notification from readthedocs.oauth.notifications import ( - AttachWebhookNotification, - InvalidProjectWebhookNotification, + MESSAGE_OAUTH_WEBHOOK_INVALID, + MESSAGE_OAUTH_WEBHOOK_NO_ACCOUNT, + MESSAGE_OAUTH_WEBHOOK_NO_PERMISSIONS, ) from readthedocs.oauth.services.base import SyncServiceError from readthedocs.oauth.utils import SERVICE_MAP @@ -166,17 +169,22 @@ def attach_webhook(project_pk, user_pk, integration=None): if not project or not user: return False - project_notification = InvalidProjectWebhookNotification( - context_object=project, - user=user, - success=False, - ) if integration: service = SERVICE_MAP.get(integration.integration_type) if not service: log.warning("There are no registered services in the application.") - project_notification.send() + Notification.objects.add( + message_id=MESSAGE_OAUTH_WEBHOOK_INVALID, + attached_to=project, + dismissable=True, + format_values={ + "url_integrations": reverse( + "projects_integrations", + args=[project.slug], + ), + }, + ) return None else: for service_cls in registry: @@ -185,23 +193,27 @@ def attach_webhook(project_pk, user_pk, integration=None): break else: log.warning("There are no registered services in the application.") - project_notification.send() + Notification.objects.add( + message_id=MESSAGE_OAUTH_WEBHOOK_INVALID, + attached_to=project, + dismissable=True, + format_values={ + "url_integrations": reverse( + "projects_integrations", + args=[project.slug], + ), + }, + ) return None provider = allauth_registry.by_id(service.adapter.provider_id) - notification = AttachWebhookNotification( - context_object=provider, - extra_context={"project": project}, - user=user, - success=None, - ) user_accounts = service.for_user(user) for account in user_accounts: success, __ = account.setup_webhook(project, integration=integration) if success: - notification.success = True - notification.send() + # NOTE: do we want to communicate that we connect the webhook here? + # messages.add_message(request, "Webhook successfully added.") project.has_valid_webhook = True project.save() @@ -209,12 +221,27 @@ def attach_webhook(project_pk, user_pk, integration=None): # No valid account found if user_accounts: - notification.success = False - notification.reason = AttachWebhookNotification.NO_PERMISSIONS + Notification.objects.add( + message_id=MESSAGE_OAUTH_WEBHOOK_NO_PERMISSIONS, + dismissable=True, + attached_to=project, + format_values={ + "provider_name": provider.name, + "url_docs_webhook": "https://docs.readthedocs.io/page/webhooks.html", + }, + ) else: - notification.success = False - notification.reason = AttachWebhookNotification.NO_ACCOUNTS + Notification.objects.add( + message_id=MESSAGE_OAUTH_WEBHOOK_NO_ACCOUNT, + dismissable=True, + attached_to=project, + format_values={ + "provider_name": provider.name, + "url_connect_account": reverse( + "project_integrations", + args=[project.slug], + ), + }, + ) - project_notification.send() - notification.send() return False diff --git a/readthedocs/projects/admin.py b/readthedocs/projects/admin.py index 5a0bd4a005e..63956565657 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 @@ -29,7 +28,6 @@ WebHook, WebHookEvent, ) -from .notifications import ResourceUsageNotification from .tag_utils import import_tags from .tasks.utils import clean_project_resources @@ -50,16 +48,6 @@ def has_delete_permission(self, request, obj=None): return False -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 +248,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 +264,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/apps.py b/readthedocs/projects/apps.py index c1154b6c5ad..f4a1525c190 100644 --- a/readthedocs/projects/apps.py +++ b/readthedocs/projects/apps.py @@ -7,6 +7,8 @@ class ProjectsConfig(AppConfig): name = "readthedocs.projects" def ready(self): + # Load and register notification messages for this application + import readthedocs.projects.notifications # noqa import readthedocs.projects.tasks.builds # noqa import readthedocs.projects.tasks.search # noqa import readthedocs.projects.tasks.utils # noqa diff --git a/readthedocs/projects/exceptions.py b/readthedocs/projects/exceptions.py index a48106e7ed0..fa15a79a2fe 100644 --- a/readthedocs/projects/exceptions.py +++ b/readthedocs/projects/exceptions.py @@ -1,7 +1,5 @@ """Project exceptions.""" -from django.conf import settings -from django.utils.translation import gettext_noop as _ from readthedocs.doc_builder.exceptions import BuildAppError, BuildUserError @@ -10,60 +8,23 @@ class ProjectConfigurationError(BuildUserError): """Error raised trying to configure a project for build.""" - NOT_FOUND = _( - "A configuration file was not found. " - "Make sure you have a conf.py file in your repository.", - ) - - MULTIPLE_CONF_FILES = _( - "We found more than one conf.py and are not sure which one to use. " - "Please, specify the correct file under the Advanced settings tab " - "in the project's Admin.", - ) + NOT_FOUND = "project:sphinx:conf-py-not-found" + MULTIPLE_CONF_FILES = "project:sphinx:multiple-conf-py-files-found" class UserFileNotFound(BuildUserError): - FILE_NOT_FOUND = _("The file {} doesn't exist. Make sure it's a valid file path.") + FILE_NOT_FOUND = "project:file:not-found" class RepositoryError(BuildUserError): """Failure during repository operation.""" - PRIVATE_ALLOWED = _( - "There was a problem connecting to your repository, " - "ensure that your repository URL is correct.", - ) - PRIVATE_NOT_ALLOWED = _( - "There was a problem connecting to your repository, " - "ensure that your repository URL is correct and your repository is public. " - "Private repositories are not supported.", - ) - DUPLICATED_RESERVED_VERSIONS = _( - "You can not have two versions with the name latest or stable." - " Ensure you don't have both a branch and a tag with this name." - ) - - FAILED_TO_CHECKOUT = _("Failed to checkout revision: {}") - - GENERIC_ERROR = _( - "There was a problem cloning your repository. " - "Please check the command output for more information.", - ) - - # NOTE: we are not using `@property` here because Python 3.8 does not - # suport `@property` together with `@classmethod`. On Python >= 3.9, we - # could call `RepositoryError.CLONE_ERROR` without parenthesis and it will - # work. However, for now, we are just using a class method and calling it - # as a function/method. - @classmethod - def CLONE_ERROR(cls): # noqa: N802 - if settings.ALLOW_PRIVATE_REPOS: - return cls.PRIVATE_ALLOWED - return cls.PRIVATE_NOT_ALLOWED - - def get_default_message(self): - return self.GENERIC_ERROR + CLONE_ERROR_WITH_PRIVATE_REPO_ALLOWED = "project:repository:private-clone-error" + CLONE_ERROR_WITH_PRIVATE_REPO_NOT_ALLOWED = "project:repository:public-clone-error" + DUPLICATED_RESERVED_VERSIONS = "project:repository:duplicated-reserved-versions" + FAILED_TO_CHECKOUT = "project:repository:checkout-failed" + GENERIC = "project:repository:generic-error" class SyncRepositoryLocked(BuildAppError): diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index d27e0fafb2d..5dea927ed40 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 NewNotification from readthedocs.projects import constants from readthedocs.projects.exceptions import ProjectConfigurationError from readthedocs.projects.managers import HTMLFileManager @@ -537,6 +538,13 @@ class Project(models.Model): blank=True, ) + notifications = GenericRelation( + NewNotification, + related_query_name="project", + 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: @@ -1588,6 +1596,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( diff --git a/readthedocs/projects/notifications.py b/readthedocs/projects/notifications.py index 81a5178dca9..d8973e5b6e7 100644 --- a/readthedocs/projects/notifications.py +++ b/readthedocs/projects/notifications.py @@ -1,28 +1,135 @@ -"""Project notifications.""" +"""Notifications related to projects.""" +import textwrap -from django.urls import reverse -from django.utils.translation import gettext_lazy as _ -from messages_extends.constants import ERROR_PERSISTENT +from django.utils.translation import gettext_noop as _ -from readthedocs.notifications import Notification, SiteNotification -from readthedocs.notifications.constants import REQUIREMENT +from readthedocs.notifications.constants import ERROR, INFO +from readthedocs.notifications.messages import Message, registry +from readthedocs.projects.exceptions import ( + ProjectConfigurationError, + RepositoryError, + UserFileNotFound, +) - -class ResourceUsageNotification(Notification): - name = "resource_usage" - context_object_name = "project" - subject = "Builds for {{ project.name }} are using too many resources" - level = REQUIREMENT - - -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 +MESSAGE_PROJECT_SKIP_BUILDS = "project:invalid:skip-builds" +messages = [ + Message( + id=MESSAGE_PROJECT_SKIP_BUILDS, + header=_("Build skipped for this project"), + body=_( + textwrap.dedent( + """ + Your project is currently disabled for abuse of the system. + Please make sure it isn't using unreasonable amounts of resources or triggering lots of builds in a short amount of time. + Please contact support to get your project re-enabled. + """ + ).strip(), + ), + type=INFO, + ), + Message( + id=RepositoryError.GENERIC, + header=_("Error while cloning the repository"), + body=_( + textwrap.dedent( + """ + There was a problem cloning your repository. + Please check the command output for more information. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=RepositoryError.DUPLICATED_RESERVED_VERSIONS, + header=_("Duplicated reserved versions"), + body=_( + textwrap.dedent( + """ + You can not have two versions with the name latest or stable. + Ensure you don't have both a branch and a tag with this name. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=RepositoryError.FAILED_TO_CHECKOUT, + header=_("Error while checking out the repository"), + body=_( + textwrap.dedent( + """ + Failed to checkout revision: {revision} + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=RepositoryError.CLONE_ERROR_WITH_PRIVATE_REPO_ALLOWED, + header=_("Error while cloning the repository"), + body=_( + textwrap.dedent( + """ + There was a problem connecting to your repository, + ensure that your repository URL is correct. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=RepositoryError.CLONE_ERROR_WITH_PRIVATE_REPO_NOT_ALLOWED, + header=_("Error while cloning the repository"), + body=_( + textwrap.dedent( + """ + There was a problem connecting to your repository, + ensure that your repository URL is correct and your repository is public. + Private repositories are not supported. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ProjectConfigurationError.NOT_FOUND, + header=_("Sphinx configuration file is missing"), + body=_( + textwrap.dedent( + """ + A configuration file was not found. + Make sure you have a conf.py file in your repository. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=ProjectConfigurationError.MULTIPLE_CONF_FILES, + header=_("Multiple Sphinx configuration files found"), + body=_( + textwrap.dedent( + """ + We found more than one conf.py and are not sure which one to use. + Please, specify the correct file under the Advanced settings tab + in the project's Admin. + """ + ).strip(), + ), + type=ERROR, + ), + Message( + id=UserFileNotFound.FILE_NOT_FOUND, + header=_("Expected file not found"), + body=_( + textwrap.dedent( + """ + The file {filename} doesn't exist. Make sure it's a valid file path. + """ + ).strip(), + ), + type=ERROR, + ), +] +registry.add(messages) diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index 19a0e508a85..ccd7ef219a6 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/readthedocs/projects/tasks/builds.py @@ -37,8 +37,8 @@ from readthedocs.builds.models import APIVersion, Build from readthedocs.builds.signals import build_complete from readthedocs.builds.utils import memcache_lock -from readthedocs.config import ConfigError from readthedocs.config.config import BuildConfigV2 +from readthedocs.config.exceptions import ConfigError from readthedocs.doc_builder.director import BuildDirector from readthedocs.doc_builder.environments import ( DockerBuildEnvironment, @@ -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, @@ -305,16 +299,14 @@ class UpdateDocsTask(SyncRepositoryMixin, Task): # Do not send notifications on failure builds for these exceptions. exceptions_without_notifications = ( - BuildCancelled, - BuildMaxConcurrencyError, - BuildUserSkip, - ProjectBuildsSkippedError, + BuildCancelled.CANCELLED_BY_USER, + BuildUserError.MAX_CONCURRENCY, + BuildUserError.SKIPPED_EXIT_CODE_183, + BuildAppError.BUILDS_DISABLED, ) # Do not send external build status on failure builds for these exceptions. - exceptions_without_external_build_status = ( - BuildMaxConcurrencyError, - ) + exceptions_without_external_build_status = (BuildUserError.MAX_CONCURRENCY,) acks_late = True track_started = True @@ -339,7 +331,7 @@ def sigint_received(*args, **kwargs): log.warning('Ignoring cancelling the build at "Uploading" state.') return - raise BuildCancelled + raise BuildCancelled(message_id=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 +365,17 @@ 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, + format_values={ + "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 @@ -417,6 +410,10 @@ def before_start(self, task_id, args, kwargs): # because they just build the latest commit for that version self.data.build_commit = kwargs.get('build_commit') + self.data.build_director = BuildDirector( + data=self.data, + ) + log.bind( # NOTE: ``self.data.build`` is just a regular dict, not an APIBuild :'( builder=self.data.build['builder'], @@ -475,46 +472,37 @@ def on_failure(self, exc, task_id, args, kwargs, einfo): 'id': self.data.build_pk, } - # 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), - ), - ), - ) # 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'], - ) + if isinstance(exc, BuildAppError): + 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 - # 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 + 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 = BuildUserError.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 = 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 + + # Attach the notification to the build + self.data.build_director.attach_notification(message_id, 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'], @@ -527,15 +515,16 @@ def on_failure(self, exc, task_id, args, kwargs, einfo): # Oh, I think this is to differentiate a task triggered with # `Build.commit` than a one triggered just with the `Version` to build # the _latest_ commit of it - if self.data.build_commit and not isinstance( - exc, self.exceptions_without_external_build_status + if ( + self.data.build_commit + and message_id not in self.exceptions_without_external_build_status ): version_type = None if self.data.version: 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 +583,10 @@ 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, + format_values={ + "artifact_type": artifact_type, + }, ) # Check if there are multiple files on artifact directories. @@ -694,7 +684,9 @@ def on_retry(self, exc, task_id, args, kwargs, einfo): project_slug=self.data.project.slug, version_slug=self.data.version.slug, ) - self.data.build['error'] = exc.message + self.data.director.attach_notification( + BuildMaxConcurrencyError.LIMIT_REACHED + ) self.update_build(state=BUILD_STATE_TRIGGERED) def after_return(self, status, retval, task_id, args, kwargs, einfo): @@ -762,10 +754,6 @@ def update_build(self, state=None): log.exception("Error while updating the build object.", state=state) def execute(self): - self.data.build_director = BuildDirector( - data=self.data, - ) - # Clonning self.update_build(state=BUILD_STATE_CLONING) diff --git a/readthedocs/projects/tasks/utils.py b/readthedocs/projects/tasks/utils.py index 45056023204..e157f14051b 100644 --- a/readthedocs/projects/tasks/utils.py +++ b/readthedocs/projects/tasks/utils.py @@ -8,7 +8,6 @@ from django.conf import settings from django.db.models import Q from django.utils import timezone -from django.utils.translation import gettext_lazy as _ from readthedocs.builds.constants import ( BUILD_FINAL_STATES, @@ -18,6 +17,8 @@ from readthedocs.builds.models import Build from readthedocs.builds.tasks import send_build_status from readthedocs.core.utils.filesystem import safe_rmtree +from readthedocs.doc_builder.exceptions import BuildAppError +from readthedocs.notifications.models import Notification from readthedocs.storage import build_media_storage from readthedocs.worker import app @@ -129,12 +130,13 @@ def finish_inactive_builds(): build.success = False build.state = BUILD_STATE_CANCELLED - build.error = _( - "This build was terminated due to inactivity. If you " - "continue to encounter this error, file a support " - "request with and reference this build id ({}).".format(build.pk), - ) build.save() + + Notification.objects.add( + message_id=BuildAppError.BUILD_TERMINATED_DUE_INACTIVITY, + attached_to=build, + ) + builds_finished.append(build.pk) projects_finished.add(build.project.slug) diff --git a/readthedocs/projects/tests/mockers.py b/readthedocs/projects/tests/mockers.py index 1149b4016d8..dc4e9a23fe6 100644 --- a/readthedocs/projects/tests/mockers.py +++ b/readthedocs/projects/tests/mockers.py @@ -242,3 +242,8 @@ def _mock_api(self): f"{settings.SLUMBER_API_HOST}/api/v2/revoke/", status_code=204, ) + + self.requestsmock.post( + f"{settings.SLUMBER_API_HOST}/api/v2/notifications/", + status_code=204, + ) diff --git a/readthedocs/projects/tests/test_build_tasks.py b/readthedocs/projects/tests/test_build_tasks.py index de60cd4d5f3..10578bc8ed4 100644 --- a/readthedocs/projects/tests/test_build_tasks.py +++ b/readthedocs/projects/tests/test_build_tasks.py @@ -13,10 +13,11 @@ EXTERNAL, ) from readthedocs.builds.models import Build -from readthedocs.config import ALL, ConfigError +from readthedocs.config import ALL from readthedocs.config.config import BuildConfigV2 +from readthedocs.config.exceptions import ConfigError from readthedocs.config.tests.test_config import get_build_config -from readthedocs.doc_builder.exceptions import BuildAppError +from readthedocs.doc_builder.exceptions import BuildUserError from readthedocs.projects.exceptions import RepositoryError from readthedocs.projects.models import EnvironmentVariable, Project, WebHookEvent from readthedocs.projects.tasks.builds import sync_repository_task, update_docs_task @@ -637,11 +638,10 @@ def test_failed_build( # Force an exception from the execution of the task. We don't really # care "where" it was raised: setup, build, syncing directories, etc - execute.side_effect = Exception('Force and exception here.') + execute.side_effect = BuildUserError(message_id=BuildUserError.GENERIC) self._trigger_update_docs_task() - # It has to be called twice, ``before_start`` and ``after_return`` clean_build.assert_has_calls([ mock.call(mock.ANY), # the argument is an APIVersion @@ -670,15 +670,27 @@ def test_failed_build( # and the task won't be run. assert not BuildData.objects.all().exists() + notification_request = self.requests_mock.request_history[-3] + assert notification_request._request.method == "POST" + assert notification_request.path == "/api/v2/notifications/" + assert notification_request.json() == { + "attached_to": f"build/{self.build.pk}", + "message_id": BuildUserError.GENERIC, + "state": "unread", + "dismissable": False, + "news": False, + "format_values": {}, + } + # Test we are updating the DB by calling the API with the updated build object # The second last one should be the PATCH for the build - api_request = self.requests_mock.request_history[-2] - assert api_request._request.method == "PATCH" - assert api_request.path == "/api/v2/build/1/" - assert api_request.json() == { + build_status_request = self.requests_mock.request_history[-2] + assert build_status_request._request.method == "PATCH" + assert build_status_request.path == "/api/v2/build/1/" + assert build_status_request.json() == { "builder": mock.ANY, "commit": self.build.commit, - "error": BuildAppError.GENERIC_WITH_BUILD_ID.format(build_id=self.build.pk), + "error": "", # We are not sending ``error`` anymore "id": self.build.pk, "length": mock.ANY, "state": "finished", @@ -1851,21 +1863,32 @@ class TestBuildTaskExceptionHandler(BuildEnvironmentBase): @mock.patch("readthedocs.doc_builder.director.load_yaml_config") def test_config_file_exception(self, load_yaml_config): load_yaml_config.side_effect = ConfigError( - code="invalid", message="Invalid version in config file." + message_id=ConfigError.INVALID_VERSION, ) self._trigger_update_docs_task() - # This is a known exceptions. We hit the API saving the correct error - # in the Build object. In this case, the "error message" coming from - # the exception will be shown to the user - api_request = self.requests_mock.request_history[-2] - assert api_request._request.method == "PATCH" - assert api_request.path == "/api/v2/build/1/" - assert api_request.json() == { + # This is a known exceptions. We hit the notification API to attach a + # notification to this particular build. + notification_request = self.requests_mock.request_history[-3] + assert notification_request._request.method == "POST" + assert notification_request.path == "/api/v2/notifications/" + assert notification_request.json() == { + "attached_to": f"build/{self.build.pk}", + "message_id": ConfigError.INVALID_VERSION, + "state": "unread", + "dismissable": False, + "news": False, + "format_values": {}, + } + + build_status_request = self.requests_mock.request_history[-2] + assert build_status_request._request.method == "PATCH" + assert build_status_request.path == "/api/v2/build/1/" + assert build_status_request.json() == { "id": 1, "state": "finished", "commit": "a1b2c3", - "error": "Problem in your project's configuration. Invalid version in config file.", + "error": "", # We not sending "error" anymore "success": False, "builder": mock.ANY, "length": 0, @@ -1926,4 +1949,4 @@ def test_check_duplicate_reserved_version_latest(self, on_failure, verbose_name) exception = on_failure.call_args[0][0] assert isinstance(exception, RepositoryError) == True - assert exception.message == RepositoryError.DUPLICATED_RESERVED_VERSIONS + assert exception.message_id == RepositoryError.DUPLICATED_RESERVED_VERSIONS diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 9c895d8f9e0..4068063a937 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,17 +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: - 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') diff --git a/readthedocs/rtd_tests/tests/test_backend.py b/readthedocs/rtd_tests/tests/test_backend.py index 720276d7fde..1f66323f8d7 100644 --- a/readthedocs/rtd_tests/tests/test_backend.py +++ b/readthedocs/rtd_tests/tests/test_backend.py @@ -160,8 +160,8 @@ def test_git_checkout_invalid_revision(self): with self.assertRaises(RepositoryError) as e: repo.checkout(version) self.assertEqual( - str(e.exception), - RepositoryError.FAILED_TO_CHECKOUT.format(version), + e.exception.message_id, + RepositoryError.FAILED_TO_CHECKOUT, ) def test_check_for_submodules(self): @@ -408,8 +408,8 @@ def test_checkout_invalid_revision(self): with self.assertRaises(RepositoryError) as e: repo.checkout(version) self.assertEqual( - str(e.exception), - RepositoryError.FAILED_TO_CHECKOUT.format(version), + e.exception.message_id, + RepositoryError.FAILED_TO_CHECKOUT, ) def test_parse_tags(self): diff --git a/readthedocs/rtd_tests/tests/test_builds.py b/readthedocs/rtd_tests/tests/test_builds.py index 088e229a7c9..a0c2fa171f5 100644 --- a/readthedocs/rtd_tests/tests/test_builds.py +++ b/readthedocs/rtd_tests/tests/test_builds.py @@ -248,24 +248,6 @@ def test_build_is_stale(self): self.assertTrue(build_two.is_stale) self.assertFalse(build_three.is_stale) - def test_deprecated_config_used(self): - now = timezone.now() - - build = get( - Build, - project=self.project, - version=self.version, - date=now - datetime.timedelta(minutes=8), - state="finished", - ) - - self.assertTrue(build.deprecated_config_used()) - - build.config = {"version": 2} - build.save() - - self.assertFalse(build.deprecated_config_used()) - def test_build_is_external(self): # Turn the build version to EXTERNAL type. self.version.type = EXTERNAL diff --git a/readthedocs/rtd_tests/tests/test_celery.py b/readthedocs/rtd_tests/tests/test_celery.py index b7bd05cfa0a..54df2b23db6 100644 --- a/readthedocs/rtd_tests/tests/test_celery.py +++ b/readthedocs/rtd_tests/tests/test_celery.py @@ -5,12 +5,13 @@ from django.contrib.auth.models import User from django.test import TestCase from django_dynamic_fixture import get -from messages_extends.models import Message from readthedocs.builds import tasks as build_tasks from readthedocs.builds.constants import BUILD_STATUS_SUCCESS, EXTERNAL, LATEST from readthedocs.builds.models import Build, Version +from readthedocs.notifications.models import Notification from readthedocs.oauth.models import RemoteRepository, RemoteRepositoryRelation +from readthedocs.oauth.notifications import MESSAGE_OAUTH_BUILD_STATUS_FAILURE from readthedocs.projects.models import Project @@ -105,7 +106,7 @@ def test_send_build_status_with_remote_repo_github(self, send_build_status): external_build.commit, BUILD_STATUS_SUCCESS, ) - self.assertEqual(Message.objects.filter(user=self.eric).count(), 0) + self.assertEqual(Notification.objects.count(), 0) @patch("readthedocs.oauth.services.github.GitHubService.send_build_status") def test_send_build_status_with_social_account_github(self, send_build_status): @@ -125,7 +126,7 @@ def test_send_build_status_with_social_account_github(self, send_build_status): external_build.commit, BUILD_STATUS_SUCCESS, ) - self.assertEqual(Message.objects.filter(user=self.eric).count(), 0) + self.assertEqual(Notification.objects.count(), 0) @patch("readthedocs.oauth.services.github.GitHubService.send_build_status") def test_send_build_status_no_remote_repo_or_social_account_github( @@ -140,7 +141,17 @@ def test_send_build_status_no_remote_repo_or_social_account_github( ) send_build_status.assert_not_called() - self.assertEqual(Message.objects.filter(user=self.eric).count(), 1) + self.assertEqual(Notification.objects.count(), 1) + notification = Notification.objects.all().first() + self.assertEqual(notification.message_id, MESSAGE_OAUTH_BUILD_STATUS_FAILURE) + self.assertEqual(notification.attached_to, self.project) + self.assertEqual( + notification.format_values, + { + "provider_name": "GitHub", + "url_connect_account": "/accounts/social/connections/", + }, + ) @patch("readthedocs.oauth.services.gitlab.GitLabService.send_build_status") def test_send_build_status_with_remote_repo_gitlab(self, send_build_status): @@ -168,7 +179,7 @@ def test_send_build_status_with_remote_repo_gitlab(self, send_build_status): external_build.commit, BUILD_STATUS_SUCCESS, ) - self.assertEqual(Message.objects.filter(user=self.eric).count(), 0) + self.assertEqual(Notification.objects.count(), 0) @patch("readthedocs.oauth.services.gitlab.GitLabService.send_build_status") def test_send_build_status_with_social_account_gitlab(self, send_build_status): @@ -188,7 +199,7 @@ def test_send_build_status_with_social_account_gitlab(self, send_build_status): external_build.commit, BUILD_STATUS_SUCCESS, ) - self.assertEqual(Message.objects.filter(user=self.eric).count(), 0) + self.assertEqual(Notification.objects.count(), 0) @patch("readthedocs.oauth.services.gitlab.GitLabService.send_build_status") def test_send_build_status_no_remote_repo_or_social_account_gitlab( @@ -203,4 +214,14 @@ def test_send_build_status_no_remote_repo_or_social_account_gitlab( ) send_build_status.assert_not_called() - self.assertEqual(Message.objects.filter(user=self.eric).count(), 1) + self.assertEqual(Notification.objects.count(), 1) + notification = Notification.objects.all().first() + self.assertEqual(notification.message_id, MESSAGE_OAUTH_BUILD_STATUS_FAILURE) + self.assertEqual(notification.attached_to, self.project) + self.assertEqual( + notification.format_values, + { + "provider_name": "GitLab", + "url_connect_account": "/accounts/social/connections/", + }, + ) diff --git a/readthedocs/rtd_tests/tests/test_doc_builder.py b/readthedocs/rtd_tests/tests/test_doc_builder.py index 23fae392bc9..08004d6b0ad 100644 --- a/readthedocs/rtd_tests/tests/test_doc_builder.py +++ b/readthedocs/rtd_tests/tests/test_doc_builder.py @@ -113,11 +113,14 @@ def test_project_without_conf_py( build_env=self.build_env, python_env=python_env, ) - with pytest.raises( - ProjectConfigurationError, match=ProjectConfigurationError.NOT_FOUND - ): + with self.assertRaises(ProjectConfigurationError) as e: base_sphinx.append_conf() + self.assertEqual( + e.exception.message_id, + ProjectConfigurationError.NOT_FOUND, + ) + @patch("readthedocs.doc_builder.backends.sphinx.BaseSphinx.docs_dir") @patch("readthedocs.doc_builder.backends.sphinx.BaseSphinx.get_config_params") @patch("readthedocs.doc_builder.backends.sphinx.BaseSphinx.run") @@ -615,11 +618,10 @@ def test_empty_yaml_config(self, checkout_path): python_env=python_env, ) - with self.assertRaisesMessage( - MkDocsYAMLParseError, MkDocsYAMLParseError.EMPTY_CONFIG - ): + with self.assertRaises(MkDocsYAMLParseError) as exc: with override_settings(DOCROOT=tmpdir): self.searchbuilder.append_conf() + self.assertEqual(exc.exception.message_id, MkDocsYAMLParseError.EMPTY_CONFIG) @patch("readthedocs.projects.models.Project.checkout_path") def test_yaml_config_not_returns_dict(self, checkout_path): @@ -644,8 +646,10 @@ def test_yaml_config_not_returns_dict(self, checkout_path): python_env=python_env, ) - with self.assertRaisesMessage( - MkDocsYAMLParseError, MkDocsYAMLParseError.CONFIG_NOT_DICT - ): + with self.assertRaises(MkDocsYAMLParseError) as e: with override_settings(DOCROOT=tmpdir): self.searchbuilder.append_conf() + self.assertEqual( + e.exception.message_id, + MkDocsYAMLParseError.CONFIG_NOT_DICT, + ) diff --git a/readthedocs/rtd_tests/tests/test_imported_file.py b/readthedocs/rtd_tests/tests/test_imported_file.py index 7ea1b83ee0c..86b60c4907e 100644 --- a/readthedocs/rtd_tests/tests/test_imported_file.py +++ b/readthedocs/rtd_tests/tests/test_imported_file.py @@ -1,5 +1,6 @@ import os +import pytest from django.conf import settings from django.core.files.storage import get_storage_class from django.test import TestCase @@ -13,6 +14,7 @@ base_dir = os.path.dirname(os.path.dirname(__file__)) +@pytest.mark.search @override_settings(ELASTICSEARCH_DSL_AUTOSYNC=True) class ImportedFileTests(TestCase): fixtures = ["eric", "test_data"] diff --git a/readthedocs/rtd_tests/tests/test_notifications.py b/readthedocs/rtd_tests/tests/test_notifications.py index d6f648799a2..135a67c5000 100644 --- a/readthedocs/rtd_tests/tests/test_notifications.py +++ b/readthedocs/rtd_tests/tests/test_notifications.py @@ -4,52 +4,40 @@ 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.email 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("readthedocs.notifications.email.render_to_string") +@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" + user = fixture.get(User) build = fixture.get(Build) - req = mock.MagicMock() - notify = TestNotification(context_object=build, request=req) + notify = TestNotification(context_object=build, user=user) self.assertEqual( - notify.get_template_names("email"), + notify.get_template_names("html"), ["builds/notifications/foo_email.html"], ) self.assertEqual( - notify.get_template_names("site"), - ["builds/notifications/foo_site.html"], + notify.get_template_names("txt"), + ["builds/notifications/foo_email.txt"], ) self.assertEqual( notify.get_subject(), @@ -60,7 +48,6 @@ class TestNotification(Notification): { "foo": build, "production_uri": "https://readthedocs.org", - "request": req, # readthedocs_processor context "DASHBOARD_ANALYTICS_CODE": mock.ANY, "DO_NOT_TRACK_ENABLED": mock.ANY, @@ -76,32 +63,29 @@ class TestNotification(Notification): }, ) - notify.render("site") + notify.render("html") render_to_string.assert_has_calls( [ mock.call( context=mock.ANY, - template_name=["builds/notifications/foo_site.html"], + template_name=["builds/notifications/foo_email.html"], ), ] ) class NotificationBackendTests(TestCase): - @mock.patch("readthedocs.notifications.backends.send_email") + @mock.patch("readthedocs.notifications.email.send_email") def test_email_backend(self, send_email): - class TestNotification(Notification): + class TestNotification(EmailNotification): 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) + notify = TestNotification(context_object=build, user=user) + notify.send() send_email.assert_has_calls( [ @@ -114,155 +98,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/rtd_tests/tests/test_project.py b/readthedocs/rtd_tests/tests/test_project.py index 776dbf44908..30336ce2547 100644 --- a/readthedocs/rtd_tests/tests/test_project.py +++ b/readthedocs/rtd_tests/tests/test_project.py @@ -88,11 +88,12 @@ def test_multiple_conf_file_one_doc_in_path(self, find_method): def test_conf_file_not_found(self, find_method, full_find_method): find_method.return_value = [] full_find_method.return_value = [] - with self.assertRaisesMessage( - ProjectConfigurationError, - ProjectConfigurationError.NOT_FOUND, - ) as cm: + with self.assertRaises(ProjectConfigurationError) as e: self.pip.conf_file() + self.assertEqual( + e.exception.message_id, + ProjectConfigurationError.NOT_FOUND, + ) @patch('readthedocs.projects.models.Project.find') def test_multiple_conf_files(self, find_method): @@ -101,12 +102,14 @@ def test_multiple_conf_files(self, find_method): '/home/docs/rtfd/code/readthedocs.org/user_builds/pip/checkouts/multi-conf.py/src/sub/conf.py', '/home/docs/rtfd/code/readthedocs.org/user_builds/pip/checkouts/multi-conf.py/src/sub/src/conf.py', ] - with self.assertRaisesMessage( - ProjectConfigurationError, - ProjectConfigurationError.MULTIPLE_CONF_FILES, - ) as cm: + with self.assertRaises(ProjectConfigurationError) as e: self.pip.conf_file() + self.assertEqual( + e.exception.message_id, + ProjectConfigurationError.MULTIPLE_CONF_FILES, + ) + def test_get_storage_path(self): for type_ in MEDIA_TYPES: self.assertEqual( diff --git a/readthedocs/settings/base.py b/readthedocs/settings/base.py index 90bc763f4aa..24fb48d56b1 100644 --- a/readthedocs/settings/base.py +++ b/readthedocs/settings/base.py @@ -224,11 +224,11 @@ def INSTALLED_APPS(self): # noqa 'rest_framework', 'rest_framework.authtoken', "rest_framework_api_key", + "generic_relations", 'corsheaders', 'annoying', 'django_extensions', 'crispy_forms', - 'messages_extends', 'django_elasticsearch_dsl', 'django_filters', 'polymorphic', @@ -349,13 +349,6 @@ def MIDDLEWARE(self): }, ] - MESSAGE_STORAGE = 'readthedocs.notifications.storages.FallbackUniqueStorage' - - NOTIFICATION_BACKENDS = [ - 'readthedocs.notifications.backends.EmailBackend', - 'readthedocs.notifications.backends.SiteBackend', - ] - # Paths SITE_ROOT = os.path.dirname( os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) @@ -423,6 +416,7 @@ def TEMPLATES(self): 'django.template.context_processors.request', # Read the Docs processor 'readthedocs.core.context_processors.readthedocs_processor', + 'readthedocs.core.context_processors.user_notifications', ], }, }, @@ -486,11 +480,6 @@ def TEMPLATES(self): 'schedule': crontab(minute='*/15'), 'options': {'queue': 'web'}, }, - 'every-three-hour-clear-persistent-messages': { - 'task': 'readthedocs.core.tasks.clear_persistent_messages', - 'schedule': crontab(minute=0, hour='*/3'), - 'options': {'queue': 'web'}, - }, 'every-day-delete-old-search-queries': { 'task': 'readthedocs.search.tasks.delete_old_search_queries_from_db', 'schedule': crontab(minute=0, hour=0), diff --git a/readthedocs/subscriptions/notifications.py b/readthedocs/subscriptions/notifications.py index 0bbc52d538a..3ec27f094ff 100644 --- a/readthedocs/subscriptions/notifications.py +++ b/readthedocs/subscriptions/notifications.py @@ -1,12 +1,13 @@ """Organization level notifications.""" +import textwrap -from django.urls import reverse +from django.utils.translation import gettext_noop as _ 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.constants import INFO +from readthedocs.notifications.email import EmailNotification +from readthedocs.notifications.messages import Message, registry from readthedocs.organizations.models import Organization from readthedocs.subscriptions.constants import DISABLE_AFTER_DAYS @@ -16,16 +17,15 @@ class SubscriptionNotificationMixin: """Force to read templates from the subscriptions app.""" app_templates = "subscriptions" + context_object_name = "organization" -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(): @@ -36,17 +36,17 @@ 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. @@ -56,12 +56,12 @@ 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. @@ -72,9 +72,7 @@ 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 @@ -87,20 +85,19 @@ 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 +MESSAGE_ORGANIZATION_DISABLED = "organization:disabled" +messages = [ + Message( + id=MESSAGE_ORGANIZATION_DISABLED, + header=_("Your organization has been disabled"), + body=_( + textwrap.dedent( + """ + The organization "{instance.name}" is currently disabled. You need to renew your subscription to keep using Read the Docs + """ + ).strip(), + ), + type=INFO, + ), +] +registry.add(messages) diff --git a/readthedocs/subscriptions/tests/test_tasks.py b/readthedocs/subscriptions/tests/test_tasks.py index 13c71050f19..102a74be035 100644 --- a/readthedocs/subscriptions/tests/test_tasks.py +++ b/readthedocs/subscriptions/tests/test_tasks.py @@ -20,10 +20,9 @@ RTD_ALLOW_ORGANIZATIONS=True, RTD_ORG_DEFAULT_STRIPE_SUBSCRIPTION_PRICE="trialing", ) -@mock.patch("readthedocs.notifications.backends.send_email") -@mock.patch("readthedocs.notifications.storages.FallbackUniqueStorage") +@mock.patch("readthedocs.notifications.email.send_email") class DailyEmailTests(TestCase): - def test_trial_ending(self, mock_storage_class, mock_send_email): + def test_trial_ending(self, mock_send_email): """Trial ending daily email.""" now = timezone.now() @@ -54,22 +53,8 @@ def test_trial_ending(self, mock_storage_class, mock_send_email): created=now - timedelta(days=25), ) - mock_storage = mock.Mock() - mock_storage_class.return_value = mock_storage - daily_email() - self.assertEqual(mock_storage.add.call_count, 1) - mock_storage.add.assert_has_calls( - [ - mock.call( - message=mock.ANY, - extra_tags="", - level=31, - user=owner1, - ), - ] - ) self.assertEqual(mock_send_email.call_count, 1) mock_send_email.assert_has_calls( [ @@ -83,9 +68,7 @@ def test_trial_ending(self, mock_storage_class, mock_send_email): ] ) - def test_organizations_to_be_disable_email( - self, mock_storage_class, mock_send_email - ): + def test_organizations_to_be_disabled_email(self, mock_send_email): """Subscription ended ``DISABLE_AFTER_DAYS`` days ago daily email.""" customer1 = get(djstripe.Customer) latest_invoice1 = get( @@ -127,22 +110,8 @@ def test_organizations_to_be_disable_email( stripe_subscription=sub2, ) - mock_storage = mock.Mock() - mock_storage_class.return_value = mock_storage - daily_email() - self.assertEqual(mock_storage.add.call_count, 1) - mock_storage.add.assert_has_calls( - [ - mock.call( - message=mock.ANY, - extra_tags="", - level=31, - user=owner1, - ), - ] - ) self.assertEqual(mock_send_email.call_count, 1) mock_send_email.assert_has_calls( [ diff --git a/readthedocs/templates/base.html b/readthedocs/templates/base.html index 06a6700694d..7bd99f15458 100644 --- a/readthedocs/templates/base.html +++ b/readthedocs/templates/base.html @@ -103,8 +103,24 @@
+ {% comment %} + ``user_notifications`` comes from a Context Processor. + We need to use a CustomUser to have access to "user.notifications" + See https://docs.djangoproject.com/en/4.2/ref/settings/#std-setting-AUTH_USER_MODEL + {% endcomment %} + {% if user_notifications %} +
    + {% for notification in user_notifications %} +
  • + {{ notification.get_message.get_rendered_body|safe }} +
    • + {% endfor %} +
+ {% endif %} + {% block notify %} + {# TODO: migrate these notifications to new system #} {% if messages %}
    {% for message in messages %} diff --git a/readthedocs/templates/builds/build_detail.html b/readthedocs/templates/builds/build_detail.html index 540c8806e54..0f35005e633 100644 --- a/readthedocs/templates/builds/build_detail.html +++ b/readthedocs/templates/builds/build_detail.html @@ -150,6 +150,7 @@
{% if request.user|is_admin:project %} + {# TODO: migrate these "build ideas" to the new notification system #} {% if not build.success and "setup.py install" in build.commands.last.output %}

@@ -161,45 +162,22 @@

{% endif %} + {% endif %} - {# 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 %} -
-

- {% blocktrans trimmed with config_file_link="https://blog.readthedocs.com/use-build-os-config/" %} - Your builds will stop working soon!
- "build.image" config key is deprecated and it will be removed soon. - Read our blog post to know how to migrate to new key "build.os" - and ensure your project continues building successfully. - {% endblocktrans %} -

-
- {% endif %} - {% if build.finished and build.deprecated_config_used %} -
-

- {% blocktrans trimmed with config_file_link="https://blog.readthedocs.com/migrate-configuration-v2/" %} - Your builds will stop working soon!
- Configuration files will soon be required by projects, and will no longer be optional. - Read our blog post to create one - and ensure your project continues building successfully. - {% endblocktrans %} -

-
- {% endif %} - {% endif %} + {% comment %} + Show non-error notifications as "build-ideas" (e.g. "build.commands is in beta") - {% 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 %} -

-
+ This temporal until we render these notifications from the front-end. + {% endcomment %} + {% 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 #} @@ -233,6 +211,30 @@

{% trans "Build Errors" %}

{% else %} {# Show new command output if lacking build.output #} + {% comment %} + Show error notifications as "build-error" (e.g. "syntax error in YAML") + + This temporal until we render these notifications from the front-end. + {% endcomment %} + {% 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 %} + + {% comment %} + After deploying the new notification system (`Notification` model), + we won't be populating `Build.error` field anymore and these errors won't be shown anymore. + However, old builds will still need to render the `Build.error` message + and that's why we are keeping this HTML + KO code here. + + In the future, we can decide fully migrate `Build.error` -> `Notification` and remove this + block completely. Read more at + https://github.com/readthedocs/readthedocs.org/issues/10980#issuecomment-1876785304 + {% endcomment %}