From c036e632915f8a660f3455265c6ac8459ff316a2 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 31 Oct 2023 16:25:13 -0500 Subject: [PATCH 01/25] Redirects: improvements from design doc Implements https://dev.readthedocs.io/en/latest/design/redirects.html --- readthedocs/api/v3/serializers.py | 6 + readthedocs/projects/forms.py | 15 +- readthedocs/proxito/views/mixins.py | 6 +- readthedocs/proxito/views/serve.py | 82 ++++----- .../migrations/0006_add_new_fields.py | 64 ++++++++ readthedocs/redirects/models.py | 155 ++++++++++-------- readthedocs/redirects/querysets.py | 92 +++++++---- 7 files changed, 274 insertions(+), 146 deletions(-) create mode 100644 readthedocs/redirects/migrations/0006_add_new_fields.py diff --git a/readthedocs/api/v3/serializers.py b/readthedocs/api/v3/serializers.py index dc99707327c..04a13cb5db3 100644 --- a/readthedocs/api/v3/serializers.py +++ b/readthedocs/api/v3/serializers.py @@ -847,8 +847,14 @@ class Meta: "type", "from_url", "to_url", + "force", + "enabled", + "description", + "http_status", "_links", ] + # TODO: allow editing this field for projects that have this feature enabled. + read_only_fields = ["force"] class RedirectCreateSerializer(RedirectSerializerBase): diff --git a/readthedocs/projects/forms.py b/readthedocs/projects/forms.py index fc812c54a7b..2f427603eb5 100644 --- a/readthedocs/projects/forms.py +++ b/readthedocs/projects/forms.py @@ -740,12 +740,25 @@ class RedirectForm(forms.ModelForm): class Meta: model = Redirect - fields = ["project", "redirect_type", "from_url", "to_url", "force"] + fields = [ + "project", + "redirect_type", + "from_url", + "to_url", + "http_status", + "force", + "enabled", + "description", + ] def __init__(self, *args, **kwargs): self.project = kwargs.pop('project', None) super().__init__(*args, **kwargs) + # Remove the nullable option from the form. + self.fields["enabled"].widget = forms.CheckboxInput() + self.fields["enabled"].empty_value = False + if self.project.has_feature(Feature.ALLOW_FORCED_REDIRECTS): # Remove the nullable option from the form. # TODO: remove after migration. diff --git a/readthedocs/proxito/views/mixins.py b/readthedocs/proxito/views/mixins.py index 9d1bf5fc001..058c2b33f98 100644 --- a/readthedocs/proxito/views/mixins.py +++ b/readthedocs/proxito/views/mixins.py @@ -340,7 +340,7 @@ def system_redirect( return resp def get_redirect( - self, project, lang_slug, version_slug, filename, full_path, forced_only=False + self, project, lang_slug, version_slug, filename, path, forced_only=False ): """ Check for a redirect for this project that matches ``full_path``. @@ -351,8 +351,8 @@ def get_redirect( redirect_path, http_status = project.redirects.get_redirect_path_with_status( language=lang_slug, version_slug=version_slug, - path=filename, - full_path=full_path, + filename=filename, + path=path, forced_only=forced_only, ) return redirect_path, http_status diff --git a/readthedocs/proxito/views/serve.py b/readthedocs/proxito/views/serve.py index a6c60a823f6..e9cb7e9558b 100644 --- a/readthedocs/proxito/views/serve.py +++ b/readthedocs/proxito/views/serve.py @@ -331,27 +331,28 @@ def serve_path(self, request, path): is_external_version=unresolved_domain.is_from_external_domain, ) - # Check for forced redirects. - redirect_path, http_status = self.get_redirect( - project=project, - lang_slug=project.language, - version_slug=version.slug, - filename=filename, - full_path=request.path, - forced_only=True, - ) - if redirect_path and http_status: - log.bind(forced_redirect=True) - try: - return self.get_redirect_response( - request=request, - redirect_path=redirect_path, - proxito_path=request.path, - http_status=http_status, - ) - except InfiniteRedirectException: - # Continue with our normal serve. - pass + # Check for forced redirects on non-external domains only. + if not unresolved_domain.is_from_external_domain: + redirect_path, http_status = self.get_redirect( + project=project, + lang_slug=project.language, + version_slug=version.slug, + filename=filename, + path=request.path, + forced_only=True, + ) + if redirect_path and http_status: + log.bind(forced_redirect=True) + try: + return self.get_redirect_response( + request=request, + redirect_path=redirect_path, + proxito_path=request.path, + http_status=http_status, + ) + except InfiniteRedirectException: + # Continue with our normal serve. + pass # Check user permissions and return an unauthed response if needed. if not self.allowed_user(request, version): @@ -479,27 +480,28 @@ def get(self, request, proxito_path): if response: return response - # Check and perform redirects on 404 handler - # NOTE: this redirect check must be done after trying files like + # Check and perform redirects on 404 handler for non-external domains only. + # NOTE: This redirect check must be done after trying files like # ``index.html`` and ``README.html`` to emulate the behavior we had when # serving directly from NGINX without passing through Python. - redirect_path, http_status = self.get_redirect( - project=project, - lang_slug=lang_slug, - version_slug=version_slug, - filename=filename, - full_path=proxito_path, - ) - if redirect_path and http_status: - try: - return self.get_redirect_response( - request, redirect_path, proxito_path, http_status - ) - except InfiniteRedirectException: - # ``get_redirect_response`` raises this when it's redirecting back to itself. - # We can safely ignore it here because it's logged in ``canonical_redirect``, - # and we don't want to issue infinite redirects. - pass + if not unresolved_domain.is_from_external_domain: + redirect_path, http_status = self.get_redirect( + project=project, + lang_slug=lang_slug, + version_slug=version_slug, + filename=filename, + path=proxito_path, + ) + if redirect_path and http_status: + try: + return self.get_redirect_response( + request, redirect_path, proxito_path, http_status + ) + except InfiniteRedirectException: + # ``get_redirect_response`` raises this when it's redirecting back to itself. + # We can safely ignore it here because it's logged in ``canonical_redirect``, + # and we don't want to issue infinite redirects. + pass # Register 404 pages into our database for user's analytics self._register_broken_link( diff --git a/readthedocs/redirects/migrations/0006_add_new_fields.py b/readthedocs/redirects/migrations/0006_add_new_fields.py new file mode 100644 index 00000000000..92c90d4f477 --- /dev/null +++ b/readthedocs/redirects/migrations/0006_add_new_fields.py @@ -0,0 +1,64 @@ +# Generated by Django 4.2.5 on 2023-10-31 17:57 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("redirects", "0005_allow_to_force_redirects"), + ] + + operations = [ + migrations.AddField( + model_name="redirect", + name="description", + field=models.CharField( + blank=True, max_length=255, verbose_name="Description", null=True + ), + ), + migrations.AddField( + model_name="redirect", + name="enabled", + field=models.BooleanField( + default=True, + help_text="Enable or disable the redirect.", + verbose_name="Enabled", + null=True, + ), + ), + migrations.AlterField( + model_name="redirect", + name="http_status", + field=models.SmallIntegerField( + choices=[ + (302, "302 - Temporary Redirect"), + (301, "301 - Permanent Redirect"), + ], + default=302, + verbose_name="HTTP status code", + ), + ), + migrations.AlterField( + model_name="redirect", + name="status", + field=models.BooleanField( + choices=[(True, "Active"), (False, "Inactive")], default=True, null=True + ), + ), + migrations.AlterField( + model_name="redirect", + name="redirect_type", + field=models.CharField( + choices=[ + ("prefix", "Prefix Redirect"), + ("page", "Page Redirect"), + ("exact", "Exact Redirect"), + ("clean_url_to_html", "Clean URL to HTML (file/ to file.html)"), + ("html_to_clean_url", "HTML to clean URL (file.html to file/)"), + ], + help_text="The type of redirect you wish to use.", + max_length=255, + verbose_name="Redirect Type", + ), + ), + ] diff --git a/readthedocs/redirects/models.py b/readthedocs/redirects/models.py index 58ecbef039c..048e728f21c 100644 --- a/readthedocs/redirects/models.py +++ b/readthedocs/redirects/models.py @@ -15,8 +15,8 @@ log = structlog.get_logger(__name__) HTTP_STATUS_CHOICES = ( - (301, _("301 - Permanent Redirect")), (302, _("302 - Temporary Redirect")), + (301, _("301 - Permanent Redirect")), ) STATUS_CHOICES = ( @@ -24,19 +24,22 @@ (False, _("Inactive")), ) +PAGE_REDIRECT = "page" +EXACT_REDIRECT = "exact" +CLEAN_URL_TO_HTML_REDIRECT = "clean_url_to_html" +HTML_TO_CLEAN_URL_REDIRECT = "html_to_clean_url" + TYPE_CHOICES = ( - ("prefix", _("Prefix Redirect")), - ("page", _("Page Redirect")), - ("exact", _("Exact Redirect")), - ("sphinx_html", _("Sphinx HTMLDir -> HTML")), - ("sphinx_htmldir", _("Sphinx HTML -> HTMLDir")), - # ('advanced', _('Advanced')), + (PAGE_REDIRECT, _("Page Redirect")), + (EXACT_REDIRECT, _("Exact Redirect")), + (CLEAN_URL_TO_HTML_REDIRECT, _("Clean URL to HTML (file/ to file.html)")), + (HTML_TO_CLEAN_URL_REDIRECT, _("HTML to clean URL (file.html to file/)")), ) # FIXME: this help_text message should be dynamic since "Absolute path" doesn't # make sense for "Prefix Redirects" since the from URL is considered after the -# ``/$lang/$version/`` part. Also, there is a feature for the "Exact -# Redirects" that should be mentioned here: the usage of ``$rest`` +# ``/$lang/$version/`` part. Also, there is a feature for the "Exact Redirects" +# that should be mentioned here: the usage of ``*``. from_url_helptext = _( "Absolute path, excluding the domain. " "Example: /docs/ or /install.html", @@ -73,8 +76,7 @@ class Redirect(models.Model): blank=True, ) - # We are denormalizing the database here to easily query for Exact Redirects - # with ``$rest`` on them from El Proxito + # Store the from_url without the ``*`` wildcard in it for easier and faster querying. from_url_without_rest = models.CharField( max_length=255, db_index=True, @@ -90,6 +92,7 @@ class Redirect(models.Model): help_text=to_url_helptext, blank=True, ) + force = models.BooleanField( _("Force redirect"), null=True, @@ -98,11 +101,27 @@ class Redirect(models.Model): ) http_status = models.SmallIntegerField( - _("HTTP Status"), + _("HTTP status code"), choices=HTTP_STATUS_CHOICES, default=302, ) - status = models.BooleanField(choices=STATUS_CHOICES, default=True) + + enabled = models.BooleanField( + _("Enabled"), + default=True, + null=True, + help_text=_("Enable or disable the redirect."), + ) + + description = models.CharField( + _("Description"), + blank=True, + null=True, + max_length=255, + ) + + # TODO: remove this field and use `enabled` instead. + status = models.BooleanField(choices=STATUS_CHOICES, default=True, null=True) create_dt = models.DateTimeField(auto_now_add=True) update_dt = models.DateTimeField(auto_now=True) @@ -115,10 +134,21 @@ class Meta: ordering = ("-update_dt",) def save(self, *args, **kwargs): - if self.redirect_type == "exact" and "$rest" in self.from_url: - self.from_url_without_rest = self.from_url.replace("$rest", "") + self.from_url = self.normalize_path(self.from_url) + self.from_url_without_rest = None + if self.redirect_type in [ + PAGE_REDIRECT, + EXACT_REDIRECT, + ] and self.from_url.endswith("*"): + self.from_url_without_rest = self.from_url.removesuffix("*") super().save(*args, **kwargs) + def normalize_path(self, path): + """Normalize a path to be used for matching.""" + path = "/" + path.lstrip("/") + path = path.rstrip("/") + return path + def __str__(self): redirect_text = "{type}: {from_to_url}" if self.redirect_type in ["prefix", "page", "exact"]: @@ -167,7 +197,7 @@ def get_full_path( filename=filename, ) - def get_redirect_path(self, path, full_path=None, language=None, version_slug=None): + def get_redirect_path(self, filename, path=None, language=None, version_slug=None): method = getattr( self, "redirect_{type}".format( @@ -175,51 +205,42 @@ def get_redirect_path(self, path, full_path=None, language=None, version_slug=No ), ) return method( - path, full_path=full_path, language=language, version_slug=version_slug + filename=filename, path=path, language=language, version_slug=version_slug ) - def redirect_prefix(self, path, full_path, language=None, version_slug=None): - if path.startswith(self.from_url): - log.debug("Redirecting...", redirect=self) - # pep8 and blank don't agree on having a space before :. - cut_path = path[len(self.from_url) :] # noqa - - to = self.get_full_path( - filename=cut_path, - language=language, - version_slug=version_slug, - allow_crossdomain=False, - ) - return to + def redirect_page(self, filename, path, language=None, version_slug=None): + log.debug("Redirecting...", redirect=self) + to = self.to_url + if self.from_url.endswith("*"): + splat = filename[len(self.from_url_without_rest) - 1 :] + to = to.replace(":splat", splat) + return self.get_full_path( + filename=to, + language=language, + version_slug=version_slug, + allow_crossdomain=True, + ) - def redirect_page(self, path, full_path, language=None, version_slug=None): - if path == self.from_url: - log.debug("Redirecting...", redirect=self) - to = self.get_full_path( - filename=self.to_url.lstrip("/"), - language=language, - version_slug=version_slug, - allow_crossdomain=True, - ) + def redirect_exact(self, filename, path, language=None, version_slug=None): + log.debug("Redirecting...", redirect=self) + if self.from_url.endswith("*"): + splat = path[len(self.from_url_without_rest) - 1 :] + to = self.to_url.replace(":splat", splat) return to + return self.to_url - def redirect_exact(self, path, full_path, language=None, version_slug=None): - if full_path == self.from_url: - log.debug("Redirecting...", redirect=self) - return self.to_url - # Handle full sub-level redirects - if "$rest" in self.from_url: - match = self.from_url.split("$rest", maxsplit=1)[0] - if full_path.startswith(match): - cut_path = full_path.replace(match, self.to_url, 1) - return cut_path - - def redirect_sphinx_html(self, path, full_path, language=None, version_slug=None): - for ending in ["/", "/index.html"]: - if path.endswith(ending): - log.debug("Redirecting...", redirect=self) - path = path[1:] # Strip leading slash. - to = re.sub(ending + "$", ".html", path) + def redirect_clean_url_to_html( + self, filename, path, language=None, version_slug=None + ): + log.debug("Redirecting...", redirect=self) + suffixes = ["/", "/index.html"] + for suffix in suffixes: + if filename.endswith(suffix): + to = filename[: -len(suffix)] + if not to: + to = "index.html" + else: + to += ".html" return self.get_full_path( filename=to, language=language, @@ -227,16 +248,14 @@ def redirect_sphinx_html(self, path, full_path, language=None, version_slug=None allow_crossdomain=False, ) - def redirect_sphinx_htmldir( - self, path, full_path, language=None, version_slug=None + def redirect_html_to_clean_url( + self, filename, path, language=None, version_slug=None ): - if path.endswith(".html"): - log.debug("Redirecting...", redirect=self) - path = path[1:] # Strip leading slash. - to = re.sub(".html$", "/", path) - return self.get_full_path( - filename=to, - language=language, - version_slug=version_slug, - allow_crossdomain=False, - ) + log.debug("Redirecting...", redirect=self) + to = filename.removesuffix(".html") + "/" + return self.get_full_path( + filename=to, + language=language, + version_slug=version_slug, + allow_crossdomain=False, + ) diff --git a/readthedocs/redirects/querysets.py b/readthedocs/redirects/querysets.py index 63b258214c7..88bb1d29dcd 100644 --- a/readthedocs/redirects/querysets.py +++ b/readthedocs/redirects/querysets.py @@ -6,6 +6,12 @@ from django.db.models import CharField, F, Q, Value from readthedocs.core.permissions import AdminPermission +from readthedocs.redirects.models import ( + CLEAN_URL_TO_HTML_REDIRECT, + EXACT_REDIRECT, + HTML_TO_CLEAN_URL_REDIRECT, + PAGE_REDIRECT, +) log = structlog.get_logger(__name__) @@ -34,7 +40,7 @@ def api(self, user=None): return queryset def get_redirect_path_with_status( - self, path, full_path=None, language=None, version_slug=None, forced_only=False + self, filename, path=None, language=None, version_slug=None, forced_only=False ): """ Get the final redirect with its status code. @@ -44,69 +50,87 @@ def get_redirect_path_with_status( :param forced_only: Include only forced redirects in the results. """ # Small optimization to skip executing the big query below. - if forced_only and not self.filter(force=True).exists(): + # TODO: use filter(enabled=True) once we have removed the null option from the field. + if forced_only and not self.filter(force=True).exclude(enabled=False).exists(): return None, None + normalized_filename = self._normalize_path(filename) normalized_path = self._normalize_path(path) - normalized_full_path = self._normalize_path(full_path) - # add extra fields with the ``path`` and ``full_path`` to perform a + # Useful to allow redirects to match paths with or without trailling slash. + # For example, ``/docs`` will match ``/docs/`` and ``/docs``. + filename_without_trailling_slash = normalized_filename.rstrip("/") + path_without_trailling_slash = normalized_path.rstrip("/") + + # Add extra fields with the ``filename`` and ``path`` to perform a # filter at db level instead with Python. queryset = self.annotate( + filename=Value( + filename, + output_field=CharField(), + ), path=Value( normalized_path, output_field=CharField(), ), - full_path=Value( - normalized_full_path, + filename_without_trailling_slash=Value( + filename_without_trailling_slash, + output_field=CharField(), + ), + path_without_trailling_slash=Value( + path_without_trailling_slash, output_field=CharField(), ), - ) - prefix = Q( - redirect_type="prefix", - full_path__startswith=F("from_url"), ) page = Q( - redirect_type="page", - path__exact=F("from_url"), + redirect_type=PAGE_REDIRECT, + filename_without_trailling_slash__exact=F("from_url"), ) exact = Q( - redirect_type="exact", - from_url__endswith="$rest", - full_path__startswith=F("from_url_without_rest"), + redirect_type=EXACT_REDIRECT, + from_url_without_rest__isnull=False, + path__startswith=F("from_url_without_rest"), ) | Q( - redirect_type="exact", - full_path__exact=F("from_url"), + redirect_type=EXACT_REDIRECT, + from_url_without_rest__isnull=True, + path_without_trailling_slash__exact=F("from_url"), ) - sphinx_html = Q( - redirect_type="sphinx_html", - path__endswith="/", + clean_url_to_html = Q( + redirect_type=CLEAN_URL_TO_HTML_REDIRECT, + filename__endswith="/", ) | Q( - redirect_type="sphinx_html", - path__endswith="/index.html", + redirect_type=CLEAN_URL_TO_HTML_REDIRECT, + filename__endswith="/index.html", ) - sphinx_htmldir = Q( - redirect_type="sphinx_htmldir", - path__endswith=".html", + html_to_clean_url = Q( + redirect_type=HTML_TO_CLEAN_URL_REDIRECT, + filename__endswith=".html", ) - queryset = queryset.filter(prefix | page | exact | sphinx_html | sphinx_htmldir) + if filename: + queryset = queryset.filter( + page | exact | clean_url_to_html | html_to_clean_url + ) + else: + # If the filename is empty, we only need to match exact redirects. + # Since the other types of redirects are not valid without a filename. + queryset = queryset.filter(exact) + + # TODO: use filter(enabled=True) once we have removed the null option from the field. + queryset = queryset.exclude(enabled=False) if forced_only: queryset = queryset.filter(force=True) - # There should be one and only one redirect returned by this query. I - # can't think in a case where there can be more at this point. I'm - # leaving the loop just in case for now - for redirect in queryset.select_related("project"): + redirect = queryset.select_related("project").first() + if redirect: new_path = redirect.get_redirect_path( + filename=normalized_filename, path=normalized_path, - full_path=normalized_full_path, language=language, version_slug=version_slug, ) - if new_path: - return new_path, redirect.http_status - return (None, None) + return new_path, redirect.http_status + return None, None def _normalize_path(self, path): r""" From a0ad6e3b26672c38d822601525e62b962c04823a Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 2 Nov 2023 15:08:37 -0500 Subject: [PATCH 02/25] Move to constants --- readthedocs/api/v3/serializers.py | 2 +- readthedocs/api/v3/tests/test_redirects.py | 5 ++-- readthedocs/redirects/constants.py | 18 ++++++++++++++ .../migrations/0006_add_new_fields.py | 2 +- readthedocs/redirects/models.py | 24 ++----------------- readthedocs/redirects/querysets.py | 2 +- readthedocs/redirects/tests/test_views.py | 9 +++---- 7 files changed, 31 insertions(+), 31 deletions(-) create mode 100644 readthedocs/redirects/constants.py diff --git a/readthedocs/api/v3/serializers.py b/readthedocs/api/v3/serializers.py index 04a13cb5db3..204ed24c73d 100644 --- a/readthedocs/api/v3/serializers.py +++ b/readthedocs/api/v3/serializers.py @@ -27,7 +27,7 @@ Project, ProjectRelationship, ) -from readthedocs.redirects.models import TYPE_CHOICES as REDIRECT_TYPE_CHOICES +from readthedocs.redirects.constants import TYPE_CHOICES as REDIRECT_TYPE_CHOICES from readthedocs.redirects.models import Redirect diff --git a/readthedocs/api/v3/tests/test_redirects.py b/readthedocs/api/v3/tests/test_redirects.py index 05483c12205..afe0db4ae1f 100644 --- a/readthedocs/api/v3/tests/test_redirects.py +++ b/readthedocs/api/v3/tests/test_redirects.py @@ -1,3 +1,4 @@ +from readthedocs.redirects.constants import CLEAN_URL_TO_HTML_REDIRECT from .mixins import APIEndpointMixin from django.urls import reverse @@ -146,7 +147,7 @@ def test_projects_redirects_type_prefix_list_post(self): def test_projects_redirects_type_sphinx_html_list_post(self): self.assertEqual(Redirect.objects.count(), 1) data = { - "type": "sphinx_html", + "type": CLEAN_URL_TO_HTML_REDIRECT, } self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") @@ -163,7 +164,7 @@ def test_projects_redirects_type_sphinx_html_list_post(self): self.assertEqual(Redirect.objects.all().count(), 2) redirect = Redirect.objects.first() - self.assertEqual(redirect.redirect_type, "sphinx_html") + self.assertEqual(redirect.redirect_type, CLEAN_URL_TO_HTML_REDIRECT) self.assertEqual(redirect.from_url, "") self.assertEqual(redirect.to_url, "") diff --git a/readthedocs/redirects/constants.py b/readthedocs/redirects/constants.py new file mode 100644 index 00000000000..f4594fd29a9 --- /dev/null +++ b/readthedocs/redirects/constants.py @@ -0,0 +1,18 @@ +from django.utils.translation import gettext_lazy as _ + +HTTP_STATUS_CHOICES = ( + (302, _("302 - Temporary Redirect")), + (301, _("301 - Permanent Redirect")), +) + +PAGE_REDIRECT = "page" +EXACT_REDIRECT = "exact" +CLEAN_URL_TO_HTML_REDIRECT = "clean_url_to_html" +HTML_TO_CLEAN_URL_REDIRECT = "html_to_clean_url" + +TYPE_CHOICES = ( + (PAGE_REDIRECT, _("Page Redirect")), + (EXACT_REDIRECT, _("Exact Redirect")), + (CLEAN_URL_TO_HTML_REDIRECT, _("Clean URL to HTML (file/ to file.html)")), + (HTML_TO_CLEAN_URL_REDIRECT, _("HTML to clean URL (file.html to file/)")), +) diff --git a/readthedocs/redirects/migrations/0006_add_new_fields.py b/readthedocs/redirects/migrations/0006_add_new_fields.py index 92c90d4f477..91d2ba382ea 100644 --- a/readthedocs/redirects/migrations/0006_add_new_fields.py +++ b/readthedocs/redirects/migrations/0006_add_new_fields.py @@ -42,7 +42,7 @@ class Migration(migrations.Migration): model_name="redirect", name="status", field=models.BooleanField( - choices=[(True, "Active"), (False, "Inactive")], default=True, null=True + choices=[], default=True, null=True ), ), migrations.AlterField( diff --git a/readthedocs/redirects/models.py b/readthedocs/redirects/models.py index 048e728f21c..378e8b62bb7 100644 --- a/readthedocs/redirects/models.py +++ b/readthedocs/redirects/models.py @@ -11,30 +11,10 @@ from readthedocs.projects.models import Project from .querysets import RedirectQuerySet +from readthedocs.redirects.constants import TYPE_CHOICES, HTTP_STATUS_CHOICES, PAGE_REDIRECT, EXACT_REDIRECT log = structlog.get_logger(__name__) -HTTP_STATUS_CHOICES = ( - (302, _("302 - Temporary Redirect")), - (301, _("301 - Permanent Redirect")), -) - -STATUS_CHOICES = ( - (True, _("Active")), - (False, _("Inactive")), -) - -PAGE_REDIRECT = "page" -EXACT_REDIRECT = "exact" -CLEAN_URL_TO_HTML_REDIRECT = "clean_url_to_html" -HTML_TO_CLEAN_URL_REDIRECT = "html_to_clean_url" - -TYPE_CHOICES = ( - (PAGE_REDIRECT, _("Page Redirect")), - (EXACT_REDIRECT, _("Exact Redirect")), - (CLEAN_URL_TO_HTML_REDIRECT, _("Clean URL to HTML (file/ to file.html)")), - (HTML_TO_CLEAN_URL_REDIRECT, _("HTML to clean URL (file.html to file/)")), -) # FIXME: this help_text message should be dynamic since "Absolute path" doesn't # make sense for "Prefix Redirects" since the from URL is considered after the @@ -121,7 +101,7 @@ class Redirect(models.Model): ) # TODO: remove this field and use `enabled` instead. - status = models.BooleanField(choices=STATUS_CHOICES, default=True, null=True) + status = models.BooleanField(choices=[], default=True, null=True) create_dt = models.DateTimeField(auto_now_add=True) update_dt = models.DateTimeField(auto_now=True) diff --git a/readthedocs/redirects/querysets.py b/readthedocs/redirects/querysets.py index 88bb1d29dcd..1892650ab0b 100644 --- a/readthedocs/redirects/querysets.py +++ b/readthedocs/redirects/querysets.py @@ -6,7 +6,7 @@ from django.db.models import CharField, F, Q, Value from readthedocs.core.permissions import AdminPermission -from readthedocs.redirects.models import ( +from readthedocs.redirects.constants import ( CLEAN_URL_TO_HTML_REDIRECT, EXACT_REDIRECT, HTML_TO_CLEAN_URL_REDIRECT, diff --git a/readthedocs/redirects/tests/test_views.py b/readthedocs/redirects/tests/test_views.py index f03cc4d7f86..ad13067a276 100644 --- a/readthedocs/redirects/tests/test_views.py +++ b/readthedocs/redirects/tests/test_views.py @@ -5,6 +5,7 @@ from readthedocs.organizations.models import Organization from readthedocs.projects.models import Project +from readthedocs.redirects.constants import EXACT_REDIRECT, PAGE_REDIRECT from readthedocs.redirects.models import Redirect @@ -16,7 +17,7 @@ def setUp(self): self.redirect = get( Redirect, project=self.project, - redirect_type="exact", + redirect_type=EXACT_REDIRECT, from_url="/404.html", to_url="/en/latest/", ) @@ -27,7 +28,7 @@ def test_create_redirect(self): resp = self.client.post( reverse("projects_redirects_create", args=[self.project.slug]), data={ - "redirect_type": "page", + "redirect_type": PAGE_REDIRECT, "from_url": "/config.html", "to_url": "/configuration.html", }, @@ -42,14 +43,14 @@ def test_update_redirect(self): "projects_redirects_edit", args=[self.project.slug, self.redirect.pk] ), data={ - "redirect_type": "page", + "redirect_type": PAGE_REDIRECT, "from_url": "/config.html", "to_url": "/configuration.html", }, ) self.assertEqual(resp.status_code, 302) redirect = self.project.redirects.get() - self.assertEqual(redirect.redirect_type, "page") + self.assertEqual(redirect.redirect_type, PAGE_REDIRECT) self.assertEqual(redirect.from_url, "/config.html") self.assertEqual(redirect.to_url, "/configuration.html") self.assertEqual(self.project.redirects.all().count(), 1) From adc6bda9dba3b56b7ae113f43c0520dcdd929772 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 6 Nov 2023 12:41:10 -0500 Subject: [PATCH 03/25] Format --- readthedocs/api/v3/tests/test_redirects.py | 5 +++-- readthedocs/redirects/migrations/0006_add_new_fields.py | 4 +--- readthedocs/redirects/models.py | 7 ++++++- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/readthedocs/api/v3/tests/test_redirects.py b/readthedocs/api/v3/tests/test_redirects.py index afe0db4ae1f..18990d66a9b 100644 --- a/readthedocs/api/v3/tests/test_redirects.py +++ b/readthedocs/api/v3/tests/test_redirects.py @@ -1,9 +1,10 @@ -from readthedocs.redirects.constants import CLEAN_URL_TO_HTML_REDIRECT -from .mixins import APIEndpointMixin from django.urls import reverse +from readthedocs.redirects.constants import CLEAN_URL_TO_HTML_REDIRECT from readthedocs.redirects.models import Redirect +from .mixins import APIEndpointMixin + class RedirectsEndpointTests(APIEndpointMixin): def test_unauthed_projects_redirects_list(self): diff --git a/readthedocs/redirects/migrations/0006_add_new_fields.py b/readthedocs/redirects/migrations/0006_add_new_fields.py index 91d2ba382ea..45e76772e5f 100644 --- a/readthedocs/redirects/migrations/0006_add_new_fields.py +++ b/readthedocs/redirects/migrations/0006_add_new_fields.py @@ -41,9 +41,7 @@ class Migration(migrations.Migration): migrations.AlterField( model_name="redirect", name="status", - field=models.BooleanField( - choices=[], default=True, null=True - ), + field=models.BooleanField(choices=[], default=True, null=True), ), migrations.AlterField( model_name="redirect", diff --git a/readthedocs/redirects/models.py b/readthedocs/redirects/models.py index 378e8b62bb7..c5436389324 100644 --- a/readthedocs/redirects/models.py +++ b/readthedocs/redirects/models.py @@ -9,9 +9,14 @@ from readthedocs.core.resolver import resolve_path from readthedocs.projects.models import Project +from readthedocs.redirects.constants import ( + EXACT_REDIRECT, + HTTP_STATUS_CHOICES, + PAGE_REDIRECT, + TYPE_CHOICES, +) from .querysets import RedirectQuerySet -from readthedocs.redirects.constants import TYPE_CHOICES, HTTP_STATUS_CHOICES, PAGE_REDIRECT, EXACT_REDIRECT log = structlog.get_logger(__name__) From 97d6e3780e3983abc6aed49ce2921cb3a0f76cd6 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 6 Nov 2023 17:58:32 -0500 Subject: [PATCH 04/25] Fix test --- readthedocs/api/v3/serializers.py | 28 +++++++++++ .../responses/projects-redirects-detail.json | 8 ++- .../projects-redirects-detail_PUT.json | 8 ++- .../responses/projects-redirects-list.json | 8 ++- .../projects-redirects-list_POST.json | 8 ++- readthedocs/api/v3/tests/test_redirects.py | 50 +++++++++---------- .../migrations/0006_add_new_fields.py | 6 ++- readthedocs/redirects/models.py | 1 + readthedocs/redirects/tests/test_views.py | 2 + 9 files changed, 84 insertions(+), 35 deletions(-) diff --git a/readthedocs/api/v3/serializers.py b/readthedocs/api/v3/serializers.py index 204ed24c73d..bca56ff061e 100644 --- a/readthedocs/api/v3/serializers.py +++ b/readthedocs/api/v3/serializers.py @@ -856,6 +856,34 @@ class Meta: # TODO: allow editing this field for projects that have this feature enabled. read_only_fields = ["force"] + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self._removed_redirects = [ + ("prefix", "Removed, use an exact redirect `/prefix/*` instead."), + ("sphinx_html", "Renamed, use `clean_url_to_html` instead."), + ("sphinx_htmldir", "Renamed, use `html_to_clean_url` instead."), + ] + self.fields["type"].choices = ( + list(REDIRECT_TYPE_CHOICES) + self._removed_redirects + ) + + def validate_type(self, value): + if value == "prefix": + raise serializers.ValidationError( + _( + "Prefix redirects have been removed. Please use an exact redirect `/prefix/*` instead. See ." + ) + ) + elif value == "sphinx_html": + raise serializers.ValidationError( + _("sphinx_html redirect has been renamed to clean_url_to_html.") + ) + elif value == "sphinx_htmldir": + raise serializers.ValidationError( + _("sphinx_htmldir redirect has been renamed to html_to_clean_url.") + ) + return value + class RedirectCreateSerializer(RedirectSerializerBase): pass diff --git a/readthedocs/api/v3/tests/responses/projects-redirects-detail.json b/readthedocs/api/v3/tests/responses/projects-redirects-detail.json index 87992a9840d..6f10d37bc6c 100644 --- a/readthedocs/api/v3/tests/responses/projects-redirects-detail.json +++ b/readthedocs/api/v3/tests/responses/projects-redirects-detail.json @@ -5,9 +5,13 @@ }, "created": "2019-04-29T10:00:00Z", "modified": "2019-04-29T12:00:00Z", - "from_url": "/docs/", + "from_url": "/docs", "pk": 1, "project": "project", "type": "page", - "to_url": "/documentation/" + "to_url": "/documentation/", + "http_status": 302, + "description": "", + "enabled": true, + "force": false } diff --git a/readthedocs/api/v3/tests/responses/projects-redirects-detail_PUT.json b/readthedocs/api/v3/tests/responses/projects-redirects-detail_PUT.json index eab7e6a9e6a..67754b75cd5 100644 --- a/readthedocs/api/v3/tests/responses/projects-redirects-detail_PUT.json +++ b/readthedocs/api/v3/tests/responses/projects-redirects-detail_PUT.json @@ -5,9 +5,13 @@ }, "created": "2019-04-29T10:00:00Z", "modified": "2019-04-29T12:00:00Z", - "from_url": "/changed/", + "from_url": "/changed", "pk": 1, "project": "project", "type": "page", - "to_url": "/toanother/" + "to_url": "/toanother/", + "http_status": 302, + "description": "", + "enabled": true, + "force": false } diff --git a/readthedocs/api/v3/tests/responses/projects-redirects-list.json b/readthedocs/api/v3/tests/responses/projects-redirects-list.json index ce8ca8ea42a..12b936c4b5c 100644 --- a/readthedocs/api/v3/tests/responses/projects-redirects-list.json +++ b/readthedocs/api/v3/tests/responses/projects-redirects-list.json @@ -10,11 +10,15 @@ }, "created": "2019-04-29T10:00:00Z", "modified": "2019-04-29T12:00:00Z", - "from_url": "/docs/", + "from_url": "/docs", "pk": 1, "project": "project", "type": "page", - "to_url": "/documentation/" + "to_url": "/documentation/", + "http_status": 302, + "description": "", + "enabled": true, + "force": false } ] } diff --git a/readthedocs/api/v3/tests/responses/projects-redirects-list_POST.json b/readthedocs/api/v3/tests/responses/projects-redirects-list_POST.json index 90285fb9406..6dc60752d77 100644 --- a/readthedocs/api/v3/tests/responses/projects-redirects-list_POST.json +++ b/readthedocs/api/v3/tests/responses/projects-redirects-list_POST.json @@ -5,9 +5,13 @@ }, "created": "2019-04-29T10:00:00Z", "modified": "2019-04-29T12:00:00Z", - "from_url": "/page/", + "from_url": "/page", "pk": 2, "project": "project", "type": "page", - "to_url": "/another/" + "to_url": "/another/", + "http_status": 302, + "description": "", + "enabled": true, + "force": false } diff --git a/readthedocs/api/v3/tests/test_redirects.py b/readthedocs/api/v3/tests/test_redirects.py index 18990d66a9b..879cc431fe7 100644 --- a/readthedocs/api/v3/tests/test_redirects.py +++ b/readthedocs/api/v3/tests/test_redirects.py @@ -120,32 +120,30 @@ def test_projects_redirects_list_post(self): self._get_response_dict("projects-redirects-list_POST"), ) - def test_projects_redirects_type_prefix_list_post(self): - self.assertEqual(Redirect.objects.count(), 1) - data = { - "from_url": "/redirect-this/", - "type": "prefix", - } - - self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") - response = self.client.post( - reverse( - "projects-redirects-list", - kwargs={ - "parent_lookup_project__slug": self.project.slug, - }, - ), - data, - ) - self.assertEqual(response.status_code, 201) - self.assertEqual(Redirect.objects.all().count(), 2) - - redirect = Redirect.objects.first() - self.assertEqual(redirect.redirect_type, "prefix") - self.assertEqual(redirect.from_url, "/redirect-this/") - self.assertEqual(redirect.to_url, "") - - def test_projects_redirects_type_sphinx_html_list_post(self): + def test_projects_redirects_old_type_post(self): + for redirect_type in ["prefix", "sphinx_html", "sphinx_htmldir"]: + self.assertEqual(Redirect.objects.count(), 1) + data = { + "from_url": "/redirect-this/", + "type": redirect_type, + } + + self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") + response = self.client.post( + reverse( + "projects-redirects-list", + kwargs={ + "parent_lookup_project__slug": self.project.slug, + }, + ), + data, + ) + self.assertEqual(response.status_code, 400) + self.assertEqual(Redirect.objects.all().count(), 1) + json_response = response.json() + self.assertIn("type", json_response) + + def test_projects_redirects_type_clean_url_to_html_list_post(self): self.assertEqual(Redirect.objects.count(), 1) data = { "type": CLEAN_URL_TO_HTML_REDIRECT, diff --git a/readthedocs/redirects/migrations/0006_add_new_fields.py b/readthedocs/redirects/migrations/0006_add_new_fields.py index 45e76772e5f..d63eb0aeb6d 100644 --- a/readthedocs/redirects/migrations/0006_add_new_fields.py +++ b/readthedocs/redirects/migrations/0006_add_new_fields.py @@ -13,7 +13,11 @@ class Migration(migrations.Migration): model_name="redirect", name="description", field=models.CharField( - blank=True, max_length=255, verbose_name="Description", null=True + blank=True, + max_length=255, + verbose_name="Description", + null=True, + default="", ), ), migrations.AddField( diff --git a/readthedocs/redirects/models.py b/readthedocs/redirects/models.py index c5436389324..f2eeefa4c92 100644 --- a/readthedocs/redirects/models.py +++ b/readthedocs/redirects/models.py @@ -103,6 +103,7 @@ class Redirect(models.Model): blank=True, null=True, max_length=255, + default="", ) # TODO: remove this field and use `enabled` instead. diff --git a/readthedocs/redirects/tests/test_views.py b/readthedocs/redirects/tests/test_views.py index ad13067a276..fd047d8ab82 100644 --- a/readthedocs/redirects/tests/test_views.py +++ b/readthedocs/redirects/tests/test_views.py @@ -31,6 +31,7 @@ def test_create_redirect(self): "redirect_type": PAGE_REDIRECT, "from_url": "/config.html", "to_url": "/configuration.html", + "http_status": 302, }, ) self.assertEqual(resp.status_code, 302) @@ -46,6 +47,7 @@ def test_update_redirect(self): "redirect_type": PAGE_REDIRECT, "from_url": "/config.html", "to_url": "/configuration.html", + "http_status": 302, }, ) self.assertEqual(resp.status_code, 302) From 1caa26d07e82478f8408ae2b128f1e428e42d2d3 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 6 Nov 2023 23:37:04 -0500 Subject: [PATCH 05/25] Fix tests --- .../proxito/tests/test_old_redirects.py | 89 ++++++++++--------- readthedocs/redirects/models.py | 4 +- readthedocs/redirects/querysets.py | 30 +++---- 3 files changed, 64 insertions(+), 59 deletions(-) diff --git a/readthedocs/proxito/tests/test_old_redirects.py b/readthedocs/proxito/tests/test_old_redirects.py index 807c4b039a5..1d97c2ccef0 100644 --- a/readthedocs/proxito/tests/test_old_redirects.py +++ b/readthedocs/proxito/tests/test_old_redirects.py @@ -15,6 +15,11 @@ from readthedocs.builds.models import Version from readthedocs.projects.models import Feature +from readthedocs.redirects.constants import ( + CLEAN_URL_TO_HTML_REDIRECT, + EXACT_REDIRECT, + HTML_TO_CLEAN_URL_REDIRECT, +) from readthedocs.redirects.models import Redirect from .base import BaseDocServing @@ -153,7 +158,7 @@ def test_forced_redirect(self): fixture.get( Redirect, project=self.project, - redirect_type="exact", + redirect_type=EXACT_REDIRECT, from_url="/en/latest/install.html", to_url="/en/latest/tutorial/install.html", force=True, @@ -172,7 +177,7 @@ def test_infinite_redirect(self): fixture.get( Redirect, project=self.project, - redirect_type="exact", + redirect_type=EXACT_REDIRECT, from_url="/en/latest/install.html", to_url="/en/latest/install.html", ) @@ -187,7 +192,7 @@ def test_infinite_redirect_changing_protocol(self): fixture.get( Redirect, project=self.project, - redirect_type="exact", + redirect_type=EXACT_REDIRECT, from_url="/en/latest/install.html", to_url=f"https://{host}/en/latest/install.html", ) @@ -335,7 +340,7 @@ def test_redirect_exact(self): fixture.get( Redirect, project=self.project, - redirect_type="exact", + redirect_type=EXACT_REDIRECT, from_url="/en/latest/install.html", to_url="/en/latest/tutorial/install.html", ) @@ -352,7 +357,7 @@ def test_redirect_exact_looks_like_version(self): fixture.get( Redirect, project=self.project, - redirect_type="exact", + redirect_type=EXACT_REDIRECT, from_url="/en/versions.json", to_url="/en/latest/versions.json", ) @@ -365,20 +370,20 @@ def test_redirect_exact_looks_like_version(self): "http://project.dev.readthedocs.io/en/latest/versions.json", ) - def test_redirect_exact_with_rest(self): + def test_redirect_exact_with_wildcard(self): """ - Exact redirects can have a ``$rest`` in the ``from_url``. + Exact redirects can have a ``*`` at the end of ``from_url``. Use case: we want to deprecate version ``2.0`` and replace it by - ``3.0``. We write an exact redirect from ``/en/2.0/$rest`` to - ``/en/3.0/``. + ``3.0``. We write an exact redirect from ``/en/2.0/*`` to + ``/en/3.0/:splat``. """ fixture.get( Redirect, project=self.project, - redirect_type="exact", - from_url="/en/latest/$rest", - to_url="/en/version/", # change version + redirect_type=EXACT_REDIRECT, + from_url="/en/latest/*", + to_url="/en/version/:splat", # change version ) self.assertEqual(self.project.redirects.count(), 1) r = self.client.get( @@ -398,9 +403,9 @@ def test_redirect_exact_with_rest(self): fixture.get( Redirect, project=self.translation, - redirect_type="exact", - from_url="/es/version/$rest", - to_url="/en/master/", # change language and version + redirect_type=EXACT_REDIRECT, + from_url="/es/version/*", + to_url="/en/master/:splat", # change language and version ) r = self.client.get( "/es/version/guides/install.html", @@ -428,7 +433,7 @@ def test_redirect_inactive_version(self): fixture.get( Redirect, project=self.project, - redirect_type="exact", + redirect_type=EXACT_REDIRECT, from_url="/en/oldversion/", to_url="/en/newversion/", ) @@ -511,7 +516,7 @@ def test_redirect_html(self): fixture.get( Redirect, project=self.project, - redirect_type="sphinx_html", + redirect_type=CLEAN_URL_TO_HTML_REDIRECT, ) r = self.client.get( "/en/latest/faq/", headers={"host": "project.dev.readthedocs.io"} @@ -533,7 +538,7 @@ def test_redirect_html_root_index(self): fixture.get( Redirect, project=self.project, - redirect_type="sphinx_html", + redirect_type=CLEAN_URL_TO_HTML_REDIRECT, ) with override_settings(PYTHON_MEDIA=False): @@ -553,12 +558,14 @@ def test_redirect_html_root_index(self): r = self.client.get( "/en/latest/", headers={"host": "project.dev.readthedocs.io"} ) + breakpoint() + print() def test_redirect_html_index(self): fixture.get( Redirect, project=self.project, - redirect_type="sphinx_html", + redirect_type=CLEAN_URL_TO_HTML_REDIRECT, ) r = self.client.get( "/en/latest/faq/index.html", headers={"host": "project.dev.readthedocs.io"} @@ -573,7 +580,7 @@ def test_redirect_htmldir(self): fixture.get( Redirect, project=self.project, - redirect_type="sphinx_htmldir", + redirect_type=CLEAN_URL_TO_HTML_REDIRECT, ) r = self.client.get( "/en/latest/faq.html", headers={"host": "project.dev.readthedocs.io"} @@ -624,7 +631,7 @@ def test_no_forced_redirect(self): fixture.get( Redirect, project=self.project, - redirect_type="exact", + redirect_type=EXACT_REDIRECT, from_url="/en/latest/install.html", to_url="/en/latest/tutorial/install.html", force=False, @@ -675,7 +682,7 @@ def test_infinite_redirect_changing_protocol(self): fixture.get( Redirect, project=self.project, - redirect_type="exact", + redirect_type=EXACT_REDIRECT, from_url="/install.html", to_url=f"https://{host}/install.html", force=True, @@ -730,7 +737,7 @@ def test_redirect_exact(self): fixture.get( Redirect, project=self.project, - redirect_type="exact", + redirect_type=EXACT_REDIRECT, from_url="/en/latest/install.html", to_url="/en/latest/tutorial/install.html", force=True, @@ -744,13 +751,13 @@ def test_redirect_exact(self): "http://project.dev.readthedocs.io/en/latest/tutorial/install.html", ) - def test_redirect_exact_with_rest(self): + def test_redirect_exact_with_wildcard(self): fixture.get( Redirect, project=self.project, - redirect_type="exact", - from_url="/en/latest/$rest", - to_url="/en/version/", + redirect_type=EXACT_REDIRECT, + from_url="/en/latest/*", + to_url="/en/version/:splat", force=True, ) self.assertEqual(self.project.redirects.count(), 1) @@ -767,9 +774,9 @@ def test_redirect_exact_with_rest(self): fixture.get( Redirect, project=self.translation, - redirect_type="exact", - from_url="/es/latest/$rest", - to_url="/en/master/", + redirect_type=EXACT_REDIRECT, + from_url="/es/latest/*", + to_url="/en/master/:splat", force=True, ) r = self.client.get( @@ -831,7 +838,7 @@ def test_redirect_html(self): fixture.get( Redirect, project=self.project, - redirect_type="sphinx_html", + redirect_type=CLEAN_URL_TO_HTML_REDIRECT, force=True, ) r = self.client.get( @@ -847,7 +854,7 @@ def test_redirect_html_index(self): fixture.get( Redirect, project=self.project, - redirect_type="sphinx_html", + redirect_type=CLEAN_URL_TO_HTML_REDIRECT, force=True, ) r = self.client.get( @@ -863,7 +870,7 @@ def test_redirect_htmldir(self): fixture.get( Redirect, project=self.project, - redirect_type="sphinx_htmldir", + redirect_type=HTML_TO_CLEAN_URL_REDIRECT, force=True, ) r = self.client.get( @@ -879,7 +886,7 @@ def test_redirect_with_301_status(self): fixture.get( Redirect, project=self.project, - redirect_type="exact", + redirect_type=EXACT_REDIRECT, from_url="/en/latest/install.html", to_url="/en/latest/tutorial/install.html", http_status=301, @@ -978,7 +985,7 @@ def test_redirect_sphinx_htmldir_crossdomain(self): fixture.get( Redirect, project=self.project, - redirect_type="sphinx_htmldir", + redirect_type=HTML_TO_CLEAN_URL_REDIRECT, ) urls = [ @@ -1012,7 +1019,7 @@ def test_redirect_sphinx_html_crossdomain(self): fixture.get( Redirect, project=self.project, - redirect_type="sphinx_html", + redirect_type=CLEAN_URL_TO_HTML_REDIRECT, ) urls = [ @@ -1047,9 +1054,9 @@ def test_redirect_using_projects_prefix(self): redirect = fixture.get( Redirect, project=self.project, - redirect_type="exact", - from_url="/projects/$rest", - to_url="https://example.com/projects/", + redirect_type=EXACT_REDIRECT, + from_url="/projects/*", + to_url="https://example.com/projects/:splat", ) self.assertEqual(self.project.redirects.count(), 1) r = self.client.get( @@ -1062,8 +1069,8 @@ def test_redirect_using_projects_prefix(self): "https://example.com/projects/deleted-subproject/en/latest/guides/install.html", ) - redirect.from_url = "/projects/not-found/$rest" - redirect.to_url = "/projects/subproject/" + redirect.from_url = "/projects/not-found/*" + redirect.to_url = "/projects/subproject/:splat" redirect.save() r = self.client.get( "/projects/not-found/en/latest/guides/install.html", diff --git a/readthedocs/redirects/models.py b/readthedocs/redirects/models.py index f2eeefa4c92..4548acaf335 100644 --- a/readthedocs/redirects/models.py +++ b/readthedocs/redirects/models.py @@ -198,7 +198,7 @@ def redirect_page(self, filename, path, language=None, version_slug=None): log.debug("Redirecting...", redirect=self) to = self.to_url if self.from_url.endswith("*"): - splat = filename[len(self.from_url_without_rest) - 1 :] + splat = filename[len(self.from_url_without_rest) :] to = to.replace(":splat", splat) return self.get_full_path( filename=to, @@ -210,7 +210,7 @@ def redirect_page(self, filename, path, language=None, version_slug=None): def redirect_exact(self, filename, path, language=None, version_slug=None): log.debug("Redirecting...", redirect=self) if self.from_url.endswith("*"): - splat = path[len(self.from_url_without_rest) - 1 :] + splat = path[len(self.from_url_without_rest) :] to = self.to_url.replace(":splat", splat) return to return self.to_url diff --git a/readthedocs/redirects/querysets.py b/readthedocs/redirects/querysets.py index 1892650ab0b..45745dfd659 100644 --- a/readthedocs/redirects/querysets.py +++ b/readthedocs/redirects/querysets.py @@ -95,22 +95,20 @@ def get_redirect_path_with_status( from_url_without_rest__isnull=True, path_without_trailling_slash__exact=F("from_url"), ) - clean_url_to_html = Q( - redirect_type=CLEAN_URL_TO_HTML_REDIRECT, - filename__endswith="/", - ) | Q( - redirect_type=CLEAN_URL_TO_HTML_REDIRECT, - filename__endswith="/index.html", - ) - html_to_clean_url = Q( - redirect_type=HTML_TO_CLEAN_URL_REDIRECT, - filename__endswith=".html", - ) - - if filename: - queryset = queryset.filter( - page | exact | clean_url_to_html | html_to_clean_url - ) + clean_url_to_html = Q(redirect_type=CLEAN_URL_TO_HTML_REDIRECT) + html_to_clean_url = Q(redirect_type=HTML_TO_CLEAN_URL_REDIRECT) + + if filename in ["/index.html", "/"]: + # If the filename is the root index file, we only need to match page and exact redirects. + # Since we don't have a filename to redirect to, since ``/``/``/index.html`` is already the root. + queryset = queryset.filter(page | exact) + elif filename: + if filename.endswith(("/index.html", "/")): + queryset = queryset.filter(page | exact | clean_url_to_html) + elif filename.endswith(".html"): + queryset = queryset.filter(page | exact | html_to_clean_url) + else: + queryset = queryset.filter(page | exact) else: # If the filename is empty, we only need to match exact redirects. # Since the other types of redirects are not valid without a filename. From 50363441f119d5c59a011348d69a410dbcc8822e Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 6 Nov 2023 23:49:13 -0500 Subject: [PATCH 06/25] Linter --- readthedocs/api/v3/serializers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/readthedocs/api/v3/serializers.py b/readthedocs/api/v3/serializers.py index bca56ff061e..5462ddeb9c8 100644 --- a/readthedocs/api/v3/serializers.py +++ b/readthedocs/api/v3/serializers.py @@ -874,11 +874,11 @@ def validate_type(self, value): "Prefix redirects have been removed. Please use an exact redirect `/prefix/*` instead. See ." ) ) - elif value == "sphinx_html": + if value == "sphinx_html": raise serializers.ValidationError( _("sphinx_html redirect has been renamed to clean_url_to_html.") ) - elif value == "sphinx_htmldir": + if value == "sphinx_htmldir": raise serializers.ValidationError( _("sphinx_htmldir redirect has been renamed to html_to_clean_url.") ) From 41d49231335c5c7547ff228ab368805e29d33eac Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 7 Nov 2023 12:45:40 -0500 Subject: [PATCH 07/25] Fix tests --- .../proxito/tests/test_old_redirects.py | 45 ++++++++++--------- readthedocs/redirects/models.py | 35 ++++++++++----- 2 files changed, 50 insertions(+), 30 deletions(-) diff --git a/readthedocs/proxito/tests/test_old_redirects.py b/readthedocs/proxito/tests/test_old_redirects.py index 1d97c2ccef0..f7382659f8d 100644 --- a/readthedocs/proxito/tests/test_old_redirects.py +++ b/readthedocs/proxito/tests/test_old_redirects.py @@ -210,13 +210,14 @@ def test_redirect_prefix_infinite(self): return a 404. These examples comes from this issue: - * http://github.com/rtfd/readthedocs.org/issues/4673 + * http://github.com/readthedocs/readthedocs.org/issues/4673 """ fixture.get( Redirect, project=self.project, - redirect_type="prefix", - from_url="/", + redirect_type=EXACT_REDIRECT, + from_url="/*", + to_url="/en/latest/:splat", ) r = self.client.get( "/redirect.html", headers={"host": "project.dev.readthedocs.io"} @@ -244,8 +245,9 @@ def test_redirect_prefix_infinite(self): def test_redirect_root(self): Redirect.objects.create( project=self.project, - redirect_type="prefix", - from_url="/woot/", + redirect_type=EXACT_REDIRECT, + from_url="/woot/*", + to_url="/en/latest/:splat", ) r = self.client.get( "/woot/faq.html", headers={"host": "project.dev.readthedocs.io"} @@ -558,8 +560,6 @@ def test_redirect_html_root_index(self): r = self.client.get( "/en/latest/", headers={"host": "project.dev.readthedocs.io"} ) - breakpoint() - print() def test_redirect_html_index(self): fixture.get( @@ -580,7 +580,7 @@ def test_redirect_htmldir(self): fixture.get( Redirect, project=self.project, - redirect_type=CLEAN_URL_TO_HTML_REDIRECT, + redirect_type=HTML_TO_CLEAN_URL_REDIRECT, ) r = self.client.get( "/en/latest/faq.html", headers={"host": "project.dev.readthedocs.io"} @@ -595,8 +595,9 @@ def test_redirect_root_with_301_status(self): fixture.get( Redirect, project=self.project, - redirect_type="prefix", - from_url="/woot/", + redirect_type=EXACT_REDIRECT, + from_url="/woot/*", + to_url="/en/latest/:splat", http_status=301, ) r = self.client.get( @@ -613,8 +614,9 @@ def test_not_found_page_without_trailing_slash(self): fixture.get( Redirect, project=self.project, - redirect_type="prefix", - from_url="/", + redirect_type=EXACT_REDIRECT, + from_url="/*", + to_url="/en/latest/:splat", ) with self.assertRaises(Http404): @@ -652,8 +654,9 @@ def test_prefix_redirect(self): fixture.get( Redirect, project=self.project, - redirect_type="prefix", - from_url="/woot/", + redirect_type=EXACT_REDIRECT, + from_url="/woot/*", + to_url="/en/latest/:splat", force=True, ) r = self.client.get( @@ -918,8 +921,9 @@ def test_redirect_prefix_crossdomain(self): fixture.get( Redirect, project=self.project, - redirect_type="prefix", - from_url="/", + redirect_type=EXACT_REDIRECT, + from_url="/*", + to_url="/en/latest/:splat", ) urls = [ @@ -960,8 +964,9 @@ def test_redirect_prefix_crossdomain_with_newline_chars(self): fixture.get( Redirect, project=self.project, - redirect_type="prefix", - from_url="/", + redirect_type=EXACT_REDIRECT, + from_url="/*", + to_url="/en/latest/:splat", ) urls = [ ( @@ -978,7 +983,7 @@ def test_redirect_prefix_crossdomain_with_newline_chars(self): self.assertEqual(r.status_code, 302, url) self.assertEqual(r["Location"], expected_location, url) - def test_redirect_sphinx_htmldir_crossdomain(self): + def test_redirect_html_to_clean_url_crossdomain(self): """ Avoid redirecting to an external site unless the external site is in to_url """ @@ -1014,7 +1019,7 @@ def test_redirect_sphinx_htmldir_crossdomain(self): self.assertEqual(r.status_code, 302, url) self.assertEqual(r["Location"], expected_location, url) - def test_redirect_sphinx_html_crossdomain(self): + def test_redirect_clean_url_to_html_crossdomain(self): """Avoid redirecting to an external site unless the external site is in to_url.""" fixture.get( Redirect, diff --git a/readthedocs/redirects/models.py b/readthedocs/redirects/models.py index 4548acaf335..3ce50d34bab 100644 --- a/readthedocs/redirects/models.py +++ b/readthedocs/redirects/models.py @@ -194,14 +194,33 @@ def get_redirect_path(self, filename, path=None, language=None, version_slug=Non filename=filename, path=path, language=language, version_slug=version_slug ) + def _redirect_with_wildcard(self, current_path): + if self.from_url.endswith("*"): + # Detect infinite redirects of the form: + # /dir/* -> /dir/subdir/:splat + # For example: + # /dir/test.html will redirect to /dir/subdir/test.html, + # and if file doesn't exist, it will redirect to + # /dir/subdir/subdir/test.html and then to /dir/subdir/subdir/test.html and so on. + if ":splat" in self.to_url: + to_url_without_splat = self.to_url.split(":splat")[0] + if current_path.startswith(to_url_without_splat): + log.debug( + "Infinite redirect loop detected", + redirect=self, + ) + return None + + splat = current_path[len(self.from_url_without_rest) :] + to_url = self.to_url.replace(":splat", splat) + return to_url + return self.to_url + def redirect_page(self, filename, path, language=None, version_slug=None): log.debug("Redirecting...", redirect=self) - to = self.to_url - if self.from_url.endswith("*"): - splat = filename[len(self.from_url_without_rest) :] - to = to.replace(":splat", splat) + to_url = self._redirect_with_wildcard(current_path=filename) return self.get_full_path( - filename=to, + filename=to_url, language=language, version_slug=version_slug, allow_crossdomain=True, @@ -209,11 +228,7 @@ def redirect_page(self, filename, path, language=None, version_slug=None): def redirect_exact(self, filename, path, language=None, version_slug=None): log.debug("Redirecting...", redirect=self) - if self.from_url.endswith("*"): - splat = path[len(self.from_url_without_rest) :] - to = self.to_url.replace(":splat", splat) - return to - return self.to_url + return self._redirect_with_wildcard(current_path=path) def redirect_clean_url_to_html( self, filename, path, language=None, version_slug=None From 5cb323c5c422f425c683d7a3c5d5984742496669 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 7 Nov 2023 14:18:46 -0500 Subject: [PATCH 08/25] Fix tests --- readthedocs/api/v3/serializers.py | 3 +- .../proxito/tests/test_old_redirects.py | 259 ++++++++++++++++-- readthedocs/redirects/models.py | 14 +- readthedocs/redirects/querysets.py | 13 +- 4 files changed, 262 insertions(+), 27 deletions(-) diff --git a/readthedocs/api/v3/serializers.py b/readthedocs/api/v3/serializers.py index 5462ddeb9c8..a4b76aa33df 100644 --- a/readthedocs/api/v3/serializers.py +++ b/readthedocs/api/v3/serializers.py @@ -858,8 +858,9 @@ class Meta: def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) + # Allow using the old redirect types, so we can raise the proper error in ``validate_type`. self._removed_redirects = [ - ("prefix", "Removed, use an exact redirect `/prefix/*` instead."), + ("prefix", "Removed, use an `exact` redirect instead."), ("sphinx_html", "Renamed, use `clean_url_to_html` instead."), ("sphinx_htmldir", "Renamed, use `html_to_clean_url` instead."), ] diff --git a/readthedocs/proxito/tests/test_old_redirects.py b/readthedocs/proxito/tests/test_old_redirects.py index f7382659f8d..649f5445bba 100644 --- a/readthedocs/proxito/tests/test_old_redirects.py +++ b/readthedocs/proxito/tests/test_old_redirects.py @@ -19,6 +19,7 @@ CLEAN_URL_TO_HTML_REDIRECT, EXACT_REDIRECT, HTML_TO_CLEAN_URL_REDIRECT, + PAGE_REDIRECT, ) from readthedocs.redirects.models import Redirect @@ -202,7 +203,7 @@ def test_infinite_redirect_changing_protocol(self): with pytest.raises(Http404): self.client.get("/en/latest/install.html?foo=bar", headers={"host": host}) - def test_redirect_prefix_infinite(self): + def test_exact_redirect_avoid_infinite_redirect(self): """ Avoid infinite redirects. @@ -238,10 +239,78 @@ def test_redirect_prefix_infinite(self): ) with self.assertRaises(Http404): - r = self.client.get( + self.client.get( "/en/latest/redirect/", headers={"host": "project.dev.readthedocs.io"} ) + fixture.get( + Redirect, + project=self.project, + redirect_type=EXACT_REDIRECT, + from_url="/en/latest/*", + to_url="/en/latest/subdir/:splat", + ) + r = self.client.get( + "/en/latest/redirect.html", headers={"host": "project.dev.readthedocs.io"} + ) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], + "http://project.dev.readthedocs.io/en/latest/subdir/redirect.html", + ) + + with self.assertRaises(Http404): + self.client.get( + "/en/latest/subdir/redirect.html", + headers={"host": "project.dev.readthedocs.io"}, + ) + + def test_page_redirect_avoid_infinite_redirect(self): + fixture.get( + Redirect, + project=self.project, + redirect_type=PAGE_REDIRECT, + from_url="/*", + to_url="/subdir/:splat", + ) + r = self.client.get( + "/en/latest/redirect.html", headers={"host": "project.dev.readthedocs.io"} + ) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], + "http://project.dev.readthedocs.io/en/latest/subdir/redirect.html", + ) + + with self.assertRaises(Http404): + self.client.get( + "/en/latest/subdir/redirect.html", + headers={"host": "project.dev.readthedocs.io"}, + ) + + fixture.get( + Redirect, + project=self.project, + redirect_type=PAGE_REDIRECT, + from_url="/dir/*", + to_url="/dir/subdir/:splat", + ) + r = self.client.get( + "/en/latest/dir/redirect.html", + headers={"host": "project.dev.readthedocs.io"}, + ) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], + "http://project.dev.readthedocs.io/en/latest/dir/subdir/redirect.html", + ) + + with self.assertRaises(Http404): + self.client.get( + "/en/latest/dir/subdir/redirect.html", + headers={"host": "project.dev.readthedocs.io"}, + ) + def test_redirect_root(self): Redirect.objects.create( project=self.project, @@ -268,7 +337,7 @@ def test_redirect_root(self): def test_redirect_page(self): Redirect.objects.create( project=self.project, - redirect_type="page", + redirect_type=PAGE_REDIRECT, from_url="/install.html", to_url="/tutorial/install.html", ) @@ -289,7 +358,7 @@ def test_redirect_with_query_params_from_url(self): ) Redirect.objects.create( project=self.project, - redirect_type="page", + redirect_type=PAGE_REDIRECT, from_url="/install.html", to_url="/tutorial/install.html", ) @@ -310,7 +379,7 @@ def test_redirect_with_query_params_to_url(self): ) Redirect.objects.create( project=self.project, - redirect_type="page", + redirect_type=PAGE_REDIRECT, from_url="/install.html", to_url="/tutorial/install.html?query=one", ) @@ -419,6 +488,73 @@ def test_redirect_exact_with_wildcard(self): "http://project.dev.readthedocs.io/en/master/guides/install.html", ) + fixture.get( + Redirect, + project=self.project, + redirect_type=EXACT_REDIRECT, + from_url="/en/latest/tutorials/*", + to_url="/en/latest/tutorial.html", + ) + r = self.client.get( + "/en/latest/tutorials/install.html", + headers={"host": "project.dev.readthedocs.io"}, + ) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], "http://project.dev.readthedocs.io/en/latest/tutorial.html" + ) + + def test_page_redirect_with_wildcard(self): + fixture.get( + Redirect, + project=self.project, + redirect_type=PAGE_REDIRECT, + from_url="/*", + to_url="/guides/:splat", + ) + r = self.client.get( + "/en/latest/install.html", + headers={"host": "project.dev.readthedocs.io"}, + ) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], + "http://project.dev.readthedocs.io/en/latest/guides/install.html", + ) + + fixture.get( + Redirect, + project=self.project, + redirect_type=PAGE_REDIRECT, + from_url="/guides/*", + to_url="/guides/redirects/:splat", + ) + r = self.client.get( + "/en/latest/guides/install.html", + headers={"host": "project.dev.readthedocs.io"}, + ) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], + "http://project.dev.readthedocs.io/en/latest/guides/redirects/install.html", + ) + + fixture.get( + Redirect, + project=self.project, + redirect_type=PAGE_REDIRECT, + from_url="/tutorials/*", + to_url="/tutorial.html", + ) + r = self.client.get( + "/en/latest/tutorials/install.html", + headers={"host": "project.dev.readthedocs.io"}, + ) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], "http://project.dev.readthedocs.io/en/latest/tutorial.html" + ) + def test_redirect_inactive_version(self): """ Inactive Version (``active=False``) should redirect properly. @@ -452,7 +588,7 @@ def test_redirect_keeps_version_number(self): fixture.get( Redirect, project=self.project, - redirect_type="page", + redirect_type=PAGE_REDIRECT, from_url="/how_to_install.html", to_url="/install.html", ) @@ -472,7 +608,7 @@ def test_redirect_keeps_language(self): fixture.get( Redirect, project=self.project, - redirect_type="page", + redirect_type=PAGE_REDIRECT, from_url="/how_to_install.html", to_url="/install.html", ) @@ -490,7 +626,7 @@ def test_redirect_recognizes_custom_cname(self): fixture.get( Redirect, project=self.project, - redirect_type="page", + redirect_type=PAGE_REDIRECT, from_url="/install.html", to_url="/tutorial/install.html", ) @@ -626,6 +762,40 @@ def test_not_found_page_without_trailing_slash(self): headers={"host": "project.dev.readthedocs.io"}, ) + def test_page_redirect_with_and_without_trailing_slash(self): + fixture.get( + Redirect, + project=self.project, + redirect_type=PAGE_REDIRECT, + from_url="/install", + to_url="/tutorial/install/", + ) + + for url in ["/en/latest/install", "/en/latest/install/"]: + resp = self.client.get(url, headers={"host": "project.dev.readthedocs.io"}) + self.assertEqual(resp.status_code, 302) + self.assertEqual( + resp["Location"], + "http://project.dev.readthedocs.io/en/latest/tutorial/install/", + ) + + def test_exact_redirect_with_and_without_trailing_slash(self): + fixture.get( + Redirect, + project=self.project, + redirect_type=EXACT_REDIRECT, + from_url="/en/latest/install", + to_url="/en/latest/tutorial/install/", + ) + + for url in ["/en/latest/install", "/en/latest/install/"]: + resp = self.client.get(url, headers={"host": "project.dev.readthedocs.io"}) + self.assertEqual(resp.status_code, 302) + self.assertEqual( + resp["Location"], + "http://project.dev.readthedocs.io/en/latest/tutorial/install/", + ) + @override_settings(PUBLIC_DOMAIN="dev.readthedocs.io") class UserForcedRedirectTests(BaseDocServing): @@ -643,7 +813,7 @@ def test_no_forced_redirect(self): ) self.assertEqual(r.status_code, 200) - def test_prefix_redirect(self): + def test_exact_redirect_with_wildcard(self): """ Test prefix redirect. @@ -669,7 +839,7 @@ def test_infinite_redirect(self): fixture.get( Redirect, project=self.project, - redirect_type="page", + redirect_type=PAGE_REDIRECT, from_url="/install.html", to_url="/install.html", force=True, @@ -703,7 +873,7 @@ def test_redirect_page(self): fixture.get( Redirect, project=self.project, - redirect_type="page", + redirect_type=PAGE_REDIRECT, from_url="/install.html", to_url="/tutorial/install.html", force=True, @@ -721,7 +891,7 @@ def test_redirect_with_query_params(self): fixture.get( Redirect, project=self.project, - redirect_type="page", + redirect_type=PAGE_REDIRECT, from_url="/install.html", to_url="/tutorial/install.html", force=True, @@ -798,7 +968,7 @@ def test_redirect_keeps_language(self): fixture.get( Redirect, project=self.project, - redirect_type="page", + redirect_type=PAGE_REDIRECT, from_url="/how_to_install.html", to_url="/install.html", force=True, @@ -817,7 +987,7 @@ def test_redirect_recognizes_custom_cname(self): fixture.get( Redirect, project=self.project, - redirect_type="page", + redirect_type=PAGE_REDIRECT, from_url="/install.html", to_url="/tutorial/install.html", force=True, @@ -911,7 +1081,7 @@ def test_redirect_with_301_status(self): ROOT_URLCONF="readthedocs.proxito.tests.handler_404_urls", ) class UserRedirectCrossdomainTest(BaseDocServing): - def test_redirect_prefix_crossdomain(self): + def test_redirect_exact_redirect_with_wildcard_crossdomain(self): """ Avoid redirecting to an external site unless the external site is in to_url. @@ -960,7 +1130,7 @@ def test_redirect_prefix_crossdomain(self): self.assertEqual(r.status_code, 302, url) self.assertEqual(r["Location"], expected_location, url) - def test_redirect_prefix_crossdomain_with_newline_chars(self): + def test_redirect_exact_with_wildcard_crossdomain_with_newline_chars(self): fixture.get( Redirect, project=self.project, @@ -1086,3 +1256,60 @@ def test_redirect_using_projects_prefix(self): r["Location"], "http://project.dev.readthedocs.io/projects/subproject/en/latest/guides/install.html", ) + + def test_page_redirect_crossdomain(self): + fixture.get( + Redirect, + project=self.project, + redirect_type=PAGE_REDIRECT, + from_url="/install.html", + to_url="https://example.com/", + ) + r = self.client.get( + "/en/latest/install.html", headers={"host": "project.dev.readthedocs.io"} + ) + self.assertEqual(r.status_code, 302) + self.assertEqual(r["Location"], "https://example.com/") + + def test_page_redirect_with_wildcard_crossdomain(self): + fixture.get( + Redirect, + project=self.project, + redirect_type=PAGE_REDIRECT, + from_url="/tutorial/*", + to_url="https://example.com/:splat", + ) + r = self.client.get( + "/en/latest/tutorial/install.html", + headers={"host": "project.dev.readthedocs.io"}, + ) + self.assertEqual(r.status_code, 302) + self.assertEqual(r["Location"], "https://example.com/install.html") + + def test_exact_redirect_crossdomain(self): + fixture.get( + Redirect, + project=self.project, + redirect_type=EXACT_REDIRECT, + from_url="/en/latest/install.html", + to_url="https://example.com/", + ) + r = self.client.get( + "/en/latest/install.html", headers={"host": "project.dev.readthedocs.io"} + ) + self.assertEqual(r.status_code, 302) + self.assertEqual(r["Location"], "https://example.com/") + + def test_exact_redirect_with_wildcard_crossdomain(self): + fixture.get( + Redirect, + project=self.project, + redirect_type=EXACT_REDIRECT, + from_url="/en/latest/*", + to_url="https://example.com/:splat", + ) + r = self.client.get( + "/en/latest/install.html", headers={"host": "project.dev.readthedocs.io"} + ) + self.assertEqual(r.status_code, 302) + self.assertEqual(r["Location"], "https://example.com/install.html") diff --git a/readthedocs/redirects/models.py b/readthedocs/redirects/models.py index 3ce50d34bab..0389ca06020 100644 --- a/readthedocs/redirects/models.py +++ b/readthedocs/redirects/models.py @@ -219,12 +219,14 @@ def _redirect_with_wildcard(self, current_path): def redirect_page(self, filename, path, language=None, version_slug=None): log.debug("Redirecting...", redirect=self) to_url = self._redirect_with_wildcard(current_path=filename) - return self.get_full_path( - filename=to_url, - language=language, - version_slug=version_slug, - allow_crossdomain=True, - ) + if to_url: + return self.get_full_path( + filename=to_url, + language=language, + version_slug=version_slug, + allow_crossdomain=True, + ) + return None def redirect_exact(self, filename, path, language=None, version_slug=None): log.debug("Redirecting...", redirect=self) diff --git a/readthedocs/redirects/querysets.py b/readthedocs/redirects/querysets.py index 45745dfd659..7ff48844c9e 100644 --- a/readthedocs/redirects/querysets.py +++ b/readthedocs/redirects/querysets.py @@ -84,16 +84,21 @@ def get_redirect_path_with_status( ) page = Q( redirect_type=PAGE_REDIRECT, + from_url_without_rest__isnull=True, filename_without_trailling_slash__exact=F("from_url"), + ) | Q( + redirect_type=PAGE_REDIRECT, + from_url_without_rest__isnull=False, + filename__startswith=F("from_url_without_rest"), ) exact = Q( - redirect_type=EXACT_REDIRECT, - from_url_without_rest__isnull=False, - path__startswith=F("from_url_without_rest"), - ) | Q( redirect_type=EXACT_REDIRECT, from_url_without_rest__isnull=True, path_without_trailling_slash__exact=F("from_url"), + ) | Q( + redirect_type=EXACT_REDIRECT, + from_url_without_rest__isnull=False, + path__startswith=F("from_url_without_rest"), ) clean_url_to_html = Q(redirect_type=CLEAN_URL_TO_HTML_REDIRECT) html_to_clean_url = Q(redirect_type=HTML_TO_CLEAN_URL_REDIRECT) From df8ccab5cd9f50669207624cecab3881f668c237 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 7 Nov 2023 14:32:10 -0500 Subject: [PATCH 09/25] Linter --- readthedocs/redirects/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/redirects/models.py b/readthedocs/redirects/models.py index 0389ca06020..dddb12d0c8c 100644 --- a/readthedocs/redirects/models.py +++ b/readthedocs/redirects/models.py @@ -203,7 +203,7 @@ def _redirect_with_wildcard(self, current_path): # and if file doesn't exist, it will redirect to # /dir/subdir/subdir/test.html and then to /dir/subdir/subdir/test.html and so on. if ":splat" in self.to_url: - to_url_without_splat = self.to_url.split(":splat")[0] + to_url_without_splat = self.to_url.split(":splat", maxsplit=1)[0] if current_path.startswith(to_url_without_splat): log.debug( "Infinite redirect loop detected", From cf578b2090e5ceb9f634687501fae22b0d76db45 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 7 Nov 2023 16:39:08 -0500 Subject: [PATCH 10/25] Update migration --- readthedocs/redirects/migrations/0006_add_new_fields.py | 1 - 1 file changed, 1 deletion(-) diff --git a/readthedocs/redirects/migrations/0006_add_new_fields.py b/readthedocs/redirects/migrations/0006_add_new_fields.py index d63eb0aeb6d..16ceb7a792d 100644 --- a/readthedocs/redirects/migrations/0006_add_new_fields.py +++ b/readthedocs/redirects/migrations/0006_add_new_fields.py @@ -52,7 +52,6 @@ class Migration(migrations.Migration): name="redirect_type", field=models.CharField( choices=[ - ("prefix", "Prefix Redirect"), ("page", "Page Redirect"), ("exact", "Exact Redirect"), ("clean_url_to_html", "Clean URL to HTML (file/ to file.html)"), From d9aa41ae45ab0d1cdf4ddc1d95fecc8ea5fea4ad Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 7 Nov 2023 18:38:49 -0500 Subject: [PATCH 11/25] Add extra validations --- readthedocs/api/v3/serializers.py | 23 ++++++++ .../proxito/tests/test_old_redirects.py | 51 +++++++++++++++++ readthedocs/redirects/models.py | 56 +++++++++++++++---- 3 files changed, 120 insertions(+), 10 deletions(-) diff --git a/readthedocs/api/v3/serializers.py b/readthedocs/api/v3/serializers.py index a4b76aa33df..8f7adc81c56 100644 --- a/readthedocs/api/v3/serializers.py +++ b/readthedocs/api/v3/serializers.py @@ -29,6 +29,7 @@ ) from readthedocs.redirects.constants import TYPE_CHOICES as REDIRECT_TYPE_CHOICES from readthedocs.redirects.models import Redirect +from readthedocs.redirects.validators import validate_redirect class UserSerializer(FlexFieldsModelSerializer): @@ -885,6 +886,28 @@ def validate_type(self, value): ) return value + def create(self, validated_data): + validate_redirect( + project=validated_data["project"], + pk=None, + redirect_type=validated_data["redirect_type"], + from_url=validated_data["from_url"], + to_url=validated_data["to_url"], + error_class=serializers.ValidationError, + ) + return super().create(validated_data) + + def update(self, instance, validated_data): + validate_redirect( + project=instance.project, + pk=instance.pk, + redirect_type=validated_data["redirect_type"], + from_url=validated_data["from_url"], + to_url=validated_data["to_url"], + error_class=serializers.ValidationError, + ) + return super().update(instance, validated_data) + class RedirectCreateSerializer(RedirectSerializerBase): pass diff --git a/readthedocs/proxito/tests/test_old_redirects.py b/readthedocs/proxito/tests/test_old_redirects.py index 649f5445bba..39eba586b7c 100644 --- a/readthedocs/proxito/tests/test_old_redirects.py +++ b/readthedocs/proxito/tests/test_old_redirects.py @@ -13,6 +13,7 @@ from django.http import Http404 from django.test.utils import override_settings +from readthedocs.builds.constants import EXTERNAL from readthedocs.builds.models import Version from readthedocs.projects.models import Feature from readthedocs.redirects.constants import ( @@ -153,6 +154,7 @@ def test_root_redirect_with_query_params(self): PYTHON_MEDIA=True, PUBLIC_DOMAIN="dev.readthedocs.io", ROOT_URLCONF="readthedocs.proxito.tests.handler_404_urls", + RTD_EXTERNAL_VERSION_DOMAIN="readthedocs.build", ) class UserRedirectTests(MockStorageMixin, BaseDocServing): def test_forced_redirect(self): @@ -173,6 +175,55 @@ def test_forced_redirect(self): "http://project.dev.readthedocs.io/en/latest/tutorial/install.html", ) + def test_disabled_redirect(self): + redirect = fixture.get( + Redirect, + project=self.project, + redirect_type=EXACT_REDIRECT, + from_url="/en/latest/install.html", + to_url="/en/latest/tutorial/install.html", + enabled=True, + ) + url = "/en/latest/install.html" + r = self.client.get(url, headers={"host": "project.dev.readthedocs.io"}) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], + "http://project.dev.readthedocs.io/en/latest/tutorial/install.html", + ) + + redirect.enabled = False + redirect.save() + + with self.assertRaises(Http404): + self.client.get(url, headers={"host": "project.dev.readthedocs.io"}) + + def test_redirect_ignored_on_external_domain(self): + fixture.get( + Redirect, + project=self.project, + redirect_type=EXACT_REDIRECT, + from_url="/*", + to_url="/en/latest/:splat", + ) + url = "/install.html" + r = self.client.get(url, headers={"host": "project.dev.readthedocs.io"}) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], "http://project.dev.readthedocs.io/en/latest/install.html" + ) + + fixture.get( + Version, + project=self.project, + active=True, + built=True, + slug="22", + type=EXTERNAL, + ) + with self.assertRaises(Http404): + self.client.get(url, headers={"host": "project--22.readthedocs.build"}) + def test_infinite_redirect(self): host = "project.dev.readthedocs.io" fixture.get( diff --git a/readthedocs/redirects/models.py b/readthedocs/redirects/models.py index dddb12d0c8c..cea9fae0025 100644 --- a/readthedocs/redirects/models.py +++ b/readthedocs/redirects/models.py @@ -10,11 +10,12 @@ from readthedocs.core.resolver import resolve_path from readthedocs.projects.models import Project from readthedocs.redirects.constants import ( - EXACT_REDIRECT, + CLEAN_URL_TO_HTML_REDIRECT, + HTML_TO_CLEAN_URL_REDIRECT, HTTP_STATUS_CHOICES, - PAGE_REDIRECT, TYPE_CHOICES, ) +from readthedocs.redirects.validators import validate_redirect from .querysets import RedirectQuerySet @@ -120,21 +121,56 @@ class Meta: ordering = ("-update_dt",) def save(self, *args, **kwargs): - self.from_url = self.normalize_path(self.from_url) self.from_url_without_rest = None if self.redirect_type in [ - PAGE_REDIRECT, - EXACT_REDIRECT, - ] and self.from_url.endswith("*"): - self.from_url_without_rest = self.from_url.removesuffix("*") + CLEAN_URL_TO_HTML_REDIRECT, + HTML_TO_CLEAN_URL_REDIRECT, + ]: + # These redirects don't make use of the ``from_url``/``to_url`` fields. + self.to_url = "" + self.from_url = "" + else: + self.to_url = self.normalize_to_url(self.to_url) + self.from_url = self.normalize_from_url(self.from_url) + if self.from_url.endswith("*"): + self.from_url_without_rest = self.from_url.removesuffix("*") + super().save(*args, **kwargs) - def normalize_path(self, path): - """Normalize a path to be used for matching.""" - path = "/" + path.lstrip("/") + def normalize_from_url(self, path): + """ + Normalize from_url to be used for matching. + + Normalize the path to always start with one slash, + and end without a slash, so we can match both, + with and without a trailing slash. + """ path = path.rstrip("/") + path = "/" + path.lstrip("/") return path + def normalize_to_url(self, path): + """ + Normalize to_url to be used for redirecting. + + Normalize the path to always start with one slash, + if the path is not an absolute URL. + Otherwise, return the path as is. + """ + if re.match("^https?://", path): + return path + path = "/" + path.lstrip("/") + return path + + def clean(self): + validate_redirect( + project=self.project, + pk=self.pk, + redirect_type=self.redirect_type, + from_url=self.from_url, + to_url=self.to_url, + ) + def __str__(self): redirect_text = "{type}: {from_to_url}" if self.redirect_type in ["prefix", "page", "exact"]: From 6060874aab8cd8f81bf484d854427a51356569c4 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 7 Nov 2023 18:42:51 -0500 Subject: [PATCH 12/25] Missing file! --- readthedocs/redirects/validators.py | 40 +++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 readthedocs/redirects/validators.py diff --git a/readthedocs/redirects/validators.py b/readthedocs/redirects/validators.py new file mode 100644 index 00000000000..ee26b899431 --- /dev/null +++ b/readthedocs/redirects/validators.py @@ -0,0 +1,40 @@ +from django.core.exceptions import ValidationError + +from readthedocs.redirects.constants import ( + CLEAN_URL_TO_HTML_REDIRECT, + EXACT_REDIRECT, + HTML_TO_CLEAN_URL_REDIRECT, + PAGE_REDIRECT, +) + + +def validate_redirect( + *, project, pk, redirect_type, from_url, to_url, error_class=ValidationError +): + """ + Validation for redirects. + + This is in a separate function so we can use it in the clean method of the model + (used in forms), and in the Django Rest Framework serializer (used in the API). + Since DRF doesn't call the clean method of the model. + """ + if redirect_type in [EXACT_REDIRECT, PAGE_REDIRECT]: + if from_url.endswith("$rest"): + raise error_class("The $rest wildcard have been removed in favor of *.") + if "*" in from_url and not from_url.endswith("*"): + raise error_class("The * wildcard must be at the end of the path.") + if ":splat" in to_url and not from_url.endswith("*"): + raise error_class( + "The * wildcard must be at the end of from_url to use the :splat placeholder in to_url." + ) + + if redirect_type in [CLEAN_URL_TO_HTML_REDIRECT, HTML_TO_CLEAN_URL_REDIRECT]: + redirect_exists = ( + project.redirects.filter(redirect_type=redirect_type) + .exclude(pk=pk) + .exists() + ) + if redirect_exists: + raise error_class( + f"Only one redirect of type `{redirect_type}` is allowed per project." + ) From dbb599cd3a0ebc779c7f4e29f83fc8ff81fb5236 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 8 Nov 2023 12:34:01 -0500 Subject: [PATCH 13/25] Fix tests --- readthedocs/api/v3/serializers.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/readthedocs/api/v3/serializers.py b/readthedocs/api/v3/serializers.py index 8f7adc81c56..0b59534478e 100644 --- a/readthedocs/api/v3/serializers.py +++ b/readthedocs/api/v3/serializers.py @@ -891,8 +891,8 @@ def create(self, validated_data): project=validated_data["project"], pk=None, redirect_type=validated_data["redirect_type"], - from_url=validated_data["from_url"], - to_url=validated_data["to_url"], + from_url=validated_data.get("from_url", ""), + to_url=validated_data.get("to_url", ""), error_class=serializers.ValidationError, ) return super().create(validated_data) @@ -902,8 +902,8 @@ def update(self, instance, validated_data): project=instance.project, pk=instance.pk, redirect_type=validated_data["redirect_type"], - from_url=validated_data["from_url"], - to_url=validated_data["to_url"], + from_url=validated_data.get("from_url", ""), + to_url=validated_data.get("to_url", ""), error_class=serializers.ValidationError, ) return super().update(instance, validated_data) From 3940463fd85040b564b01f98084b1ea39bbc142a Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 4 Dec 2023 19:49:15 -0500 Subject: [PATCH 14/25] Implement ordering --- readthedocs/api/v3/serializers.py | 1 + readthedocs/builds/models.py | 109 +------------- readthedocs/projects/ordering.py | 134 ++++++++++++++++++ readthedocs/projects/urls/private.py | 6 + readthedocs/projects/views/private.py | 16 +++ .../migrations/0006_add_new_fields.py | 8 ++ readthedocs/redirects/models.py | 38 +++-- .../templates/redirects/redirect_list.html | 29 +++- readthedocs/redirects/validators.py | 2 +- 9 files changed, 225 insertions(+), 118 deletions(-) create mode 100644 readthedocs/projects/ordering.py diff --git a/readthedocs/api/v3/serializers.py b/readthedocs/api/v3/serializers.py index 2ac6f0a9523..3d3a947d337 100644 --- a/readthedocs/api/v3/serializers.py +++ b/readthedocs/api/v3/serializers.py @@ -856,6 +856,7 @@ class Meta: "enabled", "description", "http_status", + "position", "_links", ] # TODO: allow editing this field for projects that have this feature enabled. diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index 1a236ee7710..82646cf5532 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -8,7 +8,6 @@ import structlog from django.conf import settings from django.db import models -from django.db.models import F from django.urls import reverse from django.utils import timezone from django.utils.translation import gettext @@ -81,6 +80,7 @@ SPHINX_SINGLEHTML, ) from readthedocs.projects.models import APIProject, Project +from readthedocs.projects.ordering import ProjectItemPositionManager from readthedocs.projects.validators import validate_build_config_file from readthedocs.projects.version_handling import determine_stable_version @@ -1283,6 +1283,8 @@ class VersionAutomationRule(PolymorphicModel, TimeStampedModel): choices=VERSION_TYPES, ) + _position_manager = ProjectItemPositionManager(position_field_name="priority") + class Meta: unique_together = (('project', 'priority'),) ordering = ('priority', '-modified', '-created') @@ -1359,118 +1361,17 @@ def move(self, steps): self.priority = new_priority self.save() - def _change_priority(self): - """ - Re-order the priorities of the other rules when the priority of this rule changes. - - If the rule is new, we just need to move all other rules down, - so there is space for the new rule. - - If the rule already exists, we need to move the other rules up or down, - depending on the new priority, so we can insert the rule at the new priority. - - The save() method needs to be called after this method. - """ - total = self.project.automation_rules.count() - - # If the rule was just created, we just need to insert it at the given priority. - # We do this by moving the other rules down before saving. - if not self.pk: - # A new rule can be created at the end as max. - self.priority = min(self.priority, total) - - # A new rule can't be created with a negative priority. All rules start at 0. - self.priority = max(self.priority, 0) - - rules = ( - self.project.automation_rules.filter(priority__gte=self.priority) - # We sort the queryset in desc order - # to be updated in that order - # to avoid hitting the unique constraint (project, priority). - .order_by('-priority') - ) - expression = F('priority') + 1 - else: - current_priority = self.project.automation_rules.values_list( - "priority", - flat=True, - ).get(pk=self.pk) - - # An existing rule can't be moved past the end. - self.priority = min(self.priority, total - 1) - - # A new rule can't be created with a negative priority. all rules start at 0. - self.priority = max(self.priority, 0) - - # The rule wasn't moved, so we don't need to do anything. - if self.priority == current_priority: - return - - if self.priority > current_priority: - # It was moved down, so we need to move the other rules up. - rules = ( - self.project.automation_rules.filter( - priority__gt=current_priority, priority__lte=self.priority - ) - # We sort the queryset in asc order - # to be updated in that order - # to avoid hitting the unique constraint (project, priority). - .order_by("priority") - ) - expression = F("priority") - 1 - else: - # It was moved up, so we need to move the other rules down. - rules = ( - self.project.automation_rules.filter( - priority__lt=current_priority, priority__gte=self.priority - ) - # We sort the queryset in desc order - # to be updated in that order - # to avoid hitting the unique constraint (project, priority). - .order_by("-priority") - ) - expression = F("priority") + 1 - - # Put an impossible priority to avoid - # the unique constraint (project, priority) while updating. - # We use update() instead of save() to avoid calling the save() method again. - if self.pk: - self._meta.model.objects.filter(pk=self.pk).update(priority=total + 99) - - # NOTE: we can't use rules.update(priority=expression), because SQLite is used - # in tests and hits a UNIQUE constraint error. PostgreSQL doesn't have this issue. - # We use update() instead of save() to avoid calling the save() method. - for rule in rules: - self._meta.model.objects.filter(pk=rule.pk).update(priority=expression) - def save(self, *args, **kwargs): """Override method to update the other priorities before save.""" - self._change_priority() + self._position_manager.change_position_before_save(self) if not self.description: self.description = self.get_description() super().save(*args, **kwargs) def delete(self, *args, **kwargs): """Override method to update the other priorities after delete.""" - current_priority = self.priority - project = self.project super().delete(*args, **kwargs) - - rules = ( - project.automation_rules - .filter(priority__gte=current_priority) - # We sort the queryset in asc order - # to be updated in that order - # to avoid hitting the unique constraint (project, priority). - .order_by('priority') - ) - # We update each object one by one to - # avoid hitting the unique constraint (project, priority). - # We use update() instead of save() to avoid calling the save() method. - for rule in rules: - self._meta.model.objects.filter(pk=rule.pk).update( - priority=F("priority") - 1, - ) + self._position_manager.change_position_after_delete(self) def get_description(self): if self.description: diff --git a/readthedocs/projects/ordering.py b/readthedocs/projects/ordering.py new file mode 100644 index 00000000000..9527b385817 --- /dev/null +++ b/readthedocs/projects/ordering.py @@ -0,0 +1,134 @@ +from django.db.models import F + + +class ProjectItemPositionManager: + def __init__(self, position_field_name): + self.position_field_name = position_field_name + + def change_position_before_save(self, item): + """ + Re-order the positions of the other items when the position of this item changes. + + If the item is new, we just need to move all other items down, + so there is space for the one. + + If the item already exists, we need to move the other items up or down, + depending on the new position, so we can insert the item at the new position. + + The save() method needs to be called after this. + """ + model = item._meta.model + total = model.objects.filter(project=item.project).count() + + # If the item was just created, we just need to insert it at the given position. + # We do this by moving the other items down before saving. + if not item.pk: + # A new item can be created at the end as max. + position = min(getattr(item, self.position_field_name), total) + setattr(item, self.position_field_name, position) + + # A new item can't be created with a negative position. All items start at 0. + position = max(getattr(item, self.position_field_name), 0) + setattr(item, self.position_field_name, position) + + items = ( + model.objects.filter(project=item.project).filter( + **{self.position_field_name + "__gte": position} + ) + # We sort the queryset in desc order + # to be updated in that order + # to avoid hitting the unique constraint (project, position). + .order_by(f"-{self.position_field_name}") + ) + expression = F(self.position_field_name) + 1 + else: + current_position = model.objects.values_list( + self.position_field_name, + flat=True, + ).get(pk=item.pk) + + # An existing item can't be moved past the end. + position = min(getattr(item, self.position_field_name), total - 1) + setattr(item, self.position_field_name, position) + + # A new item can't be created with a negative position, all items start at 0. + position = max(getattr(item, self.position_field_name), 0) + setattr(item, self.position_field_name, position) + + # The item wasn't moved, so we don't need to do anything. + if position == current_position: + return + + if position > current_position: + # It was moved down, so we need to move the other items up. + items = ( + model.objects.filter(project=item.project).filter( + **{ + self.position_field_name + "__gt": current_position, + self.position_field_name + "__lte": position, + } + ) + # We sort the queryset in asc order + # to be updated in that order + # to avoid hitting the unique constraint (project, position). + .order_by(self.position_field_name) + ) + expression = F(self.position_field_name) - 1 + else: + # It was moved up, so we need to move the other items down. + items = ( + model.objects.filter(project=item.project).filter( + **{ + self.position_field_name + "__lt": current_position, + self.position_field_name + "__gte": position, + } + ) + # We sort the queryset in desc order + # to be updated in that order + # to avoid hitting the unique constraint (project, position). + .order_by(f"-{self.position_field_name}") + ) + expression = F(self.position_field_name) + 1 + + # Put an impossible position to avoid + # the unique constraint (project, position) while updating. + # We use update() instead of save() to avoid calling the save() method again. + if item.pk: + model.objects.filter(pk=item.pk).update( + **{self.position_field_name: total + 99} + ) + + # NOTE: we can't use items.update(position=expression), because SQLite is used + # in tests and hits a UNIQUE constraint error. PostgreSQL doesn't have this issue. + # We use update() instead of save() to avoid calling the save() method. + for item in items: + model.objects.filter(pk=item.pk).update( + **{self.position_field_name: expression} + ) + + def change_position_after_delete(self, item): + """ + Update the order of the other items after deleting an item. + + After deleting an item, we move the items below it up by one position. + + The delete() method needs to be called before this. + """ + model = item._meta.model + previous_position = getattr(item, self.position_field_name) + items = ( + model.objects.filter(project=item.project).filter( + **{self.position_field_name + "__gte": previous_position} + ) + # We sort the queryset in asc order + # to be updated in that order + # to avoid hitting the unique constraint (project, position). + .order_by(self.position_field_name) + ) + # We update each object one by one to + # avoid hitting the unique constraint (project, position). + # We use update() instead of save() to avoid calling the save() method. + for item in items: + model.objects.filter(pk=item.pk).update( + **{self.position_field_name: F(self.position_field_name) - 1} + ) diff --git a/readthedocs/projects/urls/private.py b/readthedocs/projects/urls/private.py index 8a926f8c5d6..afa607ae510 100644 --- a/readthedocs/projects/urls/private.py +++ b/readthedocs/projects/urls/private.py @@ -35,6 +35,7 @@ ProjectNotificationsDelete, ProjectRedirectsCreate, ProjectRedirectsDelete, + ProjectRedirectsInsert, ProjectRedirectsList, ProjectRedirectsUpdate, ProjectTranslationsDelete, @@ -141,6 +142,11 @@ ProjectRedirectsCreate.as_view(), name="projects_redirects_create", ), + re_path( + r"^(?P[-\w]+)/redirects/(?P\d+)/insert/(?P\d+)/$", + ProjectRedirectsInsert.as_view(), + name="projects_redirects_insert", + ), re_path( r"^(?P[-\w]+)/redirects/(?P[-\w]+)/edit/$", ProjectRedirectsUpdate.as_view(), diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index e5e9e63acef..80f4c970960 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -725,6 +725,22 @@ class ProjectRedirectsUpdate(ProjectRedirectsMixin, UpdateView): pass +class ProjectRedirectsInsert(ProjectRedirectsMixin, GenericModelView): + http_method_names = ["post"] + + def post(self, request, *args, **kwargs): + redirect = self.get_object() + position = int(self.kwargs["position"]) + redirect.position = position + redirect.save() + return HttpResponseRedirect( + reverse( + "projects_redirects", + args=[self.get_project().slug], + ) + ) + + class ProjectRedirectsDelete(ProjectRedirectsMixin, DeleteView): http_method_names = ['post'] diff --git a/readthedocs/redirects/migrations/0006_add_new_fields.py b/readthedocs/redirects/migrations/0006_add_new_fields.py index 16ceb7a792d..40873e0e469 100644 --- a/readthedocs/redirects/migrations/0006_add_new_fields.py +++ b/readthedocs/redirects/migrations/0006_add_new_fields.py @@ -30,6 +30,14 @@ class Migration(migrations.Migration): null=True, ), ), + migrations.AddField( + model_name="redirect", + name="position", + field=models.PositiveIntegerField( + default=0, + help_text="Order of execution of the redirect.", + ), + ), migrations.AlterField( model_name="redirect", name="http_status", diff --git a/readthedocs/redirects/models.py b/readthedocs/redirects/models.py index 913e549ad32..aa781696bcc 100644 --- a/readthedocs/redirects/models.py +++ b/readthedocs/redirects/models.py @@ -9,10 +9,13 @@ from readthedocs.core.resolver import Resolver from readthedocs.projects.models import Project +from readthedocs.projects.ordering import ProjectItemPositionManager from readthedocs.redirects.constants import ( CLEAN_URL_TO_HTML_REDIRECT, + EXACT_REDIRECT, HTML_TO_CLEAN_URL_REDIRECT, HTTP_STATUS_CHOICES, + PAGE_REDIRECT, TYPE_CHOICES, ) from readthedocs.redirects.validators import validate_redirect @@ -107,18 +110,31 @@ class Redirect(models.Model): default="", ) + position = models.PositiveIntegerField( + _("Position"), + default=0, + help_text=_("Order of execution of the redirect."), + ) + # TODO: remove this field and use `enabled` instead. status = models.BooleanField(choices=[], default=True, null=True) create_dt = models.DateTimeField(auto_now_add=True) update_dt = models.DateTimeField(auto_now=True) + _position_manager = ProjectItemPositionManager(position_field_name="position") + objects = RedirectQuerySet.as_manager() class Meta: verbose_name = _("redirect") verbose_name_plural = _("redirects") - ordering = ("-update_dt",) + ordering = ( + "position", + "-update_dt", + ) + # TODO: add the project, position unique_together constraint once + # all redirects have a position set. def save(self, *args, **kwargs): self.from_url_without_rest = None @@ -135,8 +151,13 @@ def save(self, *args, **kwargs): if self.from_url.endswith("*"): self.from_url_without_rest = self.from_url.removesuffix("*") + self._position_manager.change_position_before_save(self) super().save(*args, **kwargs) + def delete(self, *args, **kwargs): + super().delete(*args, **kwargs) + self._position_manager.change_position_after_delete(self) + def normalize_from_url(self, path): """ Normalize from_url to be used for matching. @@ -173,7 +194,7 @@ def clean(self): def __str__(self): redirect_text = "{type}: {from_to_url}" - if self.redirect_type in ["prefix", "page", "exact"]: + if self.redirect_type in [PAGE_REDIRECT, EXACT_REDIRECT]: return redirect_text.format( type=self.get_redirect_type_display(), from_to_url=self.get_from_to_url_display(), @@ -185,17 +206,10 @@ def __str__(self): ) def get_from_to_url_display(self): - if self.redirect_type in ["prefix", "page", "exact"]: - from_url = self.from_url - to_url = self.to_url - if self.redirect_type == "prefix": - to_url = "/{lang}/{version}/".format( - lang=self.project.language, - version=self.project.default_version, - ) + if self.redirect_type in [PAGE_REDIRECT, EXACT_REDIRECT]: return "{from_url} -> {to_url}".format( - from_url=from_url, - to_url=to_url, + from_url=self.from_url, + to_url=self.to_url, ) return "" diff --git a/readthedocs/redirects/templates/redirects/redirect_list.html b/readthedocs/redirects/templates/redirects/redirect_list.html index 998125f8931..2cddc3287e8 100644 --- a/readthedocs/redirects/templates/redirects/redirect_list.html +++ b/readthedocs/redirects/templates/redirects/redirect_list.html @@ -1,9 +1,14 @@ {% extends "projects/project_edit_base.html" %} {% load i18n %} +{% load static %} {% block title %}{% trans "Redirects" %}{% endblock %} +{% block extra_links %} + +{% endblock %} + {% block project-redirects-active %}active{% endblock %} {% block project_edit_content_header %}{% trans "Redirects" %}{% endblock %} @@ -26,7 +31,7 @@
-
+
    {% for redirect in redirects %} @@ -40,6 +45,28 @@
{% endif %}