Revamp Notif Logic #324
Revamp Notif Logic #324
Conversation
| is_dev = models.BooleanField(default=False) | ||
|
|
||
| class AndroidNotificationToken(NotificationToken): | ||
| pass |
There was a problem hiding this comment.
would an is_dev field not make sense here too? I don't know the Android notif system but would be surprised if they didn't have similar optionality to iOS.
There was a problem hiding this comment.
after talking to android didn't seem like they had
backend/user/models.py
Outdated
| enabled = models.BooleanField(default=True) | ||
|
|
||
| # service = models.CharField(max_length=30, choices=SERVICE_OPTIONS, default=SERVICE_PENN_MOBILE) | ||
| # enabled = models.BooleanField(default=True) |
There was a problem hiding this comment.
I assume you'll delete these commented out code once you merge the PR?
backend/user/notifications.py
Outdated
| return success_users, [] | ||
| failed_users = list(set(users) - set(success_users)) | ||
| return success_users, failed_users | ||
| send_immediate_notifications(tokens, title, body, category, is_dev, is_shadow) |
There was a problem hiding this comment.
thoughts on doing:
params = (tokens, title, body, category, is_dev, is_shadow)
if delay:
send_delayed_notifications(*params, delay=delay)
else:
send_immediate_notifications(*params)
There was a problem hiding this comment.
I feel like its overkill, we want to supply an easy to use interface, forcing the user to pass a tuple is more inconvenient.
I mean you can just take it in as args or kwargs and just shove it in that way? but um, this current one feels safer? maybe?
There was a problem hiding this comment.
this wouldn't change the interface of the functions? note the *params, its just being deconstructed.
backend/user/serializers.py
Outdated
| # def update(self, instance, validated_data): | ||
| # if instance.service != validated_data["service"]: | ||
| # raise serializers.ValidationError(detail={"detail": "Cannot change setting service."}) | ||
| # return super().update(instance, validated_data) |
There was a problem hiding this comment.
just a note so we can delete this code before merge PR?
| Return notification tokens of user. | ||
| """ | ||
|
|
||
| class NotificationTokenView(APIView, ABC): |
| class NotificationServiceView(APIView): | ||
| permission_classes = [IsAuthenticated] | ||
|
|
||
| # TODO: this is becoming a common pattern, consider abstracting |
There was a problem hiding this comment.
nah this seems low low priority, will be different PR
backend/user/views.py
Outdated
|
|
||
| users_with_service = service_obj.enabled_users.filter(username__in=usernames) | ||
|
|
||
| tokens = list(IOSNotificationToken.objects.filter(user__in=users_with_service, is_dev=is_dev).values_list( |
There was a problem hiding this comment.
just an opinion but probably prettier to extract this into a get_tokens() method within the post method but I defer to your judgement
There was a problem hiding this comment.
ifl its fine because its just doing 1 thing. Was going to with the 2 lines but then realized I need users_with_service later.
I think we do right? I changed the user relation on token to be a foreign key. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #324 +/- ##
==========================================
- Coverage 90.83% 90.59% -0.24%
==========================================
Files 63 61 -2
Lines 2629 2552 -77
==========================================
- Hits 2388 2312 -76
+ Misses 241 240 -1 ☔ View full report in Codecov by Sentry. |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Refactors notification logic, allow for multiple tokens per user, restructure to support android (very soon), make views cleaner, etc.
For future PR: fix remaining test cases and management commands.