Fix specs and implement fixes based on failing specs

5 files changed, 36 insertions, 20 deletions

diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index aa08eef081c..b60afd1560e 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -80,13 +80,16 @@ class IssuableBaseService < BaseService
     params[key] = project.labels.where(id: params[key]).pluck(:id)
   end
 
-  def process_label_ids(attributes, base_label_ids: [], merge_all: false)
+  def process_label_ids(attributes, existing_label_ids: [])
     label_ids = attributes.delete(:label_ids)
     add_label_ids = attributes.delete(:add_label_ids)
     remove_label_ids = attributes.delete(:remove_label_ids)
 
-    new_label_ids = base_label_ids
-    new_label_ids = label_ids if label_ids && (merge_all || (add_label_ids.blank? && remove_label_ids.blank?))
+    new_label_ids = existing_label_ids
+
+    override_existing = new_label_ids.empty? || (add_label_ids.blank? && remove_label_ids.blank?)
+    new_label_ids = label_ids if label_ids && override_existing
+
     new_label_ids |= add_label_ids if add_label_ids
     new_label_ids -= remove_label_ids if remove_label_ids
 
@@ -103,14 +106,8 @@ class IssuableBaseService < BaseService
     params.merge!(command_params)
   end
 
-  def create_issuable(issuable, attributes)
+  def create_issuable(issuable, attributes, label_ids:)
     issuable.with_transaction_returning_status do
-      attributes.delete(:state_event)
-      params[:author] ||= current_user
-      label_ids = process_label_ids(attributes, merge_all: true)
-
-      issuable.assign_attributes(attributes)
-
       if issuable.save
         issuable.update_attributes(label_ids: label_ids)
       end
@@ -121,8 +118,16 @@ class IssuableBaseService < BaseService
     merge_slash_commands_into_params!(issuable)
     filter_params
 
-    if params.present? && create_issuable(issuable, params)
-      handle_creation(issuable)
+    params.delete(:state_event)
+    params[:author] ||= current_user
+    label_ids = process_label_ids(params)
+
+    issuable.assign_attributes(params)
+
+    before_create(issuable)
+
+    if params.present? && create_issuable(issuable, params, label_ids: label_ids)
+      after_create(issuable)
       issuable.create_cross_references!(current_user)
       execute_hooks(issuable)
     end
@@ -130,10 +135,16 @@ class IssuableBaseService < BaseService
     issuable
   end
 
+  def before_create(issuable)
+    # To be overridden by subclasses
+  end
+
+  def after_create(issuable)
+    # To be overridden by subclasses
+  end
+
   def update_issuable(issuable, attributes)
     issuable.with_transaction_returning_status do
-      attributes[:label_ids] = process_label_ids(attributes, base_label_ids: issuable.label_ids)
-
       issuable.update(attributes.merge(updated_by: current_user))
     end
   end
@@ -145,6 +156,8 @@ class IssuableBaseService < BaseService
     filter_params
     old_labels = issuable.labels.to_a
 
+    params[:label_ids] = process_label_ids(params, existing_label_ids: issuable.label_ids)
+
     if params.present? && update_issuable(issuable, params)
       issuable.reset_events_cache
       handle_common_system_notes(issuable, old_labels: old_labels)
diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb
index 8314c9971ff..ea1690f3e38 100644
--- a/app/services/issues/create_service.rb
+++ b/app/services/issues/create_service.rb
@@ -5,12 +5,15 @@ module Issues
       @api = params.delete(:api)
 
       @issue = project.issues.new
-      @issue.spam = spam_service.check(@api)
 
       create(@issue)
     end
 
-    def handle_creation(issuable)
+    def before_create(issuable)
+      issuable.spam = spam_service.check(@api)
+    end
+
+    def after_create(issuable)
       event_service.open_issue(issuable, current_user)
       notification_service.new_issue(issuable, current_user)
       todo_service.new_issue(issuable, current_user)
diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb
index 0b592cd5620..73247e62421 100644
--- a/app/services/merge_requests/create_service.rb
+++ b/app/services/merge_requests/create_service.rb
@@ -16,7 +16,7 @@ module MergeRequests
       create(merge_request)
     end
 
-    def handle_creation(issuable)
+    def after_create(issuable)
       event_service.open_mr(issuable, current_user)
       notification_service.new_merge_request(issuable, current_user)
       todo_service.new_merge_request(issuable, current_user)
diff --git a/app/services/projects/autocomplete_service.rb b/app/services/projects/autocomplete_service.rb
index cb85ee6694d..f578f8dbea2 100644
--- a/app/services/projects/autocomplete_service.rb
+++ b/app/services/projects/autocomplete_service.rb
@@ -29,7 +29,7 @@ module Projects
 
       opts = {
         project: project,
-        noteable: noteable,
+        issuable: noteable,
         current_user: current_user
       }
       SlashCommands::InterpretService.command_definitions.map do |definition|
diff --git a/app/services/slash_commands/interpret_service.rb b/app/services/slash_commands/interpret_service.rb
index 126f97b0f9b..cb0dca94036 100644
--- a/app/services/slash_commands/interpret_service.rb
+++ b/app/services/slash_commands/interpret_service.rb
@@ -4,8 +4,8 @@ module SlashCommands
 
     attr_reader :issuable
 
-    # Takes a text and interpret the commands that are extracted from it.
-    # Returns a hash of changes to be applied to a record.
+    # Takes a text and interprets the commands that are extracted from it.
+    # Returns the content without commands, and hash of changes to be applied to a record.
     def execute(content, issuable)
       @issuable = issuable
       @updates = {}