Handle deactivated users gracefully from a bank #3631
Conversation
Josiassejod1
changed the title
| @@ -99,6 +100,7 @@ def kind | |||
| return "admin" if has_role?(Role::ORG_ADMIN, organization) | |||
| return "normal" if has_role?(Role::ORG_USER, organization) | |||
| return "partner" if has_role?(Role::PARTNER, partner) | |||
| return "deactived" if has_role?(Role::DEACTIVATED) | |||
There was a problem hiding this comment.
This doesn't seem like the right way to model the user. We shouldn't be deliberately giving them a deactivated role. If the user has no roles, we should consider that user deactivated and treat them accordingly.
There was a problem hiding this comment.
Hrm. @dorner -- Here's the thing, though. The bank can deactivate a user and reactivate them. However, there is also a case where someone may go to work for a partner after having worked for a bank (or vice versa), with possible overlap. (pls check the issue ). As things are now (deactivating the user as a user), when the bank deactivates a user, it precludes them working for a partner.
There was a problem hiding this comment.
There may well be a way of allowing this situation without using a deactivated role, and if we were to use a deactivated role (which, I admit, is how I thought this would be done, given that we allow reactivation, and that someone might (also) be working with a partner) , it would need to be deactivated in the context of an organization, not just deactivated, imo.
There was a problem hiding this comment.
Ah - I wasn't aware that the bank needed to be able to reactivate users. If that's the case, then yes an org-level deactivated role probably makes the most sense. However, there might be code lurking around that assumes that if you have any role with an org, you have the most basic one (which is ORG_USER) - so you might need to tweak that assumption. E.g. ensure that if that's the role the user would be given when logging in, the user is instead kicked out.
I'm just a bit concerned about the data - what if someone somehow ends up with both an ORG_USER and a DEACTIVATED role?
Maybe a better approach would be to add a "deactivated" column to the user_roles table and use that. Again, we'd have to ensure the login and permission logic would specifically exclude deactivated roles in 99% of cases. But there's less chance of weird data issues since you are activating and deactivating a specific role. We also get the benefit of (if we need to) in the future allowing any role to be activated and deactivated - e.g. partner users.
There was a problem hiding this comment.
See RolifyCommunity/rolify#551 - similar idea but here we'd use deactivated instead of deleted. We'd also have to use unscoped when on the page that shows deactivated users.
There was a problem hiding this comment.
FWIW I have a gut "that sounds right" reaction to the idea of adding the "deactivated" column to the user_roles table.
| class DeactiveUsers < ActiveRecord::Migration[7.0] | ||
| def change | ||
| Users.discarded.each do |user| | ||
| user.add_role(:deactivated) |
There was a problem hiding this comment.
Again - here we'd want to just remove all roles rather than add a new one.
|
Noted that @Josiassejod1 has said (on the issue) that someone can take this off their hands. Not sure if it's a continue with this PR, or a start from scratch situation. |
|
I think we have a later PR trying to address the same issue -- look into that before working on this one. |
|
Closing due to inactivity. |
Resolves #3587
Description
Type of change
How Has This Been Tested?
Screenshots