Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes #7608 - Override all puppetclass parameters in one click #1792

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
4 participants
@karmab
Copy link
Member

commented Sep 23, 2014

proposed patch for #7608 adding a link on the puppetclasses page to set all parameters of the selected class to overriden

@dLobatog

This comment has been minimized.

Copy link
Member

commented Sep 25, 2014

[test]

end
notice _("Successfully overriden all parameters of puppetclass #{pname}")
else
error _("No parameters to override for puppetclass #{pname}")

This comment has been minimized.

Copy link
@dLobatog

dLobatog Sep 25, 2014

Member

This should be a notice too, also I think variable interpolation in model i18n strings should be like

_("No parameters to override for puppetclass %{name}") % { :name => puppetclass.name }

@@ -44,6 +44,21 @@ def destroy
end
end

def override
pclass = Puppetclass.find(params[:id])

This comment has been minimized.

Copy link
@dLobatog

dLobatog Sep 25, 2014

Member

You can pass the :name in the form and add override to the before_filter, and use @puppetclass here.

def override
pclass = Puppetclass.find(params[:id])
pname = pclass.name
if not pclass.class_params.empty?

This comment has been minimized.

Copy link
@dLobatog

dLobatog Sep 25, 2014

Member

not variable.empty? ▶️ variable.present?, try to use the positive.

pname = pclass.name
if not pclass.class_params.empty?
pclass.class_params.each do |p|
p.override = true

This comment has been minimized.

Copy link
@dLobatog

dLobatog Sep 25, 2014

Member

Use class_param instead of p please. Also class_param.update_attribute(:override, true) would do this in one step.

@dLobatog dLobatog changed the title fixes #7608 proposed patch fixes #7608 - Override all puppetclass parameters in one click Sep 25, 2014

@dLobatog

This comment has been minimized.

Copy link
Member

commented Sep 25, 2014

@karmab Thanks. Tests are failing, you need to add a permission to the method in the controller to make it work.

@@ -25,6 +25,7 @@
<td><%= puppetclass.global_class_params_count %> </td>
<td><%= puppetclass.lookup_keys_count %> </td>
<td>
<%= link_to "Override", { :controller => "puppetclasses",:action => "override" , :id => puppetclass.id } , :confirm => _("Override %s?") % puppetclass.name %>

This comment has been minimized.

Copy link
@dLobatog

dLobatog Sep 25, 2014

Member

Override all parameters ? I can't think of any short description...

Maybe some window explaining what this with a "accept/cancel" buttons before actually doing it would be better.

@dLobatog

This comment has been minimized.

Copy link
Member

commented Sep 25, 2014

The button should change to "Use default class params" or something similar when all params are overridden.

@karmab karmab force-pushed the karmab:7608-globaloverride branch from 1333b93 to c6d6e18 Sep 25, 2014

@karmab

This comment has been minimized.

Copy link
Member Author

commented Sep 25, 2014

indicated changes performed ( after a bit of git struggling)

@dLobatog

This comment has been minimized.

Copy link
Member

commented Sep 25, 2014

[test]

@@ -44,6 +44,18 @@ def destroy
end
end

def override
if @puppetclass.class_params.present?

This comment has been minimized.

Copy link
@dLobatog

dLobatog Sep 25, 2014

Member

Some lines need 2 space indentation

This comment has been minimized.

Copy link
@dLobatog

dLobatog Sep 30, 2014

Member

