CreateBuildService for build creation and more validation to build model

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

9 files changed, 66 insertions, 46 deletions

diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index 1d676f9..6b0ab5c 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -73,19 +73,13 @@ class ProjectsController < ApplicationController
   end
 
   def build
-   # Ignore remove branch push
-   return head(200) if params[:after] =~ /^00000000/
+    @build = CreateBuildService.new.execute(@project, params.dup)
 
-   build_params = params.dup
-   @build = @project.register_build(build_params)
-
-   if @build
-     head 200
-   else
-     head 500
-   end
-  rescue
-    head 500
+    if @build
+      head 200
+    else
+      head 500
+    end
   end
 
   # Project status badge
diff --git a/app/models/build.rb b/app/models/build.rb
index 436479d..72b3e0c 100644
--- a/app/models/build.rb
+++ b/app/models/build.rb
@@ -27,9 +27,11 @@ class Build < ActiveRecord::Base
   attr_accessible :project_id, :ref, :sha, :before_sha,
     :status, :finished_at, :trace, :started_at, :push_data, :runner_id, :project_name
 
+  validates :before_sha, presence: true
   validates :sha, presence: true
   validates :ref, presence: true
   validates :status, presence: true
+  validate :valid_commit_sha
 
   scope :running, ->() { where(status: "running") }
   scope :pending, ->() { where(status: "pending") }
@@ -91,6 +93,12 @@ class Build < ActiveRecord::Base
     state :canceled, value: 'canceled'
   end
 
+  def valid_commit_sha
+    if self.sha =~ /\A00000000/
+      self.errors.add(:sha, " cant be 00000000 (branch removal)")
+    end
+  end
+
   def compare?
     gitlab? && before_sha
   end
diff --git a/app/models/project.rb b/app/models/project.rb
index 1a72f7e..775cc9e 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -89,28 +89,6 @@ class Project < ActiveRecord::Base
     self.token = SecureRandom.hex(15) if self.token.blank?
   end
 
-  def register_build(opts={})
-    ref = opts[:ref]
-
-    raise 'ref is not defined' unless ref
-
-    if ref.include? 'heads'
-      ref = ref.scan(/heads\/(.*)$/).flatten[0]
-    end
-
-    before_sha = opts[:before]
-    sha = opts[:after]
-
-    data = {
-      project_id: self.id,
-      ref: ref,
-      sha: sha,
-      before_sha: before_sha,
-      push_data: opts
-    }
-
-    @build = Build.create(data)
-  end
 
   def gitlab?
     gitlab_url.present?
diff --git a/app/services/create_build_service.rb b/app/services/create_build_service.rb
new file mode 100644
index 0000000..5c06223
--- /dev/null
+++ b/app/services/create_build_service.rb
@@ -0,0 +1,20 @@
+class CreateBuildService
+  def execute(project, params)
+    before_sha = params[:before]
+    sha = params[:after]
+    ref = params[:ref]
+
+    if ref && ref.start_with?('/refs/heads/')
+      ref = ref.scan(/heads\/(.*)$/).flatten[0]
+    end
+
+    data = {
+      ref: ref,
+      sha: sha,
+      before_sha: before_sha,
+      push_data: params
+    }
+
+    project.builds.create(data)
+  end
+end
diff --git a/lib/api/builds.rb b/lib/api/builds.rb
index 0982d7d..45b2d35 100644
--- a/lib/api/builds.rb
+++ b/lib/api/builds.rb
@@ -80,7 +80,7 @@ module API
         required_attributes! [:project_id, :data, :project_token]
         project = Project.find(params[:project_id])
         authenticate_project_token!(project)
-        build = project.register_build(params[:data])
+        build = CreateBuildService.new.execute(project, params[:data])
 
         if build.persisted?
           present build, with: Entities::Build
diff --git a/spec/factories/builds.rb b/spec/factories/builds.rb
index a313275..fa411d9 100644
--- a/spec/factories/builds.rb
+++ b/spec/factories/builds.rb
@@ -3,6 +3,7 @@
 FactoryGirl.define do
   factory :build do
     ref 'master'
+    before_sha '76de212e80737a608d939f648d959671fb0a0142'
     sha '97de212e80737a608d939f648d959671fb0a0142'
     started_at 'Di 29. Okt 09:51:28 CET 2013'
     finished_at 'Di 29. Okt 09:53:28 CET 2013'
diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb
index 32a0446..a0c024d 100644
--- a/spec/models/build_spec.rb
+++ b/spec/models/build_spec.rb
@@ -24,6 +24,7 @@ describe Build do
   subject { Build.new }
 
   it { should belong_to(:project) }
+  it { should validate_presence_of :before_sha }
   it { should validate_presence_of :sha }
   it { should validate_presence_of :ref }
   it { should validate_presence_of :status }
@@ -39,7 +40,7 @@ describe Build do
 
   describe "#ci_skip?" do
     let(:project) { FactoryGirl.create(:project) }
-    let(:build) { project.register_build(ref: 'master') }
+    let(:build) { FactoryGirl.create(:build, project: project) }
 
     it 'true if commit message contains [ci skip]' do
       build.stub(:git_commit_message) { 'Small typo [ci skip]' }
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index df45bea..fc89175 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -49,17 +49,8 @@ describe Project do
   context :valid_project do
     let(:project) { FactoryGirl.create :project }
 
-    describe :register_build do
-      let(:build) { project.register_build(ref: 'master', after: '31das312') }
-
-      it { build.should be_kind_of(Build) }
-      it { build.should be_pending }
-      it { build.should be_valid }
-      it { build.should == project.last_build }
-    end
-
     context :project_with_build do
-      before { project.register_build ref: 'master', after: '31das312' }
+      before { FactoryGirl.create(:build, project: project) }
 
       it { project.status.should == 'pending' }
       it { project.last_build.should be_kind_of(Build)  }
diff --git a/spec/services/create_build_service_spec.rb b/spec/services/create_build_service_spec.rb
new file mode 100644
index 0000000..11b4588
--- /dev/null
+++ b/spec/services/create_build_service_spec.rb
@@ -0,0 +1,27 @@
+require 'spec_helper'
+
+describe CreateBuildService do
+  let(:service) { CreateBuildService.new }
+  let(:project) { FactoryGirl.create(:project) }
+
+  describe :execute do
+    context 'valid params' do
+      let(:build) { service.execute(project, ref: 'refs/heads/master', before: '00000000', after: '31das312') }
+
+      it { build.should be_kind_of(Build) }
+      it { build.should be_pending }
+      it { build.should be_valid }
+      it { build.should be_persisted }
+      it { build.should == project.last_build }
+    end
+
+    context 'without params' do
+      let(:build) { service.execute(project, {}) }
+
+      it { build.should be_kind_of(Build) }
+      it { build.should be_pending }
+      it { build.should_not be_valid }
+      it { build.should_not be_persisted }
+    end
+  end
+end