Revamp notification policy options (#31343)

app/controllers/api/v1/notifications/policies_controller.rb

@@ -8,12 +8,12 @@ class Api::V1::Notifications::PoliciesController < Api::BaseController
   before_action :set_policy
 
   def show
-    render json: @policy, serializer: REST::NotificationPolicySerializer
+    render json: @policy, serializer: REST::V1::NotificationPolicySerializer
   end
 
   def update
     @policy.update!(resource_params)
-    render json: @policy, serializer: REST::NotificationPolicySerializer
+    render json: @policy, serializer: REST::V1::NotificationPolicySerializer
   end
 
   private

app/controllers/api/v2/notifications/policies_controller.rb

@@ -0,0 +1,38 @@
+# frozen_string_literal: true
+
+class Api::V2::Notifications::PoliciesController < Api::BaseController
+  before_action -> { doorkeeper_authorize! :read, :'read:notifications' }, only: :show
+  before_action -> { doorkeeper_authorize! :write, :'write:notifications' }, only: :update
+
+  before_action :require_user!
+  before_action :set_policy
+
+  def show
+    render json: @policy, serializer: REST::NotificationPolicySerializer
+  end
+
+  def update
+    @policy.update!(resource_params)
+    render json: @policy, serializer: REST::NotificationPolicySerializer
+  end
+
+  private
+
+  def set_policy
+    @policy = NotificationPolicy.find_or_initialize_by(account: current_account)
+
+    with_read_replica do
+      @policy.summarize!
+    end
+  end
+
+  def resource_params
+    params.permit(
+      :for_not_following,
+      :for_not_followers,
+      :for_new_accounts,
+      :for_private_mentions,
+      :for_limited_accounts
+    )
+  end
+end

app/models/notification_policy.rb

@@ -4,17 +4,25 @@
 #
 # Table name: notification_policies
 #
-#  id                      :bigint(8)        not null, primary key
+#  id                   :bigint(8)        not null, primary key
-#  account_id              :bigint(8)        not null
+#  account_id           :bigint(8)        not null
-#  filter_not_following    :boolean          default(FALSE), not null
+#  created_at           :datetime         not null
-#  filter_not_followers    :boolean          default(FALSE), not null
+#  updated_at           :datetime         not null
-#  filter_new_accounts     :boolean          default(FALSE), not null
+#  for_not_following    :integer          default("accept"), not null
-#  filter_private_mentions :boolean          default(TRUE), not null
+#  for_not_followers    :integer          default("accept"), not null
-#  created_at              :datetime         not null
+#  for_new_accounts     :integer          default("accept"), not null
-#  updated_at              :datetime         not null
+#  for_private_mentions :integer          default("filter"), not null
+#  for_limited_accounts :integer          default("filter"), not null
 #
 
 class NotificationPolicy < ApplicationRecord
+  self.ignored_columns += %w(
+    filter_not_following
+    filter_not_followers
+    filter_new_accounts
+    filter_private_mentions
+  )
+
   belongs_to :account
 
   has_many :notification_requests, primary_key: :account_id, foreign_key: :account_id, dependent: nil, inverse_of: false
@@ -23,11 +31,34 @@ class NotificationPolicy < ApplicationRecord
 
   MAX_MEANINGFUL_COUNT = 100
 
+  enum :for_not_following, { accept: 0, filter: 1, drop: 2 }, suffix: :not_following
+  enum :for_not_followers, { accept: 0, filter: 1, drop: 2 }, suffix: :not_followers
+  enum :for_new_accounts, { accept: 0, filter: 1, drop: 2 }, suffix: :new_accounts
+  enum :for_private_mentions, { accept: 0, filter: 1, drop: 2 }, suffix: :private_mentions
+  enum :for_limited_accounts, { accept: 0, filter: 1, drop: 2 }, suffix: :limited_accounts
+
   def summarize!
     @pending_requests_count = pending_notification_requests.first
     @pending_notifications_count = pending_notification_requests.last
   end
 
+  # Compat helpers with V1
+  def filter_not_following=(value)
+    self.for_not_following = value ? :filter : :accept
+  end
+
+  def filter_not_followers=(value)
+    self.for_not_followers = value ? :filter : :accept
+  end
+
+  def filter_new_accounts=(value)
+    self.for_new_accounts = value ? :filter : :accept
+  end
+
+  def filter_private_mentions=(value)
+    self.for_private_mentions = value ? :filter : :accept
+  end
+
   private
 
   def pending_notification_requests

app/serializers/rest/notification_policy_serializer.rb

@@ -3,10 +3,11 @@
 class REST::NotificationPolicySerializer < ActiveModel::Serializer
   # Please update `app/javascript/mastodon/api_types/notification_policies.ts` when making changes to the attributes
 
-  attributes :filter_not_following,
+  attributes :for_not_following,
-             :filter_not_followers,
+             :for_not_followers,
-             :filter_new_accounts,
+             :for_new_accounts,
-             :filter_private_mentions,
+             :for_private_mentions,
+             :for_limited_accounts,
              :summary
 
   def summary

app/serializers/rest/v1/notification_policy_serializer.rb

@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+class REST::V1::NotificationPolicySerializer < ActiveModel::Serializer
+  attributes :filter_not_following,
+             :filter_not_followers,
+             :filter_new_accounts,
+             :filter_private_mentions,
+             :summary
+
+  def summary
+    {
+      pending_requests_count: object.pending_requests_count.to_i,
+      pending_notifications_count: object.pending_notifications_count.to_i,
+    }
+  end
+
+  def filter_not_following
+    !object.accept_not_following?
+  end
+
+  def filter_not_followers
+    !object.accept_not_followers?
+  end
+
+  def filter_new_accounts
+    !object.accept_new_accounts?
+  end
+
+  def filter_private_mentions
+    !object.accept_private_mentions?
+  end
+end

app/services/notify_service.rb

@@ -16,59 +16,7 @@ class NotifyService < BaseService
     severed_relationships
   ).freeze
 
