Michal Fojtik wrote:
> On 24/03/10 14:58 -0400, Mohammed Morsi wrote:
>
> Hi,
>
> Just few notes:
>
>
Michal,
I'm actually in the process of doing this in a different way that's a
bit more generic, so this patch won't be pushed as-is -- but I've got a
couple responses to your comments here.
>> + def new_user
>> + require_privilege(Privilege::POOL_MODIFY)
>> + require_privilege(Privilege::USER_MODIFY)
>>
>
> Why you can't just simply pass a Array as an argument to this method ?
> Something like require_privileges([1, 2]).
>
>
Actually I think in this case what we really want is SET_PERMS. However
making this method accept an array would be useful if we find any other
cases where we want to verify two different privileges on a single
object -- I'm not sure we have any current need for that though.
>> +
>> + @users = User.find(:all)
>> + @roles = Role.find(:all, :conditions => [ 'scope = ?', 'PortalPool' ])
>> + @pool = PortalPool.find(params[:id])
>> + end
>> +
>> + def create_user
>> + require_privilege(Privilege::POOL_MODIFY)
>> + require_privilege(Privilege::USER_MODIFY)
>>
>
> In other hands, why not create a 'before_filter' for this ?
>
> before_filter :require_privileges
>
>
We did perm checks with before filters in ovirt, but the end result was
somewhat messy. Normally require_privilege takes an object as a second
argument -- for which object we're requiring the privilege on (in fact
for this patch @pool should be passed in here). So when we did
permission checks in before_filters we had to have additional
before_filters to do whatever is required to determine what object we're
using for permissions checks. In several cases this resulted with an
action with a completely empty body and a several-line before_filter,
which made the controller flow rather difficult to follow.
>> @@ -40,3 +40,4 @@
>> <%= link_to "Hardware Profiles", {:action => "hardware_profiles", :id =>
>> @pool.id}, :class=>"actionlink"%>
>>
>
> This could be: <%= link_to "Hardware Profiles",
> hardware_profiles_path(@pool), :class => "actionlink"%>
>
>
I prefer the former link, as it's easier to read -- clearly identifying
action, object (id), and (when appropriate) controller. But it's more of
a personal preference -- no big deal either way.
> (btw. @pool == hardware_profile ?)
>
>
Pools and HW profiles are different object types. In this case the
hardware_profiles action on the pool controller displays a list of
hardware profiles associated with the selected pool.
> - Michal
>
>
Scott
_______________________________________________
deltacloud-devel mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/deltacloud-devel
