Change admin users search filter
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.
- Being an admin visit the page /admin/users
- 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
- Verify that sorting and pagination work as it used to do
Related to #238183 (closed) and #448885 (closed)
Merge request reports
Activity
added typefeature label
Hey @bahek2462774!
Thank you for your contribution to GitLab. Please refer to the contribution documentation for an overview of the process.
When you're ready for a first review, post
@gitlab-bot ready
. If you know a relevant reviewer(s) (for example, someone that was involved in a related issue), you can also assign them directly with@gitlab-bot ready @user1 @user2
.At any time, if you need help, feel free to post
@gitlab-bot help
or initiate a mentor session on Discord. Read more on how to get help.You can comment
@gitlab-bot label <label1> <label2>
to add labels to your MR. Please see the list of allowed labels in thelabel
command documentation.This message was generated automatically. You're welcome to improve it.
added Community contribution workflowin dev labels
assigned to @bahek2462774
added linked-issue label
mentioned in issue #238183 (closed)
14 Warnings This merge request is quite big (509 lines changed), please consider splitting it into multiple merge requests. ec46878c: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 660e94a5: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines. a304c68e: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. d8e43f41: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 52d9464a: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. 310a119d: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. 310a119d: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. c97f961e: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines. 1ac4f4b0: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. 2146514f: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. c033f3a0: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. This merge request contains lines with testid selectors. Please ensure e2e:package-and-test
job is run.Labels missing: please ask a reviewer or maintainer to add backend, QA to this merge request. testid
selectorsThe following changed lines in this MR contain
testid
selectors:app/views/admin/users/_users.html.haml
- = gl_tab_link_to admin_users_path(filter: "blocked_pending_approval"), { item_active: active_when(params[:filter] == 'blocked_pending_approval'), class: 'filter-blocked-pending-approval gl-border-0!', data: { testid: 'pending-approval-tab' } } do -.row-content-block.gl-border-0{ data: { testid: "filtered-search-block" } } - = search_field_tag :search_query, params[:search_query], placeholder: s_('AdminUsers|Search by name, email, or username'), class: 'form-control search-text-input js-search-input', spellcheck: false, data: { testid: 'user-search-field' } +.admin-users-search.row-content-block.gl-lg-display-grid.gl-border-bottom-0.gl-border-top-0{ data: { testid: "filtered-search-block" } }
If the
e2e:package-and-test
job in theqa
stage has run automatically, please ensure the tests are passing. If the job has not run, please start thetrigger-omnibus-and-follow-up-e2e
job in theqa
stage and ensure the tests infollow-up-e2e:package-and-test-ee
pipeline are passing.For the list of known failures please refer to the latest pipeline triage issue.
If your changes are under a feature flag, please check our Testing with feature flags documentation for instructions.
Reviewer roulette
Category Reviewer Maintainer backend @arpitgogia
(UTC+5.5)
@ebaque
(UTC-3)
frontend @syarynovskyi
(UTC+3)
@ntepluhina
(UTC+2)
QA @jroodnick
(UTC-7)
Maintainer review is optional for QA test for spec/features/*
@hmuralidhar
(UTC+10)
Maintainer review is optional for test for spec/features/*
UX @ipelaez1
(UTC-4)
Maintainer review is optional for UX Please check reviewer's status!
Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Dangermentioned in issue gitlab-org/quality/triage-reports#16284 (closed)
added frontend label
added Category:User Management admin dashboard labels
added devopsgovern groupauthentication sectionsec labels
added UX label
Thanks for helping us improve the UX of GitLab. Your contribution is appreciated! We have pinged our UX team, so stay tuned for their feedback.
This message was generated automatically. You're welcome to improve it.
requested review from @seggenberger
- Resolved by Ivan Shtyrliaiev
@seggenberger before review could you please read my comment here ? #238183 (comment 1775339790) Thanks
- Resolved by Austin Regnery
Hey @bahek2462774
Thank you for your contribution!
Also I've found a bug:
cc @sunjungp as we recently have looked into the filtered search together
removed review request for @seggenberger
added 810 commits
-
5f1b9e52...33eecda2 - 809 commits from branch
gitlab-org:master
- 6a5a85c7 - Change admin users search filter
-
5f1b9e52...33eecda2 - 809 commits from branch
- Resolved by Austin Regnery
@bahek2462774 the sub tab
Admins
feels a little redudant given that it is an option as a filter token. What do you think about removing the tab and relocating the buttons?Proposed Removing tab
- Resolved by Austin Regnery
@bahek2462774 I was thinking about how we could approach the tokens differently.
- Access Level
- Administrator
- Auditor (new)
- Regular (new)
-
2FA Two-factor Authentication
- 2FA(Enabled) On
- 2FA(Disabled) Off We suggest avoiding disable in our recommended word list
- Status
- External
- Blocked
- Banned
- Pending approval
- Deactivated
- Trusted
- Without projects
Question: What do you think about this structure?
Edited by Austin Regnery - Access Level
- Resolved by Austin Regnery
Specific meaning, specific icon
Using an icon consistently to represent a single concept or action helps with overall learnability for a user. For example, the icon used to represent a feature or an object shouldn't be used to refer to other unrelated concepts or actions in the UI. – Pajamas
@bahek2462774 we have a guiding principle that icons should really only be used to communicate one thing. I don't feel the icons are particularly useful in the dropdown. Question: What do you think about removing the icons?
- Resolved by Austin Regnery
@aregnery could you please take a look again ? here is a video with changes user-filters
And now I have a question about "New user" button. We've removed tabs on the left side and now button takes the whole row. Should we move this button somewhere ?
@gitlab-bot ready
added workflowready for review label and removed workflowin dev label
Hi Coach
@splattael
, this Community contribution is ready for review or needs your coaching.- Do you have capacity and domain expertise to review this? If not, find one or more reviewers and assign to them.
- If you've reviewed it, add the workflowin dev label if these changes need more work before the next review.
This message was generated automatically. You're welcome to improve it.
requested review from @splattael
requested review from @aregnery and removed review request for @splattael
added 1 commit
- 05728817 - fix tests spec/features/admin/users/users_spec.rb
added 2570 commits
-
b3cd97f9...f6ff961c - 2566 commits from branch
gitlab-org:master
- 580e0694 - Change admin users search filter
- 143327f9 - Tests for admin_users_filter_app
- 7a071ddb - fix tests spec/features/admin/users/users_spec.rb
- ccc8013c - fix qa tests
Toggle commit list-
b3cd97f9...f6ff961c - 2566 commits from branch
- Resolved by Ivan Shtyrliaiev
added 467 commits
-
ccc8013c...a211bd18 - 463 commits from branch
gitlab-org:master
- 8f7d62bf - Change admin users search filter
- 982c1adf - Tests for admin_users_filter_app
- a4874afc - fix tests spec/features/admin/users/users_spec.rb
- 0072e4b9 - fix qa tests
Toggle commit list-
ccc8013c...a211bd18 - 463 commits from branch
- Resolved by Austin Regnery
@bahek2462774 this is looking so great and a substantial improvement in UX! I have a few visual nitpicks.
- The line under the tabs doesn't span the width of the container below it making it look short
- The placeholder text is getting cutoff
- 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
mentioned in issue #448885 (closed)
- Resolved by Austin Regnery
@bahek2462774 I have a few more options when using an GitLab Ultimate license. These buttons are missing the padding between them. Question: Can you adjust the spacing?
- Resolved by Austin Regnery
@bahek2462774 there is a pretty big gap between the tabs and the content while on the Cohorts tab. Question (non-blocking): Will you remove the spacing there?
Edited by Austin Regnery
- Resolved by Austin Regnery
Before This change @bahek2462774 Question: what do you think about improving some of the layout on mobile?
requested review from @aregnery
@gitlab-bot help
- Resolved by Eduardo Sanz García
Hey there @ekigbo, could you please help @bahek2462774 out?
This message was generated automatically. You're welcome to improve it.
requested review from @ekigbo
FYI on this @eduardosanz
changed milestone to %16.11
- Resolved by Ezekiel Kigbo
- Resolved by Ezekiel Kigbo
- Resolved by Ezekiel Kigbo
- Resolved by Ezekiel Kigbo
- Resolved by Ezekiel Kigbo
- Resolved by Ezekiel Kigbo
- Resolved by Ivan Shtyrliaiev
- Resolved by Eduardo Sanz García
- Resolved by Ezekiel Kigbo
- Resolved by Ezekiel Kigbo
removed review request for @ekigbo
- Resolved by Austin Regnery
@bahek2462774 nitpick: When on a smaller screen, the token choice isn't visible unless scrolled. Do you think you can address that here?
- Resolved by Eduardo Sanz García
@bahek2462774 I just wanted to thank you again for taking on this effort. I know it was not a simple change, but I think you have substantially improved a page that has been left alone for awhile.
FYI @hsutor just wanted to give you a heads up that this change has been UX approved.
added pipeline:mr-approved label
- Resolved by Eduardo Sanz García
@aregnery
, thanks for approving this merge request.This is the first time the merge request has been approved. To ensure we don't only run predictive pipelines, and we don't break
master
, please start a new pipeline before merging.Please wait for the pipeline to start before resolving this discussion and set auto-merge for the new pipeline. See merging a merge request for more details.
mentioned in issue #451605 (closed)
requested review from @eduardosanz
added 1 commit
- 9d034d7d - 451605 Replace css class with gitlab-ui util
added 1403 commits
-
9d034d7d...71b1d6bf - 1394 commits from branch
gitlab-org:master
- 64914ecb - Change admin users search filter
- 6f803411 - Tests for admin_users_filter_app
- 1aaa9825 - fix tests spec/features/admin/users/users_spec.rb
- cf7b486d - fix qa tests
- c195d36b - Fix wording
- e1cd96ac - fix blocks borders
- 19ef24bb - 448885 Reorder filters
- d1b8193c - Refactor based on MR comments
- bac7a609 - 451605 Replace css class with gitlab-ui util
Toggle commit list-
9d034d7d...71b1d6bf - 1394 commits from branch
added 223 commits
-
bac7a609...c55e1de7 - 214 commits from branch
gitlab-org:master
- 1fe8c2e9 - Change admin users search filter
- c033f3a0 - Tests for admin_users_filter_app
- 2146514f - fix tests spec/features/admin/users/users_spec.rb
- 1ac4f4b0 - fix qa tests
- c97f961e - Fix wording
- 310a119d - fix blocks borders
- 52d9464a - 448885 Reorder filters
- d8e43f41 - Refactor based on MR comments
- a304c68e - 451605 Replace css class with gitlab-ui util
Toggle commit list-
bac7a609...c55e1de7 - 214 commits from branch
- Resolved by Ezekiel Kigbo
- Resolved by Eduardo Sanz García
- Resolved by Eduardo Sanz García
- Resolved by Eduardo Sanz García
- Resolved by Eduardo Sanz García
- Resolved by Ivan Shtyrliaiev
- Resolved by Eduardo Sanz García
@bahek2462774, apologies for the late review.
I have just started the review and I send you a few minor suggestions.
- Resolved by Eduardo Sanz García
- Resolved by Eduardo Sanz García
@bahek2462774, apologies for the late review.
I have just started the review and I send you a few minor suggestions.
requested review from @eduardosanz
- Resolved by Eduardo Sanz García
- Resolved by Eduardo Sanz García
- Resolved by Eduardo Sanz García
- Resolved by Eduardo Sanz García
- Resolved by Eduardo Sanz García
- Resolved by Eduardo Sanz García
- Resolved by Eduardo Sanz García
- Resolved by Eduardo Sanz García
- Resolved by 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)
mentioned in issue gitlab-com/www-gitlab-com#34862 (closed)
mentioned in issue gitlab-org/gitlab-services/design.gitlab.com#1396
- Resolved by Eduardo Sanz García
@jglassman1 Thoughts on this one needing a docs update? I looked at https://docs.gitlab.com/ee/administration/admin_area.html#administering-users and it still references the tabs, which will no longer be a thing after this MR.
mentioned in merge request !149272 (merged)
requested review from @eduardosanz
mentioned in issue #455718 (closed)
- Resolved by Eduardo Sanz García
mentioned in issue #455735 (closed)
@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:
- Ensure the merge request is not in Draft status.
- Start a pipeline (especially important for Community contribution merge requests).
- 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:
- React with a
or a on this comment to describe your experience. - 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!
This message was generated automatically. You're welcome to improve it.
- React with a
Merged! Congrats, @bahek2462774!
mentioned in commit 5f32d095
added workflowstaging-canary label and removed workflowready for review label
mentioned in issue gitlab-org/quality/pipeline-triage#247 (closed)
@eduardosanz This MR has caused a broken master, because it causes https://gitlab.com/gitlab-org/gitlab/-/issues/455758.
The e2e: test-on-gdk child pipeline does not run in pipelines running in the fork, unfortunately.
Edited by Harsha Muralidharsubmit_search_term
andselect_tokens
are helpers defined inspec/features
but are not available inqa/qa/page/admin/overview/users/index.rb
. This has led to broken E2E specs onmaster
.!144907 (diffs) and !144907 (diffs) (for reference)
I am sorry!
I had a feeling this was going to happen. I will take care of moving forward, maybe with a feature flag...
Edited by Eduardo Sanz García@eduardosanz No problem at all. Revert MR was merged.
I believe
pipeline: run-all-e2e
label should run all E2E tests in the MR.Thanks for the reminder, @acunskis! I had the feeling that I was missing something...
@eduardosanz should I revert the merged docs MR as well? Or leave it because you're going to correct the issue and merge the feature again?
Edited by Jon GlassmanThanks @hmuralidhar @acunskis @eduardosanz Is there smth I could help with ?
and question re following tickets this and this Given the fact MR was reverted how do I proceed with those following tickets ? Do I make new branches from the current origin master ? or I need to make a new branch from the fork master? Or new branch from this one (238183-remake-admin-users-search-filter) ?
@eduardosanz @bahek2462774 FYI, Jon and I are reverting the docs as well: !149374 (merged)
In the future MR, could you include the docs together with the code? That way everything will be in sync and we won't need to juggle multiple MRs
@bahek2462774, do not worry, leave it up to me to re-merge the code. I will make sure you are attributed. I will deal with the follow-up tickets.
@marcel.amirault, I will include the documentation as well.
Actually, I didn't include the documentation on the new MR, !149471 (merged), because the new navigation is under a experiment feature flag. If admins don't like the new navigation we can rollback.
I will include the documentation, when I remove the feature flag.
Is there smth I could help with?
@bahek2462774, I have dealt with the issues you mentioned (1, 2) in the new MR.
However, I would be grateful if you could do two things:
- I believe
js-users-tabs
is not associated with any JS functionality. If so it should be replaced by adata-testid
if possible. - The admins may find annoying that we don't show the counts of how many users of each type there are. Could you investigate how to include the count in the token only if there is no text filter?
Thanks!
Edited by Eduardo Sanz García- I believe
Thanks @eduardosanz
I believe
js-users-tabs
is not associated with any JS functionality. If so it should be replaced by adata-testid
if possible.Sure, will do that. Should it be a follow up ticket ? If so could you please create it and assign it to me?
The admins may find annoying that we don't show the counts of how many users of each type there are. Could you investigate how to include the count in the token only if there is no text filter?
what would the proper place for such information? put it directly into the token text (into token's options text) ?
Sure, will do that. Should it be a follow up ticket ? If so could you please create it and assign it to me?
Yes, I will do that on Monday.
what would the proper place for such information? put it directly into the token text (into token's options text)?
Something like this
Banned ⓫
?One more thing, could you add in gitlab-ui the fix to make the input field 100% width in the
GlFilteredSearch
component? Thanks!@eduardosanz Could you please create a follow up ticket for adding a number of users for each filter type.
And it's still not clear how it should look. The idea is to show a users number before we apply filter. I could add this info into data attributes and pass it into vue app (and add it into options text). It's gonna look smth like this
@bahek2462774, here is the issue: #456332 (closed). You are assigned.
The above could be a way to do it. We will need a bit of UX input.
mentioned in merge request gitlab-org/quality/engineering-productivity/fast-quarantine!175 (merged)
mentioned in commit 4b5a377e
mentioned in merge request !149355 (merged)
added workflowcanary label and removed workflowstaging-canary label
mentioned in issue gitlab-org/release/tasks#10083 (closed)
added workflowstaging label and removed workflowcanary label
added workflowproduction label and removed workflowstaging label
mentioned in issue #254377 (closed)
mentioned in merge request !149471 (merged)
mentioned in merge request !149494 (closed)
added workflowpost-deploy-db-production label and removed workflowproduction label
mentioned in issue #456332 (closed)
added releasedcandidate label
mentioned in merge request !150220 (closed)
mentioned in merge request kubitus-project/kubitus-installer!2945 (merged)
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.
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:
only find within the active users.
The above search is functionally equivalent to this, if the 'Active' filter existed:
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.
mentioned in issue #461672 (closed)
mentioned in issue #462877 (closed)
mentioned in merge request !153944 (merged)
added releasedpublished label and removed releasedcandidate label
added pipelinetier-3 label
mentioned in merge request !165403 (merged)
mentioned in issue #498337 (closed)