Commit 98c4f3ac authored by Mayra Cabrera's avatar Mayra Cabrera 🏙

Addresses backend comments

- Creates a stages_controller with a play_manual action
- Renames blocked_or_skipped to playable
- Make new service re-use the Ci::Build#play method.
- Rename playable? to manual_playable? so the action is more descriptive
- Log an error in case the user does not have access to one of the
instances
parent 6469d4cc
Pipeline #59178379 passed with stages
in 62 minutes and 38 seconds
...@@ -24,7 +24,7 @@ export default { ...@@ -24,7 +24,7 @@ export default {
:groups="stage.groups" :groups="stage.groups"
:stage-connector-class="stageConnectorClass(index, stage)" :stage-connector-class="stageConnectorClass(index, stage)"
:is-first-column="isFirstColumn(index)" :is-first-column="isFirstColumn(index)"
:action="stage.play_manual_details" :action="stage.status.action"
@refreshPipelineGraph="refreshPipelineGraph" @refreshPipelineGraph="refreshPipelineGraph"
/> />
</ul> </ul>
......
...@@ -6,7 +6,7 @@ class Projects::PipelinesController < Projects::ApplicationController ...@@ -6,7 +6,7 @@ class Projects::PipelinesController < Projects::ApplicationController
before_action :authorize_read_pipeline! before_action :authorize_read_pipeline!
before_action :authorize_read_build!, only: [:index] before_action :authorize_read_build!, only: [:index]
before_action :authorize_create_pipeline!, only: [:new, :create] before_action :authorize_create_pipeline!, only: [:new, :create]
before_action :authorize_update_pipeline!, only: [:retry, :cancel, :play_all_manual] before_action :authorize_update_pipeline!, only: [:retry, :cancel]
around_action :allow_gitaly_ref_name_caching, only: [:index, :show] around_action :allow_gitaly_ref_name_caching, only: [:index, :show]
...@@ -147,19 +147,6 @@ class Projects::PipelinesController < Projects::ApplicationController ...@@ -147,19 +147,6 @@ class Projects::PipelinesController < Projects::ApplicationController
@counts[:failed] = @project.all_pipelines.failed.count(:all) @counts[:failed] = @project.all_pipelines.failed.count(:all)
end end
def play_all_manual
return not_found unless 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)
.represent(pipeline_stage, play_all: false)
end
private private
def serialize_pipelines def serialize_pipelines
...@@ -209,8 +196,4 @@ class Projects::PipelinesController < Projects::ApplicationController ...@@ -209,8 +196,4 @@ class Projects::PipelinesController < Projects::ApplicationController
view_context.limited_counter_with_delimiter(finder.execute) view_context.limited_counter_with_delimiter(finder.execute)
end end
def pipeline_stage
@pipeline_stage ||= pipeline.find_stage_by_name(params[:stage])
end
end end
# frozen_string_literal: true
class Projects::StagesController < Projects::PipelinesController
before_action :authorize_update_pipeline!
def play_manual
::Ci::PlayManualStageService
.new(@project, current_user, pipeline: pipeline)
.execute(stage)
respond_to do |format|
format.json do
render json: StageSerializer
.new(project: @project, current_user: @current_user)
.represent(stage)
end
end
end
private
def stage
@pipeline_stage ||= pipeline.find_stage_by_name!(params[:stage_name])
end
end
...@@ -59,7 +59,7 @@ module Ci ...@@ -59,7 +59,7 @@ module Ci
end end
end end
def blocked_or_skipped? def manual_playable?
%[manual scheduled skipped].include?(status.to_s) %[manual scheduled skipped].include?(status.to_s)
end end
end end
......
...@@ -771,8 +771,8 @@ module Ci ...@@ -771,8 +771,8 @@ module Ci
Gitlab::Utils.slugify(source_ref.to_s) Gitlab::Utils.slugify(source_ref.to_s)
end end
def find_stage_by_name(name) def find_stage_by_name!(name)
stages.find_by(name: name) stages.find_by!(name: name)
end end
private private
......
...@@ -120,5 +120,9 @@ module Ci ...@@ -120,5 +120,9 @@ module Ci
.new(self, current_user) .new(self, current_user)
.fabricate! .fabricate!
end end
def manual_playable?
blocked? || skipped?
end
end end
end end
...@@ -122,10 +122,6 @@ module HasStatus ...@@ -122,10 +122,6 @@ module HasStatus
BLOCKED_STATUS.include?(status) BLOCKED_STATUS.include?(status)
end end
def blocked_or_skipped?
blocked? || status == 'skipped'
end
private private
def calculate_duration def calculate_duration
......
...@@ -42,20 +42,14 @@ class StageEntity < Grape::Entity ...@@ -42,20 +42,14 @@ class StageEntity < Grape::Entity
format: :json) format: :json)
end end
expose :play_manual_details,
if: -> (status, opts) { show_play_manual_details?(opts) },
with: Stages::PlayManualEntity
private private
alias_method :stage, :object alias_method :stage, :object
def detailed_status def detailed_status
@detailed_status ||= stage.detailed_status(request.current_user) stage.detailed_status(request.current_user)
end end
alias_method :play_manual_details, :detailed_status
def grouped_statuses def grouped_statuses
@grouped_statuses ||= stage.statuses.latest_ordered.group_by(&:status) @grouped_statuses ||= stage.statuses.latest_ordered.group_by(&:status)
end end
...@@ -75,8 +69,4 @@ class StageEntity < Grape::Entity ...@@ -75,8 +69,4 @@ class StageEntity < Grape::Entity
grouped_retried_statuses.fetch(ordered_status, []) grouped_retried_statuses.fetch(ordered_status, [])
end.flatten end.flatten
end end
def show_play_manual_details?(opts)
opts.fetch(:play_all, true) && detailed_status.has_manual_builds?
end
end end
# frozen_string_literal: true
class Stages::PlayManualEntity < Grape::Entity
expose :action_icon, as: :icon
expose :action_button_title, as: :button_title
expose :action_title, as: :title
expose :action_path, as: :path
expose :action_method, as: :method
end
# frozen_string_literal: true # frozen_string_literal: true
module Ci module Ci
class PlayStageManualBuildsService < BaseService class PlayManualStageService < BaseService
def initialize(project, current_user, params) def initialize(project, current_user, params)
super super
@pipeline = params[:pipeline] @pipeline = params[:pipeline]
@stage = params[:stage]
end end
def execute def execute(stage)
raise Gitlab::Access::AccessDeniedError unless can?(current_user, :update_pipeline, pipeline) stage.builds.manual.each do |build|
next unless build.playable?
manual_builds.map(&:enqueue) build.play(current_user)
manual_builds.update_all(user_id: current_user.id, updated_at: Time.now) rescue Gitlab::Access::AccessDeniedError
logger.error(message: 'Unable to play manual action', build_id: build.id)
end
end end
private private
attr_reader :pipeline, :stage attr_reader :pipeline, :current_user
def manual_builds def logger
@manual_builds ||= stage.builds.manual Gitlab::AppLogger
end end
end end
end end
...@@ -200,7 +200,12 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do ...@@ -200,7 +200,12 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
get :builds get :builds
get :failures get :failures
get :status get :status
post :play_all_manual, path: 'stages/:stage/play_all_manual' end
member do
resources :stages, only: [], param: :name do
post :play_manual
end
end end
end end
......
...@@ -18,10 +18,6 @@ module Gitlab ...@@ -18,10 +18,6 @@ module Gitlab
def has_action? def has_action?
false false
end end
def has_manual_builds?
false
end
end end
end end
end end
......
...@@ -11,10 +11,6 @@ module Gitlab ...@@ -11,10 +11,6 @@ module Gitlab
'play' 'play'
end end
def action_button_title
_('Play all manual')
end
def action_title def action_title
'Play all manual' 'Play all manual'
end end
...@@ -22,18 +18,22 @@ module Gitlab ...@@ -22,18 +18,22 @@ module Gitlab
def action_path def action_path
pipeline = subject.pipeline pipeline = subject.pipeline
play_all_manual_project_pipeline_path(pipeline.project, pipeline, subject.name) project_stage_play_manual_path(pipeline.project, pipeline, subject.name)
end end
def action_method def action_method
:post :post
end end
def action_button_title
_('Play all manual')
end
def self.matches?(stage, user) def self.matches?(stage, user)
stage.blocked_or_skipped? stage.manual_playable?
end end
def has_manual_builds? def has_action?
true true
end end
end end
......
...@@ -393,50 +393,4 @@ describe Projects::PipelinesController do ...@@ -393,50 +393,4 @@ describe Projects::PipelinesController do
end end
end end
end end
describe 'POST #play_all_manual.json' do
let(:pipeline) { create(:ci_pipeline, project: project) }
let(:stage_name) { 'test' }
before do
create_manual_build(pipeline, 'test', 'rspec 1/3')
create_manual_build(pipeline, 'test', 'rspec 2/3')
create_manual_build(pipeline, 'test', 'rspec 3/3')
pipeline.reload
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, stage: stage, name: name)
end
end
end end
# frozen_string_literal: true
require 'spec_helper'
describe Projects::StagesController do
let(:user) { create(:user) }
let(:project) { create(:project, :repository) }
before do
sign_in(user)
end
describe 'POST #play_manual.json' do
let(:pipeline) { create(:ci_pipeline, project: project) }
let(:stage_name) { 'test' }
before do
create_manual_build(pipeline, 'test', 'rspec 1/2')
create_manual_build(pipeline, 'test', 'rspec 2/2')
pipeline.reload
end
context 'when user does not have access' do
it 'returns not authorized' do
play_manual_stage!
expect(response).to have_gitlab_http_status(404)
end
end
context 'when user has access' do
before do
project.add_maintainer(user)
end
context 'when the stage does not exists' do
let(:stage_name) { 'deploy' }
it 'fails to play all manual' do
play_manual_stage!
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(2)
play_manual_stage!
expect(response).to have_gitlab_http_status(:ok)
expect(pipeline.builds.manual.count).to eq(0)
end
end
end
def play_manual_stage!
post :play_manual, params: {
namespace_id: project.namespace,
project_id: project,
id: pipeline.id,
stage_name: stage_name
}, format: :json
end
def create_manual_build(pipeline, stage, name)
create(:ci_build, :manual, pipeline: pipeline, stage: stage, name: name)
end
end
end
...@@ -31,7 +31,7 @@ describe Gitlab::Ci::Status::Stage::PlayManual do ...@@ -31,7 +31,7 @@ describe Gitlab::Ci::Status::Stage::PlayManual do
subject { play_manual.action_path } subject { play_manual.action_path }
it { is_expected.to eq("/#{pipeline.project.full_path}/pipelines/#{pipeline.id}/stages/#{stage.name}/play_all_manual") } it { is_expected.to eq("/#{pipeline.project.full_path}/pipelines/#{pipeline.id}/stages/#{stage.name}/play_manual") }
end end
describe '#action_method' do describe '#action_method' do
...@@ -71,10 +71,4 @@ describe Gitlab::Ci::Status::Stage::PlayManual do ...@@ -71,10 +71,4 @@ describe Gitlab::Ci::Status::Stage::PlayManual do
end end
end end
end end
describe '#has_manual_builds?' do
subject { play_manual.has_manual_builds? }
it { is_expected.to be_truthy }
end
end end
...@@ -272,4 +272,6 @@ describe Ci::LegacyStage do ...@@ -272,4 +272,6 @@ describe Ci::LegacyStage do
def create_job(type, status: 'success', stage: stage_name, **opts) def create_job(type, status: 'success', stage: stage_name, **opts)
create(type, pipeline: pipeline, stage: stage, status: status, **opts) create(type, pipeline: pipeline, stage: stage, status: status, **opts)
end end
it_behaves_like 'manual playable stage', :ci_stage
end end
...@@ -2958,7 +2958,7 @@ describe Ci::Pipeline, :mailer do ...@@ -2958,7 +2958,7 @@ describe Ci::Pipeline, :mailer do
create_list(:ci_build, 2, pipeline: pipeline, stage: stage.name) create_list(:ci_build, 2, pipeline: pipeline, stage: stage.name)
end end
subject { pipeline.find_stage_by_name(stage_name) } subject { pipeline.find_stage_by_name!(stage_name) }
context 'when stage exists' do context 'when stage exists' do
it { is_expected.to eq(stage) } it { is_expected.to eq(stage) }
...@@ -2967,7 +2967,11 @@ describe Ci::Pipeline, :mailer do ...@@ -2967,7 +2967,11 @@ describe Ci::Pipeline, :mailer do
context 'when stage does not exist' do context 'when stage does not exist' do
let(:stage_name) { 'build' } let(:stage_name) { 'build' }
it { is_expected.to be_nil } it 'raises an ActiveRecord exception' do
expect do
subject
end.to raise_exception(ActiveRecord::RecordNotFound)
end
end end
end end
end end
...@@ -285,4 +285,6 @@ describe Ci::Stage, :models do ...@@ -285,4 +285,6 @@ describe Ci::Stage, :models do
end end
end end
end end
it_behaves_like 'manual playable stage', :ci_stage_entity
end end
...@@ -367,28 +367,4 @@ describe HasStatus do ...@@ -367,28 +367,4 @@ describe HasStatus do
puts subject puts subject
end end
end end
describe '#blocked_or_skipped?' do
%w[ci_pipeline ci_stage ci_build generic_commit_status].each do |type|
let(:object) { build(type, status: status) }
context 'when status is scheduled' do
let(:status) { :scheduled }
it { is_expected.to be_truthy }
end
context 'when status is manual' do
let(:status) { :manual }
it { is_expected.to be_truthy }
end
context 'when status is skipped' do
let(:status) { :skipped }
it { is_expected.to be_truthy }
end
end
end
end end
...@@ -157,6 +157,7 @@ describe PipelineSerializer do ...@@ -157,6 +157,7 @@ describe PipelineSerializer do
it 'verifies number of queries', :request_store do it 'verifies number of queries', :request_store do
recorded = ActiveRecord::QueryRecorder.new { subject } recorded = ActiveRecord::QueryRecorder.new { subject }
expected_queries = Gitlab.ee? ? 38 : 31 expected_queries = Gitlab.ee? ? 38 : 31
expect(recorded.count).to be_within(2).of(expected_queries) expect(recorded.count).to be_within(2).of(expected_queries)
expect(recorded.cached_count).to eq(0) expect(recorded.cached_count).to eq(0)
end end
......
...@@ -48,8 +48,8 @@ describe StageEntity do ...@@ -48,8 +48,8 @@ describe StageEntity do
expect(subject[:title]).to eq 'test: passed' expect(subject[:title]).to eq 'test: passed'
end end
it 'does not contain play_manual_details info' do it 'does not contain play_details info' do
expect(subject[:play_manual_details]).not_to be_present expect(subject[:status][:action]).not_to be_present
end end
context 'when the jobs should be grouped' do context 'when the jobs should be grouped' do
...@@ -75,7 +75,7 @@ describe StageEntity do ...@@ -75,7 +75,7 @@ describe StageEntity do
let(:stage) { create(:ci_stage_entity, status: 'skipped') } let(:stage) { create(:ci_stage_entity, status: 'skipped') }
it 'contains play_all_manual' do it 'contains play_all_manual' do
expect(subject[:play_manual_details]).to be_present expect(subject[:status][:action]).to be_present
end end
end end
...@@ -83,7 +83,7 @@ describe StageEntity do ...@@ -83,7 +83,7 @@ describe StageEntity do
let(:stage) { create(:ci_stage_entity, status: 'scheduled') } let(:stage) { create(:ci_stage_entity, status: 'scheduled') }
it 'contains play_all_manual' do it 'contains play_all_manual' do
expect(subject[:play_manual_details]).to be_present expect(subject[:status][:action]).to be_present
end end
end end
...@@ -91,7 +91,7 @@ describe StageEntity do ...@@ -91,7 +91,7 @@ describe StageEntity do
let(:stage) { create(:ci_stage_entity, status: 'manual') } let(:stage) { create(:ci_stage_entity, status: 'manual') }
it 'contains play_all_manual' do it 'contains play_all_manual' do
expect(subject[:play_manual_details]).to be_present expect(subject[:status][:action]).to be_present
end end
end end
end end
......
...@@ -27,32 +27,5 @@ describe StageSerializer do ...@@ -27,32 +27,5 @@ describe StageSerializer do
expect(subject).not_to be_empty expect(subject).not_to be_empty
end end
end end
context 'with manual builds' do
let(:resource) { create(:ci_stage_entity, status: 'manual') }
subject { serializer.represent(resource, { play_all: play_all }) }
before do
create_list(:ci_build, 3, :manual,
stage: resource.name, stage_id: resource.id)
end
context 'with play all option' do
let(:play_all) { true }
it 'returns play manual details' do
expect(subject[:play_manual_details]).to be_present
end
end
context 'without play all option' do
let(:play_all) { false }
it 'does not return play manual details' do
expect(subject[:play_manual_details]).not_to be_present
end
end
end
end end
end end
# frozen_string_literal: true
require 'spec_helper'
describe Ci::PlayManualStageService, '#execute' do
let(:current_user) { create(:user) }
let(:pipeline) { create(:ci_pipeline, user: current_user) }
let(:project) { pipeline.project }
let(:service) { described_class.new(project, current_user, pipeline: pipeline) }
let(:stage_status) { 'manual' }
let(:stage) do
create(:ci_stage_entity,
pipeline: pipeline,
project: project,
name: 'test')
end