diff --git a/app/assets/javascripts/diffs/utils/diff_file.js b/app/assets/javascripts/diffs/utils/diff_file.js index a7251bfa7756f5d45f5bd6becd64ed4c949c1a1e..bcd9fa012786beb90bd1a0bcd0c47276c49a6743 100644 --- a/app/assets/javascripts/diffs/utils/diff_file.js +++ b/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 = ''; diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js index a710ac0ccf5851de3f871e0d48225689afdec5f7..9d184b14878380ba5a75be6deda95b904b1caca0 100644 --- a/app/assets/javascripts/notes/stores/getters.js +++ b/app/assets/javascripts/notes/stores/getters.js @@ -1,4 +1,5 @@ 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; diff --git a/spec/frontend/diffs/utils/diff_file_spec.js b/spec/frontend/diffs/utils/diff_file_spec.js index 3a6a537f92457e4c35a08fc4d14e7d5d8acadea9..778897be3bad92ac9665e77d20d8ded7ac16fcbb 100644 --- a/spec/frontend/diffs/utils/diff_file_spec.js +++ b/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); + }); + }); + }); }); diff --git a/spec/frontend/notes/mixins/discussion_navigation_spec.js b/spec/frontend/notes/mixins/discussion_navigation_spec.js index aba80789a013b1310c9b682d4cfeecaae1e13f3c..35b3dec6298a81c1c984e04775696cb011fffeef 100644 --- a/spec/frontend/notes/mixins/discussion_navigation_spec.js +++ b/spec/frontend/notes/mixins/discussion_navigation_spec.js @@ -59,6 +59,7 @@ describe('Discussion navigation mixin', () => { diffs: { namespaced: true, actions: { scrollToFile }, + state: { diffFiles: [] }, }, }, }); diff --git a/spec/frontend/notes/mock_data.js b/spec/frontend/notes/mock_data.js index a4aeeda48d8b34b4d8c9f5c6750b257a3a5ed3ee..c7a6ca5eae35548076841c52496da0d91dd236b0 100644 --- a/spec/frontend/notes/mock_data.js +++ b/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, }; diff --git a/spec/frontend/notes/stores/getters_spec.js b/spec/frontend/notes/stores/getters_spec.js index 9a11fdba50851206a6d7b0e0edf646af54cc1bc1..6d078dcefcf4ae18baa8258c1a16e0fabd1b89ad 100644 --- a/spec/frontend/notes/stores/getters_spec.js +++ b/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], }; + const rootState = createRootState(); - expect(getters.unresolvedDiscussionsIdsByDiff(state, localGetters)).toEqual([ + 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], }; + const rootState = createRootState(); - expect(getters.unresolvedDiscussionsIdsByDiff(state, localGetters)).toEqual([]); + expect(getters.unresolvedDiscussionsIdsByDiff(state, localGetters, rootState)).toEqual([]); }); });