From 5e8fae9ddc91c9dcbdff30b93ef0439e8890609b Mon Sep 17 00:00:00 2001 From: Suzanne Aitchison Date: Thu, 14 Jul 2022 17:32:31 +0100 Subject: [PATCH] Member index: Filter by status (#17980) * 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 * 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 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]:https://github.com/ankane/field_test/blob/ab2d7d29d0b9f789831569f6d017ef3ab59d0517/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 Co-authored-by: Fernando Valverde Co-authored-by: Fernando Valverde Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com> Co-authored-by: Jeremy Friesen Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- app/controllers/admin/users_controller.rb | 1 + app/helpers/admin/users_helper.rb | 21 +++++++ app/lib/constants/role.rb | 20 +++--- app/queries/admin/users_query.rb | 17 ++++-- app/views/admin/users/index.html.erb | 6 +- .../users/index/_applied_filters.html.erb | 19 ++++-- .../admin/users/index/_filters_modal.html.erb | 21 ++++++- .../users/index/_status_indicator.html.erb | 12 +--- .../index/_user_status_indicator.html.erb | 11 ++++ .../adminFlows/users/filterUserIndex.spec.js | 61 +++++++++++++++---- spec/queries/admin/users_query_spec.rb | 26 +++++++- 11 files changed, 167 insertions(+), 48 deletions(-) create mode 100644 app/views/admin/users/index/_user_status_indicator.html.erb diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index f0979f5b1..627e629c3 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -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], diff --git a/app/helpers/admin/users_helper.rb b/app/helpers/admin/users_helper.rb index 3f31a8354..7de7e4892 100644 --- a/app/helpers/admin/users_helper.rb +++ b/app/helpers/admin/users_helper.rb @@ -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 diff --git a/app/lib/constants/role.rb b/app/lib/constants/role.rb index ea08658a8..2edebe207 100644 --- a/app/lib/constants/role.rb +++ b/app/lib/constants/role.rb @@ -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 diff --git a/app/queries/admin/users_query.rb b/app/queries/admin/users_query.rb index b8b9e45b1..a67134ba2 100644 --- a/app/queries/admin/users_query.rb +++ b/app/queries/admin/users_query.rb @@ -9,6 +9,7 @@ module Admin # @param role [String, nil] # @param search [String, nil] # @param roles [Array, nil] + # @param statuses [Array, nil] # @param organizations [Array, 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 diff --git a/app/views/admin/users/index.html.erb b/app/views/admin/users/index.html.erb index f2f6b4f96..cc82fad18 100644 --- a/app/views/admin/users/index.html.erb +++ b/app/views/admin/users/index.html.erb @@ -23,7 +23,7 @@
<%= render "admin/users/index/user_actions_dropdown", user: user, id: "responsive-#{user.id}" %> @@ -32,7 +32,7 @@
- <%= render "admin/users/index/status_indicator", user: user %> + <%= render "admin/users/index/user_status_indicator", user: user %>