Add policy param to POST /api/v1/push/subscriptions (#16040)

With possible values `all`, `followed`, `follower`, and `none`,
control from whom notifications will generate a Web Push alert

app/controllers/api/v1/push/subscriptions_controller.rb

@@ -3,13 +3,13 @@
 class Api::V1::Push::SubscriptionsController < Api::BaseController
   before_action -> { doorkeeper_authorize! :push }
   before_action :require_user!
-  before_action :set_web_push_subscription
+  before_action :set_push_subscription
-  before_action :check_web_push_subscription, only: [:show, :update]
+  before_action :check_push_subscription, only: [:show, :update]
 
   def create
-    @web_subscription&.destroy!
+    @push_subscription&.destroy!
 
-    @web_subscription = ::Web::PushSubscription.create!(
+    @push_subscription = Web::PushSubscription.create!(
       endpoint: subscription_params[:endpoint],
       key_p256dh: subscription_params[:keys][:p256dh],
       key_auth: subscription_params[:keys][:auth],
@@ -18,31 +18,31 @@ class Api::V1::Push::SubscriptionsController < Api::BaseController
       access_token_id: doorkeeper_token.id
     )
 
-    render json: @web_subscription, serializer: REST::WebPushSubscriptionSerializer
+    render json: @push_subscription, serializer: REST::WebPushSubscriptionSerializer
   end
 
   def show
-    render json: @web_subscription, serializer: REST::WebPushSubscriptionSerializer
+    render json: @push_subscription, serializer: REST::WebPushSubscriptionSerializer
   end
 
   def update
-    @web_subscription.update!(data: data_params)
+    @push_subscription.update!(data: data_params)
-    render json: @web_subscription, serializer: REST::WebPushSubscriptionSerializer
+    render json: @push_subscription, serializer: REST::WebPushSubscriptionSerializer
   end
 
   def destroy
-    @web_subscription&.destroy!
+    @push_subscription&.destroy!
     render_empty
   end
 
   private
 
-  def set_web_push_subscription
+  def set_push_subscription
-    @web_subscription = ::Web::PushSubscription.find_by(access_token_id: doorkeeper_token.id)
+    @push_subscription = Web::PushSubscription.find_by(access_token_id: doorkeeper_token.id)
   end
 
-  def check_web_push_subscription
+  def check_push_subscription
-    not_found if @web_subscription.nil?
+    not_found if @push_subscription.nil?
   end
 
   def subscription_params
@@ -52,6 +52,6 @@ class Api::V1::Push::SubscriptionsController < Api::BaseController
   def data_params
     return {} if params[:data].blank?
 
-    params.require(:data).permit(alerts: [:follow, :follow_request, :favourite, :reblog, :mention, :poll, :status])
+    params.require(:data).permit(:policy, alerts: [:follow, :follow_request, :favourite, :reblog, :mention, :poll, :status])
   end
 end

app/controllers/api/web/push_subscriptions_controller.rb

@@ -2,6 +2,7 @@
 
 class Api::Web::PushSubscriptionsController < Api::Web::BaseController
   before_action :require_user!
+  before_action :set_push_subscription, only: :update
 
   def create
     active_session = current_session
@@ -15,9 +16,11 @@ class Api::Web::PushSubscriptionsController < Api::Web::BaseController
     alerts_enabled = active_session.detection.device.mobile? || active_session.detection.device.tablet?
 
     data = {
+      policy: 'all',
+
       alerts: {
         follow: alerts_enabled,
-        follow_request: false,
+        follow_request: alerts_enabled,
         favourite: alerts_enabled,
         reblog: alerts_enabled,
         mention: alerts_enabled,
@@ -28,7 +31,7 @@ class Api::Web::PushSubscriptionsController < Api::Web::BaseController
 
     data.deep_merge!(data_params) if params[:data]
 
-    web_subscription = ::Web::PushSubscription.create!(
+    push_subscription = ::Web::PushSubscription.create!(
       endpoint: subscription_params[:endpoint],
       key_p256dh: subscription_params[:keys][:p256dh],
       key_auth: subscription_params[:keys][:auth],
@@ -37,27 +40,27 @@ class Api::Web::PushSubscriptionsController < Api::Web::BaseController
       access_token_id: active_session.access_token_id
     )
 
-    active_session.update!(web_push_subscription: web_subscription)
+    active_session.update!(web_push_subscription: push_subscription)
 
-    render json: web_subscription, serializer: REST::WebPushSubscriptionSerializer
+    render json: push_subscription, serializer: REST::WebPushSubscriptionSerializer
   end
 
   def update
-    params.require([:id])
+    @push_subscription.update!(data: data_params)
-
+    render json: @push_subscription, serializer: REST::WebPushSubscriptionSerializer
-    web_subscription = ::Web::PushSubscription.find(params[:id])
-    web_subscription.update!(data: data_params)
-
-    render json: web_subscription, serializer: REST::WebPushSubscriptionSerializer
   end
 
   private
 
+  def set_push_subscription
+    @push_subscription = ::Web::PushSubscription.find(params[:id])
+  end
+
   def subscription_params
     @subscription_params ||= params.require(:subscription).permit(:endpoint, keys: [:auth, :p256dh])
   end
 
   def data_params
-    @data_params ||= params.require(:data).permit(alerts: [:follow, :follow_request, :favourite, :reblog, :mention, :poll, :status])
+    @data_params ||= params.require(:data).permit(:policy, alerts: [:follow, :follow_request, :favourite, :reblog, :mention, :poll, :status])
   end
 end

app/models/web/push_subscription.rb

@@ -47,7 +47,7 @@ class Web::PushSubscription < ApplicationRecord
   end
 
   def pushable?(notification)
-    ActiveModel::Type::Boolean.new.cast(data&.dig('alerts', notification.type.to_s))
+    policy_allows_notification?(notification) && alert_enabled_for_notification_type?(notification)
   end
 
   def associated_user
@@ -100,4 +100,25 @@ class Web::PushSubscription < ApplicationRecord
   def contact_email
     @contact_email ||= ::Setting.site_contact_email
   end
+
+  def alert_enabled_for_notification_type?(notification)
+    truthy?(data&.dig('alerts', notification.type.to_s))
+  end
+
+  def policy_allows_notification?(notification)
+    case data&.dig('policy')
+    when nil, 'all'
+      true
+    when 'none'
+      false
+    when 'followed'
+      notification.account.following?(notification.from_account)
+    when 'follower'
+      notification.from_account.following?(notification.account)
+    end
+  end
+
+  def truthy?(val)
+    ActiveModel::Type::Boolean.new.cast(val)
+  end
 end

spec/controllers/api/v1/push/subscriptions_controller_spec.rb

@@ -27,20 +27,27 @@ describe Api::V1::Push::SubscriptionsController do
   let(:alerts_payload) do
     {
       data: {
+        policy: 'all',
+
         alerts: {
           follow: true,
+          follow_request: true,
           favourite: false,
           reblog: true,
           mention: false,
+          poll: true,
+          status: false,
         }
       }
     }.with_indifferent_access
   end
 
   describe 'POST #create' do
-    it 'saves push subscriptions' do
+    before do
       post :create, params: create_payload
+    end
 
+    it 'saves push subscriptions' do
       push_subscription = Web::PushSubscription.find_by(endpoint: create_payload[:subscription][:endpoint])
 
       expect(push_subscription.endpoint).to eq(create_payload[:subscription][:endpoint])
@@ -52,31 +59,34 @@ describe Api::V1::Push::SubscriptionsController do
 
     it 'replaces old subscription on repeat calls' do
       post :create, params: create_payload
-      post :create, params: create_payload
-
       expect(Web::PushSubscription.where(endpoint: create_payload[:subscription][:endpoint]).count).to eq 1
     end
   end
 
   describe 'PUT #update' do
-    it 'changes alert settings' do
+    before do
       post :create, params: create_payload
       put :update, params: alerts_payload
+    end
 
+    it 'changes alert settings' do
       push_subscription = Web::PushSubscription.find_by(endpoint: create_payload[:subscription][:endpoint])
 
-      expect(push_subscription.data.dig('alerts', 'follow')).to eq(alerts_payload[:data][:alerts][:follow].to_s)
+      expect(push_subscription.data['policy']).to eq(alerts_payload[:data][:policy])
-      expect(push_subscription.data.dig('alerts', 'favourite')).to eq(alerts_payload[:data][:alerts][:favourite].to_s)
+
-      expect(push_subscription.data.dig('alerts', 'reblog')).to eq(alerts_payload[:data][:alerts][:reblog].to_s)
+      %w(follow follow_request favourite reblog mention poll status).each do |type|
-      expect(push_subscription.data.dig('alerts', 'mention')).to eq(alerts_payload[:data][:alerts][:mention].to_s)
+        expect(push_subscription.data['alerts'][type]).to eq(alerts_payload[:data][:alerts][type.to_sym].to_s)
+      end
     end
   end
 
   describe 'DELETE #destroy' do
-    it 'removes the subscription' do
+    before do
       post :create, params: create_payload
       delete :destroy
+    end
 
+    it 'removes the subscription' do
       expect(Web::PushSubscription.find_by(endpoint: create_payload[:subscription][:endpoint])).to be_nil
     end
   end

spec/controllers/api/web/push_subscriptions_controller_spec.rb

@@ -22,11 +22,16 @@ describe Api::Web::PushSubscriptionsController do
   let(:alerts_payload) do
     {
       data: {
+        policy: 'all',
+
         alerts: {
           follow: true,
+          follow_request: false,
           favourite: false,
           reblog: true,
           mention: false,
+          poll: true,
+          status: false,
         }
       }
     }
@@ -59,10 +64,11 @@ describe Api::Web::PushSubscriptionsController do
 
         push_subscription = Web::PushSubscription.find_by(endpoint: create_payload[:subscription][:endpoint])
 
-        expect(push_subscription.data['alerts']['follow']).to eq(alerts_payload[:data][:alerts][:follow].to_s)
+        expect(push_subscription.data['policy']).to eq 'all'
-        expect(push_subscription.data['alerts']['favourite']).to eq(alerts_payload[:data][:alerts][:favourite].to_s)
+
-        expect(push_subscription.data['alerts']['reblog']).to eq(alerts_payload[:data][:alerts][:reblog].to_s)
+        %w(follow follow_request favourite reblog mention poll status).each do |type|
-        expect(push_subscription.data['alerts']['mention']).to eq(alerts_payload[:data][:alerts][:mention].to_s)
+          expect(push_subscription.data['alerts'][type]).to eq(alerts_payload[:data][:alerts][type.to_sym].to_s)
+        end
       end
     end
   end
@@ -81,10 +87,11 @@ describe Api::Web::PushSubscriptionsController do
 
       push_subscription = Web::PushSubscription.find_by(endpoint: create_payload[:subscription][:endpoint])
 
-      expect(push_subscription.data['alerts']['follow']).to eq(alerts_payload[:data][:alerts][:follow].to_s)
+      expect(push_subscription.data['policy']).to eq 'all'
-      expect(push_subscription.data['alerts']['favourite']).to eq(alerts_payload[:data][:alerts][:favourite].to_s)
+
-      expect(push_subscription.data['alerts']['reblog']).to eq(alerts_payload[:data][:alerts][:reblog].to_s)
+      %w(follow follow_request favourite reblog mention poll status).each do |type|
-      expect(push_subscription.data['alerts']['mention']).to eq(alerts_payload[:data][:alerts][:mention].to_s)
+        expect(push_subscription.data['alerts'][type]).to eq(alerts_payload[:data][:alerts][type.to_sym].to_s)
+      end
     end
   end
 end

spec/models/web/push_subscription_spec.rb

@@ -1,16 +1,94 @@
 require 'rails_helper'
 
 RSpec.describe Web::PushSubscription, type: :model do
-  let(:alerts) { { mention: true, reblog: false, follow: true, follow_request: false, favourite: true } }
+  let(:account) { Fabricate(:account) }
-  let(:push_subscription) { Web::PushSubscription.new(data: { alerts: alerts }) }
+
+  let(:policy) { 'all' }
+
+  let(:data) do
+    {
+      policy: policy,
+
+      alerts: {
+        mention: true,
+        reblog: false,
+        follow: true,
+        follow_request: false,
+        favourite: true,
+      },
+    }
+  end
+
+  subject { described_class.new(data: data) }
 
   describe '#pushable?' do
-    it 'obeys alert settings' do
+    let(:notification_type) { :mention }
-      expect(push_subscription.send(:pushable?, Notification.new(activity_type: 'Mention'))).to eq true
+    let(:notification) { Fabricate(:notification, account: account, type: notification_type) }
-      expect(push_subscription.send(:pushable?, Notification.new(activity_type: 'Status'))).to eq false
+
-      expect(push_subscription.send(:pushable?, Notification.new(activity_type: 'Follow'))).to eq true
+    %i(mention reblog follow follow_request favourite).each do |type|
-      expect(push_subscription.send(:pushable?, Notification.new(activity_type: 'FollowRequest'))).to eq false
+      context "when notification is a #{type}" do
-      expect(push_subscription.send(:pushable?, Notification.new(activity_type: 'Favourite'))).to eq true
+        let(:notification_type) { type }
+
+        it "returns boolean corresonding to alert setting" do
+          expect(subject.pushable?(notification)).to eq data[:alerts][type]
+        end
+      end
+    end
+
+    context 'when policy is all' do
+      let(:policy) { 'all' }
+
+      it 'returns true' do
+        expect(subject.pushable?(notification)).to eq true
+      end
+    end
+
+    context 'when policy is none' do
+      let(:policy) { 'none' }
+
+      it 'returns false' do
+        expect(subject.pushable?(notification)).to eq false
+      end
+    end
+
+    context 'when policy is followed' do
+      let(:policy) { 'followed' }
+
+      context 'and notification is from someone you follow' do
+        before do
+          account.follow!(notification.from_account)
+        end
+
+        it 'returns true' do
+          expect(subject.pushable?(notification)).to eq true
+        end
+      end
+
+      context 'and notification is not from someone you follow' do
+        it 'returns false' do
+          expect(subject.pushable?(notification)).to eq false
+        end
+      end
+    end
+
+    context 'when policy is follower' do
+      let(:policy) { 'follower' }
+
+      context 'and notification is from someone who follows you' do
+        before do
+          notification.from_account.follow!(account)
+        end
+
+        it 'returns true' do
+          expect(subject.pushable?(notification)).to eq true
+        end
+      end
+
+      context 'and notification is not from someone who follows you' do
+        it 'returns false' do
+          expect(subject.pushable?(notification)).to eq false
+        end
+      end
     end
   end
 end