Pass additional information to the API when ordering a service catalog #2885

eclarizio · 2017-11-30T18:11:54Z

Resource action id, target id, and target type need to be passed in order to establish the context that some dynamic methods may use when going through automate. This corresponds with @jntullo's API PR here.

https://bugzilla.redhat.com/show_bug.cgi?id=1518390

@miq-bot add_label gaprindashvili/yes, bug, blocker

@miq-bot assign @h-kataria

@d-m-u, @jntullo Mind taking a look and making sure I didn't miss something obvious?

eclarizio · 2017-11-30T18:22:16Z

@syncrou Can you review as well please?

eclarizio · 2017-11-30T18:52:09Z

API PR has been merged. I would fix the code climate issue but there's just going to be a longer chain added with this PR.

My suggestion would be to maybe have each new model that gets added implement a class method called something like custom_button_endpoints that could then be utilized, but for this urgent bug fix, I think it's just not feasible to do right now.

syncrou · 2017-11-30T19:07:22Z

@eclarizio - This looks fine to me - except for the ugly argument list - which you just were talking about - I would like to see that solved - but it sounds like that is a change for another day. 👍

eclarizio · 2017-12-01T15:53:19Z

WIP-ing as I think I just found an issue. Testing and will un-WIP when I fix.

ohadlevy · 2017-12-01T15:57:54Z

can you please add tests? - thanks!

https://bugzilla.redhat.com/show_bug.cgi?id=1518390 Resource action id, target id, and target type need to be passed in order to establish the context that some dynamic methods may use when going through automate.

https://bugzilla.redhat.com/show_bug.cgi?id=1518390

eclarizio · 2017-12-04T16:15:48Z

This will need ManageIQ/manageiq-api#233 to be merged first to handle the new refresh parameters. (Well, it doesn't need to be merged first, but it won't work without it because passing the extra parameters to the API is what will establish the context)

himdel · 2017-12-04T17:01:01Z

app/controllers/catalog_controller.rb

-        :finish_submit_endpoint => svc_catalog_provision_finish_submit_endpoint,
-        :cancel_endpoint        => "/catalog/explorer"
-      }
-      dialog_initialize(ra, options)


This means no @edit.

The templates still look at @edit and even @edit[:explorer] .. is that intentional?

(at least when using the old product setting they do)

You're right, it's only for using the old product setting. I'll update with your suggested change, thanks.

himdel · 2017-12-04T17:20:29Z

Code looks good 👍

With the default settings, everything works 👍

With Settings.product.old_dialog_user_ui, I get this when I click "Order" on a service detail screen:

 Error caught: [ActionView::Template::Error] undefined method `dialog' for nil:NilClass
/home/himdel/manageiq-ui-classic/app/helpers/application_helper/dialogs.rb:153:in `build_auto_refreshable_field_indicies'
/home/himdel/manageiq-ui-classic/app/views/shared/dialogs/_dialog_provision.html.haml:23:in `__home_himdel_manageiq_ui_classic_app_views_shared_dialogs__dialog_provision_html_haml___2171982048943848243_70122831795060'
...
/home/himdel/manageiq-ui-classic/app/helpers/application_helper.rb:1740:in `block in r'
/home/himdel/manageiq-ui-classic/app/controllers/catalog_controller.rb:1992:in `replace_right_cell'
/home/himdel/manageiq-ui-classic/app/controllers/catalog_controller.rb:550:in `svc_catalog_provision'

himdel · 2017-12-04T17:22:08Z

@eclarizio

diff --git a/app/controllers/catalog_controller.rb b/app/controllers/catalog_controller.rb
index a807e5559b..f196dd8234 100644
--- a/app/controllers/catalog_controller.rb
+++ b/app/controllers/catalog_controller.rb
@@ -547,7 +547,11 @@ class CatalogController < ApplicationController
         ra, st, svc_catalog_provision_finish_submit_endpoint
       )
 
-      replace_right_cell(:action => "dialog_provision", :dialog_locals => options[:dialog_locals])
+      if Settings.product.old_dialog_user_ui
+        dialog_initialize(ra, options)
+      else
+        replace_right_cell(:action => "dialog_provision", :dialog_locals => options[:dialog_locals])
+      end
     else
       # if catalog item has no dialog and provision button was pressed from list view
       add_flash(_("No Ordering Dialog is available"), :warning)

lets me use both the old and the new version :).

https://bugzilla.redhat.com/show_bug.cgi?id=1518390 As before, resource action id, target id, and target type need to be passed to the API in order to establish the context for dialog fields when they are refreshed via the /service_dialogs endpoint

miq-bot · 2017-12-04T17:44:58Z

Checked commits eclarizio/manageiq-ui-classic@d7c7f9c~...cad3a39 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 1 offense detected

spec/services/dialog_local_service_spec.rb

