Skip to content

Proxito: redirect to default version from root language #10313

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
May 24, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions docs/user/user-defined-redirects.rst
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,20 @@ For example::
You can choose which is the :term:`default version` for Read the Docs to display.
This usually corresponds to the most recent official release from your project.

Root language redirect at ``/<lang>/``
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

A link to the root language of your documentation (`<slug>.readthedocs.io/en/`)
will redirect to the :term:`default version` of that translation,
as set in your project settings.

This works for readthedocs.io (|org_brand|) and :doc:`custom domains </custom-domains>`.
For |com_brand| this can be enabled by contacting :doc:`support </support>`.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is since this "feature" is only available when using the new proxito implementation.


For example::
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
For example::
For example, accessing the English language of the project will redirect you to the ``stable`` version::


https://docs.readthedocs.io/en/ -> https://docs.readthedocs.io/en/stable/

Shortlink with ``https://<slug>.rtfd.io``
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Expand Down
14 changes: 14 additions & 0 deletions readthedocs/core/unresolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ def __init__(self, project, language, version_slug, filename):
self.version_slug = version_slug


class TranslationWithoutVersionError(UnresolverError):
def __init__(self, project, language):
self.project = project
self.language = language


class InvalidPathForVersionedProjectError(UnresolverError):
def __init__(self, project, path):
self.project = project
Expand Down Expand Up @@ -272,6 +278,14 @@ def _match_multiversion_project(
filename=file,
)

# If only the language part was given,
# we can't resolve the version.
if version_slug is None:
raise TranslationWithoutVersionError(
project=project,
language=language,
)

