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

Alex+Ryan/Add Lockout Period #563

Draft
wants to merge 34 commits into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
4e6700e
Fix: Updated page.test file to match Ryan's file in his "add-lockout"…
alexappleget Sep 25, 2024
0c5ebe8
Fix: Made changes in entry/all page.tsx to match Ryan's past branch.
alexappleget Sep 25, 2024
c93a773
Fix: Updated LeagueEntries interface file to match Ryan's old branch
alexappleget Sep 25, 2024
9973ae7
Fix: Updated League entry tests to take in Ryan's changes from his ol…
alexappleget Sep 25, 2024
6fdee85
Fix: Updated LeagueEntries to bring in Ryan's old branch changes.
alexappleget Sep 25, 2024
ad04f2c
Fix: Updated LinkCustom test file to match Ryan's.
alexappleget Sep 25, 2024
988de13
Fix: Updated LinkCustom file to match Ryan's
alexappleget Sep 25, 2024
a576cab
Fix: Finalized changes from Ryan's branch. All tests pass.
alexappleget Sep 26, 2024
5c69b6e
Fix: Adjusted lockout logic.
alexappleget Sep 26, 2024
7f9a693
Fix: Adjusted lockout times and added comments.
alexappleget Sep 26, 2024
b9bdd9f
Merge remote-tracking branch 'origin/develop' into Alex+Ryan/add-lock…
alexappleget Sep 26, 2024
fa1c5ea
Merge remote-tracking branch 'origin/develop' into Alex+Ryan/add-lock…
alexappleget Sep 26, 2024
c25250a
Merge remote-tracking branch 'origin/develop' into Alex+Ryan/add-lock…
alexappleget Sep 30, 2024
cf4af9e
Fix: added lockout to onWeeklyChange function
alexappleget Sep 30, 2024
a7d2a11
Fix: editted the trycatch{} around the if else statement in onWeeklyC…
alexappleget Sep 30, 2024
86cacb5
Fix: removed isLockedOut prop
alexappleget Sep 30, 2024
41a72a5
Merge remote-tracking branch 'origin/develop' into Alex+Ryan/add-lock…
alexappleget Oct 7, 2024
b97abe0
Fix: Fixed testing. Tests were failing because there were duplicate i…
alexappleget Oct 7, 2024
3730ab6
Fix: Created custom hook for lockout to be usable across the applicat…
alexappleget Oct 8, 2024
3ce0434
Merge remote-tracking branch 'origin/develop' into Alex+Ryan/add-lock…
alexappleget Oct 8, 2024
2bfc389
Fix: Removed eslint comment.
alexappleget Oct 8, 2024
42eb061
Fix: Created testing for the useHook made for lockout.
alexappleget Oct 8, 2024
2deb8ba
Fix: moved files for useLockout() hook
alexappleget Oct 8, 2024
d8b2f69
Fix: Fixed eslint error.
alexappleget Oct 8, 2024
303f8a4
Fix: deleted prop from tests as it no longer exists.
alexappleget Oct 8, 2024
a8ff6a1
Merge remote-tracking branch 'origin/develop' into Alex+Ryan/add-lock…
alexappleget Oct 10, 2024
0996cd7
Fix: Handled PR comments
alexappleget Oct 10, 2024
b99ebb0
Merge remote-tracking branch 'origin/develop' into Alex+Ryan/add-lock…
alexappleget Oct 12, 2024
bd81e39
Fix: Handled Braydon's PR comments and updated tests
alexappleget Oct 12, 2024
220d865
Merge branch 'develop' into Alex+Ryan/add-lockout-period
ryandotfurrer Oct 14, 2024
d32c663
refactor: sorted imports and added additional line break in test.
ryandotfurrer Oct 14, 2024
52beb96
test: remove isLockedOutProp from LeagueEntries test to reflect compo…
ryandotfurrer Oct 14, 2024
5cbe943
feat: adjust lockout period to start Saturday @ 12:00am
ryandotfurrer Oct 14, 2024
0b0ec0b
test: adjust test for testing lockout from Friday to Saturday.
ryandotfurrer Oct 15, 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
51 changes: 45 additions & 6 deletions app/(main)/league/[leagueId]/entry/[entryId]/week/Week.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ import Image from 'next/image';
import LinkCustom from '@/components/LinkCustom/LinkCustom';
import { ChevronLeft } from 'lucide-react';
import toast from 'react-hot-toast';
import { onWeeklyPickChange } from './WeekHelper';
import { useRouter } from 'next/navigation';

