diff --git a/app/controllers/devices_controller.rb b/app/controllers/devices_controller.rb index 87d176602..b4baf48dc 100644 --- a/app/controllers/devices_controller.rb +++ b/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 diff --git a/app/services/notifications/new_comment/send.rb b/app/services/notifications/new_comment/send.rb index 04c253311..759d46412 100644 --- a/app/services/notifications/new_comment/send.rb +++ b/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 diff --git a/app/services/push_notifications/send.rb b/app/services/push_notifications/send.rb index 889ae89ed..036310b00 100644 --- a/app/services/push_notifications/send.rb +++ b/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 diff --git a/spec/requests/devices_spec.rb b/spec/requests/devices_spec.rb index 0680f96e4..3c7d5977d 100644 --- a/spec/requests/devices_spec.rb +++ b/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 diff --git a/spec/services/notifications/new_comment/send_spec.rb b/spec/services/notifications/new_comment/send_spec.rb index f16b36a77..528803160 100644 --- a/spec/services/notifications/new_comment/send_spec.rb +++ b/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 diff --git a/spec/services/push_notifications/send_spec.rb b/spec/services/push_notifications/send_spec.rb index 10ebf965c..15fc40e83 100644 --- a/spec/services/push_notifications/send_spec.rb +++ b/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