-  class DismissCondition
+  class BaseCondition
-    def initialize(notification)
-      @recipient = notification.account
-      @sender = notification.from_account
-      @notification = notification
-    end
-
-    def dismiss?
-      blocked   = @recipient.unavailable?
-      blocked ||= from_self? && %i(poll severed_relationships moderation_warning).exclude?(@notification.type)
-
-      return blocked if message? && from_staff?
-
-      blocked ||= domain_blocking?
-      blocked ||= @recipient.blocking?(@sender)
-      blocked ||= @recipient.muting_notifications?(@sender)
-      blocked ||= conversation_muted?
-      blocked ||= blocked_mention? if message?
-      blocked
-    end
-
-    private
-
-    def blocked_mention?
-      FeedManager.instance.filter?(:mentions, @notification.target_status, @recipient)
-    end
-
-    def message?
-      @notification.type == :mention
-    end
-
-    def from_staff?
-      @sender.local? && @sender.user.present? && @sender.user_role&.overrides?(@recipient.user_role) && @sender.user_role&.highlighted? && @sender.user_role&.can?(*UserRole::Flags::CATEGORIES[:moderation])
-    end
-
-    def from_self?
-      @recipient.id == @sender.id
-    end
-
-    def domain_blocking?
-      @recipient.domain_blocking?(@sender.domain) && !following_sender?
-    end
-
-    def conversation_muted?
-      @notification.target_status && @recipient.muting_conversation?(@notification.target_status.conversation)
-    end
-
-    def following_sender?
-      @recipient.following?(@sender)
-    end
-  end
-
-  class FilterCondition
     NEW_ACCOUNT_THRESHOLD = 30.days.freeze
 
     NEW_FOLLOWER_THRESHOLD = 3.days.freeze
@@ -82,39 +30,16 @@ class NotifyService < BaseService
     ).freeze
 
     def initialize(notification)
-      @notification = notification
       @recipient = notification.account
       @sender = notification.from_account
+      @notification = notification
       @policy = NotificationPolicy.find_or_initialize_by(account: @recipient)
     end
 
