🚧 notification model and signals for wishlist feature #233
Conversation
1aa2177
to
d280c9d
Compare
c0d43e7
to
b173631
Compare
f9b6bc4
to
06f3ee7
Compare
| notif_type = models.CharField( | ||
| _("Type of notification"), | ||
| choices=NOTIF_TYPES, | ||
| max_length=10, | ||
| default=NOTIF_TYPE_EMAIL | ||
| ) |
There was a problem hiding this comment.
In case where user asks to be notified by email and via the interface, two notifications items will be created ?
There was a problem hiding this comment.
Yes, I think it's better to separate it in two different notification instance because it's not the same process behind and it can be notified at different datetime. Also it's easier if a User when to be notified by the interface and not by email and vice versa.
What do you think ?
| notif_subject_ctype_limit = models.Q(app_label='core', model='coursewish') \ | ||
| | models.Q(app_label='core', model='enrollment') | ||
| notif_subject_ctype = models.ForeignKey( | ||
| ContentType, | ||
| on_delete=models.CASCADE, | ||
| related_name="subject_ctype_of_notifications", | ||
| limit_choices_to=notif_subject_ctype_limit | ||
| ) | ||
| notif_subject_id = models.UUIDField() | ||
| notif_subject = GenericForeignKey('notif_subject_ctype', 'notif_subject_id') | ||
|
|
||
| notif_object_ctype_limit = models.Q(app_label='core', model='producttargetcourserelation') \ | ||
| | models.Q(app_label='core', model='courserun') \ | ||
| | models.Q(app_label='core', model='product') | ||
| notif_object_ctype = models.ForeignKey( | ||
| ContentType, | ||
| on_delete=models.CASCADE, | ||
| related_name="object_ctype_of_notifications", | ||
| limit_choices_to=notif_object_ctype_limit | ||
| ) | ||
| notif_object_id = models.UUIDField() | ||
| notif_object = GenericForeignKey('notif_object_ctype', 'notif_object_id') |
There was a problem hiding this comment.
Why do you prefix all those attributes by notif_?
There was a problem hiding this comment.
I start to prefix by "notif_" to specify that is the "subject of the notification", in my opinion "subject" and "object" are too generic worlds by themselves, but if you prefer, I can remove the prefix. :)
| notif_subject_ctype_limit = models.Q(app_label='core', model='coursewish') \ | ||
| | models.Q(app_label='core', model='enrollment') |
There was a problem hiding this comment.
In the future, external services should be able to create notifications. The kind of limit you are defining here could block this kind of feature, I think.
There was a problem hiding this comment.
Yes, if we keep the limitation, we have to not forget to add the new authorized models for the futures external services.
I think it's better to have a limitation to correctly use the Notification model but we can also remove it.
This commit is the first part of the issue 196 (feature wishlist)
This commit add:
* model CourseWish that links a User and a Course
* endpoints to use this new model, according to the auth User
* GET wish list (can be filtered by course_code)
* GET a wish
* POST a wish
* DELETE a wish
b248943
to
6417199
Compare
6417199
to
88c26bc
Compare
b173631
to
6c6e1aa
Compare
6c7da60
to
06b0584
Compare
This PR is the second part of the issue #196 (feature user course wishlist)
This PR adds:
It's related to the comment on this PR #207 (comment)