[PATCH conductor] BZ 788018 Destroy deployables when deleting its last catalog

Matt Wagner matt.wagner at redhat.com
Wed Feb 8 15:29:30 UTC 2012


On Wed, Feb 08, 2012 at 03:21:50PM +0100, jtomasek at redhat.com wrote:
> From: Jiri Tomasek <jtomasek at redhat.com>
> 
> When a Catalog is deleted, all deployables related only to it are destroyed too.
> ---
>  src/app/models/catalog.rb                        |    8 ++++----
>  src/config/locales/en.yml                        |    2 +-
>  src/spec/controllers/catalogs_controller_spec.rb |   13 ++++++-------
>  3 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/src/app/models/catalog.rb b/src/app/models/catalog.rb
> index 00d1e7a..3472c59 100644
> --- a/src/app/models/catalog.rb
> +++ b/src/app/models/catalog.rb
> @@ -38,7 +38,7 @@ class Catalog < ActiveRecord::Base
>             :include => [:role],
>             :order => "permissions.id ASC"
>  
> -  before_destroy :check_deployable_references
> +  before_destroy :destroy_deployables_related_only_to_self
>  
>    validates_presence_of :pool
>    validates_presence_of :name
> @@ -48,9 +48,9 @@ class Catalog < ActiveRecord::Base
>    PRESET_FILTERS_OPTIONS = [
>      {:title => I18n.t("catalogs.preset_filters.belongs_to_default_pool"), :id => "belongs_to_default_pool", :query => includes(:pool).where("pools.name" => "Default")}
>    ]
> -  def check_deployable_references
> -    return true if deployables.empty?
> -    !(deployables.any? {|d| d.catalogs.count == 1})
> +
> +  def destroy_deployables_related_only_to_self
> +    deployables.each {|d| d.destroy if d.catalogs.count == 1}
>    end
>  
>    private
> diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
> index 048047b..971f0f8 100644
> --- a/src/config/locales/en.yml
> +++ b/src/config/locales/en.yml
> @@ -411,7 +411,7 @@ en:
>        owner: Owner
>        pending: Pending
>        failed: Failed
> -      confirm_delete: "Are you sure you want to delete this pool?"
> +      confirm_delete: "Are you sure you want to delete this pool? All related catalogs and deployables will be deleted too!"
>        overview: Overview
>        your_pools: Your Pools
>        pool_details: Pool details
> diff --git a/src/spec/controllers/catalogs_controller_spec.rb b/src/spec/controllers/catalogs_controller_spec.rb
> index 50a53bf..8e46aaf 100644
> --- a/src/spec/controllers/catalogs_controller_spec.rb
> +++ b/src/spec/controllers/catalogs_controller_spec.rb
> @@ -33,10 +33,9 @@ describe CatalogsController do
>          response.should redirect_to catalogs_path
>        end
>  
> -      it "cannot remove catalog2 because it is the last deployable's reference " do
> -        delete :destroy, :id => @catalog.id
> -        expect {delete :destroy, :id => @catalog2.id}.not_to change(Catalog, :count).by(-1)
> -        response.should redirect_to catalog_path(@catalog2)
> +      it "cannot remove deployable related to catalog because it is also related to catalog2" do
> +        expect {delete :destroy, :id => @catalog.id}.not_to change(Deployable, :count).by(-1)

I don't think the ".by(-1)" is necessary here -- it shouldn't change at
all.

> +        response.should redirect_to catalogs_path
>        end
>  
>      context "given catalog with one deployable, that is exclusive in this catalog" do
> @@ -44,9 +43,9 @@ describe CatalogsController do
>          @catalog = Factory :catalog_with_deployable
>        end
>  
> -      it "cannot be deleted when its deployables has no reference to other catalogs" do
> -        expect {delete :destroy, :id => @catalog.id}.not_to change(Catalog, :count).by(-1)
> -        response.should redirect_to catalog_path(@catalog)
> +      it "should delete its deployables if its deployables have no reference to other catalogs" do
> +        expect {delete :destroy, :id => @catalog.id}.to change(Deployable, :count).by(-1)
> +        response.should redirect_to catalogs_path
>        end
>      end
>      end
> -- 
> 1.7.7.6

Looks good. ACK, with one small nit above.

-- Matt



More information about the aeolus-devel mailing list