maverick/docbrown
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Member index: Filter by status (#17980)

Browse source 
* show the statuses in the modal

* filter the results

* add some todo around good standing

* Enable Rails framework 7.0's isolation_level, partial_inserts, & raise_on_open_redirects (#17970)

* Member index: Show applied org filters (#17977)

* show applied org filters

* correct casing

* API V1 transition (#17835)

* API Articles v0-v1 restructure

* Remove unused helper

* Bulk move API controllers into concerns + add V1 controllers

* Extract API routes + some fixes

* Fix v1 api_controller authenticate! + add more article_controller specs

* Completed spec/requests/api/v1/articles_spec.rb

* specs up to listings

* All v1 specs except for 9 skips

* mime_types cleanup + authenticate! relocation

Co-authored-by: Fernando Valverde <fernando@visualcosita.com>

* Fix email confirmation logic when registering via Omniauth (#17878)

* Move .skip_confirmation! call to .find_or_create_user! method

* Add regression tests to avoid email confirmation delivery

* Keep the original place where we had user.skip_confirmation! too

* Update logic to require email confirmation from omniauth

* test confirmation is required with SMTP

* Keep :notice instead of :global_notice

* Forem Account bypass email confirmation reorg

* inline comment reorder + clarification

* Apply suggestions from code review

Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>

Co-authored-by: Fernando Valverde <fernando@visualcosita.com>
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>

* Member Index View Actions: "Adjust Credit Balance" (#17974)

* Adds the Adjust credit balance modal to index view

* Adds e2e tests for adjusting credit balances via modal

* Adds the ability to update credits to #user_status

* Adjusts the e2e to address failures

* Introducing a quick and dirty fix for abtests/admin (#17982)

This is a "quick and dirty" hack.  Do I like it?  No.  Do I want to delve
into further debugging/refactoring of [a 72 line method][1]?  Not at the
moment.  There are a few pathways forward, but for now, this is the
pathway to hopefully give us insight into the

**Why will this likely address the issue?**

Prior to this commit, for an experiment we would render each of the
goals (anywhere from 8 to 11); each of which would require 2 expensive
queries; which means about 16 expensive queries.

After this commit, the experiment page has none of those expensive
queries; instead each goal now runs the 2 expensive queries.

The hope is that this will help us show the results (albeit on multiple
pages) while coming in under the response time out handlers we have in
place.

**Why no tests?**

Ugh; I know right?!?  I'm beginning to ask that myself.  But for now,
because this is only visible to tech_admins, the consequences of
breaking are limited.  In otherwords, this is not a customer facing
feature, so it can be a bit less robust in it's testing.  At least
that's the rationalization I'm establishing.

Further, local tests would not reveal the production environment
complications of large data sets.  The aforementioned expensive queries
are blisteringly fast on my local machine...in part because I don't much
field_test experiment data.

Closes forem/forem#17981

Related to:

- forem/forem#17895
- forem/forem#17869

[1]:ab2d7d29d0/lib/field_test/experiment.rb (L98-L160)

* Adding test for redundant roles

* Adding filter for roles

* forget about good standing status

* remove good standing specs

* Update app/helpers/admin/users_helper.rb

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* appease rubocop

* simplify

* add e2e tests

* add the applied filter pills for status

* make sure clear all button shows

* add a comment to explain good standing

* tweak

* set the registered dates of test users

Co-authored-by: Mac Siri <mac@forem.com>
Co-authored-by: Fernando Valverde <fernando@fdo.cr>
Co-authored-by: Fernando Valverde <fernando@visualcosita.com>
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
Co-authored-by: Jeremy Friesen <jeremy.n.friesen@gmail.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This commit is contained in:
Suzanne Aitchison 2022-07-14 17:32:31 +01:00 • committed by GitHub
parent 30ac3fe80a
commit 5e8fae9ddc
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
11 changed files with 167 additions and 48 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
app/controllers/admin/users_controller.rb
View file

@ -51,6 +51,7 @@ module Admin
search: params[:search],
role: params[:role],
roles: params[:roles],
statuses: params[:statuses],
joining_start: params[:joining_start],
joining_end: params[:joining_end],
date_format: params[:date_format],

21
app/helpers/admin/users_helper.rb
View file

@ -58,6 +58,11 @@ module Admin
end
end
# We de-scoped filtering by "Good standing" due to the complexity of it not mapping directly to a specific role
def filterable_statuses
Constants::Role::BASE_ROLES.reject { |status| status == "Good standing" }
end
# Provides the remaining count when a limit for a resource is imposed on the UI.
#
# @param [Integer] The total count
@ -89,5 +94,21 @@ module Admin
str + " & #{overflow_count(count, imposed_limit: imposed_limit)} others"
end
end
# Returns a string hex code representing the indicator color for the given status (also known as BASE_ROLE)
def status_to_indicator_color(status)
case status
when "Suspended"
"#DC2626"
when "Warned"
"#F59E0B"
when "Comment Suspended"
"#DC2626"
when "Trusted"
"#059669"
else
"#A3A3A3"
end
end
end
end

20
app/lib/constants/role.rb
View file

@ -1,12 +1,15 @@
module Constants
module Role
BASE_ROLES = [
"Warned",
"Comment Suspended",
"Suspended",
"Good standing",
"Trusted",
].freeze
BASE_ROLES_LABELS_TO_WHERE_CLAUSE = {
"Warned" => { name: "warned", resource_type: nil },
"Comment Suspended" => { name: "comment_suspended", resource_type: nil },
"Suspended" => { name: "suspended", resource_type: nil },
# This "role" is a weird amalgamation of multiple roles.
"Good standing" => :good_standing,
"Trusted" => { name: "trusted", resource_type: nil }
}.freeze
BASE_ROLES = BASE_ROLES_LABELS_TO_WHERE_CLAUSE.keys.freeze
SPECIAL_ROLES_LABELS_TO_WHERE_CLAUSE = {
"Admin" => { name: "admin", resource_type: nil },
@ -28,5 +31,8 @@ module Constants
}.freeze
SPECIAL_ROLES = SPECIAL_ROLES_LABELS_TO_WHERE_CLAUSE.keys.freeze
ALL_ROLES_LABELS_TO_WHERE_CLAUSE =
SPECIAL_ROLES_LABELS_TO_WHERE_CLAUSE.merge(BASE_ROLES_LABELS_TO_WHERE_CLAUSE).freeze
end
end

17
app/queries/admin/users_query.rb
View file

@ -9,6 +9,7 @@ module Admin
# @param role [String, nil]
# @param search [String, nil]
# @param roles [Array<String>, nil]
# @param statuses [Array<String>, nil]
# @param organizations [Array<String>, nil]
# @param joining_start [String, nil]
# @param joining_end [String, nil]
@ -18,6 +19,7 @@ module Admin
search: nil,
roles: [],
organizations: [],
statuses: [],
joining_start: nil,
joining_end: nil,
date_format: "DD/MM/YYYY")
@ -25,8 +27,9 @@ module Admin
# need to favor one or the other.
if role.presence
relation = relation.with_role(role, :any)
elsif roles.presence
relation = filter_roles(relation: relation, roles: roles)
elsif roles.presence || statuses.presence
# "statuses" are a subset of roles, so we can handle these filters together
relation = filter_roles(relation: relation, roles: [roles, statuses].compact.reduce([], :|))
end
if organizations.presence
@ -76,16 +79,19 @@ module Admin
# query per role is inadequate.
#
# @see https://github.com/RolifyCommunity/rolify/blob/0c883f4173f409766338b9c6dfc64b0fc8ec8a52/lib/rolify/finders.rb#L26-L32
def self.filter_roles(relation:, roles:, role_map: Constants::Role::SPECIAL_ROLES_LABELS_TO_WHERE_CLAUSE)
def self.filter_roles(relation:, roles:, role_map: Constants::Role::ALL_ROLES_LABELS_TO_WHERE_CLAUSE)
conditions = []
values = []
# Assemble the conditions and positional parameter values
roles.each do |role|
where_clause = role_map.fetch(role)
resource_type = where_clause.fetch(:resource_type, nil)
name = where_clause.fetch(:name)
condition = "(#{Role.table_name}.name = ? AND #{Role.table_name}.resource_id IS NULL"
values << where_clause.fetch(:name)
values << name
# We need to use `IS NULL` instead of `= NULL` as those are different meanings.
if resource_type.nil?
@ -98,7 +104,8 @@ module Admin
conditions << condition
end
relation.joins(:roles).where(%((#{conditions.join(') OR (')})), *values)
sub_query = User.select(:id).distinct.joins(:roles).where(%((#{conditions.join(') OR (')})), *values)
relation.where(id: sub_query)
end
private_class_method :filter_roles
end

6
app/views/admin/users/index.html.erb
View file

@ -23,7 +23,7 @@
</div>
<div class="flex justify-between items-center grow-1">
<div class="hidden m:block">
<%= render "admin/users/index/status_indicator", user: user %>
<%= render "admin/users/index/user_status_indicator", user: user %>
</div>
<div class="ml-auto">
<%= render "admin/users/index/user_actions_dropdown", user: user, id: "responsive-#{user.id}" %>
@ -32,7 +32,7 @@
</header>
<div class="flex justify-between mb-2 block m:hidden">
<div>
<%= render "admin/users/index/status_indicator", user: user %>
<%= render "admin/users/index/user_status_indicator", user: user %>
</div>
<!-- %=
# TODO: [@forem/admin-experience] Uncomment this chunk of code containing role badges once we finalize which roles to include.
@ -84,7 +84,7 @@
</div>
</td>
<td>
<%= render "admin/users/index/status_indicator", user: user %>
<%= render "admin/users/index/user_status_indicator", user: user %>
</td>
<td>
<%= format_last_activity_timestamp(user.last_activity) %>

19
app/views/admin/users/index/_applied_filters.html.erb
View file

@ -10,6 +10,17 @@
<%= role %>
<%= crayons_icon_tag("x.svg", class: "c-pill__action-icon", aria_hidden: true) %>
</button>
<% end %>
<% params[:statuses]&.each do |status| %>
<button
data-filter-key="statuses[]"
data-filter-value="<%= status %>"
aria-label="Remove filter: <%= status %>"
class="c-pill c-pill--action-icon c-pill--action-icon--destructive"
type="button">
<%= render "admin/users/index/status_indicator", status: status %>
<%= crayons_icon_tag("x.svg", class: "c-pill__action-icon", aria_hidden: true) %>
</button>
<% end %>
<% params[:organizations]&.each do |org_id| %>
<% org = @organizations.find_by(id: org_id) %>
@ -23,19 +34,19 @@
<%= crayons_icon_tag("x.svg", class: "c-pill__action-icon", aria_hidden: true) %>
</button>
<% end %>
<% if params[:joining_start]&.present? || params[:joining_end]&.present? %>
<% if params[:joining_start].present? || params[:joining_end].present? %>
<button
data-filter-key="joining_date"
aria-label="Remove filter: Joining date"
class="c-pill c-pill--action-icon c-pill--action-icon--destructive"
type="button">
<% start_date = params[:joining_start]&.blank? ? "Community creation" : params[:joining_start] %>
<% end_date = params[:joining_end]&.blank? ? "Today" : params[:joining_end] %>
<% start_date = params[:joining_start].presence || "Community creation" %>
<% end_date = params[:joining_end].presence || "Today" %>
<%= start_date %> - <%= end_date %>
<%= crayons_icon_tag("x.svg", class: "c-pill__action-icon", aria_hidden: true) %>
</button>
<% end %>
<% if params[:roles] || params[:organizations] || params[:joining_start]&.present? || params[:joining_end]&.present? %>
<% if params[:roles] || params[:organizations] || params[:joining_start].present? || params[:joining_end].present? || params[:statuses] %>
<button class="js-clear-filters-btn c-btn" aria-label="Clear all filters">Clear all</button>
<% end %>
</div>

21
app/views/admin/users/index/_filters_modal.html.erb
View file

@ -39,8 +39,25 @@
</fieldset>
</details>
<details data-section="status" class="admin-details">
<summary class="py-4 flex justify-between text-uppercase color-base-60 fs-s">Status<%= crayons_icon_tag("chevron-down", aria_hidden: true, class: "summary-icon") %></summary>
Status options
<summary class="py-4 flex justify-between text-uppercase color-base-60 fs-s">
<span class="flex">
Status
<span class="js-filtered-indicator-statuses relative ml-2 <%= params[:statuses].blank? ? "hidden" : "" %>">
<span class="block absolute c-indicator c-indicator--info"></span>
</span>
</span>
<%= crayons_icon_tag("chevron-down", aria_hidden: true, class: "summary-icon") %>
</summary>
<fieldset class="js-statuses-fieldset pb-4 flex flex-col items-start">
<legend class="screen-reader-only">Status</legend>
<% filterable_statuses.each_with_index do |status, index| %>
<div class="crayons-field crayons-field--checkbox">
<%= f.check_box :statuses, { multiple: true, class: "crayons-checkbox", id: "filter-status-#{index}", checked: params[:statuses]&.include?(status) }, status, false %>
<%= f.label :statuses, for: "filter-status-#{index}", class: "crayons-field__label" do %><%= render "admin/users/index/status_indicator", status: status %><% end %>
</div>
<% end %>
<button type="button" class="js-clear-filter-btn c-btn <%= params[:statuses].blank? ? "hidden" : "" %>" data-checkbox-fieldset-selector=".js-statuses-fieldset" data-filter-indicator-selector=".js-filtered-indicator-statuses" aria-label="Clear statuses filter">Clear filter</button>
</fieldset>
</details>
<details data-section="joining-date" class="admin-details">
<summary class="py-4 flex justify-between text-uppercase color-base-60 fs-s">

12
app/views/admin/users/index/_status_indicator.html.erb
View file

@ -1,11 +1 @@
<% if user.suspended? %>
<span class="c-indicator mr-2 inline-block" style="--bg: #DC2626"></span>Suspended
<% elsif user.warned? %>
<span class="c-indicator mr-2 inline-block" style="--bg: #F59E0B"></span>Warned
<% elsif user.comment_suspended? %>
<span class="c-indicator mr-2 inline-block" style="--bg: #DC2626"></span>Comment suspended
<% elsif user.trusted? %>
<span class="c-indicator mr-2 inline-block" style="--bg: #059669"></span>Trusted
<% else %>
<span class="c-indicator mr-2 inline-block" style="--bg: #A3A3A3"></span>Good standing
<% end %>
<span class="c-indicator mr-2 inline-block" style="--bg: <%= status_to_indicator_color(status) %>"></span><%= status %>

11
app/views/admin/users/index/_user_status_indicator.html.erb Normal file
View file

@ -0,0 +1,11 @@
<% if user.suspended? %>
<%= render "admin/users/index/status_indicator", status: "Suspended" %>
<% elsif user.warned? %>
<%= render "admin/users/index/status_indicator", status: "Warned" %>
<% elsif user.comment_suspended? %>
<%= render "admin/users/index/status_indicator", status: "Comment Suspended" %>
<% elsif user.trusted? %>
<%= render "admin/users/index/status_indicator", status: "Trusted" %>
<% else %>
<%= render "admin/users/index/status_indicator", status: "Good standing" %>
<% end %>

61
cypress/e2e/seededFlows/adminFlows/users/filterUserIndex.spec.js
View file

@ -10,22 +10,24 @@ describe('Filter user index', () => {
.then((user) => cy.loginAndVisit(user, '/admin/member_manager/users'));
});
const openFiltersModal = () =>
cy.findByRole('button', { name: 'Filter' }).click();
it('Collapses previously opened sections when a new section is expanded', () => {
// TODO: When the V1 Filter input is removed, we can change this to cy.findByRole('button', { name: 'Filter' })
cy.findAllByRole('button', { name: 'Filter' }).last().click();
openFiltersModal();
cy.getModal().within(() => {
cy.findAllByText('Member roles').first().click();
cy.findByRole('group', { name: 'Member roles' }).should('be.visible');
cy.findByText('Status').click();
cy.findByText('Status options').should('be.visible');
cy.findAllByText('Status').first().click();
cy.findByRole('group', { name: 'Status' }).should('be.visible');
cy.findByRole('group', { name: 'Member roles' }).should('not.be.visible');
});
});
it('Displays and clears applied filters', () => {
cy.findAllByRole('button', { name: 'Filter' }).last().click();
openFiltersModal();
cy.getModal().within(() => {
cy.findAllByText('Member roles').first().click();
@ -38,13 +40,17 @@ describe('Filter user index', () => {
cy.findByRole('group', { name: 'Organizations' }).should('be.visible');
cy.findByRole('checkbox', { name: 'Bachmanity' }).check();
cy.findAllByText('Status').first().click();
cy.findByRole('group', { name: 'Status' }).should('be.visible');
cy.findByRole('checkbox', { name: 'Trusted' }).check();
cy.findByRole('button', { name: 'Apply filters' }).click();
});
// Verify applied filter pills are visible
cy.findAllByRole('button', { name: /Remove filter/ }).should(
'have.length',
3,
4,
);
cy.findByRole('button', { name: 'Remove filter: Admin' }).click();
@ -54,7 +60,7 @@ describe('Filter user index', () => {
);
cy.findAllByRole('button', { name: /Remove filter/ }).should(
'have.length',
2,
3,
);
cy.findByRole('button', { name: 'Clear all filters' }).click();
@ -68,7 +74,7 @@ describe('Filter user index', () => {
describe('Member roles', () => {
it('Expands and collapses list of roles', () => {
cy.findAllByRole('button', { name: 'Filter' }).last().click();
openFiltersModal();
cy.getModal().within(() => {
cy.findAllByText('Member roles').first().click();
cy.findByRole('group', { name: 'Member roles' })
@ -100,7 +106,7 @@ describe('Filter user index', () => {
});
it('Filters by a single role', () => {
cy.findAllByRole('button', { name: 'Filter' }).last().click();
openFiltersModal();
cy.getModal().within(() => {
cy.findAllByText('Member roles').first().click();
cy.findByRole('group', { name: 'Member roles' }).should('be.visible');
@ -113,7 +119,7 @@ describe('Filter user index', () => {
});
it('Filters by multiple roles', () => {
cy.findAllByRole('button', { name: 'Filter' }).last().click();
openFiltersModal();
cy.getModal().within(() => {
cy.findAllByText('Member roles').first().click();
cy.findByRole('group', { name: 'Member roles' }).should('be.visible');
@ -127,7 +133,7 @@ describe('Filter user index', () => {
});
it('Clears filters', () => {
cy.findAllByRole('button', { name: 'Filter' }).last().click();
openFiltersModal();
cy.getModal().within(() => {
cy.findAllByText('Member roles').first().click();
cy.findByRole('group', { name: 'Member roles' }).should('be.visible');
@ -156,7 +162,7 @@ describe('Filter user index', () => {
describe('Organizations', () => {
it('filters by organizations', () => {
cy.findAllByRole('button', { name: 'Filter' }).last().click();
openFiltersModal();
cy.getModal().within(() => {
cy.findAllByText('Organizations').first().click();
cy.findByRole('group', { name: 'Organizations' }).should('be.visible');
@ -170,6 +176,35 @@ describe('Filter user index', () => {
});
});
describe('Statuses', () => {
it('Filters by a single status', () => {
openFiltersModal();
cy.getModal().within(() => {
cy.findAllByText('Status').first().click();
cy.findByRole('group', { name: 'Status' }).should('be.visible');
cy.findByRole('checkbox', { name: 'Trusted' }).check();
cy.findByRole('button', { name: 'Apply filters' }).click();
});
// Check expected number of users appear in list
cy.findAllByRole('row').should('have.length', 4);
});
it('Filters by multiple statuses', () => {
openFiltersModal();
cy.getModal().within(() => {
cy.findAllByText('Status').first().click();
cy.findByRole('group', { name: 'Status' }).should('be.visible');
cy.findByRole('checkbox', { name: 'Trusted' }).check();
cy.findByRole('checkbox', { name: 'Suspended' }).check();
cy.findByRole('button', { name: 'Apply filters' }).click();
});
// Check expected number of users appear in list
cy.findAllByRole('row').should('have.length', 5);
});
});
describe('Joining date', () => {
it('filters by joining date', () => {
cy.findByRole('button', { name: 'Filter' }).click();
@ -224,7 +259,7 @@ describe('Filter user index', () => {
describe('Multiple filters', () => {
it('filters by multiple criteria', () => {
cy.findAllByRole('button', { name: 'Filter' }).last().click();
openFiltersModal();
cy.getModal().within(() => {
cy.findAllByText('Member roles').first().click();
cy.findByRole('group', { name: 'Member roles' }).should('be.visible');

26
spec/queries/admin/users_query_spec.rb
View file

@ -3,11 +3,13 @@ require "rails_helper"
RSpec.describe Admin::UsersQuery, type: :query do
subject do
described_class.call(search: search, role: role, roles: roles, organizations: organizations,
joining_start: joining_start, joining_end: joining_end, date_format: date_format)
joining_start: joining_start, joining_end: joining_end, date_format: date_format,
statuses: statuses)
end
let(:role) { nil }
let(:roles) { [] }
let(:statuses) { [] }
let(:organizations) { [] }
let(:search) { [] }
let(:joining_start) { nil }
@ -28,11 +30,14 @@ RSpec.describe Admin::UsersQuery, type: :query do
u.add_role(:single_resource_admin, DataUpdateScript)
end
end
let!(:user8) { create(:user, :comment_suspended, name: "Bob", registered_at: "2020-10-08T13:09:47+0000") }
let!(:user9) { create(:user, name: "Lucia", registered_at: "2020-10-08T13:09:47+0000") }
let!(:user10) { create(:user, :warned, name: "Billie", registered_at: "2020-10-08T13:09:47+0000") }
describe ".call" do
context "when no arguments are given" do
it "returns all users" do
expect(described_class.call).to eq([user7, user6, user5, user4, user3, user2, user])
expect(described_class.call).to eq([user10, user9, user8, user7, user6, user5, user4, user3, user2, user])
end
end
@ -75,8 +80,23 @@ RSpec.describe Admin::UsersQuery, type: :query do
context "when given multiple single_resource_admin roles" do
let(:roles) { ["Admin", "Super Admin", "Resource Admin: DataUpdateScript", "Resource Admin: DisplayAd"] }
let!(:user8) { create(:user).tap { |u| u.add_role(:single_resource_admin, DisplayAd) } }
# This user is provided to ensure our test looks for unique users even if they have duplicate roles
let!(:user9) { create(:user, :super_admin).tap { |u| u.add_role(:single_resource_admin, DisplayAd) } }
it { is_expected.to eq([user8, user7, user6, user5, user4]) }
it { is_expected.to eq([user9, user8, user7, user6, user5, user4]) }
end
context "when given statuses" do
let(:statuses) { ["Warned", "Comment Suspended"] }
it { is_expected.to eq([user10, user8]) }
end
context "when given both roles and statuses" do
let(:statuses) { ["Warned"] }
let(:roles) { ["Admin"] }
it { is_expected.to eq([user10, user5, user4]) }
end
context "when given organizations" do