-
Notifications
You must be signed in to change notification settings - Fork 166
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
Notification Model and Endpoints #286
Conversation
see #287 (comment) |
recipient = models.ForeignKey( | ||
Creator, on_delete=models.CASCADE, null=True, related_name="notification_recipient", blank=True) | ||
source = models.ForeignKey( | ||
Creator, on_delete=models.CASCADE, null=True, related_name="notification_source", blank=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think source
pointing to the Creator
model is a little bit undesirable. We need to think about the bigger picture. Notification is the place where all the information the Zubhub APP wants to communicate to the user lives. It can be a registration welcome message, email verification reminder/message (or phone verification reminder) or a whole bunch of things that are not related to another user.
I think source should be removed entirely. All notifications are messages from Zubhub app. Any information about the reason for the notification will be included in the notification message itself. We can parse this message if we need to do fancy stuffs (like link to the profile of the user that liked your project, etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand what you're saying but we were more concerned about getting the profile pic and username for certain notification messages. For example, if a notification stated "<user_name> started following you" we could reference that user in source
and have "started following you" in the message
. This would allow us to have the profile picture and username in the notification message and also link to their profile when clicked on. Do you know how we might be able to get something like the user's profile picture without using a source
field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simply adding the username of the user that followed another user in the notification message is enough to get all the extra interactions we need. The profile picture is auto-generated based on the username, so having the username is enough to get the profile picture. as for linking to the profile, there is a function in the frontend that parses strings and can detect the presence of sub-strings with a particular format (@username). With this we can format the incoming notification string and replace @username with <a href="link to profile">username</a>
.
Of course if a user changes their username, then the profile can no longer be found. But this is not much of a problem since a username is not something that changes often and if it does, we can simply report that the user profile is not found. The bigger issue is restricting the notification model to just user-to-user notifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will also think more about this to see if there is a better way. But I'm sure source pointing to Creator model is not a good design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point in the future, it seems logical to allow users to upload their own profile pics and in that case, this wouldn't work. Some of the concerns you've outlined with your design are also valid so we should try to find a better design that minimizes bugs and allows for feature additions in the future.
Would it be ok to make the source
field NULL for non-user-generated notifications and also add a notification_type
field to the model. This would be an enum that could hold values such as follow
, like
, email_verification
so that the frontend would know how to appropriately display the notification. Depending on whether the notification_type
is email_verification
or something different, the frontend could choose to show an icon that appropriately reflects it. There don't seem to be too many notification use cases so this enum wouldn't end up being too long.
@@ -66,6 +76,9 @@ class Creator(AbstractUser): | |||
role = models.PositiveSmallIntegerField( | |||
choices=ROLE_CHOICES, blank=True, null=True, default=CREATOR) | |||
search_vector = SearchVectorField(null=True) | |||
contact = models.PositiveSmallIntegerField( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
contact should be inside the Settings model since it's something that is not directly related to user but is more like a settings stuff.
app_name = "notifications" | ||
|
||
urlpatterns = [ | ||
path('', NoticationListAPIView.as_view(), name='list_notifications'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will this endpoint be used for? I don't see us ever needing to fetch all notifications in the database.
name='update_notification'), | ||
path('<uuid:pk>/delete/', DeleteNotificationAPIView.as_view(), | ||
name='delete_notification'), | ||
path('<uuid:pk>/notifications/', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notification is a personal stuff and a user should only be able to fetch their own notifications. for a user to be able to fetch their notifications, they need to be logged in. If a user is logged in, we know who the user is and we can get their username and id from requests.user
. uuid:pk is redundant here since we already have the user's information in the request since they are logged in.
class NoticationListAPIView(ListAPIView): | ||
queryset = Notification.objects.all() | ||
serializer_class = NotificationSerializer | ||
permission_classes = [AllowAny] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of NotificationListAPIView should be removed.
permission_classes = [AllowAny] | ||
|
||
|
||
class UpdateNotificationAPIView(UpdateAPIView): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to MarkNotificationAsViewedAPIView
or something similar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add throttling
queryset = Notification.objects.all() | ||
serializer_class = NotificationSerializer | ||
|
||
def delete(self, request, *args, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove all of delete
method too. Django can handle single actions like deletion without much overriding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add throttling
@@ -0,0 +1,11 @@ | |||
def notification_changed(obj, instance): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe delete this function? it doesn't serve any purpose now. see other reviews to know why.
model = Notification | ||
fields = ( | ||
'id', 'message', 'recipient', 'source', 'viewed', 'date' | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
read_only_fields = ["id", "message", "recipient", "date"]
once a notification has been created, it shouldn't be that easy to change those fields assigned to read_only_fields.
It should only be possible to change the viewed field in notifications from False to True (not from True to False).
As for source field, see other reviews as to why I removed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should overide update
method here to check if a notification is viewed or not and if not viewed, set as viewed.
It feels like the whole notification stuff should be added into the creator app directly because it's too lean and depends a lot on the Creator model to be a standalone app on it's own. the Notification model can be added into creators/models.py, same with serializer and views. We shouldn't have a standalone notification app. |
zubhub_backend/zubhub/APIS/urls.py
Outdated
path('upload_file/', UploadFileAPIView, name="upload_file"), | ||
path('delete_file/', DeleteFileAPIView, name="delete_file"), | ||
path('upload_file_to_local/', UploadFileToLocalAPIView, | ||
======= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hello @zorazrr , you should fix this merge conflict when you can. for the purpose of having a consistent style for creating url endpoints, we changed all use of underscores to "-" e.g. upload_file was changed to upload-file.
creator = models.OneToOneField( | ||
Creator, on_delete=models.CASCADE, primary_key=True) | ||
subscribe = models.BooleanField(blank=True, default=False) | ||
contact = models.PositiveSmallIntegerField( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this @zorazrr, this is definitely better. Another suggestion, right now we send all notifications to both email and phone. With this, we can have a logic every time we send notifications that detect the user's preferred notification channels and only notify then through that channel (you can also create a task for this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In PR #306, @zorazrr is working on creating a helper function that will call one of the task functions (send_text, send_whatsapp, send_email) based on a user's channel preferences so that other functions just have to call this helper function and not worry about that detail. This function will also add the notification to the notifications model appropriately. My plan was to add this function in a utils.py file in the notifications folder. Do you feel that would be the right place for it?
# Register your models here. | ||
from .models import Notification | ||
|
||
admin.site.register(Notification) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can improve this a bit. Right now in the admin site this will probably just be displaying the default model str method return value. That a difficult ux for admins to handle since they'll probably have no idea about the context of the notification (to who, why, from where, etc). That improvement can be made later.
|
||
urlpatterns = [ | ||
path('<uuid:pk>/update/', MarkNotificationAsViewedAPIView.as_view(), | ||
name='update_notification'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when you can @zorazrr spend sometime converting all your underscore based urls to use "-" instead. I admit it was my fault for creating a mixed pattern before. I fixed the mixed pattern by refactoring all urls to not use underscores anymore. You can help me keep to this pattern by changing these too.
* initial commit * fix namings * Add update and delete requests * Add boilerplate stuff * finish all endpoints * Add source and preferred contact * modifications to notification endpoints * fix notification list view * hyphens * address merge conflict * Update start Co-authored-by: Zora Zhang <[email protected]> Co-authored-by: Grace Zhang <[email protected]> Co-authored-by: Aditya Jain <[email protected]> Co-authored-by: Zora Zhang <[email protected]>
* Create notifications folder in zubhub_backend/zubhub, begin implementing test functions * Move send_whatsapp function to adapters.py, create task in tasks.py, create send-notification path for testing, test get method in views * Create notifications folder in zubhub_backend/zubhub, begin implementing test functions * Move send_whatsapp function to adapters.py, create task in tasks.py, create send-notification path for testing, test get method in views * Use whatsapp phone in adapter * Fix whatsapp message sending * Fix bug in send_whatsapp function in adapters.py * Fix settings name * Notification Model and Endpoints (#286) * initial commit * fix namings * Add update and delete requests * Add boilerplate stuff * finish all endpoints * Add source and preferred contact * modifications to notification endpoints * fix notification list view * hyphens * address merge conflict * Update start Co-authored-by: Zora Zhang <[email protected]> Co-authored-by: Grace Zhang <[email protected]> Co-authored-by: Aditya Jain <[email protected]> Co-authored-by: Zora Zhang <[email protected]> * Add notification button components (#302) * Add notification button components * Fix popper positioning * Add popper arrow and inner content * Give dropdown box shadow * Add mobile view * Fix box shadow on arrow for dropdown panel * Fix coloring of unread notifications text * Add push notification util function and use CreatorSerializer (#316) * Add push notification util function and use CreatorSerializer for notification serializer * Add notification type field to notification model * Add type to notification serializer and add migration * Remove references to message property of notification * Add viewed to notification serializer * Use creator minimal serializer * Add new link property to notification model * Notification Helper Function (#306) * initial setup * add setting * fix the way to get setting * commit * finished and tested function * add original code back in * modify function to send mass text/email * Add channel verification Co-authored-by: Zora Zhang <[email protected]> Co-authored-by: Aditya Jain <[email protected]> * Fix migrations * Add several functions and maps for notifications logic (#324) * Add several functions and maps for notifications logic, not working * Finish notification sending function * Remove redundant check for valid notification channel for notification type * Send notifications when notification events occur * Recommend unused function * Add throttle classes back * Fix file extensions * Fix notification template prefix * Add message field to notification model to support notifications that mention project names * Properly create migrations and use char field * Use name instead of label for django integer choice class * Change notification link attribute to be a char field * Fix email templates for notifications * Notifications panel loading notifications complete (#341) * Notifications panel loading notifications complete * Use artifical loading for pages after 1 * Change padding * Add comment for recent notification threshold * Remove console log * Format * Remove unncessary space * Reset scroll on notification tab change * Add notification grouping code (#351) * Add notification grouping code * Use set to determine valid grouped notification types * Actually make the grouping work * Persist context for pushing notifications * Remove prints * Fix django db warning' * Return notification in all branch paths * No need to return notification * Notification Component for Notification Panel (#311) * Outline for Notification components and stylings * Testing notification * Complete majority of stylings for notification component, add dummy component and props to PageWrapper.jsx * Complete stylings for notification component, fix hovers! * Make small style changes to notification component * Begin connecting to backend using realistic parameters, not clean or very functional yet * Begin adapting notification panel to hold notification components * Finish stylings for notification component and message rendering * Finish up API methods to updated viewed notification, add token constant to component * Add logic for rendering notification timestamp, redirect link, and notification message * Clean up and reduce code, remove redunant or unused sections * Move renderTimeAgo and getNotification to new NotificationScripts.js file, simplify logic in scripts, remove viewComment from userActions.js, call API.viewNotification directly in component * Add proper date formatting using dFormatter, string truncation for notification message * Add double image design to nofications, add project titles to views, fix small format issue in comment template * Finish up issue, update notification templates for consitency, fix bug in project details useEffect * Update clapped web_many message to not include strong tags * Update web_many message templates to include project name * Fix url * Minor fixes, add mobile width dependency, update getMessage() to get consistent character limits, remove mock data from PageWrapper Co-authored-by: Aditya Jain <[email protected]> * Notification feature fix (#429) * Fix most notification feature bugs * Fix app bar on top of other elements * Fix panel zindex * Fix panel loading * Fix comment notif Co-authored-by: Aditya Jain <[email protected]> * Add quotes to template * linearize migrations * Fix import * Add project to template * Styling fixes * Delete .gitignore Co-authored-by: saxytony <[email protected]> Co-authored-by: AndrewLester <[email protected]> Co-authored-by: Zora Zhang <[email protected]> Co-authored-by: Zora Zhang <[email protected]> Co-authored-by: Grace Zhang <[email protected]> Co-authored-by: Zora Zhang <[email protected]> Co-authored-by: saxytony <[email protected]>
* Create notifications folder in zubhub_backend/zubhub, begin implementing test functions * Move send_whatsapp function to adapters.py, create task in tasks.py, create send-notification path for testing, test get method in views * Create notifications folder in zubhub_backend/zubhub, begin implementing test functions * Move send_whatsapp function to adapters.py, create task in tasks.py, create send-notification path for testing, test get method in views * Use whatsapp phone in adapter * Fix whatsapp message sending * Fix bug in send_whatsapp function in adapters.py * Fix settings name * Notification Model and Endpoints (#286) * initial commit * fix namings * Add update and delete requests * Add boilerplate stuff * finish all endpoints * Add source and preferred contact * modifications to notification endpoints * fix notification list view * hyphens * address merge conflict * Update start Co-authored-by: Zora Zhang <[email protected]> Co-authored-by: Grace Zhang <[email protected]> Co-authored-by: Aditya Jain <[email protected]> Co-authored-by: Zora Zhang <[email protected]> * Add notification button components (#302) * Add notification button components * Fix popper positioning * Add popper arrow and inner content * Give dropdown box shadow * Add mobile view * Fix box shadow on arrow for dropdown panel * Fix coloring of unread notifications text * Add push notification util function and use CreatorSerializer (#316) * Add push notification util function and use CreatorSerializer for notification serializer * Add notification type field to notification model * Add type to notification serializer and add migration * Remove references to message property of notification * Add viewed to notification serializer * Use creator minimal serializer * Add new link property to notification model * Notification Helper Function (#306) * initial setup * add setting * fix the way to get setting * commit * finished and tested function * add original code back in * modify function to send mass text/email * Add channel verification Co-authored-by: Zora Zhang <[email protected]> Co-authored-by: Aditya Jain <[email protected]> * Fix migrations * Add several functions and maps for notifications logic (#324) * Add several functions and maps for notifications logic, not working * Finish notification sending function * Remove redundant check for valid notification channel for notification type * Send notifications when notification events occur * Recommend unused function * Add throttle classes back * Fix file extensions * Fix notification template prefix * Add message field to notification model to support notifications that mention project names * Properly create migrations and use char field * Use name instead of label for django integer choice class * Change notification link attribute to be a char field * Fix email templates for notifications * Notifications panel loading notifications complete (#341) * Notifications panel loading notifications complete * Use artifical loading for pages after 1 * Change padding * Add comment for recent notification threshold * Remove console log * Format * Remove unncessary space * Reset scroll on notification tab change * Add notification grouping code (#351) * Add notification grouping code * Use set to determine valid grouped notification types * Actually make the grouping work * Persist context for pushing notifications * Remove prints * Fix django db warning' * Return notification in all branch paths * No need to return notification * Notification Component for Notification Panel (#311) * Outline for Notification components and stylings * Testing notification * Complete majority of stylings for notification component, add dummy component and props to PageWrapper.jsx * Complete stylings for notification component, fix hovers! * Make small style changes to notification component * Begin connecting to backend using realistic parameters, not clean or very functional yet * Begin adapting notification panel to hold notification components * Finish stylings for notification component and message rendering * Finish up API methods to updated viewed notification, add token constant to component * Add logic for rendering notification timestamp, redirect link, and notification message * Clean up and reduce code, remove redunant or unused sections * Move renderTimeAgo and getNotification to new NotificationScripts.js file, simplify logic in scripts, remove viewComment from userActions.js, call API.viewNotification directly in component * Add proper date formatting using dFormatter, string truncation for notification message * Add double image design to nofications, add project titles to views, fix small format issue in comment template * Finish up issue, update notification templates for consitency, fix bug in project details useEffect * Update clapped web_many message to not include strong tags * Update web_many message templates to include project name * Fix url * Minor fixes, add mobile width dependency, update getMessage() to get consistent character limits, remove mock data from PageWrapper Co-authored-by: Aditya Jain <[email protected]> * Notification feature fix (#429) * Fix most notification feature bugs * Fix app bar on top of other elements * Fix panel zindex * Fix panel loading * Fix comment notif Co-authored-by: Aditya Jain <[email protected]> * Add quotes to template * linearize migrations * Fix import * Add project to template * Styling fixes * Delete .gitignore Co-authored-by: saxytony <[email protected]> Co-authored-by: AndrewLester <[email protected]> Co-authored-by: Zora Zhang <[email protected]> Co-authored-by: Zora Zhang <[email protected]> Co-authored-by: Grace Zhang <[email protected]> Co-authored-by: Zora Zhang <[email protected]> Co-authored-by: saxytony <[email protected]>
Notification Model and Endpoints
Create notification model and corresponding endpoints
Closes #284
Changes
Create notification model
Create endpoints for
Add notification preference enum in creator model
Requests / Responses
Request
GET
/api/notifications/
Returns a list of notificationsGET
/api/notifications/[user-id]/notifications/
Returns a list of notifications associated with a certain userPOST
/[notification-id]/update/
Updates a notificationDELETE
/[notificaiton-id]/delete/
Deletes a notification