🚧 notification model and signals for wishlist feature #233
Conversation
MorganeAD
force-pushed
the
issue-196_morgane_course_user_wishlist
branch
from
1aa2177 to
d280c9d
Compare
MorganeAD
force-pushed
the
issue-196_morgane_course_user_wishlist_part2
branch
from
c0d43e7 to
b173631
Compare
MorganeAD
force-pushed
the
issue-196_morgane_course_user_wishlist
branch
2 times, most recently
from
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
MorganeAD
force-pushed
the
issue-196_morgane_course_user_wishlist
branch
2 times, most recently
from
b248943 to
6417199
Compare
MorganeAD
force-pushed
the
issue-196_morgane_course_user_wishlist
branch
from
6417199 to
88c26bc
Compare
MorganeAD
force-pushed
the
issue-196_morgane_course_user_wishlist_part2
branch
from
b173631 to
6c6e1aa
Compare
sampaccoud
force-pushed
the
issue-196_morgane_course_user_wishlist
branch
5 times, most recently
from
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)