Commit 38cf0b67 authored by Filipa Lacerda's avatar Filipa Lacerda

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
parents 9e4e1aee 90673dbc
...@@ -22,7 +22,7 @@ export default { ...@@ -22,7 +22,7 @@ export default {
type: Object, type: Object,
required: true, required: true,
}, },
position: { linePosition: {
type: String, type: String,
required: false, required: false,
default: '', default: '',
...@@ -81,7 +81,7 @@ export default { ...@@ -81,7 +81,7 @@ export default {
noteTargetLine: this.noteTargetLine, noteTargetLine: this.noteTargetLine,
diffViewType: this.diffViewType, diffViewType: this.diffViewType,
diffFile: selectedDiffFile, diffFile: selectedDiffFile,
linePosition: this.position, linePosition: this.linePosition,
}); });
this.saveNote(postData) this.saveNote(postData)
......
...@@ -92,7 +92,7 @@ export default { ...@@ -92,7 +92,7 @@ export default {
:diff-file-hash="diffFileHash" :diff-file-hash="diffFileHash"
:line="line.left" :line="line.left"
:note-target-line="line.left" :note-target-line="line.left"
position="left" line-position="left"
/> />
</td> </td>
<td class="notes_line new"></td> <td class="notes_line new"></td>
...@@ -111,7 +111,7 @@ export default { ...@@ -111,7 +111,7 @@ export default {
:diff-file-hash="diffFileHash" :diff-file-hash="diffFileHash"
:line="line.right" :line="line.right"
:note-target-line="line.right" :note-target-line="line.right"
position="right" line-position="right"
/> />
</td> </td>
</tr> </tr>
......
...@@ -7,6 +7,7 @@ export const CONTEXT_LINE_TYPE = 'context'; ...@@ -7,6 +7,7 @@ export const CONTEXT_LINE_TYPE = 'context';
export const EMPTY_CELL_TYPE = 'empty-cell'; export const EMPTY_CELL_TYPE = 'empty-cell';
export const COMMENT_FORM_TYPE = 'commentForm'; export const COMMENT_FORM_TYPE = 'commentForm';
export const DIFF_NOTE_TYPE = 'DiffNote'; export const DIFF_NOTE_TYPE = 'DiffNote';
export const LEGACY_DIFF_NOTE_TYPE = 'LegacyDiffNote';
export const NOTE_TYPE = 'Note'; export const NOTE_TYPE = 'Note';
export const NEW_LINE_TYPE = 'new'; export const NEW_LINE_TYPE = 'new';
export const OLD_LINE_TYPE = 'old'; export const OLD_LINE_TYPE = 'old';
......
...@@ -90,16 +90,18 @@ export default { ...@@ -90,16 +90,18 @@ export default {
const firstDiscussion = discussions[0]; const firstDiscussion = discussions[0];
const isDiffDiscussion = firstDiscussion.diff_discussion; const isDiffDiscussion = firstDiscussion.diff_discussion;
const hasLineCode = firstDiscussion.line_code; const hasLineCode = firstDiscussion.line_code;
const isResolvable = firstDiscussion.resolvable;
const diffPosition = diffPositionByLineCode[firstDiscussion.line_code]; const diffPosition = diffPositionByLineCode[firstDiscussion.line_code];
if ( if (
selectedFile && selectedFile &&
isDiffDiscussion && isDiffDiscussion &&
hasLineCode && hasLineCode &&
isResolvable &&
diffPosition && diffPosition &&
isDiscussionApplicableToLine(firstDiscussion, diffPosition) isDiscussionApplicableToLine({
discussion: firstDiscussion,
diffPosition,
latestDiff: state.latestDiff,
})
) { ) {
const targetLine = selectedFile.parallelDiffLines.find( const targetLine = selectedFile.parallelDiffLines.find(
line => line =>
......
...@@ -4,6 +4,7 @@ import { ...@@ -4,6 +4,7 @@ import {
LINE_POSITION_LEFT, LINE_POSITION_LEFT,
LINE_POSITION_RIGHT, LINE_POSITION_RIGHT,
TEXT_DIFF_POSITION_TYPE, TEXT_DIFF_POSITION_TYPE,
LEGACY_DIFF_NOTE_TYPE,
DIFF_NOTE_TYPE, DIFF_NOTE_TYPE,
NEW_LINE_TYPE, NEW_LINE_TYPE,
OLD_LINE_TYPE, OLD_LINE_TYPE,
...@@ -60,7 +61,10 @@ export function getNoteFormData(params) { ...@@ -60,7 +61,10 @@ export function getNoteFormData(params) {
noteable_type: noteableType, noteable_type: noteableType,
noteable_id: noteableData.id, noteable_id: noteableData.id,
commit_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, line_code: noteTargetLine.lineCode,
}, },
}; };
...@@ -230,7 +234,16 @@ export function getDiffPositionByLineCode(diffFiles) { ...@@ -230,7 +234,16 @@ export function getDiffPositionByLineCode(diffFiles) {
const { lineCode, oldLine, newLine } = line; const { lineCode, oldLine, newLine } = line;
if (lineCode) { 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) { ...@@ -241,9 +254,15 @@ export function getDiffPositionByLineCode(diffFiles) {
// This method will check whether the discussion is still applicable // This method will check whether the discussion is still applicable
// to the diff line in question regarding different versions of the MR // to the diff line in question regarding different versions of the MR
export function isDiscussionApplicableToLine(discussion, diffPosition) { export function isDiscussionApplicableToLine({ discussion, diffPosition, latestDiff }) {
const { lineCode, ...diffPositionCopy } = diffPosition;
if (discussion.original_position && discussion.position) {
const originalRefs = convertObjectPropsToCamelCase(discussion.original_position.formatter); const originalRefs = convertObjectPropsToCamelCase(discussion.original_position.formatter);
const refs = convertObjectPropsToCamelCase(discussion.position.formatter); const refs = convertObjectPropsToCamelCase(discussion.position.formatter);
return _.isEqual(refs, diffPosition) || _.isEqual(originalRefs, diffPosition); return _.isEqual(refs, diffPositionCopy) || _.isEqual(originalRefs, diffPositionCopy);
}
return latestDiff && discussion.active && lineCode === discussion.line_code;
} }
...@@ -27,7 +27,7 @@ export const getQuickActionText = note => { ...@@ -27,7 +27,7 @@ export const getQuickActionText = note => {
export const reduceDiscussionsToLineCodes = selectedDiscussions => export const reduceDiscussionsToLineCodes = selectedDiscussions =>
selectedDiscussions.reduce((acc, note) => { 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 // For context about line notes: there might be multiple notes with the same line code
const items = acc[note.line_code] || []; const items = acc[note.line_code] || [];
items.push(note); items.push(note);
......
---
title: Correctly show legacy diff notes in the merge request changes tab
merge_request: 21652
author:
type: fixed
...@@ -200,23 +200,20 @@ describe 'Merge request > User posts diff notes', :js do ...@@ -200,23 +200,20 @@ describe 'Merge request > User posts diff notes', :js do
end end
context 'with a new line' do context 'with a new line' do
# TODO: https://gitlab.com/gitlab-org/gitlab-ce/issues/48034 it 'allows commenting' do
xit 'allows commenting' do should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]'))
should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]').find(:xpath, '..'))
end end
end end
context 'with an old line' do context 'with an old line' do
# TODO: https://gitlab.com/gitlab-org/gitlab-ce/issues/48034 it 'allows commenting' do
xit 'allows commenting' do should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]'))
should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]').find(:xpath, '..'))
end end
end end
context 'with an unchanged line' do context 'with an unchanged line' do
# TODO: https://gitlab.com/gitlab-org/gitlab-ce/issues/48034 it 'allows commenting' do
xit 'allows commenting' do should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]'))
should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]').find(:xpath, '..'))
end end
end end
......
...@@ -157,6 +157,7 @@ describe('DiffsStoreActions', () => { ...@@ -157,6 +157,7 @@ describe('DiffsStoreActions', () => {
newPath: 'file1', newPath: 'file1',
oldLine: 5, oldLine: 5,
oldPath: 'file2', oldPath: 'file2',
lineCode: 'ABC_1_1',
}, },
}, },
}, },
......
...@@ -162,6 +162,7 @@ describe('DiffsStoreMutations', () => { ...@@ -162,6 +162,7 @@ describe('DiffsStoreMutations', () => {
}; };
const state = { const state = {
latestDiff: true,
diffFiles: [ diffFiles: [
{ {
fileHash: 'ABC', fileHash: 'ABC',
...@@ -229,6 +230,76 @@ describe('DiffsStoreMutations', () => { ...@@ -229,6 +230,76 @@ describe('DiffsStoreMutations', () => {
expect(state.diffFiles[0].highlightedDiffLines[0].discussions.length).toEqual(2); expect(state.diffFiles[0].highlightedDiffLines[0].discussions.length).toEqual(2);
expect(state.diffFiles[0].highlightedDiffLines[0].discussions[1].id).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', () => { describe('REMOVE_LINE_DISCUSSIONS', () => {
......
...@@ -3,6 +3,7 @@ import { ...@@ -3,6 +3,7 @@ import {
LINE_POSITION_LEFT, LINE_POSITION_LEFT,
LINE_POSITION_RIGHT, LINE_POSITION_RIGHT,
TEXT_DIFF_POSITION_TYPE, TEXT_DIFF_POSITION_TYPE,
LEGACY_DIFF_NOTE_TYPE,
DIFF_NOTE_TYPE, DIFF_NOTE_TYPE,
NEW_LINE_TYPE, NEW_LINE_TYPE,
OLD_LINE_TYPE, OLD_LINE_TYPE,
...@@ -151,6 +152,64 @@ describe('DiffsStoreUtils', () => { ...@@ -151,6 +152,64 @@ describe('DiffsStoreUtils', () => {
data: postData, 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', () => { describe('addLineReferences', () => {
...@@ -291,13 +350,72 @@ describe('DiffsStoreUtils', () => { ...@@ -291,13 +350,72 @@ describe('DiffsStoreUtils', () => {
it('returns true when the discussion is up to date', () => { it('returns true when the discussion is up to date', () => {
expect( expect(
utils.isDiscussionApplicableToLine(discussions.upToDateDiscussion1, diffPosition), utils.isDiscussionApplicableToLine({
discussion: discussions.upToDateDiscussion1,
diffPosition,
latestDiff: true,
}),
).toBe(true); ).toBe(true);
}); });
it('returns false when the discussion is not up to date', () => { it('returns false when the discussion is not up to date', () => {
expect( 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); ).toBe(false);
}); });
}); });
......
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