On Wed, Jan 11, 2012 at 07:07:56PM +0100, jzigmund(a)redhat.com wrote:
From: Jozef Zigmund <jzigmund(a)redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=770815
---
src/app/controllers/catalogs_controller.rb | 22 ++++++++++++++++------
src/app/models/catalog.rb | 6 ++++++
src/app/models/catalog_entry.rb | 6 ++++++
src/app/models/deployable.rb | 2 +-
src/config/locales/en.yml | 10 ++++++++--
5 files changed, 37 insertions(+), 9 deletions(-)
diff --git a/src/app/controllers/catalogs_controller.rb
b/src/app/controllers/catalogs_controller.rb
index 2dfdb00..882ffe1 100644
--- a/src/app/controllers/catalogs_controller.rb
+++ b/src/app/controllers/catalogs_controller.rb
@@ -80,21 +80,31 @@ class CatalogsController < ApplicationController
end
def multi_destroy
+ deleted = []
+ not_deleted = []
catalogs = Catalog.find(params[:catalogs_selected])
catalogs.to_a.each do |catalog|
require_privilege(Privilege::MODIFY, catalog)
- catalog.destroy
+ if catalog.destroy
+ deleted << catalog.name
+ else
+ not_deleted << catalog.name
+ end
end
- redirect_to catalogs_path, :notice =>
t("catalogs.flash.notice.deleted", :count => catalogs.count)
+ flash[:notice] = t("catalogs.flash.notice.deleted", :count =>
deleted.count, :deleted => deleted.join(', ')) unless deleted.empty?
+ flash[:error] = t("catalogs.flash.error.not_deleted", :count =>
not_deleted.count, :not_deleted => not_deleted.join(', ')) unless
not_deleted.empty?
+ redirect_to catalogs_path
end
def destroy
catalog = Catalog.find(params[:id])
require_privilege(Privilege::MODIFY, catalog)
- catalog.destroy
-
- respond_to do |format|
- format.html { redirect_to catalogs_path, :notice =>
t("catalogs.flash.notice.deleted", :count => 1) }
+ if catalog.destroy
+ flash[:notice] = t("catalogs.flash.notice.one_deleted")
+ redirect_to catalogs_path
+ else
+ flash[:error] = t("catalogs.flash.error.one_not_deleted")
+ redirect_to catalog_path(catalog)
end
end
diff --git a/src/app/models/catalog.rb b/src/app/models/catalog.rb
index 542d1f5..b2762f1 100644
--- a/src/app/models/catalog.rb
+++ b/src/app/models/catalog.rb
@@ -38,6 +38,8 @@ class Catalog < ActiveRecord::Base
:include => [:role],
:order => "permissions.id ASC"
+ before_destroy :check_deployable_references
+
validates_presence_of :pool
validates_presence_of :name
validates_uniqueness_of :name
@@ -46,6 +48,10 @@ 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.detect {|d| d.catalogs.count < 2} == nil ? true : false
I think the above line could be rewritten as:
deployables.any? {|d| d.catalogs.count > 1}
+ end
private
diff --git a/src/app/models/catalog_entry.rb b/src/app/models/catalog_entry.rb
index 0cef9e7..3deb9d9 100644
--- a/src/app/models/catalog_entry.rb
+++ b/src/app/models/catalog_entry.rb
@@ -35,6 +35,12 @@ class CatalogEntry < ActiveRecord::Base
validates_uniqueness_of :catalog_id, :scope => [:deployable_id]
+ before_destroy :check_deployable_references
+
+ def check_deployable_references
+ return deployable.catalogs.count < 2 ? false : true
Isn't this the same as:
return deployable.catalogs.count > 1
Besides eliminating the need for a ternary, it feels a little more
natural to say "more than one" here, as opposed to "less than two".
+ end
+
# This probably goes away once we separate catalog entry creation from deployables
accepts_nested_attributes_for :deployable
end
diff --git a/src/app/models/deployable.rb b/src/app/models/deployable.rb
index 54d6129..78847f1 100644
--- a/src/app/models/deployable.rb
+++ b/src/app/models/deployable.rb
@@ -32,7 +32,7 @@ class Deployable < ActiveRecord::Base
has_many :permissions, :as => :permission_object, :dependent => :destroy,
:include => [:role],
:order => "permissions.id ASC"
- has_many :catalog_entries, :dependent => :destroy
+ has_many :catalog_entries, :dependent => :delete_all
has_many :catalogs, :through => :catalog_entries
belongs_to :owner, :class_name => "User", :foreign_key =>
"owner_id"
diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
index b1feea1..833dfa2 100644
--- a/src/config/locales/en.yml
+++ b/src/config/locales/en.yml
@@ -668,8 +668,14 @@ en:
one: Catalog created!
other: Catalogs created!
deleted:
- one: Catalog deleted!
- other: "%{count} Catalogs deleted!"
+ one: Catalog %{deleted} deleted!
+ other: "%{count} catalogs %{deleted} deleted!"
+ one_deleted: Catalog deleted!
+ error:
+ not_deleted:
+ one: "Catalog %{not_deleted} cannot be deleted! Some deployable has the
last reference to this catalog!"
+ other: "These catalogs %{not_deleted} cannot be deleted! Some of their
deployables have the last reference to the catalogs!"
+ one_not_deleted: "Catalog cannot be deleted! Some deployable has the last
reference to this catalog!"
preset_filters:
all_catalogs: "All Catalogs"
belongs_to_default_pool: "Belongs to Default Pool"
--
1.7.7.5
Reading over the BZ and dwelling on this for a bit, I think that we want
to also delete the Deployable in this case, with the same condition used
above -- if it doesn't belong to anything else. Otherwise, I end up with
the same problem described in the BZ -- if I delete a Catalog containing
a certain Deployable, it *looks* like that Deployable is deleted since I
cannot find it in the UI. But then if I try to create a new deployable
with the name of something that was in the deleted catalog and I cannot,
because I get that the error that the name is already in use.
I've also got a couple of nits inline above.
Best,
Matt