From 8064ab84a68d439ae5a33f02fd42690f23d1b536 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Mon, 10 Sep 2018 19:41:57 +0100 Subject: [PATCH] Re-enable legacy diff notes on merge request diffs This re-enables legacy diff notes on the merge request diffs This feature was not workig correctly after the Vue refactor LegacyDiffNotes have no `position`, instead they only have a `line_code` As an extra, this also re-enables commenting on legacy diffs. Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/48873 --- .../diffs/components/diff_line_note_form.vue | 4 +-- .../components/parallel_diff_comment_row.vue | 4 +-- app/assets/javascripts/diffs/constants.js | 1 + .../javascripts/diffs/store/mutations.js | 3 ++- app/assets/javascripts/diffs/store/utils.js | 26 +++++++++++++++---- app/assets/javascripts/notes/stores/utils.js | 6 ++++- .../unreleased/mr-legacy-diff-notes.yml | 5 ++++ .../user_posts_diff_notes_spec.rb | 15 +++++------ 8 files changed, 44 insertions(+), 20 deletions(-) create mode 100644 changelogs/unreleased/mr-legacy-diff-notes.yml diff --git a/app/assets/javascripts/diffs/components/diff_line_note_form.vue b/app/assets/javascripts/diffs/components/diff_line_note_form.vue index a0dc381ccc7..0fa14615532 100644 --- a/app/assets/javascripts/diffs/components/diff_line_note_form.vue +++ b/app/assets/javascripts/diffs/components/diff_line_note_form.vue @@ -22,7 +22,7 @@ export default { type: Object, required: true, }, - position: { + linePosition: { type: String, required: false, default: '', @@ -81,7 +81,7 @@ export default { noteTargetLine: this.noteTargetLine, diffViewType: this.diffViewType, diffFile: selectedDiffFile, - linePosition: this.position, + linePosition: this.linePosition, }); this.saveNote(postData) diff --git a/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue b/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue index 26417c350cb..3339c56cbb6 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue @@ -92,7 +92,7 @@ export default { :diff-file-hash="diffFileHash" :line="line.left" :note-target-line="line.left" - position="left" + line-position="left" /> @@ -111,7 +111,7 @@ export default { :diff-file-hash="diffFileHash" :line="line.right" :note-target-line="line.right" - position="right" + line-position="right" /> diff --git a/app/assets/javascripts/diffs/constants.js b/app/assets/javascripts/diffs/constants.js index f68afa44837..2795dddfc48 100644 --- a/app/assets/javascripts/diffs/constants.js +++ b/app/assets/javascripts/diffs/constants.js @@ -7,6 +7,7 @@ export const CONTEXT_LINE_TYPE = 'context'; export const EMPTY_CELL_TYPE = 'empty-cell'; export const COMMENT_FORM_TYPE = 'commentForm'; export const DIFF_NOTE_TYPE = 'DiffNote'; +export const LEGACY_DIFF_NOTE_TYPE = 'LegacyDiffNote'; export const NOTE_TYPE = 'Note'; export const NEW_LINE_TYPE = 'new'; export const OLD_LINE_TYPE = 'old'; diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index 6dc5bf16c65..b6818f905e5 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -90,7 +90,8 @@ export default { const firstDiscussion = discussions[0]; const isDiffDiscussion = firstDiscussion.diff_discussion; const hasLineCode = firstDiscussion.line_code; - const isResolvable = firstDiscussion.resolvable; + const isResolvable = + firstDiscussion.resolvable || (!firstDiscussion.resolvable && !firstDiscussion.position); const diffPosition = diffPositionByLineCode[firstDiscussion.line_code]; if ( diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index b7e52a8f37f..d521e4584ad 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -4,6 +4,7 @@ import { LINE_POSITION_LEFT, LINE_POSITION_RIGHT, TEXT_DIFF_POSITION_TYPE, + LEGACY_DIFF_NOTE_TYPE, DIFF_NOTE_TYPE, NEW_LINE_TYPE, OLD_LINE_TYPE, @@ -60,7 +61,7 @@ export function getNoteFormData(params) { noteable_type: noteableType, noteable_id: noteableData.id, commit_id: '', - type: DIFF_NOTE_TYPE, + type: diffFile.diffRefs.startSha ? DIFF_NOTE_TYPE : LEGACY_DIFF_NOTE_TYPE, line_code: noteTargetLine.lineCode, }, }; @@ -230,7 +231,16 @@ export function getDiffPositionByLineCode(diffFiles) { const { lineCode, oldLine, newLine } = line; if (lineCode) { - acc[lineCode] = { baseSha, headSha, startSha, newPath, oldPath, oldLine, newLine }; + acc[lineCode] = { + baseSha, + headSha, + startSha, + newPath, + oldPath, + oldLine, + newLine, + lineCode, + }; } }); } @@ -242,8 +252,14 @@ export function getDiffPositionByLineCode(diffFiles) { // This method will check whether the discussion is still applicable // to the diff line in question regarding different versions of the MR export function isDiscussionApplicableToLine(discussion, diffPosition) { - const originalRefs = convertObjectPropsToCamelCase(discussion.original_position.formatter); - const refs = convertObjectPropsToCamelCase(discussion.position.formatter); + const { lineCode, ...diffPositionCopy } = diffPosition; - return _.isEqual(refs, diffPosition) || _.isEqual(originalRefs, diffPosition); + if (discussion.original_position && discussion.position) { + const originalRefs = convertObjectPropsToCamelCase(discussion.original_position.formatter); + const refs = convertObjectPropsToCamelCase(discussion.position.formatter); + + return _.isEqual(refs, diffPositionCopy) || _.isEqual(originalRefs, diffPositionCopy); + } + + return lineCode === discussion.line_code; } diff --git a/app/assets/javascripts/notes/stores/utils.js b/app/assets/javascripts/notes/stores/utils.js index 8ccbdb4c130..e2dd81273f5 100644 --- a/app/assets/javascripts/notes/stores/utils.js +++ b/app/assets/javascripts/notes/stores/utils.js @@ -27,7 +27,11 @@ export const getQuickActionText = note => { export const reduceDiscussionsToLineCodes = selectedDiscussions => selectedDiscussions.reduce((acc, note) => { - if (note.diff_discussion && note.line_code && note.resolvable) { + if ( + note.diff_discussion && + note.line_code && + (note.resolvable || (!note.resolvable && !note.position)) + ) { // For context about line notes: there might be multiple notes with the same line code const items = acc[note.line_code] || []; items.push(note); diff --git a/changelogs/unreleased/mr-legacy-diff-notes.yml b/changelogs/unreleased/mr-legacy-diff-notes.yml new file mode 100644 index 00000000000..38b161c2a37 --- /dev/null +++ b/changelogs/unreleased/mr-legacy-diff-notes.yml @@ -0,0 +1,5 @@ +--- +title: Correctly show legacy diff notes in the merge request changes tab +merge_request: +author: +type: fixed diff --git a/spec/features/merge_request/user_posts_diff_notes_spec.rb b/spec/features/merge_request/user_posts_diff_notes_spec.rb index b6ed3686de2..fa148715855 100644 --- a/spec/features/merge_request/user_posts_diff_notes_spec.rb +++ b/spec/features/merge_request/user_posts_diff_notes_spec.rb @@ -200,23 +200,20 @@ describe 'Merge request > User posts diff notes', :js do end context 'with a new line' do - # TODO: https://gitlab.com/gitlab-org/gitlab-ce/issues/48034 - xit 'allows commenting' do - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]').find(:xpath, '..')) + it 'allows commenting' do + should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]')) end end context 'with an old line' do - # TODO: https://gitlab.com/gitlab-org/gitlab-ce/issues/48034 - xit 'allows commenting' do - should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]').find(:xpath, '..')) + it 'allows commenting' do + should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]')) end end context 'with an unchanged line' do - # TODO: https://gitlab.com/gitlab-org/gitlab-ce/issues/48034 - xit 'allows commenting' do - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]').find(:xpath, '..')) + it 'allows commenting' do + should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]')) end end