diff --git a/app/models/generic_commit_status.rb b/app/models/generic_commit_status.rb
index fa54e3540d07bd99637c76b65d7b53ba4acc58c5..8867ba0d2ff5db2300f66b37558108974cac423f 100644
--- a/app/models/generic_commit_status.rb
+++ b/app/models/generic_commit_status.rb
@@ -1,6 +1,10 @@
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
diff --git a/changelogs/unreleased/fix-external-status-badge-links.yml b/changelogs/unreleased/fix-external-status-badge-links.yml
new file mode 100644
index 0000000000000000000000000000000000000000..2287a9b76c42a4552cfe5ea0ca55b109a697da9b
--- /dev/null
+++ b/changelogs/unreleased/fix-external-status-badge-links.yml
@@ -0,0 +1,4 @@
+---
+title: Link external build badge to its target URL
+merge_request: 8611
+author:
diff --git a/lib/api/commit_statuses.rb b/lib/api/commit_statuses.rb
index 4bbdf06a49cb9e298108917494593f0622f4422c..b6e6820c3f4b41e23445c0023d657522052adcdf 100644
--- a/lib/api/commit_statuses.rb
+++ b/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'
diff --git a/lib/gitlab/ci/status/external/common.rb b/lib/gitlab/ci/status/external/common.rb
new file mode 100644
index 0000000000000000000000000000000000000000..4969a3508628bf646aabcf8173320fde2b5e4654
--- /dev/null
+++ b/lib/gitlab/ci/status/external/common.rb
@@ -0,0 +1,22 @@
+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
diff --git a/lib/gitlab/ci/status/external/factory.rb b/lib/gitlab/ci/status/external/factory.rb
new file mode 100644
index 0000000000000000000000000000000000000000..07b15bd8d97336d4d8c52250c2796f105b5b759b
--- /dev/null
+++ b/lib/gitlab/ci/status/external/factory.rb
@@ -0,0 +1,13 @@
+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
diff --git a/spec/features/projects/pipelines/pipeline_spec.rb b/spec/features/projects/pipelines/pipeline_spec.rb
index 14e009daba831d66a867053eb42bd65cecbc6005..e673ece37c3b2f6060cef18612f23fea9189aeed 100644
--- a/spec/features/projects/pipelines/pipeline_spec.rb
+++ b/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
diff --git a/spec/lib/gitlab/ci/status/external/common_spec.rb b/spec/lib/gitlab/ci/status/external/common_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..5a97d98b55f674db8cd23da51ddffcb4846fe4fd
--- /dev/null
+++ b/spec/lib/gitlab/ci/status/external/common_spec.rb
@@ -0,0 +1,39 @@
+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
diff --git a/spec/lib/gitlab/ci/status/external/factory_spec.rb b/spec/lib/gitlab/ci/status/external/factory_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..c96fd53e730cbdf729a5362197c8a4052bfdb452
--- /dev/null
+++ b/spec/lib/gitlab/ci/status/external/factory_spec.rb
@@ -0,0 +1,38 @@
+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
diff --git a/spec/models/generic_commit_status_spec.rb b/spec/models/generic_commit_status_spec.rb
index 6004bfdb7b747960da08681e0705fce223bb2282..f4c3e6d503fa827cd9af81f873ad7f0581b67163 100644
--- a/spec/models/generic_commit_status_spec.rb
+++ b/spec/models/generic_commit_status_spec.rb
@@ -1,10 +1,20 @@
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
diff --git a/spec/requests/api/commit_statuses_spec.rb b/spec/requests/api/commit_statuses_spec.rb
index 335efc4db6cf69dcbf3e6262d47419cd89b07cfa..c1c7c0882decb8fbe2fc0233fad07744b433e57e 100644
--- a/spec/requests/api/commit_statuses_spec.rb
+++ b/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