Merge branch '228675-separate-filtering-users-from-sorting-users-add-sort-dropdown' into 'master'

Add sort dropdown to group members view

See merge request gitlab-org/gitlab!49168

app/assets/javascripts/members/components/filter_sort/filter_sort_container.vue

 <script>
 import { mapState } from 'vuex';
 import MembersFilteredSearchBar from './members_filtered_search_bar.vue';
+import SortDropdown from './sort_dropdown.vue';
 
 export default {
   name: 'FilterSortContainer',
-  components: { MembersFilteredSearchBar },
+  components: { MembersFilteredSearchBar, SortDropdown },
   computed: {
-    ...mapState(['filteredSearchBar']),
+    ...mapState(['filteredSearchBar', 'tableSortableFields']),
+    showContainer() {
+      return this.filteredSearchBar.show || this.showSortDropdown;
+    },
+    showSortDropdown() {
+      return this.tableSortableFields.length;
+    },
   },
 };
 </script>
 
 <template>
-  <div v-if="filteredSearchBar.show" class="gl-bg-gray-10 gl-p-5">
-    <members-filtered-search-bar />
+  <div v-if="showContainer" class="gl-bg-gray-10 gl-p-3 gl-display-md-flex">
+    <members-filtered-search-bar v-if="filteredSearchBar.show" class="gl-p-3 gl-flex-grow-1" />
+    <sort-dropdown v-if="showSortDropdown" class="gl-p-3 gl-flex-shrink-0" />
   </div>
 </template>

app/assets/javascripts/members/components/filter_sort/sort_dropdown.vue

+<script>
+import { mapState } from 'vuex';
+import { GlDropdown, GlDropdownItem, GlFormGroup } from '@gitlab/ui';
+import { parseSortParam, buildSortUrl } from '~/members/utils';
+import { FIELDS } from '~/members/constants';
+
+export default {
+  name: 'SortDropdown',
+  components: { GlDropdown, GlDropdownItem, GlFormGroup },
+  computed: {
+    ...mapState(['tableSortableFields', 'filteredSearchBar']),
+    sort() {
+      return parseSortParam(this.tableSortableFields);
+    },
+    filteredOptions() {
+      const buildOption = (field, sortDesc) => ({
+        ...(sortDesc ? field.sort.desc : field.sort.asc),
+        key: field.key,
+        sortDesc,
+        url: buildSortUrl({
+          sortBy: field.key,
+          sortDesc,
+          filteredSearchBarTokens: this.filteredSearchBar.tokens,
+          filteredSearchBarSearchParam: this.filteredSearchBar.searchParam,
+        }),
+      });
+
+      return FIELDS.filter(
+        field => this.tableSortableFields.includes(field.key) && field.sort,
+      ).flatMap(field => [buildOption(field, false), buildOption(field, true)]);
+    },
+  },
+  methods: {
+    isChecked(key, sortDesc) {
+      return this.sort?.sortBy === key && this.sort?.sortDesc === sortDesc;
+    },
+  },
+};
+</script>
+
+<template>
+  <gl-form-group
+    :label="__('Sort by')"
+    class="gl-mb-0"
+    label-cols="auto"
+    label-class="gl-align-self-center gl-pb-0!"
+  >
+    <gl-dropdown
+      :text="sort.sortByLabel"
+      block
+      toggle-class="gl-mb-0"
+      data-testid="members-sort-dropdown"
+      right
+    >
+      <gl-dropdown-item
+        v-for="option in filteredOptions"
+        :key="option.param"
+        :href="option.url"
+        is-check-item
+        :is-checked="isChecked(option.key, option.sortDesc)"
+      >
+        {{ option.label }}
+      </gl-dropdown-item>
+    </gl-dropdown>
+  </gl-form-group>
+</template>

app/assets/javascripts/members/constants.js

-import { __ } from '~/locale';
+import { __, s__ } from '~/locale';
+
+const ACCOUNT_SORT_ASC_LABEL = s__('Members|Account, ascending');
 
 export const FIELDS = [
   {
     key: 'account',
     label: __('Account'),
+    sort: {
+      asc: {
+        param: 'name_asc',
+        label: ACCOUNT_SORT_ASC_LABEL,
+      },
+      desc: {
+        param: 'name_desc',
+        label: s__('Members|Account, descending'),
+      },
+    },
   },
   {
     key: 'source',
@@ -16,6 +28,16 @@ export const FIELDS = [
     label: __('Access granted'),
     thClass: 'col-meta',
     tdClass: 'col-meta',
+    sort: {
+      asc: {
+        param: 'last_joined',
+        label: s__('Members|Access granted, ascending'),
+      },
+      desc: {
+        param: 'oldest_joined',
+        label: s__('Members|Access granted, descending'),
+      },
+    },
   },
   {
     key: 'invited',
@@ -40,6 +62,16 @@ export const FIELDS = [
     label: __('Max role'),
     thClass: 'col-max-role',
     tdClass: 'col-max-role',
+    sort: {
+      asc: {
+        param: 'access_level_asc',
+        label: s__('Members|Max role, ascending'),
+      },
+      desc: {
+        param: 'access_level_desc',
+        label: s__('Members|Max role, descending'),
+      },
+    },
   },
   {
     key: 'expiration',
@@ -47,6 +79,19 @@ export const FIELDS = [
     thClass: 'col-expiration',
     tdClass: 'col-expiration',
   },
+  {
+    key: 'lastSignIn',
+    sort: {
+      asc: {
+        param: 'recent_sign_in',
+        label: s__('Members|Last sign-in, ascending'),
+      },
+      desc: {
+        param: 'oldest_sign_in',
+        label: s__('Members|Last sign-in, descending'),
+      },
+    },
+  },
   {
     key: 'actions',
     thClass: 'col-actions',
@@ -55,6 +100,12 @@ export const FIELDS = [
   },
 ];
 
+export const DEFAULT_SORT = {
+  sortBy: 'account',
+  sortDesc: false,
+  sortByLabel: ACCOUNT_SORT_ASC_LABEL,
+};
+
 export const AVATAR_SIZE = 48;
 
 export const MEMBER_TYPES = {

app/assets/javascripts/members/utils.js

 import { __ } from '~/locale';
+import { getParameterByName } from '~/lib/utils/common_utils';
+import { setUrlParams } from '~/lib/utils/url_utility';
+import { FIELDS, DEFAULT_SORT } from './constants';
 
 export const generateBadges = (member, isCurrentUser) => [
   {
@@ -44,5 +47,54 @@ export const canUpdate = (member, currentUserId, sourceId) => {
   );
 };
 
+export const parseSortParam = sortableFields => {
+  const sortParam = getParameterByName('sort');
+
+  const sortedField = FIELDS.filter(field => sortableFields.includes(field.key)).find(
+    field => field.sort?.asc?.param === sortParam || field.sort?.desc?.param === sortParam,
+  );
+
+  if (!sortedField) {
+    return DEFAULT_SORT;
+  }
+
+  const isDesc = sortedField?.sort?.desc?.param === sortParam;
+
+  return {
+    sortBy: sortedField.key,
+    sortDesc: isDesc,
+    sortByLabel: isDesc ? sortedField?.sort?.desc?.label : sortedField?.sort?.asc?.label,
+  };
+};
+
+export const buildSortUrl = ({
+  sortBy,
+  sortDesc,
+  filteredSearchBarTokens,
+  filteredSearchBarSearchParam,
+}) => {
+  const sortDefinition = FIELDS.find(field => field.key === sortBy)?.sort;
+
+  if (!sortDefinition) {
+    return '';
+  }
+
+  const sortParam = sortDesc ? sortDefinition.desc.param : sortDefinition.asc.param;
+
+  const filterParams =
+    filteredSearchBarTokens?.reduce((accumulator, token) => {
+      return {
+        ...accumulator,
+        [token]: getParameterByName(token),
+      };
+    }, {}) || {};
+
+  if (filteredSearchBarSearchParam) {
+    filterParams[filteredSearchBarSearchParam] = getParameterByName(filteredSearchBarSearchParam);
+  }
+
+  return setUrlParams({ ...filterParams, sort: sortParam }, window.location.href, true);
+};
+
 // Defined in `ee/app/assets/javascripts/vue_shared/components/members/utils.js`
 export const canOverride = () => false;

locale/gitlab.pot

@@ -16900,6 +16900,18 @@ msgstr ""
 msgid "Members|2FA"
 msgstr ""
 
+msgid "Members|Access granted, ascending"
+msgstr ""
+
+msgid "Members|Access granted, descending"
+msgstr ""
+
+msgid "Members|Account, ascending"
+msgstr ""
+
+msgid "Members|Account, descending"
+msgstr ""
+
 msgid "Members|An error occurred while trying to enable LDAP override, please try again."
 msgstr ""
 
@@ -16963,9 +16975,21 @@ msgstr ""
 msgid "Members|LDAP override enabled."
 msgstr ""
 
+msgid "Members|Last sign-in, ascending"
+msgstr ""
+
+msgid "Members|Last sign-in, descending"
+msgstr ""
+
 msgid "Members|Leave \"%{source}\""
 msgstr ""
 
+msgid "Members|Max role, ascending"
+msgstr ""
+
+msgid "Members|Max role, descending"
+msgstr ""
+
 msgid "Members|Membership"
 msgstr ""
 

spec/features/groups/members/sort_members_spec.rb

@@ -9,17 +9,96 @@ RSpec.describe 'Groups > Members > Sort members', :js do
   let(:developer) { create(:user, name: 'Mary Jane', last_sign_in_at: 5.days.ago) }
   let(:group)     { create(:group) }
 
-  dropdown_toggle_selector = '[data-testid="user-sort-dropdown"] [data-testid="dropdown-toggle"]'
-
   before do
-    stub_feature_flags(group_members_filtered_search: false)
-
     create(:group_member, :owner, user: owner, group: group, created_at: 5.days.ago)
     create(:group_member, :developer, user: developer, group: group, created_at: 3.days.ago)
 
     sign_in(owner)
   end
 
+  context 'when `group_members_filtered_search` feature flag is enabled' do
+    dropdown_toggle_selector = '[data-testid="members-sort-dropdown"] > button'
+
+    it 'sorts account by default' do
+      visit_members_list(sort: nil)
+
+      expect(first_row.text).to include(owner.name)
+      expect(second_row.text).to include(developer.name)
+      expect(page).to have_css(dropdown_toggle_selector, text: 'Account, ascending')
+    end
+
+    it 'sorts by max role ascending' do
+      visit_members_list(sort: :access_level_asc)
+
+      expect(first_row.text).to include(developer.name)
+      expect(second_row.text).to include(owner.name)
+      expect(page).to have_css(dropdown_toggle_selector, text: 'Max role, ascending')
+    end
+
+    it 'sorts by max role descending' do
+      visit_members_list(sort: :access_level_desc)
+
+      expect(first_row.text).to include(owner.name)
+      expect(second_row.text).to include(developer.name)
+      expect(page).to have_css(dropdown_toggle_selector, text: 'Max role, descending')
+    end
+
+    it 'sorts by access granted ascending' do
+      visit_members_list(sort: :last_joined)
+
+      expect(first_row.text).to include(developer.name)
+      expect(second_row.text).to include(owner.name)
+      expect(page).to have_css(dropdown_toggle_selector, text: 'Access granted, ascending')
+    end
+
+    it 'sorts by access granted descending' do
+      visit_members_list(sort: :oldest_joined)
+
+      expect(first_row.text).to include(owner.name)
+      expect(second_row.text).to include(developer.name)
+      expect(page).to have_css(dropdown_toggle_selector, text: 'Access granted, descending')
+    end
+
+    it 'sorts by account ascending' do
+      visit_members_list(sort: :name_asc)
+
+      expect(first_row.text).to include(owner.name)
+      expect(second_row.text).to include(developer.name)
+      expect(page).to have_css(dropdown_toggle_selector, text: 'Account, ascending')
+    end
+
+    it 'sorts by account descending' do
+      visit_members_list(sort: :name_desc)
+
+      expect(first_row.text).to include(developer.name)
+      expect(second_row.text).to include(owner.name)
+      expect(page).to have_css(dropdown_toggle_selector, text: 'Account, descending')
+    end
+
+    it 'sorts by last sign-in ascending', :clean_gitlab_redis_shared_state do
+      visit_members_list(sort: :recent_sign_in)
+
+      expect(first_row.text).to include(owner.name)
+      expect(second_row.text).to include(developer.name)
+      expect(page).to have_css(dropdown_toggle_selector, text: 'Last sign-in, ascending')
+    end
+
+    it 'sorts by last sign-in descending', :clean_gitlab_redis_shared_state do
+      visit_members_list(sort: :oldest_sign_in)
+
+      expect(first_row.text).to include(developer.name)
+      expect(second_row.text).to include(owner.name)
+      expect(page).to have_css(dropdown_toggle_selector, text: 'Last sign-in, descending')
+    end
+  end
+
+  context 'when `group_members_filtered_search` feature flag is disabled' do
+    dropdown_toggle_selector = '[data-testid="user-sort-dropdown"] [data-testid="dropdown-toggle"]'
+
+    before do
+      stub_feature_flags(group_members_filtered_search: false)
+    end
+
     it 'sorts alphabetically by default' do
       visit_members_list(sort: nil)
 
@@ -91,6 +170,7 @@ RSpec.describe 'Groups > Members > Sort members', :js do
       expect(second_row.text).to include(owner.name)
       expect(page).to have_css(dropdown_toggle_selector, text: 'Oldest sign in')
     end
+  end
 
   def visit_members_list(sort:)
     visit group_group_members_path(group.to_param, sort: sort)

spec/frontend/members/components/filter_sort/filter_sort_container_spec.js

@@ -2,6 +2,7 @@ import { shallowMount, createLocalVue } from '@vue/test-utils';
 import Vuex from 'vuex';
 import FilterSortContainer from '~/members/components/filter_sort/filter_sort_container.vue';
 import MembersFilteredSearchBar from '~/members/components/filter_sort/members_filtered_search_bar.vue';
+import SortDropdown from '~/members/components/filter_sort/sort_dropdown.vue';
 
 const localVue = createLocalVue();
 localVue.use(Vuex);
@@ -19,6 +20,7 @@ describe('FilterSortContainer', () => {
           placeholder: 'Filter members',
           recentSearchesStorageKey: 'group_members',
         },
+        tableSortableFields: ['account'],
         ...state,
       },
     });
@@ -29,12 +31,13 @@ describe('FilterSortContainer', () => {
     });
   };
 
-  describe('when `filteredSearchBar.show` is `false`', () => {
+  describe('when `filteredSearchBar.show` is `false` and `tableSortableFields` is empty', () => {
     it('renders nothing', () => {
       createComponent({
         filteredSearchBar: {
           show: false,
         },
+        tableSortableFields: [],
       });
 
       expect(wrapper.html()).toBe('');
@@ -52,4 +55,14 @@ describe('FilterSortContainer', () => {
       expect(wrapper.find(MembersFilteredSearchBar).exists()).toBe(true);
     });
   });
+
+  describe('when `tableSortableFields` is set', () => {
+    it('renders `SortDropdown`', () => {
+      createComponent({
+        tableSortableFields: ['account'],
+      });
+
+      expect(wrapper.find(SortDropdown).exists()).toBe(true);
+    });
+  });
 });

spec/frontend/members/components/filter_sort/sort_dropdown_spec.js

+import { mount, createLocalVue } from '@vue/test-utils';
+import { within } from '@testing-library/dom';
+import Vuex from 'vuex';
+import { GlDropdownItem } from '@gitlab/ui';
+import SortDropdown from '~/members/components/filter_sort/sort_dropdown.vue';
+
+const localVue = createLocalVue();
+localVue.use(Vuex);
+
+describe('SortDropdown', () => {
+  let wrapper;
+
+  const URL_HOST = 'https://localhost/';
+
+  const createComponent = state => {
+    const store = new Vuex.Store({
+      state: {
+        sourceId: 1,
+        tableSortableFields: ['account', 'granted', 'expires', 'maxRole', 'lastSignIn'],
+        filteredSearchBar: {
+          show: true,
+          tokens: ['two_factor'],
+          searchParam: 'search',
+          placeholder: 'Filter members',
+          recentSearchesStorageKey: 'group_members',
+        },
+        ...state,
+      },
+    });
+
+    wrapper = mount(SortDropdown, {
+      localVue,
+      store,
+    });
+  };
+
+  const findDropdownToggle = () => wrapper.find('button[aria-haspopup="true"]');
+  const findDropdownItemByText = text =>
+    wrapper
+      .findAll(GlDropdownItem)
+      .wrappers.find(dropdownItemWrapper => dropdownItemWrapper.text() === text);
+
+  describe('dropdown options', () => {
+    beforeEach(() => {
+      delete window.location;
+      window.location = new URL(URL_HOST);
+    });
+
+    it('adds dropdown items for all the sortable fields', () => {
+      const URL_FILTER_PARAMS = '?two_factor=enabled&search=foobar';
+      const EXPECTED_BASE_URL = `${URL_HOST}${URL_FILTER_PARAMS}&sort=`;
+
+      window.location.search = URL_FILTER_PARAMS;
+
+      const expectedDropdownItems = [
+        {
+          label: 'Account, ascending',
+          url: `${EXPECTED_BASE_URL}name_asc`,
+        },
+        {
+          label: 'Account, descending',
+          url: `${EXPECTED_BASE_URL}name_desc`,
+        },
+        {
+          label: 'Access granted, ascending',
+          url: `${EXPECTED_BASE_URL}last_joined`,
+        },
+        {
+          label: 'Access granted, descending',
+          url: `${EXPECTED_BASE_URL}oldest_joined`,
+        },
+        {
+          label: 'Max role, ascending',
+          url: `${EXPECTED_BASE_URL}access_level_asc`,
+        },
+        {
+          label: 'Max role, descending',
+          url: `${EXPECTED_BASE_URL}access_level_desc`,
+        },
+        {
+          label: 'Last sign-in, ascending',
+          url: `${EXPECTED_BASE_URL}recent_sign_in`,
+        },
+        {
+          label: 'Last sign-in, descending',
+          url: `${EXPECTED_BASE_URL}oldest_sign_in`,
+        },
+      ];
+
+      createComponent();
+
+      expectedDropdownItems.forEach(expectedDropdownItem => {
+        const dropdownItem = findDropdownItemByText(expectedDropdownItem.label);
+
+        expect(dropdownItem).not.toBe(null);
+        expect(dropdownItem.find('a').attributes('href')).toBe(expectedDropdownItem.url);
+      });
+    });
+
+    it('checks selected sort option', () => {
+      window.location.search = '?sort=access_level_asc';
+
+      createComponent();
+
+      expect(findDropdownItemByText('Max role, ascending').props('isChecked')).toBe(true);
+    });
+  });
+
+  describe('dropdown toggle', () => {
+    beforeEach(() => {
+      delete window.location;
+      window.location = new URL(URL_HOST);
+    });
+
+    it('defaults to sorting by "Account, ascending"', () => {
+      createComponent();
+
+      expect(findDropdownToggle().text()).toBe('Account, ascending');
+    });
+
+    it('sets text as selected sort option', () => {
+      window.location.search = '?sort=access_level_asc';
+
+      createComponent();
+
+      expect(findDropdownToggle().text()).toBe('Max role, ascending');
+    });
+  });
+
+  it('renders dropdown label', () => {
+    createComponent();
+
+    expect(within(wrapper.element).queryByText('Sort by')).not.toBe(null);
+  });
+});

spec/frontend/members/utils_spec.js

@@ -7,13 +7,17 @@ import {
   canResend,
   canUpdate,
   canOverride,
+  parseSortParam,
+  buildSortUrl,
 } from '~/members/utils';
+import { DEFAULT_SORT } from '~/members/constants';
 import { member as memberMock, group, invite } from './mock_data';
 
 const DIRECT_MEMBER_ID = 178;
 const INHERITED_MEMBER_ID = 179;
 const IS_CURRENT_USER_ID = 123;
 const IS_NOT_CURRENT_USER_ID = 124;
+const URL_HOST = 'https://localhost/';
 
 describe('Members Utils', () => {
   describe('generateBadges', () => {
@@ -119,4 +123,110 @@ describe('Members Utils', () => {
       expect(canOverride(memberMock)).toBe(false);
     });
   });
+
+  describe('parseSortParam', () => {
+    beforeEach(() => {
+      delete window.location;
+      window.location = new URL(URL_HOST);
+    });
+
+    describe('when `sort` param is not present', () => {
+      it('returns default sort options', () => {
+        window.location.search = '';
+
+        expect(parseSortParam(['account'])).toEqual(DEFAULT_SORT);
+      });
+    });
+
+    describe('when field passed in `sortableFields` argument does not have `sort` key defined', () => {
+      it('returns default sort options', () => {
+        window.location.search = '?sort=source_asc';
+
+        expect(parseSortParam(['source'])).toEqual(DEFAULT_SORT);
+      });
+    });
+
+    describe.each`
+      sortParam              | expected
+      ${'name_asc'}          | ${{ sortBy: 'account', sortDesc: false, sortByLabel: 'Account, ascending' }}
+      ${'name_desc'}         | ${{ sortBy: 'account', sortDesc: true, sortByLabel: 'Account, descending' }}
+      ${'last_joined'}       | ${{ sortBy: 'granted', sortDesc: false, sortByLabel: 'Access granted, ascending' }}
+      ${'oldest_joined'}     | ${{ sortBy: 'granted', sortDesc: true, sortByLabel: 'Access granted, descending' }}
+      ${'access_level_asc'}  | ${{ sortBy: 'maxRole', sortDesc: false, sortByLabel: 'Max role, ascending' }}
+      ${'access_level_desc'} | ${{ sortBy: 'maxRole', sortDesc: true, sortByLabel: 'Max role, descending' }}
+      ${'recent_sign_in'}    | ${{ sortBy: 'lastSignIn', sortDesc: false, sortByLabel: 'Last sign-in, ascending' }}
+      ${'oldest_sign_in'}    | ${{ sortBy: 'lastSignIn', sortDesc: true, sortByLabel: 'Last sign-in, descending' }}
+    `('when `sort` query string param is `$sortParam`', ({ sortParam, expected }) => {
+      it(`returns ${JSON.stringify(expected)}`, async () => {
+        window.location.search = `?sort=${sortParam}`;
+
+        expect(parseSortParam(['account', 'granted', 'expires', 'maxRole', 'lastSignIn'])).toEqual(
+          expected,
+        );
+      });
+    });
+  });
+
+  describe('buildSortUrl', () => {
+    beforeEach(() => {
+      delete window.location;
+      window.location = new URL(URL_HOST);
+    });
+
+    describe('when field passed in `sortBy` argument does not have `sort` key defined', () => {
+      it('returns an empty string', () => {
+        expect(
+          buildSortUrl({
+            sortBy: 'source',
+            sortDesc: false,
+            filteredSearchBarTokens: [],
+            filteredSearchBarSearchParam: 'search',
+          }),
+        ).toBe('');
+      });
+    });
+
+    describe('when there are no filter params set', () => {
+      it('sets `sort` param', () => {
+        expect(
+          buildSortUrl({
+            sortBy: 'account',
+            sortDesc: false,
+            filteredSearchBarTokens: [],
+            filteredSearchBarSearchParam: 'search',
+          }),
+        ).toBe(`${URL_HOST}?sort=name_asc`);
+      });
+    });
+
+    describe('when filter params are set', () => {
+      it('merges the `sort` param with the filter params', () => {
+        window.location.search = '?two_factor=enabled&with_inherited_permissions=exclude';
+
+        expect(
+          buildSortUrl({
+            sortBy: 'account',
+            sortDesc: false,
+            filteredSearchBarTokens: ['two_factor', 'with_inherited_permissions'],
+            filteredSearchBarSearchParam: 'search',
+          }),
+        ).toBe(`${URL_HOST}?two_factor=enabled&with_inherited_permissions=exclude&sort=name_asc`);
+      });
+    });
+
+    describe('when search param is set', () => {
+      it('merges the `sort` param with the search param', () => {
+        window.location.search = '?search=foobar';
+
+        expect(
+          buildSortUrl({
+            sortBy: 'account',
+            sortDesc: false,
+            filteredSearchBarTokens: ['two_factor', 'with_inherited_permissions'],
+            filteredSearchBarSearchParam: 'search',
+          }),
+        ).toBe(`${URL_HOST}?search=foobar&sort=name_asc`);
+      });
+    });
+  });
 });