-    def filter?
-      return false unless Notification::PROPERTIES[@notification.type][:filterable]
-      return false if override_for_sender?
-
-      from_limited? ||
-        filtered_by_not_following_policy? ||
-        filtered_by_not_followers_policy? ||
-        filtered_by_new_accounts_policy? ||
-        filtered_by_private_mentions_policy?
-    end
-
     private
 
-    def filtered_by_not_following_policy?
+    def filterable_type?
-      @policy.filter_not_following? && not_following?
+      Notification::PROPERTIES[@notification.type][:filterable]
-    end
-
-    def filtered_by_not_followers_policy?
-      @policy.filter_not_followers? && not_follower?
-    end
-
-    def filtered_by_new_accounts_policy?
-      @policy.filter_new_accounts? && new_account?
-    end
-
-    def filtered_by_private_mentions_policy?
-      @policy.filter_private_mentions? && not_following? && private_mention_not_in_response?
     end
 
     def not_following?
@@ -174,6 +99,112 @@ class NotifyService < BaseService
     end
   end
 
+  class DropCondition < BaseCondition
+    def drop?
+      blocked   = @recipient.unavailable?
+      blocked ||= from_self? && %i(poll severed_relationships moderation_warning).exclude?(@notification.type)
+
+      return blocked if message? && from_staff?
+
+      blocked ||= domain_blocking?
+      blocked ||= @recipient.blocking?(@sender)
+      blocked ||= @recipient.muting_notifications?(@sender)
+      blocked ||= conversation_muted?
+      blocked ||= blocked_mention? if message?
+
+      return true if blocked
+      return false unless filterable_type?
+      return false if override_for_sender?
+
+      blocked_by_limited_accounts_policy? ||
+        blocked_by_not_following_policy? ||
+        blocked_by_not_followers_policy? ||
+        blocked_by_new_accounts_policy? ||
+        blocked_by_private_mentions_policy?
+    end
+
+    private
+
+    def blocked_mention?
+      FeedManager.instance.filter?(:mentions, @notification.target_status, @recipient)
+    end
+
+    def message?
+      @notification.type == :mention
+    end
+
+    def from_staff?
+      @sender.local? && @sender.user.present? && @sender.user_role&.overrides?(@recipient.user_role) && @sender.user_role&.highlighted? && @sender.user_role&.can?(*UserRole::Flags::CATEGORIES[:moderation])
+    end
+
+    def from_self?
+      @recipient.id == @sender.id
+    end
+
+    def domain_blocking?
+      @recipient.domain_blocking?(@sender.domain) && not_following?
+    end
+
+    def conversation_muted?
+      @notification.target_status && @recipient.muting_conversation?(@notification.target_status.conversation)
+    end
+
+    def blocked_by_not_following_policy?
+      @policy.drop_not_following? && not_following?
+    end
+
+    def blocked_by_not_followers_policy?
+      @policy.drop_not_followers? && not_follower?
+    end
+
+    def blocked_by_new_accounts_policy?
+      @policy.drop_new_accounts? && new_account? && not_following?
+    end
+
+    def blocked_by_private_mentions_policy?
+      @policy.drop_private_mentions? && not_following? && private_mention_not_in_response?
+    end
+
+    def blocked_by_limited_accounts_policy?
+      @policy.drop_limited_accounts? && @sender.silenced? && not_following?
+    end
+  end
+
+  class FilterCondition < BaseCondition
+    def filter?
+      return false unless filterable_type?
+      return false if override_for_sender?
+
+      filtered_by_limited_accounts_policy? ||
+        filtered_by_not_following_policy? ||
+        filtered_by_not_followers_policy? ||
+        filtered_by_new_accounts_policy? ||
+        filtered_by_private_mentions_policy?
+    end
+
+    private
+
+    def filtered_by_not_following_policy?
+      @policy.filter_not_following? && not_following?
+    end
+
+    def filtered_by_not_followers_policy?
+      @policy.filter_not_followers? && not_follower?
+    end
+
+    def filtered_by_new_accounts_policy?
+      @policy.filter_new_accounts? && new_account? && not_following?
+    end
+
+    def filtered_by_private_mentions_policy?
+      @policy.filter_private_mentions? && not_following? && private_mention_not_in_response?
+    end
+
+    def filtered_by_limited_accounts_policy?
+      @policy.filter_limited_accounts? && @sender.silenced? && not_following?
+    end
+  end
+
   def call(recipient, type, activity)
     return if recipient.user.nil?
 
