-
Notifications
You must be signed in to change notification settings - Fork 1
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
HEEDLS-558 Show tracking system to UserAdmin #500
HEEDLS-558 Show tracking system to UserAdmin #500
Conversation
@@ -13,7 +13,7 @@ public class ApplicationSelectorController : Controller | |||
public IActionResult Index() | |||
{ | |||
var learningPortalAccess = User.GetCustomClaimAsBool(CustomClaimTypes.LearnUserAuthenticated) ?? false; | |||
var trackingSystemAccess = User.GetCustomClaimAsBool(CustomClaimTypes.UserCentreAdmin) ?? false; | |||
var trackingSystemAccess = User.GetCustomClaimAsBool(CustomClaimTypes.UserUserAdmin) | User.GetCustomClaimAsBool(CustomClaimTypes.UserCentreManager) | User.GetCustomClaimAsBool(CustomClaimTypes.UserCentreAdmin) ?? false; |
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.
This should be able to be replaced with User.HasCentreAdminPermissions(), I think the resulting bool is the same.
If it can't be replaced by that smaller helper method, you need to run the formatter on this to break up the long line. If you don't know how to do that drop me a message.
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 think the long line can be replaced with a claims helper method, otherwise it needs the formatter running.
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.
Looks good.
a3f9798
to
182deee
Compare
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.
Looks good! 👍
A small bug fix - I changed the tracking system access to accept users who are User Admins, Centre Managers, or Centre Admins, rather than assuming that if a user is a Centre Admin they have those other permissions.