-
Notifications
You must be signed in to change notification settings - Fork 3
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(role based view): implemented a role based view for user groups #175
Conversation
} | ||
} catch (e) { | ||
console.log({ e }); | ||
return { user: null }; | ||
return { user: null } satisfies OneMacUser; |
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.
satisfies
😎 TypeScript boss right there!
<div className="w-full px-4 py-1 lg:px-8 text-xs mx-auto flex gap-2 items-center justify-center bg-red-200 "> | ||
<p className="text-center text-base"> | ||
You do not have access to view the entire application.{" "} | ||
<span className="text-blue-600">Please visit IDM</span> to request |
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 a link to IDM
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.
link from correct link later.
|
||
useEffect(() => { | ||
if (data?.user?.["custom:cms-roles"] === "onemac-micro-statesubmitter") { | ||
return navigate("/"); |
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 sure if it's worth changing or not, but figured I'd throw a fun fact/tip in. React Router v6 has two ways to navigate! You can use the hook function, or the <Navigate />
element. The way you've done it here is the former, and if you wanted to you could also do something like this:
return data?.user?.["custom:cms-roles"] === "onemac-micro-statesubmitter"
? <Navigate to={ROUTES.HOME} />
: <OsProvider> ... </OsProvider>
Again, not sure there's a benefit of one over the other in this case, but it's a nifty tool!
|
||
useEffect(() => { | ||
if (data?.user?.["custom:cms-roles"] === "onemac-micro-statesubmitter") { | ||
return navigate("/"); |
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.
Within the navigate()
arguments, we should be using the ROUTES
constant instead of string paths
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.
Navigate is still using the hard string instead of ROUTES.HOME
const getLinks = (isAuthenticated: boolean) => { | ||
if (isAuthenticated) { | ||
const getLinks = (isAuthenticated: boolean, role?: string) => { | ||
if (isAuthenticated && role !== "onemac-micro-statesubmitter") { |
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.
Are we sure this is right? State Submitters will need Dashboard access as far as I know
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.
Thanks Keven, This is absolutely not the role we want, i was using this to test. i have a meet to mike at 10. This was what i needed is thought on regarding users. this is also causing e2e to fail because the test is looking for the dashboard to indicate the user is logged in
"attributes": [ | ||
{ | ||
"Name": "email", | ||
"Value": "[email protected]" |
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.
Bad football? This must be Sheffield United's email address 😂
|
||
export const UsaBanner = () => { | ||
const [isOpen, setIsOpen] = useState(false); | ||
const isDesktop = useMediaQuery("(min-width: 640px)"); | ||
const userContext = useUserContext(); | ||
const role = userContext?.user?.["custom:cms-roles"]?false:true; |
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.
Since userContext
is technically returning a React State item, role would be safer with useMemo
. React State is fickle in my experience, so hooks like useMemo
allow us to derive values from a piece of state and update that value when the state changes. We can also simplify your later check by including the userContext?.user
boolean check from line 66 in the memoized variable.
For example:
const role = userContext?.user?.["custom:cms-roles"] ? false : true;
// ...
{ role && userContext?.user && ( ... )}
can be memoized and used as such
const hasAccess = useMemo(() =>
// checks both user data is present and user has cms-roles
userContext?.user && !userContext?.user?.["custom:cms-roles"],
[userContext] // this is the "dependency array", any React State used in memos are dependencies
)
// ...
{ hasAccess && ( ... )}
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.
@@ -83,14 +84,16 @@ const ResponsiveNav = ({ isDesktop }: ResponsiveNavProps) => { | |||
const [prevMediaQuery, setPrevMediaQuery] = useState(isDesktop); | |||
const [isOpen, setIsOpen] = useState(false); | |||
const { isLoading, isError, data } = useGetUser(); | |||
const userContext = useUserContext(); | |||
const role = userContext?.user?.["custom:cms-roles"]?true: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.
Also a candidate for useMemo
|
||
useEffect(() => { | ||
if (data?.user?.["custom:cms-roles"] === "onemac-micro-statesubmitter") { | ||
return navigate("/"); |
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.
Navigate is still using the hard string instead of ROUTES.HOME
@@ -59,6 +63,16 @@ export const UsaBanner = () => { | |||
</div> | |||
</button> | |||
)} | |||
{role && userContext?.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.
This condition is a bit confusing because at first glance it looks like a user would have a role with access but we're showing a banner communicating they don't have access. Something like { !hasAccess && ( ... )}
would represent the intent a lot better. It reads more like "if they don't (!) have access (hasAccess), then render (...)"
label: "Status", | ||
cell: (data) => (props?.isCms ? data.cmsStatus : data.stateStatus), | ||
cell: (data) => (props?.isCms ? data.stateStatus : data.stateStatus), |
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.
Lines 46 and 48 here are both saying "If the user is CMS, show them the stateStatus, else, show them the stateStatus". Why wouldn't we want to show them the CMS value if they're CMS?
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.
Based on what we talked about, I think the check should be more like
(props.isCms && !props.user.[cms-roles].includes(UserRole.HELPDESK) ? cmsStatus : stateStatus
That way we still show CMS statuses to everyone at CMS except the helpdesk as the AC says
label: "Status", | ||
cell: (data) => (props?.isCms ? data.cmsStatus : data.stateStatus), | ||
cell: (data) => (props?.isCms ? data.stateStatus : data.stateStatus), |
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.
Same as above
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.
Code looks good!
🎉 This PR is included in version 1.5.0-val.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Purpose
The purpose of this ticket was to implement a logic that allows us to control the user's view in the us based on user role
Linked Issues to Close
https://qmacbis.atlassian.net/browse/OY2-25702
Approach
Wrote a logic that restrict dashboard view for users without proper role and directs them to a message to go to IDM upon signing into mako and sign up.
Assorted Notes/Considerations/Learning
List any other information that you think is important... a post-merge activity, someone to notify, what you learned, etc.