Commit f59649d6 authored by Justin Boyson's avatar Justin Boyson Committed by Phil Hughes

Fix issues with MLC in parallel mode

Add support to side by side diffs for
multiline comments.

Most of this is around accounting for `left`
and `right` lines.
parent 9b61fc94
...@@ -71,8 +71,8 @@ export default { ...@@ -71,8 +71,8 @@ export default {
<ul class="notes draft-notes"> <ul class="notes draft-notes">
<noteable-note <noteable-note
:note="draft" :note="draft"
:diff-lines="diffFile.highlighted_diff_lines"
:line="line" :line="line"
:discussion-root="true"
class="draft-note" class="draft-note"
@handleEdit="handleEditing" @handleEdit="handleEditing"
@cancelForm="handleNotEditing" @cancelForm="handleNotEditing"
......
...@@ -35,11 +35,15 @@ export default { ...@@ -35,11 +35,15 @@ export default {
<tr :class="className" class="notes_holder"> <tr :class="className" class="notes_holder">
<td class="notes_line old"></td> <td class="notes_line old"></td>
<td class="notes-content parallel old" colspan="2"> <td class="notes-content parallel old" colspan="2">
<div v-if="leftDraft.isDraft" class="content"><draft-note :draft="leftDraft" /></div> <div v-if="leftDraft.isDraft" class="content">
<draft-note :draft="leftDraft" :line="line.left" />
</div>
</td> </td>
<td class="notes_line new"></td> <td class="notes_line new"></td>
<td class="notes-content parallel new" colspan="2"> <td class="notes-content parallel new" colspan="2">
<div v-if="rightDraft.isDraft" class="content"><draft-note :draft="rightDraft" /></div> <div v-if="rightDraft.isDraft" class="content">
<draft-note :draft="rightDraft" :line="line.right" />
</div>
</td> </td>
</tr> </tr>
</template> </template>
...@@ -52,14 +52,12 @@ export default { ...@@ -52,14 +52,12 @@ export default {
}); });
}, },
linePosition() { linePosition() {
if (this.draft.position && this.draft.position.position_type === IMAGE_DIFF_POSITION_TYPE) { if (this.position?.position_type === IMAGE_DIFF_POSITION_TYPE) {
// eslint-disable-next-line @gitlab/require-i18n-strings // eslint-disable-next-line @gitlab/require-i18n-strings
return `${this.draft.position.x}x ${this.draft.position.y}y`; return `${this.position.x}x ${this.position.y}y`;
} }
const position = this.discussion ? this.discussion.position : this.draft.position; return this.position?.new_line || this.position?.old_line;
return position?.new_line || position?.old_line;
}, },
content() { content() {
const el = document.createElement('div'); const el = document.createElement('div');
...@@ -70,11 +68,14 @@ export default { ...@@ -70,11 +68,14 @@ export default {
showLinePosition() { showLinePosition() {
return this.draft.file_hash || this.isDiffDiscussion; return this.draft.file_hash || this.isDiffDiscussion;
}, },
position() {
return this.draft.position || this.discussion.position;
},
startLineNumber() { startLineNumber() {
return getStartLineNumber(this.draft.position?.line_range); return getStartLineNumber(this.position?.line_range);
}, },
endLineNumber() { endLineNumber() {
return getEndLineNumber(this.draft.position?.line_range); return getEndLineNumber(this.position?.line_range);
}, },
}, },
methods: { methods: {
......
...@@ -8,7 +8,10 @@ import MultilineCommentForm from '../../notes/components/multiline_comment_form. ...@@ -8,7 +8,10 @@ import MultilineCommentForm from '../../notes/components/multiline_comment_form.
import autosave from '../../notes/mixins/autosave'; import autosave from '../../notes/mixins/autosave';
import userAvatarLink from '../../vue_shared/components/user_avatar/user_avatar_link.vue'; import userAvatarLink from '../../vue_shared/components/user_avatar/user_avatar_link.vue';
import { DIFF_NOTE_TYPE } from '../constants'; import { DIFF_NOTE_TYPE } from '../constants';
import { commentLineOptions } from '../../notes/components/multiline_comment_utils'; import {
commentLineOptions,
formatLineRange,
} from '../../notes/components/multiline_comment_utils';
export default { export default {
components: { components: {
...@@ -44,8 +47,10 @@ export default { ...@@ -44,8 +47,10 @@ export default {
data() { data() {
return { return {
commentLineStart: { commentLineStart: {
lineCode: this.line.line_code, line_code: this.line.line_code,
type: this.line.type, type: this.line.type,
old_line: this.line.old_line,
new_line: this.line.new_line,
}, },
}; };
}, },
...@@ -74,19 +79,26 @@ export default { ...@@ -74,19 +79,26 @@ export default {
diffViewType: this.diffViewType, diffViewType: this.diffViewType,
diffFile: this.diffFile, diffFile: this.diffFile,
linePosition: this.linePosition, linePosition: this.linePosition,
lineRange: { lineRange: formatLineRange(this.commentLineStart, this.line),
start_line_code: this.commentLineStart.lineCode,
start_line_type: this.commentLineStart.type,
end_line_code: this.line.line_code,
end_line_type: this.line.type,
},
}; };
}, },
diffFile() { diffFile() {
return this.getDiffFileByHash(this.diffFileHash); return this.getDiffFileByHash(this.diffFileHash);
}, },
commentLineOptions() { commentLineOptions() {
return commentLineOptions(this.diffFile.highlighted_diff_lines, this.line.line_code); const combineSides = (acc, { left, right }) => {
// ignore null values match lines
if (left && left.type !== 'match') acc.push(left);
// if the line_codes are identically, return to avoid duplicates
if (left?.line_code === right?.line_code) return acc;
if (right && right.type !== 'match') acc.push(right);
return acc;
};
const side = this.line.type === 'new' ? 'right' : 'left';
const lines = this.diffFile.highlighted_diff_lines.length
? this.diffFile.highlighted_diff_lines
: this.diffFile.parallel_diff_lines.reduce(combineSides, []);
return commentLineOptions(lines, this.line, this.line.line_code, side);
}, },
}, },
mounted() { mounted() {
...@@ -136,10 +148,7 @@ export default { ...@@ -136,10 +148,7 @@ export default {
<template> <template>
<div class="content discussion-form discussion-form-container discussion-notes"> <div class="content discussion-form discussion-form-container discussion-notes">
<div <div v-if="glFeatures.multilineComments" class="gl-mb-3 gl-text-gray-700">
v-if="glFeatures.multilineComments"
class="gl-mb-3 gl-text-gray-700 gl-border-gray-200 gl-border-b-solid gl-border-b-1 gl-pb-3"
>
<multiline-comment-form <multiline-comment-form
v-model="commentLineStart" v-model="commentLineStart"
:line="line" :line="line"
......
...@@ -108,6 +108,7 @@ export default { ...@@ -108,6 +108,7 @@ export default {
:commit="commit" :commit="commit"
:help-page-path="helpPagePath" :help-page-path="helpPagePath"
:show-reply-button="userCanReply" :show-reply-button="userCanReply"
:discussion-root="true"
@handleDeleteNote="$emit('deleteNote')" @handleDeleteNote="$emit('deleteNote')"
@startReplying="$emit('startReplying')" @startReplying="$emit('startReplying')"
> >
...@@ -151,6 +152,7 @@ export default { ...@@ -151,6 +152,7 @@ export default {
:note="componentData(note)" :note="componentData(note)"
:help-page-path="helpPagePath" :help-page-path="helpPagePath"
:line="diffLine" :line="diffLine"
:discussion-root="index === 0"
@handleDeleteNote="$emit('deleteNote')" @handleDeleteNote="$emit('deleteNote')"
> >
<slot v-if="index === 0" slot="avatar-badge" name="avatar-badge"></slot> <slot v-if="index === 0" slot="avatar-badge" name="avatar-badge"></slot>
......
...@@ -21,10 +21,23 @@ export default { ...@@ -21,10 +21,23 @@ export default {
}, },
data() { data() {
return { return {
commentLineStart: { commentLineStart: {},
lineCode: this.lineRange ? this.lineRange.start_line_code : this.line.line_code, commentLineEndType: this.lineRange?.end?.line_type || this.line.type,
type: this.lineRange ? this.lineRange.start_line_type : this.line.type, };
},
computed: {
lineNumber() {
return this.commentLineOptions[this.commentLineOptions.length - 1].text;
},
}, },
created() {
const line = this.lineRange?.start || this.line;
this.commentLineStart = {
line_code: line.line_code,
type: line.type,
old_line: line.old_line,
new_line: line.new_line,
}; };
}, },
methods: { methods: {
...@@ -34,6 +47,10 @@ export default { ...@@ -34,6 +47,10 @@ export default {
getLineClasses(line) { getLineClasses(line) {
return getLineClasses(line); return getLineClasses(line);
}, },
updateCommentLineStart(value) {
this.commentLineStart = value;
this.$emit('input', value);
},
}, },
}; };
</script> </script>
...@@ -55,12 +72,12 @@ export default { ...@@ -55,12 +72,12 @@ export default {
:options="commentLineOptions" :options="commentLineOptions"
size="sm" size="sm"
class="gl-w-auto gl-vertical-align-baseline" class="gl-w-auto gl-vertical-align-baseline"
@change="$emit('input', $event)" @change="updateCommentLineStart"
/> />
</template> </template>
<template #end> <template #end>
<span :class="getLineClasses(line)"> <span :class="getLineClasses(line)">
{{ getSymbol(line) + (line.new_line || line.old_line) }} {{ lineNumber }}
</span> </span>
</template> </template>
</gl-sprintf> </gl-sprintf>
......
...@@ -7,11 +7,19 @@ export function getSymbol(type) { ...@@ -7,11 +7,19 @@ export function getSymbol(type) {
} }
function getLineNumber(lineRange, key) { function getLineNumber(lineRange, key) {
if (!lineRange || !key) return ''; if (!lineRange || !key || !lineRange[key]) return '';
const lineCode = lineRange[`${key}_line_code`] || ''; const { new_line: newLine, old_line: oldLine, type } = lineRange[key];
const lineType = lineRange[`${key}_line_type`] || ''; const otherKey = key === 'start' ? 'end' : 'start';
const lines = lineCode.split('_') || [];
const lineNumber = lineType === 'old' ? lines[1] : lines[2]; // By default we want to see the "old" or "left side" line number
// The exception is if the "end" line is on the "right" side
// `otherLineType` is only used if `type` is null to make sure the line
// number relfects the "right" side number, if that is the side
// the comment form is located on
const otherLineType = !type ? lineRange[otherKey]?.type : null;
const lineType = type || '';
let lineNumber = oldLine;
if (lineType === 'new' || otherLineType === 'new') lineNumber = newLine;
return (lineNumber && getSymbol(lineType) + lineNumber) || ''; return (lineNumber && getSymbol(lineType) + lineNumber) || '';
} }
...@@ -37,21 +45,48 @@ export function getLineClasses(line) { ...@@ -37,21 +45,48 @@ export function getLineClasses(line) {
]; ];
} }
export function commentLineOptions(diffLines, lineCode) { export function commentLineOptions(diffLines, startingLine, lineCode, side = 'left') {
const selectedIndex = diffLines.findIndex(line => line.line_code === lineCode); const preferredSide = side === 'left' ? 'old_line' : 'new_line';
const fallbackSide = preferredSide === 'new_line' ? 'old_line' : 'new_line';
const notMatchType = l => l.type !== 'match'; const notMatchType = l => l.type !== 'match';
const linesCopy = [...diffLines]; // don't mutate the argument
const startingLineCode = startingLine.line_code;
const currentIndex = linesCopy.findIndex(line => line.line_code === lineCode);
// We're limiting adding comments to only lines above the current line // We're limiting adding comments to only lines above the current line
// to make rendering simpler. Future interations will use a more // to make rendering simpler. Future interations will use a more
// intuitive dragging interface that will make this unnecessary // intuitive dragging interface that will make this unnecessary
const upToSelected = diffLines.slice(0, selectedIndex + 1); const upToSelected = linesCopy.slice(0, currentIndex + 1);
// Only include the lines up to the first "Show unchanged lines" block // Only include the lines up to the first "Show unchanged lines" block
// i.e. not a "match" type // i.e. not a "match" type
const lines = takeRightWhile(upToSelected, notMatchType); const lines = takeRightWhile(upToSelected, notMatchType);
return lines.map(l => ({ // If the selected line is "hidden" in an unchanged line block
value: { lineCode: l.line_code, type: l.type }, // or "above" the current group of lines add it to the array so
text: `${getSymbol(l.type)}${l.new_line || l.old_line}`, // that the drop down is not defaulted to empty
})); const selectedIndex = lines.findIndex(line => line.line_code === startingLineCode);
if (selectedIndex < 0) lines.unshift(startingLine);
return lines.map(l => {
const { line_code, type, old_line, new_line } = l;
return {
value: { line_code, type, old_line, new_line },
text: `${getSymbol(type)}${l[preferredSide] || l[fallbackSide]}`,
};
});
}
export function formatLineRange(start, end) {
const extractProps = ({ line_code, type, old_line, new_line }) => ({
line_code,
type,
old_line,
new_line,
});
return {
start: extractProps(start),
end: extractProps(end),
};
} }
...@@ -21,6 +21,7 @@ import { ...@@ -21,6 +21,7 @@ import {
getEndLineNumber, getEndLineNumber,
getLineClasses, getLineClasses,
commentLineOptions, commentLineOptions,
formatLineRange,
} from './multiline_comment_utils'; } from './multiline_comment_utils';
import MultilineCommentForm from './multiline_comment_form.vue'; import MultilineCommentForm from './multiline_comment_form.vue';
...@@ -62,10 +63,15 @@ export default { ...@@ -62,10 +63,15 @@ export default {
default: false, default: false,
}, },
diffLines: { diffLines: {
type: Object, type: Array,
required: false, required: false,
default: null, default: null,
}, },
discussionRoot: {
type: Boolean,
required: false,
default: false,
},
}, },
data() { data() {
return { return {
...@@ -73,10 +79,7 @@ export default { ...@@ -73,10 +79,7 @@ export default {
isDeleting: false, isDeleting: false,
isRequesting: false, isRequesting: false,
isResolving: false, isResolving: false,
commentLineStart: { commentLineStart: {},
line_code: this.line?.line_code,
type: this.line?.type,
},
}; };
}, },
computed: { computed: {
...@@ -144,25 +147,42 @@ export default { ...@@ -144,25 +147,42 @@ export default {
return getEndLineNumber(this.lineRange); return getEndLineNumber(this.lineRange);
}, },
showMultiLineComment() { showMultiLineComment() {
return ( if (!this.glFeatures.multilineComments) return false;
this.glFeatures.multilineComments && if (this.isEditing) return true;
this.startLineNumber &&
this.endLineNumber && return this.line && this.discussionRoot && this.startLineNumber !== this.endLineNumber;
(this.startLineNumber !== this.endLineNumber || this.isEditing)
);
}, },
commentLineOptions() { commentLineOptions() {
if (this.diffLines) { if (!this.diffFile || !this.line) return [];
return commentLineOptions(this.diffLines, this.line.line_code);
const sideA = this.line.type === 'new' ? 'right' : 'left';
const sideB = sideA === 'left' ? 'right' : 'left';
const lines = this.diffFile.highlighted_diff_lines.length
? this.diffFile.highlighted_diff_lines
: this.diffFile.parallel_diff_lines.map(l => l[sideA] || l[sideB]);
return commentLineOptions(lines, this.commentLineStart, this.line.line_code, sideA);
},
diffFile() {
if (this.commentLineStart.line_code) {
const lineCode = this.commentLineStart.line_code.split('_')[0];
return this.getDiffFileByHash(lineCode);
} }
const diffFile = this.diffFile || this.getDiffFileByHash(this.targetNoteHash); return null;
if (!diffFile) return null;
return commentLineOptions(diffFile.highlighted_diff_lines, this.line.line_code);
}, },
}, },
created() { created() {
const line = this.note.position?.line_range?.start || this.line;
this.commentLineStart = line
? {
line_code: line.line_code,
type: line.type,
old_line: line.old_line,
new_line: line.new_line,
}
: {};
eventHub.$on('enterEditMode', ({ noteId }) => { eventHub.$on('enterEditMode', ({ noteId }) => {
if (noteId === this.note.id) { if (noteId === this.note.id) {
this.isEditing = true; this.isEditing = true;
...@@ -224,13 +244,11 @@ export default { ...@@ -224,13 +244,11 @@ export default {
formUpdateHandler(noteText, parentElement, callback, resolveDiscussion) { formUpdateHandler(noteText, parentElement, callback, resolveDiscussion) {
const position = { const position = {
...this.note.position, ...this.note.position,
line_range: {
start_line_code: this.commentLineStart?.lineCode,
start_line_type: this.commentLineStart?.type,
end_line_code: this.line?.line_code,
end_line_type: this.line?.type,
},
}; };
if (this.commentLineStart && this.line)
position.line_range = formatLineRange(this.commentLineStart, this.line);
this.$emit('handleUpdateNote', { this.$emit('handleUpdateNote', {
note: this.note, note: this.note,
noteText, noteText,
...@@ -246,7 +264,7 @@ export default { ...@@ -246,7 +264,7 @@ export default {
note: { note: {
target_type: this.getNoteableData.targetType, target_type: this.getNoteableData.targetType,
target_id: this.note.noteable_id, target_id: this.note.noteable_id,
note: { note: noteText }, note: { note: noteText, position: JSON.stringify(position) },
}, },
}; };
this.isRequesting = true; this.isRequesting = true;
...@@ -317,14 +335,17 @@ export default { ...@@ -317,14 +335,17 @@ export default {
> >
<div v-if="showMultiLineComment" data-testid="multiline-comment"> <div v-if="showMultiLineComment" data-testid="multiline-comment">
<multiline-comment-form <multiline-comment-form
v-if="isEditing && commentLineOptions && line" v-if="isEditing && note.position"
v-model="commentLineStart" v-model="commentLineStart"
:line="line" :line="line"
:comment-line-options="commentLineOptions" :comment-line-options="commentLineOptions"
:line-range="note.position.line_range" :line-range="note.position.line_range"
class="gl-mb-3 gl-text-gray-700 gl-border-gray-200 gl-border-b-solid gl-border-b-1 gl-pb-3" class="gl-mb-3 gl-text-gray-700"
/> />
<div v-else class="gl-mb-3 gl-text-gray-700"> <div
v-else
class="gl-mb-3 gl-text-gray-700 gl-border-gray-200 gl-border-b-solid gl-border-b-1 gl-pb-3"
>
<gl-sprintf :message="__('Comment on lines %{startLine} to %{endLine}')"> <gl-sprintf :message="__('Comment on lines %{startLine} to %{endLine}')">
<template #startLine> <template #startLine>
<span :class="getLineClasses(startLineNumber)">{{ startLineNumber }}</span> <span :class="getLineClasses(startLineNumber)">{{ startLineNumber }}</span>
......
...@@ -4,6 +4,7 @@ import { TEXT_DIFF_POSITION_TYPE, IMAGE_DIFF_POSITION_TYPE } from '~/diffs/const ...@@ -4,6 +4,7 @@ import { TEXT_DIFF_POSITION_TYPE, IMAGE_DIFF_POSITION_TYPE } from '~/diffs/const
import createFlash from '~/flash'; import createFlash from '~/flash';
import { s__ } from '~/locale'; import { s__ } from '~/locale';
import { clearDraft } from '~/lib/utils/autosave'; import { clearDraft } from '~/lib/utils/autosave';
import { formatLineRange } from '~/notes/components/multiline_comment_utils';
export default { export default {
computed: { computed: {
...@@ -45,6 +46,9 @@ export default { ...@@ -45,6 +46,9 @@ export default {
}); });
}, },
addToReview(note) { addToReview(note) {
const lineRange =
(this.line && this.commentLineStart && formatLineRange(this.commentLineStart, this.line)) ||
{};
const positionType = this.diffFileCommentForm const positionType = this.diffFileCommentForm
? IMAGE_DIFF_POSITION_TYPE ? IMAGE_DIFF_POSITION_TYPE
: TEXT_DIFF_POSITION_TYPE; : TEXT_DIFF_POSITION_TYPE;
...@@ -60,6 +64,7 @@ export default { ...@@ -60,6 +64,7 @@ export default {
linePosition: this.position, linePosition: this.position,
positionType, positionType,
...this.diffFileCommentForm, ...this.diffFileCommentForm,
lineRange,
}); });
const diffFileHeadSha = this.commit && this?.diffFile?.diff_refs?.head_sha; const diffFileHeadSha = this.commit && this?.diffFile?.diff_refs?.head_sha;
......
...@@ -226,7 +226,7 @@ module NotesActions ...@@ -226,7 +226,7 @@ module NotesActions
end end
def update_note_params def update_note_params
params.require(:note).permit(:note) params.require(:note).permit(:note, :position)
end end
def set_polling_interval_header def set_polling_interval_header
......
...@@ -782,10 +782,14 @@ Diff comments also contain position: ...@@ -782,10 +782,14 @@ Diff comments also contain position:
"old_line": 27, "old_line": 27,
"new_line": 27, "new_line": 27,
"line_range": { "line_range": {
"start_line_code": "588440f66559714280628a4f9799f0c4eb880a4a_10_10", "start": {
"start_line_type": "new", "line_code": "588440f66559714280628a4f9799f0c4eb880a4a_10_10",
"end_line_code": "588440f66559714280628a4f9799f0c4eb880a4a_11_11", "type": "new",
"end_line_type": "old" },
"end": {
"line_code": "588440f66559714280628a4f9799f0c4eb880a4a_11_11",
"type": "old"
},
} }
}, },
"resolved": false, "resolved": false,
...@@ -833,7 +837,7 @@ POST /projects/:id/merge_requests/:merge_request_iid/discussions ...@@ -833,7 +837,7 @@ POST /projects/:id/merge_requests/:merge_request_iid/discussions
Parameters: Parameters:
| Attribute | Type | Required | Description | | Attribute | Type | Required | Description |
| --------------------------------------- | -------------- | -------- | ----------- | | ---------------------------------------- | -------------- | -------- | ----------- |
| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) | | `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) |
| `merge_request_iid` | integer | yes | The IID of a merge request | | `merge_request_iid` | integer | yes | The IID of a merge request |
| `body` | string | yes | The content of the thread | | `body` | string | yes | The content of the thread |
...@@ -848,10 +852,12 @@ Parameters: ...@@ -848,10 +852,12 @@ Parameters:
| `position[old_path]` | string | no | File path before change | | `position[old_path]` | string | no | File path before change |
| `position[old_line]` | integer | no | Line number before change (for 'text' diff notes) | | `position[old_line]` | integer | no | Line number before change (for 'text' diff notes) |
| `position[line_range]` | hash | no | Line range for a multi-line diff note | | `position[line_range]` | hash | no | Line range for a multi-line diff note |
| `position[line_range][start_line_code]` | string | yes | Line code for the start line | | `position[line_range][start]` | hash | no | Multiline note starting line |
| `position[line_range][end_line_code]` | string | yes | Line code for the end line | | `position[line_range][start][line_code]` | string | yes | Line code for the start line |
| `position[line_range][start_line_type]` | string | yes | Line type for the start line | | `position[line_range][start][type]` | string | yes | Line type for the start line |
| `position[line_range][end_line_type]` | string | yes | Line type for the end line | | `position[line_range][end]` | hash | no | Multiline note ending line |
| `position[line_range][end][line_code]` | string | yes | Line code for the end line |
| `position[line_range][end][type]` | string | yes | Line type for the end line |
| `position[width]` | integer | no | Width of the image (for 'image' diff notes) | | `position[width]` | integer | no | Width of the image (for 'image' diff notes) |
| `position[height]` | integer | no | Height of the image (for 'image' diff notes) | | `position[height]` | integer | no | Height of the image (for 'image' diff notes) |
| `position[x]` | integer | no | X coordinate (for 'image' diff notes) | | `position[x]` | integer | no | X coordinate (for 'image' diff notes) |
......
...@@ -76,10 +76,18 @@ module API ...@@ -76,10 +76,18 @@ module API
optional :y, type: Integer, desc: 'Y coordinate in the image' optional :y, type: Integer, desc: 'Y coordinate in the image'
optional :line_range, type: Hash, desc: 'Multi-line start and end' do optional :line_range, type: Hash, desc: 'Multi-line start and end' do
requires :start_line_code, type: String, desc: 'Start line code for multi-line note' optional :start, type: Hash do
requires :end_line_code, type: String, desc: 'End line code for multi-line note' optional :line_code, type: String, desc: 'Start line code for multi-line note'
requires :start_line_type, type: String, desc: 'Start line type for multi-line note' optional :type, type: String, desc: 'Start line type for multi-line note'
requires :end_line_type, type: String, desc: 'End line type for multi-line note' optional :old_line, type: String, desc: 'Start old_line line number'
optional :new_line, type: String, desc: 'Start new_line line number'
end
optional :end, type: Hash do
optional :line_code, type: String, desc: 'End line code for multi-line note'
optional :type, type: String, desc: 'End line type for multi-line note'
optional :old_line, type: String, desc: 'End old_line line number'
optional :new_line, type: String, desc: 'End new_line line number'
end
end end
end end
end end
......
...@@ -117,9 +117,58 @@ RSpec.describe 'User comments on a diff', :js do ...@@ -117,9 +117,58 @@ RSpec.describe 'User comments on a diff', :js do
context 'when adding multiline comments' do context 'when adding multiline comments' do
it 'saves a multiline comment' do it 'saves a multiline comment' do
click_diff_line(find("[id='#{sample_commit.line_code}']")) click_diff_line(find("[id='#{sample_commit.line_code}']"))
add_comment('-13', '+14')
end
context 'when in side-by-side view' do
before do
visit(diffs_project_merge_request_path(project, merge_request, view: 'parallel'))
end
# In `files/ruby/popen.rb`
it 'allows comments for changes involving both sides' do
# click +15, select -13 add and verify comment
click_diff_line(find('div[data-path="files/ruby/popen.rb"] .new_line a[data-linenumber="15"]').find(:xpath, '../..'), 'right')
add_comment('-13', '+15')
end
it 'allows comments to start above hidden lines and end below' do
# click +28, select 21 add and verify comment
click_diff_line(find('div[data-path="files/ruby/popen.rb"] .new_line a[data-linenumber="28"]').find(:xpath, '../..'), 'right')
add_comment('21', '+28')
end
it 'allows comments on previously hidden lines at the top of a file' do
# Click -9, expand up, select 1 add and verify comment
page.within('[data-path="files/ruby/popen.rb"]') do
all('.js-unfold-all')[0].click
end
click_diff_line(find('div[data-path="files/ruby/popen.rb"] .old_line a[data-linenumber="9"]').find(:xpath, '../..'), 'left')
add_comment('1', '-9')
end
it 'allows comments on previously hidden lines the middle of a file' do
# Click 27, expand up, select 18, add and verify comment
page.within('[data-path="files/ruby/popen.rb"]') do
all('.js-unfold-all')[1].click
end
click_diff_line(find('div[data-path="files/ruby/popen.rb"] .old_line a[data-linenumber="21"]').find(:xpath, '../..'), 'left')
add_comment('18', '21')
end
it 'allows comments on previously hidden lines at the bottom of a file' do
# Click +28, expand down, select 37 add and verify comment
page.within('[data-path="files/ruby/popen.rb"]') do
all('.js-unfold-down')[1].click
end
click_diff_line(find('div[data-path="files/ruby/popen.rb"] .old_line a[data-linenumber="30"]').find(:xpath, '../..'), 'left')
add_comment('+28', '37')
end
end
def add_comment(start_line, end_line)
page.within('.discussion-form') do page.within('.discussion-form') do
find('#comment-line-start option', text: '-13').select_option find('#comment-line-start option', exact_text: start_line).select_option
end end
page.within('.js-discussion-note-form') do page.within('.js-discussion-note-form') do
...@@ -131,7 +180,7 @@ RSpec.describe 'User comments on a diff', :js do ...@@ -131,7 +180,7 @@ RSpec.describe 'User comments on a diff', :js do
page.within('.notes_holder') do page.within('.notes_holder') do
expect(page).to have_content('Line is wrong') expect(page).to have_content('Line is wrong')
expect(page).to have_content('Comment on lines -13 to +14') expect(page).to have_content("Comment on lines #{start_line} to #{end_line}")
end end
visit(merge_request_path(merge_request)) visit(merge_request_path(merge_request))
......
...@@ -78,10 +78,18 @@ describe('DiffLineNoteForm', () => { ...@@ -78,10 +78,18 @@ describe('DiffLineNoteForm', () => {
.mockReturnValue(Promise.resolve()); .mockReturnValue(Promise.resolve());
const lineRange = { const lineRange = {
start_line_code: wrapper.vm.commentLineStart.lineCode, start: {
start_line_type: wrapper.vm.commentLineStart.type, line_code: wrapper.vm.commentLineStart.line_code,
end_line_code: wrapper.vm.line.line_code, type: wrapper.vm.commentLineStart.type,
end_line_type: wrapper.vm.line.type, new_line: 1,
old_line: null,
},
end: {
line_code: wrapper.vm.line.line_code,
type: wrapper.vm.line.type,
new_line: 1,
old_line: null,
},
}; };
const formData = { const formData = {
......
...@@ -5,32 +5,19 @@ import { ...@@ -5,32 +5,19 @@ import {
} from '~/notes/components/multiline_comment_utils'; } from '~/notes/components/multiline_comment_utils';
describe('Multiline comment utilities', () => { describe('Multiline comment utilities', () => {
describe('getStartLineNumber', () => { describe('get start & end line numbers', () => {
const lineRanges = ['old', 'new', null].map(type => ({
start: { new_line: 1, old_line: 1, type },
end: { new_line: 2, old_line: 2, type },
}));
it.each` it.each`
lineCode | type | result lineRange | start | end
${'abcdef_1_1'} | ${'old'} | ${'-1'} ${lineRanges[0]} | ${'-1'} | ${'-2'}
${'abcdef_1_1'} | ${'new'} | ${'+1'} ${lineRanges[1]} | ${'+1'} | ${'+2'}
${'abcdef_1_1'} | ${null} | ${'1'} ${lineRanges[2]} | ${'1'} | ${'2'}
${'abcdef'} | ${'new'} | ${''} `('returns line numbers `$start` & `$end`', ({ lineRange, start, end }) => {
${'abcdef'} | ${'old'} | ${''} expect(getStartLineNumber(lineRange)).toEqual(start);
${'abcdef'} | ${null} | ${''} expect(getEndLineNumber(lineRange)).toEqual(end);
`('returns line number', ({ lineCode, type, result }) => {
expect(getStartLineNumber({ start_line_code: lineCode, start_line_type: type })).toEqual(
result,
);
});
});
describe('getEndLineNumber', () => {
it.each`
lineCode | type | result
${'abcdef_1_1'} | ${'old'} | ${'-1'}
${'abcdef_1_1'} | ${'new'} | ${'+1'}
${'abcdef_1_1'} | ${null} | ${'1'}
${'abcdef'} | ${'new'} | ${''}
${'abcdef'} | ${'old'} | ${''}
${'abcdef'} | ${null} | ${''}
`('returns line number', ({ lineCode, type, result }) => {
expect(getEndLineNumber({ end_line_code: lineCode, end_line_type: type })).toEqual(result);
}); });
}); });
describe('getSymbol', () => { describe('getSymbol', () => {
......
...@@ -46,12 +46,30 @@ describe('issue_note', () => { ...@@ -46,12 +46,30 @@ describe('issue_note', () => {
it('should render if has multiline comment', () => { it('should render if has multiline comment', () => {
const position = { const position = {
line_range: { line_range: {
start_line_code: 'abc_1_1', start: {
end_line_code: 'abc_2_2', line_code: 'abc_1_1',
type: null,
old_line: '1',
new_line: '1',
}, },
end: {
line_code: 'abc_2_2',
type: null,
old_line: '2',
new_line: '2',
},
},
};
const line = {
line_code: 'abc_1_1',
type: null,
old_line: '1',
new_line: '1',
}; };
wrapper.setProps({ wrapper.setProps({
note: { ...note, position }, note: { ...note, position },
discussionRoot: true,
line,
}); });
return wrapper.vm.$nextTick().then(() => { return wrapper.vm.$nextTick().then(() => {
...@@ -62,12 +80,30 @@ describe('issue_note', () => { ...@@ -62,12 +80,30 @@ describe('issue_note', () => {
it('should not render if has single line comment', () => { it('should not render if has single line comment', () => {
const position = { const position = {
line_range: { line_range: {
start_line_code: 'abc_1_1', start: {
end_line_code: 'abc_1_1', line_code: 'abc_1_1',
type: null,
old_line: '1',
new_line: '1',
}, },
end: {
line_code: 'abc_1_1',
type: null,
old_line: '1',
new_line: '1',
},
},
};
const line = {
line_code: 'abc_1_1',
type: null,
old_line: '1',
new_line: '1',
}; };
wrapper.setProps({ wrapper.setProps({
note: { ...note, position }, note: { ...note, position },
discussionRoot: true,
line,
}); });
return wrapper.vm.$nextTick().then(() => { return wrapper.vm.$nextTick().then(() => {
......
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