Refactor commits/builds logic

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

26 files changed, 158 insertions, 246 deletions

diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index f21578f..98f13bd 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -73,9 +73,9 @@ class ProjectsController < ApplicationController
   end
 
   def build
-    @builds = CreateBuildsService.new.execute(@project, params.dup)
+    @commit = CreateCommitService.new.execute(@project, params.dup)
 
-    if @builds.any? && @builds.any?(&:persisted?)
+    if @commit && @commit.valid?
       head 201
     else
       head 400
diff --git a/app/helpers/builds_helper.rb b/app/helpers/builds_helper.rb
index 4182675..131d454 100644
--- a/app/helpers/builds_helper.rb
+++ b/app/helpers/builds_helper.rb
@@ -21,10 +21,6 @@ module BuildsHelper
     end
   end
 
-  def build_link build
-    link_to(build.short_sha, project_build_path(build.project, build))
-  end
-
   def build_url(build)
     project_build_url(build.project, build)
   end
diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb
index a79f729..c0a8f31 100644
--- a/app/helpers/commits_helper.rb
+++ b/app/helpers/commits_helper.rb
@@ -1,5 +1,7 @@
 module CommitsHelper
   def commit_status_alert_class(commit)
+    return unless commit
+
     case commit.status
     when 'success'
       'alert-success'
@@ -9,4 +11,8 @@ module CommitsHelper
       'alert-warning'
     end
   end
+
+  def commit_link(commit)
+    link_to(commit.short_sha, project_commit_path(commit.project, commit))
+  end
 end
diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb
index ebc06c3..08ad3eb 100644
--- a/app/helpers/projects_helper.rb
+++ b/app/helpers/projects_helper.rb
@@ -1,14 +1,4 @@
 module ProjectsHelper
-  def project_statuc_class(project)
-    if project.last_build.try :success?
-      'alert-success'
-    elsif project.last_build.try :failed?
-      'alert-danger'
-    else
-      'alert-warning'
-    end
-  end
-
   def ref_tab_class ref = nil
     'active' if ref == @ref
   end
diff --git a/app/models/build.rb b/app/models/build.rb
index 7486492..ccf08e1 100644
--- a/app/models/build.rb
+++ b/app/models/build.rb
@@ -121,6 +121,9 @@ class Build < ActiveRecord::Base
     state :canceled, value: 'canceled'
   end
 
+  delegate :ref, :sha, :short_sha, :before_sha,
+    to: :commit, prefix: false
+
   def trace_html
     html = Ansi2html::convert(trace) if trace.present?
     html ||= ''
@@ -150,28 +153,6 @@ class Build < ActiveRecord::Base
     end
   end
 
-
-  # The following methods are provided for Grape::Entity and end up being
-  # useful everywhere else to reduce the changes needed for parallel builds.
-  def ref
-    commit.ref
-  end
-
-  def sha
-    commit.sha
-  end
-
-  def short_sha
-    commit.short_sha
-  end
-
-  def before_sha
-    commit.before_sha end
-
-  def allow_git_fetch
-    commit.allow_git_fetch
-  end
-
   def project
     commit.project
   end
@@ -181,11 +162,15 @@ class Build < ActiveRecord::Base
   end
 
   def project_name
-    commit.project_name
+    project.name
   end
 
   def repo_url
-    commit.repo_url
+    project.repo_url_with_auth
+  end
+
+  def allow_git_fetch
+    project.allow_git_fetch
   end
 
   def update_coverage
diff --git a/app/models/commit.rb b/app/models/commit.rb
index 4abe0da..6817101 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -76,24 +76,6 @@ class Commit < ActiveRecord::Base
     nil
   end
 
-  # Build a clone-able repo url
-  # using http and basic auth
-  def repo_url
-    auth = "gitlab-ci-token:#{project.token}@"
-    url = project.gitlab_url + ".git"
-    url.sub(/^https?:\/\//) do |prefix|
-      prefix + auth
-    end
-  end
-
-  def allow_git_fetch
-    project.allow_git_fetch
-  end
-
-  def project_name
-    project.name
-  end
-
   def project_recipients
     recipients = project.email_recipients.split(' ')
     recipients << git_author_email if project.email_add_committer?
@@ -153,17 +135,20 @@ class Commit < ActiveRecord::Base
     status == 'failed'
   end
 
+  # TODO: implement
   def canceled?
   end
 
   def duration
+    @duration ||= builds.select(&:finished_at).sum(&:duration)
   end
 
   def finished_at
+    @finished_at ||= builds.order('finished_at ASC').first.try(:finished_at)
   end
 
   def coverage
-    if builds.size == 1
+    if project.coverage_enabled? && builds.size == 1
       builds.first.coverage
     end
   end
diff --git a/app/models/project.rb b/app/models/project.rb
index 314f7e8..9bf3302 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -25,6 +25,8 @@
 #
 
 class Project < ActiveRecord::Base
+  include ProjectStatus
+
   attr_accessible :name, :path, :scripts, :timeout, :token, :timeout_in_minutes,
     :default_ref, :gitlab_url, :always_build, :polling_interval,
     :public, :ssh_url_to_repo, :gitlab_id, :allow_git_fetch, :skip_refs,
@@ -116,44 +118,6 @@ ls -la
     gitlab_url.present?
   end
 
-  def status
-    last_build.status if last_build
-  end
-
-  def broken?
-    last_build.failed? || last_build.canceled? if last_build
-  end
-
-  def success?
-    last_build.success? if last_build
-  end
-
-  def broken_or_success?
-    broken? || success?
-  end
-
-  def last_build
-    @last_build ||= commits.last.last_build if commits.any?
-  end
-
-  def last_build_date
-    last_build.try(:created_at)
-  end
-
-  def human_status
-    status
-  end
-
-  def last_build_for_ref(ref)
-    commit_for_ref = commits.where(ref: ref).last
-    Build.where(commit_id: commit_for_ref).last
-  end
-
-  def last_build_for_sha(sha)
-    commit = commits.find_by_sha(sha)
-    commit.last_build unless commit.nil?
-  end
-
   def tracked_refs
     @tracked_refs ||= default_ref.split(",").map{|ref| ref.strip}
   end
@@ -175,14 +139,6 @@ ls -la
     web_hooks.any?
   end
 
-  # only check for toggling build status within same ref.
-  def last_build_changed_status?
-    commits_for_ref = commit.where(ref: last_build.ref)
-    last_builds = Build.where(commit_id: commits_for_ref).order('id DESC').limit(2)
-    return false if last_builds.size < 2
-    return last_builds[0].status != last_builds[1].status
-  end
-
   def timeout_in_minutes
     timeout / 60
   end
@@ -214,4 +170,14 @@ ls -la
       errors.add(:jobs, "At least one foo")
     end
   end
+
+  # Build a clone-able repo url
+  # using http and basic auth
+  def repo_url_with_auth
+    auth = "gitlab-ci-token:#{token}@"
+    url = gitlab_url + ".git"
+    url.sub(/^https?:\/\//) do |prefix|
+      prefix + auth
+    end
+  end
 end
diff --git a/app/models/project_status.rb b/app/models/project_status.rb
new file mode 100644
index 0000000..b14cb4f
--- /dev/null
+++ b/app/models/project_status.rb
@@ -0,0 +1,45 @@
+module ProjectStatus
+  def status
+    last_commit.status if last_commit
+  end
+
+  def broken?
+    last_commit.failed? if last_commit
+  end
+
+  def success?
+    last_commit.success? if last_commit
+  end
+
+  def broken_or_success?
+    broken? || success?
+  end
+
+  def last_commit
+    @last_commit ||= commits.last if commits.any?
+  end
+
+  def last_commit_date
+    last_commit.try(:created_at)
+  end
+
+  def human_status
+    status
+  end
+
+  # only check for toggling build status within same ref.
+  def last_commit_changed_status?
+    ref = last_commit.ref
+    last_commits = commits.where(ref: ref).order('id DESC').limit(2)
+
+    if last_commits.size < 2
+      false
+    else
+      last_commits[0].status != last_commits[1].status
+    end
+  end
+
+  def last_commit_for_ref(ref)
+    commits.where(ref: ref).order('id DESC').first
+  end
+end
diff --git a/app/services/create_builds_service.rb b/app/services/create_builds_service.rb
deleted file mode 100644
index 8608227..0000000
--- a/app/services/create_builds_service.rb
+++ /dev/null
@@ -1,7 +0,0 @@
-class CreateBuildsService
-  def execute(project, params)
-    commit = Commit.where(project: project).where(sha: params[:after]).first
-    commit ||= CreateCommitService.new.execute(project, params)
-    commit.create_builds
-  end
-end
diff --git a/app/services/create_commit_service.rb b/app/services/create_commit_service.rb
index f37d787..ee951c0 100644
--- a/app/services/create_commit_service.rb
+++ b/app/services/create_commit_service.rb
@@ -8,7 +8,19 @@ class CreateCommitService
       ref = ref.scan(/heads\/(.*)$/).flatten[0]
     end
 
-    return false if project.skip_ref?(ref)
+    # Skip branch removal
+    if sha =~ /\A0+\Z/
+      return false
+    end
+
+    # Dont create commit if we already have one
+    if commit_exists?(project, sha)
+      return false
+    end
+
+    if project.skip_ref?(ref)
+      return false
+    end
 
     data = {
       ref: ref,
@@ -25,6 +37,12 @@ class CreateCommitService
       }
     }
 
-    project.commits.create(data)
+    commit = project.commits.create(data)
+    commit.create_builds
+    commit
+  end
+
+  def commit_exists?(project, sha)
+    project.commits.where(sha: sha).any?
   end
 end
diff --git a/app/services/image_for_build_service.rb b/app/services/image_for_build_service.rb
index 79bf2ee..7dfcfaa 100644
--- a/app/services/image_for_build_service.rb
+++ b/app/services/image_for_build_service.rb
@@ -1,16 +1,15 @@
 class ImageForBuildService
   def execute(project, params)
-    if params[:sha]
-      # Look for last build if commit sha provided
-      build = project.last_build_for_sha(params[:sha])
-      image_name = image_for_build(build)
-    elsif params[:ref]
-      # Look for last build per branch
-      build = project.last_build_for_ref(params[:ref])
-      image_name = image_for_build(build)
-    else
-      image_name = 'unknown.png'
-    end
+    image_name =
+      if params[:sha]
+        commit = project.commits.find_by(sha: params[:sha])
+        image_for_commit(commit)
+      elsif params[:ref]
+        commit = project.last_commit_for_ref(params[:ref])
+        image_for_commit(commit)
+      else
+        'unknown.png'
+      end
 
     image_path = Rails.root.join('public', image_name)
 
@@ -22,17 +21,9 @@ class ImageForBuildService
 
   private
 
-  def image_for_build(build)
-    return 'unknown.png' unless build
+  def image_for_commit(commit)
+    return 'unknown.png' unless commit
 
-    if build.success?
-      'success.png'
-    elsif build.failed?
-      'failed.png'
-    elsif build.active?
-      'running.png'
-    else
-      'unknown.png'
-    end
+    commit.status + ".png"
   end
 end
diff --git a/app/views/admin/projects/_project.html.haml b/app/views/admin/projects/_project.html.haml
index a8ee2c3..da3469a 100644
--- a/app/views/admin/projects/_project.html.haml
+++ b/app/views/admin/projects/_project.html.haml
@@ -1,14 +1,14 @@
-- last_build = project.last_build
-%tr.alert{class: project_statuc_class(project) }
+- last_commit = project.last_commit
+%tr.alert{class: commit_status_alert_class(last_commit) }
   %td
     = project.id
   %td
     = link_to [:admin, project] do
       %strong= project.name
   %td
-    - if last_build
-      #{last_build.status} (#{build_link(last_build)})
-      = time_ago_in_words project.last_build_date
+    - if last_commit
+      #{last_commit.status} (#{commit_link(last_commit)})
+      = time_ago_in_words project.last_commit_date
       ago
     - else
       No builds yet
@@ -20,7 +20,7 @@
       %i.icon-lock
       Private
   %td
-    = project.builds.count
+    = project.commits.count
   %td
     = link_to [:admin, project], method: :delete, class: 'btn btn-danger btn-xs' do
       %i.icon-remove
diff --git a/app/views/admin/projects/show.html.haml b/app/views/admin/projects/show.html.haml
index fee018a..fd57f50 100644
--- a/app/views/admin/projects/show.html.haml
+++ b/app/views/admin/projects/show.html.haml
@@ -32,8 +32,8 @@
         %strong= @project.public
       %p
         Last build:
-        - if @project.last_build
-          = time_ago_in_words @project.last_build_date
+        - if @project.last_commit
+          = time_ago_in_words @project.last_commit_date
     %br
     %fieldset
       %legend Email notification
diff --git a/app/views/builds/_build.html.haml b/app/views/builds/_build.html.haml
index 853556c..4e04f62 100644
--- a/app/views/builds/_build.html.haml
+++ b/app/views/builds/_build.html.haml
@@ -10,11 +10,6 @@
     - if build.job
       = build.job.name
 
-  %td.build-branch
-    - unless @ref
-      %span
-        = link_to build.ref, project_path(@project, ref: build.ref)
-
   %td.duration
     - if build.duration
       #{distance_of_time_in_words build.duration}
diff --git a/app/views/commits/_commit.html.haml b/app/views/commits/_commit.html.haml
index 554a05d..70861ac 100644
--- a/app/views/commits/_commit.html.haml
+++ b/app/views/commits/_commit.html.haml
@@ -15,7 +15,7 @@
         = link_to commit.ref, project_path(@project, ref: commit.ref)
 
   %td.duration
-    - if commit.duration
+    - if commit.duration > 0
       #{distance_of_time_in_words commit.duration}
 
   %td.timestamp
diff --git a/app/views/commits/show.html.haml b/app/views/commits/show.html.haml
index d7c907b..07d7eb9 100644
--- a/app/views/commits/show.html.haml
+++ b/app/views/commits/show.html.haml
@@ -27,7 +27,7 @@
     .col-sm-6
       %p
         %span.attr-name Author:
-        #{@commit.git_author_name}
+        #{@commit.git_author_name} (#{@commit.git_author_email})
       - if @commit.created_at
         %p
           %span.attr-name Created at:
@@ -40,7 +40,12 @@
   .status
     = @commit.status.titleize
 
-%h3 Builds
+%h3
+  Builds
+  - if @commit.duration > 0
+    %small.pull-right
+      %i.icon-time
+      #{distance_of_time_in_words @commit.duration}
 
 %table.builds
   %thead
@@ -48,7 +53,6 @@
       %th Status
       %th Build ID
       %th Job
-      %th Branch
       %th Duration
       %th Finished at
       - if @project.coverage_enabled?
diff --git a/app/views/projects/_gitlab.html.haml b/app/views/projects/_gitlab.html.haml
index d51aca1..650219e 100644
--- a/app/views/projects/_gitlab.html.haml
+++ b/app/views/projects/_gitlab.html.haml
@@ -15,9 +15,9 @@
     %tr
       %th ID
       %th Project Name
-      %th Last build
+      %th Last commit
       %th Access
-      %th Builds
+      %th Commits
 
   = render @projects
 
diff --git a/app/views/projects/_project.html.haml b/app/views/projects/_project.html.haml
index 49c8653..ca39ece 100644
--- a/app/views/projects/_project.html.haml
+++ b/app/views/projects/_project.html.haml
@@ -1,14 +1,14 @@
-- last_build = project.last_build
-%tr.alert{class: project_statuc_class(project) }
+- last_commit = project.last_commit
+%tr.alert{class: commit_status_alert_class(last_commit) }
   %td
     = project.id
   %td
     = link_to project do
       %strong= project.name
   %td
-    - if last_build
-      #{last_build.status} (#{build_link(last_build)})
-      = time_ago_in_words project.last_build_date
+    - if last_commit
+      #{last_commit.status} (#{commit_link(last_commit)})
+      = time_ago_in_words project.last_commit_date
       ago
     - else
       No builds yet
@@ -20,4 +20,4 @@
       %i.icon-lock
       Private
   %td
-    = project.builds.count
+    = project.commits.count
diff --git a/app/views/projects/_public.html.haml b/app/views/projects/_public.html.haml
index f2e887c..6dc3181 100644
--- a/app/views/projects/_public.html.haml
+++ b/app/views/projects/_public.html.haml
@@ -8,9 +8,9 @@
       %tr
         %th ID
         %th Name
-        %th Last build
+        %th Last commit
         %th Access
-        %th Builds
+        %th Commits
       = render @projects
   = paginate @projects
 - else
diff --git a/lib/api/builds.rb b/lib/api/builds.rb
index 1103dad..e4d6e4b 100644
--- a/lib/api/builds.rb
+++ b/lib/api/builds.rb
@@ -89,10 +89,10 @@ module API
         required_attributes! [:project_id, :data, :project_token]
         project = Project.find(params[:project_id])
         authenticate_project_token!(project)
-        builds = CreateBuildsService.new.execute(project, params[:data])
+        commit = CreateCommitService.new.execute(project, params[:data])
 
         # Temporary solution to keep api compatibility
-        build = builds.first
+        build = commit.builds.first
 
         if build.persisted?
           present build, with: Entities::Build
diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb
index 753da0d..f8b44e1 100644
--- a/spec/models/build_spec.rb
+++ b/spec/models/build_spec.rb
@@ -207,7 +207,7 @@ describe Build do
   describe :allow_git_fetch do
     subject { build.allow_git_fetch }
 
-    it { should eq(commit.allow_git_fetch) }
+    it { should eq(project.allow_git_fetch) }
   end
 
   describe :project do
@@ -225,13 +225,13 @@ describe Build do
   describe :project_name do
     subject { build.project_name }
 
-    it { should eq(commit.project_name) }
+    it { should eq(project.name) }
   end
 
   describe :repo_url do
     subject { build.repo_url }
 
-    it { should eq(commit.repo_url) }
+    it { should eq(project.repo_url_with_auth) }
   end
 
   describe :extract_coverage do
diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb
index 75f236b..2b14053 100644
--- a/spec/models/commit_spec.rb
+++ b/spec/models/commit_spec.rb
@@ -141,32 +141,9 @@ describe Commit do
     it { commit.sha.should start_with(subject) }
   end
 
-  describe :repo_url do
-    subject { commit_with_project.repo_url }
-
-    it { should be_a(String) }
-    it { should end_with(".git") }
-    it { should start_with(project.gitlab_url[0..6]) }
-    it { should include(project.token) }
-    it { should include('gitlab-ci-token') }
-    it { should include(project.gitlab_url[7..-1]) }
-  end
-
   describe :gitlab? do
     subject { commit_with_project.gitlab? }
 
     it { should eq(project.gitlab?) }
   end
-
-  describe :allow_git_fetch do
-    subject { commit_with_project.allow_git_fetch }
-
-    it { should eq(project.allow_git_fetch) }
-  end
-
-  describe :name do
-    subject { commit_with_project.project_name }
-
-    it { should eq(project.name) }
-  end
 end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index fd33ddd..9e44542 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -58,7 +58,7 @@ describe Project do
       end
 
       it { project.status.should == 'pending' }
-      it { project.last_build.should be_kind_of(Build)  }
+      it { project.last_commit.should be_kind_of(Commit)  }
       it { project.human_status.should == 'pending' }
     end
   end
@@ -122,29 +122,16 @@ describe Project do
     it { parsed_project.gitlab_id.should eq(189) }
     it { parsed_project.gitlab_url.should eq("http://localhost:3000/gitlab/api-gitlab-org") }
   end
-end
-
-# == Schema Information
-#
-# Table name: projects
-#
-#  id                        :integer          not null, primary key
-#  name                      :string(255)      not null
-#  timeout                   :integer          default(1800), not null
-#  scripts                   :text             default(""), not null
-#  created_at                :datetime         not null
-#  updated_at                :datetime         not null
-#  token                     :string(255)
-#  default_ref               :string(255)
-#  gitlab_url                :string(255)
-#  always_build              :boolean          default(FALSE), not null
-#  polling_interval          :integer
-#  public                    :boolean          default(FALSE), not null
-#  ssh_url_to_repo           :string(255)
-#  gitlab_id                 :integer
-#  allow_git_fetch           :boolean          default(TRUE), not null
-#  email_recipients          :string(255)
-#  email_add_committer       :boolean          default(TRUE), not null
-#  email_only_breaking_build :boolean          default(TRUE), not null
-#
 
+  describe :repo_url_with_auth do
+    let(:project) { FactoryGirl.create :project }
+    subject { project.repo_url_with_auth }
+
+    it { should be_a(String) }
+    it { should end_with(".git") }
+    it { should start_with(project.gitlab_url[0..6]) }
+    it { should include(project.token) }
+    it { should include('gitlab-ci-token') }
+    it { should include(project.gitlab_url[7..-1]) }
+  end
+end
diff --git a/spec/services/create_builds_service_spec.rb b/spec/services/create_builds_service_spec.rb
deleted file mode 100644
index 4c79988..0000000
--- a/spec/services/create_builds_service_spec.rb
+++ /dev/null
@@ -1,29 +0,0 @@
-require 'spec_helper'
-
-describe CreateBuildsService do
-  let(:service) { CreateBuildsService.new }
-  let(:project) { FactoryGirl.create(:project) }
-
-  describe :execute do
-    context 'valid params' do
-      let(:builds) { service.execute(project, ref: 'refs/heads/master', before: '00000000', after: '31das312') }
-      let(:build) { builds.first }
-
-      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(:builds) { service.execute(project, {}) }
-      let(:build) { builds.first }
-
-      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
diff --git a/spec/services/create_commit_service_spec.rb b/spec/services/create_commit_service_spec.rb
index bb2d2d9..44691cd 100644
--- a/spec/services/create_commit_service_spec.rb
+++ b/spec/services/create_commit_service_spec.rb
@@ -12,6 +12,7 @@ describe CreateCommitService do
       it { commit.should be_valid }
       it { commit.should be_persisted }
       it { commit.should == project.commits.last }
+      it { commit.builds.first.should be_kind_of(Build) }
     end
 
     context 'without params' do
diff --git a/spec/services/image_for_build_service_spec.rb b/spec/services/image_for_build_service_spec.rb
index 2c9da1f..01a48e7 100644
--- a/spec/services/image_for_build_service_spec.rb
+++ b/spec/services/image_for_build_service_spec.rb
@@ -10,6 +10,7 @@ describe ImageForBuildService do
     before { build }
 
     context 'branch name' do
+      before { build.run! }
       let(:image) { service.execute(project, ref: 'master') }
 
       it { image.should be_kind_of(OpenStruct) }
@@ -26,6 +27,7 @@ describe ImageForBuildService do
     end
 
     context 'commit sha' do
+      before { build.run! }
       let(:image) { service.execute(project, sha: build.sha) }
 
       it { image.should be_kind_of(OpenStruct) }