Skip to content

Commit

Permalink
File tree diff: migrate feature flag to model field (#11793)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
humitos authored Nov 27, 2024
1 parent 777eb1a commit 6112959
Show file tree
Hide file tree
Showing 14 changed files with 165 additions and 128 deletions.
18 changes: 2 additions & 16 deletions readthedocs/projects/forms.py
Original file line number Diff line number Diff line change
@@ -1,21 +1,18 @@
"""Project forms."""

import datetime
import json
from random import choice
from re import fullmatch
from urllib.parse import urlparse

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
Expand Down Expand Up @@ -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):
Expand Down
37 changes: 37 additions & 0 deletions readthedocs/projects/migrations/0139_addons_filetreediff_field.py
Original file line number Diff line number Diff line change
@@ -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),
]
Original file line number Diff line number Diff line change
@@ -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)'),
),
]
29 changes: 29 additions & 0 deletions readthedocs/projects/migrations/0141_create_addonsconfig.py
Original file line number Diff line number Diff line change
@@ -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),
]
15 changes: 10 additions & 5 deletions readthedocs/projects/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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(
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand Down
34 changes: 5 additions & 29 deletions readthedocs/projects/signals.py
Original file line number Diff line number Diff line change
Expand Up @@ -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__)

Expand All @@ -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)
12 changes: 8 additions & 4 deletions readthedocs/projects/tasks/search.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
3 changes: 1 addition & 2 deletions readthedocs/projects/tests/test_version_handling.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down
4 changes: 0 additions & 4 deletions readthedocs/projects/views/private.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@
WebHookForm,
)
from readthedocs.projects.models import (
AddonsConfig,
Domain,
EmailHook,
EnvironmentVariable,
Expand Down Expand Up @@ -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(
Expand Down
4 changes: 1 addition & 3 deletions readthedocs/proxito/tests/responses/v1.json
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 1 addition & 3 deletions readthedocs/proxito/tests/test_headers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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"}
)
Expand Down
Loading

0 comments on commit 6112959

Please sign in to comment.