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

Addons: allow users to show/hide notifications on latest/non-stable #11718

Merged
merged 11 commits into from
Nov 4, 2024
4 changes: 2 additions & 2 deletions docs/user/addons.rst
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ and can be accessed via hotkeys or on screen UI elements.
:doc:`DocDiff </pull-requests>`
Highlight changed output from pull requests

:doc:`Pull request notification </pull-requests>`
Notify readers that they are reading docs from a pull request
:doc:`Documentation notification </doc-notifications>`
Alert users to various documentation states

:doc:`Flyout </flyout-menu>`
Easily switch between versions and translations
Expand Down
26 changes: 26 additions & 0 deletions docs/user/doc-notifications.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
Documentation notifications
===========================

Documentation notifications alert users to information about the documentation they are viewing.
These notifications can be enabled or disabled by the project maintainer,
and are displayed to the user in the rendered documentation.

Overview
--------

The current notifications are:

- **Pull request warning**: Show a notification on builds from pull requests, with a link back to the pull request for giving feedback.
- **Latest version warning**: Show a notification on latest version, warning users that they are reading docs from a development version.
- **Non-stable version warning**: Show a notification on non-stable versions, warning users that they are reading docs from a non-stable release.

Manage notifications
--------------------

To manage notifications for your project:

1. Go to the :term:`dashboard`.
2. Click on a project name.
3. Go to :guilabel:`Settings`.
4. In the left bar, go to :guilabel:`Addons`.
5. Configure each Addon in the :guilabel:`Notifications` section.
1 change: 1 addition & 0 deletions docs/user/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ Read the Docs: documentation simplified
/localization
/versioning-schemes
/custom-domains
/doc-notifications
/canonical-urls
/reference/cdn
/reference/sitemaps
Expand Down
13 changes: 8 additions & 5 deletions readthedocs/projects/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -654,23 +654,26 @@ class Meta:
"analytics_enabled",
"doc_diff_enabled",
"doc_diff_root_selector",
"external_version_warning_enabled",
"flyout_enabled",
"flyout_sorting",
"flyout_sorting_latest_stable_at_beginning",
"flyout_sorting_custom_pattern",
"hotkeys_enabled",
"search_enabled",
"stable_latest_version_warning_enabled",
"notifications_enabled",
"notifications_show_on_latest",
"notifications_show_on_non_stable",
"notifications_show_on_external",
)
labels = {
"enabled": _("Enable Addons"),
"external_version_warning_enabled": _(
"notifications_show_on_external": _(
"Show a notification on builds from pull requests"
),
"stable_latest_version_warning_enabled": _(
"Show a notification on non-stable and latest versions"
"notifications_show_on_non_stable": _(
"Show a notification on non-stable versions"
),
"notifications_show_on_latest": _("Show a notification on latest version"),
}
widgets = {
"doc_diff_root_selector": forms.TextInput(
Expand Down
80 changes: 80 additions & 0 deletions readthedocs/projects/migrations/0128_addons_notifications.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
# Generated by Django 4.2.16 on 2024-10-29 14:05

from django.db import migrations, models
from django_safemigrate import Safe


def forward_add_fields(apps, schema_editor):
AddonsConfig = apps.get_model("projects", "AddonsConfig")
for addons in AddonsConfig.objects.filter(project__isnull=False):
addons.notifications_show_on_latest = (
addons.stable_latest_version_warning_enabled
)
addons.notifications_show_on_non_stable = (
addons.stable_latest_version_warning_enabled
)
addons.notifications_show_on_external = addons.external_version_warning_enabled
addons.save()


def reverse_remove_fields(apps, schema_editor):
AddonsConfig = apps.get_model("projects", "AddonsConfig")
for addons in AddonsConfig.objects.filter(project__isnull=False):
addons.stable_latest_version_warning_enabled = (
addons.notifications_show_on_latest
or addons.notifications_show_on_non_stable
)
addons.external_version_warning_enabled = addons.notifications_show_on_external
addons.save()


class Migration(migrations.Migration):
safe = Safe.before_deploy

dependencies = [
("projects", "0127_default_to_semver"),
]

operations = [
migrations.AddField(
model_name="addonsconfig",
name="notifications_enabled",
field=models.BooleanField(default=True),
),
migrations.AddField(
model_name="addonsconfig",
name="notifications_show_on_external",
field=models.BooleanField(default=True),
),
migrations.AddField(
model_name="addonsconfig",
name="notifications_show_on_latest",
field=models.BooleanField(default=True),
),
migrations.AddField(
model_name="addonsconfig",
name="notifications_show_on_non_stable",
field=models.BooleanField(default=True),
),
migrations.AddField(
model_name="historicaladdonsconfig",
name="notifications_enabled",
field=models.BooleanField(default=True),
),
migrations.AddField(
model_name="historicaladdonsconfig",
name="notifications_show_on_external",
field=models.BooleanField(default=True),
),
migrations.AddField(
model_name="historicaladdonsconfig",
name="notifications_show_on_latest",
field=models.BooleanField(default=True),
),
migrations.AddField(
model_name="historicaladdonsconfig",
name="notifications_show_on_non_stable",
field=models.BooleanField(default=True),
),
migrations.RunPython(forward_add_fields, reverse_remove_fields),
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# Generated by Django 4.2.16 on 2024-10-29 14:09

from django.db import migrations
from django_safemigrate import Safe


class Migration(migrations.Migration):
safe = Safe.after_deploy

dependencies = [
("projects", "0128_addons_notifications"),
]

operations = [
migrations.RemoveField(
model_name="addonsconfig",
name="external_version_warning_enabled",
),
migrations.RemoveField(
model_name="addonsconfig",
name="stable_latest_version_warning_enabled",
),
migrations.RemoveField(
model_name="historicaladdonsconfig",
name="external_version_warning_enabled",
),
migrations.RemoveField(
model_name="historicaladdonsconfig",
name="stable_latest_version_warning_enabled",
),
]
10 changes: 5 additions & 5 deletions readthedocs/projects/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -178,9 +178,6 @@ class AddonsConfig(TimeStampedModel):
help_text="CSS selector for the main content of the page",
)

# External version warning
external_version_warning_enabled = models.BooleanField(default=True)

# EthicalAds
ethicalads_enabled = models.BooleanField(default=True)

Expand Down Expand Up @@ -215,8 +212,11 @@ class AddonsConfig(TimeStampedModel):
search_enabled = models.BooleanField(default=True)
search_default_filter = models.CharField(null=True, blank=True, max_length=128)

# Stable/Latest version warning
stable_latest_version_warning_enabled = models.BooleanField(default=True)
# Notifications
notifications_enabled = models.BooleanField(default=True)
notifications_show_on_latest = models.BooleanField(default=True)
notifications_show_on_non_stable = models.BooleanField(default=True)
notifications_show_on_external = models.BooleanField(default=True)
Comment on lines +216 to +219
Copy link
Member

Choose a reason for hiding this comment

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

All these should be nullable to avoid downtime.

https://dev.readthedocs.io/en/stable/migrations.html#adding-a-new-field

If we are okay with not allowing to create/edit this model while the migration takes place, that's okay, I guess.

Copy link
Member

Choose a reason for hiding this comment

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

Seems fine, it should be a quick migration?

Copy link
Member

Choose a reason for hiding this comment

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

I do wonder if we want to default all of these to True, though? In particular, I'm not sure we want latest to have a default warning?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the field has a default value it doesn't need to be nullable, right? Once the field is added, it will be the default value automatically and when creating a new row from an old instance without passing the new value.

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 do wonder if we want to default all of these to True, though? In particular, I'm not sure we want latest to have a default warning?

I haven't thought too much about this, I just followed the defaults we had previously. Apart from that, I think it's good to enable all the addons by default as a way to immediately expose these features to users and leave users to decide if they want to use them or not.

The only ones I'm have to turn off by default are those that cost us money and are not very used by our users, eg. analytics.

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 happy to talk more about this and change this default now and/or in the future, tho.

Copy link
Member

Choose a reason for hiding this comment

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

If the field has a default value it doesn't need to be nullable, right?

No, the default value is set at the django level, not at the db level. Django 5.x has an option to set default values at the db level.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point 👍🏼 . We should mention that in that document to avoid this confusion every time 😄 .



class AddonSearchFilter(TimeStampedModel):
Expand Down
10 changes: 5 additions & 5 deletions readthedocs/proxito/tests/responses/v1.json
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,6 @@
"enabled": false,
"code": null
},
"external_version_warning": {
"enabled": true
},
"hotkeys": {
"enabled": true,
"doc_diff": {
Expand All @@ -139,8 +136,11 @@
"trigger": "Slash"
}
},
"non_latest_version_warning": {
"enabled": true
"notifications": {
"enabled": true,
"show_on_external": true,
"show_on_latest": true,
"show_on_non_stable": true
},
"doc_diff": {
"enabled": true,
Expand Down
11 changes: 8 additions & 3 deletions readthedocs/proxito/tests/test_hosting.py
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,10 @@ def test_disabled_addons_via_addons_config(self):
addons.flyout_enabled = False
addons.hotkeys_enabled = False
addons.search_enabled = False
addons.stable_latest_version_warning_enabled = False
addons.notifications_enabled = False
addons.notifications_show_on_latest = False
addons.notifications_show_on_non_stable = False
addons.notifications_show_on_external = False
addons.save()

r = self.client.get(
Expand All @@ -166,8 +169,10 @@ def test_disabled_addons_via_addons_config(self):
)
assert r.status_code == 200
assert r.json()["addons"]["analytics"]["enabled"] is False
assert r.json()["addons"]["external_version_warning"]["enabled"] is False
assert r.json()["addons"]["non_latest_version_warning"]["enabled"] is False
assert r.json()["addons"]["notifications"]["enabled"] is False
assert r.json()["addons"]["notifications"]["show_on_latest"] is False
assert r.json()["addons"]["notifications"]["show_on_non_stable"] is False
assert r.json()["addons"]["notifications"]["show_on_external"] is False
assert r.json()["addons"]["doc_diff"]["enabled"] is False
assert r.json()["addons"]["flyout"]["enabled"] is False
assert r.json()["addons"]["search"]["enabled"] is False
Expand Down
16 changes: 5 additions & 11 deletions readthedocs/proxito/views/hosting.py
Original file line number Diff line number Diff line change
Expand Up @@ -455,17 +455,11 @@ def _v1(self, project, version, build, filename, url, request):
# https://github.com/readthedocs/readthedocs.org/issues/9530
"code": project.analytics_code,
},
"external_version_warning": {
"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": 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]",
"notifications": {
"enabled": project.addons.notifications_enabled,
"show_on_latest": project.addons.notifications_show_on_latest,
"show_on_non_stable": project.addons.notifications_show_on_non_stable,
"show_on_external": project.addons.notifications_show_on_external,
},
"flyout": {
"enabled": project.addons.flyout_enabled,
Expand Down
24 changes: 15 additions & 9 deletions readthedocs/rtd_tests/tests/test_project_forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -1133,14 +1133,16 @@ def test_addonsconfig_form(self):
"enabled": True,
"analytics_enabled": False,
"doc_diff_enabled": False,
"external_version_warning_enabled": True,
"flyout_enabled": True,
"flyout_sorting": ADDONS_FLYOUT_SORTING_CALVER,
"flyout_sorting_latest_stable_at_beginning": True,
"flyout_sorting_custom_pattern": None,
"hotkeys_enabled": False,
"search_enabled": False,
"stable_latest_version_warning_enabled": True,
"notifications_enabled": True,
"notifications_show_on_latest": True,
"notifications_show_on_non_stable": True,
"notifications_show_on_external": True,
}
form = AddonsConfigForm(data=data, project=self.project)
self.assertTrue(form.is_valid())
Expand All @@ -1149,7 +1151,10 @@ def test_addonsconfig_form(self):
self.assertEqual(self.project.addons.enabled, True)
self.assertEqual(self.project.addons.analytics_enabled, False)
self.assertEqual(self.project.addons.doc_diff_enabled, False)
self.assertEqual(self.project.addons.external_version_warning_enabled, True)
self.assertEqual(self.project.addons.notifications_enabled, True)
self.assertEqual(self.project.addons.notifications_show_on_latest, True)
self.assertEqual(self.project.addons.notifications_show_on_non_stable, True)
self.assertEqual(self.project.addons.notifications_show_on_external, True)
self.assertEqual(self.project.addons.flyout_enabled, True)
self.assertEqual(
self.project.addons.flyout_sorting,
Expand All @@ -1162,24 +1167,25 @@ def test_addonsconfig_form(self):
self.assertEqual(self.project.addons.flyout_sorting_custom_pattern, None)
self.assertEqual(self.project.addons.hotkeys_enabled, False)
self.assertEqual(self.project.addons.search_enabled, False)
self.assertEqual(
self.project.addons.stable_latest_version_warning_enabled,
True,
)
self.assertEqual(self.project.addons.notifications_show_on_latest, True)
self.assertEqual(self.project.addons.notifications_show_on_non_stable, True)
self.assertEqual(self.project.addons.notifications_show_on_external, True)

def test_addonsconfig_form_invalid_sorting_custom_pattern(self):
data = {
"enabled": True,
"analytics_enabled": False,
"doc_diff_enabled": False,
"external_version_warning_enabled": True,
"flyout_enabled": True,
"flyout_sorting": ADDONS_FLYOUT_SORTING_CUSTOM_PATTERN,
"flyout_sorting_latest_stable_at_beginning": True,
"flyout_sorting_custom_pattern": None,
"hotkeys_enabled": False,
"search_enabled": False,
"stable_latest_version_warning_enabled": True,
"notifications_enabled": True,
"notifications_show_on_latest": True,
"notifications_show_on_non_stable": True,
"notifications_show_on_external": True,
}
form = AddonsConfigForm(data=data, project=self.project)
self.assertFalse(form.is_valid())
Expand Down