Skip to content
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.

Sign up for GitHub

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

File tree diff: migrate feature flag to model field #11793

Merged
merged 6 commits into from
Nov 27, 2024
Merged

File tree diff: migrate feature flag to model field #11793

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4827478
File tree diff: migrate feature flag to model field
humitos Nov 25, 2024
a4bdf67
Update migration
humitos Nov 25, 2024
7de7658
Update tests
humitos Nov 25, 2024
c4d822f
Update tests
humitos Nov 25, 2024
0e5ce09
Merge branch 'main' into humitos/featureflag-to-field
humitos Nov 27, 2024
e5a50bf
Addons: base version to compare against (DocDiff and File Tree Diff) …
humitos Nov 27, 2024
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
37 changes: 37 additions & 0 deletions readthedocs/projects/migrations/0139_addons_filetreediff_field.py
View file
Open in desktop
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),
]
8 changes: 3 additions & 5 deletions readthedocs/projects/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,9 @@ class AddonsConfig(TimeStampedModel):
# EthicalAds
ethicalads_enabled = models.BooleanField(default=True)

# File Tree Diff
filetreediff_enabled = models.BooleanField(default=False, null=True, blank=True)
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we probably want to discuss the branding here before we bake it into the models. I feel like File Tree Diff is a weird name, and we probably want something a bit more user friendly.

I know we talked about Files changed being joint branding for docdiff & FTD, but I'm open to other ideas. We could always just have Files changed be a top-level concept for a workflow that includes docdiff and filetreediff?

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 like the "generic name" Files changed to involve both features: docdiff and file tree diff. Due to each addon being very specific (but related, tho), I'm not able to find better names for each of them. Both names are not ideal for marketing purposes; but are pretty descriptive of what they are... for a developer at least 😅

I think it's hard to explain what they do in a simple and nice name:

  • Documentation page changes
  • Files changed between versions

Copy link
Member Author

Choose a reason for hiding this comment

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

@agjohnson @stsewd do you have any idea/suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

Documentation diff / DocDiff is also something we could use as overarching branding if we wanted, and is at least "brandable".

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I've also been advocating for not using file tree diff to describe this addon or the feature. I think it makes the most sense for both of these features, and the addons and UI, to be combined.

Files changed at least describes the addon now. I don't feel it makes sense as a standalone addon that users can configure though.

Before we ship this, it feels like these should be a single, unified addon/feature. I don't particular like DocDiff for this. Diff view is short and clear, Documentation diff works too. Something closer to that works best I think.

Copy link
Member Author

@humitos humitos Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We could use visualdiff for docdiff and fileschanged for filetreediff. Those names are pretty explicit about what they are and I think they work well for marketing as well. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

He, we are already using Visual Diff in the new dashboard actually:

Screenshot_2024-11-26_18-38-46

Copy link
Contributor

@agjohnson agjohnson Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think visual diff covers both well, I don't see a long term reason to use two names/addons/UIs.

Short term, I don't have opinions about not using filechanged but I would not surface this name in configuration/UI/marketing either. It should just be a single feature.

Copy link
Member Author

@humitos humitos Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

OK. I will keep docdiff and filetreediff for the models and code names because we are already using those names in our code. When we expose this to users, I suppose we will use "Visual diff" as we are already doing, and maybe "Files changed" if we have 2 different features.

I don't want to rename the model fields to use fileschanged if we are already using filetreediff for the API response, for the function that calculate the diff, etc. It will just add more confusion to the code.

I'm happy to rename the code methods, functions, model fields, if we finally find a name that we like and we are convinced about 😄


# Flyout
flyout_enabled = models.BooleanField(default=True)
flyout_sorting = models.CharField(
Expand Down Expand Up @@ -1914,7 +1917,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 +1977,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
7 changes: 3 additions & 4 deletions readthedocs/projects/tasks/search.py
View file
Open in desktop
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,9 @@ 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
create_manifest = version.project.addons.filetreediff_enabled and (
version.is_external or version.slug == LATEST
)
create_manifest = has_feature and (version.is_external or version.slug == LATEST)
if create_manifest:
file_manifest_indexer = FileManifestIndexer(
version=version,
Expand Down
20 changes: 5 additions & 15 deletions readthedocs/proxito/tests/test_hosting.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
PUBLIC,
SINGLE_VERSION_WITHOUT_TRANSLATIONS,
)
from readthedocs.projects.models import AddonsConfig, Domain, Feature, Project
from readthedocs.projects.models import AddonsConfig, Domain, Project


@override_settings(
Expand Down Expand Up @@ -57,6 +57,8 @@ def setUp(self):
versioning_scheme=MULTIPLE_VERSIONS_WITH_TRANSLATIONS,
)

fixture.get(AddonsConfig, project=self.project)

for tag in ("tag", "project", "test"):
self.project.tags.add(tag)

Expand Down Expand Up @@ -138,10 +140,6 @@ 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
Expand Down Expand Up @@ -723,11 +721,6 @@ def test_custom_domain_url(self):
)

def test_linkpreviews(self):
addons = fixture.get(
AddonsConfig,
project=self.project,
)

addons.linkpreviews_enabled = True
addons.save()

Expand Down Expand Up @@ -895,11 +888,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,
Expand Down
4 changes: 2 additions & 2 deletions readthedocs/proxito/views/hosting.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 AddonsConfig, Project
from readthedocs.projects.version_handling import (
comparable_version,
sort_versions_calver,
Expand Down Expand Up @@ -637,7 +637,7 @@ 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()
Expand Down
13 changes: 6 additions & 7 deletions readthedocs/rtd_tests/tests/test_imported_file.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 AddonsConfig, HTMLFile, ImportedFile, Project
from readthedocs.projects.tasks.search import index_build
from readthedocs.search.documents import PageDocument

Expand All @@ -25,6 +25,7 @@ class ImportedFileTests(TestCase):

def setUp(self):
self.project = get(Project)
get(AddonsConfig, project=self.project)
self.version = self.project.versions.get(slug=LATEST)
self.build = get(
Build,
Expand Down Expand Up @@ -352,14 +353,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,
Expand Down Expand Up @@ -401,5 +399,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)