Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redirect to partners dashboard with flash message for org-related pages #5089

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

coalest
Copy link
Collaborator

@coalest coalest commented Mar 10, 2025

Resolves #4511

Description

  • Redirect to partners dashboard when user logged in as partner tries to access org user pages.

Type of change

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

How Has This Been Tested?

Only manually. Tests to come.

@coalest
Copy link
Collaborator Author

coalest commented Mar 10, 2025

@dorner Regarding testing this change, do you think we need to test every organization-related route (like I did with the distributions request specs already)? If so, that will take a while...

@coalest
Copy link
Collaborator Author

coalest commented Mar 10, 2025

@cielf If an org user tries to go to a partner page, they will receive this message:
Logged in user is not set up as a 'partner'.

And in this issue's description, it is saying if a partner tries to access an org page, they should receive this message:
That screen is not available. Please switch to the correct role and try again.

Do we want to make these messages be a little more consistent? (Ie make the second message more similar to the first or vice versa)

P.S. This PR is missing some tests, but from a functional view it's ready for testing (in theory). Although maybe we should wait for manually testing until the tests have been finished.

@coalest
Copy link
Collaborator Author

coalest commented Mar 10, 2025

Note: This issue also occurs for super admins trying to request individual org related pages. For example, a super admin trying to view /donations or /distributions pages. This PR also fixes that issue with the similar response — redirecting to the admin dashboard page with the flash message.

@cielf
Copy link
Collaborator

cielf commented Mar 10, 2025

@coalest -- I like the "That screen is not available. Please switch to the correct role and try again." message a bit better.

Though it would be better still to say "That screen is not available. Please switch to (insert bank name or partner name here) and try again." The users aren't as familiar with the term "role" as we are.

@cielf
Copy link
Collaborator

cielf commented Mar 10, 2025

@coalest Regarding the testing -- is it a fix in a general place? We know that we've seen the "NoMethod" error with a current_organization is nil in the following situations in the last 30 days: partners#index, dashboard#index, donations#index, users#index, distributions#index, adjustments#index. Those would be the highest priority.

@coalest
Copy link
Collaborator Author

coalest commented Mar 10, 2025

@cielf yea this fix is general. So that's why I wanted to double check before I go through and add the test for every organization request. I can go ahead and add specs for those actions (as both a partner and a super admin).

@coalest
Copy link
Collaborator Author

coalest commented Mar 11, 2025

@coalest -- I like the "That screen is not available. Please switch to the correct role and try again." message a bit better.

Though it would be better still to say "That screen is not available. Please switch to (insert bank name or partner name here) and try again." The users aren't as familiar with the term "role" as we are.

Questions:

  1. Do partners always have two roles (one partner one organizational)? Should the bank name default to "the correct role" if the partner doesn't have a second organization user role?
  2. I imagine the messages aren't as important for super admins, but what should it be there? Because super admins don't necessarily have one organization to reference directly. So when a super admin tries to access an org-related page, maybe "That screen is not available. Please switch to an organization user and try again."?

@coalest
Copy link
Collaborator Author

coalest commented Mar 11, 2025

Added specs for all the common pages mentioned above that trigger the 500 error.

@cielf
Copy link
Collaborator

cielf commented Mar 11, 2025

Most partners will only have a partner role -- we wouldn't expect ones that don't to be accessing bank-facing pages. We could just say "as a bank" or "as a partner" instead of getting specific -- That would cover off both cases.

Similarly for the super user.

@coalest
Copy link
Collaborator Author

coalest commented Mar 12, 2025

@cielf So instead of "That screen is not available. Please switch to the correct role and try again.", we will do "That screen is not available. Please try again as a partner." for partner pages and "...try again as a bank." for org pages?

@coalest
Copy link
Collaborator Author

coalest commented Mar 12, 2025

Also should this be added to the documentation somewhere?

@cielf
Copy link
Collaborator

cielf commented Mar 12, 2025

@cielf So instead of "That screen is not available. Please switch to the correct role and try again.", we will do "That screen is not available. Please try again as a partner." for partner pages and "...try again as a bank." for org pages?

Yes

@cielf
Copy link
Collaborator

cielf commented Mar 12, 2025

"Also should this be added to the documentation somewhere?" I don't think it needs to be --- they are doing something outside the "normal" way to use the program. We should maybe figure out why they are doing it that way -- does it save them keystrokes or something?

@coalest
Copy link
Collaborator Author

coalest commented Mar 13, 2025

Updated with the new flash message text, should be ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need a 'graceful landing' if someone goes to a screen they don't have access to while logged in.
2 participants