Merge branch '273328-global-search-sort-ux' into 'master'

Global Search - UX Refresh for Sort Button

See merge request gitlab-org/gitlab!52387

app/assets/javascripts/search/index.js

@@ -5,6 +5,7 @@ import { queryToObject } from '~/lib/utils/url_utility';
 import createStore from './store';
 import { initTopbar } from './topbar';
 import { initSidebar } from './sidebar';
+import { initSearchSort } from './sort';
 
 export const initSearchApp = () => {
   // Similar to url_utility.decodeUrlParameter
@@ -16,6 +17,7 @@ export const initSearchApp = () => {
 
   initTopbar(store);
   initSidebar(store);
+  initSearchSort(store);
 
   setHighlightClass(query.search); // Code Highlighting
   refreshCounts(); // Other Scope Tab Counts

app/assets/javascripts/search/sort/components/app.vue

+<script>
+import { mapState, mapActions } from 'vuex';
+import {
+  GlButtonGroup,
+  GlButton,
+  GlDropdown,
+  GlDropdownItem,
+  GlTooltipDirective,
+} from '@gitlab/ui';
+import { SORT_DIRECTION_UI } from '../constants';
+
+export default {
+  name: 'GlobalSearchSort',
+  components: {
+    GlButtonGroup,
+    GlButton,
+    GlDropdown,
+    GlDropdownItem,
+  },
+  directives: {
+    GlTooltip: GlTooltipDirective,
+  },
+  props: {
+    searchSortOptions: {
+      type: Array,
+      required: true,
+    },
+  },
+  computed: {
+    ...mapState(['query']),
+    selectedSortOption: {
+      get() {
+        const { sort } = this.query;
+
+        if (!sort) {
+          return this.searchSortOptions[0];
+        }
+
+        const sortOption = this.searchSortOptions.find((option) => {
+          if (!option.sortable) {
+            return option.sortParam === sort;
+          }
+
+          return Object.values(option.sortParam).indexOf(sort) !== -1;
+        });
+
+        // Handle invalid sort param
+        return sortOption || this.searchSortOptions[0];
+      },
+      set(value) {
+        this.setQuery({ key: 'sort', value });
+        this.applyQuery();
+      },
+    },
+    sortDirectionData() {
+      if (!this.selectedSortOption.sortable) {
+        return SORT_DIRECTION_UI.disabled;
+      }
+
+      return this.query?.sort?.includes('asc') ? SORT_DIRECTION_UI.asc : SORT_DIRECTION_UI.desc;
+    },
+  },
+  methods: {
+    ...mapActions(['applyQuery', 'setQuery']),
+    handleSortChange(option) {
+      if (!option.sortable) {
+        this.selectedSortOption = option.sortParam;
+      } else {
+        // Default new sort options to desc
+        this.selectedSortOption = option.sortParam.desc;
+      }
+    },
+    handleSortDirectionChange() {
+      this.selectedSortOption =
+        this.sortDirectionData.direction === 'desc'
+          ? this.selectedSortOption.sortParam.asc
+          : this.selectedSortOption.sortParam.desc;
+    },
+  },
+};
+</script>
+
+<template>
+  <gl-button-group>
+    <gl-dropdown :text="selectedSortOption.title" :right="true" class="w-100">
+      <gl-dropdown-item
+        v-for="sortOption in searchSortOptions"
+        :key="sortOption.title"
+        is-check-item
+        :is-checked="sortOption.title === selectedSortOption.title"
+        @click="handleSortChange(sortOption)"
+        >{{ sortOption.title }}</gl-dropdown-item
+      >
+    </gl-dropdown>
+    <gl-button
+      v-gl-tooltip
+      :disabled="!selectedSortOption.sortable"
+      :title="sortDirectionData.tooltip"
+      :icon="sortDirectionData.icon"
+      @click="handleSortDirectionChange"
+    />
+  </gl-button-group>
+</template>

app/assets/javascripts/search/sort/constants.js

+import { __ } from '~/locale';
+
+export const SORT_DIRECTION_UI = {
+  disabled: {
+    direction: null,
+    tooltip: '',
+    icon: 'sort-highest',
+  },
+  desc: {
+    direction: 'desc',
+    tooltip: __('Sort direction: Descending'),
+    icon: 'sort-highest',
+  },
+  asc: {
+    direction: 'asc',
+    tooltip: __('Sort direction: Ascending'),
+    icon: 'sort-lowest',
+  },
+};

app/assets/javascripts/search/sort/index.js

+import Vue from 'vue';
+import Translate from '~/vue_shared/translate';
+import GlobalSearchSort from './components/app.vue';
+
+Vue.use(Translate);
+
+export const initSearchSort = (store) => {
+  const el = document.getElementById('js-search-sort');
+
+  if (!el) return false;
+
+  let { searchSortOptions } = el.dataset;
+
+  searchSortOptions = JSON.parse(searchSortOptions);
+
+  return new Vue({
+    el,
+    store,
+    render(createElement) {
+      return createElement(GlobalSearchSort, {
+        props: {
+          searchSortOptions,
+        },
+      });
+    },
+  });
+};

app/helpers/search_helper.rb

@@ -129,6 +129,18 @@ module SearchHelper
     @search_service ||= ::SearchService.new(current_user, params.merge(confidential: Gitlab::Utils.to_boolean(params[:confidential])))
   end
 
+  def search_sort_options
+    options = []
+    options << {
+      title: _('Created date'),
+      sortable: true,
+      sortParam: {
+        asc: 'created_asc',
+        desc: 'created_desc'
+      }
+    }
+  end
+
   private
 
   # Autocomplete results for various settings pages

app/helpers/sorting_helper.rb

@@ -30,8 +30,7 @@ module SortingHelper
       sort_value_contacted_date    => sort_title_contacted_date,
       sort_value_relative_position => sort_title_relative_position,
       sort_value_size              => sort_title_size,
-      sort_value_expire_date       => sort_title_expire_date,
-      sort_value_relevant          => sort_title_relevant
+      sort_value_expire_date       => sort_title_expire_date
     }
   end
 
@@ -85,13 +84,6 @@ module SortingHelper
     }
   end
 