/**
* Renders the weekly picks page.
Expand All @@ -52,6 +54,36 @@ const Week = ({ entry, league, NFLTeams, week }: IWeekProps): JSX.Element => {
const { user, updateCurrentWeek, updateWeeklyPicks, weeklyPicks } =
useDataStore((state) => state);
const { isSignedIn } = useAuthContext();
const [lockedOut, setLockedOut] = useState<boolean>(false);
const router = useRouter();

useEffect(() => {
/**
* Checks if the user is locked out from making a pick.
*/
const checkLockout = (): void => {
const currentDateAndTime = new Date();
const day = currentDateAndTime.getUTCDay();
const hours = currentDateAndTime.getUTCHours();
if (
(day === 5 && hours >= 0) || // Friday at 12am UTC (Thurs 8pm CT)
ryandotfurrer marked this conversation as resolved.
Show resolved Hide resolved
day > 5 || // Friday and Saturday
day === 0 || // Sunday
day === 1 || // Monday
(day === 2 && hours < 12) // Tuesday at 12pm UTC (8am CT)
ryandotfurrer marked this conversation as resolved.
Show resolved Hide resolved
) {
setLockedOut(true);
} else {
setLockedOut(false);
}
alexappleget marked this conversation as resolved.
Show resolved Hide resolved
};

checkLockout();

const intervalId = setInterval(checkLockout, 60 * 60 * 1000);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain what this setInterval does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it tells it to check the lockout times every hour. Is this correct @ryandotfurrer ?

Copy link
Member

Choose a reason for hiding this comment

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

@alexappleget that is correct.


return (): void => clearInterval(intervalId);
}, []);

/**
* Fetches the current game week.
Expand Down Expand Up @@ -195,6 +227,7 @@ const Week = ({ entry, league, NFLTeams, week }: IWeekProps): JSX.Element => {

/**
* Handles the weekly pick team change
* @param lockedOut - the lockout logic.
* @param teamSelect - the selected team name.
* @returns {void}
*/
Expand All @@ -215,12 +248,18 @@ const Week = ({ entry, league, NFLTeams, week }: IWeekProps): JSX.Element => {
};

