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.

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
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
9 changes: 9 additions & 0 deletions api/apiFunctions.interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,15 @@ export interface IAccountData {
password: string;
}
export interface IUser {
// for the appwrite auth collection
id: string;
email: string;
leagues: string[];
labels: string[];
}

export interface ICollectionuser {
// for the custom user collection
vmaineng marked this conversation as resolved.
Show resolved Hide resolved
id: string;
email: string;
leagues: string[];
Expand Down
5 changes: 4 additions & 1 deletion api/apiFunctions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
ILeague,
IGameWeek,
IUser,
ICollectionuser,
IWeeklyPicks,
INFLTeam,
} from './apiFunctions.interface';
Expand Down Expand Up @@ -74,7 +75,9 @@ export async function logoutAccount(): Promise<object | Error> {
* @param userId - The user ID
* @returns {Models.DocumentList<Models.Document> | Error} - The user object or an error
*/
export async function getCurrentUser(userId: IUser['id']): Promise<IUser> {
export async function getCurrentUser(
userId: IUser['id'],
): Promise<ICollectionuser> {
try {
const user = await databases.listDocuments(
appwriteConfig.databaseId,
Expand Down
13 changes: 8 additions & 5 deletions context/AuthContextProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { account } from '@/api/config';
import { useRouter } from 'next/navigation';
import { useDataStore } from '@/store/dataStore';
import type { DataStore } from '@/store/dataStore';
import { IUser } from '@/api/apiFunctions.interface';
import { ICollectionuser, IUser } from '@/api/apiFunctions.interface';
import { getCurrentUser } from '@/api/apiFunctions';
import { loginAccount, logoutHandler } from './AuthHelper';
import { usePathname } from 'next/navigation';
Expand Down Expand Up @@ -50,8 +50,12 @@ export const AuthContextProvider = ({
getUser();
return;
}

setIsSignedIn(true);
}, [user]);
if (pathname.startsWith('/admin')) {
!user.labels.includes('admin') && router.push('/');
}
}, [user, pathname]);

Copy link
Contributor

@vmaineng vmaineng Aug 19, 2024

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
Expand Down Expand Up @@ -89,9 +93,8 @@ export const AuthContextProvider = ({

try {
const user = await account.get();
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.

const userData: ICollectionuser = await getCurrentUser(user.$id);
updateUser(userData.id, userData.email, userData.leagues, user.labels);
} catch (error) {
resetUser();
setIsSignedIn(false);
Expand Down
7 changes: 7 additions & 0 deletions store/dataStore.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const userData = {
userId: '123',
userEmail: '[email protected]',
leagues: ['123456'],
labels: ['admin'],
};

const NFLTeam = [
Expand Down Expand Up @@ -50,11 +51,14 @@ describe('Data Store', () => {
userData.userId,
userData.userEmail,
userData.leagues,
userData.labels,
);
});

expect(result.current.user.id).toBe(userData.userId);
expect(result.current.user.email).toBe(userData.userEmail);
expect(result.current.user.labels).toStrictEqual(userData.labels);
expect(result.current.user.leagues).toStrictEqual(userData.leagues);
});
it('Checks the reset user state matches default', () => {
const { result } = renderHook(() => useDataStore());
Expand All @@ -64,12 +68,15 @@ describe('Data Store', () => {
userData.userId,
userData.userEmail,
userData.leagues,
userData.labels,
);
result.current.resetUser();
});

expect(result.current.user.id).toBe('');
expect(result.current.user.email).toBe('');
expect(result.current.user.labels).toStrictEqual([]);
expect(result.current.user.leagues).toStrictEqual([]);
});
});

Expand Down
5 changes: 4 additions & 1 deletion store/dataStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ interface IDataStoreAction {
id: IUser['id'],
email: IUser['email'],
leagues: IUser['leagues'],
labels: IUser['labels'],
) => void;
updateWeeklyPicks: ({
leagueId,
Expand All @@ -56,6 +57,7 @@ const initialState: IDataStoreState = {
id: '',
email: '',
leagues: [],
labels: [],
},
weeklyPicks: {
leagueId: '',
Expand Down Expand Up @@ -101,12 +103,13 @@ export const useDataStore = create<DataStore>((set) => ({
* @param leagues - The user league
* @returns {void}
*/
updateUser: (id, email, leagues): void =>
updateUser: (id, email, leagues, labels): void =>
set(
produce((state: IDataStoreState) => {
state.user.id = id;
state.user.email = email;
state.user.leagues = [...leagues];
state.user.labels = [...labels];
}),
),
/**
Expand Down
9 changes: 9 additions & 0 deletions utils/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,3 +144,12 @@ export const getUserLeagues = async (
export const getUserEntries = async (userId: IUser['id'], leagueId: ILeague['leagueId']): Promise<IEntry[]> => {
return await getCurrentUserEntries(userId, leagueId);
}

/**
* Check if the route is an /admin route
* @param path - The path to check
* @returns {boolean} - Whether the route is an /admin route
*/
export const isAdminRoute = (path: string): boolean => {
return path.startsWith('/admin');
};