Merge branch 'feature/file-reviews/add-mr-id-to-file-id' into 'master'

Add MR information to the diff file IDs

See merge request gitlab-org/gitlab!50132

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

@@ -4,6 +4,7 @@ import {
   DIFF_FILE_MANUAL_COLLAPSE,
   DIFF_FILE_AUTOMATIC_COLLAPSE,
 } from '../constants';
+import { getDerivedMergeRequestInformation } from './merge_request';
 import { uuids } from './uuids';
 function fileSymlinkInformation(file, fileList) {
@@ -34,8 +35,12 @@ function collapsed(file) {
 }
 function identifier(file) {
+  const { userOrGroup, project, id } = getDerivedMergeRequestInformation({
+    endpoint: file.load_collapsed_diff_url,
+  });
+
   return uuids({
-    seeds: [file.file_identifier_hash, file.blob?.id],
+    seeds: [userOrGroup, project, id, file.file_identifier_hash, file.blob?.id],
   })[0];
 }
@@ -48,10 +53,10 @@ export function prepareRawDiffFile({ file, allFiles, meta = false }) {
     },
   };
-  // It's possible, but not confirmed, that `content_sha` isn't available sometimes
+  // It's possible, but not confirmed, that `blob.id` isn't available sometimes
   // See: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/49506#note_464692057
   // We don't want duplicate IDs if that's the case, so we just don't assign an ID
-  if (!meta && file.blob?.id) {
+  if (!meta && file.blob?.id && file.load_collapsed_diff_url) {
     additionalProperties.id = identifier(file);
   }

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

+const endpointRE = /^(\/?(.+?)\/(.+?)\/-\/merge_requests\/(\d+)).*$/i;
+
 export function getDerivedMergeRequestInformation({ endpoint } = {}) {
-  const mrPath = endpoint
-    ?.split('/')
-    .slice(0, -1)
-    .join('/');
+  let mrPath;
+  let userOrGroup;
+  let project;
+  let id;
+  const matches = endpointRE.exec(endpoint);
+
+  if (matches) {
+    [, mrPath, userOrGroup, project, id] = matches;
+  }
   return {
     mrPath,
+    userOrGroup,
+    project,
+    id,
   };
 }

spec/frontend/diffs/utils/diff_file_spec.js

 import { prepareRawDiffFile } from '~/diffs/utils/diff_file';
 function getDiffFiles() {
+  const loadFull = 'namespace/project/-/merge_requests/12345/diff_for_path?file_identifier=abc';
+
   return [
     {
       blob: {
@@ -8,6 +10,7 @@ function getDiffFiles() {
       },
       file_hash: 'ABC', // This file is just a normal file
       file_identifier_hash: 'ABC1',
+      load_collapsed_diff_url: loadFull,
     },
     {
       blob: {
@@ -15,6 +18,7 @@ function getDiffFiles() {
       },
       file_hash: 'DEF', // This file replaces a symlink
       file_identifier_hash: 'DEF1',
+      load_collapsed_diff_url: loadFull,
       a_mode: '0',
       b_mode: '0755',
     },
@@ -24,6 +28,7 @@ function getDiffFiles() {
       },
       file_hash: 'DEF', // This symlink is replaced by a file
       file_identifier_hash: 'DEF2',
+      load_collapsed_diff_url: loadFull,
       a_mode: '120000',
       b_mode: '0',
     },
@@ -33,6 +38,7 @@ function getDiffFiles() {
       },
       file_hash: 'GHI', // This symlink replaces a file
       file_identifier_hash: 'GHI1',
+      load_collapsed_diff_url: loadFull,
       a_mode: '0',
       b_mode: '120000',
     },
@@ -42,6 +48,7 @@ function getDiffFiles() {
       },
       file_hash: 'GHI', // This file is replaced by a symlink
       file_identifier_hash: 'GHI2',
+      load_collapsed_diff_url: loadFull,
       a_mode: '0755',
       b_mode: '0',
     },
@@ -86,11 +93,11 @@ describe('diff_file utilities', () => {
     it.each`
       fileIndex | id
-      ${0}      | ${'8dcd585e-a421-4dab-a04e-6f88c81b7b4c'}
-      ${1}      | ${'3f178b78-392b-44a4-bd7d-5d6192208a97'}
-      ${2}      | ${'3d9e1354-cddf-4a11-8234-f0413521b2e5'}
-      ${3}      | ${'460f005b-d29d-43c1-9a08-099a7c7f08de'}
-      ${4}      | ${'d8c89733-6ce1-4455-ae3d-f8aad6ee99f9'}
+      ${0}      | ${'68296a4f-f1c7-445a-bd0e-6e3b02c4eec0'}
+      ${1}      | ${'051c9bb8-cdba-4eb7-b8d1-508906e6d8ba'}
+      ${2}      | ${'ed3d53d5-5da0-412d-a3c6-7213f84e88d3'}
+      ${3}      | ${'39d998dc-bc69-4b19-a6af-41e4369c2bd5'}
+      ${4}      | ${'7072d115-ce39-423c-8346-9fcad58cd68e'}
     `('sets the file id properly { id: $id } on normal diff files', ({ fileIndex, id }) => {
       const preppedFile = prepareRawDiffFile({
         file: files[fileIndex],
@@ -122,5 +129,18 @@ describe('diff_file utilities', () => {
       expect(preppedFile).not.toHaveProp('id');
     });
+
+    it('does not set the id property if the file is missing a `load_collapsed_diff_url` property', () => {
+      const fileMissingContentSha = { ...files[0] };
+
+      delete fileMissingContentSha.load_collapsed_diff_url;
+
+      const preppedFile = prepareRawDiffFile({
+        file: fileMissingContentSha,
+        allFiles: files,
+      });
+
+      expect(preppedFile).not.toHaveProp('id');
+    });
   });
 });

spec/frontend/diffs/utils/merge_request_spec.js

@@ -2,16 +2,28 @@ import { getDerivedMergeRequestInformation } from '~/diffs/utils/merge_request';
 import { diffMetadata } from '../mock_data/diff_metadata';
 describe('Merge Request utilities', () => {
+  const derivedMrInfo = {
+    mrPath: '/gitlab-org/gitlab-test/-/merge_requests/4',
+    userOrGroup: 'gitlab-org',
+    project: 'gitlab-test',
+    id: '4',
+  };
+  const unparseableEndpoint = {
+    mrPath: undefined,
+    userOrGroup: undefined,
+    project: undefined,
+    id: undefined,
+  };
+
   describe('getDerivedMergeRequestInformation', () => {
     const endpoint = `${diffMetadata.latest_version_path}.json?searchParam=irrelevant`;
-    const mrPath = diffMetadata.latest_version_path.replace(/\/diffs$/, '');
     it.each`
       argument                   | response
-      ${{ endpoint }}            | ${{ mrPath }}
-      ${{}}                      | ${{ mrPath: undefined }}
-      ${{ endpoint: undefined }} | ${{ mrPath: undefined }}
-      ${{ endpoint: null }}      | ${{ mrPath: undefined }}
+      ${{ endpoint }}            | ${derivedMrInfo}
+      ${{}}                      | ${unparseableEndpoint}
+      ${{ endpoint: undefined }} | ${unparseableEndpoint}
+      ${{ endpoint: null }}      | ${unparseableEndpoint}
     `('generates the correct derived results based on $argument', ({ argument, response }) => {
       expect(getDerivedMergeRequestInformation(argument)).toStrictEqual(response);
     });