From 97192d9a77c0b4b68afe50d6a94d87110a8adbcd Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Thu, 22 Aug 2019 04:17:12 +0200 Subject: [PATCH] Fix remote and staff-removed statuses leaving media behind for a day (#11638) The reason for unattaching media instead of removing it is to support delete & redraft functionality, but remote or staff-removed statuses will never be redrafted, so the media should be deleted immediately --- app/controllers/api/v1/statuses_controller.rb | 2 +- app/lib/activitypub/activity/delete.rb | 2 +- app/models/form/status_batch.rb | 2 +- app/services/batched_remove_status_service.rb | 2 +- app/services/remove_status_service.rb | 12 ++++++++++++ app/workers/removal_worker.rb | 4 ++-- .../admin/reported_statuses_controller_spec.rb | 2 +- spec/controllers/admin/statuses_controller_spec.rb | 2 +- spec/models/form/status_batch_spec.rb | 4 ++-- 9 files changed, 22 insertions(+), 10 deletions(-) diff --git a/app/controllers/api/v1/statuses_controller.rb b/app/controllers/api/v1/statuses_controller.rb index 71a505c26cd..39ca564821f 100644 --- a/app/controllers/api/v1/statuses_controller.rb +++ b/app/controllers/api/v1/statuses_controller.rb @@ -53,7 +53,7 @@ class Api::V1::StatusesController < Api::BaseController @status = Status.where(account_id: current_user.account).find(params[:id]) authorize @status, :destroy? - RemovalWorker.perform_async(@status.id) + RemovalWorker.perform_async(@status.id, redraft: true) render json: @status, serializer: REST::StatusSerializer, source_requested: true end diff --git a/app/lib/activitypub/activity/delete.rb b/app/lib/activitypub/activity/delete.rb index 1f2b40c1506..3450604629b 100644 --- a/app/lib/activitypub/activity/delete.rb +++ b/app/lib/activitypub/activity/delete.rb @@ -70,7 +70,7 @@ class ActivityPub::Activity::Delete < ActivityPub::Activity end def delete_now! - RemoveStatusService.new.call(@status) + RemoveStatusService.new.call(@status, redraft: false) end def payload diff --git a/app/models/form/status_batch.rb b/app/models/form/status_batch.rb index 933dfdaca1b..831d8b7c52f 100644 --- a/app/models/form/status_batch.rb +++ b/app/models/form/status_batch.rb @@ -34,7 +34,7 @@ class Form::StatusBatch def delete_statuses Status.where(id: status_ids).reorder(nil).find_each do |status| - RemovalWorker.perform_async(status.id) + RemovalWorker.perform_async(status.id, redraft: false) Tombstone.find_or_create_by(uri: status.uri, account: status.account, by_moderator: true) log_action :destroy, status end diff --git a/app/services/batched_remove_status_service.rb b/app/services/batched_remove_status_service.rb index 6df8d4769eb..3638134be76 100644 --- a/app/services/batched_remove_status_service.rb +++ b/app/services/batched_remove_status_service.rb @@ -8,7 +8,7 @@ class BatchedRemoveStatusService < BaseService # Dispatch Salmon deletes, unique per domain, of the deleted statuses, but only local ones # Remove statuses from home feeds # Push delete events to streaming API for home feeds and public feeds - # @param [Status] statuses A preferably batched array of statuses + # @param [Enumerable] statuses A preferably batched array of statuses # @param [Hash] options # @option [Boolean] :skip_side_effects def call(statuses, **options) diff --git a/app/services/remove_status_service.rb b/app/services/remove_status_service.rb index 91c934181b7..685c1d4bffe 100644 --- a/app/services/remove_status_service.rb +++ b/app/services/remove_status_service.rb @@ -4,6 +4,11 @@ class RemoveStatusService < BaseService include Redisable include Payloadable + # Delete a status + # @param [Status] status + # @param [Hash] options + # @option [Boolean] :redraft + # @options [Boolean] :original_removed def call(status, **options) @payload = Oj.dump(event: :delete, payload: status.id.to_s) @status = status @@ -24,6 +29,7 @@ class RemoveStatusService < BaseService remove_from_public remove_from_media if status.media_attachments.any? remove_from_spam_check + remove_media @status.destroy! else @@ -143,6 +149,12 @@ class RemoveStatusService < BaseService redis.publish('timeline:public:local:media', @payload) if @status.local? end + def remove_media + return if @options[:redraft] + + @status.media_attachments.destroy_all + end + def remove_from_spam_check redis.zremrangebyscore("spam_check:#{@status.account_id}", @status.id, @status.id) end diff --git a/app/workers/removal_worker.rb b/app/workers/removal_worker.rb index 19a660dd3e0..14423a4fb56 100644 --- a/app/workers/removal_worker.rb +++ b/app/workers/removal_worker.rb @@ -3,8 +3,8 @@ class RemovalWorker include Sidekiq::Worker - def perform(status_id) - RemoveStatusService.new.call(Status.find(status_id)) + def perform(status_id, options = {}) + RemoveStatusService.new.call(Status.find(status_id), **options.symbolize_keys) rescue ActiveRecord::RecordNotFound true end diff --git a/spec/controllers/admin/reported_statuses_controller_spec.rb b/spec/controllers/admin/reported_statuses_controller_spec.rb index c358506d6dc..bd146b79560 100644 --- a/spec/controllers/admin/reported_statuses_controller_spec.rb +++ b/spec/controllers/admin/reported_statuses_controller_spec.rb @@ -47,7 +47,7 @@ describe Admin::ReportedStatusesController do it 'removes a status' do allow(RemovalWorker).to receive(:perform_async) subject.call - expect(RemovalWorker).to have_received(:perform_async).with(status_ids.first) + expect(RemovalWorker).to have_received(:perform_async).with(status_ids.first, redraft: false) end end diff --git a/spec/controllers/admin/statuses_controller_spec.rb b/spec/controllers/admin/statuses_controller_spec.rb index 1a08c10b7e8..6b06343efbe 100644 --- a/spec/controllers/admin/statuses_controller_spec.rb +++ b/spec/controllers/admin/statuses_controller_spec.rb @@ -65,7 +65,7 @@ describe Admin::StatusesController do it 'removes a status' do allow(RemovalWorker).to receive(:perform_async) subject.call - expect(RemovalWorker).to have_received(:perform_async).with(status_ids.first) + expect(RemovalWorker).to have_received(:perform_async).with(status_ids.first, redraft: false) end end diff --git a/spec/models/form/status_batch_spec.rb b/spec/models/form/status_batch_spec.rb index 00c790a11f9..f9c58c90f88 100644 --- a/spec/models/form/status_batch_spec.rb +++ b/spec/models/form/status_batch_spec.rb @@ -41,12 +41,12 @@ describe Form::StatusBatch do it 'call RemovalWorker' do form.save - expect(RemovalWorker).to have_received(:perform_async).with(status.id) + expect(RemovalWorker).to have_received(:perform_async).with(status.id, redraft: false) end it 'do not call RemovalWorker' do form.save - expect(RemovalWorker).not_to have_received(:perform_async).with(another_status.id) + expect(RemovalWorker).not_to have_received(:perform_async).with(another_status.id, redraft: false) end end end