diff options
author | Filipa Lacerda <filipa@gitlab.com> | 2018-09-19 15:03:37 +0000 |
---|---|---|
committer | Filipa Lacerda <filipa@gitlab.com> | 2018-09-19 15:03:37 +0000 |
commit | 38cf0b67b588b6f07adb0bde048ca4f568816598 (patch) | |
tree | 41cb3544a0f79c9a847855072e459d2c3ddc6fbe | |
parent | 9e4e1aee500b44e2568fc439ae53077967ebb0c3 (diff) | |
parent | 90673dbcb82f78c53d071994ca65e3ace7a28f62 (diff) | |
download | gitlab-ce-38cf0b67b588b6f07adb0bde048ca4f568816598.tar.gz |
Merge branch 'mr-legacy-diff-notes' into 'master'
Re-enable legacy diff notes on merge request diffs
Closes #48873
See merge request gitlab-org/gitlab-ce!21652
-rw-r--r-- | app/assets/javascripts/diffs/components/diff_line_note_form.vue | 4 | ||||
-rw-r--r-- | app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue | 4 | ||||
-rw-r--r-- | app/assets/javascripts/diffs/constants.js | 1 | ||||
-rw-r--r-- | app/assets/javascripts/diffs/store/mutations.js | 8 | ||||
-rw-r--r-- | app/assets/javascripts/diffs/store/utils.js | 31 | ||||
-rw-r--r-- | app/assets/javascripts/notes/stores/utils.js | 2 | ||||
-rw-r--r-- | changelogs/unreleased/mr-legacy-diff-notes.yml | 5 | ||||
-rw-r--r-- | spec/features/merge_request/user_posts_diff_notes_spec.rb | 15 | ||||
-rw-r--r-- | spec/javascripts/diffs/store/actions_spec.js | 1 | ||||
-rw-r--r-- | spec/javascripts/diffs/store/mutations_spec.js | 71 | ||||
-rw-r--r-- | spec/javascripts/diffs/store/utils_spec.js | 122 |
11 files changed, 239 insertions, 25 deletions
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" /> </td> <td class="notes_line new"></td> @@ -111,7 +111,7 @@ export default { :diff-file-hash="diffFileHash" :line="line.right" :note-target-line="line.right" - position="right" + line-position="right" /> </td> </tr> 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..59a2c09e54f 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -90,16 +90,18 @@ export default { const firstDiscussion = discussions[0]; const isDiffDiscussion = firstDiscussion.diff_discussion; const hasLineCode = firstDiscussion.line_code; - const isResolvable = firstDiscussion.resolvable; const diffPosition = diffPositionByLineCode[firstDiscussion.line_code]; if ( selectedFile && isDiffDiscussion && hasLineCode && - isResolvable && diffPosition && - isDiscussionApplicableToLine(firstDiscussion, diffPosition) + isDiscussionApplicableToLine({ + discussion: firstDiscussion, + diffPosition, + latestDiff: state.latestDiff, + }) ) { const targetLine = selectedFile.parallelDiffLines.find( line => diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index b7e52a8f37f..834a94ea42a 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,10 @@ export function getNoteFormData(params) { noteable_type: noteableType, noteable_id: noteableData.id, commit_id: '', - type: DIFF_NOTE_TYPE, + type: + diffFile.diffRefs.startSha && diffFile.diffRefs.headSha + ? DIFF_NOTE_TYPE + : LEGACY_DIFF_NOTE_TYPE, line_code: noteTargetLine.lineCode, }, }; @@ -230,7 +234,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, + }; } }); } @@ -241,9 +254,15 @@ 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); +export function isDiscussionApplicableToLine({ discussion, diffPosition, latestDiff }) { + 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 latestDiff && discussion.active && lineCode === discussion.line_code; } diff --git a/app/assets/javascripts/notes/stores/utils.js b/app/assets/javascripts/notes/stores/utils.js index 8ccbdb4c130..0e41ff03d67 100644 --- a/app/assets/javascripts/notes/stores/utils.js +++ b/app/assets/javascripts/notes/stores/utils.js @@ -27,7 +27,7 @@ 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) { // 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..bca5ac8297f --- /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: 21652 +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 diff --git a/spec/javascripts/diffs/store/actions_spec.js b/spec/javascripts/diffs/store/actions_spec.js index cfb8f862598..fd5c5e104b4 100644 --- a/spec/javascripts/diffs/store/actions_spec.js +++ b/spec/javascripts/diffs/store/actions_spec.js @@ -157,6 +157,7 @@ describe('DiffsStoreActions', () => { newPath: 'file1', oldLine: 5, oldPath: 'file2', + lineCode: 'ABC_1_1', }, }, }, diff --git a/spec/javascripts/diffs/store/mutations_spec.js b/spec/javascripts/diffs/store/mutations_spec.js index 7eeca6712cc..9a5d8dfbd15 100644 --- a/spec/javascripts/diffs/store/mutations_spec.js +++ b/spec/javascripts/diffs/store/mutations_spec.js @@ -162,6 +162,7 @@ describe('DiffsStoreMutations', () => { }; const state = { + latestDiff: true, diffFiles: [ { fileHash: 'ABC', @@ -229,6 +230,76 @@ describe('DiffsStoreMutations', () => { expect(state.diffFiles[0].highlightedDiffLines[0].discussions.length).toEqual(2); expect(state.diffFiles[0].highlightedDiffLines[0].discussions[1].id).toEqual(2); }); + + it('should add legacy discussions to the given line', () => { + const diffPosition = { + baseSha: 'ed13df29948c41ba367caa757ab3ec4892509910', + headSha: 'b921914f9a834ac47e6fd9420f78db0f83559130', + newLine: null, + newPath: '500-lines-4.txt', + oldLine: 5, + oldPath: '500-lines-4.txt', + startSha: 'ed13df29948c41ba367caa757ab3ec4892509910', + lineCode: 'ABC_1', + }; + + const state = { + latestDiff: true, + diffFiles: [ + { + fileHash: 'ABC', + parallelDiffLines: [ + { + left: { + lineCode: 'ABC_1', + discussions: [], + }, + right: { + lineCode: 'ABC_1', + discussions: [], + }, + }, + ], + highlightedDiffLines: [ + { + lineCode: 'ABC_1', + discussions: [], + }, + ], + }, + ], + }; + const discussions = [ + { + id: 1, + line_code: 'ABC_1', + diff_discussion: true, + active: true, + }, + { + id: 2, + line_code: 'ABC_1', + diff_discussion: true, + active: true, + }, + ]; + + const diffPositionByLineCode = { + ABC_1: diffPosition, + }; + + mutations[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { + fileHash: 'ABC', + discussions, + diffPositionByLineCode, + }); + + expect(state.diffFiles[0].parallelDiffLines[0].left.discussions.length).toEqual(2); + expect(state.diffFiles[0].parallelDiffLines[0].left.discussions[1].id).toEqual(2); + + expect(state.diffFiles[0].highlightedDiffLines[0].discussions.length).toEqual(2); + expect(state.diffFiles[0].highlightedDiffLines[0].discussions[1].id).toEqual(2); + }); }); describe('REMOVE_LINE_DISCUSSIONS', () => { diff --git a/spec/javascripts/diffs/store/utils_spec.js b/spec/javascripts/diffs/store/utils_spec.js index 4b5cf450c68..6138b9701f4 100644 --- a/spec/javascripts/diffs/store/utils_spec.js +++ b/spec/javascripts/diffs/store/utils_spec.js @@ -3,6 +3,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, @@ -151,6 +152,64 @@ describe('DiffsStoreUtils', () => { data: postData, }); }); + + it('should create legacy note form data', () => { + const diffFile = getDiffFileMock(); + delete diffFile.diffRefs.startSha; + delete diffFile.diffRefs.headSha; + + noteableDataMock.targetType = MERGE_REQUEST_NOTEABLE_TYPE; + + const options = { + note: 'Hello world!', + noteableData: noteableDataMock, + noteableType: MERGE_REQUEST_NOTEABLE_TYPE, + diffFile, + noteTargetLine: { + lineCode: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_3', + metaData: null, + newLine: 3, + oldLine: 1, + }, + diffViewType: PARALLEL_DIFF_VIEW_TYPE, + linePosition: LINE_POSITION_LEFT, + }; + + const position = JSON.stringify({ + base_sha: diffFile.diffRefs.baseSha, + start_sha: undefined, + head_sha: undefined, + old_path: diffFile.oldPath, + new_path: diffFile.newPath, + position_type: TEXT_DIFF_POSITION_TYPE, + old_line: options.noteTargetLine.oldLine, + new_line: options.noteTargetLine.newLine, + }); + + const postData = { + view: options.diffViewType, + line_type: options.linePosition === LINE_POSITION_RIGHT ? NEW_LINE_TYPE : OLD_LINE_TYPE, + merge_request_diff_head_sha: undefined, + in_reply_to_discussion_id: '', + note_project_id: '', + target_type: options.noteableType, + target_id: options.noteableData.id, + note: { + noteable_type: options.noteableType, + noteable_id: options.noteableData.id, + commit_id: '', + type: LEGACY_DIFF_NOTE_TYPE, + line_code: options.noteTargetLine.lineCode, + note: options.note, + position, + }, + }; + + expect(utils.getNoteFormData(options)).toEqual({ + endpoint: options.noteableData.create_note_path, + data: postData, + }); + }); }); describe('addLineReferences', () => { @@ -291,13 +350,72 @@ describe('DiffsStoreUtils', () => { it('returns true when the discussion is up to date', () => { expect( - utils.isDiscussionApplicableToLine(discussions.upToDateDiscussion1, diffPosition), + utils.isDiscussionApplicableToLine({ + discussion: discussions.upToDateDiscussion1, + diffPosition, + latestDiff: true, + }), ).toBe(true); }); it('returns false when the discussion is not up to date', () => { expect( - utils.isDiscussionApplicableToLine(discussions.outDatedDiscussion1, diffPosition), + utils.isDiscussionApplicableToLine({ + discussion: discussions.outDatedDiscussion1, + diffPosition, + latestDiff: true, + }), + ).toBe(false); + }); + + it('returns true when line codes match and discussion does not contain position and is not active', () => { + const discussion = { ...discussions.outDatedDiscussion1, line_code: 'ABC_1', active: false }; + delete discussion.original_position; + delete discussion.position; + + expect( + utils.isDiscussionApplicableToLine({ + discussion, + diffPosition: { + ...diffPosition, + lineCode: 'ABC_1', + }, + latestDiff: true, + }), + ).toBe(false); + }); + + it('returns true when line codes match and discussion does not contain position and is active', () => { + const discussion = { ...discussions.outDatedDiscussion1, line_code: 'ABC_1', active: true }; + delete discussion.original_position; + delete discussion.position; + + expect( + utils.isDiscussionApplicableToLine({ + discussion, + diffPosition: { + ...diffPosition, + lineCode: 'ABC_1', + }, + latestDiff: true, + }), + ).toBe(true); + }); + + it('returns false when not latest diff', () => { + const discussion = { ...discussions.outDatedDiscussion1, line_code: 'ABC_1', active: true }; + delete discussion.original_position; + delete discussion.position; + + expect( + utils.isDiscussionApplicableToLine({ + discussion, + diffPosition: { + ...diffPosition, + lineCode: 'ABC_1', + }, + latestDiff: false, + }), ).toBe(false); }); }); |