Skip to content

G
gitlab-ce
Commit ef42af9a authored by Dmitriy Zaporozhets's avatar Dmitriy Zaporozhets
Browse files
Options

Merge branch 'improve-mr-api' into 'master' 

Improve consistency and duplication for Merge Request API

* Follow REST for merge request API route
* Remove repeating comments API for MR
Signed-off-by: default avatarDmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

Fixes #4759 and #11810 

See merge request !2639
parents 695fb209 dfb8803c
Show whitespace changes
Inline Side-by-side
Showing with 221 additions and 268 deletions
CHANGELOG
View file @ ef42af9a
......@@ -14,6 +14,8 @@ v 8.5.0 (unreleased)
- Track project import failure
- Fix visibility level text in admin area (Zeger-Jan van de Weg)
- Update the ExternalIssue regex pattern (Blake Hitchcock)
- Deprecate API "merge_request/:merge_request_id/comments". Use "merge_requests/:merge_request_id/notes" instead
- Deprecate API "merge_request/:merge_request_id/...". Use "merge_requests/:merge_request_id/..." instead
v 8.4.2
- Bump required gitlab-workhorse version to bring in a fix for missing
......
doc/api/merge_requests.md
View file @ ef42af9a
......@@ -60,7 +60,7 @@ Parameters:
Shows information about a single merge request.
```
GET /projects/:id/merge_request/:merge_request_id
GET /projects/:id/merge_requests/:merge_request_id
```
Parameters:
......@@ -105,7 +105,7 @@ Parameters:
Get a list of merge request commits.
```
GET /projects/:id/merge_request/:merge_request_id/commits
GET /projects/:id/merge_requests/:merge_request_id/commits
```
Parameters:
......@@ -142,7 +142,7 @@ Parameters:
Shows information about the merge request including its files and changes.
```
GET /projects/:id/merge_request/:merge_request_id/changes
GET /projects/:id/merge_requests/:merge_request_id/changes
```
Parameters:
......@@ -264,7 +264,7 @@ If an error occurs, an error number and a message explaining the reason is retur
Updates an existing merge request. You can change the target branch, title, or even close the MR.
```
PUT /projects/:id/merge_request/:merge_request_id
PUT /projects/:id/merge_requests/:merge_request_id
```
Parameters:
......@@ -323,7 +323,7 @@ If merge request is already merged or closed - you get 405 and error message 'Me
If you don't have permissions to accept this merge request - you'll get a 401
```
PUT /projects/:id/merge_request/:merge_request_id/merge
PUT /projects/:id/merge_requests/:merge_request_id/merge
```
Parameters:
......@@ -373,7 +373,7 @@ If the merge request is already merged or closed - you get 405 and error message
In case the merge request is not set to be merged when the build succeeds, you'll also get a 406 error.
```
PUT /projects/:id/merge_request/:merge_request_id/cancel_merge_when_build_succeeds
PUT /projects/:id/merge_requests/:merge_request_id/cancel_merge_when_build_succeeds
```
Parameters:
......@@ -409,66 +409,6 @@ Parameters:
}
```
## Post comment to MR
Adds a comment to a merge request.
```
POST /projects/:id/merge_request/:merge_request_id/comments
```
Parameters:
- `id` (required) - The ID of a project
- `merge_request_id` (required) - ID of merge request
- `note` (required) - Text of comment
```json
{
"note": "text1"
}
```
## Get the comments on a MR
Gets all the comments associated with a merge request.
```
GET /projects/:id/merge_request/:merge_request_id/comments
```
Parameters:
- `id` (required) - The ID of a project
- `merge_request_id` (required) - ID of merge request
```json
[
{
"note": "this is the 1st comment on the 2merge merge request",
"author": {
"id": 11,
"username": "admin",
"email": "admin@example.com",
"name": "Administrator",
"state": "active",
"created_at": "2014-03-06T08:17:35.000Z"
}
},
{
"note": "Status changed to closed",
"author": {
"id": 11,
"username": "admin",
"email": "admin@example.com",
"name": "Administrator",
"state": "active",
"created_at": "2014-03-06T08:17:35.000Z"
}
}
]
```
## Comments on merge requets
Comments are done via the notes resource.
Comments are done via the [notes](notes.md) resource.
lib/api/merge_requests.rb
View file @ ef42af9a
......@@ -59,6 +59,50 @@ module API
present paginate(merge_requests), with: Entities::MergeRequest
end
# Create MR
#
# Parameters:
#
# id (required) - The ID of a project - this will be the source of the merge request
# source_branch (required) - The source branch
# target_branch (required) - The target branch
# target_project_id - The target project of the merge request defaults to the :id of the project
# assignee_id - Assignee user ID
# title (required) - Title of MR
# description - Description of MR
# labels (optional) - Labels for MR as a comma-separated list
#
# Example:
# POST /projects/:id/merge_requests
#
post ":id/merge_requests" do
authorize! :create_merge_request, user_project
required_attributes! [:source_branch, :target_branch, :title]
attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :target_project_id, :description]
# Validate label names in advance
if (errors = validate_label_params(params)).any?
render_api_error!({ labels: errors }, 400)
end
merge_request = ::MergeRequests::CreateService.new(user_project, current_user, attrs).execute
if merge_request.valid?
# Find or create labels and attach to issue
if params[:labels].present?
merge_request.add_labels_by_names(params[:labels].split(","))
end
present merge_request, with: Entities::MergeRequest
else
handle_merge_request_errors! merge_request.errors
end
end
# Routing "merge_request/:merge_request_id/..." is DEPRECATED and WILL BE REMOVED in version 9.0
# Use "merge_requests/:merge_request_id/..." instead.
#
[":id/merge_request/:merge_request_id", ":id/merge_requests/:merge_request_id"].each do |path|
# Show MR
#
# Parameters:
......@@ -66,9 +110,9 @@ module API
# merge_request_id (required) - The ID of MR
#
# Example:
# GET /projects/:id/merge_request/:merge_request_id
# GET /projects/:id/merge_requests/:merge_request_id
#
get ":id/merge_request/:merge_request_id" do
get path do
merge_request = user_project.merge_requests.find(params[:merge_request_id])
authorize! :read_merge_request, merge_request
......@@ -83,9 +127,9 @@ module API
# merge_request_id (required) - The ID of MR
#
# Example:
# GET /projects/:id/merge_request/:merge_request_id/commits
# GET /projects/:id/merge_requests/:merge_request_id/commits
#
get ':id/merge_request/:merge_request_id/commits' do
get "#{path}/commits" do
merge_request = user_project.merge_requests.
find(params[:merge_request_id])
authorize! :read_merge_request, merge_request
......@@ -99,55 +143,15 @@ module API
# merge_request_id (required) - The ID of MR
#
# Example:
# GET /projects/:id/merge_request/:merge_request_id/changes
# GET /projects/:id/merge_requests/:merge_request_id/changes
#
get ':id/merge_request/:merge_request_id/changes' do
get "#{path}/changes" do
merge_request = user_project.merge_requests.
find(params[:merge_request_id])
authorize! :read_merge_request, merge_request
present merge_request, with: Entities::MergeRequestChanges
end
# Create MR
#
# Parameters:
#
# id (required) - The ID of a project - this will be the source of the merge request
# source_branch (required) - The source branch
# target_branch (required) - The target branch
# target_project_id - The target project of the merge request defaults to the :id of the project
# assignee_id - Assignee user ID
# title (required) - Title of MR
# description - Description of MR
# labels (optional) - Labels for MR as a comma-separated list
#
# Example:
# POST /projects/:id/merge_requests
#
post ":id/merge_requests" do
authorize! :create_merge_request, user_project
required_attributes! [:source_branch, :target_branch, :title]
attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :target_project_id, :description]
# Validate label names in advance
if (errors = validate_label_params(params)).any?
render_api_error!({ labels: errors }, 400)
end
merge_request = ::MergeRequests::CreateService.new(user_project, current_user, attrs).execute
if merge_request.valid?
# Find or create labels and attach to issue
if params[:labels].present?
merge_request.add_labels_by_names(params[:labels].split(","))
end
present merge_request, with: Entities::MergeRequest
else
handle_merge_request_errors! merge_request.errors
end
end
# Update MR
#
# Parameters:
......@@ -160,9 +164,9 @@ module API
# description - Description of MR
# labels (optional) - Labels for a MR as a comma-separated list
# Example:
# PUT /projects/:id/merge_request/:merge_request_id
# PUT /projects/:id/merge_requests/:merge_request_id
#
put ":id/merge_request/:merge_request_id" do
put path do
attrs = attributes_for_keys [:target_branch, :assignee_id, :title, :state_event, :description]
merge_request = user_project.merge_requests.find(params[:merge_request_id])
authorize! :update_merge_request, merge_request
......@@ -201,9 +205,9 @@ module API
# should_remove_source_branch (optional) - When true, the source branch will be deleted if possible
# merge_when_build_succeeds (optional) - When true, this MR will be merged when the build succeeds
# Example:
# PUT /projects/:id/merge_request/:merge_request_id/merge
# PUT /projects/:id/merge_requests/:merge_request_id/merge
#
put ":id/merge_request/:merge_request_id/merge" do
put "#{path}/merge" do
merge_request = user_project.merge_requests.find(params[:merge_request_id])
# Merge request can not be merged
......@@ -236,7 +240,7 @@ module API
# id (required) - The ID of a project
# merge_request_id (required) - ID of MR
#
post ":id/merge_request/:merge_request_id/cancel_merge_when_build_succeeds" do
post "#{path}/cancel_merge_when_build_succeeds" do
merge_request = user_project.merge_requests.find(params[:merge_request_id])
unauthorized! unless merge_request.can_cancel_merge_when_build_succeeds?(current_user)
......@@ -244,15 +248,18 @@ module API
::MergeRequest::MergeWhenBuildSucceedsService.new(merge_request.target_project, current_user).cancel(merge_request)
end
# Duplicate. DEPRECATED and WILL BE REMOVED in 9.0.
# Use GET "/projects/:id/merge_requests/:merge_request_id/notes" instead
#
# Get a merge request's comments
#
# Parameters:
# id (required) - The ID of a project
# merge_request_id (required) - ID of MR
# Examples:
# GET /projects/:id/merge_request/:merge_request_id/comments
# GET /projects/:id/merge_requests/:merge_request_id/comments
#
get ":id/merge_request/:merge_request_id/comments" do
get "#{path}/comments" do
merge_request = user_project.merge_requests.find(params[:merge_request_id])
authorize! :read_merge_request, merge_request
......@@ -260,6 +267,9 @@ module API
present paginate(merge_request.notes.fresh), with: Entities::MRNote
end
# Duplicate. DEPRECATED and WILL BE REMOVED in 9.0.
# Use POST "/projects/:id/merge_requests/:merge_request_id/notes" instead
#
# Post comment to merge request
#
# Parameters:
......@@ -267,9 +277,9 @@ module API
# merge_request_id (required) - ID of MR
# note (required) - Text of comment
# Examples:
# POST /projects/:id/merge_request/:merge_request_id/comments
# POST /projects/:id/merge_requests/:merge_request_id/comments
#
post ":id/merge_request/:merge_request_id/comments" do
post "#{path}/comments" do
required_attributes! [:note]
merge_request = user_project.merge_requests.find(params[:merge_request_id])
......@@ -292,4 +302,5 @@ module API
end
end
end
end
end
spec/requests/api/merge_requests_spec.rb
View file @ ef42af9a
......@@ -109,9 +109,9 @@ describe API::API, api: true do
end
end
describe "GET /projects/:id/merge_request/:merge_request_id" do
describe "GET /projects/:id/merge_requests/:merge_request_id" do
it "should return merge_request" do
get api("/projects/#{project.id}/merge_request/#{merge_request.id}", user)
get api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user)
expect(response.status).to eq(200)
expect(json_response['title']).to eq(merge_request.title)
expect(json_response['iid']).to eq(merge_request.iid)
......@@ -126,14 +126,14 @@ describe API::API, api: true do
end
it "should return a 404 error if merge_request_id not found" do
get api("/projects/#{project.id}/merge_request/999", user)
get api("/projects/#{project.id}/merge_requests/999", user)
expect(response.status).to eq(404)
end
end
describe 'GET /projects/:id/merge_request/:merge_request_id/commits' do
describe 'GET /projects/:id/merge_requests/:merge_request_id/commits' do
context 'valid merge request' do
before { get api("/projects/#{project.id}/merge_request/#{merge_request.id}/commits", user) }
before { get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/commits", user) }
let(:commit) { merge_request.commits.first }
it { expect(response.status).to eq 200 }
......@@ -143,20 +143,20 @@ describe API::API, api: true do
end
it 'returns a 404 when merge_request_id not found' do
get api("/projects/#{project.id}/merge_request/999/commits", user)
get api("/projects/#{project.id}/merge_requests/999/commits", user)
expect(response.status).to eq(404)
end
end
describe 'GET /projects/:id/merge_request/:merge_request_id/changes' do
describe 'GET /projects/:id/merge_requests/:merge_request_id/changes' do
it 'should return the change information of the merge_request' do
get api("/projects/#{project.id}/merge_request/#{merge_request.id}/changes", user)
get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/changes", user)
expect(response.status).to eq 200
expect(json_response['changes'].size).to eq(merge_request.diffs.size)
end
it 'returns a 404 when merge_request_id not found' do
get api("/projects/#{project.id}/merge_request/999/changes", user)
get api("/projects/#{project.id}/merge_requests/999/changes", user)
expect(response.status).to eq(404)
end
end
......@@ -311,19 +311,19 @@ describe API::API, api: true do
end
end
describe "PUT /projects/:id/merge_request/:merge_request_id to close MR" do
describe "PUT /projects/:id/merge_requests/:merge_request_id to close MR" do
it "should return merge_request" do
put api("/projects/#{project.id}/merge_request/#{merge_request.id}", user), state_event: "close"
put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user), state_event: "close"
expect(response.status).to eq(200)
expect(json_response['state']).to eq('closed')
end
end
describe "PUT /projects/:id/merge_request/:merge_request_id/merge" do
describe "PUT /projects/:id/merge_requests/:merge_request_id/merge" do
let(:ci_commit) { create(:ci_commit_without_jobs) }
it "should return merge_request in case of success" do
put api("/projects/#{project.id}/merge_request/#{merge_request.id}/merge", user)
put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user)
expect(response.status).to eq(200)
end
......@@ -332,7 +332,7 @@ describe API::API, api: true do
allow_any_instance_of(MergeRequest).
to receive(:can_be_merged?).and_return(false)
put api("/projects/#{project.id}/merge_request/#{merge_request.id}/merge", user)
put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user)
expect(response.status).to eq(406)
expect(json_response['message']).to eq('Branch cannot be merged')
......@@ -340,14 +340,14 @@ describe API::API, api: true do
it "should return 405 if merge_request is not open" do
merge_request.close
put api("/projects/#{project.id}/merge_request/#{merge_request.id}/merge", user)
put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user)
expect(response.status).to eq(405)
expect(json_response['message']).to eq('405 Method Not Allowed')
end
it "should return 405 if merge_request is a work in progress" do
merge_request.update_attribute(:title, "WIP: #{merge_request.title}")
put api("/projects/#{project.id}/merge_request/#{merge_request.id}/merge", user)
put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user)
expect(response.status).to eq(405)
expect(json_response['message']).to eq('405 Method Not Allowed')
end
......@@ -355,7 +355,7 @@ describe API::API, api: true do
it "should return 401 if user has no permissions to merge" do
user2 = create(:user)
project.team << [user2, :reporter]
put api("/projects/#{project.id}/merge_request/#{merge_request.id}/merge", user2)
put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user2)
expect(response.status).to eq(401)
expect(json_response['message']).to eq('401 Unauthorized')
end
......@@ -364,7 +364,7 @@ describe API::API, api: true do
allow_any_instance_of(MergeRequest).to receive(:ci_commit).and_return(ci_commit)
allow(ci_commit).to receive(:active?).and_return(true)
put api("/projects/#{project.id}/merge_request/#{merge_request.id}/merge", user), merge_when_build_succeeds: true
put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user), merge_when_build_succeeds: true
expect(response.status).to eq(200)
expect(json_response['title']).to eq('Test')
......@@ -372,33 +372,33 @@ describe API::API, api: true do
end
end
describe "PUT /projects/:id/merge_request/:merge_request_id" do
describe "PUT /projects/:id/merge_requests/:merge_request_id" do
it "should return merge_request" do
put api("/projects/#{project.id}/merge_request/#{merge_request.id}", user), title: "New title"
put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user), title: "New title"
expect(response.status).to eq(200)
expect(json_response['title']).to eq('New title')
end
it "should return merge_request" do
put api("/projects/#{project.id}/merge_request/#{merge_request.id}", user), description: "New description"
put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user), description: "New description"
expect(response.status).to eq(200)
expect(json_response['description']).to eq('New description')
end
it "should return 400 when source_branch is specified" do
put api("/projects/#{project.id}/merge_request/#{merge_request.id}", user),
put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user),
source_branch: "master", target_branch: "master"
expect(response.status).to eq(400)
end
it "should return merge_request with renamed target_branch" do
put api("/projects/#{project.id}/merge_request/#{merge_request.id}", user), target_branch: "wiki"
put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user), target_branch: "wiki"
expect(response.status).to eq(200)
expect(json_response['target_branch']).to eq('wiki')
end
it 'should return 400 on invalid label names' do
put api("/projects/#{project.id}/merge_request/#{merge_request.id}",
put api("/projects/#{project.id}/merge_requests/#{merge_request.id}",
user),
title: 'new issue',
labels: 'label, ?'
......@@ -407,11 +407,11 @@ describe API::API, api: true do
end
end
describe "POST /projects/:id/merge_request/:merge_request_id/comments" do
describe "POST /projects/:id/merge_requests/:merge_request_id/comments" do
it "should return comment" do
original_count = merge_request.notes.size
post api("/projects/#{project.id}/merge_request/#{merge_request.id}/comments", user), note: "My comment"
post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/comments", user), note: "My comment"
expect(response.status).to eq(201)
expect(json_response['note']).to eq('My comment')
expect(json_response['author']['name']).to eq(user.name)
......@@ -420,20 +420,20 @@ describe API::API, api: true do
end
it "should return 400 if note is missing" do
post api("/projects/#{project.id}/merge_request/#{merge_request.id}/comments", user)
post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/comments", user)
expect(response.status).to eq(400)
end
it "should return 404 if note is attached to non existent merge request" do
post api("/projects/#{project.id}/merge_request/404/comments", user),
post api("/projects/#{project.id}/merge_requests/404/comments", user),
note: 'My comment'
expect(response.status).to eq(404)
end
end
describe "GET :id/merge_request/:merge_request_id/comments" do
describe "GET :id/merge_requests/:merge_request_id/comments" do
it "should return merge_request comments ordered by created_at" do
get api("/projects/#{project.id}/merge_request/#{merge_request.id}/comments", user)
get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/comments", user)
expect(response.status).to eq(200)
expect(json_response).to be_an Array
expect(json_response.length).to eq(2)
......@@ -443,7 +443,7 @@ describe API::API, api: true do
end
it "should return a 404 error if merge_request_id not found" do
get api("/projects/#{project.id}/merge_request/999/comments", user)
get api("/projects/#{project.id}/merge_requests/999/comments", user)
expect(response.status).to eq(404)
end
end
......
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
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7