Added the device_tokens endpoint #329
Conversation
| before_filter :authenticate_user! | ||
|
|
||
| def create | ||
| @device_token = DeviceToken.all.build device_token_params.merge! user_id: current_user.id |
There was a problem hiding this comment.
is .all() really needed to instantiate a new DeviceToken?
There was a problem hiding this comment.
It's enough with a DeviceToken.new(...), isn't it?
| @@ -0,0 +1,3 @@ | |||
| window.TimeOverflowRegisterExpoDeviceToken = function (token) { | |||
| $.post('/device_tokens', { token: token }); | |||
| } | |||
There was a problem hiding this comment.
Maybe doc the "hack" (:trollface:) somewhere in the wiki and add a link there here?
| before_filter :authenticate_user! | ||
|
|
||
| def create | ||
| @device_token = DeviceToken.all.build device_token_params.merge! user_id: current_user.id |
There was a problem hiding this comment.
It's enough with a DeviceToken.new(...), isn't it?
| { | ||
| "to" => token, | ||
| "title" => @notification.title, | ||
| "body" => @notification.body |
There was a problem hiding this comment.
Please, use attr_readers for the ivars
| def broadcast | ||
| notification = PostNotification.new | ||
|
|
||
| @post.organization.users.each do |user| |
There was a problem hiding this comment.
I would decouple this class from the details of the data model by passing the list of users in the constructor instead. This way if we ever change the data model we don't need to touch the push notifications implementation.
| t.timestamps | ||
| end | ||
| add_index :device_tokens, [:user_id, :token] | ||
| add_index :device_tokens, [:user_id] |
There was a problem hiding this comment.
This one is useless due to the left-most thingie. Postgresql can use that one above when only needing user_id
| @@ -0,0 +1,19 @@ | |||
| module PushNotifications | |||
| class UsersBroadcasterService | |||
There was a problem hiding this comment.
you're still working on it, right? because I don't see this class used.
There was a problem hiding this comment.
this class will be used once we implement the logic behind sending push notifications to users :)
|
|
||
| t.timestamps | ||
| end | ||
| add_index :device_tokens, [:user_id, :token] |
|
|
||
| t.timestamps | ||
| end | ||
| add_index :device_tokens, [:user_id, :token], unique: true |
| @@ -0,0 +1,19 @@ | |||
| class DeviceTokensController < PostsController | |||
There was a problem hiding this comment.
Why is this controller inheriting from PostsController?
|
If I understand this correctly we're still missing
right? In that case I would remove the route from this PR, merge and have an upcoming PR adding the services. It feels like all we can do as a first step for push notifications is done here. Also, it will make it easier to review these services and classes later. |
|
Yup, I would also merge this, even if we there's nothing that sends notifications. I wouldn't remove the route though, since we could start registering mobile devices. |
In order to send push notifications we need a place to store the device tokens for each users. In this PR:
device_tokens) with a minimal API (just the create action).The future TimeOverflow mobile app will be implemented in this repo: https://github.com/coopdevs/timeoverflow-mobile-app 👶📱