Change PushNotifications::Send to use user_ids (#13036)

* Changes PushNotifications::Send to use user_ids

* Changes devices#destroy to use unauthenticated_params

* Fix failing spec - new logic reverses order

* Apply suggestions from code review

Co-authored-by: Jamie Gaskins <jgaskins@gmail.com>

* Only send new_comment PN with Rpush behind FeatureFlag + spec tweak

* Add query order explicitly to avoid flakyness

* Review feedback - remove order clause in query & other tweaks

Co-authored-by: Jamie Gaskins <jgaskins@gmail.com>

app/controllers/devices_controller.rb

@@ -3,6 +3,7 @@ class DevicesController < ApplicationController
   # This replaces the Authenticated Users Pusher Beams solution.
   # See: https://github.com/forem/forem/pull/12419/files#r563906038
   before_action :authenticate_user!, only: [:create]
+  skip_before_action :verify_authenticity_token, only: [:destroy]
 
   rescue_from ActiveRecord::ActiveRecordError do |exc|
     render json: { error: exc.message, status: 422 }, status: :unprocessable_entity
@@ -18,7 +19,7 @@ class DevicesController < ApplicationController
   end
 
   def destroy
-    device = Device.find_by(id: params[:id])
+    device = Device.find_by(unauthenticated_params)
     unless device
       render json: { error: "Not Found", status: 404 }, status: :not_found
       return
@@ -42,4 +43,19 @@ class DevicesController < ApplicationController
       app_bundle: params[:app_bundle]
     }
   end
+
+  # Unauthenticated params are used for `destroy` because in the mobile apps
+  # we react to when a user "has logged out", meaning we no longer have the
+  # `current_user` to validate ownership of the Device. In this case we
+  # confirm the ownership if all the values from `unauthenticated_params`
+  # match a Device. This is guaranteed because the PN token is unique per app
+  # & device, i.e. only the real owner of the device is able to provide them.
+  def unauthenticated_params
+    {
+      user_id: params[:id],
+      token: params[:token],
+      platform: params[:platform],
+      app_bundle: params[:app_bundle]
+    }
+  end
 end

app/services/notifications/new_comment/send.rb

@@ -17,14 +17,14 @@ module Notifications
         return if comment.score.negative?
 
         user_ids = Set.new(comment_user_ids + subscribed_user_ids + top_level_user_ids + author_subscriber_user_ids)
+        user_ids.delete(comment.user_id)
 
         json_data = {
           user: user_data(comment.user),
           comment: comment_data(comment)
         }
 
-        targets = []
-        user_ids.delete(comment.user_id).each do |user_id|
+        user_ids.each do |user_id|
           Notification.create(
             user_id: user_id,
             notifiable_id: comment.id,
@@ -32,12 +32,26 @@ module Notifications
             action: nil,
             json_data: json_data,
           )
-
-          targets << "user-notifications-#{user_id}" if User.find_by(id: user_id)&.mobile_comment_notifications
         end
 
-        # Sends the push notification to Pusher Beams channels. Batch is in place to respect Pusher 100 channel limit.
-        targets.each_slice(100) { |batch| send_push_notifications(batch) }
+        targets = User.where(id: user_ids, mobile_comment_notifications: true).ids
+
+        # Pusher Beams uses named Pub/Sub channels instead of raw user_ids
+        target_channels = targets.map { |id| "user-notifications-#{id}" }
+        # Sends the push notification to Pusher Beams channels.
+        # Batch is in place to respect Pusher 100 channel limit.
+        target_channels.each_slice(100) { |batch| send_push_notifications(batch) }
+
+        if FeatureFlag.enabled?(:mobile_notifications)
+          # Send PNs using Rpush
+          url_path = Rails.application.routes.url_helpers.notifications_path(:comments)
+          PushNotifications::Send.call(
+            user_ids: targets,
+            title: "@#{comment.user.username}",
+            body: "Re: #{comment.parent_or_root_article.title.strip}",
+            payload: { url: URL.url(url_path) },
+          )
+        end
 
         return unless comment.commentable.organization_id
 

app/services/push_notifications/send.rb

@@ -1,11 +1,11 @@
 module PushNotifications
   class Send
-    def self.call(user:, title:, body:, payload:)
-      new(user: user, title: title, body: body, payload: payload).call
+    def self.call(user_ids:, title:, body:, payload:)
+      new(user_ids: user_ids, title: title, body: body, payload: payload).call
     end
 
-    def initialize(user:, title:, body:, payload:)
-      @user = user
+    def initialize(user_ids:, title:, body:, payload:)
+      @user_ids = user_ids
       @title = title
       @body = body
       @payload = payload
@@ -14,7 +14,7 @@ module PushNotifications
     def call
       return unless FeatureFlag.enabled?(:mobile_notifications)
 
-      @user.devices.find_each do |device|
+      Device.where(user_id: @user_ids).find_each do |device|
         device.create_notification(@title, @body, @payload)
       end
 

spec/requests/devices_spec.rb

@@ -46,17 +46,39 @@ RSpec.describe "Devices", type: :request do
     let(:device) { create(:device, user: user) }
 
     context "when device not found" do
+      let(:incomplete_params) do
+        {
+          platform: device.platform,
+          app_bundle: device.app_bundle
+        }
+      end
+
       it "returns an error" do
         delete "/users/devices/123"
         expect(response.status).to eq(404)
         expect(response.parsed_body["error"]).to eq("Not Found")
         expect(response.parsed_body["status"]).to eq(404)
       end
+
+      it "return an error if device id doesn't match params" do
+        delete "/users/devices/123", params: incomplete_params
+        expect(response.status).to eq(404)
+        expect(response.parsed_body["error"]).to eq("Not Found")
+        expect(response.parsed_body["status"]).to eq(404)
+      end
     end
 
     context "when device deleted" do
+      let(:params) do
+        {
+          token: device.token,
+          platform: device.platform,
+          app_bundle: device.app_bundle
+        }
+      end
+
       it "deletes the device" do
-        delete "/users/devices/#{device.id}"
+        delete "/users/devices/#{device.user.id}", params: params
         expect(user.devices.count).to eq(0)
         expect(response.status).to eq(204)
         expect(Device.find_by(id: device.id)).to be_nil

spec/services/notifications/new_comment/send_spec.rb

@@ -110,7 +110,9 @@ RSpec.describe Notifications::NewComment::Send, type: :service do
 
     channels = ["user-notifications-#{user2.id}", "user-notifications-#{user.id}"]
     payload = described_class.new(comment_sent).__send__(:push_notification_payload)
-    expect(Pusher::PushNotifications).to have_received(:publish_to_interests).with(interests: channels,
-                                                                                   payload: payload)
+    expect(Pusher::PushNotifications).to have_received(:publish_to_interests) do |block|
+      expect(block[:interests]).to match_array(channels)
+      expect(block[:payload]).to eq(payload)
+    end
   end
 end

spec/services/push_notifications/send_spec.rb

@@ -2,14 +2,23 @@ require "rails_helper"
 
 RSpec.describe PushNotifications::Send, type: :service do
   let(:user) { create(:user) }
+  let(:user2) { create(:user) }
   let(:params) do
     {
-      user: user,
+      user_ids: [user.id],
       title: "Alert",
       body: "some alert here",
       payload: ""
     }
   end
+  let(:many_targets_params) do
+    {
+      user_ids: [user.id, user2.id],
+      title: "Alert 2",
+      body: "some other alert",
+      payload: ""
+    }
+  end
 
   context "with feature flag set to false/off" do
     before { allow(FeatureFlag).to receive(:enabled?).with(:mobile_notifications).and_return(false) }
@@ -33,7 +42,7 @@ RSpec.describe PushNotifications::Send, type: :service do
     end
   end
 
-  context "with devices for user" do
+  context "with devices for one user" do
     before do
       allow(FeatureFlag).to receive(:enabled?).with(:mobile_notifications).and_return(true)
       allow(ApplicationConfig).to receive(:[]).with("RPUSH_IOS_PEM").and_return("dGVzdGluZw==")
@@ -55,4 +64,28 @@ RSpec.describe PushNotifications::Send, type: :service do
         .and change(PushNotifications::DeliverWorker.jobs, :size).by(1)
     end
   end
+
+  context "with devices for multiple users" do
+    before do
+      allow(FeatureFlag).to receive(:enabled?).with(:mobile_notifications).and_return(true)
+      allow(ApplicationConfig).to receive(:[]).with("RPUSH_IOS_PEM").and_return("dGVzdGluZw==")
+      allow(ApplicationConfig).to receive(:[]).with("COMMUNITY_NAME").and_return("Forem")
+      create(:device, user: user)
+      create(:device, user: user2)
+    end
+
+    it "creates a notification and enqueues it" do
+      expect { described_class.call(many_targets_params) }
+        .to change { Rpush::Client::Redis::Notification.all.count }.by(2)
+        .and change { PushNotifications::DeliverWorker.jobs.size }.by(1)
+    end
+
+    it "creates a single notification for each of the user's devices when they have multiple" do
+      create(:device, user: user)
+
+      expect { described_class.call(many_targets_params) }
+        .to change { Rpush::Client::Redis::Notification.all.count }.by(3)
+        .and change { PushNotifications::DeliverWorker.jobs.size }.by(1)
+    end
+  end
 end