From 7a94a2d35cf02598aac6360a0843f66492a0f3f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20R=C5=AF=C5=BEi=C4=8Dka?= Date: Mon, 6 Aug 2018 05:03:25 -0700 Subject: [PATCH] Fixes #21704 - Prevent user from rerunning a job with missing template --- .../api/v2/job_invocations_controller.rb | 11 ++++++++--- app/models/job_invocation_composer.rb | 11 ++++++++++- app/views/job_invocations/_form.html.erb | 12 ++++++++++-- .../foreman_remote_execution_factories.rb | 2 +- .../api/v2/job_invocations_controller_test.rb | 14 ++++++++++++++ test/unit/job_invocation_composer_test.rb | 18 ++++++++++++++++++ 6 files changed, 61 insertions(+), 7 deletions(-) diff --git a/app/controllers/api/v2/job_invocations_controller.rb b/app/controllers/api/v2/job_invocations_controller.rb index 0e5903bf8..36a7d7116 100644 --- a/app/controllers/api/v2/job_invocations_controller.rb +++ b/app/controllers/api/v2/job_invocations_controller.rb @@ -111,9 +111,14 @@ def cancel param :failed_only, :bool def rerun composer = JobInvocationComposer.from_job_invocation(@job_invocation, params) - composer.trigger! - @job_invocation = composer.job_invocation - process_response @job_invocation + if composer.rerun_possible? + composer.trigger! + @job_invocation = composer.job_invocation + process_response @job_invocation + else + render :json => { :error => _('Could not rerun job %{id} because its template could not be found') % { :id => composer.reruns } }, + :status => 404 + end end private diff --git a/app/models/job_invocation_composer.rb b/app/models/job_invocation_composer.rb index ae2923a8e..3accc382c 100644 --- a/app/models/job_invocation_composer.rb +++ b/app/models/job_invocation_composer.rb @@ -186,7 +186,8 @@ def params :concurrency_control => concurrency_control_params, :execution_timeout_interval => job_invocation.execution_timeout_interval, :remote_execution_feature_id => job_invocation.remote_execution_feature_id, - :template_invocations => template_invocations_params }.with_indifferent_access + :template_invocations => template_invocations_params, + :reruns => job_invocation.id }.with_indifferent_access end private @@ -297,6 +298,7 @@ def job_template end attr_accessor :params, :job_invocation, :host_ids, :search_query + attr_reader :reruns delegate :job_category, :remote_execution_feature_id, :pattern_template_invocations, :template_invocations, :targeting, :triggering, :to => :job_invocation def initialize(params, set_defaults = false) @@ -304,6 +306,7 @@ def initialize(params, set_defaults = false) @set_defaults = set_defaults @job_invocation = JobInvocation.new @job_invocation.task_group = JobInvocationTaskGroup.new + @reruns = params[:reruns] compose @host_ids = validate_host_ids(params[:host_ids]) @@ -342,6 +345,8 @@ def compose job_invocation.key_passphrase = params[:key_passphrase] job_invocation.sudo_password = params[:sudo_password] + job_invocation.job_category = nil unless rerun_possible? + self end # rubocop:enable Metrics/AbcSize @@ -470,6 +475,10 @@ def job_template_ids job_invocation.pattern_template_invocations.map(&:template_id) end + def rerun_possible? + !(reruns && job_invocation.pattern_template_invocations.empty?) + end + private # builds input values for a given templates id based on params diff --git a/app/views/job_invocations/_form.html.erb b/app/views/job_invocations/_form.html.erb index ff6b3636a..9cc224864 100644 --- a/app/views/job_invocations/_form.html.erb +++ b/app/views/job_invocations/_form.html.erb @@ -6,7 +6,15 @@ <%= form_for @composer.job_invocation, :html => {'data-refresh-url' => refresh_job_invocations_path, :id => 'job_invocation_form'} do |f| %> - <%= selectable_f f, :job_category, @composer.available_job_categories, {}, :label => _('Job category') %> + <% unless @composer.rerun_possible? %> + <%= alert :text => _('Could not rerun job %{id} because its template could not be found') % { :id => @composer.reruns }, + :class => 'alert alert-block alert-danger has-error', + :close => false %> + <% end %> + + <%= selectable_f f, :job_category, @composer.available_job_categories, + { :selected => @composer.job_invocation.job_category, :include_blank => @composer.job_invocation.job_category.nil? }, + :label => _('Job category') %> <%= f.hidden_field(:remote_execution_feature_id, :value => @composer.remote_execution_feature_id) %> <% selected_templates_per_provider = {} %> @@ -116,5 +124,5 @@ <%= render :partial => 'preview_hosts_modal' %> - <%= submit_or_cancel f, false, :cancel_path => job_invocations_path %> + <%= submit_or_cancel f, false, :cancel_path => job_invocations_path, :disabled => !@composer.rerun_possible? %> <% end %> diff --git a/test/factories/foreman_remote_execution_factories.rb b/test/factories/foreman_remote_execution_factories.rb index 584681272..803901fba 100644 --- a/test/factories/foreman_remote_execution_factories.rb +++ b/test/factories/foreman_remote_execution_factories.rb @@ -1,7 +1,7 @@ FactoryBot.define do factory :job_template do sequence(:name) { |n| "Job template #{n}" } - sequence(:job_category) { |n| "job name #{n}" } + sequence(:job_category) { |n| "Job name #{n}" } template 'id' provider_type 'SSH' organizations { [Organization.find_by(name: 'Organization 1')] } if SETTINGS[:organizations_enabled] diff --git a/test/functional/api/v2/job_invocations_controller_test.rb b/test/functional/api/v2/job_invocations_controller_test.rb index 5a797ee10..1a499e09b 100644 --- a/test/functional/api/v2/job_invocations_controller_test.rb +++ b/test/functional/api/v2/job_invocations_controller_test.rb @@ -6,6 +6,11 @@ class JobInvocationsControllerTest < ActionController::TestCase setup do @invocation = FactoryBot.create(:job_invocation, :with_template, :with_task) @template = FactoryBot.create(:job_template, :with_input) + + # Without this the template in template_invocations and in pattern_template_invocations + # would belong to different job_categories, causing trouble when trying to rerun + @invocation.job_category = @invocation.pattern_template_invocations.first.template.job_category + @invocation.save! end test 'should get index' do @@ -178,6 +183,8 @@ class JobInvocationsControllerTest < ActionController::TestCase test 'should rerun failed only' do @invocation = FactoryBot.create(:job_invocation, :with_template, :with_failed_task) + @invocation.job_category = @invocation.pattern_template_invocations.first.template.job_category + @invocation.save! JobInvocation.any_instance.expects(:generate_description) JobInvocationComposer.any_instance .expects(:validate_job_category) @@ -191,6 +198,13 @@ class JobInvocationsControllerTest < ActionController::TestCase targeting.user_id.must_equal users(:admin).id targeting.search_query.must_equal "name ^ (#{hostnames.join(',')})" end + + test 'should return 404 if template is not found' do + @invocation.job_category = 'Missing category' + @invocation.save! + post :rerun, params: { :id => @invocation.id } + assert_response 404 + end end end end diff --git a/test/unit/job_invocation_composer_test.rb b/test/unit/job_invocation_composer_test.rb index bba5da9f8..c1e702199 100644 --- a/test/unit/job_invocation_composer_test.rb +++ b/test/unit/job_invocation_composer_test.rb @@ -156,6 +156,24 @@ class JobInvocationComposerTest < ActiveSupport::TestCase end end + describe '#rerun_possible?' do + it 'is true when not rerunning' do + composer.must_be :rerun_possible? + end + + it 'is true when rerunning with pattern tempalte invocations' do + composer.expects(:reruns).returns(1) + composer.job_invocation.expects(:pattern_template_invocations).returns([1]) + composer.must_be :rerun_possible? + end + + it 'is false when rerunning without pattern template invocations' do + composer.expects(:reruns).returns(1) + composer.job_invocation.expects(:pattern_template_invocations).returns([]) + composer.wont_be :rerun_possible? + end + end + describe '#selected_job_templates' do it 'returns no template if none was selected through params' do composer.selected_job_templates.must_be_empty