From: Jan Provaznik <jprovazn(a)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((a)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 {(a)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?((a)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?((a)provider_account).should be_true
@provider_account.should be_frozen
end
--
1.7.11.2