From b0b8f438699070d1fe0e624a4ff1b9b4439dbeba Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 7 Dec 2022 15:57:15 -0700 Subject: [PATCH 1/5] Add the ability to get version info from derived MR info Since we're already extracting information from the MR endpoint, this addition also extracts the diffId and startSha if they're available. Many MR URLs will not have these values, but that's okay. Notably, the presence of the diffId is not - by itself - enough to indicate that the MR endpoint we're looking at is a "sub"-version of the whole MR. The BASE -> HEAD diff can be represented as a single diffId. In fact, BASE -> HEAD can also be represented as a combination of a diffId plus a startSha. So even knowing that we're for sure looking at an MR version isn't conceptually possible knowing only the endpoint. Since all the endpoints in an MR lack the protocol/domain, we use "https://gitlab.com" as a dummy root so we can successfully construct a new URL object. Since - for this purpose - we don't care what the root is, it's just used to get the URL constructor to succeed. An alternative would be to use something like `document.location.protocol` + `document.location.host`, but this seems unnecessarily brittle given that we immediately throw away everything except for the search parameters. --- .../javascripts/diffs/utils/merge_request.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/app/assets/javascripts/diffs/utils/merge_request.js b/app/assets/javascripts/diffs/utils/merge_request.js index edb4304f558199..43e04a814c5ad1 100644 --- a/app/assets/javascripts/diffs/utils/merge_request.js +++ b/app/assets/javascripts/diffs/utils/merge_request.js @@ -1,14 +1,30 @@ 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, }; } -- GitLab From 3af970d34e35440383172c08d086ed04355a4d1f Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 7 Dec 2022 15:58:43 -0700 Subject: [PATCH 2/5] Include the diffId and startSha when loading a collapsed diff This prevents loading the `HEAD` version of a file's diff when viewing a comparison between versions in the UI. Changelog: fixed --- app/assets/javascripts/diffs/store/actions.js | 33 +++++++++++-------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index fabaee2c4961a3..96a739178207f2 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/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. -- GitLab From 66001200cb3816c941352dbc4e35bca368fb186a Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 7 Dec 2022 16:34:33 -0700 Subject: [PATCH 3/5] Test new information derived from MR endpoint --- .../diffs/utils/merge_request_spec.js | 46 +++++++++++++++++-- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/spec/frontend/diffs/utils/merge_request_spec.js b/spec/frontend/diffs/utils/merge_request_spec.js index 8c7b1e1f2a5d9a..b14c0169b7c440 100644 --- a/spec/frontend/diffs/utils/merge_request_spec.js +++ b/spec/frontend/diffs/utils/merge_request_spec.js @@ -2,30 +2,70 @@ 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 })).toEqual( + expect.objectContaining(derivedBaseInfo), + ); + }); + + it.each` + url | versionPart + ${endpoint} | ${derivedVersionInfo} + ${`${bare}?diff_id=4`} | ${{ ...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 })).toEqual( + expect.objectContaining(versionPart), + ); + }, + ); + + it('extracts nothing if there is no available version-like information in the URL', () => { + expect(getDerivedMergeRequestInformation({ endpoint: bare })).toEqual( + expect.objectContaining(noVersion), + ); + }); + }); }); }); -- GitLab From e0f40ae3333a5683520acdfeb04139718ff9137b Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 7 Dec 2022 17:44:13 -0700 Subject: [PATCH 4/5] Test that diffs load with the correct version info --- spec/frontend/diffs/store/actions_spec.js | 44 +++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index 87366cdbfc5b56..9e0ffbf757fd13 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/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', () => { -- GitLab From dec57acc84bcb45a5fb0ac71c83abfa8784b5b1e Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 12 Dec 2022 12:31:10 -0700 Subject: [PATCH 5/5] Use `.toMatchObject` instead of `objectContaining` --- spec/frontend/diffs/utils/merge_request_spec.js | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/spec/frontend/diffs/utils/merge_request_spec.js b/spec/frontend/diffs/utils/merge_request_spec.js index b14c0169b7c440..c070e8c004da4c 100644 --- a/spec/frontend/diffs/utils/merge_request_spec.js +++ b/spec/frontend/diffs/utils/merge_request_spec.js @@ -42,29 +42,23 @@ describe('Merge Request utilities', () => { endpoint = diffMetadata.merge_request_diffs[0].compare_path; it('still gets the correct derived information', () => { - expect(getDerivedMergeRequestInformation({ endpoint })).toEqual( - expect.objectContaining(derivedBaseInfo), - ); + expect(getDerivedMergeRequestInformation({ endpoint })).toMatchObject(derivedBaseInfo); }); it.each` url | versionPart ${endpoint} | ${derivedVersionInfo} - ${`${bare}?diff_id=4`} | ${{ ...derivedVersionInfo, startSha: undefined }} + ${`${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 })).toEqual( - expect.objectContaining(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 })).toEqual( - expect.objectContaining(noVersion), - ); + expect(getDerivedMergeRequestInformation({ endpoint: bare })).toMatchObject(noVersion); }); }); }); -- GitLab