-
Notifications
You must be signed in to change notification settings - Fork 985
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 #1646, #3103 - enable cloning for templates, and add read-only options #1559
Conversation
@@ -41,6 +42,12 @@ def to_param | |||
"#{id}-#{name.parameterize}" | |||
end | |||
|
|||
def clone | |||
template = self.dup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use deep_cloneable to clone the relationships with operating systems at least? https://github.com/theforeman/foreman/blob/develop/app/models/host/managed.rb#L645 is an example
when 'destroy' | ||
'destroy' | ||
when 'index', 'show', 'clone' | ||
'view' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't clone require the 'create' permission (even though i realize its not doing much but redirecting to new)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Host and Host group both use 'view' for clone. I assume because the permission is evaluated on the original object, and you only need to view that one.
Although it is confusing to show a clone link if the user ultimately doesn't have create rights.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, then i'm okay with that if the foreman-ers are okay with it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's correct, though the link should only be visible if the user has the create_templates permission (due to access_permissions.rb).
When i go to clone a template, the org and location list is empty. I would expect it to have whatever org and location I'm in similar to when i create a new template. |
@jlsherrill What should the behavior be? At the moment, I clone the org and locs from the previous template. I guess I should take only the current taxonomies, if set? |
@stbenjam i could see an argument for either/both. a) Since its a clone you'd expect everything I don't change to be the same (the way you've implemented it). For the katello usecase, katello can override the ConfigTemplate#clone method if need be. After thinking about it, i'd probably lean towards a) However I'm not sure about situation b). |
allowed_changes << _("locations") if show_location_tab? | ||
allowed_changes << _("organizations") if show_organization_tab? %> | ||
<%= alert :class => 'alert-warning', :header => '', | ||
:text => icon_text("warning-sign", (_('Note: This template is read only. You may only change the %s. Please %s it to customize.') % |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use hash syntax when multiple variables are interpolated: http://projects.theforeman.org/projects/foreman/wiki/Translating#Translating-for-developers
Two nitpicks, "read only" should be hyphenated, one space after a period for consistency.
Integration tests are failing, I'd guess you have a SQL query error occurring which is invalidating the txn. Could you also add a functional test for the clone route? |
@config_template = ConfigTemplate.new | ||
end | ||
|
||
def clone | ||
@clone = @config_template.clone | ||
@config_template = @clone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@clone
seems unnecessary here, @config_template = @config_template.clone
would work too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took this verbatim from the existing clone methods in other models. I assumed they did this for a reason, but you're right that should work.
Overall +1, I think marking our default templates as read-only is fine (we can disable editing, but still perhaps allow complete deletion if the user really doesn't care?) I'd suggest a boolean flag for distributor templates vs user templates, and then the distributor ones could be shown differently in the index, perhaps a separate tab or graying out of lines? Clicking on 5 templates trying to find one you can edit isn't helpful... |
@GregSutcliffe A tab looks like it won't be too simple to implement, maybe a column? A user can also search "editable = True" if we enable the field for search. |
how about a generic lock option? (make the unlock a separate process?) |
@ohadlevy I hadn't planned to make them user-unlockable, the reasoning is to support default templates that can be added to multiple orgs. We don't want a user in Org A to be able to edit "Kickstart default RHEL" and affect a user in Org B. They'll have to clone to customize. But we want both Org A and Org B to see all the default templates. Then when a user creates Org C, Katello will assign all the templates that have this 'distributor' flag that @GregSutcliffe mentioned to that org too. I also didn't plan to change Foreman's way of doing things, because locking the default templates breaks users who have edited them already, and people who use the community-templates plug-in. Do you still want the ability to lock/unlock them in the UI by an admin? I think we'd end up having to remove that in Katello, we want to be able to always control the default templates. |
@@ -24,6 +34,7 @@ def create | |||
end | |||
|
|||
def edit | |||
load_vars_for_ajax |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. what is the reason why need this ? AFAIR we dont have any ajax in the template editor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we load the new form, the associations aren't saved in the database so we have to store them in variables like @OperatingSystems, @oraganizations, @Locations. It's not ajax here, but it's how it's named for the similar feature in other models.
@@ -3,7 +3,7 @@ | |||
gem 'uglifier', '>= 1.0.3' | |||
gem 'execjs', '< 2.1.0' | |||
gem "jquery-rails", "2.0.3" | |||
gem 'jquery-ui-rails' | |||
gem 'jquery-ui-rails', '< 5.0.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just out of curiosity, why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of #1564 ,which is fixed now. I'll remove this here.
@stbenjam i'm concerned about minimizing where katello diverges from core, where possible. I don't think we consume templates that differently, so I'd rather design a good workflow now, than end up in another mess later. I think templates need to be unlockable, but with a big disclaimer about how that means the user won't get updates to that template in future releases. Give power to the user, don't lock them in. At upgrade time, you can then update Templates.where(matching name && locked). Would that be sufficient for Katello too? re: my earlier point, I'm less concerned about how it looks, whatever works for you, so long as we can visually tell on the index whats editable and what isn't. |
Ok just pushed the latest iteration, taking @ohadlevy's locked suggestion. I like the usage workflow better. @GregSutcliffe , I added an icon to indicate locked -- what do you think? We need different icons but just an example. Maybe one for unlocked too? By default, only an admin can lock/unlock templates. I like this because then a company can provide all the organizations a set of "standard" templates for them to use, not just the ones provided by us. |
@@ -18,13 +18,16 @@ | |||
</tr> | |||
<% for config_template in @config_templates %> | |||
<tr> | |||
<td><%= link_to_if_authorized h(config_template), hash_for_edit_config_template_path(:id => config_template.to_param).merge(:auth_object => config_template, :authorizer => authorizer, :permission => 'edit_templates') %></td> | |||
<td> <%= image_tag("locked.png") if config_template.locked? %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the lock icon we already have --> http://getbootstrap.com/components/
@stbenjam Looks good, will have to do a proper code reviwe soonish. I agree with your reasons for admin only, but there's no reason not to have a permission admins can grant if they wish - just don't enable it on any roles by default ;) No need for an unlocked icon, that will clutter and make it hard to differentiate at speed. |
@GregSutcliffe @ohadlevy @elobato Cool, thanks for all the feedback so far. I think this is better than the first iteration. |
validates :name, :presence => true, :uniqueness => true | ||
validates :name, :template, :presence => true | ||
validates :template_kind_id, :presence => true, :unless => Proc.new {|t| t.snippet } | ||
validate :changes_when_template_is_locked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validate :changes, :if => lambda { |template| template.locked? || template.locked_changed? }
And remove the early return on changes_when_template_is_locked
.
No description provided.