Merge pull request #7465 from Razer6/better_label_color_validation

Better label color validation, fixes #7454

app/assets/javascripts/application.js.coffee

@@ -53,15 +53,40 @@ window.split = (val) ->
 window.extractLast = (term) ->
   return split( term ).pop()
 
+window.rstrip = (val) ->
+  return val.replace(/\s+$/, '')
+
 # Disable button if text field is empty
 window.disableButtonIfEmptyField = (field_selector, button_selector) ->
   field = $(field_selector)
-  closest_submit = field.closest("form").find(button_selector)
+  closest_submit = field.closest('form').find(button_selector)
+
+  closest_submit.disable() if rstrip(field.val()) is ""
+
+  field.on 'input', ->
+    if rstrip($(@).val()) is ""
+      closest_submit.disable()
+    else
+      closest_submit.enable()
+
+# Disable button if any input field with given selector is empty
+window.disableButtonIfAnyEmptyField = (form, form_selector, button_selector) ->
+  closest_submit = form.find(button_selector)
+  empty = false
+  form.find('input').filter(form_selector).each ->
+    empty = true if rstrip($(this).val()) is ""
+
+  if empty
+    closest_submit.disable()
+  else
+    closest_submit.enable()
 
-  closest_submit.disable() if field.val().replace(/\s+$/, "") is ""
+  form.keyup ->
+    empty = false
+    form.find('input').filter(form_selector).each ->
+      empty = true if rstrip($(this).val()) is ""
 
-  field.on "input", ->
-    if $(@).val().replace(/\s+$/, "") is ""
+    if empty
       closest_submit.disable()
     else
       closest_submit.enable()

app/assets/javascripts/dispatcher.js.coffee

@@ -50,6 +50,8 @@ class Dispatcher
         new TreeView()
       when 'projects:blob:show'
         new BlobView()
+      when 'projects:labels:new'
+        new Labels()
 
     switch path.first()
       when 'admin' then new Admin()

app/assets/javascripts/labels.js.coffee

+class Labels
+  constructor: ->
+    form = $('.label-form')
+    @setupLabelForm(form)
+    @cleanBinding()
+    @addBinding()
+    @updateColorPreview
+
+  addBinding: ->
+    $(document).on 'click', '.suggest-colors a', @setSuggestedColor
+    $(document).on 'input', 'input#label_color', @updateColorPreview
+
+  cleanBinding: ->
+    $(document).off 'click', '.suggest-colors a'
+    $(document).off 'input', 'input#label_color'
+
+  # Initializes the form to disable the save button if no color or title is entered
+  setupLabelForm: (form) ->
+    disableButtonIfAnyEmptyField form, '.form-control', form.find('.js-save-button')
+
+  # Updates the the preview color with the hex-color input
+  updateColorPreview: =>
+    previewColor = $('input#label_color').val()
+    $('div.label-color-preview').css('background-color', previewColor)
+
+  # Updates the preview color with a click on a suggested color
+  setSuggestedColor: (e) =>
+    color = $(e.currentTarget).data('color')
+    $('input#label_color').val(color)
+    @updateColorPreview()
+    # Notify the form, that color has changed
+    $('.label-form').trigger('keyup')
+    e.preventDefault()
+
+@Labels = Labels

app/models/concerns/issuable.rb

@@ -140,7 +140,8 @@ module Issuable
 
   def add_labels_by_names(label_names)
     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
     end
   end

app/models/label.rb

 class Label < ActiveRecord::Base
+  DEFAULT_COLOR = '#82C5FF'
+
   belongs_to :project
   has_many :label_links, dependent: :destroy
   has_many :issues, through: :label_links, source: :target, source_type: 'Issue'
 
-  validates :color, format: { with: /\A\#[0-9A-Fa-f]{6}+\Z/ }, allow_blank: true
+  validates :color,
+            format: { with: /\A\#[0-9A-Fa-f]{6}+\Z/ },
+            allow_blank: false
   validates :project, presence: true
 
-  # Dont allow '?', '&', and ',' for label titles
-  validates :title, presence: true, format: { with: /\A[^&\?,&]*\z/ }
+  # Don't allow '?', '&', and ',' for label titles
+  validates :title,
+            presence: true,
+            format: { with: /\A[^&\?,&]*\z/ },
+            uniqueness: { scope: :project_id }
 
   scope :order_by_name, -> { reorder("labels.title ASC") }
 

app/views/projects/labels/_form.html.haml

@@ -28,22 +28,6 @@
              
 
   .form-actions
-    = f.submit 'Save', class: 'btn btn-save'
+    = f.submit 'Save', class: 'btn btn-save js-save-button'
     = link_to "Cancel", project_labels_path(@project), class: 'btn btn-cancel'
 
-
-:coffeescript
-  updateColorPreview = ->
-    previewColor = $('input#label_color').val()
-    $('div.label-color-preview').css('background-color', previewColor)
-
-  $('.suggest-colors a').on 'click', (e) ->
-    color = $(this).data("color")
-    $('input#label_color').val(color)
-    updateColorPreview()
-    e.preventDefault()
-
-  $('input#label_color').on 'input', ->
-    updateColorPreview()
-
-  updateColorPreview()

doc/api/issues.md

@@ -157,6 +157,9 @@ Parameters:
 - `milestone_id` (optional) - The ID of a milestone to assign 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
 
 Updates an existing project issue. This function is also used to mark an issue as closed.
@@ -176,6 +179,9 @@ Parameters:
 - `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)
 
+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**)
 
 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.

doc/api/merge_requests.md

@@ -139,6 +139,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
 
 Updates an existing merge request. You can change branches, title, or even close the MR.
@@ -186,9 +189,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
 
-Merge changes submitted with MR usign this API.
+Merge changes submitted with MR using this API.
 
 If merge success you get 200 OK.
 

features/project/issues/labels.feature

@@ -10,7 +10,7 @@ Feature: Project Labels
     And I should see label "feature"
 
   Scenario: I create new label
-    Given I visit new label page
+    Given I visit project "Shop" new label page
     When I submit new label 'support'
     Then I should see label 'support'
 
@@ -23,3 +23,21 @@ Feature: Project Labels
   Scenario: I remove label
     When I remove label 'bug'
     Then I should not see label 'bug'
+
+  Scenario: I create a label with invalid color
+    Given I visit project "Shop" new label page
+    When I submit new label with invalid color
+    Then I should see label color error message
+
+  Scenario: I create a label that already exists
+    Given I visit project "Shop" new label page
+    When I submit new label 'bug'
+    Then I should see label label exist error message
+
+  Scenario: I create the same label on another project
+    Given I own project "Forum"
+    And I visit project "Forum" labels page
+    And I visit project "Forum" new label page
+    When I submit new label 'bug'
+    Then I should see label 'bug'
+

features/steps/project/labels.rb

@@ -31,6 +31,36 @@ class ProjectLabels < Spinach::FeatureSteps
     click_button 'Save'
   end
 
+  step 'I submit new label \'bug\'' do
+    fill_in 'Title', with: 'bug'
+    fill_in 'Background Color', with: '#F95610'
+    click_button 'Save'
+  end
+
+  step 'I submit new label with invalid color' do
+    fill_in 'Title', with: 'support'
+    fill_in 'Background Color', with: '#12'
+    click_button 'Save'
+  end
+
+  step 'I should see label label exist error message' do
+    within '.label-form' do
+      page.should have_content 'Title has already been taken'
+    end
+  end
+
+  step 'I should see label color error message' do
+    within '.label-form' do
+      page.should have_content 'Color is invalid'
+    end
+  end
+
+  step 'I should see label \'bug\'' do
+    within '.manage-labels-list' do
+      page.should have_content 'bug'
+    end
+  end
+
   step 'I should not see label \'bug\'' do
     within '.manage-labels-list' do
       page.should_not have_content 'bug'

features/steps/shared/paths.rb

@@ -287,10 +287,22 @@ module SharedPaths
   end
 
   step 'I visit project "Shop" labels page' do
+    project = Project.find_by(name: 'Shop')
     visit project_labels_path(project)
   end
 
-  step 'I visit new label page' do
+  step 'I visit project "Forum" labels page' do
+    project = Project.find_by(name: 'Forum')
+    visit project_labels_path(project)
+  end
+
+  step 'I visit project "Shop" new label page' do
+    project = Project.find_by(name: 'Shop')
+    visit new_project_label_path(project)
+  end
+
+  step 'I visit project "Forum" new label page' do
+    project = Project.find_by(name: 'Forum')
     visit new_project_label_path(project)
   end
 

lib/api/helpers.rb

@@ -112,6 +112,21 @@ module API
       ActionController::Parameters.new(attrs).permit!
     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
 
     def forbidden!

lib/api/issues.rb

@@ -51,12 +51,18 @@ module API
         required_attributes! [:title]
         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
 
         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?
-            issue.add_labels_by_names(params[:labels].split(","))
+            issue.add_labels_by_names(params[:labels].split(','))
           end
 
           present issue, with: Entities::Issue
@@ -83,12 +89,19 @@ module API
         authorize! :modify_issue, issue
         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)
 
         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?
-            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
 
           present issue, with: Entities::Issue

lib/api/merge_requests.rb

@@ -76,6 +76,12 @@ module API
         authorize! :write_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 validate_label_params(params)
+          return render_api_error!('Label names invalid', 405)
+        end
+
         merge_request = ::MergeRequests::CreateService.new(user_project, current_user, attrs).execute
 
         if merge_request.valid?
@@ -109,6 +115,12 @@ module API
         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])
         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)
 
         if merge_request.valid?

spec/requests/api/issues_spec.rb

@@ -5,6 +5,10 @@ describe API::API, api: true  do
   let(:user) { create(:user) }
   let!(:project) { create(:project, namespace: user.namespace ) }
   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] }
 
   describe "GET /issues" do
@@ -68,6 +72,14 @@ describe API::API, api: true  do
       post api("/projects/#{project.id}/issues", user), labels: 'label, label2'
       response.status.should == 400
     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
 
   describe "PUT /projects/:id/issues/:issue_id to update only title" do
@@ -84,6 +96,14 @@ describe API::API, api: true  do
         title: 'updated title'
       response.status.should == 404
     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
 
   describe "PUT /projects/:id/issues/:issue_id to update state and label" do

spec/requests/api/merge_requests_spec.rb

@@ -78,9 +78,14 @@ describe API::API, api: true  do
     context 'between branches projects' do
       it "should return merge_request" do
         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
         json_response['title'].should == 'Test merge_request'
+        json_response['labels'].should == ['label', 'label2']
       end
 
       it "should return 422 when source_branch equals target_branch" do
@@ -106,6 +111,17 @@ describe API::API, api: true  do
         target_branch: 'master', source_branch: 'stable'
         response.status.should == 400
       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
 
     context 'forked projects' do
@@ -235,6 +251,15 @@ describe API::API, api: true  do
       response.status.should == 200
       json_response['target_branch'].should == 'wiki'
     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
 
   describe "POST /projects/:id/merge_request/:merge_request_id/comments" do