Ovirt devel - Apr 2009 - [PATCH server] controller service layer refactoring for pools.

This is the second round of controller/service layer refactoring changes -- in
this case for the pool-related controllers (HW, VM, and Smart pools)

Signed-off-by: Scott Seago <sseago at redhat.com>
---
 src/app/controllers/application.rb               |   26 ++--
 src/app/controllers/hardware_controller.rb       |  210 ++++------------------
 src/app/controllers/pool_controller.rb           |  130 ++++++++++++--
 src/app/controllers/resources_controller.rb      |  122 ++++---------
 src/app/controllers/smart_pools_controller.rb    |  120 +++++-------
 src/app/controllers/storage_volume_controller.rb |   39 +++--
 src/app/models/hardware_pool.rb                  |   48 -----
 src/app/models/host.rb                           |    4 +
 src/app/models/pool.rb                           |    3 +
 src/app/models/smart_pool.rb                     |   19 --
 src/app/models/storage_pool.rb                   |    4 +
 src/app/models/vm.rb                             |    6 +
 src/app/models/vm_resource_pool.rb               |    4 +
 src/app/models/vm_task.rb                        |    8 +-
 src/app/services/application_service.rb          |   28 +++-
 src/app/services/hardware_pool_service.rb        |  125 +++++++++++++
 src/app/services/pool_service.rb                 |   60 ++++++
 src/app/services/smart_pool_service.rb           |   79 ++++++++
 src/app/services/vm_resource_pool_service.rb     |   76 ++++++++
 src/app/views/resources/quick_summary.rhtml      |    2 +-
 src/app/views/resources/vm_actions.rhtml         |    6 +-
 src/app/views/smart_pools/_form.rhtml            |    2 +-
 22 files changed, 671 insertions(+), 450 deletions(-)
 create mode 100644 src/app/services/hardware_pool_service.rb
 create mode 100644 src/app/services/pool_service.rb
 create mode 100644 src/app/services/smart_pool_service.rb
 create mode 100644 src/app/services/vm_resource_pool_service.rb

diff --git a/src/app/controllers/application.rb
b/src/app/controllers/application.rb
index e5f4d4b..289d950 100644
--- a/src/app/controllers/application.rb
+++ b/src/app/controllers/application.rb
@@ -118,17 +118,7 @@ class ApplicationController < ActionController::Base
   def handle_auth_error(msg)
     respond_to do |format|
       format.html do
-        @title = "Access denied"
-        @errmsg = msg
-        @ajax = params[:ajax]
-        @nolayout = params[:nolayout]
-        if @ajax
-          render :template => 'layouts/popup-error', :layout =>
'tabs-and-content'
-        elsif @nolayout
-          render :template => 'layouts/popup-error', :layout =>
'help-and-content'
-        else
-          render :template => 'layouts/popup-error', :layout =>
'popup'
-        end
+        html_error_page(msg)
       end
       format.json do
         @json_hash ||= {}
@@ -139,7 +129,19 @@ class ApplicationController < ActionController::Base
       format.xml { head :forbidden }
     end
   end
-
+  def html_error_page(msg)
+    @title = "Access denied"
+    @errmsg = msg
+    @ajax = params[:ajax]
+    @nolayout = params[:nolayout]
+    if @ajax
+      render :template => 'layouts/popup-error', :layout =>
'tabs-and-content'
+    elsif @nolayout
+      render :template => 'layouts/popup-error', :layout =>
'help-and-content'
+    else
+      render :template => 'layouts/popup-error', :layout =>
'popup'
+    end
+  end
   # don't define find_opts for array inputs
   def json_hash(full_items, attributes, arg_list=[], find_opts={},
id_method=:id)
     page = params[:page].to_i
diff --git a/src/app/controllers/hardware_controller.rb
b/src/app/controllers/hardware_controller.rb
index 2158e08..8261cd7 100644
--- a/src/app/controllers/hardware_controller.rb
+++ b/src/app/controllers/hardware_controller.rb
@@ -17,8 +17,10 @@
 # MA  02110-1301, USA.  A copy of the GNU General Public License is
 # also available at http://www.gnu.org/copyleft/gpl.html.
 #
+require 'services/hardware_pool_service'
 
 class HardwareController < PoolController
+  include HardwarePoolService
 
   EQ_ATTRIBUTES = [ :name, :parent_id ]
 
@@ -27,10 +29,7 @@ class HardwareController < PoolController
   verify :method => [:post, :delete], :only => :destroy,
          :redirect_to => { :action => :list }
 
-  before_filter :pre_modify, :only => [:add_hosts, :move_hosts,
-                                       :add_storage, :move_storage,
-                                       :create_storage, :delete_storage,
-                                       :move, :removestorage]
+  before_filter :pre_modify, :only => [:move, :removestorage]
 
   def index
     if params[:path]
@@ -113,8 +112,13 @@ class HardwareController < PoolController
   end
 
   def show_storage
-    @storage_tree = @pool.storage_tree(:filter_unavailable => false,
:include_used => true).to_json
-    show
+    begin
+      svc_show(params[:id])
+      @storage_tree = @pool.storage_tree(:filter_unavailable => false,
:include_used => true).to_json
+      render_show
+    rescue PermissionError => perm_error
+      handle_auth_error(perm_error.message)
+    end
   end
 
   def show_tasks
@@ -124,12 +128,12 @@ class HardwareController < PoolController
 
   def hosts_json
     if params[:exclude_host]
-      pre_show
+      pre_show_pool
       hosts = @pool.hosts
       find_opts = {:conditions => ["id != ?",
params[:exclude_host]]}
       include_pool = false
     elsif params[:id]
-      pre_show
+      pre_show_pool
       hosts = @pool.hosts
       find_opts = {}
       include_pool = false
@@ -154,7 +158,7 @@ class HardwareController < PoolController
 
   def storage_pools_json
     if params[:id]
-      pre_show
+      pre_show_pool
       storage_pools = @pool.storage_pools
       find_opts = {:conditions => "type !=
'LvmStoragePool'"}
       include_pool = false
@@ -190,144 +194,49 @@ class HardwareController < PoolController
     super
   end
 
-  def create
-    resource_type = params[:resource_type]
-    resource_ids_str = params[:resource_ids]
-    resource_ids = []
-    resource_ids = resource_ids_str.split(",").collect {|x| x.to_i}
if resource_ids_str
-    begin
-      @pool.create_with_resources(@parent, resource_type, resource_ids)
-      respond_to do |format|
-        format.html {
-          reply = { :object => "pool", :success => true,
-            :alert => "Hardware Pool was successfully created." }
-          reply[:resource_type] = resource_type if resource_type
-          render :json => reply
-        }
-        format.xml {
-          render :xml => @pool.to_xml(XML_OPTS),
-          :status => :created,
-          :location => hardware_pool_url(@pool)
-        }
-      end
-    rescue
-      respond_to do |format|
-        format.json {
-          render :json => { :object => "pool", :success =>
false,
-            :errors => @pool.errors.localize_error_messages.to_a  }
-        }
-        format.xml  { render :xml => @pool.errors,
-          :status => :unprocessable_entity }
-      end
-    end
-  end
-
-  def update
-    if params[:hardware_pool]
-      # FIXME: For the REST API, we allow moving hosts/storage through
-      # update.  It makes that operation convenient for clients, though makes
-      # the implementation here somewhat ugly.
-      [:hosts, :storage_pools].each do |k|
-        objs = params[:hardware_pool].delete(k)
-        ids = objs.reject{ |obj| obj[:hardware_pool_id] == @pool.id}.
-          collect{ |obj| obj[:id] }
-        if ids.size > 0
-          # FIXME: use self.move_hosts/self.move_storage
-          if k == :hosts
-            @pool.move_hosts(ids, @pool.id)
-          else
-            @pool.move_storage(ids, @pool.id)
-          end
-        end
-      end
-      # FIXME: HTML views should use :hardware_pool
-      params[:pool] = params.delete(:hardware_pool)
-    end
-
-    begin
-      @pool.update_attributes!(params[:pool])
-      respond_to do |format|
-        format.json {
-          render :json => { :object => "pool", :success =>
true,
-            :alert => "Hardware Pool was successfully modified." }
-        }
-        format.xml {
-          render :xml => @pool.to_xml(XML_OPTS),
-          :status => :created,
-          :location => hardware_pool_url(@pool)
-        }
-      end
-    rescue
-      respond_to do |format|
-        format.json {
-          render :json => { :object => "pool", :success =>
false,
-            :errors => @pool.errors.localize_error_messages.to_a}
-        }
-        format.xml {
-          render :xml => @pool.errors,
-          :status => :unprocessable_entity
-        }
-      end
-    end
+  def additional_create_params
+    {:resource_type => params[:resource_type],
+      :resource_ids => params[:resource_ids],
+      :parent_id => (params[:hardware_pool] ?
+                     params[:hardware_pool][:parent_id] :
+                     params[:parent_id])}
   end
 
   def add_hosts
-    edit_items(Host, :move_hosts, @pool.id, :add)
+    edit_items(Host, @pool.id, :add)
   end
 
   def move_hosts
-    edit_items(Host, :move_hosts, params[:target_pool_id], :move)
+    edit_items(Host, params[:target_pool_id], :move)
   end
 
   def add_storage
-    edit_items(StoragePool, :move_storage, @pool.id, :add)
+    edit_items(StoragePool, @pool.id, :add)
   end
 
   def move_storage
-    edit_items(StoragePool, :move_storage, params[:target_pool_id], :move)
+    edit_items(StoragePool, params[:target_pool_id], :move)
   end
 
-  #FIXME: we need permissions checks. user must have permission on src pool
-  # in addition to the current pool (which is checked). We also need to fail
-  # for storage that aren't currently empty
-  def edit_items(item_class, item_method, target_pool_id, item_action)
-    resource_ids_str = params[:resource_ids]
-    resource_ids = resource_ids_str.split(",").collect {|x| x.to_i}
-
-    # if user doesn't have modify permission on both source and destination
-    unless @pool.can_modify(@user) and
Pool.find(target_pool_id).can_modify(@user)
-        render :json => { :success => false,
-               :alert => "Cannot #{item_action.to_s}
#{item_class.table_name.humanize} without admin permissions on both pools"
}
-        return
-    end
-
-    # relay error message if movable check fails for any resource
-    success = true
-    failed_resources = ""
-    resource_ids.each {|x|
-       unless item_class.find(x).movable?
-         success = false
-         failed_resources += x.to_s + " "
-       end
-    }
-    resource_ids.delete_if { |x| ! item_class.find(x).movable? }
-
+  def edit_items(item_class, target_pool_id, item_action)
     begin
-      @pool.transaction do
-        @pool.send(item_method, resource_ids, target_pool_id)
+      if item_class == Host
+        alert = svc_move_hosts(params[:id],
params[:resource_ids].split(","), target_pool_id)
+      elsif item_class == StoragePool
+        alert = svc_move_storage(params[:id],
params[:resource_ids].split(","), target_pool_id)
+      else
+        raise "invalid class #{item_class}"
       end
-    rescue
-      success = false
-    end
-
-    if success
-      render :json => { :success => true,
-        :alert => "#{item_action.to_s}
#{item_class.table_name.humanize} successful.",
+      render :json => { :success => true, :alert => alert,
         :storage => @pool.storage_tree({:filter_unavailable => false,
:include_used => true, :state => item_action.to_s})}
-    else
-      render :json => { :success => false,
-         :alert => "#{item_action.to_s}
#{item_class.table_name.humanize} failed" +
-                   (failed_resources == "" ? "." : "
for " + failed_resources) }
+    rescue PermissionError => perm_error
+      handle_auth_error(perm_error.message)
+      # If we need to give more details as to which hosts/storage succeeded,
+      # they're in the exception
+    rescue PartialSuccessError => error
+      render :json => { :success => false, :alert => error.message }
+    rescue Exception => ex
+      render :json => { :success => false, :alert => error.message }
     end
   end
 
@@ -335,33 +244,6 @@ class HardwareController < PoolController
     render :layout => 'popup'
   end
 
-  def destroy
-    parent = @pool.parent
-    if not(parent)
-      alert="You can't delete the top level Hardware pool."
-      success=false
-      status=:method_not_allowed
-    elsif not(@pool.children.empty?)
-      alert = "You can't delete a Pool without first deleting its
children."
-      success=false
-      status=:conflict
-    else
-      if @pool.move_contents_and_destroy
-        alert="Hardware Pool was successfully deleted."
-        success=true
-        status=:ok
-      else
-        alert="Failed to delete hardware pool."
-        success=false
-        status=:internal_server_error
-      end
-    end
-    respond_to do |format|
-      format.json { render :json => { :object => "pool",
:success => success,
-                                      :alert => alert } }
-      format.xml { head status }
-    end
-   end
 
   protected
   #filter methods
@@ -369,25 +251,11 @@ class HardwareController < PoolController
     @pool = HardwarePool.new
     super
   end
-  def pre_create
-    # FIXME: REST and browsers send params differently. Should be fixed
-    # in the views
-    if params[:pool]
-      @pool = HardwarePool.new(params[:pool])
-    else
-      @pool = HardwarePool.new(params[:hardware_pool])
-    end
-    super
-  end
   def pre_edit
     @pool = HardwarePool.find(params[:id])
     @parent = @pool.parent
     set_perms(@pool)
   end
-  def pre_show
-    @pool = HardwarePool.find(params[:id])
-    super
-  end
   def pre_modify
     pre_edit
     authorize_admin
diff --git a/src/app/controllers/pool_controller.rb
b/src/app/controllers/pool_controller.rb
index 06f8768..41c1be9 100644
--- a/src/app/controllers/pool_controller.rb
+++ b/src/app/controllers/pool_controller.rb
@@ -20,12 +20,9 @@
 
 class PoolController < ApplicationController
 
-  before_filter :pre_show_pool, :only => [:show_vms, :show_users,
-                                          :show_hosts, :show_storage,
-                                          :users_json, :show_tasks, :tasks,
+  before_filter :pre_show_pool, :only => [:users_json, :show_tasks, :tasks,
                                           :vm_pools_json,
-                                          :pools_json, :show_pools,
-                                          :storage_volumes_json,
:quick_summary]
+                                          :pools_json, :storage_volumes_json]
 
   XML_OPTS  = {
     :include => [ :storage_pools, :hosts, :quota ]
@@ -40,6 +37,15 @@ class PoolController < ApplicationController
   end
 
   def show
+    begin
+      svc_show(params[:id])
+      render_show
+    rescue PermissionError => perm_error
+      handle_auth_error(perm_error.message)
+    end
+  end
+
+  def render_show
     respond_to do |format|
       format.html {
         render :layout => 'tabs-and-content' if params[:ajax]
@@ -49,10 +55,15 @@ class PoolController < ApplicationController
         render :xml => @pool.to_xml(XML_OPTS)
       }
     end
-  end
 
+  end
   def quick_summary
-    render :layout => 'selection'
+    begin
+      svc_show(params[:id])
+      render :layout => 'selection'
+    rescue PermissionError => perm_error
+      handle_auth_error(perm_error.message)
+    end
   end
 
   # resource's users list page
@@ -97,30 +108,111 @@ class PoolController < ApplicationController
     render :layout => 'popup'
   end
 
+  def create
+    # FIXME: REST and browsers send params differently. Should be fixed
+    # in the views
+    begin
+      alert = svc_create(params[:pool] ? params[:pool] :
params[:hardware_pool],
+                         additional_create_params)
+      respond_to do |format|
+        format.html {
+          reply = { :object => "pool", :success => true,
+            :alert => alert }
+          reply[:resource_type] = params[:resource_type] if
params[:resource_type]
+          render :json => reply
+        }
+        format.xml {
+          render :xml => @pool.to_xml(XML_OPTS),
+          :status => :created,
+          :location => hardware_pool_url(@pool)
+        }
+      end
+    rescue PermissionError => perm_error
+      handle_auth_error(perm_error.message)
+    rescue Exception => ex
+      respond_to do |format|
+        format.json { json_error("pool", @pool, ex) }
+        format.xml  { render :xml => @pool.errors,
+          :status => :unprocessable_entity }
+      end
+    end
+  end
+
+  def update
+    begin
+      alert = svc_update(params[:id], params[:pool] ? params[:pool] :
+                                      params[:hardware_pool])
+      respond_to do |format|
+        format.html {
+          reply = { :object => "pool", :success => true, :alert
=> alert }
+          render :json => reply
+        }
+        format.xml {
+          render :xml => @pool.to_xml(XML_OPTS),
+          :status => :created,
+          :location => hardware_pool_url(@pool)
+        }
+      end
+    rescue PermissionError => perm_error
+      handle_auth_error(perm_error.message)
+    rescue Exception => ex
+      respond_to do |format|
+        format.json { json_error("pool", @pool, ex) }
+        format.xml  { render :xml => @pool.errors,
+          :status => :unprocessable_entity }
+      end
+    end
+  end
+
+  def additional_create_params
+    {}
+  end
+
   def edit
     render :layout => 'popup'
   end
 
+  def destroy
+    alert = nil
+    success = true
+    status = :ok
+    begin
+      alert = svc_destroy(params[:id])
+    rescue ActionError => error
+      alert = error.message
+      success = false
+      status = :conflict
+    rescue PermissionError => error
+      alert = error.message
+      success = false
+      status = :forbidden
+    rescue Exception => error
+      alert = error.message
+      success = false
+      status = :method_not_allowed
+    end
+    respond_to do |format|
+      format.json { render :json => { :object => "pool",
:success => success,
+          :alert => alert } }
+      format.xml { head status }
+    end
+  end
+
   protected
   def pre_new
     @parent = Pool.find(params[:parent_id])
     set_perms(@parent)
   end
-  def pre_create
-    #this is currently only true for the rest API for hardware pools
-    if params[:hardware_pool]
-      @parent = Pool.find(params[:hardware_pool][:parent_id])
-    else
-      @parent = Pool.find(params[:parent_id])
-    end
-    set_perms(@parent)
-  end
   def pre_show_pool
-    pre_show
-  end
-  def pre_show
+    @pool = Pool.find(params[:id])
     set_perms(@pool)
     authorize_view
   end
+  # FIXME: remove these when service transition is complete. these are here
+  # to keep from running permissions checks and other setup steps twice
+  def tmp_pre_update
+  end
+  def tmp_authorize_admin
+  end
 
 end
diff --git a/src/app/controllers/resources_controller.rb
b/src/app/controllers/resources_controller.rb
index c61ef46..63bc98c 100644
--- a/src/app/controllers/resources_controller.rb
+++ b/src/app/controllers/resources_controller.rb
@@ -16,15 +16,15 @@
 # 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.
+require 'services/vm_resource_pool_service'
 
 class ResourcesController < PoolController
+  include VmResourcePoolService
   def index
     list
     render :action => 'list'
   end
 
-  before_filter :pre_vm_actions, :only => [:vm_actions]
-
   # GETs should be safe (see http://www.w3.org/2001/tag/doc/whenToUseGet.html)
   verify :method => :post, :only => [ :destroy, :create, :update ],
          :redirect_to => { :action => :list }
@@ -69,89 +69,62 @@ class ResourcesController < PoolController
   end
 
   def vms_json
-    pre_show
+    pre_show_pool
     super(:full_items => @pool.vms, :find_opts => {}, :include_pool =>
:true)
   end
 
-  def create
-    begin
-      @pool.create_with_parent(@parent)
-      render :json => { :object => "vm_resource_pool", :success
=> true,
-                        :alert => "Virtual Machine Pool was
successfully created." }
-    rescue
-      render :json => { :object => "vm_resource_pool", :success
=> false,
-                        :errors =>
@pool.errors.localize_error_messages.to_a}
-    end    
-  end
-
-  def update
-    begin
-      @pool.update_attributes!(params[:pool])
-      render :json => { :object => "vm_resource_pool", :success
=> true,
-                        :alert => "Virtual Machine Pool was
successfully modified." }
-    rescue
-      render :json => { :object => "vm_resource_pool", :success
=> false,
-                        :errors =>
@pool.errors.localize_error_messages.to_a}
-    end
+  def additional_create_params
+    {:parent_id => (params[:hardware_pool] ?
+                    params[:hardware_pool][:parent_id] :
+                    params[:parent_id])}
   end
 
-  #FIXME: we need permissions checks. user must have permission. We also need
to fail
+   #FIXME: we need permissions checks. user must have permission. We also need
to fail
   # for pools that aren't currently empty
   def delete
-    vm_pool_ids_str = params[:vm_pool_ids]
-    vm_pool_ids = vm_pool_ids_str.split(",").collect {|x| x.to_i}
-    vm_pool_names = []
-    begin
-      VmResourcePool.transaction do
-        pools = VmResourcePool.find(:all, :conditions => "id in
(#{vm_pool_ids.join(', ')})")
-        pools.each do |pool|
-          vm_pool_names << pool.name
-          pool.destroy
-        end
+    vm_pool_ids = params[:vm_pool_ids].split(",")
+    successes = []
+    failures = {}
+    vm_pool_ids.each do |pool_id|
+      begin
+        svc_destroy(pool_id)
+        successes << @pool
+      rescue PermissionError => perm_error
+        failures[@pool] = perm_error.message
+      rescue Exception => ex
+        failures[@pool] = ex.message
       end
-      render :json => { :object => "vm_resource_pool", :success
=> true,
-                        :alert => "Virtual Machine Pools
#{vm_pool_names.join(', ')} were successfully deleted." }
-    rescue
-      render :json => { :object => "vm_resource_pool", :success
=> false,
-                        :alert => "Error in deleting Virtual Machine
Pools."}
     end
-  end
-
-  def destroy
-    if @pool.destroy
-      alert="Virtual Machine Pool was successfully deleted."
-      success=true
-    else
-      alert="Failed to delete virtual machine pool."
-      success=false
+    success = failures.empty?
+    alert = ""
+    if !successes.empty?
+      alert == "Virtual Machine Pools #{successes.collect{|pool|
pool.name}.join(', ')} were successfully deleted."
+    end
+    if !failures.empty?
+      alert == " Errors in deleting VM Pools #{failures.collect{|pool,err|
"#{pool.name}: #{err}"}.join(', ')}."
     end
-    render :json => { :object => "vm_resource_pool", :success
=> success, :alert => alert }
+    render :json => { :object => "vm_resource_pool", :success
=> success,
+                      :alert => alert }
   end
 
   def vm_actions
-    @action = params[:vm_action]
-    @action_label = VmTask.action_label(@action)
-    vms_str = params[:vm_ids]
-    @vms = vms_str.split(",").collect {|x| Vm.find(x.to_i)}
-    @success_list = []
-    @failure_list = []
     begin
-      @pool.transaction do
-        @vms.each do |vm|
-          if vm.queue_action(@user, @action)
-            @success_list << vm
-            print vm.description, vm.id, "\n"
-          else
-            @failure_list << vm
-          end
-        end
-      end
-    rescue
+      alert = svc_vm_actions_hosts(params[:id], params[:vm_action],
+                                   params[:vm_ids].split(","))
+      @success_list = @vms
+      @failures = {}
+      render :layout => 'confirmation'
+    rescue PermissionError => perm_error
+      handle_auth_error(perm_error.message)
+    rescue PartialSuccessError => error
+      @success_list = error.successes
+      @failures = error.failures
+      render :layout => 'confirmation'
+    rescue Exeption => ex
       flash[:errmsg] = 'Error queueing VM actions.'
       @success_list = []
       @failure_list = []
     end
-    render :layout => 'confirmation'    
   end
 
   protected
@@ -159,26 +132,11 @@ class ResourcesController < PoolController
     @pool = VmResourcePool.new
     super
   end
-  def pre_create
-    @pool = VmResourcePool.new(params[:pool])
-    super
-  end
   def pre_edit
     @pool = VmResourcePool.find(params[:id])
     @parent = @pool.parent
     @current_pool_id=@pool.id
     set_perms(@pool.parent)
   end
-  def pre_show
-    @pool = VmResourcePool.find(params[:id])
-    super
-    @is_hwpool_admin = @pool.parent.can_modify(@user)
-  end
-  def pre_vm_actions
-    @pool = VmResourcePool.find(params[:id])
-    @parent = @pool.parent
-    set_perms(@pool)
-    authorize_user
-  end
 
 end
diff --git a/src/app/controllers/smart_pools_controller.rb
b/src/app/controllers/smart_pools_controller.rb
index fb6ccb5..bc20024 100644
--- a/src/app/controllers/smart_pools_controller.rb
+++ b/src/app/controllers/smart_pools_controller.rb
@@ -17,14 +17,12 @@
 # MA  02110-1301, USA.  A copy of the GNU General Public License is
 # also available at http://www.gnu.org/copyleft/gpl.html.
 #
+require 'services/smart_pool_service'
 
 class SmartPoolsController < PoolController
+  include SmartPoolService
 
-  before_filter :pre_modify, :only => [:add_hosts, :remove_hosts,
-                                       :add_storage, :remove_storage,
-                                       :add_vms, :remove_vms,
-                                       :add_pools, :remove_pools,
-                                       :add_items, :add_pool_dialog]
+  before_filter :pre_modify, :only => [:add_pool_dialog]
   def show_vms
     show
   end
@@ -38,30 +36,19 @@ class SmartPoolsController < PoolController
   end
 
   def show_storage
-    @storage_tree = @pool.storage_tree(:filter_unavailable => false,
:include_used => true).to_json
-    show
-  end
-
-  def create
     begin
-      @pool.create_with_parent(@parent)
-      render :json => { :object => "smart_pool", :success =>
true,
-                        :alert => "Smart Pool was successfully
created." }
-    rescue
-      render :json => { :object => "smart_pool", :success =>
false,
-                        :errors =>
@pool.errors.localize_error_messages.to_a}
+      svc_show(params[:id])
+      @storage_tree = @pool.storage_tree(:filter_unavailable => false,
:include_used => true).to_json
+      render_show
+    rescue PermissionError => perm_error
+      handle_auth_error(perm_error.message)
     end
   end
 
-  def update
-    begin
-      @pool.update_attributes!(params[:smart_pool])
-      render :json => { :object => "smart_pool", :success =>
true,
-                        :alert => "Smart Pool was successfully
modified." }
-    rescue
-      render :json => { :object => "smart_pool", :success =>
false,
-                        :errors =>
@pool.errors.localize_error_messages.to_a}
-    end
+  def additional_create_params
+    {:parent_id => (params[:hardware_pool] ?
+                    params[:hardware_pool][:parent_id] :
+                    params[:parent_id])}
   end
 
   def add_pool_dialog
@@ -98,7 +85,7 @@ class SmartPoolsController < PoolController
 
   def items_json_internal(item_class, item_assoc)
     if params[:id]
-      pre_show
+      pre_show_pool
       full_items = @pool.send(item_assoc)
       find_opts = {}
       include_pool = false
@@ -120,80 +107,82 @@ class SmartPoolsController < PoolController
   end
 
   def add_hosts
-    edit_items(Host, :add_items, :add)
+    add_or_remove_items(Host, :add)
   end
 
   def remove_hosts
-    edit_items(Host, :remove_items, :remove)
+    add_or_remove_items(Host, :remove)
   end
 
   def add_storage
-    edit_items(StoragePool, :add_items, :add)
+    add_or_remove_items(StoragePool, :add)
   end
 
   def remove_storage
-    edit_items(StoragePool, :remove_items, :remove)
+    add_or_remove_items(StoragePool, :remove)
   end
 
   def add_vms
-    edit_items(Vm, :add_items, :add)
+    add_or_remove_items(Vm, :add)
   end
 
   def remove_vms
-    edit_items(Vm, :remove_items, :remove)
+    add_or_remove_items(Vm, :remove)
   end
 
   def add_pools
-    edit_items(Pool, :add_items, :add)
+    add_or_remove_items(Pool, :add)
   end
 
   def remove_pools
-    edit_items(Pool, :remove_items, :remove)
+    add_or_remove_items(Pool, :remove)
   end
 
-  def edit_items(item_class, item_method, item_action)
-    resource_ids_str = params[:resource_ids]
-    resource_ids = resource_ids_str.split(",").collect {|x| x.to_i}
+  def add_or_remove_items(item_class, item_action)
     begin
-      @pool.send(item_method,item_class, resource_ids)
-      render :json => { :success => true,
-        :alert => "#{item_action.to_s}
#{item_class.table_name.humanize} successful." }
+      alert = svc_add_remove_items(params[:id], item_class, item_action,
+                           params[:resource_ids].split(","))
+      render :json => { :success => true, :alert => alert}
     rescue
       render :json => { :success => false,
         :alert => "#{item_action.to_s}
#{item_class.table_name.humanize} failed." }
+    rescue PermissionError => perm_error
+      handle_auth_error(perm_error.message)
+      # If we need to give more details as to which hosts/storage succeeded,
+      # they're in the exception
+    rescue PartialSuccessError => error
+      render :json => { :success => false, :alert => error.message }
+    rescue Exception => ex
+      render :json => { :success => false, :alert => error.message }
     end
   end
 
   def add_items
     class_and_ids_str = params[:class_and_ids]
-    class_and_ids = class_and_ids_str.split(",").collect {|x|
x.split("_")}
+    class_and_ids = class_and_ids_str.split(",").collect do
|class_and_id_str|
+      class_and_id = class_and_id_str.split("_")
+      class_and_id[0] = class_and_id[0].constantize
+      class_and_id[1] = class_and_id[1].to_a
+    end
 
     begin
-      @pool.transaction do
-        class_and_ids.each do |class_and_id|
-         
@pool.add_item(class_and_id[0].constantize.find(class_and_id[1].to_i))
-        end
-      end
-      render :json => { :success => true,
-        :alert => "Add items to smart pool successful." }
-    rescue => ex
+      alert = svc_add_remove_items(params[:id], nil, :add, class_and_ids)
+      render :json => { :success => true, :alert => alert}
+    rescue
       render :json => { :success => false,
-          :alert => "Add items to smart pool failed: " +
ex.message }
+        :alert => "#{item_action.to_s} failed." }
+    rescue PermissionError => perm_error
+      handle_auth_error(perm_error.message)
+    # If we need to give more details as to which hosts/storage succeeded,
+    # they're in the exception
+    rescue PartialSuccessError => error
+      render :json => { :success => false, :alert => error.message }
+    rescue Exception => ex
+      render :json => { :success => false, :alert => error.message }
     end
 
   end
 
-  def destroy
-    if @pool.destroy
-      alert="Smart Pool was successfully deleted."
-      success=true
-    else
-      alert="Failed to delete Smart pool."
-      success=false
-    end
-    render :json => { :object => "smart_pool", :success =>
success, :alert => alert }
-  end
-
   protected
   #filter methods
   def pre_new
@@ -201,20 +190,11 @@ class SmartPoolsController < PoolController
     @parent = DirectoryPool.get_or_create_user_root(get_login_user)
     set_perms(@parent)
   end
-  def pre_create
-    @pool = SmartPool.new(params[:smart_pool])
-    @parent = DirectoryPool.get_or_create_user_root(get_login_user)
-    set_perms(@parent)
-  end
   def pre_edit
     @pool = SmartPool.find(params[:id])
     @parent = @pool.parent
     set_perms(@pool)
   end
-  def pre_show
-    @pool = SmartPool.find(params[:id])
-    super
-  end
   def pre_modify
     pre_edit
     authorize_admin
diff --git a/src/app/controllers/storage_volume_controller.rb
b/src/app/controllers/storage_volume_controller.rb
index b6b0593..d4a2561 100644
--- a/src/app/controllers/storage_volume_controller.rb
+++ b/src/app/controllers/storage_volume_controller.rb
@@ -22,19 +22,16 @@ class StorageVolumeController < ApplicationController
   def new
     @return_to_workflow = params[:return_to_workflow] || false
     if params[:storage_pool_id]
-      @storage_pool = StoragePool.find(params[:storage_pool_id])
       unless @storage_pool.user_subdividable
-        #fixme: proper error page for popups
-        redirect_to :controller => 'dashboard'
+        html_error_page("User-created storage volumes are not supported on
this pool")
         return
       end
-      new_volume_internal(@storage_pool,
-                          { :storage_pool_id => params[:storage_pool_id]})
+      @storage_volume = StorageVolume.factory(@storage_pool.get_type_label,
+                                              { :storage_pool_id =>
+                                                params[:storage_pool_id]})
     else
-      @source_volume = StorageVolume.find(params[:source_volume_id])
       unless @source_volume.supports_lvm_subdivision
-        #fixme: proper error page for popups
-        redirect_to :controller => 'dashboard'
+        html_error_page("LVM is not supported for this storage
volume")
         return
       end
       lvm_pool = @source_volume.lvm_storage_pool
@@ -47,7 +44,8 @@ class StorageVolumeController < ApplicationController
         lvm_pool.source_volumes << @source_volume
         lvm_pool.save!
       end
-      new_volume_internal(lvm_pool, { :storage_pool_id => lvm_pool.id})
+      @storage_volume = StorageVolume.factory(lvm_pool.get_type_label,
+                                              { :storage_pool_id =>
lvm_pool.id})
       @storage_volume.lv_owner_perms='0744'
       @storage_volume.lv_group_perms='0744'
       @storage_volume.lv_mode_perms='0744'
@@ -99,7 +97,7 @@ class StorageVolumeController < ApplicationController
         format.html { render :layout => 'selection' }
         format.json do
           attr_list = []
-          attr_list << :id if (@storage_pool.user_subdividable and
authorized?(Privilege::MODIFY)
+          attr_list << :id if (@storage_pool.user_subdividable and
authorized?(Privilege::MODIFY))
           attr_list += [:display_name, :size_in_gb, :get_type_label]
           json_list(@storage_pool.storage_volumes, attr_list)
         end
@@ -109,8 +107,6 @@ class StorageVolumeController < ApplicationController
   end
 
   def destroy
-    @storage_volume = StorageVolume.find(params[:id])
-    set_perms(@storage_volume.storage_pool.hardware_pool)
     unless authorized?(Privilege::MODIFY) and
@storage_volume.storage_pool.user_subdividable
       handle_auth_error("You do not have permission to delete this storage
volume.")
     else
@@ -123,6 +119,16 @@ class StorageVolumeController < ApplicationController
     end
   end
 
+  def pre_new
+    if params[:storage_pool_id]
+      @storage_pool = StoragePool.find(params[:storage_pool_id])
+      set_perms(@storage_pool.hardware_pool)
+    else
+      @source_volume = StorageVolume.find(params[:source_volume_id])
+      set_perms(@source_volume.storage_pool.hardware_pool)
+    end
+  end
+
   def pre_create
     volume = params[:storage_volume]
     unless type = params[:storage_type]
@@ -132,14 +138,13 @@ class StorageVolumeController < ApplicationController
     set_perms(@storage_volume.storage_pool.hardware_pool)
     authorize_admin
   end
-
-  private
-  def new_volume_internal(storage_pool, new_params)
-    @storage_volume = StorageVolume.factory(storage_pool.get_type_label,
new_params)
+  # will go away w/ svc layer
+  def pre_edit
+    @storage_volume = StorageVolume.find(params[:id])
     set_perms(@storage_volume.storage_pool.hardware_pool)
-    authorize_admin
   end
 
+  private
   def delete_volume_internal(volume)
     begin
       name = volume.display_name
diff --git a/src/app/models/hardware_pool.rb b/src/app/models/hardware_pool.rb
index 7015854..0e7e745 100644
--- a/src/app/models/hardware_pool.rb
+++ b/src/app/models/hardware_pool.rb
@@ -40,54 +40,6 @@ class HardwarePool < Pool
     hw_root ? hw_root.named_child(DEFAULT_POOL_NAME) : nil
   end
 
-  def create_with_resources(parent, resource_type= nil, resource_ids=[])
-    create_with_parent(parent) do
-      if resource_type == "hosts"
-        move_hosts(resource_ids, id)
-      elsif resource_type == "storage"
-        move_storage(resource_ids, id)
-      end
-    end
-  end
-
-  def move_hosts(host_ids, target_pool_id) 
-    hosts = Host.find(:all, :conditions => "id in
(#{host_ids.join(', ')})")
-    transaction do
-      hosts.each do |host|
-        host.hardware_pool = HardwarePool.find(target_pool_id)
-        host.save!
-      end
-    end
-  end
-
-  def move_storage(storage_pool_ids, target_pool_id)
-    storage_pools = StoragePool.find(:all, :conditions => "id in
(#{storage_pool_ids.join(', ')})")
-    transaction do
-      storage_pools.each do |storage_pool|
-        storage_pool.hardware_pool_id = target_pool_id
-        storage_pool.save!
-      end
-    end
-  end
-
-
-  # todo: does this method still make sense? or should we just enforce
"empty" pools?
-  def move_contents_and_destroy
-    transaction do 
-      parent_id = parent.id
-      hosts.each do |host| 
-        host.hardware_pool_id=parent_id
-        host.save
-      end
-      storage_pools.each do |vol| 
-        vol.hardware_pool_id=parent_id
-        vol.save
-      end
-      # what about quotas -- for now they're deleted
-      destroy
-    end
-  end
-
   def total_storage_volumes
     storage_pools.inject(0) { |sum, pool| sum += pool.storage_volumes.size}
   end
diff --git a/src/app/models/host.rb b/src/app/models/host.rb
index 06d7388..0665c3f 100644
--- a/src/app/models/host.rb
+++ b/src/app/models/host.rb
@@ -134,6 +134,10 @@ class Host < ActiveRecord::Base
     hardware_pool.search_users
   end
 
+  def permission_obj
+    hardware_pool
+  end
+
   def movable?
      return vms.size == 0
   end
diff --git a/src/app/models/pool.rb b/src/app/models/pool.rb
index 6f2a086..2979fcb 100644
--- a/src/app/models/pool.rb
+++ b/src/app/models/pool.rb
@@ -306,6 +306,9 @@ class Pool < ActiveRecord::Base
   def class_and_id
     self.class.name + "_" + self.id.to_s
   end
+  def permission_obj
+    self
+  end
   protected
   def traverse_parents
     if id
diff --git a/src/app/models/smart_pool.rb b/src/app/models/smart_pool.rb
index dba000d..1f718a8 100644
--- a/src/app/models/smart_pool.rb
+++ b/src/app/models/smart_pool.rb
@@ -53,25 +53,6 @@ class SmartPool < Pool
                                   :tagged_id=>item.id}).destroy
   end
 
-  def add_items(item_class, item_ids)
-    items = item_class.find(:all, :conditions => "id in
(#{item_ids.join(', ')})")
-    transaction do
-      items.each { |item| add_item(item)}
-    end
-  end
-
-  def remove_items(item_class, item_ids)
-      tags = smart_pool_tags.find(:all,
-                                  :conditions => "tagged_id in
-                                                  (#{item_ids.join(',
')})
-                                                  and
tagged_type='#{item_class.name}'")
-      transaction do
-        tags.each do |tag|
-          tag.destroy
-        end
-      end
-  end
-
   def self.smart_pools_for_user(user)
     nested_pools = DirectoryPool.get_smart_root.full_set_nested(
                        :privilege => Privilege::MODIFY, :user => user,
diff --git a/src/app/models/storage_pool.rb b/src/app/models/storage_pool.rb
index fbe5954..92548bd 100644
--- a/src/app/models/storage_pool.rb
+++ b/src/app/models/storage_pool.rb
@@ -155,6 +155,10 @@ class StoragePool < ActiveRecord::Base
     return_hash
   end
 
+  def permission_obj
+    hardware_pool
+  end
+
   def movable?
     storage_volumes.each{ |x|
        return false unless x.movable?
diff --git a/src/app/models/vm.rb b/src/app/models/vm.rb
index c62595a..6880c22 100644
--- a/src/app/models/vm.rb
+++ b/src/app/models/vm.rb
@@ -177,11 +177,17 @@ class Vm < ActiveRecord::Base
      :in => EFFECTIVE_STATE.keys
 
 
+  def get_vm_pool
+    vm_resource_pool
+  end
   def get_hardware_pool
     pool = vm_resource_pool
     pool = pool.get_hardware_pool if pool
     pool
   end
+  def permission_obj
+    vm_resource_pool
+  end
   def storage_volume_ids
     storage_volumes.collect {|x| x.id }
   end
diff --git a/src/app/models/vm_resource_pool.rb
b/src/app/models/vm_resource_pool.rb
index 1ab4ab1..e41dcab 100644
--- a/src/app/models/vm_resource_pool.rb
+++ b/src/app/models/vm_resource_pool.rb
@@ -32,6 +32,10 @@ class VmResourcePool < Pool
     traverse_parents { |pool| pool if pool[:type] == HardwarePool.name}
   end
 
+  def get_vm_pool
+    self
+  end
+
   def allocated_resources(exclude_vm = nil)
     pending_cpus = 0
     pending_memory = 0
diff --git a/src/app/models/vm_task.rb b/src/app/models/vm_task.rb
index ed33564..d7afe54 100644
--- a/src/app/models/vm_task.rb
+++ b/src/app/models/vm_task.rb
@@ -36,7 +36,7 @@ class VmTask < Task
   # for migrate VM action, args provides the optional target host
   ACTION_MIGRATE_VM  = "migrate_vm"
 
-  PRIV_OBJECT_VM_POOL = "vm_resource_pool"
+  PRIV_OBJECT_VM_POOL = "get_vm_pool"
   PRIV_OBJECT_HW_POOL = "get_hardware_pool"
 
 
@@ -145,6 +145,12 @@ class VmTask < Task
     actions
   end
 
+  def self.action_privilege(action)
+    return ACTIONS[action][:privilege][0]
+  end
+  def self.action_privilege_object(action, obj)
+    return obj.send(ACTIONS[action][:privilege][1])
+  end
   def self.action_label(action)
     return ACTIONS[action][:label]
   end
diff --git a/src/app/services/application_service.rb
b/src/app/services/application_service.rb
index 4dc5eba..251b37a 100644
--- a/src/app/services/application_service.rb
+++ b/src/app/services/application_service.rb
@@ -29,18 +29,34 @@
 module ApplicationService
   class PermissionError < RuntimeError; end
   class ActionError < RuntimeError; end
+  class PartialSuccessError < RuntimeError
+    def initialize(msg, failures={}, successes=[])
+      @failures = failures
+      @successes = successes
+    end
+    def failures
+      @failures
+    end
+    def successes
+      @successes
+    end
+  end
 
   # Including class must provide a GET_LOGIN_USER
 
+  def user
+    @user ||= get_login_user
+  end
+
   def set_perms(perm_obj)
+    return if @perm_obj && @perm_obj.id == perm_obj.id
     @perm_obj = perm_obj
     @current_pool_id ||= perm_obj.id
-    @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)
+    @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
 
   def authorized?(privilege, perm_obj=nil)
diff --git a/src/app/services/hardware_pool_service.rb
b/src/app/services/hardware_pool_service.rb
new file mode 100644
index 0000000..0dbe8dc
--- /dev/null
+++ b/src/app/services/hardware_pool_service.rb
@@ -0,0 +1,125 @@
+#
+# Copyright (C) 2009 Red Hat, Inc.
+# Written by Scott Seago <sseago at redhat.com>,
+#            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.
+# Mid-level API: Business logic around HW pools
+module HardwarePoolService
+
+  include PoolService
+
+  def svc_create(pool_hash, other_args)
+    # from before_filter
+    @pool = HardwarePool.new(pool_hash)
+    @parent = Pool.find(other_args[:parent_id])
+    authorized!(Privilege::MODIFY, at parent)
+
+    alert = "Hardware Pool was successfully created."
+    Pool.transaction do
+      @pool.create_with_parent(@parent)
+      begin
+        if other_args[:resource_type] == "hosts"
+          svc_move_hosts(@pool.id,
other_args[:resource_ids].split(","), @pool.id)
+        elsif other_args[:resource_type] == "storage"
+          svc_move_storage(@pool.id,
other_args[:resource_ids].split(","), @pool.id)
+        end
+        # wrapped in a transaction, so fail on partial success
+      rescue PartialEuccessError => ex
+        raise ActionError.new("Could not move all hosts or storage to this
pool")
+      end
+    end
+    return alert
+  end
+
+  def svc_move_hosts(pool_id, host_ids, target_pool_id)
+    svc_move_items_internal(pool_id, Host, host_ids, target_pool_id)
+  end
+  def svc_move_storage(pool_id, storage_pool_ids, target_pool_id)
+    svc_move_items_internal(pool_id, StoragePool, storage_pool_ids,
target_pool_id)
+  end
+  def svc_move_items_internal(pool_id, item_class, resource_ids,
target_pool_id)
+    # from before_filter
+    @pool = HardwarePool.find(pool_id)
+    target_pool = Pool.find(target_pool_id)
+    authorized!(Privilege::MODIFY,target_pool)
+    authorized!(Privilege::MODIFY, at pool) unless @pool == target_pool
+
+    resources = item_class.find(resource_ids)
+
+    # relay error message if movable check fails for any resource
+    success = true
+    failed_resources = {}
+    successful_resources = []
+    resources.each do |resource|
+      begin
+        if !resource.movable?
+          failed_resources[resource] = "Not Movable"
+        elsif ! resource.hardware_pool.can_modify(@user)
+          failed_resources[resource] = "Failed permission check"
+        else
+          resource.hardware_pool = target_pool
+          resource.save!
+          successful_resources << resource
+        end
+      rescue Exception => ex
+        failed_resources[resource] = ex.message
+      end
+    end
+    unless failed_resources.empty?
+      raise PartialSuccessError.new("Move
#{item_class.table_name.humanize} only partially successful",
+                                    failed_resources, successful_resources)
+    end
+    return "Move #{item_class.table_name.humanize} successful."
+  end
+
+  def additional_update_actions(pool, pool_hash)
+    # FIXME: For the REST API, we allow moving hosts/storage through
+    # update.  It makes that operation convenient for clients, though makes
+    # the implementation here somewhat ugly.
+    begin
+      [:hosts, :storage_pools].each do |k|
+        objs = pool_hash.delete(k)
+        ids = objs.reject{ |obj| obj[:hardware_pool_id] == @pool.id}.
+          collect{ |obj| obj[:id] }
+        if ids.size > 0
+          if k == :hosts
+            svc_move_hosts(pool.id, ids, pool.id)
+          else
+            svc_move_storage(pool.id, ids, pool.id)
+          end
+        end
+      end
+      # wrapped in a transaction, so fail on partial success
+    rescue PartialEuccessError => ex
+      raise ActionError.new("Could not move all hosts or storage to this
pool")
+    end
+  end
+
+  def check_destroy_preconditions
+    msg = nil
+    if @pool == HardwarePool.get_default_pool
+      msg = "You can't delete the top level Hardware pool"
+    elsif not(@pool.children.empty?)
+      msg = "You can't delete a Pool without first deleting its
children."
+    elsif not(@pool.hosts.empty?)
+      msg = "You can't delete a Pool without first moving its
hosts."
+    elsif not(@pool.storage_pools.empty?)
+      msg = "You can't delete a Pool without first moving its
storage."
+    end
+    raise ActionError.new(msg) if msg
+  end
+end
diff --git a/src/app/services/pool_service.rb b/src/app/services/pool_service.rb
new file mode 100644
index 0000000..8073067
--- /dev/null
+++ b/src/app/services/pool_service.rb
@@ -0,0 +1,60 @@
+#
+# Copyright (C) 2009 Red Hat, Inc.
+# Written by Scott Seago <sseago at redhat.com>,
+#            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.
+# Mid-level API: Business logic around pools
+module PoolService
+
+  include ApplicationService
+
+  def svc_show(pool_id)
+    # from before_filter
+    @pool = Pool.find(pool_id)
+    authorized!(Privilege::VIEW, at pool)
+  end
+
+  def update_perms
+    set_perms(@pool)
+  end
+  def additional_update_actions(pool, pool_hash)
+  end
+
+  def svc_update(pool_id, pool_hash)
+    # from before_filter
+    @pool = Pool.find(params[:id])
+    @parent = @pool.parent
+    update_perms
+    authorized!(Privilege::Modify)
+    Pool.transaction do
+      additional_update_actions(@pool, pool_hash)
+      @pool.update_attributes!(pool_hash)
+    end
+  end
+
+  def svc_destroy(pool_id)
+    # from before_filter
+    @pool = Pool.find(pool_id)
+    authorized!(Privilege::MODIFY, @pool)
+    check_destroy_preconditions
+    @pool.destroy
+    return "Pool was successfully deleted."
+  end
+
+  def check_destroy_preconditions
+  end
+end
diff --git a/src/app/services/smart_pool_service.rb
b/src/app/services/smart_pool_service.rb
new file mode 100644
index 0000000..344b9e8
--- /dev/null
+++ b/src/app/services/smart_pool_service.rb
@@ -0,0 +1,79 @@
+#
+# Copyright (C) 2009 Red Hat, Inc.
+# Written by Scott Seago <sseago at redhat.com>,
+#            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.
+# Mid-level API: Business logic around smart pools
+module SmartPoolService
+
+  include PoolService
+
+  def svc_create(pool_hash, other_args)
+    # from before_filter
+    @pool = SmartPool.new(pool_hash)
+    @parent = DirectoryPool.get_or_create_user_root(get_login_user)
+    authorized!(Privilege::MODIFY, at parent)
+
+    alert = "Smart Pool was successfully created."
+    @pool.create_with_parent(@parent)
+    return alert
+  end
+
+  # if item_class is nil, resource_ids is an array of [class, id] pairs
+  def svc_add_remove_items(pool_id, item_class, item_action, resource_ids)
+    # from before_filter
+    @pool = SmartPool.find(pool_id)
+    @parent = @pool.parent
+    authorized!(Privilege::MODIFY, at pool)
+    unless [:add, :remove].include?(item_action)
+      raise ActionError.new("Invalid action #{item_action}")
+    end
+    if item_class
+      resources = item_class.find(resource_ids)
+    else
+      resources = resource_ids.collect {|the_class,id| the_class.find(id)}
+    end
+
+    # relay error message if movable check fails for any resource
+    success = true
+    failed_resources = {}
+    successful_resources = []
+    resources.each do |resource|
+      begin
+        if item_action == :add
+          if ! resource.permission_obj.can_view(@user)
+            failed_resources[resource] = "Failed permission check"
+          else
+            @pool.add_item(resource)
+            successful_resources << resource
+          end
+        elsif item_action == :remove
+          @pool.remove_item(resource)
+          successful_resources << resource
+        end
+      rescue Exception => ex
+        failed_resources[resource] = ex.message
+      end
+    end
+    unless failed_resources.empty?
+      raise PartialSuccessError.new("#{item_action.to_s}
#{item_class.table_name.humanize if item_class} only partially successful",
+                                    failed_resources, successful_resources)
+    end
+    return "#{item_action.to_s} #{item_class.table_name.humanize}
successful."
+  end
+
+end
diff --git a/src/app/services/vm_resource_pool_service.rb
b/src/app/services/vm_resource_pool_service.rb
new file mode 100644
index 0000000..30f7106
--- /dev/null
+++ b/src/app/services/vm_resource_pool_service.rb
@@ -0,0 +1,76 @@
+#
+# Copyright (C) 2009 Red Hat, Inc.
+# Written by Scott Seago <sseago at redhat.com>,
+#            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.
+# Mid-level API: Business logic around VM pools
+module VmResourcePoolService
+
+  include PoolService
+
+  def svc_create(pool_hash, other_args)
+    # from before_filter
+    @pool = VmResourcePool.new(pool_hash)
+    @parent = Pool.find(other_args[:parent_id])
+    authorized!(Privilege::MODIFY, at parent)
+
+    alert = "VM Pool was successfully created."
+    @pool.create_with_parent(@parent)
+    return alert
+  end
+
+  def update_perms
+    @current_pool_id=@pool.id
+    set_perms(@pool.parent)
+  end
+
+  def svc_vm_actions(pool_id, vm_action, vm_ids)
+    # from before_filter
+    @pool = VmResourcePool.find(pool_id)
+    @parent = @pool.parent
+    @action = vm_action
+    @action_label = VmTask.action_label(@action)
+    authorized!(VmTask.action_privilege(@action),
+                VmTask.action_privilege_object(@action, at pool))
+
+    @vms = Vm.find(vm_ids)
+
+    successful_vms = []
+    failed_vms = {}
+    @vms.each do |vm|
+      begin
+        if vm.vm_resource_pool != @pool
+          failed_vms[vm] = "VM #{vm.description} does not belong to the
current pool."
+        elsif vm.queue_action(@user, @action)
+          successful_vms << vm
+        else
+          failed_vms[vm] = "unavailable action"
+        end
+      rescue Exception => ex
+        failed_vms[vm] = ex.message
+      end
+    end
+    unless failed_vms.empty?
+      raise PartialSuccessError.new("#{@action} only partially
successful",
+                                    failed_vms, successful_vms)
+    end
+    return "Action #{@action} successful."
+  end
+
+
+
+end
diff --git a/src/app/views/resources/quick_summary.rhtml
b/src/app/views/resources/quick_summary.rhtml
index 31c4033..70b8df6 100644
--- a/src/app/views/resources/quick_summary.rhtml
+++ b/src/app/views/resources/quick_summary.rhtml
@@ -2,7 +2,7 @@
   <%=h @pool.name %> quota
 <%- end -%>
 <%- content_for :action_links do -%>
-  <%if @is_hwpool_admin -%>
+  <%if @pool.parent.can_modify(@user) -%>
     <%= link_to image_tag("icon_edit.png") + " Edit",
                           {:controller => 'resources', :action =>
'edit', :id => @pool},
                           :rel=>"facebox[.bolder]",
:class=>"selection_facebox" %>
diff --git a/src/app/views/resources/vm_actions.rhtml
b/src/app/views/resources/vm_actions.rhtml
index d556b94..b3fa1ad 100644
--- a/src/app/views/resources/vm_actions.rhtml
+++ b/src/app/views/resources/vm_actions.rhtml
@@ -25,10 +25,10 @@ Action succeeded for these VMs:
   <% end %>
 </ul>
 
-Action invalid for these VMs:
+Action failed for these VMs:
 <ul>
-  <% for vm in @failure_list %>
-    <li><%= vm.description %></li>
+  <% for vm, msg in @failures %>
+    <li><%= vm.description %>: <%= msg %></li>
   <% end %>
 </ul>
 </div>
diff --git a/src/app/views/smart_pools/_form.rhtml
b/src/app/views/smart_pools/_form.rhtml
index 2f2156a..c1ec6bd 100644
--- a/src/app/views/smart_pools/_form.rhtml
+++ b/src/app/views/smart_pools/_form.rhtml
@@ -1,6 +1,6 @@
 <%= error_messages_for 'vm_resource_pool' %>
 
 <!--[form:vm_resource_pool]-->
-<%= text_field_with_label "Name", 'smart_pool',
'name'  %>
+<%= text_field_with_label "Name", 'pool', 'name' 
%>
 <!--[eoform:vm_resource_pool]-->
 
-- 
1.6.0.6

On Tue, 2009-04-28 at 22:10 +0000, Scott Seago wrote:
> This is the second round of controller/service layer refactoring
> changes -- in this case for the pool-related controllers (HW, VM, and
> Smart pools)
This looks mostly good, modulo comments below. It would also be good to
include rdoc comments in the service modules to document required
permissions and instance variables that get set.
> diff --git a/src/app/controllers/hardware_controller.rb
b/src/app/controllers/hardware_controller.rb
> index 2158e08..8261cd7 100644
> --- a/src/app/controllers/hardware_controller.rb
> +++ b/src/app/controllers/hardware_controller.rb
> @@ -17,8 +17,10 @@
>  # MA  02110-1301, USA.  A copy of the GNU General Public License is
>  # also available at http://www.gnu.org/copyleft/gpl.html.
>  #
> +require 'services/hardware_pool_service'
This breaks class reloading in the development environment, and is not
necessary at all. Should be removed
> @@ -113,8 +112,13 @@ class HardwareController < PoolController
>    end
>  
>    def show_storage
> -    @storage_tree = @pool.storage_tree(:filter_unavailable => false,
:include_used => true).to_json
> -    show
> +    begin
> +      svc_show(params[:id])
> +      @storage_tree = @pool.storage_tree(:filter_unavailable => false,
:include_used => true).to_json
> +      render_show
> +    rescue PermissionError => perm_error
> +      handle_auth_error(perm_error.message)
> +    end
>    end
I know that it's just moving code around, but please avoid lines with
more than 80 chars.
> @@ -190,144 +194,49 @@ class HardwareController < PoolController
 
>    def add_hosts
> -    edit_items(Host, :move_hosts, @pool.id, :add)
> +    edit_items(Host, @pool.id, :add)
>    end
>  
>    def move_hosts
> -    edit_items(Host, :move_hosts, params[:target_pool_id], :move)
> +    edit_items(Host, params[:target_pool_id], :move)
>    end
>  
>    def add_storage
> -    edit_items(StoragePool, :move_storage, @pool.id, :add)
> +    edit_items(StoragePool, @pool.id, :add)
>    end
>  
>    def move_storage
> -    edit_items(StoragePool, :move_storage, params[:target_pool_id], :move)
> +    edit_items(StoragePool, params[:target_pool_id], :move)
>    end
>  
> -  #FIXME: we need permissions checks. user must have permission on src
pool
> -  # in addition to the current pool (which is checked). We also need to
fail
> -  # for storage that aren't currently empty
> -  def edit_items(item_class, item_method, target_pool_id, item_action)
> -    resource_ids_str = params[:resource_ids]
> -    resource_ids = resource_ids_str.split(",").collect {|x|
x.to_i}
> -
> -    # if user doesn't have modify permission on both source and
destination
> -    unless @pool.can_modify(@user) and
Pool.find(target_pool_id).can_modify(@user)
> -        render :json => { :success => false,
> -               :alert => "Cannot #{item_action.to_s}
#{item_class.table_name.humanize} without admin permissions on both pools"
}
> -        return
> -    end
> -
> -    # relay error message if movable check fails for any resource
> -    success = true
> -    failed_resources = ""
> -    resource_ids.each {|x|
> -       unless item_class.find(x).movable?
> -         success = false
> -         failed_resources += x.to_s + " "
> -       end
> -    }
> -    resource_ids.delete_if { |x| ! item_class.find(x).movable? }
> -
> +  def edit_items(item_class, target_pool_id, item_action)
>      begin
> -      @pool.transaction do
> -        @pool.send(item_method, resource_ids, target_pool_id)
> +      if item_class == Host
> +        alert = svc_move_hosts(params[:id],
params[:resource_ids].split(","), target_pool_id)
> +      elsif item_class == StoragePool
> +        alert = svc_move_storage(params[:id],
params[:resource_ids].split(","), target_pool_id)
> +      else
> +        raise "invalid class #{item_class}"
>        end
> -    rescue
> -      success = false
> -    end
> -
> -    if success
> -      render :json => { :success => true,
> -        :alert => "#{item_action.to_s}
#{item_class.table_name.humanize} successful.",
> +      render :json => { :success => true, :alert => alert,
>          :storage => @pool.storage_tree({:filter_unavailable =>
false, :include_used => true, :state => item_action.to_s})}
> -    else
> -      render :json => { :success => false,
> -         :alert => "#{item_action.to_s}
#{item_class.table_name.humanize} failed" +
> -                   (failed_resources == "" ? "." :
" for " + failed_resources) }
> +    rescue PermissionError => perm_error
> +      handle_auth_error(perm_error.message)
> +      # If we need to give more details as to which hosts/storage
succeeded,
> +      # they're in the exception
> +    rescue PartialSuccessError => error
> +      render :json => { :success => false, :alert =>
error.message }
> +    rescue Exception => ex
> +      render :json => { :success => false, :alert =>
error.message }
>      end
>    end
This is a good example of how our current error handling makes things
more complex: we have move_hosts/move_storage actions, call a generic
edit_items, and then dispatch in that method on the class of one of the
parameters.

First off, it would be more rubyish if edit_items took a block that has
the proper calls to svc_move_hosts/svc_move_storage. Second, it seems
the main point of edit_items is to factor all the error handling
overhead intoa common method.

For the time being, edit_items should take a block; in the future we
need to look into handling more errors with rescue_with, avoiding all
that error handling clutter altogether.
> diff --git a/src/app/controllers/pool_controller.rb
b/src/app/controllers/pool_controller.rb
> index 06f8768..41c1be9 100644
> --- a/src/app/controllers/pool_controller.rb
> +++ b/src/app/controllers/pool_controller.rb
> @@ -40,6 +37,15 @@ class PoolController < ApplicationController
>    end
>  
>    def show
> +    begin
> +      svc_show(params[:id])
> +      render_show
> +    rescue PermissionError => perm_error
> +      handle_auth_error(perm_error.message)
> +    end
> +  end
> +
> +  def render_show
>      respond_to do |format|
>        format.html {
>          render :layout => 'tabs-and-content' if params[:ajax]
> @@ -49,10 +55,15 @@ class PoolController < ApplicationController
>          render :xml => @pool.to_xml(XML_OPTS)
>        }
>      end
> -  end
Why the separate method render_show ? I don't think it simplifies things
compared to inlining that method.
> @@ -97,30 +108,111 @@ class PoolController < ApplicationController
> +    rescue Exception => ex
> +      respond_to do |format|
> +        format.json { json_error("pool", @pool, ex) }
> +        format.xml  { render :xml => @pool.errors,
> +          :status => :unprocessable_entity }
> +      end
> +    end
While I'm on a tear about error handling, I really don't like seeing
'rescue Exception' or even worse a bare rescue. It would be nice if we
could finda way to report errors for json and xml with additional error
templates.
> +  end
> +
> +  def update
> +    begin
> +      alert = svc_update(params[:id], params[:pool] ? params[:pool] :
> +                                      params[:hardware_pool])
> +      respond_to do |format|
> +        format.html {
> +          reply = { :object => "pool", :success => true,
:alert => alert }
> +          render :json => reply
> +        }
Is that right ? format.html and render :json ?
> diff --git a/src/app/controllers/resources_controller.rb
b/src/app/controllers/resources_controller.rb
> index c61ef46..63bc98c 100644
> --- a/src/app/controllers/resources_controller.rb
> +++ b/src/app/controllers/resources_controller.rb
> @@ -16,15 +16,15 @@
>  # 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.
> +require 'services/vm_resource_pool_service'
Not needed
> @@ -69,89 +69,62 @@ class ResourcesController < PoolController
...
> -  def destroy
> -    if @pool.destroy
> -      alert="Virtual Machine Pool was successfully deleted."
> -      success=true
> -    else
> -      alert="Failed to delete virtual machine pool."
> -      success=false
> +    success = failures.empty?
> +    alert = ""
> +    if !successes.empty?
> +      alert == "Virtual Machine Pools #{successes.collect{|pool|
pool.name}.join(', ')} were successfully deleted."
> +    end
> +    if !failures.empty?
> +      alert == " Errors in deleting VM Pools
#{failures.collect{|pool,err| "#{pool.name}: #{err}"}.join(',
')}."
>      end
s/==/=/

The logic also deserves a comment that clobbering alert on failure is
intentional.
> diff --git a/src/app/controllers/smart_pools_controller.rb
b/src/app/controllers/smart_pools_controller.rb
> index fb6ccb5..bc20024 100644
> --- a/src/app/controllers/smart_pools_controller.rb
> +++ b/src/app/controllers/smart_pools_controller.rb
> @@ -17,14 +17,12 @@
>  # MA  02110-1301, USA.  A copy of the GNU General Public License is
>  # also available at http://www.gnu.org/copyleft/gpl.html.
>  #
> +require 'services/smart_pool_service'
Not needed.
> diff --git a/src/app/controllers/storage_volume_controller.rb
b/src/app/controllers/storage_volume_controller.rb
> index b6b0593..d4a2561 100644
> --- a/src/app/controllers/storage_volume_controller.rb
> +++ b/src/app/controllers/storage_volume_controller.rb
This seems tangential to the pool refactoring - could that go into a
separate patch ?
> diff --git a/src/app/models/host.rb b/src/app/models/host.rb
> index 06d7388..0665c3f 100644
> --- a/src/app/models/host.rb
> +++ b/src/app/models/host.rb
> @@ -134,6 +134,10 @@ class Host < ActiveRecord::Base
>      hardware_pool.search_users
>    end
>  
> +  def permission_obj
> +    hardware_pool
> +  end
> +
Nice !
> diff --git a/src/app/services/application_service.rb
b/src/app/services/application_service.rb
> index 4dc5eba..251b37a 100644
> --- a/src/app/services/application_service.rb
> +++ b/src/app/services/application_service.rb
> @@ -29,18 +29,34 @@
>  module ApplicationService
>    class PermissionError < RuntimeError; end
>    class ActionError < RuntimeError; end
> +  class PartialSuccessError < RuntimeError
> +    def initialize(msg, failures={}, successes=[])
> +      @failures = failures
> +      @successes = successes
> +    end
> +    def failures
> +      @failures
> +    end
> +    def successes
> +      @successes
> +    end
> +  end
attr_reader :failures, :successes ?
>    # Including class must provide a GET_LOGIN_USER
>  
> +  def user
> +    @user ||= get_login_user
> +  end
> +
I don't think this is the right place to cache the user - if we want
that, it should be done in get_login_user. In any event, this and the
changes to set_perm_obj should be in a separate patch.
> diff --git a/src/app/services/hardware_pool_service.rb
b/src/app/services/hardware_pool_service.rb
> new file mode 100644
> index 0000000..0dbe8dc
> --- /dev/null
> +++ b/src/app/services/hardware_pool_service.rb
> @@ -0,0 +1,125 @@
> +#
> +# Copyright (C) 2009 Red Hat, Inc.
> +# Written by Scott Seago <sseago at redhat.com>,
> +#            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.
> +# Mid-level API: Business logic around HW pools
> +module HardwarePoolService
> +
> +  include PoolService
> +
> +  def svc_create(pool_hash, other_args)
How about 'svc_create(parent_id, resource_types, resource_ids,
pool_hash)' ? The signature feels a little WUI specific.
> +    # from before_filter
> +    @pool = HardwarePool.new(pool_hash)
> +    @parent = Pool.find(other_args[:parent_id])
> +    authorized!(Privilege::MODIFY, at parent)
> +
> +    alert = "Hardware Pool was successfully created."
> +    Pool.transaction do
> +      @pool.create_with_parent(@parent)
> +      begin
> +        if other_args[:resource_type] == "hosts"
> +          svc_move_hosts(@pool.id,
other_args[:resource_ids].split(","), @pool.id)
> +        elsif other_args[:resource_type] == "storage"
> +          svc_move_storage(@pool.id,
other_args[:resource_ids].split(","), @pool.id)
> +        end
> +        # wrapped in a transaction, so fail on partial success
> +      rescue PartialEuccessError => ex
> +        raise ActionError.new("Could not move all hosts or storage to
this pool")
Why not make throwing PartialSuccessError part of the method's
signature ? Does wrapping in an ActionError really buy us anything ?
> +      end
> +    end
> +    return alert
> +  end
> +
> +  def svc_move_hosts(pool_id, host_ids, target_pool_id)
> +    svc_move_items_internal(pool_id, Host, host_ids, target_pool_id)
> +  end
> +  def svc_move_storage(pool_id, storage_pool_ids, target_pool_id)
> +    svc_move_items_internal(pool_id, StoragePool, storage_pool_ids,
target_pool_id)
> +  end
> +  def svc_move_items_internal(pool_id, item_class, resource_ids,
target_pool_id)
> +    # from before_filter
> +    @pool = HardwarePool.find(pool_id)
> +    target_pool = Pool.find(target_pool_id)
> +    authorized!(Privilege::MODIFY,target_pool)
> +    authorized!(Privilege::MODIFY, at pool) unless @pool == target_pool
Does it even make sense to do anything if @pool == target_pool ? We
should just bail.
> diff --git a/src/app/services/pool_service.rb
b/src/app/services/pool_service.rb
> new file mode 100644
> index 0000000..8073067
> --- /dev/null
> +++ b/src/app/services/pool_service.rb
> @@ -0,0 +1,60 @@
...
> +  def svc_show(pool_id)
> +    # from before_filter
> +    @pool = Pool.find(pool_id)
> +    authorized!(Privilege::VIEW, at pool)
> +  end
> +
> +  def update_perms
> +    set_perms(@pool)
> +  end
In the host controller, I addressed the same problem with

        private 
        def lookup(id, priv)
          @pool = Pool.find(id)
          authorized!(priv, @pool)
        end

Of course, I would prefer that here, too.
> +  def svc_update(pool_id, pool_hash)
> +    # from before_filter
> +    @pool = Pool.find(params[:id])
> +    @parent = @pool.parent
> +    update_perms
> +    authorized!(Privilege::Modify)
Should be Privilege::MODIFY

David

David Lutterkort wrote:
> On Tue, 2009-04-28 at 22:10 +0000, Scott Seago wrote:
>   
>> This is the second round of controller/service layer refactoring
>> changes -- in this case for the pool-related controllers (HW, VM, and
>> Smart pools)
>>     
>
> This looks mostly good, modulo comments below. It would also be good to
> include rdoc comments in the service modules to document required
> permissions and instance variables that get set.
>
>   
Sending updated patch shortly with the fixes below:
>> diff --git a/src/app/controllers/hardware_controller.rb
b/src/app/controllers/hardware_controller.rb
>> index 2158e08..8261cd7 100644
>> --- a/src/app/controllers/hardware_controller.rb
>> +++ b/src/app/controllers/hardware_controller.rb
>>     
>
>   
>> @@ -17,8 +17,10 @@
>>  # MA  02110-1301, USA.  A copy of the GNU General Public License is
>>  # also available at http://www.gnu.org/copyleft/gpl.html.
>>  #
>> +require 'services/hardware_pool_service'
>>     
>
> This breaks class reloading in the development environment, and is not
> necessary at all. Should be removed
>
>   
Done
>> @@ -113,8 +112,13 @@ class HardwareController < PoolController
>>    end
>>  
>>    def show_storage
>> -    @storage_tree = @pool.storage_tree(:filter_unavailable =>
false, :include_used => true).to_json
>> -    show
>> +    begin
>> +      svc_show(params[:id])
>> +      @storage_tree = @pool.storage_tree(:filter_unavailable =>
false, :include_used => true).to_json
>> +      render_show
>> +    rescue PermissionError => perm_error
>> +      handle_auth_error(perm_error.message)
>> +    end
>>    end
>>     
>
> I know that it's just moving code around, but please avoid lines with
> more than 80 chars.
>
>   
Done
>> @@ -190,144 +194,49 @@ class HardwareController < PoolController
>>     
>  
>   
>>    def add_hosts
>> -    edit_items(Host, :move_hosts, @pool.id, :add)
>> +    edit_items(Host, @pool.id, :add)
>>    end
>>  
>>    def move_hosts
>> -    edit_items(Host, :move_hosts, params[:target_pool_id], :move)
>> +    edit_items(Host, params[:target_pool_id], :move)
>>    end
>>  
>>    def add_storage
>> -    edit_items(StoragePool, :move_storage, @pool.id, :add)
>> +    edit_items(StoragePool, @pool.id, :add)
>>    end
>>  
>>    def move_storage
>> -    edit_items(StoragePool, :move_storage, params[:target_pool_id],
:move)
>> +    edit_items(StoragePool, params[:target_pool_id], :move)
>>    end
>>  
>> -  #FIXME: we need permissions checks. user must have permission on src
pool
>> -  # in addition to the current pool (which is checked). We also need
to fail
>> -  # for storage that aren't currently empty
>> -  def edit_items(item_class, item_method, target_pool_id, item_action)
>> -    resource_ids_str = params[:resource_ids]
>> -    resource_ids = resource_ids_str.split(",").collect {|x|
x.to_i}
>> -
>> -    # if user doesn't have modify permission on both source and
destination
>> -    unless @pool.can_modify(@user) and
Pool.find(target_pool_id).can_modify(@user)
>> -        render :json => { :success => false,
>> -               :alert => "Cannot #{item_action.to_s}
#{item_class.table_name.humanize} without admin permissions on both pools"
}
>> -        return
>> -    end
>> -
>> -    # relay error message if movable check fails for any resource
>> -    success = true
>> -    failed_resources = ""
>> -    resource_ids.each {|x|
>> -       unless item_class.find(x).movable?
>> -         success = false
>> -         failed_resources += x.to_s + " "
>> -       end
>> -    }
>> -    resource_ids.delete_if { |x| ! item_class.find(x).movable? }
>> -
>> +  def edit_items(item_class, target_pool_id, item_action)
>>      begin
>> -      @pool.transaction do
>> -        @pool.send(item_method, resource_ids, target_pool_id)
>> +      if item_class == Host
>> +        alert = svc_move_hosts(params[:id],
params[:resource_ids].split(","), target_pool_id)
>> +      elsif item_class == StoragePool
>> +        alert = svc_move_storage(params[:id],
params[:resource_ids].split(","), target_pool_id)
>> +      else
>> +        raise "invalid class #{item_class}"
>>        end
>> -    rescue
>> -      success = false
>> -    end
>> -
>> -    if success
>> -      render :json => { :success => true,
>> -        :alert => "#{item_action.to_s}
#{item_class.table_name.humanize} successful.",
>> +      render :json => { :success => true, :alert => alert,
>>          :storage => @pool.storage_tree({:filter_unavailable =>
false, :include_used => true, :state => item_action.to_s})}
>> -    else
>> -      render :json => { :success => false,
>> -         :alert => "#{item_action.to_s}
#{item_class.table_name.humanize} failed" +
>> -                   (failed_resources == "" ? "." :
" for " + failed_resources) }
>> +    rescue PermissionError => perm_error
>> +      handle_auth_error(perm_error.message)
>> +      # If we need to give more details as to which hosts/storage
succeeded,
>> +      # they're in the exception
>> +    rescue PartialSuccessError => error
>> +      render :json => { :success => false, :alert =>
error.message }
>> +    rescue Exception => ex
>> +      render :json => { :success => false, :alert =>
error.message }
>>      end
>>    end
>>     
>
> This is a good example of how our current error handling makes things
> more complex: we have move_hosts/move_storage actions, call a generic
> edit_items, and then dispatch in that method on the class of one of the
> parameters.
>
> First off, it would be more rubyish if edit_items took a block that has
> the proper calls to svc_move_hosts/svc_move_storage. Second, it seems
> the main point of edit_items is to factor all the error handling
> overhead intoa common method.
>
> For the time being, edit_items should take a block; in the future we
> need to look into handling more errors with rescue_with, avoiding all
> that error handling clutter altogether.
>
>   
OK so we'll revisit this when we add the rescue_with stuff, but for now 
I've actually changed this to take a symbol arg for the method name. 
Since the only thing that's different here is which method is calling, 
this avoids the if block with two method invocations (the args are the 
same in both cases) -- I started to do blocks but I realized that I'd 
have to repeat the method argument generation in each block, which is 
also rather un-rubyish.
>> @@ -97,30 +108,111 @@ class PoolController < ApplicationController
>>     
>> +  end
>> +
>> +  def update
>> +    begin
>> +      alert = svc_update(params[:id], params[:pool] ? params[:pool] :
>> +                                      params[:hardware_pool])
>> +      respond_to do |format|
>> +        format.html {
>> +          reply = { :object => "pool", :success =>
true, :alert => alert }
>> +          render :json => reply
>> +        }
>>     
>
> Is that right ? format.html and render :json ?
>   
fixed
>> diff --git a/src/app/controllers/resources_controller.rb
b/src/app/controllers/resources_controller.rb
>> index c61ef46..63bc98c 100644
>> --- a/src/app/controllers/resources_controller.rb
>> +++ b/src/app/controllers/resources_controller.rb
>> @@ -16,15 +16,15 @@
>>  # 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.
>> +require 'services/vm_resource_pool_service'
>>     
>
> Not needed
>   
Fixed
>> @@ -69,89 +69,62 @@ class ResourcesController < PoolController
>>     
>
> ...
>
>   
>> -  def destroy
>> -    if @pool.destroy
>> -      alert="Virtual Machine Pool was successfully deleted."
>> -      success=true
>> -    else
>> -      alert="Failed to delete virtual machine pool."
>> -      success=false
>> +    success = failures.empty?
>> +    alert = ""
>> +    if !successes.empty?
>> +      alert == "Virtual Machine Pools #{successes.collect{|pool|
pool.name}.join(', ')} were successfully deleted."
>> +    end
>> +    if !failures.empty?
>> +      alert == " Errors in deleting VM Pools
#{failures.collect{|pool,err| "#{pool.name}: #{err}"}.join(',
')}."
>>      end
>>     
>
> s/==/=/
>
> The logic also deserves a comment that clobbering alert on failure is
> intentional.
>
>   
Fixed this -- and no alert clobbering on delete either.
>> diff --git a/src/app/controllers/smart_pools_controller.rb
b/src/app/controllers/smart_pools_controller.rb
>> index fb6ccb5..bc20024 100644
>> --- a/src/app/controllers/smart_pools_controller.rb
>> +++ b/src/app/controllers/smart_pools_controller.rb
>> @@ -17,14 +17,12 @@
>>  # MA  02110-1301, USA.  A copy of the GNU General Public License is
>>  # also available at http://www.gnu.org/copyleft/gpl.html.
>>  #
>> +require 'services/smart_pool_service'
>>     
> Not needed.
>   
Done
>> diff --git a/src/app/controllers/storage_volume_controller.rb
b/src/app/controllers/storage_volume_controller.rb
>> index b6b0593..d4a2561 100644
>> --- a/src/app/controllers/storage_volume_controller.rb
>> +++ b/src/app/controllers/storage_volume_controller.rb
>>     
>
> This seems tangential to the pool refactoring - could that go into a
> separate patch ?
>
>   
OK I'll pull this out into a later patch.
>> diff --git a/src/app/services/application_service.rb
b/src/app/services/application_service.rb
>> index 4dc5eba..251b37a 100644
>> --- a/src/app/services/application_service.rb
>> +++ b/src/app/services/application_service.rb
>> @@ -29,18 +29,34 @@
>>  module ApplicationService
>>    class PermissionError < RuntimeError; end
>>    class ActionError < RuntimeError; end
>> +  class PartialSuccessError < RuntimeError
>> +    def initialize(msg, failures={}, successes=[])
>> +      @failures = failures
>> +      @successes = successes
>> +    end
>> +    def failures
>> +      @failures
>> +    end
>> +    def successes
>> +      @successes
>> +    end
>> +  end
>>     
>
> attr_reader :failures, :successes ?
>   
Fixed this
>>    # Including class must provide a GET_LOGIN_USER
>>  
>> +  def user
>> +    @user ||= get_login_user
>> +  end
>> +
>>     
>
> I don't think this is the right place to cache the user - if we want
> that, it should be done in get_login_user. In any event, this and the
> changes to set_perm_obj should be in a separate patch.
>
>   
I guess I'll un-do this for now then and later re-create this later, as 
I don't trust git rebase -i to be useful for me to separate commits 
_within_ a file.
>> diff --git a/src/app/services/pool_service.rb
b/src/app/services/pool_service.rb
>> new file mode 100644
>> index 0000000..8073067
>> --- /dev/null
>> +++ b/src/app/services/pool_service.rb
>> @@ -0,0 +1,60 @@
>> +  def svc_update(pool_id, pool_hash)
>> +    # from before_filter
>> +    @pool = Pool.find(params[:id])
>> +    @parent = @pool.parent
>> +    update_perms
>> +    authorized!(Privilege::Modify)
>>     
>
> Should be Privilege::MODIFY
>
> David
>
>
>   
Fixed