Update users' current_group on group deletion #16809

lgalis · 2018-01-11T21:23:27Z

When deleting a group, update the current_group for users that have the deleted group as the current_group

Links

https://bugzilla.redhat.com/show_bug.cgi?id=1264967

Steps for Testing/QA

Create 1 new group
Create 1 or new 2 users and assign them to the newly created group plus one additional group each.
Make sure the new group is the current group for at least one of the users.
Delete the group create in step1.

The user that had the current_group set to the new group can log in with the changes in this PR, as its new current_group is the second group it belonged to.

lgalis · 2018-01-15T12:36:18Z

@lpichler , @gtanzillo - please review

lgalis · 2018-01-15T13:24:42Z

@miq-bot add_label bug, gaprindashvili/yes, blocker

JPrause · 2018-01-16T16:51:43Z

@lpichler @gtanzillo as this involves a blocker issue, can this be reviewed asap.

lpichler · 2018-01-16T16:53:07Z

@JPrause I am working on it 👍

lpichler · 2018-01-16T16:59:43Z

app/models/miq_group.rb

+  end
+
+  def reset_current_group_for_users
+    User.where(:current_group_id => id).each {|u| u.change_current_group(id)}


it looks like we have method for it already User#current_group_by_description

gtanzillo · 2018-01-16T17:04:59Z

spec/models/miq_group_spec.rb

+      group2 = FactoryGirl.create(:miq_group)
+      FactoryGirl.create(:user, :miq_groups => [group, group2], :current_group => group2)
+      FactoryGirl.create(:user, :miq_groups => [group, group2], :current_group => group)
+      expect { expect { group.destroy }.to raise_error(RuntimeError, /The group is the login group for at least one user in the group/) }.to_not change { MiqGroup.count }


Shouldn't succeed because both users are members of another group and you new logic would switch to current_group to group2 for the second user?

lpichler

discussed with @lgalis and LGTM 👍

carbonin · 2018-01-16T22:24:57Z

app/models/user.rb

+  def change_current_group(id)
+    user_groups = miq_group_ids
+    user_groups.delete(id)
+    raise _("The user's loging group cannot be changed.") if user_groups.empty?


Is there a typo in this error message?

It looks like this would be the case when the user's only group would be the one deleted.
What is a "loging" group? Even if that's supposed to be "login group" I'm still not sure what that means.

carbonin · 2018-01-16T22:27:05Z

app/models/miq_group.rb

  def ensure_can_be_destroyed
    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?
+    reset_current_group_for_users if current_group_for_any_user?


This is a strange side effect of a method that seems to be checking if a group is allowed to be destroyed.

The way this is written it feels like this should be two separate steps:

Ensure that the group in question is not the only group assigned to any user

When (or just before) the group is actually being removed (not in this method), reassign the current group.

Ensure that the group in question is not the only group assigned to any user

Ah I see that this check is being done by the line above with if single_group_users?, but I still feel like we could add another hook for this, maybe after_destroy?

I did consider that, however, the group should not be destroyed if the current_group cannot be changed for any user that has this as the current_group.

As I read it the only reason the current_group would not be able to be changed is if a user had no other group to change to. This case is already covered by another assertion already in this method, so I don't see any reason this can't be moved.

I moved the changing of the current_group for the users that requires it to the after_destroy.

…ave this group as the current_group

… is deleted

… use the current_group id.

Move reset_current_group to :after_destroy

miq-bot · 2018-01-17T19:18:17Z

Checked commits lgalis/manageiq@2939ab3~...a615f97 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 🍰

gtanzillo

👍 LGTM

gtanzillo · 2018-01-17T19:44:38Z

@carbonin Are you good with this PR at this point?

carbonin

This looks great! 👍

…tion Update users' current_group on group deletion (cherry picked from commit abc4775) https://bugzilla.redhat.com/show_bug.cgi?id=1536035

simaishi · 2018-01-18T14:10:42Z

Gaprindashvili backport details:

$ git log -1
commit 90adee097c739ce6091f4678a57bd411a31839dd
Author: Gregg Tanzillo <[email protected]>
Date:   Wed Jan 17 15:04:35 2018 -0500

    Merge pull request #16809 from lgalis/update_user_login_group_on_deletion
    
    Update users' current_group on group  deletion
    (cherry picked from commit abc4775d630e06a363827f60a58ecae9b3e68e6f)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1536035

lgalis force-pushed the update_user_login_group_on_deletion branch from 86363a6 to cccef2d Compare January 11, 2018 21:25

lgalis changed the title ~~[WIP] Update user current_group on group deletion~~ [WIP] Update users' current_group on group deletion Jan 11, 2018

miq-bot added the wip label Jan 11, 2018

lgalis mentioned this pull request Jan 11, 2018

[WIP] Deletion not allowed for a group if the group is the login group for any user #16758

Closed

lgalis force-pushed the update_user_login_group_on_deletion branch from cccef2d to 10d2205 Compare January 14, 2018 12:06