try {
toast.custom(
<Alert
variant={AlertVariants.Error}
message={`Team selection has been locked for the week!`}
/>,
);
if (lockedOut) {

Choose a reason for hiding this comment

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

Do we need to nest this if statement in the try block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. So what it does is run the api functions inside the try block. But, if its during the lockdown period it will block the api functions from running and instead run the toast notification. It is a way to stop hackers from hitting the api's when they shouldn't.

Copy link
Member

Choose a reason for hiding this comment

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

@braydoncoyer please let me know if you need more info on this

Copy link

@braydoncoyer braydoncoyer Oct 23, 2024

Choose a reason for hiding this comment

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

@alexappleget @ryandotfurrer Yep, I get the purpose of the Try block. I think I was more asking if the try block could be moved inside the conditional to better represent which part of the code may cause/throw an error.

I'll leave this as a nit because as-is this likely works. However, I'm always a big fan of being more specific about which part of the code may need to have errors caught.

if (lockedOut) {
  ... code
} else {
  try {
    await onWeeklyPickChange(params);
    setUserPick(teamSelect);
    ...
  } catch (error) {
   ...
  }
  console.error(params);
}

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 see any new unit tests for this.

Copy link
Member

Choose a reason for hiding this comment

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

I will add these in ASAP.

Copy link
Member

Choose a reason for hiding this comment

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

@shashilo can you elaborate what test exactly you're looking for?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a conditional statement, so you need to have a positive and negative unit tests for this. Checking that both conditions are accurate when their conditions are met.

toast.custom(
<Alert
variant={AlertVariants.Error}
message={`Team selection has been locked for the week!`}
/>,
);
} else {
await onWeeklyPickChange(params);
setUserPick(teamSelect);
router.push(`/league/${league}/entry/all`);
}
choir241 marked this conversation as resolved.
Show resolved Hide resolved
console.error(params);
} catch (error) {
console.error('Submission error:', error);
Expand Down
8 changes: 4 additions & 4 deletions app/(main)/league/[leagueId]/entry/all/page.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ describe('League entries page (Entry Component)', () => {
screen.queryByTestId('add-new-entry-button'),
).not.toBeInTheDocument();
});
it('should display "Make Pick" button when no pick is set', async () => {
it('should display "Make Pick" link when no pick is set', async () => {
mockGetCurrentUserEntries.mockResolvedValueOnce([
{
$id: '123',
Expand All @@ -315,13 +315,13 @@ describe('League entries page (Entry Component)', () => {
render(<Entry params={{ leagueId: '123' }} />);

await waitFor(() => {
expect(screen.getByTestId('league-entry-pick-button')).toHaveTextContent(
expect(screen.getByTestId('league-entry-pick-link')).toHaveTextContent(
'Make Pick',
);
});
});

it('should render team logo and change button to "Change Pick" when a pick is made', async () => {
it('should render team logo and change link to "Change Pick" when a pick is made', async () => {
mockUseDataStore.mockReturnValue({
...mockUseDataStore(),
currentWeek: 1,
Expand All @@ -343,7 +343,7 @@ describe('League entries page (Entry Component)', () => {
'/_next/image?url=%2Fpackers-logo.png&w=96&q=75',
);

expect(screen.getByTestId('league-entry-pick-button')).toHaveTextContent(
expect(screen.getByTestId('league-entry-pick-link')).toHaveTextContent(
'Change Pick',
);
});
Expand Down
2 changes: 1 addition & 1 deletion app/(main)/league/[leagueId]/entry/all/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -217,10 +217,10 @@ const Entry = ({
return (
<section key={entry.$id}>
<LeagueEntries
key={entry.$id}
entryName={entry.name}
isEliminated={entry.eliminated}
isPickSet={isPickSet}
key={entry.$id}
linkUrl={linkUrl}
userPickHistory={userPickHistory}
selectedTeamLogo={selectedTeamLogo}
Expand Down
26 changes: 15 additions & 11 deletions components/LeagueEntries/LeagueEntries.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,30 +5,31 @@ import React from 'react';
describe('LeagueEntries', () => {
it(`renders 'default' state without a pick made`, () => {
render(
<LeagueEntries entryName="Entry 1" linkUrl="" userPickHistory={[]} />,
<LeagueEntries entryName="Entry 1" isLockedOutProp={false} linkUrl="" userPickHistory={[]} />,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where's the test where isLockedOutProp is true?

Copy link
Member

Choose a reason for hiding this comment

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

I will add this.

);

const leagueEntryContainerCard = screen.getByTestId(
'league-entry-container-card',
);
const leagueEntryNumber = screen.getByTestId('league-entry-number');
const entryStatus = screen.getByTestId('entry-status');
const leagueEntryPickButton = screen.getByTestId(
'league-entry-pick-button',
const leagueEntryPickLink = screen.getByTestId(
'league-entry-pick-link',
);
const userHistoryPicks = screen.queryByTestId('user-pick-history');

expect(entryStatus).toHaveTextContent('alive');
expect(leagueEntryContainerCard).toBeInTheDocument();
expect(leagueEntryNumber).toHaveTextContent('Entry 1');
expect(leagueEntryPickButton).toHaveTextContent('Make Pick');
expect(leagueEntryPickLink).toHaveTextContent('Make Pick');
expect(userHistoryPicks).not.toBeInTheDocument();
});

it('renders as if the user made a pick', () => {
render(
<LeagueEntries
entryName="Entry 2"
isLockedOutProp={false}
linkUrl=""
isPickSet={true}
userPickHistory={['/team-a-logo.png']}
Expand All @@ -40,14 +41,14 @@ describe('LeagueEntries', () => {
);
const leagueEntryNumber = screen.getByTestId('league-entry-number');
const entryStatus = screen.getByTestId('entry-status');
const leagueEntryPickButton = screen.getByTestId(
'league-entry-pick-button',
const leagueEntryPickLink = screen.getByTestId(
'league-entry-pick-link',
);

expect(entryStatus).toHaveTextContent('alive');
expect(leagueEntryContainerCard).toBeInTheDocument();
expect(leagueEntryNumber).toHaveTextContent('Entry 2');
expect(leagueEntryPickButton).toHaveTextContent('Change Pick');
expect(leagueEntryPickLink).toHaveTextContent('Change Pick');
expect(screen.queryByTestId('user-pick-history')).toBeInTheDocument();
});

Expand All @@ -56,6 +57,7 @@ describe('LeagueEntries', () => {
<LeagueEntries
entryName="Entry 3"
isEliminated
isLockedOutProp={false}
isPickSet={false}
linkUrl=""
userPickHistory={['/team-a-logo.png']}
Expand All @@ -80,6 +82,7 @@ describe('LeagueEntries', () => {
render(
<LeagueEntries
entryName="Entry 2"
isLockedOutProp={false}
isPickSet={true}
linkUrl={linkUrl}
selectedTeamLogo={teamLogoUrl}
Expand All @@ -92,16 +95,16 @@ describe('LeagueEntries', () => {
);
const leagueEntryNumber = screen.getByTestId('league-entry-number');
const entryStatus = screen.getByTestId('entry-status');
const leagueEntryPickButton = screen.getByTestId(
'league-entry-pick-button',
const leagueEntryPickLink = screen.getByTestId(
'league-entry-pick-link',
);
const leagueLink = screen.getByTestId('league-entry-pick-button-link');
const leagueLink = screen.getByTestId('league-entry-pick-link');
const leagueEntryLogo = screen.getByTestId('league-entry-logo');

expect(leagueEntryContainerCard).toBeInTheDocument();
expect(entryStatus).toHaveTextContent('alive');
expect(leagueEntryNumber).toHaveTextContent('Entry 2');
expect(leagueEntryPickButton).toHaveTextContent('Change Pick');
expect(leagueEntryPickLink).toHaveTextContent('Change Pick');
expect(leagueLink).toHaveAttribute('href', linkUrl);
expect(leagueEntryLogo).toBeInTheDocument();
expect(leagueEntryLogo).toHaveAttribute(
Expand All @@ -120,6 +123,7 @@ describe('LeagueEntries', () => {
render(
<LeagueEntries
entryName="Entry 2"
isLockedOutProp={false}
isPickSet={true}
linkUrl={linkUrl}
selectedTeamLogo={teamLogoUrl}
Expand Down
68 changes: 56 additions & 12 deletions components/LeagueEntries/LeagueEntries.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
// Copyright (c) Gridiron Survivor.
// Licensed under the MIT License.

import { Button } from '../Button/Button';
import { cn } from '@/utils/utils';
import { EntryStatus } from '../EntryStatus/EntryStatus';
import { ILeagueEntriesProps } from './LeagueEntries.interface';
import React, { JSX } from 'react';
import Link from 'next/link';
import React, { JSX, useEffect, useState } from 'react';
import LinkCustom from '../LinkCustom/LinkCustom';
import Image from 'next/image';

/**
Expand All @@ -18,16 +17,53 @@ import Image from 'next/image';
* @param props.isPickSet - if true, the team logo of the picked team shows up on the LeagueEntries card and the button changes from "make a pick" to "chagne pick"
* @param props.userPickHistory - the user's pick history for this entry
* @param props.selectedTeamLogo - the team logo
* @param props.lockout - if true, the user is locked out from making a pick
* @returns {React.JSX.Element} - A div element that contains the user's entry information
*/

/**
* Display all entries for a league.
* @param {string} leagueId - The league id.
* @returns {JSX.Element} The rendered entries component.
*/
const LeagueEntries = ({
entryName,
linkUrl,
isEliminated = false,
isPickSet = false,
linkUrl,
userPickHistory,
selectedTeamLogo = '',
}: ILeagueEntriesProps): JSX.Element => {
const [isLockedOut, setLockedOut] = useState<boolean>(false);

useEffect(() => {
/**
* Checks if the user is locked out from making a pick.
*/
const checkLockout = (): void => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is exactly the same code as the one in Week.tsx. When this occurs, it's a good idea to move this to a shared helper function.

const currentDateAndTime = new Date();
const day = currentDateAndTime.getUTCDay();
const hours = currentDateAndTime.getUTCHours();
if (
(day === 5 && hours >= 0) || // Friday at 12am UTC (Thurs 8pm CT)
day > 5 || // Friday and Saturday
day === 0 || // Sunday
day === 1 || // Monday
(day === 2 && hours < 12) // Tuesday at 12pm UTC (8am CT)
) {
setLockedOut(true);
} else {
setLockedOut(false);
}
};

alexappleget marked this conversation as resolved.
Show resolved Hide resolved
checkLockout();

const intervalId = setInterval(checkLockout, 60 * 60 * 1000);

return (): void => clearInterval(intervalId);
}, []);

return (
<div
data-testid="league-entry-container-card"
Expand Down Expand Up @@ -107,14 +143,22 @@ const LeagueEntries = ({
data-testid="league-entry-pick-button-container"
>
{!isEliminated && (
<Link href={linkUrl} data-testid="league-entry-pick-button-link">
<Button
className="league-entry-pick-button"
data-testid="league-entry-pick-button"
label={isPickSet ? 'Change Pick' : 'Make Pick'}
variant={isPickSet ? 'secondary' : 'default'}
/>
</Link>
<LinkCustom
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are 3 conditions in this component. This is best, it's having conditional statements for 2 separate components. It's easier to read and maintain.

aria-disabled={isLockedOut === true ? 'true' : 'false'}
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Why can't we use truthy/falsy values here and write the ternary operation like the following: isLockedOut ? 'true' : 'false'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryandotfurrer could you help answer this? i didnt understand it as well

Copy link
Member

Choose a reason for hiding this comment

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

@alexappleget This was a question previously brought up on my PR. Chris had suggested removing isLockedOut as a prop from the parent component, settings the default state to false, and seeing if that would allow us to remove the truthy/falsy values.

The truthy/falsy values being isLockedOut === true

So instead it could be rewritten as a true ternary: aria-disabled={isLockedOut ? 'true' : 'false'}

Copy link
Contributor

@Danielle254 Danielle254 Oct 9, 2024

Choose a reason for hiding this comment

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

Agree. isLockedOut is a truthy value on its own. It's unnecessary to include the isLockedOut === true part.
aria-disabled={isLockedOut ? 'true' : 'false'} is cleaner and functions the same

Copy link
Member

Choose a reason for hiding this comment

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

@Danielle254 Yes, I too agree. That is how I originally tried developing it but it was not working as intended.

As per my previous comment, @chris-nowicki thought this may be to it being in the parent component as a prop.

className={cn(
'league-entry-pick-link',
isLockedOut === true ? 'opacity-50 cursor-not-allowed' : '',
)}
dataTestidProp="league-entry-pick-link"
href={linkUrl}
onClick={(e: { preventDefault: () => unknown }) =>
isLockedOut === true && e.preventDefault()
}
size={'defaultButton'}
variant={isPickSet ? 'secondaryButton' : 'primaryButton'}
choir241 marked this conversation as resolved.
Show resolved Hide resolved
>
{isPickSet ? 'Change Pick' : 'Make Pick'}
</LinkCustom>
)}
</div>
</section>
Expand Down
2 changes: 1 addition & 1 deletion components/LinkCustom/LinkCustom.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import LinkCustom from './LinkCustom';
describe('LinkCustom Component', () => {
it('renders with default props', () => {
render(
<LinkCustom children="Test link" href="https://example.com"></LinkCustom>,
<LinkCustom children="Test link" dataTestidProp="linkCustom" href="https://example.com"></LinkCustom>,
);
const link = screen.getByTestId('linkCustom');
expect(link).toBeInTheDocument();
Expand Down
Loading