Skip to content

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

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 11 commits into from
Nov 4, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5d5f64d
Addons: allow users to show/hide notifications on latest/non-stable
humitos Oct 29, 2024
232bbbe
Add basic docs on Doc notifications
ericholscher Oct 29, 2024
057d581
Update readthedocs/projects/forms.py
humitos Oct 30, 2024
65cece8
Update test to fix it
humitos Oct 30, 2024
2138ad7
Merge branch 'humitos/addons-notifications-latest-stable' of github.c…
humitos Oct 30, 2024
8a8323b
Quotes :(
humitos Oct 30, 2024
0294f2d
Update API response for tests
humitos Oct 30, 2024
bed03bd
Lint
humitos Oct 30, 2024
c4bea78
Merge branch 'main' of github.com:readthedocs/readthedocs.org into hu…
humitos Oct 30, 2024
104fdd0
lint
humitos Oct 30, 2024
b19d0dc
lint: meh, pre-commit is failing locally for some reason
humitos Oct 30, 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
13 changes: 8 additions & 5 deletions readthedocs/projects/forms.py
View file
Open in desktop
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 versions"),
}
widgets = {
"doc_diff_root_selector": forms.TextInput(
Expand Down
73 changes: 73 additions & 0 deletions readthedocs/projects/migrations/0128_addons_notifications.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
# 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),
]
31 changes: 31 additions & 0 deletions readthedocs/projects/migrations/0129_addons_remove_old_fields.py
View file
Open in desktop
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
View file
Open in desktop
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
11 changes: 8 additions & 3 deletions readthedocs/proxito/tests/test_hosting.py
View file
Open in desktop
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
View file
Open in desktop
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
17 changes: 12 additions & 5 deletions readthedocs/rtd_tests/tests/test_project_forms.py
View file
Open in desktop
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 @@ -1172,14 +1177,16 @@ def test_addonsconfig_form_invalid_sorting_custom_pattern(self):
"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