-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat: add dept admin, add facility admin #737
base: main
Are you sure you want to change the base?
Conversation
5ca52cf
to
ac6b1b7
Compare
ac6b1b7
to
7a25366
Compare
Learning Platforms | ||
</Link> | ||
</li> | ||
{(isSysAdmin(user) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we do something like a canSwitchFacilities
that does that check, that way it can be used both ways and we can distill it down to one point of entry in case anything changes or we add roles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a couple minor things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
during testing I found a bug that should be fixed. So the issue is that you can duplicate a user in the system by character case and this will cause issue with a user with the same name logging onto the system. I believe the issue is with line 155 where the user.username within sql should be wrapped in a function to make the case of the username lowercase.
db.Raw("SELECT EXISTS(SELECT 1 FROM users WHERE username = ? OR email = ?)", strings.ToLower(username), email)
SET role = 'admin' | ||
WHERE role = 'department_admin'; | ||
|
||
delete from public.user_roles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the goose down statement is ran here it will violate foreign key constraints because the user is mapped to the user_roles. Should the users with facility_admin role be updated to just admin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure to tell you the truth, The idea with this change currently is that, any current admins when this migration happens, basically already have department admin controls, so, we switch them to department admin, and then any admins created from those department admins, will actually be facility admins, I don't know enough about goose to know if this is an issue, is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, it's an issue with goose. It will cause the script to fail. To be on the safe side I think you should change your goose down to: (basically just the update statement was changed here)
INSERT INTO public.user_roles(name) VALUES ('admin');
UPDATE public.users
SET role = 'admin'
WHERE role IN ( 'department_admin', 'facility_admin' );
delete from public.user_roles
where name in ('department_admin', 'facility_admin');
@@ -34,8 +34,10 @@ func (db *DB) GetCurrentUsers(qCtx *models.QueryContext, role string) ([]models. | |||
} | |||
tx := db.Model(&models.User{}).Where("facility_id = ?", qCtx.FacilityID) | |||
switch role { | |||
case "admin": | |||
tx = tx.Where("role IN ('admin', 'system_admin')") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed when logging in as a SuperAdmin I was unable to see SuperAdmin witthin the Admin listing screen, is this correct functionality?
@@ -22,7 +22,9 @@ export default function ExpandableCardGrid<T>({ | |||
const [expanded, setExpanded] = useState<boolean>(false); | |||
const slice = expanded ? items.length : cols; | |||
const isAdmin = | |||
user?.role === UserRole.Admin || user?.role === UserRole.SystemAdmin; | |||
user?.role === UserRole.FacilityAdmin || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you call isAdministrator(user)
here? import isAdministrator
@@ -34,6 +34,7 @@ export default function AdminLayer1() { | |||
useEffect(() => { | |||
console.log(timeFilter); | |||
}, [timeFilter]); | |||
console.log(favoritedLibraries?.data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this console.log come out here?
@@ -125,9 +125,13 @@ export default function HelpfulLinksManagement() { | |||
mutate={updateLinks} | |||
showModal={showModifyLink} | |||
role={ | |||
isAdministrator(user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking at the HelpfuLlinkCard
you should only need to pass the user role here and not have to execute any role checks here. The underlying logic handles checking the role using AdminRoles.includes(role)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the call to load data initially and on refresh for facility_admin roles is loading 'all' cache by default
/api/users/${user?.id}/admin-layer2?facility=${facility}&reset=${resetCache}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the call to load data initially and on refresh for facility_admin roles is loading 'all' cache by default
/api/login-metrics?facility=${facility}&days=${days}&reset=${resetCache}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When adding a Program as a facility administrator, i noticed that the program doesn't show up in the users listing screen?
Description of the change
This pull request slightly revamps the admin roles and abilities. Pre this pull-request we had two different types of admins, system admins, and admins, system admins were presumed to be a member of Unlocked Labs, while admins were presumed to be an administrator user from the department of corrections. This had very little separation of concerns and warranted a standard admin to access every facility within that running instance of the applications database, warranting potential worrisome conditions, as a department of corrections may not want an administrator from one facility having power over the users, and or admins of another facility. We fix this concern in this pull request by creating Department Admins, and Facility Admins, System Admins did not change. Currently this does not separate things out too much deeper, but it does allow for a separation of concerns and abilities.
Facility Admins
Department Admins
System Admins
No Changes Made
Related issues: Closes FEAT: New Roles > Department Admin and Facility Admin #715
Screenshot(s)
Video shows users being created, in a linear fashion from system admin -> Dept Admin -> Facility admin
https://www.loom.com/share/738b6d35e0f5452786c122d944683052?sid=7371616d-2b4f-453c-b7e7-ff16ad36a5cb
Facility drop down for programs (staging)
![image](https://private-user-images.githubusercontent.com/163448150/412760729-00c59ffc-0a7f-4d44-82d0-9fd185bf1fb1.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk2MTA0ODUsIm5iZiI6MTczOTYxMDE4NSwicGF0aCI6Ii8xNjM0NDgxNTAvNDEyNzYwNzI5LTAwYzU5ZmZjLTBhN2YtNGQ0NC04MmQwLTlmZDE4NWJmMWZiMS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjE1JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxNVQwOTAzMDVaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT0wNWY0ZjU4MzdkMjNlMWE4OGE0YTc2Y2Q3MTg5NDZiMGJhNGYwOGQwYjc4NThiMzY1NzI3NzMyMDNmMTU5MTNmJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.FTNCxWCUkQGcsMHsQvUqnay_NabhYYZW6huYrEe6_h4)
Additional context
Also, while working through things I noticed that the facility drop down on the Programs page was not mapping and or receiving any facilities, this was because the route loader in Programs.tsx was conflicting with the route loader being used in app.tsx, I moved the routeloader to a parent component, and this fixed the issue. Please reference the video, where I explicitly show the facilities drop down on the programs page, comparative to the screenshot that was taken in staging.