Merge branch 'tor/defect/mr-versions-wrong-file-diff' into 'master'

Include MR version information when loading file diffs

See merge request !106378



Merged-by: Thomas Randolph <trandolph@gitlab.com>
Approved-by: Lee Tickett <ltickett@gitlab.com>
Reviewed-by: Coung Ngo <cngo@gitlab.com>
Reviewed-by: Lee Tickett <ltickett@gitlab.com>

app/assets/javascripts/diffs/store/actions.js

@@ -444,20 +444,27 @@ export const scrollToLineIfNeededParallel = (_, line) => {
   }
 };
 
-export const loadCollapsedDiff = ({ commit, getters, state }, file) =>
-  axios
-    .get(file.load_collapsed_diff_url, {
-      params: {
-        commit_id: getters.commitId,
-        w: state.showWhitespace ? '0' : '1',
-      },
-    })
-    .then((res) => {
-      commit(types.ADD_COLLAPSED_DIFFS, {
-        file,
-        data: res.data,
-      });
+export const loadCollapsedDiff = ({ commit, getters, state }, file) => {
+  const versionPath = state.mergeRequestDiff?.version_path;
+  const loadParams = {
+    commit_id: getters.commitId,
+    w: state.showWhitespace ? '0' : '1',
+  };
+
+  if (versionPath) {
+    const { diffId, startSha } = getDerivedMergeRequestInformation({ endpoint: versionPath });
+
+    loadParams.diff_id = diffId;
+    loadParams.start_sha = startSha;
+  }
+
+  return axios.get(file.load_collapsed_diff_url, { params: loadParams }).then((res) => {
+    commit(types.ADD_COLLAPSED_DIFFS, {
+      file,
+      data: res.data,
     });
+  });
+};
 
 /**
  * Toggles the file discussions after user clicked on the toggle discussions button.

app/assets/javascripts/diffs/utils/merge_request.js

 const endpointRE = /^(\/?(.+?)\/(.+?)\/-\/merge_requests\/(\d+)).*$/i;
 
+function getVersionInfo({ endpoint } = {}) {
+  const dummyRoot = 'https://gitlab.com';
+  const endpointUrl = new URL(endpoint, dummyRoot);
+  const params = Object.fromEntries(endpointUrl.searchParams.entries());
+
+  const { start_sha: startSha, diff_id: diffId } = params;
+
+  return {
+    diffId,
+    startSha,
+  };
+}
+
 export function getDerivedMergeRequestInformation({ endpoint } = {}) {
   let mrPath;
   let userOrGroup;
   let project;
   let id;
+  let diffId;
+  let startSha;
   const matches = endpointRE.exec(endpoint);
 
   if (matches) {
     [, mrPath, userOrGroup, project, id] = matches;
+    ({ diffId, startSha } = getVersionInfo({ endpoint }));
   }
 
   return {
@@ -16,5 +32,7 @@ export function getDerivedMergeRequestInformation({ endpoint } = {}) {
     userOrGroup,
     project,
     id,
+    diffId,
+    startSha,
   };
 }

spec/frontend/diffs/store/actions_spec.js

@@ -606,6 +606,50 @@ describe('DiffsStoreActions', () => {
         params: { commit_id: '123', w: '0' },
       });
     });
+
+    describe('version parameters', () => {
+      const diffId = '4';
+      const startSha = 'abc';
+      const pathRoot = 'a/a/-/merge_requests/1';
+      let file;
+      let getters;
+
+      beforeAll(() => {
+        file = { load_collapsed_diff_url: '/load/collapsed/diff/url' };
+        getters = {};
+      });
+
+      beforeEach(() => {
+        jest.spyOn(axios, 'get').mockReturnValue(Promise.resolve({ data: {} }));
+      });
+
+      it('fetches the data when there is no mergeRequestDiff', () => {
+        diffActions.loadCollapsedDiff({ commit() {}, getters, state }, file);
+
+        expect(axios.get).toHaveBeenCalledWith(file.load_collapsed_diff_url, {
+          params: expect.any(Object),
+        });
+      });
+
+      it.each`
+        desc                                   | versionPath                                              | start_sha    | diff_id
+        ${'no additional version information'} | ${`${pathRoot}?search=terms`}                            | ${undefined} | ${undefined}
+        ${'the diff_id'}                       | ${`${pathRoot}?diff_id=${diffId}`}                       | ${undefined} | ${diffId}
+        ${'the start_sha'}                     | ${`${pathRoot}?start_sha=${startSha}`}                   | ${startSha}  | ${undefined}
+        ${'all available version information'} | ${`${pathRoot}?diff_id=${diffId}&start_sha=${startSha}`} | ${startSha}  | ${diffId}
+      `('fetches the data and includes $desc', ({ versionPath, start_sha, diff_id }) => {
+        jest.spyOn(axios, 'get').mockReturnValue(Promise.resolve({ data: {} }));
+
+        diffActions.loadCollapsedDiff(
+          { commit() {}, getters, state: { mergeRequestDiff: { version_path: versionPath } } },
+          file,
+        );
+
+        expect(axios.get).toHaveBeenCalledWith(file.load_collapsed_diff_url, {
+          params: expect.objectContaining({ start_sha, diff_id }),
+        });
+      });
+    });
   });
 
   describe('toggleFileDiscussions', () => {

spec/frontend/diffs/utils/merge_request_spec.js

@@ -2,30 +2,64 @@ import { getDerivedMergeRequestInformation } from '~/diffs/utils/merge_request';
 import { diffMetadata } from '../mock_data/diff_metadata';
 
 describe('Merge Request utilities', () => {
-  const derivedMrInfo = {
+  const derivedBaseInfo = {
     mrPath: '/gitlab-org/gitlab-test/-/merge_requests/4',
     userOrGroup: 'gitlab-org',
     project: 'gitlab-test',
     id: '4',
   };
+  const derivedVersionInfo = {
+    diffId: '4',
+    startSha: 'eb227b3e214624708c474bdab7bde7afc17cefcc',
+  };
+  const noVersion = {
+    diffId: undefined,
+    startSha: undefined,
+  };
   const unparseableEndpoint = {
     mrPath: undefined,
     userOrGroup: undefined,
     project: undefined,
     id: undefined,
+    ...noVersion,
   };
 
   describe('getDerivedMergeRequestInformation', () => {
-    const endpoint = `${diffMetadata.latest_version_path}.json?searchParam=irrelevant`;
+    let endpoint = `${diffMetadata.latest_version_path}.json?searchParam=irrelevant`;
 
     it.each`
       argument                   | response
-      ${{ endpoint }}            | ${derivedMrInfo}
+      ${{ endpoint }}            | ${{ ...derivedBaseInfo, ...noVersion }}
       ${{}}                      | ${unparseableEndpoint}
       ${{ endpoint: undefined }} | ${unparseableEndpoint}
       ${{ endpoint: null }}      | ${unparseableEndpoint}
     `('generates the correct derived results based on $argument', ({ argument, response }) => {
       expect(getDerivedMergeRequestInformation(argument)).toStrictEqual(response);
     });
+
+    describe('version information', () => {
+      const bare = diffMetadata.latest_version_path;
+      endpoint = diffMetadata.merge_request_diffs[0].compare_path;
+
+      it('still gets the correct derived information', () => {
+        expect(getDerivedMergeRequestInformation({ endpoint })).toMatchObject(derivedBaseInfo);
+      });
+
+      it.each`
+        url                                                   | versionPart
+        ${endpoint}                                           | ${derivedVersionInfo}
+        ${`${bare}?diff_id=${derivedVersionInfo.diffId}`}     | ${{ ...derivedVersionInfo, startSha: undefined }}
+        ${`${bare}?start_sha=${derivedVersionInfo.startSha}`} | ${{ ...derivedVersionInfo, diffId: undefined }}
+      `(
+        'generates the correct derived version information based on $url',
+        ({ url, versionPart }) => {
+          expect(getDerivedMergeRequestInformation({ endpoint: url })).toMatchObject(versionPart);
+        },
+      );
+
+      it('extracts nothing if there is no available version-like information in the URL', () => {
+        expect(getDerivedMergeRequestInformation({ endpoint: bare })).toMatchObject(noVersion);
+      });
+    });
   });
 });