Closed jp524 closed 1 month ago
@jp524 this is an enormous PR which I can't possibly look through all at once. Can this be split up into logical pieces?
I think your comment is correct though - this is likely blocked on #4293 .
Closing this. #4331 has been created and contains commits that have been re-organized into smaller logical pieces.
Resolves #4240
Description
Now that roles are implemented, the URLs don’t need to be prepended with
organization_name
anymore. The role determines the organization that a user can access.The main things I changed are:
organization_url_options
anddefault_url_options
are removed since we don’t need to append theorganization_name
param anymoreroutes.rb
, view files and test files to no longer useorganization_name
Discussion
One thing remains up for discussion at this point: what behavior should be allowed for super admins?
My understanding of the current behavior is: super admins can access routes that are outside of the
/admin
namespace. This is done by prepending the URL with theorganization_name
. There is no way for a super admin to switch roles.In this PR since the
organization_name
is removed from the URL, the super admins won’t be able to access routes outside of the/admin
namespace unless theorganization_name
param is added to the URL. I removed a couple of links that would bring a super admin in the/admin
namespace to a place outside of it (see example of such link in screenshot below).Until a PR exists for #4293, super admins won’t be able to access routes outside of
/admins
. If this is not okay I can find a way to automatically append theorganization_name
param to any request made by a user with super admin role.Type of change
/admin
namespace (see Discussion section)How Has This Been Tested?
Tests files have been updated to not use the
organization_name
param. I ran the test suite locally every so often to ensure changes were having the intended effect. I also addressed some flakiness I was encountering in the audit system tests by implementing the fix described in PR #4248. I’m aware that this is not an ideal solution but it helps for now.