Addons: allow users to show/hide notifications on latest/non-stable #11718
Conversation
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
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
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Seems fine, it should be a quick migration?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I do wonder if we want to default all of these to
True, though? In particular, I'm not sure we wantlatestto 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.
There was a problem hiding this comment.
I'm happy to talk more about this and change this default now and/or in the future, tho.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good point 👍🏼 . We should mention that in that document to avoid this confusion every time 😄 .
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
Seems fine, it should be a quick migration?
|
Pushed up a basic start of docs. |
Co-authored-by: Eric Holscher <25510+ericholscher@users.noreply.github.com>
…om:readthedocs/readthedocs.org into humitos/addons-notifications-latest-stable
…mitos/addons-notifications-latest-stable
|
@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 |
|
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 |
|
Yeap, that's the reason. Updating the file |
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_warningandexternal_version_warningare combined intonotifications. 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