Commit 8064ab84 authored by Phil Hughes's avatar Phil Hughes

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
parent 5a8908bf
......@@ -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)
......
......@@ -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>
......
......@@ -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';
......
......@@ -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 (
......
......@@ -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;
}
......@@ -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);
......
---
title: Correctly show legacy diff notes in the merge request changes tab
merge_request:
author:
type: fixed
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment