From fa9a857f55e3e073e783d05eb477695880892077 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 3 Oct 2022 20:32:19 -0600 Subject: [PATCH 1/5] Disable commenting on lines that will fail to save the comment 1. Lines must have a line code to save a comment 2. Lines in files that are only moved HAVE a line code, but still fail. Changelog: fixed --- .../javascripts/diffs/components/diff_row_utils.js | 12 ++++++++---- app/assets/javascripts/diffs/store/utils.js | 9 ++++++++- app/assets/stylesheets/pages/notes.scss | 2 +- locale/gitlab.pot | 6 ++++++ 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/diffs/components/diff_row_utils.js b/app/assets/javascripts/diffs/components/diff_row_utils.js index 7732badde341f2..2da44cfc73507c 100644 --- a/app/assets/javascripts/diffs/components/diff_row_utils.js +++ b/app/assets/javascripts/diffs/components/diff_row_utils.js @@ -60,18 +60,22 @@ export const addCommentTooltip = (line) => { if (!line) return tooltip; tooltip = __('Add a comment to this line or drag for multiple lines'); - const brokenSymlinks = line.commentsDisabled; + const { brokenSymlink, brokenLineCode, fileOnlyMoved } = line.problems; - if (brokenSymlinks) { - if (brokenSymlinks.wasSymbolic || brokenSymlinks.isSymbolic) { + if (brokenSymlink) { + if (brokenSymlink.wasSymbolic || brokenSymlink.isSymbolic) { tooltip = __( 'Commenting on symbolic links that replace or are replaced by files is currently not supported.', ); - } else if (brokenSymlinks.wasReal || brokenSymlinks.isReal) { + } else if (brokenSymlink.wasReal || brokenSymlink.isReal) { tooltip = __( 'Commenting on files that replace or are replaced by symbolic links is currently not supported.', ); } + } else if (fileOnlyMoved) { + tooltip = __('Commenting on files that are only moved or renamed is currently not supported'); + } else if (brokenLineCode) { + tooltip = __('Commenting on this line is currently not supported'); } return tooltip; diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index cf86ebea4a9f76..bd3fbff0450516 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -324,15 +324,22 @@ function cleanRichText(text) { } function prepareLine(line, file) { + const problems = { + brokenSymlink: file.brokenSymlink, + brokenLineCode: line.line_code === null, + fileOnlyMoved: file.renamed_file && file.added_lines === 0 && file.removed_lines === 0, + }; + if (!line.alreadyPrepared) { Object.assign(line, { - commentsDisabled: file.brokenSymlink, + commentsDisabled: problems.brokenSymlink || problems.fileOnlyMoved || problems.brokenLineCode, rich_text: cleanRichText(line.rich_text), discussionsExpanded: true, discussions: [], hasForm: false, text: undefined, alreadyPrepared: true, + problems, }); } } diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index 438b7b1afa6b8c..955dcdc1c0f3b3 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -968,7 +968,7 @@ $system-note-svg-size: 1rem; height: 12px; } - &:hover, + &:hover:not([disabled]), &.inverted { &::before { background-color: $white; diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 6ba1f00376b0f8..cc45e95ef47a84 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -9700,12 +9700,18 @@ msgstr "" msgid "Comment/Reply (quoting selected text)" msgstr "" +msgid "Commenting on files that are only moved or renamed is currently not supported" +msgstr "" + msgid "Commenting on files that replace or are replaced by symbolic links is currently not supported." msgstr "" msgid "Commenting on symbolic links that replace or are replaced by files is currently not supported." msgstr "" +msgid "Commenting on this line is currently not supported" +msgstr "" + msgid "Comments" msgstr "" -- GitLab From f45baa88889716bcb8f3e8afa077f900df9717bf Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 5 Oct 2022 16:38:12 -0600 Subject: [PATCH 2/5] Update tests to include new "problems" property of diff lines --- .../diffs/components/diff_row_utils_spec.js | 41 +++++++++++++------ spec/frontend/diffs/mock_data/diff_file.js | 30 ++++++++++++++ 2 files changed, 58 insertions(+), 13 deletions(-) diff --git a/spec/frontend/diffs/components/diff_row_utils_spec.js b/spec/frontend/diffs/components/diff_row_utils_spec.js index 8b25691ce34dcf..ff1849681ff184 100644 --- a/spec/frontend/diffs/components/diff_row_utils_spec.js +++ b/spec/frontend/diffs/components/diff_row_utils_spec.js @@ -9,6 +9,18 @@ import { const LINE_CODE = 'abc123'; +function problemsClone({ + brokenSymlink = false, + brokenLineCode = false, + fileOnlyMoved = false, +} = {}) { + return { + brokenSymlink, + brokenLineCode, + fileOnlyMoved, + }; +} + describe('isHighlighted', () => { it('should return true if line is highlighted', () => { const line = { line_code: LINE_CODE }; @@ -147,25 +159,27 @@ describe('addCommentTooltip', () => { }); it('should return drag comment tooltip when dragging is enabled', () => { - expect(utils.addCommentTooltip({})).toEqual(dragTooltip); + expect(utils.addCommentTooltip({ problems: problemsClone() })).toEqual(dragTooltip); }); it('should return broken symlink tooltip', () => { - expect(utils.addCommentTooltip({ commentsDisabled: { wasSymbolic: true } })).toEqual( - brokenSymLinkTooltip, - ); - expect(utils.addCommentTooltip({ commentsDisabled: { isSymbolic: true } })).toEqual( - brokenSymLinkTooltip, - ); + expect( + utils.addCommentTooltip({ + problems: problemsClone({ brokenSymlink: { wasSymbolic: true } }), + }), + ).toEqual(brokenSymLinkTooltip); + expect( + utils.addCommentTooltip({ problems: problemsClone({ brokenSymlink: { isSymbolic: true } }) }), + ).toEqual(brokenSymLinkTooltip); }); it('should return broken real tooltip', () => { - expect(utils.addCommentTooltip({ commentsDisabled: { wasReal: true } })).toEqual( - brokenRealTooltip, - ); - expect(utils.addCommentTooltip({ commentsDisabled: { isReal: true } })).toEqual( - brokenRealTooltip, - ); + expect( + utils.addCommentTooltip({ problems: problemsClone({ brokenSymlink: { wasReal: true } }) }), + ).toEqual(brokenRealTooltip); + expect( + utils.addCommentTooltip({ problems: problemsClone({ brokenSymlink: { isReal: true } }) }), + ).toEqual(brokenRealTooltip); }); }); @@ -211,6 +225,7 @@ describe('mapParallel', () => { discussions: [{}], discussionsExpanded: true, hasForm: true, + problems: problemsClone(), }; const content = { diffFile: {}, diff --git a/spec/frontend/diffs/mock_data/diff_file.js b/spec/frontend/diffs/mock_data/diff_file.js index dd200b0248c91d..e0e5778e0d56f1 100644 --- a/spec/frontend/diffs/mock_data/diff_file.js +++ b/spec/frontend/diffs/mock_data/diff_file.js @@ -1,3 +1,11 @@ +function problemsClone() { + return { + brokenSymlink: false, + brokenLineCode: false, + fileOnlyMoved: false, + }; +} + export const getDiffFileMock = () => ({ submodule: false, submodule_link: null, @@ -61,6 +69,7 @@ export const getDiffFileMock = () => ({ text: ' - Bad dates\n', rich_text: ' - Bad dates\n', meta_data: null, + problems: problemsClone(), }, { line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_2', @@ -71,6 +80,7 @@ export const getDiffFileMock = () => ({ text: '\n', rich_text: '\n', meta_data: null, + problems: problemsClone(), }, { line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_3', @@ -81,6 +91,7 @@ export const getDiffFileMock = () => ({ text: 'v6.8.0\n', rich_text: 'v6.8.0\n', meta_data: null, + problems: problemsClone(), }, { line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_2_4', @@ -91,6 +102,7 @@ export const getDiffFileMock = () => ({ text: '\n', rich_text: '\n', meta_data: null, + problems: problemsClone(), }, { line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_3_5', @@ -101,6 +113,7 @@ export const getDiffFileMock = () => ({ text: 'v6.7.0\n', rich_text: 'v6.7.0\n', meta_data: null, + problems: problemsClone(), }, { line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_6', @@ -111,6 +124,7 @@ export const getDiffFileMock = () => ({ text: '\n', rich_text: '\n', meta_data: null, + problems: problemsClone(), }, { line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_7', @@ -121,6 +135,7 @@ export const getDiffFileMock = () => ({ text: '\n', rich_text: '\n', meta_data: null, + problems: problemsClone(), }, { line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_9', @@ -131,6 +146,7 @@ export const getDiffFileMock = () => ({ text: '\n', rich_text: '\n', meta_data: null, + problems: problemsClone(), }, { line_code: null, @@ -144,6 +160,7 @@ export const getDiffFileMock = () => ({ old_pos: 3, new_pos: 5, }, + problems: problemsClone(), }, ], parallel_diff_lines: [ @@ -158,6 +175,7 @@ export const getDiffFileMock = () => ({ text: ' - Bad dates\n', rich_text: ' - Bad dates\n', meta_data: null, + problems: problemsClone(), }, }, { @@ -171,6 +189,7 @@ export const getDiffFileMock = () => ({ text: '\n', rich_text: '\n', meta_data: null, + problems: problemsClone(), }, }, { @@ -183,6 +202,7 @@ export const getDiffFileMock = () => ({ text: 'v6.8.0\n', rich_text: 'v6.8.0\n', meta_data: null, + problems: problemsClone(), }, right: { line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_3', @@ -193,6 +213,7 @@ export const getDiffFileMock = () => ({ text: 'v6.8.0\n', rich_text: 'v6.8.0\n', meta_data: null, + problems: problemsClone(), }, }, { @@ -205,6 +226,7 @@ export const getDiffFileMock = () => ({ text: '\n', rich_text: '\n', meta_data: null, + problems: problemsClone(), }, right: { line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_2_4', @@ -215,6 +237,7 @@ export const getDiffFileMock = () => ({ text: '\n', rich_text: '\n', meta_data: null, + problems: problemsClone(), }, }, { @@ -227,6 +250,7 @@ export const getDiffFileMock = () => ({ text: 'v6.7.0\n', rich_text: 'v6.7.0\n', meta_data: null, + problems: problemsClone(), }, right: { line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_3_5', @@ -237,6 +261,7 @@ export const getDiffFileMock = () => ({ text: 'v6.7.0\n', rich_text: 'v6.7.0\n', meta_data: null, + problems: problemsClone(), }, }, { @@ -249,6 +274,7 @@ export const getDiffFileMock = () => ({ text: '\n', rich_text: '\n', meta_data: null, + problems: problemsClone(), }, right: { line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_7', @@ -259,6 +285,7 @@ export const getDiffFileMock = () => ({ text: '\n', rich_text: '\n', meta_data: null, + problems: problemsClone(), }, }, { @@ -272,6 +299,7 @@ export const getDiffFileMock = () => ({ text: '\n', rich_text: '\n', meta_data: null, + problems: problemsClone(), }, }, { @@ -287,6 +315,7 @@ export const getDiffFileMock = () => ({ old_pos: 3, new_pos: 5, }, + problems: problemsClone(), }, right: { line_code: null, @@ -300,6 +329,7 @@ export const getDiffFileMock = () => ({ old_pos: 3, new_pos: 5, }, + problems: problemsClone(), }, }, ], -- GitLab From da92c88ba988471e71641ee6b6dcfba5163a93bd Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 5 Oct 2022 17:42:34 -0600 Subject: [PATCH 3/5] Test the new tooltips --- .../diffs/components/diff_row_utils_spec.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/spec/frontend/diffs/components/diff_row_utils_spec.js b/spec/frontend/diffs/components/diff_row_utils_spec.js index ff1849681ff184..d0437808a179f7 100644 --- a/spec/frontend/diffs/components/diff_row_utils_spec.js +++ b/spec/frontend/diffs/components/diff_row_utils_spec.js @@ -152,6 +152,9 @@ describe('addCommentTooltip', () => { 'Commenting on symbolic links that replace or are replaced by files is currently not supported.'; const brokenRealTooltip = 'Commenting on files that replace or are replaced by symbolic links is currently not supported.'; + const lineMovedOrRenamedFileTooltip = + 'Commenting on files that are only moved or renamed is currently not supported'; + const lineWithNoLineCodeTooltip = 'Commenting on this line is currently not supported'; const dragTooltip = 'Add a comment to this line or drag for multiple lines'; it('should return default tooltip', () => { @@ -181,6 +184,18 @@ describe('addCommentTooltip', () => { utils.addCommentTooltip({ problems: problemsClone({ brokenSymlink: { isReal: true } }) }), ).toEqual(brokenRealTooltip); }); + + it('reports a tooltip when the line is in a file that has only been moved or renamed', () => { + expect(utils.addCommentTooltip({ problems: problemsClone({ fileOnlyMoved: true }) })).toEqual( + lineMovedOrRenamedFileTooltip, + ); + }); + + it("reports a tooltip when the line doesn't have a line code to leave a comment on", () => { + expect(utils.addCommentTooltip({ problems: problemsClone({ brokenLineCode: true }) })).toEqual( + lineWithNoLineCodeTooltip, + ); + }); }); describe('parallelViewLeftLineType', () => { -- GitLab From 435a13e182f22159af20d8b2ea1053c5aa21c93b Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 14 Oct 2022 12:12:58 -0600 Subject: [PATCH 4/5] Test updated diff file / diff line formatting --- app/assets/javascripts/diffs/store/utils.js | 6 ++- spec/frontend/diffs/store/utils_spec.js | 54 +++++++++++++++------ 2 files changed, 42 insertions(+), 18 deletions(-) diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index bd3fbff0450516..0519ca3d7151e6 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -326,13 +326,15 @@ function cleanRichText(text) { function prepareLine(line, file) { const problems = { brokenSymlink: file.brokenSymlink, - brokenLineCode: line.line_code === null, + brokenLineCode: !line.line_code, fileOnlyMoved: file.renamed_file && file.added_lines === 0 && file.removed_lines === 0, }; if (!line.alreadyPrepared) { Object.assign(line, { - commentsDisabled: problems.brokenSymlink || problems.fileOnlyMoved || problems.brokenLineCode, + commentsDisabled: Boolean( + problems.brokenSymlink || problems.fileOnlyMoved || problems.brokenLineCode, + ), rich_text: cleanRichText(line.rich_text), discussionsExpanded: true, discussions: [], diff --git a/spec/frontend/diffs/store/utils_spec.js b/spec/frontend/diffs/store/utils_spec.js index 3f870a98396d02..b5c44b084d8b90 100644 --- a/spec/frontend/diffs/store/utils_spec.js +++ b/spec/frontend/diffs/store/utils_spec.js @@ -311,9 +311,14 @@ describe('DiffsStoreUtils', () => { describe('prepareLineForRenamedFile', () => { const diffFile = { file_hash: 'file-hash', + brokenSymlink: false, + renamed_file: false, + added_lines: 1, + removed_lines: 1, }; const lineIndex = 4; const sourceLine = { + line_code: 'abc', foo: 'test', rich_text: '

rich

', // Note the leading space }; @@ -328,6 +333,12 @@ describe('DiffsStoreUtils', () => { hasForm: false, text: undefined, alreadyPrepared: true, + commentsDisabled: false, + problems: { + brokenLineCode: false, + brokenSymlink: false, + fileOnlyMoved: false, + }, }; let preppedLine; @@ -360,24 +371,35 @@ describe('DiffsStoreUtils', () => { }); it.each` - brokenSymlink - ${false} - ${{}} - ${'anything except `false`'} + brokenSymlink | renamed | added | removed | lineCode | commentsDisabled + ${false} | ${false} | ${0} | ${0} | ${'a'} | ${false} + ${{}} | ${false} | ${1} | ${1} | ${'a'} | ${true} + ${'truthy'} | ${false} | ${1} | ${1} | ${'a'} | ${true} + ${false} | ${true} | ${1} | ${1} | ${'a'} | ${false} + ${false} | ${true} | ${1} | ${0} | ${'a'} | ${false} + ${false} | ${true} | ${0} | ${1} | ${'a'} | ${false} + ${false} | ${true} | ${0} | ${0} | ${'a'} | ${true} `( - "properly assigns each line's `commentsDisabled` as the same value as the parent file's `brokenSymlink` value (`$brokenSymlink`)", - ({ brokenSymlink }) => { - preppedLine = utils.prepareLineForRenamedFile({ - diffViewType: INLINE_DIFF_VIEW_TYPE, - line: sourceLine, + "properly sets a line's `commentsDisabled` to '$commentsDisabled' for file and line settings { brokenSymlink: $brokenSymlink, renamed: $renamed, added: $added, removed: $removed, line_code: $lineCode }", + ({ brokenSymlink, renamed, added, removed, lineCode, commentsDisabled }) => { + const line = { + ...sourceLine, + line_code: lineCode, + }; + const file = { + ...diffFile, + brokenSymlink, + renamed_file: renamed, + added_lines: added, + removed_lines: removed, + }; + const preparedLine = utils.prepareLineForRenamedFile({ index: lineIndex, - diffFile: { - ...diffFile, - brokenSymlink, - }, + diffFile: file, + line, }); - expect(preppedLine.commentsDisabled).toStrictEqual(brokenSymlink); + expect(preparedLine.commentsDisabled).toBe(commentsDisabled); }, ); }); @@ -477,7 +499,7 @@ describe('DiffsStoreUtils', () => { it('adds the `.brokenSymlink` property to each diff file', () => { preparedDiff.diff_files.forEach((file) => { - expect(file).toEqual(expect.objectContaining({ brokenSymlink: false })); + expect(file).toHaveProperty('brokenSymlink', false); }); }); @@ -490,7 +512,7 @@ describe('DiffsStoreUtils', () => { ].flatMap((file) => [...file[INLINE_DIFF_LINES_KEY]]); lines.forEach((line) => { - expect(line.commentsDisabled).toBe(false); + expect(line.problems.brokenSymlink).toBe(false); }); }); }); -- GitLab From da930ed841dc03870e5647e9a63a06df33fa44e7 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 17 Oct 2022 11:14:27 -0600 Subject: [PATCH 5/5] Don't try to compute the tooltip on lines without problems Generally-speaking, these are "frontend-only" lines (like comment forms) and you can't comment on them anyway. --- .../javascripts/diffs/components/diff_row_utils.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/diffs/components/diff_row_utils.js b/app/assets/javascripts/diffs/components/diff_row_utils.js index 2da44cfc73507c..e0749b63021d76 100644 --- a/app/assets/javascripts/diffs/components/diff_row_utils.js +++ b/app/assets/javascripts/diffs/components/diff_row_utils.js @@ -57,9 +57,16 @@ export const classNameMapCell = ({ line, hll, isLoggedIn, isHover }) => { export const addCommentTooltip = (line) => { let tooltip; - if (!line) return tooltip; + if (!line) { + return tooltip; + } tooltip = __('Add a comment to this line or drag for multiple lines'); + + if (!line.problems) { + return tooltip; + } + const { brokenSymlink, brokenLineCode, fileOnlyMoved } = line.problems; if (brokenSymlink) { -- GitLab