Merge branch '322755-fix-sort-parameter-in-issues-page-refactor' into 'master'

Fix sort params in issues page refactor

See merge request gitlab-org/gitlab!61369

app/assets/javascripts/issues_list/components/issues_list_app.vue

@@ -16,12 +16,17 @@ import IssuableByEmail from '~/issuable/components/issuable_by_email.vue';
 import IssuableList from '~/issuable_list/components/issuable_list_root.vue';
 import { IssuableListTabs, IssuableStates } from '~/issuable_list/constants';
 import {
+  apiSortParams,
   CREATED_DESC,
   i18n,
   MAX_LIST_SIZE,
   PAGE_SIZE,
-  RELATIVE_POSITION_ASC,
-  sortParams,
+  PARAM_PAGE,
+  PARAM_SORT,
+  PARAM_STATE,
+  RELATIVE_POSITION_DESC,
+  UPDATED_DESC,
+  urlSortParams,
 } from '~/issues_list/constants';
 import {
   convertToApiParams,
@@ -46,11 +51,8 @@ import eventHub from '../eventhub';
 import IssueCardTimeInfo from './issue_card_time_info.vue';
 
 export default {
-  CREATED_DESC,
   i18n,
   IssuableListTabs,
-  PAGE_SIZE,
-  sortParams,
   components: {
     CsvImportExportButtons,
     GlButton,
@@ -138,18 +140,18 @@ export default {
     },
   },
   data() {
-    const orderBy = getParameterByName('order_by');
-    const sort = getParameterByName('sort');
+    const state = getParameterByName(PARAM_STATE);
+    const defaultSortKey = state === IssuableStates.Closed ? UPDATED_DESC : CREATED_DESC;
 
     return {
       exportCsvPathWithQuery: this.getExportCsvPathWithQuery(),
       filterTokens: getFilterTokens(window.location.search),
       isLoading: false,
       issues: [],
-      page: toNumber(getParameterByName('page')) || 1,
+      page: toNumber(getParameterByName(PARAM_PAGE)) || 1,
       showBulkEditSidebar: false,
-      sortKey: getSortKey(orderBy, sort) || CREATED_DESC,
-      state: getParameterByName('state') || IssuableStates.Opened,
+      sortKey: getSortKey(getParameterByName(PARAM_SORT)) || defaultSortKey,
+      state: state || IssuableStates.Opened,
       totalIssues: 0,
     };
   },
@@ -158,7 +160,7 @@ export default {
       return this.showBulkEditSidebar || !this.issues.length;
     },
     isManualOrdering() {
-      return this.sortKey === RELATIVE_POSITION_ASC;
+      return this.sortKey === RELATIVE_POSITION_DESC;
     },
     isOpenTab() {
       return this.state === IssuableStates.Opened;
@@ -288,7 +290,7 @@ export default {
         page: this.page,
         search: this.searchQuery,
         state: this.state,
-        ...sortParams[this.sortKey],
+        ...urlSortParams[this.sortKey],
         ...this.urlFilterParams,
       };
     },
@@ -354,11 +356,11 @@ export default {
         .get(this.endpoint, {
           params: {
             page: this.page,
-            per_page: this.$options.PAGE_SIZE,
+            per_page: PAGE_SIZE,
             search: this.searchQuery,
             state: this.state,
             with_labels_details: true,
-            ...sortParams[this.sortKey],
+            ...apiSortParams[this.sortKey],
             ...this.apiFilterParams,
           },
         })

app/assets/javascripts/issues_list/constants.js

@@ -88,21 +88,22 @@ export const i18n = {
 
 export const JIRA_IMPORT_SUCCESS_ALERT_HIDE_MAP_KEY = 'jira-import-success-alert-hide-map';
 
-export const BLOCKING_ISSUES_ASC = 'BLOCKING_ISSUES_ASC';
+export const PARAM_PAGE = 'page';
+export const PARAM_SORT = 'sort';
+export const PARAM_STATE = 'state';
+
 export const BLOCKING_ISSUES_DESC = 'BLOCKING_ISSUES_DESC';
 export const CREATED_ASC = 'CREATED_ASC';
 export const CREATED_DESC = 'CREATED_DESC';
 export const DUE_DATE_ASC = 'DUE_DATE_ASC';
 export const DUE_DATE_DESC = 'DUE_DATE_DESC';
-export const LABEL_PRIORITY_ASC = 'LABEL_PRIORITY_ASC';
 export const LABEL_PRIORITY_DESC = 'LABEL_PRIORITY_DESC';
 export const MILESTONE_DUE_ASC = 'MILESTONE_DUE_ASC';
 export const MILESTONE_DUE_DESC = 'MILESTONE_DUE_DESC';
 export const POPULARITY_ASC = 'POPULARITY_ASC';
 export const POPULARITY_DESC = 'POPULARITY_DESC';
-export const PRIORITY_ASC = 'PRIORITY_ASC';
 export const PRIORITY_DESC = 'PRIORITY_DESC';
-export const RELATIVE_POSITION_ASC = 'RELATIVE_POSITION_ASC';
+export const RELATIVE_POSITION_DESC = 'RELATIVE_POSITION_DESC';
 export const UPDATED_ASC = 'UPDATED_ASC';
 export const UPDATED_DESC = 'UPDATED_DESC';
 export const WEIGHT_ASC = 'WEIGHT_ASC';
@@ -111,13 +112,19 @@ export const WEIGHT_DESC = 'WEIGHT_DESC';
 const SORT_ASC = 'asc';
 const SORT_DESC = 'desc';
 
+const CREATED_DATE_SORT = 'created_date';
+const CREATED_ASC_SORT = 'created_asc';
+const UPDATED_DESC_SORT = 'updated_desc';
+const UPDATED_ASC_SORT = 'updated_asc';
+const MILESTONE_SORT = 'milestone';
+const MILESTONE_DUE_DESC_SORT = 'milestone_due_desc';
+const DUE_DATE_DESC_SORT = 'due_date_desc';
+const POPULARITY_ASC_SORT = 'popularity_asc';
+const WEIGHT_DESC_SORT = 'weight_desc';
+const BLOCKING_ISSUES_DESC_SORT = 'blocking_issues_desc';
 const BLOCKING_ISSUES = 'blocking_issues';
 
-export const sortParams = {
-  [PRIORITY_ASC]: {
-    order_by: PRIORITY,
-    sort: SORT_ASC,
-  },
+export const apiSortParams = {
   [PRIORITY_DESC]: {
     order_by: PRIORITY,
     sort: SORT_DESC,
@@ -162,15 +169,11 @@ export const sortParams = {
     order_by: POPULARITY,
     sort: SORT_DESC,
   },
-  [LABEL_PRIORITY_ASC]: {
-    order_by: LABEL_PRIORITY,
-    sort: SORT_ASC,
-  },
   [LABEL_PRIORITY_DESC]: {
     order_by: LABEL_PRIORITY,
     sort: SORT_DESC,
   },
-  [RELATIVE_POSITION_ASC]: {
+  [RELATIVE_POSITION_DESC]: {
     order_by: RELATIVE_POSITION,
     per_page: 100,
     sort: SORT_ASC,
@@ -183,16 +186,64 @@ export const sortParams = {
     order_by: WEIGHT,
     sort: SORT_DESC,
   },
-  [BLOCKING_ISSUES_ASC]: {
-    order_by: BLOCKING_ISSUES,
-    sort: SORT_ASC,
-  },
   [BLOCKING_ISSUES_DESC]: {
     order_by: BLOCKING_ISSUES,
     sort: SORT_DESC,
   },
 };
 
+export const urlSortParams = {
+  [PRIORITY_DESC]: {
+    sort: PRIORITY,
+  },
+  [CREATED_ASC]: {
+    sort: CREATED_ASC_SORT,
+  },
+  [CREATED_DESC]: {
+    sort: CREATED_DATE_SORT,
+  },
+  [UPDATED_ASC]: {
+    sort: UPDATED_ASC_SORT,
+  },
+  [UPDATED_DESC]: {
+    sort: UPDATED_DESC_SORT,
+  },
+  [MILESTONE_DUE_ASC]: {
+    sort: MILESTONE_SORT,
+  },
+  [MILESTONE_DUE_DESC]: {
+    sort: MILESTONE_DUE_DESC_SORT,
+  },
+  [DUE_DATE_ASC]: {
+    sort: DUE_DATE,
+  },
+  [DUE_DATE_DESC]: {
+    sort: DUE_DATE_DESC_SORT,
+  },
+  [POPULARITY_ASC]: {
+    sort: POPULARITY_ASC_SORT,
+  },
+  [POPULARITY_DESC]: {
+    sort: POPULARITY,
+  },
+  [LABEL_PRIORITY_DESC]: {
+    sort: LABEL_PRIORITY,
+  },
+  [RELATIVE_POSITION_DESC]: {
+    sort: RELATIVE_POSITION,
+    per_page: 100,
+  },
+  [WEIGHT_ASC]: {
+    sort: WEIGHT,
+  },
+  [WEIGHT_DESC]: {
+    sort: WEIGHT_DESC_SORT,
+  },
+  [BLOCKING_ISSUES_DESC]: {
+    sort: BLOCKING_ISSUES_DESC_SORT,
+  },
+};
+
 export const MAX_LIST_SIZE = 10;
 
 export const FILTERED_SEARCH_TERM = 'filtered-search-term';

app/assets/javascripts/issues_list/utils.js

 import {
-  BLOCKING_ISSUES_ASC,
   BLOCKING_ISSUES_DESC,
   CREATED_ASC,
   CREATED_DESC,
@@ -7,30 +6,26 @@ import {
   DUE_DATE_DESC,
   FILTERED_SEARCH_TERM,
   filters,
-  LABEL_PRIORITY_ASC,
   LABEL_PRIORITY_DESC,
   MILESTONE_DUE_ASC,
   MILESTONE_DUE_DESC,
   NORMAL_FILTER,
   POPULARITY_ASC,
   POPULARITY_DESC,
-  PRIORITY_ASC,
   PRIORITY_DESC,
-  RELATIVE_POSITION_ASC,
-  sortParams,
+  RELATIVE_POSITION_DESC,
   SPECIAL_FILTER,
   SPECIAL_FILTER_VALUES,
   UPDATED_ASC,
   UPDATED_DESC,
+  urlSortParams,
   WEIGHT_ASC,
   WEIGHT_DESC,
 } from '~/issues_list/constants';
 import { __ } from '~/locale';
 
-export const getSortKey = (orderBy, sort) =>
-  Object.keys(sortParams).find(
-    (key) => sortParams[key].order_by === orderBy && sortParams[key].sort === sort,
-  );
+export const getSortKey = (sort) =>
+  Object.keys(urlSortParams).find((key) => urlSortParams[key].sort === sort);
 
 export const getSortOptions = (hasIssueWeightsFeature, hasBlockedIssuesFeature) => {
   const sortOptions = [
@@ -38,7 +33,7 @@ export const getSortOptions = (hasIssueWeightsFeature, hasBlockedIssuesFeature)
       id: 1,
       title: __('Priority'),
       sortDirection: {
-        ascending: PRIORITY_ASC,
+        ascending: PRIORITY_DESC,
         descending: PRIORITY_DESC,
       },
     },
@@ -86,7 +81,7 @@ export const getSortOptions = (hasIssueWeightsFeature, hasBlockedIssuesFeature)
       id: 7,
       title: __('Label priority'),
       sortDirection: {
-        ascending: LABEL_PRIORITY_ASC,
+        ascending: LABEL_PRIORITY_DESC,
         descending: LABEL_PRIORITY_DESC,
       },
     },
@@ -94,8 +89,8 @@ export const getSortOptions = (hasIssueWeightsFeature, hasBlockedIssuesFeature)
       id: 8,
       title: __('Manual'),
       sortDirection: {
-        ascending: RELATIVE_POSITION_ASC,
-        descending: RELATIVE_POSITION_ASC,
+        ascending: RELATIVE_POSITION_DESC,
+        descending: RELATIVE_POSITION_DESC,
       },
     },
   ];
@@ -116,7 +111,7 @@ export const getSortOptions = (hasIssueWeightsFeature, hasBlockedIssuesFeature)
       id: 10,
       title: __('Blocking'),
       sortDirection: {
-        ascending: BLOCKING_ISSUES_ASC,
+        ascending: BLOCKING_ISSUES_DESC,
         descending: BLOCKING_ISSUES_DESC,
       },
     });

app/assets/javascripts/vue_shared/components/filtered_search_bar/filtered_search_bar_root.vue

@@ -93,9 +93,9 @@ export default {
             sortBy.sortDirection.descending === this.initialSortBy,
         )
         .pop();
-      selectedSortDirection = this.initialSortBy.endsWith('_desc')
-        ? SortDirection.descending
-        : SortDirection.ascending;
+      selectedSortDirection = Object.keys(selectedSortOption.sortDirection).find(
+        (key) => selectedSortOption.sortDirection[key] === this.initialSortBy,
+      );
     }
 
     return {

spec/features/issuables/sorting_list_spec.rb

@@ -57,7 +57,7 @@ RSpec.describe 'Sort Issuable List' do
         it 'is "last updated"' do
           visit_merge_requests_with_state(project, 'merged')
 
-          expect(find('.filter-dropdown-container')).to have_content('Last updated')
+          expect(page).to have_button 'Last updated'
           expect(first_merge_request).to include(last_updated_issuable.title)
           expect(last_merge_request).to include(first_updated_issuable.title)
         end
@@ -69,7 +69,7 @@ RSpec.describe 'Sort Issuable List' do
         it 'is "last updated"' do
           visit_merge_requests_with_state(project, 'closed')
 
-          expect(find('.filter-dropdown-container')).to have_content('Last updated')
+          expect(page).to have_button 'Last updated'
           expect(first_merge_request).to include(last_updated_issuable.title)
           expect(last_merge_request).to include(first_updated_issuable.title)
         end
@@ -81,7 +81,7 @@ RSpec.describe 'Sort Issuable List' do
         it 'is "created date"' do
           visit_merge_requests_with_state(project, 'all')
 
-          expect(find('.filter-dropdown-container')).to have_content('Created date')
+          expect(page).to have_button 'Created date'
           expect(first_merge_request).to include(last_created_issuable.title)
           expect(last_merge_request).to include(first_created_issuable.title)
         end
@@ -94,15 +94,13 @@ RSpec.describe 'Sort Issuable List' do
       it 'supports sorting in asc and desc order' do
         visit_merge_requests_with_state(project, 'open')
 
-        page.within('.filter-dropdown-container') do
         click_button('Created date')
         click_link('Last updated')
-        end
 
         expect(first_merge_request).to include(last_updated_issuable.title)
         expect(last_merge_request).to include(first_updated_issuable.title)
 
-        find('.filter-dropdown-container .rspec-reverse-sort').click
+        click_on 'Sort direction'
 
         expect(first_merge_request).to include(first_updated_issuable.title)
         expect(last_merge_request).to include(last_updated_issuable.title)
@@ -133,7 +131,7 @@ RSpec.describe 'Sort Issuable List' do
         it 'is "created date"' do
           visit_issues project
 
-          expect(find('.filter-dropdown-container')).to have_content('Created date')
+          expect(page).to have_button 'Created date'
           expect(first_issue).to include(last_created_issuable.title)
           expect(last_issue).to include(first_created_issuable.title)
         end
@@ -145,7 +143,7 @@ RSpec.describe 'Sort Issuable List' do
         it 'is "created date"' do
           visit_issues_with_state(project, 'opened')
 
-          expect(find('.filter-dropdown-container')).to have_content('Created date')
+          expect(page).to have_button 'Created date'
           expect(first_issue).to include(last_created_issuable.title)
           expect(last_issue).to include(first_created_issuable.title)
         end
@@ -157,7 +155,7 @@ RSpec.describe 'Sort Issuable List' do
         it 'is "last updated"' do
           visit_issues_with_state(project, 'closed')
 
-          expect(find('.filter-dropdown-container')).to have_content('Last updated')
+          expect(page).to have_button 'Last updated'
           expect(first_issue).to include(last_updated_issuable.title)
           expect(last_issue).to include(first_updated_issuable.title)
         end
@@ -169,7 +167,7 @@ RSpec.describe 'Sort Issuable List' do
         it 'is "created date"' do
           visit_issues_with_state(project, 'all')
 
-          expect(find('.filter-dropdown-container')).to have_content('Created date')
+          expect(page).to have_button 'Created date'
           expect(first_issue).to include(last_created_issuable.title)
           expect(last_issue).to include(first_created_issuable.title)
         end
@@ -183,7 +181,7 @@ RSpec.describe 'Sort Issuable List' do
         end
 
         it 'shows the sort order as created date' do
-          expect(find('.filter-dropdown-container')).to have_content('Created date')
+          expect(page).to have_button 'Created date'
           expect(first_issue).to include(last_created_issuable.title)
           expect(last_issue).to include(first_created_issuable.title)
         end
@@ -196,15 +194,17 @@ RSpec.describe 'Sort Issuable List' do
       it 'supports sorting in asc and desc order' do
         visit_issues_with_state(project, 'opened')
 
-        page.within('.filter-dropdown-container') do
         click_button('Created date')
-          click_link('Last updated')
-        end
+        click_on('Last updated')
+
+        wait_for_requests
 
         expect(first_issue).to include(last_updated_issuable.title)
         expect(last_issue).to include(first_updated_issuable.title)
 
-        find('.filter-dropdown-container .rspec-reverse-sort').click
+        click_on 'Sort direction'
+
+        wait_for_requests
 
         expect(first_issue).to include(first_updated_issuable.title)
         expect(last_issue).to include(last_updated_issuable.title)

spec/features/projects/labels/issues_sorted_by_priority_spec.rb

@@ -33,16 +33,16 @@ RSpec.describe 'Issue prioritization' do
       sign_in user
       visit project_issues_path(project, sort: 'label_priority')
 
+      wait_for_requests
+
       # Ensure we are indicating that issues are sorted by priority
-      expect(page).to have_selector('.dropdown', text: 'Label priority')
+      expect(page).to have_button 'Label priority'
 
-      page.within('.issues-holder') do
       issue_titles = all('.issues-list .issue-title-text').map(&:text)
 
       expect(issue_titles).to eq(%w(issue_4 issue_3 issue_5 issue_2 issue_1))
     end
   end
-  end
 
   context 'when issues have multiple labels', :js do
     it 'are sorted properly' do
@@ -72,9 +72,10 @@ RSpec.describe 'Issue prioritization' do
       sign_in user
       visit project_issues_path(project, sort: 'label_priority')
 
-      expect(page).to have_selector('.dropdown', text: 'Label priority')
+      wait_for_requests
+
+      expect(page).to have_button 'Label priority'
 
-      page.within('.issues-holder') do
       issue_titles = all('.issues-list .issue-title-text').map(&:text)
 
       expect(issue_titles[0..1]).to contain_exactly('issue_5', 'issue_8')
@@ -82,5 +83,4 @@ RSpec.describe 'Issue prioritization' do
       expect(issue_titles[5..-1]).to eq(%w(issue_2 issue_4 issue_6))
     end
   end
-  end
 end

spec/frontend/issues_list/components/issues_list_app_spec.js

@@ -11,11 +11,12 @@ import IssuableList from '~/issuable_list/components/issuable_list_root.vue';
 import { IssuableListTabs, IssuableStates } from '~/issuable_list/constants';
 import IssuesListApp from '~/issues_list/components/issues_list_app.vue';
 import {
+  apiSortParams,
   CREATED_DESC,
   PAGE_SIZE,
   PAGE_SIZE_MANUAL,
-  RELATIVE_POSITION_ASC,
-  sortParams,
+  RELATIVE_POSITION_DESC,
+  urlSortParams,
 } from '~/issues_list/constants';
 import eventHub from '~/issues_list/eventhub';
 import { getSortOptions } from '~/issues_list/utils';
@@ -146,7 +147,7 @@ describe('IssuesListApp component', () => {
     describe('csv import/export component', () => {
       describe('when user is signed in', () => {
         it('renders', async () => {
-          const search = '?page=1&search=refactor&state=opened&order_by=created_at&sort=desc';
+          const search = '?page=1&search=refactor&state=opened&sort=created_date';
 
           global.jsdom.reconfigure({ url: `${TEST_HOST}${search}` });
 
@@ -239,14 +240,14 @@ describe('IssuesListApp component', () => {
     });
 
     describe('sort', () => {
-      it.each(Object.keys(sortParams))('is set as %s from the url params', (sortKey) => {
-        global.jsdom.reconfigure({ url: setUrlParams(sortParams[sortKey], TEST_HOST) });
+      it.each(Object.keys(urlSortParams))('is set as %s from the url params', (sortKey) => {
+        global.jsdom.reconfigure({ url: setUrlParams(urlSortParams[sortKey], TEST_HOST) });
 
         wrapper = mountComponent();
 
         expect(findIssuableList().props()).toMatchObject({
           initialSortBy: sortKey,
-          urlParams: sortParams[sortKey],
+          urlParams: urlSortParams[sortKey],
         });
       });
     });
@@ -528,7 +529,7 @@ describe('IssuesListApp component', () => {
     });
 
     describe('when "sort" event is emitted by IssuableList', () => {
-      it.each(Object.keys(sortParams))(
+      it.each(Object.keys(apiSortParams))(
         'fetches issues with correct params with payload `%s`',
         async (sortKey) => {
           wrapper = mountComponent();
@@ -539,10 +540,10 @@ describe('IssuesListApp component', () => {
 
           expect(axiosMock.history.get[1].params).toEqual({
             page: xPage,
-            per_page: sortKey === RELATIVE_POSITION_ASC ? PAGE_SIZE_MANUAL : PAGE_SIZE,
+            per_page: sortKey === RELATIVE_POSITION_DESC ? PAGE_SIZE_MANUAL : PAGE_SIZE,
             state,
             with_labels_details: true,
-            ...sortParams[sortKey],
+            ...apiSortParams[sortKey],
           });
         },
       );

spec/frontend/issues_list/utils_spec.js

@@ -8,7 +8,7 @@ import {
   urlParams,
   urlParamsWithSpecialValues,
 } from 'jest/issues_list/mock_data';
-import { sortParams } from '~/issues_list/constants';
+import { urlSortParams } from '~/issues_list/constants';
 import {
   convertToApiParams,
   convertToSearchQuery,
@@ -19,9 +19,9 @@ import {
 } from '~/issues_list/utils';
 
 describe('getSortKey', () => {
-  it.each(Object.keys(sortParams))('returns %s given the correct inputs', (sortKey) => {
-    const { order_by, sort } = sortParams[sortKey];
-    expect(getSortKey(order_by, sort)).toBe(sortKey);
+  it.each(Object.keys(urlSortParams))('returns %s given the correct inputs', (sortKey) => {
+    const { sort } = urlSortParams[sortKey];
+    expect(getSortKey(sort)).toBe(sortKey);
   });
 });