-
Notifications
You must be signed in to change notification settings - Fork 102
feat: add push notifications #1704
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
base: dev
Are you sure you want to change the base?
Conversation
986c2f5
to
19100ed
Compare
This is great!!! Are you all done or still working on stuff? great work!! |
Everything is done, I'll try adding that feature 👍 |
51736f7
to
27ee277
Compare
73f2fd1
to
fa19346
Compare
cc9af14
to
6129d74
Compare
7df9991
to
f212a96
Compare
f219e26
to
9ca0850
Compare
|
848cc66
to
c134c08
Compare
@alanzhu0 everythings done 👍 |
Idk why it's failing I ran build_docs like 20 times |
to test:
|
7d829ab
to
d655102
Compare
d655102
to
f1b0bf8
Compare
4909092
to
852be7e
Compare
813a811
to
93640c4
Compare
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.
To do
@@ -20,3 +27,6 @@ class Meta: | |||
"is_secret": "This will prevent Ion administrators from viewing individual users' votes.", | |||
"is_election": "Enable election formatting and results features.", | |||
} | |||
|
|||
# We need to make sure the send_notification field doesn't look out of place on the form |
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.
out of order
def send_poll_notification(obj: Poll) -> None: | ||
"""Send a (Web)push notification asking all users who can see the poll to vote | ||
|
||
obj: The poll object |
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.
improve type annotation :Poll:
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 function to send poll push notification
devices = WebPushDevice.objects.filter(user__in=users) | ||
else: | ||
users = User.objects.filter(Q(groups__in=obj.groups.all()) & Q(push_notification_preferences__poll_notifications=True)) | ||
devices = WebPushDevice.objects.filter(user__in=users) |
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.
move devices out since it's shared in both code blocks
also
user_filter = Q(push_notification_preferences__poll_notifications=True)
else:
user_filter = Q(groups__in=obj.groups.all(), push_notification_preferences__poll_notifications=True)
users = User.objects.filter(user_filter).distinct() # if users are in multiple groups??
"eighth_waitlist_notifications": user.push_notification_preferences.eighth_waitlist_notifications, | ||
"glance_notifications": user.push_notification_preferences.glance_notifications, | ||
"announcement_notifications": user.push_notification_preferences.announcement_notifications, | ||
"poll_notifications": user.push_notification_preferences.poll_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.
use a for loop and getattr
additionally, maybe prefs._meta.get_fields()
?
to auto update this function when a new field is added to push notification user preferences
notification_fields = [
field.name for field in prefs._meta.get_fields()
if field.name.endswith('_notifications') and not field.is_relation
]
|
||
return redirect("preferences") | ||
elif request.POST.get("updatepushprefs").lower() == "true": | ||
push_notifications_options_form = save_push_notifications_options(request, user) |
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.
put this as the if statement first, and if updatepushprefs == "" as the else
also save request.POST.get("updatepushprefs", "").lower() as a variable and check that way
|
||
|
||
@shared_task | ||
def push_delayed_bus_notifications(bus_number: str) -> None: |
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.
send delayed bus push notifications
|
||
Returns: | ||
None, or the datetime indicating when the task is scheduled if return_result is 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.
see below for how schedule should be a separate function
obj: The announcement object | ||
|
||
""" | ||
|
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.
send_announcement_posted_push_notification
else: | ||
users = User.objects.filter(Q(groups__in=obj.groups.all()) & Q(push_notification_preferences__announcement_notifications=True)) | ||
devices = WebPushDevice.objects.filter(user__in=users) | ||
|
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.
can be optimized per below (see .distinct() comment)
"notify_post": ( | ||
"If this box is checked, students who have signed up for email " | ||
"notifications will receive an email " | ||
"and those who have signed up for push notifications will receive a " |
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.
separate email and push notifs out into two separate functions/arguments
Proposed changes
Brief description of rationale
Many of Ion's current notification features rely on sending emails but people check push notifications more often. This makes the PWA experience more intuitive especially on mobile
Notes
create_vapid_keys.py
) should be generated once and never changed, otherwise all push subscriptions will be invalidated and users will have to manually subscribe to push notifications againVideo of it working on windows
Video of it working on iOS