diff --git a/lib/api/draft_notes.rb b/lib/api/draft_notes.rb
index df9e060e5927be054c72cb464f03fd2161c54fa4..ff71d6bebb5df219f8fdd28a7fc87e018ba472e6 100644
--- a/lib/api/draft_notes.rb
+++ b/lib/api/draft_notes.rb
@@ -45,9 +45,42 @@ def authorize_admin_draft!(draft_note)
access_denied! unless can?(current_user, :admin_note, draft_note)
end
+ params :positional do
+ optional :position, type: Hash do
+ requires :base_sha, type: String, desc: 'Base commit SHA in the source branch'
+ requires :start_sha, type: String, desc: 'SHA referencing commit in target branch'
+ requires :head_sha, type: String, desc: 'SHA referencing HEAD of this merge request'
+ requires :position_type, type: String, desc: 'Type of the position reference', values: %w[text image]
+ optional :new_path, type: String, desc: 'File path after change'
+ 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 :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'
+ optional :y, type: Integer, desc: 'Y coordinate in the image'
+
+ optional :line_range, type: Hash, desc: 'Multi-line start and end' do
+ 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
+
def draft_note_params
{
note: params[:note],
+ position: params[:position],
commit_id: params[:commit_id] == 'undefined' ? nil : params[:commit_id],
resolve_discussion: params[:resolve_discussion] || false
}
@@ -107,6 +140,7 @@ def draft_note_params
optional :in_reply_to_discussion_id, type: Integer, desc: 'The ID of a discussion the draft note replies to.'
optional :commit_id, type: String, desc: 'The sha of a commit to associate the draft note to.'
optional :resolve_discussion, type: Boolean, desc: 'The associated discussion should be resolved.'
+ use :positional
end
post ":id/merge_requests/:merge_request_iid/draft_notes", feature_category: :code_review_workflow do
authorize_create_note!(params: params)
@@ -135,6 +169,7 @@ def draft_note_params
requires :merge_request_iid, type: Integer, desc: "The ID of a merge request."
requires :draft_note_id, type: Integer, desc: "The ID of a draft note"
optional :note, type: String, allow_blank: false, desc: 'The content of a note.'
+ use :positional
end
put ":id/merge_requests/:merge_request_iid/draft_notes/:draft_note_id", feature_category: :code_review_workflow do
bad_request!('Missing params to modify') unless params[:note].present?
@@ -144,7 +179,7 @@ def draft_note_params
if draft_note
authorize_admin_draft!(draft_note)
- draft_note.update!(note: params[:note])
+ draft_note.update!(note: params[:note], position: params[:position])
present draft_note, with: Entities::DraftNote
else
not_found!("Draft Note")
diff --git a/spec/requests/api/draft_notes_spec.rb b/spec/requests/api/draft_notes_spec.rb
index 3911bb8bc0061022e0b255406406e0bd2076137d..f15ed6e2d5f15cbffda8598b0cf765ae8dc1e217 100644
--- a/spec/requests/api/draft_notes_spec.rb
+++ b/spec/requests/api/draft_notes_spec.rb
@@ -5,7 +5,7 @@
RSpec.describe API::DraftNotes, feature_category: :code_review_workflow do
let_it_be(:user) { create(:user) }
let_it_be(:user_2) { create(:user) }
- let_it_be(:project) { create(:project, :public) }
+ let_it_be(:project) { create(:project, :public, :repository) }
let_it_be(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: user) }
let_it_be(:private_project) { create(:project, :private) }
@@ -184,6 +184,24 @@ def create_draft_note(params = {}, url = base_url)
end
end
+ context "when using a diff with position" do
+ let!(:draft_note) { create(:draft_note_on_text_diff, merge_request: merge_request, author: user) }
+
+ it_behaves_like 'diff draft notes API', 'iid'
+
+ context "when position is for a previous commit on the merge request" do
+ it "returns a 400 bad request error because the line_code is old" do
+ # SHA taken from an earlier commit listed in spec/factories/merge_requests.rb
+ position = draft_note.position.to_h.merge(new_line: 'c1acaa58bbcbc3eafe538cb8274ba387047b69f8')
+
+ post api("/projects/#{project.id}/merge_requests/#{merge_request['iid']}/draft_notes", user),
+ params: { body: 'hi!', position: position }
+
+ expect(response).to have_gitlab_http_status(:bad_request)
+ end
+ end
+ end
+
context "when attempting to resolve a disscussion" do
context "when providing a non-existant ID" do
it "returns a 400 Bad Request" do
diff --git a/spec/support/shared_examples/requests/api/draft_notes_shared_examples.rb b/spec/support/shared_examples/requests/api/draft_notes_shared_examples.rb
new file mode 100644
index 0000000000000000000000000000000000000000..40825cdd5ed99b1e27604ec84bc7982ff3dc0756
--- /dev/null
+++ b/spec/support/shared_examples/requests/api/draft_notes_shared_examples.rb
@@ -0,0 +1,91 @@
+# frozen_string_literal: true
+
+RSpec.shared_examples 'diff draft notes API' do |id_name|
+ describe "post /projects/:id/merge_requests/:merge_request_id/draft_notes" do
+ it "creates a new diff draft note" do
+ line_range = {
+ "start" => {
+ "line_code" => Gitlab::Git.diff_line_code(draft_note.position.file_path, 1, 1),
+ "type" => draft_note.position.type
+ },
+ "end" => {
+ "line_code" => Gitlab::Git.diff_line_code(draft_note.position.file_path, 2, 2),
+ "type" => draft_note.position.type
+ }
+ }
+
+ position = draft_note.position.to_h.merge({ line_range: line_range }).except(:ignore_whitespace_change)
+
+ post api("/projects/#{project.id}/merge_requests/#{merge_request[id_name]}/draft_notes", user),
+ params: { note: 'hi!', position: position }
+
+ expect(response).to have_gitlab_http_status(:created)
+ expect(json_response['note']).to eq('hi!')
+ expect(json_response['position']).to eq(position.stringify_keys)
+ end
+
+ context "when position is invalid" do
+ it "returns a 400 bad request error when position is not plausible" do
+ position = draft_note.position.to_h.merge(new_line: '100000')
+
+ post api("/projects/#{project.id}/merge_requests/#{merge_request[id_name]}/draft_notes", user),
+ params: { body: 'hi!', position: position }
+
+ expect(response).to have_gitlab_http_status(:bad_request)
+ end
+
+ it "returns a 400 bad request error when the position is not valid for this discussion" do
+ position = draft_note.position.to_h.merge(new_line: '588440f66559714280628a4f9799f0c4eb880a4a')
+
+ post api("/projects/#{project.id}/merge_requests/#{merge_request[id_name]}/draft_notes", user),
+ params: { body: 'hi!', position: position }
+
+ expect(response).to have_gitlab_http_status(:bad_request)
+ end
+ end
+ end
+
+ describe "put /projects/:id/merge_requests/:merge_request_id/draft_notes/:draft_note_id" do
+ it "modifies a draft note" do
+ line_range = {
+ "start" => {
+ "line_code" => Gitlab::Git.diff_line_code(draft_note.position.file_path, 3, 3),
+ "type" => draft_note.position.type
+ },
+ "end" => {
+ "line_code" => Gitlab::Git.diff_line_code(draft_note.position.file_path, 4, 4),
+ "type" => draft_note.position.type
+ }
+ }
+
+ position = draft_note.position.to_h.merge({ line_range: line_range }).except(:ignore_whitespace_change)
+
+ put api("/projects/#{project.id}/merge_requests/#{merge_request[id_name]}/draft_notes/#{draft_note.id}", user),
+ params: { note: 'hola!', position: position }
+
+ expect(response).to have_gitlab_http_status(:ok)
+ expect(json_response['note']).to eq('hola!')
+ expect(json_response['position']).to eq(position.stringify_keys)
+ end
+
+ it "returns bad request for an empty note" do
+ line_range = {
+ "start" => {
+ "line_code" => Gitlab::Git.diff_line_code(draft_note.position.file_path, 3, 3),
+ "type" => draft_note.position.type
+ },
+ "end" => {
+ "line_code" => Gitlab::Git.diff_line_code(draft_note.position.file_path, 4, 4),
+ "type" => draft_note.position.type
+ }
+ }
+
+ position = draft_note.position.to_h.merge({ line_range: line_range }).except(:ignore_whitespace_change)
+
+ put api("/projects/#{project.id}/merge_requests/#{merge_request[id_name]}/draft_notes/#{draft_note.id}", user),
+ params: { note: '', position: position }
+
+ expect(response).to have_gitlab_http_status(:bad_request)
+ end
+ end
+end