if external_version_slug and external_version_slug != version_slug:
raise InvalidExternalVersionError(
project=project,
Expand Down
49 changes: 49 additions & 0 deletions readthedocs/proxito/tests/test_redirects.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
from django.test import override_settings
from django_dynamic_fixture import get

from readthedocs.builds.models import Version
from readthedocs.projects.constants import PUBLIC
from readthedocs.projects.models import Feature
from readthedocs.proxito.constants import RedirectType
from readthedocs.subscriptions.constants import TYPE_CNAME
Expand All @@ -11,6 +13,7 @@

@override_settings(
PUBLIC_DOMAIN='dev.readthedocs.io',
RTD_EXTERNAL_VERSION_DOMAIN="dev.readthedocs.build",
PUBLIC_DOMAIN_USES_HTTPS=True,
RTD_DEFAULT_FEATURES={
TYPE_CNAME: 1,
Expand Down Expand Up @@ -412,3 +415,49 @@ def setUp(self):
default_true=True,
future_default_true=True,
)

def test_redirect_from_root_language_to_default_version(self):
paths = [
"/en",
"/en/",
Comment on lines +421 to +422
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

]
for path in paths:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a for here, shouldn't these be pytest.parametrize? (I know you love that one 😉 ). It would be clearer when the test fail to know what is the exact case we are testing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note there are more cases like this in the other tests we can change to pytest.parametrize.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think parametrize doesn't work with tests that inherit from TestCase, I'll give it a try

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r = self.client.get(
path,
secure=True,
HTTP_HOST="project.dev.readthedocs.io",
)
self.assertEqual(r.status_code, 302)
self.assertEqual(
r["Location"],
"https://project.dev.readthedocs.io/en/latest/",
)
self.assertEqual(r.headers["CDN-Cache-Control"], "public")
self.assertEqual(r.headers["Cache-Tag"], "project")
self.assertEqual(r.headers["X-RTD-Redirect"], RedirectType.system.name)

def test_redirect_from_root_language_to_default_external_version(self):
get(
Version,
slug="10",
project=self.project,
privacy_level=PUBLIC,
)
paths = [
"/en",
"/en/",
]
for path in paths:
r = self.client.get(
path,
secure=True,
HTTP_HOST="project--10.dev.readthedocs.build",
)
self.assertEqual(r.status_code, 302)
self.assertEqual(
r["Location"],
"https://project--10.dev.readthedocs.build/en/10/",
)
self.assertEqual(r.headers["CDN-Cache-Control"], "public")
self.assertEqual(r.headers["Cache-Tag"], "project")
self.assertEqual(r.headers["X-RTD-Redirect"], RedirectType.system.name)
28 changes: 27 additions & 1 deletion readthedocs/proxito/views/serve.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
InvalidExternalVersionError,
InvalidPathForVersionedProjectError,
TranslationNotFoundError,
TranslationWithoutVersionError,
VersionNotFoundError,
unresolver,
)
Expand Down Expand Up @@ -357,6 +358,25 @@ def get_using_unresolver(self, request):
# TODO: find a better way to pass this to the middleware.
request.path_project_slug = exc.project.slug
raise Http404
except TranslationWithoutVersionError as exc:
project = exc.project
# TODO: find a better way to pass this to the middleware.
request.path_project_slug = project.slug

if unresolved_domain.is_from_external_domain:
version_slug = unresolved_domain.external_version_slug
else:
version_slug = None
# Redirect to the default version of the current translation.
# This is `/en -> /en/latest/` or
# `/projects/subproject/en/ -> /projects/subproject/en/latest/`.
return self.system_redirect(
request=request,
final_project=project,
version_slug=version_slug,
filename="",
is_external_version=unresolved_domain.is_from_external_domain,
)
except InvalidPathForVersionedProjectError as exc:
project = exc.project
if unresolved_domain.is_from_external_domain:
Expand Down Expand Up @@ -745,7 +765,9 @@ def get_using_unresolver(self, request, path):

project = None
version = None
filename = None
# If we weren't able to resolve a filename,
# then the path is the filename.
filename = path
lang_slug = None
version_slug = None
# Try to map the current path to a project/version/filename.
Expand Down Expand Up @@ -778,6 +800,10 @@ def get_using_unresolver(self, request, path):
version_slug = exc.version_slug
filename = exc.filename
contextualized_404_class = ProjectTranslationHttp404
except TranslationWithoutVersionError as exc:
project = exc.project
lang_slug = exc.language
# TODO: Use a contextualized 404
except InvalidExternalVersionError as exc:
project = exc.project
# TODO: Use a contextualized 404
Expand Down
27 changes: 26 additions & 1 deletion readthedocs/rtd_tests/tests/test_unresolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
InvalidPathForVersionedProjectError,
SuspiciousHostnameError,
TranslationNotFoundError,
TranslationWithoutVersionError,
VersionNotFoundError,
unresolve,
)
Expand Down Expand Up @@ -99,10 +100,34 @@ def test_path_no_version(self):
"https://pip.readthedocs.io/en/",
]
for url in urls:
with pytest.raises(VersionNotFoundError) as excinfo:
with pytest.raises(TranslationWithoutVersionError) as excinfo:
unresolve(url)
exc = excinfo.value
self.assertEqual(exc.project, self.pip)
self.assertEqual(exc.language, "en")

urls = [
"https://pip.readthedocs.io/ja",
"https://pip.readthedocs.io/ja/",
]
for url in urls:
with pytest.raises(TranslationWithoutVersionError) as excinfo:
unresolve(url)
exc = excinfo.value
self.assertEqual(exc.project, self.translation)
self.assertEqual(exc.language, "ja")

def test_invalid_language_no_version(self):
urls = [
"https://pip.readthedocs.io/es",
"https://pip.readthedocs.io/es/",
]
for url in urls:
with pytest.raises(TranslationNotFoundError) as excinfo:
unresolve(url)
exc = excinfo.value
self.assertEqual(exc.project, self.pip)
self.assertEqual(exc.language, "es")
self.assertEqual(exc.version_slug, None)
self.assertEqual(exc.filename, "/")

Expand Down