3372: Allow users to be added to multiple roles #3386

dorner · 2023-02-17T20:45:32Z

Resolves #3372

Description

With the new role capability, there should be nothing stopping a user from belonging to multiple organizations or partners if necessary. This change ensures that the current role (organization or partner) is reflected in whatever the user sees, which allows for the role switcher to work in all cases.

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Local and unit tests.

awwaiid · 2023-02-26T02:42:23Z

I don't normally ask for a history squash/rewrite .... but in this case ....... :)

dorner · 2023-02-28T14:42:23Z

Most of those commits were unrelated to the actual ticket. I've deleted them outright and they should no longer be necessary now that importmaps have been merged.

cielf · 2023-03-03T03:46:26Z

@dorner IIRTC, at least some of the tests that are failing are related to this subject matter...

cielf · 2023-03-03T04:25:21Z

spec/controllers/donations_controller_spec.rb

@@ -160,22 +160,6 @@
        expect(subject).to redirect_to(dashboard_path)
      end
    end
-


I'm not sure why you are removing these tests. On the surface it seems like we would still want to make sure that you can't access donations for a different org, and that you have to be signed in to access the donations

True - let me rejig these instead. The issue is that it was depending on the organization_id in the parameter, which is no longer the real indication of which organization you're looking at. In fact, we could remove that organization_id which we add to every URL - it makes it a bit confusing.

Ah I know why I removed them - because I added application-level tests instead. So rather than testing a single controller we're now testing every controller in the app 😄

Alright, then.

cielf · 2023-03-03T04:33:43Z

@dorner. I've skimmed the changes, and IIRTC, if someone gets invited to a second role, they are not getting invited. Which, on the one hand, means they don't have to accept the second invite, and have the whole annoying "you can't sign in, you have a pending invitation" thing. Yay!

