Add line range to diff note position

Adds support for accepting a hash for `line_range` with start and
end line codes. This is the easiest way to pass this data between
frontend and backend.

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,
+        line_range: null,
         line_code: line.line_code,
         position_type: 'text',
       };

app/models/diff_note_position.rb

@@ -2,6 +2,7 @@
 
 class DiffNotePosition < ApplicationRecord
   belongs_to :note
+  attr_accessor :line_range
 
   enum diff_content_type: {
     text: 0,
@@ -42,6 +43,7 @@ class DiffNotePosition < ApplicationRecord
   def self.position_to_attrs(position)
     position_attrs = position.to_h
     position_attrs[:diff_content_type] = position_attrs.delete(:position_type)
+    position_attrs.delete(:line_range)
     position_attrs
   end
 end

changelogs/unreleased/multi-line-notes-take-3.yml

+---
+title: Add line range to diff note position
+merge_request: 29135
+author:
+type: added

doc/api/discussions.md

@@ -739,7 +739,7 @@ GET /projects/:id/merge_requests/:merge_request_iid/discussions
 ]
 ```
 
-Diff comments contain also position:
+Diff comments also contain position:
 
 ```json
 [
@@ -774,7 +774,11 @@ Diff comments contain also position:
           "new_path": "package.json",
           "position_type": "text",
           "old_line": 27,
-          "new_line": 27
+          "new_line": 27,
+          "line_range": {
+            "start_line_code": "588440f66559714280628a4f9799f0c4eb880a4a_10_10",
+            "end_line_code": "588440f66559714280628a4f9799f0c4eb880a4a_11_11"
+          }
         },
         "resolved": false,
         "resolvable": true,
@@ -820,25 +824,28 @@ 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[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_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[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: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/merge_requests/11/discussions?body=comment

lib/api/discussions.rb

@@ -74,6 +74,11 @@ module API
             optional :height, type: Integer, desc: 'Height of the image'
             optional :x, type: Integer, desc: 'X 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
+              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'
+            end
           end
         end
         post ":id/#{noteables_path}/:noteable_id/discussions" do

lib/gitlab/diff/formatters/text_formatter.rb

@@ -6,10 +6,12 @@ module Gitlab
       class TextFormatter < BaseFormatter
         attr_reader :old_line
         attr_reader :new_line
+        attr_reader :line_range
 
         def initialize(attrs)
           @old_line = attrs[:old_line]
           @new_line = attrs[:new_line]
+          @line_range = attrs[:line_range]
 
           super(attrs)
         end
@@ -23,7 +25,7 @@ module Gitlab
         end
 
         def to_h
-          super.merge(old_line: old_line, new_line: new_line)
+          super.merge(old_line: old_line, new_line: new_line, line_range: line_range)
         end
 
         def line_age

spec/factories/diff_position.rb

@@ -34,10 +34,20 @@ FactoryBot.define do
       position_type { 'text' }
       old_line { 10 }
       new_line { 10 }
+      line_range { nil }
 
       trait :added do
         old_line { nil }
       end
+
+      trait :multi_line do
+        line_range do
+          {
+            start_line_code: Gitlab::Git.diff_line_code(file, 10, 10),
+            end_line_code: Gitlab::Git.diff_line_code(file, 12, 13)
+          }
+        end
+      end
     end
 
     factory :image_diff_position do

spec/frontend/diffs/store/actions_spec.js

@@ -466,6 +466,7 @@ describe('DiffsStoreActions', () => {
                   old_path: 'file2',
                   line_code: 'ABC_1_1',
                   position_type: 'text',
+                  line_range: null,
                 },
               },
               hash: 'ABC_123',

spec/lib/gitlab/diff/formatters/text_formatter_spec.rb

@@ -9,7 +9,8 @@ describe Gitlab::Diff::Formatters::TextFormatter do
       start_sha: 456,
       head_sha: 789,
       old_path: 'old_path.txt',
-      new_path: 'new_path.txt'
+      new_path: 'new_path.txt',
+      line_range: nil
     }
   end
 

spec/lib/gitlab/diff/position_spec.rb

@@ -28,6 +28,7 @@ describe Gitlab::Diff::Position do
       new_path: "files/ruby/popen.rb",
       old_line: nil,
       new_line: 14,
+      line_range: nil,
       base_sha: nil,
       head_sha: nil,
       start_sha: nil,

spec/models/diff_note_position_spec.rb

@@ -40,4 +40,11 @@ describe DiffNotePosition, type: :model do
 
     expect { diff_note_position.save! }.to raise_error(ActiveRecord::RecordNotUnique)
   end
+
+  it 'accepts a line_range attribute' do
+    diff_note_position = build(:diff_note_position)
+
+    expect(diff_note_position).to respond_to(:line_range)
+    expect(diff_note_position).to respond_to(:line_range=)
+  end
 end

spec/support/shared_examples/models/diff_positionable_note_shared_examples.rb

@@ -13,6 +13,7 @@ RSpec.shared_examples 'a valid diff positionable note' do |factory_on_commit|
         new_path: "files/ruby/popen.rb",
         old_line: nil,
         new_line: 14,
+        line_range: nil,
         diff_refs: diff_refs
       )
     end

spec/support/shared_examples/requests/api/diff_discussions_shared_examples.rb

@@ -22,12 +22,18 @@ RSpec.shared_examples 'diff discussions API' do |parent_type, noteable_type, id_
       expect(json_response['id']).to eq(diff_note.discussion_id)
       expect(json_response['notes'].first['body']).to eq(diff_note.note)
       expect(json_response['notes'].first['position']).to eq(diff_note.position.to_h.stringify_keys)
+      expect(json_response['notes'].first['line_range']).to eq(nil)
     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
+      line_range = {
+        "start_line_code" => Gitlab::Git.diff_line_code(diff_note.position.file_path, 1, 1),
+        "end_line_code" => Gitlab::Git.diff_line_code(diff_note.position.file_path, 2, 2)
+      }
+
+      position = diff_note.position.to_h.merge({ line_range: line_range })
 
       post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions", user),
         params: { body: 'hi!', position: position }