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

Addons: update form to show all the options #11031

Merged
merged 10 commits into from
Jan 22, 2024
Merged

Addons: update form to show all the options #11031

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
919d10a
Addons: update form to show all the options
humitos Jan 15, 2024
b6f51e8
Addons: create an `AddonsConfig` for projects automatically
humitos Jan 16, 2024
7dfe3a3
Addons: remove features related to them
humitos Jan 16, 2024
984277f
Create `AddonsConfig` object automatically
humitos Jan 18, 2024
1382a8b
Remove the Crispy layout from Python code
humitos Jan 18, 2024
d65bb87
Update tests
humitos Jan 18, 2024
2c37afe
Typo
humitos Jan 18, 2024
133dd49
Missing save the object
humitos Jan 18, 2024
38d7f19
Make form label more explicit
humitos Jan 19, 2024
af95aff
Make label more explicit
humitos Jan 22, 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
24 changes: 17 additions & 7 deletions readthedocs/projects/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,17 +529,27 @@ class AddonsConfigForm(forms.ModelForm):

class Meta:
model = AddonsConfig
fields = ("enabled", "project")
fields = (
"enabled",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say the only thing I noticed is that this field renders as Enabled, and should probably be more descriptive regardless of the pattern we use -- Enable Addons or something.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good point. I originally used the layout approach and added some text explaining this was "global". I'll check with the serializer if I can change how it renders since we are already using the help_text to explain the same thing.

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 also thought we can use better labels for:

            "external_version_warning_enabled": _("Show a notification on builds from Pull Requests"),
            "stable_latest_version_warning_enabled": _("Show a notification on non-stable and latest versions"),

What do you think?

Also also, we can remove the enabled part from all of them and just leave the name of the addon with the checkbox?

Copy link
Member Author

Choose a reason for hiding this comment

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

Screenshot_2024-01-19_15-43-50

Copy link
Contributor

@agjohnson agjohnson Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I also thought we can use better labels for

Yeah those look better! Just a small note that "Pull Request" should use proper noun capitalization, "pull request builds" can be used instead. I follow what GitHub uses: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests

Also also, we can remove the enabled part

Aye, we could for this UI. Though thinking ahead to when we split it up to multiple pages/tabs, we might want the full verbose version "Flyout enabled"? I suppose I'd leave the "enabled" part on the text for now and if we want to remove it later we can. For now, the extra enabled isn't making the options any less clear.

"project",
"analytics_enabled",
"doc_diff_enabled",
"external_version_warning_enabled",
"flyout_enabled",
"hotkeys_enabled",
"search_enabled",
"stable_latest_version_warning_enabled",
)

def __init__(self, *args, **kwargs):
self.project = kwargs.pop("project", None)
kwargs["instance"] = getattr(self.project, "addons", None)
super().__init__(*args, **kwargs)
addons, created = AddonsConfig.objects.get_or_create(project=self.project)
if created:
addons.enabled = False
addons.save()

try:
self.fields["enabled"].initial = self.project.addons.enabled
except AddonsConfig.DoesNotExist:
self.fields["enabled"].initial = False
kwargs["instance"] = addons
super().__init__(*args, **kwargs)

def clean_project(self):
return self.project
Expand Down
58 changes: 0 additions & 58 deletions readthedocs/projects/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1977,25 +1977,6 @@ def add_features(sender, **kwargs):
# Build related features
SCALE_IN_PROTECTION = "scale_in_prtection"

# Addons related features
HOSTING_INTEGRATIONS = "hosting_integrations"
# NOTE: this is mainly temporal while we are rolling these features out.
# The idea here is to have more control over particular projects and do some testing.
# All these features will be enabled by default to all projects,
# and we can disable them if we want to
ADDONS_ANALYTICS_DISABLED = "addons_analytics_disabled"
ADDONS_DOC_DIFF_DISABLED = "addons_doc_diff_disabled"
ADDONS_ETHICALADS_DISABLED = "addons_ethicalads_disabled"
ADDONS_EXTERNAL_VERSION_WARNING_DISABLED = (
"addons_external_version_warning_disabled"
)
ADDONS_FLYOUT_DISABLED = "addons_flyout_disabled"
ADDONS_NON_LATEST_VERSION_WARNING_DISABLED = (
"addons_non_latest_version_warning_disabled"
)
ADDONS_SEARCH_DISABLED = "addons_search_disabled"
ADDONS_HOTKEYS_DISABLED = "addons_hotkeys_disabled"

