Ovirt devel - May 2009 - [PATCH server] final cleanup for service layer refactoring.

I've pulled out the no-longer-necessary remnants of the old way of handling
auth and before_filters as well as fixing a couple bugs that had crept in along
the way.

Unit test fixes for the refactoring will follow in a subsequent patch.

Signed-off-by: Scott Seago <sseago at redhat.com>
---
 src/app/controllers/application.rb                 |   46 ----------------
 src/app/controllers/hardware_controller.rb         |   20 ++++----
 src/app/controllers/host_controller.rb             |   55 --------------------
 src/app/controllers/network_controller.rb          |    9 ---
 src/app/controllers/nic_controller.rb              |   28 ----------
 src/app/controllers/permission_controller.rb       |    7 ---
 src/app/controllers/pool_controller.rb             |    7 ---
 src/app/controllers/quota_controller.rb            |    8 ---
 src/app/controllers/smart_pools_controller.rb      |    2 +-
 src/app/controllers/storage_controller.rb          |    8 ---
 src/app/controllers/storage_volume_controller.rb   |    8 ---
 src/app/controllers/vm_controller.rb               |   13 +----
 src/app/services/hardware_pool_service.rb          |    1 -
 src/app/services/pool_service.rb                   |    1 -
 src/app/services/smart_pool_service.rb             |    3 +-
 src/app/services/vm_resource_pool_service.rb       |    1 -
 src/app/services/vm_service.rb                     |   24 +++++---
 .../addhost.html.erb => hardware/addhost.rhtml}    |    8 ++--
 src/app/views/hardware/show_hosts.rhtml            |    4 +-
 19 files changed, 35 insertions(+), 218 deletions(-)
 rename src/app/views/{host/addhost.html.erb => hardware/addhost.rhtml} (76%)

diff --git a/src/app/controllers/application.rb
b/src/app/controllers/application.rb
index e9c515f..040b8a3 100644
--- a/src/app/controllers/application.rb
+++ b/src/app/controllers/application.rb
@@ -30,19 +30,6 @@ class ApplicationController < ActionController::Base
   init_gettext "ovirt"
   layout :choose_layout
 
-  # FIXME: once service layer is complete, the following before_filters will be
-  # removed as their functionality has been moved to the service layer
-  # pre_new
-  # pre_create
-  # pre_edit
-  # pre_show
-  # authorize_admin
-  before_filter :pre_new, :only => [:new]
-  before_filter :pre_create, :only => [:create]
-  # the following is to facilitate transition to service layer
-  before_filter :tmp_pre_update, :only => [:edit, :update, :destroy]
-  before_filter :pre_show, :only => [:show]
-  before_filter :tmp_authorize_admin, :only => [:new, :edit, :create,
:update, :destroy]
   before_filter :is_logged_in, :get_help_section
 
   # General error handlers, must be in order from least specific
@@ -83,39 +70,6 @@ class ApplicationController < ActionController::Base
   protected
   # permissions checking
 
-  def pre_new
-  end
-  def pre_edit
-  end
-
-  # FIXME: remove these when service layer transition is complete
-  def tmp_pre_update
-    pre_edit
-  end
-  def tmp_authorize_admin
-    authorize_admin
-  end
-  def pre_create
-  end
-  def pre_show
-  end
-
-  # These authorize_XXX methods should go away once we're fully converted
to
-  # the service layer
-  def authorize_view
-    authorize_action(Privilege::VIEW)
-  end
-  def authorize_user
-    authorize_action(Privilege::VM_CONTROL)
-  end
-  def authorize_admin
-    authorize_action(Privilege::MODIFY)
-  end
-  def authorize_action(privilege)
-    authorized!(privilege)
-    true
-  end
-
   def handle_perm_error(error)
     handle_error(:error => error, :status => :forbidden,
                  :title => "Access denied")
diff --git a/src/app/controllers/hardware_controller.rb
b/src/app/controllers/hardware_controller.rb
index 56b6e08..4d93e2f 100644
--- a/src/app/controllers/hardware_controller.rb
+++ b/src/app/controllers/hardware_controller.rb
@@ -57,18 +57,13 @@ class HardwareController < PoolController
   end
 
   def json_view_tree
-    json_tree_internal(Privilege::VIEW, :select_hardware_and_vm_pools)
+    json_tree_internal(:svc_show, :select_hardware_and_vm_pools)
   end
   def json_move_tree
-    json_tree_internal(Privilege::MODIFY, :select_hardware_pools)
-  end
-  def json_tree_internal(privilege, filter_method)
-    id = params[:id]
-    if id
-      @pool = Pool.find(id)
-      set_perms(@pool)
-      return unless authorize_action(privilege)
-    end
+    json_tree_internal(:svc_modify, :select_hardware_pools)
+  end
+  def json_tree_internal(perm_method, filter_method)
+    self.send(perm_method, params[:id]) if params[:id]
     if @pool
       pools = @pool.children
       open_list = []
@@ -232,4 +227,9 @@ class HardwareController < PoolController
     render :layout => 'popup'
   end
 
+  def addhost
+    svc_modify(params[:id])
+    render :layout => 'popup'
+  end
+
 end
diff --git a/src/app/controllers/host_controller.rb
b/src/app/controllers/host_controller.rb
index 9b683fa..0d8a879 100644
--- a/src/app/controllers/host_controller.rb
+++ b/src/app/controllers/host_controller.rb
@@ -56,45 +56,11 @@ class HostController < ApplicationController
   def snapshot_graph
   end
 
-  def addhost
-    # FIXME: This probably should go into PoolService.svc_modify,
-    # so that we have permission checks in only one place
-
-    # Old pre_addhost
-    @pool = Pool.find(params[:hardware_pool_id])
-    @parent = @pool.parent
-    set_perms(@pool)
-    authorize_admin
-    # Old addhost
-    @hardware_pool = Pool.find(params[:hardware_pool_id])
-    render :layout => 'popup'
-  end
-
   def add_to_smart_pool
     @pool = SmartPool.find(params[:smart_pool_id])
     render :layout => 'popup'
   end
 
-  # FIXME: We implement the standard controller actions, but catch
-  # them in filters and kick out friendly warnings that you can't
-  # perform them on hosts. Tat's overkill - the only way for a user
-  # to get to these actions is with URL surgery or from a bug in the
-  # application, both of which deserve a noisy error
-  def new
-  end
-
-  def create
-  end
-
-  def edit
-  end
-
-  def update
-  end
-
-  def destroy
-  end
-
   def host_action
     action = params[:action_type]
     if["disable", "enable",
"clear_vms"].include?(action)
@@ -144,25 +110,4 @@ class HostController < ApplicationController
     render :json => bondings.collect{ |x| {:id => x.id, :name =>
x.name} }
   end
 
-  private
-  #filter methods
-  def pre_new
-    flash[:notice] = 'Hosts may not be edited via the web UI'
-    redirect_to :controller => 'hardware', :action =>
'show', :id => params[:hardware_pool_id]
-  end
-  def pre_create
-    flash[:notice] = 'Hosts may not be edited via the web UI'
-    redirect_to :controller => 'dashboard'
-  end
-  def pre_edit
-    @host = Host.find(params[:id])
-    flash[:notice] = 'Hosts may not be edited via the web UI'
-    redirect_to :action=> 'show', :id => @host
-  end
-  def pre_show
-    @host = Host.find(params[:id])
-    set_perms(@host.hardware_pool)
-  end
-
-
 end
diff --git a/src/app/controllers/network_controller.rb
b/src/app/controllers/network_controller.rb
index 5af0a73..9066724 100644
--- a/src/app/controllers/network_controller.rb
+++ b/src/app/controllers/network_controller.rb
@@ -175,13 +175,4 @@ class NetworkController < ApplicationController
     alert = svc_destroy_bonding(params[:id])
     render :json => {:object => "bonding", :success => true,
:alert => alert}
   end
-
-  protected
-  # 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/nic_controller.rb
b/src/app/controllers/nic_controller.rb
index 8387f1c..2f6a2e5 100644
--- a/src/app/controllers/nic_controller.rb
+++ b/src/app/controllers/nic_controller.rb
@@ -26,32 +26,4 @@ class NicController < ApplicationController
   def show
     svc_show(params[:id])
   end
