diff --git a/README.md b/README.md index eaed022..6dd99e3 100644 --- a/README.md +++ b/README.md @@ -18,7 +18,8 @@ The service mounts all operations below `/v6/support`: replies read - `POST|DELETE /v6/support/tickets/:ticketId/assignees/me` — assign or unassign the current Support Team user -- `POST /v6/support/tickets/:ticketId/close` — close a ticket as Support Team +- `POST /v6/support/tickets/:ticketId/close` — close a ticket assigned to the + current Support Team user Interactive OpenAPI documentation is served at `/v6/support/api-docs`. @@ -33,6 +34,7 @@ the role as one multi-word value. Prisma owns a dedicated PostgreSQL `support` schema. The initial migration creates tickets, chronologically ordered responses, many-to-many assignees, ticket read states, response read receipts, and a notification outbox. +Closed ticket responses include the stored closer user ID for audit display. An absent receipt means unread. List responses compare the caller's `lastReadAt` with the latest request, response, or close activity. Opening or diff --git a/src/tickets/dto/ticket-response.dto.ts b/src/tickets/dto/ticket-response.dto.ts index 83aa2d3..070fb6e 100644 --- a/src/tickets/dto/ticket-response.dto.ts +++ b/src/tickets/dto/ticket-response.dto.ts @@ -78,6 +78,9 @@ export class TicketSummaryDto { @ApiPropertyOptional({ format: 'date-time' }) closedAt?: Date; + @ApiPropertyOptional({ example: '123456' }) + closedByUserId?: string; + @ApiProperty({ format: 'date-time' }) updatedAt!: Date; diff --git a/src/tickets/tickets.controller.ts b/src/tickets/tickets.controller.ts index b917845..43002d9 100644 --- a/src/tickets/tickets.controller.ts +++ b/src/tickets/tickets.controller.ts @@ -197,7 +197,7 @@ export class TicketsController { } /** - * Closes an open ticket as a Support Team user. + * Closes an open ticket assigned to the current Support Team user. * * @param actor Authenticated Topcoder support user. * @param ticketId Ticket UUID. @@ -208,7 +208,8 @@ export class TicketsController { @ApiOperation({ summary: 'Close a resolved support ticket' }) @ApiOkResponse({ type: TicketDetailDto }) @ApiForbiddenResponse({ - description: 'The caller is not on the Support Team.', + description: + 'The caller is not on the Support Team or is not assigned to the ticket.', }) async close( @CurrentActor() actor: SupportActor, diff --git a/src/tickets/tickets.service.spec.ts b/src/tickets/tickets.service.spec.ts index e03a559..39aa0b3 100644 --- a/src/tickets/tickets.service.spec.ts +++ b/src/tickets/tickets.service.spec.ts @@ -40,6 +40,7 @@ function createTicketRecord( assignees: [], challengeId: null, closedAt: null, + closedByUserId: null, description: 'The challenge submission is unavailable.', id: 'ticket-1', memberHandle: 'member_one', @@ -67,6 +68,7 @@ function createTicketRecord( */ function createHarness() { const tx = { + $queryRaw: jest.fn().mockResolvedValue([]), responseReadReceipt: { createMany: jest.fn().mockResolvedValue({ count: 0 }), updateMany: jest.fn().mockResolvedValue({ count: 0 }), @@ -451,12 +453,18 @@ describe('TicketsService', () => { try { const { notificationOutbox, service, tx } = createHarness(); tx.supportTicket.findUnique - .mockResolvedValueOnce({ status: TicketStatus.OPEN }) + .mockResolvedValueOnce({ + assignees: [{ userId: support.userId }], + status: TicketStatus.OPEN, + }) .mockResolvedValueOnce({ memberUserId: member.userId, status: TicketStatus.CLOSED, }) - .mockResolvedValueOnce({ status: TicketStatus.OPEN }); + .mockResolvedValueOnce({ + assignees: [{ userId: support.userId }], + status: TicketStatus.OPEN, + }); tx.supportResponse.create.mockResolvedValue({ id: 'response-reopen' }); const firstClosedAt = new Date('2026-08-07T02:00:00.000Z'); @@ -472,6 +480,13 @@ describe('TicketsService', () => { jest.setSystemTime(secondClosedAt); await service.close(support, 'ticket-1'); + expect(tx.supportTicket.updateMany).toHaveBeenCalledWith({ + data: expect.objectContaining({ + closedByUserId: support.userId, + status: TicketStatus.CLOSED, + }), + where: { id: 'ticket-1', status: TicketStatus.OPEN }, + }); expect(notificationOutbox.queueTicketClosed).toHaveBeenNthCalledWith( 1, tx, @@ -518,6 +533,25 @@ describe('TicketsService', () => { expect(tx.ticketAssignee.upsert).not.toHaveBeenCalled(); }); + it('locks the ticket before unassigning to serialize against closure', async () => { + const { service, tx } = createHarness(); + tx.supportTicket.findUnique.mockResolvedValue({ + status: TicketStatus.OPEN, + }); + + await service.unassignMe(support, 'ticket-1'); + + expect(tx.$queryRaw).toHaveBeenCalledTimes(1); + const lockQuery = tx.$queryRaw.mock.calls[0][0] as { strings: string[] }; + expect(lockQuery.strings.join('')).toContain('FOR UPDATE'); + expect(tx.$queryRaw.mock.invocationCallOrder[0]).toBeLessThan( + tx.supportTicket.findUnique.mock.invocationCallOrder[0], + ); + expect(tx.ticketAssignee.deleteMany).toHaveBeenCalledWith({ + where: { ticketId: 'ticket-1', userId: support.userId }, + }); + }); + it('role-gates both assignment and ticket closure', async () => { const { db, memberDirectory, service } = createHarness(); @@ -531,19 +565,48 @@ describe('TicketsService', () => { expect(db.$transaction).not.toHaveBeenCalled(); }); + it('rejects closure by unassigned support before writing or notifying', async () => { + const { db, notificationOutbox, service, tx } = createHarness(); + tx.supportTicket.findUnique.mockResolvedValue({ + assignees: [], + status: TicketStatus.OPEN, + }); + + await expect(service.close(support, 'ticket-1')).rejects.toMatchObject({ + message: 'Assign this support ticket to yourself before closing it.', + }); + expect(tx.$queryRaw).toHaveBeenCalledTimes(1); + const lockQuery = tx.$queryRaw.mock.calls[0][0] as { strings: string[] }; + expect(lockQuery.strings.join('')).toContain('FOR UPDATE'); + expect(tx.$queryRaw.mock.invocationCallOrder[0]).toBeLessThan( + tx.supportTicket.findUnique.mock.invocationCallOrder[0], + ); + expect(tx.supportTicket.updateMany).not.toHaveBeenCalled(); + expect(tx.ticketReadState.upsert).not.toHaveBeenCalled(); + expect(notificationOutbox.queueTicketClosed).not.toHaveBeenCalled(); + expect(notificationOutbox.dispatch).not.toHaveBeenCalled(); + expect(db.supportTicket.findUnique).not.toHaveBeenCalled(); + }); + it('treats an already-closed ticket as idempotent without enqueueing again', async () => { const { db, notificationOutbox, service, tx } = createHarness(); const closedAt = new Date('2026-08-07T02:00:00.000Z'); tx.supportTicket.findUnique.mockResolvedValue({ + assignees: [], status: TicketStatus.CLOSED, }); db.supportTicket.findUnique.mockResolvedValue( - createTicketRecord({ closedAt, status: TicketStatus.CLOSED }), + createTicketRecord({ + closedAt, + closedByUserId: support.userId, + status: TicketStatus.CLOSED, + }), ); const result = await service.close(support, 'ticket-1'); expect(result.status).toBe(TicketStatus.CLOSED); + expect(result.closedByUserId).toBe(support.userId); expect(tx.supportTicket.updateMany).not.toHaveBeenCalled(); expect(notificationOutbox.queueTicketClosed).not.toHaveBeenCalled(); expect(notificationOutbox.dispatch).not.toHaveBeenCalled(); @@ -552,6 +615,7 @@ describe('TicketsService', () => { it('does not create a close event when a concurrent close transition wins', async () => { const { notificationOutbox, service, tx } = createHarness(); tx.supportTicket.findUnique.mockResolvedValue({ + assignees: [{ userId: support.userId }], status: TicketStatus.OPEN, }); tx.supportTicket.updateMany.mockResolvedValueOnce({ count: 0 }); diff --git a/src/tickets/tickets.service.ts b/src/tickets/tickets.service.ts index 3f6d288..165c49e 100644 --- a/src/tickets/tickets.service.ts +++ b/src/tickets/tickets.service.ts @@ -433,6 +433,7 @@ export class TicketsService { ): Promise { this.assertSupportTeam(actor); await this.db.$transaction(async (tx) => { + await this.lockTicket(tx, ticketId); const ticket = await tx.supportTicket.findUnique({ select: { status: true }, where: { id: ticketId }, @@ -508,7 +509,7 @@ export class TicketsService { } /** - * Closes an open ticket as the authenticated Support Team member. + * Closes an open ticket assigned to the authenticated Support Team member. * * A conditional update makes concurrent and repeated closes idempotent. Only * the transaction that changes OPEN to CLOSED queues close email and Slack @@ -517,14 +518,22 @@ export class TicketsService { * @param actor authenticated Support Team member resolving the ticket. * @param ticketId target support ticket UUID. * @returns updated full ticket detail. - * @throws ForbiddenException when the actor lacks the Support Team role. + * @throws ForbiddenException when the actor lacks the Support Team role or + * is not assigned to the ticket. * @throws NotFoundException when the ticket does not exist. */ async close(actor: SupportActor, ticketId: string): Promise { this.assertSupportTeam(actor); const notificationIds = await this.db.$transaction(async (tx) => { + await this.lockTicket(tx, ticketId); const ticket = await tx.supportTicket.findUnique({ - select: { status: true }, + select: { + assignees: { + select: { userId: true }, + where: { userId: actor.userId }, + }, + status: true, + }, where: { id: ticketId }, }); if (!ticket) { @@ -533,6 +542,11 @@ export class TicketsService { if (ticket.status === TicketStatus.CLOSED) { return []; } + if (ticket.assignees.length === 0) { + throw new ForbiddenException( + 'Assign this support ticket to yourself before closing it.', + ); + } const closedAt = new Date(); const closeUpdate = await tx.supportTicket.updateMany({ @@ -564,6 +578,26 @@ export class TicketsService { return this.getById(actor, ticketId); } + /** + * Locks a ticket row so unassignment and closure decisions cannot interleave. + * + * @param tx active Prisma transaction. + * @param ticketId target support ticket UUID. + * @returns a promise resolved after PostgreSQL acquires the row lock. + * @throws Prisma database errors. + */ + private async lockTicket( + tx: Prisma.TransactionClient, + ticketId: string, + ): Promise { + await tx.$queryRaw(Prisma.sql` + SELECT "id" + FROM "support"."support_tickets" + WHERE "id" = ${ticketId}::uuid + FOR UPDATE + `); + } + /** * Rejects access unless the actor owns the ticket or belongs to Support Team. * @@ -639,6 +673,7 @@ export class TicketsService { assignees: record.assignees.map((assignee) => this.toAssignee(assignee)), challengeId: record.challengeId ?? undefined, closedAt: record.closedAt ?? undefined, + closedByUserId: record.closedByUserId ?? undefined, description: record.description, hasUnread: !actorReadState || actorReadState.lastReadAt < latestActivityAt, @@ -678,6 +713,7 @@ export class TicketsService { assignees: record.assignees.map((assignee) => this.toAssignee(assignee)), challengeId: record.challengeId ?? undefined, closedAt: record.closedAt ?? undefined, + closedByUserId: record.closedByUserId ?? undefined, description: record.description, hasUnread: !actorReadState || actorReadState.lastReadAt < latestActivityAt,