FEATURES = (
(
MKDOCS_THEME_RTD,
Expand Down Expand Up @@ -2093,45 +2074,6 @@ def add_features(sender, **kwargs):
SCALE_IN_PROTECTION,
_("Build: Set scale-in protection before/after building."),
),
# Addons related features.
(
HOSTING_INTEGRATIONS,
_(
"Proxito: Inject 'readthedocs-addons.js' as <script> HTML tag in responses."
),
),
(
ADDONS_ANALYTICS_DISABLED,
_("Addons: Disable Analytics."),
),
(
ADDONS_DOC_DIFF_DISABLED,
_("Addons: Disable Doc Diff."),
),
(
ADDONS_ETHICALADS_DISABLED,
_("Addons: Disable EthicalAds."),
),
(
ADDONS_EXTERNAL_VERSION_WARNING_DISABLED,
_("Addons: Disable External version warning."),
),
(
ADDONS_FLYOUT_DISABLED,
_("Addons: Disable Flyout."),
),
(
ADDONS_NON_LATEST_VERSION_WARNING_DISABLED,
_("Addons: Disable Non latest version warning."),
),
(
ADDONS_SEARCH_DISABLED,
_("Addons: Disable Search."),
),
(
ADDONS_HOTKEYS_DISABLED,
_("Addons: Disable Hotkeys."),
),
)

FEATURES = sorted(FEATURES, key=lambda l: l[1])
Expand Down
58 changes: 18 additions & 40 deletions readthedocs/proxito/tests/test_hosting.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
PUBLIC,
SINGLE_VERSION_WITHOUT_TRANSLATIONS,
)
from readthedocs.projects.models import Domain, Feature, Project
from readthedocs.projects.models import Domain, Project


@override_settings(
Expand Down Expand Up @@ -128,42 +128,20 @@ def test_get_config_unsupported_version(self):
assert r.status_code == 400
assert r.json() == self._get_response_dict("v2")

def test_disabled_addons_via_feature_flags(self):
fixture.get(
Feature,
projects=[self.project],
feature_id=Feature.ADDONS_ANALYTICS_DISABLED,
)
fixture.get(
Feature,
projects=[self.project],
feature_id=Feature.ADDONS_EXTERNAL_VERSION_WARNING_DISABLED,
)
fixture.get(
Feature,
projects=[self.project],
feature_id=Feature.ADDONS_NON_LATEST_VERSION_WARNING_DISABLED,
)
fixture.get(
Feature,
projects=[self.project],
feature_id=Feature.ADDONS_DOC_DIFF_DISABLED,
)
fixture.get(
Feature,
projects=[self.project],
feature_id=Feature.ADDONS_FLYOUT_DISABLED,
)
fixture.get(
Feature,
projects=[self.project],
feature_id=Feature.ADDONS_SEARCH_DISABLED,
)
fixture.get(
Feature,
projects=[self.project],
feature_id=Feature.ADDONS_HOTKEYS_DISABLED,
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.stable_latest_version_warning_enabled = False
addons.save()

r = self.client.get(
reverse("proxito_readthedocs_docs_addons"),
Expand Down Expand Up @@ -678,7 +656,7 @@ def test_number_of_queries_project_version_slug(self):
active=True,
)

with self.assertNumQueries(17):
with self.assertNumQueries(20):
r = self.client.get(
reverse("proxito_readthedocs_docs_addons"),
{
Expand Down Expand Up @@ -707,7 +685,7 @@ def test_number_of_queries_url(self):
active=True,
)

with self.assertNumQueries(17):
with self.assertNumQueries(20):
r = self.client.get(
reverse("proxito_readthedocs_docs_addons"),
{
Expand Down Expand Up @@ -743,7 +721,7 @@ def test_number_of_queries_url_subproject(self):
active=True,
)

with self.assertNumQueries(21):
with self.assertNumQueries(24):
r = self.client.get(
reverse("proxito_readthedocs_docs_addons"),
{
Expand All @@ -769,7 +747,7 @@ def test_number_of_queries_url_translations(self):
language=language,
)

with self.assertNumQueries(21):
with self.assertNumQueries(24):
r = self.client.get(
reverse("proxito_readthedocs_docs_addons"),
{
Expand Down
35 changes: 12 additions & 23 deletions readthedocs/proxito/views/hosting.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
from readthedocs.core.resolver import Resolver
from readthedocs.core.unresolver import UnresolverError, unresolver
from readthedocs.core.utils.extend import SettingsOverrideObject
from readthedocs.projects.models import Feature, Project
from readthedocs.projects.models import AddonsConfig, Project

log = structlog.get_logger(__name__) # noqa

Expand Down Expand Up @@ -280,15 +280,9 @@ def _v0(self, project, version, build, filename, url, user):
# en (original), es, ru
project_translations = itertools.chain([main_project], project_translations)

# Make one DB query here and then check on Python code
# TODO: make usage of ``Project.addons.<name>_enabled`` to decide if enabled
#
# NOTE: using ``feature_id__startswith="addons_"`` to make the query faster.
# It went down from 20ms to 1ms since it does not have to check the
# `Project.pub_date` against all the features.
project_features = project.features.filter(
feature_id__startswith="addons_"
).values_list("feature_id", flat=True)
# Automatically create an AddonsConfig with the default values for
# projects that don't have one already
AddonsConfig.objects.get_or_create(project=project)

data = {
"api_version": "0",
Expand Down Expand Up @@ -320,24 +314,21 @@ def _v0(self, project, version, build, filename, url, user):
# serializer than the keys ``project``, ``version`` and ``build`` from the top level.
"addons": {
"analytics": {
"enabled": Feature.ADDONS_ANALYTICS_DISABLED
not in project_features,
"enabled": project.addons.analytics_enabled,
# TODO: consider adding this field into the ProjectSerializer itself.
# NOTE: it seems we are removing this feature,
# so we may not need the ``code`` attribute here
# https://github.com/readthedocs/readthedocs.org/issues/9530
"code": project.analytics_code,
},
"external_version_warning": {
"enabled": Feature.ADDONS_EXTERNAL_VERSION_WARNING_DISABLED
not in project_features,
"enabled": project.addons.external_version_warning_enabled,
# NOTE: I think we are moving away from these selectors
# since we are doing floating noticications now.
# "query_selector": "[role=main]",
},
"non_latest_version_warning": {
"enabled": Feature.ADDONS_NON_LATEST_VERSION_WARNING_DISABLED
not in project_features,
"enabled": project.addons.stable_latest_version_warning_enabled,
# NOTE: I think we are moving away from these selectors
# since we are doing floating noticications now.
# "query_selector": "[role=main]",
Expand All @@ -346,7 +337,7 @@ def _v0(self, project, version, build, filename, url, user):
),
},
"flyout": {
"enabled": Feature.ADDONS_FLYOUT_DISABLED not in project_features,
"enabled": project.addons.flyout_enabled,
"translations": [
{
# TODO: name this field "display_name"
Expand Down Expand Up @@ -398,7 +389,7 @@ def _v0(self, project, version, build, filename, url, user):
# },
},
"search": {
"enabled": Feature.ADDONS_SEARCH_DISABLED not in project_features,
"enabled": project.addons.search_enabled,
"project": project.slug,
"version": version.slug if version else None,
"api_endpoint": "/_/api/v3/search/",
Expand All @@ -416,7 +407,7 @@ def _v0(self, project, version, build, filename, url, user):
else None,
},
"hotkeys": {
"enabled": Feature.ADDONS_HOTKEYS_DISABLED not in project_features,
"enabled": project.addons.hotkeys_enabled,
"doc_diff": {
"enabled": True,
"trigger": "KeyD", # Could be something like "Ctrl + D"
Expand All @@ -437,8 +428,7 @@ def _v0(self, project, version, build, filename, url, user):
data["addons"].update(
{
"doc_diff": {
"enabled": Feature.ADDONS_DOC_DIFF_DISABLED
not in project_features,
"enabled": project.addons.doc_diff_enabled,
# "http://test-builds-local.devthedocs.org/en/latest/index.html"
"base_url": resolver.resolve(
project=project,
Expand Down Expand Up @@ -478,8 +468,7 @@ def _v0(self, project, version, build, filename, url, user):
data["addons"].update(
{
"ethicalads": {
"enabled": Feature.ADDONS_ETHICALADS_DISABLED
not in project_features,
"enabled": project.addons.ethicalads_enabled,
# NOTE: this endpoint is not authenticated, the user checks are done over an annonymous user for now
#
# NOTE: it requires ``settings.USE_PROMOS=True`` to return ``ad_free=false`` here
Expand Down