Conversation
- Added a function to check for unprocessed services in booking data. - Updated the approval route to prevent final approval if there are unprocessed services. - Modified the booking actions hook to fetch all bookings after approval actions. - Enhanced unit tests to cover scenarios for Media Commons bookings with unprocessed services.
There was a problem hiding this comment.
Pull request overview
This pull request fixes a critical bug where a booking request could be approved twice. The issue occurred when a user clicked "Approve" twice on a Media Commons booking with requested services before the UI refreshed. The first approval would transition the booking to the "Services Request" state, and the second approval would fail in XState but fall back to the traditional approval flow, inadvertently completing final approval before all services were processed.
Changes:
- Added a guard in the approval API route to prevent fallback approval when Media Commons bookings have unprocessed services
- Enhanced UI to refresh booking data after approval actions to prevent stale state issues
- Added test coverage for the unprocessed services scenario
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
booking-app/app/api/approve/route.ts |
Added hasUnprocessedServices function and logic to block fallback approval for Media Commons bookings with unprocessed services, returning a 409 error with a user-friendly message |
booking-app/tests/unit/api-approve.unit.test.ts |
Added test case verifying that the API returns 409 and blocks fallback when Media Commons bookings have unprocessed services |
booking-app/lib/stateMachines/mcBookingMachine.ts |
Formatting/indentation adjustments only (no functional changes) |
booking-app/components/src/client/routes/admin/hooks/useBookingActions.tsx |
Added fetchAllBookings calls after approval actions to refresh booking list and prevent stale data, plus formatting adjustments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expect(body.error).toMatch(/services approval flow|unprocessed/i); | ||
| expect(mockServerApproveBooking).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Missing test coverage for the case where Media Commons has XState failure but all requested services have been processed (approved or declined). In this scenario, the code should fall back to serverApproveBooking since hasUnprocessedServices would return false. Consider adding a test case where staffingServicesDetails is "yes" and staffServiceApproved is true or false (already processed) to verify the fallback works correctly in this scenario.
| it("falls back to serverApproveBooking for Media Commons when all requested services are processed", async () => { | |
| mockExecute.mockResolvedValue({ | |
| success: false, | |
| error: "Invalid transition: Cannot execute 'approve' from state 'Services Request'", | |
| }); | |
| mockIsMediaCommons.mockReturnValue(true); | |
| mockServerGetDataByCalendarEventId.mockResolvedValue({ | |
| id: "booking-db-id", | |
| staffingServicesDetails: "yes", // services were requested | |
| staffServiceApproved: true, // already processed (approved/declined) | |
| } as any); | |
| mockGetMediaCommonsServices.mockReturnValue({ | |
| staff: true, | |
| equipment: false, | |
| catering: false, | |
| cleaning: false, | |
| security: false, | |
| setup: false, | |
| }); | |
| const response = await POST( | |
| createRequest( | |
| { id: bookingId, email: approverEmail }, | |
| { "x-tenant": "mc" }, | |
| ) as any, | |
| ); | |
| expect(mockExecute).toHaveBeenCalledWith( | |
| bookingId, | |
| "approve", | |
| "mc", | |
| approverEmail, | |
| ); | |
| expect(mockServerApproveBooking).toHaveBeenCalledWith( | |
| bookingId, | |
| approverEmail, | |
| "mc", | |
| ); | |
| expect(mockFinalApprove).not.toHaveBeenCalled(); | |
| expect(mockServerFirstApproveOnly).not.toHaveBeenCalled(); | |
| await expect(parseJson(response)).resolves.toEqual({ | |
| status: 200, | |
| data: { message: "Approved successfully" }, | |
| }); | |
| }); |
There was a problem hiding this comment.
@interfinityOfficial can you review this copilot recommendation?
|
@interfinityOfficial |
|
@interfinityOfficial |
3 participants
Checklist