❗ - Line 11, Col 9 - Style/FirstParameterIndentation - Indent the first parameter one step more than service.determine_dialog_locals_for_svc_catalog_provision(.

Pass additional information to the API when ordering a service catalog (cherry picked from commit 6e2fb56) https://bugzilla.redhat.com/show_bug.cgi?id=1520678

simaishi · 2017-12-04T22:39:29Z

Gaprindashvili backport details:

$ git log -1
commit 5f6a67f853b481499a837062009cae887467f07d
Author: Dan Clarizio <[email protected]>
Date:   Mon Dec 4 14:07:47 2017 -0800

    Merge pull request #2885 from eclarizio/BZ1518390
    
    Pass additional information to the API when ordering a service catalog
    (cherry picked from commit 6e2fb56a6c1e59525a4b1d31770f65c1ca554bb4)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1520678

miq-bot added blocker bug gaprindashvili/yes labels Nov 30, 2017

miq-bot assigned h-kataria Nov 30, 2017

eclarizio changed the title ~~Pass additional information to the API when ordering a service catalog~~ [WIP] Pass additional information to the API when ordering a service catalog Dec 1, 2017

miq-bot added the wip label Dec 1, 2017

eclarizio added 2 commits December 1, 2017 09:19

Pass additional information to the API when ordering a service catalog

d7c7f9c

https://bugzilla.redhat.com/show_bug.cgi?id=1518390 Resource action id, target id, and target type need to be passed in order to establish the context that some dynamic methods may use when going through automate.

Extract dialog local logic and add specs around it

bcfee50

https://bugzilla.redhat.com/show_bug.cgi?id=1518390

eclarizio force-pushed the BZ1518390 branch from 8646d2c to bcfee50 Compare December 1, 2017 17:19

eclarizio changed the title ~~[WIP] Pass additional information to the API when ordering a service catalog~~ Pass additional information to the API when ordering a service catalog Dec 4, 2017

eclarizio mentioned this pull request Dec 4, 2017

Specify a target and resource when refreshing a dialog field ManageIQ/manageiq-api#233

Merged

miq-bot removed the wip label Dec 4, 2017

dclarizio added the services label Dec 4, 2017

dclarizio requested a review from himdel December 4, 2017 16:36

himdel added the pending core label Dec 4, 2017

himdel reviewed Dec 4, 2017

View reviewed changes

eclarizio force-pushed the BZ1518390 branch from 2d6b88c to cad3a39 Compare December 4, 2017 17:34

himdel approved these changes Dec 4, 2017

View reviewed changes

dclarizio assigned dclarizio and unassigned h-kataria Dec 4, 2017

dclarizio removed the pending core label Dec 4, 2017

dclarizio merged commit 6e2fb56 into ManageIQ:master Dec 4, 2017

dclarizio added this to the Sprint 75 Ending Dec 11, 2017 milestone Dec 4, 2017

simaishi pushed a commit that referenced this pull request Dec 4, 2017

Merge pull request #2885 from eclarizio/BZ1518390

5f6a67f

Pass additional information to the API when ordering a service catalog (cherry picked from commit 6e2fb56) https://bugzilla.redhat.com/show_bug.cgi?id=1520678

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Dec 4, 2017

eclarizio mentioned this pull request Dec 7, 2017

BZ#1518390-Pass additional information to the API when refreshing a dialog field ManageIQ/manageiq-ui-service#1324

Merged

lgalis mentioned this pull request Dec 7, 2017

Dialogs for custom buttons are not displayed #2984

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass additional information to the API when ordering a service catalog #2885

Pass additional information to the API when ordering a service catalog #2885

eclarizio commented Nov 30, 2017

eclarizio commented Nov 30, 2017

eclarizio commented Nov 30, 2017

syncrou commented Nov 30, 2017

eclarizio commented Dec 1, 2017

ohadlevy commented Dec 1, 2017

eclarizio commented Dec 4, 2017 •

edited

Loading

himdel Dec 4, 2017 •

edited

Loading

eclarizio Dec 4, 2017 •

edited

Loading

himdel commented Dec 4, 2017

himdel commented Dec 4, 2017

miq-bot commented Dec 4, 2017

simaishi commented Dec 4, 2017

Pass additional information to the API when ordering a service catalog #2885

Pass additional information to the API when ordering a service catalog #2885

Conversation

eclarizio commented Nov 30, 2017

eclarizio commented Nov 30, 2017

eclarizio commented Nov 30, 2017

syncrou commented Nov 30, 2017

eclarizio commented Dec 1, 2017

ohadlevy commented Dec 1, 2017

eclarizio commented Dec 4, 2017 • edited Loading

himdel Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

eclarizio Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

himdel commented Dec 4, 2017

himdel commented Dec 4, 2017

miq-bot commented Dec 4, 2017

simaishi commented Dec 4, 2017

eclarizio commented Dec 4, 2017 •

edited

Loading

himdel Dec 4, 2017 •

edited

Loading

eclarizio Dec 4, 2017 •

edited

Loading