-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Questions about the notifications
app
#32249
Comments
@ormsbee To answer your questions in order.
|
Ah, ok. So you're going to generate them during enrollment for new entries going forward, and backfill for older enrollments via management command. When i said "auto-generated", I was thinking about how a GET request to the UserNotificationPreferenceView will lazily get or create the NotificationPreference the notification preferences for that user.
Please state that explicitly in an ADR inside this app, or in a
That sort of bidirectional relationship is what concerns me. I can see two straightforward paths for this sort of app: 1:
|
|
I don't have access to the technical design doc for this, so I'm just speculating based on what's been said in this thread, particularly:
That implies that individual apps are given control over when to queue up user notifications, though it's ultimately the The data for the We might even get to situations where apps are receiving signals from other apps and deciding to send user notifications about that. Like say you had a plugin that did some kind of course recommendation. I could imagine it catching the |
Thanks for calling my attention to this Dave.
Absolutely. As maintainers of openedx-events we would be happy to collaborate on this to make the event be available sooner.
We have had many requests over the years to make a better notifications system for openedx. Most recently as part of the Spanish Universities project. I just wrote an issue in the campus project roadmap that covers the use cases that they would like to implement. I think having possibilities to extend the notification rules would be a great enabler for many of our customers.
Given the discussion that unfolded in this thread I think as a potential developer of extensions for this I would like to have options to modify:
I'm assuming a lot here, but I would thing that being able to import the base classes from a different repo would make it easier to extend. |
The way this is currently implemented is via a set of per-user preferences, the defaults for which is hardcoded for the moment, but I imagine it could be configurable (maybe via Django settings) in the future? |
Hi everyone! Thanks for participating in this thread. I'm the product manager for the Infinity squad. Here's the specification document of web notifications for forums: https://openedx.atlassian.net/wiki/spaces/OEPM/pages/3768123519/Spec+Web+notifications+for+forums |
@ayub02: Thank you! |
@ayub02: Is there a corresponding technical document by any chance? |
Yes. @asadazam93 is working on that. I know that he plans to share a draft soon. |
Fantastic, thank you! |
@ormsbee Thank you for your suggestions. I think creating a signal in open edX events for a notification is a great approach and we certainly want to use that. I am creating an architecture document that should display the flow in detail. I am already including the open edX events architecture in that document as I feel that is a must have. Would like to hear your thoughts on the document. |
@asadazam93: Thank you! Your arch writeup looks like it's in a reviewable state, so I'll leave inline comments there. |
@saadyousafarbi, @asadazam93, @AhtishamShahid, @muhammadadeeltajamul: I came across the
notifications
app recently because of the ORM warning, and looked around for a bit. I had a number of questions/concerns:It's currently listening for changes to the
CourseEnrollment
model directly, but it should probably be made to use the public event signals defined for enrollments instead. (Though in the case of that particular handler, it doesn't look like it's necessary at all? Because it's only creating theNotificationPreference
that's auto-generated by API endpoint calls anyway?It looks like this is being built in a way that the
notifications
app needs to be aware of all the things that we're going to send notifications about. So for instance, the app controlling enrollments isn't deciding to send a notification–thenotifications
app is listening for enrollment events and deciding to send that to the user. But won't we want to send notifications from apps that are in plugins, or other external repos like enterprise?This largely depends on the answer to (2), but having it outside of edx-platform would make it easier to use from plugins.
get_*
methods on the models?This is much less important than the other questions, but why do the models have getter methods? It's not conventional for models, and it doesn't actually prevent mutation of the model fields since they're still publicly exposed.
I get that this is still in the early stages, and that much of this code might be proof-of-concept or scaffolding. But I'm not clear on the long term technical plan for this app, and the one wiki entry I'm aware of is on 2U's wiki and isn't public.
Thank you.
The text was updated successfully, but these errors were encountered: