diff --git a/app/controllers/api/v1/notifications/policies_controller.rb b/app/controllers/api/v1/notifications/policies_controller.rb index 1ec336f9a..9d70c283b 100644 --- a/app/controllers/api/v1/notifications/policies_controller.rb +++ b/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 diff --git a/app/controllers/api/v2/notifications/policies_controller.rb b/app/controllers/api/v2/notifications/policies_controller.rb new file mode 100644 index 000000000..637587967 --- /dev/null +++ b/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 diff --git a/app/models/notification_policy.rb b/app/models/notification_policy.rb index 2bb58004e..3b16f33d8 100644 --- a/app/models/notification_policy.rb +++ b/app/models/notification_policy.rb @@ -4,17 +4,25 @@ # # Table name: notification_policies # -# id :bigint(8) not null, primary key -# account_id :bigint(8) not null -# filter_not_following :boolean default(FALSE), not null -# filter_not_followers :boolean default(FALSE), not null -# filter_new_accounts :boolean default(FALSE), not null -# filter_private_mentions :boolean default(TRUE), not null -# created_at :datetime not null -# updated_at :datetime not null +# id :bigint(8) not null, primary key +# account_id :bigint(8) not null +# created_at :datetime not null +# updated_at :datetime not null +# for_not_following :integer default("accept"), not null +# for_not_followers :integer default("accept"), not null +# for_new_accounts :integer default("accept"), 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 diff --git a/app/serializers/rest/notification_policy_serializer.rb b/app/serializers/rest/notification_policy_serializer.rb index 8bf85250f..3902c1a04 100644 --- a/app/serializers/rest/notification_policy_serializer.rb +++ b/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, - :filter_not_followers, - :filter_new_accounts, - :filter_private_mentions, + attributes :for_not_following, + :for_not_followers, + :for_new_accounts, + :for_private_mentions, + :for_limited_accounts, :summary def summary diff --git a/app/serializers/rest/v1/notification_policy_serializer.rb b/app/serializers/rest/v1/notification_policy_serializer.rb new file mode 100644 index 000000000..e1bbdc44f --- /dev/null +++ b/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 diff --git a/app/services/notify_service.rb b/app/services/notify_service.rb index b0bef8cd6..788381fe6 100644 --- a/app/services/notify_service.rb +++ b/app/services/notify_service.rb @@ -16,59 +16,7 @@ class NotifyService < BaseService severed_relationships ).freeze - class DismissCondition - 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 + class BaseCondition 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? - @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? - end - - def filtered_by_private_mentions_policy? - @policy.filter_private_mentions? && not_following? && private_mention_not_in_response? + def filterable_type? + Notification::PROPERTIES[@notification.type][:filterable] 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? - DismissCondition.new(@notification).dismiss? + def drop? + DropCondition.new(@notification).drop? end def filter? diff --git a/config/routes/api.rb b/config/routes/api.rb index fa74c025b..488bdb745 100644 --- a/config/routes/api.rb +++ b/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 diff --git a/db/migrate/20240808114841_add_new_notification_policies.rb b/db/migrate/20240808114841_add_new_notification_policies.rb new file mode 100644 index 000000000..9087ee35d --- /dev/null +++ b/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 diff --git a/db/migrate/20240808124338_migrate_notifications_policy_v2.rb b/db/migrate/20240808124338_migrate_notifications_policy_v2.rb new file mode 100644 index 000000000..2e0684826 --- /dev/null +++ b/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 diff --git a/db/post_migrate/20240808124339_post_deployment_migrate_notifications_policy_v2.rb b/db/post_migrate/20240808124339_post_deployment_migrate_notifications_policy_v2.rb new file mode 100644 index 000000000..eb0c90972 --- /dev/null +++ b/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 diff --git a/db/post_migrate/20240808125420_drop_old_policies_from_notifications_policy.rb b/db/post_migrate/20240808125420_drop_old_policies_from_notifications_policy.rb new file mode 100644 index 000000000..99ab1e434 --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index d4796079c..f01e11792 100644 --- a/db/schema.rb +++ b/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 diff --git a/lib/tasks/tests.rake b/lib/tasks/tests.rake index c8e4dc31c..cb7fce313 100644 --- a/lib/tasks/tests.rake +++ b/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 - puts 'Notification policy not migrated as expected' + unless policy.for_private_mentions == 'accept' && policy.for_not_following == 'filter' + puts "Notification policy not migrated as expected: #{policy.for_private_mentions.inspect}, #{policy.for_not_following.inspect}" exit(1) end diff --git a/spec/requests/api/v1/notifications/policies_spec.rb b/spec/requests/api/v1/notifications/policies_spec.rb index cbd449977..a73d4217b 100644 --- a/spec/requests/api/v1/notifications/policies_spec.rb +++ b/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( diff --git a/spec/requests/api/v2/notifications/policies_spec.rb b/spec/requests/api/v2/notifications/policies_spec.rb new file mode 100644 index 000000000..f9860b5fb --- /dev/null +++ b/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 diff --git a/spec/services/notify_service_spec.rb b/spec/services/notify_service_spec.rb index d64cfe590..935b94c70 100644 --- a/spec/services/notify_service_spec.rb +++ b/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 - context 'when sender is silenced' do + describe '#drop' 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