Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 50 additions & 20 deletions apps/backend/src/app/api/bookings/route.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,15 @@ describe("/api/bookings", async () => {
const gameSessionDataService = new GameSessionDataService()
const userDataService = new UserDataService()

describe("POST", () => {
const now = new Date()
const now = new Date()

const currentSemesterCreateMock: CreateSemesterData = {
...semesterCreateMock,
startDate: new Date(Date.UTC(now.getUTCFullYear(), now.getUTCMonth() - 1, 1)).toISOString(),
endDate: new Date(Date.UTC(now.getUTCFullYear(), now.getUTCMonth() + 4, 0)).toISOString(),
}
const currentSemesterCreateMock: CreateSemesterData = {
...semesterCreateMock,
startDate: new Date(Date.UTC(now.getUTCFullYear(), now.getUTCMonth() - 1, 1)).toISOString(),
endDate: new Date(Date.UTC(now.getUTCFullYear(), now.getUTCMonth() + 4, 0)).toISOString(),
}

describe("POST", () => {
it("should return a 401 if token is missing", async () => {
const res = await POST(createMockNextRequest())
expect(res.status).toBe(StatusCodes.UNAUTHORIZED)
Expand Down Expand Up @@ -105,6 +105,11 @@ describe("/api/bookings", async () => {
futureGameSessionCreateMock,
)

// Ensure member has enough remaining sessions so we hit the weekly limit, not "No remaining sessions"
await userDataService.updateUser(memberUserMock.id, {
remainingSessions: 10,
})

await bookingDataService.createBooking({
...bookingCreateMock,
gameSession: gameSession.id,
Expand All @@ -126,7 +131,7 @@ describe("/api/bookings", async () => {
expect(await res.json()).toStrictEqual({ error: "Weekly booking limit reached" })
})

it("should return a 403 if a member has reached their max number of bookings per week", async () => {
it("should return a 403 if a casual has reached their max number of bookings per week", async () => {
cookieStore.set(AUTH_COOKIE_NAME, casualToken)

const currentSemester = await semesterDataService.createSemester(currentSemesterCreateMock)
Expand Down Expand Up @@ -162,7 +167,7 @@ describe("/api/bookings", async () => {

const res = await POST(req)
expect(res.status).toBe(StatusCodes.FORBIDDEN)
expect(await res.json()).toStrictEqual({ error: "Weekly booking limit reached" })
expect(await res.json()).toStrictEqual({ error: "No remaining sessions" })
})
Comment on lines 134 to 171
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on second thought, I'd prefer the error message be "Weekly booking limit reached" as it makes more sense for casuals to see a weekly limit reached rather than no remaining sessions (they never have remaining sessions as 1 remaining session indicates "member", but if casuals are displayed "1" too which makes the role system confusing), this is open for discussion.

the proposed logic change would involve only checking remainingSessions for members (essentially casuals bypass the initial remainingSessionsBasedOnRole check and only just check existing weekly bookings over here:

// Important: this controls the weekly booking limit for both casual and member users, as it counts all bookings regardless of user role.
const allUpcomingBookings = await bookingDataService.getAllCurrentWeekBookingsByUserId(
req.user.id,
currentSemester,
)
if (allUpcomingBookings.length >= getMaxBookingSize(req.user)) {
return NextResponse.json(
{ error: "Weekly booking limit reached" },
{ status: StatusCodes.FORBIDDEN },
)
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree, having a role based on another value is confusing and users do not need to know that they become a casual if they have 0 sessions, rather it should be enough to specify that their sessions only apply to the casual slots.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users need to know that they become casual when they have 0 sessions, this was specified by client as they have a very different booking criteria as far as I am aware.

Copy link
Copy Markdown
Member Author

@choden-dev choden-dev Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it quite easy to show the user they are casual without linking it to the sessions? It's important from a business logic perspective but from a UX perspective they only need to know if they are casual/member and the implications of such.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is important business logic as now we display 1 session and if a user tops up, thinking they'd get 3 sessions would be wrong! Hence adding a tooltip would clarify it in a business perspective

Copy link
Copy Markdown
Member Author

@choden-dev choden-dev Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this comment you made addresses that? i.e it shouldn't be a backend thing


it("should call the booking confirmation email service", async () => {
Expand Down Expand Up @@ -198,13 +203,18 @@ describe("/api/bookings", async () => {
)
})

it("should return a 409 if the user has already made a booking for the session", async () => {
cookieStore.set(AUTH_COOKIE_NAME, casualToken)
const gameSession = await gameSessionDataService.createGameSession(gameSessionCreateMock)
it("should return a 409 if a member user has already made a booking for the session", async () => {
cookieStore.set(AUTH_COOKIE_NAME, memberToken)
const currentSemester = await semesterDataService.createSemester(currentSemesterCreateMock)
const gameSession = await gameSessionDataService.createGameSession({
...gameSessionCreateMock,
semester: currentSemester,
})

await bookingDataService.createBooking({
...bookingCreateMock,
gameSession: gameSession.id,
user: casualUserMock,
user: memberUserMock,
})

const req = createMockNextRequest("/api/bookings", "POST", {
Expand All @@ -219,13 +229,25 @@ describe("/api/bookings", async () => {

it("should return a 403 if the user has no remaining sessions", async () => {
cookieStore.set(AUTH_COOKIE_NAME, casualToken)
const gameSession = await gameSessionDataService.createGameSession(gameSessionCreateMock)
await userDataService.updateUser(casualUserMock.id, {
remainingSessions: -1,
const currentSemester = await semesterDataService.createSemester(currentSemesterCreateMock)
const gameSession = await gameSessionDataService.createGameSession({
...gameSessionCreateMock,
semester: currentSemester,
})
const gameSession2 = await gameSessionDataService.createGameSession({
...gameSessionCreateMock,
semester: currentSemester,
})

// Create a booking on a different session so getRemainingSessions returns 0 for casual
await bookingDataService.createBooking({
...bookingCreateMock,
gameSession: gameSession.id,
user: casualUserMock,
})

const req = createMockNextRequest("", "POST", {
gameSession,
gameSession: gameSession2,
playerLevel: PlayLevel.beginner,
} satisfies CreateBookingRequest)
const res = await POST(req)
Expand Down Expand Up @@ -268,7 +290,11 @@ describe("/api/bookings", async () => {

it("should return a 403 if the user is a casual and has exceeded booking capacity", async () => {
cookieStore.set(AUTH_COOKIE_NAME, casualToken)
const gameSession = await gameSessionDataService.createGameSession(gameSessionCreateMock)
const currentSemester = await semesterDataService.createSemester(currentSemesterCreateMock)
const gameSession = await gameSessionDataService.createGameSession({
...gameSessionCreateMock,
semester: currentSemester,
})
await bookingDataService.createBooking({
...bookingCreateMock,
gameSession: gameSession.id,
Expand All @@ -284,12 +310,16 @@ describe("/api/bookings", async () => {
const res = await POST(req)

expect(res.status).toBe(StatusCodes.FORBIDDEN)
expect(await res.json()).toStrictEqual({ error: "Session is full for the selected user role" })
expect(await res.json()).toStrictEqual({ error: "No remaining sessions" })
})
Comment on lines 291 to 314
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

based on the test description, it should be returning "Session is full for the selected user role". since the logic was changed to:

check remaining sessions (for casual user mock) -> check game session capacities

we need to change the description and add a test to test for the initial test topic

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For casuals this would be the correct message for their role?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the error is only because booking create mock is using casual user and that causes it to show no remaining sessions. The point of the test was to test for casual capacities


it("should a 403 if the user is a member and has exceeded booking capacity", async () => {
cookieStore.set(AUTH_COOKIE_NAME, memberToken)
const gameSession = await gameSessionDataService.createGameSession(gameSessionCreateMock)
const currentSemester = await semesterDataService.createSemester(currentSemesterCreateMock)
const gameSession = await gameSessionDataService.createGameSession({
...gameSessionCreateMock,
semester: currentSemester,
})
await bookingDataService.createBooking({
...bookingCreateMock,
gameSession: gameSession.id,
Expand Down
43 changes: 32 additions & 11 deletions apps/backend/src/app/api/bookings/route.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {
CreateBookingRequestSchema,
GameBookingStrategy,
getMaxBookingSize,
MembershipType,
type RequestWithUser,
Expand All @@ -14,6 +15,7 @@ import BookingDataService from "@/data-layer/services/BookingDataService"
import GameSessionDataService from "@/data-layer/services/GameSessionDataService"
import SemesterDataService from "@/data-layer/services/SemesterDataService"
import UserDataService from "@/data-layer/services/UserDataService"
import { getRemainingSessions } from "@/data-layer/utils/GameSessionUtils"

class RouteWrapper {
@Security("jwt")
Expand Down Expand Up @@ -44,16 +46,27 @@ class RouteWrapper {
// Refetch user data as JWT stored data could be outdated
const userData = await userDataService.getUserById(req.user.id)

if ((userData.remainingSessions ?? 0) <= -1)
const remainingSessionsBasedOnRole = await getRemainingSessions(
userData,
semesterDataService,
bookingDataService,
userDataService,
)

if (remainingSessionsBasedOnRole <= 0)
return NextResponse.json(
{ error: "No remaining sessions" },
Comment on lines +49 to 58
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I like the logic here although it seems to be overlapping with standard member logic and could be refined down to calling getAllCurrentWeekBookingsByUserId once for both casuals and regulars. Note that this logic is used again in lines:

const allUpcomingBookings = await bookingDataService.getAllCurrentWeekBookingsByUserId(
req.user.id,
currentSemester,
)
if (allUpcomingBookings.length >= getMaxBookingSize(req.user)) {
return NextResponse.json(
{ error: "Weekly booking limit reached" },
{ status: StatusCodes.FORBIDDEN },
)
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For casuals this would be the correct message for their role?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of them are correct, in this case, it would be ideal to simplify the logic as the same logic is being checked twice

Copy link
Copy Markdown
Member Author

@choden-dev choden-dev Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree from an efficiency point of view, but realistically you should not know that the implementation of getRemainingSessions does the check, so they are actually doing different things from a black box perspective (weekly bookings vs getting remaining sessions)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, I'm happy with your implementation

{ status: StatusCodes.FORBIDDEN },
)

const bookingStrategy =
userData.role === MembershipType.member
? GameBookingStrategy.MEMBER
: GameBookingStrategy.CASUAL
if (
(userData.role === MembershipType.casual &&
(bookingStrategy === GameBookingStrategy.CASUAL &&
bookings.length >= gameSession.casualCapacity) ||
(userData.role === MembershipType.member && bookings.length >= gameSession.capacity)
(bookingStrategy === GameBookingStrategy.MEMBER && bookings.length >= gameSession.capacity)
)
return NextResponse.json(
{ error: "Session is full for the selected user role" },
Expand All @@ -72,6 +85,7 @@ class RouteWrapper {
// Check if the user's booking limit has been reached
const currentSemester = await semesterDataService.getCurrentSemester()

// Important: this controls the weekly booking limit for both casual and member users, as it counts all bookings regardless of user role.
const allUpcomingBookings = await bookingDataService.getAllCurrentWeekBookingsByUserId(
req.user.id,
currentSemester,
Expand All @@ -91,14 +105,21 @@ class RouteWrapper {

await MailService.sendBookingConfirmation(newBooking)

const newRemainingSessions = (userData.remainingSessions ?? 0) - 1
// Demote user to casual if session count is lower than or equal to 0
await userDataService.updateUser(req.user.id, {
remainingSessions: newRemainingSessions,
role: newRemainingSessions <= 0 ? MembershipType.casual : req.user.role,
})

return NextResponse.json({ data: newBooking }, { status: StatusCodes.CREATED })
switch (bookingStrategy) {
case GameBookingStrategy.CASUAL:
// Casual bookings do not affect remaining sessions or membership status
return NextResponse.json({ data: newBooking }, { status: StatusCodes.CREATED })
case GameBookingStrategy.MEMBER: {
const newRemainingSessions = (userData.remainingSessions ?? 0) - 1
// Demote user to casual if session count is lower than or equal to 0
await userDataService.updateUser(req.user.id, {
remainingSessions: newRemainingSessions,
role: newRemainingSessions <= 0 ? MembershipType.casual : req.user.role,
})

return NextResponse.json({ data: newBooking }, { status: StatusCodes.CREATED })
}
}
} catch (error) {
if (error instanceof NotFound) {
return NextResponse.json(
Expand Down
95 changes: 95 additions & 0 deletions apps/backend/src/app/api/me/bookings/remaining/route.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
import { AUTH_COOKIE_NAME } from "@repo/shared"
import { getReasonPhrase, StatusCodes } from "http-status-codes"
import { cookies } from "next/headers"
import BookingDataService from "@/data-layer/services/BookingDataService"
import GameSessionDataService from "@/data-layer/services/GameSessionDataService"
import SemesterDataService from "@/data-layer/services/SemesterDataService"
import { createMockNextRequest } from "@/test-config/backend-utils"
import { bookingCreateMock } from "@/test-config/mocks/Booking.mock"
import { gameSessionCreateMock } from "@/test-config/mocks/GameSession.mock"
import { semesterCreateMock } from "@/test-config/mocks/Semester.mock"
import { casualToken, memberToken } from "@/test-config/vitest.setup"
import { GET } from "./route"

describe("/api/me/bookings/remaining", async () => {
const cookieStore = await cookies()
const bookingDataService = new BookingDataService()
const gameSessionDataService = new GameSessionDataService()
const semesterDataService = new SemesterDataService()

beforeEach(async () => {
const currentDate = new Date()
const startDate = new Date(currentDate.getTime() - 24 * 60 * 60 * 1000) // 1 day ago
const endDate = new Date(currentDate.getTime() + 24 * 60 * 60 * 1000) // 1 day from now

await semesterDataService.createSemester({
...semesterCreateMock,
startDate: startDate.toISOString(),
endDate: endDate.toISOString(),
})
})

describe("GET", () => {
it("should return 1 remaining session for a casual user with no bookings this week", async () => {
cookieStore.set(AUTH_COOKIE_NAME, casualToken)

const response = await GET(createMockNextRequest("/api/me/bookings/remaining"))

expect(response.status).toBe(StatusCodes.OK)
const json = await response.json()
expect(json.data.remainingSessions).toBe(1)
})

it("should return 0 remaining sessions for a casual user with a booking this week", async () => {
cookieStore.set(AUTH_COOKIE_NAME, casualToken)

const now = new Date()
const gameSession = await gameSessionDataService.createGameSession({
...gameSessionCreateMock,
startTime: new Date(now.getTime() + 24 * 60 * 60 * 1000).toISOString(),
endTime: new Date(now.getTime() + 24 * 60 * 60 * 1000).toISOString(),
})

await bookingDataService.createBooking({
...bookingCreateMock,
gameSession,
})

const response = await GET(createMockNextRequest("/api/me/bookings/remaining"))

expect(response.status).toBe(StatusCodes.OK)
const json = await response.json()
expect(json.data.remainingSessions).toBe(0)
})

it("should return remaining sessions from user object for a member user", async () => {
cookieStore.set(AUTH_COOKIE_NAME, memberToken)

const response = await GET(createMockNextRequest("/api/me/bookings/remaining"))

expect(response.status).toBe(StatusCodes.OK)
const json = await response.json()
expect(typeof json.data.remainingSessions).toBe("number")
})
Comment on lines +71 to +73
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't necessarily make the test correct, please check if the remainingSessions is equal to memberUserMock.remainingSessions

remainingSessions: 5,


it("should return 500 and handle unexpected errors", async () => {
cookieStore.set(AUTH_COOKIE_NAME, casualToken)

const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {})
const mockGetCurrentSemester = vi
.spyOn(SemesterDataService.prototype, "getCurrentSemester")
.mockRejectedValueOnce(new Error("Database error"))

const response = await GET(createMockNextRequest("/api/me/bookings/remaining"))

expect(response.status).toBe(StatusCodes.INTERNAL_SERVER_ERROR)
const json = await response.json()
expect(json.error).toBe(getReasonPhrase(StatusCodes.INTERNAL_SERVER_ERROR))
expect(consoleErrorSpy).toHaveBeenCalled()
expect(mockGetCurrentSemester).toHaveBeenCalled()

consoleErrorSpy.mockRestore()
mockGetCurrentSemester.mockRestore()
})
})
})
34 changes: 34 additions & 0 deletions apps/backend/src/app/api/me/bookings/remaining/route.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import type { RequestWithUser } from "@repo/shared"
import { getReasonPhrase, StatusCodes } from "http-status-codes"
import { NextResponse } from "next/server"
import { Security } from "@/business-layer/middleware/Security"
import BookingDataService from "@/data-layer/services/BookingDataService"
import SemesterDataService from "@/data-layer/services/SemesterDataService"
import UserDataService from "@/data-layer/services/UserDataService"
import { getRemainingSessions } from "@/data-layer/utils/GameSessionUtils"

class RouteWrapper {
@Security("jwt")
static async GET(req: RequestWithUser) {
try {
return NextResponse.json({
data: {
remainingSessions: await getRemainingSessions(
req.user,
new SemesterDataService(),
new BookingDataService(),
new UserDataService(),
),
},
})
} catch (error) {
console.error(error)
return NextResponse.json(
{ error: getReasonPhrase(StatusCodes.INTERNAL_SERVER_ERROR) },
{ status: StatusCodes.INTERNAL_SERVER_ERROR },
)
}
}
}

export const { GET } = RouteWrapper
Loading