From 66b847607acfb51bdcdb691414e6ab9a4db13c20 Mon Sep 17 00:00:00 2001
From: tiagonbotelho <tiagonbotelho@hotmail.com>
Date: Thu, 18 Aug 2016 11:24:44 +0100
Subject: if issue is not valid we revert back to the old labels when updating

---
 CHANGELOG                             |  2 +-
 app/services/issuable_base_service.rb | 14 ++++++++++++++
 lib/api/helpers.rb                    |  8 --------
 lib/api/issues.rb                     | 11 ++---------
 spec/requests/api/issues_spec.rb      |  2 +-
 5 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG
index e345a6f7da3..ce05c03bbfc 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -31,6 +31,7 @@ v 8.12.0 (unreleased)
   - Capitalize mentioned issue timeline notes (ClemMakesApps)
   - Use the default branch for displaying the project icon instead of master !5792 (Hannes Rosenögger)
   - Adds response mime type to transaction metric action when it's not HTML
+  - Creating an issue through our API now emails label subscribers !5720
 
 v 8.11.3 (unreleased)
   - Allow system info page to handle case where info is unavailable
@@ -55,7 +56,6 @@ v 8.11.0
   - Remove the http_parser.rb dependency by removing the tinder gem. !5758 (tbalthazar)
   - Add Koding (online IDE) integration
   - Ability to specify branches for Pivotal Tracker integration (Egor Lynko)
-  - Creating an issue through our API now emails label subscribers !5720
   - Fix don't pass a local variable called `i` to a partial. !20510 (herminiotorres)
   - Fix rename `add_users_into_project` and `projects_ids`. !20512 (herminiotorres)
   - Fix adding line comments on the initial commit to a repo !5900
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 3b37365612e..7b1810f436d 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -45,6 +45,7 @@ class IssuableBaseService < BaseService
 
     unless can?(current_user, ability, project)
       params.delete(:milestone_id)
+      params.delete(:labels)
       params.delete(:add_label_ids)
       params.delete(:remove_label_ids)
       params.delete(:label_ids)
@@ -72,6 +73,7 @@ class IssuableBaseService < BaseService
     filter_labels_in_param(:add_label_ids)
     filter_labels_in_param(:remove_label_ids)
     filter_labels_in_param(:label_ids)
+    find_or_create_label_ids
   end
 
   def filter_labels_in_param(key)
@@ -80,6 +82,18 @@ class IssuableBaseService < BaseService
     params[key] = project.labels.where(id: params[key]).pluck(:id)
   end
 
+  def find_or_create_label_ids
+    return unless params[:labels]
+
+    params[:label_ids] = params[:labels].split(",").map do |label_name|
+      project.labels.create_with(color: Label::DEFAULT_COLOR)
+                    .find_or_create_by(title: label_name.strip)
+                    .id
+    end
+
+    params.delete(:labels)
+  end
+
   def process_label_ids(attributes, existing_label_ids: nil)
     label_ids = attributes.delete(:label_ids)
     add_label_ids = attributes.delete(:add_label_ids)
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index dbad86d8926..da4b1bf9902 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -102,14 +102,6 @@ module API
       label || not_found!('Label')
     end
 
-    def get_label_ids(labels)
-      labels.split(",").map do |label_name|
-        user_project.labels.create_with(color: Label::DEFAULT_COLOR)
-                           .find_or_create_by(title: label_name.strip)
-                           .id
-      end
-    end
-
     def find_project_issue(id)
       issue = user_project.issues.find(id)
       not_found! unless can?(current_user, :read_issue, issue)
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index 39a46f69f16..d0bc7243e54 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -154,9 +154,7 @@ module API
           render_api_error!({ labels: errors }, 400)
         end
 
-        # Find or create labels to attach to the issue. Labels are vaild
-        # because we already checked its name, so there can't be an error here
-        attrs[:label_ids] = get_label_ids(params[:labels]) if params[:labels].present?
+        attrs[:labels] = params[:labels] if params[:labels]
 
         issue = ::Issues::CreateService.new(user_project, current_user, attrs.merge(request: request, api: true)).execute
 
@@ -198,12 +196,7 @@ module API
           render_api_error!({ labels: errors }, 400)
         end
 
-        # 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] && can?(current_user, :admin_issue, user_project)
-          issue.remove_labels
-          attrs[:label_ids] = get_label_ids(params[:labels])
-        end
+        attrs[:labels] = params[:labels] if params[:labels]
 
         issue = ::Issues::UpdateService.new(user_project, current_user, attrs).execute(issue)
 
diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb
index 009fb3b2d70..3362a88d798 100644
--- a/spec/requests/api/issues_spec.rb
+++ b/spec/requests/api/issues_spec.rb
@@ -647,7 +647,7 @@ describe API::API, api: true  do
     end
 
     it "sends notifications for subscribers of newly added labels when issue is updated" do
-      label = project.labels.first
+      label = create(:label, title: 'foo', color: '#FFAABB', project: project)
       label.toggle_subscription(user2)
 
       perform_enqueued_jobs do
-- 
cgit v1.2.1