-  def search_reverse_sort_options_hash
-    {
-      sort_value_recently_created => sort_value_oldest_created,
-      sort_value_oldest_created   => sort_value_recently_created
-    }
-  end
-
   def groups_sort_options_hash
     {
       sort_value_name             => sort_title_name,
@@ -229,10 +221,6 @@ module SortingHelper
     sort_options_hash[sort_value]
   end
 
-  def search_sort_option_title(sort_value)
-    sort_options_hash[sort_value]
-  end
-
   def sort_direction_icon(sort_value)
     case sort_value
     when sort_value_milestone, sort_value_due_date, /_asc\z/
@@ -271,13 +259,6 @@ module SortingHelper
     sort_direction_button(url, reverse_sort, sort_value)
   end
 
-  def search_sort_direction_button(sort_value)
-    reverse_sort = search_reverse_sort_options_hash[sort_value]
-    url = page_filter_path(sort: reverse_sort)
-
-    sort_direction_button(url, reverse_sort, sort_value)
-  end
-
   def packages_sort_options_hash
     {
       sort_value_recently_created  => sort_title_created_date,

app/helpers/sorting_titles_values_helper.rb

@@ -166,10 +166,6 @@ module SortingTitlesValuesHelper
     s_('SortOptions|Expired date')
   end
 
-  def sort_title_relevant
-    s_('SortOptions|Relevant')
-  end
-
   # Values.
   def sort_value_access_level_asc
     'access_level_asc'
@@ -330,10 +326,6 @@ module SortingTitlesValuesHelper
   def sort_value_expire_date
     'expired_asc'
   end
-
-  def sort_value_relevant
-    'relevant'
-  end
 end
 
 SortingHelper.include_if_ee('::EE::SortingTitlesValuesHelper')

app/views/search/_results_status.html.haml

@@ -22,4 +22,4 @@
           = _("in group %{link_to_group}").html_safe % { link_to_group: link_to_group }
     - if search_service.show_sort_dropdown?
       .gl-md-display-flex.gl-flex-direction-column
-        = render partial: 'search/sort_dropdown'
+        #js-search-sort{ data: { "search-sort-options" => search_sort_options.to_json } }

app/views/search/_sort_dropdown.html.haml

-- sort_value = @sort
-- sort_title = search_sort_option_title(sort_value)
-
-.dropdown.gl-display-inline-block.gl-ml-3.filter-dropdown-container
-  .btn-group{ role: 'group' }
-    .btn-group{ role: 'group' }
-    %button.dropdown-menu-toggle{ type: 'button', data: { toggle: 'dropdown', display: 'static' }, class: 'btn btn-default' }
-      = sort_title
-      = sprite_icon('chevron-down', css_class: 'dropdown-menu-toggle-icon gl-top-3')
-    %ul.dropdown-menu.dropdown-menu-right.dropdown-menu-selectable.dropdown-menu-sort
-      %li
-        = render_if_exists('search/sort_by_relevancy', sort_title: sort_title)
-        = sortable_item(sort_title_recently_created, page_filter_path(sort: sort_value_recently_created), sort_title)
-    = search_sort_direction_button(sort_value)

changelogs/unreleased/273328-global-search-sort-ux.yml

+---
+title: Global Search - UX Refresh for Sort Button
+merge_request: 52387
+author:
+type: changed

ee/app/helpers/ee/search_helper.rb

@@ -116,6 +116,20 @@ module EE
       tag.div(message.html_safe, data: { testid: 'es-status-marker', enabled: enabled })
     end
 
+    override :search_sort_options
+    def search_sort_options
+      options = []
+      if search_service.use_elasticsearch?
+        options << {
+          title: _('Most relevant'),
+          sortable: false,
+          sortParam: 'relevant'
+        }
+      end
+
+      options + super
+    end
+
     private
 
     def recent_epics_autocomplete(term)

ee/app/views/search/_sort_by_relevancy.haml

-- if search_service.use_elasticsearch?
-  = sortable_item(sort_title_relevant, page_filter_path(sort: sort_value_relevant), sort_title)

ee/spec/helpers/search_helper_spec.rb

@@ -247,4 +247,43 @@ RSpec.describe SearchHelper do
       end
     end
   end
+
+  describe '#search_sort_options_json' do
+    let(:user) { create(:user) }
+
+    mock_relevant_sort = {
+      title: _('Most relevant'),
+      sortable: false,
+      sortParam: 'relevant'
+    }
+
+    mock_created_sort = {
+      title: _('Created date'),
+      sortable: true,
+      sortParam: {
+        asc: 'created_asc',
+        desc: 'created_desc'
+      }
+    }
+
+    before do
+      allow(self).to receive(:current_user).and_return(user)
+    end
+
+    context 'with advanced search enabled' do
+      before do
+        stub_ee_application_setting(search_using_elasticsearch: true)
+      end
+
+      it 'returns the correct data' do
+        expect(search_sort_options).to eq([mock_relevant_sort, mock_created_sort])
+      end
+    end
+
+    context 'with basic search enabled' do
+      it 'returns the correct data' do
+        expect(search_sort_options).to eq([mock_created_sort])
+      end
+    end
+  end
 end

ee/spec/views/search/_sort_dropdown.html.haml_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe 'search/_sort_dropdown' do
-  context 'when the search page is opened' do
-    before do
-      @scope = 'issues'
-    end
-
-    context 'with advanced search' do
-      before do
-        @search_service = instance_double(SearchService, use_elasticsearch?: true)
-      end
-
-      it 'displays the correct sort elements' do
-        render
-
-        expect(rendered).to have_selector('a', text: 'Relevant')
-        expect(rendered).to have_selector('a', text: 'Last created')
-      end
-    end
-
-    context 'without advanced search' do
-      before do
-        @search_service = instance_double(SearchService, use_elasticsearch?: false)
-      end
-
-      it 'displays the correct sort elements' do
-        render
-
-        expect(rendered).not_to have_selector('a', text: 'Relevant')
-        expect(rendered).to have_selector('a', text: 'Last created')
-      end
-    end
-  end
-end

locale/gitlab.pot

@@ -19071,6 +19071,9 @@ msgstr ""
 msgid "More than %{number_commits_distance} commits different with %{default_branch}"
 msgstr ""
 
+msgid "Most relevant"
+msgstr ""
+
 msgid "Most stars"
 msgstr ""
 
@@ -27287,9 +27290,6 @@ msgstr ""
 msgid "SortOptions|Recently starred"
 msgstr ""
 
-msgid "SortOptions|Relevant"
-msgstr ""
-
 msgid "SortOptions|Size"
 msgstr ""
 

spec/features/search/user_searches_for_issues_spec.rb

@@ -75,7 +75,7 @@ RSpec.describe 'User searches for issues', :js do
         expect(page.all('.search-result-row').last).to have_link(issue1.title)
       end
 
-      find('.reverse-sort-btn').click
+      find('[data-testid="sort-highest-icon"]').click
 
       page.within('.results') do
         expect(page.all('.search-result-row').first).to have_link(issue1.title)

spec/features/search/user_searches_for_merge_requests_spec.rb

@@ -5,8 +5,14 @@ require 'spec_helper'
 RSpec.describe 'User searches for merge requests', :js do
   let(:user) { create(:user) }
   let(:project) { create(:project, namespace: user.namespace) }
-  let!(:merge_request1) { create(:merge_request, title: 'Foo', source_project: project, target_project: project) }
-  let!(:merge_request2) { create(:merge_request, :simple, title: 'Bar', source_project: project, target_project: project) }
+  let!(:merge_request1) { create(:merge_request, title: 'Merge Request Foo', source_project: project, target_project: project, created_at: 1.hour.ago) }
+  let!(:merge_request2) { create(:merge_request, :simple, title: 'Merge Request Bar', source_project: project, target_project: project) }
+
+  def search_for_mr(search)
+    fill_in('dashboard_search', with: search)
+    find('.btn-search').click
+    select_search_scope('Merge requests')
+  end
 
   before do
     project.add_maintainer(user)
@@ -18,9 +24,7 @@ RSpec.describe 'User searches for merge requests', :js do
   include_examples 'top right search form'
 
   it 'finds a merge request' do
-    fill_in('dashboard_search', with: merge_request1.title)
-    find('.btn-search').click
-    select_search_scope('Merge requests')
+    search_for_mr(merge_request1.title)
 
     page.within('.results') do
       expect(page).to have_link(merge_request1.title)
@@ -28,6 +32,22 @@ RSpec.describe 'User searches for merge requests', :js do
     end
   end
 
+  it 'sorts by created date' do
+    search_for_mr('Merge Request')
+
+    page.within('.results') do
+      expect(page.all('.search-result-row').first).to have_link(merge_request2.title)
+      expect(page.all('.search-result-row').last).to have_link(merge_request1.title)
+    end
+
+    find('[data-testid="sort-highest-icon"]').click
+
+    page.within('.results') do
+      expect(page.all('.search-result-row').first).to have_link(merge_request1.title)
+      expect(page.all('.search-result-row').last).to have_link(merge_request2.title)
+    end
+  end
+
   context 'when on a project page' do
     it 'finds a merge request' do
       find('[data-testid="project-filter"]').click
@@ -38,9 +58,7 @@ RSpec.describe 'User searches for merge requests', :js do
         click_on(project.full_name)
       end
 
-      fill_in('dashboard_search', with: merge_request1.title)
-      find('.btn-search').click
-      select_search_scope('Merge requests')
+      search_for_mr(merge_request1.title)
 
       page.within('.results') do
         expect(page).to have_link(merge_request1.title)

spec/frontend/search/mock_data.js

@@ -45,3 +45,19 @@ export const MOCK_PROJECTS = [
     id: 'test_2',
   },
 ];
+
+export const MOCK_SORT_OPTIONS = [
+  {
+    title: 'Most relevant',
+    sortable: false,
+    sortParam: 'relevant',
+  },
+  {
+    title: 'Created date',
+    sortable: true,
+    sortParam: {
+      asc: 'created_asc',
+      desc: 'created_desc',
+    },
+  },
+];

spec/frontend/search/sort/components/app_spec.js

+import Vuex from 'vuex';
+import { createLocalVue, shallowMount } from '@vue/test-utils';
+import { GlButtonGroup, GlButton, GlDropdown, GlDropdownItem } from '@gitlab/ui';
+import { MOCK_QUERY, MOCK_SORT_OPTIONS } from 'jest/search/mock_data';
+import GlobalSearchSort from '~/search/sort/components/app.vue';
+import { SORT_DIRECTION_UI } from '~/search/sort/constants';
+
+const localVue = createLocalVue();
+localVue.use(Vuex);
+
+describe('GlobalSearchSort', () => {
+  let wrapper;
+
+  const actionSpies = {
+    setQuery: jest.fn(),
+    applyQuery: jest.fn(),
+  };
+
+  const defaultProps = {
+    searchSortOptions: MOCK_SORT_OPTIONS,
+  };
+
+  const createComponent = (initialState, props) => {
+    const store = new Vuex.Store({
+      state: {
+        query: MOCK_QUERY,
+        ...initialState,
+      },
+      actions: actionSpies,
+    });
+
+    wrapper = shallowMount(GlobalSearchSort, {
+      localVue,
+      store,
+      propsData: {
+        ...defaultProps,
+        ...props,
+      },
+    });
+  };
+
+  afterEach(() => {
+    wrapper.destroy();
+    wrapper = null;
+  });
+
+  const findSortButtonGroup = () => wrapper.find(GlButtonGroup);
+  const findSortDropdown = () => wrapper.find(GlDropdown);
+  const findSortDirectionButton = () => wrapper.find(GlButton);
+  const findDropdownItems = () => findSortDropdown().findAll(GlDropdownItem);
+  const findDropdownItemsText = () => findDropdownItems().wrappers.map((w) => w.text());
+
+  describe('template', () => {
+    beforeEach(() => {
+      createComponent();
+    });
+
+    it('renders Sort Button Group', () => {
+      expect(findSortButtonGroup().exists()).toBe(true);
+    });
+
+    it('renders Sort Dropdown', () => {
+      expect(findSortDropdown().exists()).toBe(true);
+    });
+
+    it('renders Sort Direction Button', () => {
+      expect(findSortDirectionButton().exists()).toBe(true);
+    });
+  });
+
+  describe('Sort Dropdown Items', () => {
+    describe('renders', () => {
+      beforeEach(() => {
+        createComponent();
+      });
+
+      it('an instance for each namespace', () => {
+        expect(findDropdownItemsText()).toStrictEqual(
+          MOCK_SORT_OPTIONS.map((option) => option.title),
+        );
+      });
+    });
+
+    describe.each`
+      sortQuery                              | value
+      ${null}                                | ${MOCK_SORT_OPTIONS[0].title}
+      ${'asdf'}                              | ${MOCK_SORT_OPTIONS[0].title}
+      ${MOCK_SORT_OPTIONS[0].sortParam}      | ${MOCK_SORT_OPTIONS[0].title}
+      ${MOCK_SORT_OPTIONS[1].sortParam.desc} | ${MOCK_SORT_OPTIONS[1].title}
+      ${MOCK_SORT_OPTIONS[1].sortParam.asc}  | ${MOCK_SORT_OPTIONS[1].title}
+    `('selected', ({ sortQuery, value }) => {
+      describe(`when sort option is ${sortQuery}`, () => {
+        beforeEach(() => {
+          createComponent({ query: { sort: sortQuery } });
+        });
+
+        it('is set correctly', () => {
+          expect(findSortDropdown().attributes('text')).toBe(value);
+        });
+      });
+    });
+  });
+
+  describe.each`
+    description              | sortQuery                              | sortUi                        | disabled
+    ${'non-sortable'}        | ${MOCK_SORT_OPTIONS[0].sortParam}      | ${SORT_DIRECTION_UI.disabled} | ${'true'}
+    ${'descending sortable'} | ${MOCK_SORT_OPTIONS[1].sortParam.desc} | ${SORT_DIRECTION_UI.desc}     | ${undefined}
+    ${'ascending sortable'}  | ${MOCK_SORT_OPTIONS[1].sortParam.asc}  | ${SORT_DIRECTION_UI.asc}      | ${undefined}
+  `('Sort Direction Button', ({ description, sortQuery, sortUi, disabled }) => {
+    describe(`when sort option is ${description}`, () => {
+      beforeEach(() => {
+        createComponent({ query: { sort: sortQuery } });
+      });
+
+      it('sets the UI correctly', () => {
+        expect(findSortDirectionButton().attributes('disabled')).toBe(disabled);
+        expect(findSortDirectionButton().attributes('title')).toBe(sortUi.tooltip);
+        expect(findSortDirectionButton().attributes('icon')).toBe(sortUi.icon);
+      });
+    });
+  });
+
+  describe('actions', () => {
+    describe.each`
+      description       | index | value
+      ${'non-sortable'} | ${0}  | ${MOCK_SORT_OPTIONS[0].sortParam}
+      ${'sortable'}     | ${1}  | ${MOCK_SORT_OPTIONS[1].sortParam.desc}
+    `('handleSortChange', ({ description, index, value }) => {
+      describe(`when clicking a ${description} option`, () => {
+        beforeEach(() => {
+          createComponent();
+          findDropdownItems().at(index).vm.$emit('click');
+        });
+
+        it('calls setQuery and applyQuery correctly', () => {
+          expect(actionSpies.setQuery).toHaveBeenCalledTimes(1);
+          expect(actionSpies.applyQuery).toHaveBeenCalledTimes(1);
+          expect(actionSpies.setQuery).toHaveBeenCalledWith(expect.any(Object), {
+            key: 'sort',
+            value,
+          });
+        });
+      });
+    });
+
+    describe.each`
+      description     | sortQuery                              | value
+      ${'descending'} | ${MOCK_SORT_OPTIONS[1].sortParam.desc} | ${MOCK_SORT_OPTIONS[1].sortParam.asc}
+      ${'ascending'}  | ${MOCK_SORT_OPTIONS[1].sortParam.asc}  | ${MOCK_SORT_OPTIONS[1].sortParam.desc}
+    `('handleSortDirectionChange', ({ description, sortQuery, value }) => {
+      describe(`when toggling a ${description} option`, () => {
+        beforeEach(() => {
+          createComponent({ query: { sort: sortQuery } });
+          findSortDirectionButton().vm.$emit('click');
+        });
+
+        it('calls setQuery and applyQuery correctly', () => {
+          expect(actionSpies.setQuery).toHaveBeenCalledTimes(1);
+          expect(actionSpies.applyQuery).toHaveBeenCalledTimes(1);
+          expect(actionSpies.setQuery).toHaveBeenCalledWith(expect.any(Object), {
+            key: 'sort',
+            value,
+          });
+        });
+      });
+    });
+  });
+});

spec/helpers/search_helper_spec.rb

@@ -610,4 +610,25 @@ RSpec.describe SearchHelper do
       end
     end
   end
+
+  describe '#search_sort_options' do
+    let(:user) { create(:user) }
+
+    mock_created_sort = {
+      title: _('Created date'),
+      sortable: true,
+      sortParam: {
+        asc: 'created_asc',
+        desc: 'created_desc'
+      }
+    }
+
+    before do
+      allow(self).to receive(:current_user).and_return(user)
+    end
+
+    it 'returns the correct data' do
+      expect(search_sort_options).to eq([mock_created_sort])
+    end
+  end
 end

spec/helpers/sorting_helper_spec.rb

@@ -50,24 +50,6 @@ RSpec.describe SortingHelper do
     end
   end
 
-  describe '#search_sort_direction_button' do
-    before do
-      set_sorting_url 'test_label'
-    end
-
-    it 'keeps label filter param' do
-      expect(search_sort_direction_button('created_asc')).to include('label_name=test_label')
-    end
-
-    it 'returns icon with sort-lowest when sort is asc' do
-      expect(search_sort_direction_button('created_asc')).to include('sort-lowest')
-    end
-
-    it 'returns icon with sort-highest when sort is desc' do
-      expect(search_sort_direction_button('created_desc')).to include('sort-highest')
-    end
-  end
-
   def stub_controller_path(value)
     allow(helper.controller).to receive(:controller_path).and_return(value)
   end