From 8b74aa42176dabf77c3b4d02c80bcc47d9d70e8e Mon Sep 17 00:00:00 2001
From: Matt Jankowski <mjankowski@thoughtbot.com>
Date: Fri, 14 Apr 2017 05:10:28 -0400
Subject: [PATCH] Admin reports controller improvements (#1714)

* Simplify admin/reports controller filtering for index

* Rename parameter to resolved

* Fix issue where reports view could not access filter_link_to

* Add coverage for admin/reports controller

* DRY up resolution of related reports for target account

* Clean up admin/reports routes

* Add Report#statuses method

* DRY up current account action taken params

* Rubocop styles
---
 .../admin/reported_statuses_controller.rb     | 18 +++++
 app/controllers/admin/reports_controller.rb   | 70 ++++++++++------
 app/helpers/admin/accounts_helper.rb          |  2 +-
 app/models/report.rb                          |  4 +
 app/views/admin/reports/index.html.haml       |  4 +-
 app/views/admin/reports/show.html.haml        | 12 +--
 config/routes.rb                              |  9 +--
 .../reported_statuses_controller_spec.rb      | 21 +++++
 .../admin/reports_controller_spec.rb          | 80 ++++++++++++++++++-
 spec/fabricators/report_fabricator.rb         |  2 +
 spec/models/report_spec.rb                    | 10 ++-
 11 files changed, 187 insertions(+), 45 deletions(-)
 create mode 100644 app/controllers/admin/reported_statuses_controller.rb
 create mode 100644 spec/controllers/admin/reported_statuses_controller_spec.rb

diff --git a/app/controllers/admin/reported_statuses_controller.rb b/app/controllers/admin/reported_statuses_controller.rb
new file mode 100644
index 000000000..7ae420dfe
--- /dev/null
+++ b/app/controllers/admin/reported_statuses_controller.rb
@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+module Admin
+  class ReportedStatusesController < BaseController
+    def destroy
+      status = Status.find params[:id]
+
+      RemovalWorker.perform_async(status.id)
+      redirect_to admin_report_path(report)
+    end
+
+    private
+
+    def report
+      Report.find(params[:report_id])
+    end
+  end
+end
diff --git a/app/controllers/admin/reports_controller.rb b/app/controllers/admin/reports_controller.rb
index 3c3082318..4a6f9ea7f 100644
--- a/app/controllers/admin/reports_controller.rb
+++ b/app/controllers/admin/reports_controller.rb
@@ -5,38 +5,60 @@ module Admin
     before_action :set_report, except: [:index]
 
     def index
-      @reports = Report.includes(:account, :target_account).order('id desc').page(params[:page])
-      @reports = params[:action_taken].present? ? @reports.resolved : @reports.unresolved
+      @reports = filtered_reports.page(params[:page])
     end
 
-    def show
-      @statuses = Status.where(id: @report.status_ids)
-    end
+    def show; end
 
-    def resolve
-      @report.update(action_taken: true, action_taken_by_account_id: current_account.id)
-      redirect_to admin_report_path(@report)
-    end
-
-    def suspend
-      Admin::SuspensionWorker.perform_async(@report.target_account.id)
-      Report.unresolved.where(target_account: @report.target_account).update_all(action_taken: true, action_taken_by_account_id: current_account.id)
-      redirect_to admin_report_path(@report)
-    end
-
-    def silence
-      @report.target_account.update(silenced: true)
-      Report.unresolved.where(target_account: @report.target_account).update_all(action_taken: true, action_taken_by_account_id: current_account.id)
-      redirect_to admin_report_path(@report)
-    end
-
-    def remove
-      RemovalWorker.perform_async(params[:status_id])
+    def update
+      process_report
       redirect_to admin_report_path(@report)
     end
 
     private
 
+    def process_report
+      case params[:outcome].to_s
+      when 'resolve'
+        @report.update(action_taken_by_current_attributes)
+      when 'suspend'
+        Admin::SuspensionWorker.perform_async(@report.target_account.id)
+        resolve_all_target_account_reports
+      when 'silence'
+        @report.target_account.update(silenced: true)
+        resolve_all_target_account_reports
+      else
+        raise ActiveRecord::RecordNotFound
+      end
+    end
+
+    def action_taken_by_current_attributes
+      { action_taken: true, action_taken_by_account_id: current_account.id }
+    end
+
+    def resolve_all_target_account_reports
+      unresolved_reports_for_target_account.update_all(
+        action_taken_by_current_attributes
+      )
+    end
+
+    def unresolved_reports_for_target_account
+      Report.where(
+        target_account: @report.target_account
+      ).unresolved
+    end
+
+    def filtered_reports
+      filtering_scope.order('id desc').includes(
+        :account,
+        :target_account
+      )
+    end
+
+    def filtering_scope
+      params[:resolved].present? ? Report.resolved : Report.unresolved
+    end
+
     def set_report
       @report = Report.find(params[:id])
     end
diff --git a/app/helpers/admin/accounts_helper.rb b/app/helpers/admin/accounts_helper.rb
index 6cda77819..497abf80d 100644
--- a/app/helpers/admin/accounts_helper.rb
+++ b/app/helpers/admin/accounts_helper.rb
@@ -2,7 +2,7 @@
 
 module Admin::AccountsHelper
   def filter_params(more_params)
-    params.permit(:local, :remote, :by_domain, :silenced, :suspended, :recent).merge(more_params)
+    params.permit(:local, :remote, :by_domain, :silenced, :suspended, :recent, :resolved).merge(more_params)
   end
 
   def filter_link_to(text, more_params)
diff --git a/app/models/report.rb b/app/models/report.rb
index fd8e46aac..54157ed8c 100644
--- a/app/models/report.rb
+++ b/app/models/report.rb
@@ -7,4 +7,8 @@ class Report < ApplicationRecord
 
   scope :unresolved, -> { where(action_taken: false) }
   scope :resolved,   -> { where(action_taken: true) }
+
+  def statuses
+    Status.where(id: status_ids)
+  end
 end
diff --git a/app/views/admin/reports/index.html.haml b/app/views/admin/reports/index.html.haml
index d5deec8f6..7309c719a 100644
--- a/app/views/admin/reports/index.html.haml
+++ b/app/views/admin/reports/index.html.haml
@@ -5,8 +5,8 @@
   .filter-subset
     %strong= t('admin.reports.status')
     %ul
-      %li= filter_link_to t('admin.reports.unresolved'), action_taken: nil
-      %li= filter_link_to t('admin.reports.resolved'), action_taken: '1'
+      %li= filter_link_to t('admin.reports.unresolved'), resolved: nil
+      %li= filter_link_to t('admin.reports.resolved'), resolved: '1'
 
 = form_tag do
 
diff --git a/app/views/admin/reports/show.html.haml b/app/views/admin/reports/show.html.haml
index a7430f396..5391d99a8 100644
--- a/app/views/admin/reports/show.html.haml
+++ b/app/views/admin/reports/show.html.haml
@@ -14,15 +14,15 @@
   \:
   = @report.comment.presence || t('reports.comment.none')
 
-- unless @statuses.empty?
+- unless @report.statuses.empty?
   %hr/
 
-  - @statuses.each do |status|
+  - @report.statuses.each do |status|
     .report-status
       .activity-stream.activity-stream-headless
         .entry= render partial: 'stream_entries/simple_status', locals: { status: status }
       .report-status__actions
-        = link_to remove_admin_report_path(@report, status_id: status.id), method: :post, class: 'icon-button', style: 'font-size: 24px; width: 24px; height: 24px', title: t('admin.reports.delete') do
+        = link_to admin_report_reported_status_path(@report, status), method: :delete, class: 'icon-button', style: 'font-size: 24px; width: 24px; height: 24px', title: t('admin.reports.delete') do
           = fa_icon 'trash'
 
 - if !@report.action_taken?
@@ -30,10 +30,10 @@
 
   %div{ style: 'overflow: hidden' }
     %div{ style: 'float: right' }
-      = link_to t('admin.reports.silence_account'), silence_admin_report_path(@report), method: :post, class: 'button'
-      = link_to t('admin.reports.suspend_account'), suspend_admin_report_path(@report), method: :post, class: 'button'
+      = 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'), resolve_admin_report_path(@report), method: :post, class: 'button'
+      = 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?
   %hr/
 
diff --git a/config/routes.rb b/config/routes.rb
index 6e48d3b92..045be940e 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -80,13 +80,8 @@ Rails.application.routes.draw do
     resources :domain_blocks, only: [:index, :new, :create]
     resources :settings, only: [:index, :update]
 
-    resources :reports, only: [:index, :show] do
-      member do
-        post :resolve
-        post :silence
-        post :suspend
-        post :remove
-      end
+    resources :reports, only: [:index, :show, :update] do
+      resources :reported_statuses, only: :destroy
     end
 
     resources :accounts, only: [:index, :show] do
diff --git a/spec/controllers/admin/reported_statuses_controller_spec.rb b/spec/controllers/admin/reported_statuses_controller_spec.rb
new file mode 100644
index 000000000..4d6926e1a
--- /dev/null
+++ b/spec/controllers/admin/reported_statuses_controller_spec.rb
@@ -0,0 +1,21 @@
+require 'rails_helper'
+
+describe Admin::ReportedStatusesController do
+  let(:user) { Fabricate(:user, admin: true) }
+  before do
+    sign_in user, scope: :user
+  end
+
+  describe 'DELETE #destroy' do
+    it 'removes a status' do
+      report = Fabricate(:report)
+      status = Fabricate(:status)
+      allow(RemovalWorker).to receive(:perform_async)
+
+      delete :destroy, params: { report_id: report, id: status }
+      expect(response).to redirect_to(admin_report_path(report))
+      expect(RemovalWorker).
+        to have_received(:perform_async).with(status.id)
+    end
+  end
+end
diff --git a/spec/controllers/admin/reports_controller_spec.rb b/spec/controllers/admin/reports_controller_spec.rb
index 622ea87c1..ab93c6271 100644
--- a/spec/controllers/admin/reports_controller_spec.rb
+++ b/spec/controllers/admin/reports_controller_spec.rb
@@ -1,14 +1,86 @@
 require 'rails_helper'
 
-RSpec.describe Admin::ReportsController, type: :controller do
+describe Admin::ReportsController do
+  let(:user) { Fabricate(:user, admin: true) }
+  before do
+    sign_in user, scope: :user
+  end
+
   describe 'GET #index' do
-    before do
-      sign_in Fabricate(:user, admin: true), scope: :user
+    it 'returns http success with no filters' do
+      allow(Report).to receive(:unresolved).and_return(Report.all)
+      get :index
+
+      expect(response).to have_http_status(:success)
+      expect(Report).to have_received(:unresolved)
     end
 
+    it 'returns http success with resolved filter' do
+      allow(Report).to receive(:resolved).and_return(Report.all)
+      get :index, params: { resolved: 1 }
+
+      expect(response).to have_http_status(:success)
+      expect(Report).to have_received(:resolved)
+    end
+  end
+
+  describe 'GET #show' do
     it 'returns http success' do
-      get :index
+      report = Fabricate(:report)
+
+      get :show, params: { id: report }
       expect(response).to have_http_status(:success)
     end
   end
+
+  describe 'PUT #update' do
+    describe 'with an unknown outcome' do
+      it 'rejects the change' do
+        report = Fabricate(:report)
+        put :update, params: { id: report, outcome: 'unknown' }
+
+        expect(response).to have_http_status(:missing)
+      end
+    end
+
+    describe 'with an outcome of `resolve`' do
+      it 'resolves the report' do
+        report = Fabricate(:report)
+
+        put :update, params: { id: report, outcome: 'resolve' }
+        expect(response).to redirect_to(admin_report_path(report))
+        report.reload
+        expect(report.action_taken_by_account).to eq user.account
+        expect(report.action_taken).to eq true
+      end
+    end
+
+    describe 'with an outcome of `suspend`' do
+      it 'suspends the reported account' do
+        report = Fabricate(:report)
+        allow(Admin::SuspensionWorker).to receive(:perform_async)
+
+        put :update, params: { id: report, outcome: 'suspend' }
+        expect(response).to redirect_to(admin_report_path(report))
+        report.reload
+        expect(report.action_taken_by_account).to eq user.account
+        expect(report.action_taken).to eq true
+        expect(Admin::SuspensionWorker).
+          to have_received(:perform_async).with(report.target_account_id)
+      end
+    end
+
+    describe 'with an outsome of `silence`' do
+      it 'silences the reported account' do
+        report = Fabricate(:report)
+
+        put :update, params: { id: report, outcome: 'silence' }
+        expect(response).to redirect_to(admin_report_path(report))
+        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
+  end
 end
diff --git a/spec/fabricators/report_fabricator.rb b/spec/fabricators/report_fabricator.rb
index b9fa360a7..5bd4a63f0 100644
--- a/spec/fabricators/report_fabricator.rb
+++ b/spec/fabricators/report_fabricator.rb
@@ -1,4 +1,6 @@
 Fabricator(:report) do
+  account
+  target_account { Fabricate(:account) }
   comment      "You nasty"
   action_taken false
 end
diff --git a/spec/models/report_spec.rb b/spec/models/report_spec.rb
index ade53cffa..80b8cc55b 100644
--- a/spec/models/report_spec.rb
+++ b/spec/models/report_spec.rb
@@ -1,5 +1,13 @@
 require 'rails_helper'
 
-RSpec.describe Report, type: :model do
+describe Report do
+  describe 'statuses' do
+    it 'returns the statuses for the report' do
+      status = Fabricate(:status)
+      _other = Fabricate(:status)
+      report = Fabricate(:report, status_ids: [status.id])
 
+      expect(report.statuses).to eq [status]
+    end
+  end
 end