diff --git a/app/assets/javascripts/admin/abuse_reports/components/abuse_report_row.vue b/app/assets/javascripts/admin/abuse_reports/components/abuse_report_row.vue index b229dd9e993828cc2c524f6db940dd1d7e12e3e3..291833959f24698c1ee2adafec8c69813bbc8feb 100644 --- a/app/assets/javascripts/admin/abuse_reports/components/abuse_report_row.vue +++ b/app/assets/javascripts/admin/abuse_reports/components/abuse_report_row.vue @@ -14,6 +14,13 @@ export default { ListItem, AbuseCategory, }, + i18n: { + updatedAt: __('Updated %{timeAgo}'), + createdAt: __('Created %{timeAgo}'), + deletedUser: s__('AbuseReports|Deleted user'), + row: s__('AbuseReports|%{reportedUser} reported for %{category} by %{reporter}'), + rowWithCount: s__('AbuseReports|%{reportedUser} reported for %{category} by %{count} users'), + }, props: { report: { type: Object, @@ -25,18 +32,24 @@ export default { const { sort } = queryToObject(window.location.search); const { createdAt, updatedAt } = this.report; const { template, timeAgo } = Object.values(SORT_UPDATED_AT.sortDirection).includes(sort) - ? { template: __('Updated %{timeAgo}'), timeAgo: updatedAt } - : { template: __('Created %{timeAgo}'), timeAgo: createdAt }; + ? { template: this.$options.i18n.updatedAt, timeAgo: updatedAt } + : { template: this.$options.i18n.createdAt, timeAgo: createdAt }; return sprintf(template, { timeAgo: getTimeago().format(timeAgo) }); }, title() { - const { reportedUser, category, reporter } = this.report; - const template = s__('AbuseReports|%{reportedUser} reported for %{category} by %{reporter}'); - return sprintf(template, { - reportedUser: reportedUser?.name || s__('AbuseReports|Deleted user'), - reporter: reporter?.name || s__('AbuseReports|Deleted user'), + const { reportedUser, category, reporter, count } = this.report; + + const reportedUserName = reportedUser?.name || this.$options.i18n.deletedUser; + const reporterName = reporter?.name || this.$options.i18n.deletedUser; + + const i18nRowCount = count > 1 ? this.$options.i18n.rowWithCount : this.$options.i18n.row; + + return sprintf(i18nRowCount, { + reportedUser: reportedUserName, + reporter: reporterName, category, + count, }); }, }, diff --git a/app/assets/javascripts/admin/abuse_reports/components/abuse_reports_filtered_search_bar.vue b/app/assets/javascripts/admin/abuse_reports/components/abuse_reports_filtered_search_bar.vue index b1eb5371a35919ebb1f99b97531617fc28312e41..bab0fe6dd7dc42215ac3681219be08879069b3a7 100644 --- a/app/assets/javascripts/admin/abuse_reports/components/abuse_reports_filtered_search_bar.vue +++ b/app/assets/javascripts/admin/abuse_reports/components/abuse_reports_filtered_search_bar.vue @@ -4,43 +4,52 @@ import { FILTERED_SEARCH_TERM } from '~/vue_shared/components/filtered_search_ba import FilteredSearchBar from '~/vue_shared/components/filtered_search_bar/filtered_search_bar_root.vue'; import { FILTERED_SEARCH_TOKENS, - DEFAULT_SORT, - SORT_OPTIONS, - isValidSortKey, + DEFAULT_SORT_STATUS_OPEN, + DEFAULT_SORT_STATUS_CLOSED, } from '~/admin/abuse_reports/constants'; -import { buildFilteredSearchCategoryToken, isValidStatus } from '~/admin/abuse_reports/utils'; + +import { + buildFilteredSearchCategoryToken, + isValidStatus, + isOpenStatus, + isValidSortKey, + sortOptions, +} from '~/admin/abuse_reports/utils'; export default { name: 'AbuseReportsFilteredSearchBar', components: { FilteredSearchBar }, - sortOptions: SORT_OPTIONS, inject: ['categories'], data() { return { initialFilterValue: [], - initialSortBy: DEFAULT_SORT, + initialSortBy: DEFAULT_SORT_STATUS_OPEN, }; }, computed: { tokens() { return [...FILTERED_SEARCH_TOKENS, buildFilteredSearchCategoryToken(this.categories)]; }, + query() { + return queryToObject(window.location.search); + }, + currentSortOptions() { + return sortOptions(this.query.status); + }, }, created() { - const query = queryToObject(window.location.search); + const { query } = this; // Backend shows open reports by default if status param is not specified. // To match that behavior, update the current URL to include status=open - // query when no status query is specified on load. + // query when no status is specified on load. if (!isValidStatus(query.status)) { query.status = 'open'; updateHistory({ url: setUrlParams(query), replace: true }); } - const sort = this.currentSortKey(); - if (sort) { - this.initialSortBy = query.sort; - } + const sortKey = this.currentSortKey(); + this.initialSortBy = sortKey; const tokens = this.tokens .filter((token) => query[token.type]) @@ -56,9 +65,13 @@ export default { }, methods: { currentSortKey() { - const { sort } = queryToObject(window.location.search); + const { status, sort } = this.query; - return isValidSortKey(sort) ? sort : undefined; + if (!isValidSortKey(status, sort) || !sort) { + return isOpenStatus(status) ? DEFAULT_SORT_STATUS_OPEN : DEFAULT_SORT_STATUS_CLOSED; + } + + return sort; }, handleFilter(tokens) { let params = tokens.reduce((accumulator, token) => { @@ -76,6 +89,7 @@ export default { }, {}); const sort = this.currentSortKey(); + if (sort) { params = { ...params, sort }; } @@ -83,7 +97,7 @@ export default { redirectTo(setUrlParams(params, window.location.href, true)); // eslint-disable-line import/no-deprecated }, handleSort(sort) { - const { page, ...query } = queryToObject(window.location.search); + const { page, ...query } = this.query; redirectTo(setUrlParams({ ...query, sort }, window.location.href, true)); // eslint-disable-line import/no-deprecated }, @@ -101,7 +115,7 @@ export default { :search-input-placeholder="__('Filter reports')" :initial-filter-value="initialFilterValue" :initial-sort-by="initialSortBy" - :sort-options="$options.sortOptions" + :sort-options="currentSortOptions" data-testid="abuse-reports-filtered-search-bar" @onFilter="handleFilter" @onSort="handleSort" diff --git a/app/assets/javascripts/admin/abuse_reports/constants.js b/app/assets/javascripts/admin/abuse_reports/constants.js index acb79293dfb274aa55ef9ad8057e41666c9a9b77..8b14745543e81105f5a31d9c8816a8f7ccaedbe0 100644 --- a/app/assets/javascripts/admin/abuse_reports/constants.js +++ b/app/assets/javascripts/admin/abuse_reports/constants.js @@ -7,10 +7,9 @@ import { } from '~/vue_shared/components/filtered_search_bar/constants'; import { s__, __ } from '~/locale'; -const STATUS_OPTIONS = [ - { value: 'closed', title: __('Closed') }, - { value: 'open', title: __('Open') }, -]; +export const STATUS_OPEN = { value: 'open', title: __('Open') }; + +const STATUS_OPTIONS = [{ value: 'closed', title: __('Closed') }, STATUS_OPEN]; export const FILTERED_SEARCH_TOKEN_USER = { type: 'user', @@ -39,30 +38,39 @@ export const FILTERED_SEARCH_TOKEN_STATUS = { operators: OPERATORS_IS, }; -export const DEFAULT_SORT = 'created_at_desc'; -export const SORT_UPDATED_AT = Object.freeze({ +export const DEFAULT_SORT_STATUS_OPEN = 'number_of_reports_desc'; +export const DEFAULT_SORT_STATUS_CLOSED = 'created_at_desc'; + +export const SORT_UPDATED_AT = { id: 20, title: __('Updated date'), sortDirection: { descending: 'updated_at_desc', ascending: 'updated_at_asc', }, -}); -const SORT_CREATED_AT = Object.freeze({ +}; + +const SORT_CREATED_AT = { id: 10, title: __('Created date'), sortDirection: { - descending: DEFAULT_SORT, + descending: DEFAULT_SORT_STATUS_CLOSED, ascending: 'created_at_asc', }, -}); +}; + +const SORT_NUMBER_OF_REPORTS = { + id: 30, + title: __('Number of Reports'), + sortDirection: { + descending: DEFAULT_SORT_STATUS_OPEN, + }, +}; -export const SORT_OPTIONS = [SORT_CREATED_AT, SORT_UPDATED_AT]; +export const SORT_OPTIONS_STATUS_CLOSED = [SORT_CREATED_AT, SORT_UPDATED_AT]; -export const isValidSortKey = (key) => - SORT_OPTIONS.some( - (sort) => sort.sortDirection.ascending === key || sort.sortDirection.descending === key, - ); +// when filtered for status=open reports, add an additional sorting option -> number of reports +export const SORT_OPTIONS_STATUS_OPEN = [SORT_NUMBER_OF_REPORTS, ...SORT_OPTIONS_STATUS_CLOSED]; export const FILTERED_SEARCH_TOKEN_CATEGORY = { type: 'category', @@ -74,9 +82,9 @@ export const FILTERED_SEARCH_TOKEN_CATEGORY = { }; export const FILTERED_SEARCH_TOKENS = [ + FILTERED_SEARCH_TOKEN_STATUS, FILTERED_SEARCH_TOKEN_USER, FILTERED_SEARCH_TOKEN_REPORTER, - FILTERED_SEARCH_TOKEN_STATUS, ]; export const ABUSE_CATEGORIES = { diff --git a/app/assets/javascripts/admin/abuse_reports/index.js b/app/assets/javascripts/admin/abuse_reports/index.js index dbc466af2d2bb7ba072e83ff095ec99655abbc5e..e4174e6c8519a5802b9931ae3aa2971cb71177e0 100644 --- a/app/assets/javascripts/admin/abuse_reports/index.js +++ b/app/assets/javascripts/admin/abuse_reports/index.js @@ -19,6 +19,7 @@ export const initAbuseReportsApp = () => { return new Vue({ el, + name: 'AbuseReportsAppRoot', provide: { categories }, render: (createElement) => createElement(AbuseReportsApp, { diff --git a/app/assets/javascripts/admin/abuse_reports/utils.js b/app/assets/javascripts/admin/abuse_reports/utils.js index d30e8fb0ae59aa646955f36d9ca110b0c8978abb..a3d05e4dcb3cbcc98cd2edfa05abc6c6eabd99be 100644 --- a/app/assets/javascripts/admin/abuse_reports/utils.js +++ b/app/assets/javascripts/admin/abuse_reports/utils.js @@ -1,4 +1,10 @@ -import { FILTERED_SEARCH_TOKEN_CATEGORY, FILTERED_SEARCH_TOKEN_STATUS } from './constants'; +import { + FILTERED_SEARCH_TOKEN_CATEGORY, + FILTERED_SEARCH_TOKEN_STATUS, + STATUS_OPEN, + SORT_OPTIONS_STATUS_OPEN, + SORT_OPTIONS_STATUS_CLOSED, +} from './constants'; export const buildFilteredSearchCategoryToken = (categories) => { const options = categories.map((c) => ({ value: c, title: c })); @@ -7,3 +13,13 @@ export const buildFilteredSearchCategoryToken = (categories) => { export const isValidStatus = (status) => FILTERED_SEARCH_TOKEN_STATUS.options.map((o) => o.value).includes(status); + +export const isOpenStatus = (status) => status === STATUS_OPEN.value; + +export const sortOptions = (status) => + isOpenStatus(status) ? SORT_OPTIONS_STATUS_OPEN : SORT_OPTIONS_STATUS_CLOSED; + +export const isValidSortKey = (status, key) => + sortOptions(status).some( + (sort) => sort.sortDirection.ascending === key || sort.sortDirection.descending === key, + ); diff --git a/app/finders/abuse_reports_finder.rb b/app/finders/abuse_reports_finder.rb index 6a6d04131944c2eb18a214767ed55c70e3469105..43cebd16d92cba8869d62437b92d12e43ff93b7c 100644 --- a/app/finders/abuse_reports_finder.rb +++ b/app/finders/abuse_reports_finder.rb @@ -3,9 +3,13 @@ class AbuseReportsFinder attr_reader :params, :reports - DEFAULT_STATUS_FILTER = 'open' - DEFAULT_SORT = 'created_at_desc' - ALLOWED_SORT = [DEFAULT_SORT, *%w[created_at_asc updated_at_desc updated_at_asc]].freeze + STATUS_OPEN = 'open' + + DEFAULT_SORT_STATUS_CLOSED = 'created_at_desc' + ALLOWED_SORT = [DEFAULT_SORT_STATUS_CLOSED, *%w[created_at_asc updated_at_desc updated_at_asc]].freeze + + DEFAULT_SORT_STATUS_OPEN = 'number_of_reports_desc' + SORT_BY_COUNT = [DEFAULT_SORT_STATUS_OPEN].freeze def initialize(params = {}) @params = params @@ -14,6 +18,7 @@ def initialize(params = {}) def execute filter_reports + aggregate_reports sort_reports reports.with_users.page(params[:page]) @@ -22,20 +27,28 @@ def execute private def filter_reports - filter_by_user_id + if Feature.disabled?(:abuse_reports_list) + filter_by_user_id + return + end + filter_by_status filter_by_user filter_by_reporter - filter_by_status filter_by_category end + def filter_by_user_id + return unless params[:user_id].present? + + @reports = @reports.by_user_id(params[:user_id]) + end + def filter_by_status - return unless Feature.enabled?(:abuse_reports_list) return unless params[:status].present? status = params[:status] - status = DEFAULT_STATUS_FILTER unless status.in?(AbuseReport.statuses.keys) + status = STATUS_OPEN unless status.in?(AbuseReport.statuses.keys) case status when 'open' @@ -69,10 +82,13 @@ def filter_by_reporter @reports = @reports.by_reporter_id(user_id) end - def filter_by_user_id - return unless params[:user_id].present? + def sort_key + sort_key = params[:sort] - @reports = @reports.by_user_id(params[:user_id]) + return sort_key if sort_key.in?(ALLOWED_SORT + SORT_BY_COUNT) + return DEFAULT_SORT_STATUS_OPEN if status_open? + + DEFAULT_SORT_STATUS_CLOSED end def sort_reports @@ -81,13 +97,31 @@ def sort_reports return end - sort_by = params[:sort] - sort_by = DEFAULT_SORT unless sort_by.in?(ALLOWED_SORT) + # let sub_query in aggregate_reports do the sorting if sorting by number of reports + return if sort_key.in?(SORT_BY_COUNT) - @reports = @reports.order_by(sort_by) + @reports = @reports.order_by(sort_key) end def find_user_id(username) User.by_username(username).pick(:id) end + + def status_open? + return unless Feature.enabled?(:abuse_reports_list) && params[:status].present? + + status = params[:status] + status = STATUS_OPEN unless status.in?(AbuseReport.statuses.keys) + + status == STATUS_OPEN + end + + def aggregate_reports + if status_open? + sort_by_count = sort_key.in?(SORT_BY_COUNT) + @reports = @reports.aggregated_by_user_and_category(sort_by_count) + end + + @reports + end end diff --git a/app/models/abuse_report.rb b/app/models/abuse_report.rb index 1d2eee828271454fcac4f163bebc44e8131874df..210fd85ec7b7bd37e52a4141ca594ad231603b30 100644 --- a/app/models/abuse_report.rb +++ b/app/models/abuse_report.rb @@ -214,6 +214,24 @@ def validate_screenshot_is_image extension_list: valid_image_extensions.to_sentence(last_word_connector: ' or ')) ) end + + def self.aggregated_by_user_and_category(sort_by_count = false) + sub_query = self + .select('user_id, category, COUNT(id) as count', 'MIN(id) as min') + .group(:user_id, :category) + + reports = AbuseReport.with_users + .open + .select('aggregated.*, status, id, reporter_id, created_at, updated_at') + .from(sub_query, :aggregated) + .joins('INNER JOIN abuse_reports on aggregated.min = abuse_reports.id') + + if sort_by_count + reports.order(count: :desc, created_at: :desc) + else + reports + end + end end AbuseReport.prepend_mod diff --git a/app/serializers/admin/abuse_report_entity.rb b/app/serializers/admin/abuse_report_entity.rb index 58637445e819c5a7840f127b747b0a4154fae5c7..22395a2fe91d3cfba942ef27e74fbd2d4bcb53f6 100644 --- a/app/serializers/admin/abuse_report_entity.rb +++ b/app/serializers/admin/abuse_report_entity.rb @@ -7,6 +7,7 @@ class AbuseReportEntity < Grape::Entity expose :category expose :created_at expose :updated_at + expose :count expose :reported_user do |report| UserEntity.represent(report.user, only: [:name]) @@ -19,5 +20,11 @@ class AbuseReportEntity < Grape::Entity expose :report_path do |report| admin_abuse_report_path(report) end + + private + + def count + object.has_attribute?(:count) ? object.count : 1 + end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 3bfce5e0273be5d17da7af3c0e13d05796e8214a..2fb3622097c706a615108df42a040530e3dd2503 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -2207,6 +2207,9 @@ msgstr "" msgid "AbuseReportEvent|Successfully scheduled the user for deletion and closed the report" msgstr "" +msgid "AbuseReports|%{reportedUser} reported for %{category} by %{count} users" +msgstr "" + msgid "AbuseReports|%{reportedUser} reported for %{category} by %{reporter}" msgstr "" @@ -31647,6 +31650,9 @@ msgstr "" msgid "Number of LOCs per commit" msgstr "" +msgid "Number of Reports" +msgstr "" + msgid "Number of commits" msgstr "" diff --git a/spec/features/admin/admin_abuse_reports_spec.rb b/spec/features/admin/admin_abuse_reports_spec.rb index 9739ea53f81bdc265ecc2cae5f4cca3033169126..18bc851558db27aef69153a8915f505524d96df8 100644 --- a/spec/features/admin/admin_abuse_reports_spec.rb +++ b/spec/features/admin/admin_abuse_reports_spec.rb @@ -2,27 +2,29 @@ require 'spec_helper' -RSpec.describe "Admin::AbuseReports", :js, feature_category: :shared do +RSpec.describe "Admin::AbuseReports", :js, feature_category: :insider_threat do let_it_be(:user) { create(:user) } let_it_be(:admin) { create(:admin) } - context 'as an admin' do - describe 'displayed reports' do - include FilteredSearchHelpers + let_it_be(:open_report) { create(:abuse_report, created_at: 5.days.ago, updated_at: 2.days.ago, category: 'spam', user: user) } + let_it_be(:open_report2) { create(:abuse_report, created_at: 4.days.ago, updated_at: 3.days.ago, category: 'phishing') } + let_it_be(:closed_report) { create(:abuse_report, :closed, user: user, category: 'spam') } - let_it_be(:open_report) { create(:abuse_report, created_at: 5.days.ago, updated_at: 2.days.ago) } - let_it_be(:open_report2) { create(:abuse_report, created_at: 4.days.ago, updated_at: 3.days.ago, category: 'phishing') } - let_it_be(:closed_report) { create(:abuse_report, :closed) } + describe 'as an admin' do + before do + sign_in(admin) + gitlab_enable_admin_mode_sign_in(admin) + end - let(:abuse_report_row_selector) { '[data-testid="abuse-report-row"]' } + context 'when abuse_reports_list feature flag is enabled' do + include FilteredSearchHelpers before do - sign_in(admin) - gitlab_enable_admin_mode_sign_in(admin) - visit admin_abuse_reports_path end + let(:abuse_report_row_selector) { '[data-testid="abuse-report-row"]' } + it 'only includes open reports by default' do expect_displayed_reports_count(2) @@ -68,7 +70,8 @@ end it 'can be sorted by created_at and updated_at in desc and asc order', :aggregate_failures do - # created_at desc (default) + sort_by 'Created date' + # created_at desc expect(report_rows[0].text).to include(report_text(open_report2)) expect(report_rows[1].text).to include(report_text(open_report)) @@ -78,25 +81,90 @@ expect(report_rows[0].text).to include(report_text(open_report)) expect(report_rows[1].text).to include(report_text(open_report2)) - # updated_at ascending + # updated_at asc sort_by 'Updated date' expect(report_rows[0].text).to include(report_text(open_report2)) expect(report_rows[1].text).to include(report_text(open_report)) - # updated_at descending + # updated_at desc toggle_sort_direction expect(report_rows[0].text).to include(report_text(open_report)) expect(report_rows[1].text).to include(report_text(open_report2)) end + context 'when multiple reports for the same user are created' do + let_it_be(:open_report3) { create(:abuse_report, category: 'spam', user: user) } + let_it_be(:closed_report2) { create(:abuse_report, :closed, user: user, category: 'spam') } + + it 'aggregates open reports by user & category', :aggregate_failures do + expect_displayed_reports_count(2) + + expect_aggregated_report_shown(open_report, 2) + expect_report_shown(open_report2) + end + + it 'can sort aggregated reports by number_of_reports in desc order only', :aggregate_failures do + sort_by 'Number of Reports' + + expect(report_rows[0].text).to include(aggregated_report_text(open_report, 2)) + expect(report_rows[1].text).to include(report_text(open_report2)) + + toggle_sort_direction + + expect(report_rows[0].text).to include(aggregated_report_text(open_report, 2)) + expect(report_rows[1].text).to include(report_text(open_report2)) + end + + it 'can sort aggregated reports by created_at and updated_at in desc and asc order', :aggregate_failures do + # number_of_reports desc (default) + expect(report_rows[0].text).to include(aggregated_report_text(open_report, 2)) + expect(report_rows[1].text).to include(report_text(open_report2)) + + # created_at desc + sort_by 'Created date' + + expect(report_rows[0].text).to include(report_text(open_report2)) + expect(report_rows[1].text).to include(aggregated_report_text(open_report, 2)) + + # created_at asc + toggle_sort_direction + + expect(report_rows[0].text).to include(aggregated_report_text(open_report, 2)) + expect(report_rows[1].text).to include(report_text(open_report2)) + + sort_by 'Updated date' + + # updated_at asc + expect(report_rows[0].text).to include(report_text(open_report2)) + expect(report_rows[1].text).to include(aggregated_report_text(open_report, 2)) + + # updated_at desc + toggle_sort_direction + + expect(report_rows[0].text).to include(aggregated_report_text(open_report, 2)) + expect(report_rows[1].text).to include(report_text(open_report2)) + end + + it 'does not aggregate closed reports', :aggregate_failures do + filter %w[Status Closed] + + expect_displayed_reports_count(2) + expect_report_shown(closed_report, closed_report2) + end + end + def report_rows page.all(abuse_report_row_selector) end def report_text(report) - "#{report.user.name} reported for #{report.category}" + "#{report.user.name} reported for #{report.category} by #{report.reporter.name}" + end + + def aggregated_report_text(report, count) + "#{report.user.name} reported for #{report.category} by #{count} users" end def expect_report_shown(*reports) @@ -111,6 +179,12 @@ def expect_report_not_shown(*reports) end end + def expect_aggregated_report_shown(*reports, count) + reports.each do |r| + expect(page).to have_content(aggregated_report_text(r, count)) + end + end + def expect_displayed_reports_count(count) expect(page).to have_css(abuse_report_row_selector, count: count) end @@ -138,71 +212,30 @@ def sort_by(sort) before do stub_feature_flags(abuse_reports_list: false) - sign_in(admin) - gitlab_enable_admin_mode_sign_in(admin) + visit admin_abuse_reports_path end - describe 'if a user has been reported for abuse' do - let_it_be(:abuse_report) { create(:abuse_report, user: user) } - - describe 'in the abuse report view' do - before do - visit admin_abuse_reports_path - end - - it 'presents information about abuse report' do - expect(page).to have_content('Abuse Reports') - - expect(page).to have_content(user.name) - expect(page).to have_content(abuse_report.reporter.name) - expect(page).to have_content(abuse_report.message) - expect(page).to have_link(user.name, href: user_path(user)) - end - - it 'present actions items' do - expect(page).to have_link('Remove user & report') - expect(page).to have_link('Block user') - expect(page).to have_link('Remove user') - end - end + it 'displays all abuse reports', :aggregate_failures do + expect_report_shown(open_report) + expect_report_actions_shown(open_report) - describe 'in the profile page of the user' do - it 'shows a link to view user in the admin area' do - visit user_path(user) + expect_report_shown(open_report2) + expect_report_actions_shown(open_report2) - expect(page).to have_link 'View user in admin area', href: admin_user_path(user) - end - end + expect_report_shown(closed_report) + expect_report_actions_shown(closed_report) end - describe 'if an admin has been reported for abuse' do + context 'when an admin has been reported for abuse' do let_it_be(:admin_abuse_report) { create(:abuse_report, user: admin) } - describe 'in the abuse report view' do - before do - visit admin_abuse_reports_path - end - - it 'presents information about abuse report' do - page.within(:table_row, { "User" => admin.name }) do - expect(page).to have_content(admin.name) - expect(page).to have_content(admin_abuse_report.reporter.name) - expect(page).to have_content(admin_abuse_report.message) - expect(page).to have_link(admin.name, href: user_path(admin)) - end - end - - it 'does not present actions items' do - page.within(:table_row, { "User" => admin.name }) do - expect(page).not_to have_link('Remove user & report') - expect(page).not_to have_link('Block user') - expect(page).not_to have_link('Remove user') - end - end + it 'displays the abuse report without actions' do + expect_report_shown(admin_abuse_report) + expect_report_actions_not_shown(admin_abuse_report) end end - describe 'if a many users have been reported for abuse' do + context 'when multiple users have been reported for abuse' do let(:report_count) { AbuseReport.default_per_page + 3 } before do @@ -211,8 +244,8 @@ def sort_by(sort) end end - describe 'in the abuse report view' do - it 'presents information about abuse report' do + context 'in the abuse report view', :aggregate_failures do + it 'adds pagination' do visit admin_abuse_reports_path expect(page).to have_selector('.pagination') @@ -221,12 +254,8 @@ def sort_by(sort) end end - describe 'filtering by user' do - let!(:user2) { create(:user) } - let!(:abuse_report) { create(:abuse_report, user: user) } - let!(:abuse_report_2) { create(:abuse_report, user: user2) } - - it 'shows only single user report' do + context 'when filtering reports' do + it 'can be filtered by reported-user', :aggregate_failures do visit admin_abuse_reports_path page.within '.filter-form' do @@ -234,14 +263,39 @@ def sort_by(sort) wait_for_requests page.within '.dropdown-menu-user' do - click_link user2.name + click_link user.name end wait_for_requests end - expect(page).to have_content(user2.name) - expect(page).not_to have_content(user.name) + expect_report_shown(open_report) + expect_report_shown(closed_report) + end + end + + def expect_report_shown(report) + page.within(:table_row, { "User" => report.user.name, "Reported by" => report.reporter.name }) do + expect(page).to have_content(report.user.name) + expect(page).to have_content(report.reporter.name) + expect(page).to have_content(report.message) + expect(page).to have_link(report.user.name, href: user_path(report.user)) + end + end + + def expect_report_actions_shown(report) + page.within(:table_row, { "User" => report.user.name, "Reported by" => report.reporter.name }) do + expect(page).to have_link('Remove user & report') + expect(page).to have_link('Block user') + expect(page).to have_link('Remove user') + end + end + + def expect_report_actions_not_shown(report) + page.within(:table_row, { "User" => report.user.name, "Reported by" => report.reporter.name }) do + expect(page).not_to have_link('Remove user & report') + expect(page).not_to have_link('Block user') + expect(page).not_to have_link('Remove user') end end end diff --git a/spec/finders/abuse_reports_finder_spec.rb b/spec/finders/abuse_reports_finder_spec.rb index ee93d042ca2a3cb994471a2bcc7e4360275c88e1..0b641d0cb08254f98fbf4ed9fd5eb90c3022c42f 100644 --- a/spec/finders/abuse_reports_finder_spec.rb +++ b/spec/finders/abuse_reports_finder_spec.rb @@ -2,142 +2,205 @@ require 'spec_helper' -RSpec.describe AbuseReportsFinder, '#execute' do - let_it_be(:user1) { create(:user) } - let_it_be(:user2) { create(:user) } - let_it_be(:reporter) { create(:user) } - let_it_be(:abuse_report_1) { create(:abuse_report, id: 20, category: 'spam', user: user1) } - let_it_be(:abuse_report_2) do - create(:abuse_report, :closed, id: 30, category: 'phishing', user: user2, reporter: reporter) - end +RSpec.describe AbuseReportsFinder, feature_category: :insider_threat do + let_it_be(:user_1) { create(:user) } + let_it_be(:user_2) { create(:user) } - let(:params) { {} } + let_it_be(:reporter_1) { create(:user) } + let_it_be(:reporter_2) { create(:user) } - subject { described_class.new(params).execute } + let_it_be(:abuse_report_1) do + create(:abuse_report, :open, category: 'spam', user: user_1, reporter: reporter_1, id: 1) + end - context 'when params is empty' do - it 'returns all abuse reports' do - expect(subject).to match_array([abuse_report_1, abuse_report_2]) - end + let_it_be(:abuse_report_2) do + create(:abuse_report, :closed, category: 'phishing', user: user_2, reporter: reporter_2, id: 2) end - context 'when params[:user_id] is present' do - let(:params) { { user_id: user2 } } + let(:params) { {} } - it 'returns abuse reports for the specified user' do - expect(subject).to match_array([abuse_report_2]) - end - end + subject(:finder) { described_class.new(params).execute } - shared_examples 'returns filtered reports' do |filter_field| - it "returns abuse reports filtered by #{filter_field}_id" do - expect(subject).to match_array(filtered_reports) + describe '#execute' do + context 'when params is empty' do + it 'returns all abuse reports' do + expect(finder).to match_array([abuse_report_1, abuse_report_2]) + end end - context "when no user has username = params[:#{filter_field}]" do - before do - allow(User).to receive_message_chain(:by_username, :pick) - .with(params[filter_field]) - .with(:id) - .and_return(nil) + shared_examples 'returns filtered reports' do |filter_field| + it "returns abuse reports filtered by #{filter_field}_id" do + expect(finder).to match_array(filtered_reports) end - it 'returns all abuse reports' do - expect(subject).to match_array([abuse_report_1, abuse_report_2]) + context "when no user has username = params[:#{filter_field}]" do + before do + allow(User).to receive_message_chain(:by_username, :pick) + .with(params[filter_field]) + .with(:id) + .and_return(nil) + end + + it 'returns all abuse reports' do + expect(finder).to match_array([abuse_report_1, abuse_report_2]) + end end end - end - context 'when params[:user] is present' do - it_behaves_like 'returns filtered reports', :user do - let(:params) { { user: user1.username } } - let(:filtered_reports) { [abuse_report_1] } + context 'when params[:user] is present' do + it_behaves_like 'returns filtered reports', :user do + let(:params) { { user: user_1.username } } + let(:filtered_reports) { [abuse_report_1] } + end end - end - context 'when params[:reporter] is present' do - it_behaves_like 'returns filtered reports', :reporter do - let(:params) { { reporter: reporter.username } } - let(:filtered_reports) { [abuse_report_2] } + context 'when params[:reporter] is present' do + it_behaves_like 'returns filtered reports', :reporter do + let(:params) { { reporter: reporter_1.username } } + let(:filtered_reports) { [abuse_report_1] } + end end - end - context 'when params[:status] is present' do - context 'when value is "open"' do + context 'when params[:status] = open' do let(:params) { { status: 'open' } } it 'returns only open abuse reports' do - expect(subject).to match_array([abuse_report_1]) + expect(finder).to match_array([abuse_report_1]) end end - context 'when value is "closed"' do + context 'when params[:status] = closed' do let(:params) { { status: 'closed' } } it 'returns only closed abuse reports' do - expect(subject).to match_array([abuse_report_2]) + expect(finder).to match_array([abuse_report_2]) end end - context 'when value is not a valid status' do + context 'when params[:status] is not a valid status' do let(:params) { { status: 'partial' } } it 'defaults to returning open abuse reports' do - expect(subject).to match_array([abuse_report_1]) + expect(finder).to match_array([abuse_report_1]) end end - context 'when abuse_reports_list feature flag is disabled' do - before do - stub_feature_flags(abuse_reports_list: false) - end + context 'when params[:category] is present' do + let(:params) { { category: 'phishing' } } - it 'does not filter by status' do - expect(subject).to match_array([abuse_report_1, abuse_report_2]) + it 'returns abuse reports with the specified category' do + expect(subject).to match_array([abuse_report_2]) end end - end - context 'when params[:category] is present' do - let(:params) { { category: 'phishing' } } + describe 'aggregating reports' do + context 'when multiple open reports exist' do + let(:params) { { status: 'open' } } - it 'returns abuse reports with the specified category' do - expect(subject).to match_array([abuse_report_2]) - end - end + # same category and user as abuse_report_1 -> will get aggregated + let_it_be(:abuse_report_3) do + create(:abuse_report, :open, category: abuse_report_1.category, user: abuse_report_1.user, id: 3) + end - describe 'sorting' do - let(:params) { { sort: 'created_at_asc' } } + # different category, but same user as abuse_report_1 -> won't get aggregated + let_it_be(:abuse_report_4) do + create(:abuse_report, :open, category: 'phishing', user: abuse_report_1.user, id: 4) + end - it 'returns reports sorted by the specified sort attribute' do - expect(subject).to eq [abuse_report_1, abuse_report_2] - end + it 'aggregates open reports by user and category' do + expect(finder).to match_array([abuse_report_1, abuse_report_4]) + end + + it 'sorts by aggregated_count in descending order and created_at in descending order' do + expect(finder).to eq([abuse_report_1, abuse_report_4]) + end + + it 'returns count with aggregated reports' do + expect(finder[0].count).to eq(2) + end + + context 'when a different sorting attribute is given' do + let(:params) { { status: 'open', sort: 'created_at_desc' } } - context 'when sort is not specified' do - let(:params) { {} } + it 'returns reports sorted by the specified sort attribute' do + expect(subject).to eq([abuse_report_4, abuse_report_1]) + end + end - it "returns reports sorted by #{described_class::DEFAULT_SORT}" do - expect(subject).to eq [abuse_report_2, abuse_report_1] + context 'when params[:sort] is invalid' do + let(:params) { { status: 'open', sort: 'invalid' } } + + it 'sorts reports by aggregated_count in descending order' do + expect(finder).to eq([abuse_report_1, abuse_report_4]) + end + end end - end - context 'when sort is not supported' do - let(:params) { { sort: 'superiority' } } + context 'when multiple closed reports exist' do + let(:params) { { status: 'closed' } } + + # same user and category as abuse_report_2 -> won't get aggregated + let_it_be(:abuse_report_5) do + create(:abuse_report, :closed, category: abuse_report_2.category, user: abuse_report_2.user, id: 5) + end + + it 'does not aggregate closed reports' do + expect(finder).to match_array([abuse_report_2, abuse_report_5]) + end + + it 'sorts reports by created_at in descending order' do + expect(finder).to eq([abuse_report_5, abuse_report_2]) + end + + context 'when a different sorting attribute is given' do + let(:params) { { status: 'closed', sort: 'created_at_asc' } } - it "returns reports sorted by #{described_class::DEFAULT_SORT}" do - expect(subject).to eq [abuse_report_2, abuse_report_1] + it 'returns reports sorted by the specified sort attribute' do + expect(subject).to eq([abuse_report_2, abuse_report_5]) + end + end + + context 'when params[:sort] is invalid' do + let(:params) { { status: 'closed', sort: 'invalid' } } + + it 'sorts reports by created_at in descending order' do + expect(finder).to eq([abuse_report_5, abuse_report_2]) + end + end end end - context 'when abuse_reports_list feature flag is disabled' do - let_it_be(:abuse_report_3) { create(:abuse_report, id: 10) } - + context 'when legacy view is enabled' do before do stub_feature_flags(abuse_reports_list: false) end - it 'returns reports sorted by id in descending order' do - expect(subject).to eq [abuse_report_2, abuse_report_1, abuse_report_3] + context 'when params is empty' do + it 'returns all abuse reports' do + expect(subject).to match_array([abuse_report_1, abuse_report_2]) + end + end + + context 'when params[:user_id] is present' do + let(:params) { { user_id: user_1 } } + + it 'returns abuse reports for the specified user' do + expect(subject).to match_array([abuse_report_1]) + end + end + + context 'when sorting' do + it 'returns reports sorted by id in descending order' do + expect(subject).to match_array([abuse_report_2, abuse_report_1]) + end + end + + context 'when any of the new filters are present such as params[:status]' do + let(:params) { { status: 'open' } } + + it 'returns all abuse reports' do + expect(subject).to match_array([abuse_report_1, abuse_report_2]) + end end end end diff --git a/spec/frontend/admin/abuse_reports/components/abuse_report_row_spec.js b/spec/frontend/admin/abuse_reports/components/abuse_report_row_spec.js index 03bf510f3adf35e33a43133397584a444ef2c968..8482faccca0016a23661bddb4b50de640d820534 100644 --- a/spec/frontend/admin/abuse_reports/components/abuse_report_row_spec.js +++ b/spec/frontend/admin/abuse_reports/components/abuse_report_row_spec.js @@ -94,4 +94,19 @@ describe('AbuseReportRow', () => { it('renders abuse category', () => { expect(findAbuseCategory().exists()).toBe(true); }); + + describe('aggregated report', () => { + const mockAggregatedAbuseReport = mockAbuseReports[1]; + const { reportedUser, category, count } = mockAggregatedAbuseReport; + + beforeEach(() => { + createComponent({ report: mockAggregatedAbuseReport }); + }); + + it('displays title with number of aggregated reports', () => { + expect(findAbuseReportTitle().text()).toMatchInterpolatedText( + `${reportedUser.name} reported for ${category} by ${count} users`, + ); + }); + }); }); diff --git a/spec/frontend/admin/abuse_reports/components/abuse_reports_filtered_search_bar_spec.js b/spec/frontend/admin/abuse_reports/components/abuse_reports_filtered_search_bar_spec.js index 1f3f2caa995ad4d98553641c73cfb97b7f30a9e0..dda9263d0942295aa985c23c70fe0de53c0aeb0a 100644 --- a/spec/frontend/admin/abuse_reports/components/abuse_reports_filtered_search_bar_spec.js +++ b/spec/frontend/admin/abuse_reports/components/abuse_reports_filtered_search_bar_spec.js @@ -8,8 +8,10 @@ import { FILTERED_SEARCH_TOKEN_REPORTER, FILTERED_SEARCH_TOKEN_STATUS, FILTERED_SEARCH_TOKEN_CATEGORY, - DEFAULT_SORT, - SORT_OPTIONS, + DEFAULT_SORT_STATUS_OPEN, + DEFAULT_SORT_STATUS_CLOSED, + SORT_OPTIONS_STATUS_OPEN, + SORT_OPTIONS_STATUS_CLOSED, } from '~/admin/abuse_reports/constants'; import FilteredSearchBar from '~/vue_shared/components/filtered_search_bar/filtered_search_bar_root.vue'; import { FILTERED_SEARCH_TERM } from '~/vue_shared/components/filtered_search_bar/constants'; @@ -53,8 +55,8 @@ describe('AbuseReportsFilteredSearchBar', () => { recentSearchesStorageKey: 'abuse_reports', searchInputPlaceholder: 'Filter reports', tokens: [...FILTERED_SEARCH_TOKENS, categoryToken], - initialSortBy: DEFAULT_SORT, - sortOptions: SORT_OPTIONS, + initialSortBy: DEFAULT_SORT_STATUS_OPEN, + sortOptions: SORT_OPTIONS_STATUS_OPEN, }); }); @@ -87,6 +89,10 @@ describe('AbuseReportsFilteredSearchBar', () => { createComponent(); expect(findFilteredSearchBar().props('initialFilterValue')).toMatchObject([ + { + type: FILTERED_SEARCH_TOKEN_STATUS.type, + value: { data: 'closed', operator: '=' }, + }, { type: FILTERED_SEARCH_TOKEN_USER.type, value: { data: 'mr_abuser', operator: '=' }, @@ -95,16 +101,12 @@ describe('AbuseReportsFilteredSearchBar', () => { type: FILTERED_SEARCH_TOKEN_REPORTER.type, value: { data: 'ms_nitch', operator: '=' }, }, - { - type: FILTERED_SEARCH_TOKEN_STATUS.type, - value: { data: 'closed', operator: '=' }, - }, ]); }); describe('initial sort', () => { it.each( - SORT_OPTIONS.flatMap(({ sortDirection: { descending, ascending } }) => [ + SORT_OPTIONS_STATUS_OPEN.flatMap(({ sortDirection: { descending, ascending } }) => [ descending, ascending, ]), @@ -115,16 +117,20 @@ describe('AbuseReportsFilteredSearchBar', () => { createComponent(); - expect(findFilteredSearchBar().props('initialSortBy')).toEqual(sortBy); + if (sortBy) { + expect(findFilteredSearchBar().props('initialSortBy')).toEqual(sortBy); + } else { + expect(findFilteredSearchBar().props('initialSortBy')).toEqual(DEFAULT_SORT_STATUS_OPEN); + } }, ); - it(`uses ${DEFAULT_SORT} as initialSortBy when sort query param is invalid`, () => { + it(`uses ${DEFAULT_SORT_STATUS_OPEN} as initialSortBy when sort query param is invalid`, () => { setWindowLocation(`?sort=unknown`); createComponent(); - expect(findFilteredSearchBar().props('initialSortBy')).toEqual(DEFAULT_SORT); + expect(findFilteredSearchBar().props('initialSortBy')).toEqual(DEFAULT_SORT_STATUS_OPEN); }); }); @@ -161,26 +167,39 @@ describe('AbuseReportsFilteredSearchBar', () => { (filterToken) => { createComponentAndFilter([filterToken]); const { type, value } = filterToken; - expect(redirectTo).toHaveBeenCalledWith(`https://localhost/?${type}=${value.data}`); // eslint-disable-line import/no-deprecated + + // eslint-disable-next-line import/no-deprecated + expect(redirectTo).toHaveBeenCalledWith( + `https://localhost/?${type}=${value.data}&sort=${DEFAULT_SORT_STATUS_OPEN}`, + ); }, ); it('ignores search query param', () => { const searchFilterToken = { type: FILTERED_SEARCH_TERM, value: { data: 'ignored' } }; createComponentAndFilter([USER_FILTER_TOKEN, searchFilterToken]); - expect(redirectTo).toHaveBeenCalledWith('https://localhost/?user=mr_abuser'); // eslint-disable-line import/no-deprecated + + // eslint-disable-next-line import/no-deprecated + expect(redirectTo).toHaveBeenCalledWith( + `https://localhost/?user=mr_abuser&sort=${DEFAULT_SORT_STATUS_OPEN}`, + ); }); it('redirects without page query param', () => { createComponentAndFilter([USER_FILTER_TOKEN], '?page=2'); - expect(redirectTo).toHaveBeenCalledWith('https://localhost/?user=mr_abuser'); // eslint-disable-line import/no-deprecated + + // eslint-disable-next-line import/no-deprecated + expect(redirectTo).toHaveBeenCalledWith( + `https://localhost/?user=mr_abuser&sort=${DEFAULT_SORT_STATUS_OPEN}`, + ); }); it('redirects with existing sort query param', () => { - createComponentAndFilter([USER_FILTER_TOKEN], `?sort=${DEFAULT_SORT}`); + createComponentAndFilter([USER_FILTER_TOKEN], `?sort=${DEFAULT_SORT_STATUS_OPEN}`); + // eslint-disable-next-line import/no-deprecated expect(redirectTo).toHaveBeenCalledWith( - `https://localhost/?user=mr_abuser&sort=${DEFAULT_SORT}`, + `https://localhost/?user=mr_abuser&sort=${DEFAULT_SORT_STATUS_OPEN}`, ); }); }); @@ -222,4 +241,42 @@ describe('AbuseReportsFilteredSearchBar', () => { ); }); }); + + describe('sortOptions', () => { + describe('when status is closed', () => { + beforeEach(() => { + setWindowLocation('?status=closed'); + + createComponent(); + }); + + it('only shows created_at & updated_at as sorting options', () => { + expect(findFilteredSearchBar().props('sortOptions')).toMatchObject( + SORT_OPTIONS_STATUS_CLOSED, + ); + }); + + it('initially sorts by created_at_desc', () => { + expect(findFilteredSearchBar().props('initialSortBy')).toEqual(DEFAULT_SORT_STATUS_CLOSED); + }); + }); + + describe('when status is open', () => { + beforeEach(() => { + setWindowLocation('?status=open'); + + createComponent(); + }); + + it('shows number of reports as an additional sorting option', () => { + expect(findFilteredSearchBar().props('sortOptions')).toMatchObject( + SORT_OPTIONS_STATUS_OPEN, + ); + }); + + it('initially sorts by number_of_reports_desc', () => { + expect(findFilteredSearchBar().props('initialSortBy')).toEqual(DEFAULT_SORT_STATUS_OPEN); + }); + }); + }); }); diff --git a/spec/frontend/admin/abuse_reports/mock_data.js b/spec/frontend/admin/abuse_reports/mock_data.js index 1ea6ea7d1310ee1a808cc8a2a1d9dabbedf9d147..33a28a21cca47c02ff4173de858e834507eb702a 100644 --- a/spec/frontend/admin/abuse_reports/mock_data.js +++ b/spec/frontend/admin/abuse_reports/mock_data.js @@ -6,6 +6,7 @@ export const mockAbuseReports = [ reporter: { name: 'Ms. Admin' }, reportedUser: { name: 'Mr. Abuser' }, reportPath: '/admin/abuse_reports/1', + count: 1, }, { category: 'phishing', @@ -14,5 +15,6 @@ export const mockAbuseReports = [ reporter: { name: 'Ms. Reporter' }, reportedUser: { name: 'Mr. Phisher' }, reportPath: '/admin/abuse_reports/2', + count: 2, }, ]; diff --git a/spec/models/abuse_report_spec.rb b/spec/models/abuse_report_spec.rb index 6192a271028f3ada5dc7cbf1c38cd0867a8c3179..584f9b010ad5000c1e8b50f1b2e21e56cb2a1cd7 100644 --- a/spec/models/abuse_report_spec.rb +++ b/spec/models/abuse_report_spec.rb @@ -164,6 +164,34 @@ expect(described_class.by_category('phishing')).to match_array([report2]) end end + + describe '.aggregated_by_user_and_category' do + let_it_be(:report3) { create(:abuse_report, category: report1.category, user: report1.user) } + let_it_be(:report4) { create(:abuse_report, category: 'phishing', user: report1.user) } + let_it_be(:report5) { create(:abuse_report, category: report1.category, user: build(:user)) } + + let_it_be(:sort_by_count) { true } + + subject(:aggregated) { described_class.aggregated_by_user_and_category(sort_by_count) } + + context 'when sort_by_count = true' do + it 'sorts by aggregated_count in descending order and created_at in descending order' do + expect(aggregated).to eq([report1, report5, report4, report]) + end + + it 'returns count with aggregated reports' do + expect(aggregated[0].count).to eq(2) + end + end + + context 'when sort_by_count = false' do + let_it_be(:sort_by_count) { false } + + it 'does not sort using a specific order' do + expect(aggregated).to match_array([report, report1, report4, report5]) + end + end + end end describe 'before_validation' do diff --git a/spec/serializers/admin/abuse_report_entity_spec.rb b/spec/serializers/admin/abuse_report_entity_spec.rb index 003d76a172fcd6c6668ddabcfce4dafda717062b..c7f57258f40205597a349f42bdb3b71b2815e500 100644 --- a/spec/serializers/admin/abuse_report_entity_spec.rb +++ b/spec/serializers/admin/abuse_report_entity_spec.rb @@ -19,6 +19,7 @@ :category, :created_at, :updated_at, + :count, :reported_user, :reporter, :report_path