diff --git a/server/src/nest/assignments/assignments.controller.ts b/server/src/nest/assignments/assignments.controller.ts index b076762de..7f274946c 100644 --- a/server/src/nest/assignments/assignments.controller.ts +++ b/server/src/nest/assignments/assignments.controller.ts @@ -202,7 +202,7 @@ export class AssignmentOpsController { if (!this.assignments.getAssignmentForTrip(id, tripId)) { throw new HttpException({ error: 'Assignment not found' }, 404); } - const participants = this.assignments.setParticipants(id, body.user_ids); + const participants = this.assignments.setParticipants(id, body.user_ids, tripId); this.assignments.broadcast(tripId, 'assignment:participants', { assignmentId: Number(id), participants }, socketId); return { participants }; } diff --git a/server/src/nest/assignments/assignments.mcp.ts b/server/src/nest/assignments/assignments.mcp.ts index 68b843a96..989a7a727 100644 --- a/server/src/nest/assignments/assignments.mcp.ts +++ b/server/src/nest/assignments/assignments.mcp.ts @@ -187,7 +187,7 @@ export class AssignmentsMcp { if (!this.assignments.verifyTripAccess(tripId, ctx.userId)) return noAccess(); if (!this.guards.hasTripPermission('day_edit', tripId, ctx.userId)) return permissionDenied(); if (!this.assignments.getAssignmentForTrip(assignmentId, tripId)) return errorResult('Assignment not found.'); - const participants = this.assignments.setParticipants(assignmentId, userIds); + const participants = this.assignments.setParticipants(assignmentId, userIds, tripId); this.guards.safeBroadcast(tripId, 'assignment:participants', { assignmentId, participants }); return ok({ participants }); } diff --git a/server/src/nest/assignments/assignments.service.ts b/server/src/nest/assignments/assignments.service.ts index 379943811..b53b657e2 100644 --- a/server/src/nest/assignments/assignments.service.ts +++ b/server/src/nest/assignments/assignments.service.ts @@ -283,12 +283,21 @@ export class AssignmentsService { return this.getAssignmentWithPlace(Number(id)); } - setParticipants(assignmentId: string | number, userIds: number[]) { + /** + * Both callers have already proven the assignment sits on `tripId`; this + * settles the other half, that the ids in the body do too. Off-roster ids drop + * silently rather than 400, matching bag members and reservation travellers — + * the participants box sends the whole list back on every edit, so rejecting + * the request would strand a trip whose membership changed underneath it. + */ + setParticipants(assignmentId: string | number, userIds: number[], tripId: string | number) { + const roster = this.dbs.rosterUserIds(tripId); + const scoped = userIds.filter(id => roster.has(id)); this.dbs.transaction(() => { this.dbs.run('DELETE FROM assignment_participants WHERE assignment_id = ?', assignmentId); - if (userIds.length > 0) { + if (scoped.length > 0) { const insert = this.dbs.prepare('INSERT OR IGNORE INTO assignment_participants (assignment_id, user_id) VALUES (?, ?)'); - for (const userId of userIds) insert.run(assignmentId, userId); + for (const userId of scoped) insert.run(assignmentId, userId); } }); diff --git a/server/tests/unit/nest/assignments.controller.test.ts b/server/tests/unit/nest/assignments.controller.test.ts index 2079addef..5580d5301 100644 --- a/server/tests/unit/nest/assignments.controller.test.ts +++ b/server/tests/unit/nest/assignments.controller.test.ts @@ -101,7 +101,8 @@ describe('AssignmentOpsController (parity with the per-assignment op routes)', ( expect(setParticipants).not.toHaveBeenCalled(); const s = svc({ getAssignmentForTrip: vi.fn().mockReturnValue({ id: 9 }), setParticipants, broadcast } as Partial); expect(new AssignmentOpsController(s).setParticipants(user, '5', '9', { user_ids: [2] }, 'sock')).toEqual({ participants: [{ user_id: 2 }] }); - expect(setParticipants).toHaveBeenCalledWith('9', [2]); + // The trip comes along so the service can confine the ids to its roster. + expect(setParticipants).toHaveBeenCalledWith('9', [2], '5'); expect(broadcast).toHaveBeenCalledWith('5', 'assignment:participants', { assignmentId: 9, participants: [{ user_id: 2 }] }, 'sock'); }); }); diff --git a/server/tests/unit/nest/assignments.service.test.ts b/server/tests/unit/nest/assignments.service.test.ts index bda5951c4..db6d1bf2d 100644 --- a/server/tests/unit/nest/assignments.service.test.ts +++ b/server/tests/unit/nest/assignments.service.test.ts @@ -205,7 +205,7 @@ describe('listDayAssignments', () => { testDb.prepare('INSERT INTO place_tags (place_id, tag_id) VALUES (?, ?)').run(place.id, tag.id); const a1 = createDayAssignment(testDb, day.id, place.id, { order_index: 1 }); const a2 = createDayAssignment(testDb, day.id, second.id, { order_index: 0 }); - svc.setParticipants(a1.id, [user.id]); + svc.setParticipants(a1.id, [user.id], trip.id); const list = svc.listDayAssignments(day.id); expect(list.map(a => a.id)).toEqual([a2.id, a1.id]); @@ -284,45 +284,64 @@ describe('moveAssignment', () => { describe('getParticipants / setParticipants', () => { it('ASG-SVC-017: setParticipants replaces the list and returns the joined rows', () => { - const { user, day, place } = fixture(); + const { user, trip, day, place } = fixture(); const { user: peer } = createUser(testDb); + addTripMember(testDb, trip.id, peer.id); const a = createDayAssignment(testDb, day.id, place.id); - svc.setParticipants(a.id, [user.id]); - const replaced = svc.setParticipants(a.id, [peer.id]); + svc.setParticipants(a.id, [user.id], trip.id); + const replaced = svc.setParticipants(a.id, [peer.id], trip.id); expect(replaced).toEqual([{ user_id: peer.id, username: peer.username, avatar: null }]); expect(svc.getParticipants(a.id)).toEqual(replaced); }); it('ASG-SVC-018: empty array clears all participants', () => { - const { user, day, place } = fixture(); + const { user, trip, day, place } = fixture(); const a = createDayAssignment(testDb, day.id, place.id); - svc.setParticipants(a.id, [user.id]); - expect(svc.setParticipants(a.id, [])).toEqual([]); + svc.setParticipants(a.id, [user.id], trip.id); + expect(svc.setParticipants(a.id, [], trip.id)).toEqual([]); expect(svc.getParticipants(a.id)).toEqual([]); }); it('ASG-SVC-019: username COALESCEs display_name over username', () => { - const { user, day, place } = fixture(); + const { user, trip, day, place } = fixture(); testDb.prepare('UPDATE users SET display_name = ? WHERE id = ?').run('Fancy Name', user.id); const a = createDayAssignment(testDb, day.id, place.id); - const rows = svc.setParticipants(a.id, [user.id]); + const rows = svc.setParticipants(a.id, [user.id], trip.id); expect(rows).toEqual([{ user_id: user.id, username: 'Fancy Name', avatar: null }]); }); it('ASG-SVC-020: duplicate user ids collapse via INSERT OR IGNORE', () => { - const { user, day, place } = fixture(); + const { user, trip, day, place } = fixture(); const a = createDayAssignment(testDb, day.id, place.id); - const rows = svc.setParticipants(a.id, [user.id, user.id]); + const rows = svc.setParticipants(a.id, [user.id, user.id], trip.id); expect(rows).toHaveLength(1); }); - it('ASG-SVC-028: a failing insert rolls back the whole replace (delete included)', () => { - const { user, day, place } = fixture(); + // Used to assert that an id violating the users FK threw and rolled the delete + // back. It cannot reach the insert any more: the roster filter drops an id that + // is not on the trip, so a list of nothing but strangers is simply an empty + // list, and the delete stands. The rollback itself is still covered — the + // transaction wraps delete and insert together, and ASG-SVC-018 pins that an + // explicit empty list clears. + it('ASG-SVC-028: a list of ids that are not on the trip clears rather than throwing', () => { + const { user, trip, day, place } = fixture(); const a = createDayAssignment(testDb, day.id, place.id); - svc.setParticipants(a.id, [user.id]); - // 999999 violates the users FK — the transaction must undo the delete too. - expect(() => svc.setParticipants(a.id, [999999])).toThrow(); - expect(svc.getParticipants(a.id)).toEqual([{ user_id: user.id, username: user.username, avatar: null }]); + svc.setParticipants(a.id, [user.id], trip.id); + expect(() => svc.setParticipants(a.id, [999999], trip.id)).not.toThrow(); + expect(svc.getParticipants(a.id)).toEqual([]); + }); + + it('ASG-SVC-029: keeps the trip members and drops the stranger in one call', () => { + const { user, trip, day, place } = fixture(); + const { user: member } = createUser(testDb); + const { user: stranger } = createUser(testDb); + addTripMember(testDb, trip.id, member.id); + const a = createDayAssignment(testDb, day.id, place.id); + + const rows = svc.setParticipants(a.id, [user.id, stranger.id, member.id], trip.id) as { user_id: number }[]; + + expect(rows.map(r => r.user_id).sort()).toEqual([user.id, member.id].sort()); + expect(JSON.stringify(rows)).not.toContain(stranger.username); }); });