refactors update issue api request and some minor comments

4 files changed, 41 insertions, 22 deletions

diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index e06c37c323e..3b37365612e 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -162,7 +162,12 @@ class IssuableBaseService < BaseService
 
     if params.present? && update_issuable(issuable, params)
       issuable.reset_events_cache
-      handle_common_system_notes(issuable, old_labels: old_labels)
+
+      # We do not touch as it will affect a update on updated_at field
+      ActiveRecord::Base.no_touching do
+        handle_common_system_notes(issuable, old_labels: old_labels)
+      end
+
       handle_changes(issuable, old_labels: old_labels, old_mentioned_users: old_mentioned_users)
       issuable.create_new_cross_references!(current_user)
       execute_hooks(issuable, 'update')
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index da4b1bf9902..dbad86d8926 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -102,6 +102,14 @@ 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 9a042e6e70d..39a46f69f16 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -154,14 +154,9 @@ module API
           render_api_error!({ labels: errors }, 400)
         end
 
-        # Find or create labels
-        if params[:labels].present?
-          attrs[:label_ids] = params[: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
+        # 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?
 
         issue = ::Issues::CreateService.new(user_project, current_user, attrs.merge(request: request, api: true)).execute
 
@@ -203,17 +198,16 @@ 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
+
         issue = ::Issues::UpdateService.new(user_project, current_user, attrs).execute(issue)
 
         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] && can?(current_user, :admin_issue, user_project)
-            issue.remove_labels
-            # Create and add labels to the new created issue
-            issue.add_labels_by_names(params[:labels].split(','))
-          end
-
           present issue, with: Entities::Issue, current_user: current_user
         else
           render_validation_error!(issue)
diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb
index a4c91252472..009fb3b2d70 100644
--- a/spec/requests/api/issues_spec.rb
+++ b/spec/requests/api/issues_spec.rb
@@ -479,16 +479,16 @@ describe API::API, api: true  do
       expect(json_response['labels']).to eq(['label', 'label2'])
     end
 
-    it "emails label subscribers" do
-      clear_enqueued_jobs
+    it "sends notifications for subscribers of newly added labels" do
       label = project.labels.first
       label.toggle_subscription(user2)
 
-      expect do
+      perform_enqueued_jobs do
         post api("/projects/#{project.id}/issues", user),
           title: 'new issue', labels: label.title
-      end.to change{enqueued_jobs.size}.by(1)
-      expect(response.status).to eq(201)
+      end
+
+      should_email(user2)
     end
 
     it "returns a 400 bad request if title not given" do
@@ -646,6 +646,18 @@ describe API::API, api: true  do
       expect(json_response['labels']).to eq([label.title])
     end
 
+    it "sends notifications for subscribers of newly added labels when issue is updated" do
+      label = project.labels.first
+      label.toggle_subscription(user2)
+
+      perform_enqueued_jobs do
+        put api("/projects/#{project.id}/issues/#{issue.id}", user),
+          title: 'updated title', labels: label.title
+      end
+
+      should_email(user2)
+    end
+
     it 'removes all labels' do
       put api("/projects/#{project.id}/issues/#{issue.id}", user),
           labels: ''