Commit 611282b2 authored by Mayra Cabrera's avatar Mayra Cabrera

Addresses backend comments

- Moves manual builds query into model
- Removes unneeded methods from new Entity, and gitlab routing helper
- Removes extra file commited by accident
parent a56b53ed
Pipeline #57875967 (#1180089) failed with stages
in 39 minutes and 33 seconds
......@@ -150,8 +150,10 @@ class Projects::PipelinesController < Projects::ApplicationController
def play_all_manual
return not_found unless pipeline_stage
::Ci::PlayStageManualBuildsService.new(@project, current_user)
.execute(pipeline_stage)
options = { pipeline: pipeline, stage: pipeline_stage }
::Ci::PlayStageManualBuildsService.new(@project, current_user, options)
.execute
render json: StageSerializer
.new(project: @project, current_user: @current_user)
......
......@@ -193,10 +193,4 @@ module GitlabRoutingHelper
project = schedule.project
take_ownership_project_pipeline_schedule_path(project, schedule, *args)
end
# Stages
def play_all_project_stage_path(pipeline, stage_name)
project = pipeline.project
play_all_manual_namespace_project_pipeline_path(project.namespace, project, pipeline, stage: stage_name) # rubocop:disable Cop/ProjectPathHelper
end
end
......@@ -5,6 +5,7 @@ module Ci
# We should migrate this object to actual database record in the future
class LegacyStage
include StaticModel
include Gitlab::Utils::StrongMemoize
attr_reader :pipeline, :name
......@@ -51,14 +52,14 @@ module Ci
status.to_s == 'success'
end
def manual_playable?
%w[success running failed].include?(status.to_s)
end
def blocked_or_skipped?
%[manual scheduled skipped].include?(status.to_s)
end
def manual_playable?
manual_playable_statuses.include?(status.to_s) && with_manual_builds
end
def has_warnings?
if @warnings.is_a?(Integer)
@warnings > 0
......@@ -66,5 +67,17 @@ module Ci
statuses.latest.failed_but_allowed.any?
end
end
private
def manual_playable_statuses
%w[success running failed]
end
def with_manual_builds
strong_memoize(:manual_builds) do
builds.manual.exists?
end
end
end
end
......@@ -6,6 +6,7 @@ module Ci
include Importable
include HasStatus
include Gitlab::OptimisticLocking
include Gitlab::Utils::StrongMemoize
enum status: HasStatus::STATUSES_ENUM
......@@ -101,7 +102,7 @@ module Ci
end
def manual_playable?
%w[success running failed].include?(status)
manual_playable_statuses.include?(status) && with_manual_builds
end
def has_warnings?
......@@ -124,5 +125,17 @@ module Ci
.new(self, current_user)
.fabricate!
end
private
def manual_playable_statuses
%w[success running failed]
end
def with_manual_builds
strong_memoize(:manual_builds) do
builds.manual.exists?
end
end
end
end
# frozen_string_literal: true
module Stages
class DetailedStatusEntity < DetailedStatusEntity
expose :manual_builds_details,
if: -> (status, _) { status.has_manual_builds? },
with: Stages::PlayManualEntity
end
end
......@@ -4,17 +4,23 @@ module Ci
class PlayStageManualBuildsService < BaseService
include Gitlab::Utils::StrongMemoize
def execute(stage)
@stage = stage
def initialize(project, current_user, params)
super
manual_builds.each do |build|
build.play(current_user)
end
@pipeline = params[:pipeline]
@stage = params[:stage]
end
def execute
raise Gitlab::Access::AccessDeniedError unless can?(current_user, :update_pipeline, pipeline)
manual_builds.map(&:enqueue)
manual_builds.update(user_id: current_user.id)
end
private
attr_reader :stage
attr_reader :pipeline, :stage
def manual_builds
strong_memoize(:manual_builds) do
......
......@@ -5,12 +5,6 @@ module Gitlab
module Status
module Stage
class PlayManual < Status::Extended
include GitlabRoutingHelper
def label
'play all manual'
end
def action_icon
'play'
end
......@@ -24,7 +18,9 @@ module Gitlab
end
def action_path
play_all_project_stage_path(subject.pipeline, subject.name)
pipeline = subject.pipeline
play_all_manual_project_pipeline_path(pipeline.project, pipeline, subject.name)
end
def action_method
......@@ -32,8 +28,7 @@ module Gitlab
end
def self.matches?(stage, user)
stage.blocked_or_skipped? ||
(stage.manual_playable? && stage.builds.manual.exists?)
stage.blocked_or_skipped? || stage.manual_playable?
end
def has_manual_builds?
......
......@@ -404,31 +404,38 @@ describe Projects::PipelinesController do
create_manual_build(pipeline, 'test', 'rspec 3/3')
pipeline.reload
post :play_all_manual, params: {
namespace_id: project.namespace,
project_id: project,
id: pipeline.id,
stage: stage_name
}, format: :json
end
context 'when the stage does not exists' do
let(:stage_name) { 'deploy' }
it 'fails to play all manual' do
play_all_manual!
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'when the stage exists' do
it 'starts all manual jobs' do
expect(pipeline.builds.manual.count).to eq(3)
play_all_manual!
expect(response).to have_gitlab_http_status(:ok)
expect(pipeline.builds.manual.count).to eq(0)
end
end
def play_all_manual!
post :play_all_manual, params: {
namespace_id: project.namespace,
project_id: project,
id: pipeline.id,
stage: stage_name
}, format: :json
end
def create_manual_build(pipeline, stage, name)
create(:ci_build, :manual,
pipeline: pipeline,
......
......@@ -6,12 +6,6 @@ describe Gitlab::Ci::Status::Stage::PlayManual do
let(:stage) { double('stage') }
let(:play_manual) { described_class.new(stage) }
describe '#label' do
subject { play_manual.label }
it { is_expected.to eq('play all manual') }
end
describe '#action_icon' do
subject { play_manual.action_icon }
......@@ -81,7 +75,7 @@ describe Gitlab::Ci::Status::Stage::PlayManual do
it { is_expected.to be_truthy }
end
context 'and does not have manual buidls' do
context 'and does not have manual builds' do
it { is_expected.to be_falsy }
end
end
......
......@@ -272,4 +272,12 @@ describe Ci::LegacyStage do
def create_job(type, status: 'success', stage: stage_name, **opts)
create(type, pipeline: pipeline, stage: stage, status: status, **opts)
end
describe '#manual_playable?' do
def create_manual_builds(stage)
create_list(:ci_build, 2, :manual, stage: stage.name, pipeline: stage.pipeline)
end
it_behaves_like 'stage manual builds', :ci_stage
end
end
......@@ -152,7 +152,7 @@ describe Ci::Stage, :models do
%w[created] | :created
%w[success] | :passed
%w[pending] | :pending
%w[skipped] | 'play all manual'
%w[skipped] | :skipped
%w[canceled] | :canceled
%w[success failed] | :failed
%w[running pending] | :running
......@@ -190,21 +190,6 @@ describe Ci::Stage, :models do
expect(subject.label).to eq 'passed with warnings'
end
end
context 'when stage has manual builds' do
before do
create(:ci_build, project: stage.project,
pipeline: stage.pipeline,
stage_id: stage.id,
status: 'manual')
stage.update_status
end
it 'is passed with warnings' do
expect(subject.label).to eq 'play all manual'
end
end
end
describe '#groups' do
......@@ -302,16 +287,10 @@ describe Ci::Stage, :models do
end
describe '#manual_playable?' do
let(:success) { create(:ci_stage_entity, status: 'success') }
let(:failed) { create(:ci_stage_entity, status: 'failed') }
let(:running) { create(:ci_stage_entity, status: 'running') }
context 'when status is success, running, or failed' do
it 'returns true' do
[success, failed, running].each do |stage|
expect(stage.manual_playable?).to be_truthy
end
end
def create_manual_builds(stage)
create_list(:ci_build, 2, :manual, stage: stage.name, stage_id: stage.id)
end
it_behaves_like 'stage manual builds', :ci_stage_entity
end
end
......@@ -156,8 +156,7 @@ describe PipelineSerializer do
it 'verifies number of queries', :request_store do
recorded = ActiveRecord::QueryRecorder.new { subject }
expected_queries = Gitlab.ee? ? 51 : 45
expected_queries = Gitlab.ee? ? 50 : 43
expect(recorded.count).to be_within(2).of(expected_queries)
expect(recorded.cached_count).to eq(0)
end
......@@ -177,7 +176,7 @@ describe PipelineSerializer do
# pipeline. With the same ref this check is cached but if refs are
# different then there is an extra query per ref
# https://gitlab.com/gitlab-org/gitlab-ce/issues/46368
expected_queries = Gitlab.ee? ? 57 : 51
expected_queries = Gitlab.ee? ? 55 : 49
expect(recorded.count).to be_within(2).of(expected_queries)
expect(recorded.cached_count).to eq(0)
end
......
......@@ -48,7 +48,7 @@ describe StageEntity do
expect(subject[:title]).to eq 'test: passed'
end
it 'does not contain play_all_manual info' do
it 'does not contain play_manual_details info' do
expect(subject[:play_manual_details]).not_to be_present
end
......
......@@ -5,45 +5,74 @@ require 'spec_helper'
describe Ci::PlayStageManualBuildsService, '#execute' do
let(:pipeline) { create(:ci_pipeline) }
let(:project) { pipeline.project }
let(:stage) { create(:ci_stage_entity, pipeline: pipeline, project: project, name: 'test') }
let(:current_user) { create(:user) }
let(:service) do
described_class.new(project, current_user)
let(:stage) do
create(:ci_stage_entity,
pipeline: pipeline,
project: project,
name: 'test')
end
before do
project.add_maintainer(current_user)
let(:options) do
{
pipeline: stage.pipeline,
stage: stage
}
end
context 'when pipeline has manual builds' do
before do
create_list(:ci_build, 3, :manual,
pipeline: pipeline,
stage: 'test',
stage_id: stage.id)
let(:service) do
described_class.new(project, current_user, options)
end
service.execute(stage)
pipeline.reload
end
context 'when user does not have access' do
it 'throws an exception' do
project.add_reporter(current_user)
it 'starts manual builds from pipeline' do
expect(pipeline.builds.manual.count).to eq(0)
expect do
service.execute
end.to raise_error(Gitlab::Access::AccessDeniedError)
end
end
context 'when pipeline has no manual builds' do
context 'when user has access' do
before do
create_list(:ci_build, 2, :failed,
pipeline: pipeline,
stage: 'build')
project.add_maintainer(current_user)
end
service.execute(pipeline)
pipeline.reload
context 'when pipeline has manual builds' do
before do
create_builds_for_stage('manual')
service.execute
pipeline.reload
end
it 'starts manual builds from pipeline' do
expect(pipeline.builds.manual.count).to eq(0)
end
it 'updates manual builds' do
pipeline.builds.each do |build|
expect(build.user).to eq(current_user)
end
end
end
it 'does not update the builds' do
expect(pipeline.builds.failed.count).to eq(2)
context 'when pipeline has no manual builds' do
before do
create_builds_for_stage('failed')
service.execute
pipeline.reload
end
it 'does not update the builds' do
expect(pipeline.builds.failed.count).to eq(3)
end
end
end
def create_builds_for_stage(status)
create_list(:ci_build, 3, status: status, pipeline: pipeline, stage: stage.name, stage_id: stage.id)
end
end
# frozen_string_literal: true
shared_examples 'stage manual builds' do |stage_factory|
subject { stage.manual_playable? }
context 'when it has a manual playable status' do
let(:stage) { create(stage_factory, status: 'success') }
context 'and manual builds' do
before do
create_manual_builds(stage)
end
it { is_expected.to be_truthy }
end
context 'and no manual builds' do
it { is_expected.to be_falsy }
end
end
context 'when it has any other status' do
let(:stage) { create(stage_factory, status: 'canceled') }
it { is_expected.to be_falsy }
end
end
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment