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.

Sign up for GitHub

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

chris/feat(#381): Implement User Role Access #458

Merged
merged 15 commits into from
Oct 23, 2024
Merged

chris/feat(#381): Implement User Role Access #458

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e870455
feat(#381): Implement User Role Access
chris-nowicki Aug 7, 2024
3e5b4b0
fix: restored changed files from develop that did not need to be changed
chris-nowicki Aug 7, 2024
9a4ea6b
Merge branch 'develop' into chris/add-admin-user-permissions
chris-nowicki Aug 7, 2024
e0e9a0c
Merge branch 'develop' into chris/add-admin-user-permissions
chris-nowicki Aug 17, 2024
7bbd79a
fix: wildcard route check for /admin routes
chris-nowicki Aug 17, 2024
b9f9114
fix: unused state
chris-nowicki Aug 17, 2024
cc406d4
fix: api call not pulling in user labels
chris-nowicki Aug 17, 2024
3cf0d8b
fix: admin route waiting for user to load to check for labels
chris-nowicki Aug 17, 2024
deeb21d
fix: types with different user types (Appwrite Auth and Custom Collec…
chris-nowicki Aug 17, 2024
4fad4c4
Merge branch 'develop' into chris/add-admin-user-permissions
chris-nowicki Aug 24, 2024
8229a37
Merge branch 'develop' into chris/add-admin-user-permissions
chris-nowicki Aug 26, 2024
8f91a3e
Merge branch 'develop' into chris/add-admin-user-permissions
chris-nowicki Oct 17, 2024
a0c18ad
add helper function for isAuthRoute
chris-nowicki Oct 17, 2024
a3a6946
updated pnpmlock.yml
chris-nowicki Oct 21, 2024
819f1fe
Merge branch 'develop' into chris/add-admin-user-permissions
shashilo Oct 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions context/AuthContextProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { IUser } from '@/api/apiFunctions.interface';
import { getCurrentUser } from '@/api/apiFunctions';
import { loginAccount } from './AuthHelper';
import { usePathname } from 'next/navigation';
import { adminRoutes } from '@/lib/adminRoutes';

type UserCredentials = {
email: string;
Expand Down Expand Up @@ -40,6 +41,7 @@ export const AuthContextProvider = ({
children: React.ReactNode;
}): JSX.Element => {
const [isSignedIn, setIsSignedIn] = useState<boolean>(false);
const [isSuperAdmin, setIsSuperAdmin] = useState<boolean>(false);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this needs to be a useState. It's going to cause a rehydration of the entire application that may be alreayd handled from other auth functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ done.

const { updateUser, resetUser, user } = useDataStore<DataStore>(
(state) => state,
);
Expand All @@ -54,6 +56,14 @@ export const AuthContextProvider = ({
setIsSignedIn(true);
}, [user]);

useMemo(() => {
if (isSignedIn) {
choir241 marked this conversation as resolved.
Show resolved Hide resolved
if (adminRoutes.includes(pathname)) {
!isSuperAdmin && router.push('/league/all');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The homepage should redirect them to league/all already. Once this is set to the real homepage, we won't need to modify this again.

Suggested change
!isSuperAdmin && router.push('/league/all');
!isSuperAdmin && router.push('/');

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shashilo tried this out and nope I am not redirected automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done ✅

}
}
}, [pathname]);

Copy link
Contributor

@vmaineng vmaineng Aug 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we write out other edge cases such as

  1. if user.labels is undefined or not yet populated
  2. if the logic has multiple stated updates (i.e. user or pathname changing frequently causing multiple redirects (to prevent adding to the history stack as a user can go forward and backward)

What are your thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The way the useEffect is written user will be populated if it hits this point
    if (user.id === '' || user.email === '') {
      getUser();
      return;
    }
    ```

2. This is a question I'd like to defer to @shashilo ... right now this is the only way I was able to get it to work due when we change pathname it loads the data again (using cached data) but we still need to wait for it to load.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vmaineng brings up a great point. If this is a case in our app, we should write it out by making a unit test for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ pathname helper function added to utils with a unit test to test pathname changing.

/**
* Authenticate and set session state
* @param user - The user credentials.
Expand Down Expand Up @@ -97,6 +107,9 @@ export const AuthContextProvider = ({

try {
const user = await account.get();
if (user.labels.includes('admin')) {
setIsSuperAdmin(true);
}
choir241 marked this conversation as resolved.
Show resolved Hide resolved
const userData: IUser = await getCurrentUser(user.$id);
updateUser(userData.id, userData.email, userData.leagues);
return userData;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the return removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ it is back in :). Not sure why it was removed.

Expand Down
9 changes: 9 additions & 0 deletions lib/adminRoutes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Copyright (c) Gridiron Survivor.
// Licensed under the MIT License.

export const adminRoutes: string[] = [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't this be a wildcard? admin/*? Then, we don't need to track every page.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ done

'/admin',
'/admin/leagues',
'/admin/players',
'/admin/notifications',
];