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

humitos · 2024-10-29T14:44:45Z

We currently allow users to disable the latest and non-stable notifications with only one checkbox. This PR split this checkbox into two, so users can decide to show a notification on latest and/or on non-stable versions individually.

Note this PR also changes the addons API response: non_latest_version_warning and external_version_warning are combined into notifications. I think nobody is using this yet, so I'm not expecting issues with it. Let me know if you think differently here.

Required by readthedocs/addons#133

We currently allow users to disable the latest and non-stable notifications with only one checkbox. This PR split this checkbox into two, so users can decide to show a notification on latest and/or on non-stable versions individually. Note this PR also changes the addons API response: `non_latest_version_warning` and `external_version_warning` are combined into `notifications`. I think nobody is using this yet, so I'm not expecting issues with it. Let me know if you think differently here. Required by readthedocs/addons#133

Closes #133 Requires readthedocs/readthedocs.org#11718

Instead of showing one checkbox to enable "Show notification on latest and non-stable versions" we have two checkboxes now: - "Show notification on latest version" - "Show notification on non-stable versions" Requires readthedocs/addons#414 Requires readthedocs/readthedocs.org#11718

stsewd · 2024-10-29T15:13:47Z

readthedocs/projects/models.py

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


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.

Seems fine, it should be a quick migration?

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?

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.

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.

I'm happy to talk more about this and change this default now and/or in the future, tho.

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.

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

ericholscher

Looks good to me. This seems like a good chance to add some docs for this functionality, since we're changing it? It's listed in the main addons page as PR build notifications, but not any other subpage. I might suggest adding a page for Documentation Notifications, or at least listing them a bit more nicely in that page.

ericholscher · 2024-10-29T22:26:12Z

readthedocs/projects/models.py

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


Seems fine, it should be a quick migration?

readthedocs/projects/forms.py

ericholscher · 2024-10-29T23:04:04Z

Pushed up a basic start of docs.

Co-authored-by: Eric Holscher <[email protected]>

…om:readthedocs/readthedocs.org into humitos/addons-notifications-latest-stable

…mitos/addons-notifications-latest-stable

humitos · 2024-10-30T09:57:38Z

@agjohnson there is a test failing for ext-theme that I wasn't able to understand what's going on. It seems a crispy field cannot be rendered due to the field passed being "", which confuses me 🤷🏼 . Do you know what I'm doing wrong here?

humitos · 2024-10-30T10:00:15Z

Is it because it's using the old/deleted fields? If that's the case, this will be solved once we merge readthedocs/ext-theme#513

humitos · 2024-10-30T10:02:21Z

Yeap, that's the reason. Updating the file .tox/ext-theme/lib/python3.10/site-packages/readthedocsext/theme/templates/projects/addons_form.html solves this issue 💯 . Tests pass locally with this change. So, we are ready to merge here.

Instead of showing one checkbox to enable "Show notification on latest and non-stable versions" we have two checkboxes now: - "Show notification on latest version" - "Show notification on non-stable versions" ![Screenshot_2024-10-29_15-52-36](https://github.com/user-attachments/assets/60a35c09-cf9d-4429-af53-41515f00ecac) Requires readthedocs/addons#414 Requires readthedocs/readthedocs.org#11718

Closes #133 Requires readthedocs/readthedocs.org#11718

sentry-io · 2024-11-05T16:25:46Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ KeyboardInterrupt readthedocs.projects.migrations.0128_addons_not... View Issue

_{Did you find this useful? React with a 👍 or 👎}

humitos requested a review from a team as a code owner October 29, 2024 14:44

humitos requested a review from ericholscher October 29, 2024 14:44

auto-assign bot assigned humitos Oct 29, 2024

humitos added a commit to readthedocs/addons that referenced this pull request Oct 29, 2024

Notifications: allow disable latest and/or non-stable

1327731

Closes #133 Requires readthedocs/readthedocs.org#11718

humitos mentioned this pull request Oct 29, 2024

Notifications: allow disable latest and/or non-stable readthedocs/addons#414

Merged

humitos mentioned this pull request Oct 29, 2024

Addons: split single checkbox into two readthedocs/ext-theme#513

Merged

stsewd reviewed Oct 29, 2024

View reviewed changes

ericholscher approved these changes Oct 29, 2024

View reviewed changes

ericholscher reviewed Oct 29, 2024

View reviewed changes

readthedocs/projects/forms.py Outdated Show resolved Hide resolved

Add basic docs on Doc notifications

232bbbe

ericholscher requested a review from a team as a code owner October 29, 2024 23:04

ericholscher requested a review from agjohnson October 29, 2024 23:04

humitos and others added 8 commits October 30, 2024 09:04

Update readthedocs/projects/forms.py

057d581

Co-authored-by: Eric Holscher <[email protected]>

Update test to fix it

65cece8

Merge branch 'humitos/addons-notifications-latest-stable' of github.c…

2138ad7

…om:readthedocs/readthedocs.org into humitos/addons-notifications-latest-stable

Quotes :(

8a8323b

Update API response for tests

0294f2d

Lint

bed03bd

Merge branch 'main' of github.com:readthedocs/readthedocs.org into hu…

c4bea78

…mitos/addons-notifications-latest-stable

lint

104fdd0

lint: meh, pre-commit is failing locally for some reason

b19d0dc

humitos merged commit 7120315 into main Nov 4, 2024
5 of 7 checks passed

humitos deleted the humitos/addons-notifications-latest-stable branch November 4, 2024 10:15

humitos added a commit to readthedocs/addons that referenced this pull request Nov 4, 2024

Notifications: allow disable latest and/or non-stable (#414)

81c058c

Closes #133 Requires readthedocs/readthedocs.org#11718

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

humitos commented Oct 29, 2024

stsewd Oct 29, 2024

ericholscher Oct 29, 2024

ericholscher Oct 29, 2024

humitos Oct 30, 2024

humitos Oct 30, 2024

humitos Oct 30, 2024

stsewd Oct 30, 2024

humitos Oct 30, 2024

ericholscher left a comment •

edited

Loading

ericholscher Oct 29, 2024

ericholscher commented Oct 29, 2024

humitos commented Oct 30, 2024

humitos commented Oct 30, 2024

humitos commented Oct 30, 2024 •

edited

Loading

sentry-io bot commented Nov 5, 2024

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

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

Conversation

humitos commented Oct 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericholscher left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericholscher commented Oct 29, 2024

humitos commented Oct 30, 2024

humitos commented Oct 30, 2024

humitos commented Oct 30, 2024 • edited Loading

sentry-io bot commented Nov 5, 2024

Suspect Issues

ericholscher left a comment •

edited

Loading

humitos commented Oct 30, 2024 •

edited

Loading