@@ -182,7 +213,7 @@ class NotifyService < BaseService
     @notification = Notification.new(account: @recipient, type: type, activity: @activity)
 
     # For certain conditions we don't need to create a notification at all
-    return if dismiss?
+    return if drop?
 
     @notification.filtered = filter?
     @notification.group_key = notification_group_key
@@ -222,8 +253,8 @@ class NotifyService < BaseService
     "#{type_prefix}-#{hour_bucket}"
   end
 
-  def dismiss?
+  def drop?
-    DismissCondition.new(@notification).dismiss?
+    DropCondition.new(@notification).drop?
   end
 
   def filter?

config/routes/api.rb

@@ -336,6 +336,10 @@ namespace :api, format: false do
     namespace :admin do
       resources :accounts, only: [:index]
     end
+
+    namespace :notifications do
+      resource :policy, only: [:show, :update]
+    end
   end
 
   namespace :v2_alpha do

db/migrate/20240808114841_add_new_notification_policies.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class AddNewNotificationPolicies < ActiveRecord::Migration[7.1]
+  def change
+    add_column :notification_policies, :for_not_following, :integer, default: 0, null: false
+    add_column :notification_policies, :for_not_followers, :integer, default: 0, null: false
+    add_column :notification_policies, :for_new_accounts, :integer, default: 0, null: false
+    add_column :notification_policies, :for_private_mentions, :integer, default: 1, null: false
+    add_column :notification_policies, :for_limited_accounts, :integer, default: 1, null: false
+  end
+end

db/migrate/20240808124338_migrate_notifications_policy_v2.rb

@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+class MigrateNotificationsPolicyV2 < ActiveRecord::Migration[7.1]
+  disable_ddl_transaction!
+
+  # Dummy classes, to make migration possible across version changes
+  class NotificationPolicy < ApplicationRecord; end
+
+  def up
+    NotificationPolicy.in_batches.update_all(<<~SQL.squish)
+      for_not_following = CASE filter_not_following WHEN true THEN 1 ELSE 0 END,
+      for_not_followers = CASE filter_not_following WHEN true THEN 1 ELSE 0 END,
+      for_new_accounts = CASE filter_new_accounts WHEN true THEN 1 ELSE 0 END,
+      for_private_mentions = CASE filter_private_mentions WHEN true THEN 1 ELSE 0 END
+    SQL
+  end
+
+  def down
+    NotificationPolicy.in_batches.update_all(<<~SQL.squish)
+      filter_not_following = CASE for_not_following WHEN 0 THEN false ELSE true END,
+      filter_not_following = CASE for_not_followers WHEN 0 THEN false ELSE true END,
+      filter_new_accounts = CASE for_new_accounts WHEN 0 THEN false ELSE true END,
+      filter_private_mentions = CASE for_private_mentions WHEN 0 THEN false ELSE true END
+    SQL
+  end
+end

db/post_migrate/20240808124339_post_deployment_migrate_notifications_policy_v2.rb

@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+class PostDeploymentMigrateNotificationsPolicyV2 < ActiveRecord::Migration[7.1]
+  disable_ddl_transaction!
+
+  # Dummy classes, to make migration possible across version changes
+  class NotificationPolicy < ApplicationRecord; end
+
+  def up
+    NotificationPolicy.in_batches.update_all(<<~SQL.squish)
+      for_not_following = CASE filter_not_following WHEN true THEN 1 ELSE 0 END,
+      for_not_followers = CASE filter_not_following WHEN true THEN 1 ELSE 0 END,
+      for_new_accounts = CASE filter_new_accounts WHEN true THEN 1 ELSE 0 END,
+      for_private_mentions = CASE filter_private_mentions WHEN true THEN 1 ELSE 0 END
+    SQL
+  end
+
+  def down
+    NotificationPolicy.in_batches.update_all(<<~SQL.squish)
+      filter_not_following = CASE for_not_following WHEN 0 THEN false ELSE true END,
+      filter_not_following = CASE for_not_followers WHEN 0 THEN false ELSE true END,
+      filter_new_accounts = CASE for_new_accounts WHEN 0 THEN false ELSE true END,
+      filter_private_mentions = CASE for_private_mentions WHEN 0 THEN false ELSE true END
+    SQL
+  end
+end

