From 61129590c1c4993c61fb4a7d7e174c9d23c347e5 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 27 Nov 2024 13:22:35 +0100 Subject: [PATCH] File tree diff: migrate feature flag to model field (#11793) There is no need to use a feature flag for this. We can use a model field as we are doing for all the other addons. As we are not exposing this field to the user yet, they can't enable it by themselves yet. So, we still have the control of it. This follows the pattern we have for all the other addons. --- readthedocs/projects/forms.py | 18 +----- .../0139_addons_filetreediff_field.py | 37 ++++++++++++ .../0140_addons_options_base_version.py | 27 +++++++++ .../migrations/0141_create_addonsconfig.py | 29 ++++++++++ readthedocs/projects/models.py | 15 +++-- readthedocs/projects/signals.py | 34 ++--------- readthedocs/projects/tasks/search.py | 12 ++-- .../projects/tests/test_version_handling.py | 3 +- readthedocs/projects/views/private.py | 4 -- readthedocs/proxito/tests/responses/v1.json | 4 +- readthedocs/proxito/tests/test_headers.py | 4 +- readthedocs/proxito/tests/test_hosting.py | 56 ++++++++----------- readthedocs/proxito/views/hosting.py | 38 ++++++------- .../rtd_tests/tests/test_imported_file.py | 12 ++-- 14 files changed, 165 insertions(+), 128 deletions(-) create mode 100644 readthedocs/projects/migrations/0139_addons_filetreediff_field.py create mode 100644 readthedocs/projects/migrations/0140_addons_options_base_version.py create mode 100644 readthedocs/projects/migrations/0141_create_addonsconfig.py diff --git a/readthedocs/projects/forms.py b/readthedocs/projects/forms.py index b51ed12053e..e37e4ffd73c 100644 --- a/readthedocs/projects/forms.py +++ b/readthedocs/projects/forms.py @@ -1,6 +1,5 @@ """Project forms.""" -import datetime import json from random import choice from re import fullmatch @@ -8,14 +7,12 @@ import dns.name import dns.resolver -import pytz from allauth.socialaccount.models import SocialAccount from django import forms from django.conf import settings from django.contrib.auth.models import User from django.db.models import Q from django.urls import reverse -from django.utils import timezone from django.utils.translation import gettext_lazy as _ from readthedocs.builds.constants import INTERNAL @@ -689,22 +686,11 @@ class Meta: def __init__(self, *args, **kwargs): self.project = kwargs.pop("project", None) - - tzinfo = pytz.timezone("America/Los_Angeles") - addons_enabled_by_default = timezone.now() > datetime.datetime( - 2024, 10, 7, 0, 0, 0, tzinfo=tzinfo - ) - - addons, created = AddonsConfig.objects.get_or_create(project=self.project) - if created: - addons.enabled = addons_enabled_by_default - addons.save() - - kwargs["instance"] = addons + kwargs["instance"] = self.project.addons super().__init__(*args, **kwargs) # Keep the ability to disable addons completely on Read the Docs for Business - if not settings.RTD_ALLOW_ORGANIZATIONS and addons_enabled_by_default: + if not settings.RTD_ALLOW_ORGANIZATIONS: self.fields["enabled"].disabled = True def clean(self): diff --git a/readthedocs/projects/migrations/0139_addons_filetreediff_field.py b/readthedocs/projects/migrations/0139_addons_filetreediff_field.py new file mode 100644 index 00000000000..4fb8925ebe5 --- /dev/null +++ b/readthedocs/projects/migrations/0139_addons_filetreediff_field.py @@ -0,0 +1,37 @@ +# Generated by Django 4.2.16 on 2024-11-25 11:57 + +from django.db import migrations, models +from django_safemigrate import Safe + + +def forwards_func(apps, schema_editor): + """ + Enable FileTreeDiff on those projects with the feature flag enabled. + """ + Feature = apps.get_model("projects", "Feature") + feature = Feature.objects.filter(feature_id="generate_manifest_for_file_tree_diff").first() + if feature: + for project in feature.projects.all().iterator(): + project.addons.filetreediff_enabled = True + project.addons.save() + +class Migration(migrations.Migration): + safe = Safe.before_deploy + + dependencies = [ + ('projects', '0138_remove_old_fields'), + ] + + operations = [ + migrations.AddField( + model_name='addonsconfig', + name='filetreediff_enabled', + field=models.BooleanField(blank=True, default=False, null=True), + ), + migrations.AddField( + model_name='historicaladdonsconfig', + name='filetreediff_enabled', + field=models.BooleanField(blank=True, default=False, null=True), + ), + migrations.RunPython(forwards_func), + ] diff --git a/readthedocs/projects/migrations/0140_addons_options_base_version.py b/readthedocs/projects/migrations/0140_addons_options_base_version.py new file mode 100644 index 00000000000..adfc81d28cb --- /dev/null +++ b/readthedocs/projects/migrations/0140_addons_options_base_version.py @@ -0,0 +1,27 @@ +# Generated by Django 4.2.16 on 2024-11-25 12:15 + +from django.db import migrations, models +import django.db.models.deletion +from django_safemigrate import Safe + + +class Migration(migrations.Migration): + safe = Safe.before_deploy + + dependencies = [ + ('builds', '0059_add_version_date_index'), + ('projects', '0139_addons_filetreediff_field'), + ] + + operations = [ + migrations.AddField( + model_name='addonsconfig', + name='options_base_version', + field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, to='builds.version', verbose_name='Base version to compare against (eg. DocDiff, File Tree Diff)'), + ), + migrations.AddField( + model_name='historicaladdonsconfig', + name='options_base_version', + field=models.ForeignKey(blank=True, db_constraint=False, null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='builds.version', verbose_name='Base version to compare against (eg. DocDiff, File Tree Diff)'), + ), + ] diff --git a/readthedocs/projects/migrations/0141_create_addonsconfig.py b/readthedocs/projects/migrations/0141_create_addonsconfig.py new file mode 100644 index 00000000000..947f3bafe34 --- /dev/null +++ b/readthedocs/projects/migrations/0141_create_addonsconfig.py @@ -0,0 +1,29 @@ +# Generated by Django 4.2.16 on 2024-11-26 09:04 + +from django.db import migrations +from django_safemigrate import Safe + + +def forwards_func(apps, schema_editor): + """ + Create ``AddonsConfig`` for those projects without it. + + We were creating these models when the user accesses to the dashboard or the API itself. + Now, we are creating them on a ``post_save`` Django Signal. + """ + AddonsConfig = apps.get_model("projects", "AddonsConfig") + Project = apps.get_model("projects", "Project") + for project in Project.objects.filter(addons__isnull=True).iterator(): + AddonsConfig.objects.get_or_create(project=project) + + +class Migration(migrations.Migration): + safe = Safe.before_deploy + + dependencies = [ + ('projects', '0140_addons_options_base_version'), + ] + + operations = [ + migrations.RunPython(forwards_func), + ] diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index b163282473e..c62e5149a6e 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -171,6 +171,13 @@ class AddonsConfig(TimeStampedModel): # https://github.com/readthedocs/addons/pull/415 options_load_when_embedded = models.BooleanField(default=False) + options_base_version = models.ForeignKey( + "builds.Version", + verbose_name=_("Base version to compare against (eg. DocDiff, File Tree Diff)"), + null=True, + on_delete=models.SET_NULL, + ) + # Analytics # NOTE: we keep analytics disabled by default to save resources. @@ -185,6 +192,9 @@ class AddonsConfig(TimeStampedModel): # EthicalAds ethicalads_enabled = models.BooleanField(default=True) + # File Tree Diff + filetreediff_enabled = models.BooleanField(default=False, null=True, blank=True) + # Flyout flyout_enabled = models.BooleanField(default=True) flyout_sorting = models.CharField( @@ -1914,7 +1924,6 @@ def add_features(sender, **kwargs): RESOLVE_PROJECT_FROM_HEADER = "resolve_project_from_header" USE_PROXIED_APIS_WITH_PREFIX = "use_proxied_apis_with_prefix" ALLOW_VERSION_WARNING_BANNER = "allow_version_warning_banner" - GENERATE_MANIFEST_FOR_FILE_TREE_DIFF = "generate_manifest_for_file_tree_diff" # Versions sync related features SKIP_SYNC_TAGS = "skip_sync_tags" @@ -1975,10 +1984,6 @@ def add_features(sender, **kwargs): ALLOW_VERSION_WARNING_BANNER, _("Dashboard: Allow project to use the version warning banner."), ), - ( - GENERATE_MANIFEST_FOR_FILE_TREE_DIFF, - _("Build: Generate a file manifest for file tree diff."), - ), # Versions sync related features ( SKIP_SYNC_BRANCHES, diff --git a/readthedocs/projects/signals.py b/readthedocs/projects/signals.py index 1fb0db17d3c..4309270a7ce 100644 --- a/readthedocs/projects/signals.py +++ b/readthedocs/projects/signals.py @@ -6,9 +6,7 @@ from django.db.models.signals import post_save from django.dispatch import receiver -from readthedocs.builds.models import Version -from readthedocs.projects.constants import MKDOCS -from readthedocs.projects.models import AddonsConfig +from readthedocs.projects.models import AddonsConfig, Project log = structlog.get_logger(__name__) @@ -24,29 +22,7 @@ files_changed = django.dispatch.Signal() -@receiver(post_save, sender=Version) -def enable_addons_on_new_mkdocs_projects(instance, *args, **kwargs): - """ - Enable Addons on MkDocs projects. - - We removed all the `mkdocs.yml` manipulation that set the theme to `readthedocs` if - undefined and injects JS and CSS files to show the old flyout. - - Now, we are enabling addons by default on MkDocs projects to move forward - with the idea of removing the magic executed on behalves the users and - promote addons more. - - Reference https://github.com/readthedocs/addons/issues/72#issuecomment-1926647293 - """ - version = instance - project = instance.project - - if version.documentation_type == MKDOCS: - config, created = AddonsConfig.objects.get_or_create(project=project) - if created: - log.info( - "Creating AddonsConfig automatically for MkDocs project.", - project_slug=project.slug, - ) - config.enabled = True - config.save() +@receiver(post_save, sender=Project) +def create_addons_on_new_projects(instance, *args, **kwargs): + """Create ``AddonsConfig`` on new projects.""" + AddonsConfig.objects.get_or_create(project=instance) diff --git a/readthedocs/projects/tasks/search.py b/readthedocs/projects/tasks/search.py index 74cbb3adfa8..9c09398aeac 100644 --- a/readthedocs/projects/tasks/search.py +++ b/readthedocs/projects/tasks/search.py @@ -6,7 +6,7 @@ from readthedocs.builds.models import Build, Version from readthedocs.filetreediff import write_manifest from readthedocs.filetreediff.dataclasses import FileTreeDiffFile, FileTreeDiffManifest -from readthedocs.projects.models import Feature, HTMLFile, Project +from readthedocs.projects.models import HTMLFile, Project from readthedocs.projects.signals import files_changed from readthedocs.search.documents import PageDocument from readthedocs.search.utils import index_objects, remove_indexed_files @@ -166,10 +166,14 @@ def _get_indexers(*, version: Version, build: Build, search_index_name=None): # File tree diff is under a feature flag for now, # and we only allow to compare PR previews against the latest version. - has_feature = version.project.has_feature( - Feature.GENERATE_MANIFEST_FOR_FILE_TREE_DIFF + base_version = ( + version.project.addons.options_base_version.slug + if version.project.addons.options_base_version + else LATEST + ) + create_manifest = version.project.addons.filetreediff_enabled and ( + version.is_external or version.slug == base_version ) - create_manifest = has_feature and (version.is_external or version.slug == LATEST) if create_manifest: file_manifest_indexer = FileManifestIndexer( version=version, diff --git a/readthedocs/projects/tests/test_version_handling.py b/readthedocs/projects/tests/test_version_handling.py index 0f70ef16376..5be3c129d96 100644 --- a/readthedocs/projects/tests/test_version_handling.py +++ b/readthedocs/projects/tests/test_version_handling.py @@ -2,7 +2,7 @@ import pytest from readthedocs.builds.models import Build, Version -from readthedocs.projects.models import AddonsConfig, Project +from readthedocs.projects.models import Project from readthedocs.projects.version_handling import ( sort_versions_calver, sort_versions_custom_pattern, @@ -18,7 +18,6 @@ def setup(self, requests_mock): self.requests_mock = requests_mock self.project = fixture.get(Project, slug="project") - self.addons = fixture.get(AddonsConfig, project=self.project) self.version = self.project.versions.get(slug="latest") self.build = fixture.get( Build, diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 9efa0db13af..16a43710ed1 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -74,7 +74,6 @@ WebHookForm, ) from readthedocs.projects.models import ( - AddonsConfig, Domain, EmailHook, EnvironmentVariable, @@ -442,9 +441,6 @@ def done(self, form_list, **kwargs): # attributes directly from other forms project = basics_form.save() - # Create an AddonsConfig object for this project. - AddonsConfig.objects.get_or_create(project=project) - self.finish_import_project(self.request, project) return HttpResponseRedirect( diff --git a/readthedocs/proxito/tests/responses/v1.json b/readthedocs/proxito/tests/responses/v1.json index 8375ae823e1..ccfb8e2696d 100644 --- a/readthedocs/proxito/tests/responses/v1.json +++ b/readthedocs/proxito/tests/responses/v1.json @@ -149,9 +149,7 @@ "doc_diff": { "enabled": true, "base_url": "https://project.dev.readthedocs.io/en/latest/index.html", - "inject_styles": true, - "base_host": "", - "base_page": "" + "inject_styles": true }, "filetreediff": { "enabled": false diff --git a/readthedocs/proxito/tests/test_headers.py b/readthedocs/proxito/tests/test_headers.py index f8ace044030..8a4c199721b 100644 --- a/readthedocs/proxito/tests/test_headers.py +++ b/readthedocs/proxito/tests/test_headers.py @@ -12,7 +12,7 @@ from readthedocs.builds.models import Version from readthedocs.organizations.models import Organization from readthedocs.projects.constants import PRIVATE, PUBLIC -from readthedocs.projects.models import AddonsConfig, Domain, HTTPHeader +from readthedocs.projects.models import Domain, HTTPHeader from .base import BaseDocServing @@ -200,8 +200,6 @@ def test_user_domain_headers(self): self.assertEqual(r[http_header_secure], http_header_value) def test_force_addons_header(self): - fixture.get(AddonsConfig, project=self.project, enabled=True) - r = self.client.get( "/en/latest/", secure=True, headers={"host": "project.dev.readthedocs.io"} ) diff --git a/readthedocs/proxito/tests/test_hosting.py b/readthedocs/proxito/tests/test_hosting.py index 1fc367ec10f..a990fe2a984 100644 --- a/readthedocs/proxito/tests/test_hosting.py +++ b/readthedocs/proxito/tests/test_hosting.py @@ -24,7 +24,7 @@ PUBLIC, SINGLE_VERSION_WITHOUT_TRANSLATIONS, ) -from readthedocs.projects.models import AddonsConfig, Domain, Feature, Project +from readthedocs.projects.models import Domain, Project @override_settings( @@ -138,22 +138,18 @@ def test_get_config_unsupported_version(self): assert r.json() == self._get_response_dict("v3") def test_disabled_addons_via_addons_config(self): - addons = fixture.get( - AddonsConfig, - project=self.project, - ) - addons.analytics_enabled = False - addons.doc_diff_enabled = False - addons.external_version_warning_enabled = False - addons.ethicalads_enabled = False - addons.flyout_enabled = False - addons.hotkeys_enabled = False - addons.search_enabled = False - addons.notifications_enabled = False - addons.notifications_show_on_latest = False - addons.notifications_show_on_non_stable = False - addons.notifications_show_on_external = False - addons.save() + self.project.addons.analytics_enabled = False + self.project.addons.doc_diff_enabled = False + self.project.addons.external_version_warning_enabled = False + self.project.addons.ethicalads_enabled = False + self.project.addons.flyout_enabled = False + self.project.addons.hotkeys_enabled = False + self.project.addons.search_enabled = False + self.project.addons.notifications_enabled = False + self.project.addons.notifications_show_on_latest = False + self.project.addons.notifications_show_on_non_stable = False + self.project.addons.notifications_show_on_external = False + self.project.addons.save() r = self.client.get( reverse("proxito_readthedocs_docs_addons"), @@ -723,13 +719,8 @@ def test_custom_domain_url(self): ) def test_linkpreviews(self): - addons = fixture.get( - AddonsConfig, - project=self.project, - ) - - addons.linkpreviews_enabled = True - addons.save() + self.project.addons.linkpreviews_enabled = True + self.project.addons.save() r = self.client.get( reverse("proxito_readthedocs_docs_addons"), @@ -787,7 +778,7 @@ def test_number_of_queries_project_version_slug(self): active=True, ) - with self.assertNumQueries(24): + with self.assertNumQueries(20): r = self.client.get( reverse("proxito_readthedocs_docs_addons"), { @@ -816,7 +807,7 @@ def test_number_of_queries_url(self): active=True, ) - with self.assertNumQueries(26): + with self.assertNumQueries(22): r = self.client.get( reverse("proxito_readthedocs_docs_addons"), { @@ -852,7 +843,7 @@ def test_number_of_queries_url_subproject(self): active=True, ) - with self.assertNumQueries(35): + with self.assertNumQueries(31): r = self.client.get( reverse("proxito_readthedocs_docs_addons"), { @@ -878,7 +869,7 @@ def test_number_of_queries_url_translations(self): language=language, ) - with self.assertNumQueries(62): + with self.assertNumQueries(58): r = self.client.get( reverse("proxito_readthedocs_docs_addons"), { @@ -895,11 +886,8 @@ def test_number_of_queries_url_translations(self): @mock.patch("readthedocs.filetreediff.get_manifest") def test_file_tree_diff(self, get_manifest): - get( - Feature, - projects=[self.project], - feature_id=Feature.GENERATE_MANIFEST_FOR_FILE_TREE_DIFF, - ) + self.project.addons.filetreediff_enabled = True + self.project.addons.save() pr_version = get( Version, project=self.project, @@ -953,7 +941,7 @@ def test_file_tree_diff(self, get_manifest): ], ), ] - with self.assertNumQueries(30): + with self.assertNumQueries(25): r = self.client.get( reverse("proxito_readthedocs_docs_addons"), { diff --git a/readthedocs/proxito/views/hosting.py b/readthedocs/proxito/views/hosting.py index 4315de518d4..933f359fbec 100644 --- a/readthedocs/proxito/views/hosting.py +++ b/readthedocs/proxito/views/hosting.py @@ -30,7 +30,7 @@ ADDONS_FLYOUT_SORTING_PYTHON_PACKAGING, ADDONS_FLYOUT_SORTING_SEMVER_READTHEDOCS_COMPATIBLE, ) -from readthedocs.projects.models import AddonsConfig, Feature, Project +from readthedocs.projects.models import Project from readthedocs.projects.version_handling import ( comparable_version, sort_versions_calver, @@ -338,10 +338,6 @@ def _v1(self, project, version, build, filename, url, request): sorted_versions_active_built_not_hidden = Version.objects.none() user = request.user - # Automatically create an AddonsConfig with the default values for - # projects that don't have one already - AddonsConfig.objects.get_or_create(project=project) - versions_active_built_not_hidden = ( self._get_versions(request, project) .select_related("project") @@ -574,6 +570,11 @@ def _v1(self, project, version, build, filename, url, request): # If we don't know the filename, we cannot return the data required by DocDiff to work. # In that case, we just don't include the `doc_diff` object in the response. if url: + base_version_slug = ( + project.addons.options_base_version.slug + if project.addons.options_base_version + else LATEST + ) data["addons"].update( { "doc_diff": { @@ -581,20 +582,13 @@ def _v1(self, project, version, build, filename, url, request): # "http://test-builds-local.devthedocs.org/en/latest/index.html" "base_url": resolver.resolve( project=project, - # NOTE: we are using LATEST version to compare against to for now. - # Ideally, this should be configurable by the user. - version_slug=LATEST, + version_slug=base_version_slug, language=project.language, filename=filename, ) if filename else None, "inject_styles": True, - # NOTE: `base_host` and `base_page` are not required, since - # we are constructing the `base_url` in the backend instead - # of the frontend, as the doc-diff extension does. - "base_host": "", - "base_page": "", }, } ) @@ -637,16 +631,18 @@ def _get_filetreediff_response(self, *, request, project, version, resolver): if not version.is_external: return None - if not project.has_feature(Feature.GENERATE_MANIFEST_FOR_FILE_TREE_DIFF): + if not project.addons.filetreediff_enabled: return None - latest_version = project.get_latest_version() - if not latest_version or not self._has_permission( - request=request, version=latest_version + base_version = ( + project.addons.options_base_version or project.get_latest_version() + ) + if not base_version or not self._has_permission( + request=request, version=base_version ): return None - diff = get_diff(version_a=version, version_b=latest_version) + diff = get_diff(version_a=version, version_b=base_version) if not diff: return None @@ -666,7 +662,7 @@ def _get_filetreediff_response(self, *, request, project, version, resolver): "base": resolver.resolve_version( project=project, filename=filename, - version=latest_version, + version=base_version, ), }, } @@ -684,7 +680,7 @@ def _get_filetreediff_response(self, *, request, project, version, resolver): "base": resolver.resolve_version( project=project, filename=filename, - version=latest_version, + version=base_version, ), }, } @@ -702,7 +698,7 @@ def _get_filetreediff_response(self, *, request, project, version, resolver): "base": resolver.resolve_version( project=project, filename=filename, - version=latest_version, + version=base_version, ), }, } diff --git a/readthedocs/rtd_tests/tests/test_imported_file.py b/readthedocs/rtd_tests/tests/test_imported_file.py index 9795780a13d..50af94abc74 100644 --- a/readthedocs/rtd_tests/tests/test_imported_file.py +++ b/readthedocs/rtd_tests/tests/test_imported_file.py @@ -11,7 +11,7 @@ from readthedocs.builds.constants import BUILD_STATE_FINISHED, EXTERNAL, LATEST from readthedocs.builds.models import Build, Version from readthedocs.filetreediff.dataclasses import FileTreeDiffFile, FileTreeDiffManifest -from readthedocs.projects.models import Feature, HTMLFile, ImportedFile, Project +from readthedocs.projects.models import HTMLFile, ImportedFile, Project from readthedocs.projects.tasks.search import index_build from readthedocs.search.documents import PageDocument @@ -352,14 +352,11 @@ def test_update_content(self): def test_create_file_tree_manifest(self, write_manifest): assert self.version.slug == LATEST index_build(self.build.pk) - # Feature flag is not enabled. + # File Tree Diff is not enabled by default write_manifest.assert_not_called() - get( - Feature, - feature_id=Feature.GENERATE_MANIFEST_FOR_FILE_TREE_DIFF, - projects=[self.project], - ) + self.project.addons.filetreediff_enabled = True + self.project.addons.save() index_build(self.build.pk) manifest = FileTreeDiffManifest( build_id=self.build.pk, @@ -401,5 +398,6 @@ def test_create_file_tree_manifest(self, write_manifest): new_version.save() with override_settings(DOCROOT=self.test_dir): self._copy_storage_dir(new_version) + write_manifest.reset_mock() index_build(self.build.pk) write_manifest.assert_called_once_with(new_version, manifest)