Expose error messages from ServiceTemplate.orderable?

[ghost]

This pull request is a follow-up of ManageIQ/manageiq#19186 to accept an array of error messages from ServiceTemplate.orderable?, and to display these messages in the response.

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1744197
Depends on: ManageIQ/manageiq#19186

[ghost]

@miq-bot add-label enhancement, ivanchuk/yes, wip

[ghost]

@miq-bot remove-label wip
@miq-bot add-reviewer @d-m-u
@miq-bot add-reviewer @agrare

[agrare]

@miq-bot assign @abellotti

[agrare]

@fdupont-redhat can you look at the failing spec?

[d-m-u]

@agrare I think those two test failures are preexisting at the moment and have nothing to do with this code

[agrare]

Oh it looks like master has been failing for a while...

[agrare]

Yeah just noticed that @d-m-u thanks

[d-m-u]

they're fixed by #662

[ghost]

@agrare @d-m-u I'll wait for #662 to be merged then.

[abellotti]

maybe I'm missing something, but I don't see validate_order, api_request_allowed? or orderable? called from the API.

If they're no longer needed, they should go. though I do have a concern that we're letting go of the logic in api_request_allowed? checking UI calls & Settings.product. wasn't this needed for a fix at some point ?

[ghost]

@abellotti that's an error. I've re-added the check with an error message.

[abellotti]

Merci Fabien for the update.

[d-m-u]

Looks okay to me, @abellotti please take a look

[abellotti]

Ok, the problem with introducing the validate_<action> method is that it has a special purpose for the API to determine if an action should even be listed in the action list of a resource. So we really cannot have that here, also, validate_<action> should not raise exceptions. Best to put that exception as part of the ordering method. Here's the diff:

diff --git a/app/controllers/api/mixins/service_templates.rb b/app/controllers/api/mixins/service_templates.rb
index 3c24b0fb..bde03fca 100644
--- a/app/controllers/api/mixins/service_templates.rb
+++ b/app/controllers/api/mixins/service_templates.rb
@@ -3,7 +3,7 @@ module Api
     module ServiceTemplates
       def order_service_template(id, data, scheduled_time = nil)
         service_template = resource_search(id, :service_templates, ServiceTemplate)
-        validate_order(service_template)
+        raise BadRequestError, "Service ordering via API is not allowed" unless api_request_allowed?
         raise BadRequestError, "#{service_template_ident(service_template)} cannot be ordered - #{service_template.unsupported_reason(:order)}" unless service_template.supports_order?
 
         request_result = service_template.order(User.current_user, (data || {}), order_request_options, scheduled_time)
@@ -16,13 +16,6 @@ module Api
 
       private
 
-      def validate_order(_service_template)
-        raise BadRequestError, "Service ordering via API is not allowed" unless api_request_allowed?
-
-        true
-      end
-      alias orderable? validate_order
-
       def api_request_allowed?
         return true if request_from_ui?
         Settings.product.allow_api_service_ordering
diff --git a/spec/requests/service_templates_spec.rb b/spec/requests/service_templates_spec.rb
index 23093ed9..54e780ff 100644
--- a/spec/requests/service_templates_spec.rb
+++ b/spec/requests/service_templates_spec.rb
@@ -628,6 +628,7 @@ describe "Service Templates API" do
 
     context "with the product setting not allowing automate to run on submit" do
       let(:allow_api_service_ordering) { false }
+      let(:template_no_display) { FactoryBot.create(:service_template, :display => false) }
 
       context "if the token info is blank" do
         before do
@@ -636,7 +637,7 @@ describe "Service Templates API" do
 
         it "rejects the request" do
           api_basic_authorize action_identifier(:service_templates, :order, :resource_actions, :post)
-          post(api_service_template_url(nil, service_template), :params => { :action => "order" })
+          post(api_service_template_url(nil, template_no_display), :params => { :action => "order" })
           expected = {
             "error" => a_hash_including(
               "kind"    => "bad_request",

[miq-bot]

Checked commits fabiendupont/manageiq-api@d2c94a9~...67c5ce9 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

[abellotti]

Thanks @fdupont-redhat for the fix and update. LGTM!! 👍

[simaishi]

Ivanchuk backport details:

$ git log -1
commit f852e171afddb2f52d772126e958fa24c27c519b
Author: Alberto Bellotti <[email protected]>
Date:   Wed Sep 11 08:48:40 2019 -0400

    Merge pull request #656 from fdupont-redhat/v2v_allow_error_messages_in_service_template_orderable
    
    Expose error messages from ServiceTemplate.orderable?
    
    (cherry picked from commit 2aa1e4cfd2a4c12fc81a76533aba95b5b1050149)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1768520