rubyforgood
Deactivating users should address the role, rather than deactivating the user per se
Summary
When a bank deactivates a user, it should be addressing the role, rather than removing their ability to log in.
Why?
We have cases where a user goes to work for a partner after having worked for a bank. Right now, they can't do that under the same email address.
Details
Currently, when the bank deactivates a user, it is "discarding" that user -- it's essentially a flag on the user record.
A business scenario is that someone is working for a bank (using their own email), then they go and work for a partner. The bank disables them (naturally), but when the partner tries to add them, they currently will get a 'PG::UniqueViolation: ERROR: duplicate key value violates unique constraint "index_users_on_email" DETAIL: Key (email)=([email protected]) already exists.'
This should be handled, instead, by removing their roles with that bank and adding a "deactivated user" role, so that the bank can still reactivate them in the same way we do now?) (the reactivation will have to be adapted, of course)
We will also need a migration to convert currently deactivated users to users with a deactivated role
Criteria for completion
- [ ] functionality as described
- [ ] tests to support the functionality
- [ ] update tests regarding how the deactivation worked before.
Thanks @Josiassejod1 ! FWIW, you might want to take a quick look at @dorner's PR #3386 in case there's any overlap.
Hey @Josiassejod1 -- just a note that the reactivation will also need to be looked at (one of those obvious to me, but you're new to the project things)
This issue has been inactive for 247 hours (10.29 days) and will be automatically unassigned after 113 more hours (4.71 days).
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
This issue has been inactive for 367 hours (15.29 days) and is past the limit of 360 hours (15.00 days) so is being unassigned.
@cielf would like to take this over. Can look at this today and should have some time in the next week or so to work on it
This issue is marked as stale due to no activity within 30 days. If no further activity is detected within 7 days, it will be unassigned.
Whoops, sorry this fell under my radar. Will get going on this today+sunday
This issue is marked as stale due to no activity within 30 days. If no further activity is detected within 7 days, it will be unassigned.
Had some troubles getting to a solution, will be making another attempt over the holidays
This issue is marked as stale due to no activity within 30 days. If no further activity is detected within 7 days, it will be unassigned.
No luck with this, nor time at the moment to keep digging. @cielf @dorner Please unassign me so someone else can take a look!
Edit: Was able to do so myself!
@cielf @dorner having looked at this it seems like I'm going to need to convert discarded users to users with a deactivated role.
Is there any value in storing when a user was deactivated? discarded_at is obviously storing when they were discarded but it seems like the checks are just if the field is set.
So if I was was creating a column on user roles should it be a Boolean deactivated or a timestamp deactivated at?
I can imagine wanting to know when someone was deactivated, but it's a stretch.
Would it not make sense to make "deactivated" a role itself, rather than having it be a flag?
I was gonna make deactivated a flag on a role. Otherwise it's the same issue where the deactivation addresses the user not the role.
Like let's say an org deactivates a user (giving them the deactivated role), they work for partner who reactivates them (or they don't so they are also a deactivated partner user), suddenly they are reactivated with org as well.
Not quite so -- because Role is a combination of level of access and resource -- resource points to the associated org or partner (unless it's superadmin)
So you could be deactivated with Pawnee Diaper Bank, while having partner access with Helpful Food Pantry, if we made deactivated a level of access, similar to org_user or org_admin, but with no powers.
(The "Organization on the User record is an artifact of the old way of doing things -- we're working toward getting rid of it)
Role is a combination of level of access and resource -- resource points to the associated org or partner (unless it's superadmin)
Ok that makes sense. it would be like Role::DEACTIVATED, resource: org or whatever.
I am mainly favoring the idea of the column because that is what was discussed here https://github.com/rubyforgood/human-essentials/pull/3631#discussion_r1223473637. I also think the mental model is a bit simpler, especially around reactivation. And there are a few edge cases that are harder.
But are there any particular reasons that the column approach is worse?
I was thinking that adding a column for something that is only used 3% of the time might not be the way to go, and I find thinking of deactivated as a (null) access level fairly natural. But I wasn't aware that D had opined that column was the way to go. In either case, we are going to have to worry about people who have multiple access levels -- there are a lot of users who have both org_user and org_admin, IIRC.
I think the column is the way to go assuming there are ever situations where a user has two roles for the same resource that should have different activation states.
Example 1: user is an org admin and org user. I assume we should be able to deactivate user without deactivating admin and vice versa (and be able reactivate as well). Example 2: if we add resource-less roles (like the NDBN role) there would not be a way to differentiate which resource-less role is being deactivated.
I'm not saying column is not the way to go -- At this point I'm just raising that you might be designing to a not-yet-existing scenario (which I have certainly done many times) .
I could see an additional role that has less rights than user, but I don't foresee the rights within an organization being other than a hierarchy. As far as I've seen, they just aren't a silo kind of situation.
Example #2 seems somewhat more likely, though I expect we'd always have superadmin allowed to do anything that is across the board.
I still think the column on the role is the right way to go - you can deactivate any individual role. Yes, we need to ensure that deactivating an org_admin will also deactivate org_user, but that's not hard to do, and we'd have to do the same thing no matter how we do this.
@cielf how should we deal with reactivation/deactivation of org admins? Currently there is no deactivation of org admins (from what I can tell). You can only demote them to org users then deactivate.
Hmmm. I suspect what we have now is a poor approximation of reality. I suspect there are very few downgrades to org_user without actually deactivating.
Based on previously seen data - I suspect when we promote to org_admin we are merely adding the org-admin role and keeping the org_user tole -- just something to keep an eye out for when you're reasoning about this.
Given we only have the two bank roles - can we add a deactivate to the org admin (though, of course, not to ones own access), and if someone is deactivated have "restore as user" and "restore as admin"?
I double checked prod --- it definitely looks like our bank admins have both org_user and org_admin, for what it's worth (this just based on the number of users that have multiple roles)
Sorry for this @elasticspoon but now that I've finally seen your PR for this, it clicked for me. I think I might be able to possibly save you some work.
@cielf why are we allowing deactivation at all? Why are we not simply deleting the role itself (i.e. the users_role record)? The user stays where it is. A user without a valid role can't log in, which essentially means they are deactivated. And if the user has a different role, then that's the one that will be activated and they can't access the previous role. If the bank wants to reinstate them, they just do an invite and (I think) the system is already smart enough to just add the role to the existing user.
What do we get out of keeping the role where it is but with a deactivated flag?