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 df5214ea7ab0f935430c2117c2b26f8c07601c87..75e4ae63c18e0c25f0f121a67cafdd4120109142 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 (showDraftsForThisSide && draftsForFile?.[key]) { + 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 f610ac979ca7ac9bd124d61c9124dfa0af609c10..7732badde341f2e3293d59cb8c752519e5915939 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), @@ -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/app/assets/javascripts/diffs/components/diff_view.vue b/app/assets/javascripts/diffs/components/diff_view.vue index 91bf3283379e3b37091d2b0969a010945bcdb962..5ea118afe78a23892b308357374a0cf8b25e17d4 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 693b4a8469441c786318234aefbdb82db1f575da..d41bb160e96701fcbf7d72687753cb3f12f2abf0 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 b991096d92540131cc1e67e7490e66865492b9c1..7fff675b970a0ee5296a95c82e064ba91c081da3 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 { diff --git a/spec/features/merge_request/batch_comments_spec.rb b/spec/features/merge_request/batch_comments_spec.rb index bccdc3c4c62467e13f1dd4bc287b7e91fb1ed4c7..f01217df8c57f564a1c00df1bfc55a68cbaa4139 100644 --- a/spec/features/merge_request/batch_comments_spec.rb +++ b/spec/features/merge_request/batch_comments_spec.rb @@ -74,6 +74,24 @@ 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 + + # 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!') + 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...') } diff --git a/spec/frontend/diffs/components/diff_content_spec.js b/spec/frontend/diffs/components/diff_content_spec.js index 9f593ee0d49931d0568b0adffdd15acef57ecfa7..0bce6451ce4b86400170ed2f8994e3cbaa919612 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 a74013dc2d49f94674a37af7ec0e9b0e6ce94a79..a7a95ed2f35c3599dc2b49fa875761b6bb72bda1 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 930b8bcdb08092a401bced4f537fe65932955ce4..8b25691ce34dcfa4533840f0292d0557e2da1d9d 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 1dd4a2f6c2360bc7bd117c3adc57d47407117604..9bff6bd14f1e298e2641b95a9376b4dab52abfdf 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 2ca421a20b4bdd3672597472b49d965bd1e38204..befab3b676b40c8f47d158cc2b829474b89e7145 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: [], }, }, ],