Fix review commentsfix-commit-status-rendering

- Remove unused Gitlab::Application.routes.url_helpers from Ci::Build
- Remove too much logic from a view, use Ci::Commit.matrix_builds
- Use ci_status_with_icon
- Don't describe symbols

6 files changed, 27 insertions, 25 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 6c1ca8db24f..7d33838044b 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -37,8 +37,6 @@
 
 module Ci
   class Build < CommitStatus
-    include Gitlab::Application.routes.url_helpers
-
     LAZY_ATTRIBUTES = ['trace']
 
     belongs_to :runner, class_name: 'Ci::Runner'
diff --git a/app/models/ci/commit.rb b/app/models/ci/commit.rb
index 12c60158d46..f4cf7034b14 100644
--- a/app/models/ci/commit.rb
+++ b/app/models/ci/commit.rb
@@ -113,6 +113,12 @@ module Ci
       latest_statuses.select { |status| status.ref == ref }
     end
 
+    def matrix_builds(build = nil)
+      matrix_builds = builds.latest.ordered
+      matrix_builds = matrix_builds.similar(build) if build
+      matrix_builds.to_a
+    end
+
     def retried
       @retried ||= (statuses.order(id: :desc) - statuses.latest)
     end
diff --git a/app/views/projects/builds/show.html.haml b/app/views/projects/builds/show.html.haml
index dbbf382fa2a..b02aee3db21 100644
--- a/app/views/projects/builds/show.html.haml
+++ b/app/views/projects/builds/show.html.haml
@@ -13,7 +13,7 @@
       = link_to "merge request ##{merge_request.iid}", merge_request_path(merge_request)
 
   #up-build-trace
-  - builds = @build.commit.builds.similar(@build).latest.ordered.to_a
+  - builds = @build.commit.matrix_builds(@build)
   - if builds.size > 1
     %ul.nav-links.no-top.no-bottom
       - builds.each do |build|
diff --git a/app/views/projects/ci/builds/_build.html.haml b/app/views/projects/ci/builds/_build.html.haml
index 195fd4a9d51..7123efffd5b 100644
--- a/app/views/projects/ci/builds/_build.html.haml
+++ b/app/views/projects/ci/builds/_build.html.haml
@@ -2,8 +2,7 @@
   %td.status
     - if can?(current_user, :read_build, build)
       = link_to namespace_project_build_url(build.project.namespace, build.project, build), class: "ci-status ci-#{build.status}" do
-        = ci_icon_for_status(build.status)
-        = build.status
+        = ci_status_with_icon(build.status)
     - else
       = ci_status_with_icon(build.status)
 
diff --git a/app/views/projects/generic_commit_statuses/_generic_commit_status.html.haml b/app/views/projects/generic_commit_statuses/_generic_commit_status.html.haml
index ac29f323b4c..4143ea13063 100644
--- a/app/views/projects/generic_commit_statuses/_generic_commit_status.html.haml
+++ b/app/views/projects/generic_commit_statuses/_generic_commit_status.html.haml
@@ -2,8 +2,7 @@
   %td.status
     - if can?(current_user, :read_commit_status, generic_commit_status) && generic_commit_status.target_url
       = link_to generic_commit_status.target_url, class: "ci-status ci-#{generic_commit_status.status}" do
-        = ci_icon_for_status(generic_commit_status.status)
-        = generic_commit_status.status
+        = ci_status_with_icon(generic_commit_status.status)
     - else
       = ci_status_with_icon(generic_commit_status.status)
 
diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb
index f4f817297b9..b7457808040 100644
--- a/spec/models/build_spec.rb
+++ b/spec/models/build_spec.rb
@@ -9,7 +9,7 @@ describe Ci::Build, models: true do
 
   it { is_expected.to respond_to :trace_html }
 
-  describe :first_pending do
+  describe '#first_pending' do
     let(:first) { FactoryGirl.create :ci_build, commit: commit, status: 'pending', created_at: Date.yesterday }
     let(:second) { FactoryGirl.create :ci_build, commit: commit, status: 'pending' }
     before { first; second }
@@ -19,7 +19,7 @@ describe Ci::Build, models: true do
     it('returns with the first pending build') { is_expected.to eq(first) }
   end
 
-  describe :create_from do
+  describe '#create_from' do
     before do
       build.status = 'success'
       build.save
