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
This commit is contained in:
Eugen Rochko 2019-08-22 04:17:12 +02:00 committed by GitHub
parent e9c3d1ef46
commit 97192d9a77
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
9 changed files with 22 additions and 10 deletions

View file

@ -53,7 +53,7 @@ class Api::V1::StatusesController < Api::BaseController
@status = Status.where(account_id: current_user.account).find(params[:id]) @status = Status.where(account_id: current_user.account).find(params[:id])
authorize @status, :destroy? authorize @status, :destroy?
RemovalWorker.perform_async(@status.id) RemovalWorker.perform_async(@status.id, redraft: true)
render json: @status, serializer: REST::StatusSerializer, source_requested: true render json: @status, serializer: REST::StatusSerializer, source_requested: true
end end

View file

@ -70,7 +70,7 @@ class ActivityPub::Activity::Delete < ActivityPub::Activity
end end
def delete_now! def delete_now!
RemoveStatusService.new.call(@status) RemoveStatusService.new.call(@status, redraft: false)
end end
def payload def payload

View file

@ -34,7 +34,7 @@ class Form::StatusBatch
def delete_statuses def delete_statuses
Status.where(id: status_ids).reorder(nil).find_each do |status| 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) Tombstone.find_or_create_by(uri: status.uri, account: status.account, by_moderator: true)
log_action :destroy, status log_action :destroy, status
end end

View file

@ -8,7 +8,7 @@ class BatchedRemoveStatusService < BaseService
# Dispatch Salmon deletes, unique per domain, of the deleted statuses, but only local ones # Dispatch Salmon deletes, unique per domain, of the deleted statuses, but only local ones
# Remove statuses from home feeds # Remove statuses from home feeds
# Push delete events to streaming API for home feeds and public 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 # @param [Hash] options
# @option [Boolean] :skip_side_effects # @option [Boolean] :skip_side_effects
def call(statuses, **options) def call(statuses, **options)

View file

@ -4,6 +4,11 @@ class RemoveStatusService < BaseService
include Redisable include Redisable
include Payloadable include Payloadable
# Delete a status
# @param [Status] status
# @param [Hash] options
# @option [Boolean] :redraft
# @options [Boolean] :original_removed
def call(status, **options) def call(status, **options)
@payload = Oj.dump(event: :delete, payload: status.id.to_s) @payload = Oj.dump(event: :delete, payload: status.id.to_s)
@status = status @status = status
@ -24,6 +29,7 @@ class RemoveStatusService < BaseService
remove_from_public remove_from_public
remove_from_media if status.media_attachments.any? remove_from_media if status.media_attachments.any?
remove_from_spam_check remove_from_spam_check
remove_media
@status.destroy! @status.destroy!
else else
@ -143,6 +149,12 @@ class RemoveStatusService < BaseService
redis.publish('timeline:public:local:media', @payload) if @status.local? redis.publish('timeline:public:local:media', @payload) if @status.local?
end end
def remove_media
return if @options[:redraft]
@status.media_attachments.destroy_all
end
def remove_from_spam_check def remove_from_spam_check
redis.zremrangebyscore("spam_check:#{@status.account_id}", @status.id, @status.id) redis.zremrangebyscore("spam_check:#{@status.account_id}", @status.id, @status.id)
end end

View file

@ -3,8 +3,8 @@
class RemovalWorker class RemovalWorker
include Sidekiq::Worker include Sidekiq::Worker
def perform(status_id) def perform(status_id, options = {})
RemoveStatusService.new.call(Status.find(status_id)) RemoveStatusService.new.call(Status.find(status_id), **options.symbolize_keys)
rescue ActiveRecord::RecordNotFound rescue ActiveRecord::RecordNotFound
true true
end end

View file

@ -47,7 +47,7 @@ describe Admin::ReportedStatusesController do
it 'removes a status' do it 'removes a status' do
allow(RemovalWorker).to receive(:perform_async) allow(RemovalWorker).to receive(:perform_async)
subject.call 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
end end

View file

@ -65,7 +65,7 @@ describe Admin::StatusesController do
it 'removes a status' do it 'removes a status' do
allow(RemovalWorker).to receive(:perform_async) allow(RemovalWorker).to receive(:perform_async)
subject.call 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
end end

View file

@ -41,12 +41,12 @@ describe Form::StatusBatch do
it 'call RemovalWorker' do it 'call RemovalWorker' do
form.save 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 end
it 'do not call RemovalWorker' do it 'do not call RemovalWorker' do
form.save 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 end
end end