Commit cbc90565 authored by Robert Schilling's avatar Robert Schilling

Do label validation for issues/merge requests API

parent 04ad197b
...@@ -140,7 +140,8 @@ module Issuable ...@@ -140,7 +140,8 @@ module Issuable
def add_labels_by_names(label_names) def add_labels_by_names(label_names)
label_names.each do |label_name| label_names.each do |label_name|
label = project.labels.find_or_create_by(title: label_name.strip) label = project.labels.create_with(
color: Label::DEFAULT_COLOR).find_or_create_by(title: label_name.strip)
self.labels << label self.labels << label
end end
end end
......
class Label < ActiveRecord::Base class Label < ActiveRecord::Base
DEFAULT_COLOR = '#82C5FF'
belongs_to :project belongs_to :project
has_many :label_links, dependent: :destroy has_many :label_links, dependent: :destroy
has_many :issues, through: :label_links, source: :target, source_type: 'Issue' has_many :issues, through: :label_links, source: :target, source_type: 'Issue'
......
...@@ -157,6 +157,9 @@ Parameters: ...@@ -157,6 +157,9 @@ Parameters:
- `milestone_id` (optional) - The ID of a milestone to assign issue - `milestone_id` (optional) - The ID of a milestone to assign issue
- `labels` (optional) - Comma-separated label names for an issue - `labels` (optional) - Comma-separated label names for an issue
If the operation is successful, 200 and the newly created issue is returned.
If an error occurs, an error number and a message explaining the reason is returned.
## Edit issue ## Edit issue
Updates an existing project issue. This function is also used to mark an issue as closed. Updates an existing project issue. This function is also used to mark an issue as closed.
...@@ -176,6 +179,9 @@ Parameters: ...@@ -176,6 +179,9 @@ Parameters:
- `labels` (optional) - Comma-separated label names for an issue - `labels` (optional) - Comma-separated label names for an issue
- `state_event` (optional) - The state event of an issue ('close' to close issue and 'reopen' to reopen it) - `state_event` (optional) - The state event of an issue ('close' to close issue and 'reopen' to reopen it)
If the operation is successful, 200 and the updated issue is returned.
If an error occurs, an error number and a message explaining the reason is returned.
## Delete existing issue (**Deprecated**) ## Delete existing issue (**Deprecated**)
The function is deprecated and returns a `405 Method Not Allowed` error if called. An issue gets now closed and is done by calling `PUT /projects/:id/issues/:issue_id` with parameter `closed` set to 1. The function is deprecated and returns a `405 Method Not Allowed` error if called. An issue gets now closed and is done by calling `PUT /projects/:id/issues/:issue_id` with parameter `closed` set to 1.
......
...@@ -136,6 +136,9 @@ Parameters: ...@@ -136,6 +136,9 @@ Parameters:
} }
``` ```
If the operation is successful, 200 and the newly created merge request is returned.
If an error occurs, an error number and a message explaining the reason is returned.
## Update MR ## Update MR
Updates an existing merge request. You can change branches, title, or even close the MR. Updates an existing merge request. You can change branches, title, or even close the MR.
...@@ -183,9 +186,12 @@ Parameters: ...@@ -183,9 +186,12 @@ Parameters:
} }
``` ```
If the operation is successful, 200 and the updated merge request is returned.
If an error occurs, an error number and a message explaining the reason is returned.
## Accept MR ## Accept MR
Merge changes submitted with MR usign this API. Merge changes submitted with MR using this API.
If merge success you get 200 OK. If merge success you get 200 OK.
......
...@@ -112,6 +112,21 @@ module API ...@@ -112,6 +112,21 @@ module API
ActionController::Parameters.new(attrs).permit! ActionController::Parameters.new(attrs).permit!
end end
# Helper method for validating all labels against its names
def validate_label_params(params)
if params[:labels].present?
params[:labels].split(',').each do |label_name|
label = user_project.labels.create_with(
color: Label::DEFAULT_COLOR).find_or_initialize_by(
title: label_name.strip)
if label.invalid?
return true
end
end
end
false
end
# error helpers # error helpers
def forbidden! def forbidden!
......
...@@ -51,12 +51,18 @@ module API ...@@ -51,12 +51,18 @@ module API
required_attributes! [:title] required_attributes! [:title]
attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id] attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id]
# Validate label names in advance
if validate_label_params(params)
return render_api_error!('Label names invalid', 405)
end
issue = ::Issues::CreateService.new(user_project, current_user, attrs).execute issue = ::Issues::CreateService.new(user_project, current_user, attrs).execute
if issue.valid? if issue.valid?
# Find or create labels and attach to issue # Find or create labels and attach to issue. Labels are valid because
# we already checked its name, so there can't be an error here
if params[:labels].present? if params[:labels].present?
issue.add_labels_by_names(params[:labels].split(",")) issue.add_labels_by_names(params[:labels].split(','))
end end
present issue, with: Entities::Issue present issue, with: Entities::Issue
...@@ -83,12 +89,19 @@ module API ...@@ -83,12 +89,19 @@ module API
authorize! :modify_issue, issue authorize! :modify_issue, issue
attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id, :state_event] attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id, :state_event]
# Validate label names in advance
if validate_label_params(params)
return render_api_error!('Label names invalid', 405)
end
issue = ::Issues::UpdateService.new(user_project, current_user, attrs).execute(issue) issue = ::Issues::UpdateService.new(user_project, current_user, attrs).execute(issue)
if issue.valid? if issue.valid?
# Find or create labels and attach to issue # Find or create labels and attach to issue. Labels are valid because
# we already checked its name, so there can't be an error here
if params[:labels].present? if params[:labels].present?
issue.add_labels_by_names(params[:labels].split(",")) # Create and add labels to the new created issue
issue.add_labels_by_names(params[:labels].split(','))
end end
present issue, with: Entities::Issue present issue, with: Entities::Issue
......
...@@ -76,6 +76,12 @@ module API ...@@ -76,6 +76,12 @@ module API
authorize! :write_merge_request, user_project authorize! :write_merge_request, user_project
required_attributes! [:source_branch, :target_branch, :title] required_attributes! [:source_branch, :target_branch, :title]
attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :target_project_id, :description] attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :target_project_id, :description]
# Validate label names in advance
if validate_label_params(params)
return render_api_error!('Label names invalid', 405)
end
merge_request = ::MergeRequests::CreateService.new(user_project, current_user, attrs).execute merge_request = ::MergeRequests::CreateService.new(user_project, current_user, attrs).execute
if merge_request.valid? if merge_request.valid?
...@@ -109,6 +115,12 @@ module API ...@@ -109,6 +115,12 @@ module API
attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :state_event, :description] attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :state_event, :description]
merge_request = user_project.merge_requests.find(params[:merge_request_id]) merge_request = user_project.merge_requests.find(params[:merge_request_id])
authorize! :modify_merge_request, merge_request authorize! :modify_merge_request, merge_request
# Validate label names in advance
if validate_label_params(params)
return render_api_error!('Label names invalid', 405)
end
merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, attrs).execute(merge_request) merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, attrs).execute(merge_request)
if merge_request.valid? if merge_request.valid?
......
...@@ -5,6 +5,10 @@ describe API::API, api: true do ...@@ -5,6 +5,10 @@ describe API::API, api: true do
let(:user) { create(:user) } let(:user) { create(:user) }
let!(:project) { create(:project, namespace: user.namespace ) } let!(:project) { create(:project, namespace: user.namespace ) }
let!(:issue) { create(:issue, author: user, assignee: user, project: project) } let!(:issue) { create(:issue, author: user, assignee: user, project: project) }
let!(:label) do
create(:label, title: 'label', color: '#FFAABB', project: project)
end
before { project.team << [user, :reporter] } before { project.team << [user, :reporter] }
describe "GET /issues" do describe "GET /issues" do
...@@ -68,6 +72,14 @@ describe API::API, api: true do ...@@ -68,6 +72,14 @@ describe API::API, api: true do
post api("/projects/#{project.id}/issues", user), labels: 'label, label2' post api("/projects/#{project.id}/issues", user), labels: 'label, label2'
response.status.should == 400 response.status.should == 400
end end
it 'should return 405 on invalid label names' do
post api("/projects/#{project.id}/issues", user),
title: 'new issue',
labels: 'label, ?'
response.status.should == 405
json_response['message'].should == 'Label names invalid'
end
end end
describe "PUT /projects/:id/issues/:issue_id to update only title" do describe "PUT /projects/:id/issues/:issue_id to update only title" do
...@@ -84,6 +96,14 @@ describe API::API, api: true do ...@@ -84,6 +96,14 @@ describe API::API, api: true do
title: 'updated title' title: 'updated title'
response.status.should == 404 response.status.should == 404
end end
it 'should return 405 on invalid label names' do
put api("/projects/#{project.id}/issues/#{issue.id}", user),
title: 'updated title',
labels: 'label, ?'
response.status.should == 405
json_response['message'].should == 'Label names invalid'
end
end end
describe "PUT /projects/:id/issues/:issue_id to update state and label" do describe "PUT /projects/:id/issues/:issue_id to update state and label" do
......
...@@ -78,9 +78,14 @@ describe API::API, api: true do ...@@ -78,9 +78,14 @@ describe API::API, api: true do
context 'between branches projects' do context 'between branches projects' do
it "should return merge_request" do it "should return merge_request" do
post api("/projects/#{project.id}/merge_requests", user), post api("/projects/#{project.id}/merge_requests", user),
title: 'Test merge_request', source_branch: "stable", target_branch: "master", author: user title: 'Test merge_request',
source_branch: 'stable',
target_branch: 'master',
author: user,
labels: 'label, label2'
response.status.should == 201 response.status.should == 201
json_response['title'].should == 'Test merge_request' json_response['title'].should == 'Test merge_request'
json_response['labels'].should == ['label', 'label2']
end end
it "should return 422 when source_branch equals target_branch" do it "should return 422 when source_branch equals target_branch" do
...@@ -106,6 +111,17 @@ describe API::API, api: true do ...@@ -106,6 +111,17 @@ describe API::API, api: true do
target_branch: 'master', source_branch: 'stable' target_branch: 'master', source_branch: 'stable'
response.status.should == 400 response.status.should == 400
end end
it 'should return 405 on invalid label names' do
post api("/projects/#{project.id}/merge_requests", user),
title: 'Test merge_request',
source_branch: 'stable',
target_branch: 'master',
author: user,
labels: 'label, ?'
response.status.should == 405
json_response['message'].should == 'Label names invalid'
end
end end
context 'forked projects' do context 'forked projects' do
...@@ -235,6 +251,15 @@ describe API::API, api: true do ...@@ -235,6 +251,15 @@ describe API::API, api: true do
response.status.should == 200 response.status.should == 200
json_response['target_branch'].should == 'wiki' json_response['target_branch'].should == 'wiki'
end end
it 'should return 405 on invalid label names' do
put api("/projects/#{project.id}/merge_request/#{merge_request.id}",
user),
title: 'new issue',
labels: 'label, ?'
response.status.should == 405
json_response['message'].should == 'Label names invalid'
end
end end
describe "POST /projects/:id/merge_request/:merge_request_id/comments" do describe "POST /projects/:id/merge_request/:merge_request_id/comments" do
......
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