Skip to content

Addons: update form to show all the options #11031

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

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 3 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
78 changes: 77 additions & 1 deletion readthedocs/projects/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,17 @@ 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)
Expand All @@ -541,6 +551,72 @@ def __init__(self, *args, **kwargs):
except AddonsConfig.DoesNotExist:
self.fields["enabled"].initial = False

fieldsets = [
Fieldset(
_("Global"),
"enabled",
),
Fieldset(
_("Analytics"),
HTML(
"<p>Analytics addon allows you to store traffic analytics in your project.</p>"
),
*[field for field in self.fields if field.startswith("analytics_")],
),
Fieldset(
_("DocDiff"),
HTML(
"<p>DocDicc addon helps you to review changes from PR by visually showing the differences.</p>"
),
*[field for field in self.fields if field.startswith("doc_diff_")],
),
Fieldset(
_("Flyout"),
HTML(
"<p>Flyout addon shows a small menu at bottom-right of each page with useful links to switch between versions and translations.</p>"
),
*[field for field in self.fields if field.startswith("flyout_")],
),
Fieldset(
_("Hotkeys"),
HTML(
"<p>Hotkeys addon enables keyboard shortcuts to access other addons quickly.</p>"
),
*[field for field in self.fields if field.startswith("hotkeys_")],
),
Fieldset(
_("Search"),
HTML(
"<p>Search addon improves your search experience with the search-as-you-type feature.</p>"
),
*[field for field in self.fields if field.startswith("search_")],
),
Fieldset(
_("Warning on PR previews"),
HTML("<p>Shows a warning notification on PR previews.</p>"),
*[
field
for field in self.fields
if field.startswith("external_version_warning_")
],
),
Fieldset(
_("Warning on non-stable/latest versions"),
HTML(
"<p>Shows a warning notification stable and latest versions letting the user know they may be reading an outdated page or a non-released one.</p>"
),
*[
field
for field in self.fields
if field.startswith("stable_latest_version_warning_")
],
),
]
Copy link
Contributor

@agjohnson agjohnson Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Noted at readthedocs/ext-theme#262 (comment)

Two things that are important here:

First, I think this interface will eventually use a tabbed interface, not fieldsets, or might even use separate form/views entirely -- we'll know more once we start adding deeper configuration options here. But for that, I am gathering fieldsets will probably be removed in the future.

Second, I wanted to discuss avoiding (and probably removing) use of Crispy layouts and the layout API. Having HTML authoring removed from template code is a bit awkward already, just given the code/template separation and repository split, but it gets much worse if we want JS/Knockout/etc code in the form.

For that, I might suggest keeping this incredibly simple for a first pass and removing fieldsets and Python HTML authoring.

Copy link
Member Author

Choose a reason for hiding this comment

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

For that, I might suggest keeping this incredibly simple for a first pass and removing fieldsets and Python HTML authoring.

How do you imagine that version? Just a bunch of inline checkboxes, as shown in readthedocs/ext-theme#262 (comment))?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that seems like plenty 👍

It will look minimal, but should be fine -- especially with a fix for block display checkboxes. We definitely will want to improve on that later in some way in the future, and we can start mocking out what that looks like once we know what other configuration fields we want there.

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'm not sure how to fix the inline checkbox CSS issue, but I found where is the issue at least.

.ui.checkbox {
  display: inline-block;
}

Removing that property it displays as we want, I'd say.

Screenshot_2024-01-18_13-20-22

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 will move forward with the required changes on this PR and we can tackle that CSS issue in ext-theme, I guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that sounds good. I agree it's probably minor and I should be able to squeeze a fix into the crispy field template before deploy


self.helper = FormHelper()
self.helper.layout = Layout(*fieldsets)
self.helper.add_input(Submit("save", _("Save")))

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
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