From e85cffb2362f914c0f2f7ced4112430b30bc7997 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Mon, 2 Apr 2018 22:04:14 +0200 Subject: [PATCH] Feature: Report improvements (#6967) (#7000) * Implement Assignment of Reports (#6967) * Change translation of admin.report.comment.label to "Report Comment" for clarity As we'll soon add the ability for reports to have comments on them, this clarification makes sense. * Implement notes for Reports This enables moderators to leave comments about a report whilst they work on it * Fix display of report moderation notes * Allow reports to be reopened / marked as unresolved * Redirect to reports listing upon resolution of report * Implement "resolve with note" functionality * Add inverse relationship for report notes * Remove additional database querying when loading report notes * Fix tests for reports * Fix localisations for report notes / reports --- .../admin/report_notes_controller.rb | 49 +++++++++++ app/controllers/admin/reports_controller.rb | 20 ++++- app/helpers/admin/action_logs_helper.rb | 2 +- app/models/account.rb | 2 + app/models/report.rb | 4 + app/models/report_note.rb | 21 +++++ app/policies/report_note_policy.rb | 17 ++++ .../admin/report_notes/_report_note.html.haml | 11 +++ app/views/admin/reports/_report.html.haml | 5 ++ app/views/admin/reports/index.html.haml | 1 + app/views/admin/reports/show.html.haml | 84 +++++++++++++++---- config/locales/en.yml | 22 ++++- config/routes.rb | 2 + ...1200_add_assigned_account_id_to_reports.rb | 5 ++ .../20180402040909_create_report_notes.rb | 14 ++++ db/schema.rb | 16 +++- .../admin/reports_controller_spec.rb | 40 ++++++++- 17 files changed, 290 insertions(+), 25 deletions(-) create mode 100644 app/controllers/admin/report_notes_controller.rb create mode 100644 app/models/report_note.rb create mode 100644 app/policies/report_note_policy.rb create mode 100644 app/views/admin/report_notes/_report_note.html.haml create mode 100644 db/migrate/20180402031200_add_assigned_account_id_to_reports.rb create mode 100644 db/migrate/20180402040909_create_report_notes.rb diff --git a/app/controllers/admin/report_notes_controller.rb b/app/controllers/admin/report_notes_controller.rb new file mode 100644 index 00000000000..ef8c0f4690b --- /dev/null +++ b/app/controllers/admin/report_notes_controller.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +module Admin + class ReportNotesController < BaseController + before_action :set_report_note, only: [:destroy] + + def create + authorize ReportNote, :create? + + @report_note = current_account.report_notes.new(resource_params) + + if @report_note.save + if params[:create_and_resolve] + @report_note.report.update!(action_taken: true, action_taken_by_account_id: current_account.id) + log_action :resolve, @report_note.report + + redirect_to admin_reports_path, notice: I18n.t('admin.reports.resolved_msg') + else + redirect_to admin_report_path(@report_note.report_id), notice: I18n.t('admin.report_notes.created_msg') + end + else + @report = @report_note.report + @report_notes = @report.notes.latest + @form = Form::StatusBatch.new + + render template: 'admin/reports/show' + end + end + + def destroy + authorize @report_note, :destroy? + @report_note.destroy! + redirect_to admin_report_path(@report_note.report_id), notice: I18n.t('admin.report_notes.destroyed_msg') + end + + private + + def resource_params + params.require(:report_note).permit( + :content, + :report_id + ) + end + + def set_report_note + @report_note = ReportNote.find(params[:id]) + end + end +end diff --git a/app/controllers/admin/reports_controller.rb b/app/controllers/admin/reports_controller.rb index 75db6b78aa4..fc3785e3b44 100644 --- a/app/controllers/admin/reports_controller.rb +++ b/app/controllers/admin/reports_controller.rb @@ -11,19 +11,35 @@ module Admin def show authorize @report, :show? + @report_note = @report.notes.new + @report_notes = @report.notes.latest @form = Form::StatusBatch.new end def update authorize @report, :update? process_report - redirect_to admin_report_path(@report) + + if @report.action_taken? + redirect_to admin_reports_path, notice: I18n.t('admin.reports.resolved_msg') + else + redirect_to admin_report_path(@report) + end end private def process_report case params[:outcome].to_s + when 'assign_to_self' + @report.update!(assigned_account_id: current_account.id) + log_action :assigned_to_self, @report + when 'unassign' + @report.update!(assigned_account_id: nil) + log_action :unassigned, @report + when 'reopen' + @report.update!(action_taken: false, action_taken_by_account_id: nil) + log_action :reopen, @report when 'resolve' @report.update!(action_taken_by_current_attributes) log_action :resolve, @report @@ -32,11 +48,13 @@ module Admin log_action :resolve, @report log_action :suspend, @report.target_account resolve_all_target_account_reports + @report.reload when 'silence' @report.target_account.update!(silenced: true) log_action :resolve, @report log_action :silence, @report.target_account resolve_all_target_account_reports + @report.reload else raise ActiveRecord::RecordNotFound end diff --git a/app/helpers/admin/action_logs_helper.rb b/app/helpers/admin/action_logs_helper.rb index 78278c70047..7c26c0b05b8 100644 --- a/app/helpers/admin/action_logs_helper.rb +++ b/app/helpers/admin/action_logs_helper.rb @@ -86,7 +86,7 @@ module Admin::ActionLogsHelper opposite_verbs?(log) ? 'negative' : 'positive' when :update, :reset_password, :disable_2fa, :memorialize 'neutral' - when :demote, :silence, :disable, :suspend, :remove_avatar + when :demote, :silence, :disable, :suspend, :remove_avatar, :reopen 'negative' when :destroy opposite_verbs?(log) ? 'positive' : 'negative' diff --git a/app/models/account.rb b/app/models/account.rb index a34b6a2d3cc..446144a3e45 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -95,6 +95,8 @@ class Account < ApplicationRecord has_many :reports has_many :targeted_reports, class_name: 'Report', foreign_key: :target_account_id + has_many :report_notes, dependent: :destroy + # Moderation notes has_many :account_moderation_notes, dependent: :destroy has_many :targeted_moderation_notes, class_name: 'AccountModerationNote', foreign_key: :target_account_id, dependent: :destroy diff --git a/app/models/report.rb b/app/models/report.rb index dd123fc151e..f5b37cb6d04 100644 --- a/app/models/report.rb +++ b/app/models/report.rb @@ -12,12 +12,16 @@ # account_id :integer not null # action_taken_by_account_id :integer # target_account_id :integer not null +# assigned_account_id :integer # class Report < ApplicationRecord belongs_to :account belongs_to :target_account, class_name: 'Account' belongs_to :action_taken_by_account, class_name: 'Account', optional: true + belongs_to :assigned_account, class_name: 'Account', optional: true + + has_many :notes, class_name: 'ReportNote', foreign_key: :report_id, inverse_of: :report, dependent: :destroy scope :unresolved, -> { where(action_taken: false) } scope :resolved, -> { where(action_taken: true) } diff --git a/app/models/report_note.rb b/app/models/report_note.rb new file mode 100644 index 00000000000..3d12cf7b6f9 --- /dev/null +++ b/app/models/report_note.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true +# == Schema Information +# +# Table name: report_notes +# +# id :integer not null, primary key +# content :text not null +# report_id :integer not null +# account_id :integer not null +# created_at :datetime not null +# updated_at :datetime not null +# + +class ReportNote < ApplicationRecord + belongs_to :account + belongs_to :report, inverse_of: :notes + + scope :latest, -> { reorder('created_at ASC') } + + validates :content, presence: true, length: { maximum: 500 } +end diff --git a/app/policies/report_note_policy.rb b/app/policies/report_note_policy.rb new file mode 100644 index 00000000000..694bc096b5a --- /dev/null +++ b/app/policies/report_note_policy.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class ReportNotePolicy < ApplicationPolicy + def create? + staff? + end + + def destroy? + admin? || owner? + end + + private + + def owner? + record.account_id == current_account&.id + end +end diff --git a/app/views/admin/report_notes/_report_note.html.haml b/app/views/admin/report_notes/_report_note.html.haml new file mode 100644 index 00000000000..60ac5d0d5b0 --- /dev/null +++ b/app/views/admin/report_notes/_report_note.html.haml @@ -0,0 +1,11 @@ +%tr + %td + %p + %strong= report_note.account.acct + on + %time.formatted{ datetime: report_note.created_at.iso8601, title: l(report_note.created_at) } + = l report_note.created_at + = table_link_to 'trash', t('admin.reports.notes.delete'), admin_report_note_path(report_note), method: :delete if can?(:destroy, report_note) + %br/ + %br/ + = simple_format(h(report_note.content)) diff --git a/app/views/admin/reports/_report.html.haml b/app/views/admin/reports/_report.html.haml index d5eb161b99b..d266f4840b1 100644 --- a/app/views/admin/reports/_report.html.haml +++ b/app/views/admin/reports/_report.html.haml @@ -17,5 +17,10 @@ %span{ title: t('admin.accounts.media_attachments') } = fa_icon('camera') = report.media_attachments.count + %td + - if report.assigned_account.nil? + \- + - else + = link_to report.assigned_account.acct, admin_account_path(report.assigned_account.id) %td = table_link_to 'circle', t('admin.reports.view'), admin_report_path(report) diff --git a/app/views/admin/reports/index.html.haml b/app/views/admin/reports/index.html.haml index 577c68a8640..3b127c4fc08 100644 --- a/app/views/admin/reports/index.html.haml +++ b/app/views/admin/reports/index.html.haml @@ -20,6 +20,7 @@ %th= t('admin.reports.reported_by') %th= t('admin.reports.comment.label') %th= t('admin.reports.report_contents') + %th= t('admin.reports.assigned') %th %tbody = render @reports diff --git a/app/views/admin/reports/show.html.haml b/app/views/admin/reports/show.html.haml index 5747cc27408..e7634a034c4 100644 --- a/app/views/admin/reports/show.html.haml +++ b/app/views/admin/reports/show.html.haml @@ -4,24 +4,68 @@ - content_for :page_title do = t('admin.reports.report', id: @report.id) +%div{ style: 'overflow: hidden; margin-bottom: 20px' } + - if !@report.action_taken? + %div{ style: 'float: right' } + = link_to t('admin.reports.silence_account'), admin_report_path(@report, outcome: 'silence'), method: :put, class: 'button' + = link_to t('admin.reports.suspend_account'), admin_report_path(@report, outcome: 'suspend'), method: :put, class: 'button' + %div{ style: 'float: left' } + = link_to t('admin.reports.mark_as_resolved'), admin_report_path(@report, outcome: 'resolve'), method: :put, class: 'button' + - else + = link_to t('admin.reports.mark_as_unresolved'), admin_report_path(@report, outcome: 'reopen'), method: :put, class: 'button' + +.table-wrapper + %table.table.inline-table + %tbody + %tr + %th= t('admin.reports.updated_at') + %td{colspan: 2} + %time.formatted{ datetime: @report.updated_at.iso8601 } + %tr + %th= t('admin.reports.status') + %td{colspan: 2} + - if @report.action_taken? + = t('admin.reports.resolved') + = table_link_to 'envelope-open', t('admin.reports.reopen'), admin_report_path(@report, outcome: 'reopen'), method: :put + - else + = t('admin.reports.unresolved') + - if !@report.action_taken_by_account.nil? + %tr + %th= t('admin.reports.action_taken_by') + %td= @report.action_taken_by_account.acct + - else + %tr + %th= t('admin.reports.assigned') + %td + - if @report.assigned_account.nil? + \- + - else + = link_to @report.assigned_account.acct, admin_account_path(@report.assigned_account.id) + %td{style: "text-align: right"} + - if @report.assigned_account != current_user.account + = table_link_to 'user', t('admin.reports.assign_to_self'), admin_report_path(@report, outcome: 'assign_to_self'), method: :put + - if !@report.assigned_account.nil? + = table_link_to 'trash', t('admin.reports.unassign'), admin_report_path(@report, outcome: 'unassign'), method: :put + .report-accounts .report-accounts__item - %strong= t('admin.reports.reported_account') + %h3= t('admin.reports.reported_account') = render 'authorize_follows/card', account: @report.target_account, admin: true = render 'admin/accounts/card', account: @report.target_account .report-accounts__item - %strong= t('admin.reports.reported_by') + %h3= t('admin.reports.reported_by') = render 'authorize_follows/card', account: @report.account, admin: true = render 'admin/accounts/card', account: @report.account -%p - %strong= t('admin.reports.comment.label') - \: - = simple_format(@report.comment.presence || t('admin.reports.comment.none')) +%h3= t('admin.reports.comment.label') + += simple_format(@report.comment.presence || t('admin.reports.comment.none')) - unless @report.statuses.empty? %hr/ + %h3= t('admin.reports.statuses') + = form_for(@form, url: admin_report_reported_statuses_path(@report.id)) do |f| .batch-form-box .batch-checkbox-all @@ -46,14 +90,20 @@ %hr/ -- if !@report.action_taken? - %div{ style: 'overflow: hidden' } - %div{ style: 'float: right' } - = link_to t('admin.reports.silence_account'), admin_report_path(@report, outcome: 'silence'), method: :put, class: 'button' - = link_to t('admin.reports.suspend_account'), admin_report_path(@report, outcome: 'suspend'), method: :put, class: 'button' - %div{ style: 'float: left' } - = link_to t('admin.reports.mark_as_resolved'), admin_report_path(@report, outcome: 'resolve'), method: :put, class: 'button' -- elsif !@report.action_taken_by_account.nil? - %p - %strong #{t('admin.reports.action_taken_by')}: - = @report.action_taken_by_account.acct +%h3= t('admin.reports.notes.label') + +- if @report_notes.length > 0 + .table-wrapper + %table.table + %thead + %tr + %th + %tbody + = render @report_notes + += simple_form_for @report_note, url: admin_report_notes_path do |f| + = render 'shared/error_messages', object: @report_note + = f.input :content + = f.hidden_field :report_id + = f.button :button, t('admin.reports.notes.create'), type: :submit + = f.button :button, t('admin.reports.notes.create_and_resolve'), type: :submit, name: :create_and_resolve diff --git a/config/locales/en.yml b/config/locales/en.yml index fb2bbf4fec2..51d9c906d08 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -137,6 +137,7 @@ en: web: Web action_logs: actions: + assigned_to_self_report: "%{name} assigned report %{target} to themselves" confirm_user: "%{name} confirmed e-mail address of user %{target}" create_custom_emoji: "%{name} uploaded new emoji %{target}" create_domain_block: "%{name} blocked domain %{target}" @@ -153,10 +154,12 @@ en: memorialize_account: "%{name} turned %{target}'s account into a memoriam page" promote_user: "%{name} promoted user %{target}" remove_avatar_user: "%{name} removed %{target}'s avatar" + reopen_report: "%{name} reopened report %{target}" reset_password_user: "%{name} reset password of user %{target}" - resolve_report: "%{name} dismissed report %{target}" + resolve_report: "%{name} resolved report %{target}" silence_account: "%{name} silenced %{target}'s account" suspend_account: "%{name} suspended %{target}'s account" + unassigned_report: "%{name} unassigned report %{target}" unsilence_account: "%{name} unsilenced %{target}'s account" unsuspend_account: "%{name} unsuspended %{target}'s account" update_custom_emoji: "%{name} updated emoji %{target}" @@ -242,15 +245,26 @@ en: expired: Expired title: Filter title: Invites + report_notes: + created_msg: Moderation note successfully created! + destroyed_msg: Moderation note successfully destroyed! reports: action_taken_by: Action taken by are_you_sure: Are you sure? + assign_to_self: Assign to me + assigned: Assigned Moderator comment: - label: Comment + label: Report Comment none: None delete: Delete id: ID mark_as_resolved: Mark as resolved + mark_as_unresolved: Mark as unresolved + notes: + create: Add Note + create_and_resolve: Resolve with Note + delete: Delete + label: Notes nsfw: 'false': Unhide media attachments 'true': Hide media attachments @@ -259,12 +273,16 @@ en: reported_account: Reported account reported_by: Reported by resolved: Resolved + resolved_msg: Report successfully resolved! silence_account: Silence account status: Status + statuses: Reported Toots suspend_account: Suspend account target: Target title: Reports + unassign: Unassign unresolved: Unresolved + updated_at: Updated view: View settings: activity_api_enabled: diff --git a/config/routes.rb b/config/routes.rb index 9a446056210..4b5ba5c967d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -137,6 +137,8 @@ Rails.application.routes.draw do resources :reported_statuses, only: [:create, :update, :destroy] end + resources :report_notes, only: [:create, :destroy] + resources :accounts, only: [:index, :show] do member do post :subscribe diff --git a/db/migrate/20180402031200_add_assigned_account_id_to_reports.rb b/db/migrate/20180402031200_add_assigned_account_id_to_reports.rb new file mode 100644 index 00000000000..0456839c4a1 --- /dev/null +++ b/db/migrate/20180402031200_add_assigned_account_id_to_reports.rb @@ -0,0 +1,5 @@ +class AddAssignedAccountIdToReports < ActiveRecord::Migration[5.1] + def change + add_reference :reports, :assigned_account, null: true, default: nil, foreign_key: { on_delete: :nullify, to_table: :accounts }, index: false + end +end diff --git a/db/migrate/20180402040909_create_report_notes.rb b/db/migrate/20180402040909_create_report_notes.rb new file mode 100644 index 00000000000..732ddf8259c --- /dev/null +++ b/db/migrate/20180402040909_create_report_notes.rb @@ -0,0 +1,14 @@ +class CreateReportNotes < ActiveRecord::Migration[5.1] + def change + create_table :report_notes do |t| + t.text :content, null: false + t.references :report, null: false + t.references :account, null: false + + t.timestamps + end + + add_foreign_key :report_notes, :reports, column: :report_id, on_delete: :cascade + add_foreign_key :report_notes, :accounts, column: :account_id, on_delete: :cascade + end +end diff --git a/db/schema.rb b/db/schema.rb index 18c61dbe021..a9733a2aefb 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180310000000) do +ActiveRecord::Schema.define(version: 20180402040909) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -355,6 +355,16 @@ ActiveRecord::Schema.define(version: 20180310000000) do t.index ["status_id", "preview_card_id"], name: "index_preview_cards_statuses_on_status_id_and_preview_card_id" end + create_table "report_notes", force: :cascade do |t| + t.text "content", null: false + t.bigint "report_id", null: false + t.bigint "account_id", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["account_id"], name: "index_report_notes_on_account_id" + t.index ["report_id"], name: "index_report_notes_on_report_id" + end + create_table "reports", force: :cascade do |t| t.bigint "status_ids", default: [], null: false, array: true t.text "comment", default: "", null: false @@ -364,6 +374,7 @@ ActiveRecord::Schema.define(version: 20180310000000) do t.bigint "account_id", null: false t.bigint "action_taken_by_account_id" t.bigint "target_account_id", null: false + t.bigint "assigned_account_id" t.index ["account_id"], name: "index_reports_on_account_id" t.index ["target_account_id"], name: "index_reports_on_target_account_id" end @@ -569,7 +580,10 @@ ActiveRecord::Schema.define(version: 20180310000000) do add_foreign_key "oauth_access_tokens", "oauth_applications", column: "application_id", name: "fk_f5fc4c1ee3", on_delete: :cascade add_foreign_key "oauth_access_tokens", "users", column: "resource_owner_id", name: "fk_e84df68546", on_delete: :cascade add_foreign_key "oauth_applications", "users", column: "owner_id", name: "fk_b0988c7c0a", on_delete: :cascade + add_foreign_key "report_notes", "accounts", on_delete: :cascade + add_foreign_key "report_notes", "reports", on_delete: :cascade add_foreign_key "reports", "accounts", column: "action_taken_by_account_id", name: "fk_bca45b75fd", on_delete: :nullify + add_foreign_key "reports", "accounts", column: "assigned_account_id", on_delete: :nullify add_foreign_key "reports", "accounts", column: "target_account_id", name: "fk_eb37af34f0", on_delete: :cascade add_foreign_key "reports", "accounts", name: "fk_4b81f7522c", on_delete: :cascade add_foreign_key "session_activations", "oauth_access_tokens", column: "access_token_id", name: "fk_957e5bda89", on_delete: :cascade diff --git a/spec/controllers/admin/reports_controller_spec.rb b/spec/controllers/admin/reports_controller_spec.rb index 71a185147bf..9be298df6af 100644 --- a/spec/controllers/admin/reports_controller_spec.rb +++ b/spec/controllers/admin/reports_controller_spec.rb @@ -61,7 +61,7 @@ describe Admin::ReportsController do report = Fabricate(:report) put :update, params: { id: report, outcome: 'resolve' } - expect(response).to redirect_to(admin_report_path(report)) + expect(response).to redirect_to(admin_reports_path) report.reload expect(report.action_taken_by_account).to eq user.account expect(report.action_taken).to eq true @@ -74,7 +74,7 @@ describe Admin::ReportsController do allow(Admin::SuspensionWorker).to receive(:perform_async) put :update, params: { id: report, outcome: 'suspend' } - expect(response).to redirect_to(admin_report_path(report)) + expect(response).to redirect_to(admin_reports_path) report.reload expect(report.action_taken_by_account).to eq user.account expect(report.action_taken).to eq true @@ -88,12 +88,46 @@ describe Admin::ReportsController do report = Fabricate(:report) put :update, params: { id: report, outcome: 'silence' } - expect(response).to redirect_to(admin_report_path(report)) + expect(response).to redirect_to(admin_reports_path) report.reload expect(report.action_taken_by_account).to eq user.account expect(report.action_taken).to eq true expect(report.target_account).to be_silenced end end + + describe 'with an outsome of `reopen`' do + it 'reopens the report' do + report = Fabricate(:report) + + put :update, params: { id: report, outcome: 'reopen' } + expect(response).to redirect_to(admin_report_path(report)) + report.reload + expect(report.action_taken_by_account).to eq nil + expect(report.action_taken).to eq false + end + end + + describe 'with an outsome of `assign_to_self`' do + it 'reopens the report' do + report = Fabricate(:report) + + put :update, params: { id: report, outcome: 'assign_to_self' } + expect(response).to redirect_to(admin_report_path(report)) + report.reload + expect(report.assigned_account).to eq user.account + end + end + + describe 'with an outsome of `unassign`' do + it 'reopens the report' do + report = Fabricate(:report) + + put :update, params: { id: report, outcome: 'unassign' } + expect(response).to redirect_to(admin_report_path(report)) + report.reload + expect(report.assigned_account).to eq nil + end + end end end