Update isUnusedAttendanceCode to allow any non-conflicting Event #426
Conversation
…d, and don't count it if so
…d, and don't count it if so
yimmyj
requested review from
sumeet-bansal,
shravanhariharan2,
dowhep and
nik-dange
as code owners
|
Thanks for contributing! |
|
|
||
| // If there are eligible events, return the first one | ||
| if (validEvents.length > 0) { | ||
| return validEvents[0]; |
There was a problem hiding this comment.
What defines the first event? Is it sorted by start time?
There was a problem hiding this comment.
In theory there should never be more than one eligible event; that would mean that there are two events occurring at the same time sharing an attendance code which wouldn't make any sense. This is handled by isAvailableAttendanceCode and tested in the attached postman screenshots.
However, since the check-in period is defined as 30 minutes before and after an event, two events happening one after another with the same attendance code could cause an issue.
The solution to this could be to simply disallow this case, which involves changing isAvailableAttendanceCode to check for conflicting events in this "check-in period" rather than the duration of the event itself. Let me know your thoughts on this change!
There was a problem hiding this comment.
I agree with the "check-in period" approach. However, realistically, we can allow more padding room cases like this, such as not allowing events with the same checkin code 3 days before and after the event. This would avoid the edge case of having a time precisely in the "check-in period"s of two close events with the same checkin code.
| // Otherwise, find the closest event to the current time | ||
| const currentTime = new Date(); | ||
| let closestEvent = null; | ||
| let closestTimeDifference = Infinity; | ||
|
|
||
| matchingEvents.forEach((event) => { | ||
| const eventStartTime = new Date(event.start); | ||
| const timeDifference = Math.abs(eventStartTime.getTime() - currentTime.getTime()); | ||
|
|
||
| // Update closest event if necessary | ||
| if (timeDifference < closestTimeDifference) { | ||
| closestEvent = event; | ||
| closestTimeDifference = timeDifference; | ||
| } | ||
| }); | ||
|
|
||
| return closestEvent; |
There was a problem hiding this comment.
Just curious, what's the edge case that warrants this code? Shouldn't all potentially valid events be caught in the above code? I know we're validating it one more time in the AttendanceService, just curious what the reasoning for adding it was
There was a problem hiding this comment.
And side note, if there is a use case that warrants this code, does it make more sense to use validEvents instead of matchingEvents?
There was a problem hiding this comment.
The use case is the small pop-up that announces whether an event "hasn't occurred yet / has already occurred / doesn't exist" when the user enters an attendance code that isn't matched to an event currently going on, in AttendanceService's validateEventToAttend method.
The way it used to work when attendance codes were unique is that the validate method would simply check if the one event with the matching attendance code hasn't started, has already ended, or doesn't exist in the schema.
However with repeated attendance codes, it is possible for events sharing the same code to be simultaneously "too early" AND "too late," so I chose to return the closest event to the current date as a "guess" as to what the user meant to check in to when they entered a code. For example, if code "ABC" was used both for an event a year ago and two days in the future, I'm assuming that the user meant to check in to the one two days in the future, and will show the "hasn't occurred yet" message. Let me know if this implementation needs more consideration.
There was a problem hiding this comment.
I believe we shouldn't have the "hasn't occurred yet" message at all as it give away the checkin code for a future event, nullifying the secrecy of the checkin code.
|
|
||
| // If there are eligible events, return the first one | ||
| if (validEvents.length > 0) { | ||
| return validEvents[0]; |
There was a problem hiding this comment.
I agree with the "check-in period" approach. However, realistically, we can allow more padding room cases like this, such as not allowing events with the same checkin code 3 days before and after the event. This would avoid the edge case of having a time precisely in the "check-in period"s of two close events with the same checkin code.
| // Otherwise, find the closest event to the current time | ||
| const currentTime = new Date(); | ||
| let closestEvent = null; | ||
| let closestTimeDifference = Infinity; | ||
|
|
||
| matchingEvents.forEach((event) => { | ||
| const eventStartTime = new Date(event.start); | ||
| const timeDifference = Math.abs(eventStartTime.getTime() - currentTime.getTime()); | ||
|
|
||
| // Update closest event if necessary | ||
| if (timeDifference < closestTimeDifference) { | ||
| closestEvent = event; | ||
| closestTimeDifference = timeDifference; | ||
| } | ||
| }); | ||
|
|
||
| return closestEvent; |
There was a problem hiding this comment.
I believe we shouldn't have the "hasn't occurred yet" message at all as it give away the checkin code for a future event, nullifying the secrecy of the checkin code.
…3 days, removed 'event is in the future' message
No description provided.