Merge branch 'feature/file-reviews/diff-file-ids' into 'master'

Identify Full Diff Files

See merge request gitlab-org/gitlab!49506

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

@@ -191,7 +191,13 @@ export const fetchDiffFilesMeta = ({ commit, state }) => {
       commit(types.SET_MERGE_REQUEST_DIFFS, data.merge_request_diffs || []);
       commit(types.SET_DIFF_METADATA, strippedData);
 
-      worker.postMessage(prepareDiffData(data, state.diffFiles));
+      worker.postMessage(
+        prepareDiffData({
+          diff: data,
+          priorFiles: state.diffFiles,
+          meta: true,
+        }),
+      );
 
       return data;
     })

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

@@ -73,7 +73,10 @@ export default {
   },
 
   [types.SET_DIFF_DATA_BATCH](state, data) {
-    const files = prepareDiffData(data, state.diffFiles);
+    const files = prepareDiffData({
+      diff: data,
+      priorFiles: state.diffFiles,
+    });
 
     Object.assign(state, {
       ...convertObjectPropsToCamelCase(data),
@@ -143,7 +146,7 @@ export default {
   },
 
   [types.ADD_COLLAPSED_DIFFS](state, { file, data }) {
-    const files = prepareDiffData(data);
+    const files = prepareDiffData({ diff: data });
     const [newFileData] = files.filter(f => f.file_hash === file.file_hash);
     const selectedFile = state.diffFiles.find(f => f.file_hash === file.file_hash);
     Object.assign(selectedFile, { ...newFileData });

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

@@ -386,9 +386,9 @@ function deduplicateFilesList(files) {
   return Object.values(dedupedFiles);
 }
 
-export function prepareDiffData(diff, priorFiles = []) {
+export function prepareDiffData({ diff, priorFiles = [], meta = false }) {
   const cleanedFiles = (diff.diff_files || [])
-    .map((file, index, allFiles) => prepareRawDiffFile({ file, allFiles }))
+    .map((file, index, allFiles) => prepareRawDiffFile({ file, allFiles, meta }))
     .map(ensureBasicDiffFileLines)
     .map(prepareDiffFileLines)
     .map(finalizeDiffFile);

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

@@ -4,6 +4,7 @@ import {
   DIFF_FILE_MANUAL_COLLAPSE,
   DIFF_FILE_AUTOMATIC_COLLAPSE,
 } from '../constants';
+import { uuids } from './uuids';
 
 function fileSymlinkInformation(file, fileList) {
   const duplicates = fileList.filter(iteratedFile => iteratedFile.file_hash === file.file_hash);
@@ -32,16 +33,29 @@ function collapsed(file) {
   };
 }
 
-export function prepareRawDiffFile({ file, allFiles }) {
-  Object.assign(file, {
+function identifier(file) {
+  return uuids({
+    seeds: [file.file_identifier_hash, file.content_sha],
+  })[0];
+}
+
+export function prepareRawDiffFile({ file, allFiles, meta = false }) {
+  const additionalProperties = {
     brokenSymlink: fileSymlinkInformation(file, allFiles),
     viewer: {
       ...file.viewer,
       ...collapsed(file),
     },
-  });
+  };
+
+  // It's possible, but not confirmed, that `content_sha` 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.content_sha) {
+    additionalProperties.id = identifier(file);
+  }
 
-  return file;
+  return Object.assign(file, additionalProperties);
 }
 
 export function collapsedType(file) {

spec/frontend/diffs/store/utils_spec.js

@@ -409,10 +409,13 @@ describe('DiffsStoreUtils', () => {
           diff_files: [{ ...mock, [INLINE_DIFF_LINES_KEY]: undefined }],
         };
 
-        preparedDiff.diff_files = utils.prepareDiffData(preparedDiff);
-        splitInlineDiff.diff_files = utils.prepareDiffData(splitInlineDiff);
-        splitParallelDiff.diff_files = utils.prepareDiffData(splitParallelDiff);
-        completedDiff.diff_files = utils.prepareDiffData(completedDiff, [mock]);
+        preparedDiff.diff_files = utils.prepareDiffData({ diff: preparedDiff });
+        splitInlineDiff.diff_files = utils.prepareDiffData({ diff: splitInlineDiff });
+        splitParallelDiff.diff_files = utils.prepareDiffData({ diff: splitParallelDiff });
+        completedDiff.diff_files = utils.prepareDiffData({
+          diff: completedDiff,
+          priorFiles: [mock],
+        });
       });
 
       it('sets the renderIt and collapsed attribute on files', () => {
@@ -447,7 +450,10 @@ describe('DiffsStoreUtils', () => {
           content_sha: 'ABC',
           file_hash: 'DEF',
         };
-        const updatedFilesList = utils.prepareDiffData({ diff_files: [fakeNewFile] }, priorFiles);
+        const updatedFilesList = utils.prepareDiffData({
+          diff: { diff_files: [fakeNewFile] },
+          priorFiles,
+        });
 
         expect(updatedFilesList).toEqual([mock, fakeNewFile]);
       });
@@ -460,7 +466,10 @@ describe('DiffsStoreUtils', () => {
           { ...mock, [INLINE_DIFF_LINES_KEY]: undefined },
           { ...mock, [INLINE_DIFF_LINES_KEY]: undefined, content_sha: 'ABC', file_hash: 'DEF' },
         ];
-        const updatedFilesList = utils.prepareDiffData({ diff_files: fakeBatch }, priorFiles);
+        const updatedFilesList = utils.prepareDiffData({
+          diff: { diff_files: fakeBatch },
+          priorFiles,
+        });
 
         expect(updatedFilesList).toEqual([
           mock,
@@ -498,7 +507,7 @@ describe('DiffsStoreUtils', () => {
       beforeEach(() => {
         mock = getDiffMetadataMock();
 
-        preparedDiffFiles = utils.prepareDiffData(mock);
+        preparedDiffFiles = utils.prepareDiffData({ diff: mock, meta: true });
       });
 
       it('sets the renderIt and collapsed attribute on files', () => {
@@ -514,7 +523,7 @@ describe('DiffsStoreUtils', () => {
         const fileMock = getDiffFileMock();
         const metaData = getDiffMetadataMock();
         const priorFiles = [fileMock];
-        const updatedFilesList = utils.prepareDiffData(metaData, priorFiles);
+        const updatedFilesList = utils.prepareDiffData({ diff: metaData, priorFiles, meta: true });
 
         expect(updatedFilesList.length).toEqual(2);
         expect(updatedFilesList[0]).toEqual(fileMock);
@@ -539,7 +548,7 @@ describe('DiffsStoreUtils', () => {
         const fileMock = getDiffFileMock();
         const metaMock = getDiffMetadataMock();
         const priorFiles = [{ ...fileMock }];
-        const updatedFilesList = utils.prepareDiffData(metaMock, priorFiles);
+        const updatedFilesList = utils.prepareDiffData({ diff: metaMock, priorFiles, meta: true });
 
         expect(updatedFilesList).toEqual([
           fileMock,

spec/frontend/diffs/utils/diff_file_spec.js

 import { prepareRawDiffFile } from '~/diffs/utils/diff_file';
 
-const DIFF_FILES = [
-  {
-    file_hash: 'ABC', // This file is just a normal file
-  },
-  {
-    file_hash: 'DEF', // This file replaces a symlink
-    a_mode: '0',
-    b_mode: '0755',
-  },
-  {
-    file_hash: 'DEF', // This symlink is replaced by a file
-    a_mode: '120000',
-    b_mode: '0',
-  },
-  {
-    file_hash: 'GHI', // This symlink replaces a file
-    a_mode: '0',
-    b_mode: '120000',
-  },
-  {
-    file_hash: 'GHI', // This file is replaced by a symlink
-    a_mode: '0755',
-    b_mode: '0',
-  },
-];
-
+function getDiffFiles() {
+  return [
+    {
+      file_hash: 'ABC', // This file is just a normal file
+      file_identifier_hash: 'ABC1',
+      content_sha: 'C047347',
+    },
+    {
+      file_hash: 'DEF', // This file replaces a symlink
+      file_identifier_hash: 'DEF1',
+      content_sha: 'C047347',
+      a_mode: '0',
+      b_mode: '0755',
+    },
+    {
+      file_hash: 'DEF', // This symlink is replaced by a file
+      file_identifier_hash: 'DEF2',
+      content_sha: 'C047347',
+      a_mode: '120000',
+      b_mode: '0',
+    },
+    {
+      file_hash: 'GHI', // This symlink replaces a file
+      file_identifier_hash: 'GHI1',
+      content_sha: 'C047347',
+      a_mode: '0',
+      b_mode: '120000',
+    },
+    {
+      file_hash: 'GHI', // This file is replaced by a symlink
+      file_identifier_hash: 'GHI2',
+      content_sha: 'C047347',
+      a_mode: '0755',
+      b_mode: '0',
+    },
+  ];
+}
 function makeBrokenSymlinkObject(replaced, wasSymbolic, isSymbolic, wasReal, isReal) {
   return {
     replaced,
@@ -38,6 +49,12 @@ function makeBrokenSymlinkObject(replaced, wasSymbolic, isSymbolic, wasReal, isR
 
 describe('diff_file utilities', () => {
   describe('prepareRawDiffFile', () => {
+    let files;
+
+    beforeEach(() => {
+      files = getDiffFiles();
+    });
+
     it.each`
       fileIndex | description                               | brokenSymlink
       ${0}      | ${'a file that is not symlink-adjacent'}  | ${false}
@@ -49,12 +66,51 @@ describe('diff_file utilities', () => {
       'properly marks $description with the correct .brokenSymlink value',
       ({ fileIndex, brokenSymlink }) => {
         const preppedRaw = prepareRawDiffFile({
-          file: DIFF_FILES[fileIndex],
-          allFiles: DIFF_FILES,
+          file: files[fileIndex],
+          allFiles: files,
         });
 
         expect(preppedRaw.brokenSymlink).toStrictEqual(brokenSymlink);
       },
     );
+
+    it.each`
+      fileIndex | id
+      ${0}      | ${'e075da30-4ec7-4e1c-a505-fe0fb0efe2d8'}
+      ${1}      | ${'5ab05419-123e-4d18-8454-0b8c3d9f3f91'}
+      ${2}      | ${'94eb6bba-575c-4504-bd8e-5d302364d31e'}
+      ${3}      | ${'06d669b2-29b7-4f47-9731-33fc38a8db61'}
+      ${4}      | ${'edd3e8f9-07f9-4647-8171-544c72e5a175'}
+    `('sets the file id properly { id: $id } on normal diff files', ({ fileIndex, id }) => {
+      const preppedFile = prepareRawDiffFile({
+        file: files[fileIndex],
+        allFiles: files,
+      });
+
+      expect(preppedFile.id).toBe(id);
+    });
+
+    it('does not set the `id` property for metadata diff files', () => {
+      const preppedFile = prepareRawDiffFile({
+        file: files[0],
+        allFiles: files,
+        meta: true,
+      });
+
+      expect(preppedFile).not.toHaveProp('id');
+    });
+
+    it('does not set the id property if the file is missing a `content_sha`', () => {
+      const fileMissingContentSha = { ...files[0] };
+
+      delete fileMissingContentSha.content_sha;
+
+      const preppedFile = prepareRawDiffFile({
+        file: fileMissingContentSha,
+        allFiles: files,
+      });
+
+      expect(preppedFile).not.toHaveProp('id');
+    });
   });
 });