lgalis changed the title ~~[WIP] Update users' current_group on group deletion~~ Update users' current_group on group deletion Jan 15, 2018

lgalis changed the title ~~Update users' current_group on group deletion~~ [WIP] Update users' current_group on group deletion Jan 15, 2018

miq-bot added blocker bug gaprindashvili/yes labels Jan 15, 2018

lpichler reviewed Jan 16, 2018

View reviewed changes

gtanzillo reviewed Jan 16, 2018

View reviewed changes

lpichler approved these changes Jan 16, 2018

View reviewed changes

lgalis changed the title ~~[WIP] Update users' current_group on group deletion~~ Update users' current_group on group deletion Jan 16, 2018

miq-bot removed the wip label Jan 16, 2018

carbonin reviewed Jan 16, 2018

View reviewed changes

lgalis force-pushed the update_user_login_group_on_deletion branch 3 times, most recently from dd317da to 105e7a6 Compare January 17, 2018 11:21

carbonin assigned gtanzillo Jan 17, 2018

lgalis force-pushed the update_user_login_group_on_deletion branch from 105e7a6 to e876972 Compare January 17, 2018 14:45

lgalis changed the title ~~Update users' current_group on group deletion~~ [WIP]Update users' current_group on group deletion Jan 17, 2018

lgalis force-pushed the update_user_login_group_on_deletion branch from e876972 to 2670de6 Compare January 17, 2018 15:02

lgalis changed the title ~~[WIP]Update users' current_group on group deletion~~ Update users' current_group on group deletion Jan 17, 2018

lgalis force-pushed the update_user_login_group_on_deletion branch from 27caac8 to ab7229b Compare January 17, 2018 16:00

lgalis force-pushed the update_user_login_group_on_deletion branch from ab7229b to d029675 Compare January 17, 2018 16:21

lgalis added 7 commits January 17, 2018 14:16

When deleting a group, change the current_user_group for users that h…

2939ab3

…ave this group as the current_group

Specs for changing the current_group for users when the current group…

9c4488a

… is deleted

Address rubocop style comments

d63e313

Updates to change_current_group

7e591df

Fix typo in change_current group's error message. Remove argument and…

aad0638

… use the current_group id.

Added specs for change_current_group

2b3223f

Move reset_current_group to :after_destroy

Move reset_current_group to :after_destroy

a615f97

lgalis force-pushed the update_user_login_group_on_deletion branch from d029675 to a615f97 Compare January 17, 2018 19:16

gtanzillo approved these changes Jan 17, 2018

View reviewed changes

carbonin approved these changes Jan 17, 2018

View reviewed changes

gtanzillo added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 17, 2018

gtanzillo merged commit abc4775 into ManageIQ:master Jan 17, 2018

lgalis deleted the update_user_login_group_on_deletion branch January 17, 2018 20:17

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jan 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update users' current_group on group deletion #16809

Update users' current_group on group deletion #16809

lgalis commented Jan 11, 2018 •

edited

Loading

lgalis commented Jan 15, 2018

lgalis commented Jan 15, 2018

JPrause commented Jan 16, 2018

lpichler commented Jan 16, 2018

lpichler Jan 16, 2018 •

edited

Loading

gtanzillo Jan 16, 2018

lpichler left a comment

carbonin Jan 16, 2018

carbonin Jan 16, 2018 •

edited

Loading

carbonin Jan 16, 2018

lgalis Jan 16, 2018

carbonin Jan 17, 2018

lgalis Jan 17, 2018 •

edited

Loading

miq-bot commented Jan 17, 2018

gtanzillo left a comment

gtanzillo commented Jan 17, 2018

carbonin left a comment

simaishi commented Jan 18, 2018

Update users' current_group on group deletion #16809

Update users' current_group on group deletion #16809

Conversation

lgalis commented Jan 11, 2018 • edited Loading

Links

Steps for Testing/QA

lgalis commented Jan 15, 2018

lgalis commented Jan 15, 2018

JPrause commented Jan 16, 2018

lpichler commented Jan 16, 2018

lpichler Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

gtanzillo Jan 16, 2018

Choose a reason for hiding this comment

lpichler left a comment

Choose a reason for hiding this comment

carbonin Jan 16, 2018

Choose a reason for hiding this comment

carbonin Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

carbonin Jan 16, 2018

Choose a reason for hiding this comment

lgalis Jan 16, 2018

Choose a reason for hiding this comment

carbonin Jan 17, 2018

Choose a reason for hiding this comment

lgalis Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

miq-bot commented Jan 17, 2018

gtanzillo left a comment

Choose a reason for hiding this comment

gtanzillo commented Jan 17, 2018

carbonin left a comment

Choose a reason for hiding this comment

simaishi commented Jan 18, 2018

lgalis commented Jan 11, 2018 •

edited

Loading

lpichler Jan 16, 2018 •

edited

Loading

carbonin Jan 16, 2018 •

edited

Loading

lgalis Jan 17, 2018 •

edited

Loading