Merge branch '64320-refactor-mr-services' into 'master'

Code extraction - refactoring of MR services classes

See merge request gitlab-org/gitlab!49827

app/services/issuable_base_service.rb

@@ -190,11 +190,7 @@ class IssuableBaseService < BaseService
     change_additional_attributes(issuable)
     old_associations = associations_before_update(issuable)
 
-    label_ids = process_label_ids(params, existing_label_ids: issuable.label_ids)
-    if labels_changing?(issuable.label_ids, label_ids)
-      params[:label_ids] = label_ids
-      issuable.touch
-    end
+    assign_requested_labels(issuable)
 
     if issuable.changed? || params.present?
       issuable.assign_attributes(params)
@@ -297,10 +293,6 @@ class IssuableBaseService < BaseService
     update_task(issuable)
   end
 
-  def labels_changing?(old_label_ids, new_label_ids)
-    old_label_ids.sort != new_label_ids.sort
-  end
-
   def has_title_or_description_changed?(issuable)
     issuable.title_changed? || issuable.description_changed?
   end
@@ -349,6 +341,20 @@ class IssuableBaseService < BaseService
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
+  def assign_requested_labels(issuable)
+    label_ids = process_label_ids(params, existing_label_ids: issuable.label_ids)
+    return unless ids_changing?(issuable.label_ids, label_ids)
+
+    params[:label_ids] = label_ids
+    issuable.touch
+  end
+
+  # Arrays of ids are used, but we should really use sets of ids, so
+  # let's have an helper to properly check if some ids are changing
+  def ids_changing?(old_array, new_array)
+    old_array.sort != new_array.sort
+  end
+
   def toggle_award(issuable)
     award = params.delete(:emoji_award)
     AwardEmojis::ToggleService.new(issuable, award, current_user).execute if award

app/services/merge_requests/build_service.rb

@@ -16,30 +16,17 @@ module MergeRequests
       merge_request.source_project = find_source_project
       merge_request.target_project = find_target_project
 
-      # Source project sets the default source branch removal setting
-      merge_request.merge_params['force_remove_source_branch'] =
-        if params.key?(:force_remove_source_branch)
-          params.delete(:force_remove_source_branch)
-        else
-          merge_request.source_project.remove_source_branch_after_merge?
-        end
+      # Force remove the source branch?
+      merge_request.merge_params['force_remove_source_branch'] = force_remove_source_branch
 
+      # Only assign merge requests params that are allowed
       self.params = assign_allowed_merge_params(merge_request, params)
 
+      # Filter out params that are either not allowed or invalid
       filter_params(merge_request)
 
-      # merge_request.assign_attributes(...) below is a Rails
-      # method that only work if all the params it is passed have
-      # corresponding fields in the database. As there are no fields
-      # in the database for :add_label_ids and :remove_label_ids, we
-      # need to remove them from the params before the call to
-      # merge_request.assign_attributes(...)
-      #
-      # IssuableBaseService#process_label_ids takes care
-      # of the removal.
-      params[:label_ids] = process_label_ids(params, extra_label_ids: merge_request.label_ids.to_a)
-
-      merge_request.assign_attributes(params.to_h.compact)
+      # Filter out :add_label_ids and :remove_label_ids params
+      filter_label_id_params
 
       merge_request.compare_commits = []
       set_merge_request_target_branch
@@ -74,6 +61,29 @@ module MergeRequests
              :errors,
              to: :merge_request
 
+    def force_remove_source_branch
+      if params.key?(:force_remove_source_branch)
+        params.delete(:force_remove_source_branch)
+      else
+        merge_request.source_project.remove_source_branch_after_merge?
+      end
+    end
+
+    def filter_label_id_params
+      # merge_request.assign_attributes(...) below is a Rails
+      # method that only work if all the params it is passed have
+      # corresponding fields in the database. As there are no fields
+      # in the database for :add_label_ids and :remove_label_ids, we
+      # need to remove them from the params before the call to
+      # merge_request.assign_attributes(...)
+      #
+      # IssuableBaseService#process_label_ids takes care
+      # of the removal.
+      params[:label_ids] = process_label_ids(params, extra_label_ids: merge_request.label_ids.to_a)
+
+      merge_request.assign_attributes(params.to_h.compact)
+    end
+
     def find_source_project
       source_project = project_from_params(:source_project)
       return source_project if source_project.present? && can?(current_user, :create_merge_request_from, source_project)

