Ovirt devel - May 2009 - re-sending outstanding controller refactoring patches after rebase

I've rebased the patch series to the current next branch and am sending them
again.

Signed-off-by: Scott Seago <sseago at redhat.com>
---
 src/app/controllers/storage_volume_controller.rb |  164 +++++-----------------
 src/app/models/lvm_storage_volume.rb             |    8 +
 src/app/services/storage_volume_service.rb       |  145 +++++++++++++++++++
 3 files changed, 187 insertions(+), 130 deletions(-)
 create mode 100644 src/app/services/storage_volume_service.rb

diff --git a/src/app/controllers/storage_volume_controller.rb
b/src/app/controllers/storage_volume_controller.rb
index 6bdbbdc..b87f9b6 100644
--- a/src/app/controllers/storage_volume_controller.rb
+++ b/src/app/controllers/storage_volume_controller.rb
@@ -18,156 +18,60 @@
 # also available at http://www.gnu.org/copyleft/gpl.html.
 
 class StorageVolumeController < ApplicationController
+  include StorageVolumeService
 
   def new
+    svc_new(params[:storage_pool_id], params[:source_volume_id])
     @return_to_workflow = params[:return_to_workflow] || false
-    if params[:storage_pool_id]
-      unless @storage_pool.user_subdividable
-        html_error_page("User-created storage volumes are not supported on
this pool")
-        return
-      end
-      @storage_volume = StorageVolume.factory(@storage_pool.get_type_label,
-                                              { :storage_pool_id =>
-                                                params[:storage_pool_id]})
-    else
-      unless @source_volume.supports_lvm_subdivision
-        html_error_page("LVM is not supported for this storage
volume")
-        return
-      end
-      lvm_pool = @source_volume.lvm_storage_pool
-      unless lvm_pool
-        # FIXME: what should we do about VG/LV names?
-        # for now auto-create VG name as ovirt_vg_#{@source_volume.id}
-        new_params = { :vg_name =>
"ovirt_vg_#{@source_volume.id}",
-          :hardware_pool_id => @source_volume.storage_pool.hardware_pool_id}
-        lvm_pool = StoragePool.factory(StoragePool::LVM, new_params)
-        lvm_pool.source_volumes << @source_volume
-        lvm_pool.save!
-      end
-      @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'
-    end
     render :layout => 'popup'
   end
 
   def create
-    begin
-      StorageVolume.transaction do
-        @storage_volume.save!
-        @task = StorageVolumeTask.new({ :user        => @user,
-                              :task_target => @storage_volume,
-                              :action      =>
StorageVolumeTask::ACTION_CREATE_VOLUME,
-                              :state       => Task::STATE_QUEUED})
-        @task.save!
-      end
-      respond_to do |format|
-        format.json { render :json => { :object =>
"storage_volume",
-            :success => true,
-            :alert => "Storage Volume was successfully created." ,
-            :new_volume =>
@storage_volume.storage_tree_element({:filter_unavailable => false, :state
=> 'new'})} }
-        format.xml { render :xml => @storage_volume,
-            :status => :created,
-            # FIXME: create storage_volume_url method if relevant
-            :location => storage_pool_url(@storage_volume)
-        }
-      end
-    rescue => ex
-      # FIXME: need to distinguish volume vs. task save errors
-      respond_to do |format|
-        format.json {
-          json_hash = { :object => "storage_volume", :success
=> false,
-            :errors => @storage_volume.errors.localize_error_messages.to_a 
}
-          json_hash[:message] = ex.message if json_hash[:errors].empty?
-          render :json => json_hash }
-        format.xml { render :xml => @storage_volume.errors,
-          :status => :unprocessable_entity }
-      end
+    volume = params[:storage_volume]
+    unless type = params[:storage_type]
+      type = volume.delete(:storage_type)
+    end
+    alert = svc_create(type, volume)
+    respond_to do |format|
+      format.json { render :json => { :object =>
"storage_volume",
+          :success => true, :alert => alert,
+          :new_volume => @storage_volume.storage_tree_element(
+                                {:filter_unavailable => false, :state =>
'new'})} }
+      format.xml { render :xml => @storage_volume,
+        :status => :created,
+        :location => storage_pool_url(@storage_volume)
+      }
     end
   end
 
   def show
-    @storage_volume = StorageVolume.find(params[:id])
-    set_perms(@storage_volume.storage_pool.hardware_pool)
-    @storage_pool = @storage_volume.storage_pool
-    if authorize_view
-      respond_to do |format|
-        format.html { render :layout => 'selection' }
-        format.json do
-          attr_list = []
-          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
-        format.xml { render :xml => @storage_volume.to_xml }
+    svc_show(params[:id])
+    respond_to do |format|
+      format.html { render :layout => 'selection' }
+      format.json do
+        attr_list = []
+        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
+      format.xml { render :xml => @storage_volume.to_xml }
     end
   end
 
   def destroy
-    unless authorized?(Privilege::MODIFY) and
@storage_volume.storage_pool.user_subdividable
-      handle_error(:message =>
-                   "You do not have permission to delete this storage
volume.",
-                   :status => :forbidden,
-                   :title => "Access Denied")
-    else
-      alert, success = delete_volume_internal(@storage_volume)
-      respond_to do |format|
-        format.json { render :json => { :object =>
"storage_volume",
-            :success => success, :alert => alert } }
-        format.xml { head(success ? :ok : :method_not_allowed) }
-      end
+    alert = svc_destroy(params[:id])
+    respond_to do |format|
+      format.json { render :json => { :object =>
"storage_volume",
+          :success => true, :alert => alert } }
+      format.xml { head(:ok) }
     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
+  # 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 pre_create
-    volume = params[:storage_volume]
-    unless type = params[:storage_type]
-      type = volume.delete(:storage_type)
-    end
-    @storage_volume = StorageVolume.factory(type, volume)
-    set_perms(@storage_volume.storage_pool.hardware_pool)
-    authorize_admin
-  end
-  # will go away w/ svc layer
-  def pre_edit
-    @storage_volume = StorageVolume.find(params[:id])
-    set_perms(@storage_volume.storage_pool.hardware_pool)
-  end
-
-  private
-  def delete_volume_internal(volume)
-    begin
-      name = volume.display_name
-      if !volume.vms.empty?
-        vm_list = volume.vms.collect {|vm| vm.description}.join(", ")
-        ["Storage Volume #{name} must be unattached from VMs (#{vm_list})
before deleting it.",
-         false]
-      else
-        volume.state=StorageVolume::STATE_PENDING_DELETION
-        volume.save!
-        @task = StorageVolumeTask.new({ :user        => @user,
-                              :task_target => volume,
-                              :action      =>
StorageVolumeTask::ACTION_DELETE_VOLUME,
-                              :state       => Task::STATE_QUEUED})
-        @task.save!
-        ["Storage Volume #{name} deletion was successfully queued.",
true]
-      end
-    rescue => ex
-      ["Failed to delete storage volume #{name}
(#{ex.message}.",false]
-    end
+  def tmp_authorize_admin
   end
 
 end
diff --git a/src/app/models/lvm_storage_volume.rb
b/src/app/models/lvm_storage_volume.rb
index 5b6177b..8fd1bac 100644
--- a/src/app/models/lvm_storage_volume.rb
+++ b/src/app/models/lvm_storage_volume.rb
@@ -18,6 +18,14 @@
 # also available at http://www.gnu.org/copyleft/gpl.html.
 
 class LvmStorageVolume < StorageVolume
+
+  def initialize(params)
+    super
+    self.lv_owner_perms='0744' unless self.lv_owner_perms
+    self.lv_group_perms='0744' unless self.lv_group_perms
+    self.lv_mode_perms='0744' unless self.lv_mode_perms
+  end
+
   def display_name
     "#{get_type_label}: #{storage_pool.vg_name}:#{lv_name}"
   end
diff --git a/src/app/services/storage_volume_service.rb
b/src/app/services/storage_volume_service.rb
new file mode 100644
index 0000000..6d338a5
--- /dev/null
+++ b/src/app/services/storage_volume_service.rb
@@ -0,0 +1,145 @@
+#
+# Copyright (C) 2009 Red Hat, Inc.
+# Written by Scott Seago <sseago 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 storage volumes
+module StorageVolumeService
+
+  include ApplicationService
+
+  # Load the StorageVolume with +id+ for viewing
+  #
+  # === Instance variables
+  # [<tt>@storage_volume</tt>] stores the Storage Volume with +id+
+  # [<tt>@storage_pool</tt>] stores the Storage Volume's
Storage Pool
+  # === Required permissions
+  # [<tt>Privilege::VIEW</tt>] on StorageVolume's HardwarePool
+  def svc_show(id)
+    lookup(id,Privilege::VIEW)
+  end
+
+  # Load the StorageVolume with +id+ for editing
+  #
+  # === Instance variables
+  # [<tt>@storage_volume</tt>] stores the Storage Volume with +id+
+  # [<tt>@storage_pool</tt>] stores the Storage Volume's
Storage Pool
+  # === Required permissions
+  # [<tt>Privilege::MODIFY</tt>] on StorageVolume's
HardwarePool
+  def svc_modify(id)
+    lookup(id,Privilege::MODIFY)
+  end
+
+  # Load a new StorageVolume for creating
+  #
+  # === Instance variables
+  # [<tt>@storage_volume</tt>] loads a new StorageVolume object
into memory
+  # [<tt>@storage_pool</tt>] Storage pool containing
<tt>@storage_volume</tt>
+  # [<tt>@source_volume</tt>] Storage volume containing the LVM
+  #                           <tt>@storage_pool</tt> if storage
type is LVM
+  # === Required permissions
+  # [<tt>Privilege::MODIFY</tt>] for the storage volume's
HardwarePool
+  def svc_new(storage_pool_id, source_volume_id)
+    if storage_pool_id
+      @storage_pool = StoragePool.find(storage_pool_id)
+      unless @storage_pool.user_subdividable
+        raise ActionError.new("Unsupported action for " +
+                              "#{@storage_pool.get_type_label}
volumes.")
+      end
+    else
+      @source_volume = StorageVolume.find(source_volume_id)
+      @storage_pool = @source_volume.storage_pool
+      unless @source_volume.supports_lvm_subdivision
+        raise ActionError.new("LVM is not supported for this storage
volume")
+      end
+    end
+    authorized!(Privilege::MODIFY, at storage_pool.hardware_pool)
+
+    if source_volume_id
+      @storage_pool = @source_volume.lvm_storage_pool
+      unless @storage_pool
+        # FIXME: what should we do about VG/LV names?
+        # for now auto-create VG name as ovirt_vg_#{@source_volume.id}
+        new_params = { :vg_name =>
"ovirt_vg_#{@source_volume.id}",
+          :hardware_pool_id => @source_volume.storage_pool.hardware_pool_id}
+        @storage_pool = StoragePool.factory(StoragePool::LVM, new_params)
+        @storage_pool.source_volumes << @source_volume
+        @storage_pool.save!
+      end
+    end
+    @storage_volume = StorageVolume.factory(@storage_pool.get_type_label,
+                                            { :storage_pool_id =>
+                                              @storage_pool.id})
+  end
+
+  # Create a new StorageVolume
+  #
+  # === Instance variables
+  # [<tt>@storage_volume</tt>] the newly-created StorageVolume
+  # === Required permissions
+  # [<tt>Privilege::MODIFY</tt>] for the storage volume's
HardwarePool
+  def svc_create(storage_type, storage_volume_hash)
+    @storage_volume = StorageVolume.factory(storage_type, storage_volume_hash)
+    authorized!(Privilege::MODIFY, at
storage_volume.storage_pool.hardware_pool)
+    StorageVolume.transaction do
+      @storage_volume.save!
+      @task = StorageVolumeTask.new({ :user        => @user,
+                                      :task_target => @storage_volume,
+                      :action      =>
StorageVolumeTask::ACTION_CREATE_VOLUME,
+                                      :state       => Task::STATE_QUEUED})
+        @task.save!
+    end
+    return "Storage Volume was successfully created."
+  end
+
+  # Queues StorageVolume with +id+ for deletion
+  #
+  # === Instance variables
+  # [<tt>@storage_volume</tt>] stores the Storage Volume with +id+
+  # [<tt>@storage_pool</tt>] stores the Storage Volume's
Storage Pool
+  # === Required permissions
+  # [<tt>Privilege::MODIFY</tt>] on StorageVolume's
HardwarePool
+  def svc_destroy(id)
+    lookup(id, Privilege::MODIFY)
+    unless @storage_pool.user_subdividable
+      raise ActionError.new("Unsupported action for " +
+                            "#{@storage_volume.get_type_label}
volumes.")
+    end
+    unless @storage_volume.vms.empty?
+      vms = @storage_volume.vms.collect {|vm| vm.description}.join(",
")
+      raise ActionError.new("Cannot delete storage assigned to VMs
(#{vms})")
+    end
+    name = @storage_volume.display_name
+    StorageVolume.transaction do
+      @storage_volume.state=StorageVolume::STATE_PENDING_DELETION
+      @storage_volume.save!
+      @task = StorageVolumeTask.new({ :user        => @user,
+                                      :task_target => @storage_volume,
+                       :action      =>
StorageVolumeTask::ACTION_DELETE_VOLUME,
+                                      :state       => Task::STATE_QUEUED})
+      @task.save!
+    end
+    return "Storage Volume #{name} deletion was successfully queued."
+  end
+
+  private
+  def lookup(id, priv)
+    @storage_volume = StorageVolume.find(id)
+    @storage_pool = @storage_volume.storage_pool
+    authorized!(priv, at storage_pool.hardware_pool)
+  end
+
+end
-- 
1.6.0.6