@@ -33,7 +33,7 @@ describe Ci::Build, models: true do
     end
   end
 
-  describe :ignored? do
+  describe '#ignored?' do
     subject { build.ignored? }
 
     context 'if build is not allowed to fail' do
@@ -69,7 +69,7 @@ describe Ci::Build, models: true do
     end
   end
 
-  describe :trace do
+  describe '#trace' do
     subject { build.trace_html }
 
     it { is_expected.to be_empty }
@@ -101,7 +101,7 @@ describe Ci::Build, models: true do
   #   it { is_expected.to eq(commit.project.timeout) }
   # end
 
-  describe :options do
+  describe '#options' do
     let(:options) do
       {
         image: "ruby:2.1",
@@ -122,25 +122,25 @@ describe Ci::Build, models: true do
   #   it { is_expected.to eq(project.allow_git_fetch) }
   # end
 
-  describe :project do
+  describe '#project' do
     subject { build.project }
 
     it { is_expected.to eq(commit.project) }
   end
 
-  describe :project_id do
+  describe '#project_id' do
     subject { build.project_id }
 
     it { is_expected.to eq(commit.project_id) }
   end
 
-  describe :project_name do
+  describe '#project_name' do
     subject { build.project_name }
 
     it { is_expected.to eq(project.name) }
   end
 
-  describe :extract_coverage do
+  describe '#extract_coverage' do
     context 'valid content & regex' do
       subject { build.extract_coverage('Coverage 1033 / 1051 LOC (98.29%) covered', '\(\d+.\d+\%\) covered') }
 
@@ -172,7 +172,7 @@ describe Ci::Build, models: true do
     end
   end
 
-  describe :variables do
+  describe '#variables' do
     context 'returns variables' do
       subject { build.variables }
 
@@ -242,7 +242,7 @@ describe Ci::Build, models: true do
     end
   end
 
-  describe :can_be_served? do
+  describe '#can_be_served?' do
     let(:runner) { FactoryGirl.create :ci_runner }
 
     before { build.project.runners << runner }
@@ -277,7 +277,7 @@ describe Ci::Build, models: true do
     end
   end
 
-  describe :any_runners_online? do
+  describe '#any_runners_online?' do
     subject { build.any_runners_online? }
 
     context 'when no runners' do
@@ -312,7 +312,7 @@ describe Ci::Build, models: true do
     end
   end
 
-  describe :stuck? do
+  describe '#stuck?' do
     subject { build.stuck? }
 
     %w(pending).each do |state|
@@ -343,7 +343,7 @@ describe Ci::Build, models: true do
     end
   end
 
-  describe :artifacts? do
+  describe '#artifacts?' do
     subject { build.artifacts? }
 
     context 'artifacts archive does not exist' do
@@ -358,7 +358,7 @@ describe Ci::Build, models: true do
   end
 
 
-  describe :artifacts_metadata? do
+  describe '#artifacts_metadata?' do
     subject { build.artifacts_metadata? }
     context 'artifacts metadata does not exist' do
       it { is_expected.to be_falsy }
@@ -370,7 +370,7 @@ describe Ci::Build, models: true do
     end
   end
 
-  describe :repo_url do
+  describe '#repo_url' do
     let(:build) { FactoryGirl.create :ci_build }
     let(:project) { build.project }
 
@@ -384,7 +384,7 @@ describe Ci::Build, models: true do
     it { is_expected.to include(project.web_url[7..-1]) }
   end
 
-  describe :depends_on_builds do
+  describe '#depends_on_builds' do
     let!(:build) { FactoryGirl.create :ci_build, commit: commit, name: 'build', stage_idx: 0, stage: 'build' }
     let!(:rspec_test) { FactoryGirl.create :ci_build, commit: commit, name: 'rspec', stage_idx: 1, stage: 'test' }
     let!(:rubocop_test) { FactoryGirl.create :ci_build, commit: commit, name: 'rubocop', stage_idx: 1, stage: 'test' }
@@ -416,7 +416,7 @@ describe Ci::Build, models: true do
                        created_at: created_at)
   end
 
-  describe :merge_request do
+  describe '#merge_request' do
     context 'when a MR has a reference to the commit' do
       before do
         @merge_request = create_mr(build, commit, factory: :merge_request)