[Ovirt-devel] [PATCH server] Revised attempt at refactoring the vm_controller using the service layer.

[Scott Seago]

Scott Seago wrote:
> David Lutterkort wrote:
>
>> I've given this some light testing, and as far as I can tell everything
>> works well; so ACK from me. The comments below are minor nits and some
>> ideas on further refactoring that should happen separately.
>>
I've send an updated patch which addresses a couple of the below issues.

>>
>> This code is all over the place, i.e.
>>         @current_pool_id = some_pool.id
>>         set_perms(some_pool)
>>
>> Why not pull setting @current_pool_id into set_perms ?
>>
>>   
> I was hoping to do that too -- the main problem I ran into was it 
> wasn't _always_ the same ID. But perhaps I could make an optional 
> param for when it differs. Yes, I think I can do that. The most 
> obvious place it differs is for new pool actions -- the current (shown 
> in nav) pool is different from the new one being created. But since 
> that's more the exception I can probably remove 90% of that problem.
OK I added  the following line to the set_perms method:
  @current_pool_id ||= perm_obj.id

So now I'm only explicitly setting @current_pool_id when it differs from 
perm_obj -- and there are only two instances of that. The first is for 
the 'edit VM pool' page since the ability to edit a VM pool is tied to 
the parent pool rather than the current pool (so individual users can't 
edit their own quotas). The second instance is for the migrate action in 
vm_controller, since migration permission depends on HW pool permissions 
not current VM pool permissions.

So the net effect here is I was able to remove 17 of the 19 instances 
where we were setting @current_pool_id manually.
>>> diff --git a/src/app/models/vm.rb b/src/app/models/vm.rb
>>> index c62595a..4c7925c 100644
>>> --- a/src/app/models/vm.rb
>>> +++ b/src/app/models/vm.rb
>>> @@ -220,6 +220,7 @@ class Vm < ActiveRecord::Base
>>>        self[:boot_device]= BOOT_DEV_HD
>>>        self[:provisioning]= nil
>>>      else
>>> +      # FIXME: Should this case trigger an error instead?
>>>        self[:boot_device]= BOOT_DEV_NETWORK
>>>        self[:provisioning]= settings
>>>      end
>>>     
>>
>> Should probably be a separate patch
>>
>>   
> Actually I'll probably just remove it. Initially it looked like that 
> would be an issue for the refactoring of the cobbler bits, but once I 
> discovered that I could do everything in the provisioning method with 
> already-existing Vm methods rather than the somewhat clumsy actions on 
> the hash this particular bit above isn't really relevant to what I'm 
> doing here.
OK I've removed this.
>>> diff --git a/src/app/services/application_service.rb 
>>> b/src/app/services/application_service.rb
>>> new file mode 100644
>>> index 0000000..e2d0d38
>>> --- /dev/null
>>> +++ b/src/app/services/application_service.rb
>>>     
>>
>>  
>>> +module ApplicationService
>>> +  class PermissionError < RuntimeError; end
>>> +  class ActionError < RuntimeError; end
>>> +
>>> +  # Including class must provide a GET_LOGIN_USER
>>> +
>>> +  def set_perms(perm_obj)
>>> +    @perm_obj = perm_obj
>>> +    @user = get_login_user
>>> +    @can_view = @perm_obj.can_view(@user)
>>> +    @can_control_vms = @perm_obj.can_control_vms(@user)
>>> +    @can_modify = @perm_obj.can_modify(@user)
>>> +    @can_view_perms = @perm_obj.can_view_perms(@user)
>>> +    @can_set_perms = @perm_obj.can_set_perms(@user)
>>> +  end
>>>     
>>
>> As we talked about on IRC, longer term it would be better if the
>> @can_XXX attributes became methods on the service.
>>
>> I also think the can_XXX methods on pool should be removed and we should
>> have something like the following in AppService:
>>
>>         def user
>>           @user ||= get_login_user
>>         end
>>                 def has_privilege(priv, perm_obj = nil)
>>           perm_obj ||= @perm_obj
>>           perm_obj.has_privilege(user, priv)
>>         end
>>                   def can_view(perm_obj = nil)
>>           has_privilege(Privilege::VIEW, perm_obj)
>>         end
>>
>>   
> How will this work from the Views? Right now the only place we access 
> @can_XXX is in the views, but generally views only have access to 
> controller instance variables, not methods..
> But certianly the can_XXX methods on pool could go away.
Hmm. It also looks like we're using the pool.can_XXX methods in some 
places in the  views currently -- for example we only show what host a 
VM is running on if the hw_pool.can_view(@user) is true. So that 
combined with the issues in getting at the controller methods from the 
views would suggest that we leave this bit alone for the moment.

Scott