Merge branch 'fix/external-status-badge-links' into 'master'

Link external commit status badges to target URLs

Closes #25662

See merge request !8611

app/models/generic_commit_status.rb

 class GenericCommitStatus < CommitStatus
   before_validation :set_default_values
 
+  validates :target_url, addressable_url: true,
+                         length: { maximum: 255 },
+                         allow_nil: true
+
   # GitHub compatible API
   alias_attribute :context, :name
 
@@ -12,4 +16,10 @@ class GenericCommitStatus < CommitStatus
   def tags
     [:external]
   end
+
+  def detailed_status(current_user)
+    Gitlab::Ci::Status::External::Factory
+      .new(self, current_user)
+      .fabricate!
+  end
 end

changelogs/unreleased/fix-external-status-badge-links.yml

+---
+title: Link external build badge to its target URL
+merge_request: 8611
+author:

lib/api/commit_statuses.rb

@@ -78,6 +78,8 @@ module API
           description: params[:description]
         )
 
+        render_validation_error!(status) if status.invalid?
+
         begin
           case params[:state]
           when 'pending'

lib/gitlab/ci/status/external/common.rb

+module Gitlab
+  module Ci
+    module Status
+      module External
+        module Common
+          def has_details?
+            subject.target_url.present? &&
+              can?(user, :read_commit_status, subject)
+          end
+
+          def details_path
+            subject.target_url
+          end
+
+          def has_action?
+            false
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/ci/status/external/factory.rb

+module Gitlab
+  module Ci
+    module Status
+      module External
+        class Factory < Status::Factory
+          def self.common_helpers
+            Status::External::Common
+          end
+        end
+      end
+    end
+  end
+end

spec/features/projects/pipelines/pipeline_spec.rb

@@ -11,18 +11,42 @@ describe 'Pipeline', :feature, :js do
     project.team << [user, :developer]
   end
 
+  shared_context 'pipeline builds' do
+    let!(:build_passed) do
+      create(:ci_build, :success,
+             pipeline: pipeline, stage: 'build', name: 'build')
+    end
+
+    let!(:build_failed) do
+      create(:ci_build, :failed,
+             pipeline: pipeline, stage: 'test', name: 'test', commands: 'test')
+    end
+
+    let!(:build_running) do
+      create(:ci_build, :running,
+             pipeline: pipeline, stage: 'deploy', name: 'deploy')
+    end
+
+    let!(:build_manual) do
+      create(:ci_build, :manual,
+             pipeline: pipeline, stage: 'deploy', name: 'manual-build')
+    end
+
+    let!(:build_external) do
+      create(:generic_commit_status, status: 'success',
+                                     pipeline: pipeline,
+                                     name: 'jenkins',
+                                     stage: 'external',
+                                     target_url: 'http://gitlab.com/status')
+    end
+  end
+
   describe 'GET /:project/pipelines/:id' do
+    include_context 'pipeline builds'
+
     let(:project) { create(:project) }
     let(:pipeline) { create(:ci_pipeline, project: project, ref: 'master', sha: project.commit.id) }
 
-    before do
-      @success = create(:ci_build, :success, pipeline: pipeline, stage: 'build', name: 'build')
-      @failed = create(:ci_build, :failed, pipeline: pipeline, stage: 'test', name: 'test', commands: 'test')
-      @running = create(:ci_build, :running, pipeline: pipeline, stage: 'deploy', name: 'deploy')
-      @manual = create(:ci_build, :manual, pipeline: pipeline, stage: 'deploy', name: 'manual-build')
-      @external = create(:generic_commit_status, status: 'success', pipeline: pipeline, name: 'jenkins', stage: 'external')
-    end
-
     before { visit namespace_project_pipeline_path(project.namespace, project, pipeline) }
 
     it 'shows the pipeline graph' do
@@ -116,6 +140,7 @@ describe 'Pipeline', :feature, :js do
         it 'shows the success icon and the generic comit status build' do
           expect(page).to have_selector('.ci-status-icon-success')
           expect(page).to have_content('jenkins')
+          expect(page).to have_link('jenkins', href: 'http://gitlab.com/status')
         end
       end
     end
@@ -157,26 +182,22 @@ describe 'Pipeline', :feature, :js do
   end
 
   describe 'GET /:project/pipelines/:id/builds' do
+    include_context 'pipeline builds'
+
     let(:project) { create(:project) }
     let(:pipeline) { create(:ci_pipeline, project: project, ref: 'master', sha: project.commit.id) }
 
     before do
-      @success = create(:ci_build, :success, pipeline: pipeline, stage: 'build', name: 'build')
-      @failed = create(:ci_build, :failed, pipeline: pipeline, stage: 'test', name: 'test', commands: 'test')
-      @running = create(:ci_build, :running, pipeline: pipeline, stage: 'deploy', name: 'deploy')
-      @manual = create(:ci_build, :manual, pipeline: pipeline, stage: 'deploy', name: 'manual-build')
-      @external = create(:generic_commit_status, status: 'success', pipeline: pipeline, name: 'jenkins', stage: 'external')
+      visit builds_namespace_project_pipeline_path(project.namespace, project, pipeline)
     end
 
-    before { visit builds_namespace_project_pipeline_path(project.namespace, project, pipeline)}
-
     it 'shows a list of builds' do
       expect(page).to have_content('Test')
-      expect(page).to have_content(@success.id)
+      expect(page).to have_content(build_passed.id)
       expect(page).to have_content('Deploy')
-      expect(page).to have_content(@failed.id)
-      expect(page).to have_content(@running.id)
-      expect(page).to have_content(@external.id)
+      expect(page).to have_content(build_failed.id)
+      expect(page).to have_content(build_running.id)
+      expect(page).to have_content(build_external.id)
       expect(page).to have_content('Retry failed')
       expect(page).to have_content('Cancel running')
       expect(page).to have_link('Play')
@@ -230,7 +251,7 @@ describe 'Pipeline', :feature, :js do
         end
       end
 
-      it { expect(@manual.reload).to be_pending }
+      it { expect(build_manual.reload).to be_pending }
     end
   end
 end

spec/lib/gitlab/ci/status/external/common_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Ci::Status::External::Common do
+  let(:user) { create(:user) }
+  let(:project) { external_status.project }
+  let(:external_target_url) { 'http://example.gitlab.com/status' }
+
+  let(:external_status) do
+    create(:generic_commit_status, target_url: external_target_url)
+  end
+
+  subject do
+    Gitlab::Ci::Status::Core
+      .new(external_status, user)
+      .extend(described_class)
+  end
+
+  describe '#has_action?' do
+    it { is_expected.not_to have_action }
+  end
+
+  describe '#has_details?' do
+    context 'when user has access to read commit status' do
+      before { project.team << [user, :developer] }
+
+      it { is_expected.to have_details }
+    end
+
+    context 'when user does not have access to read commit status' do
+      it { is_expected.not_to have_details }
+    end
+  end
+
+  describe '#details_path' do
+    it 'links to the external target URL' do
+      expect(subject.details_path).to eq external_target_url
+    end
+  end
+end

spec/lib/gitlab/ci/status/external/factory_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Ci::Status::External::Factory do
+  let(:user) { create(:user) }
+  let(:project) { resource.project }
+  let(:status) { factory.fabricate! }
+  let(:factory) { described_class.new(resource, user) }
+  let(:external_url) { 'http://gitlab.com/status' }
+
+  before do
+    project.team << [user, :developer]
+  end
+
+  context 'when external status has a simple core status' do
+    HasStatus::AVAILABLE_STATUSES.each do |simple_status|
+      context "when core status is #{simple_status}" do
+        let(:resource) do
+          create(:generic_commit_status, status: simple_status,
+                                         target_url: external_url)
+        end
+
+        let(:expected_status) do
+          Gitlab::Ci::Status.const_get(simple_status.capitalize)
+        end
+
+        it "fabricates a core status #{simple_status}" do
+          expect(status).to be_a expected_status
+        end
+
+        it 'extends core status with common methods' do
+          expect(status).to have_details
+          expect(status).not_to have_action
+          expect(status.details_path).to eq external_url
+        end
+      end
+    end
+  end
+end

spec/models/generic_commit_status_spec.rb

 require 'spec_helper'
 
 describe GenericCommitStatus, models: true do
-  let(:pipeline) { create(:ci_pipeline) }
+  let(:project) { create(:empty_project) }
+  let(:pipeline) { create(:ci_pipeline, project: project) }
+  let(:external_url) { 'http://example.gitlab.com/status' }
 
   let(:generic_commit_status) do
-    create(:generic_commit_status, pipeline: pipeline)
+    create(:generic_commit_status, pipeline: pipeline,
+                                   target_url: external_url)
+  end
+
+  describe 'validations' do
+    it { is_expected.to validate_length_of(:target_url).is_at_most(255) }
+    it { is_expected.to allow_value(nil).for(:target_url) }
+    it { is_expected.to allow_value('http://gitlab.com/s').for(:target_url) }
+    it { is_expected.not_to allow_value('javascript:alert(1)').for(:target_url) }
   end
 
   describe '#context' do
@@ -22,10 +32,25 @@ describe GenericCommitStatus, models: true do
 
   describe '#detailed_status' do
     let(:user) { create(:user) }
+    let(:status) { generic_commit_status.detailed_status(user) }
 
     it 'returns detailed status object' do
-      expect(generic_commit_status.detailed_status(user))
-        .to be_a Gitlab::Ci::Status::Success
+      expect(status).to be_a Gitlab::Ci::Status::Success
+    end
+
+    context 'when user has ability to see datails' do
+      before { project.team << [user, :developer] }
+
+      it 'details path points to an external URL' do
+        expect(status).to have_details
+        expect(status.details_path).to eq external_url
+      end
+    end
+
+    context 'when user should not see details' do
+      it 'does not have details' do
+        expect(status).not_to have_details
+      end
     end
   end
 

spec/requests/api/commit_statuses_spec.rb

@@ -152,8 +152,11 @@ describe API::CommitStatuses, api: true do
 
       context 'with all optional parameters' do
         before do
-          optional_params = { state: 'success', context: 'coverage',
-                              ref: 'develop', target_url: 'url', description: 'test' }
+          optional_params = { state: 'success',
+                              context: 'coverage',
+                              ref: 'develop',
+                              description: 'test',
+                              target_url: 'http://gitlab.com/status' }
 
           post api(post_url, developer), optional_params
         end
@@ -164,12 +167,12 @@ describe API::CommitStatuses, api: true do
           expect(json_response['status']).to eq('success')
           expect(json_response['name']).to eq('coverage')
           expect(json_response['ref']).to eq('develop')
-          expect(json_response['target_url']).to eq('url')
           expect(json_response['description']).to eq('test')
+          expect(json_response['target_url']).to eq('http://gitlab.com/status')
         end
       end
 
-      context 'invalid status' do
+      context 'when status is invalid' do
         before { post api(post_url, developer), state: 'invalid' }
 
         it 'does not create commit status' do
@@ -177,7 +180,7 @@ describe API::CommitStatuses, api: true do
         end
       end
 
-      context 'request without state' do
+      context 'when request without a state made' do
         before { post api(post_url, developer) }
 
         it 'does not create commit status' do
@@ -185,7 +188,7 @@ describe API::CommitStatuses, api: true do
         end
       end
 
-      context 'invalid commit' do
+      context 'when commit SHA is invalid' do
         let(:sha) { 'invalid_sha' }
         before { post api(post_url, developer), state: 'running' }
 
@@ -193,6 +196,19 @@ describe API::CommitStatuses, api: true do
           expect(response).to have_http_status(404)
         end
       end
+
+      context 'when target URL is an invalid address' do
+        before do
+          post api(post_url, developer), state: 'pending',
+                                         target_url: 'invalid url'
+        end
+
+        it 'responds with bad request status and validation errors' do
+          expect(response).to have_http_status(400)
+          expect(json_response['message']['target_url'])
+            .to include 'must be a valid URL'
+        end
+      end
     end
 
     context 'reporter user' do