diff --git a/app/controllers/admin/reports_controller.rb b/app/controllers/admin/reports_controller.rb index aa877f1448c..95a304768ce 100644 --- a/app/controllers/admin/reports_controller.rb +++ b/app/controllers/admin/reports_controller.rb @@ -6,7 +6,16 @@ module Admin def index authorize :report, :index? + + # We previously only supported searching reports by target account domain, + # target account ID or account ID, we now have more search options, but + # it's important that we don't break any saved queries people may have: + return redirect_to_new_filter if reports_filter.outdated? + @reports = filtered_reports.page(params[:page]) + rescue Mastodon::InvalidParameterError => e + flash.now[:error] = e.message + @reports = [] end def show @@ -49,12 +58,20 @@ module Admin private + def reports_filter + @reports_filter ||= ReportFilter.new(filter_params) + end + def filtered_reports - ReportFilter.new(filter_params).results.order(id: :desc).includes(:account, :target_account) + reports_filter.results.order(id: :desc).includes(:account, :target_account) end def filter_params - params.slice(*ReportFilter::KEYS).permit(*ReportFilter::KEYS) + params.slice(*ReportFilter::ALL_KEYS).permit(*ReportFilter::ALL_KEYS) + end + + def redirect_to_new_filter + redirect_to admin_reports_path(reports_filter.updated_filter) end def set_report diff --git a/app/controllers/api/v1/admin/reports_controller.rb b/app/controllers/api/v1/admin/reports_controller.rb index 9b5beeab67e..d53c36a9003 100644 --- a/app/controllers/api/v1/admin/reports_controller.rb +++ b/app/controllers/api/v1/admin/reports_controller.rb @@ -14,13 +14,7 @@ class Api::V1::Admin::ReportsController < Api::BaseController after_action :verify_authorized after_action :insert_pagination_headers, only: :index - FILTER_PARAMS = %i( - resolved - account_id - target_account_id - ).freeze - - PAGINATION_PARAMS = (%i(limit) + FILTER_PARAMS).freeze + PAGINATION_PARAMS = (%i(limit) + ReportFilter::ALL_KEYS).freeze def index authorize :report, :index? @@ -86,7 +80,16 @@ class Api::V1::Admin::ReportsController < Api::BaseController end def filter_params - params.permit(*FILTER_PARAMS) + filter_params = params.slice(*ReportFilter::API_KEYS).permit(*ReportFilter::API_KEYS) + + resolved = filter_params.delete(:resolved) + if resolved.present? + filter_params[:status] = ActiveModel::Type::Boolean.new.cast(resolved) ? 'resolved' : 'unresolved' + elsif filter_params[:status].nil? + filter_params[:status] = 'unresolved' + end + + filter_params end def next_path diff --git a/app/javascript/styles/mastodon/admin.scss b/app/javascript/styles/mastodon/admin.scss index c3a14a4fbde..c9dcf2d3286 100644 --- a/app/javascript/styles/mastodon/admin.scss +++ b/app/javascript/styles/mastodon/admin.scss @@ -2177,3 +2177,12 @@ a.sparkline { } } } + +select#search_type { + width: 33ch; +} + +.input--with-select { + display: flex; + gap: 15px; +} diff --git a/app/models/report_filter.rb b/app/models/report_filter.rb index 9d2b0fb3742..77848a5ce63 100644 --- a/app/models/report_filter.rb +++ b/app/models/report_filter.rb @@ -2,10 +2,39 @@ class ReportFilter KEYS = %i( + status + search_type + search_term + target_origin + account_id + target_account_id + ).freeze + + OUTDATED_ADMIN_KEYS = %i( + resolved + by_target_domain + account_id + target_account_id + ).freeze + + ALL_KEYS = KEYS + OUTDATED_ADMIN_KEYS + API_KEYS = KEYS + %i( resolved account_id target_account_id - by_target_domain + ).freeze + + SEARCH_TYPES = %w( + source + target + ).freeze + + TARGET_ORIGINS = %w( + local + remote + ).freeze + + FILTER_PARAMS = %i( target_origin ).freeze @@ -15,11 +44,71 @@ class ReportFilter @params = params end - def results - scope = Report.unresolved + def outdated? + # We always need a status parameter: + return true if @params.exclude? :status + OUTDATED_ADMIN_KEYS.any? { |key, _value| @params.include? key } + end + + def updated_filter + updated_params = @params.to_hash + + # Old parameters: + by_target_domain = updated_params.delete('by_target_domain') + account_id = updated_params.delete('account_id') + target_account_id = updated_params.delete('target_account_id') + resolved = updated_params.delete('resolved') + existing_status = updated_params.delete('status') + + status_filter = if existing_status + existing_status + elsif resolved.present? + resolved == '1' ? 'resolved' : 'unresolved' + elsif by_target_domain || target_account_id || account_id + 'all' + else + 'unresolved' + end + + updated_params['status'] = status_filter + + if by_target_domain + return updated_params.merge({ + search_type: 'target', + search_term: by_target_domain, + }) + end + + account = if account_id + Account.find(account_id) + elsif target_account_id + Account.find(target_account_id) + end + + if account + return updated_params.merge({ + search_type: target_account_id.present? ? 'target' : 'source', + search_term: "@#{account.acct}", + }) + end + + updated_params + end + + def results + raise Mastodon::InvalidParameterError, "Unknown parameter(s): #{unknown_params.join(', ')}" if unknown_params.any? + + scope = initial_scope + + # If we're searching, then no other filters can be applied, as the other + # filters conflict with the search filter: + return scope.merge search_scope if searching? + + # Otherwise, apply the other filters relevant_params.each do |key, value| - scope = scope.merge scope_for(key, value) + new_scope = scope_for(key, value) + scope = scope.merge new_scope if new_scope end scope @@ -29,39 +118,99 @@ class ReportFilter def relevant_params params.tap do |args| - args.delete(:target_origin) if origin_is_remote_and_domain_present? + args.delete(:status) + args.delete(:search_type) + args.delete(:search_term) end end - def origin_is_remote_and_domain_present? - params[:target_origin] == 'remote' && params[:by_target_domain].present? + def initial_scope + case params[:status] + when 'resolved' + Report.resolved + when 'all' + Report.unscoped + else + # catches both no status and 'unresolved' + Report.unresolved + end + end + + def account_filter + if params[:search_term].starts_with? '@' + username, domain = params[:search_term].delete_prefix('@').split('@', 2) + + # If the domain part is the local domain, we remove the domain part: + domain = nil if TagManager.instance.local_domain?(domain) + + # It doesn't make sense to search for `@username@domain` since we don't + # have the reporter's full handle for remote reports, we only know the + # origin domain. + raise Mastodon::InvalidParameterError, 'You cannot search for reports from a specific remote user' if search_type == :source && domain.present? + + # Ensure we have a valid username: + raise Mastodon::InvalidParameterError, "Invalid username for search: #{username}" unless Account::USERNAME_ONLY_RE.match?(username) + + Account.where(username: username, domain: domain) + else + domain = params[:search_term] + + # If the domain part is the local domain, we need to use nil for the domain search: + domain = nil if TagManager.instance.local_domain?(domain) + + # FIXME: We should probably find a way to reuse DomainValidator here: + raise Mastodon::InvalidParameterError, "Invalid domain for search: #{domain}" if domain.present? && !domain.include?('.') + + Account.where(domain: domain) + end + end + + def search_type + if SEARCH_TYPES.include? params[:search_type] + params[:search_type].to_sym + else + raise Mastodon::InvalidParameterError, "Invalid search type: #{params[:search_type]}" + end + end + + def search_scope + case search_type + when :target + Report.where(target_account: account_filter) + when :source + Report.where(account: account_filter) + end end def scope_for(key, value) case key.to_sym - when :by_target_domain - Report.where(target_account: Account.where(domain: value)) - when :resolved - Report.resolved - when :account_id - Report.where(account_id: value) - when :target_account_id - Report.where(target_account_id: value) when :target_origin target_origin_scope(value) + when :target_account_id + Report.where(target_account_id: value) + when :account_id + Report.where(account_id: value) else raise Mastodon::InvalidParameterError, "Unknown filter: #{key}" end end def target_origin_scope(value) + raise Mastodon::InvalidParameterError, "Unknown origin value: #{value}" unless TARGET_ORIGINS.include? value + case value.to_sym when :local Report.where(target_account: Account.local) when :remote Report.where(target_account: Account.remote) - else - raise Mastodon::InvalidParameterError, "Unknown value: #{value}" end end + + def searching? + params[:search_term].present? && params[:search_type].present? + end + + def unknown_params + params.keys.reject { |param| ReportFilter::KEYS.include? param.to_sym } + end end diff --git a/app/views/admin/reports/index.html.haml b/app/views/admin/reports/index.html.haml index e08218a05d9..716dd27c8bb 100644 --- a/app/views/admin/reports/index.html.haml +++ b/app/views/admin/reports/index.html.haml @@ -5,31 +5,36 @@ .filter-subset %strong= t('admin.reports.status') %ul - %li= filter_link_to t('admin.reports.unresolved'), resolved: nil - %li= filter_link_to t('admin.reports.resolved'), resolved: '1' + %li= filter_link_to t('admin.reports.all'), status: 'all' + %li= filter_link_to t('admin.reports.unresolved'), status: 'unresolved' + %li= filter_link_to t('admin.reports.resolved'), status: 'resolved' .filter-subset %strong= t('admin.reports.target_origin') %ul %li= filter_link_to t('admin.accounts.location.all'), target_origin: nil - %li= filter_link_to t('admin.accounts.location.local'), target_origin: 'local' - %li= filter_link_to t('admin.accounts.location.remote'), target_origin: 'remote' + %li= filter_link_to t('admin.accounts.location.local'), target_origin: 'local', search_term: nil, search_type: nil + %li= filter_link_to t('admin.accounts.location.remote'), target_origin: 'remote', search_term: nil, search_type: nil = form_with url: admin_reports_url, method: :get, class: :simple_form do |form| .fields-group - - ReportFilter::KEYS.each do |key| - = form.hidden_field key, value: params[key] if params[key].present? + = form.hidden_field :status, value: 'all' - - %i(by_target_domain).each do |key| - .input.string.optional - = form.text_field key, - value: params[key], - class: 'string optional', - placeholder: I18n.t("admin.reports.#{key}") + .input.string.optional.input--with-select + = form.select :search_type, options_for_select([[I18n.t('admin.reports.search_type_target'), 'target'], [I18n.t('admin.reports.search_type_source'), 'source']], params[:search_type] || 'target') + = form.text_field :search_term, + value: params[:search_term], + class: 'string optional', + placeholder: I18n.t('admin.reports.search_term') + + %p.hint= t('admin.reports.search_help_html') .actions %button.button= t('admin.accounts.search') = link_to t('admin.accounts.reset'), admin_reports_path, class: 'button button--dangerous' +- if @reports.empty? + = nothing_here 'nothing-here' + - @reports.group_by(&:target_account).each do |target_account, reports| .report-card .report-card__profile @@ -72,8 +77,14 @@ = t('admin.reports.forwarded_to', domain: target_account.domain) .report-card__summary__item__assigned + - if report.action_taken? + = t('admin.reports.resolved') + - else + = t('admin.reports.unresolved') + %br - if report.assigned_account.present? = admin_account_link_to report.assigned_account - else - \- -= paginate @reports + = t('admin.reports.not_assigned') + += paginate @reports unless @reports.empty? diff --git a/app/views/admin/reports/show.html.haml b/app/views/admin/reports/show.html.haml index 86f87f197eb..8f6fd1c3a2e 100644 --- a/app/views/admin/reports/show.html.haml +++ b/app/views/admin/reports/show.html.haml @@ -61,7 +61,11 @@ - if @report.unresolved? %hr.spacer/ - %p#actions= t(@report.target_account.local? ? 'admin.reports.actions_description_html' : 'admin.reports.actions_description_remote_html') + %p#actions + - if @report.target_account.local? + = t('admin.reports.actions_description_html') + - else + = t('admin.reports.actions_description_remote_html') = render partial: 'admin/reports/actions', locals: { report: @report, statuses: @statuses } diff --git a/config/locales/en.yml b/config/locales/en.yml index ffe6028e729..c822b8d5f90 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -665,13 +665,13 @@ en: actions_description_remote_html: Decide which action to take to resolve this report. This will only affect how your server communicates with this remote account and handle its content. actions_no_posts: This report doesn't have any associated posts to delete add_to_report: Add more to report + all: all already_suspended_badges: local: Already suspended on this server remote: Already suspended on their server are_you_sure: Are you sure? assign_to_self: Assign to me assigned: Assigned moderator - by_target_domain: Domain of reported account cancel: Cancel category: Category category_description_html: The reason this account and/or content was reported will be cited in communication with the reported account @@ -689,6 +689,7 @@ en: mark_as_sensitive: Mark as sensitive mark_as_unresolved: Mark as unresolved no_one_assigned: No one + not_assigned: Not Assigned notes: create: Add note create_and_resolve: Resolve with note @@ -698,15 +699,16 @@ en: 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 report: 'Report #%{id}' - reported_account: Reported account reported_by: Reported by reported_with_application: Reported with application resolved: Resolved resolved_msg: Report successfully resolved! + search_help_html: 'Note: You can only search for reports about @username@domain, as remote reports are only from a domain, not a specific account.' + search_term: domain, @username or @username@domain + search_type_source: Reports from + search_type_target: Reports about skip_to_actions: Skip to actions status: Status statuses: Reported content @@ -734,7 +736,6 @@ en: unassign: Unassign unknown_action_msg: 'Unknown action: %{action}' unresolved: Unresolved - updated_at: Updated view_profile: View profile roles: add_new: Add role diff --git a/config/navigation.rb b/config/navigation.rb index 66fdfdfe585..04be5555a1f 100644 --- a/config/navigation.rb +++ b/config/navigation.rb @@ -52,7 +52,7 @@ SimpleNavigation::Configuration.run do |navigation| end n.item :moderation, safe_join([material_symbol('gavel'), t('moderation.title')]), nil, if: -> { current_user.can?(:manage_reports, :view_audit_log, :manage_users, :manage_invites, :manage_taxonomies, :manage_federation, :manage_blocks) && !self_destruct } do |s| - s.item :reports, safe_join([material_symbol('flag'), t('admin.reports.title')]), admin_reports_path, highlights_on: %r{/admin/reports|admin/report_notes}, if: -> { current_user.can?(:manage_reports) } + s.item :reports, safe_join([material_symbol('flag'), t('admin.reports.title')]), admin_reports_path({ status: 'unresolved' }), highlights_on: %r{/admin/reports|admin/report_notes}, if: -> { current_user.can?(:manage_reports) } s.item :appeals, safe_join([material_symbol('feedback'), t('admin.disputes.appeals.title')]), admin_disputes_appeals_path, highlights_on: %r{/admin/disputes/appeals}, if: -> { current_user.can?(:manage_appeals) } s.item :accounts, safe_join([material_symbol('groups'), t('admin.accounts.title')]), admin_accounts_path(origin: 'local'), highlights_on: %r{/admin/accounts|admin/account_moderation_notes|/admin/pending_accounts|/admin/users}, if: -> { current_user.can?(:manage_users) } s.item :tags, safe_join([material_symbol('tag'), t('admin.tags.title')]), admin_tags_path, highlights_on: %r{/admin/tags}, if: -> { current_user.can?(:manage_taxonomies) } diff --git a/spec/controllers/admin/reports_controller_spec.rb b/spec/controllers/admin/reports_controller_spec.rb new file mode 100644 index 00000000000..eb32c21f599 --- /dev/null +++ b/spec/controllers/admin/reports_controller_spec.rb @@ -0,0 +1,228 @@ +# frozen_string_literal: true + +require 'rails_helper' +require 'debug' + +RSpec.describe Admin::ReportsController do + render_views + + let(:user) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')) } + + before do + sign_in user, scope: :user + end + + describe 'GET #index' do + # This isn't the primary entrypoint anymore, as navigation links with the + # status=unresolved filter applied. + it 'redirects if no filters are supplied' do + get :index + + expect(response) + .to redirect_to admin_reports_path({ status: 'unresolved' }) + end + + it 'returns http success with status filter of unresolved' do + specified = Fabricate(:report, action_taken_at: nil) + Fabricate(:report, action_taken_at: Time.now.utc) + + get :index, params: { status: 'unresolved' } + + expect(response).to have_http_status(200) + + expect(report_groups.size).to eq 1 + expect(report_groups[0][:reports].size).to eq 1 + expect(report_groups[0][:reports][0][:link]).to include specified.id.to_s + end + + it 'returns http success with status filter of resolved' do + specified = Fabricate(:report, action_taken_at: Time.now.utc) + Fabricate(:report, action_taken_at: nil) + + get :index, params: { status: 'resolved' } + + expect(response).to have_http_status(200) + + expect(report_groups.size).to eq 1 + expect(report_groups[0][:reports].size).to eq 1 + expect(report_groups[0][:reports][0][:link]).to include specified.id.to_s + end + + it 'returns http success with status filter of all' do + targeted_account = Fabricate(:account) + resolved = Fabricate(:report, target_account: targeted_account, action_taken_at: Time.now.utc) + unresolved = Fabricate(:report, target_account: targeted_account, action_taken_at: nil) + + get :index, params: { status: 'all' } + + expect(response).to have_http_status(200) + + expect(report_groups.size).to eq 1 + expect(report_groups[0][:target_account_link]).to include targeted_account.id.to_s + expect(report_groups[0][:reports].size).to eq 2 + expect(report_groups[0][:reports][0][:link]).to include unresolved.id.to_s + expect(report_groups[0][:reports][1][:link]).to include resolved.id.to_s + end + + it 'returns the correct results with a search filter about an account' do + targeted_account = Fabricate(:account) + not_targeted_account = Fabricate(:account) + + targeted = Fabricate(:report, action_taken_at: nil, target_account: targeted_account) + Fabricate(:report, action_taken_at: nil, target_account: not_targeted_account) + + get :index, params: { search_type: 'target', search_term: "@#{targeted_account.acct}", status: 'all' } + + expect(response).to have_http_status(200) + + expect(report_groups.size).to eq 1 + expect(report_groups[0][:target_account_link]).to include targeted_account.id.to_s + expect(report_groups[0][:reports].size).to eq 1 + expect(report_groups[0][:reports][0][:link]).to include targeted.id.to_s + end + + def report_groups + response.parsed_body.css('.report-card').map do |card| + target_account_link = card.css('.report-card__profile a.account__display-name').attr('href').value + reports = card.css('.report-card__summary__item').map do |report| + { + reported_by: report.css('.report-card__summary__item__reported-by').first.text.strip, + link: report.css('.report-card__summary__item__content > a').attr('href').value, + } + end + + { + target_account_link: target_account_link, + reports: reports, + } + end + end + + describe 'outdated filters' do + # As we're changing how the report search field works, we need to ensure we + # don't break anyone's existing searches: + it 'redirects if by_target_domain is used' do + get :index, params: { by_target_domain: 'social.example' } + + expect(response) + .to redirect_to admin_reports_path({ search_type: 'target', search_term: 'social.example', status: 'all' }) + end + + it 'redirects if resolved is used' do + get :index, params: { resolved: '1' } + + expect(response) + .to redirect_to admin_reports_path({ status: 'resolved' }) + end + + it 'redirects if resolved and by_target_domain are used' do + get :index, params: { resolved: '1', by_target_domain: 'social.example' } + + expect(response).to redirect_to admin_reports_path({ + search_type: 'target', + search_term: 'social.example', + status: 'resolved', + }) + end + + it 'redirects if resolved is false and by_target_domain is used' do + get :index, params: { resolved: '0', by_target_domain: 'social.example' } + + expect(response).to redirect_to admin_reports_path({ + search_type: 'target', + search_term: 'social.example', + status: 'unresolved', + }) + end + + it 'redirects if target_account_id filter is used' do + account = Fabricate(:account) + get :index, params: { target_account_id: account.id } + + expect(response).to redirect_to admin_reports_path({ + search_type: 'target', + search_term: "@#{account.acct}", + status: 'all', + }) + end + + it 'redirects if account_id filter is used' do + account = Fabricate(:account) + get :index, params: { account_id: account.id } + + expect(response).to redirect_to admin_reports_path({ + search_type: 'source', + search_term: "@#{account.acct}", + status: 'all', + }) + end + end + end + + describe 'GET #show' do + it 'renders report' do + report = Fabricate(:report) + + get :show, params: { id: report } + + expect(response).to have_http_status(200) + expect(response.body).to include(report.target_account.acct) + end + end + + describe 'POST #resolve' do + it 'resolves the report' do + report = Fabricate(:report) + + put :resolve, params: { id: 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 be true + expect(last_action_log.target).to eq(report) + end + end + + describe 'POST #reopen' do + it 'reopens the report' do + report = Fabricate(:report, action_taken_at: 3.days.ago) + + put :reopen, params: { id: report } + expect(response).to redirect_to(admin_report_path(report)) + report.reload + expect(report.action_taken_by_account).to be_nil + expect(report.action_taken?).to be false + expect(last_action_log.target).to eq(report) + end + end + + describe 'POST #assign_to_self' do + it 'reopens the report' do + report = Fabricate(:report) + + put :assign_to_self, params: { id: report } + expect(response).to redirect_to(admin_report_path(report)) + report.reload + expect(report.assigned_account).to eq user.account + expect(last_action_log.target).to eq(report) + end + end + + describe 'POST #unassign' do + it 'reopens the report' do + report = Fabricate(:report, assigned_account_id: Account.last.id) + + put :unassign, params: { id: report } + expect(response).to redirect_to(admin_report_path(report)) + report.reload + expect(report.assigned_account).to be_nil + expect(last_action_log.target).to eq(report) + end + end + + private + + def last_action_log + Admin::ActionLog.last + end +end diff --git a/spec/models/report_filter_spec.rb b/spec/models/report_filter_spec.rb index 51933e475ad..86894fe7192 100644 --- a/spec/models/report_filter_spec.rb +++ b/spec/models/report_filter_spec.rb @@ -21,7 +21,7 @@ RSpec.describe ReportFilter do describe 'with valid params' do it 'combines filters on Report' do - filter = described_class.new(account_id: '123', resolved: true, target_account_id: '456') + filter = described_class.new(account_id: '123', status: 'resolved', target_account_id: '456') allow(Report).to receive_messages(where: Report.none, resolved: Report.none) filter.results @@ -31,12 +31,12 @@ RSpec.describe ReportFilter do end end - context 'when given remote target_origin and also by_target_domain' do + context 'when given remote search_type and search_term of a domain' do let!(:matching_report) { Fabricate :report, target_account: Fabricate(:account, domain: 'match.example') } let!(:non_matching_report) { Fabricate :report, target_account: Fabricate(:account, domain: 'other.example') } it 'preserves the domain value' do - filter = described_class.new(by_target_domain: 'match.example', target_origin: 'remote') + filter = described_class.new(search_term: 'match.example', search_type: 'target') expect(filter.results) .to include(matching_report) diff --git a/spec/requests/api/v1/admin/reports_spec.rb b/spec/requests/api/v1/admin/reports_spec.rb index 54dd4c9c8c4..24572e8ad1d 100644 --- a/spec/requests/api/v1/admin/reports_spec.rb +++ b/spec/requests/api/v1/admin/reports_spec.rb @@ -67,8 +67,19 @@ RSpec.describe 'Reports' do .to match_array(expected_response) end - context 'with resolved param' do + context 'with outdated resolved param' do let(:params) { { resolved: true } } + let(:scope) { Report.resolved } + + it 'returns the resolved reports' do + subject + + expect(response.parsed_body).to match_array(expected_response) + end + end + + context 'with status param of resolved' do + let(:params) { { status: 'resolved' } } let(:scope) { Report.resolved } it 'returns only the resolved reports' do @@ -78,6 +89,17 @@ RSpec.describe 'Reports' do end end + context 'with status param of all' do + let(:params) { { status: 'all' } } + let(:scope) { Report.all } + + it 'returns only the resolved reports' do + subject + + expect(response.parsed_body).to match_array(expected_response) + end + end + context 'with account_id param' do let(:params) { { account_id: reporter.id } } let(:scope) { Report.unresolved.where(account: reporter) }