From 241b8982c45591affe4bfaafdf5821073555c6b9 Mon Sep 17 00:00:00 2001 From: Robert May Date: Mon, 30 Mar 2020 13:20:19 +0100 Subject: [PATCH 1/5] Adding `additional_lines` field to text position This adds a field for tracking additional lines on a text position, e.g. for displaying a multi-line comment in the UI. --- lib/api/discussions.rb | 1 + lib/gitlab/diff/formatters/text_formatter.rb | 4 +++- spec/factories/diff_position.rb | 5 +++++ spec/lib/gitlab/diff/formatters/text_formatter_spec.rb | 3 ++- spec/lib/gitlab/diff/position_spec.rb | 1 + .../models/diff_positionable_note_shared_examples.rb | 1 + 6 files changed, 13 insertions(+), 2 deletions(-) diff --git a/lib/api/discussions.rb b/lib/api/discussions.rb index 8ff275a3a1b0f0..128d1afdf30e65 100644 --- a/lib/api/discussions.rb +++ b/lib/api/discussions.rb @@ -70,6 +70,7 @@ class Discussions < Grape::API optional :new_line, type: Integer, desc: 'Line number after change' optional :old_path, type: String, desc: 'File path before change' optional :old_line, type: Integer, desc: 'Line number before change' + optional :additional_lines, type: Integer, desc: 'Number of lines to include before/after the line number' optional :width, type: Integer, desc: 'Width of the image' optional :height, type: Integer, desc: 'Height of the image' optional :x, type: Integer, desc: 'X coordinate in the image' diff --git a/lib/gitlab/diff/formatters/text_formatter.rb b/lib/gitlab/diff/formatters/text_formatter.rb index 5b670b1f83bd3a..e6853a9d5e889d 100644 --- a/lib/gitlab/diff/formatters/text_formatter.rb +++ b/lib/gitlab/diff/formatters/text_formatter.rb @@ -6,10 +6,12 @@ module Formatters class TextFormatter < BaseFormatter attr_reader :old_line attr_reader :new_line + attr_reader :additional_lines def initialize(attrs) @old_line = attrs[:old_line] @new_line = attrs[:new_line] + @additional_lines = attrs[:additional_lines] || 0 super(attrs) end @@ -23,7 +25,7 @@ def complete? end def to_h - super.merge(old_line: old_line, new_line: new_line) + super.merge(old_line: old_line, new_line: new_line, additional_lines: additional_lines) end def line_age diff --git a/spec/factories/diff_position.rb b/spec/factories/diff_position.rb index a43c5afdff4f12..461db31a063002 100644 --- a/spec/factories/diff_position.rb +++ b/spec/factories/diff_position.rb @@ -34,10 +34,15 @@ position_type { 'text' } old_line { 10 } new_line { 10 } + additional_lines { 0 } trait :added do old_line { nil } end + + trait :multi_line do + additional_lines { 1 } + end end factory :image_diff_position do diff --git a/spec/lib/gitlab/diff/formatters/text_formatter_spec.rb b/spec/lib/gitlab/diff/formatters/text_formatter_spec.rb index 33d4994f5db5db..4b037ebd710d3d 100644 --- a/spec/lib/gitlab/diff/formatters/text_formatter_spec.rb +++ b/spec/lib/gitlab/diff/formatters/text_formatter_spec.rb @@ -9,7 +9,8 @@ start_sha: 456, head_sha: 789, old_path: 'old_path.txt', - new_path: 'new_path.txt' + new_path: 'new_path.txt', + additional_lines: 0 } end diff --git a/spec/lib/gitlab/diff/position_spec.rb b/spec/lib/gitlab/diff/position_spec.rb index 4b11ff16c381d7..aebaaaf883d923 100644 --- a/spec/lib/gitlab/diff/position_spec.rb +++ b/spec/lib/gitlab/diff/position_spec.rb @@ -28,6 +28,7 @@ new_path: "files/ruby/popen.rb", old_line: nil, new_line: 14, + additional_lines: 0, base_sha: nil, head_sha: nil, start_sha: nil, diff --git a/spec/support/shared_examples/models/diff_positionable_note_shared_examples.rb b/spec/support/shared_examples/models/diff_positionable_note_shared_examples.rb index 38a9f1fe098f28..f6cef03f188893 100644 --- a/spec/support/shared_examples/models/diff_positionable_note_shared_examples.rb +++ b/spec/support/shared_examples/models/diff_positionable_note_shared_examples.rb @@ -13,6 +13,7 @@ new_path: "files/ruby/popen.rb", old_line: nil, new_line: 14, + additional_lines: 0, diff_refs: diff_refs ) end -- GitLab From 25815cd953ecf4d02c1c9d644e0c3e93da8f1818 Mon Sep 17 00:00:00 2001 From: Robert May Date: Mon, 30 Mar 2020 17:47:35 +0100 Subject: [PATCH 2/5] Add tests to ensure additional_lines is persisting --- .../api/diff_discussions_shared_examples.rb | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/spec/support/shared_examples/requests/api/diff_discussions_shared_examples.rb b/spec/support/shared_examples/requests/api/diff_discussions_shared_examples.rb index 583475678f193b..bf74ba61d0cb63 100644 --- a/spec/support/shared_examples/requests/api/diff_discussions_shared_examples.rb +++ b/spec/support/shared_examples/requests/api/diff_discussions_shared_examples.rb @@ -15,19 +15,34 @@ end describe "GET /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id" do - it "returns a discussion by id" do + before do get api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions/#{diff_note.discussion_id}", user) + end + it "is successful" do expect(response).to have_gitlab_http_status(:ok) + end + + it "returns a discussion by id" do expect(json_response['id']).to eq(diff_note.discussion_id) + end + + it "returns the note body" do expect(json_response['notes'].first['body']).to eq(diff_note.note) + end + + it "returns the note position" do expect(json_response['notes'].first['position']).to eq(diff_note.position.to_h.stringify_keys) end + + it "returns the note position additional_lines attribute" do + expect(json_response['notes'].first['position']['additional_lines']).to eq(0) + end end describe "POST /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions" do it "creates a new diff note" do - position = diff_note.position.to_h + position = diff_note.position.to_h.merge({ additional_lines: -5 }) post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions", user), params: { body: 'hi!', position: position } -- GitLab From 705c3ec97422b9247f1aa826efc2c824752bb38b Mon Sep 17 00:00:00 2001 From: Robert May Date: Wed, 1 Apr 2020 14:26:40 +0100 Subject: [PATCH 3/5] Fix for frontend comparison issue The frontend seems to duplicate the diff position by line-code generator used in the backend, and has hard-coded fields. This previously didn't feature the `additional_lines` key I added, so when it compared one of these generated diff positions to one received from the backend, they didn't match. --- app/assets/javascripts/diffs/store/utils.js | 1 + 1 file changed, 1 insertion(+) diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index 9c788e283b9fdb..140bc682645e5b 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -424,6 +424,7 @@ export function getDiffPositionByLineCode(diffFiles, useSingleDiffStyle) { old_path: file.old_path, old_line: line.old_line, new_line: line.new_line, + additional_lines: 0, line_code: line.line_code, position_type: 'text', }; -- GitLab From 4f27552553e04c51bf48cd363cf978063c5c2df0 Mon Sep 17 00:00:00 2001 From: Robert May Date: Wed, 1 Apr 2020 14:45:55 +0100 Subject: [PATCH 4/5] Add changelog entry --- changelogs/unreleased/multi-line-notes-take-2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/multi-line-notes-take-2.yml diff --git a/changelogs/unreleased/multi-line-notes-take-2.yml b/changelogs/unreleased/multi-line-notes-take-2.yml new file mode 100644 index 00000000000000..953a43caed780d --- /dev/null +++ b/changelogs/unreleased/multi-line-notes-take-2.yml @@ -0,0 +1,5 @@ +--- +title: Adding additional_lines to text position +merge_request: 28326 +author: +type: added -- GitLab From 5512779d6070076197935106d1453f74c2529538 Mon Sep 17 00:00:00 2001 From: Robert May Date: Wed, 1 Apr 2020 15:35:04 +0100 Subject: [PATCH 5/5] Fix JS test error --- spec/frontend/diffs/store/actions_spec.js | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index 8a1c3e56e5a5cf..ffbd9ce14bcbbf 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/spec/frontend/diffs/store/actions_spec.js @@ -466,6 +466,7 @@ describe('DiffsStoreActions', () => { old_path: 'file2', line_code: 'ABC_1_1', position_type: 'text', + additional_lines: 0, }, }, hash: 'ABC_123', -- GitLab