human-essentials
human-essentials copied to clipboard
rubyforgood
Replace columns with roles
Resolves #3098
Description
This introduces the use of the Rolify gem to manage user roles. This replaces the use of the following columns on the users table:
- organization_id
- partner_id
- super_admin
- organization_admin
Because roles are a many-to-many relationship, it allows us to easily create additional roles - for example, partner admin, or to associate a single user with multiple organizations or partners.
The "switch role" button on the top right has been changed to make use of roles.
One important change is that the relationship between users and organizations / partners now goes through multiple tables (because the idea that a user "belongs to" a single organization is no longer true). I was still able to define a relationship called "organization" which assumes that a user does belong to one organization, which is still the case in pretty much every situation right now. We can look at changing that later if we decide we have to... but I hope we don't.
The only UI that went through a significant change was the organization creation page, which used nested forms, which don't work when you introduce additional join tables. There was an "org admin?" checkbox on that page, but from what I could tell it was mostly ignored - when creating an org with a single user, I can't think of any use case where that single user wouldn't be an admin. Feel free to comment / question this change.
Type of change
- Internal change
How Has This Been Tested?
Local and unit tests.
Screenshots
Just writing down here some things I noticed while testing. I found that the role isn't added when adding a co-worker as a Org Admin via the dropdown options in my Co-workers.
Hey, @dorner nice work! I'am still going through this PR and I think maybe it is a good idea to work together to build out a breakout plan for what we need to either check or do to complete this set of work. Here is a short list of what I'am thinking already:
- [x] Address the issue posed above about inviting another "co-worker"/"user" to an organization
- [x] Align on role nomenclature, should we use the term "org_user' or "org_admin" or just "user" and "admin" and scope to the resource?
- [ ] Figure out what other areas we want to QA on + let's run this QA process on a production copy.
- [x] Align on next steps.
Happy to talk through this in the afternoon this week or on Sunday.
@edwinthinks nice call on the service module - that solved a lot of problems. Pretty sure everything should go through the same flow now, so it should all work! (I'll wait for confirmation from the spec suite though :D )
@dorner I think this is looking good! I think we can move to the phase of testing this on a production data set. I think covering the bases is inviting the various roles, making requests, and making distributions are sufficient.
How do you want to handle the QAing? Two QAs better than one aka you and I can test this on production copies locally?
@edwinthinks ideally yes, but my life is kind of crazy right now since we moved into a new house yesterday. I'm not sure I'll be able to dedicate any time to this for a couple of weeks.
@edwinthinks ideally yes, but my life is kind of crazy right now since we moved into a new house yesterday. I'm not sure I'll be able to dedicate any time to this for a couple of weeks.
No worries @dorner -- I'll queue up some time to do some QA work on the roles. I believe when I tested it last it was working well! I'll let ya know
Here is the list of items that I'am going to QA for visibility
- [x] Test migration works on production copy and login works as expected for: - Orgs User with both partner and organization roles - Orgs User with just org role - Partner with just partner role
- [x] Organization creation flow via Account Request
- [x] Organization inviting Partner and registering
- [x] Organization inviting other co-workers to join the organization
- [x] Partner inviting co-workers to join partner organization
@dorner I think this is good to go. I QA all that I mentioned above and I did notice one small thing that I think we can come back to later. The option to switch roles is still visible even if you are both org_admin and org_user, it is a slightly odd UX but I think it is okay and shouldn't block this.
Let me know if you agree that we are ready to merge in knowing that.
Interesting... I didn't see that when I tested. This code explicitly deals with that case https://github.com/rubyforgood/human-essentials/pull/3117/files#diff-9802ca3c9c4cf89904fd44bc114e35ebdf2c5dd3d5b645491e2b253e1afef29bR95
@dorner this happens when the session[:role_id] hasn't been defined yet and it sets it to the first role according to the definition of current_role. The issue with this is that the code you shared removes the ORG_USER role from the list of switchable_roles and so what I'am seeing is the system thinks I can switch to the ORG_ADMIN role:
What I'am seeing here is the option to switch to an org admin.
Does this make sense?
Ahhh the code doesn't check the current role when it removes it. I can fix this.
Hmm... actually, this case shouldn't be possible: https://github.com/rubyforgood/human-essentials/blob/3098-roles/app/controllers/users/sessions_controller.rb#L19
If you can be an org user and an org admin, you'll always be an org admin. You should never have the option to switch to org admin unless that role is for a different organization than your current role.
@dorner this is after accepting an invite that logs me in via different means, could that be the reason why it isn't working for this case?
Hmm... could be. Do you know what controller that goes through? Might have missed a flow.
@dorner it's handled by devise and I believe we can override the controller lightly to add that logic you shared above.
@edwinthinks can you share exact reproduction instructions? I can't seem to get to this point.
@dorner hey there, the way to get here is by accepting an invite from creating an organization. The flow starts from the splash page to request an account.
@edwinthinks found the issue - looks like the invitation flow doesn't go through the sessions#create method. I updated current_role to always pull the right order of roles. Going to wait for tests, but local testing seems to indicate this works.
Nice @dorner - I'll give it one last QA and if its good. Let's mergy merge