From 46e902f1f373210f75e24bbbdcf0850b20bb1c53 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Wed, 13 Mar 2024 05:22:43 -0400 Subject: [PATCH 1/3] Merge `api/v1/accounts/credentials` controller spec into existing request spec (#29006) --- .../accounts/credentials_controller_spec.rb | 105 ------------------ .../api/v1/accounts/credentials_spec.rb | 74 +++++++++--- 2 files changed, 61 insertions(+), 118 deletions(-) delete mode 100644 spec/controllers/api/v1/accounts/credentials_controller_spec.rb diff --git a/spec/controllers/api/v1/accounts/credentials_controller_spec.rb b/spec/controllers/api/v1/accounts/credentials_controller_spec.rb deleted file mode 100644 index a62fa54e60..0000000000 --- a/spec/controllers/api/v1/accounts/credentials_controller_spec.rb +++ /dev/null @@ -1,105 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -describe Api::V1::Accounts::CredentialsController do - render_views - - let(:user) { Fabricate(:user) } - let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: scopes) } - - context 'with an oauth token' do - before do - allow(controller).to receive(:doorkeeper_token) { token } - end - - describe 'GET #show' do - let(:scopes) { 'read:accounts' } - - it 'returns http success' do - get :show - expect(response).to have_http_status(200) - end - end - - describe 'PATCH #update' do - let(:scopes) { 'write:accounts' } - - describe 'with valid data' do - before do - allow(ActivityPub::UpdateDistributionWorker).to receive(:perform_async) - - patch :update, params: { - display_name: "Alice Isn't Dead", - note: "Hi!\n\nToot toot!", - avatar: fixture_file_upload('avatar.gif', 'image/gif'), - header: fixture_file_upload('attachment.jpg', 'image/jpeg'), - source: { - privacy: 'unlisted', - sensitive: true, - }, - } - end - - it 'updates account info', :aggregate_failures do - expect(response).to have_http_status(200) - - user.reload - user.account.reload - - expect(user.account.display_name).to eq("Alice Isn't Dead") - expect(user.account.note).to eq("Hi!\n\nToot toot!") - expect(user.account.avatar).to exist - expect(user.account.header).to exist - expect(user.setting_default_privacy).to eq('unlisted') - expect(user.setting_default_sensitive).to be(true) - - expect(ActivityPub::UpdateDistributionWorker).to have_received(:perform_async).with(user.account_id) - end - end - - describe 'with empty source list' do - before do - patch :update, params: { - display_name: "I'm a cat", - source: {}, - }, as: :json - end - - it 'returns http success' do - expect(response).to have_http_status(200) - end - end - - describe 'with invalid data' do - before do - patch :update, params: { note: 'This is too long. ' * 30 } - end - - it 'returns http unprocessable entity' do - expect(response).to have_http_status(422) - end - end - end - end - - context 'without an oauth token' do - before do - allow(controller).to receive(:doorkeeper_token).and_return(nil) - end - - describe 'GET #show' do - it 'returns http unauthorized' do - get :show - expect(response).to have_http_status(401) - end - end - - describe 'PATCH #update' do - it 'returns http unauthorized' do - patch :update, params: { note: 'Foo' } - expect(response).to have_http_status(401) - end - end - end -end diff --git a/spec/requests/api/v1/accounts/credentials_spec.rb b/spec/requests/api/v1/accounts/credentials_spec.rb index b13e79b12b..737348c2dd 100644 --- a/spec/requests/api/v1/accounts/credentials_spec.rb +++ b/spec/requests/api/v1/accounts/credentials_spec.rb @@ -15,15 +15,11 @@ RSpec.describe 'credentials API' do it_behaves_like 'forbidden for wrong scope', 'write write:accounts' - it 'returns http success' do - subject - - expect(response).to have_http_status(200) - end - - it 'returns the expected content' do + it 'returns http success with expected content' do subject + expect(response) + .to have_http_status(200) expect(body_as_json).to include({ source: hash_including({ discoverable: false, @@ -34,24 +30,55 @@ RSpec.describe 'credentials API' do end end - describe 'POST /api/v1/accounts/update_credentials' do + describe 'PATCH /api/v1/accounts/update_credentials' do subject do patch '/api/v1/accounts/update_credentials', headers: headers, params: params end - let(:params) { { discoverable: true, locked: false, indexable: true } } + before { allow(ActivityPub::UpdateDistributionWorker).to receive(:perform_async) } + + let(:params) do + { + avatar: fixture_file_upload('avatar.gif', 'image/gif'), + discoverable: true, + display_name: "Alice Isn't Dead", + header: fixture_file_upload('attachment.jpg', 'image/jpeg'), + indexable: true, + locked: false, + note: 'Hello!', + source: { + privacy: 'unlisted', + sensitive: true, + }, + } + end it_behaves_like 'forbidden for wrong scope', 'read read:accounts' - it 'returns http success' do - subject + describe 'with empty source list' do + let(:params) { { display_name: "I'm a cat", source: {} } } - expect(response).to have_http_status(200) + it 'returns http success' do + subject + expect(response).to have_http_status(200) + end end - it 'returns JSON with updated attributes' do + describe 'with invalid data' do + let(:params) { { note: 'This is too long. ' * 30 } } + + it 'returns http unprocessable entity' do + subject + expect(response).to have_http_status(422) + end + end + + it 'returns http success with updated JSON attributes' do subject + expect(response) + .to have_http_status(200) + expect(body_as_json).to include({ source: hash_including({ discoverable: true, @@ -59,6 +86,27 @@ RSpec.describe 'credentials API' do }), locked: false, }) + + expect(ActivityPub::UpdateDistributionWorker) + .to have_received(:perform_async).with(user.account_id) + end + + def expect_account_updates + expect(user.account.reload) + .to have_attributes( + display_name: eq("Alice Isn't Dead"), + note: 'Hello!', + avatar: exist, + header: exist + ) + end + + def expect_user_updates + expect(user.reload) + .to have_attributes( + setting_default_privacy: eq('unlisted'), + setting_default_sensitive: be(true) + ) end end end From 27fd084cb5ccb4bf6b2a3eb28b0b123063230c10 Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 13 Mar 2024 11:17:55 +0100 Subject: [PATCH 2/3] Exempt some notification types from notification filtering (#29565) --- app/services/notify_service.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/app/services/notify_service.rb b/app/services/notify_service.rb index 428fdb4d47..66cbff0ef8 100644 --- a/app/services/notify_service.rb +++ b/app/services/notify_service.rb @@ -68,6 +68,13 @@ class NotifyService < BaseService NEW_FOLLOWER_THRESHOLD = 3.days.freeze + NON_FILTERABLE_TYPES = %i( + admin.sign_up + admin.report + poll + update + ).freeze + def initialize(notification) @notification = notification @recipient = notification.account @@ -76,6 +83,7 @@ class NotifyService < BaseService end def filter? + return false if NON_FILTERABLE_TYPES.include?(@notification.type) return false if override_for_sender? from_limited? || From b43eaa4517107326c7e73b949cec759f841b4a30 Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 13 Mar 2024 11:35:49 +0100 Subject: [PATCH 3/3] Refactor notification filtering behavior definition (#29567) --- app/models/notification.rb | 46 +++++++++++++++++++++++++--------- app/services/notify_service.rb | 2 +- 2 files changed, 35 insertions(+), 13 deletions(-) diff --git a/app/models/notification.rb b/app/models/notification.rb index e322daea4a..861a154369 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -29,18 +29,40 @@ class Notification < ApplicationRecord 'Poll' => :poll, }.freeze - TYPES = %i( - mention - status - reblog - follow - follow_request - favourite - poll - update - admin.sign_up - admin.report - ).freeze + PROPERTIES = { + mention: { + filterable: true, + }.freeze, + status: { + filterable: false, + }.freeze, + reblog: { + filterable: true, + }.freeze, + follow: { + filterable: true, + }.freeze, + follow_request: { + filterable: true, + }.freeze, + favourite: { + filterable: true, + }.freeze, + poll: { + filterable: false, + }.freeze, + update: { + filterable: false, + }.freeze, + 'admin.sign_up': { + filterable: false, + }.freeze, + 'admin.report': { + filterable: false, + }.freeze, + }.freeze + + TYPES = PROPERTIES.keys.freeze TARGET_STATUS_INCLUDES_BY_TYPE = { status: :status, diff --git a/app/services/notify_service.rb b/app/services/notify_service.rb index 66cbff0ef8..f3d16f1be7 100644 --- a/app/services/notify_service.rb +++ b/app/services/notify_service.rb @@ -83,7 +83,7 @@ class NotifyService < BaseService end def filter? - return false if NON_FILTERABLE_TYPES.include?(@notification.type) + return false unless Notification::PROPERTIES[@notification.type][:filterable] return false if override_for_sender? from_limited? ||