replace org_user deactivation with removal of org_user role #4477
Conversation
elasticspoon
force-pushed
the
feat/replace-role-deactivation-with-removal
branch
2 times, most recently
from
23f1269
to
3a66ddc
Compare
elasticspoon
changed the title
|
|
||
| Warden::Manager.after_set_user do |user, auth, opts| | ||
| if user.roles.empty? | ||
| auth.logout | ||
| throw(:warden) | ||
| end | ||
| end |
There was a problem hiding this comment.
This little change deals with the situation where a user has no roles (in that case we don't let them log in). This mirrors the behavior of dealing with discarded users (that is done with a default scope on users default_scope { kept }).
I considered 2 other options:
- adding to the default scope to filter out users without roles but I was worried this could affect existing behavior
- attempted to modify the
createmethod of the session controller but that resulted in a confusing error for the user (it would seem like they logged in and got logged out immediately). I needed to address the logic in thesupermethod.
|
|
||
| def reactivate_user | ||
| user = User.with_discarded.find_by!(id: params[:user_id]) | ||
| def remove_user |
There was a problem hiding this comment.
I modified this method rather than using the admin roles controller because that you required the super admin role and I thought it did not makes sense to alter that logic
| fill_in "user_email", with: "no_role_user@example.com" | ||
| fill_in "user_password", with: DEFAULT_USER_PASSWORD | ||
| find('input[name="commit"]').click | ||
| expect(page).to have_content("You need to sign in before continuing.") |
There was a problem hiding this comment.
I don't love this error message but I could not figure out how to change it.
I tried throw(:warden, action: :invalid) but that still gave the same error message.
There was a problem hiding this comment.
Not ideal, but this kind of code is super hairy. I'd timebox it and if you can't change it, I'm OK with it as is. @cielf for final decision here.
The issue was that banks could "deactivate" a user (which is essentially a soft delete of the whole User record). However, if that user then went to a Partner they would not be able to invite that user because that user would exist (new User cannot be created) but be soft deleted (existing User is treated like they didn't have permissions). We changed the logic around deactivation to address the role not the user. I attempted to implement logic around deactivating roles themselves in rubyforgood#4392, rubyforgood#4382. But the logic became very complex. We decided (rubyforgood#3587 (comment)) to go for a more architerurally simple solution of removing the deactivation functionality and just deleting the roles. To migrate the existing data I decided to reactivate any discarded user with the org user role and remove the org user role from them. (My guess is that those are the org users that actually got deactivated)
elasticspoon
force-pushed
the
feat/replace-role-deactivation-with-removal
branch
from
3a66ddc
to
e817a2a
Compare
There was a problem hiding this comment.
This is what I came up with in terms of a data migration. I am not totally sure it make sense. But my logic was: a discarded user with the 'org_user' role is probably someone that got deactivated.
| user.roles.where(name: "org_user").delete_all | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
I checked the data, and every discarded user has an org_user role. This makes sense, because you can only discard a user from the organization user management page. So I think you're safe to not have to check the role name.
In fact, you should just delete all roles that belong to current discarded users - many of them also have partner roles, but if they're discarded, they wouldn't have been able to log in. So effectively all their roles should be removed.
There was a problem hiding this comment.
Updated this. Since all discarded users are org users might as well simplify it further and just deal exclusively with discarded users.
| fill_in "user_email", with: "no_role_user@example.com" | ||
| fill_in "user_password", with: DEFAULT_USER_PASSWORD | ||
| find('input[name="commit"]').click | ||
| expect(page).to have_content("You need to sign in before continuing.") |
There was a problem hiding this comment.
Not ideal, but this kind of code is super hairy. I'd timebox it and if you can't change it, I'm OK with it as is. @cielf for final decision here.
also simplification/correction of some test logic
All discarded users are org_user meaning they have been deactivated. To simplify everything we will just remove all thier roles.
|
I hope it's going to be a pretty rare case that someone is going to try to sign in after their role has been deleted, so I'm ok with leaving the slightly confusing error message. Not the best place to be using our resources if it's too hairy. |
|
Looks good to me! @cielf did you want to take a look? |
Yes. |
|
@elasticspoon: Your PR |
Resolves #3587
Description
Problem
The issue was that banks could "deactivate" a user (which is essentially a soft delete of the whole
Userrecord). However, if that user then went to aPartnerthey would not be able to invite that user because that user would exist (new User cannot be created) but be soft deleted (existing User is treated like they didn't have permissions).Solution
Roles got added in #3117. Therefore, the logic around deactivation should address the role not the user.
However, there is a caveat. I attempted to implement logic around deactivating roles themselves in #4392, #4382. But the logic became very complex. We decided (#3587 (comment)) to go for a more architerurally simple solution of removing the deactivation functionality and just deleting the roles.
Drawbacks
There are two main drawbacks with this approach.
Existing Data
What do we do with existing discarded users? Discarded == soft deleted. I can imagine a world where a user was discarded without that meaning to be a "deactivation".
My current idea is that any User that is discarded and only had the
org_userrole was probably a deactivation and we can reactivate them and remove theorg_userrole from them. But this requires some more information.Loss of Granularity
Previously we had 3 states for
org_users: active, not anorg_userand deactivated (meaning they were a user at one point but not any more).Now there are only 2 states: user or not. This means we have a bit less information, we can no longer track if someone was once a user but not anymore. Does this matter? Probably not.
Type of change
How Has This Been Tested?
Request + System specs
Screenshots