From 91ef24d0e3d6415e6aa91b3ee6066cf56b4703e1 Mon Sep 17 00:00:00 2001 From: Claire Date: Fri, 28 Mar 2025 17:12:32 +0100 Subject: [PATCH] Add delay to profile updates to debounce them (#34137) --- .../api/v1/accounts/credentials_controller.rb | 2 +- .../api/v1/profile/avatars_controller.rb | 2 +- .../api/v1/profile/headers_controller.rb | 2 +- .../settings/pictures_controller.rb | 2 +- .../settings/privacy_controller.rb | 2 +- .../settings/profiles_controller.rb | 2 +- .../activitypub/update_distribution_worker.rb | 2 + .../accounts/credentials_controller_spec.rb | 4 +- .../settings/privacy_controller_spec.rb | 70 +++++++++++++++++++ .../settings/profiles_controller_spec.rb | 8 +-- .../api/v1/accounts/credentials_spec.rb | 41 ++++++++++- spec/requests/api/v1/profiles_spec.rb | 16 +---- 12 files changed, 127 insertions(+), 26 deletions(-) create mode 100644 spec/controllers/settings/privacy_controller_spec.rb diff --git a/app/controllers/api/v1/accounts/credentials_controller.rb b/app/controllers/api/v1/accounts/credentials_controller.rb index 76ba758245..12187078d0 100644 --- a/app/controllers/api/v1/accounts/credentials_controller.rb +++ b/app/controllers/api/v1/accounts/credentials_controller.rb @@ -14,7 +14,7 @@ class Api::V1::Accounts::CredentialsController < Api::BaseController @account = current_account UpdateAccountService.new.call(@account, account_params, raise_error: true) current_user.update(user_params) if user_params - ActivityPub::UpdateDistributionWorker.perform_async(@account.id) + ActivityPub::UpdateDistributionWorker.perform_in(ActivityPub::UpdateDistributionWorker::DEBOUNCE_DELAY, @account.id) render json: @account, serializer: REST::CredentialAccountSerializer end diff --git a/app/controllers/api/v1/profile/avatars_controller.rb b/app/controllers/api/v1/profile/avatars_controller.rb index bc4d01a597..e6c954ed63 100644 --- a/app/controllers/api/v1/profile/avatars_controller.rb +++ b/app/controllers/api/v1/profile/avatars_controller.rb @@ -7,7 +7,7 @@ class Api::V1::Profile::AvatarsController < Api::BaseController def destroy @account = current_account UpdateAccountService.new.call(@account, { avatar: nil }, raise_error: true) - ActivityPub::UpdateDistributionWorker.perform_async(@account.id) + ActivityPub::UpdateDistributionWorker.perform_in(ActivityPub::UpdateDistributionWorker::DEBOUNCE_DELAY, @account.id) render json: @account, serializer: REST::CredentialAccountSerializer end end diff --git a/app/controllers/api/v1/profile/headers_controller.rb b/app/controllers/api/v1/profile/headers_controller.rb index 9f4daa2f77..4472a01b05 100644 --- a/app/controllers/api/v1/profile/headers_controller.rb +++ b/app/controllers/api/v1/profile/headers_controller.rb @@ -7,7 +7,7 @@ class Api::V1::Profile::HeadersController < Api::BaseController def destroy @account = current_account UpdateAccountService.new.call(@account, { header: nil }, raise_error: true) - ActivityPub::UpdateDistributionWorker.perform_async(@account.id) + ActivityPub::UpdateDistributionWorker.perform_in(ActivityPub::UpdateDistributionWorker::DEBOUNCE_DELAY, @account.id) render json: @account, serializer: REST::CredentialAccountSerializer end end diff --git a/app/controllers/settings/pictures_controller.rb b/app/controllers/settings/pictures_controller.rb index 58a4325307..7e61e6d580 100644 --- a/app/controllers/settings/pictures_controller.rb +++ b/app/controllers/settings/pictures_controller.rb @@ -8,7 +8,7 @@ module Settings def destroy if valid_picture? if UpdateAccountService.new.call(@account, { @picture => nil, "#{@picture}_remote_url" => '' }) - ActivityPub::UpdateDistributionWorker.perform_async(@account.id) + ActivityPub::UpdateDistributionWorker.perform_in(ActivityPub::UpdateDistributionWorker::DEBOUNCE_DELAY, @account.id) redirect_to settings_profile_path, notice: I18n.t('generic.changes_saved_msg'), status: 303 else redirect_to settings_profile_path diff --git a/app/controllers/settings/privacy_controller.rb b/app/controllers/settings/privacy_controller.rb index 1102c89fad..6c4836ec56 100644 --- a/app/controllers/settings/privacy_controller.rb +++ b/app/controllers/settings/privacy_controller.rb @@ -8,7 +8,7 @@ class Settings::PrivacyController < Settings::BaseController def update if UpdateAccountService.new.call(@account, account_params.except(:settings)) current_user.update!(settings_attributes: account_params[:settings]) - ActivityPub::UpdateDistributionWorker.perform_async(@account.id) + ActivityPub::UpdateDistributionWorker.perform_in(ActivityPub::UpdateDistributionWorker::DEBOUNCE_DELAY, @account.id) redirect_to settings_privacy_path, notice: I18n.t('generic.changes_saved_msg') else render :show diff --git a/app/controllers/settings/profiles_controller.rb b/app/controllers/settings/profiles_controller.rb index 8ae69b7fe0..2d64132751 100644 --- a/app/controllers/settings/profiles_controller.rb +++ b/app/controllers/settings/profiles_controller.rb @@ -9,7 +9,7 @@ class Settings::ProfilesController < Settings::BaseController def update if UpdateAccountService.new.call(@account, account_params) - ActivityPub::UpdateDistributionWorker.perform_async(@account.id) + ActivityPub::UpdateDistributionWorker.perform_in(ActivityPub::UpdateDistributionWorker::DEBOUNCE_DELAY, @account.id) redirect_to settings_profile_path, notice: I18n.t('generic.changes_saved_msg') else @account.build_fields diff --git a/app/workers/activitypub/update_distribution_worker.rb b/app/workers/activitypub/update_distribution_worker.rb index a04ac621f3..9a418f0f3d 100644 --- a/app/workers/activitypub/update_distribution_worker.rb +++ b/app/workers/activitypub/update_distribution_worker.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class ActivityPub::UpdateDistributionWorker < ActivityPub::RawDistributionWorker + DEBOUNCE_DELAY = 5.seconds + sidekiq_options queue: 'push', lock: :until_executed, lock_ttl: 1.day.to_i # Distribute an profile update to servers that might have a copy diff --git a/spec/controllers/api/v1/accounts/credentials_controller_spec.rb b/spec/controllers/api/v1/accounts/credentials_controller_spec.rb index b5d5c37a9c..e050ab47ce 100644 --- a/spec/controllers/api/v1/accounts/credentials_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/credentials_controller_spec.rb @@ -27,7 +27,7 @@ describe Api::V1::Accounts::CredentialsController do describe 'with valid data' do before do - allow(ActivityPub::UpdateDistributionWorker).to receive(:perform_async) + allow(ActivityPub::UpdateDistributionWorker).to receive(:perform_in) patch :update, params: { display_name: "Alice Isn't Dead", @@ -58,7 +58,7 @@ describe Api::V1::Accounts::CredentialsController do end it 'queues up an account update distribution' do - expect(ActivityPub::UpdateDistributionWorker).to have_received(:perform_async).with(user.account_id) + expect(ActivityPub::UpdateDistributionWorker).to have_received(:perform_in).with(anything, user.account_id) end end diff --git a/spec/controllers/settings/privacy_controller_spec.rb b/spec/controllers/settings/privacy_controller_spec.rb new file mode 100644 index 0000000000..4fccf098c8 --- /dev/null +++ b/spec/controllers/settings/privacy_controller_spec.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Settings::PrivacyController do + render_views + + let!(:user) { Fabricate(:user) } + let(:account) { user.account } + + before do + sign_in user, scope: :user + end + + describe 'GET #show' do + before do + get :show + end + + it 'returns http success with private cache control headers', :aggregate_failures do + expect(response) + .to have_http_status(200) + expect(response.headers['Cache-Control']).to include('private, no-store') + end + end + + describe 'PUT #update' do + context 'when update succeeds' do + before do + allow(ActivityPub::UpdateDistributionWorker).to receive(:perform_in) + end + + it 'updates the user profile' do + put :update, params: { account: { discoverable: '1', settings: { indexable: '1' } } } + + expect(account.reload.discoverable) + .to be(true) + + expect(response) + .to redirect_to(settings_privacy_path) + + expect(ActivityPub::UpdateDistributionWorker) + .to have_received(:perform_in).with(anything, account.id) + end + end + + context 'when update fails' do + before do + allow(UpdateAccountService).to receive(:new).and_return(failing_update_service) + allow(ActivityPub::UpdateDistributionWorker).to receive(:perform_in) + end + + it 'updates the user profile' do + put :update, params: { account: { discoverable: '1', settings: { indexable: '1' } } } + + expect(response) + .to render_template(:show) + + expect(ActivityPub::UpdateDistributionWorker) + .to_not have_received(:perform_in) + end + + private + + def failing_update_service + instance_double(UpdateAccountService, call: false) + end + end + end +end diff --git a/spec/controllers/settings/profiles_controller_spec.rb b/spec/controllers/settings/profiles_controller_spec.rb index 806fad19a8..8235eaaa42 100644 --- a/spec/controllers/settings/profiles_controller_spec.rb +++ b/spec/controllers/settings/profiles_controller_spec.rb @@ -32,23 +32,23 @@ RSpec.describe Settings::ProfilesController do end it 'updates the user profile' do - allow(ActivityPub::UpdateDistributionWorker).to receive(:perform_async) + allow(ActivityPub::UpdateDistributionWorker).to receive(:perform_in) put :update, params: { account: { display_name: 'New name' } } expect(account.reload.display_name).to eq 'New name' expect(response).to redirect_to(settings_profile_path) - expect(ActivityPub::UpdateDistributionWorker).to have_received(:perform_async).with(account.id) + expect(ActivityPub::UpdateDistributionWorker).to have_received(:perform_in).with(anything, account.id) end end describe 'PUT #update with new profile image' do it 'updates profile image' do - allow(ActivityPub::UpdateDistributionWorker).to receive(:perform_async) + allow(ActivityPub::UpdateDistributionWorker).to receive(:perform_in) expect(account.avatar.instance.avatar_file_name).to be_nil put :update, params: { account: { avatar: fixture_file_upload('avatar.gif', 'image/gif') } } expect(response).to redirect_to(settings_profile_path) expect(account.reload.avatar.instance.avatar_file_name).to_not be_nil - expect(ActivityPub::UpdateDistributionWorker).to have_received(:perform_async).with(account.id) + expect(ActivityPub::UpdateDistributionWorker).to have_received(:perform_in).with(anything, account.id) 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..6a7d35861e 100644 --- a/spec/requests/api/v1/accounts/credentials_spec.rb +++ b/spec/requests/api/v1/accounts/credentials_spec.rb @@ -39,7 +39,25 @@ RSpec.describe 'credentials API' do patch '/api/v1/accounts/update_credentials', headers: headers, params: params end - let(:params) { { discoverable: true, locked: false, indexable: true } } + before do + allow(ActivityPub::UpdateDistributionWorker).to receive(:perform_in) + end + + 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' @@ -59,6 +77,27 @@ RSpec.describe 'credentials API' do }), locked: false, }) + + expect(ActivityPub::UpdateDistributionWorker) + .to have_received(:perform_in).with(anything, 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 diff --git a/spec/requests/api/v1/profiles_spec.rb b/spec/requests/api/v1/profiles_spec.rb index 26a9b848e5..b7760bce6d 100644 --- a/spec/requests/api/v1/profiles_spec.rb +++ b/spec/requests/api/v1/profiles_spec.rb @@ -16,7 +16,7 @@ RSpec.describe 'Deleting profile images' do describe 'DELETE /api/v1/profile' do before do - allow(ActivityPub::UpdateDistributionWorker).to receive(:perform_async) + allow(ActivityPub::UpdateDistributionWorker).to receive(:perform_in) end context 'when deleting an avatar' do @@ -48,12 +48,7 @@ RSpec.describe 'Deleting profile images' do account.reload expect(account.header).to exist - end - - it 'queues up an account update distribution' do - delete '/api/v1/profile/avatar', headers: headers - - expect(ActivityPub::UpdateDistributionWorker).to have_received(:perform_async).with(account.id) + expect(ActivityPub::UpdateDistributionWorker).to have_received(:perform_in).with(anything, account.id) end end @@ -86,12 +81,7 @@ RSpec.describe 'Deleting profile images' do account.reload expect(account.header).to_not exist - end - - it 'queues up an account update distribution' do - delete '/api/v1/profile/header', headers: headers - - expect(ActivityPub::UpdateDistributionWorker).to have_received(:perform_async).with(account.id) + expect(ActivityPub::UpdateDistributionWorker).to have_received(:perform_in).with(anything, account.id) end end end