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 #7121 - move smart proxy action buttons to helper #1695

Closed
wants to merge 1 commit into from

Conversation

stbenjam
Copy link
Member

No description provided.


if puppetca
display_link_if_authorized(_("Autosign"), hash_for_smart_proxy_autosign_index_path(:smart_proxy_id => proxy).
merge(:auth_object => proxy, :permission => 'view_smart_proxies_autosign', :authorizer => authorizer))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why can't this link be added in the above if puppetca?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more or less cut and paste from the existing code :-) It just makes it easier for plug-ins to modify if it's in a helper method, it also makes the template look nicer.

There's a technical reason for the two if's: they will nil if the user isn't authorized, so it would result in for example:
['Certificates', nil] which action_buttons will compact.

It will be more complicated in one if block.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I guess it will work in one if, if you want I'll move it, but the goal wasn't to change the code here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure if there was a technical reason I was missing.
If it's the same I would prefer it was in one if for clarity

@stbenjam
Copy link
Member Author

@orrabin Fixed, uses 1 if statement.

@@ -0,0 +1,19 @@
module SmartProxiesHelper
def proxy_actions proxy, authorizer
[if proxy.features.detect{ |feature| feature.name == "Puppet CA" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation's two spaces too deep in this method

@stbenjam
Copy link
Member Author

@domcleal Fixed the indentation.

Are you all planning for rubocop in Foreman? I also reduced the number of problems except for line lengths, methods like hash_for_smart_proxy_autosign_index_path make it difficult.

@domcleal
Copy link
Contributor

I wasn't worried about the rest of the style really, it was just the indentation. Unfortunately your changes have broken the syntax on 1.8.

@stbenjam
Copy link
Member Author

Sorry, fixed.

@domcleal
Copy link
Contributor

Merged as da24313 for 1.6.0-RC2, thanks @stbenjam.

@domcleal domcleal closed this Aug 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants