Alex+Ryan/Add Lockout Period #563
Conversation
|
Gridiron Survivor Application
Project name: Gridiron Survivor Application
Only deployments on the production branch are activated automatically. If you'd like to activate this deployment, navigate to your deployments. Learn more about Appwrite Function deployments.
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| await onWeeklyPickChange(params); | ||
| setUserPick(teamSelect); | ||
| router.push(`/league/${league}/entry/all`); | ||
| if (lockedOut) { |
There was a problem hiding this comment.
Do we need to nest this if statement in the try block?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@braydoncoyer please let me know if you need more info on this
There was a problem hiding this comment.
@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);
}
hooks/useLockout.test.ts
Outdated
| afterEach(() => { | ||
| getUTCDaySpy.mockRestore(); | ||
| getUTCHoursSpy.mockRestore(); | ||
| }) |
There was a problem hiding this comment.
nit: can we get a newline here? If Prettier or another formatter is run in a pipeline, feel free to ignore.
There was a problem hiding this comment.
@braydoncoyer done and will be reflected in the next push.
hooks/useLockout.ts
Outdated
| * Function to constantly check the current time every hour and set the lockout accordingly. | ||
| * @returns - The value of lockedOut. | ||
| */ | ||
| const useLockout = (): boolean => { |
There was a problem hiding this comment.
Since this hook returns a binary value to indicate a user's ability to perform a task, would a more specific name be effective?
Ex: useIsUserLockedOut
There was a problem hiding this comment.
I actually like this name. I moreso just used useLockout to match everything. I'll change it :)
There was a problem hiding this comment.
@braydoncoyer this has been updated by @alexappleget. Please resolve this convo when you can; thanks!
hooks/useLockout.ts
Outdated
| const day = currentDateAndTime.getUTCDay(); | ||
| const hours = currentDateAndTime.getUTCHours(); | ||
| if ( | ||
| (day === 5 && hours >= 0) || // Thursday at 9pm EST |
There was a problem hiding this comment.
Do we care about time zones? I'm still ramping up and this may be a well-known answer but one I think is worth me asking.
There was a problem hiding this comment.
these numbers are UTC time which is 4 hours ahead of me. I left the comments for easy visual understanding. It handles when the lockdown period is.
There was a problem hiding this comment.
Do we care about time zones? I'm still ramping up and this may be a well-known answer but one I think is worth me asking.
@braydoncoyer I used UTC so we don't have to worry about time zones. I only left the comments in there for reference, like Alex said.
Not to mention, however, that Appwrite also uses UTC. So if we need to tie this in with that we don't need to make changes again.
hooks/useLockout.ts
Outdated
| day === 1 || // Monday | ||
| (day === 2 && hours < 12) // Tuesday at 9am EST | ||
| ) { | ||
| setLockedOut(true); |
There was a problem hiding this comment.
Looks like we may be updating this state regardless if the value has changed.
I wonder if a check could be made to ensure we only update state if the value has changed.
const isLockedOut =
(day === 5 && hours >= 0) || // Thursday at 9pm EST
day > 5 || // Friday and Saturday
day === 0 || // Sunday
day === 1 || // Monday
(day === 2 && hours < 12); // Tuesday at 9am EST
if (isLockedOut !== lockedOut) {
setLockedOut(isLockedOut);
}There was a problem hiding this comment.
I apologize that I dont follow. When Ryan made this function, it is to handle the buttons being disabled, etc. During the specific days/times itll keep lockedOut as true which will lock everything down. Then outside of those days/times itll have it set to false which will unlock everything for users to use. And with the interval it checks the time every hour.
There was a problem hiding this comment.
@braydoncoyer like @alexappleget said, this is simply checking several statements and, if any of them are true, it will change the state. Are you saying you think this is changing the state multiple times?
Below is a screenshot of what it is achieving; notice the user cannot click and the links are disabled.
There was a problem hiding this comment.
@ryandotfurrer Thanks for the clarification!
My original comment could have been more clear. I think that we may set state to a value (let's say true) even if the value is already true. This seems redundant. It may not actually work like this -- I'd have to pull down and test locally, but perhaps you know the answer off the top of your head!
hooks/useIsUserLockedOut.ts
Outdated
| const day = currentDateAndTime.getUTCDay(); | ||
| const hours = currentDateAndTime.getUTCHours(); | ||
| if ( | ||
| (day === 5 && hours >= 0) || // Thursday at 9pm EST |
There was a problem hiding this comment.
Since we have the other change to not allow picks if a game has been played, let's update the lockdown period to Sat midnight. 95% of the NFL games are on Sun/Mon.
| */ | ||
| const useIsUserLockedOut = (): boolean => { | ||
| const [lockedOut, setLockedOut] = useState<boolean>(false); | ||
| useEffect(() => { |
There was a problem hiding this comment.
Why does this have to be a useEffect?
There was a problem hiding this comment.
This is a useEffect as it is sets up the setInterval which checks the time every hour.
There was a problem hiding this comment.
But why is it needed? Looks like the code can function without it. useEffects will cause unnecessary re-renders if one uses it incorrectly. Figure out if the code needs it, and if it renders too many times.
|
|
||
| checkLockout(); | ||
|
|
||
| const intervalId = setInterval(checkLockout, 60 * 60 * 1000); |
There was a problem hiding this comment.
Again, what is 60, 60, and 1000? Put these into CONST instead, so anyone can read and understand what each value means. Also, what's the reasoning behind useing setInterval?
There was a problem hiding this comment.
60 * 60 * 1000 creates a setInterval that checks the time once per hour. The reasoning behind the setInterval is so that it checks the time every hour and can appropriately update the state of setLockedOut.
What do you mean by COST?
There was a problem hiding this comment.
It was a long week. I meant CONST. A constant variable. What represents these individual numbers?
| await onWeeklyPickChange(params); | ||
| setUserPick(teamSelect); | ||
| router.push(`/league/${league}/entry/all`); | ||
| if (lockedOut) { |
There was a problem hiding this comment.
I don't see any new unit tests for this.
There was a problem hiding this comment.
I will add these in ASAP.
There was a problem hiding this comment.
@shashilo can you elaborate what test exactly you're looking for?
There was a problem hiding this comment.
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.
| */ | ||
| const useIsUserLockedOut = (): boolean => { | ||
| const [lockedOut, setLockedOut] = useState<boolean>(false); | ||
| useEffect(() => { |
There was a problem hiding this comment.
But why is it needed? Looks like the code can function without it. useEffects will cause unnecessary re-renders if one uses it incorrectly. Figure out if the code needs it, and if it renders too many times.
|
|
||
| checkLockout(); | ||
|
|
||
| const intervalId = setInterval(checkLockout, 60 * 60 * 1000); |
There was a problem hiding this comment.
It was a long week. I meant CONST. A constant variable. What represents these individual numbers?
Closes #525
Took over Ryan's task and cherry picked his commits over.
UPDATE: Created a custom useHook for the lockout period as
useLockout()Everything works as it should. With this new useHook, you can import the useLockout hook anywhere and just do
const lockedOut = useLockout()and it'll know if lockedOut is true or false.