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

[David Lutterkort]

On Mon, 2009-04-20 at 20:26 +0000, Scott Seago wrote:
> This code is based on David Lutterkort's proposal outlined in
> https://www.redhat.com/archives/ovirt-devel/2009-April/msg00086.html
> 
> I've included service-layer methods for show, create, update, destroy,
> vm_action, and cancel_queued_tasks. These are the concrete actions
> that make sense as part of the API. Most of the remaining
> vm_controller methods are pure-WUI actions -- for things such as
> generating the web form for a new VM (the subsequent 'create' action
> _is_ part of the API).

I really like that, and I also like that most controller methods are now
very short, and give you a much better impression what happens to set up
the view.

The one thing that sticks out is the 'delete' method - that should just
repeatedly call svc_destroy (wrapping the deletion in a txn is somewhat
bogus here, the user should be ok with having only part of the list of
VM's deleted)

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.

> diff --git a/src/app/controllers/application.rb b/src/app/controllers/application.rb
> index fa88773..e5f4d4b 100644
> --- a/src/app/controllers/application.rb
> +++ b/src/app/controllers/application.rb

> +  def authorize_view(msg=nil)
> +    authorize_action(Privilege::VIEW,msg)
> +  end

This is unrelated, but it seems odd that we both have authorize_view
here and Pool.can_view - I would argue that the Pool.can_XXX methods
should go away in favor of authorize_XXX and can_XXX methods in the
AppService module (see below)

I like though how authorize_view simplifies the places where we check
for view permissions.

> diff --git a/src/app/controllers/smart_pools_controller.rb b/src/app/controllers/smart_pools_controller.rb
> index f5f3c9d..93c81ee 100644
> --- a/src/app/controllers/smart_pools_controller.rb
> +++ b/src/app/controllers/smart_pools_controller.rb
> @@ -199,19 +199,19 @@ class SmartPoolsController < PoolController
>    def pre_new
>      @pool = SmartPool.new
>      @parent = DirectoryPool.get_or_create_user_root(get_login_user)
> -    @perm_obj = @parent
> +    set_perms(@parent)
>      @current_pool_id=@parent.id
>    end
>    def pre_create
>      @pool = SmartPool.new(params[:smart_pool])
>      @parent = DirectoryPool.get_or_create_user_root(get_login_user)
> -    @perm_obj = @parent
> +    set_perms(@parent)
>      @current_pool_id=@parent.id
>    end
>    def pre_edit
>      @pool = SmartPool.find(params[:id])
>      @parent = @pool.parent
> -    @perm_obj = @pool
> +    set_perms(@pool)
>      @current_pool_id=@pool.id

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 ?

> 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

> 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

> diff --git a/src/app/views/vm/_form.rhtml b/src/app/views/vm/_form.rhtml
> index 58a3d61..610f2bc 100644
> --- a/src/app/views/vm/_form.rhtml
> +++ b/src/app/views/vm/_form.rhtml
> @@ -3,8 +3,7 @@
>  <%  start_resources = @vm.vm_resource_pool.available_resources_for_vm(@vm) %>
>  
>  <!--[form:vm]-->
> -<%= hidden_field 'vm', 'vm_resource_pool_id' unless @vm_resource_pool_name %>
> -<%= hidden_field_tag 'vm_resource_pool_name', @vm_resource_pool_name if @vm_resource_pool_name %>
> +<%= hidden_field 'vm', 'vm_resource_pool_id' %>
>  <%= hidden_field_tag 'hardware_pool_id', @hardware_pool.id if @hardware_pool %>
>  
>      <%= text_field_with_label "Name:", "vm", "description", {:style=>"width:250px;"}  %>

Could this go into a separate commit, too ?

David