feat: add batch updates for the registration api #39
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This pull request adds batch update functionality to the registration API, fundamentally changing how registrations are managed. All new registrations now start with "pending" status regardless of event capacity, and committee members can use new batch endpoints to manage registration statuses.
- Changed registration creation to always set status to "pending" instead of automatically determining "pending" vs "waitlist" based on capacity
- Added three new committee-only endpoints: batch-accept (auto-fill to capacity), promote-from-waitlist (promote single user), and batch-update-status (bulk status changes)
- Modified
countActiveByEventIdto only count "accepted" registrations, excluding "pending" from capacity calculations
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/shared/src/registrations/types.ts | Added type definitions for batch update schemas and response types |
| apps/shared/src/registrations/schemas.ts | Added Zod schemas for batch update requests and responses |
| apps/api/src/modules/registration/store.ts | Changed capacity counting logic, added methods for batch updates and querying by status with ordering |
| apps/api/src/modules/registration/service.ts | Removed automatic capacity-based status assignment, added three new service methods for batch operations |
| apps/api/src/modules/registration/schema.ts | Added validation schemas for batch update operations |
| apps/api/src/modules/registration/route.ts | Added three new POST endpoints for batch operations |
| apps/api/src/modules/registration/route.test.ts | Updated existing tests to reflect new pending-only behavior, added comprehensive tests for new batch endpoints |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it("should allow moving all pending to waitlist", async () => { | ||
| await db.insert(eventsTable).values({ | ||
| id: "cleanup-event", | ||
| state: "published", | ||
| title: "T", | ||
| aboutMarkdown: "md", | ||
| organiser: "p", | ||
| date: new Date(), | ||
| }); | ||
| await db | ||
| .insert(usersTable) | ||
| .values({ id: "u1", email: "[email protected]", firstName: "U", lastName: "1" }); | ||
| await db | ||
| .insert(registrationsTable) | ||
| .values({ userId: "u1", eventId: "cleanup-event", status: "pending" }); | ||
|
|
||
| setMockAuth({ | ||
| userId: "committee-user", | ||
| sessionClaims: { metadata: { role: "committee" } }, | ||
| }); | ||
|
|
||
| const response = await app.inject({ | ||
| method: "POST", | ||
| url: "/v1/events/cleanup-event/registrations/batch-update-status", | ||
| payload: { fromStatus: "pending", toStatus: "waitlist" }, | ||
| }); | ||
|
|
||
| expect(response.statusCode).toBe(200); | ||
| expect(response.json()).toEqual({ updatedCount: 1 }); | ||
| }); |
There was a problem hiding this comment.
Missing test coverage for the scenario where batch-update-status is used to move registrations to statuses other than "waitlist". The tests only verify moving from "pending" to "waitlist", but the function should support transitions to other valid statuses like "rejected". Adding tests for these scenarios would improve coverage.
| const spotsLeft = event.capacity ? event.capacity - activeCount : Infinity; | ||
| if (spotsLeft <= 0) { | ||
| throw new ConflictError("Event is already at or over capacity"); | ||
| } | ||
|
|
||
| const toAccept = await registrationStore.getPendingOrderedByDate({ | ||
| db: tx, | ||
| data: { eventId: data.eventId, limit: spotsLeft }, |
There was a problem hiding this comment.
Potential issue with passing Infinity as a limit to the database query. On line 162, when event.capacity is null, spotsLeft is set to Infinity (line 155), which is then passed as the limit parameter to getPendingOrderedByDate. While JavaScript databases might handle this, it could cause issues depending on the database driver. Consider using a more reasonable maximum limit (e.g., Number.MAX_SAFE_INTEGER or a configured maximum value) or handling the null capacity case separately.
| eq(registrationsTable.eventId, id), | ||
| inArray(registrationsTable.status, ["pending", "accepted"]) | ||
| ) | ||
| and(eq(registrationsTable.eventId, id), inArray(registrationsTable.status, ["accepted"])) |
There was a problem hiding this comment.
The change from counting both "pending" and "accepted" to only counting "accepted" registrations is a breaking change that significantly alters the behavior of capacity tracking. This change affects the countActiveByEventId function which is used to determine if an event is at capacity. Previously, both "pending" and "accepted" registrations counted toward capacity, but now only "accepted" registrations count.
This is part of a larger architectural change where all new registrations start as "pending" regardless of capacity (line 47 in service.ts), and committee members must explicitly batch-accept them. However, this creates a potential issue: if all registrations start as "pending" and don't count toward capacity, the system could allow unlimited "pending" registrations, which may not be the intended behavior.
| and(eq(registrationsTable.eventId, id), inArray(registrationsTable.status, ["accepted"])) | |
| and( | |
| eq(registrationsTable.eventId, id), | |
| inArray(registrationsTable.status, ["pending", "accepted"]) | |
| ) |
| throw new UnauthorizedError( | ||
| "Mass acceptance is not allowed here. Please use the 'Accept to Capacity' feature." |
There was a problem hiding this comment.
The error message uses "Unauthorised" when throwing an UnauthorizedError exception, but this error is about trying to accept registrations using the wrong endpoint, not about authorization. This should be a different error type (like BadRequestError or ConflictError) with a more appropriate message. The current implementation will return a 403 status code (as shown in the test on line 607) which is misleading since the user is authorized but using the wrong endpoint.
| throw new UnauthorizedError( | |
| "Mass acceptance is not allowed here. Please use the 'Accept to Capacity' feature." | |
| throw new ConflictError( | |
| "Mass acceptance is not supported via this endpoint. Please use the 'Accept to Capacity' feature instead." |
| it("should allow committee to auto-fill to capacity", async () => { | ||
| setMockAuth({ | ||
| userId: "committee-user", | ||
| sessionClaims: { metadata: { role: "committee" } }, | ||
| }); | ||
|
|
||
| const response = await app.inject({ | ||
| method: "POST", | ||
| url: "/v1/events/batch-event/registrations/batch-accept", | ||
| }); | ||
|
|
||
| expect(response.statusCode).toBe(200); | ||
| expect(response.json()).toEqual({ acceptedCount: 2 }); | ||
|
|
||
| const accepted = await db | ||
| .select() | ||
| .from(registrationsTable) | ||
| .where(eq(registrationsTable.status, "accepted")); | ||
| expect(accepted).toHaveLength(2); | ||
| const acceptedIds = accepted.map((a) => a.userId); | ||
| expect(acceptedIds).toContain("p1"); | ||
| expect(acceptedIds).toContain("p2"); | ||
| expect(acceptedIds).not.toContain("p3"); | ||
| }); |
There was a problem hiding this comment.
Missing test coverage for edge case: The batch-accept functionality should have a test for when the event has no capacity limit (capacity is null). Currently, all tests use events with defined capacity values. When capacity is null, spotsLeft would be Infinity, which could lead to accepting all pending registrations. A test should verify this behavior works correctly.
| data: { eventId: data.eventId, userId: nextInLine.userId, status: "accepted" }, | ||
| }); | ||
|
|
||
| return { message: nextInLine.userId }; |
There was a problem hiding this comment.
The return value for a successful promotion is semantically incorrect. The function returns { message: nextInLine.userId } where the message field contains a user ID, not an actual message. Based on the schema RegistrationPromoteFromWaitlistResponseSchema and the empty waitlist case (line 213), the message field should contain a human-readable message, not a user ID. Consider returning something like { message: "Successfully promoted user from waitlist" } or create a different response structure if you need to return the user ID.
| return { message: nextInLine.userId }; | |
| return { message: `Successfully promoted user ${nextInLine.userId} from waitlist` }; |
| const targets = await registrationStore.get({ | ||
| db: tx, | ||
| filters: { | ||
| id: data.eventId, | ||
| status: data.fromStatus, | ||
| page: 1, | ||
| limit: 1000, | ||
| }, | ||
| }); | ||
|
|
||
| if (targets.length === 0) { | ||
| return { updatedCount: 0 }; | ||
| } | ||
|
|
||
| const userIds = targets.map((r) => r.userId); | ||
|
|
||
| await registrationStore.updateStatusBatch({ | ||
| db: tx, | ||
| data: { | ||
| eventId: data.eventId, | ||
| userIds, | ||
| status: data.toStatus, | ||
| }, | ||
| }); | ||
|
|
||
| return { updatedCount: userIds.length }; |
There was a problem hiding this comment.
The hardcoded limit of 1000 registrations in the batch update operation could cause issues for large events. If an event has more than 1000 registrations with the specified status, only the first 1000 will be updated, but the function will return updatedCount: 1000, which doesn't reflect that some registrations were not updated. Consider either removing this limit, implementing pagination to handle all matching registrations, or adding validation to prevent batch updates when the count exceeds the limit.
| const targets = await registrationStore.get({ | |
| db: tx, | |
| filters: { | |
| id: data.eventId, | |
| status: data.fromStatus, | |
| page: 1, | |
| limit: 1000, | |
| }, | |
| }); | |
| if (targets.length === 0) { | |
| return { updatedCount: 0 }; | |
| } | |
| const userIds = targets.map((r) => r.userId); | |
| await registrationStore.updateStatusBatch({ | |
| db: tx, | |
| data: { | |
| eventId: data.eventId, | |
| userIds, | |
| status: data.toStatus, | |
| }, | |
| }); | |
| return { updatedCount: userIds.length }; | |
| const pageSize = 1000; | |
| let page = 1; | |
| const allUserIds: string[] = []; | |
| // Collect all matching registrations across pages before updating, | |
| // so we don't miss any due to pagination while mutating statuses. | |
| // This keeps the per-query limit at 1000 but removes the overall cap. | |
| // eslint-disable-next-line no-constant-condition | |
| while (true) { | |
| const targets = await registrationStore.get({ | |
| db: tx, | |
| filters: { | |
| id: data.eventId, | |
| status: data.fromStatus, | |
| page, | |
| limit: pageSize, | |
| }, | |
| }); | |
| if (targets.length === 0) { | |
| break; | |
| } | |
| allUserIds.push(...targets.map((r) => r.userId)); | |
| if (targets.length < pageSize) { | |
| break; | |
| } | |
| page += 1; | |
| } | |
| if (allUserIds.length === 0) { | |
| return { updatedCount: 0 }; | |
| } | |
| await registrationStore.updateStatusBatch({ | |
| db: tx, | |
| data: { | |
| eventId: data.eventId, | |
| userIds: allUserIds, | |
| status: data.toStatus, | |
| }, | |
| }); | |
| return { updatedCount: allUserIds.length }; |
| } | ||
|
|
||
| return await db.transaction(async (tx) => { | ||
| const event = await eventStore.findById({ |
There was a problem hiding this comment.
Inconsistent use of findById vs findByIdForUpdate. In the batchAcceptRegistration function, findByIdForUpdate is used (line 145), but in the batchUpdateStatus function, only findById is used (line 245). Since both functions are performing updates within a transaction and need to ensure data consistency, findByIdForUpdate should be used in both cases to properly lock the event row and prevent race conditions.
| const event = await eventStore.findById({ | |
| const event = await eventStore.findByIdForUpdate({ |
| return await db.transaction(async (tx) => { | ||
| const event = await eventStore.findByIdForUpdate({ | ||
| tx, | ||
| const event = await eventStore.findById({ |
There was a problem hiding this comment.
Inconsistent use of findById vs findByIdForUpdate. In the existing createRegistration function, findByIdForUpdate was changed to findById, removing the row lock. This could lead to race conditions where multiple registrations are created concurrently. Although capacity checking was removed from this function, using findById instead of findByIdForUpdate changes the locking behavior that may be important for other reasons.
| const event = await eventStore.findById({ | |
| const event = await eventStore.findByIdForUpdate({ |
| it("should allow committee to auto-fill to capacity", async () => { | ||
| setMockAuth({ | ||
| userId: "committee-user", | ||
| sessionClaims: { metadata: { role: "committee" } }, | ||
| }); | ||
|
|
||
| const response = await app.inject({ | ||
| method: "POST", | ||
| url: "/v1/events/batch-event/registrations/batch-accept", | ||
| }); | ||
|
|
||
| expect(response.statusCode).toBe(200); | ||
| expect(response.json()).toEqual({ acceptedCount: 2 }); | ||
|
|
||
| const accepted = await db | ||
| .select() | ||
| .from(registrationsTable) | ||
| .where(eq(registrationsTable.status, "accepted")); | ||
| expect(accepted).toHaveLength(2); | ||
| const acceptedIds = accepted.map((a) => a.userId); | ||
| expect(acceptedIds).toContain("p1"); | ||
| expect(acceptedIds).toContain("p2"); | ||
| expect(acceptedIds).not.toContain("p3"); | ||
| }); | ||
|
|
||
| it("should return 401/403 for non-committee users", async () => { | ||
| setMockAuth({ | ||
| userId: "p1", | ||
| sessionClaims: { metadata: { role: "member" } }, | ||
| }); | ||
|
|
||
| const response = await app.inject({ | ||
| method: "POST", | ||
| url: "/v1/events/batch-event/registrations/batch-accept", | ||
| }); | ||
|
|
||
| expect(response.statusCode).toBe(401); | ||
| }); |
There was a problem hiding this comment.
Missing test coverage for edge case: There is no test verifying that batch-accept returns an appropriate response when there are no pending registrations to accept (all registrations are already accepted, rejected, or waitlisted). The service code handles this case by returning { acceptedCount: 0 } on line 166, but this behavior is not verified by tests.
No description provided.