Review of edx-notifications
Design Review
The overall design of the work is sound and has already been thoroughly reviewed by the Architect Council of that time period. The proposed concepts still remain valid and I don't see any reason to deviate significantly from the original design. However, two main things stand out as necessary to amend before we proceed with further work:
Need to decouple the eventing infrastructure from the notifications messaging infrastructure.
Currently, the concepts of subscribers/publishers/channels are specific to notification-type messages. In other words, the system assumes that all subscribers are notification delivery channels, such as SMS, browser, etc.
However, to create a scalable implementation that serves the multiple feature-sets and components of our platform, the underlying pub/sub framework should not assume the purpose of the subscribers. Rather, notification delivery channels can be built as a layer on top of the subscription layer.
Need to add an intermediary routing governance component that manages notifications across multiple channels.
Without a configurable policy component that governs delivery of notifications across all channels, the user experience will be less from ideal if the user interacts with the system through multiple interfaces - email/mobile/web/etc.
Currently, notification events are sent directly to delivery channels. Although a channel may have its own configurable delivery policy setting, it is myopic to its own channel and not considered within the context of its co-existence with the other registered channels.
Ideally, the policy should be configurable by the learners, the course teams, and open-edX operators/admins.
Implementation Review
There are several usability features that were implemented for the in-browser notifications that are worthy of reusing. For instance:
Aggregating notifications so the user sees an aggregated count of the event, rather than individual events. For example, number of followers on your post.
There are also several scalability issues that are embedded in the core implementation that will not suffice for the edX scale. For instance:
Web site polls every 30 seconds to find new notifications
There are some code maintenance issues that would need to be improved. For instance:
django.contrib.messages.middleware.MessageMiddleware is installed but doesn't seem to be used.
@Nimisha Asthagiri (Deactivated), I see you all might be dusting off edx-proctoring. Let me know if you have any questions.
In short, IMO, the two big scalability things are to either get websockets or long-poll working for notification delivery (or at least signalling that a notification is ready, and then do an API call to get payload). We didn't have time on the original implementation to do that.
Also, and I don't think you called this out, is the "durable" storage is in MySQL, you all might want to think about a different storage backend as there's one table which might grow very large (the one that keeps track of Notification→User read/unread state, I can't remember the table name). There's an incomplete PR to try to get it over to MongoDB.
Best wishes