Commit 7ad93ab2 authored by jubianchi's avatar jubianchi

Improve labels validation and expose error messages

parent ed9e922d
...@@ -138,6 +138,10 @@ module Issuable ...@@ -138,6 +138,10 @@ module Issuable
labels.order('title ASC').pluck(:title) labels.order('title ASC').pluck(:title)
end end
def remove_labels
labels.delete_all
end
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.create_with( label = project.labels.create_with(
......
...@@ -6,14 +6,14 @@ class Label < ActiveRecord::Base ...@@ -6,14 +6,14 @@ class Label < ActiveRecord::Base
has_many :issues, through: :label_links, source: :target, source_type: 'Issue' has_many :issues, through: :label_links, source: :target, source_type: 'Issue'
validates :color, validates :color,
format: { with: /\A\#[0-9A-Fa-f]{6}+\Z/ }, format: { with: /\A#[0-9A-Fa-f]{6}\Z/ },
allow_blank: false allow_blank: false
validates :project, presence: true validates :project, presence: true
# Don't allow '?', '&', and ',' for label titles # Don't allow '?', '&', and ',' for label titles
validates :title, validates :title,
presence: true, presence: true,
format: { with: /\A[^&\?,&]*\z/ }, format: { with: /\A[^&\?,&]+\z/ },
uniqueness: { scope: :project_id } uniqueness: { scope: :project_id }
scope :order_by_name, -> { reorder("labels.title ASC") } scope :order_by_name, -> { reorder("labels.title ASC") }
......
...@@ -114,17 +114,21 @@ module API ...@@ -114,17 +114,21 @@ module API
# Helper method for validating all labels against its names # Helper method for validating all labels against its names
def validate_label_params(params) def validate_label_params(params)
errors = {}
if params[:labels].present? if params[:labels].present?
params[:labels].split(',').each do |label_name| params[:labels].split(',').each do |label_name|
label = user_project.labels.create_with( label = user_project.labels.create_with(
color: Label::DEFAULT_COLOR).find_or_initialize_by( color: Label::DEFAULT_COLOR).find_or_initialize_by(
title: label_name.strip) title: label_name.strip)
if label.invalid? if label.invalid?
return true errors[label.title] = label.errors
end end
end end
end end
false
errors
end end
# error helpers # error helpers
......
...@@ -52,8 +52,8 @@ module API ...@@ -52,8 +52,8 @@ module API
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 # Validate label names in advance
if validate_label_params(params) if (errors = validate_label_params(params)).any?
return render_api_error!('Label names invalid', 405) render_api_error!({ labels: errors }, 400)
end end
issue = ::Issues::CreateService.new(user_project, current_user, attrs).execute issue = ::Issues::CreateService.new(user_project, current_user, attrs).execute
...@@ -90,8 +90,8 @@ module API ...@@ -90,8 +90,8 @@ module API
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 # Validate label names in advance
if validate_label_params(params) if (errors = validate_label_params(params)).any?
return render_api_error!('Label names invalid', 405) render_api_error!({ labels: errors }, 400)
end end
issue = ::Issues::UpdateService.new(user_project, current_user, attrs).execute(issue) issue = ::Issues::UpdateService.new(user_project, current_user, attrs).execute(issue)
...@@ -99,7 +99,8 @@ module API ...@@ -99,7 +99,8 @@ module API
if issue.valid? if issue.valid?
# Find or create labels and attach to issue. Labels are valid because # 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 # we already checked its name, so there can't be an error here
if params[:labels].present? unless params[:labels].nil?
issue.remove_labels
# Create and add labels to the new created issue # Create and add labels to the new created issue
issue.add_labels_by_names(params[:labels].split(',')) issue.add_labels_by_names(params[:labels].split(','))
end end
......
...@@ -73,12 +73,12 @@ describe API::API, api: true do ...@@ -73,12 +73,12 @@ describe API::API, api: true do
response.status.should == 400 response.status.should == 400
end end
it 'should return 405 on invalid label names' do it 'should return 400 on invalid label names' do
post api("/projects/#{project.id}/issues", user), post api("/projects/#{project.id}/issues", user),
title: 'new issue', title: 'new issue',
labels: 'label, ?' labels: 'label, ?'
response.status.should == 405 response.status.should == 400
json_response['message'].should == 'Label names invalid' json_response['message']['labels']['?']['title'].should == ['is invalid']
end end
end end
...@@ -97,12 +97,56 @@ describe API::API, api: true do ...@@ -97,12 +97,56 @@ describe API::API, api: true do
response.status.should == 404 response.status.should == 404
end end
it 'should return 405 on invalid label names' do it 'should return 400 on invalid label names' do
put api("/projects/#{project.id}/issues/#{issue.id}", user), put api("/projects/#{project.id}/issues/#{issue.id}", user),
title: 'updated title', title: 'updated title',
labels: 'label, ?' labels: 'label, ?'
response.status.should == 405 response.status.should == 400
json_response['message'].should == 'Label names invalid' json_response['message']['labels']['?']['title'].should == ['is invalid']
end
end
describe 'PUT /projects/:id/issues/:issue_id to update labels' do
let!(:label) { create(:label, title: 'dummy', project: project) }
let!(:label_link) { create(:label_link, label: label, target: issue) }
it 'should not update labels if not present' do
put api("/projects/#{project.id}/issues/#{issue.id}", user),
title: 'updated title'
response.status.should == 200
json_response['labels'].should == [label.title]
end
it 'should remove all labels' do
put api("/projects/#{project.id}/issues/#{issue.id}", user),
labels: ''
response.status.should == 200
json_response['labels'].should == []
end
it 'should update labels' do
put api("/projects/#{project.id}/issues/#{issue.id}", user),
labels: 'foo,bar'
response.status.should == 200
json_response['labels'].should include 'foo'
json_response['labels'].should include 'bar'
end
it 'should return 400 on invalid label names' do
put api("/projects/#{project.id}/issues/#{issue.id}", user),
labels: 'label, ?'
response.status.should == 400
json_response['message']['labels']['?']['title'].should == ['is invalid']
end
it 'should allow special label names' do
put api("/projects/#{project.id}/issues/#{issue.id}", user),
labels: 'label:foo, label-bar,label_bar,label/bar'
response.status.should == 200
json_response['labels'].should include 'label:foo'
json_response['labels'].should include 'label-bar'
json_response['labels'].should include 'label_bar'
json_response['labels'].should include 'label/bar'
end end
end end
......
...@@ -50,6 +50,14 @@ describe API::API, api: true do ...@@ -50,6 +50,14 @@ describe API::API, api: true do
json_response['message'].should == 'Color is invalid' json_response['message'].should == 'Color is invalid'
end end
it 'should return 400 for too long color code' do
post api("/projects/#{project.id}/labels", user),
name: 'Foo',
color: '#FFAAFFFF'
response.status.should == 400
json_response['message'].should == 'Color is invalid'
end
it 'should return 400 for invalid name' do it 'should return 400 for invalid name' do
post api("/projects/#{project.id}/labels", user), post api("/projects/#{project.id}/labels", user),
name: '?', name: '?',
...@@ -147,5 +155,13 @@ describe API::API, api: true do ...@@ -147,5 +155,13 @@ describe API::API, api: true do
response.status.should == 400 response.status.should == 400
json_response['message'].should == 'Color is invalid' json_response['message'].should == 'Color is invalid'
end end
it 'should return 400 for too long color code' do
post api("/projects/#{project.id}/labels", user),
name: 'Foo',
color: '#FFAAFFFF'
response.status.should == 400
json_response['message'].should == 'Color is invalid'
end
end end
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