From e06b252b80c992a34d63a3f317599b2cd655db5c Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Sat, 11 Oct 2025 18:53:14 +0200 Subject: [PATCH 1/8] Improve filtering of reports --- app/controllers/admin/reports_controller.rb | 31 +++ .../api/v1/admin/reports_controller.rb | 13 +- app/javascript/styles/mastodon/admin.scss | 9 + app/models/report_filter.rb | 82 +++++++- app/views/admin/reports/index.html.haml | 35 ++-- app/views/admin/reports/show.html.haml | 6 +- config/locales/en.yml | 10 +- config/navigation.rb | 2 +- .../admin/reports_controller_spec.rb | 176 ++++++++++++++++++ spec/models/report_filter_spec.rb | 2 +- spec/requests/api/v1/admin/reports_spec.rb | 23 ++- 11 files changed, 350 insertions(+), 39 deletions(-) create mode 100644 spec/controllers/admin/reports_controller_spec.rb diff --git a/app/controllers/admin/reports_controller.rb b/app/controllers/admin/reports_controller.rb index aa877f1448c..3ee62780ad4 100644 --- a/app/controllers/admin/reports_controller.rb +++ b/app/controllers/admin/reports_controller.rb @@ -6,6 +6,22 @@ module Admin def index authorize :report, :index? + + # We previously only supported searching by target account domain for + # reports, 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 outdated_filter? + + # If there isn't a status filter parameter, redirect to include the status parameter as unresolved, + # this ensures the "status" option menu always shows a highlighted option. + if filter_params.exclude? :status + if params.slice(*ReportFilter::DIRECT_KEYS).present? + return redirect_to admin_reports_path(filter_params.merge({ status: 'all' })) + else + return redirect_to admin_reports_path(filter_params.merge({ status: 'unresolved' })) + end + end + @reports = filtered_reports.page(params[:page]) end @@ -57,6 +73,21 @@ module Admin params.slice(*ReportFilter::KEYS).permit(*ReportFilter::KEYS) end + def outdated_filter? + params.include?(:by_target_domain) || params.include?(:resolved) + end + + def redirect_to_new_filter + by_target_domain = params.delete(:by_target_domain) + resolved = params.delete(:resolved) + + redirect_to admin_reports_path filter_params.merge({ + search_type: 'target', + search_term: by_target_domain, + status: resolved == '1' ? 'resolved' : 'unresolved', + }) + end + def set_report @report = Report.find(params[:id]) end diff --git a/app/controllers/api/v1/admin/reports_controller.rb b/app/controllers/api/v1/admin/reports_controller.rb index 9b5beeab67e..5af412f3d02 100644 --- a/app/controllers/api/v1/admin/reports_controller.rb +++ b/app/controllers/api/v1/admin/reports_controller.rb @@ -14,16 +14,13 @@ 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::KEYS).freeze def index authorize :report, :index? + + return redirect_to api_v1_admin_reports_path filter_params.merge(status: 'resolved') if params[:resolved].present? + render json: @reports, each_serializer: REST::Admin::ReportSerializer end @@ -86,7 +83,7 @@ class Api::V1::Admin::ReportsController < Api::BaseController end def filter_params - params.permit(*FILTER_PARAMS) + params.slice(*ReportFilter::KEYS).permit(*ReportFilter::KEYS) end def next_path diff --git a/app/javascript/styles/mastodon/admin.scss b/app/javascript/styles/mastodon/admin.scss index 2e05284e282..3e674f1d072 100644 --- a/app/javascript/styles/mastodon/admin.scss +++ b/app/javascript/styles/mastodon/admin.scss @@ -2144,3 +2144,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..165c2ba50ac 100644 --- a/app/models/report_filter.rb +++ b/app/models/report_filter.rb @@ -2,10 +2,22 @@ class ReportFilter KEYS = %i( - resolved + status + search_type + search_term + account_id + target_account_id + target_origin + ).freeze + + DIRECT_KEYS = %i( + account_id + target_account_id + ).freeze + + FILTER_PARAMS = %i( account_id target_account_id - by_target_domain target_origin ).freeze @@ -16,10 +28,18 @@ class ReportFilter end def results - scope = Report.unresolved + 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 @@ -37,12 +57,48 @@ class ReportFilter params[:target_origin] == 'remote' && params[:by_target_domain].present? end + 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_search_filter + if params[:search_term].includes? '@' + username, domain = params[:search_term].delete_prefix('@').split('@', 2) + + raise Mastodon::InvalidParameterError, "Invalid username for search: #{username}" unless Account::USERNAME_ONLY_RE.match?(username) + raise Mastodon::InvalidParameterError, "Invalid domain for search: #{domain}" if domain && !domain.includes?('.') + + Account.where(username: username, domain: domain) + else + domain = params[:search_term] + + raise Mastodon::InvalidParameterError, "Invalid domain for search: #{domain}" unless domain.includes?('.') + + Account.where(domain: domain) + end + end + + def search_scope + case params[:search_type].to_sym + when :target + Report.where(target_account: account_search_filter) + when :source + Report.where(account: account_search_filter) + else + raise Mastodon::InvalidParameterError, "Unknown search type: #{params[:search_type]}" + 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 @@ -61,7 +117,15 @@ class ReportFilter when :remote Report.where(target_account: Account.remote) else - raise Mastodon::InvalidParameterError, "Unknown value: #{value}" + raise Mastodon::InvalidParameterError, "Unknown origin 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..df3c50347f0 100644 --- a/app/views/admin/reports/index.html.haml +++ b/app/views/admin/reports/index.html.haml @@ -5,31 +5,34 @@ .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') .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 +75,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 - \- + = t('admin.reports.not_assigned') + = paginate @reports 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 e6604e9284a..f261f038607 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,15 @@ 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_term: domain or @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 +735,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 4829f9efd2d..1abe1ca0b70 100644 --- a/config/navigation.rb +++ b/config/navigation.rb @@ -51,7 +51,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..e20e1602c10 --- /dev/null +++ b/spec/controllers/admin/reports_controller_spec.rb @@ -0,0 +1,176 @@ +# frozen_string_literal: true + +require 'rails_helper' + +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' } + + reports = assigns(:reports).to_a + expect(reports.size).to eq 1 + expect(reports[0]).to eq specified + expect(response).to have_http_status(200) + 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' } + + reports = assigns(:reports).to_a + expect(reports.size).to eq 1 + expect(reports[0]).to eq specified + + expect(response).to have_http_status(200) + end + + it 'returns http success with status filter of all' do + resolved = Fabricate(:report, action_taken_at: Time.now.utc) + unresolved = Fabricate(:report, action_taken_at: nil) + + get :index, params: { status: 'all' } + + reports = assigns(:reports).to_a + expect(reports.size).to eq 2 + + # unresolved is the most recently created report, so it's first: + expect(reports[0]).to eq unresolved + expect(reports[1]).to eq resolved + + expect(response).to have_http_status(200) + end + + it 'returns http success with target_account_id filter' 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: { target_account_id: targeted_account.id } + + reports = assigns(:reports).to_a + expect(reports.size).to eq 1 + expect(reports[0]).to eq targeted + + expect(response).to have_http_status(200) + 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: 'unresolved' }) + end + + it 'redirects if resolved is used' do + get :index, params: { resolved: '1' } + + expect(response) + .to redirect_to admin_reports_path({ search_type: 'target', search_term: nil, 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 + end + end + + describe 'GET #show' do + it 'renders report' do + report = Fabricate(:report) + + get :show, params: { id: report } + + expect(assigns(:report)).to eq report + expect(response).to have_http_status(200) + 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..39b7d3dfa78 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 diff --git a/spec/requests/api/v1/admin/reports_spec.rb b/spec/requests/api/v1/admin/reports_spec.rb index 987f0eda7fb..90a60d16c50 100644 --- a/spec/requests/api/v1/admin/reports_spec.rb +++ b/spec/requests/api/v1/admin/reports_spec.rb @@ -71,8 +71,18 @@ 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 } } + + it 'redirects to the new filter path' do + subject + + expect(response).to redirect_to api_v1_admin_reports_url({ status: 'resolved' }) + end + end + + context 'with status param of resolved' do + let(:params) { { status: 'resolved' } } let(:scope) { Report.resolved } it 'returns only the resolved reports' do @@ -82,6 +92,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(body_as_json).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) } From 4afc05c374571870e9e693f977cb1f4c5a11c341 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Sat, 11 Oct 2025 20:44:16 +0200 Subject: [PATCH 2/8] Improve redirection --- app/controllers/admin/reports_controller.rb | 37 ++---- .../api/v1/admin/reports_controller.rb | 12 +- app/models/report_filter.rb | 121 ++++++++++++++---- app/views/admin/reports/index.html.haml | 4 +- config/locales/en.yml | 3 +- 5 files changed, 123 insertions(+), 54 deletions(-) diff --git a/app/controllers/admin/reports_controller.rb b/app/controllers/admin/reports_controller.rb index 3ee62780ad4..bda8c1613f3 100644 --- a/app/controllers/admin/reports_controller.rb +++ b/app/controllers/admin/reports_controller.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true +require 'debug' module Admin class ReportsController < BaseController before_action :set_report, except: [:index] @@ -10,19 +11,12 @@ module Admin # We previously only supported searching by target account domain for # reports, 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 outdated_filter? - - # If there isn't a status filter parameter, redirect to include the status parameter as unresolved, - # this ensures the "status" option menu always shows a highlighted option. - if filter_params.exclude? :status - if params.slice(*ReportFilter::DIRECT_KEYS).present? - return redirect_to admin_reports_path(filter_params.merge({ status: 'all' })) - else - return redirect_to admin_reports_path(filter_params.merge({ status: 'unresolved' })) - end - end + 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 @@ -65,27 +59,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) - end - - def outdated_filter? - params.include?(:by_target_domain) || params.include?(:resolved) + params.slice(*ReportFilter::ALL_KEYS).permit(*ReportFilter::ALL_KEYS) end def redirect_to_new_filter - by_target_domain = params.delete(:by_target_domain) - resolved = params.delete(:resolved) - - redirect_to admin_reports_path filter_params.merge({ - search_type: 'target', - search_term: by_target_domain, - status: resolved == '1' ? 'resolved' : 'unresolved', - }) + 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 5af412f3d02..520388d01ca 100644 --- a/app/controllers/api/v1/admin/reports_controller.rb +++ b/app/controllers/api/v1/admin/reports_controller.rb @@ -14,12 +14,12 @@ class Api::V1::Admin::ReportsController < Api::BaseController after_action :verify_authorized after_action :insert_pagination_headers, only: :index - PAGINATION_PARAMS = (%i(limit) + ReportFilter::KEYS).freeze + PAGINATION_PARAMS = (%i(limit) + ReportFilter::ALL_KEYS).freeze def index authorize :report, :index? - return redirect_to api_v1_admin_reports_path filter_params.merge(status: 'resolved') if params[:resolved].present? + return redirect_to api_v1_admin_reports_path(reports_filter.updated_filter) if reports_filter.outdated? render json: @reports, each_serializer: REST::Admin::ReportSerializer end @@ -74,8 +74,12 @@ class Api::V1::Admin::ReportsController < Api::BaseController @report = Report.find(params[:id]) end + def reports_filter + @reports_filter ||= ReportFilter.new(filter_params) + end + def filtered_reports - ReportFilter.new(filter_params).results + reports_filter.results end def report_params @@ -83,7 +87,7 @@ class Api::V1::Admin::ReportsController < Api::BaseController end def filter_params - params.slice(*ReportFilter::KEYS).permit(*ReportFilter::KEYS) + params.slice(*ReportFilter::ALL_KEYS).permit(*ReportFilter::ALL_KEYS) end def next_path diff --git a/app/models/report_filter.rb b/app/models/report_filter.rb index 165c2ba50ac..999156d056d 100644 --- a/app/models/report_filter.rb +++ b/app/models/report_filter.rb @@ -1,23 +1,34 @@ # frozen_string_literal: true +require 'debug' class ReportFilter KEYS = %i( status search_type search_term - account_id - target_account_id target_origin ).freeze - DIRECT_KEYS = %i( + OUTDATED_KEYS = %i( + resolved + by_target_domain account_id target_account_id ).freeze + ALL_KEYS = KEYS + OUTDATED_KEYS + + SEARCH_TYPES = %w( + source + target + ).freeze + + TARGET_ORIGINS = %w( + local + remote + ).freeze + FILTER_PARAMS = %i( - account_id - target_account_id target_origin ).freeze @@ -27,6 +38,53 @@ class ReportFilter @params = params end + def outdated? + # We always need a status parameter: + return true if @params.exclude? :status + + OUTDATED_KEYS.any? { |key, _value| @params.include? key } + end + + def updated_filter + updated_params = @params.to_hash + + resolved = updated_params.delete('resolved') + status_filter = if resolved.present? + resolved == '1' ? 'resolved' : 'unresolved' + else + 'all' + end + + # 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') + + 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? @@ -49,14 +107,12 @@ 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? - end - def initial_scope case params[:status] when 'resolved' @@ -69,35 +125,54 @@ class ReportFilter end end - def account_search_filter - if params[:search_term].includes? '@' + 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) - raise Mastodon::InvalidParameterError, "Invalid domain for search: #{domain}" if domain && !domain.includes?('.') Account.where(username: username, domain: domain) else domain = params[:search_term] - raise Mastodon::InvalidParameterError, "Invalid domain for search: #{domain}" unless domain.includes?('.') + # 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_scope - case params[:search_type].to_sym - when :target - Report.where(target_account: account_search_filter) - when :source - Report.where(account: account_search_filter) + def search_type + if SEARCH_TYPES.include? params[:search_type] + params[:search_type].to_sym else - raise Mastodon::InvalidParameterError, "Unknown search type: #{params[:search_type]}" + 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) + # NOTE: account_id and target_account_id could be rewritten to search filters: case key.to_sym when :account_id Report.where(account_id: value) @@ -111,13 +186,13 @@ class ReportFilter 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 origin value: #{value}" end end diff --git a/app/views/admin/reports/index.html.haml b/app/views/admin/reports/index.html.haml index df3c50347f0..716dd27c8bb 100644 --- a/app/views/admin/reports/index.html.haml +++ b/app/views/admin/reports/index.html.haml @@ -26,6 +26,8 @@ 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' @@ -85,4 +87,4 @@ - else = t('admin.reports.not_assigned') -= paginate @reports += paginate @reports unless @reports.empty? diff --git a/config/locales/en.yml b/config/locales/en.yml index f261f038607..9726bd2d628 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -705,7 +705,8 @@ en: reported_with_application: Reported with application resolved: Resolved resolved_msg: Report successfully resolved! - search_term: domain or @username or @username@domain + 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 From d2b185743bb6611ac4ee58592ab657edf94bac10 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Sat, 11 Oct 2025 20:48:06 +0200 Subject: [PATCH 3/8] Remove outdated filters --- app/models/report_filter.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/app/models/report_filter.rb b/app/models/report_filter.rb index 999156d056d..5648bcc7ddc 100644 --- a/app/models/report_filter.rb +++ b/app/models/report_filter.rb @@ -172,12 +172,7 @@ class ReportFilter end def scope_for(key, value) - # NOTE: account_id and target_account_id could be rewritten to search filters: case key.to_sym - 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) else From 883b62b368fa560e745a8db5c6b1f1415a10cbdd Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Sat, 11 Oct 2025 20:49:24 +0200 Subject: [PATCH 4/8] Update comment --- app/controllers/admin/reports_controller.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/admin/reports_controller.rb b/app/controllers/admin/reports_controller.rb index bda8c1613f3..96ec433930e 100644 --- a/app/controllers/admin/reports_controller.rb +++ b/app/controllers/admin/reports_controller.rb @@ -8,9 +8,9 @@ module Admin def index authorize :report, :index? - # We previously only supported searching by target account domain for - # reports, we now have more search options, but it's important that we - # don't break any saved queries people may have: + # 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]) From 6aad732fc5b94ab49d4f9b634f9f343b62386cd7 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Sat, 11 Oct 2025 20:49:49 +0200 Subject: [PATCH 5/8] Remove debugs --- app/controllers/admin/reports_controller.rb | 1 - app/models/report_filter.rb | 1 - 2 files changed, 2 deletions(-) diff --git a/app/controllers/admin/reports_controller.rb b/app/controllers/admin/reports_controller.rb index 96ec433930e..95a304768ce 100644 --- a/app/controllers/admin/reports_controller.rb +++ b/app/controllers/admin/reports_controller.rb @@ -1,6 +1,5 @@ # frozen_string_literal: true -require 'debug' module Admin class ReportsController < BaseController before_action :set_report, except: [:index] diff --git a/app/models/report_filter.rb b/app/models/report_filter.rb index 5648bcc7ddc..b0cce56770c 100644 --- a/app/models/report_filter.rb +++ b/app/models/report_filter.rb @@ -1,6 +1,5 @@ # frozen_string_literal: true -require 'debug' class ReportFilter KEYS = %i( status From 891faa5d7416b466f669ca3de7c799e327fe912c Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Tue, 14 Oct 2025 22:09:16 +0200 Subject: [PATCH 6/8] Preserve old API interface --- .../api/v1/admin/reports_controller.rb | 20 ++++++++++--------- app/models/report_filter.rb | 17 +++++++++++++--- spec/requests/api/v1/admin/reports_spec.rb | 7 ++++--- 3 files changed, 29 insertions(+), 15 deletions(-) diff --git a/app/controllers/api/v1/admin/reports_controller.rb b/app/controllers/api/v1/admin/reports_controller.rb index 520388d01ca..d53c36a9003 100644 --- a/app/controllers/api/v1/admin/reports_controller.rb +++ b/app/controllers/api/v1/admin/reports_controller.rb @@ -18,9 +18,6 @@ class Api::V1::Admin::ReportsController < Api::BaseController def index authorize :report, :index? - - return redirect_to api_v1_admin_reports_path(reports_filter.updated_filter) if reports_filter.outdated? - render json: @reports, each_serializer: REST::Admin::ReportSerializer end @@ -74,12 +71,8 @@ class Api::V1::Admin::ReportsController < Api::BaseController @report = Report.find(params[:id]) end - def reports_filter - @reports_filter ||= ReportFilter.new(filter_params) - end - def filtered_reports - reports_filter.results + ReportFilter.new(filter_params).results end def report_params @@ -87,7 +80,16 @@ class Api::V1::Admin::ReportsController < Api::BaseController end def filter_params - params.slice(*ReportFilter::ALL_KEYS).permit(*ReportFilter::ALL_KEYS) + 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/models/report_filter.rb b/app/models/report_filter.rb index b0cce56770c..a63acafd72a 100644 --- a/app/models/report_filter.rb +++ b/app/models/report_filter.rb @@ -6,16 +6,23 @@ class ReportFilter search_type search_term target_origin + account_id + target_account_id ).freeze - OUTDATED_KEYS = %i( + OUTDATED_ADMIN_KEYS = %i( resolved by_target_domain account_id target_account_id ).freeze - ALL_KEYS = KEYS + OUTDATED_KEYS + ALL_KEYS = KEYS + OUTDATED_ADMIN_KEYS + API_KEYS = KEYS + %i( + resolved + account_id + target_account_id + ).freeze SEARCH_TYPES = %w( source @@ -41,7 +48,7 @@ class ReportFilter # We always need a status parameter: return true if @params.exclude? :status - OUTDATED_KEYS.any? { |key, _value| @params.include? key } + OUTDATED_ADMIN_KEYS.any? { |key, _value| @params.include? key } end def updated_filter @@ -174,6 +181,10 @@ class ReportFilter case key.to_sym 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 diff --git a/spec/requests/api/v1/admin/reports_spec.rb b/spec/requests/api/v1/admin/reports_spec.rb index 90a60d16c50..c1be7ec3ae5 100644 --- a/spec/requests/api/v1/admin/reports_spec.rb +++ b/spec/requests/api/v1/admin/reports_spec.rb @@ -73,11 +73,12 @@ RSpec.describe 'Reports' do context 'with outdated resolved param' do let(:params) { { resolved: true } } + let(:scope) { Report.resolved } - it 'redirects to the new filter path' do + it 'returns the resolved reports' do subject - expect(response).to redirect_to api_v1_admin_reports_url({ status: 'resolved' }) + expect(response.parsed_body).to match_array(expected_response) end end @@ -99,7 +100,7 @@ RSpec.describe 'Reports' do it 'returns only the resolved reports' do subject - expect(body_as_json).to match_array(expected_response) + expect(response.parsed_body).to match_array(expected_response) end end From b348ca25c43e115fbe6bc727dc1eb1d1849c5d1f Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Tue, 14 Oct 2025 23:21:27 +0200 Subject: [PATCH 7/8] Update specs --- app/models/report_filter.rb | 19 ++-- .../admin/reports_controller_spec.rb | 104 +++++++++++++----- 2 files changed, 90 insertions(+), 33 deletions(-) diff --git a/app/models/report_filter.rb b/app/models/report_filter.rb index a63acafd72a..77848a5ce63 100644 --- a/app/models/report_filter.rb +++ b/app/models/report_filter.rb @@ -54,17 +54,22 @@ class ReportFilter def updated_filter updated_params = @params.to_hash - resolved = updated_params.delete('resolved') - status_filter = if resolved.present? - resolved == '1' ? 'resolved' : 'unresolved' - else - 'all' - end - # 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 diff --git a/spec/controllers/admin/reports_controller_spec.rb b/spec/controllers/admin/reports_controller_spec.rb index e20e1602c10..eb32c21f599 100644 --- a/spec/controllers/admin/reports_controller_spec.rb +++ b/spec/controllers/admin/reports_controller_spec.rb @@ -1,8 +1,9 @@ # frozen_string_literal: true require 'rails_helper' +require 'debug' -describe Admin::ReportsController do +RSpec.describe Admin::ReportsController do render_views let(:user) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')) } @@ -27,10 +28,11 @@ describe Admin::ReportsController do get :index, params: { status: 'unresolved' } - reports = assigns(:reports).to_a - expect(reports.size).to eq 1 - expect(reports[0]).to eq specified 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 @@ -39,43 +41,61 @@ describe Admin::ReportsController do get :index, params: { status: 'resolved' } - reports = assigns(:reports).to_a - expect(reports.size).to eq 1 - expect(reports[0]).to eq specified - 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 - resolved = Fabricate(:report, action_taken_at: Time.now.utc) - unresolved = Fabricate(:report, action_taken_at: nil) + 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' } - reports = assigns(:reports).to_a - expect(reports.size).to eq 2 - - # unresolved is the most recently created report, so it's first: - expect(reports[0]).to eq unresolved - expect(reports[1]).to eq resolved - 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 http success with target_account_id filter' do + 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: { target_account_id: targeted_account.id } - - reports = assigns(:reports).to_a - expect(reports.size).to eq 1 - expect(reports[0]).to eq targeted + 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 @@ -85,14 +105,14 @@ describe Admin::ReportsController 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: 'unresolved' }) + .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({ search_type: 'target', search_term: nil, status: 'resolved' }) + .to redirect_to admin_reports_path({ status: 'resolved' }) end it 'redirects if resolved and by_target_domain are used' do @@ -104,6 +124,38 @@ describe Admin::ReportsController do 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 @@ -113,8 +165,8 @@ describe Admin::ReportsController do get :show, params: { id: report } - expect(assigns(:report)).to eq report expect(response).to have_http_status(200) + expect(response.body).to include(report.target_account.acct) end end From aecca89b888fb69949cdbb585a2f6ad5a126fb88 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Tue, 14 Oct 2025 23:47:47 +0200 Subject: [PATCH 8/8] Update specs --- spec/models/report_filter_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/models/report_filter_spec.rb b/spec/models/report_filter_spec.rb index 39b7d3dfa78..86894fe7192 100644 --- a/spec/models/report_filter_spec.rb +++ b/spec/models/report_filter_spec.rb @@ -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)