From b3920e178f5118b4d477cd67a671ea1b5087b963 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 20 Sep 2022 22:21:22 -0600 Subject: [PATCH 1/7] Allow multiple drafts on a single line in MR reviews Changelog: fixed --- .../stores/modules/batch_comments/getters.js | 19 ++++++++++--------- .../diffs/components/diff_row_utils.js | 4 ++-- .../diffs/components/diff_view.vue | 18 ++++++++++++------ .../diffs/mixins/draft_comments.js | 2 +- app/assets/stylesheets/framework/diffs.scss | 4 ++++ 5 files changed, 29 insertions(+), 18 deletions(-) diff --git a/app/assets/javascripts/batch_comments/stores/modules/batch_comments/getters.js b/app/assets/javascripts/batch_comments/stores/modules/batch_comments/getters.js index df5214ea7ab0f9..ff25bedf94c1e6 100644 --- a/app/assets/javascripts/batch_comments/stores/modules/batch_comments/getters.js +++ b/app/assets/javascripts/batch_comments/stores/modules/batch_comments/getters.js @@ -22,7 +22,11 @@ export const draftsPerFileHashAndLine = (state) => acc[draft.file_hash] = {}; } - acc[draft.file_hash][draft.line_code] = draft; + if (!acc[draft.file_hash][draft.line_code]) { + acc[draft.file_hash][draft.line_code] = []; + } + + acc[draft.file_hash][draft.line_code].push(draft); } return acc; @@ -61,18 +65,15 @@ export const shouldRenderDraftRowInDiscussion = (state, getters) => (discussionI export const draftForDiscussion = (state, getters) => (discussionId) => getters.draftsPerDiscussionId[discussionId] || {}; -export const draftForLine = (state, getters) => (diffFileSha, line, side = null) => { +export const draftsForLine = (state, getters) => (diffFileSha, line, side = null) => { const draftsForFile = getters.draftsPerFileHashAndLine[diffFileSha]; - const key = side !== null ? parallelLineKey(line, side) : line.line_code; + const showDraftsForThisSide = showDraftOnSide(line, side); - if (draftsForFile) { - const draft = draftsForFile[key]; - if (draft && showDraftOnSide(line, side)) { - return draft; - } + if (draftsForFile && showDraftsForThisSide) { + return draftsForFile[key]; } - return {}; + return []; }; export const draftsForFile = (state) => (diffFileSha) => diff --git a/app/assets/javascripts/diffs/components/diff_row_utils.js b/app/assets/javascripts/diffs/components/diff_row_utils.js index f610ac979ca7ac..c5e7ae15fa6d31 100644 --- a/app/assets/javascripts/diffs/components/diff_row_utils.js +++ b/app/assets/javascripts/diffs/components/diff_row_utils.js @@ -108,7 +108,7 @@ export const mapParallel = (content) => (line) => { ...left, renderDiscussion: hasExpandedDiscussionOnLeft, hasDraft: content.hasParallelDraftLeft(content.diffFile.file_hash, line), - lineDraft: content.draftForLine(content.diffFile.file_hash, line, 'left'), + lineDrafts: content.draftsForLine(content.diffFile.file_hash, line, 'left'), hasCommentForm: left.hasForm, isConflictMarker: line.left.type === CONFLICT_MARKER_OUR || line.left.type === CONFLICT_MARKER_THEIR, @@ -123,7 +123,7 @@ export const mapParallel = (content) => (line) => { hasExpandedDiscussionOnRight && right.type && right.type !== EXPANDED_LINE_TYPE, ), hasDraft: content.hasParallelDraftRight(content.diffFile.file_hash, line), - lineDraft: content.draftForLine(content.diffFile.file_hash, line, 'right'), + lineDrafts: content.draftsForLine(content.diffFile.file_hash, line, 'right'), hasCommentForm: Boolean(right.hasForm && right.type && right.type !== EXPANDED_LINE_TYPE), emptyCellClassMap: { conflict_their: line.left?.type === CONFLICT_OUR }, addCommentTooltip: addCommentTooltip(line.right), diff --git a/app/assets/javascripts/diffs/components/diff_view.vue b/app/assets/javascripts/diffs/components/diff_view.vue index 91bf3283379e3b..5ea118afe78a23 100644 --- a/app/assets/javascripts/diffs/components/diff_view.vue +++ b/app/assets/javascripts/diffs/components/diff_view.vue @@ -177,6 +177,12 @@ export default { getCodeQualityLine(line) { return (line.left ?? line.right)?.codequality?.[0]?.line; }, + lineDrafts(line, side) { + return (line[side]?.lineDrafts || []).filter((entry) => entry.isDraft); + }, + lineHasDrafts(line, side) { + return this.lineDrafts(line, side).length > 0; + }, }, userColorScheme: window.gon.user_color_scheme, }; @@ -297,19 +303,19 @@ export default { class="diff-grid-drafts diff-tr notes_holder" >
-
- +
+
-
- +
+
diff --git a/app/assets/javascripts/diffs/mixins/draft_comments.js b/app/assets/javascripts/diffs/mixins/draft_comments.js index 693b4a8469441c..d41bb160e96701 100644 --- a/app/assets/javascripts/diffs/mixins/draft_comments.js +++ b/app/assets/javascripts/diffs/mixins/draft_comments.js @@ -5,7 +5,7 @@ export default { ...mapGetters('batchComments', [ 'shouldRenderDraftRow', 'shouldRenderParallelDraftRow', - 'draftForLine', + 'draftsForLine', 'draftsForFile', 'hasParallelDraftLeft', 'hasParallelDraftRight', diff --git a/app/assets/stylesheets/framework/diffs.scss b/app/assets/stylesheets/framework/diffs.scss index b991096d925401..73b58932bfc7cd 100644 --- a/app/assets/stylesheets/framework/diffs.scss +++ b/app/assets/stylesheets/framework/diffs.scss @@ -619,6 +619,10 @@ table.code { .diff-grid-drafts { display: grid; grid-template-columns: 1fr 1fr; + + .content + .content { + @include gl-border-t(); + } } &.inline-diff-view { -- GitLab From 85a7c3d272c4216835a6252e5a5bf08929247587 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 21 Sep 2022 00:15:47 -0600 Subject: [PATCH 2/7] Test that two drafts on the same line will both show --- spec/features/merge_request/batch_comments_spec.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/spec/features/merge_request/batch_comments_spec.rb b/spec/features/merge_request/batch_comments_spec.rb index bccdc3c4c62467..c6c7436ac9e341 100644 --- a/spec/features/merge_request/batch_comments_spec.rb +++ b/spec/features/merge_request/batch_comments_spec.rb @@ -74,6 +74,18 @@ expect(page).to have_selector('.draft-note-component', text: 'Testing update') end + context 'multiple times on the same diff line' do + it 'shows both drafts at once' do + write_diff_comment + # You cannot get the same element to hover twice in a row without moving first! + page.execute_script "window.scrollTo(0,0)" + write_diff_comment(text: 'A second draft!', button_text: 'Add to review') + + expect(page).to have_text('Line is wrong') + expect(page).to have_text('A second draft!') + end + end + context 'with image and file draft note' do let(:merge_request) { create(:merge_request_with_diffs, :with_image_diffs, source_project: project) } let!(:draft_on_text) { create(:draft_note_on_text_diff, merge_request: merge_request, author: user, path: 'README.md', note: 'Lorem ipsum on text...') } -- GitLab From 7b32da698b9bba4330d5614f2216ebae5fe16e3d Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 21 Sep 2022 00:30:12 -0600 Subject: [PATCH 3/7] Update JS tests with list of drafts instead of a single draft --- .../stores/modules/batch_comments/getters.js | 2 +- .../diffs/components/diff_row_utils.js | 2 +- .../diffs/components/diff_content_spec.js | 2 +- spec/frontend/diffs/components/diff_row_spec.js | 2 +- .../diffs/components/diff_row_utils_spec.js | 6 +++--- spec/frontend/diffs/components/diff_view_spec.js | 16 ++++++++-------- .../diffs/mock_data/diff_code_quality.js | 6 +++--- 7 files changed, 18 insertions(+), 18 deletions(-) diff --git a/app/assets/javascripts/batch_comments/stores/modules/batch_comments/getters.js b/app/assets/javascripts/batch_comments/stores/modules/batch_comments/getters.js index ff25bedf94c1e6..75e4ae63c18e0c 100644 --- a/app/assets/javascripts/batch_comments/stores/modules/batch_comments/getters.js +++ b/app/assets/javascripts/batch_comments/stores/modules/batch_comments/getters.js @@ -70,7 +70,7 @@ export const draftsForLine = (state, getters) => (diffFileSha, line, side = null const key = side !== null ? parallelLineKey(line, side) : line.line_code; const showDraftsForThisSide = showDraftOnSide(line, side); - if (draftsForFile && showDraftsForThisSide) { + if (showDraftsForThisSide && draftsForFile?.[key]) { return draftsForFile[key]; } return []; diff --git a/app/assets/javascripts/diffs/components/diff_row_utils.js b/app/assets/javascripts/diffs/components/diff_row_utils.js index c5e7ae15fa6d31..7732badde341f2 100644 --- a/app/assets/javascripts/diffs/components/diff_row_utils.js +++ b/app/assets/javascripts/diffs/components/diff_row_utils.js @@ -145,7 +145,7 @@ export const mapParallel = (content) => (line) => { lineCode: lineCode(line), isMetaLineLeft: isMetaLine(left?.type), isMetaLineRight: isMetaLine(right?.type), - draftRowClasses: left?.lineDraft > 0 || right?.lineDraft > 0 ? '' : 'js-temp-notes-holder', + draftRowClasses: left?.hasDraft || right?.hasDraft ? '' : 'js-temp-notes-holder', renderCommentRow, commentRowClasses: hasDiscussions(left) || hasDiscussions(right) ? '' : 'js-temp-notes-holder', }; diff --git a/spec/frontend/diffs/components/diff_content_spec.js b/spec/frontend/diffs/components/diff_content_spec.js index 9f593ee0d49931..0bce6451ce4b86 100644 --- a/spec/frontend/diffs/components/diff_content_spec.js +++ b/spec/frontend/diffs/components/diff_content_spec.js @@ -53,7 +53,7 @@ describe('DiffContent', () => { namespaced: true, getters: { draftsForFile: () => () => true, - draftForLine: () => () => true, + draftsForLine: () => () => true, shouldRenderDraftRow: () => () => true, hasParallelDraftLeft: () => () => true, hasParallelDraftRight: () => () => true, diff --git a/spec/frontend/diffs/components/diff_row_spec.js b/spec/frontend/diffs/components/diff_row_spec.js index a74013dc2d49f9..a7a95ed2f35c35 100644 --- a/spec/frontend/diffs/components/diff_row_spec.js +++ b/spec/frontend/diffs/components/diff_row_spec.js @@ -219,7 +219,7 @@ describe('DiffRow', () => { shouldRenderDraftRow: jest.fn(), hasParallelDraftLeft: jest.fn(), hasParallelDraftRight: jest.fn(), - draftForLine: jest.fn(), + draftsForLine: jest.fn().mockReturnValue([]), }; const applyMap = mapParallel(mockDiffContent); diff --git a/spec/frontend/diffs/components/diff_row_utils_spec.js b/spec/frontend/diffs/components/diff_row_utils_spec.js index 930b8bcdb08092..8b25691ce34dcf 100644 --- a/spec/frontend/diffs/components/diff_row_utils_spec.js +++ b/spec/frontend/diffs/components/diff_row_utils_spec.js @@ -216,7 +216,7 @@ describe('mapParallel', () => { diffFile: {}, hasParallelDraftLeft: () => false, hasParallelDraftRight: () => false, - draftForLine: () => ({}), + draftsForLine: () => [], }; const line = { left: side, right: side }; const expectation = { @@ -234,13 +234,13 @@ describe('mapParallel', () => { const leftExpectation = { renderDiscussion: true, hasDraft: false, - lineDraft: {}, + lineDrafts: [], hasCommentForm: true, }; const rightExpectation = { renderDiscussion: false, hasDraft: false, - lineDraft: {}, + lineDrafts: [], hasCommentForm: false, }; const mapped = utils.mapParallel(content)(line); diff --git a/spec/frontend/diffs/components/diff_view_spec.js b/spec/frontend/diffs/components/diff_view_spec.js index 1dd4a2f6c2360b..9bff6bd14f1e29 100644 --- a/spec/frontend/diffs/components/diff_view_spec.js +++ b/spec/frontend/diffs/components/diff_view_spec.js @@ -21,7 +21,7 @@ describe('DiffView', () => { getters: { shouldRenderDraftRow: () => false, shouldRenderParallelDraftRow: () => () => true, - draftForLine: () => false, + draftsForLine: () => false, draftsForFile: () => false, hasParallelDraftLeft: () => false, hasParallelDraftRight: () => false, @@ -75,12 +75,12 @@ describe('DiffView', () => { }); it.each` - type | side | container | sides | total - ${'parallel'} | ${'left'} | ${'.old'} | ${{ left: { lineDraft: {}, renderDiscussion: true }, right: { lineDraft: {}, renderDiscussion: true } }} | ${2} - ${'parallel'} | ${'right'} | ${'.new'} | ${{ left: { lineDraft: {}, renderDiscussion: true }, right: { lineDraft: {}, renderDiscussion: true } }} | ${2} - ${'inline'} | ${'left'} | ${'.old'} | ${{ left: { lineDraft: {}, renderDiscussion: true } }} | ${1} - ${'inline'} | ${'left'} | ${'.old'} | ${{ left: { lineDraft: {}, renderDiscussion: true } }} | ${1} - ${'inline'} | ${'left'} | ${'.old'} | ${{ left: { lineDraft: {}, renderDiscussion: true } }} | ${1} + type | side | container | sides | total + ${'parallel'} | ${'left'} | ${'.old'} | ${{ left: { lineDrafts: [], renderDiscussion: true }, right: { lineDrafts: [], renderDiscussion: true } }} | ${2} + ${'parallel'} | ${'right'} | ${'.new'} | ${{ left: { lineDrafts: [], renderDiscussion: true }, right: { lineDrafts: [], renderDiscussion: true } }} | ${2} + ${'inline'} | ${'left'} | ${'.old'} | ${{ left: { lineDrafts: [], renderDiscussion: true } }} | ${1} + ${'inline'} | ${'left'} | ${'.old'} | ${{ left: { lineDrafts: [], renderDiscussion: true } }} | ${1} + ${'inline'} | ${'left'} | ${'.old'} | ${{ left: { lineDrafts: [], renderDiscussion: true } }} | ${1} `( 'renders a $type comment row with comment cell on $side', ({ type, container, sides, total }) => { @@ -95,7 +95,7 @@ describe('DiffView', () => { it('renders a draft row', () => { const wrapper = createWrapper({ - diffLines: [{ renderCommentRow: true, left: { lineDraft: { isDraft: true } } }], + diffLines: [{ renderCommentRow: true, left: { lineDrafts: [{ isDraft: true }] } }], }); expect(wrapper.findComponent(DraftNote).exists()).toBe(true); }); diff --git a/spec/frontend/diffs/mock_data/diff_code_quality.js b/spec/frontend/diffs/mock_data/diff_code_quality.js index 2ca421a20b4bdd..befab3b676b40c 100644 --- a/spec/frontend/diffs/mock_data/diff_code_quality.js +++ b/spec/frontend/diffs/mock_data/diff_code_quality.js @@ -36,7 +36,7 @@ export const diffCodeQuality = { old_line: 1, new_line: null, codequality: [], - lineDraft: {}, + lineDrafts: [], }, }, { @@ -45,7 +45,7 @@ export const diffCodeQuality = { old_line: 2, new_line: 1, codequality: [], - lineDraft: {}, + lineDrafts: [], }, }, { @@ -55,7 +55,7 @@ export const diffCodeQuality = { new_line: 2, codequality: [multipleFindingsArr[0]], - lineDraft: {}, + lineDrafts: [], }, }, ], -- GitLab From 1ad6fc6f9d7488f99ffcd96c63a3a6c508e61268 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 21 Sep 2022 15:30:52 -0600 Subject: [PATCH 4/7] Fix weirdly broken test (helpers) --- spec/features/merge_request/batch_comments_spec.rb | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/spec/features/merge_request/batch_comments_spec.rb b/spec/features/merge_request/batch_comments_spec.rb index c6c7436ac9e341..f01217df8c57f5 100644 --- a/spec/features/merge_request/batch_comments_spec.rb +++ b/spec/features/merge_request/batch_comments_spec.rb @@ -77,9 +77,15 @@ context 'multiple times on the same diff line' do it 'shows both drafts at once' do write_diff_comment - # You cannot get the same element to hover twice in a row without moving first! - page.execute_script "window.scrollTo(0,0)" - write_diff_comment(text: 'A second draft!', button_text: 'Add to review') + + # All of the Diff helpers like click_diff_line (or write_diff_comment) + # fail very badly when run a second time. + # This recreates the relevant logic. + line = find_by_scrolling("[id='#{sample_compare.changes[0][:line_code]}']") + line.hover + line.find('.js-add-diff-note-button').click + + write_comment(text: 'A second draft!', button_text: 'Add to review') expect(page).to have_text('Line is wrong') expect(page).to have_text('A second draft!') -- GitLab From 6d893a1da87e3895acde9794a55f1e96b110c47f Mon Sep 17 00:00:00 2001 From: Olena Horal-Koretska Date: Tue, 27 Sep 2022 16:43:26 +0000 Subject: [PATCH 5/7] Remove Sass utility function call for the bare utility name --- app/assets/stylesheets/framework/diffs.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/stylesheets/framework/diffs.scss b/app/assets/stylesheets/framework/diffs.scss index 73b58932bfc7cd..7fff675b970a0e 100644 --- a/app/assets/stylesheets/framework/diffs.scss +++ b/app/assets/stylesheets/framework/diffs.scss @@ -621,7 +621,7 @@ table.code { grid-template-columns: 1fr 1fr; .content + .content { - @include gl-border-t(); + @include gl-border-t; } } -- GitLab From 2a9291826f2e11a94cf8d28a294e034c0a44d9c0 Mon Sep 17 00:00:00 2001 From: Olena Horal-Koretska Date: Tue, 27 Sep 2022 16:43:58 +0000 Subject: [PATCH 6/7] Use a logical OR assignment to create the new empty array if necessary --- .../batch_comments/stores/modules/batch_comments/getters.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/app/assets/javascripts/batch_comments/stores/modules/batch_comments/getters.js b/app/assets/javascripts/batch_comments/stores/modules/batch_comments/getters.js index 75e4ae63c18e0c..dbb9ed5c06c5ea 100644 --- a/app/assets/javascripts/batch_comments/stores/modules/batch_comments/getters.js +++ b/app/assets/javascripts/batch_comments/stores/modules/batch_comments/getters.js @@ -22,11 +22,7 @@ export const draftsPerFileHashAndLine = (state) => acc[draft.file_hash] = {}; } - if (!acc[draft.file_hash][draft.line_code]) { - acc[draft.file_hash][draft.line_code] = []; - } - - acc[draft.file_hash][draft.line_code].push(draft); + (acc[draft.file_hash][draft.line_code] ||= []).push(draft); } return acc; -- GitLab From 98892a8018b2419693b1960355c1444fd1482b1c Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 27 Sep 2022 14:45:11 -0600 Subject: [PATCH 7/7] Revert "Use a logical OR assignment to create the new empty array if necessary" This reverts commit 2a9291826f2e11a94cf8d28a294e034c0a44d9c0. This causes a Webpack compile error when trying to create a production bundle. --- .../batch_comments/stores/modules/batch_comments/getters.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/batch_comments/stores/modules/batch_comments/getters.js b/app/assets/javascripts/batch_comments/stores/modules/batch_comments/getters.js index dbb9ed5c06c5ea..75e4ae63c18e0c 100644 --- a/app/assets/javascripts/batch_comments/stores/modules/batch_comments/getters.js +++ b/app/assets/javascripts/batch_comments/stores/modules/batch_comments/getters.js @@ -22,7 +22,11 @@ export const draftsPerFileHashAndLine = (state) => acc[draft.file_hash] = {}; } - (acc[draft.file_hash][draft.line_code] ||= []).push(draft); + if (!acc[draft.file_hash][draft.line_code]) { + acc[draft.file_hash][draft.line_code] = []; + } + + acc[draft.file_hash][draft.line_code].push(draft); } return acc; -- GitLab