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

lgalis · 2017-05-09T18:47:41Z

Allow deletion of groups when all users within the group also belong to other groups.

Links

https://bugzilla.redhat.com/show_bug.cgi?id=1264967
https://www.pivotaltracker.com/n/projects/1613907/stories/131568585

lgalis · 2017-05-11T20:14:28Z

@miq-bot add_label enhancement

lgalis · 2017-05-11T20:15:23Z

@lpichler, @gtanzillo - please review

chessbyte · 2017-05-16T20:38:13Z

app/models/miq_group.rb

+    own_users = []
+    users.each { |user| own_users << user if user.miq_groups.count == 1 }
+    own_users
+  end


I believe this can be more eloquent as

def own_users users.select { |user| user.miq_groups.count == 1 } end

def has_single_group_user? users.any? { |user| user.miq_groups.count == 1 } end

Naming is hard. Then below...

raise _("Still has users assigned.") if has_single_group_user?

I believe that we can remove N+1 here by this query:

def users_assigned_to_other_groups? group_user_ids = user_ids User.includes(:miq_groups).where(:id => group_user_ids).where.not(:miq_groups => {:id => id}).count == group_user_ids.size end

which means:
select users of the group and elimited the users with the our group(which we want to delete)
and if all (comparing with == group_user_ids.size)users will belongs to any other groups than we can delete it.

also, I would like to change message to something more descriptive.

raise _("All users don't belong to other groups ") unless users_assigned_to_other_groups?

What do you think?

lpichler

@lgalis Tested with LDAP and works well and I added other suggestions.

Also, I found a bug.
When you are deleting group which is current group of users than the application is broken. After re-login application works and the other group is assigned as default.

But I am not sure what to do in this case:

We can just refuse to delete of such group.
We can reassign the other group to logged user.
Logout.

thanks

chrispy1 · 2017-05-17T15:36:40Z

@miq-bot add_label euwe/yes
@miq-bot add_label blocker

chrispy1 · 2017-05-17T15:37:28Z

@miq-bot add_label fine/yes

lgalis · 2017-05-22T17:20:07Z

@lpichler, @jrafanie - please review again

jrafanie · 2017-05-22T17:56:20Z

app/models/miq_group.rb

@@ -230,8 +230,18 @@ def validate_default_tenant
    end
  end

+  def single_group_users?
+    group_user_ids = user_ids
+    users.includes(:miq_groups).where(:id => group_user_ids).where.not(:miq_groups => {:id => id}).count != group_user_ids.size


Looks good to me, but we might want tests just for this method to ensure it does the right thing in a few different scenarios.

ok - there are specs that cover different scenarios for deletion - but I can add specs for just this method as well.

…er groups

…p also belong to other groups

…the current login group

miq-bot · 2017-05-24T03:33:56Z

Checked commits lgalis/manageiq@7558f9f~...18e29cc with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. ⭐

lgalis · 2017-05-24T12:49:34Z

@lpichler , @jrafanie - requested changes made

lpichler

LGTM thanks @lgalis!

jrafanie · 2017-05-24T17:47:53Z

spec/models/miq_group_spec.rb

@@ -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


returns false if any users... right?

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

@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'

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

Allow deletion of groups with users belonging to other groups (cherry picked from commit 828b50b) https://bugzilla.redhat.com/show_bug.cgi?id=1322396

simaishi · 2017-05-24T18:06:58Z

Euwe backport details:

$ git log -1
commit 6dd942b66aa0f67a74acfc181ddd3c293bd8fdb7
Author: Joe Rafaniello <jrafanie@users.noreply.github.com>
Date:   Wed May 24 13:48:04 2017 -0400

    Merge pull request #15041 from lgalis/allow_not_empty_group_deletion
    
    Allow deletion of groups with users belonging to other groups
    (cherry picked from commit 828b50b82e5a7dedb2394e4db942bf1357360e6c)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1322396

Allow deletion of groups with users belonging to other groups (cherry picked from commit 828b50b) https://bugzilla.redhat.com/show_bug.cgi?id=1460307

simaishi · 2017-06-09T15:56:24Z

Fine backport details:

$ git log -1
commit caa1b85a0d0ff8f73790a565843fc9866ec06aec
Author: Joe Rafaniello <jrafanie@users.noreply.github.com>
Date:   Wed May 24 13:48:04 2017 -0400

    Merge pull request #15041 from lgalis/allow_not_empty_group_deletion
    
    Allow deletion of groups with users belonging to other groups
    (cherry picked from commit 828b50b82e5a7dedb2394e4db942bf1357360e6c)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1460307

miq-bot added the wip label May 9, 2017

lgalis force-pushed the allow_not_empty_group_deletion branch 5 times, most recently from 5595336 to 9b3fa7b Compare May 11, 2017 20:11

miq-bot added the enhancement label May 11, 2017

lgalis changed the title ~~[WIP] Allow deletion of groups with users belonging to other groups~~ Allow deletion of groups with users belonging to other groups May 16, 2017

miq-bot removed the wip label May 16, 2017

chessbyte reviewed May 16, 2017

View reviewed changes

chessbyte assigned jrafanie May 16, 2017

lpichler suggested changes May 17, 2017

View reviewed changes

miq-bot added euwe/yes blocker labels May 17, 2017

miq-bot added the fine/yes label May 17, 2017

simaishi removed the blocker label May 17, 2017

lgalis changed the title ~~Allow deletion of groups with users belonging to other groups~~ [WIP]Allow deletion of groups with users belonging to other groups May 18, 2017

miq-bot added the wip label May 18, 2017

lgalis force-pushed the allow_not_empty_group_deletion branch 3 times, most recently from 17cc7f7 to 7eff596 Compare May 22, 2017 17:14

lgalis changed the title ~~[WIP]Allow deletion of groups with users belonging to other groups~~ Allow deletion of groups with users belonging to other groups May 22, 2017

miq-bot removed the wip label May 22, 2017

jrafanie reviewed May 22, 2017

View reviewed changes

lgalis changed the title ~~Allow deletion of groups with users belonging to other groups~~ [WIP] Allow deletion of groups with users belonging to other groups May 22, 2017

miq-bot added the wip label May 22, 2017

lgalis force-pushed the allow_not_empty_group_deletion branch 2 times, most recently from 6c88998 to bc7b891 Compare May 23, 2017 21:30

lgalis added 3 commits May 23, 2017 23:28

Allow deletion of groups if the users in the group also belong to oth…

7558f9f

…er groups

Updated spec for allowing deletion of groups if the users in the grou…

a5d4c56

…p also belong to other groups

Update query and error message, add check and not allow deletion for …

18e29cc

…the current login group

lgalis force-pushed the allow_not_empty_group_deletion branch from bc7b891 to 18e29cc Compare May 24, 2017 03:28

lgalis changed the title ~~[WIP] Allow deletion of groups with users belonging to other groups~~ Allow deletion of groups with users belonging to other groups May 24, 2017

miq-bot removed the wip label May 24, 2017

lpichler approved these changes May 24, 2017

View reviewed changes

jrafanie reviewed May 24, 2017

View reviewed changes

jrafanie merged commit 828b50b into ManageIQ:master May 24, 2017

jrafanie added this to the Sprint 62 Ending Jun 5, 2017 milestone May 24, 2017

simaishi added euwe/backported blocker and removed euwe/yes labels May 24, 2017

simaishi added fine/backported and removed fine/yes labels Jun 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lgalis commented May 9, 2017 •

edited

Loading

lgalis commented May 11, 2017

lgalis commented May 11, 2017

chessbyte May 16, 2017

jrafanie May 16, 2017

lpichler May 17, 2017 •

edited

Loading

lpichler left a comment

chrispy1 commented May 17, 2017

chrispy1 commented May 17, 2017

lgalis commented May 22, 2017

jrafanie May 22, 2017

lgalis May 22, 2017

miq-bot commented May 24, 2017

lgalis commented May 24, 2017

lpichler left a comment

jrafanie May 24, 2017

lgalis May 24, 2017 •

edited

Loading

jrafanie May 24, 2017

simaishi commented May 24, 2017

simaishi commented Jun 9, 2017

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

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

Conversation

lgalis commented May 9, 2017 • edited Loading

Links

lgalis commented May 11, 2017

lgalis commented May 11, 2017

chessbyte May 16, 2017

Choose a reason for hiding this comment

jrafanie May 16, 2017

Choose a reason for hiding this comment

lpichler May 17, 2017 • edited Loading

Choose a reason for hiding this comment

lpichler left a comment

Choose a reason for hiding this comment

chrispy1 commented May 17, 2017

chrispy1 commented May 17, 2017

lgalis commented May 22, 2017

jrafanie May 22, 2017

Choose a reason for hiding this comment

lgalis May 22, 2017

Choose a reason for hiding this comment

miq-bot commented May 24, 2017

lgalis commented May 24, 2017

lpichler left a comment

Choose a reason for hiding this comment

jrafanie May 24, 2017

Choose a reason for hiding this comment

lgalis May 24, 2017 • edited Loading

Choose a reason for hiding this comment

jrafanie May 24, 2017

Choose a reason for hiding this comment

simaishi commented May 24, 2017

simaishi commented Jun 9, 2017

lgalis commented May 9, 2017 •

edited

Loading

lpichler May 17, 2017 •

edited

Loading

lgalis May 24, 2017 •

edited

Loading