Fixes #11458 - Override inline for class parameters #2858

tbrisker · 2015-10-22T15:17:39Z

This implements override inline for smart class parameters and smart
variables in host and hostgroup edit forms.
Global variable override inline will be done in a seperate PR.

Authors:
Ori Rabin orrabin@redhat.com
Tomer Brisker tbrisker@redhat.com

domcleal · 2015-11-02T13:46:21Z

app/helpers/lookup_keys_helper.rb

        diagnostic_helper = popover('', _("Optional parameter without value.<br/><i>Won\'t be given to Puppet.</i> <br><br><b>Description:</b> %s") % key.description, :icon => "warning-sign")
      end
    else
-      diagnostic_helper = popover('', _("<b>Description:</b> %{desc}<br><b>Type:</b> %{type}<br> <b>Matcher:</b> %{matcher}") % { :desc => key.description, :type => key.key_type, :matcher => matcher}, :data => { :placement => 'top' })
+      diagnostic_helper = popover('', _("<b>Description:</b> %{desc}<br/><b>Type:</b> %{type}<br/> <b>Matcher:</b> %{matcher} <br/><b>Value:</b> %{original_value}") % { :desc => key.description, :type => key.key_type, :matcher => matcher, :original_value => original_value }, :data => { :placement => 'top' }, :title => _("Original value info"))


Is it worth calling this the inherited or original value, rather than value?

Note that the popover title states this is all info related to the original value, do you think this needs more clarificaion?

domcleal · 2015-11-02T14:02:46Z

When editing a host, global parameters inherited from a host group etc. no longer are readonly - they have editable text boxes.
Moving the use_puppet_default help text to the top instead of next to every checkbox is good, but consider adding a tooltip to the checkbox just so you know what it is. It doesn't need the full explanation, just the title.
The value fields are a lot more narrow than the previous overridden value fields, and slightly narrower than the inherited value fields used to be. I think this will be OK because of the full screen button, but can you see if we can reclaim any more horizontal space?

Perhaps from the Use Puppet default column or even going back to icons instead of override/remove buttons?

It would be good to highlight overridden fields more, as this was a benefit for some users of the old layout. The background colour of the text field and presence of the checkbox are the main visual clues right now, but I'd suggest also changing the border colour of the value text boxes (perhaps to blue?) or making the parameter name bold.

Else probably something for a future PR, some links/buttons at the top to filter/only view overridden parameters.

Can the remove/override buttons be the same width?
I don't think the PR makes it any worse than it currently is, but FYI I'm bumping into this nasty issue when overriding YAML lookup keys inherited from host groups: http://projects.theforeman.org/issues/12284

Looks really good, I'm impressed!

tbrisker · 2015-11-03T13:33:37Z

@domcleal I have updated to original commit to address your first comment, and added the corrections for the rest of the comments to a separate commit which can be squashed later if accepted, just to allow simple reversion if we decide to leave it as is and address those issues in the future.

dLobatog · 2015-11-05T12:40:58Z

@tbrisker I've taken a look - not properly reviewed it at all, but from the UI/UX point of view.

Every time I select an environment, a new header Puppet class parameters is added. See it in action

The yellow background color for overrides is doesn't fit very well I would say. Also for parameters without value, it shows the warning in yellow by default (when it's not overridden), and the border in red?

.

What @domcleal suggested seems to look better, I would also suggest to display a small popover 'Overridden' when you hover the mouse on that table row:

The override button looks very out of place, and it shows up on the Name cell. Why not adding it to the value textarea? Or even the actions column is better than this one I'd say.
Long Puppet class names are truncated but it'd be nice to see the whole name on mouse hover.
'Use puppet default' doesn't work at all? It doesn't show checkboxes.

Let me know what you think 👌

tbrisker · 2015-11-05T15:16:20Z

@dLobatog I addressed most of your comments.
For the values that have an empty default - if they are not required they show a yellow warning and the border is yellow, if they are required the border is red. I have changed it now so that clicking on override will remove the border.
Long class names should show whole name on hover, however the tooltip is attached to the <td> so it might appear higher if the class has multiple parameters.
'Use puppet default' checkbox is only displayed for smart class parameters, not smart variables as they have no puppet default.

dLobatog · 2015-11-06T18:25:23Z

@tbrisker Thanks! A few more:

I don't see any tooltips on hover
Top left and bottom left border-radius of the override button should be 0px to blend in with the full screen button.
Is this the way required empty values are supposed to look? It looks a bit strange in my opinion, the warning sign is enough in my opinion. I can't see any guidelines for highlighting elements in a table on Patternfly @kybaker any hints?

This doesn't have a lot to do with the PR, but what do you think about softening the borders of the input-group-addon and the textarea? I might do it on a PR after this unless you want to include it here, it's fairly easy.

dLobatog · 2015-11-17T07:35:46Z

I tried to fix a bit issues with key_with_diagnostic and _class_parameters on tbrisker#2 , I'll comment in line there

