diff --git a/readthedocs/api/v3/tests/test_projects.py b/readthedocs/api/v3/tests/test_projects.py index 3bde989e316..41b9f38f731 100644 --- a/readthedocs/api/v3/tests/test_projects.py +++ b/readthedocs/api/v3/tests/test_projects.py @@ -212,7 +212,10 @@ def test_projects_detail_other_user(self): response = self.client.get(url, data) self.assertEqual(response.status_code, 404) - @override_settings(ALLOW_PRIVATE_REPOS=True) + @override_settings( + ALLOW_PRIVATE_REPOS=True, + RTD_ALLOW_ORGANIZATIONS=True, + ) def test_own_projects_detail_privacy_levels_enabled(self): url = reverse( "projects-detail", @@ -227,10 +230,12 @@ def test_own_projects_detail_privacy_levels_enabled(self): self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") response = self.client.get(url, query_params) self.assertEqual(response.status_code, 200) - self.assertDictEqual( - response.json(), - self._get_response_dict("projects-detail"), - ) + + expected = self._get_response_dict("projects-detail") + # No users when organzations are enabled + expected.pop("users") + + self.assertDictEqual(response.json(), expected) self.project.privacy_level = "private" self.project.external_builds_privacy_level = "private" @@ -245,6 +250,10 @@ def test_own_projects_detail_privacy_levels_enabled(self): # We don't care about the modified date. expected.pop("modified") response.pop("modified") + + # No users when organzations are enabled + expected.pop("users") + self.assertDictEqual(response, expected) def test_projects_superproject_anonymous_user(self): diff --git a/readthedocs/api/v3/tests/test_subprojects.py b/readthedocs/api/v3/tests/test_subprojects.py index ab81e6fe5b1..df6b635da78 100644 --- a/readthedocs/api/v3/tests/test_subprojects.py +++ b/readthedocs/api/v3/tests/test_subprojects.py @@ -155,7 +155,6 @@ def test_projects_subprojects_detail_other_user(self): def test_projects_subprojects_list_post(self): newproject = self._create_new_project() - self.organization.projects.add(newproject) self.assertEqual(self.project.subprojects.count(), 1) url = reverse( diff --git a/readthedocs/audit/models.py b/readthedocs/audit/models.py index e800c4d16a4..0cdee013525 100644 --- a/readthedocs/audit/models.py +++ b/readthedocs/audit/models.py @@ -223,7 +223,7 @@ def save(self, **kwargs): if self.project: self.log_project_id = self.project.id self.log_project_slug = self.project.slug - organization = self.project.organizations.first() + organization = self.project.organization if organization: self.organization = organization if self.organization: diff --git a/readthedocs/audit/tests/test_models.py b/readthedocs/audit/tests/test_models.py index 7f1f2dd5d40..d0e0b6d2f2d 100644 --- a/readthedocs/audit/tests/test_models.py +++ b/readthedocs/audit/tests/test_models.py @@ -1,5 +1,5 @@ from django.contrib.auth.models import User -from django.test import TestCase +from django.test import TestCase, override_settings from django_dynamic_fixture import get from readthedocs.audit.models import AuditLog @@ -7,6 +7,7 @@ from readthedocs.projects.models import Project +@override_settings(RTD_ALLOW_ORGANIZATIONS=True) class TestAuditModels(TestCase): def setUp(self): self.user = get(User) diff --git a/readthedocs/audit/tests/test_tasks.py b/readthedocs/audit/tests/test_tasks.py index 7b3efbc4974..2686869ff6f 100644 --- a/readthedocs/audit/tests/test_tasks.py +++ b/readthedocs/audit/tests/test_tasks.py @@ -1,7 +1,7 @@ from unittest import mock from django.contrib.auth.models import User -from django.test import TestCase +from django.test import TestCase, override_settings from django.utils import timezone from django_dynamic_fixture import get @@ -11,6 +11,7 @@ from readthedocs.projects.models import Project +@override_settings(RTD_ALLOW_ORGANIZATIONS=True) class AuditTasksTest(TestCase): def setUp(self): self.user = get(User) diff --git a/readthedocs/builds/querysets.py b/readthedocs/builds/querysets.py index 7bc04fe68bb..c244ee27226 100644 --- a/readthedocs/builds/querysets.py +++ b/readthedocs/builds/querysets.py @@ -246,7 +246,7 @@ def concurrent(self, project): # If the project belongs to an organization, count all the projects # from this organization as well - organization = project.organizations.first() + organization = project.organization if organization: query |= Q(project__in=organization.projects.all()) diff --git a/readthedocs/builds/tests/test_build_queryset.py b/readthedocs/builds/tests/test_build_queryset.py index 856f2b805e4..a08330413cf 100644 --- a/readthedocs/builds/tests/test_build_queryset.py +++ b/readthedocs/builds/tests/test_build_queryset.py @@ -1,3 +1,4 @@ +from django.test import override_settings import django_dynamic_fixture as fixture import pytest @@ -79,6 +80,7 @@ def test_concurrent_builds_translations(self): assert (True, 4, 4) == Build.objects.concurrent(translation) assert (True, 4, 4) == Build.objects.concurrent(project) + @override_settings(RTD_ALLOW_ORGANIZATIONS=True) def test_concurrent_builds_organization(self): organization = fixture.get( Organization, @@ -111,6 +113,7 @@ def test_concurrent_builds_organization(self): ) assert (True, 6, 4) == Build.objects.concurrent(project) + @override_settings(RTD_ALLOW_ORGANIZATIONS=True) def test_concurrent_builds_organization_limited(self): organization = fixture.get( Organization, @@ -138,6 +141,7 @@ def test_concurrent_builds_organization_limited(self): # from ``project_with_builds`` as well assert (False, 2, 10) == Build.objects.concurrent(project_without_builds) + @override_settings(RTD_ALLOW_ORGANIZATIONS=True) def test_concurrent_builds_organization_and_project_limited(self): organization = fixture.get( Organization, diff --git a/readthedocs/core/permissions.py b/readthedocs/core/permissions.py index 0cdc36d1e86..42a058ec13e 100644 --- a/readthedocs/core/permissions.py +++ b/readthedocs/core/permissions.py @@ -118,7 +118,7 @@ def owners(cls, obj): if isinstance(obj, Project): if settings.RTD_ALLOW_ORGANIZATIONS: - obj = obj.organizations.first() + obj = obj.organization else: return obj.users.all() diff --git a/readthedocs/core/resolver.py b/readthedocs/core/resolver.py index 6afd80a0abb..d8d268e31c4 100644 --- a/readthedocs/core/resolver.py +++ b/readthedocs/core/resolver.py @@ -198,11 +198,16 @@ def _get_project_domain(self, project, external_version_slug=None, use_canonical domain = self._get_project_subdomain(canonical_project) if external_version_slug: domain = self._get_external_subdomain(canonical_project, external_version_slug) - elif use_canonical_domain and self._use_cname(canonical_project): + # NOTE: call _use_cname only if the project has a canonical custom domain. + # Calling _use_cname is more expensive when we have organizations. + elif ( + use_canonical_domain + and canonical_project.canonical_custom_domain + and self._use_cname(canonical_project) + ): domain_object = canonical_project.canonical_custom_domain - if domain_object: - use_https = domain_object.https - domain = domain_object.domain + use_https = domain_object.https + domain = domain_object.domain return domain, use_https @@ -365,4 +370,22 @@ def _fix_filename(self, filename): def _use_cname(self, project): """Test if to allow direct serving for project on CNAME.""" + # If the project belongs to an organization, use the other + # method, to allow caching the result for that organization. + if project.organization: + return self._organization_allows_custom_domain(project.organization) + return bool(get_feature(project, feature_type=TYPE_CNAME)) + + @cache + def _organization_allows_custom_domain(self, organization): + """ + Test if the organization allows custom domains. + + .. note:: + + This is done on a separate method, so we can cache the result + for each organization. This is useful when resolving multiple + projects from the same organization. + """ + return bool(get_feature(organization, feature_type=TYPE_CNAME)) diff --git a/readthedocs/invitations/tests/test_views.py b/readthedocs/invitations/tests/test_views.py index 9a3692f646c..fafdf8b08ad 100644 --- a/readthedocs/invitations/tests/test_views.py +++ b/readthedocs/invitations/tests/test_views.py @@ -1,5 +1,5 @@ from django.contrib.auth.models import User -from django.test import TestCase +from django.test import TestCase, override_settings from django.urls import reverse from django.utils import timezone from django_dynamic_fixture import get @@ -10,6 +10,7 @@ from readthedocs.projects.models import Project +@override_settings(RTD_ALLOW_ORGANIZATIONS=True) class TestViews(TestCase): def setUp(self): self.user = get(User) diff --git a/readthedocs/organizations/tasks.py b/readthedocs/organizations/tasks.py index 1c61af81694..8b130909573 100644 --- a/readthedocs/organizations/tasks.py +++ b/readthedocs/organizations/tasks.py @@ -18,7 +18,7 @@ def mark_organization_assets_not_cleaned(build_pk): log.debug("Build does not exist.", build_pk=build_pk) return - organization = build.project.organizations.first() + organization = build.project.organization if organization and organization.artifacts_cleaned: log.info("Marking organization as not cleaned.", origanization_slug=organization.slug) organization.artifacts_cleaned = False diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 63ae7811f27..59bd8360190 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -1490,7 +1490,7 @@ def get_subproject_candidates(self, user): Both projects need to share the same owner/admin. """ - organization = self.organizations.first() + organization = self.organization queryset = ( Project.objects.for_admin_user(user) .filter(organizations=organization) @@ -1500,8 +1500,17 @@ def get_subproject_candidates(self, user): ) return queryset - @property + @cached_property def organization(self): + # If organizations aren't supported, + # we don't need to query the database. + if not settings.RTD_ALLOW_ORGANIZATIONS: + return None + + if hasattr(self, "_organizations"): + if self._organizations: + return self._organizations[0] + return None return self.organizations.first() @property @@ -1780,7 +1789,7 @@ def get_payload(self, version, build, event): return None project = version.project - organization = project.organizations.first() + organization = project.organization organization_name = "" organization_slug = "" diff --git a/readthedocs/projects/querysets.py b/readthedocs/projects/querysets.py index 8b9a2883dd1..fc68d1e722a 100644 --- a/readthedocs/projects/querysets.py +++ b/readthedocs/projects/querysets.py @@ -5,6 +5,7 @@ from django.db.models import Count from django.db.models import Exists from django.db.models import OuterRef +from django.db.models import Prefetch from django.db.models import Q from readthedocs.core.permissions import AdminPermission @@ -85,7 +86,7 @@ def is_active(self, project): """ spam_project = False any_owner_banned = any(u.profile.banned for u in project.users.all()) - organization = project.organizations.first() + organization = project.organization if "readthedocsext.spamfighting" in settings.INSTALLED_APPS: from readthedocsext.spamfighting.utils import spam_score # noqa @@ -123,7 +124,7 @@ def max_concurrent_builds(self, project): from readthedocs.subscriptions.constants import TYPE_CONCURRENT_BUILDS max_concurrent_organization = None - organization = project.organizations.first() + organization = project.organization if organization: max_concurrent_organization = organization.max_concurrent_builds @@ -156,6 +157,33 @@ def prefetch_latest_build(self): _has_good_build=Exists(Build.internal.filter(project=OuterRef("pk"), success=True)) ) + def prefetch_organization(self, select_related: list[str] | None = None): + """ + Prefetch the organizations related to the projects. + + :param select_related: List of related fields to select_related + when fetching the organizations. + + .. note:: + + This would normally be a select_related call, + but since our relationship is ManyToMany, + we need to use prefetch_related. + """ + # If organizations aren't supported, + # we don't need to query the database. + if not settings.RTD_ALLOW_ORGANIZATIONS: + return self + + from readthedocs.organizations.models import Organization + + query = Organization.objects.all() + if select_related: + query = query.select_related(*select_related) + return self.prefetch_related( + Prefetch("organizations", queryset=query, to_attr="_organizations") + ) + # Aliases def dashboard(self, user): diff --git a/readthedocs/rtd_tests/tests/test_project_querysets.py b/readthedocs/rtd_tests/tests/test_project_querysets.py index 0876bc71eea..10423f9a385 100644 --- a/readthedocs/rtd_tests/tests/test_project_querysets.py +++ b/readthedocs/rtd_tests/tests/test_project_querysets.py @@ -2,7 +2,7 @@ import django_dynamic_fixture as fixture from django.contrib.auth.models import User -from django.test import TestCase +from django.test import TestCase, override_settings from django_dynamic_fixture import get from readthedocs.organizations.models import Organization @@ -90,6 +90,7 @@ def test_subproject_queryset_as_manager_gets_correct_class(self): "ManagerFromParentRelatedProjectQuerySet", ) + @override_settings(RTD_ALLOW_ORGANIZATIONS=True) def test_is_active(self): project = get(Project, skip=False) self.assertTrue(Project.objects.is_active(project)) @@ -109,10 +110,14 @@ def test_is_active(self): organization = get(Organization, disabled=False) organization.projects.add(project) + # Clear cached organization + del project.organization self.assertTrue(Project.objects.is_active(project)) organization.disabled = True organization.save() + # Clear cached organization + del project.organization self.assertFalse(Project.objects.is_active(project)) def test_dashboard(self): @@ -188,6 +193,7 @@ def test_for_user_and_viewer_same_user(self): self.assertEqual(query.count(), len(projects)) self.assertEqual(set(query), projects) + @override_settings(RTD_ALLOW_ORGANIZATIONS=True) def test_only_owner(self): user = get(User) another_user = get(User) diff --git a/readthedocs/search/api/v2/serializers.py b/readthedocs/search/api/v2/serializers.py index 836b12551da..9ef92565791 100644 --- a/readthedocs/search/api/v2/serializers.py +++ b/readthedocs/search/api/v2/serializers.py @@ -98,18 +98,19 @@ class PageSearchSerializer(serializers.Serializer): def __init__(self, *args, projects=None, **kwargs): if projects: context = kwargs.setdefault("context", {}) + # NOTE: re-using the resolver for different projects usually doesn't save queries, + # but when projects belong to the same organization, it does. + resolver = Resolver() + context["resolver"] = resolver context["projects_data"] = { - project.slug: self._build_project_data(project, version=version) + project.slug: self._build_project_data(project, version=version, resolver=resolver) for project, version in projects } super().__init__(*args, **kwargs) - def _build_project_data(self, project, version): + def _build_project_data(self, project, version, resolver: Resolver): """Build a `ProjectData` object given a project and its version.""" - # NOTE: re-using the resolver doesn't help here, - # as this method is called just once per project, - # re-using the resolver is useful when resolving the same project multiple times. - url = Resolver().resolve_version(project, version) + url = resolver.resolve_version(project, version) project_alias = None if project.parent_relationship: project_alias = project.parent_relationship.alias @@ -135,6 +136,7 @@ def _get_project_data(self, obj): if project_data: return project_data + resolver = self.context.get("resolver", Resolver()) version = ( Version.objects.filter(project__slug=obj.project, slug=obj.version) .select_related("project") @@ -143,7 +145,9 @@ def _get_project_data(self, obj): if version: project = version.project projects_data = self.context.setdefault("projects_data", {}) - projects_data[obj.project] = self._build_project_data(project, version=version) + projects_data[obj.project] = self._build_project_data( + project, version=version, resolver=resolver + ) return projects_data[obj.project] return None diff --git a/readthedocs/search/api/v3/executor.py b/readthedocs/search/api/v3/executor.py index 8a8ea68df01..e061533c48f 100644 --- a/readthedocs/search/api/v3/executor.py +++ b/readthedocs/search/api/v3/executor.py @@ -129,6 +129,7 @@ def _get_subprojects(self, project, version_slug=None): If `version_slug` is None, we will always use the default version. """ relationships = project.subprojects.select_related("child") + organization = project.organization for relationship in relationships: subproject = relationship.child # NOTE: Since we already have the superproject relationship, @@ -136,6 +137,10 @@ def _get_subprojects(self, project, version_slug=None): # when using Project.parent_relationship property. # The superproject instannce is also shared among all subprojects. subproject._superprojects = [relationship] + # NOTE: Since we already have the organization from the parent project, + # we can set it to each subproject to avoid an extra query later + # when using the Project.organization property. + subproject._organizations = [organization] if organization else [] version = None if version_slug: version = self._get_project_version( @@ -214,7 +219,11 @@ def _split_project_and_version(self, term): def _get_project_and_version(self, value): project_slug, version_slug = self._split_project_and_version(value) - project = Project.objects.filter(slug=project_slug).first() + project = ( + Project.objects.filter(slug=project_slug) + .prefetch_organization(select_related=["stripe_subscription"]) + .first() + ) if not project: return None, None diff --git a/readthedocs/search/api/v3/tests/test_api.py b/readthedocs/search/api/v3/tests/test_api.py index c7324f1d990..c0f16eff7b2 100644 --- a/readthedocs/search/api/v3/tests/test_api.py +++ b/readthedocs/search/api/v3/tests/test_api.py @@ -417,6 +417,7 @@ def test_search_project_number_of_queries_without_search_recording(self): assert resp.status_code == 200 assert resp.data["results"] + @mock.patch("readthedocs.subscriptions.signals.get_stripe_client", new=mock.MagicMock()) def test_search_subprojects_number_of_queries(self): subproject = get( Project, diff --git a/readthedocs/telemetry/models.py b/readthedocs/telemetry/models.py index c06e308e674..4d5d4b6bcab 100644 --- a/readthedocs/telemetry/models.py +++ b/readthedocs/telemetry/models.py @@ -98,7 +98,7 @@ def collect(self, build, data): "id": build.version.id, "slug": build.version.slug, } - org = build.project.organizations.first() + org = build.project.organization if org: data["organization"] = { "id": org.id,