From 592fe1a3bdb5a0e359f887676a905cca94f7c633 Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 18 Jan 2023 16:40:09 +0100 Subject: [PATCH] Add confirmation screen when handling reports (#22375) * Add confirmation screen on moderation actions * Add flash notice when a report has been processed * Refactor tests * Add tests --- .../admin/account_actions_controller.rb | 2 +- .../admin/reports/actions_controller.rb | 17 ++- app/models/admin/status_batch_action.rb | 9 +- app/views/admin/reports/_actions.html.haml | 2 +- .../admin/reports/actions/preview.html.haml | 78 +++++++++++ config/i18n-tasks.yml | 2 + config/locales/en.yml | 21 +++ config/routes.rb | 6 +- .../admin/reports/actions_controller_spec.rb | 132 +++++++++++++++--- 9 files changed, 240 insertions(+), 29 deletions(-) create mode 100644 app/views/admin/reports/actions/preview.html.haml diff --git a/app/controllers/admin/account_actions_controller.rb b/app/controllers/admin/account_actions_controller.rb index 3f2e28b6ae..e89404b609 100644 --- a/app/controllers/admin/account_actions_controller.rb +++ b/app/controllers/admin/account_actions_controller.rb @@ -21,7 +21,7 @@ module Admin account_action.save! if account_action.with_report? - redirect_to admin_reports_path + redirect_to admin_reports_path, notice: I18n.t('admin.reports.processed_msg', id: params[:report_id]) else redirect_to admin_account_path(@account.id) end diff --git a/app/controllers/admin/reports/actions_controller.rb b/app/controllers/admin/reports/actions_controller.rb index 5cb5c744f2..554f7906f8 100644 --- a/app/controllers/admin/reports/actions_controller.rb +++ b/app/controllers/admin/reports/actions_controller.rb @@ -3,6 +3,11 @@ class Admin::Reports::ActionsController < Admin::BaseController before_action :set_report + def preview + authorize @report, :show? + @moderation_action = action_from_button + end + def create authorize @report, :show? @@ -13,7 +18,8 @@ class Admin::Reports::ActionsController < Admin::BaseController status_ids: @report.status_ids, current_account: current_account, report_id: @report.id, - send_email_notification: !@report.spam? + send_email_notification: !@report.spam?, + text: params[:text] ) status_batch_action.save! @@ -23,13 +29,16 @@ class Admin::Reports::ActionsController < Admin::BaseController report_id: @report.id, target_account: @report.target_account, current_account: current_account, - send_email_notification: !@report.spam? + send_email_notification: !@report.spam?, + text: params[:text] ) account_action.save! + else + return redirect_to admin_report_path(@report), alert: I18n.t('admin.reports.unknown_action_msg', action: action_from_button) end - redirect_to admin_reports_path + redirect_to admin_reports_path, notice: I18n.t('admin.reports.processed_msg', id: @report.id) end private @@ -47,6 +56,8 @@ class Admin::Reports::ActionsController < Admin::BaseController 'silence' elsif params[:suspend] 'suspend' + elsif params[:moderation_action] + params[:moderation_action] end end end diff --git a/app/models/admin/status_batch_action.rb b/app/models/admin/status_batch_action.rb index 39cd7d0eb8..b8bdec7223 100644 --- a/app/models/admin/status_batch_action.rb +++ b/app/models/admin/status_batch_action.rb @@ -6,7 +6,8 @@ class Admin::StatusBatchAction include Authorization attr_accessor :current_account, :type, - :status_ids, :report_id + :status_ids, :report_id, + :text attr_reader :send_email_notification @@ -57,7 +58,8 @@ class Admin::StatusBatchAction action: :delete_statuses, account: current_account, report: report, - status_ids: status_ids + status_ids: status_ids, + text: text ) statuses.each { |status| Tombstone.find_or_create_by(uri: status.uri, account: status.account, by_moderator: true) } unless target_account.local? @@ -95,7 +97,8 @@ class Admin::StatusBatchAction action: :mark_statuses_as_sensitive, account: current_account, report: report, - status_ids: status_ids + status_ids: status_ids, + text: text ) UserMailer.warning(target_account.user, @warning).deliver_later! if warnable? diff --git a/app/views/admin/reports/_actions.html.haml b/app/views/admin/reports/_actions.html.haml index 486eb486c7..aad4416257 100644 --- a/app/views/admin/reports/_actions.html.haml +++ b/app/views/admin/reports/_actions.html.haml @@ -1,4 +1,4 @@ -= form_tag admin_report_actions_path(@report), method: :post do += form_tag preview_admin_report_actions_path(@report), method: :post do .report-actions .report-actions__item .report-actions__item__button diff --git a/app/views/admin/reports/actions/preview.html.haml b/app/views/admin/reports/actions/preview.html.haml new file mode 100644 index 0000000000..58745319c8 --- /dev/null +++ b/app/views/admin/reports/actions/preview.html.haml @@ -0,0 +1,78 @@ +- target_acct = @report.target_account.acct +- warning_action = { 'delete' => 'delete_statuses', 'mark_as_sensitive' => 'mark_statuses_as_sensitive' }.fetch(@moderation_action, @moderation_action) + +- content_for :page_title do + = t('admin.reports.confirm_action', acct: target_acct) + += form_tag admin_report_actions_path(@report), class: 'simple_form', method: :post do + = hidden_field_tag :moderation_action, @moderation_action + + %p.hint= t("admin.reports.summary.action_preambles.#{@moderation_action}_html", acct: target_acct) + %ul.hint + %li.warning-hint= t("admin.reports.summary.actions.#{@moderation_action}_html", acct: target_acct) + - if @moderation_action == 'suspend' + %li.warning-hint= t('admin.reports.summary.delete_data_html', acct: target_acct) + - if %w(silence suspend).include?(@moderation_action) + %li.warning-hint= t('admin.reports.summary.close_reports_html', acct: target_acct) + - else + %li= t('admin.reports.summary.close_report', id: @report.id) + %li= t('admin.reports.summary.record_strike_html', acct: target_acct) + - if @report.target_account.local? && !@report.spam? + %li= t('admin.reports.summary.send_email_html', acct: target_acct) + + %hr.spacer/ + + - if @report.target_account.local? + %p.hint= t('admin.reports.summary.preview_preamble_html', acct: target_acct) + + .strike-card + - unless warning_action == 'none' + %p= t "user_mailer.warning.explanation.#{warning_action}", instance: Rails.configuration.x.local_domain + + .fields-group + = text_area_tag :text, nil, placeholder: t('admin.reports.summary.warning_placeholder') + + - if !@report.other? + %p + %strong= t('user_mailer.warning.reason') + = t("user_mailer.warning.categories.#{@report.category}") + + - if @report.violation? && @report.rule_ids.present? + %ul.strike-card__rules + - @report.rules.each do |rule| + %li + %span.strike-card__rules__text= rule.text + + - if @report.status_ids.present? && !@report.status_ids.empty? + %p + %strong= t('user_mailer.warning.statuses') + + .strike-card__statuses-list + - status_map = @report.statuses.includes(:application, :media_attachments).index_by(&:id) + + - @report.status_ids.each do |status_id| + .strike-card__statuses-list__item + - if (status = status_map[status_id.to_i]) + .one-liner + = link_to short_account_status_url(@report.target_account, status_id), class: 'emojify' do + = one_line_preview(status) + + - status.ordered_media_attachments.each do |media_attachment| + %abbr{ title: media_attachment.description } + = fa_icon 'link' + = media_attachment.file_file_name + .strike-card__statuses-list__item__meta + %time.formatted{ datetime: status.created_at.iso8601, title: l(status.created_at) }= l(status.created_at) + - unless status.application.nil? + ยท + = status.application.name + - else + .one-liner= t('disputes.strikes.status', id: status_id) + .strike-card__statuses-list__item__meta + = t('disputes.strikes.status_removed') + + %hr.spacer/ + + .actions + = link_to t('admin.reports.cancel'), admin_report_path(@report), class: 'button button-tertiary' + = button_tag t('admin.reports.confirm'), name: :confirm, class: 'button', type: :submit diff --git a/config/i18n-tasks.yml b/config/i18n-tasks.yml index c1da42bd87..46dd3124bf 100644 --- a/config/i18n-tasks.yml +++ b/config/i18n-tasks.yml @@ -58,6 +58,8 @@ ignore_unused: - 'errors.429' - 'admin.accounts.roles.*' - 'admin.action_logs.actions.*' + - 'admin.reports.summary.action_preambles.*' + - 'admin.reports.summary.actions.*' - 'admin_mailer.new_appeal.actions.*' - 'statuses.attached.*' - 'move_handler.carry_{mutes,blocks}_over_text' diff --git a/config/locales/en.yml b/config/locales/en.yml index 4143aab045..3de2f2772c 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -590,6 +590,7 @@ en: comment: none: None comment_description_html: 'To provide more information, %{name} wrote:' + confirm_action: Confirm moderation action against @%{acct} created_at: Reported delete_and_resolve: Delete posts forwarded: Forwarded @@ -606,6 +607,7 @@ en: placeholder: Describe what actions have been taken, or any other related updates... title: Notes notes_description_html: View and leave notes to other moderators and your future self + processed_msg: 'Report #%{id} successfully processed' quick_actions_description_html: 'Take a quick action or scroll down to see reported content:' remote_user_placeholder: the remote user from %{instance} reopen: Reopen report @@ -618,9 +620,28 @@ en: status: Status statuses: Reported content statuses_description_html: Offending content will be cited in communication with the reported account + summary: + action_preambles: + delete_html: 'You are about to remove some of @%{acct}''s posts. This will:' + mark_as_sensitive_html: 'You are about to mark some of @%{acct}''s posts as sensitive. This will:' + silence_html: 'You are about to limit @%{acct}''s account. This will:' + suspend_html: 'You are about to suspend @%{acct}''s account. This will:' + actions: + delete_html: Remove the offending posts + mark_as_sensitive_html: Mark the offending posts' media as sensitive + silence_html: Severely limit @%{acct}'s reach by making their profile and contents only visible to people already following them or manually looking it profile up + suspend_html: Suspend @%{acct}, making their profile and contents inaccessible and impossible to interact with + close_report: 'Mark report #%{id} as resolved' + close_reports_html: Mark all reports against @%{acct} as resolved + delete_data_html: Delete @%{acct}'s profile and contents 30 days from now unless they get unsuspended in the meantime + preview_preamble_html: "@%{acct} will receive a warning with the following contents:" + record_strike_html: Record a strike against @%{acct} to help you escalate on future violations from this account + send_email_html: Send @%{acct} a warning e-mail + warning_placeholder: Optional additional reasoning for the moderation action. target_origin: Origin of reported account title: Reports unassign: Unassign + unknown_action_msg: 'Unknown action: %{action}' unresolved: Unresolved updated_at: Updated view_profile: View profile diff --git a/config/routes.rb b/config/routes.rb index 98e19667cf..0bee2f639b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -310,7 +310,11 @@ Rails.application.routes.draw do end resources :reports, only: [:index, :show] do - resources :actions, only: [:create], controller: 'reports/actions' + resources :actions, only: [:create], controller: 'reports/actions' do + collection do + post :preview + end + end member do post :assign_to_self diff --git a/spec/controllers/admin/reports/actions_controller_spec.rb b/spec/controllers/admin/reports/actions_controller_spec.rb index 6609798dc0..9890ac9ce8 100644 --- a/spec/controllers/admin/reports/actions_controller_spec.rb +++ b/spec/controllers/admin/reports/actions_controller_spec.rb @@ -4,39 +4,131 @@ describe Admin::Reports::ActionsController do render_views let(:user) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')) } - let(:account) { Fabricate(:account) } - let!(:status) { Fabricate(:status, account: account) } - let(:media_attached_status) { Fabricate(:status, account: account) } - let!(:media_attachment) { Fabricate(:media_attachment, account: account, status: media_attached_status) } - let(:media_attached_deleted_status) { Fabricate(:status, account: account, deleted_at: 1.day.ago) } - let!(:media_attachment2) { Fabricate(:media_attachment, account: account, status: media_attached_deleted_status) } - let(:last_media_attached_status) { Fabricate(:status, account: account) } - let!(:last_media_attachment) { Fabricate(:media_attachment, account: account, status: last_media_attached_status) } - let!(:last_status) { Fabricate(:status, account: account) } before do sign_in user, scope: :user end - describe 'POST #create' do - let(:report) { Fabricate(:report, status_ids: status_ids, account: user.account, target_account: account) } - let(:status_ids) { [media_attached_status.id, media_attached_deleted_status.id] } + describe 'POST #preview' do + let(:report) { Fabricate(:report) } before do - post :create, params: { report_id: report.id, action => '' } + post :preview, params: { report_id: report.id, action => '' } end - context 'when action is mark_as_sensitive' do + context 'when the action is "suspend"' do + let(:action) { 'suspend' } + it 'returns http success' do + expect(response).to have_http_status(200) + end + end + + context 'when the action is "silence"' do + let(:action) { 'silence' } + + it 'returns http success' do + expect(response).to have_http_status(200) + end + end + + context 'when the action is "delete"' do + let(:action) { 'delete' } + + it 'returns http success' do + expect(response).to have_http_status(200) + end + end + + context 'when the action is "mark_as_sensitive"' do let(:action) { 'mark_as_sensitive' } - it 'resolves the report' do - expect(report.reload.action_taken_at).to_not be_nil - end - - it 'marks the non-deleted as sensitive' do - expect(media_attached_status.reload.sensitive).to eq true + it 'returns http success' do + expect(response).to have_http_status(200) end end end + + describe 'POST #create' do + let(:target_account) { Fabricate(:account) } + let(:statuses) { [Fabricate(:status, account: target_account), Fabricate(:status, account: target_account)] } + let!(:media) { Fabricate(:media_attachment, account: target_account, status: statuses[0]) } + let(:report) { Fabricate(:report, target_account: target_account, status_ids: statuses.map(&:id)) } + let(:text) { 'hello' } + + shared_examples 'common behavior' do + it 'closes the report' do + expect { subject }.to change { report.reload.action_taken? }.from(false).to(true) + end + + it 'creates a strike with the expected text' do + expect { subject }.to change { report.target_account.strikes.count }.by(1) + expect(report.target_account.strikes.last.text).to eq text + end + + it 'redirects' do + subject + expect(response).to redirect_to(admin_reports_path) + end + end + + shared_examples 'all action types' do + context 'when the action is "suspend"' do + let(:action) { 'suspend' } + + it_behaves_like 'common behavior' + + it 'suspends the target account' do + expect { subject }.to change { report.target_account.reload.suspended? }.from(false).to(true) + end + end + + context 'when the action is "silence"' do + let(:action) { 'silence' } + + it_behaves_like 'common behavior' + + it 'suspends the target account' do + expect { subject }.to change { report.target_account.reload.silenced? }.from(false).to(true) + end + end + + context 'when the action is "delete"' do + let(:action) { 'delete' } + + it_behaves_like 'common behavior' + end + + context 'when the action is "mark_as_sensitive"' do + let(:action) { 'mark_as_sensitive' } + let(:statuses) { [media_attached_status, media_attached_deleted_status] } + + let!(:status) { Fabricate(:status, account: target_account) } + let(:media_attached_status) { Fabricate(:status, account: target_account) } + let!(:media_attachment) { Fabricate(:media_attachment, account: target_account, status: media_attached_status) } + let(:media_attached_deleted_status) { Fabricate(:status, account: target_account, deleted_at: 1.day.ago) } + let!(:media_attachment2) { Fabricate(:media_attachment, account: target_account, status: media_attached_deleted_status) } + let(:last_media_attached_status) { Fabricate(:status, account: target_account) } + let!(:last_media_attachment) { Fabricate(:media_attachment, account: target_account, status: last_media_attached_status) } + let!(:last_status) { Fabricate(:status, account: target_account) } + + it_behaves_like 'common behavior' + + it 'marks the non-deleted as sensitive' do + subject + expect(media_attached_status.reload.sensitive).to eq true + end + end + end + + context 'action as submit button' do + subject { post :create, params: { report_id: report.id, text: text, action => '' } } + it_behaves_like 'all action types' + end + + context 'action as submit button' do + subject { post :create, params: { report_id: report.id, text: text, moderation_action: action } } + it_behaves_like 'all action types' + end + end end