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

Add push subscription infrastructure #356

Merged
merged 22 commits into from Aug 31, 2017

Conversation

Projects
None yet
2 participants
@nicksellen
Copy link
Member

nicksellen commented Aug 30, 2017

This implements #350

  • endpoints under /api/subscriptions/push/
  • supports create, get, list, delete
  • prevents duplicate subscriptions (unique on user+token)
  • on each conversation message send the message content to everyone but the author
  • new notifications for a given conversation will replace previous notifications on their phone (using tag feature)
  • will delete invalid subscriptions (if fcm returns InvalidRegistration message for that token)

Things it does not do that I am not so concerned to implement for now:

  • only send notifications if the user is not actively using the chat via websocket
@codecov

This comment has been minimized.

Copy link

codecov bot commented Aug 30, 2017

Codecov Report

Merging #356 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #356      +/-   ##
==========================================
+ Coverage    99.3%   99.33%   +0.03%     
==========================================
  Files         161      168       +7     
  Lines        4310     4505     +195     
  Branches      172      179       +7     
==========================================
+ Hits         4280     4475     +195     
  Misses         15       15              
  Partials       15       15
Impacted Files Coverage Δ
foodsaving/subscriptions/models.py 100% <100%> (ø) ⬆️
foodsaving/subscriptions/tests/test_fcm.py 100% <100%> (ø)
.../subscriptions/migrations/0002_pushsubscription.py 100% <100%> (ø)
foodsaving/subscriptions/fcm.py 100% <100%> (ø)
...ubscriptions/migrations/0003_auto_20170830_1355.py 100% <100%> (ø)
foodsaving/subscriptions/tests/test_receivers.py 100% <100%> (ø) ⬆️
foodsaving/subscriptions/receivers.py 100% <100%> (ø) ⬆️
foodsaving/subscriptions/serializers.py 100% <100%> (ø)
foodsaving/subscriptions/tests/test_api.py 100% <100%> (ø)
foodsaving/subscriptions/api.py 100% <100%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f680e03...501d0c5. Read the comment docs.

)
]

user = UserIdSerializer(

This comment has been minimized.

@nicksellen

nicksellen Aug 30, 2017

Member

Adding this was the only way I could get it to respect the UniqueTogetherValidator. I don't actually want the user info in the serializer, and adding/using UserIdSerializer was just to minimize the extra stupid data.

I suspect there is a better way to solve it.

This comment has been minimized.

@tiltec

tiltec Aug 30, 2017

Member

Seems a good workaround, maybe add a short comment in the code?


user = UserIdSerializer(
read_only=True,
default=CurrentUserDefault()

This comment has been minimized.

@nicksellen

nicksellen Aug 30, 2017

Member

This CurrentUserDefault is nice, we can replace some custom things we wrote to do that I think.

@nicksellen nicksellen requested a review from tiltec Aug 30, 2017

# add a message to the conversation
ConversationMessage.objects.create(conversation=conversation, content=content, author=author)

# we can't check it was received but the check_json_data above will at least check it sent the write info

This comment has been minimized.

@nicksellen

nicksellen Aug 30, 2017

Member

typo: write -> right

@tiltec
Copy link
Member

tiltec left a comment

Looks good, looking forward to merge this!

)
]

user = UserIdSerializer(

This comment has been minimized.

@tiltec

tiltec Aug 30, 2017

Member

Seems a good workaround, maybe add a short comment in the code?

@nicksellen nicksellen force-pushed the add-push-subscriptions branch 2 times, most recently from 6a99361 to 9e1e163 Aug 31, 2017

@nicksellen nicksellen changed the title Add push subscription infrastucture Add push subscription infrastructure Aug 31, 2017

@nicksellen nicksellen force-pushed the add-push-subscriptions branch from 9e1e163 to 501d0c5 Aug 31, 2017

@tiltec tiltec merged commit 7060532 into master Aug 31, 2017

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
ci/circleci-e2e end-to-end test suite
Details
codecov/patch 100% of diff hit (target 99.3%)
Details
codecov/project 99.33% (+0.03%) compared to f680e03
Details

@tiltec tiltec deleted the add-push-subscriptions branch Aug 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment