-
Notifications
You must be signed in to change notification settings - Fork 5
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
Update isUnusedAttendanceCode to allow any non-conflicting Event #426
base: master
Are you sure you want to change the base?
Changes from all commits
66661e0
0c3e632
95396e8
b1b16a1
d700332
ac70916
49282d1
12a7891
213ebfd
3e73caa
6cd0f48
bbb81ef
eb650ba
da01871
8a34ea5
b21299f
920976a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
import { MigrationInterface, QueryRunner } from 'typeorm'; | ||
|
||
const TABLE_NAME = 'Events'; | ||
const CONSTRAINT_NAME = 'Events_attendanceCode_key'; | ||
|
||
export class RemoveUniqueAttendanceCodeConstraint1712188218208 implements MigrationInterface { | ||
public async up(queryRunner: QueryRunner): Promise<void> { | ||
await queryRunner.query(`ALTER TABLE "${TABLE_NAME}" DROP CONSTRAINT "${CONSTRAINT_NAME}"`); | ||
} | ||
|
||
public async down(queryRunner: QueryRunner): Promise<void> { | ||
await queryRunner.query(`ALTER TABLE "${TABLE_NAME}" ADD UNIQUE ("attendanceCode")`); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import { EntityRepository, SelectQueryBuilder } from 'typeorm'; | ||
import { EntityRepository, LessThanOrEqual, MoreThanOrEqual, SelectQueryBuilder } from 'typeorm'; | ||
import { EventSearchOptions, Uuid } from '../types'; | ||
import { EventModel } from '../models/EventModel'; | ||
import { BaseRepository } from './BaseRepository'; | ||
|
@@ -33,16 +33,59 @@ export class EventRepository extends BaseRepository<EventModel> { | |
} | ||
|
||
public async findByAttendanceCode(attendanceCode: string): Promise<EventModel> { | ||
return this.repository.findOne({ attendanceCode }); | ||
// Find all events with the given attendance code | ||
const matchingEvents = await this.repository.find({ attendanceCode }); | ||
|
||
// Find all events that are currently | ||
const validEvents = matchingEvents.filter((event) => !event.isTooEarlyToAttendEvent() | ||
&& !event.isTooLateToAttendEvent()); | ||
|
||
// If there are eligible events, return the first one | ||
if (validEvents.length > 0) { | ||
return validEvents[0]; | ||
} | ||
|
||
// 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; | ||
Comment on lines
+48
to
+64
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
public async deleteEvent(event: EventModel): Promise<EventModel> { | ||
return this.repository.remove(event); | ||
} | ||
|
||
public async isUnusedAttendanceCode(attendanceCode: string): Promise<boolean> { | ||
const count = await this.repository.count({ attendanceCode }); | ||
return count === 0; | ||
public async isAvailableAttendanceCode(attendanceCode: string, start: Date, end: Date): Promise<boolean> { | ||
const bufferedStart = new Date(start); | ||
bufferedStart.setDate(bufferedStart.getDate() - 3); | ||
|
||
const bufferedEnd = new Date(end); | ||
bufferedEnd.setDate(bufferedEnd.getDate() + 3); | ||
|
||
const hasOverlap = await this.repository.find({ | ||
where: [ | ||
{ | ||
attendanceCode, | ||
start: LessThanOrEqual(bufferedEnd), | ||
end: MoreThanOrEqual(bufferedStart), | ||
}, | ||
], | ||
}); | ||
|
||
return hasOverlap.length === 0; | ||
} | ||
|
||
private getBaseEventSearchQuery(options: EventSearchOptions): SelectQueryBuilder<EventModel> { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What defines the first event? Is it sorted by start time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.