Commit 74946c19 authored by Hiroyuki Sato's avatar Hiroyuki Sato

Move validation logic to service layer

parent ba0509ed
...@@ -54,7 +54,6 @@ module Ci ...@@ -54,7 +54,6 @@ module Ci
validates :ref, presence: { unless: :importing? } validates :ref, presence: { unless: :importing? }
validates :merge_request, presence: { if: :merge_request? } validates :merge_request, presence: { if: :merge_request? }
validates :merge_request, absence: { unless: :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 :tag, inclusion: { in: [false], if: :merge_request? }
validates :status, presence: { unless: :importing? } validates :status, presence: { unless: :importing? }
validate :valid_commit_sha, unless: :importing? validate :valid_commit_sha, unless: :importing?
...@@ -751,11 +750,5 @@ module Ci ...@@ -751,11 +750,5 @@ module Ci
project.repository.keep_around(self.sha, self.before_sha) project.repository.keep_around(self.sha, self.before_sha)
end 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
end end
...@@ -63,6 +63,7 @@ module MergeRequests ...@@ -63,6 +63,7 @@ module MergeRequests
# UpdateMergeRequestsWorker could be retried by an exception. # UpdateMergeRequestsWorker could be retried by an exception.
# MR pipelines should not be recreated in such case. # MR pipelines should not be recreated in such case.
return if merge_request.merge_request_pipeline_exists? return if merge_request.merge_request_pipeline_exists?
return if merge_request.has_no_commits?
Ci::CreatePipelineService Ci::CreatePipelineService
.new(merge_request.source_project, user, ref: merge_request.source_branch) .new(merge_request.source_project, user, ref: merge_request.source_branch)
......
...@@ -2,7 +2,7 @@ require 'spec_helper' ...@@ -2,7 +2,7 @@ require 'spec_helper'
describe Ci::Pipeline, :mailer do describe Ci::Pipeline, :mailer do
let(:user) { create(:user) } let(:user) { create(:user) }
set(:project) { create(:project, :repository) } set(:project) { create(:project) }
let(:pipeline) do let(:pipeline) do
create(:ci_empty_pipeline, status: :created, project: project) create(:ci_empty_pipeline, status: :created, project: project)
...@@ -131,18 +131,12 @@ describe Ci::Pipeline, :mailer do ...@@ -131,18 +131,12 @@ describe Ci::Pipeline, :mailer do
context 'when source is merge request' do context 'when source is merge request' do
let(:source) { :merge_request } 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') } let(:merge_request) { create(:merge_request, source_project: project, source_branch: 'feature', target_project: project, target_branch: 'master') }
it { expect(pipeline).to be_valid } it { expect(pipeline).to be_valid }
end 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 context 'when merge request is empty' do
let(:merge_request) { nil } let(:merge_request) { nil }
...@@ -1042,8 +1036,6 @@ describe Ci::Pipeline, :mailer do ...@@ -1042,8 +1036,6 @@ describe Ci::Pipeline, :mailer do
end end
context 'when repository does not exist' do context 'when repository does not exist' do
let(:project) { create(:project) }
let(:pipeline) do let(:pipeline) do
create(:ci_empty_pipeline, project: project, ref: 'master') create(:ci_empty_pipeline, project: project, ref: 'master')
end end
...@@ -2079,6 +2071,7 @@ describe Ci::Pipeline, :mailer do ...@@ -2079,6 +2071,7 @@ describe Ci::Pipeline, :mailer do
end end
describe "#all_merge_requests" do describe "#all_merge_requests" do
let(:project) { create(:project) }
let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master') } let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master') }
it "returns all merge requests having the same source branch" do it "returns all merge requests having the same source branch" do
......
...@@ -1378,7 +1378,7 @@ describe MergeRequest do ...@@ -1378,7 +1378,7 @@ describe MergeRequest do
source_project: project, source_project: project,
source_branch: source_ref, source_branch: source_ref,
target_project: project, target_project: project,
target_branch: 'merge-test') target_branch: 'stable')
end end
before do before do
......
...@@ -736,15 +736,6 @@ describe Ci::CreatePipelineService do ...@@ -736,15 +736,6 @@ describe Ci::CreatePipelineService do
end end
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 context 'when merge request is created from a forked project' do
let(:merge_request) do let(:merge_request) do
create(:merge_request, create(:merge_request,
......
...@@ -226,6 +226,24 @@ describe MergeRequests::CreateService do ...@@ -226,6 +226,24 @@ describe MergeRequests::CreateService do
end end
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 context "when .gitlab-ci.yml does not have merge_requests keywords" do
let(:config) do let(:config) do
{ {
......
...@@ -156,9 +156,9 @@ describe MergeRequests::RefreshService do ...@@ -156,9 +156,9 @@ describe MergeRequests::RefreshService do
.and change { @fork_merge_request.merge_request_pipelines.count }.by(1) .and change { @fork_merge_request.merge_request_pipelines.count }.by(1)
.and change { @another_merge_request.merge_request_pipelines.count }.by(0) .and change { @another_merge_request.merge_request_pipelines.count }.by(0)
expect(@merge_request.has_commits?).to be true expect(@merge_request.has_commits?).to be_truthy
expect(@fork_merge_request.has_commits?).to be true expect(@fork_merge_request.has_commits?).to be_truthy
expect(@another_merge_request.has_commits?).to be false expect(@another_merge_request.has_commits?).to be_falsy
end end
context "when branch pipeline was created before a merge request pipline has been created" do context "when branch pipeline was created before a merge request pipline has been created" do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment