Merge branch '292042-group-members-view-migrate-sorting-dropdown-to-glsorting' into 'master'

Switch to using GlSort for group members view

See merge request gitlab-org/gitlab!49527

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 { GlSorting, GlSortingItem } from '@gitlab/ui';
+import { visitUrl } from '~/lib/utils/url_utility';
+import { parseSortParam, buildSortHref } from '~/members/utils';
 import { FIELDS } from '~/members/constants';
 
 export default {
   name: 'SortDropdown',
-  components: { GlDropdown, GlDropdownItem, GlFormGroup },
+  components: { GlSorting, GlSortingItem },
   computed: {
     ...mapState(['tableSortableFields', 'filteredSearchBar']),
     sort() {
       return parseSortParam(this.tableSortableFields);
     },
+    activeOption() {
+      return FIELDS.find(field => field.key === this.sort.sortByKey);
+    },
+    activeOptionLabel() {
+      return this.activeOption?.label;
+    },
+    isAscending() {
+      return !this.sort.sortDesc;
+    },
     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).map(
+        field => ({
+          key: field.key,
+          label: field.label,
+          href: buildSortHref({
+            sortBy: field.key,
+            sortDesc: false,
+            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;
+    isActive(key) {
+      return this.activeOption.key === key;
+    },
+    handleSortDirectionChange() {
+      visitUrl(
+        buildSortHref({
+          sortBy: this.activeOption.key,
+          sortDesc: !this.sort.sortDesc,
+          filteredSearchBarTokens: this.filteredSearchBar.tokens,
+          filteredSearchBarSearchParam: this.filteredSearchBar.searchParam,
+        }),
+      );
     },
   },
 };
 </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-sorting
+    class="gl-display-flex"
+    dropdown-class="gl-w-full"
+    data-testid="members-sort-dropdown"
+    :text="activeOptionLabel"
+    :is-ascending="isAscending"
+    :sort-direction-tool-tip="__('Sort direction')"
+    @sortDirectionChange="handleSortDirectionChange"
   >
-    <gl-dropdown
-      :text="sort.sortByLabel"
-      block
-      toggle-class="gl-mb-0"
-      data-testid="members-sort-dropdown"
-      right
+    <gl-sorting-item
+      v-for="option in filteredOptions"
+      :key="option.key"
+      :href="option.href"
+      :active="isActive(option.key)"
     >
-      <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>
+      {{ option.label }}
+    </gl-sorting-item>
+  </gl-sorting>
 </template>

app/assets/javascripts/members/constants.js

-import { __, s__ } from '~/locale';
-
-const ACCOUNT_SORT_ASC_LABEL = s__('Members|Account, ascending');
+import { __ } from '~/locale';
 
 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'),
-      },
+      asc: 'name_asc',
+      desc: 'name_desc',
     },
   },
   {
@@ -29,14 +21,8 @@ export const FIELDS = [
     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'),
-      },
+      asc: 'last_joined',
+      desc: 'oldest_joined',
     },
   },
   {
@@ -63,14 +49,8 @@ export const FIELDS = [
     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'),
-      },
+      asc: 'access_level_asc',
+      desc: 'access_level_desc',
     },
   },
   {
@@ -81,15 +61,10 @@ export const FIELDS = [
   },
   {
     key: 'lastSignIn',
+    label: __('Last sign-in'),
     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'),
-      },
+      asc: 'recent_sign_in',
+      desc: 'oldest_sign_in',
     },
   },
   {
@@ -101,9 +76,8 @@ export const FIELDS = [
 ];
 
 export const DEFAULT_SORT = {
-  sortBy: 'account',
+  sortByKey: 'account',
   sortDesc: false,
-  sortByLabel: ACCOUNT_SORT_ASC_LABEL,
 };
 
 export const AVATAR_SIZE = 48;

app/assets/javascripts/members/utils.js

@@ -51,23 +51,20 @@ 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,
+    field => field.sort?.asc === sortParam || field.sort?.desc === 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,
+    sortByKey: sortedField.key,
+    sortDesc: sortedField?.sort?.desc === sortParam,
   };
 };
 
