+
{{ startLineNumber }}
diff --git a/app/assets/javascripts/notes/mixins/diff_line_note_form.js b/app/assets/javascripts/notes/mixins/diff_line_note_form.js
index 5930b5f332157d8249470c8b8f6df519573bac57..9a2e86aeed293711d25c613ed467cb6c12d19ba5 100644
--- a/app/assets/javascripts/notes/mixins/diff_line_note_form.js
+++ b/app/assets/javascripts/notes/mixins/diff_line_note_form.js
@@ -4,6 +4,7 @@ import { TEXT_DIFF_POSITION_TYPE, IMAGE_DIFF_POSITION_TYPE } from '~/diffs/const
import createFlash from '~/flash';
import { s__ } from '~/locale';
import { clearDraft } from '~/lib/utils/autosave';
+import { formatLineRange } from '~/notes/components/multiline_comment_utils';
export default {
computed: {
@@ -45,6 +46,9 @@ export default {
});
},
addToReview(note) {
+ const lineRange =
+ (this.line && this.commentLineStart && formatLineRange(this.commentLineStart, this.line)) ||
+ {};
const positionType = this.diffFileCommentForm
? IMAGE_DIFF_POSITION_TYPE
: TEXT_DIFF_POSITION_TYPE;
@@ -60,6 +64,7 @@ export default {
linePosition: this.position,
positionType,
...this.diffFileCommentForm,
+ lineRange,
});
const diffFileHeadSha = this.commit && this?.diffFile?.diff_refs?.head_sha;
diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb
index d3dfb1813e4fef165a43ee8894d19408083b18de..eff125a19577978abc7d2984bea380a6323169d2 100644
--- a/app/controllers/concerns/notes_actions.rb
+++ b/app/controllers/concerns/notes_actions.rb
@@ -226,7 +226,7 @@ def create_note_params
end
def update_note_params
- params.require(:note).permit(:note)
+ params.require(:note).permit(:note, :position)
end
def set_polling_interval_header
diff --git a/doc/api/discussions.md b/doc/api/discussions.md
index 1f509a7aadca7d26d487f3d1331449487906dc6a..aa1a691a8f8e9413fc08ab50cca2a3ca13b4b901 100644
--- a/doc/api/discussions.md
+++ b/doc/api/discussions.md
@@ -782,10 +782,14 @@ Diff comments also contain position:
"old_line": 27,
"new_line": 27,
"line_range": {
- "start_line_code": "588440f66559714280628a4f9799f0c4eb880a4a_10_10",
- "start_line_type": "new",
- "end_line_code": "588440f66559714280628a4f9799f0c4eb880a4a_11_11",
- "end_line_type": "old"
+ "start": {
+ "line_code": "588440f66559714280628a4f9799f0c4eb880a4a_10_10",
+ "type": "new",
+ },
+ "end": {
+ "line_code": "588440f66559714280628a4f9799f0c4eb880a4a_11_11",
+ "type": "old"
+ },
}
},
"resolved": false,
@@ -832,30 +836,32 @@ POST /projects/:id/merge_requests/:merge_request_iid/discussions
Parameters:
-| Attribute | Type | Required | Description |
-| --------------------------------------- | -------------- | -------- | ----------- |
-| `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 |
-| `body` | string | yes | The content of the thread |
-| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z (requires admin or project/group owner rights) |
-| `position` | hash | no | Position when creating a diff note |
-| `position[base_sha]` | string | yes | Base commit SHA in the source branch |
-| `position[start_sha]` | string | yes | SHA referencing commit in target branch |
-| `position[head_sha]` | string | yes | SHA referencing HEAD of this merge request |
-| `position[position_type]` | string | yes | Type of the position reference', allowed values: 'text' or 'image' |
-| `position[new_path]` | string | no | File path after change |
-| `position[new_line]` | integer | no | Line number after change (for 'text' diff notes) |
-| `position[old_path]` | string | no | File path before change |
-| `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][start_line_code]` | string | yes | Line code for the start line |
-| `position[line_range][end_line_code]` | string | yes | Line code for the end line |
-| `position[line_range][start_line_type]` | string | yes | Line type for the start line |
-| `position[line_range][end_line_type]` | string | yes | Line type for the end line |
-| `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[x]` | integer | no | X coordinate (for 'image' diff notes) |
-| `position[y]` | integer | no | Y coordinate (for 'image' diff notes) |
+| Attribute | Type | Required | Description |
+| ---------------------------------------- | -------------- | -------- | ----------- |
+| `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 |
+| `body` | string | yes | The content of the thread |
+| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z (requires admin or project/group owner rights) |
+| `position` | hash | no | Position when creating a diff note |
+| `position[base_sha]` | string | yes | Base commit SHA in the source branch |
+| `position[start_sha]` | string | yes | SHA referencing commit in target branch |
+| `position[head_sha]` | string | yes | SHA referencing HEAD of this merge request |
+| `position[position_type]` | string | yes | Type of the position reference', allowed values: 'text' or 'image' |
+| `position[new_path]` | string | no | File path after change |
+| `position[new_line]` | integer | no | Line number after change (for 'text' diff notes) |
+| `position[old_path]` | string | no | File path before change |
+| `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][start]` | hash | no | Multiline note starting line |
+| `position[line_range][start][line_code]` | string | yes | Line code for the start line |
+| `position[line_range][start][type]` | string | yes | Line type for the start 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[height]` | integer | no | Height of the image (for 'image' diff notes) |
+| `position[x]` | integer | no | X coordinate (for 'image' diff notes) |
+| `position[y]` | integer | no | Y coordinate (for 'image' diff notes) |
```shell
curl --request POST --header "PRIVATE-TOKEN: " "https://gitlab.example.com/api/v4/projects/5/merge_requests/11/discussions?body=comment"
diff --git a/lib/api/discussions.rb b/lib/api/discussions.rb
index 29a13e4420dc48ccf28dd4be07f553097b804cb9..c431ec8e1e4ef35bb46c0cabd3fbd1092dfb2226 100644
--- a/lib/api/discussions.rb
+++ b/lib/api/discussions.rb
@@ -76,10 +76,18 @@ class Discussions < Grape::API::Instance
optional :y, type: Integer, desc: 'Y coordinate in the image'
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'
- requires :end_line_code, type: String, desc: 'End line code for multi-line note'
- requires :start_line_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 :start, type: Hash do
+ optional :line_code, type: String, desc: 'Start line code for multi-line note'
+ optional :type, type: String, desc: 'Start 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
diff --git a/spec/features/merge_request/user_comments_on_diff_spec.rb b/spec/features/merge_request/user_comments_on_diff_spec.rb
index 9cd3c7eaf76677c50cf0625105cfe55cae58b358..30bf82e3665964093b20bc7fdf497f6beb776dda 100644
--- a/spec/features/merge_request/user_comments_on_diff_spec.rb
+++ b/spec/features/merge_request/user_comments_on_diff_spec.rb
@@ -117,9 +117,58 @@
context 'when adding multiline comments' do
it 'saves a multiline comment' do
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
- find('#comment-line-start option', text: '-13').select_option
+ find('#comment-line-start option', exact_text: start_line).select_option
end
page.within('.js-discussion-note-form') do
@@ -131,7 +180,7 @@
page.within('.notes_holder') do
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
visit(merge_request_path(merge_request))
diff --git a/spec/frontend/diffs/components/diff_line_note_form_spec.js b/spec/frontend/diffs/components/diff_line_note_form_spec.js
index 623df8bd55e0104d2129b0476b29aa1054b36992..75ec5c202af2aed53a2b2a524c7453b69deb78cd 100644
--- a/spec/frontend/diffs/components/diff_line_note_form_spec.js
+++ b/spec/frontend/diffs/components/diff_line_note_form_spec.js
@@ -78,10 +78,18 @@ describe('DiffLineNoteForm', () => {
.mockReturnValue(Promise.resolve());
const lineRange = {
- start_line_code: wrapper.vm.commentLineStart.lineCode,
- start_line_type: wrapper.vm.commentLineStart.type,
- end_line_code: wrapper.vm.line.line_code,
- end_line_type: wrapper.vm.line.type,
+ start: {
+ line_code: wrapper.vm.commentLineStart.line_code,
+ type: wrapper.vm.commentLineStart.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 = {
diff --git a/spec/frontend/notes/components/multiline_comment_utils_spec.js b/spec/frontend/notes/components/multiline_comment_utils_spec.js
index 261bfb106e7811bc029954125c0c7e7f630c0cd5..4afb8d976712a0dae1e34d91ca53e2773478fe06 100644
--- a/spec/frontend/notes/components/multiline_comment_utils_spec.js
+++ b/spec/frontend/notes/components/multiline_comment_utils_spec.js
@@ -5,32 +5,19 @@ import {
} from '~/notes/components/multiline_comment_utils';
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`
- 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(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);
+ lineRange | start | end
+ ${lineRanges[0]} | ${'-1'} | ${'-2'}
+ ${lineRanges[1]} | ${'+1'} | ${'+2'}
+ ${lineRanges[2]} | ${'1'} | ${'2'}
+ `('returns line numbers `$start` & `$end`', ({ lineRange, start, end }) => {
+ expect(getStartLineNumber(lineRange)).toEqual(start);
+ expect(getEndLineNumber(lineRange)).toEqual(end);
});
});
describe('getSymbol', () => {
diff --git a/spec/frontend/notes/components/noteable_note_spec.js b/spec/frontend/notes/components/noteable_note_spec.js
index aa3eaa97e200c3465f171b54293ffef8d5130d98..3cb6cfe9787c9379d41af1ba239d9fea391525ea 100644
--- a/spec/frontend/notes/components/noteable_note_spec.js
+++ b/spec/frontend/notes/components/noteable_note_spec.js
@@ -46,12 +46,30 @@ describe('issue_note', () => {
it('should render if has multiline comment', () => {
const position = {
line_range: {
- start_line_code: 'abc_1_1',
- end_line_code: 'abc_2_2',
+ start: {
+ 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({
note: { ...note, position },
+ discussionRoot: true,
+ line,
});
return wrapper.vm.$nextTick().then(() => {
@@ -62,12 +80,30 @@ describe('issue_note', () => {
it('should not render if has single line comment', () => {
const position = {
line_range: {
- start_line_code: 'abc_1_1',
- end_line_code: 'abc_1_1',
+ start: {
+ 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({
note: { ...note, position },
+ discussionRoot: true,
+ line,
});
return wrapper.vm.$nextTick().then(() => {