From ee592f0df36aba995d04a969697fb7a064b6ef3c Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 6 Apr 2017 11:37:23 +0200 Subject: Improve specs related to CI/CD job environment --- spec/models/ci/build_spec.rb | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 8dbcf50ee0c..ae390afbd80 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -902,22 +902,26 @@ describe Ci::Build, :models do end describe '#persisted_environment' do - before do - @environment = create(:environment, project: project, name: "foo-#{project.default_branch}") + let!(:environment) do + create(:environment, project: project, name: "foo-#{project.default_branch}") end subject { build.persisted_environment } - context 'referenced literally' do - let(:build) { create(:ci_build, pipeline: pipeline, environment: "foo-#{project.default_branch}") } + context 'when referenced literally' do + let(:build) do + create(:ci_build, pipeline: pipeline, environment: "foo-#{project.default_branch}") + end - it { is_expected.to eq(@environment) } + it { is_expected.to eq(environment) } end - context 'referenced with a variable' do - let(:build) { create(:ci_build, pipeline: pipeline, environment: "foo-$CI_COMMIT_REF_NAME") } + context 'when referenced with a variable' do + let(:build) do + create(:ci_build, pipeline: pipeline, environment: "foo-$CI_COMMIT_REF_NAME") + end - it { is_expected.to eq(@environment) } + it { is_expected.to eq(environment) } end end -- cgit v1.2.3 From 5525db8b33bf1f76c4a8023ea4fb571d73e41b0f Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 6 Apr 2017 11:52:37 +0200 Subject: Check branch access when user triggers manual action --- app/models/ci/build.rb | 10 +++++++ spec/models/ci/build_spec.rb | 65 ++++++++++++++++++++++++++++++++++++-------- 2 files changed, 63 insertions(+), 12 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 8431c5f228c..159b3b2e101 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -115,7 +115,17 @@ module Ci commands.present? end + def can_play?(current_user) + ::Gitlab::UserAccess + .new(current_user, project: project) + .can_push_to_branch?(ref) + end + def play(current_user) + unless can_play?(current_user) + raise Gitlab::Access::AccessDeniedError + end + # Try to queue a current build if self.enqueue self.update(user: current_user) diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index ae390afbd80..8124d263fd4 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -925,6 +925,33 @@ describe Ci::Build, :models do end end + describe '#can_play?' do + before do + project.add_developer(user) + end + + let(:build) do + create(:ci_build, ref: 'some-ref', pipeline: pipeline) + end + + context 'when branch build is running for is protected' do + before do + create(:protected_branch, :no_one_can_push, + name: 'some-ref', project: project) + end + + it 'indicates that user can not trigger an action' do + expect(build.can_play?(user)).to be_falsey + end + end + + context 'when branch build is running for is not protected' do + it 'indicates that user can trigger an action' do + expect(build.can_play?(user)).to be_truthy + end + end + end + describe '#play' do let(:build) { create(:ci_build, :manual, pipeline: pipeline) } @@ -932,25 +959,39 @@ describe Ci::Build, :models do project.add_developer(user) end - context 'when build is manual' do - it 'enqueues a build' do - new_build = build.play(user) + context 'when user does not have ability to trigger action' do + before do + create(:protected_branch, :no_one_can_push, + name: build.ref, project: project) + end - expect(new_build).to be_pending - expect(new_build).to eq(build) + it 'raises an error' do + expect { build.play(user) } + .to raise_error Gitlab::Access::AccessDeniedError end end - context 'when build is passed' do - before do - build.update(status: 'success') + context 'when user has ability to trigger manual action' do + context 'when build is manual' do + it 'enqueues a build' do + new_build = build.play(user) + + expect(new_build).to be_pending + expect(new_build).to eq(build) + end end - it 'creates a new build' do - new_build = build.play(user) + context 'when build is not manual' do + before do + build.update(status: 'success') + end + + it 'creates a new build' do + new_build = build.play(user) - expect(new_build).to be_pending - expect(new_build).not_to eq(build) + expect(new_build).to be_pending + expect(new_build).not_to eq(build) + end end end end -- cgit v1.2.3 From 3dbef510c187606ef12eb9f2aaf51f30ddc3e30d Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 6 Apr 2017 12:58:13 +0200 Subject: Expose manual action abilities from a build entity --- app/serializers/build_entity.rb | 10 +++++++--- spec/serializers/build_entity_spec.rb | 28 ++++++++++++++++++++++++++-- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/app/serializers/build_entity.rb b/app/serializers/build_entity.rb index b804d6d0e8a..401a277dadc 100644 --- a/app/serializers/build_entity.rb +++ b/app/serializers/build_entity.rb @@ -12,7 +12,7 @@ class BuildEntity < Grape::Entity path_to(:retry_namespace_project_build, build) end - expose :play_path, if: ->(build, _) { build.playable? } do |build| + expose :play_path, if: proc { playable? } do |build| path_to(:play_namespace_project_build, build) end @@ -25,11 +25,15 @@ class BuildEntity < Grape::Entity alias_method :build, :object - def path_to(route, build) - send("#{route}_path", build.project.namespace, build.project, build) + def playable? + build.playable? && build.can_play?(request.user) end def detailed_status build.detailed_status(request.user) end + + def path_to(route, build) + send("#{route}_path", build.project.namespace, build.project, build) + end end diff --git a/spec/serializers/build_entity_spec.rb b/spec/serializers/build_entity_spec.rb index f76a5cf72d1..897a28b7305 100644 --- a/spec/serializers/build_entity_spec.rb +++ b/spec/serializers/build_entity_spec.rb @@ -41,13 +41,37 @@ describe BuildEntity do it 'does not contain path to play action' do expect(subject).not_to include(:play_path) end + + it 'is not a playable job' do + expect(subject[:playable]).to be false + end end context 'when build is a manual action' do let(:build) { create(:ci_build, :manual) } - it 'contains path to play action' do - expect(subject).to include(:play_path) + context 'when user is allowed to trigger action' do + before do + build.project.add_master(user) + end + + it 'contains path to play action' do + expect(subject).to include(:play_path) + end + + it 'is a playable action' do + expect(subject[:playable]).to be true + end + end + + context 'when user is not allowed to trigger action' do + it 'does not contain path to play action' do + expect(subject).not_to include(:play_path) + end + + it 'is not a playable action' do + expect(subject[:playable]).to be false + end end end end -- cgit v1.2.3 From 5b6202cce1cf7ad5da1fcbfc4e089194311f205b Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 6 Apr 2017 13:08:45 +0200 Subject: Do not show play action if user not allowed to run it --- lib/gitlab/ci/status/build/play.rb | 2 +- spec/lib/gitlab/ci/status/build/play_spec.rb | 12 ++++++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/ci/status/build/play.rb b/lib/gitlab/ci/status/build/play.rb index 3495b8d0448..29d0558a265 100644 --- a/lib/gitlab/ci/status/build/play.rb +++ b/lib/gitlab/ci/status/build/play.rb @@ -10,7 +10,7 @@ module Gitlab end def has_action? - can?(user, :update_build, subject) + can?(user, :update_build, subject) && subject.can_play?(user) end def action_icon diff --git a/spec/lib/gitlab/ci/status/build/play_spec.rb b/spec/lib/gitlab/ci/status/build/play_spec.rb index 6c97a4fe5ca..48aeb89e11f 100644 --- a/spec/lib/gitlab/ci/status/build/play_spec.rb +++ b/spec/lib/gitlab/ci/status/build/play_spec.rb @@ -17,9 +17,17 @@ describe Gitlab::Ci::Status::Build::Play do describe '#has_action?' do context 'when user is allowed to update build' do - before { build.project.team << [user, :developer] } + context 'when user can push to branch' do + before { build.project.add_master(user) } - it { is_expected.to have_action } + it { is_expected.to have_action } + end + + context 'when user can not push to the branch' do + before { build.project.add_developer(user) } + + it { is_expected.not_to have_action } + end end context 'when user is not allowed to update build' do -- cgit v1.2.3 From f647ad8f6fd076c6adcc3876b5e4a521a49c0ca8 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 6 Apr 2017 13:40:04 +0200 Subject: Fix chatops specs with incorrectly defined project --- spec/lib/gitlab/chat_commands/command_spec.rb | 14 ++++++++++---- spec/lib/gitlab/chat_commands/deploy_spec.rb | 16 ++++++++++------ 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/spec/lib/gitlab/chat_commands/command_spec.rb b/spec/lib/gitlab/chat_commands/command_spec.rb index b6e924d67be..eb4f06b371c 100644 --- a/spec/lib/gitlab/chat_commands/command_spec.rb +++ b/spec/lib/gitlab/chat_commands/command_spec.rb @@ -40,11 +40,15 @@ describe Gitlab::ChatCommands::Command, service: true do context 'when trying to do deployment' do let(:params) { { text: 'deploy staging to production' } } - let!(:build) { create(:ci_build, project: project) } + let!(:build) { create(:ci_build, pipeline: pipeline) } + let!(:pipeline) { create(:ci_pipeline, project: project) } let!(:staging) { create(:environment, name: 'staging', project: project) } let!(:deployment) { create(:deployment, environment: staging, deployable: build) } + let!(:manual) do - create(:ci_build, :manual, project: project, pipeline: build.pipeline, name: 'first', environment: 'production') + create(:ci_build, :manual, pipeline: pipeline, + name: 'first', + environment: 'production') end context 'and user can not create deployment' do @@ -56,7 +60,7 @@ describe Gitlab::ChatCommands::Command, service: true do context 'and user does have deployment permission' do before do - project.team << [user, :developer] + build.project.add_master(user) end it 'returns action' do @@ -66,7 +70,9 @@ describe Gitlab::ChatCommands::Command, service: true do context 'when duplicate action exists' do let!(:manual2) do - create(:ci_build, :manual, project: project, pipeline: build.pipeline, name: 'second', environment: 'production') + create(:ci_build, :manual, pipeline: pipeline, + name: 'second', + environment: 'production') end it 'returns error' do diff --git a/spec/lib/gitlab/chat_commands/deploy_spec.rb b/spec/lib/gitlab/chat_commands/deploy_spec.rb index b3358a32161..b33389d959e 100644 --- a/spec/lib/gitlab/chat_commands/deploy_spec.rb +++ b/spec/lib/gitlab/chat_commands/deploy_spec.rb @@ -7,7 +7,7 @@ describe Gitlab::ChatCommands::Deploy, service: true do let(:regex_match) { described_class.match('deploy staging to production') } before do - project.team << [user, :master] + project.add_master(user) end subject do @@ -23,7 +23,8 @@ describe Gitlab::ChatCommands::Deploy, service: true do context 'with environment' do let!(:staging) { create(:environment, name: 'staging', project: project) } - let!(:build) { create(:ci_build, project: project) } + let!(:pipeline) { create(:ci_pipeline, project: project) } + let!(:build) { create(:ci_build, pipeline: pipeline) } let!(:deployment) { create(:deployment, environment: staging, deployable: build) } context 'without actions' do @@ -35,7 +36,9 @@ describe Gitlab::ChatCommands::Deploy, service: true do context 'with action' do let!(:manual1) do - create(:ci_build, :manual, project: project, pipeline: build.pipeline, name: 'first', environment: 'production') + create(:ci_build, :manual, pipeline: pipeline, + name: 'first', + environment: 'production') end it 'returns success result' do @@ -45,7 +48,9 @@ describe Gitlab::ChatCommands::Deploy, service: true do context 'when duplicate action exists' do let!(:manual2) do - create(:ci_build, :manual, project: project, pipeline: build.pipeline, name: 'second', environment: 'production') + create(:ci_build, :manual, pipeline: pipeline, + name: 'second', + environment: 'production') end it 'returns error' do @@ -57,8 +62,7 @@ describe Gitlab::ChatCommands::Deploy, service: true do context 'when teardown action exists' do let!(:teardown) do create(:ci_build, :manual, :teardown_environment, - project: project, pipeline: build.pipeline, - name: 'teardown', environment: 'production') + pipeline: pipeline, name: 'teardown', environment: 'production') end it 'returns the success message' do -- cgit v1.2.3 From e533d43a8c03a9b47a7016f3fea01a00ca797778 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 6 Apr 2017 14:28:40 +0200 Subject: Fix specs related to new manual actions permissions --- spec/factories/environments.rb | 4 +- .../projects/environments/environment_spec.rb | 4 ++ spec/models/environment_spec.rb | 53 ++++++++++++++++------ spec/services/ci/process_pipeline_service_spec.rb | 7 +++ 4 files changed, 54 insertions(+), 14 deletions(-) diff --git a/spec/factories/environments.rb b/spec/factories/environments.rb index 3fbf24b5c7d..f6595751d1e 100644 --- a/spec/factories/environments.rb +++ b/spec/factories/environments.rb @@ -18,6 +18,8 @@ FactoryGirl.define do # interconnected objects to simulate a review app. # after(:create) do |environment, evaluator| + pipeline = create(:ci_pipeline, project: environment.project) + deployment = create(:deployment, environment: environment, project: environment.project, @@ -26,7 +28,7 @@ FactoryGirl.define do teardown_build = create(:ci_build, :manual, name: "#{deployment.environment.name}:teardown", - pipeline: deployment.deployable.pipeline) + pipeline: pipeline) deployment.update_column(:on_stop, teardown_build.name) environment.update_attribute(:deployments, [deployment]) diff --git a/spec/features/projects/environments/environment_spec.rb b/spec/features/projects/environments/environment_spec.rb index acc3efe04e6..2b7f67eee32 100644 --- a/spec/features/projects/environments/environment_spec.rb +++ b/spec/features/projects/environments/environment_spec.rb @@ -62,6 +62,8 @@ feature 'Environment', :feature do name: 'deploy to production') end + given(:role) { :master } + scenario 'does show a play button' do expect(page).to have_link(action.name.humanize) end @@ -132,6 +134,8 @@ feature 'Environment', :feature do on_stop: 'close_app') end + given(:role) { :master } + scenario 'does allow to stop environment' do click_link('Stop') diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index 9f0e7fbbe26..9e00f2247e8 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -191,25 +191,52 @@ describe Environment, models: true do end context 'when matching action is defined' do - let(:build) { create(:ci_build) } - let!(:deployment) { create(:deployment, environment: environment, deployable: build, on_stop: 'close_app') } + let(:pipeline) { create(:ci_pipeline, project: project) } + let(:build) { create(:ci_build, pipeline: pipeline) } + + let!(:deployment) do + create(:deployment, environment: environment, + deployable: build, + on_stop: 'close_app') + end - context 'when action did not yet finish' do - let!(:close_action) { create(:ci_build, :manual, pipeline: build.pipeline, name: 'close_app') } + context 'when user is not allowed to stop environment' do + let!(:close_action) do + create(:ci_build, :manual, pipeline: pipeline, name: 'close_app') + end - it 'returns the same action' do - expect(subject).to eq(close_action) - expect(subject.user).to eq(user) + it 'raises an exception' do + expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError) end end - context 'if action did finish' do - let!(:close_action) { create(:ci_build, :manual, :success, pipeline: build.pipeline, name: 'close_app') } + context 'when user is allowed to stop environment' do + before do + project.add_master(user) + end + + context 'when action did not yet finish' do + let!(:close_action) do + create(:ci_build, :manual, pipeline: pipeline, name: 'close_app') + end + + it 'returns the same action' do + expect(subject).to eq(close_action) + expect(subject.user).to eq(user) + end + end - it 'returns a new action of the same type' do - is_expected.to be_persisted - expect(subject.name).to eq(close_action.name) - expect(subject.user).to eq(user) + context 'if action did finish' do + let!(:close_action) do + create(:ci_build, :manual, :success, + pipeline: pipeline, name: 'close_app') + end + + it 'returns a new action of the same type' do + expect(subject).to be_persisted + expect(subject.name).to eq(close_action.name) + expect(subject.user).to eq(user) + end end end end diff --git a/spec/services/ci/process_pipeline_service_spec.rb b/spec/services/ci/process_pipeline_service_spec.rb index bb98fb37a90..7df6a81b0ab 100644 --- a/spec/services/ci/process_pipeline_service_spec.rb +++ b/spec/services/ci/process_pipeline_service_spec.rb @@ -314,6 +314,13 @@ describe Ci::ProcessPipelineService, '#execute', :services do end context 'when pipeline is promoted sequentially up to the end' do + before do + # We are using create(:empty_project), and users has to be master in + # order to execute manual action when repository does not exist. + # + project.add_master(user) + end + it 'properly processes entire pipeline' do process_pipeline -- cgit v1.2.3 From 6c0fc62ef5c4fa4535174a9f187b9853f0fb90ac Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 6 Apr 2017 15:19:52 +0200 Subject: Take branch access into account when stopping environment --- app/models/deployment.rb | 4 ++-- app/models/environment.rb | 6 ++++++ app/services/ci/stop_environments_service.rb | 8 ++++--- spec/factories/environments.rb | 6 +++++- spec/models/environment_spec.rb | 25 ++++++++++++++++++++++ spec/services/ci/stop_environments_service_spec.rb | 16 +++++++++++++- 6 files changed, 58 insertions(+), 7 deletions(-) diff --git a/app/models/deployment.rb b/app/models/deployment.rb index afad001d50f..37adfb4de73 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -85,8 +85,8 @@ class Deployment < ActiveRecord::Base end def stop_action - return nil unless on_stop.present? - return nil unless manual_actions + return unless on_stop.present? + return unless manual_actions @stop_action ||= manual_actions.find_by(name: on_stop) end diff --git a/app/models/environment.rb b/app/models/environment.rb index bf33010fd21..f8b9a21c08e 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -122,6 +122,12 @@ class Environment < ActiveRecord::Base available? && stop_action.present? end + def can_trigger_stop_action?(current_user) + return false unless stop_action? + + stop_action.can_play?(current_user) + end + def stop_with_action!(current_user) return unless available? diff --git a/app/services/ci/stop_environments_service.rb b/app/services/ci/stop_environments_service.rb index 42c72aba7dd..d1e341bc9b5 100644 --- a/app/services/ci/stop_environments_service.rb +++ b/app/services/ci/stop_environments_service.rb @@ -6,9 +6,10 @@ module Ci @ref = branch_name return unless has_ref? + return unless can?(current_user, :create_deployment, project) environments.each do |environment| - next unless can?(current_user, :create_deployment, project) + next unless environment.can_trigger_stop_action?(current_user) environment.stop_with_action!(current_user) end @@ -21,8 +22,9 @@ module Ci end def environments - @environments ||= - EnvironmentsFinder.new(project, current_user, ref: @ref, recently_updated: true).execute + @environments ||= EnvironmentsFinder + .new(project, current_user, ref: @ref, recently_updated: true) + .execute end end end diff --git a/spec/factories/environments.rb b/spec/factories/environments.rb index f6595751d1e..d8d699fb3aa 100644 --- a/spec/factories/environments.rb +++ b/spec/factories/environments.rb @@ -20,14 +20,18 @@ FactoryGirl.define do after(:create) do |environment, evaluator| pipeline = create(:ci_pipeline, project: environment.project) + deployable = create(:ci_build, name: "#{environment.name}:deploy", + pipeline: pipeline) + deployment = create(:deployment, environment: environment, project: environment.project, + deployable: deployable, ref: evaluator.ref, sha: environment.project.commit(evaluator.ref).id) teardown_build = create(:ci_build, :manual, - name: "#{deployment.environment.name}:teardown", + name: "#{environment.name}:teardown", pipeline: pipeline) deployment.update_column(:on_stop, teardown_build.name) diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index 9e00f2247e8..fb7cee47ae8 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -155,6 +155,31 @@ describe Environment, models: true do end end + describe '#can_trigger_stop_action?' do + let(:user) { create(:user) } + let(:project) { create(:project) } + + let(:environment) do + create(:environment, :with_review_app, project: project) + end + + context 'when user can trigger stop action' do + before do + project.add_developer(user) + end + + it 'returns value that evaluates to true' do + expect(environment.can_trigger_stop_action?(user)).to be_truthy + end + end + + context 'when user is not allowed to trigger stop action' do + it 'returns value that evaluates to false' do + expect(environment.can_trigger_stop_action?(user)).to be_falsey + end + end + end + describe '#stop_with_action!' do let(:user) { create(:admin) } diff --git a/spec/services/ci/stop_environments_service_spec.rb b/spec/services/ci/stop_environments_service_spec.rb index 32c72a9cf5e..98044ad232e 100644 --- a/spec/services/ci/stop_environments_service_spec.rb +++ b/spec/services/ci/stop_environments_service_spec.rb @@ -55,8 +55,22 @@ describe Ci::StopEnvironmentsService, services: true do end context 'when user does not have permission to stop environment' do + context 'when user has no access to manage deployments' do + before do + project.team << [user, :guest] + end + + it 'does not stop environment' do + expect_environment_not_stopped_on('master') + end + end + end + + context 'when branch for stop action is protected' do before do - project.team << [user, :guest] + project.add_developer(user) + create(:protected_branch, :no_one_can_push, + name: 'master', project: project) end it 'does not stop environment' do -- cgit v1.2.3 From d96e90734783a83ca4d87b961699072712077d9f Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 6 Apr 2017 15:24:48 +0200 Subject: Fix specs for build status factory and manual status --- spec/lib/gitlab/ci/status/build/factory_spec.rb | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/spec/lib/gitlab/ci/status/build/factory_spec.rb b/spec/lib/gitlab/ci/status/build/factory_spec.rb index e648a3ac3a2..2ab67127b1e 100644 --- a/spec/lib/gitlab/ci/status/build/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/build/factory_spec.rb @@ -218,9 +218,24 @@ describe Gitlab::Ci::Status::Build::Factory do expect(status.favicon).to eq 'favicon_status_manual' expect(status.label).to eq 'manual play action' expect(status).to have_details - expect(status).to have_action expect(status.action_path).to include 'play' end + + context 'when user has ability to play action' do + before do + build.project.add_master(user) + end + + it 'fabricates status that has action' do + expect(status).to have_action + end + end + + context 'when user does not have ability to play action' do + it 'fabricates status that has no action' do + expect(status).not_to have_action + end + end end context 'when build is an environment stop action' do -- cgit v1.2.3 From 3e55e0742218fe20b269dfbc4f44d3798e4b0eb6 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 6 Apr 2017 15:32:23 +0200 Subject: Check if user can trigger manual action in the UI --- app/views/projects/ci/builds/_build.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/ci/builds/_build.html.haml b/app/views/projects/ci/builds/_build.html.haml index aeed293a724..ceec36e2440 100644 --- a/app/views/projects/ci/builds/_build.html.haml +++ b/app/views/projects/ci/builds/_build.html.haml @@ -101,7 +101,7 @@ = link_to cancel_namespace_project_build_path(build.project.namespace, build.project, build, return_to: request.original_url), method: :post, title: 'Cancel', class: 'btn btn-build' do = icon('remove', class: 'cred') - elsif allow_retry - - if build.playable? && !admin + - if build.playable? && !admin && build.can_play?(current_user) = link_to play_namespace_project_build_path(build.project.namespace, build.project, build, return_to: request.original_url), method: :post, title: 'Play', class: 'btn btn-build' do = custom_icon('icon_play') - elsif build.retryable? -- cgit v1.2.3 From 7bcca2284b09e18438e6163c6ead72e10fdd2f57 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 6 Apr 2017 17:16:19 +0200 Subject: Check branch permission in manual action entity --- app/serializers/build_action_entity.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/app/serializers/build_action_entity.rb b/app/serializers/build_action_entity.rb index 184b4b7a681..3d12c64b88a 100644 --- a/app/serializers/build_action_entity.rb +++ b/app/serializers/build_action_entity.rb @@ -13,4 +13,12 @@ class BuildActionEntity < Grape::Entity end expose :playable?, as: :playable + + private + + alias_method :build, :object + + def playable? + build.playable? && build.can_play?(request.user) + end end -- cgit v1.2.3 From b09465f38d66d7ff6074843177bcdb7d72caf07f Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 12 Apr 2017 11:26:18 +0200 Subject: Implement new rule for manual actions in policies --- app/policies/ci/build_policy.rb | 14 +++++++++ spec/policies/ci/build_policy_spec.rb | 53 +++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/app/policies/ci/build_policy.rb b/app/policies/ci/build_policy.rb index 8b25332b73c..0522cbdb331 100644 --- a/app/policies/ci/build_policy.rb +++ b/app/policies/ci/build_policy.rb @@ -8,6 +8,20 @@ module Ci %w[read create update admin].each do |rule| cannot! :"#{rule}_commit_status" unless can? :"#{rule}_build" end + + can! :play_build if can_play_action? + end + + private + + alias_method :build, :subject + + def can_play_action? + return false unless build.playable? + + ::Gitlab::UserAccess + .new(user, project: build.project) + .can_push_to_branch?(build.ref) end end end diff --git a/spec/policies/ci/build_policy_spec.rb b/spec/policies/ci/build_policy_spec.rb index 0f280f32eac..e4693cdcef0 100644 --- a/spec/policies/ci/build_policy_spec.rb +++ b/spec/policies/ci/build_policy_spec.rb @@ -89,5 +89,58 @@ describe Ci::BuildPolicy, :models do end end end + + describe 'rules for manual actions' do + let(:project) { create(:project) } + + before do + project.add_developer(user) + end + + context 'when branch build is assigned to is protected' do + before do + create(:protected_branch, :no_one_can_push, + name: 'some-ref', project: project) + end + + context 'when build is a manual action' do + let(:build) do + create(:ci_build, :manual, ref: 'some-ref', pipeline: pipeline) + end + + it 'does not include ability to play build' do + expect(policies).not_to include :play_build + end + end + + context 'when build is not a manual action' do + let(:build) do + create(:ci_build, ref: 'some-ref', pipeline: pipeline) + end + + it 'does not include ability to play build' do + expect(policies).not_to include :play_build + end + end + end + + context 'when branch build is assigned to is not protected' do + context 'when build is a manual action' do + let(:build) { create(:ci_build, :manual, pipeline: pipeline) } + + it 'includes ability to play build' do + expect(policies).to include :play_build + end + end + + context 'when build is not a manual action' do + let(:build) { create(:ci_build, pipeline: pipeline) } + + it 'does not include ability to play build' do + expect(policies).not_to include :play_build + end + end + end + end end end -- cgit v1.2.3 From 6c6bc400d1d8a96f6e443788cd0b2c14addd88e3 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 12 Apr 2017 11:46:24 +0200 Subject: Move code for playing an action to separate service --- app/models/ci/build.rb | 15 +++------------ app/policies/ci/build_policy.rb | 2 +- app/services/ci/play_build_service.rb | 17 +++++++++++++++++ 3 files changed, 21 insertions(+), 13 deletions(-) create mode 100644 app/services/ci/play_build_service.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 159b3b2e101..9edc4cd96b9 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -122,18 +122,9 @@ module Ci end def play(current_user) - unless can_play?(current_user) - raise Gitlab::Access::AccessDeniedError - end - - # Try to queue a current build - if self.enqueue - self.update(user: current_user) - self - else - # Otherwise we need to create a duplicate - Ci::Build.retry(self, current_user) - end + Ci::PlayBuildService + .new(project, current_user) + .execute(self) end def cancelable? diff --git a/app/policies/ci/build_policy.rb b/app/policies/ci/build_policy.rb index 0522cbdb331..2c39d31488f 100644 --- a/app/policies/ci/build_policy.rb +++ b/app/policies/ci/build_policy.rb @@ -17,7 +17,7 @@ module Ci alias_method :build, :subject def can_play_action? - return false unless build.playable? + return false unless build.action? ::Gitlab::UserAccess .new(user, project: build.project) diff --git a/app/services/ci/play_build_service.rb b/app/services/ci/play_build_service.rb new file mode 100644 index 00000000000..c9ed45408f2 --- /dev/null +++ b/app/services/ci/play_build_service.rb @@ -0,0 +1,17 @@ +module Ci + class PlayBuildService < ::BaseService + def execute(build) + unless can?(current_user, :play_build, build) + raise Gitlab::Access::AccessDeniedError + end + + # Try to enqueue thebuild, otherwise create a duplicate. + # + if build.enqueue + build.tap { |action| action.update(user: current_user) } + else + Ci::Build.retry(build, current_user) + end + end + end +end -- cgit v1.2.3 From 7fc6b5b6ff23e2faba7f06a1362ada31f6f3436a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 12 Apr 2017 12:19:39 +0200 Subject: Do not inherit build policy in pipeline policy --- app/policies/base_policy.rb | 4 ++++ app/policies/ci/pipeline_policy.rb | 5 ++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/policies/base_policy.rb b/app/policies/base_policy.rb index 8890409d056..623424c63e0 100644 --- a/app/policies/base_policy.rb +++ b/app/policies/base_policy.rb @@ -97,6 +97,10 @@ class BasePolicy rules end + def rules + raise NotImplementedError + end + def delegate!(new_subject) @rule_set.merge(Ability.allowed(@user, new_subject)) end diff --git a/app/policies/ci/pipeline_policy.rb b/app/policies/ci/pipeline_policy.rb index 3d2eef1c50c..10aa2d3e72a 100644 --- a/app/policies/ci/pipeline_policy.rb +++ b/app/policies/ci/pipeline_policy.rb @@ -1,4 +1,7 @@ module Ci - class PipelinePolicy < BuildPolicy + class PipelinePolicy < BasePolicy + def rules + delegate! @subject.project + end end end -- cgit v1.2.3 From 55aa727eff50a9472405b302645abb54f28bdba0 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 12 Apr 2017 13:36:35 +0200 Subject: Extract build play specs and extend test examples --- spec/models/ci/build_spec.rb | 36 +--------- spec/services/ci/play_build_service_spec.rb | 105 ++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 34 deletions(-) create mode 100644 spec/services/ci/play_build_service_spec.rb diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 8124d263fd4..09e9f5bd8ba 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -959,40 +959,8 @@ describe Ci::Build, :models do project.add_developer(user) end - context 'when user does not have ability to trigger action' do - before do - create(:protected_branch, :no_one_can_push, - name: build.ref, project: project) - end - - it 'raises an error' do - expect { build.play(user) } - .to raise_error Gitlab::Access::AccessDeniedError - end - end - - context 'when user has ability to trigger manual action' do - context 'when build is manual' do - it 'enqueues a build' do - new_build = build.play(user) - - expect(new_build).to be_pending - expect(new_build).to eq(build) - end - end - - context 'when build is not manual' do - before do - build.update(status: 'success') - end - - it 'creates a new build' do - new_build = build.play(user) - - expect(new_build).to be_pending - expect(new_build).not_to eq(build) - end - end + it 'enqueues the build' do + expect(build.play(user)).to be_pending end end diff --git a/spec/services/ci/play_build_service_spec.rb b/spec/services/ci/play_build_service_spec.rb new file mode 100644 index 00000000000..d6f9fa42045 --- /dev/null +++ b/spec/services/ci/play_build_service_spec.rb @@ -0,0 +1,105 @@ +require 'spec_helper' + +describe Ci::PlayBuildService, '#execute', :services do + let(:user) { create(:user) } + let(:project) { create(:empty_project) } + let(:pipeline) { create(:ci_pipeline, project: project) } + let(:build) { create(:ci_build, :manual, pipeline: pipeline) } + + let(:service) do + described_class.new(project, user) + end + + context 'when project does not have repository yet' do + let(:project) { create(:empty_project) } + + it 'allows user with master role to play build' do + project.add_master(user) + + service.execute(build) + + expect(build.reload).to be_pending + end + + it 'does not allow user with developer role to play build' do + project.add_developer(user) + + expect { service.execute(build) } + .to raise_error Gitlab::Access::AccessDeniedError + end + end + + context 'when project has repository' do + let(:project) { create(:project) } + + it 'allows user with developer role to play a build' do + project.add_developer(user) + + service.execute(build) + + expect(build.reload).to be_pending + end + end + + context 'when build is a playable manual action' do + let(:build) { create(:ci_build, :manual, pipeline: pipeline) } + + before do + project.add_master(user) + end + + it 'enqueues the build' do + expect(service.execute(build)).to eq build + expect(build.reload).to be_pending + end + + it 'reassignes build user correctly' do + service.execute(build) + + expect(build.reload.user).to eq user + end + end + + context 'when build is not a playable manual action' do + let(:build) { create(:ci_build, when: :manual, pipeline: pipeline) } + + before do + project.add_master(user) + end + + it 'duplicates the build' do + duplicate = service.execute(build) + + expect(duplicate).not_to eq build + expect(duplicate).to be_pending + end + + it 'assigns users correctly' do + duplicate = service.execute(build) + + expect(build.user).not_to eq user + expect(duplicate.user).to eq user + end + end + + context 'when build is not action' do + let(:build) { create(:ci_build, :success, pipeline: pipeline) } + + it 'raises an error' do + expect { service.execute(build) } + .to raise_error Gitlab::Access::AccessDeniedError + end + end + + context 'when user does not have ability to trigger action' do + before do + create(:protected_branch, :no_one_can_push, + name: build.ref, project: project) + end + + it 'raises an error' do + expect { service.execute(build) } + .to raise_error Gitlab::Access::AccessDeniedError + end + end +end -- cgit v1.2.3 From 2aa211fa69ffd02ba11757e06e19d34f6ca51865 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 12 Apr 2017 13:48:43 +0200 Subject: Use build policy to determine if user can play build --- app/models/ci/build.rb | 6 ------ app/models/environment.rb | 6 ------ app/serializers/build_action_entity.rb | 2 +- app/serializers/build_entity.rb | 2 +- app/services/ci/stop_environments_service.rb | 3 ++- app/views/projects/ci/builds/_build.html.haml | 2 +- lib/gitlab/ci/status/build/play.rb | 2 +- spec/models/ci/build_spec.rb | 27 --------------------------- spec/models/environment_spec.rb | 25 ------------------------- 9 files changed, 6 insertions(+), 69 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 9edc4cd96b9..b3acb25b9ce 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -115,12 +115,6 @@ module Ci commands.present? end - def can_play?(current_user) - ::Gitlab::UserAccess - .new(current_user, project: project) - .can_push_to_branch?(ref) - end - def play(current_user) Ci::PlayBuildService .new(project, current_user) diff --git a/app/models/environment.rb b/app/models/environment.rb index f8b9a21c08e..bf33010fd21 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -122,12 +122,6 @@ class Environment < ActiveRecord::Base available? && stop_action.present? end - def can_trigger_stop_action?(current_user) - return false unless stop_action? - - stop_action.can_play?(current_user) - end - def stop_with_action!(current_user) return unless available? diff --git a/app/serializers/build_action_entity.rb b/app/serializers/build_action_entity.rb index 3d12c64b88a..0bb7e561073 100644 --- a/app/serializers/build_action_entity.rb +++ b/app/serializers/build_action_entity.rb @@ -19,6 +19,6 @@ class BuildActionEntity < Grape::Entity alias_method :build, :object def playable? - build.playable? && build.can_play?(request.user) + can?(request.user, :play_build, build) && build.playable? end end diff --git a/app/serializers/build_entity.rb b/app/serializers/build_entity.rb index 401a277dadc..f301900c43c 100644 --- a/app/serializers/build_entity.rb +++ b/app/serializers/build_entity.rb @@ -26,7 +26,7 @@ class BuildEntity < Grape::Entity alias_method :build, :object def playable? - build.playable? && build.can_play?(request.user) + can?(request.user, :play_build, build) && build.playable? end def detailed_status diff --git a/app/services/ci/stop_environments_service.rb b/app/services/ci/stop_environments_service.rb index d1e341bc9b5..bd9735fc0ac 100644 --- a/app/services/ci/stop_environments_service.rb +++ b/app/services/ci/stop_environments_service.rb @@ -9,7 +9,8 @@ module Ci return unless can?(current_user, :create_deployment, project) environments.each do |environment| - next unless environment.can_trigger_stop_action?(current_user) + next unless environment.stop_action? + next unless can?(current_user, :play_build, environment.stop_action) environment.stop_with_action!(current_user) end diff --git a/app/views/projects/ci/builds/_build.html.haml b/app/views/projects/ci/builds/_build.html.haml index ceec36e2440..769640c4842 100644 --- a/app/views/projects/ci/builds/_build.html.haml +++ b/app/views/projects/ci/builds/_build.html.haml @@ -101,7 +101,7 @@ = link_to cancel_namespace_project_build_path(build.project.namespace, build.project, build, return_to: request.original_url), method: :post, title: 'Cancel', class: 'btn btn-build' do = icon('remove', class: 'cred') - elsif allow_retry - - if build.playable? && !admin && build.can_play?(current_user) + - if build.playable? && !admin && can?(current_user, :play_build, build) = link_to play_namespace_project_build_path(build.project.namespace, build.project, build, return_to: request.original_url), method: :post, title: 'Play', class: 'btn btn-build' do = custom_icon('icon_play') - elsif build.retryable? diff --git a/lib/gitlab/ci/status/build/play.rb b/lib/gitlab/ci/status/build/play.rb index 29d0558a265..4c893f62925 100644 --- a/lib/gitlab/ci/status/build/play.rb +++ b/lib/gitlab/ci/status/build/play.rb @@ -10,7 +10,7 @@ module Gitlab end def has_action? - can?(user, :update_build, subject) && subject.can_play?(user) + can?(user, :play_build, subject) end def action_icon diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 09e9f5bd8ba..cdceca975e5 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -925,33 +925,6 @@ describe Ci::Build, :models do end end - describe '#can_play?' do - before do - project.add_developer(user) - end - - let(:build) do - create(:ci_build, ref: 'some-ref', pipeline: pipeline) - end - - context 'when branch build is running for is protected' do - before do - create(:protected_branch, :no_one_can_push, - name: 'some-ref', project: project) - end - - it 'indicates that user can not trigger an action' do - expect(build.can_play?(user)).to be_falsey - end - end - - context 'when branch build is running for is not protected' do - it 'indicates that user can trigger an action' do - expect(build.can_play?(user)).to be_truthy - end - end - end - describe '#play' do let(:build) { create(:ci_build, :manual, pipeline: pipeline) } diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index fb7cee47ae8..9e00f2247e8 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -155,31 +155,6 @@ describe Environment, models: true do end end - describe '#can_trigger_stop_action?' do - let(:user) { create(:user) } - let(:project) { create(:project) } - - let(:environment) do - create(:environment, :with_review_app, project: project) - end - - context 'when user can trigger stop action' do - before do - project.add_developer(user) - end - - it 'returns value that evaluates to true' do - expect(environment.can_trigger_stop_action?(user)).to be_truthy - end - end - - context 'when user is not allowed to trigger stop action' do - it 'returns value that evaluates to false' do - expect(environment.can_trigger_stop_action?(user)).to be_falsey - end - end - end - describe '#stop_with_action!' do let(:user) { create(:admin) } -- cgit v1.2.3 From 0b75541559c0550bd3e195ca5aca55016fa614ef Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 12 Apr 2017 15:22:32 +0200 Subject: Fix manual action entity specs --- spec/serializers/build_action_entity_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/serializers/build_action_entity_spec.rb b/spec/serializers/build_action_entity_spec.rb index 54ac17447b1..059deba5416 100644 --- a/spec/serializers/build_action_entity_spec.rb +++ b/spec/serializers/build_action_entity_spec.rb @@ -2,9 +2,10 @@ require 'spec_helper' describe BuildActionEntity do let(:build) { create(:ci_build, name: 'test_build') } + let(:request) { double('request') } let(:entity) do - described_class.new(build, request: double) + described_class.new(build, request: spy('request')) end describe '#as_json' do -- cgit v1.2.3 From 57f8f2d7ff851fc4f5d1c81a28a023855f1985b7 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 13 Apr 2017 15:06:45 +0200 Subject: Fix detailed build status specs for manual actions --- spec/lib/gitlab/ci/status/build/play_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/lib/gitlab/ci/status/build/play_spec.rb b/spec/lib/gitlab/ci/status/build/play_spec.rb index 48aeb89e11f..95f1388a9b9 100644 --- a/spec/lib/gitlab/ci/status/build/play_spec.rb +++ b/spec/lib/gitlab/ci/status/build/play_spec.rb @@ -12,7 +12,7 @@ describe Gitlab::Ci::Status::Build::Play do describe 'action details' do let(:user) { create(:user) } - let(:build) { create(:ci_build) } + let(:build) { create(:ci_build, :manual) } let(:status) { Gitlab::Ci::Status::Core.new(build, user) } describe '#has_action?' do -- cgit v1.2.3 From 52bfc0efa95f991f17618aa049f799f4e44e13ac Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sat, 22 Apr 2017 21:02:31 +0200 Subject: Fix typo in CI/CD build partial view --- app/views/projects/ci/builds/_build.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/ci/builds/_build.html.haml b/app/views/projects/ci/builds/_build.html.haml index 3e1c8f25dea..2227d36eed2 100644 --- a/app/views/projects/ci/builds/_build.html.haml +++ b/app/views/projects/ci/builds/_build.html.haml @@ -102,7 +102,7 @@ = link_to cancel_namespace_project_build_path(job.project.namespace, job.project, job, return_to: request.original_url), method: :post, title: 'Cancel', class: 'btn btn-build' do = icon('remove', class: 'cred') - elsif allow_retry - - if job.playable? && !admin && can?(current_user, :play_build, jop) + - if job.playable? && !admin && can?(current_user, :play_build, job) = link_to play_namespace_project_build_path(job.project.namespace, job.project, job, return_to: request.original_url), method: :post, title: 'Play', class: 'btn btn-build' do = custom_icon('icon_play') - elsif job.retryable? -- cgit v1.2.3 From 6baaa8a98e10ff93ba3f481052bb68fdafb6e2c1 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 1 May 2017 13:38:57 +0200 Subject: Add new ability check for stopping environment --- app/policies/environment_policy.rb | 13 ++++++- app/services/ci/stop_environments_service.rb | 9 +---- spec/policies/environment_policy_spec.rb | 57 ++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 8 deletions(-) create mode 100644 spec/policies/environment_policy_spec.rb diff --git a/app/policies/environment_policy.rb b/app/policies/environment_policy.rb index f4219569161..0b976e5664d 100644 --- a/app/policies/environment_policy.rb +++ b/app/policies/environment_policy.rb @@ -1,5 +1,16 @@ class EnvironmentPolicy < BasePolicy + + alias_method :environment, :subject + def rules - delegate! @subject.project + delegate! environment.project + + if environment.stop_action? + delegate! environment.stop_action + end + + if can?(:create_deployment) && can?(:play_build) + can! :stop_environment + end end end diff --git a/app/services/ci/stop_environments_service.rb b/app/services/ci/stop_environments_service.rb index bd9735fc0ac..43c9a065fcf 100644 --- a/app/services/ci/stop_environments_service.rb +++ b/app/services/ci/stop_environments_service.rb @@ -5,12 +5,11 @@ module Ci def execute(branch_name) @ref = branch_name - return unless has_ref? - return unless can?(current_user, :create_deployment, project) + return unless @ref.present? environments.each do |environment| next unless environment.stop_action? - next unless can?(current_user, :play_build, environment.stop_action) + next unless can?(current_user, :stop_environment, environment) environment.stop_with_action!(current_user) end @@ -18,10 +17,6 @@ module Ci private - def has_ref? - @ref.present? - end - def environments @environments ||= EnvironmentsFinder .new(project, current_user, ref: @ref, recently_updated: true) diff --git a/spec/policies/environment_policy_spec.rb b/spec/policies/environment_policy_spec.rb new file mode 100644 index 00000000000..f43caffa946 --- /dev/null +++ b/spec/policies/environment_policy_spec.rb @@ -0,0 +1,57 @@ +require 'spec_helper' + +describe Ci::EnvironmentPolicy do + let(:user) { create(:user) } + let(:project) { create(:project) } + + let(:environment) do + create(:environment, :with_review_app, project: project) + end + + let(:policies) do + described_class.abilities(user, environment).to_set + end + + describe '#rules' do + context 'when user does not have access to the project' do + let(:project) { create(:project, :private) } + + it 'does not include ability to stop environment' do + expect(policies).not_to include :stop_environment + end + end + + context 'when anonymous user has access to the project' do + let(:project) { create(:project, :public) } + + it 'does not include ability to stop environment' do + expect(policies).not_to include :stop_environment + end + end + + context 'when team member has access to the project' do + let(:project) { create(:project, :public) } + + before do + project.add_master(user) + end + + context 'when team member has ability to stop environment' do + it 'does includes ability to stop environment' do + expect(policies).to include :stop_environment + end + end + + context 'when team member has no ability to stop environment' do + before do + create(:protected_branch, :no_one_can_push, + name: 'master', project: project) + end + + it 'does not include ability to stop environment' do + expect(policies).not_to include :stop_environment + end + end + end + end +end -- cgit v1.2.3 From 4f2cc5951f3d2862a44fe124ef9a879162028e62 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 1 May 2017 14:29:20 +0200 Subject: Extend action tooltop to show info about abilities --- lib/gitlab/ci/status/build/play.rb | 6 ++- spec/lib/gitlab/ci/status/build/factory_spec.rb | 2 +- spec/lib/gitlab/ci/status/build/play_spec.rb | 65 ++++++++++++++----------- 3 files changed, 42 insertions(+), 31 deletions(-) diff --git a/lib/gitlab/ci/status/build/play.rb b/lib/gitlab/ci/status/build/play.rb index 4c893f62925..8130c7d1c90 100644 --- a/lib/gitlab/ci/status/build/play.rb +++ b/lib/gitlab/ci/status/build/play.rb @@ -6,7 +6,11 @@ module Gitlab include Status::Extended def label - 'manual play action' + if has_action? + 'manual play action' + else + 'manual play action (not allowed)' + end end def has_action? diff --git a/spec/lib/gitlab/ci/status/build/factory_spec.rb b/spec/lib/gitlab/ci/status/build/factory_spec.rb index 2ab67127b1e..2de00c28945 100644 --- a/spec/lib/gitlab/ci/status/build/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/build/factory_spec.rb @@ -216,7 +216,7 @@ describe Gitlab::Ci::Status::Build::Factory do expect(status.group).to eq 'manual' expect(status.icon).to eq 'icon_status_manual' expect(status.favicon).to eq 'favicon_status_manual' - expect(status.label).to eq 'manual play action' + expect(status.label).to include 'manual play action' expect(status).to have_details expect(status.action_path).to include 'play' end diff --git a/spec/lib/gitlab/ci/status/build/play_spec.rb b/spec/lib/gitlab/ci/status/build/play_spec.rb index 95f1388a9b9..abefdbe7c66 100644 --- a/spec/lib/gitlab/ci/status/build/play_spec.rb +++ b/spec/lib/gitlab/ci/status/build/play_spec.rb @@ -1,51 +1,58 @@ require 'spec_helper' describe Gitlab::Ci::Status::Build::Play do - let(:status) { double('core') } - let(:user) { double('user') } + let(:user) { create(:user) } + let(:build) { create(:ci_build, :manual) } + let(:status) { Gitlab::Ci::Status::Core.new(build, user) } subject { described_class.new(status) } - describe '#label' do - it { expect(subject.label).to eq 'manual play action' } - end - - describe 'action details' do - let(:user) { create(:user) } - let(:build) { create(:ci_build, :manual) } - let(:status) { Gitlab::Ci::Status::Core.new(build, user) } + context 'when user is allowed to update build' do + context 'when user can push to branch' do + before { build.project.add_master(user) } - describe '#has_action?' do - context 'when user is allowed to update build' do - context 'when user can push to branch' do - before { build.project.add_master(user) } + describe '#has_action?' do + it { is_expected.to have_action } + end - it { is_expected.to have_action } + describe '#label' do + it 'has a label that says it is a manual action' do + expect(subject.label).to eq 'manual play action' end + end + end - context 'when user can not push to the branch' do - before { build.project.add_developer(user) } + context 'when user can not push to the branch' do + before { build.project.add_developer(user) } - it { is_expected.not_to have_action } - end + describe 'has_action?' do + it { is_expected.not_to have_action } end - context 'when user is not allowed to update build' do - it { is_expected.not_to have_action } + describe '#label' do + it 'has a label that says user is not allowed to play it' do + expect(subject.label).to eq 'manual play action (not allowed)' + end end end + end - describe '#action_path' do - it { expect(subject.action_path).to include "#{build.id}/play" } + context 'when user is not allowed to update build' do + describe '#has_action?' do + it { is_expected.not_to have_action } end + end - describe '#action_icon' do - it { expect(subject.action_icon).to eq 'icon_action_play' } - end + describe '#action_path' do + it { expect(subject.action_path).to include "#{build.id}/play" } + end - describe '#action_title' do - it { expect(subject.action_title).to eq 'Play' } - end + describe '#action_icon' do + it { expect(subject.action_icon).to eq 'icon_action_play' } + end + + describe '#action_title' do + it { expect(subject.action_title).to eq 'Play' } end describe '.matches?' do -- cgit v1.2.3 From e5df86f54d6165e0b3871678dd987956dddfa061 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 1 May 2017 14:30:50 +0200 Subject: Fix Rubocop offense in environments policy class --- app/policies/environment_policy.rb | 1 - spec/models/ci/build_spec.rb | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/policies/environment_policy.rb b/app/policies/environment_policy.rb index 0b976e5664d..c0c9bbf2d9e 100644 --- a/app/policies/environment_policy.rb +++ b/app/policies/environment_policy.rb @@ -1,5 +1,4 @@ class EnvironmentPolicy < BasePolicy - alias_method :environment, :subject def rules diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index b2f9a61f7f3..5231ce28c9d 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -911,7 +911,7 @@ describe Ci::Build, :models do it { is_expected.to eq(environment) } end - context 'when referenced with a variable' do + context 'when referenced with a variable' do let(:build) do create(:ci_build, pipeline: pipeline, environment: "foo-$CI_COMMIT_REF_NAME") end -- cgit v1.2.3 From 02fc1846832593b2e803a83446690183715f5df1 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 2 May 2017 10:29:09 +0200 Subject: Improve specs for jobs API regarding manual actions --- spec/requests/api/jobs_spec.rb | 67 +++++++++++++++++++++++++++++++----------- 1 file changed, 50 insertions(+), 17 deletions(-) diff --git a/spec/requests/api/jobs_spec.rb b/spec/requests/api/jobs_spec.rb index d8a56c02a63..e5e5872dc1f 100644 --- a/spec/requests/api/jobs_spec.rb +++ b/spec/requests/api/jobs_spec.rb @@ -1,16 +1,26 @@ require 'spec_helper' -describe API::Jobs, api: true do - include ApiHelpers +describe API::Jobs, :api do + let!(:project) do + create(:project, :repository, public_builds: false) + end + + let!(:pipeline) do + create(:ci_empty_pipeline, project: project, + sha: project.commit.id, + ref: project.default_branch) + end + + let!(:build) { create(:ci_build, pipeline: pipeline) } let(:user) { create(:user) } let(:api_user) { user } - let!(:project) { create(:project, :repository, creator: user, public_builds: false) } - let!(:developer) { create(:project_member, :developer, user: user, project: project) } - let(:reporter) { create(:project_member, :reporter, project: project) } - let(:guest) { create(:project_member, :guest, project: project) } - let!(:pipeline) { create(:ci_empty_pipeline, project: project, sha: project.commit.id, ref: project.default_branch) } - let!(:build) { create(:ci_build, pipeline: pipeline) } + let(:reporter) { create(:project_member, :reporter, project: project).user } + let(:guest) { create(:project_member, :guest, project: project).user } + + before do + project.add_developer(user) + end describe 'GET /projects/:id/jobs' do let(:query) { Hash.new } @@ -213,7 +223,7 @@ describe API::Jobs, api: true do end describe 'GET /projects/:id/artifacts/:ref_name/download?job=name' do - let(:api_user) { reporter.user } + let(:api_user) { reporter } let(:build) { create(:ci_build, :artifacts, pipeline: pipeline) } before do @@ -237,7 +247,7 @@ describe API::Jobs, api: true do end context 'when logging as guest' do - let(:api_user) { guest.user } + let(:api_user) { guest } before do get_for_ref @@ -347,7 +357,7 @@ describe API::Jobs, api: true do end context 'user without :update_build permission' do - let(:api_user) { reporter.user } + let(:api_user) { reporter } it 'does not cancel job' do expect(response).to have_http_status(403) @@ -381,7 +391,7 @@ describe API::Jobs, api: true do end context 'user without :update_build permission' do - let(:api_user) { reporter.user } + let(:api_user) { reporter } it 'does not retry job' do expect(response).to have_http_status(403) @@ -457,16 +467,39 @@ describe API::Jobs, api: true do describe 'POST /projects/:id/jobs/:job_id/play' do before do - post api("/projects/#{project.id}/jobs/#{build.id}/play", user) + post api("/projects/#{project.id}/jobs/#{build.id}/play", api_user) end context 'on an playable job' do let(:build) { create(:ci_build, :manual, project: project, pipeline: pipeline) } - it 'plays the job' do - expect(response).to have_http_status(200) - expect(json_response['user']['id']).to eq(user.id) - expect(json_response['id']).to eq(build.id) + context 'when user is authorized to trigger a manual action' do + it 'plays the job' do + expect(response).to have_http_status(200) + expect(json_response['user']['id']).to eq(user.id) + expect(json_response['id']).to eq(build.id) + expect(build.reload).to be_pending + end + end + + context 'when user is not authorized to trigger a manual action' do + context 'when user does not have access to the project' do + let(:api_user) { create(:user) } + + it 'does not trigger a manual action' do + expect(build.reload).to be_manual + expect(response).to have_http_status(404) + end + end + + context 'when user is not allowed to trigger the manual action' do + let(:api_user) { reporter } + + it 'does not trigger a manual action' do + expect(build.reload).to be_manual + expect(response).to have_http_status(403) + end + end end end -- cgit v1.2.3 From f5441c93203cd5c8ccd208c00bee59432facae7f Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 2 May 2017 10:35:51 +0200 Subject: Document protected manual actions feature --- doc/ci/yaml/README.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index ad3ebd144df..0cab3270d94 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -553,6 +553,8 @@ The above script will: #### Manual actions > Introduced in GitLab 8.10. +> Blocking manual actions were introduced in GitLab 9.0 +> Protected actions were introduced in GitLab 9.2 Manual actions are a special type of job that are not executed automatically; they need to be explicitly started by a user. Manual actions can be started @@ -578,7 +580,9 @@ Optional manual actions have `allow_failure: true` set by default. **Statuses of optional actions do not contribute to overall pipeline status.** -> Blocking manual actions were introduced in GitLab 9.0 +**Manual actions do inherit permissions of protected branches. In other words, +in order to trigger a manual action assigned to a branch that the pipeline is +running for, user needs to have ability to push to this branch.** ### environment -- cgit v1.2.3 From c9def85844531ffdd2984707a1bc8cbca18f6742 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 2 May 2017 10:37:50 +0200 Subject: Add Changelog entry for protected manual actions --- .../feature-gb-manual-actions-protected-branches-permissions.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/feature-gb-manual-actions-protected-branches-permissions.yml diff --git a/changelogs/unreleased/feature-gb-manual-actions-protected-branches-permissions.yml b/changelogs/unreleased/feature-gb-manual-actions-protected-branches-permissions.yml new file mode 100644 index 00000000000..6f8e80e7d64 --- /dev/null +++ b/changelogs/unreleased/feature-gb-manual-actions-protected-branches-permissions.yml @@ -0,0 +1,4 @@ +--- +title: Implement protected manual actions +merge_request: 10494 +author: -- cgit v1.2.3 From 4ddce55315167b362056616151244f137c81945f Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 2 May 2017 11:27:10 +0200 Subject: Fix environment policy class name in specs --- spec/policies/environment_policy_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/policies/environment_policy_spec.rb b/spec/policies/environment_policy_spec.rb index f43caffa946..0e15beaa5e8 100644 --- a/spec/policies/environment_policy_spec.rb +++ b/spec/policies/environment_policy_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Ci::EnvironmentPolicy do +describe EnvironmentPolicy do let(:user) { create(:user) } let(:project) { create(:project) } -- cgit v1.2.3 From eb45582f55cae42acc41fa228f4797b4067c01dc Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 2 May 2017 12:25:30 +0200 Subject: Fix builds controller spec related to protected actions --- spec/controllers/projects/builds_controller_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/controllers/projects/builds_controller_spec.rb b/spec/controllers/projects/builds_controller_spec.rb index 22193eac672..3bf03c88ff5 100644 --- a/spec/controllers/projects/builds_controller_spec.rb +++ b/spec/controllers/projects/builds_controller_spec.rb @@ -260,6 +260,8 @@ describe Projects::BuildsController do end describe 'POST play' do + let(:project) { create(:project, :public) } + before do project.add_developer(user) sign_in(user) -- cgit v1.2.3 From 48bb8921240387ce7d8f9401c98e13a9b3d0100f Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 2 May 2017 13:21:06 +0200 Subject: Hide environment external URL button if not defined --- app/views/projects/environments/terminal.html.haml | 5 ++-- .../environments/terminal.html.haml_spec.rb | 32 ++++++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) create mode 100644 spec/views/projects/environments/terminal.html.haml_spec.rb diff --git a/app/views/projects/environments/terminal.html.haml b/app/views/projects/environments/terminal.html.haml index c8363087d6a..4c4aa0baff3 100644 --- a/app/views/projects/environments/terminal.html.haml +++ b/app/views/projects/environments/terminal.html.haml @@ -16,8 +16,9 @@ .col-sm-6 .nav-controls - = link_to @environment.external_url, class: 'btn btn-default', target: '_blank', rel: 'noopener noreferrer nofollow' do - = icon('external-link') + - if @environment.external_url.present? + = link_to @environment.external_url, class: 'btn btn-default', target: '_blank', rel: 'noopener noreferrer nofollow' do + = icon('external-link') = render 'projects/deployments/actions', deployment: @environment.last_deployment .terminal-container{ class: container_class } diff --git a/spec/views/projects/environments/terminal.html.haml_spec.rb b/spec/views/projects/environments/terminal.html.haml_spec.rb new file mode 100644 index 00000000000..d2e47225226 --- /dev/null +++ b/spec/views/projects/environments/terminal.html.haml_spec.rb @@ -0,0 +1,32 @@ +require 'spec_helper' + +describe 'projects/environments/terminal' do + let!(:environment) { create(:environment, :with_review_app) } + + before do + assign(:environment, environment) + assign(:project, environment.project) + + allow(view).to receive(:can?).and_return(true) + end + + context 'when environment has external URL' do + it 'shows external URL button' do + environment.update_attribute(:external_url, 'https://gitlab.com') + + render + + expect(rendered).to have_link(nil, href: 'https://gitlab.com') + end + end + + context 'when environment does not have external URL' do + it 'shows external URL button' do + environment.update_attribute(:external_url, nil) + + render + + expect(rendered).not_to have_link(nil, href: 'https://gitlab.com') + end + end +end -- cgit v1.2.3 From 3aa92cb5cbe8456c845e16d14489591dd81dbcb3 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 2 May 2017 13:26:53 +0200 Subject: Add changelog entry for external env URL btn fix --- .../fix-gb-hide-environment-external-url-btn-when-not-provided.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/fix-gb-hide-environment-external-url-btn-when-not-provided.yml diff --git a/changelogs/unreleased/fix-gb-hide-environment-external-url-btn-when-not-provided.yml b/changelogs/unreleased/fix-gb-hide-environment-external-url-btn-when-not-provided.yml new file mode 100644 index 00000000000..66158e337fd --- /dev/null +++ b/changelogs/unreleased/fix-gb-hide-environment-external-url-btn-when-not-provided.yml @@ -0,0 +1,4 @@ +--- +title: Hide external environment URL button on terminal page if URL is not defined +merge_request: 11029 +author: -- cgit v1.2.3 From 702b291f81b576c08f8e194d87fb779b4ce0fd6c Mon Sep 17 00:00:00 2001 From: James Lopez Date: Tue, 2 May 2017 14:55:32 +0200 Subject: remove gl_project_id for I/E version update --- app/models/commit_status.rb | 6 ------ lib/gitlab/import_export/import_export.yml | 1 - 2 files changed, 7 deletions(-) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 2c4033146bf..75d04fd2b08 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -142,12 +142,6 @@ class CommitStatus < ActiveRecord::Base canceled? && auto_canceled_by_id? end - # Added in 9.0 to keep backward compatibility for projects exported in 8.17 - # and prior. - def gl_project_id - 'dummy' - end - def detailed_status(current_user) Gitlab::Ci::Status::Factory .new(self, current_user) diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml index 899a6567768..70d2ef7103c 100644 --- a/lib/gitlab/import_export/import_export.yml +++ b/lib/gitlab/import_export/import_export.yml @@ -87,7 +87,6 @@ methods: - :type statuses: - :type - - :gl_project_id services: - :type merge_request_diff: -- cgit v1.2.3 From 72307940bb04d29dc751adb5e76330d2ce45e9ce Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 3 May 2017 12:32:39 +0200 Subject: Improve code style related to protected actions --- app/serializers/build_action_entity.rb | 2 +- app/serializers/build_entity.rb | 4 ++-- app/serializers/pipeline_entity.rb | 6 +++--- app/services/ci/play_build_service.rb | 2 +- spec/controllers/projects/builds_controller_spec.rb | 4 +--- 5 files changed, 8 insertions(+), 10 deletions(-) diff --git a/app/serializers/build_action_entity.rb b/app/serializers/build_action_entity.rb index 0bb7e561073..3eff724ae91 100644 --- a/app/serializers/build_action_entity.rb +++ b/app/serializers/build_action_entity.rb @@ -19,6 +19,6 @@ class BuildActionEntity < Grape::Entity alias_method :build, :object def playable? - can?(request.user, :play_build, build) && build.playable? + build.playable? && can?(request.user, :play_build, build) end end diff --git a/app/serializers/build_entity.rb b/app/serializers/build_entity.rb index f301900c43c..10ba7c90c10 100644 --- a/app/serializers/build_entity.rb +++ b/app/serializers/build_entity.rb @@ -12,7 +12,7 @@ class BuildEntity < Grape::Entity path_to(:retry_namespace_project_build, build) end - expose :play_path, if: proc { playable? } do |build| + expose :play_path, if: -> (*) { playable? } do |build| path_to(:play_namespace_project_build, build) end @@ -26,7 +26,7 @@ class BuildEntity < Grape::Entity alias_method :build, :object def playable? - can?(request.user, :play_build, build) && build.playable? + build.playable? && can?(request.user, :play_build, build) end def detailed_status diff --git a/app/serializers/pipeline_entity.rb b/app/serializers/pipeline_entity.rb index ad8b4d43e8f..7eb7aac72eb 100644 --- a/app/serializers/pipeline_entity.rb +++ b/app/serializers/pipeline_entity.rb @@ -48,15 +48,15 @@ class PipelineEntity < Grape::Entity end expose :commit, using: CommitEntity - expose :yaml_errors, if: ->(pipeline, _) { pipeline.has_yaml_errors? } + expose :yaml_errors, if: -> (pipeline, _) { pipeline.has_yaml_errors? } - expose :retry_path, if: proc { can_retry? } do |pipeline| + expose :retry_path, if: -> (*) { can_retry? } do |pipeline| retry_namespace_project_pipeline_path(pipeline.project.namespace, pipeline.project, pipeline.id) end - expose :cancel_path, if: proc { can_cancel? } do |pipeline| + expose :cancel_path, if: -> (*) { can_cancel? } do |pipeline| cancel_namespace_project_pipeline_path(pipeline.project.namespace, pipeline.project, pipeline.id) diff --git a/app/services/ci/play_build_service.rb b/app/services/ci/play_build_service.rb index c9ed45408f2..5b3ff1ced38 100644 --- a/app/services/ci/play_build_service.rb +++ b/app/services/ci/play_build_service.rb @@ -5,7 +5,7 @@ module Ci raise Gitlab::Access::AccessDeniedError end - # Try to enqueue thebuild, otherwise create a duplicate. + # Try to enqueue the build, otherwise create a duplicate. # if build.enqueue build.tap { |action| action.update(user: current_user) } diff --git a/spec/controllers/projects/builds_controller_spec.rb b/spec/controllers/projects/builds_controller_spec.rb index 3bf03c88ff5..3ce23c17cdc 100644 --- a/spec/controllers/projects/builds_controller_spec.rb +++ b/spec/controllers/projects/builds_controller_spec.rb @@ -260,10 +260,8 @@ describe Projects::BuildsController do end describe 'POST play' do - let(:project) { create(:project, :public) } - before do - project.add_developer(user) + project.add_master(user) sign_in(user) post_play -- cgit v1.2.3 From 88ace4a7ad0809d64f688dc1ee6fc8ea6cda9045 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 3 May 2017 12:35:13 +0200 Subject: Rephrase documentation for protected actions feature --- doc/ci/yaml/README.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index 0cab3270d94..16308a957cb 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -580,9 +580,10 @@ Optional manual actions have `allow_failure: true` set by default. **Statuses of optional actions do not contribute to overall pipeline status.** -**Manual actions do inherit permissions of protected branches. In other words, -in order to trigger a manual action assigned to a branch that the pipeline is -running for, user needs to have ability to push to this branch.** +**Manual actions are considered to be write actions, so permissions for +protected branches are used when user wants to trigger an action. In other +words, in order to trigger a manual action assigned to a branch that the +pipeline is running for, user needs to have ability to push to this branch.** ### environment -- cgit v1.2.3 From 83154f21542ec04076d20ce9c4a8997d55fc5f43 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 3 May 2017 15:42:29 +0200 Subject: Improve environment policy class --- app/policies/environment_policy.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/app/policies/environment_policy.rb b/app/policies/environment_policy.rb index c0c9bbf2d9e..cc94d4a7e05 100644 --- a/app/policies/environment_policy.rb +++ b/app/policies/environment_policy.rb @@ -4,12 +4,14 @@ class EnvironmentPolicy < BasePolicy def rules delegate! environment.project - if environment.stop_action? - delegate! environment.stop_action + if can?(:create_deployment) && environment.stop_action? + can! :stop_environment if can_play_stop_action? end + end - if can?(:create_deployment) && can?(:play_build) - can! :stop_environment - end + private + + def can_play_stop_action? + Ability.allowed?(user, :play_build, environment.stop_action) end end -- cgit v1.2.3 From a39adfb48d1a13ecc32a78844b74704ed433430f Mon Sep 17 00:00:00 2001 From: Lee Matos Date: Thu, 4 May 2017 12:13:41 -0400 Subject: clarify DB/Redis HA docs --- doc/administration/high_availability/database.md | 4 +++- doc/administration/high_availability/redis.md | 17 ++++++++--------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/doc/administration/high_availability/database.md b/doc/administration/high_availability/database.md index c22b1af8bfb..da9687aa849 100644 --- a/doc/administration/high_availability/database.md +++ b/doc/administration/high_availability/database.md @@ -27,7 +27,7 @@ If you use a cloud-managed service, or provide your own PostgreSQL: steps on the download page. 1. Create/edit `/etc/gitlab/gitlab.rb` and use the following configuration. Be sure to change the `external_url` to match your eventual GitLab front-end - URL. + URL. If there is a directive listed below that you do not see in the configuration, be sure to add it. ```ruby external_url 'https://gitlab.example.com' @@ -39,6 +39,8 @@ If you use a cloud-managed service, or provide your own PostgreSQL: unicorn['enable'] = false sidekiq['enable'] = false redis['enable'] = false + prometheus['enable'] = false + gitaly['enable'] = false gitlab_workhorse['enable'] = false mailroom['enable'] = false diff --git a/doc/administration/high_availability/redis.md b/doc/administration/high_availability/redis.md index 4638a9c9782..0e92f7c5a34 100644 --- a/doc/administration/high_availability/redis.md +++ b/doc/administration/high_availability/redis.md @@ -42,10 +42,10 @@ instances run in different machines. If you fail to provision the machines in that specific way, any issue with the shared environment can bring your entire setup down. -It is OK to run a Sentinel along with a master or slave Redis instance. -No more than one Sentinel in the same machine though. +It is OK to run a Sentinel alongside of a master or slave Redis instance. +There should be no more than one Sentinel on the same machine though. -You also need to take in consideration the underlying network topology, +You also need to take into consideration the underlying network topology, making sure you have redundant connectivity between Redis / Sentinel and GitLab instances, otherwise the networks will become a single point of failure. @@ -113,7 +113,7 @@ the Omnibus GitLab package in `5` **independent** machines, both with ### Redis setup overview You must have at least `3` Redis servers: `1` Master, `2` Slaves, and they -need to be each in a independent machine (see explanation above). +need to each be on independent machines (see explanation above). You can have additional Redis nodes, that will help survive a situation where more nodes goes down. Whenever there is only `2` nodes online, a failover @@ -232,7 +232,7 @@ Pick the one that suits your needs. This is the section where we install and setup the new Redis instances. >**Notes:** -- We assume that you install GitLab and all HA components from scratch. If you +- We assume that you have installed GitLab and all HA components from scratch. If you already have it installed and running, read how to [switch from a single-machine installation to Redis HA](#switching-from-an-existing-single-machine-installation-to-redis-ha). - Redis nodes (both master and slaves) will need the same password defined in @@ -245,10 +245,9 @@ The prerequisites for a HA Redis setup are the following: 1. Provision the minimum required number of instances as specified in the [recommended setup](#recommended-setup) section. -1. **Do NOT** install Redis or Redis Sentinel in the same machines your - GitLab application is running on. You can however opt in to install Redis - and Sentinel in the same machine (each in independent ones is recommended - though). +1. We **Do not** recommend installing Redis or Redis Sentinel in the same machines your + GitLab application is running on as this weakens your HA configuration. You can however opt in to install Redis + and Sentinel in the same machine. 1. All Redis nodes must be able to talk to each other and accept incoming connections over Redis (`6379`) and Sentinel (`26379`) ports (unless you change the default ones). -- cgit v1.2.3 From 020295fffc2329b7852c2739082986fd6b569d9e Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Sat, 22 Apr 2017 03:26:58 +0100 Subject: Use regex to skip unnecessary reference processing in ProcessCommitWorker --- app/models/concerns/mentionable.rb | 16 +++++++ .../concerns/mentionable/reference_regexes.rb | 22 ++++++++++ app/services/git_push_service.rb | 6 ++- app/workers/process_commit_worker.rb | 3 ++ spec/models/concerns/mentionable_spec.rb | 49 ++++++++++++++++++++++ spec/services/git_push_service_spec.rb | 11 ++++- spec/workers/process_commit_worker_spec.rb | 8 ++++ 7 files changed, 112 insertions(+), 3 deletions(-) create mode 100644 app/models/concerns/mentionable/reference_regexes.rb diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index 7e56e371b27..5ac56ac6fa0 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -78,6 +78,8 @@ module Mentionable # Extract GFM references to other Mentionables from this Mentionable. Always excludes its #local_reference. def referenced_mentionables(current_user = self.author) + return [] unless matches_cross_reference_regex? + refs = all_references(current_user) refs = (refs.issues + refs.merge_requests + refs.commits) @@ -87,6 +89,20 @@ module Mentionable refs.reject { |ref| ref == local_reference } end + # Uses regex to quickly determine if mentionables might be referenced + # Allows heavy processing to be skipped + def matches_cross_reference_regex? + reference_pattern = if project.default_issues_tracker? + ReferenceRegexes::DEFAULT_PATTERN + else + ReferenceRegexes::EXTERNAL_PATTERN + end + + self.class.mentionable_attrs.any? do |attr, _| + __send__(attr) =~ reference_pattern + end + end + # Create a cross-reference Note for each GFM reference to another Mentionable found in the +mentionable_attrs+. def create_cross_references!(author = self.author, without = []) refs = referenced_mentionables(author) diff --git a/app/models/concerns/mentionable/reference_regexes.rb b/app/models/concerns/mentionable/reference_regexes.rb new file mode 100644 index 00000000000..1848230ec7e --- /dev/null +++ b/app/models/concerns/mentionable/reference_regexes.rb @@ -0,0 +1,22 @@ +module Mentionable + module ReferenceRegexes + def self.reference_pattern(link_patterns, issue_pattern) + Regexp.union(link_patterns, + issue_pattern, + Commit.reference_pattern, + MergeRequest.reference_pattern) + end + + DEFAULT_PATTERN = begin + issue_pattern = Issue.reference_pattern + link_patterns = Regexp.union([Issue, Commit, MergeRequest].map(&:link_reference_pattern)) + reference_pattern(link_patterns, issue_pattern) + end + + EXTERNAL_PATTERN = begin + issue_pattern = ExternalIssue.reference_pattern + link_patterns = URI.regexp(%w(http https)) + reference_pattern(link_patterns, issue_pattern) + end + end +end diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index 45411c779cc..b8bd7720265 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -85,8 +85,10 @@ class GitPushService < BaseService default = is_default_branch? push_commits.last(PROCESS_COMMIT_LIMIT).each do |commit| - ProcessCommitWorker. - perform_async(project.id, current_user.id, commit.to_hash, default) + if commit.matches_cross_reference_regex? + ProcessCommitWorker. + perform_async(project.id, current_user.id, commit.to_hash, default) + end end end diff --git a/app/workers/process_commit_worker.rb b/app/workers/process_commit_worker.rb index 2f7967cf531..d6ed0e253ad 100644 --- a/app/workers/process_commit_worker.rb +++ b/app/workers/process_commit_worker.rb @@ -23,6 +23,9 @@ class ProcessCommitWorker return unless user commit = build_commit(project, commit_hash) + + return unless commit.matches_cross_reference_regex? + author = commit.author || user process_commit_message(project, commit, user, author, default) diff --git a/spec/models/concerns/mentionable_spec.rb b/spec/models/concerns/mentionable_spec.rb index 2092576e981..e382c7120de 100644 --- a/spec/models/concerns/mentionable_spec.rb +++ b/spec/models/concerns/mentionable_spec.rb @@ -163,3 +163,52 @@ describe Issue, "Mentionable" do end end end + +describe Commit, 'Mentionable' do + let(:project) { create(:project, :public, :repository) } + let(:commit) { project.commit } + + describe '#matches_cross_reference_regex?' do + it "is false when message doesn't reference anything" do + allow(commit.raw).to receive(:message).and_return "WIP: Do something" + + expect(commit.matches_cross_reference_regex?).to be false + end + + it 'is true if issue #number mentioned in title' do + allow(commit.raw).to receive(:message).and_return "#1" + + expect(commit.matches_cross_reference_regex?).to be true + end + + it 'is true if references an MR' do + allow(commit.raw).to receive(:message).and_return "See merge request !12" + + expect(commit.matches_cross_reference_regex?).to be true + end + + it 'is true if references a commit' do + allow(commit.raw).to receive(:message).and_return "a1b2c3d4" + + expect(commit.matches_cross_reference_regex?).to be true + end + + it 'is true if issue referenced by url' do + issue = create(:issue, project: project) + + allow(commit.raw).to receive(:message).and_return Gitlab::UrlBuilder.build(issue) + + expect(commit.matches_cross_reference_regex?).to be true + end + + context 'with external issue tracker' do + let(:project) { create(:jira_project) } + + it 'is true if external issues referenced' do + allow(commit.raw).to receive(:message).and_return 'JIRA-123' + + expect(commit.matches_cross_reference_regex?).to be true + end + end + end +end diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index 0477cac6677..8c2415b4e07 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -622,12 +622,21 @@ describe GitPushService, services: true do it 'only schedules a limited number of commits' do allow(service).to receive(:push_commits). - and_return(Array.new(1000, double(:commit, to_hash: {}))) + and_return(Array.new(1000, double(:commit, to_hash: {}, matches_cross_reference_regex?: true))) expect(ProcessCommitWorker).to receive(:perform_async).exactly(100).times service.process_commit_messages end + + it "skips commits which don't include cross-references" do + allow(service).to receive(:push_commits). + and_return([double(:commit, to_hash: {}, matches_cross_reference_regex?: false)]) + + expect(ProcessCommitWorker).not_to receive(:perform_async) + + service.process_commit_messages + end end def execute_service(project, user, oldrev, newrev, ref) diff --git a/spec/workers/process_commit_worker_spec.rb b/spec/workers/process_commit_worker_spec.rb index 9afe2e610b9..6295856b461 100644 --- a/spec/workers/process_commit_worker_spec.rb +++ b/spec/workers/process_commit_worker_spec.rb @@ -20,6 +20,14 @@ describe ProcessCommitWorker do worker.perform(project.id, -1, commit.to_hash) end + it 'does not process the commit when no issues are referenced' do + allow(worker).to receive(:build_commit).and_return(double(matches_cross_reference_regex?: false)) + + expect(worker).not_to receive(:process_commit_message) + + worker.perform(project.id, user.id, commit.to_hash) + end + it 'processes the commit message' do expect(worker).to receive(:process_commit_message).and_call_original -- cgit v1.2.3 From 29519edb55f17d0e7de5dfb289085c894b4d2826 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Mon, 24 Apr 2017 17:26:23 +0100 Subject: Cycle analytics specs needed Commit to reference issue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The plan stage both measures time taken and lists related commits. We test for commits being listed, so needed to actually mention the issue in them. An alternative would have been adding “allow_any_instance_of(Commit).to receive(:matches_cross_reference_regex?).and_return(true)” but this felt too coupled to implementation. --- spec/features/cycle_analytics_spec.rb | 3 +-- spec/lib/gitlab/cycle_analytics/events_spec.rb | 4 +--- spec/requests/projects/cycle_analytics_events_spec.rb | 4 +--- spec/support/cycle_analytics_helpers.rb | 4 ++-- 4 files changed, 5 insertions(+), 10 deletions(-) diff --git a/spec/features/cycle_analytics_spec.rb b/spec/features/cycle_analytics_spec.rb index 0648c89a5c7..46c3bc9992e 100644 --- a/spec/features/cycle_analytics_spec.rb +++ b/spec/features/cycle_analytics_spec.rb @@ -8,7 +8,7 @@ feature 'Cycle Analytics', feature: true, js: true do let(:project) { create(:project) } let(:issue) { create(:issue, project: project, created_at: 2.days.ago) } let(:milestone) { create(:milestone, project: project) } - let(:mr) { create_merge_request_closing_issue(issue) } + let(:mr) { create_merge_request_closing_issue(issue, commit_message: "References #{issue.to_reference}") } let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: mr.source_branch, sha: mr.source_branch_sha) } context 'as an allowed user' do @@ -34,7 +34,6 @@ feature 'Cycle Analytics', feature: true, js: true do before do project.team << [user, :master] - allow_any_instance_of(Gitlab::ReferenceExtractor).to receive(:issues).and_return([issue]) create_cycle deploy_master diff --git a/spec/lib/gitlab/cycle_analytics/events_spec.rb b/spec/lib/gitlab/cycle_analytics/events_spec.rb index 9d2ba481919..0d56bdd0ebd 100644 --- a/spec/lib/gitlab/cycle_analytics/events_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/events_spec.rb @@ -11,8 +11,6 @@ describe 'cycle analytics events' do end before do - allow_any_instance_of(Gitlab::ReferenceExtractor).to receive(:issues).and_return([context]) - setup(context) end @@ -332,7 +330,7 @@ describe 'cycle analytics events' do def setup(context) milestone = create(:milestone, project: project) context.update(milestone: milestone) - mr = create_merge_request_closing_issue(context) + mr = create_merge_request_closing_issue(context, commit_message: "References #{context.to_reference}") ProcessCommitWorker.new.perform(project.id, user.id, mr.commits.last.to_hash) end diff --git a/spec/requests/projects/cycle_analytics_events_spec.rb b/spec/requests/projects/cycle_analytics_events_spec.rb index 0edbffbcd3b..c2f8f33447d 100644 --- a/spec/requests/projects/cycle_analytics_events_spec.rb +++ b/spec/requests/projects/cycle_analytics_events_spec.rb @@ -11,8 +11,6 @@ describe 'cycle analytics events' do before do project.team << [user, :developer] - allow_any_instance_of(Gitlab::ReferenceExtractor).to receive(:issues).and_return([issue]) - 3.times do |count| Timecop.freeze(Time.now + count.days) do create_cycle @@ -123,7 +121,7 @@ describe 'cycle analytics events' do def create_cycle milestone = create(:milestone, project: project) issue.update(milestone: milestone) - mr = create_merge_request_closing_issue(issue) + mr = create_merge_request_closing_issue(issue, commit_message: "References #{issue.to_reference}") pipeline = create(:ci_empty_pipeline, status: 'created', project: project, ref: mr.source_branch, sha: mr.source_branch_sha) pipeline.run diff --git a/spec/support/cycle_analytics_helpers.rb b/spec/support/cycle_analytics_helpers.rb index 8ad042f5e3b..66545127a44 100644 --- a/spec/support/cycle_analytics_helpers.rb +++ b/spec/support/cycle_analytics_helpers.rb @@ -20,7 +20,7 @@ module CycleAnalyticsHelpers ref: 'refs/heads/master').execute end - def create_merge_request_closing_issue(issue, message: nil, source_branch: nil) + def create_merge_request_closing_issue(issue, message: nil, source_branch: nil, commit_message: 'commit message') if !source_branch || project.repository.commit(source_branch).blank? source_branch = generate(:branch) project.repository.add_branch(user, source_branch, 'master') @@ -30,7 +30,7 @@ module CycleAnalyticsHelpers user, generate(:branch), 'content', - message: 'commit message', + message: commit_message, branch_name: source_branch) project.repository.commit(sha) -- cgit v1.2.3 From 936367538043854c7b093b71ca315b8e469c55a4 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 5 May 2017 12:25:24 +0200 Subject: Use update build policy instead of new play policy --- app/policies/ci/build_policy.rb | 12 +++++++----- app/policies/environment_policy.rb | 2 +- app/serializers/build_action_entity.rb | 2 +- app/serializers/build_entity.rb | 2 +- app/services/ci/play_build_service.rb | 2 +- app/views/projects/ci/builds/_build.html.haml | 2 +- lib/gitlab/ci/status/build/play.rb | 2 +- spec/policies/ci/build_policy_spec.rb | 18 +++++++++--------- 8 files changed, 22 insertions(+), 20 deletions(-) diff --git a/app/policies/ci/build_policy.rb b/app/policies/ci/build_policy.rb index 2c39d31488f..d4af4490608 100644 --- a/app/policies/ci/build_policy.rb +++ b/app/policies/ci/build_policy.rb @@ -1,5 +1,7 @@ module Ci class BuildPolicy < CommitStatusPolicy + alias_method :build, :subject + def rules super @@ -9,17 +11,17 @@ module Ci cannot! :"#{rule}_commit_status" unless can? :"#{rule}_build" end - can! :play_build if can_play_action? + if can?(:update_build) && protected_action? + cannot! :update_build + end end private - alias_method :build, :subject - - def can_play_action? + def protected_action? return false unless build.action? - ::Gitlab::UserAccess + !::Gitlab::UserAccess .new(user, project: build.project) .can_push_to_branch?(build.ref) end diff --git a/app/policies/environment_policy.rb b/app/policies/environment_policy.rb index cc94d4a7e05..2fa15e64562 100644 --- a/app/policies/environment_policy.rb +++ b/app/policies/environment_policy.rb @@ -12,6 +12,6 @@ class EnvironmentPolicy < BasePolicy private def can_play_stop_action? - Ability.allowed?(user, :play_build, environment.stop_action) + Ability.allowed?(user, :update_build, environment.stop_action) end end diff --git a/app/serializers/build_action_entity.rb b/app/serializers/build_action_entity.rb index 3eff724ae91..75dda1af709 100644 --- a/app/serializers/build_action_entity.rb +++ b/app/serializers/build_action_entity.rb @@ -19,6 +19,6 @@ class BuildActionEntity < Grape::Entity alias_method :build, :object def playable? - build.playable? && can?(request.user, :play_build, build) + build.playable? && can?(request.user, :update_build, build) end end diff --git a/app/serializers/build_entity.rb b/app/serializers/build_entity.rb index 10ba7c90c10..1380b347d8e 100644 --- a/app/serializers/build_entity.rb +++ b/app/serializers/build_entity.rb @@ -26,7 +26,7 @@ class BuildEntity < Grape::Entity alias_method :build, :object def playable? - build.playable? && can?(request.user, :play_build, build) + build.playable? && can?(request.user, :update_build, build) end def detailed_status diff --git a/app/services/ci/play_build_service.rb b/app/services/ci/play_build_service.rb index 5b3ff1ced38..e24f48c2d16 100644 --- a/app/services/ci/play_build_service.rb +++ b/app/services/ci/play_build_service.rb @@ -1,7 +1,7 @@ module Ci class PlayBuildService < ::BaseService def execute(build) - unless can?(current_user, :play_build, build) + unless can?(current_user, :update_build, build) raise Gitlab::Access::AccessDeniedError end diff --git a/app/views/projects/ci/builds/_build.html.haml b/app/views/projects/ci/builds/_build.html.haml index 2227d36eed2..c0019996176 100644 --- a/app/views/projects/ci/builds/_build.html.haml +++ b/app/views/projects/ci/builds/_build.html.haml @@ -102,7 +102,7 @@ = link_to cancel_namespace_project_build_path(job.project.namespace, job.project, job, return_to: request.original_url), method: :post, title: 'Cancel', class: 'btn btn-build' do = icon('remove', class: 'cred') - elsif allow_retry - - if job.playable? && !admin && can?(current_user, :play_build, job) + - if job.playable? && !admin && can?(current_user, :update_build, job) = link_to play_namespace_project_build_path(job.project.namespace, job.project, job, return_to: request.original_url), method: :post, title: 'Play', class: 'btn btn-build' do = custom_icon('icon_play') - elsif job.retryable? diff --git a/lib/gitlab/ci/status/build/play.rb b/lib/gitlab/ci/status/build/play.rb index 8130c7d1c90..fae34a2f927 100644 --- a/lib/gitlab/ci/status/build/play.rb +++ b/lib/gitlab/ci/status/build/play.rb @@ -14,7 +14,7 @@ module Gitlab end def has_action? - can?(user, :play_build, subject) + can?(user, :update_build, subject) end def action_icon diff --git a/spec/policies/ci/build_policy_spec.rb b/spec/policies/ci/build_policy_spec.rb index e4693cdcef0..3f4ce222b60 100644 --- a/spec/policies/ci/build_policy_spec.rb +++ b/spec/policies/ci/build_policy_spec.rb @@ -108,8 +108,8 @@ describe Ci::BuildPolicy, :models do create(:ci_build, :manual, ref: 'some-ref', pipeline: pipeline) end - it 'does not include ability to play build' do - expect(policies).not_to include :play_build + it 'does not include ability to update build' do + expect(policies).not_to include :update_build end end @@ -118,8 +118,8 @@ describe Ci::BuildPolicy, :models do create(:ci_build, ref: 'some-ref', pipeline: pipeline) end - it 'does not include ability to play build' do - expect(policies).not_to include :play_build + it 'includes ability to update build' do + expect(policies).to include :update_build end end end @@ -128,16 +128,16 @@ describe Ci::BuildPolicy, :models do context 'when build is a manual action' do let(:build) { create(:ci_build, :manual, pipeline: pipeline) } - it 'includes ability to play build' do - expect(policies).to include :play_build + it 'includes ability to update build' do + expect(policies).to include :update_build end end context 'when build is not a manual action' do - let(:build) { create(:ci_build, pipeline: pipeline) } + let(:build) { create(:ci_build, pipeline: pipeline) } - it 'does not include ability to play build' do - expect(policies).not_to include :play_build + it 'includes ability to update build' do + expect(policies).to include :update_build end end end -- cgit v1.2.3 From 61dd92aaff822759941bb224de9f45bfc5f7cc9b Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 5 May 2017 13:24:07 +0200 Subject: Authorize build update on per object basis --- app/controllers/projects/application_controller.rb | 8 +++++--- app/controllers/projects/builds_controller.rb | 23 ++++++++++++++++++---- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb index 89f1128ec36..afed0ac05a0 100644 --- a/app/controllers/projects/application_controller.rb +++ b/app/controllers/projects/application_controller.rb @@ -55,13 +55,15 @@ class Projects::ApplicationController < ApplicationController (current_user && current_user.already_forked?(project)) end - def authorize_project!(action) - return access_denied! unless can?(current_user, action, project) + def authorize_action!(action) + unless can?(current_user, action, project) + return access_denied! + end end def method_missing(method_sym, *arguments, &block) if method_sym.to_s =~ /\Aauthorize_(.*)!\z/ - authorize_project!($1.to_sym) + authorize_action!($1.to_sym) else super end diff --git a/app/controllers/projects/builds_controller.rb b/app/controllers/projects/builds_controller.rb index e24fc45d166..d97bc93f8dc 100644 --- a/app/controllers/projects/builds_controller.rb +++ b/app/controllers/projects/builds_controller.rb @@ -1,7 +1,11 @@ class Projects::BuildsController < Projects::ApplicationController before_action :build, except: [:index, :cancel_all] - before_action :authorize_read_build!, only: [:index, :show, :status, :raw, :trace] - before_action :authorize_update_build!, except: [:index, :show, :status, :raw, :trace] + + before_action :authorize_read_build!, + only: [:index, :show, :status, :raw, :trace] + before_action :authorize_update_build!, + except: [:index, :show, :status, :raw, :trace, :cancel_all] + layout 'project' def index @@ -28,7 +32,12 @@ class Projects::BuildsController < Projects::ApplicationController end def cancel_all - @project.builds.running_or_pending.each(&:cancel) + return access_denied! unless can?(current_user, :update_build, project) + + @project.builds.running_or_pending.each do |build| + build.cancel if can?(current_user, :update_build, build) + end + redirect_to namespace_project_builds_path(project.namespace, project) end @@ -107,8 +116,14 @@ class Projects::BuildsController < Projects::ApplicationController private + def authorize_update_build! + return access_denied! unless can?(current_user, :update_build, build) + end + def build - @build ||= project.builds.find_by!(id: params[:id]).present(current_user: current_user) + @build ||= project.builds + .find_by!(id: params[:id]) + .present(current_user: current_user) end def build_path(build) -- cgit v1.2.3 From 3264e09c6fbe07831db74b83d6a6620d9f8f47d9 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 5 May 2017 13:25:48 +0200 Subject: Require build to be present in the controller --- app/controllers/projects/builds_controller.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/controllers/projects/builds_controller.rb b/app/controllers/projects/builds_controller.rb index d97bc93f8dc..0fd35bcb790 100644 --- a/app/controllers/projects/builds_controller.rb +++ b/app/controllers/projects/builds_controller.rb @@ -121,8 +121,7 @@ class Projects::BuildsController < Projects::ApplicationController end def build - @build ||= project.builds - .find_by!(id: params[:id]) + @build ||= project.builds.find(params[:id]) .present(current_user: current_user) end -- cgit v1.2.3 From 53219857dd9f97516c6f24f6efb4f405998d9ff2 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 5 May 2017 13:56:07 +0200 Subject: Check ability to update build on the API resource --- lib/api/jobs.rb | 9 +++++++-- lib/api/v3/builds.rb | 10 +++++++--- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/lib/api/jobs.rb b/lib/api/jobs.rb index 288b03d940c..0223957fde1 100644 --- a/lib/api/jobs.rb +++ b/lib/api/jobs.rb @@ -132,6 +132,7 @@ module API authorize_update_builds! build = get_build!(params[:job_id]) + authorize!(:update_build, build) build.cancel @@ -148,6 +149,7 @@ module API authorize_update_builds! build = get_build!(params[:job_id]) + authorize!(:update_build, build) return forbidden!('Job is not retryable') unless build.retryable? build = Ci::Build.retry(build, current_user) @@ -165,6 +167,7 @@ module API authorize_update_builds! build = get_build!(params[:job_id]) + authorize!(:update_build, build) return forbidden!('Job is not erasable!') unless build.erasable? build.erase(erased_by: current_user) @@ -181,6 +184,7 @@ module API authorize_update_builds! build = get_build!(params[:job_id]) + authorize!(:update_build, build) return not_found!(build) unless build.artifacts? build.keep_artifacts! @@ -201,6 +205,7 @@ module API build = get_build!(params[:job_id]) + authorize!(:update_build, build) bad_request!("Unplayable Job") unless build.playable? build.play(current_user) @@ -211,12 +216,12 @@ module API end helpers do - def get_build(id) + def find_build(id) user_project.builds.find_by(id: id.to_i) end def get_build!(id) - get_build(id) || not_found! + find_build(id) || not_found! end def present_artifacts!(artifacts_file) diff --git a/lib/api/v3/builds.rb b/lib/api/v3/builds.rb index 4dd03cdf24b..21935922414 100644 --- a/lib/api/v3/builds.rb +++ b/lib/api/v3/builds.rb @@ -134,6 +134,7 @@ module API authorize_update_builds! build = get_build!(params[:build_id]) + authorize!(:update_build, build) build.cancel @@ -150,6 +151,7 @@ module API authorize_update_builds! build = get_build!(params[:build_id]) + authorize!(:update_build, build) return forbidden!('Build is not retryable') unless build.retryable? build = Ci::Build.retry(build, current_user) @@ -167,6 +169,7 @@ module API authorize_update_builds! build = get_build!(params[:build_id]) + authorize!(:update_build, build) return forbidden!('Build is not erasable!') unless build.erasable? build.erase(erased_by: current_user) @@ -183,6 +186,7 @@ module API authorize_update_builds! build = get_build!(params[:build_id]) + authorize!(:update_build, build) return not_found!(build) unless build.artifacts? build.keep_artifacts! @@ -202,7 +206,7 @@ module API authorize_read_builds! build = get_build!(params[:build_id]) - + authorize!(:update_build, build) bad_request!("Unplayable Job") unless build.playable? build.play(current_user) @@ -213,12 +217,12 @@ module API end helpers do - def get_build(id) + def find_build(id) user_project.builds.find_by(id: id.to_i) end def get_build!(id) - get_build(id) || not_found! + find_build(id) || not_found! end def present_artifacts!(artifacts_file) -- cgit v1.2.3 From 2cc8f43e54d2b653a4f2e80c57339acb11dcba86 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 5 May 2017 15:13:58 +0200 Subject: Introduce generic manual action extended status class --- lib/gitlab/ci/status/build/action.rb | 23 ++++++++++++++++ lib/gitlab/ci/status/build/factory.rb | 3 ++- lib/gitlab/ci/status/build/play.rb | 6 +---- spec/lib/gitlab/ci/status/build/factory_spec.rb | 32 ++++++++++++---------- spec/lib/gitlab/ci/status/build/play_spec.rb | 36 +++++++++---------------- 5 files changed, 57 insertions(+), 43 deletions(-) create mode 100644 lib/gitlab/ci/status/build/action.rb diff --git a/lib/gitlab/ci/status/build/action.rb b/lib/gitlab/ci/status/build/action.rb new file mode 100644 index 00000000000..1397c35145a --- /dev/null +++ b/lib/gitlab/ci/status/build/action.rb @@ -0,0 +1,23 @@ +module Gitlab + module Ci + module Status + module Build + class Action < SimpleDelegator + include Status::Extended + + def label + if has_action? + __getobj__.label + else + "#{__getobj__.label} (not allowed)" + end + end + + def self.matches?(build, user) + build.action? + end + end + end + end + end +end diff --git a/lib/gitlab/ci/status/build/factory.rb b/lib/gitlab/ci/status/build/factory.rb index 38ac6edc9f1..c852d607373 100644 --- a/lib/gitlab/ci/status/build/factory.rb +++ b/lib/gitlab/ci/status/build/factory.rb @@ -8,7 +8,8 @@ module Gitlab Status::Build::Retryable], [Status::Build::FailedAllowed, Status::Build::Play, - Status::Build::Stop]] + Status::Build::Stop], + [Status::Build::Action]] end def self.common_helpers diff --git a/lib/gitlab/ci/status/build/play.rb b/lib/gitlab/ci/status/build/play.rb index fae34a2f927..3495b8d0448 100644 --- a/lib/gitlab/ci/status/build/play.rb +++ b/lib/gitlab/ci/status/build/play.rb @@ -6,11 +6,7 @@ module Gitlab include Status::Extended def label - if has_action? - 'manual play action' - else - 'manual play action (not allowed)' - end + 'manual play action' end def has_action? diff --git a/spec/lib/gitlab/ci/status/build/factory_spec.rb b/spec/lib/gitlab/ci/status/build/factory_spec.rb index 2de00c28945..39c5e3658d9 100644 --- a/spec/lib/gitlab/ci/status/build/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/build/factory_spec.rb @@ -204,11 +204,12 @@ describe Gitlab::Ci::Status::Build::Factory do it 'matches correct extended statuses' do expect(factory.extended_statuses) - .to eq [Gitlab::Ci::Status::Build::Play] + .to eq [Gitlab::Ci::Status::Build::Play, + Gitlab::Ci::Status::Build::Action] end - it 'fabricates a play detailed status' do - expect(status).to be_a Gitlab::Ci::Status::Build::Play + it 'fabricates action detailed status' do + expect(status).to be_a Gitlab::Ci::Status::Build::Action end it 'fabricates status with correct details' do @@ -247,21 +248,24 @@ describe Gitlab::Ci::Status::Build::Factory do it 'matches correct extended statuses' do expect(factory.extended_statuses) - .to eq [Gitlab::Ci::Status::Build::Stop] + .to eq [Gitlab::Ci::Status::Build::Stop, + Gitlab::Ci::Status::Build::Action] end - it 'fabricates a stop detailed status' do - expect(status).to be_a Gitlab::Ci::Status::Build::Stop + it 'fabricates action detailed status' do + expect(status).to be_a Gitlab::Ci::Status::Build::Action end - it 'fabricates status with correct details' do - expect(status.text).to eq 'manual' - expect(status.group).to eq 'manual' - expect(status.icon).to eq 'icon_status_manual' - expect(status.favicon).to eq 'favicon_status_manual' - expect(status.label).to eq 'manual stop action' - expect(status).to have_details - expect(status).to have_action + context 'when user is not allowed to execute manual action' do + it 'fabricates status with correct details' do + expect(status.text).to eq 'manual' + expect(status.group).to eq 'manual' + expect(status.icon).to eq 'icon_status_manual' + expect(status.favicon).to eq 'favicon_status_manual' + expect(status.label).to eq 'manual stop action (not allowed)' + expect(status).to have_details + expect(status).not_to have_action + end end end end diff --git a/spec/lib/gitlab/ci/status/build/play_spec.rb b/spec/lib/gitlab/ci/status/build/play_spec.rb index abefdbe7c66..f5d0f977768 100644 --- a/spec/lib/gitlab/ci/status/build/play_spec.rb +++ b/spec/lib/gitlab/ci/status/build/play_spec.rb @@ -7,38 +7,28 @@ describe Gitlab::Ci::Status::Build::Play do subject { described_class.new(status) } - context 'when user is allowed to update build' do - context 'when user can push to branch' do - before { build.project.add_master(user) } + describe '#label' do + it 'has a label that says it is a manual action' do + expect(subject.label).to eq 'manual play action' + end + end + + describe '#has_action?' do + context 'when user is allowed to update build' do + context 'when user can push to branch' do + before { build.project.add_master(user) } - describe '#has_action?' do it { is_expected.to have_action } end - describe '#label' do - it 'has a label that says it is a manual action' do - expect(subject.label).to eq 'manual play action' - end - end - end + context 'when user can not push to the branch' do + before { build.project.add_developer(user) } - context 'when user can not push to the branch' do - before { build.project.add_developer(user) } - - describe 'has_action?' do it { is_expected.not_to have_action } end - - describe '#label' do - it 'has a label that says user is not allowed to play it' do - expect(subject.label).to eq 'manual play action (not allowed)' - end - end end - end - context 'when user is not allowed to update build' do - describe '#has_action?' do + context 'when user is not allowed to update build' do it { is_expected.not_to have_action } end end -- cgit v1.2.3 From 55cec2177cd97a5c0939ab34b01aa408de887d1a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 5 May 2017 15:21:06 +0200 Subject: Refine inheritance model of extended CI/CD statuses --- lib/gitlab/ci/status/build/action.rb | 8 +++----- lib/gitlab/ci/status/build/cancelable.rb | 4 +--- lib/gitlab/ci/status/build/failed_allowed.rb | 4 +--- lib/gitlab/ci/status/build/play.rb | 4 +--- lib/gitlab/ci/status/build/retryable.rb | 4 +--- lib/gitlab/ci/status/build/stop.rb | 4 +--- lib/gitlab/ci/status/extended.rb | 12 ++++++------ lib/gitlab/ci/status/pipeline/blocked.rb | 4 +--- lib/gitlab/ci/status/success_warning.rb | 4 +--- spec/lib/gitlab/ci/status/build/factory_spec.rb | 2 +- spec/lib/gitlab/ci/status/extended_spec.rb | 6 +----- 11 files changed, 18 insertions(+), 38 deletions(-) diff --git a/lib/gitlab/ci/status/build/action.rb b/lib/gitlab/ci/status/build/action.rb index 1397c35145a..45fd0d4aa07 100644 --- a/lib/gitlab/ci/status/build/action.rb +++ b/lib/gitlab/ci/status/build/action.rb @@ -2,14 +2,12 @@ module Gitlab module Ci module Status module Build - class Action < SimpleDelegator - include Status::Extended - + class Action < Status::Extended def label if has_action? - __getobj__.label + @status.label else - "#{__getobj__.label} (not allowed)" + "#{@status.label} (not allowed)" end end diff --git a/lib/gitlab/ci/status/build/cancelable.rb b/lib/gitlab/ci/status/build/cancelable.rb index 67bbc3c4849..57b533bad99 100644 --- a/lib/gitlab/ci/status/build/cancelable.rb +++ b/lib/gitlab/ci/status/build/cancelable.rb @@ -2,9 +2,7 @@ module Gitlab module Ci module Status module Build - class Cancelable < SimpleDelegator - include Status::Extended - + class Cancelable < Status::Extended def has_action? can?(user, :update_build, subject) end diff --git a/lib/gitlab/ci/status/build/failed_allowed.rb b/lib/gitlab/ci/status/build/failed_allowed.rb index 807afe24bd5..e42d3574357 100644 --- a/lib/gitlab/ci/status/build/failed_allowed.rb +++ b/lib/gitlab/ci/status/build/failed_allowed.rb @@ -2,9 +2,7 @@ module Gitlab module Ci module Status module Build - class FailedAllowed < SimpleDelegator - include Status::Extended - + class FailedAllowed < Status::Extended def label 'failed (allowed to fail)' end diff --git a/lib/gitlab/ci/status/build/play.rb b/lib/gitlab/ci/status/build/play.rb index 3495b8d0448..c6139f1b716 100644 --- a/lib/gitlab/ci/status/build/play.rb +++ b/lib/gitlab/ci/status/build/play.rb @@ -2,9 +2,7 @@ module Gitlab module Ci module Status module Build - class Play < SimpleDelegator - include Status::Extended - + class Play < Status::Extended def label 'manual play action' end diff --git a/lib/gitlab/ci/status/build/retryable.rb b/lib/gitlab/ci/status/build/retryable.rb index 6b362af7634..505f80848b2 100644 --- a/lib/gitlab/ci/status/build/retryable.rb +++ b/lib/gitlab/ci/status/build/retryable.rb @@ -2,9 +2,7 @@ module Gitlab module Ci module Status module Build - class Retryable < SimpleDelegator - include Status::Extended - + class Retryable < Status::Extended def has_action? can?(user, :update_build, subject) end diff --git a/lib/gitlab/ci/status/build/stop.rb b/lib/gitlab/ci/status/build/stop.rb index e8530f2aaae..0b5199e5483 100644 --- a/lib/gitlab/ci/status/build/stop.rb +++ b/lib/gitlab/ci/status/build/stop.rb @@ -2,9 +2,7 @@ module Gitlab module Ci module Status module Build - class Stop < SimpleDelegator - include Status::Extended - + class Stop < Status::Extended def label 'manual stop action' end diff --git a/lib/gitlab/ci/status/extended.rb b/lib/gitlab/ci/status/extended.rb index d367c9bda69..1e8101f8949 100644 --- a/lib/gitlab/ci/status/extended.rb +++ b/lib/gitlab/ci/status/extended.rb @@ -1,13 +1,13 @@ module Gitlab module Ci module Status - module Extended - extend ActiveSupport::Concern + class Extended < SimpleDelegator + def initialize(status) + super(@status = status) + end - class_methods do - def matches?(_subject, _user) - raise NotImplementedError - end + def self.matches?(_subject, _user) + raise NotImplementedError end end end diff --git a/lib/gitlab/ci/status/pipeline/blocked.rb b/lib/gitlab/ci/status/pipeline/blocked.rb index a250c3fcb41..37dfe43fb62 100644 --- a/lib/gitlab/ci/status/pipeline/blocked.rb +++ b/lib/gitlab/ci/status/pipeline/blocked.rb @@ -2,9 +2,7 @@ module Gitlab module Ci module Status module Pipeline - class Blocked < SimpleDelegator - include Status::Extended - + class Blocked < Status::Extended def text 'blocked' end diff --git a/lib/gitlab/ci/status/success_warning.rb b/lib/gitlab/ci/status/success_warning.rb index d4cdab6957a..df6e76b0151 100644 --- a/lib/gitlab/ci/status/success_warning.rb +++ b/lib/gitlab/ci/status/success_warning.rb @@ -5,9 +5,7 @@ module Gitlab # Extended status used when pipeline or stage passed conditionally. # This means that failed jobs that are allowed to fail were present. # - class SuccessWarning < SimpleDelegator - include Status::Extended - + class SuccessWarning < Status::Extended def text 'passed' end diff --git a/spec/lib/gitlab/ci/status/build/factory_spec.rb b/spec/lib/gitlab/ci/status/build/factory_spec.rb index 39c5e3658d9..185bb9098da 100644 --- a/spec/lib/gitlab/ci/status/build/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/build/factory_spec.rb @@ -205,7 +205,7 @@ describe Gitlab::Ci::Status::Build::Factory do it 'matches correct extended statuses' do expect(factory.extended_statuses) .to eq [Gitlab::Ci::Status::Build::Play, - Gitlab::Ci::Status::Build::Action] + Gitlab::Ci::Status::Build::Action] end it 'fabricates action detailed status' do diff --git a/spec/lib/gitlab/ci/status/extended_spec.rb b/spec/lib/gitlab/ci/status/extended_spec.rb index c2d74ca5cde..6eacb07078b 100644 --- a/spec/lib/gitlab/ci/status/extended_spec.rb +++ b/spec/lib/gitlab/ci/status/extended_spec.rb @@ -1,12 +1,8 @@ require 'spec_helper' describe Gitlab::Ci::Status::Extended do - subject do - Class.new.include(described_class) - end - it 'requires subclass to implement matcher' do - expect { subject.matches?(double, double) } + expect { described_class.matches?(double, double) } .to raise_error(NotImplementedError) end end -- cgit v1.2.3 From e5f24c5490294a4e990103da24e9861f21d7bfd9 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 5 May 2017 15:31:00 +0200 Subject: Add specs for extended status for manual actions --- spec/lib/gitlab/ci/status/build/action_spec.rb | 56 ++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 spec/lib/gitlab/ci/status/build/action_spec.rb diff --git a/spec/lib/gitlab/ci/status/build/action_spec.rb b/spec/lib/gitlab/ci/status/build/action_spec.rb new file mode 100644 index 00000000000..8c25f72804b --- /dev/null +++ b/spec/lib/gitlab/ci/status/build/action_spec.rb @@ -0,0 +1,56 @@ +require 'spec_helper' + +describe Gitlab::Ci::Status::Build::Action do + let(:status) { double('core status') } + let(:user) { double('user') } + + subject do + described_class.new(status) + end + + describe '#label' do + before do + allow(status).to receive(:label).and_return('label') + end + + context 'when status has action' do + before do + allow(status).to receive(:has_action?).and_return(true) + end + + it 'does not append text' do + expect(subject.label).to eq 'label' + end + end + + context 'when status does not have action' do + before do + allow(status).to receive(:has_action?).and_return(false) + end + + it 'appends text about action not allowed' do + expect(subject.label).to eq 'label (not allowed)' + end + end + end + + describe '.matches?' do + subject { described_class.matches?(build, user) } + + context 'when build is an action' do + let(:build) { create(:ci_build, :manual) } + + it 'is a correct match' do + expect(subject).to be true + end + end + + context 'when build is not manual' do + let(:build) { create(:ci_build) } + + it 'does not match' do + expect(subject).to be false + end + end + end +end -- cgit v1.2.3 From de59bab80989eba44c38a129a235900cf71ff0b5 Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Fri, 5 May 2017 16:42:56 +0100 Subject: Another attempt at access_control_ce_spec --- spec/features/protected_tags/access_control_ce_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/protected_tags/access_control_ce_spec.rb b/spec/features/protected_tags/access_control_ce_spec.rb index a04fbcdd15f..12622cd548a 100644 --- a/spec/features/protected_tags/access_control_ce_spec.rb +++ b/spec/features/protected_tags/access_control_ce_spec.rb @@ -9,7 +9,7 @@ RSpec.shared_examples "protected tags > access control > CE" do allowed_to_create_button = find(".js-allowed-to-create") unless allowed_to_create_button.text == access_type_name - allowed_to_create_button.click + allowed_to_create_button.trigger('click') find('.create_access_levels-container .dropdown-menu li', match: :first) within('.create_access_levels-container .dropdown-menu') { click_on access_type_name } end -- cgit v1.2.3 From fc121cca5ba87abd24afbc8da2f76e14e386e4c8 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 5 May 2017 19:35:32 +0200 Subject: Do not reprocess actions when user retries pipeline User who is not allowed to trigger manual actions should not be allowed to reprocess / retrigger / retry these actions. --- app/services/ci/retry_pipeline_service.rb | 2 ++ spec/services/ci/retry_pipeline_service_spec.rb | 44 ++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/app/services/ci/retry_pipeline_service.rb b/app/services/ci/retry_pipeline_service.rb index ecc6173a96a..5b207157345 100644 --- a/app/services/ci/retry_pipeline_service.rb +++ b/app/services/ci/retry_pipeline_service.rb @@ -8,6 +8,8 @@ module Ci end pipeline.retryable_builds.find_each do |build| + next unless can?(current_user, :update_build, build) + Ci::RetryBuildService.new(project, current_user) .reprocess(build) end diff --git a/spec/services/ci/retry_pipeline_service_spec.rb b/spec/services/ci/retry_pipeline_service_spec.rb index f1b2d3a4798..40e151545c9 100644 --- a/spec/services/ci/retry_pipeline_service_spec.rb +++ b/spec/services/ci/retry_pipeline_service_spec.rb @@ -7,7 +7,9 @@ describe Ci::RetryPipelineService, '#execute', :services do let(:service) { described_class.new(project, user) } context 'when user has ability to modify pipeline' do - let(:user) { create(:admin) } + before do + project.add_master(user) + end context 'when there are already retried jobs present' do before do @@ -227,6 +229,46 @@ describe Ci::RetryPipelineService, '#execute', :services do end end + context 'when user is not allowed to trigger manual action' do + before do + project.add_developer(user) + end + + context 'when there is a failed manual action present' do + before do + create_build('test', :failed, 0) + create_build('deploy', :failed, 0, when: :manual) + create_build('verify', :canceled, 1) + end + + it 'does not reprocess manual action' do + service.execute(pipeline) + + expect(build('test')).to be_pending + expect(build('deploy')).to be_failed + expect(build('verify')).to be_created + expect(pipeline.reload).to be_running + end + end + + context 'when there is a failed manual action in later stage' do + before do + create_build('test', :failed, 0) + create_build('deploy', :failed, 1, when: :manual) + create_build('verify', :canceled, 2) + end + + it 'does not reprocess manual action' do + service.execute(pipeline) + + expect(build('test')).to be_pending + expect(build('deploy')).to be_failed + expect(build('verify')).to be_created + expect(pipeline.reload).to be_running + end + end + end + def statuses pipeline.reload.statuses end -- cgit v1.2.3 From 0d540530cad8d75ddf15de569944d7eb92cb56a5 Mon Sep 17 00:00:00 2001 From: Clement Ho Date: Fri, 5 May 2017 17:17:08 -0500 Subject: Standardize jasmine test describe block names that test specific methods --- spec/javascripts/behaviors/bind_in_out_spec.js | 12 ++++++------ spec/javascripts/blob/target_branch_dropdown_spec.js | 4 ++-- spec/javascripts/gl_form_spec.js | 6 +++--- spec/javascripts/helpers/class_spec_helper_spec.js | 2 +- spec/javascripts/line_highlighter_spec.js | 8 ++++---- spec/javascripts/merge_request_tabs_spec.js | 10 +++++----- spec/javascripts/shortcuts_issuable_spec.js | 2 +- spec/javascripts/version_check_image_spec.js | 2 +- spec/javascripts/visibility_select_spec.js | 6 +++--- 9 files changed, 26 insertions(+), 26 deletions(-) diff --git a/spec/javascripts/behaviors/bind_in_out_spec.js b/spec/javascripts/behaviors/bind_in_out_spec.js index dd9ab33289f..5ff66167718 100644 --- a/spec/javascripts/behaviors/bind_in_out_spec.js +++ b/spec/javascripts/behaviors/bind_in_out_spec.js @@ -2,7 +2,7 @@ import BindInOut from '~/behaviors/bind_in_out'; import ClassSpecHelper from '../helpers/class_spec_helper'; describe('BindInOut', function () { - describe('.constructor', function () { + describe('constructor', function () { beforeEach(function () { this.in = {}; this.out = {}; @@ -53,7 +53,7 @@ describe('BindInOut', function () { }); }); - describe('.addEvents', function () { + describe('addEvents', function () { beforeEach(function () { this.in = jasmine.createSpyObj('in', ['addEventListener']); @@ -79,7 +79,7 @@ describe('BindInOut', function () { }); }); - describe('.updateOut', function () { + describe('updateOut', function () { beforeEach(function () { this.in = { value: 'the-value' }; this.out = { textContent: 'not-the-value' }; @@ -98,7 +98,7 @@ describe('BindInOut', function () { }); }); - describe('.removeEvents', function () { + describe('removeEvents', function () { beforeEach(function () { this.in = jasmine.createSpyObj('in', ['removeEventListener']); this.updateOut = () => {}; @@ -122,7 +122,7 @@ describe('BindInOut', function () { }); }); - describe('.initAll', function () { + describe('initAll', function () { beforeEach(function () { this.ins = [0, 1, 2]; this.instances = []; @@ -153,7 +153,7 @@ describe('BindInOut', function () { }); }); - describe('.init', function () { + describe('init', function () { beforeEach(function () { spyOn(BindInOut.prototype, 'addEvents').and.callFake(function () { return this; }); spyOn(BindInOut.prototype, 'updateOut').and.callFake(function () { return this; }); diff --git a/spec/javascripts/blob/target_branch_dropdown_spec.js b/spec/javascripts/blob/target_branch_dropdown_spec.js index 4fb79663c51..bb436978a0f 100644 --- a/spec/javascripts/blob/target_branch_dropdown_spec.js +++ b/spec/javascripts/blob/target_branch_dropdown_spec.js @@ -63,7 +63,7 @@ describe('TargetBranchDropdown', () => { expect('change.branch').toHaveBeenTriggeredOn(dropdown.$dropdown); }); - describe('#dropdownData', () => { + describe('dropdownData', () => { it('cache the refs', () => { const refs = dropdown.cachedRefs; dropdown.cachedRefs = null; @@ -88,7 +88,7 @@ describe('TargetBranchDropdown', () => { }); }); - describe('#setNewBranch', () => { + describe('setNewBranch', () => { it('adds the new branch and select it', () => { const branchName = 'new_branch'; diff --git a/spec/javascripts/gl_form_spec.js b/spec/javascripts/gl_form_spec.js index 71d6e2a7e22..5ed18d0a76b 100644 --- a/spec/javascripts/gl_form_spec.js +++ b/spec/javascripts/gl_form_spec.js @@ -32,7 +32,7 @@ describe('GLForm', () => { }); }); - describe('.setupAutosize', () => { + describe('setupAutosize', () => { beforeEach((done) => { this.glForm.setupAutosize(); setTimeout(() => { @@ -59,7 +59,7 @@ describe('GLForm', () => { }); }); - describe('.setHeightData', () => { + describe('setHeightData', () => { beforeEach(() => { spyOn($.prototype, 'data'); spyOn($.prototype, 'outerHeight').and.returnValue(200); @@ -75,7 +75,7 @@ describe('GLForm', () => { }); }); - describe('.destroyAutosize', () => { + describe('destroyAutosize', () => { describe('when called', () => { beforeEach(() => { spyOn($.prototype, 'data'); diff --git a/spec/javascripts/helpers/class_spec_helper_spec.js b/spec/javascripts/helpers/class_spec_helper_spec.js index 0a61e561640..a1cfc8ba820 100644 --- a/spec/javascripts/helpers/class_spec_helper_spec.js +++ b/spec/javascripts/helpers/class_spec_helper_spec.js @@ -3,7 +3,7 @@ require('./class_spec_helper'); describe('ClassSpecHelper', () => { - describe('.itShouldBeAStaticMethod', function () { + describe('itShouldBeAStaticMethod', function () { beforeEach(() => { class TestClass { instanceMethod() { this.prop = 'val'; } diff --git a/spec/javascripts/line_highlighter_spec.js b/spec/javascripts/line_highlighter_spec.js index a1fd2d38968..b1b08989028 100644 --- a/spec/javascripts/line_highlighter_spec.js +++ b/spec/javascripts/line_highlighter_spec.js @@ -58,7 +58,7 @@ require('~/line_highlighter'); return expect(func).not.toThrow(); }); }); - describe('#clickHandler', function() { + describe('clickHandler', function() { it('handles clicking on a child icon element', function() { var spy; spy = spyOn(this["class"], 'setHash').and.callThrough(); @@ -176,7 +176,7 @@ require('~/line_highlighter'); }); }); }); - describe('#hashToRange', function() { + describe('hashToRange', function() { beforeEach(function() { return this.subject = this["class"].hashToRange; }); @@ -190,7 +190,7 @@ require('~/line_highlighter'); return expect(this.subject('#foo')).toEqual([null, null]); }); }); - describe('#highlightLine', function() { + describe('highlightLine', function() { beforeEach(function() { return this.subject = this["class"].highlightLine; }); @@ -203,7 +203,7 @@ require('~/line_highlighter'); return expect($('#LC13')).toHaveClass(this.css); }); }); - return describe('#setHash', function() { + return describe('setHash', function() { beforeEach(function() { return this.subject = this["class"].setHash; }); diff --git a/spec/javascripts/merge_request_tabs_spec.js b/spec/javascripts/merge_request_tabs_spec.js index e437333d522..254a41db160 100644 --- a/spec/javascripts/merge_request_tabs_spec.js +++ b/spec/javascripts/merge_request_tabs_spec.js @@ -47,7 +47,7 @@ require('vendor/jquery.scrollTo'); this.class.destroyPipelinesView(); }); - describe('#activateTab', function () { + describe('activateTab', function () { beforeEach(function () { spyOn($, 'ajax').and.callFake(function () {}); loadFixtures('merge_requests/merge_request_with_task_list.html.raw'); @@ -71,7 +71,7 @@ require('vendor/jquery.scrollTo'); }); }); - describe('#opensInNewTab', function () { + describe('opensInNewTab', function () { var tabUrl; var windowTarget = '_blank'; @@ -152,7 +152,7 @@ require('vendor/jquery.scrollTo'); }); }); - describe('#setCurrentAction', function () { + describe('setCurrentAction', function () { beforeEach(function () { spyOn($, 'ajax').and.callFake(function () {}); this.subject = this.class.setCurrentAction; @@ -221,7 +221,7 @@ require('vendor/jquery.scrollTo'); }); }); - describe('#tabShown', () => { + describe('tabShown', () => { beforeEach(function () { spyOn($, 'ajax').and.callFake(function (options) { options.success({ html: '' }); @@ -281,7 +281,7 @@ require('vendor/jquery.scrollTo'); }); }); - describe('#loadDiff', function () { + describe('loadDiff', function () { it('requires an absolute pathname', function () { spyOn($, 'ajax').and.callFake(function (options) { expect(options.url).toEqual('/foo/bar/merge_requests/1/diffs.json'); diff --git a/spec/javascripts/shortcuts_issuable_spec.js b/spec/javascripts/shortcuts_issuable_spec.js index 9e19dabd0e3..757b8d595a4 100644 --- a/spec/javascripts/shortcuts_issuable_spec.js +++ b/spec/javascripts/shortcuts_issuable_spec.js @@ -13,7 +13,7 @@ require('~/shortcuts_issuable'); document.querySelector('.js-new-note-form').classList.add('js-main-target-form'); this.shortcut = new ShortcutsIssuable(); }); - describe('#replyWithSelectedText', function() { + describe('replyWithSelectedText', function() { var stubSelection; // Stub window.gl.utils.getSelectedFragment to return a node with the provided HTML. stubSelection = function(html) { diff --git a/spec/javascripts/version_check_image_spec.js b/spec/javascripts/version_check_image_spec.js index 464c1fce210..83ffeca253a 100644 --- a/spec/javascripts/version_check_image_spec.js +++ b/spec/javascripts/version_check_image_spec.js @@ -3,7 +3,7 @@ const VersionCheckImage = require('~/version_check_image'); require('jquery'); describe('VersionCheckImage', function () { - describe('.bindErrorEvent', function () { + describe('bindErrorEvent', function () { ClassSpecHelper.itShouldBeAStaticMethod(VersionCheckImage, 'bindErrorEvent'); beforeEach(function () { diff --git a/spec/javascripts/visibility_select_spec.js b/spec/javascripts/visibility_select_spec.js index 9727c03c91e..b26ed41f27a 100644 --- a/spec/javascripts/visibility_select_spec.js +++ b/spec/javascripts/visibility_select_spec.js @@ -22,7 +22,7 @@ require('~/visibility_select'); spyOn(Element.prototype, 'querySelector').and.callFake(selector => mockElements[selector]); }); - describe('#constructor', function () { + describe('constructor', function () { beforeEach(function () { this.visibilitySelect = new VisibilitySelect(mockElements.container); }); @@ -48,7 +48,7 @@ require('~/visibility_select'); }); }); - describe('#init', function () { + describe('init', function () { describe('if there is a select', function () { beforeEach(function () { this.visibilitySelect = new VisibilitySelect(mockElements.container); @@ -85,7 +85,7 @@ require('~/visibility_select'); }); }); - describe('#updateHelpText', function () { + describe('updateHelpText', function () { beforeEach(function () { this.visibilitySelect = new VisibilitySelect(mockElements.container); this.visibilitySelect.init(); -- cgit v1.2.3 From 06455d2fc13ecab2d03904d6427e3e1e55328e02 Mon Sep 17 00:00:00 2001 From: Clement Ho Date: Fri, 5 May 2017 17:24:29 -0500 Subject: [skip ci] add documentation --- doc/development/fe_guide/testing.md | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/doc/development/fe_guide/testing.md b/doc/development/fe_guide/testing.md index 157c13352ca..a8c264bbd3c 100644 --- a/doc/development/fe_guide/testing.md +++ b/doc/development/fe_guide/testing.md @@ -29,6 +29,33 @@ browser and you will not have access to certain APIs, such as which will have to be stubbed. ### Writing tests + +When writing describe test blocks to test specific functions/methods, +please use the method name as the describe block name. + +```javascript +// Good +describe('methodName', () => { + it('passes', () => { + expect(true).toEqual(true); + }); +}); + +// Bad +describe('#methodName', () => { + it('passes', () => { + expect(true).toEqual(true); + }); +}); + +// Bad +describe('.methodName', () => { + it('passes', () => { + expect(true).toEqual(true); + }); +}); +``` + ### Vue.js unit tests See this [section][vue-test]. -- cgit v1.2.3 From 683be632a4ed86e2b7c218ff526688a2a66435de Mon Sep 17 00:00:00 2001 From: winh Date: Fri, 5 May 2017 21:14:58 +0200 Subject: Use absolute URLs for fixtures (!11133) --- spec/javascripts/test_bundle.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/javascripts/test_bundle.js b/spec/javascripts/test_bundle.js index 07dc51a7815..92428fd501c 100644 --- a/spec/javascripts/test_bundle.js +++ b/spec/javascripts/test_bundle.js @@ -1,8 +1,8 @@ // enable test fixtures require('jasmine-jquery'); -jasmine.getFixtures().fixturesPath = 'base/spec/javascripts/fixtures'; -jasmine.getJSONFixtures().fixturesPath = 'base/spec/javascripts/fixtures'; +jasmine.getFixtures().fixturesPath = '/base/spec/javascripts/fixtures'; +jasmine.getJSONFixtures().fixturesPath = '/base/spec/javascripts/fixtures'; // include common libraries require('~/commons/index.js'); -- cgit v1.2.3 From aa440eb1c0947d2dc551c61abbd9d271b9002050 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Sat, 6 May 2017 19:02:06 +0200 Subject: Single commit squash of all changes for https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10878 It's needed due to https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10777 being merged with squash. --- app/assets/javascripts/ci_status_icons.js | 34 ------ app/assets/javascripts/dispatcher.js | 3 +- .../javascripts/lib/utils/bootstrap_linked_tabs.js | 116 +++++++++--------- app/assets/javascripts/pipelines.js | 46 ++------ .../components/graph/action_component.vue | 59 ++++++++++ .../components/graph/dropdown_action_component.vue | 56 +++++++++ .../components/graph/dropdown_job_component.vue | 86 ++++++++++++++ .../pipelines/components/graph/graph_component.vue | 92 +++++++++++++++ .../pipelines/components/graph/job_component.vue | 124 ++++++++++++++++++++ .../components/graph/job_name_component.vue | 37 ++++++ .../components/graph/stage_column_component.vue | 64 ++++++++++ .../javascripts/pipelines/components/stage.vue | 6 +- app/assets/javascripts/pipelines/graph_bundle.js | 10 ++ .../pipelines/services/pipeline_service.js | 14 +++ .../javascripts/pipelines/stores/pipeline_store.js | 11 ++ .../javascripts/vue_shared/ci_action_icons.js | 22 ++++ .../javascripts/vue_shared/ci_status_icons.js | 55 +++++++++ .../javascripts/vue_shared/components/ci_icon.vue | 29 +++++ .../javascripts/vue_shared/mixins/tooltip.js | 9 ++ app/assets/stylesheets/pages/pipelines.scss | 69 ++++++----- app/views/ci/status/_graph_badge.html.haml | 20 ---- app/views/projects/pipelines/_graph.html.haml | 4 - app/views/projects/pipelines/_with_tabs.html.haml | 7 +- app/views/projects/stage/_graph.html.haml | 19 --- app/views/projects/stage/_in_stage_group.html.haml | 14 --- .../unreleased/25226-realtime-pipelines-fe.yml | 4 + config/webpack.config.js | 2 + spec/javascripts/bootstrap_linked_tabs_spec.js | 8 +- spec/javascripts/ci_status_icon_spec.js | 44 ------- spec/javascripts/fixtures/graph.html.haml | 1 + .../pipelines/graph/action_component_spec.js | 40 +++++++ .../graph/dropdown_action_component_spec.js | 30 +++++ .../pipelines/graph/graph_component_spec.js | 83 +++++++++++++ .../pipelines/graph/job_component_spec.js | 117 +++++++++++++++++++ .../pipelines/graph/job_name_component_spec.js | 27 +++++ .../pipelines/graph/stage_column_component_spec.js | 42 +++++++ spec/javascripts/pipelines_spec.js | 32 ++--- .../javascripts/vue_shared/ci_action_icons_spec.js | 22 ++++ spec/javascripts/vue_shared/ci_status_icon_spec.js | 27 +++++ .../vue_shared/components/ci_icon_spec.js | 130 +++++++++++++++++++++ 40 files changed, 1321 insertions(+), 294 deletions(-) delete mode 100644 app/assets/javascripts/ci_status_icons.js create mode 100644 app/assets/javascripts/pipelines/components/graph/action_component.vue create mode 100644 app/assets/javascripts/pipelines/components/graph/dropdown_action_component.vue create mode 100644 app/assets/javascripts/pipelines/components/graph/dropdown_job_component.vue create mode 100644 app/assets/javascripts/pipelines/components/graph/graph_component.vue create mode 100644 app/assets/javascripts/pipelines/components/graph/job_component.vue create mode 100644 app/assets/javascripts/pipelines/components/graph/job_name_component.vue create mode 100644 app/assets/javascripts/pipelines/components/graph/stage_column_component.vue create mode 100644 app/assets/javascripts/pipelines/graph_bundle.js create mode 100644 app/assets/javascripts/pipelines/services/pipeline_service.js create mode 100644 app/assets/javascripts/pipelines/stores/pipeline_store.js create mode 100644 app/assets/javascripts/vue_shared/ci_action_icons.js create mode 100644 app/assets/javascripts/vue_shared/ci_status_icons.js create mode 100644 app/assets/javascripts/vue_shared/components/ci_icon.vue create mode 100644 app/assets/javascripts/vue_shared/mixins/tooltip.js delete mode 100644 app/views/ci/status/_graph_badge.html.haml delete mode 100644 app/views/projects/pipelines/_graph.html.haml delete mode 100644 app/views/projects/stage/_graph.html.haml delete mode 100644 app/views/projects/stage/_in_stage_group.html.haml create mode 100644 changelogs/unreleased/25226-realtime-pipelines-fe.yml delete mode 100644 spec/javascripts/ci_status_icon_spec.js create mode 100644 spec/javascripts/fixtures/graph.html.haml create mode 100644 spec/javascripts/pipelines/graph/action_component_spec.js create mode 100644 spec/javascripts/pipelines/graph/dropdown_action_component_spec.js create mode 100644 spec/javascripts/pipelines/graph/graph_component_spec.js create mode 100644 spec/javascripts/pipelines/graph/job_component_spec.js create mode 100644 spec/javascripts/pipelines/graph/job_name_component_spec.js create mode 100644 spec/javascripts/pipelines/graph/stage_column_component_spec.js create mode 100644 spec/javascripts/vue_shared/ci_action_icons_spec.js create mode 100644 spec/javascripts/vue_shared/ci_status_icon_spec.js create mode 100644 spec/javascripts/vue_shared/components/ci_icon_spec.js diff --git a/app/assets/javascripts/ci_status_icons.js b/app/assets/javascripts/ci_status_icons.js deleted file mode 100644 index f16616873b2..00000000000 --- a/app/assets/javascripts/ci_status_icons.js +++ /dev/null @@ -1,34 +0,0 @@ -import CANCELED_SVG from 'icons/_icon_status_canceled_borderless.svg'; -import CREATED_SVG from 'icons/_icon_status_created_borderless.svg'; -import FAILED_SVG from 'icons/_icon_status_failed_borderless.svg'; -import MANUAL_SVG from 'icons/_icon_status_manual_borderless.svg'; -import PENDING_SVG from 'icons/_icon_status_pending_borderless.svg'; -import RUNNING_SVG from 'icons/_icon_status_running_borderless.svg'; -import SKIPPED_SVG from 'icons/_icon_status_skipped_borderless.svg'; -import SUCCESS_SVG from 'icons/_icon_status_success_borderless.svg'; -import WARNING_SVG from 'icons/_icon_status_warning_borderless.svg'; - -const StatusIconEntityMap = { - icon_status_canceled: CANCELED_SVG, - icon_status_created: CREATED_SVG, - icon_status_failed: FAILED_SVG, - icon_status_manual: MANUAL_SVG, - icon_status_pending: PENDING_SVG, - icon_status_running: RUNNING_SVG, - icon_status_skipped: SKIPPED_SVG, - icon_status_success: SUCCESS_SVG, - icon_status_warning: WARNING_SVG, -}; - -export { - CANCELED_SVG, - CREATED_SVG, - FAILED_SVG, - MANUAL_SVG, - PENDING_SVG, - RUNNING_SVG, - SKIPPED_SVG, - SUCCESS_SVG, - WARNING_SVG, - StatusIconEntityMap as default, -}; diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index b16ff2a0221..d27d89cf91d 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -49,6 +49,7 @@ import BlobForkSuggestion from './blob/blob_fork_suggestion'; import UserCallout from './user_callout'; import { ProtectedTagCreate, ProtectedTagEditList } from './protected_tags'; import ShortcutsWiki from './shortcuts_wiki'; +import Pipelines from './pipelines'; import BlobViewer from './blob/viewer/index'; import AutoWidthDropdownSelect from './issuable/auto_width_dropdown_select'; @@ -257,7 +258,7 @@ const ShortcutsBlob = require('./shortcuts_blob'); const { controllerAction } = document.querySelector('.js-pipeline-container').dataset; const pipelineStatusUrl = `${document.querySelector('.js-pipeline-tab-link a').getAttribute('href')}/status.json`; - new gl.Pipelines({ + new Pipelines({ initTabs: true, pipelineStatusUrl, tabsOptions: { diff --git a/app/assets/javascripts/lib/utils/bootstrap_linked_tabs.js b/app/assets/javascripts/lib/utils/bootstrap_linked_tabs.js index 2955bda1a36..0bf2ba6acc2 100644 --- a/app/assets/javascripts/lib/utils/bootstrap_linked_tabs.js +++ b/app/assets/javascripts/lib/utils/bootstrap_linked_tabs.js @@ -31,82 +31,78 @@ * * ### How to use * - * new window.gl.LinkedTabs({ + * new LinkedTabs({ * action: "#{controller.action_name}", * defaultAction: 'tab1', * parentEl: '.tab-links' * }); */ -(() => { - window.gl = window.gl || {}; +export default class LinkedTabs { + /** + * Binds the events and activates de default tab. + * + * @param {Object} options + */ + constructor(options = {}) { + this.options = options; - window.gl.LinkedTabs = class LinkedTabs { - /** - * Binds the events and activates de default tab. - * - * @param {Object} options - */ - constructor(options) { - this.options = options || {}; + this.defaultAction = this.options.defaultAction; + this.action = this.options.action || this.defaultAction; - this.defaultAction = this.options.defaultAction; - this.action = this.options.action || this.defaultAction; - - if (this.action === 'show') { - this.action = this.defaultAction; - } + if (this.action === 'show') { + this.action = this.defaultAction; + } - this.currentLocation = window.location; + this.currentLocation = window.location; - const tabSelector = `${this.options.parentEl} a[data-toggle="tab"]`; + const tabSelector = `${this.options.parentEl} a[data-toggle="tab"]`; - // since this is a custom event we need jQuery :( - $(document) - .off('shown.bs.tab', tabSelector) - .on('shown.bs.tab', tabSelector, e => this.tabShown(e)); + // since this is a custom event we need jQuery :( + $(document) + .off('shown.bs.tab', tabSelector) + .on('shown.bs.tab', tabSelector, e => this.tabShown(e)); - this.activateTab(this.action); - } + this.activateTab(this.action); + } - /** - * Handles the `shown.bs.tab` event to set the currect url action. - * - * @param {type} evt - * @return {Function} - */ - tabShown(evt) { - const source = evt.target.getAttribute('href'); + /** + * Handles the `shown.bs.tab` event to set the currect url action. + * + * @param {type} evt + * @return {Function} + */ + tabShown(evt) { + const source = evt.target.getAttribute('href'); - return this.setCurrentAction(source); - } + return this.setCurrentAction(source); + } - /** - * Updates the URL with the path that matched the given action. - * - * @param {String} source - * @return {String} - */ - setCurrentAction(source) { - const copySource = source; + /** + * Updates the URL with the path that matched the given action. + * + * @param {String} source + * @return {String} + */ + setCurrentAction(source) { + const copySource = source; - copySource.replace(/\/+$/, ''); + copySource.replace(/\/+$/, ''); - const newState = `${copySource}${this.currentLocation.search}${this.currentLocation.hash}`; + const newState = `${copySource}${this.currentLocation.search}${this.currentLocation.hash}`; - history.replaceState({ - url: newState, - }, document.title, newState); - return newState; - } + history.replaceState({ + url: newState, + }, document.title, newState); + return newState; + } - /** - * Given the current action activates the correct tab. - * http://getbootstrap.com/javascript/#tab-show - * Note: Will trigger `shown.bs.tab` - */ - activateTab() { - return $(`${this.options.parentEl} a[data-action='${this.action}']`).tab('show'); - } - }; -})(); + /** + * Given the current action activates the correct tab. + * http://getbootstrap.com/javascript/#tab-show + * Note: Will trigger `shown.bs.tab` + */ + activateTab() { + return $(`${this.options.parentEl} a[data-action='${this.action}']`).tab('show'); + } +} diff --git a/app/assets/javascripts/pipelines.js b/app/assets/javascripts/pipelines.js index 4252b615887..26a36ad54d1 100644 --- a/app/assets/javascripts/pipelines.js +++ b/app/assets/javascripts/pipelines.js @@ -1,42 +1,14 @@ -/* eslint-disable no-new, guard-for-in, no-restricted-syntax, no-continue, no-param-reassign, max-len */ +import LinkedTabs from './lib/utils/bootstrap_linked_tabs'; -require('./lib/utils/bootstrap_linked_tabs'); - -((global) => { - class Pipelines { - constructor(options = {}) { - if (options.initTabs && options.tabsOptions) { - new global.LinkedTabs(options.tabsOptions); - } - - if (options.pipelineStatusUrl) { - gl.utils.setCiStatusFavicon(options.pipelineStatusUrl); - } - - this.addMarginToBuildColumns(); +export default class Pipelines { + constructor(options = {}) { + if (options.initTabs && options.tabsOptions) { + // eslint-disable-next-line no-new + new LinkedTabs(options.tabsOptions); } - addMarginToBuildColumns() { - this.pipelineGraph = document.querySelector('.js-pipeline-graph'); - - const secondChildBuildNodes = this.pipelineGraph.querySelectorAll('.build:nth-child(2)'); - - for (const buildNodeIndex in secondChildBuildNodes) { - const buildNode = secondChildBuildNodes[buildNodeIndex]; - const firstChildBuildNode = buildNode.previousElementSibling; - if (!firstChildBuildNode || !firstChildBuildNode.matches('.build')) continue; - const multiBuildColumn = buildNode.closest('.stage-column'); - const previousColumn = multiBuildColumn.previousElementSibling; - if (!previousColumn || !previousColumn.matches('.stage-column')) continue; - multiBuildColumn.classList.add('left-margin'); - firstChildBuildNode.classList.add('left-connector'); - const columnBuilds = previousColumn.querySelectorAll('.build'); - if (columnBuilds.length === 1) previousColumn.classList.add('no-margin'); - } - - this.pipelineGraph.classList.remove('hidden'); + if (options.pipelineStatusUrl) { + gl.utils.setCiStatusFavicon(options.pipelineStatusUrl); } } - - global.Pipelines = Pipelines; -})(window.gl || (window.gl = {})); +} diff --git a/app/assets/javascripts/pipelines/components/graph/action_component.vue b/app/assets/javascripts/pipelines/components/graph/action_component.vue new file mode 100644 index 00000000000..14e485791ea --- /dev/null +++ b/app/assets/javascripts/pipelines/components/graph/action_component.vue @@ -0,0 +1,59 @@ + + diff --git a/app/assets/javascripts/pipelines/components/graph/dropdown_action_component.vue b/app/assets/javascripts/pipelines/components/graph/dropdown_action_component.vue new file mode 100644 index 00000000000..19cafff4e1c --- /dev/null +++ b/app/assets/javascripts/pipelines/components/graph/dropdown_action_component.vue @@ -0,0 +1,56 @@ + + diff --git a/app/assets/javascripts/pipelines/components/graph/dropdown_job_component.vue b/app/assets/javascripts/pipelines/components/graph/dropdown_job_component.vue new file mode 100644 index 00000000000..d597af8dfb5 --- /dev/null +++ b/app/assets/javascripts/pipelines/components/graph/dropdown_job_component.vue @@ -0,0 +1,86 @@ + + diff --git a/app/assets/javascripts/pipelines/components/graph/graph_component.vue b/app/assets/javascripts/pipelines/components/graph/graph_component.vue new file mode 100644 index 00000000000..a84161ef5e7 --- /dev/null +++ b/app/assets/javascripts/pipelines/components/graph/graph_component.vue @@ -0,0 +1,92 @@ + + diff --git a/app/assets/javascripts/pipelines/components/graph/job_component.vue b/app/assets/javascripts/pipelines/components/graph/job_component.vue new file mode 100644 index 00000000000..b39c936101e --- /dev/null +++ b/app/assets/javascripts/pipelines/components/graph/job_component.vue @@ -0,0 +1,124 @@ + + diff --git a/app/assets/javascripts/pipelines/components/graph/job_name_component.vue b/app/assets/javascripts/pipelines/components/graph/job_name_component.vue new file mode 100644 index 00000000000..d8856e10668 --- /dev/null +++ b/app/assets/javascripts/pipelines/components/graph/job_name_component.vue @@ -0,0 +1,37 @@ + + diff --git a/app/assets/javascripts/pipelines/components/graph/stage_column_component.vue b/app/assets/javascripts/pipelines/components/graph/stage_column_component.vue new file mode 100644 index 00000000000..b7da185e280 --- /dev/null +++ b/app/assets/javascripts/pipelines/components/graph/stage_column_component.vue @@ -0,0 +1,64 @@ + + diff --git a/app/assets/javascripts/pipelines/components/stage.vue b/app/assets/javascripts/pipelines/components/stage.vue index 2e485f951a1..dc42223269d 100644 --- a/app/assets/javascripts/pipelines/components/stage.vue +++ b/app/assets/javascripts/pipelines/components/stage.vue @@ -14,7 +14,7 @@ */ /* global Flash */ -import StatusIconEntityMap from '../../ci_status_icons'; +import { statusCssClasses, borderlessStatusIconEntityMap } from '../../vue_shared/ci_status_icons'; export default { props: { @@ -109,11 +109,11 @@ export default { }, triggerButtonClass() { - return `ci-status-icon-${this.stage.status.group}`; + return `ci-status-icon-${statusCssClasses[this.stage.status.icon]}`; }, svgIcon() { - return StatusIconEntityMap[this.stage.status.icon]; + return borderlessStatusIconEntityMap[this.stage.status.icon]; }, }, }; diff --git a/app/assets/javascripts/pipelines/graph_bundle.js b/app/assets/javascripts/pipelines/graph_bundle.js new file mode 100644 index 00000000000..b7a6b5d8479 --- /dev/null +++ b/app/assets/javascripts/pipelines/graph_bundle.js @@ -0,0 +1,10 @@ +import Vue from 'vue'; +import pipelineGraph from './components/graph/graph_component.vue'; + +document.addEventListener('DOMContentLoaded', () => new Vue({ + el: '#js-pipeline-graph-vue', + components: { + pipelineGraph, + }, + render: createElement => createElement('pipeline-graph'), +})); diff --git a/app/assets/javascripts/pipelines/services/pipeline_service.js b/app/assets/javascripts/pipelines/services/pipeline_service.js new file mode 100644 index 00000000000..f1cc60c1ee0 --- /dev/null +++ b/app/assets/javascripts/pipelines/services/pipeline_service.js @@ -0,0 +1,14 @@ +import Vue from 'vue'; +import VueResource from 'vue-resource'; + +Vue.use(VueResource); + +export default class PipelineService { + constructor(endpoint) { + this.pipeline = Vue.resource(endpoint); + } + + getPipeline() { + return this.pipeline.get(); + } +} diff --git a/app/assets/javascripts/pipelines/stores/pipeline_store.js b/app/assets/javascripts/pipelines/stores/pipeline_store.js new file mode 100644 index 00000000000..86ab50d8f1e --- /dev/null +++ b/app/assets/javascripts/pipelines/stores/pipeline_store.js @@ -0,0 +1,11 @@ +export default class PipelineStore { + constructor() { + this.state = {}; + + this.state.graph = []; + } + + storeGraph(graph = []) { + this.state.graph = graph; + } +} diff --git a/app/assets/javascripts/vue_shared/ci_action_icons.js b/app/assets/javascripts/vue_shared/ci_action_icons.js new file mode 100644 index 00000000000..734b3c6c45e --- /dev/null +++ b/app/assets/javascripts/vue_shared/ci_action_icons.js @@ -0,0 +1,22 @@ +import cancelSVG from 'icons/_icon_action_cancel.svg'; +import retrySVG from 'icons/_icon_action_retry.svg'; +import playSVG from 'icons/_icon_action_play.svg'; + +export default function getActionIcon(action) { + let icon; + switch (action) { + case 'icon_action_cancel': + icon = cancelSVG; + break; + case 'icon_action_retry': + icon = retrySVG; + break; + case 'icon_action_play': + icon = playSVG; + break; + default: + icon = ''; + } + + return icon; +} diff --git a/app/assets/javascripts/vue_shared/ci_status_icons.js b/app/assets/javascripts/vue_shared/ci_status_icons.js new file mode 100644 index 00000000000..48ad9214ac8 --- /dev/null +++ b/app/assets/javascripts/vue_shared/ci_status_icons.js @@ -0,0 +1,55 @@ +import BORDERLESS_CANCELED_SVG from 'icons/_icon_status_canceled_borderless.svg'; +import BORDERLESS_CREATED_SVG from 'icons/_icon_status_created_borderless.svg'; +import BORDERLESS_FAILED_SVG from 'icons/_icon_status_failed_borderless.svg'; +import BORDERLESS_MANUAL_SVG from 'icons/_icon_status_manual_borderless.svg'; +import BORDERLESS_PENDING_SVG from 'icons/_icon_status_pending_borderless.svg'; +import BORDERLESS_RUNNING_SVG from 'icons/_icon_status_running_borderless.svg'; +import BORDERLESS_SKIPPED_SVG from 'icons/_icon_status_skipped_borderless.svg'; +import BORDERLESS_SUCCESS_SVG from 'icons/_icon_status_success_borderless.svg'; +import BORDERLESS_WARNING_SVG from 'icons/_icon_status_warning_borderless.svg'; + +import CANCELED_SVG from 'icons/_icon_status_canceled.svg'; +import CREATED_SVG from 'icons/_icon_status_created.svg'; +import FAILED_SVG from 'icons/_icon_status_failed.svg'; +import MANUAL_SVG from 'icons/_icon_status_manual.svg'; +import PENDING_SVG from 'icons/_icon_status_pending.svg'; +import RUNNING_SVG from 'icons/_icon_status_running.svg'; +import SKIPPED_SVG from 'icons/_icon_status_skipped.svg'; +import SUCCESS_SVG from 'icons/_icon_status_success.svg'; +import WARNING_SVG from 'icons/_icon_status_warning.svg'; + +export const borderlessStatusIconEntityMap = { + icon_status_canceled: BORDERLESS_CANCELED_SVG, + icon_status_created: BORDERLESS_CREATED_SVG, + icon_status_failed: BORDERLESS_FAILED_SVG, + icon_status_manual: BORDERLESS_MANUAL_SVG, + icon_status_pending: BORDERLESS_PENDING_SVG, + icon_status_running: BORDERLESS_RUNNING_SVG, + icon_status_skipped: BORDERLESS_SKIPPED_SVG, + icon_status_success: BORDERLESS_SUCCESS_SVG, + icon_status_warning: BORDERLESS_WARNING_SVG, +}; + +export const statusIconEntityMap = { + icon_status_canceled: CANCELED_SVG, + icon_status_created: CREATED_SVG, + icon_status_failed: FAILED_SVG, + icon_status_manual: MANUAL_SVG, + icon_status_pending: PENDING_SVG, + icon_status_running: RUNNING_SVG, + icon_status_skipped: SKIPPED_SVG, + icon_status_success: SUCCESS_SVG, + icon_status_warning: WARNING_SVG, +}; + +export const statusCssClasses = { + icon_status_canceled: 'canceled', + icon_status_created: 'created', + icon_status_failed: 'failed', + icon_status_manual: 'manual', + icon_status_pending: 'pending', + icon_status_running: 'running', + icon_status_skipped: 'skipped', + icon_status_success: 'success', + icon_status_warning: 'warning', +}; diff --git a/app/assets/javascripts/vue_shared/components/ci_icon.vue b/app/assets/javascripts/vue_shared/components/ci_icon.vue new file mode 100644 index 00000000000..4d44baaa3c4 --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/ci_icon.vue @@ -0,0 +1,29 @@ + + diff --git a/app/assets/javascripts/vue_shared/mixins/tooltip.js b/app/assets/javascripts/vue_shared/mixins/tooltip.js new file mode 100644 index 00000000000..9bb948bff66 --- /dev/null +++ b/app/assets/javascripts/vue_shared/mixins/tooltip.js @@ -0,0 +1,9 @@ +export default { + mounted() { + $(this.$refs.tooltip).tooltip(); + }, + + updated() { + $(this.$refs.tooltip).tooltip('fixTitle'); + }, +}; diff --git a/app/assets/stylesheets/pages/pipelines.scss b/app/assets/stylesheets/pages/pipelines.scss index 530a6f3c6a1..eaf3dd49567 100644 --- a/app/assets/stylesheets/pages/pipelines.scss +++ b/app/assets/stylesheets/pages/pipelines.scss @@ -486,7 +486,7 @@ color: $gl-text-color-secondary; // Action Icons in big pipeline-graph nodes - > .ci-action-icon-container .ci-action-icon-wrapper { + > div > .ci-action-icon-container .ci-action-icon-wrapper { height: 30px; width: 30px; background: $white-light; @@ -511,7 +511,7 @@ } } - > .ci-action-icon-container { + > div > .ci-action-icon-container { position: absolute; right: 5px; top: 5px; @@ -541,7 +541,7 @@ } } - > .build-content { + > div > .build-content { display: inline-block; padding: 8px 10px 9px; width: 100%; @@ -557,34 +557,6 @@ } - .arrow { - &::before, - &::after { - content: ''; - display: inline-block; - position: absolute; - width: 0; - height: 0; - border-color: transparent; - border-style: solid; - top: 18px; - } - - &::before { - left: -5px; - margin-top: -6px; - border-width: 7px 5px 7px 0; - border-right-color: $border-color; - } - - &::after { - left: -4px; - margin-top: -9px; - border-width: 10px 7px 10px 0; - border-right-color: $white-light; - } - } - // Connect first build in each stage with right horizontal line &:first-child { &::after { @@ -859,7 +831,8 @@ border-radius: 3px; // build name - .ci-build-text { + .ci-build-text, + .ci-status-text { font-weight: 200; overflow: hidden; white-space: nowrap; @@ -911,6 +884,38 @@ } } +/** + * Top arrow in the dropdown in the big pipeline graph + */ +.big-pipeline-graph-dropdown-menu { + + &::before, + &::after { + content: ''; + display: inline-block; + position: absolute; + width: 0; + height: 0; + border-color: transparent; + border-style: solid; + top: 18px; + } + + &::before { + left: -5px; + margin-top: -6px; + border-width: 7px 5px 7px 0; + border-right-color: $border-color; + } + + &::after { + left: -4px; + margin-top: -9px; + border-width: 10px 7px 10px 0; + border-right-color: $white-light; + } +} + /** * Top arrow in the dropdown in the mini pipeline graph */ diff --git a/app/views/ci/status/_graph_badge.html.haml b/app/views/ci/status/_graph_badge.html.haml deleted file mode 100644 index 128b418090f..00000000000 --- a/app/views/ci/status/_graph_badge.html.haml +++ /dev/null @@ -1,20 +0,0 @@ --# Renders the graph node with both the status icon, status name and action icon - -- subject = local_assigns.fetch(:subject) -- status = subject.detailed_status(current_user) -- klass = "ci-status-icon ci-status-icon-#{status.group} js-ci-status-icon-#{status.group}" -- tooltip = "#{subject.name} - #{status.label}" - -- if status.has_details? - = link_to status.details_path, class: 'build-content has-tooltip', data: { toggle: 'tooltip', title: tooltip, container: 'body' } do - %span{ class: klass }= custom_icon(status.icon) - .ci-status-text= subject.name -- else - .build-content.has-tooltip{ data: { toggle: 'tooltip', title: tooltip } } - %span{ class: klass }= custom_icon(status.icon) - .ci-status-text= subject.name - -- if status.has_action? - = link_to status.action_path, class: 'ci-action-icon-container has-tooltip', method: status.action_method, data: { toggle: 'tooltip', title: status.action_title, container: 'body' } do - %i.ci-action-icon-wrapper{ class: "js-#{status.action_icon.dasherize}" } - = custom_icon(status.action_icon) diff --git a/app/views/projects/pipelines/_graph.html.haml b/app/views/projects/pipelines/_graph.html.haml deleted file mode 100644 index 0202833c0bf..00000000000 --- a/app/views/projects/pipelines/_graph.html.haml +++ /dev/null @@ -1,4 +0,0 @@ -- pipeline = local_assigns.fetch(:pipeline) -.pipeline-visualization.pipeline-graph - %ul.stage-column-list - = render partial: "projects/stage/graph", collection: pipeline.stages, as: :stage diff --git a/app/views/projects/pipelines/_with_tabs.html.haml b/app/views/projects/pipelines/_with_tabs.html.haml index 1aa48bf9813..1c7d1768aa5 100644 --- a/app/views/projects/pipelines/_with_tabs.html.haml +++ b/app/views/projects/pipelines/_with_tabs.html.haml @@ -17,8 +17,11 @@ .tab-content #js-tab-pipeline.tab-pane - .build-content.middle-block.js-pipeline-graph - = render "projects/pipelines/graph", pipeline: pipeline + #js-pipeline-graph-vue{ data: { endpoint: namespace_project_pipeline_path(@project.namespace, @project, @pipeline, format: :json) } } + + - content_for :page_specific_javascripts do + = page_specific_javascript_bundle_tag('common_vue') + = page_specific_javascript_bundle_tag('pipelines_graph') #js-tab-builds.tab-pane - if pipeline.yaml_errors.present? diff --git a/app/views/projects/stage/_graph.html.haml b/app/views/projects/stage/_graph.html.haml deleted file mode 100644 index 4ee30b023ac..00000000000 --- a/app/views/projects/stage/_graph.html.haml +++ /dev/null @@ -1,19 +0,0 @@ -- stage = local_assigns.fetch(:stage) -- statuses = stage.statuses.latest -- status_groups = statuses.sort_by(&:sortable_name).group_by(&:group_name) -%li.stage-column - .stage-name - %a{ name: stage.name } - = stage.name.titleize - .builds-container - %ul - - status_groups.each do |group_name, grouped_statuses| - - if grouped_statuses.one? - - status = grouped_statuses.first - %li.build{ 'id' => "ci-badge-#{group_name}" } - .curve - = render 'ci/status/graph_badge', subject: status - - else - %li.build{ 'id' => "ci-badge-#{group_name}" } - .curve - = render 'projects/stage/in_stage_group', name: group_name, subject: grouped_statuses diff --git a/app/views/projects/stage/_in_stage_group.html.haml b/app/views/projects/stage/_in_stage_group.html.haml deleted file mode 100644 index 671a3ef481c..00000000000 --- a/app/views/projects/stage/_in_stage_group.html.haml +++ /dev/null @@ -1,14 +0,0 @@ -- group_status = CommitStatus.where(id: subject).status -%button.dropdown-menu-toggle.build-content.has-tooltip{ type: 'button', data: { toggle: 'dropdown', title: "#{name} - #{group_status}", container: 'body' } } - %span{ class: "ci-status-icon ci-status-icon-#{group_status}" } - = ci_icon_for_status(group_status) - %span.ci-status-text - = name - %span.dropdown-counter-badge= subject.size - -%ul.dropdown-menu.big-pipeline-graph-dropdown-menu.js-grouped-pipeline-dropdown - .arrow - .scrollable-menu - - subject.each do |status| - %li - = render 'ci/status/dropdown_graph_badge', subject: status diff --git a/changelogs/unreleased/25226-realtime-pipelines-fe.yml b/changelogs/unreleased/25226-realtime-pipelines-fe.yml new file mode 100644 index 00000000000..1149c8f0eac --- /dev/null +++ b/changelogs/unreleased/25226-realtime-pipelines-fe.yml @@ -0,0 +1,4 @@ +--- +title: Re-rewrites pipeline graph in vue to support realtime data updates +merge_request: +author: diff --git a/config/webpack.config.js b/config/webpack.config.js index 119b1ea9d2e..a3dae6b2e13 100644 --- a/config/webpack.config.js +++ b/config/webpack.config.js @@ -49,6 +49,7 @@ var config = { pdf_viewer: './blob/pdf_viewer.js', pipelines: './pipelines/index.js', balsamiq_viewer: './blob/balsamiq_viewer.js', + pipelines_graph: './pipelines/graph_bundle.js', profile: './profile/profile_bundle.js', protected_branches: './protected_branches/protected_branches_bundle.js', protected_tags: './protected_tags', @@ -145,6 +146,7 @@ var config = { 'pdf_viewer', 'pipelines', 'balsamiq_viewer', + 'pipelines_graph', ], minChunks: function(module, count) { return module.resource && (/vue_shared/).test(module.resource); diff --git a/spec/javascripts/bootstrap_linked_tabs_spec.js b/spec/javascripts/bootstrap_linked_tabs_spec.js index fa9f95e16cd..a27dc48b3fd 100644 --- a/spec/javascripts/bootstrap_linked_tabs_spec.js +++ b/spec/javascripts/bootstrap_linked_tabs_spec.js @@ -1,4 +1,4 @@ -require('~/lib/utils/bootstrap_linked_tabs'); +import LinkedTabs from '~/lib/utils/bootstrap_linked_tabs'; (() => { // TODO: remove this hack! @@ -25,7 +25,7 @@ require('~/lib/utils/bootstrap_linked_tabs'); }); it('should activate the tab correspondent to the given action', () => { - const linkedTabs = new window.gl.LinkedTabs({ // eslint-disable-line + const linkedTabs = new LinkedTabs({ // eslint-disable-line action: 'tab1', defaultAction: 'tab1', parentEl: '.linked-tabs', @@ -35,7 +35,7 @@ require('~/lib/utils/bootstrap_linked_tabs'); }); it('should active the default tab action when the action is show', () => { - const linkedTabs = new window.gl.LinkedTabs({ // eslint-disable-line + const linkedTabs = new LinkedTabs({ // eslint-disable-line action: 'show', defaultAction: 'tab1', parentEl: '.linked-tabs', @@ -49,7 +49,7 @@ require('~/lib/utils/bootstrap_linked_tabs'); it('should change the url according to the clicked tab', () => { const historySpy = !phantomjs && spyOn(history, 'replaceState').and.callFake(() => {}); - const linkedTabs = new window.gl.LinkedTabs({ // eslint-disable-line + const linkedTabs = new LinkedTabs({ action: 'show', defaultAction: 'tab1', parentEl: '.linked-tabs', diff --git a/spec/javascripts/ci_status_icon_spec.js b/spec/javascripts/ci_status_icon_spec.js deleted file mode 100644 index c83416c15ef..00000000000 --- a/spec/javascripts/ci_status_icon_spec.js +++ /dev/null @@ -1,44 +0,0 @@ -import * as icons from '~/ci_status_icons'; - -describe('CI status icons', () => { - const statuses = [ - 'canceled', - 'created', - 'failed', - 'manual', - 'pending', - 'running', - 'skipped', - 'success', - 'warning', - ]; - - statuses.forEach((status) => { - it(`should export a ${status} svg`, () => { - const key = `${status.toUpperCase()}_SVG`; - - expect(Object.hasOwnProperty.call(icons, key)).toBe(true); - expect(icons[key]).toMatch(/^ { - const entityIconNames = [ - 'icon_status_canceled', - 'icon_status_created', - 'icon_status_failed', - 'icon_status_manual', - 'icon_status_pending', - 'icon_status_running', - 'icon_status_skipped', - 'icon_status_success', - 'icon_status_warning', - ]; - - entityIconNames.forEach((iconName) => { - it(`should have a '${iconName}' key`, () => { - expect(Object.hasOwnProperty.call(icons.default, iconName)).toBe(true); - }); - }); - }); -}); diff --git a/spec/javascripts/fixtures/graph.html.haml b/spec/javascripts/fixtures/graph.html.haml new file mode 100644 index 00000000000..4fedb0f1ded --- /dev/null +++ b/spec/javascripts/fixtures/graph.html.haml @@ -0,0 +1 @@ +#js-pipeline-graph-vue{ data: { endpoint: "foo" } } diff --git a/spec/javascripts/pipelines/graph/action_component_spec.js b/spec/javascripts/pipelines/graph/action_component_spec.js new file mode 100644 index 00000000000..f033956c071 --- /dev/null +++ b/spec/javascripts/pipelines/graph/action_component_spec.js @@ -0,0 +1,40 @@ +import Vue from 'vue'; +import actionComponent from '~/pipelines/components/graph/action_component.vue'; + +describe('pipeline graph action component', () => { + let component; + + beforeEach(() => { + const ActionComponent = Vue.extend(actionComponent); + component = new ActionComponent({ + propsData: { + tooltipText: 'bar', + link: 'foo', + actionMethod: 'post', + actionIcon: 'icon_action_cancel', + }, + }).$mount(); + }); + + it('should render a link', () => { + expect(component.$el.getAttribute('href')).toEqual('foo'); + }); + + it('should render the provided title as a bootstrap tooltip', () => { + expect(component.$el.getAttribute('data-original-title')).toEqual('bar'); + }); + + it('should update bootstrap tooltip when title changes', (done) => { + component.tooltipText = 'changed'; + + Vue.nextTick(() => { + expect(component.$el.getAttribute('data-original-title')).toBe('changed'); + done(); + }); + }); + + it('should render an svg', () => { + expect(component.$el.querySelector('.ci-action-icon-wrapper')).toBeDefined(); + expect(component.$el.querySelector('svg')).toBeDefined(); + }); +}); diff --git a/spec/javascripts/pipelines/graph/dropdown_action_component_spec.js b/spec/javascripts/pipelines/graph/dropdown_action_component_spec.js new file mode 100644 index 00000000000..14ff1b0d25c --- /dev/null +++ b/spec/javascripts/pipelines/graph/dropdown_action_component_spec.js @@ -0,0 +1,30 @@ +import Vue from 'vue'; +import dropdownActionComponent from '~/pipelines/components/graph/dropdown_action_component.vue'; + +describe('action component', () => { + let component; + + beforeEach(() => { + const DropdownActionComponent = Vue.extend(dropdownActionComponent); + component = new DropdownActionComponent({ + propsData: { + tooltipText: 'bar', + link: 'foo', + actionMethod: 'post', + actionIcon: 'icon_action_cancel', + }, + }).$mount(); + }); + + it('should render a link', () => { + expect(component.$el.getAttribute('href')).toEqual('foo'); + }); + + it('should render the provided title as a bootstrap tooltip', () => { + expect(component.$el.getAttribute('data-original-title')).toEqual('bar'); + }); + + it('should render an svg', () => { + expect(component.$el.querySelector('svg')).toBeDefined(); + }); +}); diff --git a/spec/javascripts/pipelines/graph/graph_component_spec.js b/spec/javascripts/pipelines/graph/graph_component_spec.js new file mode 100644 index 00000000000..a756617e65e --- /dev/null +++ b/spec/javascripts/pipelines/graph/graph_component_spec.js @@ -0,0 +1,83 @@ +import Vue from 'vue'; +import graphComponent from '~/pipelines/components/graph/graph_component.vue'; + +describe('graph component', () => { + preloadFixtures('static/graph.html.raw'); + + let GraphComponent; + + beforeEach(() => { + loadFixtures('static/graph.html.raw'); + GraphComponent = Vue.extend(graphComponent); + }); + + describe('while is loading', () => { + it('should render a loading icon', () => { + const component = new GraphComponent().$mount('#js-pipeline-graph-vue'); + expect(component.$el.querySelector('.loading-icon')).toBeDefined(); + }); + }); + + describe('with a successfull response', () => { + const interceptor = (request, next) => { + next(request.respondWith(JSON.stringify({ + details: { + stages: [{ + name: 'test', + title: 'test: passed', + status: { + icon: 'icon_status_success', + text: 'passed', + label: 'passed', + details_path: '/root/ci-mock/pipelines/123#test', + }, + path: '/root/ci-mock/pipelines/123#test', + groups: [{ + name: 'test', + size: 1, + jobs: [{ + id: 4153, + name: 'test', + status: { + icon: 'icon_status_success', + text: 'passed', + label: 'passed', + details_path: '/root/ci-mock/builds/4153', + action: { + icon: 'icon_action_retry', + title: 'Retry', + path: '/root/ci-mock/builds/4153/retry', + method: 'post', + }, + }, + }], + }], + }], + }, + }), { + status: 200, + })); + }; + + beforeEach(() => { + Vue.http.interceptors.push(interceptor); + }); + + afterEach(() => { + Vue.http.interceptors = _.without(Vue.http.interceptors, interceptor); + }); + + it('should render the graph', (done) => { + const component = new GraphComponent().$mount('#js-pipeline-graph-vue'); + + setTimeout(() => { + expect(component.$el.classList.contains('js-pipeline-graph')).toEqual(true); + + expect(component.$el.querySelector('loading-icon')).toBe(null); + + expect(component.$el.querySelector('.stage-column-list')).toBeDefined(); + done(); + }, 0); + }); + }); +}); diff --git a/spec/javascripts/pipelines/graph/job_component_spec.js b/spec/javascripts/pipelines/graph/job_component_spec.js new file mode 100644 index 00000000000..63986b6c0db --- /dev/null +++ b/spec/javascripts/pipelines/graph/job_component_spec.js @@ -0,0 +1,117 @@ +import Vue from 'vue'; +import jobComponent from '~/pipelines/components/graph/job_component.vue'; + +describe('pipeline graph job component', () => { + let JobComponent; + + const mockJob = { + id: 4256, + name: 'test', + status: { + icon: 'icon_status_success', + text: 'passed', + label: 'passed', + group: 'success', + details_path: '/root/ci-mock/builds/4256', + action: { + icon: 'icon_action_retry', + title: 'Retry', + path: '/root/ci-mock/builds/4256/retry', + method: 'post', + }, + }, + }; + + beforeEach(() => { + JobComponent = Vue.extend(jobComponent); + }); + + describe('name with link', () => { + it('should render the job name and status with a link', () => { + const component = new JobComponent({ + propsData: { + job: mockJob, + }, + }).$mount(); + + const link = component.$el.querySelector('a'); + + expect(link.getAttribute('href')).toEqual(mockJob.status.details_path); + + expect( + link.getAttribute('data-original-title'), + ).toEqual(`${mockJob.name} - ${mockJob.status.label}`); + + expect(component.$el.querySelector('.js-status-icon-success')).toBeDefined(); + + expect( + component.$el.querySelector('.ci-status-text').textContent.trim(), + ).toEqual(mockJob.name); + }); + }); + + describe('name without link', () => { + it('it should render status and name', () => { + const component = new JobComponent({ + propsData: { + job: { + id: 4256, + name: 'test', + status: { + icon: 'icon_status_success', + text: 'passed', + label: 'passed', + group: 'success', + details_path: '/root/ci-mock/builds/4256', + }, + }, + }, + }).$mount(); + + expect(component.$el.querySelector('.js-status-icon-success')).toBeDefined(); + + expect( + component.$el.querySelector('.ci-status-text').textContent.trim(), + ).toEqual(mockJob.name); + }); + }); + + describe('action icon', () => { + it('it should render the action icon', () => { + const component = new JobComponent({ + propsData: { + job: mockJob, + }, + }).$mount(); + + expect(component.$el.querySelector('a.ci-action-icon-container')).toBeDefined(); + expect(component.$el.querySelector('i.ci-action-icon-wrapper')).toBeDefined(); + }); + }); + + describe('dropdown', () => { + it('should render the dropdown action icon', () => { + const component = new JobComponent({ + propsData: { + job: mockJob, + isDropdown: true, + }, + }).$mount(); + + expect(component.$el.querySelector('a.ci-action-icon-wrapper')).toBeDefined(); + }); + }); + + it('should render provided class name', () => { + const component = new JobComponent({ + propsData: { + job: mockJob, + cssClassJobName: 'css-class-job-name', + }, + }).$mount(); + + expect( + component.$el.querySelector('a').classList.contains('css-class-job-name'), + ).toBe(true); + }); +}); diff --git a/spec/javascripts/pipelines/graph/job_name_component_spec.js b/spec/javascripts/pipelines/graph/job_name_component_spec.js new file mode 100644 index 00000000000..8e2071ba0b3 --- /dev/null +++ b/spec/javascripts/pipelines/graph/job_name_component_spec.js @@ -0,0 +1,27 @@ +import Vue from 'vue'; +import jobNameComponent from '~/pipelines/components/graph/job_name_component.vue'; + +describe('job name component', () => { + let component; + + beforeEach(() => { + const JobNameComponent = Vue.extend(jobNameComponent); + component = new JobNameComponent({ + propsData: { + name: 'foo', + status: { + icon: 'icon_status_success', + }, + }, + }).$mount(); + }); + + it('should render the provided name', () => { + expect(component.$el.querySelector('.ci-status-text').textContent.trim()).toEqual('foo'); + }); + + it('should render an icon with the provided status', () => { + expect(component.$el.querySelector('.ci-status-icon-success')).toBeDefined(); + expect(component.$el.querySelector('.ci-status-icon-success svg')).toBeDefined(); + }); +}); diff --git a/spec/javascripts/pipelines/graph/stage_column_component_spec.js b/spec/javascripts/pipelines/graph/stage_column_component_spec.js new file mode 100644 index 00000000000..aa4d6eedaf4 --- /dev/null +++ b/spec/javascripts/pipelines/graph/stage_column_component_spec.js @@ -0,0 +1,42 @@ +import Vue from 'vue'; +import stageColumnComponent from '~/pipelines/components/graph/stage_column_component.vue'; + +describe('stage column component', () => { + let component; + const mockJob = { + id: 4256, + name: 'test', + status: { + icon: 'icon_status_success', + text: 'passed', + label: 'passed', + group: 'success', + details_path: '/root/ci-mock/builds/4256', + action: { + icon: 'icon_action_retry', + title: 'Retry', + path: '/root/ci-mock/builds/4256/retry', + method: 'post', + }, + }, + }; + + beforeEach(() => { + const StageColumnComponent = Vue.extend(stageColumnComponent); + + component = new StageColumnComponent({ + propsData: { + title: 'foo', + jobs: [mockJob, mockJob, mockJob], + }, + }).$mount(); + }); + + it('should render provided title', () => { + expect(component.$el.querySelector('.stage-name').textContent.trim()).toEqual('foo'); + }); + + it('should render the provided jobs', () => { + expect(component.$el.querySelectorAll('.builds-container > ul > li').length).toEqual(3); + }); +}); diff --git a/spec/javascripts/pipelines_spec.js b/spec/javascripts/pipelines_spec.js index 72770a702d3..81ac589f4e6 100644 --- a/spec/javascripts/pipelines_spec.js +++ b/spec/javascripts/pipelines_spec.js @@ -1,30 +1,22 @@ -require('~/pipelines'); +import Pipelines from '~/pipelines'; // Fix for phantomJS if (!Element.prototype.matches && Element.prototype.webkitMatchesSelector) { Element.prototype.matches = Element.prototype.webkitMatchesSelector; } -(() => { - describe('Pipelines', () => { - preloadFixtures('static/pipeline_graph.html.raw'); +describe('Pipelines', () => { + preloadFixtures('static/pipeline_graph.html.raw'); - beforeEach(() => { - loadFixtures('static/pipeline_graph.html.raw'); - }); - - it('should be defined', () => { - expect(window.gl.Pipelines).toBeDefined(); - }); - - it('should create a `Pipelines` instance without options', () => { - expect(() => { new window.gl.Pipelines(); }).not.toThrow(); //eslint-disable-line - }); + beforeEach(() => { + loadFixtures('static/pipeline_graph.html.raw'); + }); - it('should create a `Pipelines` instance with options', () => { - const pipelines = new window.gl.Pipelines({ foo: 'bar' }); + it('should be defined', () => { + expect(Pipelines).toBeDefined(); + }); - expect(pipelines.pipelineGraph).toBeDefined(); - }); + it('should create a `Pipelines` instance without options', () => { + expect(() => { new Pipelines(); }).not.toThrow(); //eslint-disable-line }); -})(); +}); diff --git a/spec/javascripts/vue_shared/ci_action_icons_spec.js b/spec/javascripts/vue_shared/ci_action_icons_spec.js new file mode 100644 index 00000000000..2e89a07e76e --- /dev/null +++ b/spec/javascripts/vue_shared/ci_action_icons_spec.js @@ -0,0 +1,22 @@ +import getActionIcon from '~/vue_shared/ci_action_icons'; +import cancelSVG from 'icons/_icon_action_cancel.svg'; +import retrySVG from 'icons/_icon_action_retry.svg'; +import playSVG from 'icons/_icon_action_play.svg'; + +describe('getActionIcon', () => { + it('should return an empty string', () => { + expect(getActionIcon()).toEqual(''); + }); + + it('should return cancel svg', () => { + expect(getActionIcon('icon_action_cancel')).toEqual(cancelSVG); + }); + + it('should return retry svg', () => { + expect(getActionIcon('icon_action_retry')).toEqual(retrySVG); + }); + + it('should return play svg', () => { + expect(getActionIcon('icon_action_play')).toEqual(playSVG); + }); +}); diff --git a/spec/javascripts/vue_shared/ci_status_icon_spec.js b/spec/javascripts/vue_shared/ci_status_icon_spec.js new file mode 100644 index 00000000000..b6621d6054d --- /dev/null +++ b/spec/javascripts/vue_shared/ci_status_icon_spec.js @@ -0,0 +1,27 @@ +import { borderlessStatusIconEntityMap, statusIconEntityMap } from '~/vue_shared/ci_status_icons'; + +describe('CI status icons', () => { + const statuses = [ + 'icon_status_canceled', + 'icon_status_created', + 'icon_status_failed', + 'icon_status_manual', + 'icon_status_pending', + 'icon_status_running', + 'icon_status_skipped', + 'icon_status_success', + 'icon_status_warning', + ]; + + it('should have a dictionary for borderless icons', () => { + statuses.forEach((status) => { + expect(borderlessStatusIconEntityMap[status]).toBeDefined(); + }); + }); + + it('should have a dictionary for icons', () => { + statuses.forEach((status) => { + expect(statusIconEntityMap[status]).toBeDefined(); + }); + }); +}); diff --git a/spec/javascripts/vue_shared/components/ci_icon_spec.js b/spec/javascripts/vue_shared/components/ci_icon_spec.js new file mode 100644 index 00000000000..98dc6caa622 --- /dev/null +++ b/spec/javascripts/vue_shared/components/ci_icon_spec.js @@ -0,0 +1,130 @@ +import Vue from 'vue'; +import ciIcon from '~/vue_shared/components/ci_icon.vue'; + +describe('CI Icon component', () => { + let CiIcon; + beforeEach(() => { + CiIcon = Vue.extend(ciIcon); + }); + + it('should render a span element with an svg', () => { + const component = new CiIcon({ + propsData: { + status: { + icon: 'icon_status_success', + }, + }, + }).$mount(); + + expect(component.$el.tagName).toEqual('SPAN'); + expect(component.$el.querySelector('span > svg')).toBeDefined(); + }); + + it('should render a success status', () => { + const component = new CiIcon({ + propsData: { + status: { + icon: 'icon_status_success', + }, + }, + }).$mount(); + + expect(component.$el.classList.contains('ci-status-icon-success')).toEqual(true); + }); + + it('should render a failed status', () => { + const component = new CiIcon({ + propsData: { + status: { + icon: 'icon_status_failed', + }, + }, + }).$mount(); + + expect(component.$el.classList.contains('ci-status-icon-failed')).toEqual(true); + }); + + it('should render success with warnings status', () => { + const component = new CiIcon({ + propsData: { + status: { + icon: 'icon_status_warning', + }, + }, + }).$mount(); + + expect(component.$el.classList.contains('ci-status-icon-warning')).toEqual(true); + }); + + it('should render pending status', () => { + const component = new CiIcon({ + propsData: { + status: { + icon: 'icon_status_pending', + }, + }, + }).$mount(); + + expect(component.$el.classList.contains('ci-status-icon-pending')).toEqual(true); + }); + + it('should render running status', () => { + const component = new CiIcon({ + propsData: { + status: { + icon: 'icon_status_running', + }, + }, + }).$mount(); + + expect(component.$el.classList.contains('ci-status-icon-running')).toEqual(true); + }); + + it('should render created status', () => { + const component = new CiIcon({ + propsData: { + status: { + icon: 'icon_status_created', + }, + }, + }).$mount(); + + expect(component.$el.classList.contains('ci-status-icon-created')).toEqual(true); + }); + + it('should render skipped status', () => { + const component = new CiIcon({ + propsData: { + status: { + icon: 'icon_status_skipped', + }, + }, + }).$mount(); + + expect(component.$el.classList.contains('ci-status-icon-skipped')).toEqual(true); + }); + + it('should render canceled status', () => { + const component = new CiIcon({ + propsData: { + status: { + icon: 'icon_status_canceled', + }, + }, + }).$mount(); + + expect(component.$el.classList.contains('ci-status-icon-canceled')).toEqual(true); + }); + + it('should render status for manual action', () => { + const component = new CiIcon({ + propsData: { + status: { + icon: 'icon_status_manual', + }, + }, + }).$mount(); + + expect(component.$el.classList.contains('ci-status-icon-manual')).toEqual(true); + }); +}); -- cgit v1.2.3 From 13b31d25ec9c5f8d17f95ca8ad756a88b5e3dbd9 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Sat, 6 May 2017 21:35:07 +0100 Subject: Fix broken css class --- app/assets/javascripts/pipelines/components/stage.vue | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/pipelines/components/stage.vue b/app/assets/javascripts/pipelines/components/stage.vue index dc42223269d..310f44b06df 100644 --- a/app/assets/javascripts/pipelines/components/stage.vue +++ b/app/assets/javascripts/pipelines/components/stage.vue @@ -14,7 +14,7 @@ */ /* global Flash */ -import { statusCssClasses, borderlessStatusIconEntityMap } from '../../vue_shared/ci_status_icons'; +import { borderlessStatusIconEntityMap } from '../../vue_shared/ci_status_icons'; export default { props: { @@ -109,7 +109,7 @@ export default { }, triggerButtonClass() { - return `ci-status-icon-${statusCssClasses[this.stage.status.icon]}`; + return `ci-status-icon-${this.stage.status.group}`; }, svgIcon() { -- cgit v1.2.3 From 8f64091a9550f9073889752bb975d96e074fd734 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Sat, 6 May 2017 21:55:40 +0100 Subject: Adds missing CSS class --- .../javascripts/pipelines/components/graph/action_component.vue | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/assets/javascripts/pipelines/components/graph/action_component.vue b/app/assets/javascripts/pipelines/components/graph/action_component.vue index 14e485791ea..1f9e3d39779 100644 --- a/app/assets/javascripts/pipelines/components/graph/action_component.vue +++ b/app/assets/javascripts/pipelines/components/graph/action_component.vue @@ -37,6 +37,10 @@ actionIconSvg() { return getActionIcon(this.actionIcon); }, + + cssClass() { + return `js-${gl.text.dasherize(this.actionIcon)}`; + }, }, }; @@ -52,6 +56,7 @@