Custom Buttons with dialogs should be running invoke #506
Conversation
|
@miq-bot add_reviewer @eclarizio |
|
@miq-bot add_label blocker |
|
@bdunne this is a blocker and it's a single line fix, it'd be great if you'd look. |
|
Checked commit d-m-u@c0b4745 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
| @@ -193,6 +193,7 @@ def invoke_custom_action(type, resource, action, data) | |||
|
|
|||
| def invoke_custom_action_with_dialog(type, resource, action, data, custom_button) | |||
| result = begin | |||
| custom_button.publish_event(nil, resource) | |||
There was a problem hiding this comment.
If a button is pressed in the UI, will it raise an event (I assume it doesn't call this API method currently)? If so, maybe it's better to move this logic down into the model so that it's shared by both?
There was a problem hiding this comment.
The problem at the moment is there is no currently shared path. A custom button called without a dialog is processed in the invoke_custom_action method above and then gets delivered to automate from the invoke method.
Buttons with a dialog need to go through this method and ResourceActionWorkflow before sending to automate.
Maybe we refactor these methods to call into a custom button method that then branches to different paths, but maybe that is a followup PR so we can resolve this blocker issue first.
There was a problem hiding this comment.
@bdunne you okay with refactoring in a later step, please? there might be fruit snacks in it for you...
There was a problem hiding this comment.
¯\_(ツ)_/¯ I guess. I'm surprised that there is no shared code that does this. Does the UI not publish the event either?
There was a problem hiding this comment.
No sure what the "Does the UI not publish the event either?". The custom button invoked here is either with or without a service dialog. There are two separate methods here that invoke that, the method being modified here invoke_custom_action_with_dialog and invoke_custom_action which is directly above. For invoke_custom_action method the event is created as part of custom_button.invoke(resource).
Is that what you are asking?
|
@d-m-u was there any further edits for this PR prior to merge? |
|
@JPrause I think the master PR should get in first and that should address these test failures. Other than that, I need buy-in that I'm allowed to do the refactoring as a separate and later step, that's a decision that's kinda out of my hands... |
|
I think the test failure is unrelated as all of the master API's red anyway. |
|
Does that mean merge is coming? 👍 |
|
@eclarizio Do you have any outstanding issues here? @gtanzillo @abellotti Please review in @bdunne absence. |
Custom Buttons with dialogs should be running invoke (cherry picked from commit d8e8f30) https://bugzilla.redhat.com/show_bug.cgi?id=1511126
|
Hammer backport details: |
For https://bugzilla.redhat.com/show_bug.cgi?id=1511126.
Since the custom button
publish eventmethod is where we create custom button events, and we expect (per convo with GM) that all cb runs will create events, I think all cb runs, regardless of sans/avec dialogue, should be running the publish event method.As GM points out, this is a blocker bug, so perhaps refactoring this to better reflect the fact that the paths for custom buttons branch should be done after this gets merged. Er. Please, people?
depends on
ManageIQ/manageiq#18178