4503 super admin create users #5097

Benjamin-Couey · 2025-03-15T17:15:26Z

Resolves #4503

Description

This PR modifies app/controllers/admin/users_controller.rband app/views/admin/users/new.html.erb so that the super admin form for inviting a new user lets one select both the role the new user should have and the resource that role should be associated with if necessary. For the super admin role, no resource is required and so the resource dropdown is hidden via a new stimulus controller.

This PR also modifies UserInviteService in order to fascilitate the above behavior. The service now permits nil as a resource when the sole role being added is super admin, and will automatically add the org user role if only the org admin role is specified.

It is possible that some of this logic does not belong in the UserInviteService. Notably, the AddRoleService already contains logic to automatically add the org user role when the org admin role is added. In theory, the UserInviteService could call the
AddRoleService instead of directly calling add_role. However, AddRoleService relies on being passed the id of the user to add roles to, and newly created users won't have an id until after the invitation. While it would be possible to refactor AddRoleService, that seemed outside the scope of this PR and so the current solution was selected.

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

This was manually tested to verify that the form worked as expected when creating all the different roles.

Tests were added to spec/requests/admin/users_requests_spec.rb to verify that post requests could create new users with all of the possible roles (org user, org admin, partner, super admin) and that the request would fail and report an error if a role wasn't specified, or a resource wasn't specified for a role which required one. Additionally, tests were modified and added to verify that the available roles were loaded for the new and create actions.

Tests were added to spec/services/user_invite_service_spec.rb to verify that the service will accept a nil resource for the super admin role, raise an error for a nil resource for other roles, and that it will automatically add the org user role if only the org admin role is specified.

Tests were added to spec/system/admin/users_system_spec.rb to verify the workflow of a super admin creating an org user and super admin user. Tests were also added to verify that the resource dropdown is hidden when the super admin role is selected, and a flash message is shown if the form is submitted without selecting a resource with a role that requires one.

Screenshots

The new user form, showing the resource dropdown for an organization role

The new user form, hiding the resource dropdown for a the super admin role

The new user form, showing the organizations in the dropdown

The new user form, showing the partners in the dropdown

…urce

…esources should be available seems like it would come down to the user's permissions, which a controller should handle

…le type is selected

…der of role-resource and double-select controller action doesn't impact behavior

…s super admin, added comments explaining constraints

…sers in addition to org users

… user if only ORG_ADMIN role is specified

…types

…rams

…checking for error messages on missing role type or resoruce id

…verify organizations are preloaded for the new user page as this is no longer the case

…sers, automatically add the ORG_USER role, and raise an error when no resource is provided

…a user

…e hidden if super admin role is selected

…isn't selected

cielf

Thanks Benjamin...
Please address this failing sequence.....

Sign in as superadmin
Add a superuser (entirely new user -- I used myself)
accept the invitation (you'll want to sign out first -- I use a different browser as my default so I didn't have to)
As that new user
Users|All Users
find the [email protected] user
Edit

BOOM!

Edit -- you don't have to go into the new user -- you can just try editing any superuser, including the one you just added.

Benjamin-Couey · 2025-03-19T15:36:07Z

Thanks Benjamin... Please address this failing sequence.....

Sign in as superadmin Add a superuser (entirely new user -- I used myself) accept the invitation (you'll want to sign out first -- I use a different browser as my default so I didn't have to) As that new user Users|All Users find the [email protected] user Edit

BOOM!

Edit -- you don't have to go into the new user -- you can just try editing any superuser, including the one you just added.

Apologies for missing this. The error you're describing I also encountered while working on PR #5047 and since that PR fixed the bug and I didn't replicate the fix for this one. #5047 is merged into main now so it should be working.

There was also a test failing due to a small mistake I made in #5047 though that is fixed now as well. I suspect the remaining failing test, rspec ./spec/services/itemizable_update_service_spec.rb:128, is a flaky one as it's unrelated to any changes this PR makes, the test is passing locally for me, and that test has previously been reported as flaky here.

Benjamin-Couey · 2025-03-19T15:47:31Z

Again, apologies for missing this but looks like the failing test I mentioned fixing was already addressed by @coalest here.

