On 09/03/2012 09:32 AM, [email protected] wrote:
From: Jan Provaznik <[email protected]>
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 +
6 files changed, 93 insertions(+), 57 deletions(-)
Ignore this patch, pls.
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 833edaa..147b699 100644
--- a/src/config/locales/en.yml
+++ b/src/config/locales/en.yml
@@ -1183,6 +1183,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:"
@@ -1293,9 +1294,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:
@@ -1307,6 +1306,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
