Merge branch 'nfriend-fix-sort-change-pagination-bug' into 'master'

Fix Releases page pagination after sort change

See merge request gitlab-org/gitlab!63434

app/assets/javascripts/releases/components/app_index_apollo_client.vue

 <script>
 import { GlButton } from '@gitlab/ui';
 import createFlash from '~/flash';
-import { getParameterByName } from '~/lib/utils/common_utils';
+import { historyPushState, getParameterByName } from '~/lib/utils/common_utils';
 import { scrollUp } from '~/lib/utils/scroll_utils';
+import { setUrlParams } from '~/lib/utils/url_utility';
 import { __ } from '~/locale';
-import { PAGE_SIZE, RELEASED_AT_DESC } from '~/releases/constants';
+import { PAGE_SIZE, DEFAULT_SORT } from '~/releases/constants';
 import allReleasesQuery from '~/releases/graphql/queries/all_releases.query.graphql';
 import { convertAllReleasesGraphQLResponse } from '~/releases/util';
 import ReleaseBlock from './release_block.vue';
@@ -58,7 +59,7 @@ export default {
         before: getParameterByName('before'),
         after: getParameterByName('after'),
       },
-      sort: RELEASED_AT_DESC,
+      sort: DEFAULT_SORT,
     };
   },
   computed: {
@@ -145,6 +146,29 @@ export default {
       // scroll to the top of the page every time a pagination button is pressed.
       scrollUp();
     },
+    onSortChanged(newSort) {
+      if (this.sort === newSort) {
+        return;
+      }
+
+      // Remove the "before" and "after" query parameters from the URL,
+      // effectively placing the user back on page 1 of the results.
+      // This prevents the frontend from requesting the results sorted
+      // by one field (e.g. `released_at`) while using a pagination cursor
+      // intended for a different field (e.g.) `created_at`).
+      // For more details, see the MR that introduced this change:
+      // https://gitlab.com/gitlab-org/gitlab/-/merge_requests/63434
+      historyPushState(
+        setUrlParams({
+          before: null,
+          after: null,
+        }),
+      );
+
+      this.updateQueryParamsFromUrl();
+
+      this.sort = newSort;
+    },
   },
   i18n: {
     newRelease: __('New release'),
@@ -155,7 +179,7 @@ export default {
 <template>
   <div class="flex flex-column mt-2">
     <div class="gl-align-self-end gl-mb-3">
-      <releases-sort-apollo-client v-model="sort" class="gl-mr-2" />
+      <releases-sort-apollo-client :value="sort" class="gl-mr-2" @input="onSortChanged" />
 
       <gl-button
         v-if="newReleasePath"

app/assets/javascripts/releases/constants.js

@@ -47,3 +47,5 @@ export const SORT_MAP = {
     [DESCENDING_ORDER]: CREATED_DESC,
   },
 };
+
+export const DEFAULT_SORT = RELEASED_AT_DESC;

spec/frontend/releases/components/app_index_apollo_client_spec.js

@@ -4,13 +4,14 @@ import VueApollo from 'vue-apollo';
 import createMockApollo from 'helpers/mock_apollo_helper';
 import { shallowMountExtended } from 'helpers/vue_test_utils_helper';
 import createFlash from '~/flash';
+import { historyPushState } from '~/lib/utils/common_utils';
 import ReleasesIndexApolloClientApp from '~/releases/components/app_index_apollo_client.vue';
 import ReleaseBlock from '~/releases/components/release_block.vue';
 import ReleaseSkeletonLoader from '~/releases/components/release_skeleton_loader.vue';
 import ReleasesEmptyState from '~/releases/components/releases_empty_state.vue';
 import ReleasesPaginationApolloClient from '~/releases/components/releases_pagination_apollo_client.vue';
 import ReleasesSortApolloClient from '~/releases/components/releases_sort_apollo_client.vue';
-import { PAGE_SIZE, RELEASED_AT_DESC, CREATED_ASC } from '~/releases/constants';
+import { PAGE_SIZE, CREATED_ASC, DEFAULT_SORT } from '~/releases/constants';
 import allReleasesQuery from '~/releases/graphql/queries/all_releases.query.graphql';
 
 Vue.use(VueApollo);
@@ -23,6 +24,7 @@ jest.mock('~/lib/utils/common_utils', () => ({
   getParameterByName: jest
     .fn()
     .mockImplementation((parameterName) => mockQueryParams[parameterName]),
+  historyPushState: jest.fn(),
 }));
 
 describe('app_index_apollo_client.vue', () => {
@@ -225,7 +227,7 @@ describe('app_index_apollo_client.vue', () => {
         expect(allReleasesQueryMock).toHaveBeenCalledWith({
           first: PAGE_SIZE,
           fullPath: projectPath,
-          sort: RELEASED_AT_DESC,
+          sort: DEFAULT_SORT,
         });
       });
     });
@@ -241,7 +243,7 @@ describe('app_index_apollo_client.vue', () => {
           before,
           last: PAGE_SIZE,
           fullPath: projectPath,
-          sort: RELEASED_AT_DESC,
+          sort: DEFAULT_SORT,
         });
       });
     });