Since this fix is really unrelated to this PR (it's an old issue I missed somehow) does it make sense to revert my commit and use coalest's PR for the fix?

coalest · 2025-03-19T15:55:27Z

@Benjamin-Couey Whoops, didn't realize you had already fixed that test. Let me know if I should close my PR

Benjamin-Couey · 2025-03-19T18:26:16Z

Sorry, I'm going back and forth on this. I still think the fix is tangential to the issue in this PR but maybe that's fine? It makes it easier for reviewers to test the behavior? That's probably the reason to keep the commit and delete the PR.

cielf · 2025-03-20T13:45:42Z

LGTM functionally -- over to @dorner or @awwaiid for technical review.

Addressed

dorner · 2025-03-21T19:20:59Z

app/controllers/admin/users_controller.rb

-    raise "Please select an organization for the user." if organization_id.blank?
-
-    organization_id
+    raise "Please select an associated resource for the role." if params[:resource_type].to_s != Role::SUPER_ADMIN.to_s && params[:resource_id].blank?


Rather than hardcoding SUPER_ADMIN, can we have a set of non-resource roles in the Role class which right now would only have SUPER_ADMIN as a member of the set? If we ever add roles to this set (which we have discussed in the past) it would mean not having to change this.

Makes sense. These two commits address this.

dorner · 2025-03-21T19:22:25Z

app/javascript/controllers/role_resource_controller.js

Stimulus controllers should be defined generically whenever possible. Here this has nothing to do with roles and more to do with toggling classes on the destination target. You should be able to pass in the set of "toggle values" to the controller.

These recent commits make this change and rename the controller to be more generic and descriptive.

dorner · 2025-03-21T19:22:55Z

app/controllers/admin/users_controller.rb

+
+  def load_resources
+    @resources = Role::TITLES.invert
+  end


Can you not just reference this in the view? Seems odd to have a separate callback to "load" these in the controller - this is not hitting the database.

This recent commit makes this change.

dorner · 2025-03-21T19:23:11Z

app/services/user_invite_service.rb

+    # multiple roles being based makes sense is ORG_USER and ORG_ADMIN.
+
+    # Resource can be nil when the only role being added is the SUPER_ADMIN role.
+    # binding.pry


Remove comment please

This is addressed in the recent commit.

dorner · 2025-03-21T19:23:26Z

app/services/user_invite_service.rb

+
+    # Resource can be nil when the only role being added is the SUPER_ADMIN role.
+    # binding.pry
+    raise "Resource not found!" if resource.nil? && roles.first.to_s != Role::SUPER_ADMIN.to_s


See above - we should use the same list of "no-resource roles".

This recent commit makes this change.

dorner · 2025-03-21T19:24:02Z

app/services/user_invite_service.rb

+    # The logic is placed here instead of relying on the AddRoleService, as that
+    # currently only accepts users by id, and new user will not have an id
+    # until after they are invited.
+    if roles.all? { |role| role.to_s == Role::ORG_ADMIN.to_s }


This seems odd because it implies that we can give users multiple org admin roles, which we shouldn't be able to do in a single call.

I had done this because I needed to cast all the roles to string for the comparison (there are examples of roles being passed as string and symbol) but I agree it creates a strange implication.

This recent commit address this.

…serInviteService to use this definition

…ews can just reference Role::TITLES directly

…l against and passed new Role::ROLES_WITHOUT_RESOURCE

…r role should be added for each org_admin role

…s error if any of the submitted roles require a resource and one isn't provided, added associated test to verify

…OLES_WITHOUT_RESOURCE

Benjamin-Couey · 2025-03-25T14:10:52Z

@dorner Apologies for the wait, but I believe my recent commits address your comments.

dorner

Getting closer!

dorner · 2025-03-27T01:11:04Z

app/javascript/controllers/hide_by_source_val_controller.js

+export default class extends Controller {
+		static targets = ["source", "destination"]
+		static values = {
+			valuestohide: Array


this is JavaScript, so this should be camelCase: valuesToHide. You would call it as values-to-hide in the HTML.

Should be addressed in the recent commit.

dorner · 2025-03-27T01:11:45Z

app/javascript/controllers/hide_by_source_val_controller.js

+import { Controller } from "@hotwired/stimulus"
+
+export default class extends Controller {
+		static targets = ["source", "destination"]


The spacing in this file seems very out of whack.

Should be addressed in the recent commit.

dorner · 2025-03-27T01:12:14Z

app/javascript/controllers/hide_by_source_val_controller.js

+		sourceChanged() {
+				const val = $(this.sourceTarget).val()
+				this.destinationTargets.forEach(
+					destination_target => { $(destination_target).toggleClass("d-none", this.valuestohideValue.includes(val)); }


Suggested change

destination_target => { $(destination_target).toggleClass("d-none", this.valuestohideValue.includes(val)); }

destinationTarget => { $(destinationTarget).toggleClass("d-none", this.valuesToHideValue.includes(val)); }

Should be addressed in the recent commit.

dorner · 2025-03-27T01:13:17Z

app/services/user_invite_service.rb

+    # The logic is placed here instead of relying on the AddRoleService, as that
+    # currently only accepts users by id, and new user will not have an id
+    # until after they are invited.
+    if roles.map(&:to_s).include?(Role::ORG_ADMIN.to_s)


Why do we need to cast both the role and the comparison to string, if they're both symbols?

Thanks for pushing back on this. I had got it in my head that UserInviteService.invite was called with role as both a symbol and string. Looking more closely, it was only my modifications to Admin::UsersController that were submitting a string.

Should be addressed in the recent commit.

dorner · 2025-03-27T01:14:30Z

app/views/admin/users/new.html.erb

                      <%= render 'admin/users/user_form_fields', f: f %>

+                      <div data-controller="double-select" data-double-select-url-value="<%= resource_ids_admin_users_url %>">
+
+                        <div data-controller="hide-by-source-val" data-hide-by-source-val-valuestohide-value="<%= Role::ROLES_WITHOUT_RESOURCE.to_json %>">


You don't need a second div, you can add all the values to the same div:

<div data-controller="double-select hide-by-source-val" data-double-select-url-value="<%= resource_ids_admin_users_url %>" data-hide-by-source-val-values-to-hide-value="<%= Role::ROLES_WITHOUT_RESOURCE.to_json %>">

Should be addressed in the recent commit.

… div and removed redundant div

…vice to symbol, removed unnecessary casts from UserInviteService, ran linter

Benjamin-Couey · 2025-03-27T22:05:33Z

@dorner my recent commits should address your latest comments. There is a failing test, rspec ./spec/system/adjustment_system_spec.rb[1:5:2:1], but it appears unrelated to the changes I made and is passing locally which makes me suspect it might just be flaky?

I will look further into it and report what I find.

Benjamin-Couey and others added 19 commits March 2, 2025 08:06

Updated admin user controller to load super admin as a potential reso…

d839d9b

…urce

Removed resources_for_select from Role model as deciding which role r…

8b82feb

…esources should be available seems like it would come down to the user's permissions, which a controller should handle

First pass at updating new form to hide resources when super admin ro…

dd99b99

…le type is selected

Changed role-resource controller's target to be a wrapping div, so or…

e33e0cd

…der of role-resource and double-select controller action doesn't impact behavior

Updated UserInviteService to accept nil as a resource when the role i…

9cb81cc

…s super admin, added comments explaining constraints

Updated create action to handle creation of partner and super admin u…

d0b772e

…sers in addition to org users

Updated user invite service to handle adding ORG_USER role to invited…

239a0b2

… user if only ORG_ADMIN role is specified

Expanded admin users request tests to check creation of all new user …

496d4d4

…types

Added function to validate presence of necessary role and resource pa…

9549ded

…rams

Replaced test checking for error message on missing org id with test …

09886b5

…checking for error messages on missing role type or resoruce id

Fixed typo

77e16e5

Added tests to verify potential roles are preloaded, removed test to …

cc9fdce

…verify organizations are preloaded for the new user page as this is no longer the case

Added tests to verify that UserInviteService can invite super admin u…

ef7e531

…sers, automatically add the ORG_USER role, and raise an error when no resource is provided

Updated system admin user test to reflect new interface for creating …

2c83970

…a user

Added test to system admin user to specify resource dropdown should b…

ef742fa

…e hidden if super admin role is selected

Added tests to system admin user to specify behavior when a resource …

acf280b

…isn't selected

Changes made by linter

6c0d011

Tweaked comment wording

c985edf

Merge branch 'main' into 4503-super-admin-create-users

17e88ea

cielf previously requested changes Mar 17, 2025

View reviewed changes

Benjamin-Couey and others added 2 commits March 19, 2025 07:03

Merge branch 'main' into 4503-super-admin-create-users

2375d0d

Forgot to specify organization name causing test to fail

e48661c

cielf requested review from awwaiid and dorner March 20, 2025 13:46

dorner requested changes Mar 21, 2025

View reviewed changes

Benjamin-Couey and others added 10 commits March 23, 2025 08:14

Added definition of roles that don't require a resource and updated U…

27ff01f

…serInviteService to use this definition

Removed load_resources action as it doesn't touch the database, so vi…

e06585d

…ews can just reference Role::TITLES directly

Updated new controller to accept array of values to check source's va…

10e0948

…l against and passed new Role::ROLES_WITHOUT_RESOURCE

Renamed new controller to something more descriptive and generic

069e479

Reworked check for presense of org_admin to not imply that an org_use…

fb52835

…r role should be added for each org_admin role

In case ROLES_WITHOUT_RESOURCE expands, user invite service now raise…

aeb12cf

…s error if any of the submitted roles require a resource and one isn't provided, added associated test to verify

Changes made by linter

518035d

Updated validate_role_resource_params to check params against Role::R…

05bc646

…OLES_WITHOUT_RESOURCE

Merge branch 'main' into 4503-super-admin-create-users

3fa7aea

Fixed typos

17a07b5

dorner requested changes Mar 27, 2025

View reviewed changes

Benjamin-Couey and others added 4 commits March 27, 2025 11:26

Put both double-select and hide-by-source-val data controllers in one…

7faa245

… div and removed redundant div

Made variable name camelCase and fixed spacing

62e6f50

Fixed Admin::UsersController not casting role passed to UserInviteSer…

32638c6

…vice to symbol, removed unnecessary casts from UserInviteService, ran linter

Merge branch 'main' into 4503-super-admin-create-users

f1bcaad

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

4503 super admin create users #5097

4503 super admin create users #5097

Benjamin-Couey commented Mar 15, 2025

cielf left a comment •

edited

Loading

Benjamin-Couey commented Mar 19, 2025 •

edited

Loading

Benjamin-Couey commented Mar 19, 2025

coalest commented Mar 19, 2025 •

edited

Loading

Benjamin-Couey commented Mar 19, 2025

cielf commented Mar 20, 2025

dorner Mar 21, 2025

Benjamin-Couey Mar 25, 2025 •

edited

Loading

dorner Mar 21, 2025

Benjamin-Couey Mar 25, 2025 •

edited

Loading

dorner Mar 21, 2025

Benjamin-Couey Mar 25, 2025 •

edited

Loading

dorner Mar 21, 2025

Benjamin-Couey Mar 25, 2025 •

edited

Loading

dorner Mar 21, 2025

Benjamin-Couey Mar 25, 2025 •

edited

Loading

dorner Mar 21, 2025

Benjamin-Couey Mar 25, 2025 •

edited

Loading

Benjamin-Couey commented Mar 25, 2025

dorner left a comment

dorner Mar 27, 2025

Benjamin-Couey Mar 27, 2025

dorner Mar 27, 2025

Benjamin-Couey Mar 27, 2025

dorner Mar 27, 2025

Benjamin-Couey Mar 27, 2025

dorner Mar 27, 2025

Benjamin-Couey Mar 27, 2025

dorner Mar 27, 2025

Benjamin-Couey Mar 27, 2025

Benjamin-Couey commented Mar 27, 2025

	destination_target => { $(destination_target).toggleClass("d-none", this.valuestohideValue.includes(val)); }
	destinationTarget => { $(destinationTarget).toggleClass("d-none", this.valuesToHideValue.includes(val)); }

4503 super admin create users #5097

Are you sure you want to change the base?

4503 super admin create users #5097

Conversation

Benjamin-Couey commented Mar 15, 2025

Description

Type of change

How Has This Been Tested?

Screenshots

cielf left a comment • edited Loading

Choose a reason for hiding this comment

Benjamin-Couey commented Mar 19, 2025 • edited Loading

Benjamin-Couey commented Mar 19, 2025

coalest commented Mar 19, 2025 • edited Loading

Benjamin-Couey commented Mar 19, 2025

cielf commented Mar 20, 2025

Choose a reason for hiding this comment

Benjamin-Couey Mar 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benjamin-Couey Mar 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benjamin-Couey Mar 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benjamin-Couey Mar 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benjamin-Couey Mar 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benjamin-Couey Mar 25, 2025 • edited Loading

Choose a reason for hiding this comment

Benjamin-Couey commented Mar 25, 2025

dorner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benjamin-Couey commented Mar 27, 2025

cielf left a comment •

edited

Loading

Benjamin-Couey commented Mar 19, 2025 •

edited

Loading

coalest commented Mar 19, 2025 •

edited

Loading

Benjamin-Couey Mar 25, 2025 •

edited

Loading

Benjamin-Couey Mar 25, 2025 •

edited

Loading

Benjamin-Couey Mar 25, 2025 •

edited

Loading

Benjamin-Couey Mar 25, 2025 •

edited

Loading

Benjamin-Couey Mar 25, 2025 •

edited

Loading

Benjamin-Couey Mar 25, 2025 •

edited

Loading