Scheduled job to send push notifications for Post resource #377

enricostano · 2018-06-04T15:27:54Z

WAT

The workflow is the following:

Every time a Post is created or updated we create an Event
For each Event we create as many PushNotification as needed
Every 5 minutes the SendPushNotificationsJob collect all the PushNotification that have not been processed yet and it sends them

Test

All members with a mobile device associated to their account will receive a push notification for create and update of offers a inquiries

TODO

Mark PushNotification as processed
Add body to notifications https://github.com/coopdevs/timeoverflow/blob/feature/scheduled-jobs/app/services/push_notifications/broadcast.rb#L35
Check notifications title
When clicking on the notification the app should go to the Post's URL

sauloperez

We reduced complexity a great deal now 👏

sauloperez · 2018-06-06T07:12:23Z

app/services/push_notifications/broadcast.rb

+    end
+
+    def send
+      return unless push_notifications.any?


You told me Exponent::Push::Client does not check this and so it blows up if notifications is empty, right? It would make things simpler if they did.

Yep, but they don't :(

sauloperez · 2018-06-06T07:13:33Z

app/services/push_notifications/broadcast.rb

+      push_notifications.map do |push_notification|
+        {
+          'to' => push_notification.to,
+          'title' => push_notification.title


does Exponent::Push::Client requires the keys to be strings? It'd be great if we could use symbols.

It should be bossible 👍

sauloperez · 2018-06-06T07:14:37Z

config/schedule.yml

@@ -0,0 +1,4 @@
+send_push_notifications_job:
+  cron: '*/5 * * * *'


can you remind me the syntax?

It's every 5 minutes, but we still have to take a decision about this.

enricostano · 2018-06-06T15:01:03Z

db/migrate/20180604145622_add_title_to_push_notification.rb

+    add_column :push_notifications, :title, :string, null: false, default: 'Title for existing records'
+    change_column_default(:push_notifications, :title, nil)
+  end
+end


The migration will add a DEFAULT first and then remove it to allow the creation of a NOT NULL column on existing records (more info).

sauloperez · 2018-06-20T18:38:54Z

Gemfile.lock

-      elasticsearch-api (= 1.0.7)
-      elasticsearch-transport (= 1.0.7)
-    elasticsearch-api (1.0.7)
+    elasticsearch (5.0.5)


we need to take extra care about search when we test this PR 🙈

sauloperez · 2018-06-20T18:40:21Z

app/services/push_notifications/broadcast.rb

+
+      client = Exponent::Push::Client.new
+
+      push_notifications.update_all(processed_at: Time.now.utc)


I'd rather update the processed_at after publishing the notifications just in case that fails.

We discussed about this and it's simpler this way, push notifications are not really important. You can loose one.

markets

👍 @enricostano that Push Notifications thing is almost complete no? Maybe we should integrate #381 first (now that we have #356 also merged) and we'll ready to

enricostano · 2018-08-20T10:43:42Z

that Push Notifications thing is almost complete no? Maybe we should integrate #381 first (now that we have #356 also merged) and we'll ready to

@markets done here #408

This reverts commit 13ff3d1.

mllocs · 2018-08-26T12:17:38Z

db/schema.rb

@@ -164,6 +164,7 @@
    t.datetime "processed_at"
    t.datetime "created_at",      null: false
    t.datetime "updated_at",      null: false
+    t.string   "title",           null: false


why do you need a title? isn't all the info in the Event?

We need to store it in the notification. The Event doesn't understand anything about notifications.

We'll need to do the same for body

mllocs · 2018-08-26T12:24:53Z

app/jobs/send_push_notifications_job.rb

+  queue_as :cron
+
+  def perform
+    push_notifications = PushNotification.where(processed_at: nil)


I would filter out old PushNotifications that may not be relevant anymore

We'll use the processed_at to do that.

there's a limit of 100 messages per request: https://docs.expo.io/versions/latest/guides/push-notifications#http2-api

mllocs · 2018-08-26T12:25:47Z

app/models/push_notification.rb

-  validates :event, :device_token, presence: true
+  validates :event, :device_token, :title, presence: true
+
+  def to


mllocs · 2018-08-26T12:27:12Z

app/services/push_notifications/creator.rb

+        PushNotification.create!(
+          event: event,
+          device_token: device_token,
+          title: title


do we need to dupe this info? it is already stored in the Event

No, it's not. We'll need to do the same for body also.

mllocs · 2018-08-28T16:17:53Z

app/services/push_notifications/broadcast.rb

+        raise PostError, "HTTP response: #{response.code}, #{response.body}"
+      end
+
+      push_notifications.update_all(processed_at: Time.now.utc)


would you update updated_at as well?

mllocs · 2018-08-28T16:42:00Z

Relevant: https://blog.expo.io/upcoming-changes-to-the-expo-push-notification-service-summer-2018-254030da786b

enricostano · 2018-08-31T18:51:40Z

db/migrate/20180604145622_add_title_to_push_notification.rb

+class AddTitleToPushNotification < ActiveRecord::Migration
+  def up
+    add_column :push_notifications, :title, :string, null: false, default: 'Title for existing records'
+    change_column_default(:push_notifications, :title, nil)


Why this? We already set a default in the previous line, also we have a not null constrain.

hehehe, this code is yours and was about to ask the same
:D 8069d51#r193443513

THAT'S IMPOSSIBLE

enricostano · 2018-08-31T18:52:00Z

db/migrate/20180828160700_add_body_to_push_notification.rb

+class AddBodyToPushNotification < ActiveRecord::Migration
+  def up
+    add_column :push_notifications, :body, :string, null: false, default: 'Body for existing records'
+    change_column_default(:push_notifications, :body, nil)


Why this? We already set a default in the previous line, also we have a not null constrain.

enricostano · 2018-08-31T18:52:13Z

db/migrate/20180831161349_add_data_to_push_notification.rb

+class AddDataToPushNotification < ActiveRecord::Migration
+  def up
+    add_column :push_notifications, :data, :json, null: false, default: '{}'
+    change_column_default(:push_notifications, :data, nil)


Why this? We already set a default in the previous line, also we have a not null constrain.

mllocs · 2018-09-03T19:45:36Z

The mobile app part: coopdevs/timeoverflow-mobile-app#3

sauloperez

Looks really good! 👏

enricostano added the wip label Jun 4, 2018

enricostano self-assigned this Jun 4, 2018

enricostano requested review from sauloperez and mllocs June 4, 2018 16:32

sauloperez approved these changes Jun 6, 2018

View reviewed changes

enricostano force-pushed the feature/scheduled-jobs branch from 42058f3 to fb9d8b9 Compare June 6, 2018 13:35

enricostano added needs review needs testing and removed wip labels Jun 6, 2018

enricostano force-pushed the feature/scheduled-jobs branch from fb9d8b9 to cc6cea4 Compare June 6, 2018 14:59

enricostano commented Jun 6, 2018

View reviewed changes

enricostano force-pushed the feature/scheduled-jobs branch 2 times, most recently from 91626ca to dce49e0 Compare June 12, 2018 15:45

enricostano force-pushed the feature/scheduled-jobs branch from 1b6b3f4 to ed4b3a5 Compare June 19, 2018 10:42

sauloperez requested changes Jun 20, 2018

View reviewed changes

markets approved these changes Jun 21, 2018

View reviewed changes

enricostano added 9 commits August 20, 2018 12:45

Add PushNotification#to

ea38581

Add title to PushNotification

8069d51

Add PushNotification fabricator

2e8b31d

Add token to DeviceToken fabricator

ae9ef90

Create new PushNotifications with title

cb16c03

Remove unused services

2352b79

Add scheduled job to send push notifications

67c4fe8

Upgrade elasticsearch gems

13ff3d1

Flag push_notifications as processed

5c114f4

enricostano force-pushed the feature/scheduled-jobs branch from ed4b3a5 to 5c114f4 Compare August 20, 2018 10:49

Set notifications as processed after publish

9313474

mllocs self-assigned this Aug 22, 2018

Revert "Upgrade elasticsearch gems"

005e683

This reverts commit 13ff3d1.

enricostano added 5 commits August 23, 2018 10:04

Remove Expo client gem

20eb8cb

Use standard ruby net http instead of Expo client

7751766

#send is a word reserved to Ruby... :boom

e247110

Fix notifications broadcast

2698747

Use Post title as notification title

d94684e

mllocs reviewed Aug 27, 2018

View reviewed changes

mllocs reviewed Aug 28, 2018

View reviewed changes

enricostano commented Aug 31, 2018

View reviewed changes

mllocs added 2 commits September 3, 2018 20:43

Fixed migrations with defaults

fee1aac

Refactor PushNotificatiosn::Creator with a Base class

bcb8684

mllocs force-pushed the feature/scheduled-jobs branch from 533545c to bcb8684 Compare September 3, 2018 19:44

sauloperez approved these changes Sep 4, 2018

View reviewed changes

sauloperez removed the needs review label Sep 4, 2018

mllocs mentioned this pull request Sep 4, 2018

Handling notifications in the app coopdevs/timeoverflow-mobile-app#3

Merged

enricostano removed the needs testing label Sep 11, 2018

enricostano merged commit 22a26c9 into develop Sep 11, 2018

enricostano deleted the feature/scheduled-jobs branch September 11, 2018 11:36

sauloperez mentioned this pull request Sep 20, 2018

Release v1.9.0 #429

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scheduled job to send push notifications for Post resource #377

Scheduled job to send push notifications for Post resource #377

enricostano commented Jun 4, 2018 •

edited by mllocs

Loading

sauloperez left a comment

sauloperez Jun 6, 2018

enricostano Jun 6, 2018

sauloperez Jun 6, 2018

enricostano Jun 6, 2018

sauloperez Jun 6, 2018

enricostano Jun 6, 2018 •

edited

Loading

enricostano Jun 6, 2018

sauloperez Jun 20, 2018

sauloperez Jun 20, 2018

enricostano Jun 20, 2018

markets left a comment •

edited

Loading

enricostano commented Aug 20, 2018

mllocs Aug 26, 2018

enricostano Aug 27, 2018

enricostano Aug 27, 2018

mllocs Aug 26, 2018

enricostano Aug 27, 2018

mllocs Aug 28, 2018

mllocs Aug 26, 2018

enricostano Aug 27, 2018

mllocs Aug 26, 2018

enricostano Aug 27, 2018

mllocs Aug 28, 2018

mllocs commented Aug 28, 2018

enricostano Aug 31, 2018

mllocs Sep 3, 2018

enricostano Sep 11, 2018

enricostano Aug 31, 2018

enricostano Aug 31, 2018

mllocs commented Sep 3, 2018

sauloperez left a comment

		@@ -0,0 +1,4 @@
		send_push_notifications_job:
		cron: '/5 * * *'


		client = Exponent::Push::Client.new

		push_notifications.update_all(processed_at: Time.now.utc)

Scheduled job to send push notifications for Post resource #377

Scheduled job to send push notifications for Post resource #377

Conversation

enricostano commented Jun 4, 2018 • edited by mllocs Loading

WAT

Test

TODO

sauloperez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enricostano Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markets left a comment • edited Loading

Choose a reason for hiding this comment

enricostano commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mllocs commented Aug 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mllocs commented Sep 3, 2018

sauloperez left a comment

Choose a reason for hiding this comment

enricostano commented Jun 4, 2018 •

edited by mllocs

Loading

enricostano Jun 6, 2018 •

edited

Loading

markets left a comment •

edited

Loading