diff --git a/server/tests/unit/mcp/tools-days-accommodations.test.ts b/server/tests/unit/mcp/tools-days-accommodations.test.ts index 7700c353e..6139928c1 100644 --- a/server/tests/unit/mcp/tools-days-accommodations.test.ts +++ b/server/tests/unit/mcp/tools-days-accommodations.test.ts @@ -108,6 +108,27 @@ describe('Tool: create_day', () => { }); }); + it('reports every bad reference in one message rather than the first', async () => { + // The validator returns a list and the tool joins it. Without a case that + // produces more than zero errors, that join is never executed, and a tool + // that silently created a stay against a foreign place would look fine. + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id, { date: '2025-06-15' }); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ + name: 'create_accommodation', + arguments: { tripId: trip.id, place_id: 999999, start_day_id: 999998, end_day_id: day.id }, + }); + expect(result.isError).toBe(true); + const text = (result.content as { text: string }[])[0].text; + expect(text).toContain('Place not found'); + expect(text).toContain('Start day not found'); + expect(text).toContain(', '); + }); + expect(testDb.prepare('SELECT COUNT(*) AS n FROM day_accommodations').get()).toEqual({ n: 0 }); + }); + it('returns access denied for non-member', async () => { const { user } = createUser(testDb); const { user: other } = createUser(testDb); diff --git a/server/tests/unit/nest/accommodations.service.test.ts b/server/tests/unit/nest/accommodations.service.test.ts index 44ae29ff2..0112e1dbc 100644 --- a/server/tests/unit/nest/accommodations.service.test.ts +++ b/server/tests/unit/nest/accommodations.service.test.ts @@ -37,7 +37,7 @@ vi.mock('../../../src/websocket', () => ({ broadcast })); import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; -import { createUser, createTrip, createDay, createPlace, createDayAccommodation } from '../../helpers/factories'; +import { createUser, createTrip, createDay, createPlace, createDayAccommodation, addTripMember } from '../../helpers/factories'; import { DatabaseService } from '../../../src/nest/database/database.service'; import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; @@ -92,6 +92,24 @@ describe('validateAccommodationRefs', () => { const errors = svc.validateAccommodationRefs(trip.id, place.id, 99999, day.id); expect(errors.some((e: any) => e.field === 'start_day_id')).toBe(true); }); + + it('ACC-003 — reports every bad ref, end_day_id included', () => { + // The controller only surfaces errors[0], so an unchecked end_day_id would let a + // stay point at a day from another trip and never be reported. + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const errors = svc.validateAccommodationRefs(trip.id, 99999, 99998, 99997); + expect(errors.map((e: any) => e.field)).toEqual(['place_id', 'start_day_id', 'end_day_id']); + }); + + it('ACC-004 — skips the refs the caller left undefined', () => { + // The plugin update path (accommodations.rpc) passes only the fields it changes. + // If an absent ref were validated, every partial update would 403 "Place not found". + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + expect(svc.validateAccommodationRefs(trip.id)).toHaveLength(0); + expect(svc.validateAccommodationRefs(trip.id, undefined, undefined, undefined)).toHaveLength(0); + }); }); describe('createAccommodation', () => { @@ -128,6 +146,23 @@ describe('createAccommodation', () => { expect(reservation.type).toBe('hotel'); expect(reservation.status).toBe('confirmed'); }); + + it('ACC-005 — titles the partner reservation "Hotel" when the stay has no place', () => { + // day_accommodations.place_id is nullable (ON DELETE SET NULL) while + // reservations.title is NOT NULL: without the fallback the reservation insert + // fails and takes the whole transaction — and the stay — down with it. + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id) as any; + + const accom = svc.createAccommodation(trip.id, { + place_id: null as unknown as number, start_day_id: day.id, end_day_id: day.id, + }) as any; + + expect(accom.place_name).toBeNull(); + const reservation = testDb.prepare('SELECT title FROM reservations WHERE accommodation_id = ?').get(accom.id) as any; + expect(reservation.title).toBe('Hotel'); + }); }); describe('getAccommodation', () => { @@ -187,6 +222,63 @@ describe('updateAccommodation', () => { const row = svc.getAccommodation(accom.id, trip.id) as any; expect(row.confirmation).toBe('ABC123'); }); + + it('ACC-006 — updates a stay that has no linked reservation', () => { + // Stays imported before the auto-reservation existed have no partner row. The sync + // block must be skipped for them, not throw on a missing reservation. + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id) as any; + const place = createPlace(testDb, trip.id, { name: 'Hotel' }) as any; + // The factory writes the stay row directly, without the partner reservation. + const accom = createDayAccommodation(testDb, trip.id, place.id, day.id, day.id) as any; + + const existing = svc.getAccommodation(accom.id, trip.id)!; + const updated = svc.updateAccommodation(accom.id, existing as any, { check_in: '15:00' }) as any; + + expect(updated.check_in).toBe('15:00'); + expect(testDb.prepare('SELECT COUNT(*) as n FROM reservations WHERE accommodation_id = ?').get(accom.id)).toMatchObject({ n: 0 }); + }); + + it('ACC-007 — merges into the reservation metadata instead of replacing it', () => { + // The stay is created with a check-in, so the reservation already carries metadata. + // An update that only moves the check-out must parse and extend that object — + // writing a fresh one would silently drop the check-in window. + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id) as any; + const place = createPlace(testDb, trip.id, { name: 'Hotel' }) as any; + const accom = svc.createAccommodation(trip.id, { + place_id: place.id, start_day_id: day.id, end_day_id: day.id, check_in: '15:00', + }) as any; + + const existing = svc.getAccommodation(accom.id, trip.id)!; + svc.updateAccommodation(accom.id, existing as any, { check_in_end: '20:00', check_out: '11:00' }); + + const reservation = testDb.prepare('SELECT metadata FROM reservations WHERE accommodation_id = ?').get(accom.id) as any; + expect(JSON.parse(reservation.metadata)).toEqual({ + check_in_time: '15:00', check_in_end_time: '20:00', check_out_time: '11:00', + }); + }); + + it('ACC-008 — leaves the reservation confirmation number alone when the stay has none', () => { + // COALESCE(?, confirmation_number): a stay without a confirmation must not null out + // a number that was typed on the reservation itself. + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id) as any; + const place = createPlace(testDb, trip.id, { name: 'Hotel' }) as any; + const accom = svc.createAccommodation(trip.id, { + place_id: place.id, start_day_id: day.id, end_day_id: day.id, + }) as any; + testDb.prepare('UPDATE reservations SET confirmation_number = ? WHERE accommodation_id = ?').run('RES-9', accom.id); + + const existing = svc.getAccommodation(accom.id, trip.id)!; + svc.updateAccommodation(accom.id, existing as any, { check_in: '15:00' }); + + const reservation = testDb.prepare('SELECT confirmation_number FROM reservations WHERE accommodation_id = ?').get(accom.id) as any; + expect(reservation.confirmation_number).toBe('RES-9'); + }); }); describe('deleteAccommodation', () => { @@ -225,6 +317,29 @@ describe('deleteAccommodation', () => { const result = svc.deleteAccommodation(accom.id); expect(result.linkedReservationId).toBeNull(); }); + + it('ACC-009 — also deletes the budget item that hangs off the linked reservation', () => { + // The cost row references the reservation, not the stay, so the delete has to + // cascade twice. Skipping the second hop strands a budget line whose reservation + // is gone, and the costs view can no longer reach it to remove it. + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id) as any; + const place = createPlace(testDb, trip.id, { name: 'Hotel' }) as any; + const accom = svc.createAccommodation(trip.id, { + place_id: place.id, start_day_id: day.id, end_day_id: day.id, + }) as any; + + const reservation = testDb.prepare('SELECT id FROM reservations WHERE accommodation_id = ?').get(accom.id) as any; + const budgetItemId = testDb.prepare( + 'INSERT INTO budget_items (trip_id, name, category, total_price, reservation_id) VALUES (?, ?, ?, ?, ?)' + ).run(trip.id, 'Hotel stay', 'Accommodation', 240, reservation.id).lastInsertRowid as number; + + const result = svc.deleteAccommodation(accom.id); + + expect(result).toEqual({ linkedReservationId: reservation.id, deletedBudgetItemId: budgetItemId }); + expect(testDb.prepare('SELECT id FROM budget_items WHERE id = ?').get(budgetItemId)).toBeUndefined(); + }); }); describe('listAccommodations', () => { @@ -260,6 +375,155 @@ describe('quirk fixes', () => { }); }); +describe('route-facing delegators', () => { + // list/validateRefs/get/create/update/remove are what the controller, the plugin RPC + // and trip-read-model call; the MCP tools call the long names. Both sets have to land + // on the same SQL, and the short ones get raw route params — strings where the long + // ones get numbers. + + it('ACC-010 — list() returns what listAccommodations() returns, for a string and a number trip id', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id) as any; + const place = createPlace(testDb, trip.id, { name: 'Pension Alpen' }) as any; + createDayAccommodation(testDb, trip.id, place.id, day.id, day.id); + + expect(svc.list(String(trip.id))).toEqual(svc.listAccommodations(trip.id)); + expect((svc.list(trip.id) as any[])[0].place_name).toBe('Pension Alpen'); + }); + + it('ACC-011 — validateRefs() returns the same errors validateAccommodationRefs() does', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id) as any; + const place = createPlace(testDb, trip.id, { name: 'Hotel' }) as any; + + expect(svc.validateRefs(trip.id, place.id, day.id, day.id)).toHaveLength(0); + // The controller 404s with errors[0].message, so the shape matters, not just the count. + expect(svc.validateRefs(trip.id, 99999, day.id, day.id)).toEqual([{ field: 'place_id', message: 'Place not found' }]); + }); + + it('ACC-012 — create() writes the stay, get() reads it back trip-scoped', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const otherTrip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id) as any; + const place = createPlace(testDb, trip.id, { name: 'Hotel' }) as any; + + // Both ids arrive from the route as strings. + const created = svc.create(String(trip.id), { + place_id: place.id, start_day_id: day.id, end_day_id: day.id, confirmation: 'XY-1', + }) as any; + + expect(svc.get(String(created.id), String(trip.id))).toMatchObject({ id: created.id, confirmation: 'XY-1' }); + // Trip-scoped: a stay must not be readable through another trip's URL. + expect(svc.get(created.id, otherTrip.id)).toBeUndefined(); + }); + + it('ACC-013 — update() syncs the partner reservation even when the id is a string', () => { + // updateAccommodation looks the reservation up with Number(id); if that coercion + // regresses, the REST path silently stops syncing check-in times. + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id) as any; + const place = createPlace(testDb, trip.id, { name: 'Hotel' }) as any; + const accom = svc.create(trip.id, { place_id: place.id, start_day_id: day.id, end_day_id: day.id }) as any; + + const existing = svc.get(accom.id, trip.id)!; + const updated = svc.update(String(accom.id), existing as any, { check_in: '16:00', notes: 'late arrival' }) as any; + + expect(updated).toMatchObject({ check_in: '16:00', notes: 'late arrival' }); + const reservation = testDb.prepare('SELECT metadata FROM reservations WHERE accommodation_id = ?').get(accom.id) as any; + expect(JSON.parse(reservation.metadata).check_in_time).toBe('16:00'); + }); + + it('ACC-014 — remove() reports the partner reservation it took with it', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id) as any; + const place = createPlace(testDb, trip.id, { name: 'Hotel' }) as any; + const accom = svc.create(trip.id, { place_id: place.id, start_day_id: day.id, end_day_id: day.id }) as any; + const reservation = testDb.prepare('SELECT id FROM reservations WHERE accommodation_id = ?').get(accom.id) as any; + + // The controller broadcasts reservation:deleted off this return value. + expect(svc.remove(String(accom.id))).toEqual({ linkedReservationId: reservation.id, deletedBudgetItemId: null }); + expect(svc.get(accom.id, trip.id)).toBeUndefined(); + }); +}); + +describe('trip access and edit permission', () => { + it('ACC-015 — verifyTripAccess resolves the trip from a string id and from a number', () => { + // The REST path hands over the raw :tripId param, the MCP tools the parsed number. + // Without the Number() coercion one of the two callers stops finding its trip. + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + + expect(svc.verifyTripAccess(String(trip.id), user.id)).toMatchObject({ id: trip.id, user_id: user.id }); + expect(svc.verifyTripAccess(trip.id, user.id)).toMatchObject({ id: trip.id, user_id: user.id }); + }); + + it('ACC-016 — verifyTripAccess returns nothing for a user who is neither owner nor member', () => { + const { user: owner } = createUser(testDb); + const { user: stranger } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + + expect(svc.verifyTripAccess(trip.id, stranger.id)).toBeUndefined(); + }); + + it('ACC-017 — canEdit lets the owner through on the default day_edit level', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const access = svc.verifyTripAccess(trip.id, user.id)!; + + expect(svc.canEdit(access, user as any)).toBe(true); + }); + + it('ACC-018 — canEdit lets a member through: it derives isMember from the trip owner id', () => { + // day_edit defaults to trip_member. canEdit passes `trip.user_id !== user.id` as the + // isMember flag, so inverting that comparison would lock every invited traveller out + // of the day plan while the owner still edits fine. + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, member.id); + const access = svc.verifyTripAccess(trip.id, member.id)!; + + expect(svc.canEdit(access, member as any)).toBe(true); + }); + + it('ACC-019 — canEdit denies a member once day_edit is raised to trip_owner', () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, member.id); + const permissions = new PermissionsService(new DatabaseService(testDb)); + + try { + permissions.savePermissions({ day_edit: 'trip_owner' }); + expect(svc.canEdit(svc.verifyTripAccess(trip.id, member.id)!, member as any)).toBe(false); + // The owner keeps the right, otherwise the setting would lock out the whole trip. + expect(svc.canEdit(svc.verifyTripAccess(trip.id, owner.id)!, owner as any)).toBe(true); + } finally { + // The permission cache is module-scoped and outlives resetTestDb — drop the row and + // the cache together, or every later case in this file inherits the raised level. + testDb.prepare("DELETE FROM app_settings WHERE key = 'perm_day_edit'").run(); + permissions.invalidatePermissionsCache(); + } + }); + + it('ACC-020 — broadcast forwards the socket id so the originating client is not echoed', () => { + // The X-Socket-Id contract: the facade must pass the id straight through as the + // fourth argument. Dropping it makes the sender apply its own change twice. + broadcast.mockClear(); + + svc.broadcast('5', 'accommodation:deleted', { accommodationId: 9 }, 'sock-1'); + expect(broadcast).toHaveBeenCalledWith('5', 'accommodation:deleted', { accommodationId: 9 }, 'sock-1'); + + svc.broadcast('5', 'accommodation:updated', { accommodation: { id: 9 } }, undefined); + expect(broadcast).toHaveBeenLastCalledWith('5', 'accommodation:updated', { accommodation: { id: 9 } }, undefined); + }); +}); + describe('AccommodationsService wiring', () => { it('ACC-001: the module carries the controller, the service and the RPC surface', () => { expectRegisteredController(AccommodationsModule, AccommodationsController); diff --git a/server/tests/unit/nest/calendar.service.test.ts b/server/tests/unit/nest/calendar.service.test.ts index 07486b4ad..db20dbc73 100644 --- a/server/tests/unit/nest/calendar.service.test.ts +++ b/server/tests/unit/nest/calendar.service.test.ts @@ -51,7 +51,7 @@ import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { ReservationsService } from '../../../src/nest/reservations/reservations.service'; import { BudgetService } from '../../../src/nest/budget/budget.service'; import { ExchangeRatesService } from '../../../src/nest/budget/exchange-rates.service'; -import { CalendarService } from '../../../src/nest/calendar/calendar.service'; +import { CalendarService, foldICS } from '../../../src/nest/calendar/calendar.service'; import { CalendarModule } from '../../../src/nest/calendar/calendar.module'; import { expectRegisteredProvider } from '../../helpers/module-providers'; @@ -331,6 +331,256 @@ describe('exportICS', () => { expect(ics).toContain('BEGIN:VTIMEZONE\r\nTZID:Asia/Tokyo'); expect(ics).not.toContain('DTSTART:20250602T090000'); }); + + it('CAL-011: a reservation_time with seconds is kept as is and a date-only end time emits no DTEND', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Second Precision' }); + const reservation = createReservation(testDb, trip.id, { title: 'Guided Walk', type: 'activity' }); + // Importers write both shapes: "…T14:00" (padded to seconds) and "…T14:00:00" + // (already 15 chars). Padding the second one again would produce a 17-char + // value that no client parses. + testDb + .prepare('UPDATE reservations SET reservation_time=?, reservation_end_time=? WHERE id=?') + .run('2025-06-02T14:00:00', '2025-06-03', reservation.id); + + const { ics } = svc.exportICS(trip.id); + + expect(ics).toContain('DTSTART:20250602T140000'); + // The end value carries no clock time, so it cannot be a DTEND for a timed + // event; emitting the bare "20250603" would make the event span a nonsense + // range instead of being left open. + expect(ics).not.toContain('DTEND'); + }); + + it('CAL-012: reservations with no placeable time are dropped instead of emitting a broken VEVENT', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Timeless' }); + // Relative "Day N" trips store a clock time without a date; there is nothing + // to anchor it to, so the reservation must not reach the calendar at all. + const timeOnly = createReservation(testDb, trip.id, { title: 'Floating Dinner', type: 'restaurant' }); + testDb.prepare('UPDATE reservations SET reservation_time=? WHERE id=?').run('19:30', timeOnly.id); + // A transport row whose endpoints were never imported has no fallback time either. + const noEndpoints = createReservation(testDb, trip.id, { title: 'Endpointless Train', type: 'transport' }); + testDb.prepare('UPDATE reservations SET reservation_time=NULL WHERE id=?').run(noEndpoints.id); + + const { ics } = svc.exportICS(trip.id); + + expect(ics).not.toContain('SUMMARY:Floating Dinner'); + expect(ics).not.toContain('SUMMARY:Endpointless Train'); + // The trip itself has no dates and no days, so dropping both reservations + // leaves an event-free calendar rather than a partially filled one. + expect(ics).not.toContain('BEGIN:VEVENT'); + }); + + it('CAL-013: endpoints without a stored timezone fall back to their coordinates', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'No Stored Zone' }); + const reservation = createReservation(testDb, trip.id, { title: 'CDG to JFK', type: 'flight' }); + testDb.prepare('UPDATE reservations SET reservation_time=NULL WHERE id=?').run(reservation.id); + const insertEp = testDb.prepare( + 'INSERT INTO reservation_endpoints (reservation_id, role, sequence, name, code, lat, lng, timezone, local_time, local_date) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)' + ); + // Endpoints created before the importer learned to store IANA zones have only + // coordinates. Without the lookup fallback both ends would go floating and the + // subscriber would see the flight in their own zone (#1453). + insertEp.run(reservation.id, 'from', 0, 'Paris CDG', 'CDG', 49.0, 2.5, null, '09:00', '2025-06-02'); + insertEp.run(reservation.id, 'to', 1, 'New York JFK', 'JFK', 40.6, -73.8, null, '12:00', '2025-06-02'); + + const { ics } = svc.exportICS(trip.id); + + expect(ics).toContain('DTSTART;TZID=Europe/Paris:20250602T090000'); + expect(ics).toContain('DTEND;TZID=America/New_York:20250602T120000'); + }); + + it('CAL-014: a single-endpoint transport emits DTSTART only', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'One Way' }); + const reservation = createReservation(testDb, trip.id, { title: 'Airport Transfer', type: 'transport' }); + testDb.prepare('UPDATE reservations SET reservation_time=NULL WHERE id=?').run(reservation.id); + // Only the departure was imported. Using it for DTEND as well would emit a + // zero-length event; leaving DTEND out lets clients apply their default duration. + testDb.prepare( + 'INSERT INTO reservation_endpoints (reservation_id, role, sequence, name, code, lat, lng, timezone, local_time, local_date) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)' + ).run(reservation.id, 'from', 0, 'Paris CDG', 'CDG', 49.0, 2.5, 'Europe/Paris', '09:00', '2025-06-02'); + + const { ics } = svc.exportICS(trip.id); + + expect(ics).toContain('DTSTART;TZID=Europe/Paris:20250602T090000'); + expect(ics).not.toContain('DTEND'); + }); + + it('CAL-015: a timed assignment carries its notes and address into DESCRIPTION and LOCATION', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Guided Day' }); + const day = createDay(testDb, trip.id, { date: '2025-06-02' }); + const place = createPlace(testDb, trip.id, { name: 'Louvre', lat: 48.8606, lng: 2.3376 }); + testDb.prepare("UPDATE places SET address = 'Rue de Rivoli' WHERE id = ?").run(place.id); + const withNotes = createDayAssignment(testDb, day.id, place.id, { notes: 'meet the guide' }); + testDb.prepare('UPDATE day_assignments SET assignment_time=? WHERE id=?').run('09:00', withNotes.id); + const withoutNotes = createDayAssignment(testDb, day.id, place.id); + testDb.prepare('UPDATE day_assignments SET assignment_time=? WHERE id=?').run('11:00', withoutNotes.id); + + const ics = svc.exportICS(trip.id).ics.replace(/\r\n /g, ''); + + // Notes come first and the address is appended on its own line; dropping the + // separator would glue them into one unreadable run. + expect(ics).toContain('DESCRIPTION:meet the guide\\nRue de Rivoli'); + // Without notes the address must start the description rather than a stray newline. + expect(ics).toContain('DESCRIPTION:Rue de Rivoli\r\n'); + expect(ics).toContain('LOCATION:Rue de Rivoli'); + }); + + it('CAL-016: a day without a date is skipped and a titled day uses its own title', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Mixed Days' }); + // Days of a relative "Day N" trip have no date; they cannot be placed on a + // calendar, and emitting them produced a VEVENT with an empty DTSTART. + const ghost = createDay(testDb, trip.id, { title: 'Ghost Day' }); + createDayNote(testDb, ghost.id, trip.id, { text: 'never exported' }); + const named = createDay(testDb, trip.id, { date: '2025-06-02', title: 'Museum Day' }); + const place = createPlace(testDb, trip.id, { name: 'Bare Spot' }); + createDayAssignment(testDb, named.id, place.id); + + const ics = svc.exportICS(trip.id).ics.replace(/\r\n /g, ''); + + expect(ics).not.toContain('Ghost Day'); + expect(ics).not.toContain('never exported'); + // A day with a title must not be renamed to "Day ". + expect(ics).toContain('SUMMARY:Museum Day'); + // No address and no notes → the bullet stays bare, with no empty "()" or dash. + expect(ics).toContain('DESCRIPTION:• Bare Spot\r\n'); + }); + + it('CAL-017: a reservation with no type, metadata or notes emits no empty DESCRIPTION', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Bare' }); + const reservation = createReservation(testDb, trip.id, { title: 'Bare Booking', type: 'hotel' }); + // Rows imported before `type` became mandatory still exist in prod. An empty + // DESCRIPTION line is invalid enough for some clients to reject the file. + testDb + .prepare('UPDATE reservations SET type=NULL, reservation_time=? WHERE id=?') + .run('2025-06-02', reservation.id); + + const { ics } = svc.exportICS(trip.id); + + expect(ics).toContain('SUMMARY:Bare Booking'); + expect(ics).not.toContain('DESCRIPTION'); + }); + + it('CAL-018: flight metadata with only one airport emits only that side of the route', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Half Routes' }); + const inbound = createReservation(testDb, trip.id, { title: 'Inbound', type: 'flight' }); + const outbound = createReservation(testDb, trip.id, { title: 'Outbound', type: 'flight' }); + const setMeta = testDb.prepare('UPDATE reservations SET reservation_time=?, metadata=? WHERE id=?'); + // A hand-entered flight often has one airport only; the missing side must be + // left out instead of printing "To: undefined". + setMeta.run('2025-06-02T09:00', JSON.stringify({ arrival_airport: 'JFK' }), inbound.id); + setMeta.run('2025-06-09T09:00', JSON.stringify({ departure_airport: 'CDG' }), outbound.id); + + const ics = svc.exportICS(trip.id).ics.replace(/\r\n /g, ''); + + expect(ics).toContain('DESCRIPTION:Type: flight\\nTo: JFK\r\n'); + expect(ics).toContain('DESCRIPTION:Type: flight\\nFrom: CDG\r\n'); + }); + + it('CAL-019: legs and endpoints without names produce no Route line', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Nameless' }); + const legs = createReservation(testDb, trip.id, { title: 'Codeless Legs', type: 'flight' }); + // Multi-leg metadata whose legs carry no airports would otherwise render + // "Route: " with nothing after it. + testDb.prepare('UPDATE reservations SET reservation_time=?, metadata=? WHERE id=?') + .run('2025-06-02T09:00', JSON.stringify({ legs: [{}, {}] }), legs.id); + const ferry = createReservation(testDb, trip.id, { title: 'Nameless Ferry', type: 'transport' }); + testDb.prepare('UPDATE reservations SET reservation_time=NULL WHERE id=?').run(ferry.id); + const insertEp = testDb.prepare( + 'INSERT INTO reservation_endpoints (reservation_id, role, sequence, name, code, lat, lng, timezone, local_time, local_date) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)' + ); + // Same for endpoints with neither a code nor a name: the derived route would + // collapse to a single arrow between two blanks. + insertEp.run(ferry.id, 'from', 0, '', null, 1.0, 1.0, null, null, '2025-06-03'); + insertEp.run(ferry.id, 'to', 1, '', null, 1.1, 1.1, null, null, '2025-06-03'); + + const ics = svc.exportICS(trip.id).ics.replace(/\r\n /g, ''); + + expect(ics).toContain('SUMMARY:Codeless Legs'); + expect(ics).toContain('SUMMARY:Nameless Ferry'); + expect(ics).not.toContain('Route:'); + }); + + it('CAL-020: an empty trip title falls back for SUMMARY, X-WR-CALNAME and the filename', () => { + const { user } = createUser(testDb); + // The title is only NOT NULL, not non-empty; an empty one used to produce + // "SUMMARY:" and a filename of ".ics". + const trip = createTrip(testDb, user.id, { title: '', start_date: '2025-06-01', end_date: '2025-06-02' }); + + const { ics, filename } = svc.exportICS(trip.id); + + expect(ics).toContain('X-WR-CALNAME:TREK Trip'); + expect(ics).toContain('SUMMARY:Trip'); + expect(filename).toBe('trek-trip.ics'); + }); + + it('CAL-021: a corrupt day date degrades the VTIMEZONE offset instead of throwing', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Corrupt Date' }); + // days.date is free-form TEXT; a legacy/imported row can hold a date that no + // parser accepts. The representative date reaches Intl through the VTIMEZONE + // fallback, and Intl throws a RangeError on an invalid Date — which would take + // the whole export (and the trip's slot in the all-trips feed) down with it. + const day = createDay(testDb, trip.id, { date: '2025-13-45' }); + const place = createPlace(testDb, trip.id, { name: 'Senso-ji', lat: 35.7148, lng: 139.7967 }); + const assignment = createDayAssignment(testDb, day.id, place.id); + testDb.prepare('UPDATE day_assignments SET assignment_time=? WHERE id=?').run('09:00', assignment.id); + + let ics = ''; + expect(() => { ics = svc.exportICS(trip.id).ics; }).not.toThrow(); + + expect(ics).toContain('DTSTART;TZID=Asia/Tokyo:20251345T090000'); + expect(ics).toContain('BEGIN:VTIMEZONE\r\nTZID:Asia/Tokyo'); + expect(ics).toContain('TZOFFSETFROM:+0000'); + expect(ics).toContain('TZOFFSETTO:+0000'); + }); + + it('CAL-022: the timezone cache stays correct after it hits its 1000-entry bound', () => { + const { user } = createUser(testDb); + const bulkTrip = createTrip(testDb, user.id, { title: 'Cache Filler' }); + const realTrip = createTrip(testDb, user.id, { title: 'After The Bound' }); + + const insertRes = testDb.prepare('INSERT INTO reservations (trip_id, title, type, reservation_time) VALUES (?, ?, ?, NULL)'); + const insertEp = testDb.prepare( + 'INSERT INTO reservation_endpoints (reservation_id, role, sequence, name, code, lat, lng, timezone, local_time, local_date) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)' + ); + // Endpoint timezones are free-form strings written by importers and plugins, so + // the validity cache is keyed by attacker-ish input and has to be bounded. Push + // it past the bound in one export. + testDb.transaction(() => { + for (let i = 0; i < 1010; i++) { + const resId = insertRes.run(bulkTrip.id, `Bulk ${i}`, 'transport').lastInsertRowid as number; + insertEp.run(resId, 'from', 0, `Stop ${i}`, null, 49.0, 2.5, `Bogus/Zone-${i}`, '09:00', '2025-06-02'); + } + })(); + + const bulk = svc.exportICS(bulkTrip.id).ics; + // Every one of them has to reach the zone check — otherwise the bound is never + // approached and this case would pass without exercising anything. + expect(bulk).toContain('SUMMARY:Bulk 0'); + expect(bulk).toContain('SUMMARY:Bulk 1009'); + expect(bulk).toContain('DTSTART:20250602T090000'); + expect(bulk).not.toContain('TZID=Bogus/Zone-0:'); + + const reservation = createReservation(testDb, realTrip.id, { title: 'CDG to JFK', type: 'flight' }); + testDb.prepare('UPDATE reservations SET reservation_time=NULL WHERE id=?').run(reservation.id); + insertEp.run(reservation.id, 'from', 0, 'Paris CDG', 'CDG', 49.0, 2.5, 'Europe/Paris', '09:00', '2025-06-02'); + + // Clearing the cache must make later zones be re-checked. A regression that + // evicted by writing `false` instead would silently strip the TZID from every + // export after the thousandth distinct zone string. + const { ics } = svc.exportICS(realTrip.id); + expect(ics).toContain('DTSTART;TZID=Europe/Paris:20250602T090000'); + expect(ics).toContain('BEGIN:VTIMEZONE\r\nTZID:Europe/Paris'); + }); }); describe('folded quirk branches', () => { @@ -379,6 +629,65 @@ describe('folded quirk branches', () => { }); +// foldICS is the last thing that touches the bytes of both the download and the +// feed, and it is exported, so it is pinned directly instead of through a trip. +describe('foldICS', () => { + // Physical lines are measured in octets, so the split points below are byte + // offsets, not character offsets. + const octets = (s: string) => Buffer.from(s, 'utf8').length; + const unfold = (s: string) => s.replace(/\r\n /g, ''); + + it('CAL-023: lines up to 75 octets are untouched and longer ones fold at 75 then 74', () => { + const short = 'SUMMARY:' + 'a'.repeat(67); // exactly 75 octets + expect(foldICS(short)).toBe(short); + + const long = 'SUMMARY:' + 'a'.repeat(200); + const physical = foldICS(long).split('\r\n'); + expect(physical[0]).toHaveLength(75); + // Continuation lines spend one octet on the leading space, so they carry 74 + // payload octets — budgeting 75 there produces 76-octet lines that strict + // validators reject. + expect(physical[1].startsWith(' ')).toBe(true); + expect(physical[1]).toHaveLength(75); + expect(unfold(foldICS(long))).toBe(long); + }); + + it('CAL-024: a two-byte codepoint straddling the 75th octet is not split', () => { + // The fold boundary lands inside "é": without the backoff the two halves are + // decoded separately and the title arrives as U+FFFD in every client. + const line = 'DESCRIPTION:' + 'é'.repeat(60); + expect(octets(line)).toBeGreaterThan(75); + + const folded = foldICS(line); + + expect(folded).not.toContain('�'); + expect(unfold(folded)).toBe(line); + for (const part of folded.split('\r\n')) expect(octets(part)).toBeLessThanOrEqual(75); + }); + + it('CAL-025: a four-byte codepoint backs the split off by more than one octet', () => { + // An emoji needs the backoff to run several times before it reaches a lead + // byte; stopping after a single step still cuts the sequence. + const line = 'SUMMARY:' + '🎌'.repeat(40); + + const folded = foldICS(line); + + expect(folded).not.toContain('�'); + expect(unfold(folded)).toBe(line); + for (const part of folded.split('\r\n')) expect(octets(part)).toBeLessThanOrEqual(75); + }); + + it('CAL-026: folding is applied per content line, so short lines around a long one survive', () => { + const ics = 'BEGIN:VEVENT\r\nSUMMARY:' + 'ü'.repeat(80) + '\r\nEND:VEVENT'; + + const folded = foldICS(ics); + + expect(folded.startsWith('BEGIN:VEVENT\r\n')).toBe(true); + expect(folded.endsWith('\r\nEND:VEVENT')).toBe(true); + expect(unfold(folded)).toBe(ics); + }); +}); + // The snapshot below was taken from the implementation as it stood when the code // moved out of TripsService, before the calendar grew a structured model. It is // the parity net for any change to how the calendar is assembled: the 20 cases diff --git a/server/tests/unit/nest/days.service.test.ts b/server/tests/unit/nest/days.service.test.ts index 0e88a8d2d..e1034e748 100644 --- a/server/tests/unit/nest/days.service.test.ts +++ b/server/tests/unit/nest/days.service.test.ts @@ -3,7 +3,11 @@ * moved 1:1 from the legacy tests/unit/services/dayService.test.ts; * DAY-SVC-027 through DAY-SVC-032 pin the days.bridge delegation; * DAY-SVC-033 through DAY-SVC-036 pin the post-port defect fixes (update - * presence sentinels, accommodation write atomicity, batched tag load). + * presence sentinels, accommodation write atomicity, batched tag load); + * DAY-SVC-037 through DAY-SVC-054 cover the reorder/re-date machinery that + * stayed behind when the accommodation CRUD moved to accommodations/ — mostly + * the SKIP paths (booking without a day, snapshot without a date, stay that + * must not be re-anchored), which the happy-path integration test never walks. * Uses a real in-memory SQLite DB so SQL logic is exercised faithfully. */ import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest'; @@ -48,10 +52,10 @@ vi.mock('../../../src/websocket', () => ({ broadcast: vi.fn() })); import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; -import { createUser, createTrip, createDay, createPlace, createDayAssignment, createDayAccommodation } from '../../helpers/factories'; +import { createUser, createTrip, createDay, createPlace, createDayAssignment, createDayAccommodation, createDayNote, createTag } from '../../helpers/factories'; import { DatabaseService } from '../../../src/nest/database/database.service'; import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; -import { DaysService, addDays } from '../../../src/nest/days/days.service'; +import { DaysService, DayReorderError, addDays } from '../../../src/nest/days/days.service'; import { getDay as bridgeGetDay, listDays as bridgeListDays } from '../../../src/nest/days/days.bridge'; import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { QueryHelpersService } from '../../../src/nest/query-helpers/query-helpers.service'; @@ -359,6 +363,393 @@ describe('quirk fixes', () => { }); }); +// ── reorder / re-date machinery (#589) ──────────────────────────────────────── +// The integration test walks the happy paths (permute, re-pin, extend). What is +// only reachable from here are the paths where something must NOT move: a +// booking the maps say nothing about, a stay whose old dates no longer resolve, +// a trip with no dates at all. Those are the branches a regression would take +// silently — the day list still looks right while a booking or a stay quietly +// jumps to the wrong day. + +describe('restampReservationDates', () => { + it('DAY-SVC-037 — leaves a booking alone when it has no day, no time, or its day did not move', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const stayer = createDay(testDb, trip.id, { date: '2026-01-01' }); + const unmapped = createDay(testDb, trip.id, { date: '2026-01-02' }); + const halfMapped = createDay(testDb, trip.id, { date: '2026-01-03' }); + + const insert = testDb.prepare('INSERT INTO reservations (trip_id, day_id, title, reservation_time) VALUES (?, ?, ?, ?)'); + const unplaced = Number(insert.run(trip.id, null, 'Unplaced', '2026-01-01T09:00').lastInsertRowid); + const untimed = Number(insert.run(trip.id, stayer.id, 'Untimed', null).lastInsertRowid); + const parked = Number(insert.run(trip.id, stayer.id, 'Parked', '2026-01-01T19:00').lastInsertRowid); + const offMap = Number(insert.run(trip.id, unmapped.id, 'Off map', '2026-01-02T12:00').lastInsertRowid); + const halfOff = Number(insert.run(trip.id, halfMapped.id, 'Half off', '2026-01-03T12:00').lastInsertRowid); + + svc.restampReservationDates( + trip.id, + new Map([[stayer.id, '2026-01-01'], [halfMapped.id, '2026-01-03']]), + // stayer keeps its date, unmapped is in neither map, halfMapped only in the old one. + new Map([[stayer.id, '2026-01-01']]), + ); + + const times = Object.fromEntries( + (testDb.prepare('SELECT id, reservation_time FROM reservations WHERE trip_id = ?').all(trip.id) as { id: number; reservation_time: string | null }[]) + .map(r => [r.id, r.reservation_time]) + ); + expect(times).toEqual({ + [unplaced]: '2026-01-01T09:00', + [untimed]: null, + [parked]: '2026-01-01T19:00', + [offMap]: '2026-01-02T12:00', + [halfOff]: '2026-01-03T12:00', + }); + }); + + it('DAY-SVC-038 — shifts every transport leg by the booking\'s day delta and skips a leg with no date', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id, { date: '2026-01-01' }); + const flight = Number(testDb.prepare( + 'INSERT INTO reservations (trip_id, day_id, title, type, reservation_time) VALUES (?, ?, ?, ?, ?)' + ).run(trip.id, day.id, 'HEL-NRT', 'flight', '2026-01-01T08:00').lastInsertRowid); + + const endpoint = testDb.prepare( + 'INSERT INTO reservation_endpoints (reservation_id, role, sequence, name, lat, lng, local_date) VALUES (?, ?, ?, ?, ?, ?, ?)' + ); + const departure = Number(endpoint.run(flight, 'from', 0, 'HEL', 60.31, 24.96, '2026-01-01').lastInsertRowid); + // The arrival sits a day after departure; the legs must keep that distance, + // otherwise a moved overnight flight lands before it took off. + const arrival = Number(endpoint.run(flight, 'to', 1, 'NRT', 35.76, 140.38, '2026-01-02').lastInsertRowid); + const undated = Number(endpoint.run(flight, 'to', 2, 'Unknown', 0, 0, null).lastInsertRowid); + + svc.restampReservationDates(trip.id, new Map([[day.id, '2026-01-01']]), new Map([[day.id, '2026-01-04']])); + + const legs = Object.fromEntries( + (testDb.prepare('SELECT id, local_date FROM reservation_endpoints WHERE reservation_id = ?').all(flight) as { id: number; local_date: string | null }[]) + .map(l => [l.id, l.local_date]) + ); + expect(legs).toEqual({ [departure]: '2026-01-04', [arrival]: '2026-01-05', [undated]: null }); + }); + + it('DAY-SVC-039 — follows end_day_id and keeps a date-only end time date-only', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const start = createDay(testDb, trip.id, { date: '2026-01-01' }); + const end = createDay(testDb, trip.id, { date: '2026-01-03' }); + + const insert = testDb.prepare( + 'INSERT INTO reservations (trip_id, day_id, end_day_id, title, reservation_time, reservation_end_time) VALUES (?, ?, ?, ?, ?, ?)' + ); + const spanning = Number(insert.run(trip.id, start.id, end.id, 'Sleeper train', '2026-01-01T15:00', '2026-01-03').lastInsertRowid); + const openEnded = Number(insert.run(trip.id, null, end.id, 'Open ended', null, null).lastInsertRowid); + + svc.restampReservationDates( + trip.id, + new Map([[start.id, '2026-01-01'], [end.id, '2026-01-03']]), + new Map([[start.id, '2026-01-02'], [end.id, '2026-01-05']]), + ); + + const moved = testDb.prepare('SELECT reservation_time, reservation_end_time FROM reservations WHERE id = ?').get(spanning) as + { reservation_time: string; reservation_end_time: string }; + expect(moved.reservation_time).toBe('2026-01-02T15:00'); + // A date-only end time must not grow a time suffix from the new date string. + expect(moved.reservation_end_time).toBe('2026-01-05'); + expect(testDb.prepare('SELECT reservation_end_time FROM reservations WHERE id = ?').get(openEnded)) + .toMatchObject({ reservation_end_time: null }); + }); +}); + +describe('resyncAccommodationDays', () => { + /** The hotel reservation accommodations/ auto-creates next to a stay. */ + const linkHotel = (tripId: number, accId: number, dayId: number | null, time: string | null) => + Number(testDb.prepare( + "INSERT INTO reservations (trip_id, day_id, title, type, accommodation_id, reservation_time) VALUES (?, ?, 'Hotel', 'hotel', ?, ?)" + ).run(tripId, dayId, accId, time).lastInsertRowid); + + it('DAY-SVC-040 — returns before touching a hotel booking when the trip has no stays', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id, { date: '2026-05-01' }); + const res = linkHotel(trip.id, 4242, null, null); + + svc.resyncAccommodationDays(trip.id, new Map([[day.id, '2026-04-01']])); + + // No stay owns this booking any more, so the restamp must not adopt it. + expect(testDb.prepare('SELECT day_id FROM reservations WHERE id = ?').get(res)).toMatchObject({ day_id: null }); + }); + + it('DAY-SVC-041 — leaves a stay glued to its rows when the snapshot has no date for them', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const d1 = createDay(testDb, trip.id, { date: '2026-05-01' }); + const d2 = createDay(testDb, trip.id, { date: '2026-05-02' }); + const place = createPlace(testDb, trip.id, { name: 'Hotel' }); + const stay = createDayAccommodation(testDb, trip.id, place.id, d1.id, d2.id); + const res = linkHotel(trip.id, stay.id, d2.id, '2026-04-30T14:00'); + + svc.resyncAccommodationDays(trip.id, new Map()); + + expect(testDb.prepare('SELECT start_day_id, end_day_id FROM day_accommodations WHERE id = ?').get(stay.id)) + .toMatchObject({ start_day_id: d1.id, end_day_id: d2.id }); + // The linked booking still gets pulled back onto the stay's start day: its + // reservation_time is a stale snapshot of that day's date, check-in time kept. + expect(testDb.prepare('SELECT day_id, reservation_time FROM reservations WHERE id = ?').get(res)) + .toMatchObject({ day_id: d1.id, reservation_time: '2026-05-01T14:00' }); + }); + + it('DAY-SVC-042 — does not rewrite a stay whose days already hold their old dates', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const d1 = createDay(testDb, trip.id, { date: '2026-05-01' }); + const d2 = createDay(testDb, trip.id, { date: '2026-05-02' }); + const place = createPlace(testDb, trip.id, { name: 'Hotel' }); + const stay = createDayAccommodation(testDb, trip.id, place.id, d1.id, d2.id); + const res = linkHotel(trip.id, stay.id, d1.id, null); + + svc.resyncAccommodationDays(trip.id, new Map([[d1.id, '2026-05-01'], [d2.id, '2026-05-02']])); + + expect(testDb.prepare('SELECT start_day_id, end_day_id FROM day_accommodations WHERE id = ?').get(stay.id)) + .toMatchObject({ start_day_id: d1.id, end_day_id: d2.id }); + // A booking with no time at all takes the bare date, not date + empty suffix. + expect(testDb.prepare('SELECT reservation_time FROM reservations WHERE id = ?').get(res)) + .toMatchObject({ reservation_time: '2026-05-01' }); + }); + + it('DAY-SVC-043 — refuses to re-anchor a stay onto an inverted day pair', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const d1 = createDay(testDb, trip.id, { date: '2026-05-01' }); + const d2 = createDay(testDb, trip.id, { date: '2026-05-02' }); + const place = createPlace(testDb, trip.id, { name: 'Hotel' }); + const stay = createDayAccommodation(testDb, trip.id, place.id, d1.id, d2.id); + + // The range change swapped the two dates across the rows, so the day now + // holding the old start date sits AFTER the one holding the old end date. + testDb.prepare('UPDATE days SET date = ? WHERE id = ?').run('2026-05-02', d1.id); + testDb.prepare('UPDATE days SET date = ? WHERE id = ?').run('2026-05-01', d2.id); + + svc.resyncAccommodationDays(trip.id, new Map([[d1.id, '2026-05-01'], [d2.id, '2026-05-02']])); + + expect(testDb.prepare('SELECT start_day_id, end_day_id FROM day_accommodations WHERE id = ?').get(stay.id)) + .toMatchObject({ start_day_id: d1.id, end_day_id: d2.id }); + }); + + it('DAY-SVC-044 — leaves a stay alone when one of its old dates is outside the new range', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const d1 = createDay(testDb, trip.id, { date: '2026-05-01' }); + const d2 = createDay(testDb, trip.id, { date: '2026-05-02' }); + const d3 = createDay(testDb, trip.id, { date: '2026-05-03' }); + const place = createPlace(testDb, trip.id, { name: 'Hotel' }); + // d2's old date (2026-04-29) dropped out of the range: the first stay finds + // no day for its end, the second finds none for its start. + const endGone = createDayAccommodation(testDb, trip.id, place.id, d1.id, d2.id); + const startGone = createDayAccommodation(testDb, trip.id, place.id, d2.id, d3.id); + + svc.resyncAccommodationDays( + trip.id, + new Map([[d1.id, '2026-05-01'], [d2.id, '2026-04-29'], [d3.id, '2026-05-03']]), + ); + + expect(testDb.prepare('SELECT start_day_id, end_day_id FROM day_accommodations WHERE id = ?').get(endGone.id)) + .toMatchObject({ start_day_id: d1.id, end_day_id: d2.id }); + expect(testDb.prepare('SELECT start_day_id, end_day_id FROM day_accommodations WHERE id = ?').get(startGone.id)) + .toMatchObject({ start_day_id: d2.id, end_day_id: d3.id }); + }); + + it('DAY-SVC-045 — skips the linked-booking restamp when the start day has no date', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id); + const place = createPlace(testDb, trip.id, { name: 'Hostel' }); + const stay = createDayAccommodation(testDb, trip.id, place.id, day.id, day.id); + const res = linkHotel(trip.id, stay.id, null, null); + + svc.resyncAccommodationDays(trip.id, new Map([[day.id, null]])); + + // On a dateless trip there is no date to stamp; writing one would invent a + // calendar the trip does not have. + expect(testDb.prepare('SELECT day_id, reservation_time FROM reservations WHERE id = ?').get(res)) + .toMatchObject({ day_id: null, reservation_time: null }); + }); +}); + +describe('reorder', () => { + const orderedDays = (tripId: number) => + testDb.prepare('SELECT id, day_number, date FROM days WHERE trip_id = ? ORDER BY day_number').all(tripId) as + { id: number; day_number: number; date: string | null }[]; + + it('DAY-SVC-046 — rejects a same-length list that contains a foreign day id', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const d1 = createDay(testDb, trip.id); + createDay(testDb, trip.id); + + // Same length as the trip's day list, so only the membership check can catch + // it — without it the renumber would leave one day orphaned at a negative + // day_number and drop another out of the itinerary. + expect(() => svc.reorder(trip.id, [d1.id, 999999])).toThrow(DayReorderError); + expect(orderedDays(trip.id).map(d => d.day_number)).toEqual([1, 2]); + }); + + it('DAY-SVC-047 — renumbers a dateless trip without inventing dates or touching bookings', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const d1 = createDay(testDb, trip.id); + const d2 = createDay(testDb, trip.id); + const d3 = createDay(testDb, trip.id); + const res = Number(testDb.prepare( + 'INSERT INTO reservations (trip_id, day_id, title, reservation_time) VALUES (?, ?, ?, ?)' + ).run(trip.id, d2.id, 'Dinner', '2026-02-02T19:00').lastInsertRowid); + + const result = svc.reorder(trip.id, [d3.id, d1.id, d2.id]); + + expect(result.days.map(d => d.id)).toEqual([d3.id, d1.id, d2.id]); + expect(orderedDays(trip.id).map(d => d.date)).toEqual([null, null, null]); + // A trip without dates has no slots to re-pin, so the re-stamp pass is + // skipped entirely — a booking keeps whatever time it was given by hand. + expect(testDb.prepare('SELECT reservation_time FROM reservations WHERE id = ?').get(res)) + .toMatchObject({ reservation_time: '2026-02-02T19:00' }); + }); + + it('DAY-SVC-048 — pins the known dates to the leading slots and nulls the slots beyond them', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const dated = createDay(testDb, trip.id, { date: '2026-03-01' }); + const dateless = createDay(testDb, trip.id); + + svc.reorder(trip.id, [dateless.id, dated.id]); + + // The slot beyond the known dates must resolve to null: better-sqlite3 + // refuses to bind the `undefined` a bare index lookup would hand it. + expect(orderedDays(trip.id)).toEqual([ + { id: dateless.id, day_number: 1, date: '2026-03-01' }, + { id: dated.id, day_number: 2, date: null }, + ]); + }); + + it('DAY-SVC-049 — allows a move that keeps a stay ordered and rolls back one that inverts it', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const d1 = createDay(testDb, trip.id, { date: '2026-03-01' }); + const d2 = createDay(testDb, trip.id, { date: '2026-03-02' }); + const d3 = createDay(testDb, trip.id, { date: '2026-03-03' }); + const place = createPlace(testDb, trip.id, { name: 'Hotel' }); + createDayAccommodation(testDb, trip.id, place.id, d1.id, d2.id); + + // Stretching the stay over a third slot is legal — the guard only rejects + // an end that lands before its start. + svc.reorder(trip.id, [d1.id, d3.id, d2.id]); + expect(orderedDays(trip.id).map(d => d.id)).toEqual([d1.id, d3.id, d2.id]); + + expect(() => svc.reorder(trip.id, [d2.id, d3.id, d1.id])).toThrow(DayReorderError); + // The guard runs inside the transaction, so the rejected move leaves the + // day numbers AND the re-pinned dates exactly as the legal move left them. + expect(orderedDays(trip.id)).toEqual([ + { id: d1.id, day_number: 1, date: '2026-03-01' }, + { id: d3.id, day_number: 2, date: '2026-03-02' }, + { id: d2.id, day_number: 3, date: '2026-03-03' }, + ]); + }); +}); + +describe('insert', () => { + it('DAY-SVC-050 — appends at the end when no position is given and shifts nothing', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const d1 = createDay(testDb, trip.id); + const d2 = createDay(testDb, trip.id); + + const created = svc.insert(trip.id); + + expect(created).toMatchObject({ day_number: 3, date: null, assignments: [], notes_items: [] }); + const rows = testDb.prepare('SELECT id, day_number FROM days WHERE trip_id = ? ORDER BY day_number').all(trip.id) as + { id: number; day_number: number }[]; + expect(rows).toEqual([ + { id: d1.id, day_number: 1 }, + { id: d2.id, day_number: 2 }, + { id: created.id, day_number: 3 }, + ]); + }); +}); + +// ── day shaping ─────────────────────────────────────────────────────────────── + +describe('setDefaultTransportMode', () => { + it('DAY-SVC-051 — sets and clears the whole-day mode without disturbing notes or title', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id, { title: 'Arrival' }); + svc.update(day.id, day as never, { notes: 'Keep me' }); + + const set = svc.setDefaultTransportMode(day.id, 'walk'); + expect(set).toMatchObject({ default_transport_mode: 'walk', title: 'Arrival', notes: 'Keep me' }); + + // Its own endpoint exists precisely so clearing the mode cannot wipe the + // day's text the way a general update would. + const cleared = svc.setDefaultTransportMode(day.id, null); + expect(cleared).toMatchObject({ default_transport_mode: null, title: 'Arrival', notes: 'Keep me' }); + }); +}); + +describe('day shaping', () => { + it('DAY-SVC-052 — getAssignmentsForDay reports a null category for an uncategorised place', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id); + const placeId = Number(testDb.prepare('INSERT INTO places (trip_id, name, category_id) VALUES (?, ?, NULL)') + .run(trip.id, 'Uncategorised').lastInsertRowid); + createDayAssignment(testDb, day.id, placeId); + + // The LEFT JOIN yields category_name/color/icon as NULL, which must collapse + // to a null category instead of an object of nulls the client would render. + expect(svc.getAssignmentsForDay(day.id)[0].place.category).toBeNull(); + }); + + it('DAY-SVC-053 — list groups assignments, tags, participants and notes onto their own day', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const busy = createDay(testDb, trip.id); + const empty = createDay(testDb, trip.id); + const tagged = createPlace(testDb, trip.id, { name: 'Louvre' }); + const plain = createPlace(testDb, trip.id, { name: 'Pont Neuf' }); + const shared = createDayAssignment(testDb, busy.id, tagged.id, { order_index: 0 }); + createDayAssignment(testDb, busy.id, plain.id, { order_index: 1 }); + const tag = createTag(testDb, user.id, { name: 'Museum' }); + testDb.prepare('INSERT INTO place_tags (place_id, tag_id) VALUES (?, ?)').run(tagged.id, tag.id); + testDb.prepare('INSERT INTO assignment_participants (assignment_id, user_id) VALUES (?, ?)').run(shared.id, user.id); + createDayNote(testDb, busy.id, trip.id, { text: 'Breakfast', sort_order: 1 }); + createDayNote(testDb, busy.id, trip.id, { text: 'Dinner', sort_order: 2 }); + + const { days } = svc.list(trip.id); + + expect(days[0].assignments.map(a => a.place.name)).toEqual(['Louvre', 'Pont Neuf']); + expect(days[0].assignments[0].place.tags.map(t => t.name)).toEqual(['Museum']); + expect(days[0].assignments[0].participants.map(p => p.user_id)).toEqual([user.id]); + // The batch loaders return nothing for the untagged place and the assignment + // nobody joined; both have to fall back to [] rather than undefined. + expect(days[0].assignments[1].place.tags).toEqual([]); + expect(days[0].assignments[1].participants).toEqual([]); + expect(days[0].notes_items.map(n => n.text)).toEqual(['Breakfast', 'Dinner']); + // A day nobody planned anything on still ships both collections. + expect(days[1]).toMatchObject({ id: empty.id, assignments: [], notes_items: [] }); + }); + + it('DAY-SVC-054 — create keeps an explicit date and notes, update clears a title sent as null', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + + const day = svc.create(trip.id, '2026-09-14', 'Ferry to the island'); + expect(day).toMatchObject({ date: '2026-09-14', notes: 'Ferry to the island' }); + + // The MCP update_day tool clears a title by sending null, which must reach + // the column instead of being treated as "key absent, keep the old title". + const titled = svc.update(day.id, day as never, { title: 'Crossing' }); + expect(svc.update(day.id, titled as never, { title: null })).toMatchObject({ title: null, notes: 'Ferry to the island' }); + }); +}); + /** * canEdit is MCP-only now: the HTTP path checks the same right through * TripAccessGuard's @RequirePermission('day_edit'), so nothing in a controller test diff --git a/server/tests/unit/nest/feeds.service.test.ts b/server/tests/unit/nest/feeds.service.test.ts new file mode 100644 index 000000000..64d6c9a07 --- /dev/null +++ b/server/tests/unit/nest/feeds.service.test.ts @@ -0,0 +1,393 @@ +/** + * Unit tests for FeedsService — FEED-SVC-001 through FEED-SVC-021. The domain had + * no unit suite at all: everything rested on tests/e2e/feeds.e2e.test.ts, which + * drives the HTTP surface and therefore only ever merges a single trip's calendar. + * These cases pin what the e2e cannot reach — the merge rules buildUserIcs applies + * across several trips (TZID dedupe, a failing trip being skipped, header vs body + * folding) and the second-call branches of the token lifecycle. + * + * buildUserIcs/buildTripIcs consume CalendarService.buildTripCalendar's PARTS now + * instead of scanning a finished ICS document back apart, so the calendar is a stub + * here and the parts are the test's own. The token lookups are real SQL against an + * in-memory SQLite DB (same harness as calendar.service.test.ts). + */ +import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest'; + +// ── DB setup ────────────────────────────────────────────────────────────────── + +const { testDb, dbMock } = vi.hoisted(() => { + const Database = require('better-sqlite3'); + const db = new Database(':memory:'); + db.exec('PRAGMA journal_mode = WAL'); + db.exec('PRAGMA foreign_keys = ON'); + db.exec('PRAGMA busy_timeout = 5000'); + const mock = { + db, + closeDb: () => {}, + reinitialize: () => {}, + getPlaceWithTags: () => null, + canAccessTrip: (tripId: number | string, userId: number) => + db.prepare(` + SELECT t.id, t.user_id FROM trips t + LEFT JOIN trip_members m ON m.trip_id = t.id AND m.user_id = ? + WHERE t.id = ? AND (t.user_id = ? OR m.user_id IS NOT NULL) + `).get(userId, tripId, userId), + isOwner: (tripId: number | string, userId: number) => + !!db.prepare('SELECT id FROM trips WHERE id = ? AND user_id = ?').get(tripId, userId), + }; + return { testDb: db, dbMock: mock }; +}); + +vi.mock('../../../src/db/database', () => dbMock); +vi.mock('../../../src/config', () => ({ + JWT_SECRET: 'test-secret', + ENCRYPTION_KEY: 'a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2', + updateJwtSecret: () => {}, +})); +vi.mock('../../../src/websocket', () => ({ broadcast: vi.fn() })); + +import { createTables } from '../../../src/db/schema'; +import { runMigrations } from '../../../src/db/migrations'; +import { resetTestDb } from '../../helpers/test-db'; +import { createUser, createTrip, addTripMember } from '../../helpers/factories'; +import { DatabaseService } from '../../../src/nest/database/database.service'; +import { FeedsService } from '../../../src/nest/feeds/feeds.service'; +import { FeedsModule } from '../../../src/nest/feeds/feeds.module'; +import { + FeedsPublicController, + TripFeedTokenController, + UserFeedTokenController, +} from '../../../src/nest/feeds/feeds.controller'; +import type { CalendarService, TripCalendar } from '../../../src/nest/calendar/calendar.service'; +import { expectRegisteredProvider, expectRegisteredController } from '../../helpers/module-providers'; + +const BASE = 'https://trek.example.test'; + +const buildTripCalendar = vi.fn(); +const svc = new FeedsService( + new DatabaseService(testDb), + { buildTripCalendar } as unknown as CalendarService, +); + +// ── Calendar parts the stub hands back ──────────────────────────────────────── + +const vevent = (summary: string) => + `BEGIN:VEVENT\r\nUID:trek-trip-${summary}@trek\r\nDTSTAMP:20260101T000000Z\r\n` + + `DTSTART;VALUE=DATE:20260101\r\nDTEND;VALUE=DATE:20260102\r\nSUMMARY:${summary}\r\nEND:VEVENT\r\n`; + +const vtimezone = (tzid: string, tzname = tzid) => + `BEGIN:VTIMEZONE\r\nTZID:${tzid}\r\nBEGIN:STANDARD\r\nDTSTART:19700101T000000\r\n` + + `TZOFFSETFROM:+0900\r\nTZOFFSETTO:+0900\r\nTZNAME:${tzname}\r\nEND:STANDARD\r\nEND:VTIMEZONE\r\n`; + +const calendarParts = (overrides: Partial = {}): TripCalendar => ({ + calName: 'Sample', + filename: 'sample.ics', + timezones: new Map(), + events: [vevent('Sample')], + ...overrides, +}); + +beforeAll(() => { + createTables(testDb); + runMigrations(testDb); +}); + +beforeEach(() => { + resetTestDb(testDb); + buildTripCalendar.mockReset(); + buildTripCalendar.mockImplementation(() => calendarParts()); +}); + +afterAll(() => { + testDb.close(); +}); + +function seedTrip(token?: string) { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + if (token) testDb.prepare('UPDATE trips SET feed_token = ? WHERE id = ?').run(token, trip.id); + return { user, trip, tripId: String(trip.id) }; +} + +function seedUserWithToken(token: string, overrides: Partial<{ username: string }> = {}) { + const { user } = createUser(testDb, overrides); + testDb.prepare('UPDATE users SET feed_token = ? WHERE id = ?').run(token, user.id); + return user; +} + +// ── Trip feed token ─────────────────────────────────────────────────────────── + +describe('trip feed token lifecycle', () => { + it('FEED-SVC-001: reports no URL while the trip has no token', () => { + const { user, tripId } = seedTrip(); + + expect(svc.getTripToken(tripId, user.id, BASE)).toEqual({ feed_url: null }); + }); + + it('FEED-SVC-002: reports the absolute feed URL once a token exists', () => { + const { user, tripId } = seedTrip('tok-trip'); + + expect(svc.getTripToken(tripId, user.id, BASE)).toEqual({ + feed_url: `${BASE}/api/feed/trip/tok-trip.ics`, + }); + }); + + it('FEED-SVC-003: a trailing slash on the base is stripped, never doubled into //api', () => { + // APP_URL is user-supplied config; pasted with a trailing slash it would + // otherwise produce https://host//api/feed/... which some clients reject. + const { user, tripId } = seedTrip('tok-trip'); + + expect(svc.getTripToken(tripId, user.id, `${BASE}/`).feed_url).toBe( + `${BASE}/api/feed/trip/tok-trip.ics`, + ); + }); + + it('FEED-SVC-004: a user without access gets null, not the token of a foreign trip', () => { + // The token is the credential for the public feed, so leaking it through the + // authenticated GET would hand a stranger the whole trip. + const { tripId } = seedTrip('tok-trip'); + const { user: outsider } = createUser(testDb); + + expect(svc.getTripToken(tripId, outsider.id, BASE)).toEqual({ feed_url: null }); + }); + + it('FEED-SVC-005: a trip shared with the user as a member resolves too', () => { + const { trip, tripId } = seedTrip('tok-trip'); + const { user: member } = createUser(testDb); + addTripMember(testDb, trip.id, member.id); + + expect(svc.getTripToken(tripId, member.id, BASE).feed_url).toBe( + `${BASE}/api/feed/trip/tok-trip.ics`, + ); + }); + + it('FEED-SVC-006: generate mints a token once and stays idempotent', () => { + // Enabling twice must not invalidate a URL the user already handed to their + // calendar client — that is what rotate is for. + const { user, tripId } = seedTrip(); + + const first = svc.generateTripToken(tripId, user.id, BASE); + const second = svc.generateTripToken(tripId, user.id, BASE); + + expect(first.feed_url).toMatch(new RegExp(`^${BASE}/api/feed/trip/[0-9a-f-]+\\.ics$`)); + expect(second.feed_url).toBe(first.feed_url); + }); + + it('FEED-SVC-007: rotate issues a fresh token and the previous URL stops resolving', () => { + const { user, tripId } = seedTrip(); + const before = svc.generateTripToken(tripId, user.id, BASE).feed_url; + const oldToken = before.match(/trip\/([0-9a-f-]+)\.ics$/)![1]; + + const after = svc.rotateTripToken(tripId, BASE).feed_url; + + expect(after).not.toBe(before); + expect(svc.buildTripIcs(oldToken)).toBeNull(); + }); + + it('FEED-SVC-008: disable clears the column so the public URL dies', () => { + const { user, tripId } = seedTrip(); + const url = svc.generateTripToken(tripId, user.id, BASE).feed_url; + const token = url.match(/trip\/([0-9a-f-]+)\.ics$/)![1]; + + svc.disableTripToken(tripId); + + expect(svc.getTripToken(tripId, user.id, BASE)).toEqual({ feed_url: null }); + expect(svc.buildTripIcs(token)).toBeNull(); + }); +}); + +// ── User (all-trips) feed token ─────────────────────────────────────────────── + +describe('user feed token lifecycle', () => { + it('FEED-SVC-009: reports null before and the absolute URL after generation', () => { + const { user } = createUser(testDb); + + expect(svc.getUserToken(user.id, BASE)).toEqual({ feed_url: null }); + + const generated = svc.generateUserToken(user.id, BASE).feed_url; + + expect(generated).toMatch(new RegExp(`^${BASE}/api/feed/user/[0-9a-f-]+\\.ics$`)); + expect(svc.getUserToken(user.id, BASE).feed_url).toBe(generated); + }); + + it('FEED-SVC-010: generate is idempotent — the existing URL is returned unchanged', () => { + const { user } = createUser(testDb); + + const first = svc.generateUserToken(user.id, BASE); + const second = svc.generateUserToken(user.id, BASE); + + expect(second.feed_url).toBe(first.feed_url); + }); + + it('FEED-SVC-011: rotate issues a fresh token and the previous URL stops resolving', () => { + const { user } = createUser(testDb); + const before = svc.generateUserToken(user.id, BASE).feed_url; + const oldToken = before.match(/user\/([0-9a-f-]+)\.ics$/)![1]; + + const after = svc.rotateUserToken(user.id, BASE).feed_url; + + expect(after).not.toBe(before); + expect(svc.buildUserIcs(oldToken)).toBeNull(); + }); + + it('FEED-SVC-012: disable clears the column so the public URL dies', () => { + const { user } = createUser(testDb); + const url = svc.generateUserToken(user.id, BASE).feed_url; + const token = url.match(/user\/([0-9a-f-]+)\.ics$/)![1]; + + svc.disableUserToken(user.id); + + expect(svc.getUserToken(user.id, BASE)).toEqual({ feed_url: null }); + expect(svc.buildUserIcs(token)).toBeNull(); + }); +}); + +// ── buildTripIcs ────────────────────────────────────────────────────────────── + +describe('buildTripIcs', () => { + it('FEED-SVC-013: an unknown token yields null without asking the calendar', () => { + seedTrip('tok-trip'); + + expect(svc.buildTripIcs('00000000-0000-0000-0000-000000000000')).toBeNull(); + expect(buildTripCalendar).not.toHaveBeenCalled(); + }); + + it('FEED-SVC-014: a calendar that throws yields null instead of propagating', () => { + // The public feed is unauthenticated: a trip the calendar cannot render (a row + // deleted mid-request, unparseable data) has to come back as a 404, not a 500 + // that a subscribing client retries hourly forever. + seedTrip('tok-trip'); + buildTripCalendar.mockImplementation(() => { + throw new Error('calendar exploded'); + }); + + expect(svc.buildTripIcs('tok-trip')).toBeNull(); + }); + + it('FEED-SVC-015: the refresh hints sit in the preamble, ahead of X-WR-CALNAME and every component', () => { + // REFRESH-INTERVAL/X-PUBLISHED-TTL are calendar properties: RFC 5545 puts them + // before the first component, and clients that scan only the preamble stop + // re-fetching if they slip behind a VTIMEZONE. The document is concatenated from + // the calendar's parts now, so the order is an assembly decision, not a given. + const { trip } = seedTrip('tok-trip'); + buildTripCalendar.mockImplementation(() => + calendarParts({ + calName: 'Golden Trip', + filename: 'golden-trip.ics', + timezones: new Map([['Asia/Tokyo', vtimezone('Asia/Tokyo')]]), + }), + ); + + const result = svc.buildTripIcs('tok-trip'); + + expect(result).not.toBeNull(); + expect(result!.filename).toBe('golden-trip.ics'); + expect(result!.ics).toContain( + 'METHOD:PUBLISH\r\nREFRESH-INTERVAL;VALUE=DURATION:PT1H\r\nX-PUBLISHED-TTL:PT1H\r\n' + + 'X-WR-CALNAME:Golden Trip\r\nBEGIN:VTIMEZONE\r\n', + ); + expect(result!.ics.startsWith('BEGIN:VCALENDAR\r\nVERSION:2.0\r\n')).toBe(true); + expect(result!.ics.endsWith('END:VEVENT\r\nEND:VCALENDAR\r\n')).toBe(true); + expect(buildTripCalendar).toHaveBeenCalledWith(trip.id); + }); +}); + +// ── buildUserIcs ────────────────────────────────────────────────────────────── + +describe('buildUserIcs', () => { + it('FEED-SVC-016: an unknown token yields null without asking the calendar', () => { + seedUserWithToken('tok-user'); + + expect(svc.buildUserIcs('00000000-0000-0000-0000-000000000000')).toBeNull(); + expect(buildTripCalendar).not.toHaveBeenCalled(); + }); + + it('FEED-SVC-017: a trip whose calendar throws is skipped, the rest are still emitted', () => { + // One unrenderable trip must not take the whole all-trips subscription down — + // the user would silently lose every calendar entry because of a single bad row. + const user = seedUserWithToken('tok-user'); + const good = createTrip(testDb, user.id, { start_date: '2026-01-01' }); + const broken = createTrip(testDb, user.id, { start_date: '2026-02-01' }); + const alsoGood = createTrip(testDb, user.id, { start_date: '2026-03-01' }); + buildTripCalendar.mockImplementation((id: number) => { + if (id === broken.id) throw new Error('calendar exploded'); + return calendarParts({ events: [vevent(`Trip${id}`)] }); + }); + + const result = svc.buildUserIcs('tok-user'); + + expect(buildTripCalendar).toHaveBeenCalledTimes(3); + expect(result!.ics).toContain(`SUMMARY:Trip${good.id}\r\n`); + expect(result!.ics).toContain(`SUMMARY:Trip${alsoGood.id}\r\n`); + expect(result!.ics).not.toContain(`SUMMARY:Trip${broken.id}\r\n`); + expect(result!.ics.endsWith('END:VCALENDAR\r\n')).toBe(true); + }); + + it('FEED-SVC-018: a TZID shared by two trips is defined exactly once, ahead of every VEVENT', () => { + // Two VTIMEZONE blocks with the same TZID make the document invalid and clients + // drop the events referencing it; a block emitted after the VEVENT that uses it + // does not resolve either (#1453). First definition wins. + const user = seedUserWithToken('tok-user'); + const first = createTrip(testDb, user.id, { start_date: '2026-01-01' }); + createTrip(testDb, user.id, { start_date: '2026-02-01' }); + buildTripCalendar.mockImplementation((id: number) => ({ + calName: `Trip ${id}`, + filename: `trip-${id}.ics`, + timezones: new Map([ + ['Asia/Tokyo', vtimezone('Asia/Tokyo', id === first.id ? 'Asia/Tokyo' : 'Second/Definition')], + ]), + events: [vevent(`Trip${id}`)], + })); + + const { ics } = svc.buildUserIcs('tok-user')!; + + expect(ics.split('BEGIN:VTIMEZONE').length - 1).toBe(1); + expect(ics).toContain('TZNAME:Asia/Tokyo\r\n'); + expect(ics).not.toContain('Second/Definition'); + expect(ics.indexOf('BEGIN:VTIMEZONE')).toBeLessThan(ics.indexOf('BEGIN:VEVENT')); + }); + + it('FEED-SVC-019: the header is never folded, the body always is', () => { + // Folding is applied to the body only, on purpose: a long display name would + // otherwise wrap X-WR-CALNAME across two physical lines, which several clients + // render as a truncated calendar title. The body still has to fold — RFC 5545 + // caps a content line at 75 octets. + const username = 'Ferdinand-Bartholomew-'.repeat(5); + const user = seedUserWithToken('tok-user', { username }); + createTrip(testDb, user.id, { start_date: '2026-01-01' }); + const longSummary = 'A'.repeat(120); + buildTripCalendar.mockImplementation(() => calendarParts({ events: [vevent(longSummary)] })); + + const { ics, calName } = svc.buildUserIcs('tok-user')!; + + expect(calName).toBe(`${username} – All Trips`); + const preamble = ics.slice(0, ics.indexOf('BEGIN:VEVENT')); + expect(preamble).toContain(`X-WR-CALNAME:${calName}\r\n`); + expect(preamble.split('\r\n').filter((line) => line.startsWith(' '))).toEqual([]); + // The body is folded and unfolds back to the original content line. + expect(ics).toContain('\r\n '); + expect(ics.replace(/\r\n /g, '')).toContain(`SUMMARY:${longSummary}\r\n`); + }); + + it('FEED-SVC-020: the display name is escaped for the header but returned raw', () => { + // An unescaped ; or , ends the property value early, so the calendar shows up + // under a truncated name. The returned calName feeds the HTTP layer, not ICS, + // and must stay verbatim. + const user = seedUserWithToken('tok-user', { username: 'Alice; Bob, Co\\Ltd' }); + createTrip(testDb, user.id, { start_date: '2026-01-01' }); + + const { ics, calName } = svc.buildUserIcs('tok-user')!; + + expect(calName).toBe('Alice; Bob, Co\\Ltd – All Trips'); + expect(ics).toContain('X-WR-CALNAME:Alice\\; Bob\\, Co\\\\Ltd – All Trips\r\n'); + }); +}); + +describe('FeedsService wiring', () => { + it('FEED-SVC-021: the module registers the service and all three controllers', () => { + expectRegisteredProvider(FeedsModule, FeedsService); + expectRegisteredController(FeedsModule, FeedsPublicController); + expectRegisteredController(FeedsModule, TripFeedTokenController); + expectRegisteredController(FeedsModule, UserFeedTokenController); + }); +}); diff --git a/server/tests/unit/nest/trip-members.service.test.ts b/server/tests/unit/nest/trip-members.service.test.ts new file mode 100644 index 000000000..f0fbee744 --- /dev/null +++ b/server/tests/unit/nest/trip-members.service.test.ts @@ -0,0 +1,363 @@ +/** + * Unit tests for TripMembersService — MEMBERS-SVC-001 through MEMBERS-SVC-015. + * + * The roster cases that came over with the split still run in + * tests/unit/nest/trips.service.test.ts (TRIP-SVC-020…023, 030…034, 049, 052, + * 053) against this same service; they were left there so the diff shows the + * move rather than a rewrite. This file adds only what that set never reached: + * the five helpers the controller talks through, and the fallbacks each + * mutation takes when a row is missing or a name is unusable. Same in-memory + * SQLite harness, so the SQL is exercised for real. + */ +import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest'; + +// ── DB setup ────────────────────────────────────────────────────────────────── + +const { testDb, dbMock, broadcast, notifySend } = vi.hoisted(() => { + const Database = require('better-sqlite3'); + const db = new Database(':memory:'); + db.exec('PRAGMA journal_mode = WAL'); + db.exec('PRAGMA foreign_keys = ON'); + db.exec('PRAGMA busy_timeout = 5000'); + const mock = { + db, + closeDb: () => {}, + reinitialize: () => {}, + getPlaceWithTags: () => null, + canAccessTrip: (tripId: any, userId: number) => + db.prepare(` + SELECT t.id, t.user_id FROM trips t + LEFT JOIN trip_members m ON m.trip_id = t.id AND m.user_id = ? + WHERE t.id = ? AND (t.user_id = ? OR m.user_id IS NOT NULL) + `).get(userId, tripId, userId), + isOwner: (tripId: any, userId: number) => + !!db.prepare('SELECT id FROM trips WHERE id = ? AND user_id = ?').get(tripId, userId), + }; + return { + testDb: db, + dbMock: mock, + broadcast: vi.fn(), + notifySend: vi.fn().mockResolvedValue(undefined), + }; +}); + +vi.mock('../../../src/db/database', () => dbMock); +vi.mock('../../../src/config', () => ({ + JWT_SECRET: 'test-secret', + ENCRYPTION_KEY: 'a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2', + updateJwtSecret: () => {}, +})); +vi.mock('../../../src/websocket', () => ({ broadcast })); +// notifyInvite reaches the bridge through a dynamic import — keep the send in scope +// but out of the transports. +vi.mock('../../../src/nest/notifications/notifications.bridge', () => ({ send: notifySend })); + +import { createTables } from '../../../src/db/schema'; +import { runMigrations } from '../../../src/db/migrations'; +import { resetTestDb } from '../../helpers/test-db'; +import { createUser, createTrip, addTripMember } from '../../helpers/factories'; +import { DatabaseService } from '../../../src/nest/database/database.service'; +import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; +import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; +import { BudgetService } from '../../../src/nest/budget/budget.service'; +import { ExchangeRatesService } from '../../../src/nest/budget/exchange-rates.service'; +import { UserCleanupService } from '../../../src/nest/auth/user-cleanup.service'; +import { TripMembersService } from '../../../src/nest/trip-members/trip-members.service'; +import { NotFoundError, ValidationError } from '../../../src/nest/common/domain-errors'; +import type { User } from '../../../src/types'; + +const dbs = () => new DatabaseService(testDb); +const budgetSvc = new BudgetService(dbs(), new PermissionsService(dbs()), new ExchangeRatesService(), new RealtimeService()); +const roster = new TripMembersService(dbs(), budgetSvc, new UserCleanupService(dbs()), new PermissionsService(dbs()), new RealtimeService()); + +/** + * A roster whose connection reports "no such row" for the first result of every + * statement containing `match`. Both fallbacks it serves sit behind a FOREIGN + * KEY, so the missing row cannot be produced through the real schema — freezing + * the read is the only way to keep those branches honest. Same Proxy shape as + * the failingConnection helper in trips.service.test.ts. + */ +function rosterWithMissingRow(match: string) { + const conn = new Proxy(testDb, { + get(target, prop) { + if (prop === 'prepare') { + return (sql: string) => { + const stmt = target.prepare(sql); + if (!sql.includes(match)) return stmt; + return new Proxy(stmt, { + get(s, p) { + if (p === 'get') return () => undefined; + const v = (s as any)[p]; + return typeof v === 'function' ? v.bind(s) : v; + }, + }); + }; + } + const v = (target as any)[prop]; + return typeof v === 'function' ? v.bind(target) : v; + }, + }); + const frozen = { connection: conn, canAccessTrip: dbMock.canAccessTrip, isOwner: dbMock.isOwner } as unknown as DatabaseService; + return new TripMembersService(frozen, budgetSvc, new UserCleanupService(dbs()), new PermissionsService(dbs()), new RealtimeService()); +} + +beforeAll(() => { + createTables(testDb); + runMigrations(testDb); +}); + +beforeEach(() => { + resetTestDb(testDb); + broadcast.mockClear(); + notifySend.mockClear(); +}); + +afterAll(() => { + testDb.close(); +}); + +// ── Delegating helpers (what the controller calls around the mutations) ─────── + +describe('TripMembersService delegation', () => { + it('MEMBERS-SVC-001: canAccessTrip forwards the db helper — owner and member get the access row, a stranger gets nothing', () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const { user: stranger } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, member.id); + + // Every member route answers 404 on a falsy result and reads access.user_id as + // the trip owner, so a delegation that stopped forwarding would lock the whole + // roster out rather than fail loudly. + expect(roster.canAccessTrip(String(trip.id), owner.id)).toMatchObject({ user_id: owner.id }); + expect(roster.canAccessTrip(trip.id, member.id)).toMatchObject({ user_id: owner.id }); + expect(roster.canAccessTrip(trip.id, stranger.id)).toBeUndefined(); + }); + + it('MEMBERS-SVC-002: can() resolves member_manage at its trip_owner default and lets admins through', () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + + // Inviting and evicting both hang off this single call, so a member must not + // pass it while the action sits at its default level. + expect(roster.can('member_manage', 'user', owner.id, owner.id, false)).toBe(true); + expect(roster.can('member_manage', 'user', owner.id, member.id, true)).toBe(false); + expect(roster.can('member_manage', 'admin', owner.id, member.id, true)).toBe(true); + }); + + it('MEMBERS-SVC-003: broadcast forwards the socket id so the originating client is not echoed', () => { + roster.broadcast('9', 'trip:updated', { trip: { id: 9 } } as never, 'sock-1'); + expect(broadcast).toHaveBeenCalledWith('9', 'trip:updated', { trip: { id: 9 } }, 'sock-1'); + + // A request without X-Socket-Id must still reach everybody — dropping the + // argument here would exclude an arbitrary socket instead of none. + roster.broadcast('9', 'trip:updated', { trip: { id: 9 } } as never, undefined); + expect(broadcast).toHaveBeenLastCalledWith('9', 'trip:updated', { trip: { id: 9 } }, undefined); + }); + + it('MEMBERS-SVC-004: getTripForViewer re-reads the trip in list shape, with is_owner per viewer', () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const trip = createTrip(testDb, owner.id, { title: 'Handover' }); + addTripMember(testDb, trip.id, member.id); + + // The handover broadcast hands the raw :id route param straight in, so the + // named-parameter query has to keep matching a string id against the INTEGER + // column — and the payload the clients re-read must carry their own is_owner. + const asOwner = roster.getTripForViewer(String(trip.id), owner.id) as Record; + expect(asOwner).toMatchObject({ id: trip.id, title: 'Handover', is_owner: 1, owner_username: owner.username, shared_count: 1 }); + const asMember = roster.getTripForViewer(trip.id, member.id) as Record; + expect(asMember.is_owner).toBe(0); + expect(roster.getTripForViewer(999999, owner.id)).toBeUndefined(); + }); + + it('MEMBERS-SVC-005: notifyInvite sends a user-scoped trip_invite carrying the actor, invitee and trip', async () => { + const actor = { id: 4, email: 'actor@example.test' } as User; + roster.notifyInvite('9', actor, 12, 'Roadtrip', 'invitee@example.test'); + + // The bridge is imported lazily inside the call, so the send lands a microtask later. + await vi.waitFor(() => expect(notifySend).toHaveBeenCalledTimes(1)); + expect(notifySend).toHaveBeenCalledWith({ + event: 'trip_invite', + actorId: 4, + scope: 'user', + targetId: 12, + params: { trip: 'Roadtrip', actor: 'actor@example.test', invitee: 'invitee@example.test', tripId: '9' }, + }); + }); + + it('MEMBERS-SVC-006: a rejected notification never reaches the invite request', async () => { + notifySend.mockRejectedValueOnce(new Error('bridge down')); + const actor = { id: 4, email: 'actor@example.test' } as User; + + // The membership row is already committed when this fires — an unhandled + // rejection would turn a successful invite into a 500 (and crash the process + // on an unhandled promise rejection). + expect(() => roster.notifyInvite('9', actor, 12, 'Roadtrip', 'invitee@example.test')).not.toThrow(); + await vi.waitFor(() => expect(notifySend).toHaveBeenCalledTimes(1)); + }); +}); + +// ── addMember fallbacks ────────────────────────────────────────────────────── + +describe('addMember fallbacks', () => { + it("MEMBERS-SVC-007: addMember still reports a title when the trip row cannot be read ('Untitled')", () => { + const { user: owner } = createUser(testDb); + const { user: invitee } = createUser(testDb); + const trip = createTrip(testDb, owner.id, { title: 'Readable' }); + + // The title only feeds the invite notification, and it is read after the + // membership is inserted — losing that row must not cost the invitee their + // access or throw on an undefined title. + const broken = rosterWithMissingRow('SELECT title FROM trips WHERE id = ?'); + const result = broken.addMember(trip.id, invitee.email, owner.id, owner.id); + expect(result.tripTitle).toBe('Untitled'); + expect(testDb.prepare('SELECT id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, invitee.id)).toBeDefined(); + }); + + it('MEMBERS-SVC-008: addMember resolves a padded identifier and matches on username as well as email', () => { + const { user: owner } = createUser(testDb); + const { user: invitee } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + + // Pasted invites arrive with surrounding whitespace; without the trim the + // lookup misses and the box answers 'User not found' for a real account. + expect(roster.addMember(trip.id, ` ${invitee.username} `, owner.id, owner.id).member.id).toBe(invitee.id); + }); +}); + +// ── transferOwnership guard rails (#973) ───────────────────────────────────── + +describe('transferOwnership guard rails', () => { + it('MEMBERS-SVC-009: rejects a trip that no longer exists, before any other check', () => { + const { user: owner } = createUser(testDb); + + // NotFoundError, not ValidationError: the controller maps the two to 404 and + // 400. Passing the owner as the new owner too proves the trip lookup runs + // first — otherwise this would surface as 'You already own this trip'. + expect(() => roster.transferOwnership(999999, owner.id, owner.id)).toThrow(NotFoundError); + expect(() => roster.transferOwnership(999999, owner.id, owner.id)).toThrow('Trip not found'); + }); + + it('MEMBERS-SVC-010: rejects an id with no user row before it checks membership', () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + + // A stale id from the client must read as a 404 'User not found' rather than + // the 400 the non-member branch below it would produce, and the trip must keep + // its owner either way. + expect(() => roster.transferOwnership(trip.id, 999999, owner.id)).toThrow(NotFoundError); + expect(() => roster.transferOwnership(trip.id, 999999, owner.id)).toThrow('User not found'); + expect((testDb.prepare('SELECT user_id FROM trips WHERE id = ?').get(trip.id) as { user_id: number }).user_id).toBe(owner.id); + }); + + it('MEMBERS-SVC-011: completes with an empty fromEmail when the former owner cannot be read', () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, member.id); + + // fromEmail is audit detail only; an unreadable row must not abort the + // handover halfway, which would leave the owner pointer and the member rows + // disagreeing about who owns the trip. + const broken = rosterWithMissingRow('SELECT email FROM users WHERE id = ?'); + const result = broken.transferOwnership(trip.id, member.id, owner.id); + expect(result.fromEmail).toBe(''); + expect(result.toEmail).toBe(member.email); + expect((testDb.prepare('SELECT user_id FROM trips WHERE id = ?').get(trip.id) as { user_id: number }).user_id).toBe(member.id); + }); +}); + +// ── Guest name validation (#1362) ──────────────────────────────────────────── + +describe('guest name validation', () => { + it('MEMBERS-SVC-012: createGuest rejects an absent, blank or over-long name and writes nothing', () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + + expect(() => roster.createGuest(trip.id, undefined as never, owner.id)).toThrow(ValidationError); + expect(() => roster.createGuest(trip.id, ' ', owner.id)).toThrow('Guest name is required'); + expect(() => roster.createGuest(trip.id, 'x'.repeat(51), owner.id)).toThrow('Guest name must be 50 characters or fewer'); + + // The guards run ahead of the transaction, so a rejected name can never leave + // a credential-less users row behind with no trip to belong to. + expect((testDb.prepare('SELECT COUNT(*) AS n FROM users WHERE is_guest = 1').get() as { n: number }).n).toBe(0); + + // 50 is the accepted boundary the DTO shares — off by one here and the API + // starts refusing names the client believes are valid. + expect(roster.createGuest(trip.id, 'x'.repeat(50), owner.id).member.username).toHaveLength(50); + }); + + it('MEMBERS-SVC-013: renameGuest rejects an absent, blank or over-long name — before the trip-scope check', () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + const { member: guest } = roster.createGuest(trip.id, 'Ida', owner.id); + + expect(() => roster.renameGuest(trip.id, guest.id, undefined as never)).toThrow(ValidationError); + expect(() => roster.renameGuest(trip.id, guest.id, ' ')).toThrow('Guest name is required'); + expect(() => roster.renameGuest(trip.id, guest.id, 'x'.repeat(51))).toThrow('Guest name must be 50 characters or fewer'); + + // Order matters for the status code: an unusable name throws (400) even for an + // id that is not a guest of this trip, where the scope check returns false (404). + expect(() => roster.renameGuest(trip.id, owner.id, '')).toThrow('Guest name is required'); + expect((testDb.prepare('SELECT display_name FROM users WHERE id = ?').get(guest.id) as { display_name: string }).display_name).toBe('Ida'); + + // A padded name is stored trimmed, so the roster does not render the spaces. + expect(roster.renameGuest(trip.id, guest.id, ' Ida M. ')).toBe(true); + expect((testDb.prepare('SELECT display_name FROM users WHERE id = ?').get(guest.id) as { display_name: string }).display_name).toBe('Ida M.'); + }); + + it("MEMBERS-SVC-014: deleteGuest is trip-scoped — another trip's owner cannot erase this trip's guest", () => { + const { user: owner } = createUser(testDb); + const { user: other } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + const otherTrip = createTrip(testDb, other.id); + const { member: guest } = roster.createGuest(trip.id, 'Jo', owner.id); + + // The route only proves ownership of the trip in the URL, so this check is the + // only thing between it and a foreign guest's users row — and the delete + // cascades every assignment that guest is on. + expect(roster.deleteGuest(otherTrip.id, guest.id)).toBe(false); + expect(testDb.prepare('SELECT id FROM users WHERE id = ?').get(guest.id)).toBeDefined(); + }); +}); + +// ── listMembers shaping ────────────────────────────────────────────────────── + +describe('listMembers shaping', () => { + it('MEMBERS-SVC-015: avatar_url follows the storage form and the inviter is named, not just its id', () => { + const { user: owner } = createUser(testDb); + const { user: uploaded } = createUser(testDb); + const { user: sso } = createUser(testDb); + const { user: bare } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + testDb.prepare('UPDATE users SET avatar = ?, display_name = ? WHERE id = ?').run('me.png', 'Owner Displayed', owner.id); + testDb.prepare('UPDATE users SET avatar = ? WHERE id = ?').run('a.png', uploaded.id); + testDb.prepare('UPDATE users SET avatar = ? WHERE id = ?').run('https://idp.example.test/p.jpg', sso.id); + testDb.prepare('INSERT INTO trip_members (trip_id, user_id, invited_by) VALUES (?, ?, ?)').run(trip.id, uploaded.id, owner.id); + testDb.prepare('INSERT INTO trip_members (trip_id, user_id, invited_by) VALUES (?, ?, ?)').run(trip.id, sso.id, owner.id); + addTripMember(testDb, trip.id, bare.id); + + const { owner: ownerRow, members } = roster.listMembers(trip.id, owner.id); + // added_at has second resolution, so rows created in one test are tied — index + // by id rather than asserting the ORDER BY. + const byId = new Map(members.map(m => [m.id, m])); + + // An uploaded file name becomes a /uploads/avatars path; an OIDC picture claim + // (#1399) is an absolute URL and must pass through untouched; a member without + // one must stay null instead of rendering '/uploads/avatars/null'. + expect(ownerRow.avatar_url).toBe('/uploads/avatars/me.png'); + expect(byId.get(uploaded.id)!.avatar_url).toBe('/uploads/avatars/a.png'); + expect(byId.get(sso.id)!.avatar_url).toBe('https://idp.example.test/p.jpg'); + expect(byId.get(bare.id)!.avatar_url).toBeNull(); + + // The inviter is resolved through a LEFT JOIN that prefers display_name; a row + // with no inviter reports null rather than dropping out of the list. + expect(byId.get(uploaded.id)!.invited_by_username).toBe('Owner Displayed'); + expect(byId.get(bare.id)!.invited_by_username).toBeNull(); + + // is_guest is a SQLite integer on the way out and a boolean on the wire. + expect(byId.get(bare.id)!.is_guest).toBe(false); + expect(ownerRow.is_guest).toBe(false); + }); +}); diff --git a/server/tests/unit/nest/trip-read-model.service.test.ts b/server/tests/unit/nest/trip-read-model.service.test.ts new file mode 100644 index 000000000..b9af5a8e5 --- /dev/null +++ b/server/tests/unit/nest/trip-read-model.service.test.ts @@ -0,0 +1,261 @@ +/** + * Unit tests for the DI-native TripReadModelService — TRIP-READ-001..006. + * The two read aggregates (MCP summary + offline bundle) moved here when the + * trips aggregate root was split; the happy-path aggregation cases stayed in + * tests/unit/nest/trips.service.test.ts so that diff reads as a move. This file + * pins the guards around them: the early returns, the falsy-price fold, the + * checked tally and the viewer scoping that keeps other members' private + * packing items (#858) out of both aggregates. + * Uses a real in-memory SQLite DB so SQL logic is exercised faithfully. + */ +import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest'; + +// ── DB setup ────────────────────────────────────────────────────────────────── + +const { testDb, dbMock } = vi.hoisted(() => { + const Database = require('better-sqlite3'); + const db = new Database(':memory:'); + db.exec('PRAGMA journal_mode = WAL'); + db.exec('PRAGMA foreign_keys = ON'); + db.exec('PRAGMA busy_timeout = 5000'); + const mock = { + db, + closeDb: () => {}, + reinitialize: () => {}, + getPlaceWithTags: () => null, + canAccessTrip: (tripId: any, userId: number) => + db.prepare(` + SELECT t.id, t.user_id FROM trips t + LEFT JOIN trip_members m ON m.trip_id = t.id AND m.user_id = ? + WHERE t.id = ? AND (t.user_id = ? OR m.user_id IS NOT NULL) + `).get(userId, tripId, userId), + isOwner: (tripId: any, userId: number) => + !!db.prepare('SELECT id FROM trips WHERE id = ? AND user_id = ?').get(tripId, userId), + }; + return { testDb: db, dbMock: mock }; +}); + +vi.mock('../../../src/db/database', () => dbMock); +vi.mock('../../../src/config', () => ({ + JWT_SECRET: 'test-secret', + ENCRYPTION_KEY: 'a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2', + updateJwtSecret: () => {}, +})); +const { broadcast } = vi.hoisted(() => ({ broadcast: vi.fn() })); +vi.mock('../../../src/websocket', () => ({ broadcast })); + +import { createTables } from '../../../src/db/schema'; +import { runMigrations } from '../../../src/db/migrations'; +import { resetTestDb } from '../../helpers/test-db'; +import { createUser, createTrip, addTripMember } from '../../helpers/factories'; +import { DatabaseService } from '../../../src/nest/database/database.service'; +import { DaysService } from '../../../src/nest/days/days.service'; +import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; +import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; +import { TodoService } from '../../../src/nest/todo/todo.service'; +import { PackingService } from '../../../src/nest/packing/packing.service'; +import { FilesService } from '../../../src/nest/files/files.service'; +import { ReservationsService } from '../../../src/nest/reservations/reservations.service'; +import { BudgetService } from '../../../src/nest/budget/budget.service'; +import { ExchangeRatesService } from '../../../src/nest/budget/exchange-rates.service'; +import { CollabService } from '../../../src/nest/collab/collab.service'; +import { PlacesService } from '../../../src/nest/places/places.service'; +import { UserCleanupService } from '../../../src/nest/auth/user-cleanup.service'; +import { TripMembersService } from '../../../src/nest/trip-members/trip-members.service'; +import { TripReadModelService } from '../../../src/nest/trip-read-model/trip-read-model.service'; +import { AccommodationsService } from '../../../src/nest/accommodations/accommodations.service'; +import { MapsService } from '../../../src/nest/maps/maps.service'; +import { QueryHelpersService } from '../../../src/nest/query-helpers/query-helpers.service'; + +// Real sibling services over the same in-memory DB — the aggregation runs the +// actual SQL of every domain it fans out to, so a shape change downstream shows +// up here instead of being papered over by a stub. +const dbs = () => new DatabaseService(testDb); +const budgetSvc = new BudgetService(dbs(), new PermissionsService(dbs()), new ExchangeRatesService(), new RealtimeService()); +const daysSvc = new DaysService(dbs(), new PermissionsService(dbs()), new RealtimeService(), new QueryHelpersService(dbs())); +const placesSvc = new PlacesService(dbs(), new PermissionsService(dbs()), new RealtimeService(), new MapsService(dbs()), new QueryHelpersService(dbs())); +const accommodationsSvc = new AccommodationsService(dbs(), new PermissionsService(dbs()), new RealtimeService()); +const membersSvc = new TripMembersService(dbs(), budgetSvc, new UserCleanupService(dbs()), new PermissionsService(dbs()), new RealtimeService()); + +const buildReadModel = (database: DatabaseService, roster: TripMembersService = membersSvc) => + new TripReadModelService( + database, roster, daysSvc, accommodationsSvc, budgetSvc, + new PackingService(dbs(), new PermissionsService(dbs()), new RealtimeService()), + new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService()), + new CollabService(dbs(), new PermissionsService(dbs()), new RealtimeService()), + placesSvc, + new TodoService(dbs(), new PermissionsService(dbs()), new RealtimeService()), + new FilesService(dbs(), new PermissionsService(dbs()), new RealtimeService()), + ); + +const svc = buildReadModel(dbs()); + +beforeAll(() => { + createTables(testDb); + runMigrations(testDb); +}); + +beforeEach(() => { + resetTestDb(testDb); +}); + +afterAll(() => { + testDb.close(); +}); + +// ── Helpers ─────────────────────────────────────────────────────────────────── + +const addBudgetItem = (tripId: number, name: string, totalPrice: number) => + testDb.prepare("INSERT INTO budget_items (trip_id, category, name, total_price) VALUES (?, 'food', ?, ?)") + .run(tripId, name, totalPrice); + +const addPackingItem = (tripId: number, name: string, checked: number) => + testDb.prepare('INSERT INTO packing_items (trip_id, name, checked) VALUES (?, ?, ?)') + .run(tripId, name, checked); + +/** + * A DatabaseService whose connection cannot resolve the owner lookup, standing in + * for the trip row disappearing between the two SELECTs getTripSummary issues. + * Only the read model's own two statements go through this connection; the + * sibling services keep the real one. + */ +function ownerlessDbs(): DatabaseService { + const conn = new Proxy(testDb, { + get(target, prop) { + if (prop === 'prepare') { + return (sql: string) => + (sql.includes('SELECT user_id FROM trips') ? { get: () => undefined } : target.prepare(sql)); + } + const v = (target as any)[prop]; + return typeof v === 'function' ? v.bind(target) : v; + }, + }); + return { connection: conn } as unknown as DatabaseService; +} + +// ── Tests ───────────────────────────────────────────────────────────────────── + +describe('getTripSummary guards', () => { + it('TRIP-READ-001: returns null for a missing trip instead of throwing', () => { + // The MCP get_trip_summary tool hands over whatever id the model produced, so + // an unknown id has to come back as an empty answer; a throw there surfaces as + // a tool error rather than "no such trip". + expect(svc.getTripSummary(99999)).toBeNull(); + expect(svc.getTripSummary(99999, 1)).toBeNull(); + }); + + it('TRIP-READ-002: returns null when the owner row cannot be read', () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + + // Trip row and owner id are two separate SELECTs; if the second one comes back + // empty (trip deleted in between) the guard has to stop. Without it listMembers + // runs with an undefined owner id and the summary reports an ownerless trip. + expect(buildReadModel(ownerlessDbs()).getTripSummary(trip.id, owner.id)).toBeNull(); + + // Same trip through the real connection still aggregates — the null above is + // the missing owner row, not a broken fixture. + expect(svc.getTripSummary(trip.id, owner.id)).not.toBeNull(); + }); +}); + +describe('getTripSummary shaping', () => { + it('TRIP-READ-003: folds a falsy total_price into the budget total instead of poisoning it', () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addBudgetItem(trip.id, 'Dinner', 40); + // total_price is NOT NULL DEFAULT 0, so 0 is the reachable falsy price: a free + // entry someone logged to keep it on the list. The `|| 0` in the reduce is what + // keeps any such value out of the sum — drop it and a price that arrives + // non-numeric turns the whole trip total into NaN, which the MCP summary and + // the offline clients both render as an empty budget. + addBudgetItem(trip.id, 'Free walking tour', 0); + + const summary = svc.getTripSummary(trip.id, owner.id)!; + expect(summary.budget.item_count).toBe(2); + expect(summary.budget.total).toBe(40); + expect(summary.budget.currency).toBe('EUR'); + }); + + it('TRIP-READ-004: counts only checked packing items, not the whole list', () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addPackingItem(trip.id, 'Socks', 1); + addPackingItem(trip.id, 'Charger', 1); + addPackingItem(trip.id, 'Passport', 0); + + // total and checked come from the same array; if the filter is ever widened the + // packing progress the summary reports jumps to 100% while items are still open. + const summary = svc.getTripSummary(trip.id, owner.id)!; + expect(summary.packing.total).toBe(3); + expect(summary.packing.checked).toBe(2); + }); +}); + +describe('bundle shaping', () => { + it('TRIP-READ-005: keeps the member list a flat array when the roster has no members', () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + + // The bundle spreads the roster's members into the array it ships offline. The + // `|| []` is the guard for a roster shape without that array — without it the + // spread throws and the whole offline bundle fails, taking days, places and + // reservations down with it over a trip that simply has no collaborators. + const roster = vi.spyOn(membersSvc, 'listMembers').mockReturnValue({ + owner: { id: owner.id, username: 'solo' }, members: undefined, + } as never); + try { + const result = svc.bundle(String(trip.id), { user_id: owner.id }, owner.id) as any; + expect(result.members).toEqual([{ id: owner.id, username: 'solo' }]); + } finally { + roster.mockRestore(); + } + }); + + it('TRIP-READ-006: drops a falsy owner rather than shipping a hole in the member list', () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + + // filter(Boolean) exists because the owner lookup can come back empty (a user + // row deleted while the trip lingers). Clients index the member list to render + // avatars, so an undefined slot in it crashes the offline view. + const roster = vi.spyOn(membersSvc, 'listMembers').mockReturnValue({ + owner: undefined, members: [{ id: 42, username: 'left-behind' }], + } as never); + try { + const result = svc.bundle(String(trip.id), { user_id: owner.id }, owner.id) as any; + expect(result.members).toEqual([{ id: 42, username: 'left-behind' }]); + } finally { + roster.mockRestore(); + } + }); +}); + +describe('private packing items stay viewer-scoped (#858)', () => { + it("TRIP-READ-007: neither summary nor bundle leaks another member's private item", () => { + const { user: owner } = createUser(testDb); + const { user: viewer } = createUser(testDb); + const trip = createTrip(testDb, owner.id, { start_date: '2025-06-01', end_date: '2025-06-02' }); + addTripMember(testDb, trip.id, viewer.id); + testDb.prepare("INSERT INTO packing_items (trip_id, name, is_private, owner_id) VALUES (?, 'Ring', 1, ?)") + .run(trip.id, owner.id); + testDb.prepare("INSERT INTO packing_items (trip_id, name) VALUES (?, 'Tent')").run(trip.id); + + // Both read paths pass the viewer into packing.listItems; that argument is the + // ONLY thing filtering the list. If either call site loses it, listItems falls + // back to the unfiltered query and the surprise the owner is carrying shows up + // in the other member's MCP summary and in their offline cache. + const asViewer = svc.getTripSummary(trip.id, viewer.id)!; + expect(asViewer.packing.items.map((i: any) => i.name)).toEqual(['Tent']); + expect(asViewer.packing.total).toBe(1); + + const bundled = svc.bundle(String(trip.id), { user_id: owner.id }, viewer.id) as any; + expect(bundled.packingItems.map((i: any) => i.name)).toEqual(['Tent']); + + // The owner still sees their own private item through both paths, so the + // assertions above are the filter working, not an empty fixture. + expect(svc.getTripSummary(trip.id, owner.id)!.packing.items.map((i: any) => i.name)).toEqual(['Ring', 'Tent']); + expect((svc.bundle(String(trip.id), { user_id: owner.id }, owner.id) as any) + .packingItems.map((i: any) => i.name)).toEqual(['Ring', 'Tent']); + }); +}); diff --git a/server/tests/unit/nest/trips.controller.test.ts b/server/tests/unit/nest/trips.controller.test.ts index 338c0d3a3..08dc9dd41 100644 --- a/server/tests/unit/nest/trips.controller.test.ts +++ b/server/tests/unit/nest/trips.controller.test.ts @@ -2,6 +2,9 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import type { UnsplashService } from '../../../src/nest/unsplash/unsplash.service'; import { RuntimeEnvService } from '../../../src/nest/app-config/runtime-env.service'; import { HttpException } from '@nestjs/common'; +import { INTERCEPTORS_METADATA } from '@nestjs/common/constants'; +import fs from 'fs'; +import path from 'path'; import type { Request } from 'express'; vi.mock('../../../src/nest/audit/client-ip', () => ({ getClientIp: vi.fn(() => '1.2.3.4') })); @@ -102,6 +105,41 @@ describe('TripsController (parity with the legacy /api/trips route)', () => { }); }); + describe('GET /cover-images/search', () => { + it('passes the caller through to the service and unwraps the photo list', async () => { + const searchCoverImages = vi.fn().mockResolvedValue({ photos: [{ id: 'p1' }] }); + const s = svc({ searchCoverImages } as Partial); + expect(await tc(s).coverImages(user, 'kyoto')).toEqual({ photos: [{ id: 'p1' }] }); + // The Unsplash key is resolved per user behind the service; dropping the id + // here would silently search every request with an admin's key. + expect(searchCoverImages).toHaveBeenCalledWith('kyoto', 1); + }); + + it('sends an absent query as "" so the service keeps owning the empty-query 400', async () => { + const searchCoverImages = vi.fn().mockResolvedValue({ error: 'Search query is required', status: 400 }); + const s = svc({ searchCoverImages } as Partial); + expect(await thrownAsync(() => tc(s).coverImages(user, undefined))).toEqual({ status: 400, body: { error: 'Search query is required' } }); + expect(searchCoverImages).toHaveBeenCalledWith('', 1); + }); + + it('keeps the upstream status instead of flattening it into the catch-all 500', async () => { + const s = svc({ searchCoverImages: vi.fn().mockResolvedValue({ error: 'Rate limit reached', status: 429 }) } as Partial); + // A 429 that arrives as a 500 makes the client retry immediately and burn + // the rest of the hourly Unsplash quota. + expect(await thrownAsync(() => tc(s).coverImages(user, 'kyoto'))).toEqual({ status: 429, body: { error: 'Rate limit reached' } }); + }); + + it('maps an unexpected failure to a 500 that does not leak the cause', async () => { + const err = vi.spyOn(console, 'error').mockImplementation(() => {}); + try { + const s = svc({ searchCoverImages: vi.fn().mockRejectedValue(new Error('ECONNREFUSED api.unsplash.com')) } as Partial); + expect(await thrownAsync(() => tc(s).coverImages(user, 'kyoto'))).toEqual({ status: 500, body: { error: 'Error searching for cover images' } }); + } finally { + err.mockRestore(); + } + }); + }); + describe('POST / (create)', () => { it('403 without trip_create; a missing title 400s in the ZodValidationPipe', () => { expect(thrown(() => tc(svc({ can: vi.fn().mockReturnValue(false) })).create(user, { title: 'T' }, req))).toEqual({ status: 403, body: { error: 'No permission to create trips' } }); @@ -285,6 +323,84 @@ describe('TripsController (parity with the legacy /api/trips route)', () => { else process.env.DEMO_MODE = prev; } }); + + /** + * The multer options are a module-private const wired into the route through + * @UseInterceptors, so the handler tests above never reach them: a broken + * destination/filename/filter would still let every case in this file pass. + * Pulling the interceptor back off the route and reading the multer instance + * it built (multer keeps storage, limits and fileFilter verbatim) exercises + * the real callbacks instead of asserting an object literal. + */ + type StorageCb = (err: Error | null, value: string) => void; + type CoverUpload = { + storage: { + getDestination: (req: unknown, file: Express.Multer.File, cb: StorageCb) => void; + getFilename: (req: unknown, file: Express.Multer.File, cb: StorageCb) => void; + }; + limits: { fileSize: number }; + fileFilter: (req: unknown, file: Express.Multer.File, cb: (err: Error | null, accept: boolean) => void) => void; + }; + function coverUpload(): CoverUpload { + const [Interceptor] = Reflect.getMetadata(INTERCEPTORS_METADATA, TripsController.prototype.cover) as Array { multer: CoverUpload }>; + return new Interceptor().multer; + } + const coversDir = path.join('uploads', 'covers'); + + it('creates uploads/covers on the first upload and reuses it afterwards', () => { + // A fresh container has no uploads/covers, and multer does not create the + // destination itself, so without the mkdir the very first cover upload on + // a new instance fails with ENOENT. + const exists = vi.spyOn(fs, 'existsSync').mockReturnValue(false); + const mkdir = vi.spyOn(fs, 'mkdirSync').mockImplementation(() => undefined); + try { + const { getDestination } = coverUpload().storage; + const cb = vi.fn(); + getDestination(req, file, cb); + expect(mkdir).toHaveBeenCalledWith(expect.stringContaining(coversDir), { recursive: true }); + expect(cb).toHaveBeenCalledWith(null, expect.stringContaining(coversDir)); + exists.mockReturnValue(true); + mkdir.mockClear(); + getDestination(req, file, cb); + expect(mkdir).not.toHaveBeenCalled(); + expect(cb).toHaveBeenLastCalledWith(null, expect.stringContaining(coversDir)); + } finally { + exists.mockRestore(); + mkdir.mockRestore(); + } + }); + + it('gives every stored cover a fresh name and keeps the original extension', () => { + const { getFilename } = coverUpload().storage; + const cb = vi.fn(); + const upload = { originalname: 'sunset.JPG' } as Express.Multer.File; + getFilename(req, upload, cb); + getFilename(req, upload, cb); + const [first, second] = cb.mock.calls.map((c) => c[1] as string); + // Keeping the client's name would let one trip's upload overwrite another's; + // dropping the extension leaves the static handler with no mime to serve. + expect(first).toMatch(/^[0-9a-f-]{36}\.JPG$/); + expect(second).not.toBe(first); + }); + + it('accepts the four allowed image types and rejects svg, a faked extension and a non-image mime', () => { + const { fileFilter, limits } = coverUpload(); + const verdict = (originalname: string, mimetype: string) => { + const cb = vi.fn(); + fileFilter(req, { originalname, mimetype } as Express.Multer.File, cb); + return cb.mock.calls[0]; + }; + expect(verdict('a.jpg', 'image/jpeg')).toEqual([null, true]); + // The extension check is case-insensitive; phones hand up .JPG/.PNG. + expect(verdict('a.WEBP', 'image/webp')).toEqual([null, true]); + // svg passes the image/* prefix but runs script when the browser opens the + // stored cover, so it is rejected even under an allowed extension. + expect(verdict('logo.png', 'image/svg+xml')).toEqual([expect.any(Error), false]); + // A binary renamed to an image mime must still fail on the extension. + expect(verdict('payload.exe', 'image/png')).toEqual([expect.any(Error), false]); + expect(verdict('a.png', 'application/octet-stream')).toEqual([expect.any(Error), false]); + expect(limits.fileSize).toBe(20 * 1024 * 1024); + }); }); describe('GET /:id/export.ics', () => { diff --git a/server/vitest.config.ts b/server/vitest.config.ts index f01618522..6131fc524 100644 --- a/server/vitest.config.ts +++ b/server/vitest.config.ts @@ -61,7 +61,7 @@ export default defineConfig({ // A file matched by a glob below is EXCLUDED from the catch-all, so the last entry // only covers what sits directly under src/nest (app.module.ts and the like). thresholds: { - 'src/nest/accommodations/**/*.ts': { statements: 92, branches: 82, functions: 98, lines: 97 }, + 'src/nest/accommodations/**/*.ts': { statements: 92, branches: 87, functions: 99, lines: 97 }, 'src/nest/addons/**/*.ts': { statements: 99, branches: 87, functions: 99, lines: 99 }, 'src/nest/admin/**/*.ts': { statements: 86, branches: 70, functions: 89, lines: 88 }, 'src/nest/airports/**/*.ts': { statements: 68, branches: 60, functions: 93, lines: 73 }, @@ -73,14 +73,14 @@ export default defineConfig({ 'src/nest/backup/**/*.ts': { statements: 96, branches: 92, functions: 99, lines: 98 }, 'src/nest/booking-import/**/*.ts': { statements: 48, branches: 28, functions: 56, lines: 52 }, 'src/nest/budget/**/*.ts': { statements: 91, branches: 77, functions: 97, lines: 95 }, - 'src/nest/calendar/**/*.ts': { statements: 92, branches: 82, functions: 91, lines: 94 }, + 'src/nest/calendar/**/*.ts': { statements: 96, branches: 93, functions: 99, lines: 99 }, 'src/nest/categories/**/*.ts': { statements: 99, branches: 80, functions: 99, lines: 99 }, 'src/nest/collab/**/*.ts': { statements: 92, branches: 85, functions: 94, lines: 97 }, 'src/nest/collections/**/*.ts': { statements: 83, branches: 70, functions: 91, lines: 92 }, 'src/nest/common/**/*.ts': { statements: 95, branches: 89, functions: 99, lines: 97 }, 'src/nest/config/**/*.ts': { statements: 99, branches: 100, functions: 99, lines: 99 }, 'src/nest/database/**/*.ts': { statements: 99, branches: 100, functions: 99, lines: 99 }, - 'src/nest/day-notes/**/*.ts': { statements: 92, branches: 82, functions: 98, lines: 97 }, + 'src/nest/day-notes/**/*.ts': { statements: 92, branches: 83, functions: 99, lines: 99 }, 'src/nest/days/**/*.ts': { statements: 92, branches: 82, functions: 98, lines: 97 }, 'src/nest/feeds/**/*.ts': { statements: 91, branches: 83, functions: 83, lines: 91 }, 'src/nest/files/**/*.ts': { statements: 97, branches: 95, functions: 99, lines: 98 }, @@ -112,8 +112,8 @@ export default defineConfig({ 'src/nest/transit/**/*.ts': { statements: 92, branches: 83, functions: 97, lines: 94 }, 'src/nest/trip-invite/**/*.ts': { statements: 91, branches: 93, functions: 93, lines: 89 }, 'src/nest/trip-membership/**/*.ts': { statements: 99, branches: 86, functions: 99, lines: 99 }, - 'src/nest/trip-members/**/*.ts': { statements: 92, branches: 82, functions: 91, lines: 94 }, - 'src/nest/trip-read-model/**/*.ts': { statements: 92, branches: 82, functions: 91, lines: 94 }, + 'src/nest/trip-members/**/*.ts': { statements: 93, branches: 89, functions: 99, lines: 94 }, + 'src/nest/trip-read-model/**/*.ts': { statements: 97, branches: 96, functions: 99, lines: 97 }, 'src/nest/trips/**/*.ts': { statements: 92, branches: 82, functions: 91, lines: 94 }, 'src/nest/unsplash/**/*.ts': { statements: 97, branches: 82, functions: 99, lines: 96 }, 'src/nest/vacay/**/*.ts': { statements: 82, branches: 66, functions: 89, lines: 86 },