From: Jan Provaznik <[email protected]>
Environment or provider account can't be deleted if there is any existing
image (or provider image) associated with them.
This required a little bit more refactoring. All destroy 'validations'
are done in before_destroy callbacks. If some callback fails, an exception
is raised so destroy is aborted and reason is in the exception message.
To avoid having rescue blocks in all controllers, new 'safe_destroy'
ActiveRecord
method is defined, this method adds exception into object's errors and returns
true/false.
---
src/app/controllers/pool_families_controller.rb | 42 ++++++-----------
.../controllers/provider_accounts_controller.rb | 55 ++++++++++++----------
src/app/controllers/providers_controller.rb | 30 +++++++-----
src/app/models/pool_family.rb | 41 ++++++++++++++--
src/app/models/provider.rb | 16 -------
src/app/models/provider_account.rb | 31 ++++++++++--
src/config/initializers/destroy_extension.rb | 14 ++++++
src/config/locales/en.yml | 19 ++++----
src/lib/exceptions.rb | 1 +
.../provider_accounts_controller_spec.rb | 4 +-
src/spec/models/provider_account_spec.rb | 10 ++--
11 files changed, 159 insertions(+), 104 deletions(-)
create mode 100644 src/config/initializers/destroy_extension.rb
diff --git a/src/app/controllers/pool_families_controller.rb
b/src/app/controllers/pool_families_controller.rb
index 0ec70ed..585e92d 100644
--- a/src/app/controllers/pool_families_controller.rb
+++ b/src/app/controllers/pool_families_controller.rb
@@ -77,7 +77,7 @@ class PoolFamiliesController < ApplicationController
@title = @pool_family.name
save_breadcrumb(pool_family_path(@pool_family), @pool_family.name)
require_privilege(Privilege::VIEW, @pool_family)
- @all_images =
Aeolus::Image::Warehouse::Image.by_environment(@pool_family.name)
+ @all_images = @pool_family.images
@images = paginate_collection(@all_images, params[:page], PER_PAGE)
load_pool_family_tabs
@@ -96,15 +96,20 @@ class PoolFamiliesController < ApplicationController
def destroy
pool_family = PoolFamily.find(params[:id])
require_privilege(Privilege::MODIFY, pool_family)
- if pool_family == PoolFamily.default
- flash[:error] =
t("pool_families.flash.error.default_pool_family_not_deleted")
- elsif pool_family.destroy
- flash[:notice] = t "pool_families.flash.notice.deleted"
- else
- flash[:error] = t "pool_families.flash.error.not_deleted"
- end
- redirect_to pool_families_path
+ respond_to do |format|
+ if pool_family.safe_destroy
+ format.html do
+ flash[:notice] = t("pool_families.flash.notice.deleted")
+ redirect_to pool_families_path
+ end
+ else
+ format.html do
+ flash[:error] = pool_family.errors.full_messages
+ redirect_to pool_families_path
+ end
+ end
+ end
end
def add_provider_accounts
@@ -150,25 +155,6 @@ class PoolFamiliesController < ApplicationController
end
end
- def multi_destroy
- deleted = []
- not_deleted = []
- PoolFamily.find(params[:pool_family_selected]).each do |pool_family|
- if check_privilege(Privilege::MODIFY, pool_family) && pool_family.destroy
- deleted << pool_family.name
- else
- not_deleted << pool_family.name
- end
- end
- if deleted.size > 0
- flash[:notice] = t 'pool_families.flash.notice.more_deleted', :list =>
deleted.join(', ')
- end
- if not_deleted.size > 0
- flash[:error] = t 'pool_families.flash.error.more_not_deleted', :list =>
not_deleted.join(', ')
- end
- redirect_to pool_families_path
- end
-
def remove_provider_accounts
@pool_family = PoolFamily.find(params[:id])
require_privilege(Privilege::MODIFY, @pool_family)
diff --git a/src/app/controllers/provider_accounts_controller.rb
b/src/app/controllers/provider_accounts_controller.rb
index b73e57f..1abc022 100644
--- a/src/app/controllers/provider_accounts_controller.rb
+++ b/src/app/controllers/provider_accounts_controller.rb
@@ -170,26 +170,27 @@ class ProviderAccountsController < ApplicationController
def destroy
@provider_account = ProviderAccount.find(params[:id])
require_privilege(Privilege::MODIFY, @provider_account)
- is_deleted = @provider_account && @provider_account.destroy
respond_to do |format|
- format.html {
- if is_deleted
- flash[:notice] = t"provider_accounts.flash.notice.deleted"
- else
- flash[:error] = t"provider_accounts.flash.error.not_deleted"
+ if @provider_account.safe_destroy
+ format.html do
+ flash[:notice] = t("provider_accounts.flash.notice.deleted")
+ redirect_to edit_provider_path(@provider_account.provider,
+ :details_tab => 'accounts')
end
-
- @provider = Provider.find(params[:provider_id])
- redirect_to edit_provider_path(@provider, :details_tab => 'accounts')
- }
- format.xml {
- if is_deleted
+ format.xml do
render 'destroy', :locals => { :provider_account_id =>
@provider_account.id }
- else
- raise(Aeolus::Conductor::API::ProviderAccountNotFound.new(404,
t("api.error_messages.provider_account_not_found_for_id", :id => params[:id])))
end
- }
+ else
+ format.html do
+ flash[:error] = @provider_account.errors.full_messages
+ redirect_to edit_provider_path(@provider_account.provider,
+ :details_tab => 'accounts')
+ end
+ format.xml do
+ raise Aeolus::Conductor::API::Error.new(500,
@provider_account.errors.full_messages.join(', '))
+ end
+ end
end
end
@@ -202,21 +203,25 @@ 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
+ ProviderAccount.find(params[:accounts_selected]).each do |account|
+ if !check_privilege(Privilege::MODIFY, account)
+ failed << t('application_controller.permission_denied_with_prefix',
+ :prefix => account.name)
+ elsif account.safe_destroy
+ succeeded << account.name
else
- failed << account.label
+ failed << t('provider_accounts.flash.error.not_deleted_with_err',
+ :account => account.name,
+ :err => account.errors.full_messages.join(', '))
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..3b6ec23 100644
--- a/src/app/controllers/providers_controller.rb
+++ b/src/app/controllers/providers_controller.rb
@@ -184,20 +184,26 @@ 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
- }
- end
respond_to do |format|
- format.html { redirect_to providers_path }
- # FIXME: what to return in body of response, if anything?
- format.xml { render :text => '<message>OK</message>', :status => 200 }
+ if provider.safe_destroy
+ session[:current_provider_id] = nil
+ format.html do
+ flash[:notice] = t("providers.flash.notice.deleted")
+ redirect_to providers_path
+ end
+ format.xml { render :text => '<message>OK</message>', :status => 200 }
+ else
+ format.html do
+ flash[:error] = t("providers.flash.error.not_deleted_with_err",
+ :err => provider.errors.full_messages.join(', '))
+ redirect_to providers_path
+ end
+ # FIXME: what to return in body of response, if anything?
+ format.xml do
+ raise(Aeolus::Conductor::API::Error.new(500, error))
+ end
+ end
end
end
diff --git a/src/app/models/pool_family.rb b/src/app/models/pool_family.rb
index f464723..a644651 100644
--- a/src/app/models/pool_family.rb
+++ b/src/app/models/pool_family.rb
@@ -36,6 +36,10 @@ class PoolFamily < ActiveRecord::Base
include ActionView::Helpers::NumberHelper
DEFAULT_POOL_FAMILY_KEY = "default_pool_family"
+ before_destroy :check_name!
+ before_destroy :check_pools!
+ before_destroy :check_images!
+
after_update :fix_iwhd_environment_tags
has_many :pools, :dependent => :destroy
@@ -59,7 +63,6 @@ class PoolFamily < ActiveRecord::Base
validates_uniqueness_of :name
validates_presence_of :quota
- before_destroy :destroyable?
def self.default
MetadataObject.lookup(DEFAULT_POOL_FAMILY_KEY)
@@ -82,9 +85,39 @@ class PoolFamily < ActiveRecord::Base
[Pool, Quota]
end
- def destroyable?
- # A PoolFamily is destroyable if its pools are destroyable and it is not
the default PoolFamily
- pools.all? {|p| p.destroyable? } && self != PoolFamily.default
+ def check_pools!
+ cant_destroy = pools.find_all {|p| !p.destroyable?}
+ if cant_destroy.empty?
+ true
+ else
+ raise Aeolus::Conductor::Base::NotDestroyable,
+ I18n.t('pool_families.errors.not_destroyable_pools',
+ :list => cant_destroy.map {|p| p.name}.join(', '))
+ end
+ end
+
+ def check_name!
+ if self == PoolFamily.default
+ raise Aeolus::Conductor::Base::NotDestroyable,
+ I18n.t('pool_families.errors.default_pool_family_not_deleted')
+ else
+ true
+ end
+ end
+
+ def check_images!
+ referenced_images = images
+ if referenced_images.empty?
+ true
+ else
+ raise Aeolus::Conductor::Base::NotDestroyable,
+ I18n.t('pool_families.errors.associated_images',
+ :list => referenced_images.map {|i| i.name}.join(', '))
+ end
+ end
+
+ def images
+ Aeolus::Image::Warehouse::Image.by_environment(self.name)
end
def all_providers_disabled?
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..f23b286 100644
--- a/src/app/models/provider_account.rb
+++ b/src/app/models/provider_account.rb
@@ -39,6 +39,9 @@ class ProviderAccount < ActiveRecord::Base
end
include PermissionedObject
+ before_destroy :check_destroyable_instances!
+ before_destroy :check_provider_images!
+
# Relations
belongs_to :provider
belongs_to :quota, :autosave => true, :dependent => :destroy
@@ -82,7 +85,6 @@ class ProviderAccount < ActiveRecord::Base
before_create :populate_profiles_and_validate
after_create :populate_realms_and_validate
- before_destroy :destroyable?
scope :enabled, lambda { where(:provider_id => Provider.enabled) }
@@ -125,8 +127,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.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/initializers/destroy_extension.rb
b/src/config/initializers/destroy_extension.rb
new file mode 100644
index 0000000..e3d9f5d
--- /dev/null
+++ b/src/config/initializers/destroy_extension.rb
@@ -0,0 +1,14 @@
+class ActiveRecord::Base
+ # this method is used in in controllers where we need to get
+ # message why some before_destroy callback failed
+ #
+ # I'm not entirely sure that it's ideal to use obj.errors to pass
+ # errors from before_destroy callbacks, but it allows us to avoid
+ # begin-rescue blocks in most of controllers
+ def safe_destroy
+ destroy
+ rescue Aeolus::Conductor::Base::NotDestroyable => ex
+ errors.add(:base, ex.message)
+ false
+ end
+end
diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
index 063c46d..da62126 100644
--- a/src/config/locales/en.yml
+++ b/src/config/locales/en.yml
@@ -791,19 +791,21 @@ en:
deleted: "Environment was deleted."
provider_accounts_added: "These Provider Account have been added"
provider_accounts_removed: "These Provider Accounts were removed"
- more_deleted: "Deleted the following Environments: %{list}."
warning:
creation_failed: "Environment creation failed."
error:
not_updated: "Environment wasn't updated."
- not_deleted: "Environment cannot be deleted. Check that all Instances are
stopped."
- default_pool_family_not_deleted: "The default Environment cannot be
deleted."
- more_not_deleted: "Could not delete the following Environments:
%{list}."
+ not_deleted: "Environment cannot be deleted."
+ not_deleted_with_err: "Environment cannot be deleted: %{err}"
provider_accounts_not_added: "Could not add these Provider Accounts"
provider_accounts_not_removed: "Could not remove these Provider
Accounts"
select_to_add_accounts: "You must select at least one Provider Account to
add."
select_to_remove_accounts: "You must select at least one Provider Account
to remove."
no_provider_accounts: "There are no Provider Accounts available."
+ errors:
+ not_destroyable_pools: "Can not destroy following pools: %{list}."
+ default_pool_family_not_deleted: "The default Environment cannot be
deleted."
+ associated_images: "There are following associated images: %{list}. Delete
them first."
catalog_entries:
new_catalog_entry: New Deployable
index:
@@ -1184,6 +1186,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,10 +1296,7 @@ en:
one: "Account %{list} could not be added"
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: "Account %{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 +1308,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
@@ -1470,6 +1472,7 @@ en:
settings: Settings
application_controller:
permission_denied: You have insufficient privileges to perform the
selected action.
+ permission_denied_with_prefix: "%{prefix}: You have insufficient privileges to
perform the selected action."
access_denied: Access denied
some_actions_failed: Some actions failed
action_error: Action Error
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
