Merge branch 'tor/defect/unresolved-discussions-sort-order' into 'master'

Fix algorithmic sort order of unresolved discussions

See merge request gitlab-org/gitlab!85536

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

@@ -93,6 +93,27 @@ export function getShortShaFromFile(file) {
   return file.content_sha ? truncateSha(String(file.content_sha)) : null;
 }
+export function match({ fileA, fileB, mode = 'universal' } = {}) {
+  const matching = {
+    universal: (a, b) => (a?.id && b?.id ? a.id === b.id : false),
+    /*
+     * MR mode can be wildly incorrect if there is ever the possibility of files from multiple MRs
+     *  (e.g. a browser-local merge request/file cache).
+     * That's why the default here is "universal" mode: UUIDs can't conflict, but you can opt into
+     *  the dangerous one.
+     *
+     * For reference:
+     *    file_identifier_hash === sha1( `${filePath}-${Boolean(isNew)}-${Boolean(isDeleted)}-${Boolean(isRenamed)}` )
+     */
+    mr: (a, b) =>
+      a?.file_identifier_hash && b?.file_identifier_hash
+        ? a.file_identifier_hash === b.file_identifier_hash
+        : false,
+  };
+
+  return (matching[mode] || (() => false))(fileA, fileB);
+}
+
 export function stats(file) {
   let valid = false;
   let classes = '';

app/assets/javascripts/notes/stores/getters.js

 import { flattenDeep, clone } from 'lodash';
+import { match } from '~/diffs/utils/diff_file';
 import { statusBoxState } from '~/issuable/components/status_box.vue';
 import { isInMRPage } from '~/lib/utils/common_utils';
 import * as constants from '../constants';
@@ -179,29 +180,42 @@ export const unresolvedDiscussionsIdsByDate = (state, getters) =>
 // Sorts the array of resolvable yet unresolved discussions by
 // comparing file names first. If file names are the same, compares
 // line numbers.
-export const unresolvedDiscussionsIdsByDiff = (state, getters) =>
-  getters.allResolvableDiscussions
+export const unresolvedDiscussionsIdsByDiff = (state, getters, allState) => {
+  const authoritativeFiles = allState.diffs.diffFiles;
+
+  return getters.allResolvableDiscussions
     .filter((d) => !d.resolved && d.active)
     .sort((a, b) => {
+      let order = 0;
+
       if (!a.diff_file || !b.diff_file) {
-        return 0;
+        return order;
       }
-      // Get file names comparison result
-      const filenameComparison = a.diff_file.file_path.localeCompare(b.diff_file.file_path);
+      const authoritativeA = authoritativeFiles.find((source) =>
+        match({ fileA: source, fileB: a.diff_file, mode: 'mr' }),
+      );
+      const authoritativeB = authoritativeFiles.find((source) =>
+        match({ fileA: source, fileB: b.diff_file, mode: 'mr' }),
+      );
+
+      if (authoritativeA && authoritativeB) {
+        order = authoritativeA.order - authoritativeB.order;
+      }
       // Get the line numbers, to compare within the same file
       const aLines = [a.position.new_line, a.position.old_line];
       const bLines = [b.position.new_line, b.position.old_line];
-      return filenameComparison < 0 ||
-        (filenameComparison === 0 &&
+      return order < 0 ||
+        (order === 0 &&
           // .max() because one of them might be zero (if removed/added)
           Math.max(aLines[0], aLines[1]) < Math.max(bLines[0], bLines[1]))
         ? -1
         : 1;
     })
     .map((d) => d.id);
+};
 export const resolvedDiscussionCount = (state, getters) => {
   const resolvedMap = getters.resolvedDiscussionsById;

spec/frontend/diffs/utils/diff_file_spec.js

@@ -3,6 +3,7 @@ import {
   getShortShaFromFile,
   stats,
   isNotDiffable,
+  match,
 } from '~/diffs/utils/diff_file';
 import { diffViewerModes } from '~/ide/constants';
 import mockDiffFile from '../mock_data/diff_file';
@@ -262,4 +263,42 @@ describe('diff_file utilities', () => {
       expect(isNotDiffable(file)).toBe(false);
     });
   });
+
+  describe('match', () => {
+    const authorityFileId = '68296a4f-f1c7-445a-bd0e-6e3b02c4eec0';
+    const fih = 'file_identifier_hash';
+    const fihs = 'file identifier hashes';
+    let authorityFile;
+
+    beforeAll(() => {
+      const files = getDiffFiles();
+
+      authorityFile = prepareRawDiffFile({
+        file: files[0],
+        allFiles: files,
+      });
+
+      Object.freeze(authorityFile);
+    });
+
+    describe.each`
+      mode           | comparisonFiles                                                    | keyName
+      ${'universal'} | ${[{ [fih]: 'ABC1' }, { id: 'foo' }, { id: authorityFileId }]}     | ${'ids'}
+      ${'mr'}        | ${[{ id: authorityFileId }, { [fih]: 'ABC2' }, { [fih]: 'ABC1' }]} | ${fihs}
+    `('$mode mode', ({ mode, comparisonFiles, keyName }) => {
+      it(`fails to match if files or ${keyName} aren't present`, () => {
+        expect(match({ fileA: authorityFile, fileB: undefined, mode })).toBe(false);
+        expect(match({ fileA: authorityFile, fileB: null, mode })).toBe(false);
+        expect(match({ fileA: authorityFile, fileB: comparisonFiles[0], mode })).toBe(false);
+      });
+
+      it(`fails to match if the ${keyName} aren't the same`, () => {
+        expect(match({ fileA: authorityFile, fileB: comparisonFiles[1], mode })).toBe(false);
+      });
+
+      it(`matches if the ${keyName} are the same`, () => {
+        expect(match({ fileA: authorityFile, fileB: comparisonFiles[2], mode })).toBe(true);
+      });
+    });
+  });
 });