@@ -257,7 +259,7 @@ describe('app_index_apollo_client.vue', () => {
           after,
           first: PAGE_SIZE,
           fullPath: projectPath,
-          sort: RELEASED_AT_DESC,
+          sort: DEFAULT_SORT,
         });
       });
     });
@@ -273,7 +275,7 @@ describe('app_index_apollo_client.vue', () => {
           after,
           first: PAGE_SIZE,
           fullPath: projectPath,
-          sort: RELEASED_AT_DESC,
+          sort: DEFAULT_SORT,
         });
       });
     });
@@ -320,21 +322,82 @@ describe('app_index_apollo_client.vue', () => {
       createComponent();
     });
 
-    it(`sorts by ${RELEASED_AT_DESC} by default`, () => {
+    it(`sorts by ${DEFAULT_SORT} by default`, () => {
       expect(allReleasesQueryMock.mock.calls).toEqual([
-        [expect.objectContaining({ sort: RELEASED_AT_DESC })],
+        [expect.objectContaining({ sort: DEFAULT_SORT })],
       ]);
     });
 
-    it('requeries the GraphQL endpoint when the sort is changed', async () => {
+    it('requeries the GraphQL endpoint and updates the URL when the sort is changed', async () => {
       findSort().vm.$emit('input', CREATED_ASC);
 
       await wrapper.vm.$nextTick();
 
       expect(allReleasesQueryMock.mock.calls).toEqual([
-        [expect.objectContaining({ sort: RELEASED_AT_DESC })],
+        [expect.objectContaining({ sort: DEFAULT_SORT })],
         [expect.objectContaining({ sort: CREATED_ASC })],
       ]);
+
+      // URL manipulation is tested in more detail in the `describe` block below
+      expect(historyPushState).toHaveBeenCalled();
+    });
+
+    it('does not requery the GraphQL endpoint or update the URL if the sort is updated to the same value', async () => {
+      findSort().vm.$emit('input', DEFAULT_SORT);
+
+      await wrapper.vm.$nextTick();
+
+      expect(allReleasesQueryMock.mock.calls).toEqual([
+        [expect.objectContaining({ sort: DEFAULT_SORT })],
+      ]);
+
+      expect(historyPushState).not.toHaveBeenCalled();
     });
   });
+
+  describe('sorting + pagination interaction', () => {
+    const nonPaginationQueryParam = 'nonPaginationQueryParam';
+
+    beforeEach(() => {
+      historyPushState.mockImplementation((newUrl) => {
+        mockQueryParams = Object.fromEntries(new URL(newUrl).searchParams);
+      });
+    });
+
+    describe.each`
+      queryParamsBefore                      | paramName   | paramInitialValue
+      ${{ before, nonPaginationQueryParam }} | ${'before'} | ${before}
+      ${{ after, nonPaginationQueryParam }}  | ${'after'}  | ${after}
+    `(
+      'when the URL contains a "$paramName" pagination cursor',
+      ({ queryParamsBefore, paramName, paramInitialValue }) => {
+        beforeEach(async () => {
+          mockQueryParams = queryParamsBefore;
+          createComponent();
+
+          findSort().vm.$emit('input', CREATED_ASC);
+
+          await wrapper.vm.$nextTick();
+        });
+
+        it(`resets the page's "${paramName}" pagination cursor when the sort is changed`, () => {
+          const firstRequestVariables = allReleasesQueryMock.mock.calls[0][0];
+          const secondRequestVariables = allReleasesQueryMock.mock.calls[1][0];
+
+          expect(firstRequestVariables[paramName]).toBe(paramInitialValue);
+          expect(secondRequestVariables[paramName]).toBeUndefined();
+        });
+
+        it(`updates the URL to not include the "${paramName}" URL query parameter`, () => {
+          expect(historyPushState).toHaveBeenCalledTimes(1);
+
+          const updatedUrlQueryParams = Object.fromEntries(
+            new URL(historyPushState.mock.calls[0][0]).searchParams,
+          );
+
+          expect(updatedUrlQueryParams[paramName]).toBeUndefined();
+        });
+      },
+    );
+  });
 });