Fixed Service Reconfigure Form and Request Summary Pages #8953
Conversation
|
Hey @DavidResende0 , any idea why most of these fields are disabled? I have all fields disabled. In your screenshots, you have one enabled. However I was able to hit the submit button to reconfigure. Seems good. |
There's a setting when creating the dialog fields that determines whether the field can be reconfigured or not. If I was guessing, I would say you have that setting set to false. |
|
@DavidResende0 @jeffibm I can't find any services with this reconfigure button but if @jeffibm already reviewed and approved it should be fine. |
Hey @gilbert, I guess you need to select the |
| @@ -33,6 +33,8 @@ def button | |||
| service_retire | |||
| when 'service_retire_now' | |||
| service_retire_now | |||
| when 'service_reconfigure' | |||
| javascript_redirect(:action => 'service_reconfigure', :id => params[:id]) | |||
There was a problem hiding this comment.
Same question as #8930 (comment)
Should we raise here in an else case with a generic "not supported" warning? I assume before this change, it didn't do anything when you clicked the button. What's the best way to handle a button with no defined action?
There was a problem hiding this comment.
Well I'm pretty sure the button won't appear if the service can't be reconfigured but #8930 (comment) should cover this as well right?
There was a problem hiding this comment.
Yes, that would cover it.
I don't know if that's the best solution but could help us if we extend this case statement in the future. So, instead of the case statement, it would just instantiate the correct service button class and call "press" on it. Ideally, the case statement could be methods called on service button classes instead. The different button classes could implement the body of each branch of the case statement and ultimately inherent from the base service button class which could be "the else case" and could decide to raise, alert, or silently do nothing as it could be treated as an abstract class.
There was a problem hiding this comment.
Note, that could be the wrong abstraction but case statements that grow in length usually have an abstraction we can do so we can "hide" the implementation in those classes.
There was a problem hiding this comment.
@DavidResende0 I'm not suggesting we abstract classes for the service button classes in this PR, just that it's something I see the moment I see the growing case statements based on "pseudo-classlike" objects. For now, the fix + a "unsupported button action" prompt is more than enough
DavidResende0
force-pushed
the
service-reconfigure-fix
branch
from
486083e
to
c036814
Compare
|
Checked commit DavidResende0@c036814 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint app/views/miq_request/_service_reconfigure_show.html.haml
app/views/service/service_reconfigure.html.haml
|
Continuation Of: #8428
Returns the reconfigure service functionality accidentally removed in #8229. Also fixes an issue with the reconfigure service request summary page that was preventing the "Dialog Options" section from displaying correctly.
Preview:
Service Summary Page:
Reconfigure Page:
Request Summary Page:
Note: #8951 will eventually convert these pages over to react