From 7558f9fb77d9b04b2315664dbee3d185d07d01d9 Mon Sep 17 00:00:00 2001 From: lgalis Date: Tue, 9 May 2017 14:35:44 -0400 Subject: [PATCH 1/3] Allow deletion of groups if the users in the group also belong to other groups --- app/models/miq_group.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/models/miq_group.rb b/app/models/miq_group.rb index 9cd63d11958..f70e70154bf 100644 --- a/app/models/miq_group.rb +++ b/app/models/miq_group.rb @@ -230,8 +230,14 @@ def validate_default_tenant end end + def own_users + own_users = [] + users.each { |user| own_users << user if user.miq_groups.count == 1 } + own_users + end + def ensure_can_be_destroyed - raise _("Still has users assigned.") unless users.empty? + raise _("Still has users assigned.") unless own_users.empty? raise _("A tenant default group can not be deleted") if tenant_group? && referenced_by_tenant? raise _("A read only group cannot be deleted.") if system_group? end From a5d4c56add22268a0c66aa8195ad8f7e35c0e377 Mon Sep 17 00:00:00 2001 From: lgalis Date: Tue, 9 May 2017 21:55:19 -0400 Subject: [PATCH 2/3] Updated spec for allowing deletion of groups if the users in the group also belong to other groups --- spec/models/miq_group_spec.rb | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/spec/models/miq_group_spec.rb b/spec/models/miq_group_spec.rb index cf04b486357..97a2c6e9d89 100644 --- a/spec/models/miq_group_spec.rb +++ b/spec/models/miq_group_spec.rb @@ -217,18 +217,20 @@ it "fails if referenced by user#current_group" do FactoryGirl.create(:user, :miq_groups => [group]) - expect { - expect { group.destroy }.to raise_error(RuntimeError, /Still has users assigned/) - }.to_not change { MiqGroup.count } + expect { expect { group.destroy }.to raise_error(RuntimeError, /Still has users assigned/) }.to_not change { MiqGroup.count } end - it "fails if referenced by user#miq_groups" do + it "succeeds if referenced by multiple user#miq_groups" do group2 = FactoryGirl.create(:miq_group) FactoryGirl.create(:user, :miq_groups => [group, group2], :current_group => group2) + expect { group.destroy }.not_to raise_error + end - expect { - expect { group.destroy }.to raise_error(RuntimeError, /Still has users assigned/) - }.to_not change { MiqGroup.count } + it "fails if one user in the group does not belong to any other group" do + group2 = FactoryGirl.create(:miq_group) + FactoryGirl.create(:user, :miq_groups => [group, group2], :current_group => group2) + FactoryGirl.create(:user, :miq_groups => [group], :current_group => group) + expect { expect { group.destroy }.to raise_error(RuntimeError, /Still has users assigned/) }.to_not change { MiqGroup.count } end it "fails if referenced by a tenant#default_miq_group" do From 18e29cc304486d4de000b24f75b5024b64364aa7 Mon Sep 17 00:00:00 2001 From: lgalis Date: Mon, 22 May 2017 12:08:57 -0400 Subject: [PATCH 3/3] Update query and error message, add check and not allow deletion for the current login group --- app/models/miq_group.rb | 15 +++++++---- spec/models/miq_group_spec.rb | 42 ++++++++++++++++++++++++++++-- spec/models/miq_widget_set_spec.rb | 2 +- 3 files changed, 51 insertions(+), 8 deletions(-) diff --git a/app/models/miq_group.rb b/app/models/miq_group.rb index f70e70154bf..b83e4b1ed25 100644 --- a/app/models/miq_group.rb +++ b/app/models/miq_group.rb @@ -220,6 +220,12 @@ def self.with_current_user_groups current_user.admin_user? ? all : where(:id => current_user.miq_group_ids) end + def single_group_users? + group_user_ids = user_ids + return false if group_user_ids.empty? + users.includes(:miq_groups).where(:id => group_user_ids).where.not(:miq_groups => {:id => id}).count != group_user_ids.size + end + private # if this tenant is changing, make sure this is not a default group @@ -230,14 +236,13 @@ def validate_default_tenant end end - def own_users - own_users = [] - users.each { |user| own_users << user if user.miq_groups.count == 1 } - own_users + def current_user_group? + id == current_user_group.try(:id) end def ensure_can_be_destroyed - raise _("Still has users assigned.") unless own_users.empty? + raise _("The login group cannot be deleted") if current_user_group? + raise _("The group has users assigned that do not belong to any other group") if single_group_users? raise _("A tenant default group can not be deleted") if tenant_group? && referenced_by_tenant? raise _("A read only group cannot be deleted.") if system_group? end diff --git a/spec/models/miq_group_spec.rb b/spec/models/miq_group_spec.rb index 97a2c6e9d89..0b161f6d8ba 100644 --- a/spec/models/miq_group_spec.rb +++ b/spec/models/miq_group_spec.rb @@ -217,7 +217,7 @@ it "fails if referenced by user#current_group" do FactoryGirl.create(:user, :miq_groups => [group]) - expect { expect { group.destroy }.to raise_error(RuntimeError, /Still has users assigned/) }.to_not change { MiqGroup.count } + expect { expect { group.destroy }.to raise_error(RuntimeError, /The group has users assigned that do not belong to any other group/) }.to_not change { MiqGroup.count } end it "succeeds if referenced by multiple user#miq_groups" do @@ -226,11 +226,18 @@ expect { group.destroy }.not_to raise_error end + it "fails if trying to delete the current user's group" do + group2 = FactoryGirl.create(:miq_group) + user = FactoryGirl.create(:user, :miq_groups => [group, group2], :current_group => group) + User.current_user = user + expect { expect { group.destroy }.to raise_error(RuntimeError, /The login group cannot be deleted/) }.to_not change { MiqGroup.count } + end + it "fails if one user in the group does not belong to any other group" do group2 = FactoryGirl.create(:miq_group) FactoryGirl.create(:user, :miq_groups => [group, group2], :current_group => group2) FactoryGirl.create(:user, :miq_groups => [group], :current_group => group) - expect { expect { group.destroy }.to raise_error(RuntimeError, /Still has users assigned/) }.to_not change { MiqGroup.count } + expect { expect { group.destroy }.to raise_error(RuntimeError, /The group has users assigned that do not belong to any other group/) }.to_not change { MiqGroup.count } end it "fails if referenced by a tenant#default_miq_group" do @@ -460,4 +467,35 @@ expect(group.user_count).to eq(2) end end + + describe "#single_group_users?" do + it "returns false if all users in the group belong to an additional group" do + testgroup1 = FactoryGirl.create(:miq_group) + testgroup2 = FactoryGirl.create(:miq_group) + testgroup3 = FactoryGirl.create(:miq_group) + FactoryGirl.create(:user, :miq_groups => [testgroup1, testgroup2]) + FactoryGirl.create(:user, :miq_groups => [testgroup1, testgroup3]) + expect(testgroup1.single_group_users?).to eq(false) + end + + it "returns false if the group has no users" do + testgroup1 = FactoryGirl.create(:miq_group) + expect(testgroup1.single_group_users?).to eq(false) + end + + it "returns true if all users in a group do not belong to any other group" do + testgroup1 = FactoryGirl.create(:miq_group) + FactoryGirl.create(:user, :miq_groups => [testgroup1]) + FactoryGirl.create(:user, :miq_groups => [testgroup1]) + expect(testgroup1.single_group_users?).to eq(true) + end + + it "returns true if any user in a group does not belong to another group" do + testgroup1 = FactoryGirl.create(:miq_group) + testgroup2 = FactoryGirl.create(:miq_group) + FactoryGirl.create(:user, :miq_groups => [testgroup1]) + FactoryGirl.create(:user, :miq_groups => [testgroup1, testgroup2]) + expect(testgroup1.single_group_users?).to eq(true) + end + end end diff --git a/spec/models/miq_widget_set_spec.rb b/spec/models/miq_widget_set_spec.rb index 9a400591ba8..e702e3b0436 100644 --- a/spec/models/miq_widget_set_spec.rb +++ b/spec/models/miq_widget_set_spec.rb @@ -30,7 +30,7 @@ end it "the belong to group is being deleted" do - expect { group.destroy }.to raise_error(RuntimeError, /Still has users assigned/) + expect { group.destroy }.to raise_error(RuntimeError, /The group has users assigned that do not belong to any other group/) expect(MiqWidgetSet.count).to eq(2) end