Push notifications background job for Post #367

enricostano · 2018-05-25T15:50:57Z

WAT

Whit this PR we add a new step in the PostPersister service while creating or updating a Post: we also enqueue now a CreatePushNotificationsJob.

The background job will retrieve the Event and the Post associated to the event and will create as many PushNotification resources as needed.

In the case of Post it will create a PushNotification for each user in the Post's organization that have a DeviceToken. In future PRs we'll add this feature for Member and Transfer resources.

This PR only creates the PushNotification resources, it doesn't send any push notification to the mobile devices yet. The plan is to add a scheduled background job that will send them in batches.

What to test

The following workflow:

a user creates a Post
in the DB will be created as many PushNotification records as the people in the organization with a DeviceToken

TODO

Setup Redis in Travis CI or just use test engine for ActiveJob

sauloperez

Looks quite good to me, besides a few small details I'm happy to postpone my only design concern in an upcoming PR when we have a better understanding of the notifications we need to send. I just left it here for the record.

sauloperez · 2018-05-28T07:02:15Z

app/jobs/create_push_notifications_job.rb

+
+    raise 'A valid Event must be provided' unless event
+
+    ::PushNotifications::Creator.create!(event: event)


Do we really need the leading ::? This reference lives in the global namespace so I don't see why PushNotifications::Creator would have any problem finding the constant.

I'm just used to it, it makes it easier to ruby to find constants. Especially in Rails the constants lookup can be really "kafkian"...

sauloperez · 2018-05-28T07:07:22Z

app/models/push_notification.rb

+class PushNotification < ActiveRecord::Base
+  belongs_to :event, foreign_key: 'event_id'
+
+  validates_presence_of :event


Please, use the (latest) validates syntax instead. We don't know how long the old syntax will be maintained.

sauloperez · 2018-05-28T07:13:33Z

spec/models/push_notification_spec.rb

+    it { is_expected.to validate_presence_of(:event) }
+  end
+
+  describe 'Relations' do


I'd rather name it association which is the convention from Rails. The way I see it we're are testing it from the AR's perspective.

sauloperez · 2018-05-28T07:15:21Z

app/services/push_notifications/event_notifier/post_logic.rb

@@ -0,0 +1,24 @@
+module PushNotifications
+  class EventNotifier


I don't follow why we're nesting this PostLogic here? what's the actual reasoning? What confuses me I think is the fact that EventNotifier is both a class and a namespace 🤔

That's correct, I could have put PostLogic class even in the same file where EventNotifier is defined... but I don't like it much. PostLogic basically is a local business domain, only used inside EventNotifier... at least for now...

sauloperez · 2018-05-28T07:16:17Z

app/services/push_notifications/event_notifier/post_logic.rb

@@ -0,0 +1,24 @@
+module PushNotifications
+  class EventNotifier
+    class PostLogic


I would try to find a better name for this. PostLogic is rather vague and as a reader, doesn't tell me much of what to expect from this class.

Yeah, I don't like the name either... but I wanted to move forward... let's think about a better name later.

sauloperez · 2018-05-28T07:17:16Z

app/services/push_notifications/event_notifier/post_logic.rb

+
+      # Conditions for Post:
+      #  - All the members of the associated organization
+      #  - that have a DeviceToken associated


this second bullet point looks like the continuation of the previous one ⚠️ . It isn't needed, right?

I'll rewrite this doc, it's an AND basically...

sauloperez · 2018-05-28T07:22:32Z

spec/services/push_notifications/event_notifier/post_logic_spec.rb

+
+      context 'and the user has a DeviceToken associated' do
+
+        before { Fabricate(:device_token, user: user, token: 'aloha') }


the spacing up until here makes it a bit hard to read. I'd simply keep the before's right after their context.

sauloperez · 2018-05-28T07:23:13Z

spec/services/push_notifications/event_notifier/post_logic_spec.rb

+        end
+      end
+
+      context 'but the user has not a DeviceToken associated' do


⚠️ "has no Device Token"

sauloperez · 2018-05-28T07:35:28Z

app/services/push_notifications/event_notifier.rb

+
+      raise 'The resource associated to the Event is not supported'
+    end
+  end


To me, this feels like a Factory. It knows what class to instantiate and but it also delegates #users to the underlying class.

We can avoid this second unnecessary responsibility from EventNotifier by limiting it to return the PostLogic instance. This class is the one that actually implements #users. So something like:

class Creator def create! event_notifier = EventNotifierFactory.notification_for(event: event) event_notifier.users.each do PushNotification.create!(event: event) end end end

The idea here is that all kinds of event_notifier we come up with conform to the duck-type by implementing #users.

Yeah, we need to play a bit more with the relationship between PostLogic and EventNotifier

Look at PostLogic class as a way to organize things inside EventNotifier business logic. Users outside EventNotifier shouldn't know anything about it, they just ask #users to EventNotifier.

sauloperez

Looks quite good to me, besides a few small details I'm happy to postpone my only design concern for an upcoming PR when we have a better understanding of the notifications we need to send. I just left it here for the record.

sauloperez · 2018-05-28T13:51:24Z

Looks very good 👌

enricostano · 2018-05-29T15:11:18Z

Tested on staging.

enricostano added the needs review label May 25, 2018

enricostano requested review from mllocs and sauloperez May 25, 2018 15:53

enricostano self-assigned this May 25, 2018

sauloperez reviewed May 28, 2018

View reviewed changes

sauloperez approved these changes May 28, 2018

View reviewed changes

enricostano force-pushed the feature/create-push-notifications-job branch 2 times, most recently from 9cff2ec to 4dc7d64 Compare May 28, 2018 08:42

Remove unused class

39ff9d5

enricostano force-pushed the feature/create-push-notifications-job branch 2 times, most recently from b4b4fcc to 4a493c7 Compare May 28, 2018 11:43

sauloperez approved these changes May 28, 2018

View reviewed changes

Add PushNotification model

65a83a7

enricostano force-pushed the feature/create-push-notifications-job branch from ec3dc54 to 0f89488 Compare May 28, 2018 16:25

enricostano added needs testing and removed needs review labels May 29, 2018

enricostano force-pushed the feature/create-push-notifications-job branch from 0f89488 to 6e64400 Compare May 29, 2018 14:19

enricostano added 7 commits May 29, 2018 16:52

Add CreatePushNotifications job

add6a06

Add fabricators for DeviceToken and Event

ab9d641

Add PushNotification creation logic

e298a3c

Enqueue PushNotification background job on Post persistence

44211da

Run background jobs inline in test environment

6f210fc

Better background job expectation

7747798

Remove unique index on events

a2d4b8c

enricostano force-pushed the feature/create-push-notifications-job branch from b038ab8 to a2d4b8c Compare May 29, 2018 14:52

enricostano removed the needs testing label May 29, 2018

sauloperez approved these changes May 29, 2018

View reviewed changes

enricostano merged commit ec0f47d into develop May 29, 2018

enricostano deleted the feature/create-push-notifications-job branch May 29, 2018 16:39

enricostano mentioned this pull request Aug 8, 2018

Release v1.6.0 #394

Merged


		raise 'A valid Event must be provided' unless event

		::PushNotifications::Creator.create!(event: event)


		context 'and the user has a DeviceToken associated' do

		before { Fabricate(:device_token, user: user, token: 'aloha') }

Push notifications background job for Post #367

Push notifications background job for Post #367

Uh oh!

Conversation

enricostano commented May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WAT

What to test

TODO

Uh oh!

sauloperez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enricostano May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sauloperez May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sauloperez left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sauloperez commented May 28, 2018

Uh oh!

enricostano commented May 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

enricostano commented May 25, 2018 •

edited

Loading

enricostano May 28, 2018 •

edited

Loading

sauloperez May 28, 2018 •

edited

Loading

sauloperez left a comment •

edited

Loading