diff --git a/app/assets/javascripts/diffs/components/diff_row_utils.js b/app/assets/javascripts/diffs/components/diff_row_utils.js index 7732badde341f2e3293d59cb8c752519e5915939..e0749b63021d766a7826a41165890b11c48c7e8e 100644 --- a/app/assets/javascripts/diffs/components/diff_row_utils.js +++ b/app/assets/javascripts/diffs/components/diff_row_utils.js @@ -57,21 +57,32 @@ 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'); - const brokenSymlinks = line.commentsDisabled; - if (brokenSymlinks) { - if (brokenSymlinks.wasSymbolic || brokenSymlinks.isSymbolic) { + if (!line.problems) { + return tooltip; + } + + const { brokenSymlink, brokenLineCode, fileOnlyMoved } = line.problems; + + 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 cf86ebea4a9f763e6447eb4d50671ff300451e79..0519ca3d7151e6dffaa913ac35ca90f41a6b33b0 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -324,15 +324,24 @@ function cleanRichText(text) { } function prepareLine(line, file) { + const problems = { + brokenSymlink: file.brokenSymlink, + brokenLineCode: !line.line_code, + fileOnlyMoved: file.renamed_file && file.added_lines === 0 && file.removed_lines === 0, + }; + if (!line.alreadyPrepared) { Object.assign(line, { - commentsDisabled: file.brokenSymlink, + commentsDisabled: Boolean( + 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 438b7b1afa6b8cbfd2f28e2c3ae2cebf63ea567f..955dcdc1c0f3b3e39b416f763f72f3fe1fe06603 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 6ba1f00376b0f8d648010c93f21b3c818c8bb417..cc45e95ef47a84d34395df4ad5316fc4d393cbed 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 "" diff --git a/spec/frontend/diffs/components/diff_row_utils_spec.js b/spec/frontend/diffs/components/diff_row_utils_spec.js index 8b25691ce34dcfa4533840f0292d0557e2da1d9d..d0437808a179f7ae96d369622481115accbde973 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 }; @@ -140,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', () => { @@ -147,24 +162,38 @@ 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({ problems: problemsClone({ brokenSymlink: { wasReal: true } }) }), + ).toEqual(brokenRealTooltip); + expect( + 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, ); - expect(utils.addCommentTooltip({ commentsDisabled: { isReal: true } })).toEqual( - brokenRealTooltip, + }); + + 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, ); }); }); @@ -211,6 +240,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 dd200b0248c91d3c46c296097062e5a7e412727f..e0e5778e0d56f13813ab79e2b3540403ac9a4aee 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(), }, }, ], diff --git a/spec/frontend/diffs/store/utils_spec.js b/spec/frontend/diffs/store/utils_spec.js index 3f870a98396d02a65224fb8bb6e055f71a2a4ad4..b5c44b084d8b905369ad940662ccaf8f226f2aa9 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); }); }); });