Skip to content
Snippets Groups Projects

Change admin users search filter

3 unresolved threads

What does this MR do and why?

Change admin users search filter

New admin users search is supposed to have better UX to filter users

Changelog: added

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After
before after

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

  1. Being an admin visit the page /admin/users
  2. Verify that now you are able to filter users using filter-search component Filters should include
  • Access level
    • Administrator
    • External
  • two factor authentication
    • On
    • Off
  • State
    • Blocked
    • Banned
    • Pending approval
    • Deactivated
    • Without project
    • Trusted
  1. Verify that sorting and pagination work as it used to do

Related to #238183 (closed) and #448885 (closed)

Edited by Pedro Moreira da Silva

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Ivan Shtyrliaiev added 467 commits

    added 467 commits

    Compare with previous version

  • Ivan Shtyrliaiev changed the description

    changed the description

    • Resolved by Austin Regnery

      @bahek2462774 this is looking so great and a substantial improvement in UX! I have a few visual nitpicks.

      CleanShot_2024-03-08_at_08.21.51_2x

      1. The line under the tabs doesn't span the width of the container below it making it look short
      2. The placeholder text is getting cutoff
      3. It looks like there are two borders stacking below the search container

      Question: What do you think about cleaning these up?

  • @aregnery thanks for checking and pointing this out. I will fix that

  • added 2 commits

    Compare with previous version

  • mentioned in issue #448885 (closed)

  • added 1 commit

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • requested review from @aregnery

  • requested review from @ekigbo

  • FYI on this @eduardosanz

  • Hannah Sutor changed milestone to %16.11

    changed milestone to %16.11

  • Pedro Moreira da Silva changed the description

    changed the description

  • Ezekiel Kigbo
  • Ezekiel Kigbo
  • Ezekiel Kigbo
  • Ezekiel Kigbo
  • Ezekiel Kigbo
  • Ezekiel Kigbo
  • Ezekiel Kigbo
  • Ezekiel Kigbo
  • Ezekiel Kigbo removed review request for @ekigbo

    removed review request for @ekigbo

  • added 1 commit

    • c18a0e79 - Refactor based on MR comments

    Compare with previous version

  • added 1 commit

    • 814b7a2f - Refactor based on MR comments

    Compare with previous version

  • Austin Regnery approved this merge request

    approved this merge request

  • mentioned in issue #451605 (closed)

  • requested review from @eduardosanz

  • added 1 commit

    • 9d034d7d - 451605 Replace css class with gitlab-ui util

    Compare with previous version

  • Ivan Shtyrliaiev added 1403 commits

    added 1403 commits

    Compare with previous version

  • Ivan Shtyrliaiev added 223 commits

    added 223 commits

    Compare with previous version

  • Ezekiel Kigbo
  • added 1 commit

    Compare with previous version

  • added 3 commits

    • cd74ada9 - Refactor const names
    • 73c03d23 - Move degining available tokens outside of component
    • 135e7ad5 - Update po files

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    • 48cf798b - Remove availableTokens from components data

    Compare with previous version

  • Eduardo Sanz García
  • @bahek2462774, apologies for the late review.

    I have just started the review and I send you a few minor suggestions.

  • added 1 commit

    • d6b4d016 - Remove VueRouter from the app

    Compare with previous version

  • requested review from @eduardosanz

  • Eduardo Sanz García
  • Eduardo Sanz García
    • Resolved by Eduardo Sanz García

      @bahek2462774, I think the logic in admin_users_filter_app.vue could be simplify.

      See this patch:

      Click to expand
      diff --git a/app/assets/javascripts/admin/users/components/admin_users_filter_app.vue b/app/assets/javascripts/admin/users/components/admin_users_filter_app.vue
      index 7c9f1fe6d9ab..06dab9f9bdf7 100644
      --- a/app/assets/javascripts/admin/users/components/admin_users_filter_app.vue
      +++ b/app/assets/javascripts/admin/users/components/admin_users_filter_app.vue
      @@ -1,67 +1,8 @@
       <script>
      -import { GlFilteredSearch, GlFilteredSearchToken } from '@gitlab/ui';
      -import { s__, __ } from '~/locale';
      -import { setUrlParams, visitUrl, getParameterValues } from '~/lib/utils/url_utility';
      -import { OPERATORS_IS } from '~/vue_shared/components/filtered_search_bar/constants';
      -import {
      -  ALL_FILTER_TYPES,
      -  FILTER_TYPE_ADMINS,
      -  FILTER_TYPE_ENABLED_2FA,
      -  FILTER_TYPE_DISABLED_2FA,
      -  FILTER_TYPE_EXTERNAL,
      -  FILTER_TYPE_BLOCKED,
      -  FILTER_TYPE_BANNED,
      -  FILTER_TYPE_BLOCKED_PENDING_APPROVAL,
      -  FILTER_TYPE_DEACTIVATED,
      -  FILTER_TYPE_WOP,
      -  FILTER_TYPE_TRUSTED,
      -  TOKEN_ACCESS_LEVEL,
      -  TOKEN_STATE,
      -  TOKEN_2FA,
      -} from './filter_types';
      -
      -const availableTokens = [
      -  {
      -    title: s__('AdminUsers|Access level'),
      -    type: TOKEN_ACCESS_LEVEL,
      -    token: GlFilteredSearchToken,
      -    operators: OPERATORS_IS,
      -    unique: true,
      -    options: [
      -      { value: FILTER_TYPE_ADMINS, title: s__('AdminUsers|Administrator') },
      -      { value: FILTER_TYPE_EXTERNAL, title: s__('AdminUsers|External') },
      -    ],
      -  },
      -  {
      -    title: __('State'),
      -    type: TOKEN_STATE,
      -    token: GlFilteredSearchToken,
      -    operators: OPERATORS_IS,
      -    unique: true,
      -    options: [
      -      { value: FILTER_TYPE_BANNED, title: s__('AdminUsers|Banned') },
      -      { value: FILTER_TYPE_BLOCKED, title: s__('AdminUsers|Blocked') },
      -      { value: FILTER_TYPE_DEACTIVATED, title: s__('AdminUsers|Deactivated') },
      -      {
      -        value: FILTER_TYPE_BLOCKED_PENDING_APPROVAL,
      -        title: s__('AdminUsers|Pending approval'),
      -      },
      -      { value: FILTER_TYPE_TRUSTED, title: s__('AdminUsers|Trusted') },
      -      { value: FILTER_TYPE_WOP, title: s__('AdminUsers|Without projects') },
      -    ],
      -  },
      -  {
      -    title: s__('AdminUsers|Two-factor authentication'),
      -    type: TOKEN_2FA,
      -    token: GlFilteredSearchToken,
      -    operators: OPERATORS_IS,
      -    unique: true,
      -    options: [
      -      { value: FILTER_TYPE_ENABLED_2FA, title: __('On') },
      -      { value: FILTER_TYPE_DISABLED_2FA, title: __('Off') },
      -    ],
      -  },
      -];
      +import { GlFilteredSearch } from '@gitlab/ui';
      +import { setUrlParams, visitUrl } from '~/lib/utils/url_utility';
      +import { TOKENS, TOKEN_TYPES } from '../constants';
      +import { initializeValues } from '../utils';
       
       export default {
         name: 'AdminUsersFilterApp',
      @@ -70,51 +11,20 @@ export default {
         },
         data() {
           return {
      -      filterValue: [],
      +      values: initializeValues(),
           };
         },
         computed: {
      -    queryFilter() {
      -      return getParameterValues('filter')[0];
      -    },
      -    selectedFilter() {
      -      return this.filterValue.find((selectedFilter) => {
      -        return ALL_FILTER_TYPES.includes(selectedFilter.value?.data);
      -      });
      -    },
      -    selectedFilterData() {
      -      return this.selectedFilter?.value?.data;
      -    },
      -    filteredAvailableTokens() {
      -      if (this.selectedFilterData) {
      -        return availableTokens.filter((token) => {
      -          return token.options.find((option) => option.value === this.selectedFilterData);
      -        });
      -      }
      -      return availableTokens;
      -    },
      -  },
      -  created() {
      -    if (this.queryFilter) {
      -      const filter = availableTokens.find((token) => {
      -        return token.options.find((option) => option.value === this.queryFilter);
      -      });
      +    availableTokens() {
      +      // Once a token is selected, discard the rest
      +      const tokenType = this.values.find(({ type }) => TOKEN_TYPES.includes(type))?.type;
       
      -      if (filter) {
      -        this.filterValue.push({
      -          type: filter.type,
      -          value: {
      -            data: this.queryFilter,
      -            operator: filter.operators[0].value,
      -          },
      -        });
      +      if (tokenType) {
      +        return TOKENS.filter(({ type }) => type === tokenType);
             }
      -    }
       
      -    const [searchQuery] = getParameterValues('search_query');
      -    if (searchQuery) {
      -      this.filterValue.push(searchQuery);
      -    }
      +      return TOKENS;
      +    },
         },
         methods: {
           handleSearch(filters) {
      @@ -137,9 +47,9 @@ export default {
       
       <template>
         <gl-filtered-search
      -    v-model="filterValue"
      +    v-model="values"
           :placeholder="s__('AdminUsers|Search by name, email, or username')"
      -    :available-tokens="filteredAvailableTokens"
      +    :available-tokens="availableTokens"
           class="gl-mb-4"
           terms-as-tokens
           @submit="handleSearch"
      diff --git a/app/assets/javascripts/admin/users/components/filter_types.js b/app/assets/javascripts/admin/users/components/filter_types.js
      deleted file mode 100644
      index 4c8b06a6f451..000000000000
      --- a/app/assets/javascripts/admin/users/components/filter_types.js
      +++ /dev/null
      @@ -1,27 +0,0 @@
      -export const FILTER_TYPE_ADMINS = 'admins';
      -export const FILTER_TYPE_ENABLED_2FA = 'two_factor_enabled';
      -export const FILTER_TYPE_DISABLED_2FA = 'two_factor_disabled';
      -export const FILTER_TYPE_EXTERNAL = 'external';
      -export const FILTER_TYPE_BLOCKED = 'blocked';
      -export const FILTER_TYPE_BANNED = 'banned';
      -export const FILTER_TYPE_BLOCKED_PENDING_APPROVAL = 'blocked_pending_approval';
      -export const FILTER_TYPE_DEACTIVATED = 'deactivated';
      -export const FILTER_TYPE_WOP = 'wop';
      -export const FILTER_TYPE_TRUSTED = 'trusted';
      -
      -export const ALL_FILTER_TYPES = [
      -  FILTER_TYPE_ADMINS,
      -  FILTER_TYPE_ENABLED_2FA,
      -  FILTER_TYPE_DISABLED_2FA,
      -  FILTER_TYPE_EXTERNAL,
      -  FILTER_TYPE_BLOCKED,
      -  FILTER_TYPE_BANNED,
      -  FILTER_TYPE_BLOCKED_PENDING_APPROVAL,
      -  FILTER_TYPE_DEACTIVATED,
      -  FILTER_TYPE_WOP,
      -  FILTER_TYPE_TRUSTED,
      -];
      -
      -export const TOKEN_ACCESS_LEVEL = 'access_level';
      -export const TOKEN_STATE = 'state';
      -export const TOKEN_2FA = '2fa';
      diff --git a/app/assets/javascripts/admin/users/constants.js b/app/assets/javascripts/admin/users/constants.js
      index 73383623aa2d..b331e9a38994 100644
      --- a/app/assets/javascripts/admin/users/constants.js
      +++ b/app/assets/javascripts/admin/users/constants.js
      @@ -1,4 +1,6 @@
      +import { GlFilteredSearchToken } from '@gitlab/ui';
       import { s__, __ } from '~/locale';
      +import { OPERATORS_IS } from '~/vue_shared/components/filtered_search_bar/constants';
       
       export const I18N_USER_ACTIONS = {
         edit: __('Edit'),
      @@ -18,3 +20,63 @@ export const I18N_USER_ACTIONS = {
         trust: s__('AdminUsers|Trust user'),
         untrust: s__('AdminUsers|Untrust user'),
       };
      +
      +const TOKEN_ACCESS_LEVEL = 'access_level';
      +const TOKEN_STATE = 'state';
      +const TOKEN_2FA = '2fa';
      +
      +export const TOKEN_TYPES = [TOKEN_ACCESS_LEVEL, TOKEN_STATE, TOKEN_2FA];
      +
      +const OPTION_ADMINS = 'admins';
      +const OPTION_2FA_ENABLED = 'two_factor_enabled';
      +const OPTION_2FA_DISABLED = 'two_factor_disabled';
      +const OPTION_EXTERNAL = 'external';
      +const OPTION_BLOCKED = 'blocked';
      +const OPTION_BANNED = 'banned';
      +const OPTION_BLOCKED_PENDING_APPROVAL = 'blocked_pending_approval';
      +const OPTION_DEACTIVATED = 'deactivated';
      +const OPTION_WOP = 'wop';
      +const OPTION_TRUSTED = 'trusted';
      +
      +export const TOKENS = [
      +  {
      +    title: s__('AdminUsers|Access level'),
      +    type: TOKEN_ACCESS_LEVEL,
      +    token: GlFilteredSearchToken,
      +    operators: OPERATORS_IS,
      +    unique: true,
      +    options: [
      +      { value: OPTION_ADMINS, title: s__('AdminUsers|Administrator') },
      +      { value: OPTION_EXTERNAL, title: s__('AdminUsers|External') },
      +    ],
      +  },
      +  {
      +    title: __('State'),
      +    type: TOKEN_STATE,
      +    token: GlFilteredSearchToken,
      +    operators: OPERATORS_IS,
      +    unique: true,
      +    options: [
      +      { value: OPTION_BANNED, title: s__('AdminUsers|Banned') },
      +      { value: OPTION_BLOCKED, title: s__('AdminUsers|Blocked') },
      +      { value: OPTION_DEACTIVATED, title: s__('AdminUsers|Deactivated') },
      +      {
      +        value: OPTION_BLOCKED_PENDING_APPROVAL,
      +        title: s__('AdminUsers|Pending approval'),
      +      },
      +      { value: OPTION_TRUSTED, title: s__('AdminUsers|Trusted') },
      +      { value: OPTION_WOP, title: s__('AdminUsers|Without projects') },
      +    ],
      +  },
      +  {
      +    title: s__('AdminUsers|Two-factor authentication'),
      +    type: TOKEN_2FA,
      +    token: GlFilteredSearchToken,
      +    operators: OPERATORS_IS,
      +    unique: true,
      +    options: [
      +      { value: OPTION_2FA_ENABLED, title: __('On') },
      +      { value: OPTION_2FA_DISABLED, title: __('Off') },
      +    ],
      +  },
      +];
      diff --git a/app/assets/javascripts/admin/users/index.js b/app/assets/javascripts/admin/users/index.js
      index 67f4b825c9ff..1a858a6db22f 100644
      --- a/app/assets/javascripts/admin/users/index.js
      +++ b/app/assets/javascripts/admin/users/index.js
      @@ -3,14 +3,12 @@ import VueApollo from 'vue-apollo';
       import createDefaultClient from '~/lib/graphql';
       import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils';
       import csrf from '~/lib/utils/csrf';
      -import { createRouter } from './router';
       import AdminUsersApp from './components/app.vue';
       import AdminUsersFilterApp from './components/admin_users_filter_app.vue';
       import DeleteUserModal from './components/modals/delete_user_modal.vue';
       import UserActions from './components/user_actions.vue';
       
       Vue.use(VueApollo);
      -const router = createRouter();
       
       const apolloProvider = new VueApollo({
         defaultClient: createDefaultClient(),
      @@ -40,17 +38,16 @@ const initApp = (el, component, userPropKey, props = {}) => {
       export const initAdminUsersApp = (el = document.querySelector('#js-admin-users-app')) =>
         initApp(el, AdminUsersApp, 'users');
       
      +export const initAdminUserActions = (el = document.querySelector('#js-admin-user-actions')) =>
      +  initApp(el, UserActions, 'user', { showButtonLabels: true });
      +
       export const initAdminUsersFilterApp = () => {
         return new Vue({
           el: document.querySelector('#js-admin-users-filter-app'),
      -    router,
           render: (createElement) => createElement(AdminUsersFilterApp),
      -  }).$mount();
      +  });
       };
       
      -export const initAdminUserActions = (el = document.querySelector('#js-admin-user-actions')) =>
      -  initApp(el, UserActions, 'user', { showButtonLabels: true });
      -
       export const initDeleteUserModals = () => {
         return new Vue({
           functional: true,
      diff --git a/app/assets/javascripts/admin/users/utils.js b/app/assets/javascripts/admin/users/utils.js
      index f6c1091ba276..7bfb35a3b76e 100644
      --- a/app/assets/javascripts/admin/users/utils.js
      +++ b/app/assets/javascripts/admin/users/utils.js
      @@ -1,3 +1,6 @@
      +import { queryToObject } from '~/lib/utils/url_utility';
      +import { TOKENS } from './constants';
      +
       export const generateUserPaths = (paths, id) => {
         return Object.fromEntries(
           Object.entries(paths).map(([action, genericPath]) => {
      @@ -5,3 +8,33 @@ export const generateUserPaths = (paths, id) => {
           }),
         );
       };
      +
      +/**
      + * Initialize values based on the URL parameters
      + * @param {string} query
      + */
      +export function initializeValues(query = document.location.search) {
      +  const values = [];
      +
      +  const { filter, search_query: searchQuery } = queryToObject(query);
      +
      +  if (filter) {
      +    const token = TOKENS.find(({ options }) => options.some(({ value }) => value === filter));
      +
      +    if (token) {
      +      values.push({
      +        type: token.type,
      +        value: {
      +          data: filter,
      +          operator: token.operators[0].value,
      +        },
      +      });
      +    }
      +  }
      +
      +  if (searchQuery) {
      +    values.push(searchQuery);
      +  }
      +
      +  return values;
      +}
      

      I am open to discussion.

  • mentioned in issue #353277 (closed)

  • Jon Glassman mentioned in merge request !149272 (merged)

    mentioned in merge request !149272 (merged)

  • added 1 commit

    • ec46878c - Refactor according to mr comments

    Compare with previous version

  • requested review from @eduardosanz

  • Eduardo Sanz García approved this merge request

    approved this merge request

  • Eduardo Sanz García resolved all threads

    resolved all threads

  • Eduardo Sanz García
  • Eduardo Sanz García resolved all threads

    resolved all threads

  • @eduardosanz, did you forget to run a pipeline before you merged this work? Based on our code review process, if the latest pipeline was created more than 4 hours ago, you should:

    1. Ensure the merge request is not in Draft status.
    2. Start a pipeline (especially important for Community contribution merge requests).
    3. Set the merge request to auto-merge.

    This is a guideline, not a rule. Please consider replying to this comment for transparency.

    This message was generated automatically. You're welcome to improve it.

  • @bahek2462774, how was your code review experience with this merge request? Please tell us how we can continue to iterate and improve:

    1. React with a :thumbsup: or a :thumbsdown: on this comment to describe your experience.
    2. Create a new comment starting with @gitlab-bot feedback below, and leave any additional feedback you have for us in the comment.

    Subscribe to the GitLab Community Newsletter for contributor-focused content and opportunities to level up.

    Thanks for your help! :heart:

    This message was generated automatically. You're welcome to improve it.

  • mentioned in commit 5f32d095

  • mentioned in commit 4b5a377e

  • mentioned in merge request !149355 (merged)

  • added workflowstaging label and removed workflowcanary label

  • mentioned in issue #254377 (closed)

  • mentioned in merge request !149471 (merged)

  • mentioned in merge request !149494 (closed)

  • mentioned in merge request !150220 (closed)

  • mentioned in merge request !150322 (merged)

    • Hi @bahek2462774 I was pointed here as a potential place to report, but it appears that the /admin/users search functionality is not working properly. If I search for known email/users that are blocked the search will show no results. Even using the state= filters does not cause it to return the proper results. I can only find these users using the /api/v4/users?search functionality now.

    • @sgillespie2 will you open a new typebug issue and share a clip of the search not working?

    • @aregnery After playing with it a bit I was able to get the search to find the users in question by setting state=banned, but we really need the default to not be filtering invisibly. There used to be tabs that would show results of users in states other than active, but those are gone now. Does that still need a bug issue?

      The use case here is when we get requests from law enforcement or have threat intel we may search for a user by email without knowing if they have an account or what state it might be in. Having it hide results by default leads to us missing critical information.

    • Please register or sign in to reply
    • we really need the default to not be filtering invisibly... Having it hide results by default leads to us missing critical information.

      I would say that sounds like a bug to me @sgillespie2. It sounds very reasonable to search for a username and have it appear regardless if it has been banned or not.

    • @aregnery Opened here #461672 (closed) I am kinda curious to know where the problem actually is. Looking at this MR I have a vague sense of what is happening, but I'm not familiar enough with Ruby/Rails to really dive in myself.

    • @sgillespie2, the problem is that a textual search without filters:

      Screenshot_2024-05-16_at_10.00.13

      only find within the active users.

      The above search is functionally equivalent to this, if the 'Active' filter existed:

      Screenshot_2024-05-16_at_10.03.49

    • The solution would be to (1) change the backend to search by default all the users, not just within the active users and (2) add a new filter to search only 'Active' users.

      I am not sure how much work would be. In the meantime, I wonder if we should revert Enable the new navigation for Admin area > Users (!150288 - merged) and re-enable the old tabs until we iron this issue.

      /cc @adil.farrukh

      Edited by Eduardo Sanz García
    • @sgillespie2 Could I request a severity for #461672 (closed) from a security team perspective. The other primary users for this would be SM admins, and generally considered severity3 .

      It'd be preferable to not revert the change (specially as it's already out since 16.11 so the fix won't be seen till 17.1 at the earliest). @eduardosanz In addition to the options you mentioned, would we benefit from indicating to the user that the list is active, and that they should use a Banned filter to see other types? (as a boring/quick option)

    • @adil.farrukh I'd say at least severity3 possibly severity2 because while the workaround does work it is going to be difficult to communicate to everyone that they have to search every status individually or use the Users API directly to ensure they don't miss something. Some kind of warning or clear indication that it only works for Active users would hopefully help.

      In our use cases we often don't know ahead of time if there was ever an account associated with the email addresses we are given so it makes it easy to get an empty result and believe that the search worked correctly and that there were no results. I only stumbled into this because I was checking multiple emails and got no results for one I knew should have existed and then used the API to verify that search worked correctly.

      @eduardosanz The solution of searching all users and adding a filter for Active would definitely be the preferred approach and would make the UI search behave more like the Users API.

    • @eduardosanz When you get a chance, please add a guesstimate weight to Admin User search hides results by default (#461672 - closed) or let us know here and we can re-evaluate a possible revert or the additional change.

    • Please register or sign in to reply
  • mentioned in issue #461672 (closed)

  • mentioned in issue #462877 (closed)

  • Vidhya Hariharan mentioned in merge request !153944 (merged)

    mentioned in merge request !153944 (merged)

  • Austin Regnery mentioned in merge request !165403 (merged)

    mentioned in merge request !165403 (merged)

  • mentioned in issue #498337 (closed)

  • Please register or sign in to reply
    Loading