db/post_migrate/20240808125420_drop_old_policies_from_notifications_policy.rb

@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+class DropOldPoliciesFromNotificationsPolicy < ActiveRecord::Migration[7.1]
+  def change
+    safety_assured do
+      remove_column :notification_policies, :filter_not_following, :boolean, default: false, null: false
+      remove_column :notification_policies, :filter_not_followers, :boolean, default: false, null: false
+      remove_column :notification_policies, :filter_new_accounts, :boolean, default: false, null: false
+      remove_column :notification_policies, :filter_private_mentions, :boolean, default: true, null: false
+    end
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.1].define(version: 2024_07_24_181224) do
+ActiveRecord::Schema[7.1].define(version: 2024_08_08_125420) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
 
@@ -692,12 +692,13 @@ ActiveRecord::Schema[7.1].define(version: 2024_07_24_181224) do
 
   create_table "notification_policies", force: :cascade do |t|
     t.bigint "account_id", null: false
-    t.boolean "filter_not_following", default: false, null: false
-    t.boolean "filter_not_followers", default: false, null: false
-    t.boolean "filter_new_accounts", default: false, null: false
-    t.boolean "filter_private_mentions", default: true, null: false
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
+    t.integer "for_not_following", default: 0, null: false
+    t.integer "for_not_followers", default: 0, null: false
+    t.integer "for_new_accounts", default: 0, null: false
+    t.integer "for_private_mentions", default: 1, null: false
+    t.integer "for_limited_accounts", default: 1, null: false
     t.index ["account_id"], name: "index_notification_policies_on_account_id", unique: true
   end
 

lib/tasks/tests.rake

@@ -107,8 +107,8 @@ namespace :tests do
       end
 
       policy = NotificationPolicy.find_by(account: User.find(1).account)
-      unless policy.filter_private_mentions == false && policy.filter_not_following == true
+      unless policy.for_private_mentions == 'accept' && policy.for_not_following == 'filter'
-        puts 'Notification policy not migrated as expected'
+        puts "Notification policy not migrated as expected: #{policy.for_private_mentions.inspect}, #{policy.for_not_following.inspect}"
         exit(1)
       end
 

spec/requests/api/v1/notifications/policies_spec.rb

@@ -51,7 +51,7 @@ RSpec.describe 'Policies' do
 
     it 'changes notification policy and returns an updated json object', :aggregate_failures do
       expect { subject }
