Fixes #8217, #8214 - Rearranging override section in smart class parameter page and host page #1898

orrabin · 2014-11-03T07:27:27Z

No description provided.

domcleal · 2014-11-03T07:31:51Z

Missing "#" before the ticket number, so Redmine won't link it.

orrabin · 2014-11-03T07:35:22Z

@domcleal sorry, added "#".

ohadlevy · 2014-12-28T09:28:48Z

@orrabin cna you rebase please

orrabin · 2014-12-28T09:38:50Z

@ohadlevy rebased

tbrisker · 2014-12-28T12:06:25Z

app/views/common_parameters/_puppetclasses_parameters.html.erb

  <%= f.fields_for :lookup_values do |builder| %>
-    <%= render "common_parameters/puppetclass_parameter", :f => builder %>
+      <%= render "common_parameters/puppetclass_parameter", :f => builder %>


nitpick: indent

orrabin · 2014-12-29T07:32:31Z

@tbrisker thanks for your comments, can you please review again?

tbrisker · 2014-12-30T13:48:14Z

app/views/lookup_keys/_value.html.erb

-                    :onchange=>'toggleUsePuppetDefaultValue(this, "value")') if is_param %>
+  <%= checkbox_f(f, :use_puppet_default, :label => _('Use Puppet default'), :size => "col-md-8",
+                 :help_inline => popover(_("Explain use Puppet default"), _('Do not send this parameter via the ENC.<br>Puppet will use the value defined in the Puppet manifest for this parameter.')),
+                 :onchange=>'toggleUsePuppetDefaultValue(this, "value")') if is_param %>


nitpick: indentation

tbrisker · 2015-03-05T12:44:59Z

app/views/common_parameters/_puppetclass_parameter.html.erb

          <%= f.check_box :use_puppet_default, :'data-property' => 'use_puppet_default',
                        :disabled => (not authorized_via_my_scope("host_editing", "edit_params")),
                        :onchange=>'toggleUsePuppetDefaultValue(this, "value")' %>
-          <%= popover('', _('Do not send this parameter via the ENC. Puppet will use the value defined in the puppet manifest for this parameter')) %>
+          <%= popover('', _("Do not send this parameter via the ENC.<br> Puppet will use the value defined in the puppet manifest for this parameter")) %>


This also appears in app/views/lookup_keys/_value.html.erb:7 and app/views/lookup_keys/_fields.html.erb:18. However only sometimes is it a popover - consider extracting to helper or otherwise changing to ensure consistency.

tbrisker · 2015-03-05T13:22:13Z

In general, the part in the puppetclass form looks good.
However, the host[group] forms need more work. The table headers don't match the table when adding host[group] parameters, or when overriding the hostgroup parameters in the host. The "Scope" header is unclear and the content for that column is always Global.

orrabin · 2015-03-10T13:33:22Z

@tbrisker thanks for the review.
I fixed most of your comments. The host form needs some UI changes for parameters but I think it should be in a different PR.

domcleal · 2015-03-18T11:59:19Z

app/helpers/common_parameters_helper.rb

@@ -17,4 +17,8 @@ def parameter_value_field(value)
      content_tag(:span, :class => "help-block") { popover(_("Additional info"), _("<b>Source:</b> %{type} %{name}") % {:type => _(value[:source].to_s), :name => source_name})}
    end
  end
+
+  def use_puppet_default_help link_title = _("Explain use Puppet default"), title = _("Use Puppet Default")


can you make the "Default" lower case please?

domcleal · 2015-03-18T12:06:45Z

The host form still seems to have an issue with headers, I see a second "Puppet class" header which is followed by global parameters:

orrabin · 2015-03-25T12:06:49Z

@domcleal these are two problems - the second puppet classes is actually overrides and the global parameters need their own header

orrabin · 2015-03-25T12:15:55Z

@domcleal thanks for testing, I fixed you comments and rebased.

… in smart class parameter page and host page

orrabin · 2015-03-26T14:42:19Z

[test]

tbrisker · 2015-03-31T11:56:04Z

Looks fine to me, there is still more work to be done to improve the layout and UX but it can be incremental after merging this PR.

dLobatog · 2015-04-17T12:06:28Z

Merged as 2b6605c, thanks @orrabin and especially @tbrisker and @domcleal for the review.

