Extract common validations from ci services

DroneCI and TeamCity shared the same validations methods
on the data received. These validations were extracted
into a concern

app/models/concerns/service_push_data_validations.rb

@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+# This concern is used by registerd services such as TeamCityService and
+# DroneCiService and add methods to perform validations on the received
+# data.
+
+module ServicePushDataValidations
+  extend ActiveSupport::Concern
+
+  def merge_request_valid?(data)
+    data.dig(:object_attributes, :state) == 'opened' && merge_request_unchecked?(data)
+  end
+
+  def push_valid?(data)
+    data[:total_commits_count] > 0 &&
+      !branch_removed?(data) &&
+      # prefer merge request trigger over push to avoid double builds
+      !opened_merge_requests?(data)
+  end
+
+  def tag_push_valid?(data)
+    data[:total_commits_count] > 0 && !branch_removed?(data)
+  end
+
+  private
+
+  def branch_removed?(data)
+    Gitlab::Git.blank_ref?(data[:after])
+  end
+
+  def opened_merge_requests?(data)
+    project.merge_requests
+      .opened
+      .from_project(project)
+      .from_source_branches(Gitlab::Git.ref_name(data[:ref]))
+      .exists?
+  end
+
+  def merge_request_unchecked?(data)
+    MergeRequest.state_machines[:merge_status]
+      .check_state?(data.dig(:object_attributes, :merge_status))
+  end
+end

app/models/project_services/drone_ci_service.rb

@@ -2,6 +2,7 @@
 
 class DroneCiService < CiService
   include ReactiveService
+  include ServicePushDataValidations
 
   prop_accessor :drone_url, :token
   boolean_accessor :enable_ssl_verification
@@ -96,23 +97,4 @@ class DroneCiService < CiService
       { type: 'checkbox', name: 'enable_ssl_verification', title: "Enable SSL verification" }
     ]
   end
-
-  private
-
-  def tag_push_valid?(data)
-    data[:total_commits_count] > 0 && !Gitlab::Git.blank_ref?(data[:after])
-  end
-
-  def push_valid?(data)
-    opened_merge_requests = project.merge_requests.opened.where(source_project_id: project.id,
-                                                                source_branch: Gitlab::Git.ref_name(data[:ref]))
-
-    opened_merge_requests.empty? && data[:total_commits_count] > 0 &&
-      !Gitlab::Git.blank_ref?(data[:after])
-  end
-
-  def merge_request_valid?(data)
-    data[:object_attributes][:state] == 'opened' &&
-      MergeRequest.state_machines[:merge_status].check_state?(data[:object_attributes][:merge_status])
-  end
 end

app/models/project_services/teamcity_service.rb

@@ -2,6 +2,7 @@
 
 class TeamcityService < CiService
   include ReactiveService
+  include ServicePushDataValidations
 
   prop_accessor :teamcity_url, :build_type, :username, :password
 
@@ -89,20 +90,26 @@ class TeamcityService < CiService
   end
 
   def execute(data)
-    return unless supported_events.include?(data[:object_kind])
-
     case data[:object_kind]
     when 'push'
-      branch = Gitlab::Git.ref_name(data[:ref])
+      execute_push(data)
-      post_to_build_queue(data, branch) if push_valid?(data)
     when 'merge_request'
-      branch = data[:object_attributes][:source_branch]
+      execute_merge_request(data)
-      post_to_build_queue(data, branch) if merge_request_valid?(data)
     end
   end
 
   private
 
+  def execute_push(data)
+    branch = Gitlab::Git.ref_name(data[:ref])
+    post_to_build_queue(data, branch) if push_valid?(data)
+  end
+
+  def execute_merge_request(data)
+    branch = data[:object_attributes][:source_branch]
+    post_to_build_queue(data, branch) if merge_request_valid?(data)
+  end
+
   def read_build_page(response)
     if response.code != 200
       # If actual build link can't be determined,
@@ -159,27 +166,4 @@ class TeamcityService < CiService
   def basic_auth
     { username: username, password: password }
   end
-
-  def push_valid?(data)
-    data[:total_commits_count] > 0 &&
-      !branch_removed?(data) &&
-      no_open_merge_requests?(data)
-  end
-
-  def merge_request_valid?(data)
-    data.dig(:object_attributes, :state) == 'opened' &&
-      MergeRequest.state_machines[:merge_status].check_state?(data.dig(:object_attributes, :merge_status))
-  end
-
-  def branch_removed?(data)
-    Gitlab::Git.blank_ref?(data[:after])
-  end
-
-  def no_open_merge_requests?(data)
-    !project.merge_requests
-      .opened
-      .from_project(project)
-      .from_source_branches(Gitlab::Git.ref_name(data[:ref]))
-      .exists?
-  end
 end

spec/models/project_services/teamcity_service_spec.rb

@@ -7,10 +7,11 @@ describe TeamcityService, :use_clean_rails_memory_store_caching do
   include StubRequests
 
   let(:teamcity_url) { 'http://gitlab.com/teamcity' }
+  let(:project) { create(:project) }
 
   subject(:service) do
     described_class.create(
-      project: create(:project),
+      project: project,
       properties: {
         teamcity_url: teamcity_url,
         username: 'mic',
@@ -225,7 +226,7 @@ describe TeamcityService, :use_clean_rails_memory_store_caching do
       end
 
       it 'returns nil when ref is blank' do
-        data[:after] = "0000000000000000000000000000000000000000"
+        data[:after] = Gitlab::Git::BLANK_SHA
 
         expect(service.execute(data)).to be_nil
       end
@@ -235,6 +236,12 @@ describe TeamcityService, :use_clean_rails_memory_store_caching do
 
         expect(service.execute(data)).to be_nil
       end
+
+      it 'returns nil when a merge request is opened for the same ref' do
+        create(:merge_request, source_project: project, source_branch: 'dev-123_branch')
+
+        expect(service.execute(data)).to be_nil
+      end
     end
 
     context 'when merge_request' do
@@ -264,8 +271,8 @@ describe TeamcityService, :use_clean_rails_memory_store_caching do
         expect(service.execute(data)).to be_nil
       end
 
-      it 'returns nil when merge request is not unchecked or cannot_be_merged_recheck' do
+      it 'returns nil unless merge request is marked as unchecked' do
-        data[:object_attributes][:merge_status] = 'checked'
+        data[:object_attributes][:merge_status] = 'can_be_merged'
 
         expect(service.execute(data)).to be_nil
       end