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

diff --git a/app/controllers/api/v1/statuses_controller.rb b/app/controllers/api/v1/statuses_controller.rb
index 71a505c26cd0c8e3e6d82b039a01001acfc231aa..39ca564821f0d2aff7280b4042e799863657c5b0 100644 (file)
--- 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 1f2b40c1506ac89993cb451069b1fef922b8a5f5..3450604629b8b3b3168b9db72adf286783a85a99 100644 (file)
--- 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 933dfdaca1ba5a7465093b921833b05f6da960f5..831d8b7c52ffda7bde2ee71762ad65b131a53221 100644 (file)
--- 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 6df8d4769eb9bcf59a5bf5d3d25fc47d909760a1..3638134be76447612ded2a6fcc9e6766aef8ee58 100644 (file)
--- 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<Status>] 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 91c934181b7553ad6a1bbf5aeadb8a59c118b47a..685c1d4bffe92baa409c0cf92141b0d90ba2023b 100644 (file)
--- 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 19a660dd3e0190f19a6cfab2bc7389b3c57606ef..14423a4fb569bb423a88bfb1312303a1ae5a912f 100644 (file)
--- 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 c358506d6dce88b136487dbefd266d00719aec8d..bd146b79560a51cb01cdc271217e2e11951385c1 100644 (file)
--- 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 1a08c10b7e873fd1c2052419601e999d797acee3..6b06343efbe4bef4355872669eadf9a8ae8f4f60 100644 (file)
--- 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 00c790a11f936a12b25eb806588f38bd43c66285..f9c58c90f88cabc26bad40629f617b95433215d1 100644 (file)
--- 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