[PATCH conductor] BZ 812740 - check if a provider account has associated images before destroy (rev. 1)

jprovazn at redhat.com jprovazn at redhat.com
Mon Sep 3 09:41:23 UTC 2012


From: Jan Provaznik <jprovazn at redhat.com>

before_destroy callbacks now return an exception which implied refactoring of
provider and provider accounts destroy actions in controllers.
---
 .../controllers/provider_accounts_controller.rb    | 62 +++++++++++++---------
 src/app/controllers/providers_controller.rb        | 34 ++++++++----
 src/app/models/provider.rb                         | 16 ------
 src/app/models/provider_account.rb                 | 30 +++++++++--
 src/config/locales/en.yml                          |  7 +--
 src/lib/exceptions.rb                              |  1 +
 .../provider_accounts_controller_spec.rb           |  4 +-
 src/spec/models/provider_account_spec.rb           | 10 ++--
 8 files changed, 99 insertions(+), 65 deletions(-)

diff --git a/src/app/controllers/provider_accounts_controller.rb b/src/app/controllers/provider_accounts_controller.rb
index b73e57f..5e37a36 100644
--- a/src/app/controllers/provider_accounts_controller.rb
+++ b/src/app/controllers/provider_accounts_controller.rb
@@ -170,26 +170,33 @@ class ProviderAccountsController < ApplicationController
   def destroy
     @provider_account = ProviderAccount.find(params[:id])
     require_privilege(Privilege::MODIFY, @provider_account)
-    is_deleted = @provider_account && @provider_account.destroy
+    error = nil
+    begin
+      unless @provider_account.destroy
+        error = t("provider_accounts.flash.error.not_deleted")
+      end
+    rescue Aeolus::Conductor::Base::NotDestroyable => ex
+      error = t("provider_accounts.flash.error.not_deleted_with_err",
+                :err => ex.message)
+    end
 
     respond_to do |format|
-      format.html {
-        if is_deleted
-          flash[:notice] = t"provider_accounts.flash.notice.deleted"
+      format.html do
+        if error
+          flash[:error] = error
         else
-          flash[:error] = t"provider_accounts.flash.error.not_deleted"
+          flash[:notice] = t("provider_accounts.flash.notice.deleted")
         end
-
-        @provider = Provider.find(params[:provider_id])
-        redirect_to edit_provider_path(@provider, :details_tab => 'accounts')
-      }
-      format.xml {
-        if is_deleted
-          render 'destroy', :locals => { :provider_account_id => @provider_account.id }
+        redirect_to edit_provider_path(@provider_account.provider,
+                                       :details_tab => 'accounts')
+      end
+      format.xml do
+        if error
+          raise(Aeolus::Conductor::API::Error.new(500, error))
         else
-          raise(Aeolus::Conductor::API::ProviderAccountNotFound.new(404, t("api.error_messages.provider_account_not_found_for_id", :id => params[:id])))
+          render 'destroy', :locals => { :provider_account_id => @provider_account.id }
         end
-      }
+      end
     end
   end
 
@@ -202,21 +209,26 @@ class ProviderAccountsController < ApplicationController
 
     succeeded = []
     failed = []
-    @provider_accounts = ProviderAccount.find(params[:accounts_selected]).each do |account|
-      if check_privilege(Privilege::MODIFY, account) && account.destroyable?
-        account.destroy
-        succeeded << account.label
-      else
-        failed << account.label
+    ProviderAccount.find(params[:accounts_selected]).each do |account|
+      begin
+        require_privilege(Privilege::MODIFY, account)
+        if account.destroy
+          succeeded << account
+        else
+          failed << t('provider_accounts.flash.error.not_deleted')
+        end
+      rescue Aeolus::Conductor::Base::NotDestroyable => ex
+        failed << t('provider_accounts.flash.error.not_deleted_with_err',
+                    :err => ex.message)
       end
     end
 
-    unless succeeded.empty?
-      flash[:notice] = t 'provider_accounts.flash.notice.account_deleted', :count => succeeded.length, :list => succeeded.join(', ')
-    end
-    unless failed.empty?
-      flash[:error] = t 'provider_accounts.flash.error.account_not_deleted', :count => failed.length, :list => failed.join(', ')
+    if succeeded.present?
+      flash[:notice] = t('provider_accounts.flash.notice.account_deleted',
+                         :count => succeeded.length,
+                         :list => succeeded.join(', '))
     end
+    flash[:error] = failed if failed.present?
     redirect_to edit_provider_path(@provider, :details_tab => 'accounts')
   end
 
