diff options
| -rw-r--r-- | app/assets/javascripts/application.js.coffee | 33 | ||||
| -rw-r--r-- | app/assets/javascripts/dispatcher.js.coffee | 2 | ||||
| -rw-r--r-- | app/assets/javascripts/labels.js.coffee | 35 | ||||
| -rw-r--r-- | app/models/concerns/issuable.rb | 3 | ||||
| -rw-r--r-- | app/models/label.rb | 13 | ||||
| -rw-r--r-- | app/views/projects/labels/_form.html.haml | 18 | ||||
| -rw-r--r-- | doc/api/issues.md | 6 | ||||
| -rw-r--r-- | doc/api/merge_requests.md | 10 | ||||
| -rw-r--r-- | features/project/issues/labels.feature | 20 | ||||
| -rw-r--r-- | features/steps/project/labels.rb | 30 | ||||
| -rw-r--r-- | features/steps/shared/paths.rb | 14 | ||||
| -rw-r--r-- | lib/api/helpers.rb | 15 | ||||
| -rw-r--r-- | lib/api/issues.rb | 21 | ||||
| -rw-r--r-- | lib/api/merge_requests.rb | 12 | ||||
| -rw-r--r-- | spec/requests/api/issues_spec.rb | 20 | ||||
| -rw-r--r-- | spec/requests/api/merge_requests_spec.rb | 27 |
16 files changed, 245 insertions, 34 deletions
diff --git a/app/assets/javascripts/application.js.coffee b/app/assets/javascripts/application.js.coffee index 82a810b6551..1960479321c 100644 --- a/app/assets/javascripts/application.js.coffee +++ b/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() diff --git a/app/assets/javascripts/dispatcher.js.coffee b/app/assets/javascripts/dispatcher.js.coffee index 89bb475a8a6..a463a2eb194 100644 --- a/app/assets/javascripts/dispatcher.js.coffee +++ b/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() diff --git a/app/assets/javascripts/labels.js.coffee b/app/assets/javascripts/labels.js.coffee new file mode 100644 index 00000000000..8e53d6929df --- /dev/null +++ b/app/assets/javascripts/labels.js.coffee @@ -0,0 +1,35 @@ +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 diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 517e4548624..0a5fe24b5af 100644 --- a/app/models/concerns/issuable.rb +++ b/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 diff --git a/app/models/label.rb b/app/models/label.rb index ce982579675..a511b7940ed 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -1,13 +1,20 @@ 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") } diff --git a/app/views/projects/labels/_form.html.haml b/app/views/projects/labels/_form.html.haml index 2a5c907febe..72a01e1c271 100644 --- a/app/views/projects/labels/_form.html.haml +++ b/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() diff --git a/doc/api/issues.md b/doc/api/issues.md index f775d502a6d..a4b3b3e9918 100644 --- a/doc/api/issues.md +++ b/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. diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 794d46ed7c9..230f572fc3b 100644 --- a/doc/api/merge_requests.md +++ b/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,15 +189,18 @@ 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. If it has some conflicts and can not be merged - you get 405 and error message 'Branch cannot be merged' -If merge request is already merged or closed - you get 405 and error message 'Method Not Allowed' +If merge request is already merged or closed - you get 405 and error message 'Method Not Allowed' If you dont have permissions to accept this merge request - you get 401 diff --git a/features/project/issues/labels.feature b/features/project/issues/labels.feature index 4a37b6dc9fa..29cf5307271 100644 --- a/features/project/issues/labels.feature +++ b/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' + diff --git a/features/steps/project/labels.rb b/features/steps/project/labels.rb index 3d9aa29299c..8320405e096 100644 --- a/features/steps/project/labels.rb +++ b/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' diff --git a/features/steps/shared/paths.rb b/features/steps/shared/paths.rb index 4e97dba20b3..0d06383509f 100644 --- a/features/steps/shared/paths.rb +++ b/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 diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 8189e433789..d36b29a00b1 100644 --- a/lib/api/helpers.rb +++ b/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! diff --git a/lib/api/issues.rb b/lib/api/issues.rb index b29118b2fd8..055529ccbd8 100644 --- a/lib/api/issues.rb +++ b/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 diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index acca7cb6bad..0d765f9280e 100644 --- a/lib/api/merge_requests.rb +++ b/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? diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index dff7f20cb32..d8e8e4f5035 100644 --- a/spec/requests/api/issues_spec.rb +++ b/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 diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 3611d9d6dc3..58cf7f139dc 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/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 |