-        .to change { NotificationPolicy.find_or_initialize_by(account: user.account).filter_not_following }.from(false).to(true)
+        .to change { NotificationPolicy.find_or_initialize_by(account: user.account).for_not_following.to_sym }.from(:accept).to(:filter)
 
       expect(response).to have_http_status(200)
       expect(body_as_json).to include(

spec/requests/api/v2/notifications/policies_spec.rb

@@ -0,0 +1,72 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe 'Policies' do
+  let(:user)    { Fabricate(:user, account_attributes: { username: 'alice' }) }
+  let(:token)   { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: scopes) }
+  let(:scopes)  { 'read:notifications write:notifications' }
+  let(:headers) { { 'Authorization' => "Bearer #{token.token}" } }
+
+  describe 'GET /api/v2/notifications/policy', :inline_jobs do
+    subject do
+      get '/api/v2/notifications/policy', headers: headers, params: params
+    end
+
+    let(:params) { {} }
+
+    before do
+      Fabricate(:notification_request, account: user.account)
+    end
+
+    it_behaves_like 'forbidden for wrong scope', 'write write:notifications'
+
+    context 'with no options' do
+      it 'returns json with expected attributes', :aggregate_failures do
+        subject
+
+        expect(response).to have_http_status(200)
+        expect(body_as_json).to include(
+          for_not_following: 'accept',
+          for_not_followers: 'accept',
+          for_new_accounts: 'accept',
+          for_private_mentions: 'filter',
+          for_limited_accounts: 'filter',
+          summary: a_hash_including(
+            pending_requests_count: 1,
+            pending_notifications_count: 0
+          )
+        )
+      end
+    end
+  end
+
+  describe 'PUT /api/v2/notifications/policy' do
+    subject do
+      put '/api/v2/notifications/policy', headers: headers, params: params
+    end
+
+    let(:params) { { for_not_following: 'filter', for_limited_accounts: 'drop' } }
+
+    it_behaves_like 'forbidden for wrong scope', 'read read:notifications'
+
+    it 'changes notification policy and returns an updated json object', :aggregate_failures do
+      expect { subject }
+        .to change { NotificationPolicy.find_or_initialize_by(account: user.account).for_not_following.to_sym }.from(:accept).to(:filter)
+        .and change { NotificationPolicy.find_or_initialize_by(account: user.account).for_limited_accounts.to_sym }.from(:filter).to(:drop)
+
+      expect(response).to have_http_status(200)
+      expect(body_as_json).to include(
+        for_not_following: 'filter',
+        for_not_followers: 'accept',
+        for_new_accounts: 'accept',
+        for_private_mentions: 'filter',
+        for_limited_accounts: 'drop',
+        summary: a_hash_including(
+          pending_requests_count: 0,
+          pending_notifications_count: 0
+        )
+      )
+    end
+  end
+end

spec/services/notify_service_spec.rb

@@ -196,20 +196,58 @@ RSpec.describe NotifyService do
     end
   end
 
-  describe NotifyService::DismissCondition do
+  describe NotifyService::DropCondition do
     subject { described_class.new(notification) }
 
     let(:activity) { Fabricate(:mention, status: Fabricate(:status)) }
     let(:notification) { Fabricate(:notification, type: :mention, activity: activity, from_account: activity.status.account, account: activity.account) }
 
-    describe '#dismiss?' do
+    describe '#drop' do
-      context 'when sender is silenced' do
+      context 'when sender is silenced and recipient has a default policy' do
         before do
           notification.from_account.silence!
         end
 
         it 'returns false' do
-          expect(subject.dismiss?).to be false
+          expect(subject.drop?).to be false
+        end
+      end
+
+      context 'when sender is silenced and recipient has a policy to ignore silenced accounts' do
+        before do
+          notification.from_account.silence!
+          notification.account.create_notification_policy!(for_limited_accounts: :drop)
+        end
+
+        it 'returns true' do
+          expect(subject.drop?).to be true
+        end
+      end
+
+      context 'when sender is new and recipient has a default policy' do
+        it 'returns false' do
+          expect(subject.drop?).to be false
+        end
+      end
+
+      context 'when sender is new and recipient has a policy to ignore silenced accounts' do
+        before do
+          notification.account.create_notification_policy!(for_new_accounts: :drop)
+        end
+
+        it 'returns true' do
+          expect(subject.drop?).to be true
+        end
+      end
+
+      context 'when sender is new and followed and recipient has a policy to ignore silenced accounts' do
+        before do
+          notification.account.create_notification_policy!(for_new_accounts: :drop)
+          notification.account.follow!(notification.from_account)
+        end
+
+        it 'returns false' do
+          expect(subject.drop?).to be false
         end
       end
 
@@ -219,7 +257,7 @@ RSpec.describe NotifyService do
         end
 
         it 'returns true' do
-          expect(subject.dismiss?).to be true
+          expect(subject.drop?).to be true
         end
       end
     end
@@ -250,6 +288,16 @@ RSpec.describe NotifyService do
             expect(subject.filter?).to be false
           end
         end
+
+        context 'when recipient is allowing limited accounts' do
+          before do
+            notification.account.create_notification_policy!(for_limited_accounts: :accept)
+          end
+
+          it 'returns false' do
+            expect(subject.filter?).to be false
+          end
+        end
       end
 
       context 'when recipient is filtering not-followed senders' do