mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
fix(server): fix the quirks preserved by the day DI migration
This commit is contained in:
@@ -108,7 +108,10 @@ export class DaysController {
|
||||
if (!current) {
|
||||
throw new HttpException({ error: 'Day not found' }, 404);
|
||||
}
|
||||
const day = this.days.update(id, current as never, { notes: body.notes, title: body.title });
|
||||
// The zod-parsed body carries only the keys the client actually sent, so
|
||||
// the service's presence sentinels preserve the omitted column (the client
|
||||
// updates notes and title in separate requests).
|
||||
const day = this.days.update(id, current as never, body);
|
||||
this.days.broadcast(tripId, 'day:updated', { day }, socketId);
|
||||
return { day };
|
||||
}
|
||||
|
||||
@@ -55,7 +55,7 @@ export class DaysMcp {
|
||||
if (!hasTripPermission('day_edit', tripId, ctx.userId)) return permissionDenied();
|
||||
const current = this.days.getDay(dayId, tripId);
|
||||
if (!current) return { content: [{ type: 'text' as const, text: 'Day not found.' }], isError: true };
|
||||
const updated = this.days.update(dayId, current, title !== undefined ? { title } : {});
|
||||
const updated = this.days.update(dayId, current, { title });
|
||||
safeBroadcast(tripId, 'day:updated', { day: updated });
|
||||
return ok({ day: updated });
|
||||
}
|
||||
|
||||
@@ -70,7 +70,12 @@ export interface CreateAccommodationData {
|
||||
* defaults, the post-write re-selects, the two-phase negative-day_number
|
||||
* renumber and the reservation re-stamping). The legacy hand-rolled
|
||||
* BEGIN/COMMIT blocks in reorder/insert became db.transaction() (same
|
||||
* rollback-on-throw semantics, savepoint-safe when nested). Trip access
|
||||
* rollback-on-throw semantics, savepoint-safe when nested). Verified defects
|
||||
* were fixed after the port (2026-07): update() now presence-sentinels BOTH
|
||||
* columns (the legacy always-write wiped notes when only a title was sent),
|
||||
* createAccommodation/deleteAccommodation run their multi-statement writes in
|
||||
* a transaction, and getAssignmentsForDay batch-loads tags instead of one
|
||||
* query per assignment. Trip access
|
||||
* mirrors the requireTripAccess middleware (canAccessTrip); mutations use the
|
||||
* 'day_edit' permission; the WebSocket broadcast keeps its legacy call path.
|
||||
* Non-Nest consumers (legacy tripService and the transit/transports MCP
|
||||
@@ -113,12 +118,13 @@ export class DaysService {
|
||||
ORDER BY da.order_index ASC, da.created_at ASC
|
||||
`, dayId);
|
||||
|
||||
// One batched tag load instead of the legacy per-assignment query; the
|
||||
// non-compact loader returns the same full tag rows (t.* minus the join
|
||||
// key), so the output shape is unchanged.
|
||||
const tagsByPlaceId = loadTagsByPlaceIds([...new Set(assignments.map(a => a.place_id))]);
|
||||
|
||||
return assignments.map(a => {
|
||||
const tags = this.db.all(`
|
||||
SELECT t.* FROM tags t
|
||||
JOIN place_tags pt ON t.id = pt.tag_id
|
||||
WHERE pt.place_id = ?
|
||||
`, a.place_id);
|
||||
const tags = tagsByPlaceId[a.place_id] || [];
|
||||
|
||||
return {
|
||||
id: a.id,
|
||||
@@ -237,9 +243,13 @@ export class DaysService {
|
||||
}
|
||||
|
||||
update(id: string | number, current: Day, fields: { notes?: string; title?: string | null }) {
|
||||
// Both columns use the presence sentinel: an absent key preserves the
|
||||
// current value (the legacy version always wrote notes, so setting a title
|
||||
// silently wiped the day's notes — the client sends the two fields in
|
||||
// separate requests).
|
||||
this.db.run('UPDATE days SET notes = ?, title = ? WHERE id = ?',
|
||||
fields.notes || null,
|
||||
'title' in fields ? (fields.title ?? null) : current.title,
|
||||
'notes' in fields ? (fields.notes || null) : (current.notes ?? null),
|
||||
'title' in fields ? (fields.title ?? null) : (current.title ?? null),
|
||||
id
|
||||
);
|
||||
const updatedDay = this.db.get<Day>('SELECT * FROM days WHERE id = ?', id)!;
|
||||
@@ -533,28 +543,34 @@ export class DaysService {
|
||||
createAccommodation(tripId: string | number, data: CreateAccommodationData) {
|
||||
const { place_id, start_day_id, end_day_id, check_in, check_in_end, check_out, confirmation, notes } = data;
|
||||
|
||||
const result = this.db.run(
|
||||
'INSERT INTO day_accommodations (trip_id, place_id, start_day_id, end_day_id, check_in, check_in_end, check_out, confirmation, notes) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)',
|
||||
tripId, place_id, start_day_id, end_day_id, check_in || null, check_in_end || null, check_out || null, confirmation || null, notes || null
|
||||
);
|
||||
// The stay and its partner hotel reservation are one logical write —
|
||||
// atomic, so a failed reservation insert can't leave an orphan stay.
|
||||
const accommodationId = this.db.transaction(() => {
|
||||
const result = this.db.run(
|
||||
'INSERT INTO day_accommodations (trip_id, place_id, start_day_id, end_day_id, check_in, check_in_end, check_out, confirmation, notes) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)',
|
||||
tripId, place_id, start_day_id, end_day_id, check_in || null, check_in_end || null, check_out || null, confirmation || null, notes || null
|
||||
);
|
||||
|
||||
const accommodationId = result.lastInsertRowid;
|
||||
const newId = result.lastInsertRowid;
|
||||
|
||||
// Auto-create linked reservation for this accommodation
|
||||
const placeName = this.db.get<{ name: string }>('SELECT name FROM places WHERE id = ?', place_id)?.name || 'Hotel';
|
||||
const startDayDate = this.db.get<{ date: string }>('SELECT date FROM days WHERE id = ?', start_day_id)?.date || null;
|
||||
const meta: Record<string, string> = {};
|
||||
if (check_in) meta.check_in_time = check_in;
|
||||
if (check_in_end) meta.check_in_end_time = check_in_end;
|
||||
if (check_out) meta.check_out_time = check_out;
|
||||
this.db.run(`
|
||||
// Auto-create linked reservation for this accommodation
|
||||
const placeName = this.db.get<{ name: string }>('SELECT name FROM places WHERE id = ?', place_id)?.name || 'Hotel';
|
||||
const startDayDate = this.db.get<{ date: string }>('SELECT date FROM days WHERE id = ?', start_day_id)?.date || null;
|
||||
const meta: Record<string, string> = {};
|
||||
if (check_in) meta.check_in_time = check_in;
|
||||
if (check_in_end) meta.check_in_end_time = check_in_end;
|
||||
if (check_out) meta.check_out_time = check_out;
|
||||
this.db.run(`
|
||||
INSERT INTO reservations (trip_id, day_id, title, reservation_time, location, confirmation_number, notes, status, type, accommodation_id, metadata)
|
||||
VALUES (?, ?, ?, ?, ?, ?, ?, 'confirmed', 'hotel', ?, ?)
|
||||
`,
|
||||
tripId, start_day_id, placeName, startDayDate || null, null,
|
||||
confirmation || null, notes || null, accommodationId,
|
||||
Object.keys(meta).length > 0 ? JSON.stringify(meta) : null
|
||||
);
|
||||
tripId, start_day_id, placeName, startDayDate || null, null,
|
||||
confirmation || null, notes || null, newId,
|
||||
Object.keys(meta).length > 0 ? JSON.stringify(meta) : null
|
||||
);
|
||||
|
||||
return newId;
|
||||
});
|
||||
|
||||
return this.getAccommodationWithPlace(accommodationId);
|
||||
}
|
||||
@@ -595,20 +611,22 @@ export class DaysService {
|
||||
return this.getAccommodationWithPlace(Number(id));
|
||||
}
|
||||
|
||||
/** Delete accommodation and its linked reservation (and any linked budget item). */
|
||||
/** Delete accommodation and its linked reservation (and any linked budget item), atomically. */
|
||||
deleteAccommodation(id: string | number): { linkedReservationId: number | null; deletedBudgetItemId: number | null } {
|
||||
const linkedRes = this.db.get<{ id: number }>('SELECT id FROM reservations WHERE accommodation_id = ?', Number(id));
|
||||
let deletedBudgetItemId: number | null = null;
|
||||
if (linkedRes) {
|
||||
const linkedBudget = this.db.get<{ id: number }>('SELECT id FROM budget_items WHERE reservation_id = ?', linkedRes.id);
|
||||
if (linkedBudget) {
|
||||
this.db.run('DELETE FROM budget_items WHERE id = ?', linkedBudget.id);
|
||||
deletedBudgetItemId = linkedBudget.id;
|
||||
return this.db.transaction(() => {
|
||||
const linkedRes = this.db.get<{ id: number }>('SELECT id FROM reservations WHERE accommodation_id = ?', Number(id));
|
||||
let deletedBudgetItemId: number | null = null;
|
||||
if (linkedRes) {
|
||||
const linkedBudget = this.db.get<{ id: number }>('SELECT id FROM budget_items WHERE reservation_id = ?', linkedRes.id);
|
||||
if (linkedBudget) {
|
||||
this.db.run('DELETE FROM budget_items WHERE id = ?', linkedBudget.id);
|
||||
deletedBudgetItemId = linkedBudget.id;
|
||||
}
|
||||
this.db.run('DELETE FROM reservations WHERE id = ?', linkedRes.id);
|
||||
}
|
||||
this.db.run('DELETE FROM reservations WHERE id = ?', linkedRes.id);
|
||||
}
|
||||
|
||||
this.db.run('DELETE FROM day_accommodations WHERE id = ?', id);
|
||||
return { linkedReservationId: linkedRes ? linkedRes.id : null, deletedBudgetItemId };
|
||||
this.db.run('DELETE FROM day_accommodations WHERE id = ?', id);
|
||||
return { linkedReservationId: linkedRes ? linkedRes.id : null, deletedBudgetItemId };
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
@@ -139,6 +139,17 @@ describe('Days + day-notes e2e (real auth guard + temp SQLite, real day SQL)', (
|
||||
.send({ notes: 'Walking day', title: 'Arrival' });
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.day).toMatchObject({ id: 3, notes: 'Walking day', title: 'Arrival', assignments: [] });
|
||||
// The client updates title and notes in separate requests — an omitted
|
||||
// field must survive (post-port defect fix: the legacy update always wrote
|
||||
// both columns, so a title-only PUT wiped the notes).
|
||||
const titleOnly = await request(server).put('/api/trips/5/days/3').set('Cookie', sessionCookie(1))
|
||||
.send({ title: 'Renamed' });
|
||||
expect(titleOnly.status).toBe(200);
|
||||
expect(titleOnly.body.day).toMatchObject({ id: 3, notes: 'Walking day', title: 'Renamed' });
|
||||
const notesOnly = await request(server).put('/api/trips/5/days/3').set('Cookie', sessionCookie(1))
|
||||
.send({ notes: 'Museum day' });
|
||||
expect(notesOnly.status).toBe(200);
|
||||
expect(notesOnly.body.day).toMatchObject({ id: 3, notes: 'Museum day', title: 'Renamed' });
|
||||
const miss = await request(server).put('/api/trips/5/days/99').set('Cookie', sessionCookie(1)).send({ notes: 'x' });
|
||||
expect(miss.status).toBe(404);
|
||||
expect(miss.body).toEqual({ error: 'Day not found' });
|
||||
|
||||
@@ -95,6 +95,22 @@ describe('Tool: update_day', () => {
|
||||
});
|
||||
});
|
||||
|
||||
it('setting a title preserves the day notes (post-port defect fix)', async () => {
|
||||
const { user } = createUser(testDb);
|
||||
const trip = createTrip(testDb, user.id);
|
||||
const day = createDay(testDb, trip.id);
|
||||
testDb.prepare('UPDATE days SET notes = ? WHERE id = ?').run('Walking day', day.id);
|
||||
|
||||
await withHarness(user.id, async (h) => {
|
||||
const result = await h.client.callTool({
|
||||
name: 'update_day',
|
||||
arguments: { tripId: trip.id, dayId: day.id, title: 'Arrival' },
|
||||
});
|
||||
const data = parseToolResult(result) as { day: { title: string; notes: string } };
|
||||
expect(data.day).toMatchObject({ title: 'Arrival', notes: 'Walking day' });
|
||||
});
|
||||
});
|
||||
|
||||
it('broadcasts day:updated event', async () => {
|
||||
const { user } = createUser(testDb);
|
||||
const trip = createTrip(testDb, user.id);
|
||||
|
||||
@@ -1,7 +1,9 @@
|
||||
/**
|
||||
* Unit tests for the DI-native DaysService — DAY-SVC-001 through DAY-SVC-026
|
||||
* 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-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).
|
||||
* Uses a real in-memory SQLite DB so SQL logic is exercised faithfully.
|
||||
*/
|
||||
import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest';
|
||||
@@ -479,3 +481,69 @@ describe('days.bridge', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// ── post-port defect fixes ────────────────────────────────────────────────────
|
||||
|
||||
describe('quirk fixes', () => {
|
||||
it('DAY-SVC-033 — update preserves the omitted column (title-only keeps notes, notes-only keeps title)', () => {
|
||||
const { user } = createUser(testDb);
|
||||
const trip = createTrip(testDb, user.id);
|
||||
let day = createDay(testDb, trip.id);
|
||||
day = svc.update(day.id, day, { notes: 'Walking day' }) as never;
|
||||
day = svc.update(day.id, day, { title: 'Arrival' }) as never;
|
||||
expect(day).toMatchObject({ title: 'Arrival', notes: 'Walking day' });
|
||||
day = svc.update(day.id, day, { notes: 'Museum day' }) as never;
|
||||
expect(day).toMatchObject({ title: 'Arrival', notes: 'Museum day' });
|
||||
// A present key still clears via the legacy falsy coercion.
|
||||
day = svc.update(day.id, day, { notes: '' }) as never;
|
||||
expect(day.notes).toBeNull();
|
||||
expect(day.title).toBe('Arrival');
|
||||
});
|
||||
|
||||
it('DAY-SVC-034 — createAccommodation is atomic: a failed reservation insert leaves no orphan stay', () => {
|
||||
const { user } = createUser(testDb);
|
||||
const trip = createTrip(testDb, user.id);
|
||||
const day = createDay(testDb, trip.id);
|
||||
const place = createPlace(testDb, trip.id, { name: 'Hotel' });
|
||||
testDb.exec("CREATE TRIGGER boom BEFORE INSERT ON reservations BEGIN SELECT RAISE(ABORT, 'boom'); END");
|
||||
try {
|
||||
expect(() => svc.createAccommodation(trip.id, {
|
||||
place_id: place.id, start_day_id: day.id, end_day_id: day.id,
|
||||
})).toThrow();
|
||||
expect(testDb.prepare('SELECT COUNT(*) as n FROM day_accommodations WHERE trip_id = ?').get(trip.id)).toMatchObject({ n: 0 });
|
||||
} finally {
|
||||
testDb.exec('DROP TRIGGER boom');
|
||||
}
|
||||
});
|
||||
|
||||
it('DAY-SVC-035 — deleteAccommodation is atomic: a failed stay delete keeps the linked reservation', () => {
|
||||
const { user } = createUser(testDb);
|
||||
const trip = createTrip(testDb, user.id);
|
||||
const day = createDay(testDb, trip.id);
|
||||
const place = createPlace(testDb, trip.id, { name: 'Hotel' });
|
||||
const accom = svc.createAccommodation(trip.id, {
|
||||
place_id: place.id, start_day_id: day.id, end_day_id: day.id,
|
||||
}) as { id: number };
|
||||
testDb.exec("CREATE TRIGGER boom BEFORE DELETE ON day_accommodations BEGIN SELECT RAISE(ABORT, 'boom'); END");
|
||||
try {
|
||||
expect(() => svc.deleteAccommodation(accom.id)).toThrow();
|
||||
// The earlier reservation delete inside the transaction rolled back.
|
||||
expect(testDb.prepare('SELECT COUNT(*) as n FROM reservations WHERE accommodation_id = ?').get(accom.id)).toMatchObject({ n: 1 });
|
||||
} finally {
|
||||
testDb.exec('DROP TRIGGER boom');
|
||||
}
|
||||
});
|
||||
|
||||
it('DAY-SVC-036 — getAssignmentsForDay returns full tag rows from the batched load', () => {
|
||||
const { user } = createUser(testDb);
|
||||
const trip = createTrip(testDb, user.id);
|
||||
const day = createDay(testDb, trip.id);
|
||||
const place = createPlace(testDb, trip.id, { name: 'Tagged' });
|
||||
createDayAssignment(testDb, day.id, place.id);
|
||||
const tagId = Number(testDb.prepare('INSERT INTO tags (user_id, name, color) VALUES (?, ?, ?)').run(user.id, 'Food', '#ff0000').lastInsertRowid);
|
||||
testDb.prepare('INSERT INTO place_tags (place_id, tag_id) VALUES (?, ?)').run(place.id, tagId);
|
||||
|
||||
const assignments = svc.getAssignmentsForDay(day.id);
|
||||
expect(assignments[0].place.tags).toHaveLength(1);
|
||||
expect(assignments[0].place.tags[0]).toMatchObject({ id: tagId, name: 'Food', color: '#ff0000' });
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user