diff --git a/src/app/controllers/providers_controller.rb b/src/app/controllers/providers_controller.rb
index f3a9302..d036375 100644
--- a/src/app/controllers/providers_controller.rb
+++ b/src/app/controllers/providers_controller.rb
@@ -184,20 +184,34 @@ class ProvidersController < ApplicationController
   def destroy
     provider = Provider.find(params[:id])
     require_privilege(Privilege::MODIFY, provider)
-    if provider.destroy
-      session[:current_provider_id] = nil
-      flash[:notice] = t("providers.flash.notice.deleted")
-    else
-      flash[:error] = {
-        :summary => t("providers.flash.error.not_deleted"),
-        :failures => provider.errors.values.flatten
-      }
+    error = nil
+    begin
+      if provider.destroy
+        session[:current_provider_id] = nil
+      else
+        error = t("providers.flash.error.not_deleted")
+      end
+    rescue Aeolus::Conductor::Base::NotDestroyable => ex
+      error = t("providers.flash.error.not_deleted_with_err", :err => ex.message)
     end
 
     respond_to do |format|
-      format.html { redirect_to providers_path }
+      format.html do
+        if error
+          flash[:error] = error
+        else
+          flash[:notice] = t("providers.flash.notice.deleted")
+        end
+        redirect_to providers_path
+      end
       # FIXME: what to return in body of response, if anything?
-      format.xml { render :text => '<message>OK</message>', :status => 200 }
+      format.xml do
+        if error
+          raise(Aeolus::Conductor::API::Error.new(500, error))
+        else
+          render :text => '<message>OK</message>', :status => 200
+        end
+      end
     end
   end
 
diff --git a/src/app/models/provider.rb b/src/app/models/provider.rb
index 64ddb33..d1ae263 100644
--- a/src/app/models/provider.rb
+++ b/src/app/models/provider.rb
@@ -62,8 +62,6 @@ class Provider < ActiveRecord::Base
   has_many :provider_priority_group_elements, :as => :value, :dependent => :destroy
   has_many :provider_priority_groups, :through => :provider_priority_group_elements
 
-  before_destroy :destroyable?
-
   def derived_subtree(role = nil)
     subtree = super(role)
     subtree += provider_accounts if (role.nil? or role.privilege_target_match(ProviderAccount))
@@ -82,20 +80,6 @@ class Provider < ActiveRecord::Base
     end
     return url + url_extras
   end
-  # there is a destroy dependency for a cloud accounts association,
-  # but a cloud account is silently not destroyed when there is
-  # an instance for the cloud account
-  def destroyable?
-    unless self.provider_accounts.empty?
-      self.provider_accounts.each do |c|
-        unless c.instances.empty?
-          inst_list = c.instances.map {|i| i.name}.join(', ')
-          self.errors.add(:base, "there are instances for cloud account '#{c.name}': #{inst_list}")
-        end
-      end
-    end
-    return self.errors.empty?
-  end
 
   def connect
     begin
diff --git a/src/app/models/provider_account.rb b/src/app/models/provider_account.rb
index 0da9184..ed72d36 100644
--- a/src/app/models/provider_account.rb
+++ b/src/app/models/provider_account.rb
@@ -82,7 +82,8 @@ class ProviderAccount < ActiveRecord::Base
 
   before_create :populate_profiles_and_validate
   after_create :populate_realms_and_validate
-  before_destroy :destroyable?
+  before_destroy :check_destroyable_instances!
+  before_destroy :check_provider_images!
 
   scope :enabled, lambda { where(:provider_id => Provider.enabled) }
 
@@ -125,8 +126,31 @@ class ProviderAccount < ActiveRecord::Base
     [Quota]
   end
 
-  def destroyable?
-    instances.empty? || instances.all? { |i| i.destroyable? }
+  def provider_images
+    Aeolus::Image::Warehouse::ProviderImage.where(
+    "($provider == \"#{provider.name}\" && $provider_account_identifier == \"#{credentials_hash['username']}\")")
+  end
+
+  def check_provider_images!
+    imgs = provider_images.map {|pi| pi.target_image.build.image.os.name}
+    if imgs.empty?
+      true
+    else
+      raise Aeolus::Conductor::Base::NotDestroyable,
+        I18n.t('provider_accounts.errors.associated_images',
+               :images => imgs.join(', '))
+    end
+  end
+
+  def check_destroyable_instances!
+    not_destroyable_instances = instances.find_all {|i| !i.destroyable?}
+    if not_destroyable_instances.empty?
+      true
+    else
+      raise Aeolus::Conductor::Base::NotDestroyable,
+        I18n.t('provider_accounts.errors.not_destroyable_instances',
+               :instances => not_destroyable_instances.map{|i| i.name}.join(', '))
+    end
   end
 
   def connect
diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
index 063c46d..d86acfc 100644
--- a/src/config/locales/en.yml
+++ b/src/config/locales/en.yml
@@ -1184,6 +1184,7 @@ en:
         not_updated: "Cannot update the Provider."
         not_disabled: "Cannot disable the Provider."
         not_deleted: "Provider was not deleted"
+        not_deleted_with_err: "Provider was not deleted: %{err}"
       warning:
         connect_failed: "Failed to connect to Provider"
         not_stopped_instances: "Provider was not disabled. Failed to stop following instances:"
@@ -1294,9 +1295,7 @@ en:
           other: "Accounts %{list} could not be added"
         not_updated: "Provider Account wasn't updated"
         not_deleted: "Provider account was not deleted"
-        account_not_deleted:
-          one: "Account %{list} was not deleted. There are Instances associated with it."
-          other: "Accounts %{list} were not deleted. They have Instances associated with them."
+        not_deleted_with_err: "Provider account was not deleted: %{err}"
         test_connection_failed_invalid: "Test Connection Failed: Invalid Account Details"
         test_connection_failed_provider: "Test Connection Failed: Could not connect to Provider"
       warning:
@@ -1308,6 +1307,8 @@ en:
       populate_hardware_profiles_failed: "Failed to populate hardware_profiles: %{message}"
       populate_realms_failed: "Failed to populate Realms: %{message}"
       could_not_connect: "Could not connect to Provider Account.  Please contact an Administrator."
+      not_destroyable_instances: "Can not destroy following instances: %{instances}"
+      associated_images: "There are following associated provider images: %{images}. Delete them first."
   provider_priority_groups:
     index:
       add_new: Add new
diff --git a/src/lib/exceptions.rb b/src/lib/exceptions.rb
index 200aa3f..3076172 100644
--- a/src/lib/exceptions.rb
+++ b/src/lib/exceptions.rb
@@ -48,6 +48,7 @@ module Aeolus
       class ImageNotFound < StandardError; end
       class BlankImageId < StandardError; end
       class NotStoppableDeployment < StandardError; end
+      class NotDestroyable < StandardError; end
     end
 
     module MultiError
diff --git a/src/spec/controllers/provider_accounts_controller_spec.rb b/src/spec/controllers/provider_accounts_controller_spec.rb
index 35079b3..e693c2f 100644
--- a/src/spec/controllers/provider_accounts_controller_spec.rb
+++ b/src/spec/controllers/provider_accounts_controller_spec.rb
@@ -235,13 +235,13 @@ describe ProviderAccountsController do
           end
 
           it "when requested provider account doesn't exists" do
-            ProviderAccount.stub(:find).and_return(nil)
+            ProviderAccount.stub(:find).and_raise(ActiveRecord::RecordNotFound)
             get :destroy, :id => "id_that_does_not_exist"
             response.status.should be_eql(404)
             response.should have_content_type("application/xml")
             response.body.should be_xml
             subject = Nokogiri::XML(response.body)
-            subject.xpath('//error/code').text.strip.should == "ProviderAccountNotFound"
+            subject.xpath('//error/code').text.strip.should == "RecordNotFound"
           end
         end #destroy
 
diff --git a/src/spec/models/provider_account_spec.rb b/src/spec/models/provider_account_spec.rb
index 8820be9..5a49146 100644
--- a/src/spec/models/provider_account_spec.rb
+++ b/src/spec/models/provider_account_spec.rb
@@ -24,19 +24,17 @@ describe ProviderAccount do
 
   it "should not be destroyable if it has instance with status other than stopped" do
     @provider_account.instances << Instance.new
-    @provider_account.destroyable?.should be_false
-    @provider_account.destroy.should be_false
+    lambda {@provider_account.destroy}.should
+      raise_error(Aeolus::Conductor::Base::NotDestroyable)
     @provider_account.instances.each { |i| i.state = "stopped" }
-    @provider_account.destroyable?.should be_true
+    @provider_account.check_destroyable_instances!.should be_true
     @provider_account.instances.clear
-    @provider_account.destroyable?.should be_true
+    @provider_account.check_destroyable_instances!.should be_true
     @provider_account.destroy.equal?(@provider_account).should be_true
     @provider_account.should be_frozen
   end
 
   it "should be destroyable if it has a config server" do
-    @provider_account.config_server = ConfigServer.new
-    @provider_account.destroyable?.should be_true
     @provider_account.destroy.equal?(@provider_account).should be_true
     @provider_account.should be_frozen
   end
-- 
1.7.11.2




More information about the aeolus-devel mailing list