theforeman-bot added Not yet reviewed Needs testing labels Nov 3, 2014

orrabin force-pushed the 8217 branch from 37d15a4 to dc330ca Compare November 3, 2014 07:34

orrabin changed the title ~~Fixes 8217 - Rearranging override section in smart class parameter page~~ Fixes #8217 - Rearranging override section in smart class parameter page Nov 3, 2014

orrabin force-pushed the 8217 branch from dc330ca to 3f6bff7 Compare November 3, 2014 08:45

orrabin changed the title ~~Fixes #8217 - Rearranging override section in smart class parameter page~~ Fixes #8217, #8214 - Rearranging override section in smart class parameter page and host page Nov 3, 2014

orrabin force-pushed the 8217 branch 3 times, most recently from b5bc631 to 993a0aa Compare November 3, 2014 12:09

orrabin force-pushed the 8217 branch from 993a0aa to a2ad747 Compare December 28, 2014 09:36

tbrisker reviewed Dec 28, 2014
View reviewed changes

orrabin force-pushed the 8217 branch from a2ad747 to 77d7fe1 Compare December 29, 2014 07:31

tbrisker reviewed Dec 30, 2014
View reviewed changes

orrabin force-pushed the 8217 branch 2 times, most recently from d04be68 to d7010fc Compare January 1, 2015 11:28

orrabin force-pushed the 8217 branch from d7010fc to 7a3938d Compare January 15, 2015 14:58

dLobatog added Needs re-review and removed Not yet reviewed labels Feb 24, 2015

orrabin force-pushed the 8217 branch from 7a3938d to 9f2969c Compare February 25, 2015 08:13

tbrisker reviewed Mar 5, 2015
View reviewed changes

orrabin force-pushed the 8217 branch from 9f2969c to 3a6adb0 Compare March 10, 2015 13:25

domcleal reviewed Mar 18, 2015
View reviewed changes

domcleal added Waiting on contributor and removed Needs re-review labels Mar 18, 2015

orrabin force-pushed the 8217 branch from 3a6adb0 to 100dfdd Compare March 25, 2015 12:14

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

Fixes theforeman#8217, theforeman#8214 - Rearranging override section…

8509cb1

… in smart class parameter page and host page

orrabin force-pushed the 8217 branch from 100dfdd to 8509cb1 Compare March 26, 2015 08:47

dLobatog closed this Apr 17, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #8217, #8214 - Rearranging override section in smart class parameter page and host page #1898

Fixes #8217, #8214 - Rearranging override section in smart class parameter page and host page #1898

orrabin commented Nov 3, 2014

domcleal commented Nov 3, 2014

orrabin commented Nov 3, 2014

ohadlevy commented Dec 28, 2014

orrabin commented Dec 28, 2014

tbrisker Dec 28, 2014

orrabin commented Dec 29, 2014

tbrisker Dec 30, 2014

tbrisker Mar 5, 2015

tbrisker commented Mar 5, 2015

orrabin commented Mar 10, 2015

domcleal Mar 18, 2015

domcleal commented Mar 18, 2015

orrabin commented Mar 25, 2015

orrabin commented Mar 25, 2015

orrabin commented Mar 26, 2015

tbrisker commented Mar 31, 2015

dLobatog commented Apr 17, 2015

Fixes #8217, #8214 - Rearranging override section in smart class parameter page and host page #1898

Fixes #8217, #8214 - Rearranging override section in smart class parameter page and host page #1898

Conversation

orrabin commented Nov 3, 2014

domcleal commented Nov 3, 2014

orrabin commented Nov 3, 2014

ohadlevy commented Dec 28, 2014

orrabin commented Dec 28, 2014

tbrisker Dec 28, 2014

Choose a reason for hiding this comment

orrabin commented Dec 29, 2014

tbrisker Dec 30, 2014

Choose a reason for hiding this comment

tbrisker Mar 5, 2015

Choose a reason for hiding this comment

tbrisker commented Mar 5, 2015

orrabin commented Mar 10, 2015

domcleal Mar 18, 2015

Choose a reason for hiding this comment

domcleal commented Mar 18, 2015

orrabin commented Mar 25, 2015

orrabin commented Mar 25, 2015

orrabin commented Mar 26, 2015

tbrisker commented Mar 31, 2015

dLobatog commented Apr 17, 2015