Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow deletion of groups with users belonging to other groups #15041

Merged
merged 3 commits into from
May 24, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion app/models/miq_group.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -230,8 +236,13 @@ def validate_default_tenant
end
end

def current_user_group?
id == current_user_group.try(:id)
end

def ensure_can_be_destroyed
raise _("Still has users assigned.") unless 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
Expand Down
54 changes: 47 additions & 7 deletions spec/models/miq_group_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -217,18 +217,27 @@
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 "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 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, /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
Expand Down Expand Up @@ -458,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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returns false if any users... right?

If it makes sense, please reword that in a followup PR.

Copy link
Contributor Author

@lgalis lgalis May 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrafanie - all users need to belong to an additional group for the group to be deleted. I can say 'every' user in the group instead of 'all'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, thanks @lgalis! I misread it. 👍

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
2 changes: 1 addition & 1 deletion spec/models/miq_widget_set_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down