Move validation logic to service layer

7 files changed, 26 insertions, 30 deletions

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 8881e420735..30a957b4117 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -54,7 +54,6 @@ module Ci
     validates :ref, presence: { unless: :importing? }
     validates :merge_request, presence: { if: :merge_request? }
     validates :merge_request, absence: { unless: :merge_request? }
-    validate :presence_of_commits_in_merge_request, if: :merge_request?
     validates :tag, inclusion: { in: [false], if: :merge_request? }
     validates :status, presence: { unless: :importing? }
     validate :valid_commit_sha, unless: :importing?
@@ -751,11 +750,5 @@ module Ci
 
       project.repository.keep_around(self.sha, self.before_sha)
     end
-
-    def presence_of_commits_in_merge_request
-      if merge_request&.has_no_commits?
-        errors.add(:merge_request, "must have commits")
-      end
-    end
   end
 end
diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb
index fe19abf50f6..ac51fee0b3f 100644
--- a/app/services/merge_requests/base_service.rb
+++ b/app/services/merge_requests/base_service.rb
@@ -63,6 +63,7 @@ module MergeRequests
       # UpdateMergeRequestsWorker could be retried by an exception.
       # MR pipelines should not be recreated in such case.
       return if merge_request.merge_request_pipeline_exists?
+      return if merge_request.has_no_commits?
 
       Ci::CreatePipelineService
         .new(merge_request.source_project, user, ref: merge_request.source_branch)
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 2869022f2f6..17f33785fda 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Ci::Pipeline, :mailer do
   let(:user) { create(:user) }
-  set(:project) { create(:project, :repository) }
+  set(:project) { create(:project) }
 
   let(:pipeline) do
     create(:ci_empty_pipeline, status: :created, project: project)
@@ -131,18 +131,12 @@ describe Ci::Pipeline, :mailer do
     context 'when source is merge request' do
       let(:source) { :merge_request }
 
-      context 'when merge request with commits is specified' do
+      context 'when merge request is specified' do
         let(:merge_request) { create(:merge_request, source_project: project, source_branch: 'feature', target_project: project, target_branch: 'master') }
 
         it { expect(pipeline).to be_valid }
       end
 
-      context 'when merge request without commits is specified' do
-        let(:merge_request) { create(:merge_request, source_project: project, source_branch: 'not-merged-branch', target_project: project, target_branch: 'master') }
-
-        it { expect(pipeline).not_to be_valid }
-      end
-
       context 'when merge request is empty' do
         let(:merge_request) { nil }
 
@@ -1042,8 +1036,6 @@ describe Ci::Pipeline, :mailer do
     end
 
     context 'when repository does not exist' do
-      let(:project) { create(:project) }
-
       let(:pipeline) do
         create(:ci_empty_pipeline, project: project, ref: 'master')
       end
@@ -2079,6 +2071,7 @@ describe Ci::Pipeline, :mailer do
   end
 
   describe "#all_merge_requests" do
+    let(:project) { create(:project) }
     let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master') }
 
     it "returns all merge requests having the same source branch" do
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index c9643b34fa0..bfc9035cb56 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1378,7 +1378,7 @@ describe MergeRequest do
                  source_project: project,
                  source_branch: source_ref,
                  target_project: project,
-                 target_branch: 'merge-test')
+                 target_branch: 'stable')
         end
 
         before do
diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb
index be6bdc21b81..8497e90bd8b 100644
--- a/spec/services/ci/create_pipeline_service_spec.rb
+++ b/spec/services/ci/create_pipeline_service_spec.rb
@@ -736,15 +736,6 @@ describe Ci::CreatePipelineService do
               end
             end
 
-            context 'when there are no commits between source branch and target branch' do
-              let(:ref_name) { 'refs/heads/not-merged-branch' }
-
-              it 'does not create a merge request pipeline' do
-                expect(pipeline).not_to be_persisted
-                expect(pipeline.errors[:merge_request]).to eq(["must have commits"])
-              end
-            end
-
             context 'when merge request is created from a forked project' do
               let(:merge_request) do
                 create(:merge_request,
diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb
index 308f99dc0da..723cc860d0c 100644
--- a/spec/services/merge_requests/create_service_spec.rb
+++ b/spec/services/merge_requests/create_service_spec.rb
@@ -226,6 +226,24 @@ describe MergeRequests::CreateService do
           end
         end
 
+        context 'when there are no commits between source branch and target branch' do
+          let(:opts) do
+            {
+              title: 'Awesome merge_request',
+              description: 'please fix',
+              source_branch: 'not-merged-branch',
+              target_branch: 'master'
+            }
+          end
+
+          it 'does not create a merge request pipeline' do
+            expect(merge_request).to be_persisted
+
+            merge_request.reload
+            expect(merge_request.merge_request_pipelines.count).to eq(0)
+          end
+        end
+
         context "when .gitlab-ci.yml does not have merge_requests keywords" do
           let(:config) do
             {
diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb
index 57b8e71c2d5..9e9dc5a576c 100644
--- a/spec/services/merge_requests/refresh_service_spec.rb
+++ b/spec/services/merge_requests/refresh_service_spec.rb
@@ -156,9 +156,9 @@ describe MergeRequests::RefreshService do
             .and change { @fork_merge_request.merge_request_pipelines.count }.by(1)
             .and change { @another_merge_request.merge_request_pipelines.count }.by(0)
 
-          expect(@merge_request.has_commits?).to be true
-          expect(@fork_merge_request.has_commits?).to be true
-          expect(@another_merge_request.has_commits?).to be false
+          expect(@merge_request.has_commits?).to be_truthy
+          expect(@fork_merge_request.has_commits?).to be_truthy
+          expect(@another_merge_request.has_commits?).to be_falsy
         end
 
         context "when branch pipeline was created before a merge request pipline has been created" do