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 13 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
10 changes: 10 additions & 0 deletions api/apiFunctions.interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,16 @@ export interface IAccountData {
}
export interface IUser {
documentId: string;
// for the appwrite auth collection
id: string;
email: string;
leagues: string[];
labels: string[];
}

export interface ICollectionUser {
documentId: string;
// 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,
IRecoveryToken,
Expand Down Expand Up @@ -149,7 +150,9 @@ export async function updateUserEmail({
* @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
7 changes: 7 additions & 0 deletions app/(main)/league/all/page.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ jest.mock('@/store/dataStore', () => ({
id: '1234',
email: '[email protected]',
leagues: ['league1'],
labels: [],
},
allLeagues: [
{
Expand Down Expand Up @@ -82,6 +83,7 @@ describe('Leagues Component', () => {
leagues: [],
},
allLeagues: [],
labels: [],
});

render(<Leagues />);
Expand All @@ -105,6 +107,7 @@ describe('Leagues Component', () => {
leagues: [],
},
allLeagues: [],
labels: [],
});

render(<Leagues />);
Expand All @@ -121,6 +124,7 @@ describe('Leagues Component', () => {
email: '[email protected]',
id: '123',
leagues: [],
labels: [],
},
allLeagues: [
{
Expand Down Expand Up @@ -150,6 +154,7 @@ describe('Leagues Component', () => {
email: '[email protected]',
id: '123',
leagues: [],
labels: [],
};

const league = {
Expand Down Expand Up @@ -202,6 +207,7 @@ describe('Leagues Component', () => {
user.id,
user.email,
[...user.leagues, league.leagueId],
user.labels,
);
expect(toast.custom).toHaveBeenCalledWith(
<Alert
Expand All @@ -220,6 +226,7 @@ describe('Leagues Component', () => {
email: '[email protected]',
id: '123',
leagues: [],
labels: [],
};

const league = {
Expand Down
11 changes: 7 additions & 4 deletions app/(main)/league/all/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,13 @@ const Leagues = (): JSX.Element => {
});

setLeagues([...leagues, league]);
updateUser(user.documentId, user.id, user.email, [
...user.leagues,
league.leagueId,
]);
updateUser(
user.documentId,
user.id,
user.email,
[...user.leagues, league.leagueId],
user.labels,
);
toast.custom(
<Alert
variant={AlertVariants.Success}
Expand Down
2 changes: 1 addition & 1 deletion components/UpdateEmailForm/UpdateEmailForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ const UpdateEmailForm = (): JSX.Element => {
/>,
);

updateUser(user.documentId, user.id, email, user.leagues);
updateUser(user.documentId, user.id, email, user.leagues, user.labels);
form.reset({ email: email || '', password: '' });
} catch (error) {
console.error('Email Update Failed', error);
Expand Down
38 changes: 25 additions & 13 deletions context/AuthContextProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@ 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';
import { isAuthRequiredPath } from '@/utils/utils';

type UserCredentials = {
email: string;
Expand Down Expand Up @@ -50,8 +51,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 @@ -81,26 +86,33 @@ export const AuthContextProvider = ({
*/
const getUser = async (): Promise<IUser | undefined> => {
if (!isSessionInLocalStorage()) {
if (
pathname !== '/register' &&
pathname !== '/account/recovery' &&
pathname !== '/recover-password'
) {
if (isAuthRequiredPath(pathname)) {
router.push('/login');
}
return;
}

try {
const user = await account.get();
const userData: IUser = await getCurrentUser(user.$id);
const userData: ICollectionUser = await getCurrentUser(user.$id);

const currentUser: IUser = {
documentId: userData.documentId,
id: userData.id,
email: userData.email,
leagues: userData.leagues,
labels: user.labels,
};

updateUser(
userData.documentId,
userData.id,
userData.email,
userData.leagues,
currentUser.documentId,
currentUser.id,
currentUser.email,
currentUser.leagues,
user.labels,
);
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.


return currentUser;
} catch (error) {
resetUser();
setIsSignedIn(false);
Expand Down
4 changes: 2 additions & 2 deletions pnpm-lock.yaml
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 lock file updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅ fixed!

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions store/dataStore.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const userData = {
id: '123',
email: '[email protected]',
leagues: ['123456'],
labels: ['admin'],
};

const NFLTeams = [
Expand Down Expand Up @@ -61,11 +62,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 @@ -76,12 +80,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 @@ -34,6 +34,7 @@ interface IDataStoreAction {
id: IUser['id'],
email: IUser['email'],
leagues: IUser['leagues'],
labels: IUser['labels'],
) => void;
updateWeeklyPicks: ({
leagueId,
Expand Down Expand Up @@ -63,6 +64,7 @@ const initialState: IDataStoreState = {
id: '',
email: '',
leagues: [],
labels: [],
},
weeklyPicks: {
leagueId: '',
Expand Down Expand Up @@ -111,13 +113,14 @@ export const useDataStore = create<DataStore>((set) => ({
* @param selectedLeagues - The user selected league
* @returns {void}
*/
updateUser: (documentId, id, email, leagues): void =>
updateUser: (documentId, id, email, leagues, labels): void =>
set(
produce((state: IDataStoreState) => {
state.user.documentId = documentId;
state.user.id = id;
state.user.email = email;
state.user.leagues = [...leagues];
state.user.labels = [...labels];
}),
),
/**
Expand Down
28 changes: 28 additions & 0 deletions utils/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
getUserPick,
parseUserPick,
getUserLeagues,
isAuthRequiredPath,
} from './utils';
import { getCurrentLeague, getAllWeeklyPicks } from '@/api/apiFunctions';

Expand Down Expand Up @@ -179,6 +180,33 @@ describe('utils', () => {
});
});
});
describe('isAuthRequiredPath', () => {
it('should return false for non-auth paths', () => {
expect(isAuthRequiredPath('/register')).toBe(false);
expect(isAuthRequiredPath('/account/recovery')).toBe(false);
expect(isAuthRequiredPath('/recover-password')).toBe(false);
});

it('should return true for auth-required paths', () => {
expect(isAuthRequiredPath('/')).toBe(true);
expect(isAuthRequiredPath('/dashboard')).toBe(true);
expect(isAuthRequiredPath('/profile')).toBe(true);
expect(isAuthRequiredPath('/settings')).toBe(true);
});

it('should handle edge cases', () => {
expect(isAuthRequiredPath('')).toBe(true);
expect(isAuthRequiredPath('/register/')).toBe(true); // Trailing slash
expect(isAuthRequiredPath('/REGISTER')).toBe(true); // Case sensitivity
expect(isAuthRequiredPath('/register/subpage')).toBe(true);
});

it('should handle unusual inputs', () => {
expect(isAuthRequiredPath(' /register ')).toBe(true); // Spaces
expect(isAuthRequiredPath('/account/recovery?param=value')).toBe(true); // Query parameters
});
});

xdescribe('getUserLeagues', () => {
it('should return the list of leagues the user is a part of', async () => {
(getCurrentLeague as jest.Mock).mockResolvedValue(mockLeague);
Expand Down
19 changes: 19 additions & 0 deletions utils/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,15 @@ export const getUserEntries = async (userId: IUser['id'], leagueId: ILeague['lea
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');
};

/**
* Returns if the team has already been picked by the user
* @param teamName - The team name
Expand All @@ -167,3 +176,13 @@ export const hasTeamBeenPicked = (teamName: string, selectedTeams: string[]): bo
export const getNFLTeamLogo = (NFLTeams: INFLTeam[], teamName: string): string => {
return NFLTeams.find((teams) => teams.teamName === teamName)?.teamLogo as string;
}

/**
* Checks if the current path requires authentication
* @param pathname - The current path
* @returns {boolean} - Whether the current path requires authentication
*/
export const isAuthRequiredPath = (pathname: string): boolean => {
const nonAuthPaths = ['/register', '/account/recovery', '/recover-password'];
return !nonAuthPaths.includes(pathname);
};