New notification system: implementation #10922
Conversation
We are not sending any kind of notification from the Django admin. We used to use this in the past but we are not using it anymore. I'm deleting it now to avoid re-implementing it using the new notification system.
I'll come back to this later.
This allows to store values required for this notification at render time
Use `Notification.objects.add()` to create a notification with de-duplication capabilities. If the notification already exists, it updates all the fields with the new ones and set the `modified` datetime to now. This commit also migrates the "Your primary email address is not verified" to the new notification system, which it was when I implemented the de-duplication logic.
Use a small hack on build detail's page to refresh the page once the build is finished. This way, we don't have to re-implement the new notification system in the old Knockout javascript code and we focus ourselves only in the new dashboard.
|
I'm merging this PR to avoid conflicts. I'll make any required work in following PRs. |
We are using the `Notification`s attached to the object now. These old errors are migrated to the new system by a one-time script.
|
We decided to merge this after today's deploy since I wasn't able to check everything before the deploy. I created the script to migrate the old |
Definitely, I agree with this modeling and was instead thinking we'd use a separate query like
This seems like a good first step, and solves the more immediate problem of the organization notifications. We can always change these messages later to be flagged important, and use a more general pattern like a
👍 I also can't think of any messages that are important yet immediately. User notifications are special cased though, and could use this same pattern instead of a dedicated query/template context. Oh also, I realized we can maybe even show project error notifications in project listings, using an error icon for any projects with error notifications. That would at least give some higher level view into projects that have problems.
I don't recall if we've talked about this, but it could be a great UI to add. I'd say this is maybe a later phase of work, it's nice to have but not essential to the current refactor. |
| (UNREAD, UNREAD), | ||
| (READ, READ), | ||
| (DISMISSED, DISMISSED), | ||
| (CANCELLED, CANCELLED), |
There was a problem hiding this comment.
I don't have a strong preference. Either one is fine I think.
Resolved conveys the "problem was solved" which may not be true. Cancelled, on the other hand, doesn't. So, 🤷🏼
Noted in chat, but there were some issues with the notification migration and it sounds like we're probably leaning towards skipping this. The template code and KO view would stay the same as it is currently, or wouldn't change much if we supported the list of notifications with the same logic. |
Thanks for creating all of those! I've added them to our roadmap and put the API changes on this sprint. The API pieces will be required to make progress on any front end pieces and can happen before we get to any HTML/JS. I opened up readthedocs/ext-theme#259 to discuss how to proceed on the actual front end work, but I'm still feeling that these will be relatively minor compared to the last of the work on the backend. |
This reverts commit d4951a8. We decided to keep this code around for now. See #10980 (comment)
If the message ID is not found we return a generic message to avoid breaking the request and we log an internal error to fix the issue.
|
I applied all the suggestions. I'm merging this so other people can start testing it locally in their day to day development. Please, let me know if anything doesn't work as you expected and I will try to fix it before deploying it next week. |
* Notifications: small fixes found after reviewer
Use single `{}` since we are using `.format()` to format these strings.
* Handle missing `.format()` key
Avoid internal error and log the exception so we can figure it out how to solve
it. This happens when the `Notification` does not have _all_ the required `format_values`.
* Protection agasint XSS when rendering notifications
See #10922 (comment)
* Test for missing key in format values and XSS protection
* Update common/
* Lint
* Document we only support `str` and `int` for now in `format_values`
We don't support nested dictionaries in `format_values` or random objects.
Only `str` and `int`. That should be enough for now.
Skip all the values that are not `str` or `int` from the format values to render
the messages.
* Typo
Initial work towards the new notification system. I started adding some comments and deleting old code to understand the scope of the project.
Decisions made
format_valuesattribute and they are stored in a JSONField in the database, so they can be used to render the message at display time.<app>/notifications.pyand defining and registering the messages there. Then this file has to be imported from<app>/apps.pyor<app>/__init__.pyor similar approach.models.Managerthat exposes a.add()method with de-duplication capabilities.location.reload()JS to avoid re-implementing the Knockout dynamic capabilities in the old dashboard.Build.errorfield is not populated anymore from the builder. Those error are now attached asNotificationobjects to the build itself.ToDo
The work listed here is not mandatory of this PR, but I'm writing it down to keep track of it and don't forget:
location.reload()hack in the new dashboard until we write the proper Knockout code using APIv3SiteNotificationclass completelyreadthedocs-corporateat https://github.com/readthedocs/readthedocs-corporate/pull/1699readfor exampleConfigErrornotifications shown at build timeNotification.format_valuesto.contextwhich is more common in the Django worldext-themeat Implementation of new notification system ext-theme#254readthedocs-extand implement the new notification system there as wellProject.skip=Truethat createNotificationobject for those (see Notifications: create notifications for projects withskip=True#10980)Examples in screenshots
Notification error attached to a
BuildobjectNotification error attached to a
ProjectobjectNotification error attached to a
UserobjectNotification on business
Notification on new dashboard
Closes #10917