changelogs/unreleased/64320-refactor-mr-services.yml

+---
+title: Code extraction - refactoring of MR services classes
+merge_request: 49827
+author:
+type: changed

spec/services/merge_requests/push_options_handler_service_spec.rb

@@ -21,6 +21,7 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do
   let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" }
   let(:deleted_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 #{Gitlab::Git::BLANK_SHA} refs/heads/#{source_branch}" }
   let(:default_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{project.default_branch}" }
+  let(:error_mr_required) { "A merge_request.create push option is required to create a merge request for branch #{source_branch}" }
 
   shared_examples_for 'a service that can create a merge request' do
     subject(:last_mr) { MergeRequest.last }
@@ -176,11 +177,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do
       it_behaves_like 'a service that does not create a merge request'
 
       it 'adds an error to the service' do
-        error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}"
-
         service.execute
 
-        expect(service.errors).to include(error)
+        expect(service.errors).to include(error_mr_required)
       end
 
       context 'when coupled with the `create` push option' do
@@ -197,11 +196,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do
       it_behaves_like 'a service that does not create a merge request'
 
       it 'adds an error to the service' do
-        error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}"
-
         service.execute
 
-        expect(service.errors).to include(error)
+        expect(service.errors).to include(error_mr_required)
       end
 
       context 'when coupled with the `create` push option' do
@@ -263,11 +260,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do
       it_behaves_like 'a service that does not create a merge request'
 
       it 'adds an error to the service' do
-        error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}"
-
         service.execute
 
-        expect(service.errors).to include(error)
+        expect(service.errors).to include(error_mr_required)
       end
 
       context 'when coupled with the `create` push option' do
@@ -308,11 +303,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do
       it_behaves_like 'a service that does not create a merge request'
 
       it 'adds an error to the service' do
-        error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}"
-
         service.execute
 
-        expect(service.errors).to include(error)
+        expect(service.errors).to include(error_mr_required)
       end
 
       context 'when coupled with the `create` push option' do
@@ -329,11 +322,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do
       it_behaves_like 'a service that does not create a merge request'
 
       it 'adds an error to the service' do
-        error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}"
-
         service.execute
 
-        expect(service.errors).to include(error)
+        expect(service.errors).to include(error_mr_required)
       end
 
       context 'when coupled with the `create` push option' do
@@ -374,11 +365,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do
       it_behaves_like 'a service that does not create a merge request'
 
       it 'adds an error to the service' do
-        error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}"
-
         service.execute
 
-        expect(service.errors).to include(error)
+        expect(service.errors).to include(error_mr_required)
       end
 
       context 'when coupled with the `create` push option' do
@@ -395,11 +384,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do
       it_behaves_like 'a service that does not create a merge request'
 
       it 'adds an error to the service' do
-        error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}"
-
         service.execute
 
-        expect(service.errors).to include(error)
+        expect(service.errors).to include(error_mr_required)
       end
 
       context 'when coupled with the `create` push option' do
@@ -440,11 +427,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do
       it_behaves_like 'a service that does not create a merge request'
 
       it 'adds an error to the service' do
-        error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}"
-
         service.execute
 
-        expect(service.errors).to include(error)
+        expect(service.errors).to include(error_mr_required)
       end
 
       context 'when coupled with the `create` push option' do
@@ -461,11 +446,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do
       it_behaves_like 'a service that does not create a merge request'
 
       it 'adds an error to the service' do
-        error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}"
-
         service.execute
 
-        expect(service.errors).to include(error)
+        expect(service.errors).to include(error_mr_required)
       end
 
       context 'when coupled with the `create` push option' do
@@ -506,11 +489,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do
       it_behaves_like 'a service that does not create a merge request'
 
       it 'adds an error to the service' do
-        error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}"
-
         service.execute
 
-        expect(service.errors).to include(error)
+        expect(service.errors).to include(error_mr_required)
       end
 
       context 'when coupled with the `create` push option' do
@@ -527,11 +508,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do
       it_behaves_like 'a service that does not create a merge request'
 
       it 'adds an error to the service' do
-        error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}"
-
         service.execute
 
-        expect(service.errors).to include(error)
+        expect(service.errors).to include(error_mr_required)
       end
 
       context 'when coupled with the `create` push option' do