spec/frontend/notes/mixins/discussion_navigation_spec.js

@@ -59,6 +59,7 @@ describe('Discussion navigation mixin', () => {
         diffs: {
           namespaced: true,
           actions: { scrollToFile },
+          state: { diffFiles: [] },
         },
       },
     });

spec/frontend/notes/mock_data.js

@@ -1171,7 +1171,7 @@ export const discussion1 = {
   resolved: false,
   active: true,
   diff_file: {
-    file_path: 'about.md',
+    file_identifier_hash: 'discfile1',
   },
   position: {
     new_line: 50,
@@ -1189,7 +1189,7 @@ export const resolvedDiscussion1 = {
   resolvable: true,
   resolved: true,
   diff_file: {
-    file_path: 'about.md',
+    file_identifier_hash: 'discfile1',
   },
   position: {
     new_line: 50,
@@ -1208,7 +1208,7 @@ export const discussion2 = {
   resolved: false,
   active: true,
   diff_file: {
-    file_path: 'README.md',
+    file_identifier_hash: 'discfile2',
   },
   position: {
     new_line: null,
@@ -1227,7 +1227,7 @@ export const discussion3 = {
   active: true,
   resolved: false,
   diff_file: {
-    file_path: 'README.md',
+    file_identifier_hash: 'discfile3',
   },
   position: {
     new_line: 21,
@@ -1240,6 +1240,12 @@ export const discussion3 = {
   ],
 };
+export const authoritativeDiscussionFile = {
+  id: 'abc',
+  file_identifier_hash: 'discfile1',
+  order: 0,
+};
+
 export const unresolvableDiscussion = {
   resolvable: false,
 };

spec/frontend/notes/stores/getters_spec.js

@@ -12,6 +12,7 @@ import {
   discussion2,
   discussion3,
   resolvedDiscussion1,
+  authoritativeDiscussionFile,
   unresolvableDiscussion,
   draftComments,
   draftReply,
@@ -26,6 +27,23 @@ const createDiscussionNeighborParams = (discussionId, diffOrder, step) => ({
 });
 const asDraftDiscussion = (x) => ({ ...x, individual_note: true });
+const createRootState = () => {
+  return {
+    diffs: {
+      diffFiles: [
+        { ...authoritativeDiscussionFile },
+        {
+          ...authoritativeDiscussionFile,
+          ...{ id: 'abc2', file_identifier_hash: 'discfile2', order: 1 },
+        },
+        {
+          ...authoritativeDiscussionFile,
+          ...{ id: 'abc3', file_identifier_hash: 'discfile3', order: 2 },
+        },
+      ],
+    },
+  };
+};
 describe('Getters Notes Store', () => {
   let state;
@@ -226,20 +244,84 @@ describe('Getters Notes Store', () => {
       const localGetters = {
         allResolvableDiscussions: [discussion3, discussion1, discussion2],
       };
-      expect(getters.unresolvedDiscussionsIdsByDiff(state, localGetters)).toEqual([
+      const rootState = createRootState();
+      expect(getters.unresolvedDiscussionsIdsByDiff(state, localGetters, rootState)).toEqual([
         'abc1',
         'abc2',
         'abc3',
       ]);
     });
+    // This is the same test as above, but it exercises the sorting algorithm
+    // for a "strange" Diff File ordering. The intent is to ensure that even if lots
+    // of shuffling has to occur, everything still works
+
+    it('should return all discussions IDs in unusual diff order', () => {
+      const localGetters = {
+        allResolvableDiscussions: [discussion3, discussion1, discussion2],
+      };
+      const rootState = {
+        diffs: {
+          diffFiles: [
+            // 2 is first, but should sort 2nd
+            {
+              ...authoritativeDiscussionFile,
+              ...{ id: 'abc2', file_identifier_hash: 'discfile2', order: 1 },
+            },
+            // 1 is second, but should sort 3rd
+            { ...authoritativeDiscussionFile, ...{ order: 2 } },
+            // 3 is third, but should sort 1st
+            {
+              ...authoritativeDiscussionFile,
+              ...{ id: 'abc3', file_identifier_hash: 'discfile3', order: 0 },
+            },
+          ],
+        },
+      };
+
+      expect(getters.unresolvedDiscussionsIdsByDiff(state, localGetters, rootState)).toEqual([
+        'abc3',
+        'abc2',
+        'abc1',
+      ]);
+    });
+
+    it("should use the discussions array order if the files don't have explicit order values", () => {
+      const localGetters = {
+        allResolvableDiscussions: [discussion3, discussion1, discussion2], // This order is used!
+      };
+      const auth1 = { ...authoritativeDiscussionFile };
+      const auth2 = {
+        ...authoritativeDiscussionFile,
+        ...{ id: 'abc2', file_identifier_hash: 'discfile2' },
+      };
+      const auth3 = {
+        ...authoritativeDiscussionFile,
+        ...{ id: 'abc3', file_identifier_hash: 'discfile3' },
+      };
+      const rootState = {
+        diffs: { diffFiles: [auth2, auth1, auth3] }, // This order is not used!
+      };
+
+      delete auth1.order;
+      delete auth2.order;
+      delete auth3.order;
+
+      expect(getters.unresolvedDiscussionsIdsByDiff(state, localGetters, rootState)).toEqual([
+        'abc3',
+        'abc1',
+        'abc2',
+      ]);
+    });
+
     it('should return empty array if all discussions have been resolved', () => {
       const localGetters = {
         allResolvableDiscussions: [resolvedDiscussion1],
       };
-      expect(getters.unresolvedDiscussionsIdsByDiff(state, localGetters)).toEqual([]);
+      const rootState = createRootState();
+      expect(getters.unresolvedDiscussionsIdsByDiff(state, localGetters, rootState)).toEqual([]);
     });
   });