diff options
author | Jacob Vosmaer <contact@jacobvosmaer.nl> | 2014-08-20 15:09:31 +0200 |
---|---|---|
committer | Jacob Vosmaer <contact@jacobvosmaer.nl> | 2014-08-20 15:09:31 +0200 |
commit | 39464857651a0f7dc0e4a3ad89e766ea200c36ba (patch) | |
tree | 1d598967c659c4670ecd61674834aa17b4da9168 | |
parent | f5b23bc2caa8ece27abe4278f6a1b779442f85b4 (diff) | |
parent | 9d5c69019371379d055be8ba963fde78d3e0c7f6 (diff) | |
download | gitlab-ce-39464857651a0f7dc0e4a3ad89e766ea200c36ba.tar.gz |
Merge pull request #7526 from jubianchi/api/issue-api-validate-labels
API: Labels validation in issues API
-rw-r--r-- | app/models/concerns/issuable.rb | 4 | ||||
-rw-r--r-- | app/models/label.rb | 4 | ||||
-rw-r--r-- | lib/api/helpers.rb | 8 | ||||
-rw-r--r-- | lib/api/issues.rb | 11 | ||||
-rw-r--r-- | spec/models/label_spec.rb | 23 | ||||
-rw-r--r-- | spec/requests/api/issues_spec.rb | 56 | ||||
-rw-r--r-- | spec/requests/api/labels_spec.rb | 16 |
7 files changed, 107 insertions, 15 deletions
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 0a5fe24b5af..5c9b44812be 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -138,6 +138,10 @@ module Issuable labels.order('title ASC').pluck(:title) end + def remove_labels + labels.delete_all + end + def add_labels_by_names(label_names) label_names.each do |label_name| label = project.labels.create_with( diff --git a/app/models/label.rb b/app/models/label.rb index 3ff52416c24..233f477444f 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -6,14 +6,14 @@ class Label < ActiveRecord::Base has_many :issues, through: :label_links, source: :target, source_type: 'Issue' validates :color, - format: { with: /\A\#[0-9A-Fa-f]{6}+\Z/ }, + format: { with: /\A#[0-9A-Fa-f]{6}\Z/ }, allow_blank: false validates :project, presence: true # Don't allow '?', '&', and ',' for label titles validates :title, presence: true, - format: { with: /\A[^&\?,&]*\z/ }, + format: { with: /\A[^&\?,&]+\z/ }, uniqueness: { scope: :project_id } scope :order_by_name, -> { reorder("labels.title ASC") } diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index d36b29a00b1..6af0f6d1b25 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -114,17 +114,21 @@ module API # Helper method for validating all labels against its names def validate_label_params(params) + errors = {} + 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 + errors[label.title] = label.errors end end end - false + + errors end # error helpers diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 055529ccbd8..eb6a74cd2bc 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -52,8 +52,8 @@ module API 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) + if (errors = validate_label_params(params)).any? + render_api_error!({ labels: errors }, 400) end issue = ::Issues::CreateService.new(user_project, current_user, attrs).execute @@ -90,8 +90,8 @@ module API 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) + if (errors = validate_label_params(params)).any? + render_api_error!({ labels: errors }, 400) end issue = ::Issues::UpdateService.new(user_project, current_user, attrs).execute(issue) @@ -99,7 +99,8 @@ module API if issue.valid? # 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? + unless params[:labels].nil? + issue.remove_labels # Create and add labels to the new created issue issue.add_labels_by_names(params[:labels].split(',')) end diff --git a/spec/models/label_spec.rb b/spec/models/label_spec.rb index 5098af84cc3..1d273e59bde 100644 --- a/spec/models/label_spec.rb +++ b/spec/models/label_spec.rb @@ -5,4 +5,27 @@ describe Label do it { label.should be_valid } it { should belong_to(:project) } + + describe 'Validation' do + it 'should validate color code' do + build(:label, color: 'G-ITLAB').should_not be_valid + build(:label, color: 'AABBCC').should_not be_valid + build(:label, color: '#AABBCCEE').should_not be_valid + build(:label, color: '#GGHHII').should_not be_valid + build(:label, color: '#').should_not be_valid + build(:label, color: '').should_not be_valid + + build(:label, color: '#AABBCC').should be_valid + end + + it 'should validate title' do + build(:label, title: 'G,ITLAB').should_not be_valid + build(:label, title: 'G?ITLAB').should_not be_valid + build(:label, title: 'G&ITLAB').should_not be_valid + build(:label, title: '').should_not be_valid + + build(:label, title: 'GITLAB').should be_valid + build(:label, title: 'gitlab').should be_valid + end + end end diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index d8e8e4f5035..08dc94ebdf3 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -73,12 +73,12 @@ describe API::API, api: true do response.status.should == 400 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), title: 'new issue', labels: 'label, ?' - response.status.should == 405 - json_response['message'].should == 'Label names invalid' + response.status.should == 400 + json_response['message']['labels']['?']['title'].should == ['is invalid'] end end @@ -97,12 +97,56 @@ describe API::API, api: true do response.status.should == 404 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), title: 'updated title', labels: 'label, ?' - response.status.should == 405 - json_response['message'].should == 'Label names invalid' + response.status.should == 400 + 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 diff --git a/spec/requests/api/labels_spec.rb b/spec/requests/api/labels_spec.rb index cd1b84c53c9..ee9088933a1 100644 --- a/spec/requests/api/labels_spec.rb +++ b/spec/requests/api/labels_spec.rb @@ -50,6 +50,14 @@ describe API::API, api: true do json_response['message'].should == 'Color is invalid' 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 post api("/projects/#{project.id}/labels", user), name: '?', @@ -147,5 +155,13 @@ describe API::API, api: true do response.status.should == 400 json_response['message'].should == 'Color is invalid' 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 |