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.

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

humitos
Copy link
Member

@humitos humitos commented Nov 25, 2024

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.

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.
@humitos humitos requested a review from a team as a code owner November 25, 2024 12:05
@humitos humitos requested a review from stsewd November 25, 2024 12:05
@humitos humitos enabled auto-merge (squash) November 25, 2024 16:02
Comment on lines +169 to +170
create_manifest = version.project.addons.filetreediff_enabled and (
version.is_external or version.slug == LATEST
Copy link
Member

Choose a reason for hiding this comment

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

Looks like test are failing because projects don't have an addons object. I think we should create one for each project on a post save signal, otherwise we will see that error in production as well.

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 thought we had this in place already 🤔 . I know we are automatically creating an AddonsConfig somewhere..., but I don't remember wehere.

@@ -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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants