Skip to content
Permalink
Browse files

Change deletes to preserve soft-deleted statuses in unresolved reports (

#11805)

Change all account actions except "none" to resolve all unresolved reports

Refactor `SuspendAccountService` to be more readable
  • Loading branch information...
Gargron committed Sep 11, 2019
1 parent 4fe1276 commit c5d37f18cb3f4d6212fb8f3e1c4e1e027f677ec5
@@ -41,7 +41,7 @@ def approve

def reject
authorize @account.user, :reject?
SuspendAccountService.new.call(@account, including_user: true, destroy: true, skip_distribution: true)
SuspendAccountService.new.call(@account, reserve_email: false, reserve_username: false)
redirect_to admin_pending_accounts_path
end

@@ -5,10 +5,10 @@ class ReportNotesController < BaseController
before_action :set_report_note, only: [:destroy]

def create
authorize ReportNote, :create?
authorize :report_note, :create?

@report_note = current_account.report_notes.new(resource_params)
@report = @report_note.report
@report = @report_note.report

if @report_note.save
if params[:create_and_resolve]
@@ -26,9 +26,8 @@ def create

redirect_to admin_report_path(@report), notice: I18n.t('admin.report_notes.created_msg')
else
@report_notes = @report.notes.latest
@report_history = @report.history
@form = Form::StatusBatch.new
@report_notes = (@report.notes.latest + @report.history + @report.target_account.targeted_account_warnings.latest.custom).sort_by(&:created_at)
@form = Form::StatusBatch.new

render template: 'admin/reports/show'
end
@@ -58,7 +58,7 @@ def approve

def reject
authorize @account.user, :reject?
SuspendAccountService.new.call(@account, including_user: true, destroy: true, skip_distribution: true)
SuspendAccountService.new.call(@account, reserve_email: false, reserve_username: false)
render json: @account, serializer: REST::Admin::AccountSerializer
end

@@ -13,8 +13,7 @@ def perform

def delete_person
lock_or_return("delete_in_progress:#{@account.id}") do
SuspendAccountService.new.call(@account)
@account.destroy!
SuspendAccountService.new.call(@account, reserve_username: false)
end
end

@@ -115,6 +115,7 @@ class Account < ApplicationRecord
:approved?,
:pending?,
:disabled?,
:unconfirmed_or_pending?,
:role,
:admin?,
:moderator?,
@@ -83,19 +83,23 @@ def process_warning!

# A log entry is only interesting if the warning contains
# custom text from someone. Otherwise it's just noise.

log_action(:create, warning) if warning.text.present?
end

def process_reports!
return if report_id.blank?
# If we're doing "mark as resolved" on a single report,
# then we want to keep other reports open in case they
# contain new actionable information.
#
# Otherwise, we will mark all unresolved reports about
# the account as resolved.

authorize(report, :update?)
reports.each { |report| authorize(report, :update?) }

if type == 'none'
reports.each do |report|
log_action(:resolve, report)
report.resolve!(current_account)
else
Report.where(target_account: target_account).unresolved.update_all(action_taken: true, action_taken_by_account_id: current_account.id)
end
end

@@ -141,6 +145,16 @@ def status_ids
@report.status_ids if @report && include_statuses
end

def reports
@reports ||= begin
if type == 'none' && with_report?
[report]
else
Report.where(target_account: target_account).unresolved

This comment has been minimized.

Copy link
@ThibG

ThibG Sep 13, 2019

Collaborator

Does that mean it will mark all reports as resolved when e.g. silencing an user because of some behavior, even if the admin hasn't reviewed a report of more serious offense yet?

This comment has been minimized.

Copy link
@Gargron

Gargron Sep 13, 2019

Author Member

That's how it already worked except when silencing directly from account page. Just making the behaviour consistent between account/report pages. In my experience this is more useful than leaving reports unresolved and requiring 20x "mark as resolve".

end
end
end

def warning_preset
@warning_preset ||= AccountWarningPreset.find(warning_preset_id) if warning_preset_id.present?
end
@@ -69,6 +69,6 @@ def reject!
records = accounts.includes(:user)

records.each { |account| authorize(account.user, :reject?) }
.each { |account| SuspendAccountService.new.call(account, including_user: true, destroy: true, skip_distribution: true) }
.each { |account| SuspendAccountService.new.call(account, reserve_email: false, reserve_username: false) }
end
end
@@ -35,7 +35,7 @@ def change_sensitive(sensitive)
def delete_statuses
Status.where(id: status_ids).reorder(nil).find_each do |status|
status.discard
RemovalWorker.perform_async(status.id, redraft: false)
RemovalWorker.perform_async(status.id, immediate: true)
Tombstone.find_or_create_by(uri: status.uri, account: status.account, by_moderator: true)
log_action :destroy, status
end
@@ -59,6 +59,7 @@ def unassign!
end

def resolve!(acting_account)
RemovalWorker.push_bulk(Status.with_discarded.discarded.where(id: status_ids).pluck(:id)) { |status_id| [status_id, { immediate: true }] }
update!(action_taken: true, action_taken_by_account_id: acting_account.id)
end

@@ -214,6 +214,10 @@ def non_sensitive_with_media?
!sensitive? && with_media?
end

def reported?
@reported ||= Report.where(target_account: account).unresolved.where('? = ANY(status_ids)', id).exists?
end

def emojis
return @emojis if defined?(@emojis)

@@ -171,6 +171,10 @@ def functional?
confirmed? && approved? && !disabled? && !account.suspended?
end

def unconfirmed_or_pending?
!(confirmed? && approved?)
end

def inactive_message
!approved? ? :pending : super
end
@@ -53,7 +53,7 @@ def clear_media!

def suspend_accounts!
blocked_domain_accounts.without_suspended.reorder(nil).find_each do |account|
SuspendAccountService.new.call(account, suspended_at: @domain_block.created_at)
SuspendAccountService.new.call(account, reserve_username: true, suspended_at: @domain_block.created_at)
end
end

@@ -8,7 +8,8 @@ class RemoveStatusService < BaseService
# @param [Status] status
# @param [Hash] options
# @option [Boolean] :redraft
# @options [Boolean] :original_removed
# @option [Boolean] :immediate
# @option [Boolean] :original_removed
def call(status, **options)
@payload = Oj.dump(event: :delete, payload: status.id.to_s)
@status = status
@@ -31,7 +32,7 @@ def call(status, **options)
remove_from_spam_check
remove_media

@status.destroy!
@status.destroy! if @options[:immediate] || !@status.reported?
else
raise Mastodon::RaceConditionError
end
@@ -150,7 +151,7 @@ def remove_from_media
end

def remove_media
return if @options[:redraft]
return if @options[:redraft] || (!@options[:immediate] && @status.reported?)

@status.media_attachments.destroy_all
end
@@ -15,7 +15,6 @@ class SuspendAccountService < BaseService
favourites
follow_requests
list_accounts
media_attachments
mute_relationships
muted_by_relationships
notifications
@@ -32,14 +31,26 @@ class SuspendAccountService < BaseService
targeted_reports
).freeze

# Suspend an account and remove as much of its data as possible
# Suspend or remove an account and remove as much of its data
# as possible. If it's a local account and it has not been confirmed
# or never been approved, then side effects are skipped and both
# the user and account records are removed fully. Otherwise,
# it is controlled by options.
# @param [Account]
# @param [Hash] options
# @option [Boolean] :including_user Remove the user record as well
# @option [Boolean] :destroy Remove the account record instead of suspending
# @option [Boolean] :reserve_email Keep user record. Only applicable for local accounts
# @option [Boolean] :reserve_username Keep account record
# @option [Boolean] :skip_side_effects Side effects are ActivityPub and streaming API payloads
# @option [Time] :suspended_at Only applicable when :reserve_username is true
def call(account, **options)
@account = account
@options = options
@options = { reserve_username: true, reserve_email: true }.merge(options)

if @account.local? && @account.user_unconfirmed_or_pending?
@options[:reserve_email] = false
@options[:reserve_username] = false
@options[:skip_side_effects] = true
end

reject_follows!
purge_user!
@@ -60,46 +71,61 @@ def reject_follows!
def purge_user!
return if !@account.local? || @account.user.nil?

if @options[:including_user]
@options[:destroy] = true if !@account.user_confirmed? || @account.user_pending?
@account.user.destroy
else
if @options[:reserve_email]
@account.user.disable!
@account.user.invites.where(uses: 0).destroy_all
else
@account.user.destroy
end
end

def purge_content!
distribute_delete_actor! if @account.local? && !@options[:skip_distribution]
distribute_delete_actor! if @account.local? && !@options[:skip_side_effects]

@account.statuses.reorder(nil).find_in_batches do |statuses|
BatchedRemoveStatusService.new.call(statuses, skip_side_effects: @options[:destroy])
statuses.reject! { |status| reported_status_ids.include?(status.id) } if @options[:reserve_username]
BatchedRemoveStatusService.new.call(statuses, skip_side_effects: @options[:skip_side_effects])
end

@account.media_attachments.reorder(nil).find_each do |media_attachment|
next if @options[:reserve_username] && reported_status_ids.include?(media_attachment.status_id)

media_attachment.destroy
end

@account.polls.reorder(nil).find_each do |poll|
next if @options[:reserve_username] && reported_status_ids.include?(poll.status_id)

poll.destroy
end

associations_for_destruction.each do |association_name|
destroy_all(@account.public_send(association_name))
end

@account.destroy if @options[:destroy]
@account.destroy unless @options[:reserve_username]
end

def purge_profile!
# If the account is going to be destroyed
# there is no point wasting time updating
# its values first

return if @options[:destroy]
return unless @options[:reserve_username]

@account.silenced_at = nil
@account.suspended_at = @options[:suspended_at] || Time.now.utc
@account.locked = false
@account.memorial = false
@account.discoverable = false
@account.display_name = ''
@account.note = ''
@account.fields = []
@account.statuses_count = 0
@account.followers_count = 0
@account.following_count = 0
@account.moved_to_account = nil
@account.trust_level = :untrusted
@account.avatar.destroy
@account.header.destroy
@account.save!
@@ -135,11 +161,15 @@ def low_priority_delivery_inboxes
Account.inboxes - delivery_inboxes
end

def reported_status_ids
@reported_status_ids ||= Report.where(target_account: @account).unresolved.pluck(:status_ids).flatten.uniq
end

def associations_for_destruction
if @options[:destroy]
ASSOCIATIONS_ON_SUSPEND + ASSOCIATIONS_ON_DESTROY
else
if @options[:reserve_username]
ASSOCIATIONS_ON_SUSPEND
else
ASSOCIATIONS_ON_SUSPEND + ASSOCIATIONS_ON_DESTROY
end
end
end
@@ -3,7 +3,7 @@
class UnallowDomainService < BaseService
def call(domain_allow)
Account.where(domain: domain_allow.domain).find_each do |account|
SuspendAccountService.new.call(account, destroy: true)
SuspendAccountService.new.call(account, reserve_username: false)
end

domain_allow.destroy
@@ -6,6 +6,6 @@ class Admin::SuspensionWorker
sidekiq_options queue: 'pull'

def perform(account_id, remove_user = false)
SuspendAccountService.new.call(Account.find(account_id), including_user: remove_user)
SuspendAccountService.new.call(Account.find(account_id), reserve_username: true, reserve_email: !remove_user)
end
end
@@ -185,7 +185,7 @@ def delete(username)
end

say("Deleting user with #{account.statuses_count} statuses, this might take a while...")
SuspendAccountService.new.call(account, including_user: true)
SuspendAccountService.new.call(account, reserve_email: false)
say('OK', :green)
end

@@ -239,7 +239,7 @@ def cull
end

if [404, 410].include?(code)
SuspendAccountService.new.call(account, destroy: true) unless options[:dry_run]
SuspendAccountService.new.call(account, reserve_username: false) unless options[:dry_run]
1
else
# Touch account even during dry run to avoid getting the account into the window again
@@ -42,7 +42,7 @@ def purge(domain = nil)
end

processed, = parallelize_with_progress(scope) do |account|
SuspendAccountService.new.call(account, destroy: true) unless options[:dry_run]
SuspendAccountService.new.call(account, reserve_username: false, skip_side_effects: true) unless options[:dry_run]
end

DomainBlock.where(domain: domain).destroy_all unless options[:dry_run]
@@ -47,7 +47,7 @@
it 'removes a status' do
allow(RemovalWorker).to receive(:perform_async)
subject.call
expect(RemovalWorker).to have_received(:perform_async).with(status_ids.first, redraft: false)
expect(RemovalWorker).to have_received(:perform_async).with(status_ids.first, immediate: true)
end
end

@@ -65,7 +65,7 @@
it 'removes a status' do
allow(RemovalWorker).to receive(:perform_async)
subject.call
expect(RemovalWorker).to have_received(:perform_async).with(status_ids.first, redraft: false)
expect(RemovalWorker).to have_received(:perform_async).with(status_ids.first, immediate: true)
end
end

@@ -41,12 +41,12 @@

it 'call RemovalWorker' do
form.save
expect(RemovalWorker).to have_received(:perform_async).with(status.id, redraft: false)
expect(RemovalWorker).to have_received(:perform_async).with(status.id, immediate: true)
end

it 'do not call RemovalWorker' do
form.save
expect(RemovalWorker).not_to have_received(:perform_async).with(another_status.id, redraft: false)
expect(RemovalWorker).not_to have_received(:perform_async).with(another_status.id, immediate: true)
end
end
end

0 comments on commit c5d37f1

Please sign in to comment.
You can’t perform that action at this time.