From 546f390411e055c64609d551553b4d9d51ec5737 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 6 Apr 2020 17:52:00 -0500 Subject: [PATCH 01/10] Add tests --- readthedocs/rtd_tests/tests/test_resolver.py | 55 ++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/readthedocs/rtd_tests/tests/test_resolver.py b/readthedocs/rtd_tests/tests/test_resolver.py index da9a1d1ad81..5b7aa835c64 100644 --- a/readthedocs/rtd_tests/tests/test_resolver.py +++ b/readthedocs/rtd_tests/tests/test_resolver.py @@ -554,6 +554,61 @@ def test_resolver_translation(self): url = resolve(project=self.translation) self.assertEqual(url, 'http://pip.readthedocs.org/ja/latest/') + @override_settings(PRODUCTION_DOMAIN='readthedocs.org') + def test_resolver_nested_translation_and_subproject(self): + """The project is a translation, and the main translation is a subproject of a project.""" + translation = fixture.get( + Project, + slug='api-es', + language='es', + users=[self.owner], + main_language_project=self.subproject, + ) + + with override_settings(USE_SUBDOMAIN=False): + url = resolve(project=translation) + self.assertEqual( + url, 'http://readthedocs.org/docs/pip/projects/sub/es/latest/', + ) + with override_settings(USE_SUBDOMAIN=True): + url = resolve(project=translation) + self.assertEqual( + url, 'http://pip.readthedocs.org/projects/sub/es/latest/', + ) + + @override_settings(PRODUCTION_DOMAIN='readthedocs.org') + def test_resolver_nestesd_subproject_and_translation(self): + """The project is a subproject, and the superproject is a translation of a project.""" + project = fixture.get( + Project, + slug='all-docs', + language='en', + users=[self.owner], + main_language_project=None, + ) + translation = fixture.get( + Project, + slug='docs-es', + language='es', + users=[self.owner], + main_language_project=None, + ) + subproject = fixture.get( + Project, + slug='api-es', + language='es', + users=[self.owner], + main_language_project=None, + ) + self.translation.add_subproject(subproject) + + with override_settings(USE_SUBDOMAIN=False): + url = resolve(project=self.translation) + self.assertEqual(url, 'http://readthedocs.org/docs/pip/ja/latest/') + with override_settings(USE_SUBDOMAIN=True): + url = resolve(project=self.translation) + self.assertEqual(url, 'http://pip.readthedocs.org/ja/latest/') + @override_settings(PRODUCTION_DOMAIN='readthedocs.org') def test_resolver_single_version(self): self.pip.single_version = True From 558b7ae60186993ca777a105fcafb60d04dbd4b7 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 6 Apr 2020 17:56:15 -0500 Subject: [PATCH 02/10] Optimize resolver --- readthedocs/core/resolver.py | 85 ++++++++++++++++++++++++++---------- 1 file changed, 62 insertions(+), 23 deletions(-) diff --git a/readthedocs/core/resolver.py b/readthedocs/core/resolver.py index 503c7bd8daa..e1535107f93 100644 --- a/readthedocs/core/resolver.py +++ b/readthedocs/core/resolver.py @@ -103,7 +103,6 @@ def resolve_path( private=None, ): """Resolve a URL with a subset of fields defined.""" - cname = cname or project.get_canonical_custom_domain() version_slug = version_slug or project.get_default_version() language = language or project.language @@ -112,28 +111,13 @@ def resolve_path( filename = self._fix_filename(project, filename) - current_project = project - project_slug = project.slug - subproject_slug = None - # We currently support more than 2 levels of nesting subprojects and - # translations, only loop twice to avoid sticking in the loop - for _ in range(0, 2): - main_language_project = current_project.main_language_project - relation = current_project.get_parent_relationship() - - if main_language_project: - current_project = main_language_project - project_slug = main_language_project.slug - language = project.language - subproject_slug = None - elif relation: - current_project = relation.parent - project_slug = relation.parent.slug - subproject_slug = relation.alias - cname = relation.parent.domains.filter(canonical=True).first() - else: - break - + main_project, subproject_slug = self._get_canonical_project_data(project) + project_slug = main_project.slug + cname = ( + cname + or self._use_subdomain() + or main_project.get_canonical_custom_domain() + ) single_version = bool(project.single_version or single_version) return self.base_resolve_path( @@ -204,6 +188,61 @@ def resolve( ) return urlunparse((protocol, domain, path, '', query_params, '')) + def _get_canonical_project_data(self, project): + """ + Returns a tuple with (project, subproject_slug) from the canonical project of `project`. + + We currently support more than 2 levels of nesting subprojects and translations, + but we only serve 2 levels to avoid sticking in the loop. + This means, we can have the following cases: + + - The project isn't a translation or subproject + + We serve the documentation from the domain of the project itself + (main.docs.com/). + + - The project is a translation of a project + + We serve the documentation from the domain of the main translation + (main.docs.com/es/). + + - The project is a subproject of a project + + We serve the documentation from the domain of the super project + (main.docs.com/projects/subproject/). + + - The project is a translation, and the main translation is a subproject of a project, like: + + - docs.docs.com + - api.docs.com (subproject of ``docs``) + - api-es.docs.com (translation of ``api``, and current project to be served) + + We serve the documentation from the domain of the super project + (docs.docs.com/projects/api/es/). + + - The project is a subproject, and the superproject is a translation of a project, like: + + - docs.docs.com/ + - docs-es.docs.com/ (translation of ``docs``) + - api-es.docs.com/ (subproject of ``docs-es``, and current project to be served) + + We serve the documentation from the domain of the super project (the translation), + this is docs-es.docs.com/projects/api-es/es/. + """ + main_project = project + subproject_slug = None + + main_language_project = main_project.main_language_project + if main_language_project: + main_project = main_language_project + + relation = main_project.get_parent_relationship() + if relation: + main_project = relation.parent + subproject_slug = relation.alias + + return (main_project, subproject_slug) + def _get_canonical_project(self, project, projects=None): """ Recursively get canonical project for subproject or translations. From ca62a45388a2af2c70c999ce0bd0c8dbf34ea644 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 6 Apr 2020 18:43:57 -0500 Subject: [PATCH 03/10] Expand docstring --- readthedocs/core/resolver.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/readthedocs/core/resolver.py b/readthedocs/core/resolver.py index e1535107f93..6ae569b3c7b 100644 --- a/readthedocs/core/resolver.py +++ b/readthedocs/core/resolver.py @@ -228,6 +228,12 @@ def _get_canonical_project_data(self, project): We serve the documentation from the domain of the super project (the translation), this is docs-es.docs.com/projects/api-es/es/. + + In summary: If the project is a subproject, + we don't care if the superproject has a translation, + we always serve from the domain of the superproject. + If the project is a translation, + we need to check if the main translation is a subproject. """ main_project = project subproject_slug = None From fc9f296dc1fe823e426783b2d77553060bb312a2 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 6 Apr 2020 18:47:36 -0500 Subject: [PATCH 04/10] Fix typo --- readthedocs/core/resolver.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/core/resolver.py b/readthedocs/core/resolver.py index 6ae569b3c7b..e3bc7fef02e 100644 --- a/readthedocs/core/resolver.py +++ b/readthedocs/core/resolver.py @@ -230,7 +230,7 @@ def _get_canonical_project_data(self, project): this is docs-es.docs.com/projects/api-es/es/. In summary: If the project is a subproject, - we don't care if the superproject has a translation, + we don't care if the superproject is a translation, we always serve from the domain of the superproject. If the project is a translation, we need to check if the main translation is a subproject. From 2b5c295a9578f17ca8713d1c3ec699d6381191bc Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 6 Apr 2020 18:56:18 -0500 Subject: [PATCH 05/10] Update tests --- readthedocs/rtd_tests/tests/test_resolver.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_resolver.py b/readthedocs/rtd_tests/tests/test_resolver.py index 5b7aa835c64..799a65e7c75 100644 --- a/readthedocs/rtd_tests/tests/test_resolver.py +++ b/readthedocs/rtd_tests/tests/test_resolver.py @@ -577,7 +577,7 @@ def test_resolver_nested_translation_and_subproject(self): ) @override_settings(PRODUCTION_DOMAIN='readthedocs.org') - def test_resolver_nestesd_subproject_and_translation(self): + def test_resolver_nested_subproject_and_translation(self): """The project is a subproject, and the superproject is a translation of a project.""" project = fixture.get( Project, @@ -600,14 +600,14 @@ def test_resolver_nestesd_subproject_and_translation(self): users=[self.owner], main_language_project=None, ) - self.translation.add_subproject(subproject) + translation.add_subproject(subproject) with override_settings(USE_SUBDOMAIN=False): - url = resolve(project=self.translation) - self.assertEqual(url, 'http://readthedocs.org/docs/pip/ja/latest/') + url = resolve(project=subproject) + self.assertEqual(url, 'http://readthedocs.org/docs/docs-es/projects/api-es/es/latest/') with override_settings(USE_SUBDOMAIN=True): - url = resolve(project=self.translation) - self.assertEqual(url, 'http://pip.readthedocs.org/ja/latest/') + url = resolve(project=subproject) + self.assertEqual(url, 'http://docs-es.readthedocs.org/projects/api-es/es/latest/') @override_settings(PRODUCTION_DOMAIN='readthedocs.org') def test_resolver_single_version(self): From 93d838f9c467bf93ac93d3c931f1b163efeae687 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 6 Apr 2020 19:06:53 -0500 Subject: [PATCH 06/10] Set main_language_project --- readthedocs/rtd_tests/tests/test_resolver.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/readthedocs/rtd_tests/tests/test_resolver.py b/readthedocs/rtd_tests/tests/test_resolver.py index 799a65e7c75..d28cf4ea2d5 100644 --- a/readthedocs/rtd_tests/tests/test_resolver.py +++ b/readthedocs/rtd_tests/tests/test_resolver.py @@ -593,6 +593,9 @@ def test_resolver_nested_subproject_and_translation(self): users=[self.owner], main_language_project=None, ) + translation.main_language_project = project + translation.save() + subproject = fixture.get( Project, slug='api-es', From d763e88d9ccb21c091d2eb16f24167499eb915c5 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 6 Apr 2020 20:01:54 -0500 Subject: [PATCH 07/10] Don't support subprojects of translations --- readthedocs/core/resolver.py | 2 ++ readthedocs/rtd_tests/tests/test_resolver.py | 15 ++++++++------- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/readthedocs/core/resolver.py b/readthedocs/core/resolver.py index e3bc7fef02e..c353afd3925 100644 --- a/readthedocs/core/resolver.py +++ b/readthedocs/core/resolver.py @@ -229,6 +229,8 @@ def _get_canonical_project_data(self, project): We serve the documentation from the domain of the super project (the translation), this is docs-es.docs.com/projects/api-es/es/. + Note: we aren't going to support this case for now. + In summary: If the project is a subproject, we don't care if the superproject is a translation, we always serve from the domain of the superproject. diff --git a/readthedocs/rtd_tests/tests/test_resolver.py b/readthedocs/rtd_tests/tests/test_resolver.py index d28cf4ea2d5..c66ecedf3bc 100644 --- a/readthedocs/rtd_tests/tests/test_resolver.py +++ b/readthedocs/rtd_tests/tests/test_resolver.py @@ -1,14 +1,16 @@ -import django_dynamic_fixture as fixture from unittest import mock + +import django_dynamic_fixture as fixture +import pytest from django.test import TestCase, override_settings +from readthedocs.builds.constants import EXTERNAL from readthedocs.core.resolver import ( Resolver, resolve, resolve_domain, resolve_path, ) -from readthedocs.builds.constants import EXTERNAL from readthedocs.projects.constants import PRIVATE from readthedocs.projects.models import Domain, Project, ProjectRelationship from readthedocs.rtd_tests.utils import create_user @@ -555,7 +557,7 @@ def test_resolver_translation(self): self.assertEqual(url, 'http://pip.readthedocs.org/ja/latest/') @override_settings(PRODUCTION_DOMAIN='readthedocs.org') - def test_resolver_nested_translation_and_subproject(self): + def test_resolver_nested_translation_of_a_subproject(self): """The project is a translation, and the main translation is a subproject of a project.""" translation = fixture.get( Project, @@ -576,8 +578,9 @@ def test_resolver_nested_translation_and_subproject(self): url, 'http://pip.readthedocs.org/projects/sub/es/latest/', ) + @pytest.mark.xfail('We do not support this for now', strict=True) @override_settings(PRODUCTION_DOMAIN='readthedocs.org') - def test_resolver_nested_subproject_and_translation(self): + def test_resolver_nested_subproject_of_a_translation(self): """The project is a subproject, and the superproject is a translation of a project.""" project = fixture.get( Project, @@ -591,10 +594,8 @@ def test_resolver_nested_subproject_and_translation(self): slug='docs-es', language='es', users=[self.owner], - main_language_project=None, + main_language_project=project, ) - translation.main_language_project = project - translation.save() subproject = fixture.get( Project, From a4b42822e2bcbd8a4b7254e1dece69ba4e627b55 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 6 Apr 2020 20:06:51 -0500 Subject: [PATCH 08/10] Fix test --- readthedocs/rtd_tests/tests/test_resolver.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/rtd_tests/tests/test_resolver.py b/readthedocs/rtd_tests/tests/test_resolver.py index c66ecedf3bc..5e03a7e5709 100644 --- a/readthedocs/rtd_tests/tests/test_resolver.py +++ b/readthedocs/rtd_tests/tests/test_resolver.py @@ -578,7 +578,7 @@ def test_resolver_nested_translation_of_a_subproject(self): url, 'http://pip.readthedocs.org/projects/sub/es/latest/', ) - @pytest.mark.xfail('We do not support this for now', strict=True) + @pytest.mark.xfail(reason='We do not support this for now', strict=True) @override_settings(PRODUCTION_DOMAIN='readthedocs.org') def test_resolver_nested_subproject_of_a_translation(self): """The project is a subproject, and the superproject is a translation of a project.""" From 489d6b183ccba8c76bf772fcbd89d4e040269f7d Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 6 Apr 2020 20:55:46 -0500 Subject: [PATCH 09/10] Remove tests --- readthedocs/rtd_tests/tests/test_resolver.py | 26 -------------------- 1 file changed, 26 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_resolver.py b/readthedocs/rtd_tests/tests/test_resolver.py index 5e03a7e5709..a74d9c5a9de 100644 --- a/readthedocs/rtd_tests/tests/test_resolver.py +++ b/readthedocs/rtd_tests/tests/test_resolver.py @@ -259,32 +259,6 @@ def test_resolver_force_language_version(self): ) self.assertEqual(url, '/cz/foo/index.html') - def test_resolver_no_force_translation(self): - with override_settings(USE_SUBDOMAIN=False): - url = resolve_path( - project=self.translation, filename='index.html', language='cz', - ) - self.assertEqual(url, '/docs/pip/ja/latest/index.html') - with override_settings(USE_SUBDOMAIN=True): - url = resolve_path( - project=self.translation, filename='index.html', language='cz', - ) - self.assertEqual(url, '/ja/latest/index.html') - - def test_resolver_no_force_translation_with_version(self): - with override_settings(USE_SUBDOMAIN=False): - url = resolve_path( - project=self.translation, filename='index.html', language='cz', - version_slug='foo', - ) - self.assertEqual(url, '/docs/pip/ja/foo/index.html') - with override_settings(USE_SUBDOMAIN=True): - url = resolve_path( - project=self.translation, filename='index.html', language='cz', - version_slug='foo', - ) - self.assertEqual(url, '/ja/foo/index.html') - class ResolverCanonicalProject(TestCase): From 58564a7b1b215c6a8e747522eb4b0aac76e475aa Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 7 Apr 2020 15:57:15 -0500 Subject: [PATCH 10/10] Suggestion from review --- readthedocs/core/resolver.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/readthedocs/core/resolver.py b/readthedocs/core/resolver.py index c353afd3925..51330bdc04f 100644 --- a/readthedocs/core/resolver.py +++ b/readthedocs/core/resolver.py @@ -213,23 +213,22 @@ def _get_canonical_project_data(self, project): - The project is a translation, and the main translation is a subproject of a project, like: - - docs.docs.com - - api.docs.com (subproject of ``docs``) - - api-es.docs.com (translation of ``api``, and current project to be served) + - docs + - api (subproject of ``docs``) + - api-es (translation of ``api``, and current project to be served) We serve the documentation from the domain of the super project (docs.docs.com/projects/api/es/). - The project is a subproject, and the superproject is a translation of a project, like: - - docs.docs.com/ - - docs-es.docs.com/ (translation of ``docs``) - - api-es.docs.com/ (subproject of ``docs-es``, and current project to be served) + - docs + - docs-es (translation of ``docs``) + - api-es (subproject of ``docs-es``, and current project to be served) We serve the documentation from the domain of the super project (the translation), this is docs-es.docs.com/projects/api-es/es/. - - Note: we aren't going to support this case for now. + We aren't going to support this case for now. In summary: If the project is a subproject, we don't care if the superproject is a translation,