From 7996a9543dd893c772355f41a9ac41b3854fee18 Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 14 Aug 2024 09:34:30 +0200 Subject: [PATCH] Change notification request acceptance to immediately delete the request (#31256) --- .../accept_notification_request_service.rb | 3 +- app/workers/unfilter_notifications_worker.rb | 21 +++++++-- ...ccept_notification_request_service_spec.rb | 19 ++++++++ .../unfilter_notifications_worker_spec.rb | 46 +++++++++++++++++++ 4 files changed, 83 insertions(+), 6 deletions(-) create mode 100644 spec/services/accept_notification_request_service_spec.rb create mode 100644 spec/workers/unfilter_notifications_worker_spec.rb diff --git a/app/services/accept_notification_request_service.rb b/app/services/accept_notification_request_service.rb index e49eae6fd3..ad27ae3300 100644 --- a/app/services/accept_notification_request_service.rb +++ b/app/services/accept_notification_request_service.rb @@ -3,6 +3,7 @@ class AcceptNotificationRequestService < BaseService def call(request) NotificationPermission.create!(account: request.account, from_account: request.from_account) - UnfilterNotificationsWorker.perform_async(request.id) + UnfilterNotificationsWorker.perform_async(request.account_id, request.from_account_id) + request.destroy! end end diff --git a/app/workers/unfilter_notifications_worker.rb b/app/workers/unfilter_notifications_worker.rb index 223654aa16..5939a691fb 100644 --- a/app/workers/unfilter_notifications_worker.rb +++ b/app/workers/unfilter_notifications_worker.rb @@ -3,8 +3,19 @@ class UnfilterNotificationsWorker include Sidekiq::Worker - def perform(notification_request_id) - @notification_request = NotificationRequest.find(notification_request_id) + # Earlier versions of the feature passed a `notification_request` ID + # If `to_account_id` is passed, the first argument is an account ID + # TODO for after 4.3.0: drop the single-argument case + def perform(notification_request_or_account_id, from_account_id = nil) + if from_account_id.present? + @notification_request = nil + @from_account = Account.find(from_account_id) + @recipient = Account.find(notification_request_or_account_id) + else + @notification_request = NotificationRequest.find(notification_request_or_account_id) + @from_account = @notification_request.from_account + @recipient = @notification_request.account + end push_to_conversations! unfilter_notifications! @@ -16,7 +27,7 @@ class UnfilterNotificationsWorker private def push_to_conversations! - notifications_with_private_mentions.find_each { |notification| AccountConversation.add_status(@notification_request.account, notification.target_status) } + notifications_with_private_mentions.reorder(nil).find_each(order: :desc) { |notification| AccountConversation.add_status(@recipient, notification.target_status) } end def unfilter_notifications! @@ -24,11 +35,11 @@ class UnfilterNotificationsWorker end def remove_request! - @notification_request.destroy! + @notification_request&.destroy! end def filtered_notifications - Notification.where(account: @notification_request.account, from_account: @notification_request.from_account, filtered: true) + Notification.where(account: @recipient, from_account: @from_account, filtered: true) end def notifications_with_private_mentions diff --git a/spec/services/accept_notification_request_service_spec.rb b/spec/services/accept_notification_request_service_spec.rb new file mode 100644 index 0000000000..bf67a52225 --- /dev/null +++ b/spec/services/accept_notification_request_service_spec.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe AcceptNotificationRequestService do + subject { described_class.new } + + let(:notification_request) { Fabricate(:notification_request) } + + describe '#call' do + it 'destroys the notification request, creates a permission, and queues a worker' do + expect { subject.call(notification_request) } + .to change { NotificationRequest.exists?(notification_request.id) }.to(false) + .and change { NotificationPermission.exists?(account_id: notification_request.account_id, from_account_id: notification_request.from_account_id) }.to(true) + + expect(UnfilterNotificationsWorker).to have_enqueued_sidekiq_job(notification_request.account_id, notification_request.from_account_id) + end + end +end diff --git a/spec/workers/unfilter_notifications_worker_spec.rb b/spec/workers/unfilter_notifications_worker_spec.rb new file mode 100644 index 0000000000..3f43b298a5 --- /dev/null +++ b/spec/workers/unfilter_notifications_worker_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe UnfilterNotificationsWorker do + let(:recipient) { Fabricate(:account) } + let(:sender) { Fabricate(:account) } + + before do + # Populate multiple kinds of filtered notifications + private_message = Fabricate(:status, account: sender, visibility: :direct) + mention = Fabricate(:mention, account: recipient, status: private_message) + Fabricate(:notification, filtered: true, from_account: sender, account: recipient, type: :mention, activity: mention) + follow_request = sender.request_follow!(recipient) + Fabricate(:notification, filtered: true, from_account: sender, account: recipient, type: :follow_request, activity: follow_request) + end + + shared_examples 'shared behavior' do + it 'unfilters notifications and adds private messages to conversations' do + expect { subject } + .to change { recipient.notifications.where(from_account_id: sender.id).pluck(:filtered) }.from([true, true]).to([false, false]) + .and change { recipient.conversations.exists?(last_status_id: sender.statuses.first.id) }.to(true) + end + end + + describe '#perform' do + context 'with single argument (prerelease behavior)' do + subject { described_class.new.perform(notification_request.id) } + + let(:notification_request) { Fabricate(:notification_request, from_account: sender, account: recipient) } + + it_behaves_like 'shared behavior' + + it 'destroys the notification request' do + expect { subject } + .to change { NotificationRequest.exists?(notification_request.id) }.to(false) + end + end + + context 'with two arguments' do + subject { described_class.new.perform(recipient.id, sender.id) } + + it_behaves_like 'shared behavior' + end + end +end