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

Scott Seago sseago at redhat.com
Tue Apr 28 22:10:33 UTC 2009


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




More information about the ovirt-devel mailing list