FIXED (no idea why github didn't wrap this off)

@dLobatog

This comment has been minimized.

Copy link
Member

commented Sep 25, 2014

The override button should turn into an "Undo override" when all is set to true. Also "Override" means little if you're not familiar with the whole functionality, maybe "Override all parameters"?

@karmab karmab force-pushed the karmab:7608-globaloverride branch from c6d6e18 to b51d1a4 Sep 25, 2014

@karmab

This comment has been minimized.

Copy link
Member Author

commented Sep 25, 2014

so i aded the undo functionality. not so sure about the name, though "Override all parameters" or "Use default" just take too much space on the puppetclass list page

@@ -5,4 +5,15 @@ def rdoc_classes_path environment, name
klass = name.gsub('::', '/')
"puppet/rdoc/#{environment}/classes/#{klass}.html"
end
def is_overriden puppetclass

This comment has been minimized.

Copy link
@dLobatog

dLobatog Sep 26, 2014

Member

def overridden?(puppetclass) "?" methods are a bit more idiomatic.

if puppetclass.class_params.empty?
return false
end
puppetclass.class_params.each do |param|

This comment has been minimized.

Copy link
@dLobatog

dLobatog Sep 26, 2014

Member

Use puppetclass.class_params.map(&:override).include? true instead, the whole method can be substituted by that, it'll work for classes without params too.

@@ -25,7 +25,9 @@
<td><%= puppetclass.global_class_params_count %> </td>
<td><%= puppetclass.lookup_keys_count %> </td>
<td>
<%= display_delete_if_authorized hash_for_puppetclass_path(:id => puppetclass).merge(:auth_object => puppetclass, :authorizer => authorizer),
<%= link_to "Override", { :controller => "puppetclasses",:action => "override" , :id => puppetclass.name } , :confirm => _("This will set parameters of the class %s as overriden.Accept ?") % puppetclass.name if is_overriden(puppetclass) == false %>

This comment has been minimized.

Copy link
@dLobatog

dLobatog Sep 26, 2014

Member

No need for the == true or == false.

redirect_to puppetclasses_url
end

def unoverride

This comment has been minimized.

Copy link
@dLobatog

dLobatog Sep 26, 2014

Member

Please reuse override for this, you can possibly pass two parameters from the view, one for "true" or "false" depending what you want to do, and a string with "default" or "overridden".

@karmab karmab force-pushed the karmab:7608-globaloverride branch 2 times, most recently from e10f3ae to c8916cd Sep 29, 2014

@karmab

This comment has been minimized.

Copy link
Member Author

commented Sep 30, 2014

anything i should be aware to have this merged ?

@karmab karmab force-pushed the karmab:7608-globaloverride branch from c8916cd to 531bea6 Sep 30, 2014

@@ -5,4 +5,7 @@ def rdoc_classes_path environment, name
klass = name.gsub('::', '/')
"puppet/rdoc/#{environment}/classes/#{klass}.html"
end
def overriden? puppetclass
!puppetclass.class_params.map(&:override).include? false

This comment has been minimized.

Copy link
@dLobatog

dLobatog Sep 30, 2014

Member

puppetclass.class_params.map(&:override).include? true , most people find positive conditions easier to read

This comment has been minimized.

Copy link
@karmab

karmab Sep 30, 2014

Author Member

i think that wouldnt work, what we want to check if some parameters are set to false , thus indicating the class isnt overriden yet

This comment has been minimized.

Copy link
@dLobatog

dLobatog Oct 1, 2014

Member

Oh sorry, you are right, you want to make sure there are no false params.
Try puppetclass.class_params.map(&:override).all? , .all?

@@ -90,4 +103,20 @@ def redirect_back_or_default(default)
session[:redirect_to_url] = nil
end

def find_by_name

This comment has been minimized.

Copy link
@dLobatog

dLobatog Sep 30, 2014

Member

This method was removed in #1246 (merged yesterday). instead we use gem friendly_id to handle this.

You can just add :override to the before_filter :find_resource and it should work.

@karmab karmab force-pushed the karmab:7608-globaloverride branch from 531bea6 to e9da691 Sep 30, 2014

@dLobatog

This comment has been minimized.

Copy link
Member

commented Oct 1, 2014

[test]

@@ -25,7 +25,9 @@
<td><%= puppetclass.global_class_params_count %> </td>
<td><%= puppetclass.lookup_keys_count %> </td>
<td>
<%= display_delete_if_authorized hash_for_puppetclass_path(:id => puppetclass).merge(:auth_object => puppetclass, :authorizer => authorizer),
<%= link_to "Override", { :controller => "puppetclasses",:action => "override" , :message => 'overriden', :enable => true, :id => puppetclass.name } , :confirm => _("This will set parameters of the class %s as overriden.Accept ?") % puppetclass.name if not overriden?(puppetclass) %>

This comment has been minimized.

Copy link
@dLobatog

dLobatog Oct 1, 2014

Member

Don't show this or Defaults if the Puppetclass has no class parameters.
Also, use the helper action_buttons to show a dropdown with buttons, instead of two separate links.

@karmab karmab force-pushed the karmab:7608-globaloverride branch from e9da691 to 4807f4a Oct 1, 2014

@dLobatog

This comment has been minimized.

Copy link
Member

commented Oct 1, 2014

[test]

@@ -5,4 +5,15 @@ def rdoc_classes_path environment, name
klass = name.gsub('::', '/')
"puppet/rdoc/#{environment}/classes/#{klass}.html"
end
def overriden? puppetclass
if puppetclass.class_params.present?

This comment has been minimized.

Copy link
@dLobatog

dLobatog Oct 1, 2014

Member

While I understand why you did this, please just leave the method as

puppetclass.class_params.present? && puppetclass.class_params.map(&:override).all? 

That will return true if all parameters are overridden and false if they are not overridden or they don't exist. Return codes are not commonly used in this code base for this kind of events and method? are expected to return true or false in idiomatic Ruby.

This comment has been minimized.

Copy link
@karmab

karmab Oct 2, 2014

Author Member

actually , i needed different return codes for not overriden or when there are no parameters, since i dont want the override link to show up for classes without parameters

@@ -5,4 +5,15 @@ def rdoc_classes_path environment, name
klass = name.gsub('::', '/')
"puppet/rdoc/#{environment}/classes/#{klass}.html"
end
def overriden? puppetclass

This comment has been minimized.

Copy link
@dLobatog

dLobatog Oct 1, 2014

Member

😄 please fix the typo in the calls from the view.

@karmab karmab force-pushed the karmab:7608-globaloverride branch from 4807f4a to 9971404 Oct 2, 2014

@@ -25,8 +25,15 @@
<td><%= puppetclass.global_class_params_count %> </td>
<td><%= puppetclass.lookup_keys_count %> </td>
<td>
<%= display_delete_if_authorized hash_for_puppetclass_path(:id => puppetclass).merge(:auth_object => puppetclass, :authorizer => authorizer),
:confirm => _("Delete %s?") % puppetclass.name %>
<% if puppetclass.class_params.present? %>

This comment has been minimized.

Copy link
@dLobatog

dLobatog Oct 2, 2014

Member

Use 2 spaces indentation, action_buttons are not indented here and below they're only 1 space indented.

@karmab karmab force-pushed the karmab:7608-globaloverride branch from 9971404 to a887220 Oct 2, 2014

@dLobatog

This comment has been minimized.

Copy link
Member

commented Oct 2, 2014

[test] , thanks for the changes, I'll test in a bit.

@dLobatog

This comment has been minimized.

Copy link
Member

commented Oct 2, 2014

Merged as 61750a4 , thanks @karmab, hope to see you here more often 😄 !

I've squashed your commits into one (git rebase -i HEAD~xx) on merge, we typically don't merge several commits for the same functionality.

@dLobatog dLobatog closed this Oct 2, 2014

@karmab karmab deleted the karmab:7608-globaloverride branch Dec 11, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.