[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