[Ovirt-devel] [PATCH server 3/3] * app/controllers/host_controller.rb: factor biz logic out

[Scott Seago]

David Lutterkort wrote:
> The business logic is now in HostService.
>
> The main functional difference is that svc_list filters the host list by
> permission.
>
> There's a couple things I am not clear on:
>
> (1) addhost should use PoolService somehow, need to look at Scott's
>     latest patch
>
>   
I'm not sure it should use the service -- this is one of these wui-only 
form setup bits that I've been ignoring for now. I'm not sure how this 
fits into the service model. As a similar example, "new" ignores the 
services, since it just generates a web form, but "create" uses the 
service for the actual create operation.

> (2) the way we handle unsupported actions is strange: we define them and
>     kick out a nice error - in reality, people can only get there either if
>     we have a bug somewhere or if they did URL surgery; either way, they
>     deserve a gory Rails error report. We should therefore remove the
>     actions and associated nice error messages
> ---
>  src/app/controllers/host_controller.rb |  122 +++++++++++++------------------
>  src/app/services/host_service.rb       |  119 +++++++++++++++++++++++++++++++
>  2 files changed, 170 insertions(+), 71 deletions(-)
>  create mode 100644 src/app/services/host_service.rb
>
> diff --git a/src/app/controllers/host_controller.rb b/src/app/controllers/host_controller.rb
> index 994b5e2..999a104 100644
> --- a/src/app/controllers/host_controller.rb
> +++ b/src/app/controllers/host_controller.rb
>  
> @@ -73,15 +61,15 @@ class HostController < ApplicationController
>    end
>  
>    def addhost
> -    @hardware_pool = Pool.find(params[:hardware_pool_id])
> -    render :layout => 'popup'
> -  end
> -
> -  def pre_addhost
> +    # FIXME: This probably should go into PoolService.svc_modify
>   
This only generates the form for picking hosts to add to a pool. I 
originally put it in the host controller since the bulk of the form is a 
grid of hosts. However we're doing the same thing in 
hardware/show_hosts.rhtml, so we could definitely move this over to the 
pool controller if it makes sense.

> +    # Old pre_addhost
>      @pool = Pool.find(params[:hardware_pool_id])
>      @parent = @pool.parent
>      set_perms(@pool)
>      authorize_admin
> +    # Old addhost
> +    @hardware_pool = Pool.find(params[:hardware_pool_id])
> +    render :layout => 'popup'
>    end
>  
>    def add_to_smart_pool
> @@ -116,53 +104,51 @@ class HostController < ApplicationController
>  
>    def edit_network
> -    render :layout => 'popup'
> +    handle_auth_error do
> +      svc_modify(params[:id])
> +      render :layout => 'popup'
> +    end
>    end
>   
I'm not sure you want svc_modify here -- this action just generates a 
web form -- it doesn't actually modify anything here. But you'll need to 
do the auth somewhere (such as inline, unless we come up with another 
convention for auth for generating views)
>  
>    def bondings_json
> -    bondings = Host.find(params[:id]).bondings
> -    render :json => bondings.collect{ |x| {:id => x.id, :name => x.name} }
> +    handle_auth_error do
> +      svc_show(params[:id])
> +      bondings = @host.bondings
> +      render :json => bondings.collect{ |x| {:id => x.id, :name => x.name} }
> +    end
>    end
>   
BTW I haven't gone through and used svc_show for the various XXX_json 
methods in the other controllers, although in some cases it would make 
sense  to do so. The pool stuff is big enough that those changes should 
probably be done separately.
>  
>    private
> @@ -180,12 +166,6 @@ class HostController < ApplicationController
>      flash[:notice] = 'Hosts may not be edited via the web UI'
>      redirect_to :action=> 'show', :id => @host
>    end
> -  def pre_action
> -    @host = Host.find(params[:id])
> -    set_perms(@host.hardware_pool)
> -    @json_hash = { :object => :host }
> -    authorize_admin
> -  end
>    def pre_show
>      @host = Host.find(params[:id])
>      set_perms(@host.hardware_pool)
> diff --git a/src/app/services/host_service.rb b/src/app/services/host_service.rb
> new file mode 100644
> index 0000000..074d830
> --- /dev/null
> +++ b/src/app/services/host_service.rb
> @@ -0,0 +1,119 @@
> +#--
> +# Copyright (C) 2009 Red Hat, Inc.
> +# Written by David Lutterkort <lutter at redhat.com>
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; version 2 of the License.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
> +# MA  02110-1301, USA.  A copy of the GNU General Public License is
> +# also available at http://www.gnu.org/copyleft/gpl.html.
> +#++
> +# Business logic around hosts aka nodes
> +module HostService
> +
> +  include ApplicationService
> +
> +  # Host attributes on which we filter with '='
> +  EQ_ATTRIBUTES = [ :state, :arch, :hostname, :uuid,
> +                    :hardware_pool_id ]
> +
> +  # List hosts matching criteria described by +params+; only the entries
> +  # for keys in +EQ_ATTRIBUTES+ are used
> +  #
> +  # === Instance variables
> +  # [<tt>@hosts</tt>] stores list of hosts matching criteria
> +  # === Required permissions
> +  # [<tt>Privilege::VIEW</tt>] no exception raised, <tt>@hosts</tt>
> +  #                            is filtered by privilege
> +  def svc_list(params)
> +    conditions = []
> +    EQ_ATTRIBUTES.each do |attr|
> +      if params[attr]
> +        conditions << "hosts.#{attr} = :#{attr}"
> +      end
> +    end
> +    # Add permission check
> +    params = params.dup
> +    params[:user] = get_login_user
> +    params[:priv] = Privilege::VIEW
> +    conditions << "privileges.name=:priv"
> +    conditions << "permissions.uid=:user"
> +    incl = [{ :hardware_pool => { :permissions => { :role => :privileges}}}]
> +    @hosts = Host.find(:all,
> +                       :include => incl,
> +                       :conditions => [conditions.join(" and "), params],
> +                       :order => "hosts.id")
> +  end
> +
> +  # Load the Host with +id+ for viewing
> +  #
> +  # === Instance variables
> +  # [<tt>@host</tt>] stores the Host with +id+
> +  # === Required permissions
> +  # [<tt>Privilege::VIEW</tt>] on host's HardwarePool
> +  def svc_show(id)
> +    lookup(id, Privilege::VIEW)
> +  end
> +
> +  # Load the Host with +id+ for editing
> +  #
> +  # === Instance variables
> +  # [<tt>@host</tt>] stores the Host with +id+
> +  # === Required permissions
> +  # [<tt>Privilege::MODIFY</tt>] on host's HardwarePool
> +  def svc_modify(id)
> +    lookup(id, Privilege::MODIFY)
> +  end
> +
>   
OK, I think I understand what you're doing here -- I may have 
misunderstood svc_modify in my comments above -- is this meant as a 
distinct action from svc_update -- which we're using elsewhere to 
actually make changes? i.e. it's effectively svc_show with modify 
permissions enforcing? Does this belong in the svc layer? This seems 
more WUI-specific, as for the api/qmf case I'm not sure we'd be using 
this action. Looking below this is used for multiple modify actions, so 
perhaps it does belong here -- it just might not be exposed as a 
QMF-level API call -- am I understading this correctly now?



Scott