dLobatog · 2015-11-19T18:34:25Z

app/helpers/lookup_keys_helper.rb

+      value_for_key = value.try(:[], lookup_key.key)
+      if value_for_key.present?
+        [value_for_key[:value],
+          "#{value_for_key[:element]} (#{value_for_key[:element_name]})"]


Indentation's off 1 space here, rubocop's unhappy

dLobatog · 2015-11-19T19:07:44Z

@tbrisker @orrabin Aside from the class checking on value_matcher I see this buggy behavior:

It clears the original value if I decide to override it and then I change my mind

Sorry for holding this one up, but I think it's important we keep this part of the code as clear as possible since 'it needs refactoring' but it may take a long time before someone actually goes ahead and does it. Right now aside from the 2 issues above I say 👍 to the rest of the PR, thanks !

This implements override inline for smart class parameters and smart variables in host and hostgroup edit forms. Global variable override inline will be done in a seperate PR. Authors: Ori Rabin <orrabin@redhat.com> Tomer Brisker <tbrisker@redhat.com>

tbrisker · 2015-11-19T19:29:58Z

@dLobatog I addressed all your comments and fixed the js bug that cleared the field.

dLobatog · 2015-11-20T10:54:59Z

Merged as 1e1339f, thanks @tbrisker & @orrabin , nice work!

theforeman-bot added Not yet reviewed Needs testing labels Oct 22, 2015

tbrisker force-pushed the 11458 branch 3 times, most recently from fa8994b to 5820ee6 Compare October 22, 2015 17:53

tbrisker force-pushed the 11458 branch 3 times, most recently from 5820ee6 to 4b55112 Compare November 1, 2015 09:59

domcleal reviewed Nov 2, 2015
View reviewed changes

domcleal added Waiting on contributor and removed Not yet reviewed labels Nov 2, 2015

tbrisker force-pushed the 11458 branch from e8896e5 to 70e26f6 Compare November 3, 2015 13:31

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Nov 3, 2015

dLobatog added Waiting on contributor and removed Needs testing labels Nov 5, 2015

domcleal removed the Needs re-review label Nov 5, 2015

tbrisker force-pushed the 11458 branch from 70e26f6 to a0994f2 Compare November 5, 2015 15:11

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Nov 5, 2015

dLobatog added Waiting on contributor and removed Needs re-review Needs testing labels Nov 6, 2015

dLobatog removed the Needs testing label Nov 10, 2015

tbrisker force-pushed the 11458 branch from 30b48cc to bc99617 Compare November 11, 2015 11:53

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Nov 11, 2015

orrabin mentioned this pull request Nov 11, 2015

Fixes #6007 - Allowing to hide smart class parameters #2866

Closed

tbrisker force-pushed the 11458 branch 3 times, most recently from 1fdd5a2 to 69a993c Compare November 12, 2015 13:34

dLobatog mentioned this pull request Nov 17, 2015

Attempt to clean up puppetclass params helper and view tbrisker/foreman#2

Closed

tbrisker force-pushed the 11458 branch 2 times, most recently from f796329 to e878ab4 Compare November 19, 2015 13:04

dLobatog reviewed Nov 19, 2015
View reviewed changes

dLobatog added Waiting on contributor and removed Needs re-review Needs testing labels Nov 19, 2015

tbrisker force-pushed the 11458 branch from e878ab4 to 1e1339f Compare November 19, 2015 19:22

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Nov 19, 2015

dLobatog closed this Nov 20, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #11458 - Override inline for class parameters #2858

Fixes #11458 - Override inline for class parameters #2858

tbrisker commented Oct 22, 2015

domcleal Nov 2, 2015

tbrisker Nov 3, 2015

domcleal commented Nov 2, 2015

tbrisker commented Nov 3, 2015

dLobatog commented Nov 5, 2015

tbrisker commented Nov 5, 2015

dLobatog commented Nov 6, 2015

dLobatog commented Nov 17, 2015

dLobatog Nov 19, 2015

tbrisker Nov 19, 2015

dLobatog commented Nov 19, 2015

tbrisker commented Nov 19, 2015

dLobatog commented Nov 20, 2015

Fixes #11458 - Override inline for class parameters #2858

Fixes #11458 - Override inline for class parameters #2858

Conversation

tbrisker commented Oct 22, 2015

domcleal Nov 2, 2015

Choose a reason for hiding this comment

tbrisker Nov 3, 2015

Choose a reason for hiding this comment

domcleal commented Nov 2, 2015

tbrisker commented Nov 3, 2015

dLobatog commented Nov 5, 2015

tbrisker commented Nov 5, 2015

dLobatog commented Nov 6, 2015

dLobatog commented Nov 17, 2015

dLobatog Nov 19, 2015

Choose a reason for hiding this comment

tbrisker Nov 19, 2015

Choose a reason for hiding this comment

dLobatog commented Nov 19, 2015

tbrisker commented Nov 19, 2015

dLobatog commented Nov 20, 2015