From de42c5e9beb5d65e1ab1eb36c703b3cf18c58cbf Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 10 Jan 2024 16:50:39 +0100 Subject: [PATCH 1/3] Notifications: use `Template`'s Django engine to render them We dealt a few times when rendering notifications using simple Python's `.format()` and we changed our mind to use Django's engine instead. Closes #11022 --- readthedocs/config/notifications.py | 54 +++++++++---------- readthedocs/core/notifications.py | 2 +- readthedocs/domains/notifications.py | 12 ++--- readthedocs/notifications/messages.py | 61 ++++++---------------- readthedocs/oauth/notifications.py | 24 ++++----- readthedocs/projects/notifications.py | 6 +-- readthedocs/subscriptions/notifications.py | 2 +- 7 files changed, 67 insertions(+), 94 deletions(-) diff --git a/readthedocs/config/notifications.py b/readthedocs/config/notifications.py index 00463818e1f..5084a6bf86d 100644 --- a/readthedocs/config/notifications.py +++ b/readthedocs/config/notifications.py @@ -41,7 +41,7 @@ body=_( textwrap.dedent( """ - Configuration file not found in {directory}. + Configuration file not found in {{directory}}. """ ).strip(), ), @@ -53,7 +53,7 @@ body=_( textwrap.dedent( """ - The {key} configuration option is not supported in this version. + The {{key}} configuration option is not supported in this version. """ ).strip(), ), @@ -105,11 +105,11 @@ body=_( textwrap.dedent( """ - Invalid configuration option: {key}. + Invalid configuration option: {{key}}. - Read the Docs configuration file: {source_file}. + Read the Docs configuration file: {{source_file}}. - {error_message} + {{error_message}} """ ).strip(), ), @@ -145,7 +145,7 @@ body=_( textwrap.dedent( """ - The name of the packages (e.g. {package}) can't start with {prefix} + The name of the packages (e.g. {{package}}) can't start with {{prefix}} """ ).strip(), ), @@ -157,7 +157,7 @@ body=_( textwrap.dedent( """ - The name of the package {pacakge} is invalid. + The name of the package {{pacakge}} is invalid. """ ).strip(), ), @@ -213,11 +213,11 @@ ), Message( id=ConfigError.INVALID_KEY_NAME, - header=_("Invalid configuration key: {key}"), + header=_("Invalid configuration key: {{key}}"), body=_( textwrap.dedent( """ - Make sure the key name {key} is correct. + Make sure the key name {{key}} is correct. """ ).strip(), ), @@ -229,9 +229,9 @@ body=_( textwrap.dedent( """ - Error while parsing {filename}. + Error while parsing {{filename}}. - {error_message} + {{error_message}} """ ).strip(), ), @@ -248,8 +248,8 @@ body=_( textwrap.dedent( """ - Config validation error in {key}. - Expected one of (0, 1, true, false), got {value}. + Config validation error in {{key}}. + Expected one of (0, 1, true, false), got {{value}}. """ ).strip(), ), @@ -261,8 +261,8 @@ body=_( textwrap.dedent( """ - Config validation error in {key}. - Expected one of ({choices}), got {value}. + Config validation error in {{key}}. + Expected one of ({{choices}}), got {{value}}. """ ).strip(), ), @@ -274,8 +274,8 @@ body=_( textwrap.dedent( """ - Config validation error in {key}. - Expected a dictionary, got {value}. + Config validation error in {{key}}. + Expected a dictionary, got {{value}}. """ ).strip(), ), @@ -287,8 +287,8 @@ body=_( textwrap.dedent( """ - Config validation error in {key}. - The path {value} does not exist. + Config validation error in {{key}}. + The path {{value}} does not exist. """ ).strip(), ), @@ -300,8 +300,8 @@ body=_( textwrap.dedent( """ - Config validation error in {key}. - The path {value} is not a valid path pattern. + Config validation error in {{key}}. + The path {{value}} is not a valid path pattern. """ ).strip(), ), @@ -313,8 +313,8 @@ body=_( textwrap.dedent( """ - Config validation error in {key}. - Expected a string, got {value}. + Config validation error in {{key}}. + Expected a string, got {{value}}. """ ).strip(), ), @@ -326,8 +326,8 @@ body=_( textwrap.dedent( """ - Config validation error in {key}. - Expected a list, got {value}. + Config validation error in {{key}}. + Expected a list, got {{value}}. """ ).strip(), ), @@ -339,8 +339,8 @@ body=_( textwrap.dedent( """ - Config validation error in {key}. - Value {value} not found. + Config validation error in {{key}}. + Value {{value}} not found. """ ).strip(), ), diff --git a/readthedocs/core/notifications.py b/readthedocs/core/notifications.py index f38e37264c5..b94756b2488 100644 --- a/readthedocs/core/notifications.py +++ b/readthedocs/core/notifications.py @@ -16,7 +16,7 @@ textwrap.dedent( """ Your primary email address is not verified. - Please verify it here. + Please verify it here. """ ).strip(), ), diff --git a/readthedocs/domains/notifications.py b/readthedocs/domains/notifications.py index 57a3694e9d6..e2baddc0d3a 100644 --- a/readthedocs/domains/notifications.py +++ b/readthedocs/domains/notifications.py @@ -20,12 +20,12 @@ class PendingCustomDomainValidation(EmailNotification): messages = [ Message( id=MESSAGE_DOMAIN_VALIDATION_PENDING, - header=_("Pending configuration of custom domain: {domain}"), + 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. + The configuration of your custom domain {{domain}} is pending. + Go to the domain page and follow the instructions to complete it. """ ).strip(), ), @@ -36,12 +36,12 @@ class PendingCustomDomainValidation(EmailNotification): # ``message_id`` Message( id=MESSAGE_DOMAIN_VALIDATION_EXPIRED, - header=_("Validation of custom domain expired: {domain}"), + 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. + The validation period of your custom domain {{domain}} has ended. + Go to the domain page and click on "Save" to restart the process. """ ).strip(), ), diff --git a/readthedocs/notifications/messages.py b/readthedocs/notifications/messages.py index 6fef2648e9f..4684f566ae1 100644 --- a/readthedocs/notifications/messages.py +++ b/readthedocs/notifications/messages.py @@ -1,8 +1,7 @@ import textwrap -from collections import defaultdict import structlog -from django.utils.html import escape +from django.template import Context, Template from django.utils.translation import gettext_noop as _ from readthedocs.doc_builder.exceptions import ( @@ -34,26 +33,8 @@ def __repr__(self): def __str__(self): return f"Message: {self.id} | {self.header}" - def _escape_format_values(self, format_values): - """ - Escape all potential HTML tags included in format values. - - This is a protection against rendering potential values defined by the user. - It uses the Django's util function ``escape`` (similar to ``|escape`` template tag filter) - to convert HTML characters into regular characters. - - NOTE: currently, we don't support values that are not ``str`` or ``int``. - If we want to support other types or nested dictionaries, - we will need to iterate recursively to apply the ``escape`` function. - """ - return { - key: escape(value) - for key, value in format_values.items() - if isinstance(value, (str, int)) - } - def set_format_values(self, format_values): - self.format_values = self._escape_format_values(format_values) + self.format_values = format_values def get_display_icon_classes(self): if self.icon_classes: @@ -78,20 +59,12 @@ def get_display_icon_classes(self): return " ".join(classes) def get_rendered_header(self): - try: - return self.header.format(**self.format_values) - except KeyError: - # There was a key missing - log.exception("There was a missing key when formating a header's Message.") - return self.header.format_map(defaultdict(str, **self.format_values)) + template = Template(self.header) + return template.render(context=Context(self.format_values)) def get_rendered_body(self): - try: - return self.body.format(**self.format_values) - except KeyError: - # There was a key missing - log.exception("There was a missing key when formating a body's Message.") - return self.body.format_map(defaultdict(str, **self.format_values)) + template = Template(self.body) + return template.render(context=Context(self.format_values)) # TODO: review the copy of these notifications/messages on PR review and adapt them. @@ -109,7 +82,7 @@ def get_rendered_body(self): 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}). + report this error to us with your build id ({{instance.pk}}). """ ).strip(), ), @@ -123,7 +96,7 @@ def get_rendered_body(self): """ 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}). + file a support request and reference this build id ({{instance.pk}}). """ ).strip(), ), @@ -149,7 +122,7 @@ def get_rendered_body(self): body=_( textwrap.dedent( """ - Concurrency limit reached ({limit}), retrying in 5 minutes. + Concurrency limit reached ({{limit}}), retrying in 5 minutes. """ ).strip(), ), @@ -210,7 +183,7 @@ def get_rendered_body(self): body=_( textwrap.dedent( """ - Build exited due to unknown error: {message} + Build exited due to unknown error: {{message}} """ ).strip(), ), @@ -236,7 +209,7 @@ def get_rendered_body(self): body=_( textwrap.dedent( """ - Your project, organization, or user is currently building the maximum concurrency builds allowed ({limit}). + Your project, organization, or user is currently building the maximum concurrency builds allowed ({{limit}}). It will automatically retry in 5 minutes. """ ).strip(), @@ -261,7 +234,7 @@ def get_rendered_body(self): body=_( textwrap.dedent( """ - Build output directory for format "{artifact_type}" is not a directory. + Build output directory for format "{{artifact_type}}" is not a directory. """ ).strip(), ), @@ -273,7 +246,7 @@ def get_rendered_body(self): body=_( textwrap.dedent( """ - Build output directory for format "{artifact_type}" does not contain any files. + 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(), @@ -286,9 +259,9 @@ def get_rendered_body(self): body=_( textwrap.dedent( """ - Build output directory for format "{artifact_type}" contains multiple files + 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. + Please, remove all the files but the "{{artifact_type}}" you want to upload. """ ).strip(), ), @@ -421,7 +394,7 @@ def get_rendered_body(self): body=_( textwrap.dedent( """ - Problem parsing MkDocs YAML configuration. {exception} + Problem parsing MkDocs YAML configuration. {{exception}} """ ).strip(), ), @@ -457,7 +430,7 @@ def get_rendered_body(self): body=_( textwrap.dedent( """ - The "{config}" config from your MkDocs YAML config file has to be a + The "{{config}}" config from your MkDocs YAML config file has to be a list of relative paths. """ ).strip(), diff --git a/readthedocs/oauth/notifications.py b/readthedocs/oauth/notifications.py index 7e99fa540e8..f1213e81235 100644 --- a/readthedocs/oauth/notifications.py +++ b/readthedocs/oauth/notifications.py @@ -23,8 +23,8 @@ body=_( textwrap.dedent( """ - Could not add webhook for "{instance.slug}". - Please connect your {provider_name} account. + Could not add webhook for "{{instance.slug}}". + Please connect your {{provider_name}} account. """ ).strip(), ), @@ -36,8 +36,8 @@ body=_( textwrap.dedent( """ - Could not add webhook for "{instance.slug}". - Make sure you have the correct {provider_name} permissions. + Could not add webhook for "{{instance.slug}}". + Make sure you have the correct {{provider_name}} permissions. """ ).strip(), ), @@ -49,9 +49,9 @@ body=_( textwrap.dedent( """ - The project "{instance.slug}" doesn't have a valid webhook set up, + The project "{{instance.slug}}" doesn't have a valid webhook set up, commits won't trigger new builds for this project. - See the project integrations for more information. + See the project integrations for more information. """ ).strip(), ), @@ -59,13 +59,13 @@ ), Message( id=MESSAGE_OAUTH_BUILD_STATUS_FAILURE, - header=_("{provier_name} build status report failed"), + header=_("{{provier_name}} build status report failed"), body=_( textwrap.dedent( """ - Could not send {provider_name} build status report for "{instance.slug}". - Make sure you have the correct {provider_name} repository permissions and - your {provider_name} account + Could not send {{provider_name}} build status report for "{{instance.slug}}". + Make sure you have the correct {{provider_name}} repository permissions and + your {{provider_name}} account is connected to Read the Docs. """ ).strip(), @@ -78,7 +78,7 @@ body=_( textwrap.dedent( """ - Successfully added deploy key to {provider_name} project. + Successfully added deploy key to {{provider_name}} project. """ ).strip(), ), @@ -90,7 +90,7 @@ body=_( textwrap.dedent( """ - Failed to add deploy key to {provider_name} project, ensure you have the correct permissions and try importing again. + Failed to add deploy key to {{provider_name}} project, ensure you have the correct permissions and try importing again. """ ).strip(), ), diff --git a/readthedocs/projects/notifications.py b/readthedocs/projects/notifications.py index d8973e5b6e7..2d60c3c45b6 100644 --- a/readthedocs/projects/notifications.py +++ b/readthedocs/projects/notifications.py @@ -21,7 +21,7 @@ """ 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. + Please contact support to get your project re-enabled. """ ).strip(), ), @@ -59,7 +59,7 @@ body=_( textwrap.dedent( """ - Failed to checkout revision: {revision} + Failed to checkout revision: {{revision}} """ ).strip(), ), @@ -125,7 +125,7 @@ body=_( textwrap.dedent( """ - The file {filename} doesn't exist. Make sure it's a valid file path. + The file {{filename}} doesn't exist. Make sure it's a valid file path. """ ).strip(), ), diff --git a/readthedocs/subscriptions/notifications.py b/readthedocs/subscriptions/notifications.py index 518dc27d3f7..ea4281962eb 100644 --- a/readthedocs/subscriptions/notifications.py +++ b/readthedocs/subscriptions/notifications.py @@ -93,7 +93,7 @@ def for_organizations(cls): body=_( textwrap.dedent( """ - The organization "{instance.slug}" is currently disabled. You need to renew your subscription to keep using Read the Docs + The organization "{{instance.slug}}" is currently disabled. You need to renew your subscription to keep using Read the Docs """ ).strip(), ), From 0a0cf3cb89ca8f7f20887b8c65d7b8258e8c5f1e Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 10 Jan 2024 16:53:40 +0100 Subject: [PATCH 2/3] Revert #11018 and use `instance.name` again --- readthedocs/oauth/notifications.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/readthedocs/oauth/notifications.py b/readthedocs/oauth/notifications.py index f1213e81235..1bb20966d33 100644 --- a/readthedocs/oauth/notifications.py +++ b/readthedocs/oauth/notifications.py @@ -23,7 +23,7 @@ body=_( textwrap.dedent( """ - Could not add webhook for "{{instance.slug}}". + Could not add webhook for "{{instance.name}}". Please connect your {{provider_name}} account. """ ).strip(), @@ -36,7 +36,7 @@ body=_( textwrap.dedent( """ - Could not add webhook for "{{instance.slug}}". + Could not add webhook for "{{instance.name}}". Make sure you have the correct {{provider_name}} permissions. """ ).strip(), @@ -49,7 +49,7 @@ body=_( textwrap.dedent( """ - The project "{{instance.slug}}" doesn't have a valid webhook set up, + 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. """ @@ -63,7 +63,7 @@ body=_( textwrap.dedent( """ - Could not send {{provider_name}} build status report for "{{instance.slug}}". + 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. From 1661cc83e3d8715c8530d96b4485d133c187c3a4 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 10 Jan 2024 17:12:55 +0100 Subject: [PATCH 3/3] Update tests --- .../notifications/tests/test_messages.py | 27 +++---------------- 1 file changed, 4 insertions(+), 23 deletions(-) diff --git a/readthedocs/notifications/tests/test_messages.py b/readthedocs/notifications/tests/test_messages.py index edc7f9dd250..96a4f614db6 100644 --- a/readthedocs/notifications/tests/test_messages.py +++ b/readthedocs/notifications/tests/test_messages.py @@ -6,8 +6,8 @@ class TestMessage: def test_xss_protection(self): message = Message( id="test", - header="XSS: {header}", - body="XSS: {body}", + header="XSS: {{header}}", + body="XSS: {{body}}", type=INFO, ) message.set_format_values( @@ -20,30 +20,11 @@ def test_xss_protection(self): assert message.get_rendered_header() == "XSS: <p>xss</p>" assert message.get_rendered_body() == "XSS: <span>xss</span>" - def test_invalid_format_values_type(self): - message = Message( - id="test", - header="Header: {dict}", - body="Body: {dict}", - type=INFO, - ) - message.set_format_values( - { - "dict": { - "key": "value", - }, - } - ) - - # The rendered version skips the ``dict`` because it's not supported - assert message.get_rendered_header() == "Header: " - assert message.get_rendered_body() == "Body: " - def test_missing_key_format_values(self): message = Message( id="test", - header="Missing format value: {header}", - body="Missing format value: {body}", + header="Missing format value: {{header}}", + body="Missing format value: {{body}}", type=INFO, )