[PATCH conductor] BZ 812740 - check if a provider account has associated images before destroy
Jan Provazník
jprovazn at redhat.com
Mon Sep 3 07:37:15 UTC 2012
On 09/03/2012 09:32 AM, jprovazn at redhat.com wrote:
> 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 +
> 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
>
More information about the aeolus-devel
mailing list