-
-  def new
-    raise ActionError.new("Network Interfaces may not be edited via the
web UI")
-  end
-
-  def create
-    raise ActionError.new("Network Interfaces may not be edited via the
web UI")
-  end
-
-  def edit
-    raise ActionError.new("Network Interfaces may not be edited via the
web UI")
-  end
-
-  def update
-    raise ActionError.new("Network Interfaces may not be edited via the
web UI")
-  end
-
-  def destroy
-    raise ActionError.new("Network Interfaces may not be edited via the
web UI")
-  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/permission_controller.rb
b/src/app/controllers/permission_controller.rb
index 55e7942..4447e2b 100644
--- a/src/app/controllers/permission_controller.rb
+++ b/src/app/controllers/permission_controller.rb
@@ -91,11 +91,4 @@ class PermissionController < ApplicationController
     alert = svc_destroy(params[:id])
     render :json => { :object => "vm", :success => true,
:alert => alert  }
   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/pool_controller.rb
b/src/app/controllers/pool_controller.rb
index 5c5949a..1a65718 100644
--- a/src/app/controllers/pool_controller.rb
+++ b/src/app/controllers/pool_controller.rb
@@ -166,11 +166,4 @@ class PoolController < ApplicationController
   def get_parent_id
     params[:parent_id]
   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/quota_controller.rb
b/src/app/controllers/quota_controller.rb
index fcdd672..07c6121 100644
--- a/src/app/controllers/quota_controller.rb
+++ b/src/app/controllers/quota_controller.rb
@@ -51,12 +51,4 @@ class QuotaController < ApplicationController
     alert = svc_destroy(params[:id])
     render :json => { :object => "quota", :success => true,
:alert => alert }
   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/smart_pools_controller.rb
b/src/app/controllers/smart_pools_controller.rb
index 83e72b5..8762ac0 100644
--- a/src/app/controllers/smart_pools_controller.rb
+++ b/src/app/controllers/smart_pools_controller.rb
@@ -142,7 +142,7 @@ class SmartPoolsController < PoolController
     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
+      class_and_id
     end
 
     alert = svc_add_remove_items(params[:id], nil, :add, class_and_ids)
diff --git a/src/app/controllers/storage_controller.rb
b/src/app/controllers/storage_controller.rb
index 4cc4953..dac2a6b 100644
--- a/src/app/controllers/storage_controller.rb
+++ b/src/app/controllers/storage_controller.rb
@@ -157,12 +157,4 @@ class StorageController < ApplicationController
       format.xml { head(:ok) }
     end
   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/storage_volume_controller.rb
b/src/app/controllers/storage_volume_controller.rb
index b87f9b6..b89513a 100644
--- a/src/app/controllers/storage_volume_controller.rb
+++ b/src/app/controllers/storage_volume_controller.rb
@@ -66,12 +66,4 @@ class StorageVolumeController < ApplicationController
       format.xml { head(:ok) }
     end
   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/vm_controller.rb
b/src/app/controllers/vm_controller.rb
index b51f4ae..b2965af 100644
--- a/src/app/controllers/vm_controller.rb
+++ b/src/app/controllers/vm_controller.rb
@@ -135,10 +135,7 @@ class VmController < ApplicationController
   end
 
   def migrate
-    @vm = Vm.find(params[:id])
-    @current_pool_id=@vm.vm_resource_pool.id
-    set_perms(@vm.get_hardware_pool)
-    authorize_admin
+    svc_get_for_migrate(params[:id])
     render :layout => 'popup'
   end
 
@@ -173,12 +170,4 @@ class VmController < ApplicationController
       #if cobbler doesn't respond/is misconfigured/etc just don't add
profiles
     end
   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/services/hardware_pool_service.rb
b/src/app/services/hardware_pool_service.rb
index 28040f3..8b53515 100644
--- a/src/app/services/hardware_pool_service.rb
+++ b/src/app/services/hardware_pool_service.rb
@@ -90,7 +90,6 @@ module HardwarePoolService
     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
     target_pool = Pool.find(target_pool_id)
     authorized!(Privilege::MODIFY,target_pool)
     lookup(pool_id, Privilege::MODIFY)
diff --git a/src/app/services/pool_service.rb b/src/app/services/pool_service.rb
index c05b0dc..02c63f5 100644
--- a/src/app/services/pool_service.rb
+++ b/src/app/services/pool_service.rb
@@ -55,7 +55,6 @@ module PoolService
   # === Required permissions
   # [<tt>Privilege::MODIFY</tt>] for the pool
   def svc_update(id, pool_hash)
-    # from before_filter
     svc_modify(id)
     Pool.transaction do
       additional_update_actions(@pool, pool_hash)
diff --git a/src/app/services/smart_pool_service.rb
b/src/app/services/smart_pool_service.rb
index 67adfe0..28bdaf3 100644
--- a/src/app/services/smart_pool_service.rb
+++ b/src/app/services/smart_pool_service.rb
@@ -96,7 +96,8 @@ module SmartPoolService
       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."
+    return "#{item_action.to_s} #{item_class.nil? ? 'items' :
+                                  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
index 00cf982..8930a4b 100644
--- a/src/app/services/vm_resource_pool_service.rb
+++ b/src/app/services/vm_resource_pool_service.rb
@@ -73,7 +73,6 @@ module VmResourcePoolService
   # permission is action-specific as determined by
   #   <tt>VmTask.action_privilege(@action)</tt>
   def svc_vm_actions(pool_id, vm_action, vm_ids)
-    # from before_filter
     @pool = VmResourcePool.find(pool_id)
     @parent = @pool.parent
     @action = vm_action
diff --git a/src/app/services/vm_service.rb b/src/app/services/vm_service.rb
index fb58b51..4c29bcf 100644
--- a/src/app/services/vm_service.rb
+++ b/src/app/services/vm_service.rb
@@ -71,7 +71,6 @@ module VmService
   # === Required permissions
   # [<tt>Privilege::MODIFY</tt>] for the vm's VmResourcePool
   def svc_create(vm_hash, start_now)
-    # from before_filter
     vm_hash[:state] = Vm::STATE_PENDING
     @vm = Vm.new(vm_hash)
     authorized!(Privilege::MODIFY, at vm.vm_resource_pool)
@@ -106,9 +105,7 @@ module VmService
   # === Required permissions
   # [<tt>Privilege::MODIFY</tt>] for the Vm's VmResourcePool
   def svc_update(id, vm_hash, start_now, restart_now)
-    # from before_filter
-    @vm = Vm.find(id)
-    authorized!(Privilege::MODIFY, @vm.vm_resource_pool)
+    lookup(id,Privilege::MODIFY)
 
     #needs restart if certain fields are changed
     # (since those will only take effect the next startup)
@@ -169,9 +166,7 @@ module VmService
   # === Required permissions
   # [<tt>Privilege::MODIFY</tt>] for the Vm's VmResourcePool
   def svc_destroy(id)
-    # from before_filter
-    @vm = Vm.find(id)
-    authorized!(Privilege::MODIFY, @vm.vm_resource_pool)
+    lookup(id,Privilege::MODIFY)
 
     unless @vm.is_destroyable?
       raise ActionError.new("Virtual Machine must be stopped to delete
it")
@@ -205,8 +200,7 @@ module VmService
   # === Required permissions
   # [<tt>Privilege::MODIFY</tt>] for the Vm's VmResourcePool
   def svc_cancel_queued_tasks(id)
-    @vm = Vm.find(id)
-    authorized!(Privilege::MODIFY, @vm.vm_resource_pool)
+    lookup(id,Privilege::MODIFY)
 
     Task.transaction do
       @vm.tasks.queued.each { |task| task.cancel}
@@ -214,6 +208,18 @@ module VmService
     return "Queued tasks were successfully canceled."
   end
 
+  #  Retrieves the Vm with id +id+ and checks permissions for migrate
+  #
+  # === Instance variables
+  # [<tt>@vm</tt>] stores the Vm with +id+
+  # === Required permissions
+  # [<tt>Privilege::MODIFY</tt>] for the Vm's HardwarePool
+  def svc_get_for_migrate(id)
+    @vm = Vm.find(id)
+    @current_pool_id=@vm.vm_resource_pool.id
+    authorized!(Privilege::MODIFY, @vm.get_hardware_pool)
+  end
+
   protected
   def vm_provision
     if @vm.uses_cobbler?
diff --git a/src/app/views/host/addhost.html.erb
b/src/app/views/hardware/addhost.rhtml
similarity index 76%
rename from src/app/views/host/addhost.html.erb
rename to src/app/views/hardware/addhost.rhtml
index 967643e..7279731 100644
--- a/src/app/views/host/addhost.html.erb
+++ b/src/app/views/hardware/addhost.rhtml
@@ -2,14 +2,14 @@
   <%= _("Add Host") %>
 <%- end -%>
 <%- content_for :description do -%>
-  Select hosts from the list below to add to the <%= @hardware_pool.name
%> hardware pool.  <a href="#">Learn how to manage
hosts</a>
+  Select hosts from the list below to add to the <%= @pool.name %>
hardware pool.  <a href="#">Learn how to manage hosts</a>
 <%- end -%>
 <div id="dialog-content-area">
 <div class="dialog_body_small">
 <div class="panel_header"></div>
   <%= render :partial => "/host/grid", :locals => {
:table_id => "addhosts_grid",
              :hwpool => nil,
-             :exclude_pool => @hardware_pool.id,
+             :exclude_pool => @pool.id,
              :exclude_host => nil,
              :checkboxes => true,
              :on_select => "load_widget_select",
@@ -20,8 +20,8 @@
              :hosts_per_page => 10} %>
 </div>
 
-<%= popup_footer("add_hosts('#{url_for :controller =>
"hardware",
+<%= popup_footer("add_hosts('#{url_for :controller =>
"hardware",
                                       :action => "add_hosts", 
-                                      :id => @hardware_pool}')", 
+                                      :id => @pool}')",
                  "Add Hosts") %>
 </div>
diff --git a/src/app/views/hardware/show_hosts.rhtml
b/src/app/views/hardware/show_hosts.rhtml
index 09a6188..fb54373 100644
--- a/src/app/views/hardware/show_hosts.rhtml
+++ b/src/app/views/hardware/show_hosts.rhtml
@@ -1,7 +1,7 @@
 <div id="toolbar_nav">
  <ul>
     <%if @can_modify -%>
-      <li><a href="<%= url_for :controller =>
'host', :action => 'addhost', :hardware_pool_id => @pool
%>" rel="facebox[.bolder]"><%= image_tag
"icon_add_host.png", :style=>"vertical-align:middle;"
%>&nbsp;&nbsp;Add Host</a></li>
+      <li><a href="<%= url_for :controller =>
'hardware', :action => 'addhost', :id => @pool %>"
rel="facebox[.bolder]"><%= image_tag
"icon_add_host.png", :style=>"vertical-align:middle;"
%>&nbsp;&nbsp;Add Host</a></li>
       <li>
         <a id="move_link" href="#" onClick="return
validate_for_move();"><%= image_tag "icon_move.png",
:style=>"vertical-align:middle;"
%>&nbsp;&nbsp;Move</a>
         <a id="move_link_hidden" href="<%= url_for
:controller => 'hardware', :action => 'move', :id =>
@pool, :resource_type=>'hosts' %>"
rel="facebox[.bolder]" style="display:none" ></a>
@@ -115,7 +115,7 @@
             No hosts found in this pool. <br/><br/>
             <%if @can_modify -%>
               <%= image_tag "icon_add_host.png",
:style=>"vertical-align:middle;" %>&nbsp;&nbsp;
-              <a href="<%= url_for :controller => 'host',
:action => 'addhost', :hardware_pool_id => @pool %>"
rel="facebox[.bolder]">Add first host to this hardware
pool</a>
+              <a href="<%= url_for :controller =>
'hardware', :action => 'addhost', :id => @pool %>"
rel="facebox[.bolder]">Add first host to this hardware
pool</a>
             <% end -%>
           </div>
        </div>
-- 
1.6.0.6