But there would be value in them getting notified that they have been invited to the second role, which, IIRTC, they won't be? We've already seen a couple of cases where someone has signed up as a bank (when they probably shouldn't have), and then years later, is being brought on as a partner. Although we don't want that tail wagging our dog, it will be a support ticket every time a bank expects an invitation to be sent, but the user doesn't get any notification.

cielf · 2023-03-03T04:37:32Z

app/controllers/application_controller.rb

+
+    return current_role.resource if current_role.resource.is_a?(Organization)
+
+    Organization.find_by(short_name: params[:organization_id])


I'm not sure why you aren't setting @current_organization if it isn't already set, in the new version. That feels like it could have some side effects.

Nope. Did a search and nothing else reads this instance variable (as it should be).

I don't see anything that ever sets the instance variable?

cielf · 2023-03-03T04:54:45Z

Hopefully this is nearly the final nail in the grand edifice you've been building over these last few months!
I'll want to manually test the partner switching thing, though-- I've slotted that in for tomorrow afternoon.

cielf · 2023-03-03T13:58:47Z

app/services/user_invite_service.rb

@@ -9,7 +9,6 @@ def self.invite(email:, resource:, name: nil, roles: [])

    user = User.find_by(email: email)
    if user
-      user.invite!


How does this interact with the ability of the banks to resend the invitations to the partner? Maybe invite! if the user has not yet accepted their invitation/set a password?

There should be a way to force this behavior. Let me take a look.

cielf · 2023-03-03T17:16:36Z

Hopefully this is nearly the final nail in the grand edifice you've been building over these last few months! I'll want to manually test the partner switching thing, though-- I've slotted that in for tomorrow afternoon.

The partner switching thing seems to work now on local, anyway. Hooray!

dorner · 2023-03-24T21:10:17Z

@cielf made some changes and fixes addressing the comments! Need to wait for tests to run but please take another look.

cielf · 2023-03-29T00:36:29Z

@dorner Well, it looks like there are tests failing in the general area of the change -- I can take a look (probably tomorrow), but might I just as well wait until those are resolved?

cielf · 2023-04-01T21:43:49Z

@dorner As to the interplay between this and.. #3429 .. How does someone know that they have been invited to a second partner or diaper bank -- We may need to take a step back and draw out all the scenarios.

dorner · 2023-04-03T00:13:34Z

@cielf in this PR I added another e-mail to be sent out whenever a role is added to an existing user.

Not sure when I'll have time to look at the failing tests. 😦

cielf · 2023-04-03T00:16:59Z

'Salright -- I expect to not see much of you for the next week. We're purposely not doing a release next weekend, also.

dorner · 2023-04-14T21:03:38Z

Tests should all pass now! 🤞

cielf · 2023-04-26T14:46:48Z

Hey @dorner... There's a specific situation that this is not covering.
The 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 will get a 'PG::UniqueViolation: ERROR: duplicate key value violates unique constraint "index_users_on_email" DETAIL: Key (email)=(user_1@example.com) already exists.'

And it's not far-fetched, I'm afraid -- we have essentially that situation at the moment (see support starting at 3:38 on Monday April 24)
Does that needs to be a separate issue -- that when you deactivate a user as a bank, you are removing the role, rather than removing their ability to log in?

dorner · 2023-04-27T00:51:12Z

Yeah that does sound like a separate issue.

cielf · 2023-05-06T22:08:44Z

Have added the additional proto-issue to the backlog

cielf · 2023-05-24T19:13:49Z

Putting a reminder for @awwaiid here too!

awwaiid

All looks good

awwaiid · 2023-06-11T00:26:32Z

app/controllers/application_controller.rb

+
+    return current_role.resource if current_role.resource.is_a?(Organization)
+
+    Organization.find_by(short_name: params[:organization_id])


I don't see anything that ever sets the instance variable?

awwaiid · 2023-06-11T15:27:49Z

(I walked through all the code changes and did local manual testing)

dorner requested review from awwaiid and cielf February 17, 2023 20:45

3372: Allow users to be added to multiple roles

ceeb9d3

dorner force-pushed the 3372-multi-partner branch from 6321116 to ceeb9d3 Compare February 28, 2023 14:41

cielf reviewed Mar 3, 2023

View reviewed changes

Daniel Orner added 4 commits March 24, 2023 16:26

Merge branch 'main' into 3372-multi-partner

a81157d

Always reinvite users

9a0b4c0

Send notification on role add

a4a9fc3

Lint fixes

74a9c39

dorner mentioned this pull request Mar 26, 2023

Add PlantUML processing and example #3219

Merged

Daniel Orner added 2 commits April 14, 2023 17:02

Spec fixes!

c51ba90

Rubocop fixes

1e26bb7

Daniel Orner added 3 commits April 17, 2023 09:11

Merge branch 'main' into 3372-multi-partner

b969591

Spec fix - looks like unrelated to my changes?

04539fc

Raise error

424def4

cielf mentioned this pull request May 24, 2023

Deactivating users should address the role, rather than deactivating the user per se #3587

Closed

3 tasks

awwaiid approved these changes Jun 11, 2023

View reviewed changes

awwaiid merged commit 68a71c6 into main Jun 11, 2023

awwaiid deleted the 3372-multi-partner branch June 11, 2023 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3372: Allow users to be added to multiple roles #3386

3372: Allow users to be added to multiple roles #3386

dorner commented Feb 17, 2023

awwaiid commented Feb 26, 2023

dorner commented Feb 28, 2023

cielf commented Mar 3, 2023

cielf Mar 3, 2023 •

edited

Loading

dorner Mar 24, 2023

dorner Mar 24, 2023

cielf Mar 29, 2023

cielf commented Mar 3, 2023

cielf Mar 3, 2023

dorner Mar 24, 2023

awwaiid Jun 11, 2023

cielf commented Mar 3, 2023

cielf Mar 3, 2023

dorner Mar 24, 2023

cielf commented Mar 3, 2023

dorner commented Mar 24, 2023

cielf commented Mar 29, 2023 •

edited

Loading

cielf commented Apr 1, 2023

dorner commented Apr 3, 2023

cielf commented Apr 3, 2023

dorner commented Apr 14, 2023

cielf commented Apr 26, 2023 •

edited

Loading

dorner commented Apr 27, 2023

cielf commented May 6, 2023 •

edited

Loading

cielf commented May 24, 2023

awwaiid left a comment

awwaiid Jun 11, 2023

awwaiid commented Jun 11, 2023


		return current_role.resource if current_role.resource.is_a?(Organization)

		Organization.find_by(short_name: params[:organization_id])

3372: Allow users to be added to multiple roles #3386

3372: Allow users to be added to multiple roles #3386

Conversation

dorner commented Feb 17, 2023

Description

Type of change

How Has This Been Tested?

awwaiid commented Feb 26, 2023

dorner commented Feb 28, 2023

cielf commented Mar 3, 2023

cielf Mar 3, 2023 • edited Loading

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

cielf commented Mar 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cielf commented Mar 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cielf commented Mar 3, 2023

dorner commented Mar 24, 2023

cielf commented Mar 29, 2023 • edited Loading

cielf commented Apr 1, 2023

dorner commented Apr 3, 2023

cielf commented Apr 3, 2023

dorner commented Apr 14, 2023

cielf commented Apr 26, 2023 • edited Loading

dorner commented Apr 27, 2023

cielf commented May 6, 2023 • edited Loading

cielf commented May 24, 2023

awwaiid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awwaiid commented Jun 11, 2023

cielf Mar 3, 2023 •

edited

Loading

cielf commented Mar 29, 2023 •

edited

Loading

cielf commented Apr 26, 2023 •

edited

Loading

cielf commented May 6, 2023 •

edited

Loading