Design doc: new notification system #10890
Conversation
Initial design document to discuss the implementation of a new notification system.
|
Thanks for putting this together, @humitos! This is a great overview. I just got a chance to skim through this, and want to spend some time reading and thinking over it more, especially the code/data structure part. But immediately, I can say this seems to match what we talked about really well. The only major note I have right now is that we could perhaps consider the notifications around site/feature news (or any use we have planned for the bell notification tray feature) as part of a second iteration on this feature. We don't need this new feature immediately, however I fully agree it's important to be thinking of the technical implementation early on. We may even find it's easier or more productive to hard code feature onboarding content into the templates. This doesn't seem like it would change the plan so far though, all the other parts of this plan still seem usable. I'll continue with a deeper look at this later today or tomorrow, but I also don't see anything I'd immediately change with the design yet. |
I put the code/data structure in the design doc because I think it's important to have all the db data fields required to support all the user cases/goals we want to achieve. Besides the combination of a Python regular class together with a ORM model is also important in the design.
I didn't think too much about this and I'm happy to expand this a little more here --at least to know that we are creating a modeling that's expandable in the future to support this use case. However, as you already said, I don't want to go super deep into this since it's a new feature that we are not sure how we want to use it.
I think that I don't understand this sentence. What do you mean by "hard code feature onboarding content"? |
Sorry, to clarify I mean we could hardcode the new feature announcements into the UI inside templates. There are two places in the UI that I think we can consider hardcoding new feature announcements directly into the templates. First would be in some of the empty spaces in the sidebars, for example on the project listing page. The second would be to find a spot in the UI that is close to the feature that we are enabling, maybe using a popup. This is maybe less helpful as the user needs to be in the right location in the dashboard to see this popup. Something to think about more though. I will say I tend to get annoyed by UIs that constant yell at me about new features I don't care about, so that could be something to consider too. |
There was a problem hiding this comment.
I don't see anything that would be blocking us on your plan, and agree this all sounds great.
The only big question I have still is what notifications we'd want to stuff behind the top menu icon. There is both an argument for low priority notifications, but also could be an argument for all notifications. I'm not quite +0 on all notifications, but it might be something to experiment with more.
| * Re-use the new notification system for product updates (e.g. new features, deprecated config keys) | ||
| * Message content lives on Python classes that can be translated and formatted with objects (e.g. Build, Project) | ||
| * Message could have richer content (e.g. HTML code) to generate links and emphasis | ||
| * Notifications have trackable state (e.g. unread (default)=never shown, read=shown, dismissed=don't show again) |
There was a problem hiding this comment.
I'm not quite seeing 3 states here maybe. I'm guessing the read/unread status is just for the notifications in the bell menu?
I think what you're describing here works fine. The two fields status and modified_at give us almost everything we need. We likely want status for some of the queries we'll use for deduplication of notifications as well.
This information is a bit derivative too though. It would be ideal to discretely track specific datetime fields -- read_at and dismissed_at, etc. Relying on just modified_at, we lose history if the notification is updated more than once -- for example, read/displayed more than once or read then dismissed.
Just mentioning this in case we have any other reason to go this direction. If so, then status=dismissed is going to be redundant to dismissed_at__isnull=False/whatever.
But I think what you're describing almost certainly is enough.
There was a problem hiding this comment.
I haven't thought about read_at and dismissed_at, but I think they could be useful. If we implement these fields, we won't need status and it will cover all the things I want as well 👍🏼
- unread:
read_at__isnull=True(never shown to the user yet) - read:
read_at__isnull=False(shown to the user) - dismissed:
dismissed_at__isnull=False(the user clicked on the X)
There is an implementation thing that sounds a little weird here, tho. The read_at field has to be updated each time the API endpoint is hit to return the notifications. So, it's going to be a GET that returns everything but also updates a field?
There was a problem hiding this comment.
Mm yeah, I was wondering the same thing about the read state update. Whether that is status=read or read_at=..., we need to update this field at some point around the time that the user receives the message. Our options would be updating on the API response or we could move this to the front end code and update via an API with status=read/etc on a UI event -- the user sees the notification or clicks on the notification.
Both are valid, so whatever seems like the best UX. I suspect we'll want the front end version of this though.
There was a problem hiding this comment.
we could move this to the front end code and update via an API with
status=read/etc on a UI event -- the user sees the notification or clicks on the notification.
This is actually the most correct, technically speaking. I'd probably start here, yeah.
Also, I think it's better UX as well since the front-end will get "all the notifications" at once, but may display only 1. In that case, that only one has to be updated with status=read 👍🏼
I will add a PATCH endpoint to update notifications for this use case.
| # | ||
| attached_to_content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE) | ||
| attached_to_id = models.PositiveIntegerField() | ||
| attached_to = GenericForeignKey("attached_to_content_type", "attached_to_id") |
There was a problem hiding this comment.
This would be good to get more feedback on. I have used generic relations in the past, but perhaps there is a strong reason not to.
There was a problem hiding this comment.
I've used it in the past as well. I will give it a try and decide what to do at the implementation time depending on how comfortable it feels 😄
| Backward compatibility | ||
| ---------------------- | ||
|
|
||
| It's not strickly required, but if we want, we could extract the current notification logic from: |
There was a problem hiding this comment.
I'd say this is probably a requirement.
In fact, I might say these should be the first notifications to more to the new system, as I've deleted this logic from the new templates entirely already.
With notification logic in the application and in templates, adding a third layer on top will be a bit overwhelming. We can completely remove the template "notification" logic, and relatively easily too.
There was a problem hiding this comment.
I know you deleted the logic from the templates, which is 👍🏼 . Here, I'm talking about backward compatibility: showing the new notifications in the old templates --which is something I'm not planning to implement since it's a waste of time.
There was a problem hiding this comment.
The other point of this "backward compatibility" is about migrating the "old build notifications" to the new Notification model we are defining here. I wouldn't start here either as first step without doing some testing on production.
Old builds will keep showing old notifications based on Build.error fields. New builds will show only notifications from the new system.
There was a problem hiding this comment.
I'm not sure if I'm being clear here or not, but I don't plan to touch the old templates at all 😄
When I said "extract the current notification logic" I mean "re-implement the logic that lives in the template in the backend and create a Notification" --but I'm not going to touch the template or remove that logic from them.
There was a problem hiding this comment.
Yeah, that all makes sense and matches my understanding of what we want here too. I think we can pretty easily avoid doing anything that will affect legacy builds, though the new dashboard JS or templates will need to accommodate both patterns obviously. This was expected at least though.
|
It seems we are aligned with the design here. I will merge this and start working on the implementation. |
Endpoint to update a notification. This will be used by the front-end to mark a notification as read, for example. Ref: #10890 (comment)
Endpoint to update a notification. This will be used by the front-end to mark a notification as read, for example. Ref: #10890 (comment)
Initial design document to discuss the implementation of a new notification system.
Closes #10598
📚 Documentation previews 📚 https://dev--10890.org.readthedocs.build/en/10890/design/new-notifications-system.html
Related issues: