From: Jan Provaznik <jprovazn(a)redhat.com&gt; 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