-export const buildSortUrl = ({
+export const buildSortHref = ({
   sortBy,
   sortDesc,
   filteredSearchBarTokens,
@@ -79,7 +76,7 @@ export const buildSortUrl = ({
     return '';
   }
 
-  const sortParam = sortDesc ? sortDefinition.desc.param : sortDefinition.asc.param;
+  const sortParam = sortDesc ? sortDefinition.desc : sortDefinition.asc;
 
   const filterParams =
     filteredSearchBarTokens?.reduce((accumulator, token) => {

locale/gitlab.pot

@@ -15995,6 +15995,9 @@ msgstr ""
 msgid "Last seen"
 msgstr ""
 
+msgid "Last sign-in"
+msgstr ""
+
 msgid "Last successful sync"
 msgstr ""
 
@@ -16969,18 +16972,6 @@ 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 ""
 
@@ -17044,21 +17035,9 @@ 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

@@ -17,14 +17,20 @@ RSpec.describe 'Groups > Members > Sort members', :js do
   end
 
   context 'when `group_members_filtered_search` feature flag is enabled' do
-    dropdown_toggle_selector = '[data-testid="members-sort-dropdown"] > button'
+    def expect_sort_by(text, sort_direction)
+      within('[data-testid="members-sort-dropdown"]') do
+        expect(page).to have_css('button[aria-haspopup="true"]', text: text)
+        expect(page).to have_button("Sorting Direction: #{sort_direction == :asc ? 'Ascending' : 'Descending'}")
+      end
+    end
 
-    it 'sorts account by default' do
+    it 'sorts by 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')
+
+      expect_sort_by('Account', :asc)
     end
 
     it 'sorts by max role ascending' do
@@ -32,7 +38,8 @@ RSpec.describe 'Groups > Members > Sort members', :js do
 
       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')
+
+      expect_sort_by('Max role', :asc)
     end
 
     it 'sorts by max role descending' do
@@ -40,7 +47,8 @@ RSpec.describe 'Groups > Members > Sort members', :js do
 
       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')
+
+      expect_sort_by('Max role', :desc)
     end
 
     it 'sorts by access granted ascending' do
@@ -48,7 +56,8 @@ RSpec.describe 'Groups > Members > Sort members', :js do
 
       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')
+
+      expect_sort_by('Access granted', :asc)
     end
 
     it 'sorts by access granted descending' do
@@ -56,7 +65,8 @@ RSpec.describe 'Groups > Members > Sort members', :js do
 
       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')
+
+      expect_sort_by('Access granted', :desc)
     end
 
     it 'sorts by account ascending' do
@@ -64,7 +74,8 @@ RSpec.describe 'Groups > Members > Sort members', :js do
 
       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')
+
+      expect_sort_by('Account', :asc)
     end
 
     it 'sorts by account descending' do
@@ -72,7 +83,8 @@ RSpec.describe 'Groups > Members > Sort members', :js do
 
       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')
+
+      expect_sort_by('Account', :desc)
     end
 
     it 'sorts by last sign-in ascending', :clean_gitlab_redis_shared_state do
@@ -80,7 +92,8 @@ RSpec.describe 'Groups > Members > Sort members', :js do
 
       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')
+
+      expect_sort_by('Last sign-in', :asc)
     end
 
     it 'sorts by last sign-in descending', :clean_gitlab_redis_shared_state do
@@ -88,7 +101,8 @@ RSpec.describe 'Groups > Members > Sort members', :js do
 
       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')
+
+      expect_sort_by('Last sign-in', :desc)
     end
   end
 

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 { GlSorting, GlSortingItem } from '@gitlab/ui';
 import SortDropdown from '~/members/components/filter_sort/sort_dropdown.vue';
+import * as urlUtilities from '~/lib/utils/url_utility';
 
 const localVue = createLocalVue();
 localVue.use(Vuex);
@@ -34,10 +34,13 @@ describe('SortDropdown', () => {
     });
   };
 
+  const findSortingComponent = () => wrapper.find(GlSorting);
+  const findSortDirectionToggle = () =>
+    findSortingComponent().find('button[title="Sort direction"]');
   const findDropdownToggle = () => wrapper.find('button[aria-haspopup="true"]');
   const findDropdownItemByText = text =>
     wrapper
-      .findAll(GlDropdownItem)
+      .findAll(GlSortingItem)
       .wrappers.find(dropdownItemWrapper => dropdownItemWrapper.text() === text);
 
   describe('dropdown options', () => {
@@ -54,37 +57,21 @@ describe('SortDropdown', () => {
 
       const expectedDropdownItems = [
         {
-          label: 'Account, ascending',
+          label: 'Account',
           url: `${EXPECTED_BASE_URL}name_asc`,
         },
         {
-          label: 'Account, descending',
-          url: `${EXPECTED_BASE_URL}name_desc`,
-        },
-        {
-          label: 'Access granted, ascending',
+          label: 'Access granted',
           url: `${EXPECTED_BASE_URL}last_joined`,
         },
         {
-          label: 'Access granted, descending',
-          url: `${EXPECTED_BASE_URL}oldest_joined`,
-        },
-        {
-          label: 'Max role, ascending',
+          label: 'Max role',
           url: `${EXPECTED_BASE_URL}access_level_asc`,
         },
         {
-          label: 'Max role, descending',
-          url: `${EXPECTED_BASE_URL}access_level_desc`,
-        },
-        {
-          label: 'Last sign-in, ascending',
+          label: 'Last sign-in',
           url: `${EXPECTED_BASE_URL}recent_sign_in`,
         },
-        {
-          label: 'Last sign-in, descending',
-          url: `${EXPECTED_BASE_URL}oldest_sign_in`,
-        },
       ];
 
       createComponent();
@@ -102,7 +89,7 @@ describe('SortDropdown', () => {
 
       createComponent();
 
-      expect(findDropdownItemByText('Max role, ascending').props('isChecked')).toBe(true);
+      expect(findDropdownItemByText('Max role').vm.$attrs.active).toBe(true);
     });
   });
 
@@ -112,10 +99,11 @@ describe('SortDropdown', () => {
       window.location = new URL(URL_HOST);
     });
 
-    it('defaults to sorting by "Account, ascending"', () => {
+    it('defaults to sorting by "Account" in ascending order', () => {
       createComponent();
 
-      expect(findDropdownToggle().text()).toBe('Account, ascending');
+      expect(findSortingComponent().props('isAscending')).toBe(true);
+      expect(findDropdownToggle().text()).toBe('Account');
     });
 
     it('sets text as selected sort option', () => {
@@ -123,13 +111,52 @@ describe('SortDropdown', () => {
 
       createComponent();
 
-      expect(findDropdownToggle().text()).toBe('Max role, ascending');
+      expect(findDropdownToggle().text()).toBe('Max role');
     });
   });
 
-  it('renders dropdown label', () => {
-    createComponent();
+  describe('sort direction toggle', () => {
+    beforeEach(() => {
+      delete window.location;
+      window.location = new URL(URL_HOST);
+
+      jest.spyOn(urlUtilities, 'visitUrl');
+    });
+
+    describe('when current sort direction is ascending', () => {
+      beforeEach(() => {
+        window.location.search = '?sort=access_level_asc';
+
+        createComponent();
+      });
+
+      describe('when sort direction toggle is clicked', () => {
+        beforeEach(() => {
+          findSortDirectionToggle().trigger('click');
+        });
+
+        it('sorts in descending order', () => {
+          expect(urlUtilities.visitUrl).toHaveBeenCalledWith(`${URL_HOST}?sort=access_level_desc`);
+        });
+      });
+    });
 
-    expect(within(wrapper.element).queryByText('Sort by')).not.toBe(null);
+    describe('when current sort direction is descending', () => {
+      beforeEach(() => {
+        window.location.search = '?sort=access_level_desc';
+
+        createComponent();
+      });
+
+      describe('when sort direction toggle is clicked', () => {
+        beforeEach(() => {
+          findSortDirectionToggle().trigger('click');
+        });
+
+        it('sorts in ascending order', () => {
+          expect(urlUtilities.visitUrl).toHaveBeenCalledWith(`${URL_HOST}?sort=access_level_asc`);
+        });
+      });
+    });
   });
 });

spec/frontend/members/utils_spec.js

@@ -8,7 +8,7 @@ import {
   canUpdate,
   canOverride,
   parseSortParam,
-  buildSortUrl,
+  buildSortHref,
 } from '~/members/utils';
 import { DEFAULT_SORT } from '~/members/constants';
 import { member as memberMock, group, invite } from './mock_data';
@@ -148,14 +148,14 @@ describe('Members Utils', () => {
 
     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' }}
+      ${'name_asc'}          | ${{ sortByKey: 'account', sortDesc: false }}
+      ${'name_desc'}         | ${{ sortByKey: 'account', sortDesc: true }}
+      ${'last_joined'}       | ${{ sortByKey: 'granted', sortDesc: false }}
+      ${'oldest_joined'}     | ${{ sortByKey: 'granted', sortDesc: true }}
+      ${'access_level_asc'}  | ${{ sortByKey: 'maxRole', sortDesc: false }}
+      ${'access_level_desc'} | ${{ sortByKey: 'maxRole', sortDesc: true }}
+      ${'recent_sign_in'}    | ${{ sortByKey: 'lastSignIn', sortDesc: false }}
+      ${'oldest_sign_in'}    | ${{ sortByKey: 'lastSignIn', sortDesc: true }}
     `('when `sort` query string param is `$sortParam`', ({ sortParam, expected }) => {
       it(`returns ${JSON.stringify(expected)}`, async () => {
         window.location.search = `?sort=${sortParam}`;
@@ -167,7 +167,7 @@ describe('Members Utils', () => {
     });
   });
 
-  describe('buildSortUrl', () => {
+  describe('buildSortHref', () => {
     beforeEach(() => {
       delete window.location;
       window.location = new URL(URL_HOST);
@@ -176,7 +176,7 @@ describe('Members Utils', () => {
     describe('when field passed in `sortBy` argument does not have `sort` key defined', () => {
       it('returns an empty string', () => {
         expect(
-          buildSortUrl({
+          buildSortHref({
             sortBy: 'source',
             sortDesc: false,
             filteredSearchBarTokens: [],
@@ -189,7 +189,7 @@ describe('Members Utils', () => {
     describe('when there are no filter params set', () => {
       it('sets `sort` param', () => {
         expect(
-          buildSortUrl({
+          buildSortHref({
             sortBy: 'account',
             sortDesc: false,
             filteredSearchBarTokens: [],
@@ -204,7 +204,7 @@ describe('Members Utils', () => {
         window.location.search = '?two_factor=enabled&with_inherited_permissions=exclude';
 
         expect(
-          buildSortUrl({
+          buildSortHref({
             sortBy: 'account',
             sortDesc: false,
             filteredSearchBarTokens: ['two_factor', 'with_inherited_permissions'],
@@ -219,7 +219,7 @@ describe('Members Utils', () => {
         window.location.search = '?search=foobar';
 
         expect(
-          buildSortUrl({
+          buildSortHref({
             sortBy: 'account',
             sortDesc: false,
             filteredSearchBarTokens: ['two_factor', 'with_inherited_permissions'],