From 4557097f5b72c09a7300268cb1dd7e88519cf395 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 21 Apr 2022 13:13:31 -0600 Subject: [PATCH 1/4] Use the authoritative file sort to order discussions Jump to next unresolved discussion follows the displayed UI order. Changelog: fixed --- .../javascripts/diffs/utils/diff_file.js | 21 ++++++++++++++ .../javascripts/notes/stores/getters.js | 28 ++++++++++++++----- 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/diffs/utils/diff_file.js b/app/assets/javascripts/diffs/utils/diff_file.js index a7251bfa7756f5..bcd9fa012786be 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 a710ac0ccf5851..7612a0074c252b 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({ mode: 'mr', fileA: source, fileB: a.diff_file }), + ); + const authoritativeB = authoritativeFiles.find((source) => + match({ mode: 'mr', fileA: source, fileB: b.diff_file }), + ); + + 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; -- GitLab From 31d6028fd80909a71786f0f211ec08952a96c9f2 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 21 Apr 2022 13:15:09 -0600 Subject: [PATCH 2/4] Test the new sorting algorithm Since discussions now use an explicit `order` property on the canonical Diff file to sort, test all of the ways that should work (and the ways it should fail gracefully). Notably, this also tests the matching behavior coming from the Diffs app utility for diff files: `match`. --- spec/frontend/diffs/utils/diff_file_spec.js | 59 +++++++++++++ .../mixins/discussion_navigation_spec.js | 1 + spec/frontend/notes/mock_data.js | 14 ++- spec/frontend/notes/stores/getters_spec.js | 86 ++++++++++++++++++- 4 files changed, 154 insertions(+), 6 deletions(-) diff --git a/spec/frontend/diffs/utils/diff_file_spec.js b/spec/frontend/diffs/utils/diff_file_spec.js index 3a6a537f92457e..cfac73d930278b 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,62 @@ describe('diff_file utilities', () => { expect(isNotDiffable(file)).toBe(false); }); }); + + describe('match', () => { + const authorityFileId = '68296a4f-f1c7-445a-bd0e-6e3b02c4eec0'; + let authorityFile; + + beforeAll(() => { + const files = getDiffFiles(); + + authorityFile = prepareRawDiffFile({ + file: files[0], + allFiles: files, + }); + + Object.freeze(authorityFile); + }); + + describe('universal mode', () => { + const mode = 'universal'; + + it("fails to match if files or ids 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: { file_identifier_hash: 'ABC1' }, mode })).toBe( + false, + ); + }); + + it("fails to match if the ids aren't the same", () => { + expect(match({ fileA: authorityFile, fileB: { id: 'foo' }, mode })).toBe(false); + }); + + it('matches if the ids are the same', () => { + expect(match({ fileA: authorityFile, fileB: { id: authorityFileId }, mode })).toBe(true); + }); + }); + + describe('mr mode', () => { + const mode = 'mr'; + + it("fails to match if files or file identifier hashes 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: { id: authorityFileId }, mode })).toBe(false); + }); + + it("fails to match if the file identifier hashes aren't the same", () => { + expect(match({ fileA: authorityFile, fileB: { file_identifier_hash: 'ABC2' }, mode })).toBe( + false, + ); + }); + + it('matches if the file identifier hashes are the same', () => { + expect(match({ fileA: authorityFile, fileB: { file_identifier_hash: 'ABC1' }, mode })).toBe( + true, + ); + }); + }); + }); }); diff --git a/spec/frontend/notes/mixins/discussion_navigation_spec.js b/spec/frontend/notes/mixins/discussion_navigation_spec.js index aba80789a013b1..35b3dec6298a81 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 a4aeeda48d8b34..c7a6ca5eae3554 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 9a11fdba508512..6d078dcefcf4ae 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([]); }); }); -- GitLab From 3e50bafa3b1f3b56810bd4f7e035edb367e3beed Mon Sep 17 00:00:00 2001 From: Anna Vovchenko Date: Tue, 26 Apr 2022 18:47:03 +0000 Subject: [PATCH 3/4] Re-order arguments to Diff File `match` like the signature --- app/assets/javascripts/notes/stores/getters.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js index 7612a0074c252b..9d184b14878380 100644 --- a/app/assets/javascripts/notes/stores/getters.js +++ b/app/assets/javascripts/notes/stores/getters.js @@ -193,10 +193,10 @@ export const unresolvedDiscussionsIdsByDiff = (state, getters, allState) => { } const authoritativeA = authoritativeFiles.find((source) => - match({ mode: 'mr', fileA: source, fileB: a.diff_file }), + match({ fileA: source, fileB: a.diff_file, mode: 'mr' }), ); const authoritativeB = authoritativeFiles.find((source) => - match({ mode: 'mr', fileA: source, fileB: b.diff_file }), + match({ fileA: source, fileB: b.diff_file, mode: 'mr' }), ); if (authoritativeA && authoritativeB) { -- GitLab From 4126c12f4ed471477b4bf76410f3dcd20425486b Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 27 Apr 2022 01:33:11 -0600 Subject: [PATCH 4/4] Use describe.each to reduce test duplication --- spec/frontend/diffs/utils/diff_file_spec.js | 46 ++++++--------------- 1 file changed, 13 insertions(+), 33 deletions(-) diff --git a/spec/frontend/diffs/utils/diff_file_spec.js b/spec/frontend/diffs/utils/diff_file_spec.js index cfac73d930278b..778897be3bad92 100644 --- a/spec/frontend/diffs/utils/diff_file_spec.js +++ b/spec/frontend/diffs/utils/diff_file_spec.js @@ -266,6 +266,8 @@ describe('diff_file utilities', () => { describe('match', () => { const authorityFileId = '68296a4f-f1c7-445a-bd0e-6e3b02c4eec0'; + const fih = 'file_identifier_hash'; + const fihs = 'file identifier hashes'; let authorityFile; beforeAll(() => { @@ -279,45 +281,23 @@ describe('diff_file utilities', () => { Object.freeze(authorityFile); }); - describe('universal mode', () => { - const mode = 'universal'; - - it("fails to match if files or ids 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: { file_identifier_hash: 'ABC1' }, mode })).toBe( - false, - ); - }); - - it("fails to match if the ids aren't the same", () => { - expect(match({ fileA: authorityFile, fileB: { id: 'foo' }, mode })).toBe(false); - }); - - it('matches if the ids are the same', () => { - expect(match({ fileA: authorityFile, fileB: { id: authorityFileId }, mode })).toBe(true); - }); - }); - - describe('mr mode', () => { - const mode = 'mr'; - - it("fails to match if files or file identifier hashes aren't present", () => { + 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: { id: authorityFileId }, mode })).toBe(false); + expect(match({ fileA: authorityFile, fileB: comparisonFiles[0], mode })).toBe(false); }); - it("fails to match if the file identifier hashes aren't the same", () => { - expect(match({ fileA: authorityFile, fileB: { file_identifier_hash: 'ABC2' }, 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 file identifier hashes are the same', () => { - expect(match({ fileA: authorityFile, fileB: { file_identifier_hash: 'ABC1' }, mode })).toBe( - true, - ); + it(`matches if the ${keyName} are the same`, () => { + expect(match({ fileA: authorityFile, fileB: comparisonFiles[2], mode })).toBe(true); }); }); }); -- GitLab