diff --git a/client/src/store/stayStops.ts b/client/src/store/stayStops.ts index 7f5ed2f6c..6a0204478 100644 --- a/client/src/store/stayStops.ts +++ b/client/src/store/stayStops.ts @@ -4,6 +4,10 @@ import type { Assignment } from '../types' /** The day-plan half of what an accommodation write answers with. */ export interface StayStopsResult { assignment?: Assignment | null + /** The booking's own stop, carried to the day the booking now covers. */ + movedAssignment?: { assignment: Assignment; oldDayId: number } | null + /** Stops the booking let go of but left standing (a night dropped, the place kept). */ + updatedAssignments?: Assignment[] removedAssignments?: { id: number; dayId: number }[] } @@ -24,4 +28,11 @@ export function applyStayStops(result: StayStopsResult | null | undefined): void handleRemoteEvent({ type: 'assignment:deleted', assignmentId: removed.id, dayId: removed.dayId }) } if (result.assignment) handleRemoteEvent({ type: 'assignment:created', assignment: result.assignment }) + if (result.movedAssignment) { + const { assignment, oldDayId } = result.movedAssignment + handleRemoteEvent({ type: 'assignment:moved', assignment, oldDayId, newDayId: assignment.day_id }) + } + for (const updated of result.updatedAssignments ?? []) { + handleRemoteEvent({ type: 'assignment:updated', assignment: updated }) + } } diff --git a/server/src/db/migrations.ts b/server/src/db/migrations.ts index f762e9704..816243944 100644 --- a/server/src/db/migrations.ts +++ b/server/src/db/migrations.ts @@ -4868,6 +4868,38 @@ function runMigrations(db: Database.Database): void { } } }, + + /* + * Let go of a booking the day stop can no longer reach. + * + * day_assignments.accommodation_id was added as a bare INTEGER, and the only + * code that ever clears it looks the stay up by id. A stay can also vanish + * without anybody asking: day_accommodations.end_day_id is ON DELETE CASCADE, + * so shortening a trip past a booking's last night deletes the booking while + * the stop on its first night survives, now pointing at nothing. Days hides any + * stop that carries an accommodation_id, so the hotel drops out of the day list + * on every surface while the route still drives to it, and the id is + * AUTOINCREMENT, so nothing will ever come along and free the row. + * + * A trigger rather than a column rebuild: day_assignments is referenced by two + * cascading tables of its own, and SQLite fires this even when the stay went + * down with a foreign-key cascade. + */ + () => { + db.exec(` + UPDATE day_assignments SET accommodation_id = NULL + WHERE accommodation_id IS NOT NULL + AND accommodation_id NOT IN (SELECT id FROM day_accommodations) + `); + db.exec('CREATE INDEX IF NOT EXISTS idx_day_assignments_accommodation_id ON day_assignments(accommodation_id)'); + db.exec(` + CREATE TRIGGER IF NOT EXISTS trg_release_stop_on_stay_delete + AFTER DELETE ON day_accommodations + BEGIN + UPDATE day_assignments SET accommodation_id = NULL WHERE accommodation_id = OLD.id; + END + `); + }, ]; if (currentVersion < migrations.length) { diff --git a/server/src/nest/accommodations/accommodations.controller.ts b/server/src/nest/accommodations/accommodations.controller.ts index 436e56624..b81f3337b 100644 --- a/server/src/nest/accommodations/accommodations.controller.ts +++ b/server/src/nest/accommodations/accommodations.controller.ts @@ -108,7 +108,7 @@ export class AccommodationsController { const { accommodation, mirror } = this.accommodations.update(id, existing as never, { place_id, start_day_id, end_day_id, check_in, check_in_end, check_out, confirmation, notes } as never); this.accommodations.broadcast(tripId, 'accommodation:updated', { accommodation }, socketId); this.accommodations.announceMirror(tripId, mirror, (event, payload) => this.accommodations.broadcast(tripId, event, payload, socketId), socketId); - return { accommodation, assignment: mirror.created, removedAssignments: mirror.removed }; + return { accommodation, assignment: mirror.created, movedAssignment: mirror.moved, removedAssignments: mirror.removed }; } @RequirePermission('day_edit') @@ -135,6 +135,6 @@ export class AccommodationsController { this.accommodations.broadcast(tripId, 'budget:deleted', { itemId }, socketId); } this.accommodations.broadcast(tripId, 'accommodation:deleted', { accommodationId: Number(id) }, socketId); - return { success: true, removedAssignments: mirror.removed }; + return { success: true, removedAssignments: mirror.removed, updatedAssignments: mirror.updated }; } } diff --git a/server/src/nest/accommodations/accommodations.mcp.ts b/server/src/nest/accommodations/accommodations.mcp.ts index 06f0e54b2..2dd6e0e54 100644 --- a/server/src/nest/accommodations/accommodations.mcp.ts +++ b/server/src/nest/accommodations/accommodations.mcp.ts @@ -173,7 +173,9 @@ export class AccommodationsMcp { const { accommodation, mirror } = this.accommodations.updateAccommodation(accommodationId, existing, { place_id, start_day_id, end_day_id, check_in, check_in_end, check_out, confirmation, notes }); this.guards.safeBroadcast(tripId, 'accommodation:updated', { accommodation }); this.accommodations.announceMirror(tripId, mirror, this.mirrorSender(tripId)); - return ok({ accommodation, assignment: mirror.created }); + // movedAssignment rather than a delete/create pair: an edit carries the booking's + // own stop across instead of rebuilding it, so the caller sees the same row. + return ok({ accommodation, assignment: mirror.created, movedAssignment: mirror.moved, removedAssignments: mirror.removed }); } @Tool({ diff --git a/server/src/nest/accommodations/accommodations.service.ts b/server/src/nest/accommodations/accommodations.service.ts index 4f7d7d090..ad434c867 100644 --- a/server/src/nest/accommodations/accommodations.service.ts +++ b/server/src/nest/accommodations/accommodations.service.ts @@ -17,6 +17,10 @@ export type MirrorSender = (event: E, payload: Tr export interface AccommodationMirror { /** The day stop the booking added, or null when that day already held the place. */ created: MirroredAssignment | null; + /** The booking's own stop, carried to where the booking now is. */ + moved: { assignment: MirroredAssignment; oldDayId: number } | null; + /** Stops the booking still stands on but no longer owns (a night dropped, the place kept). */ + updated: MirroredAssignment[]; /** Day stops the booking took back, because it moved days or was deleted. */ removed: { id: number; dayId: number }[]; /** The place, when this write was the one that typed it as lodging. */ @@ -25,7 +29,7 @@ export interface AccommodationMirror { /** A write that left the day plan alone. Exported for the surfaces that write a * stay row themselves and have to answer with a mirror either way. */ -export const noStayMirror = (): AccommodationMirror => ({ created: null, removed: [], stamped: null }); +export const noStayMirror = (): AccommodationMirror => ({ created: null, moved: null, updated: [], removed: [], stamped: null }); const noMirror = noStayMirror; export interface DayAccommodation { @@ -172,8 +176,35 @@ export class AccommodationsService { announceMirror(tripId: string | number, mirror: AccommodationMirror, send: MirrorSender, socketId?: string): void { for (const stop of mirror.removed) send('assignment:deleted', { assignmentId: stop.id, dayId: stop.dayId }); if (mirror.created) send('assignment:created', { assignment: mirror.created }); + if (mirror.moved) { + send('assignment:moved', { + assignment: mirror.moved.assignment, + oldDayId: mirror.moved.oldDayId, + newDayId: mirror.moved.assignment.day_id, + }); + } + for (const stop of mirror.updated) send('assignment:updated', { assignment: stop }); if (mirror.stamped) send('place:updated', { place: mirror.stamped }); - if (mirror.created || mirror.removed.length > 0) this.assignments.reconcile(tripId, socketId); + + // A night is seated by its check-in, which renumbers the stops around it. The + // created/moved event alone puts the row at the end of the day on every other + // screen, so the day that changed sends its order along. + for (const dayId of this.touchedDays(mirror)) { + const orderedIds = this.db.all<{ id: number }>( + 'SELECT id FROM day_assignments WHERE day_id = ? ORDER BY order_index', dayId).map(row => row.id); + send('assignment:reordered', { dayId, orderedIds }); + } + + if (mirror.created || mirror.moved || mirror.removed.length > 0) this.assignments.reconcile(tripId, socketId); + } + + /** Days whose stop order this write can have changed, each named once. */ + private touchedDays(mirror: AccommodationMirror): number[] { + const days = new Set(); + if (mirror.created) days.add(mirror.created.day_id); + if (mirror.moved) { days.add(mirror.moved.assignment.day_id); days.add(mirror.moved.oldDayId); } + for (const stop of mirror.removed) days.add(stop.dayId); + return [...days]; } // ------------------------------------------------------------------------- @@ -241,13 +272,15 @@ export class AccommodationsService { * its position from the chain, not from the hour, so pushing past it would move a * stop the traveller placed deliberately. */ - private positionForCheckIn(dayId: number, checkIn: string | null | undefined): number | undefined { + private positionForCheckIn(dayId: number, checkIn: string | null | undefined, excludeId?: number): number | undefined { if (!checkIn) return undefined; + // excludeId leaves the row being re-seated out of the chain it is measured + // against. Without it a night parked at the end of the day can find itself. const rows = this.db.all<{ order_index: number; at: string | null }>(` SELECT da.order_index, COALESCE(da.assignment_time, p.place_time) AS at FROM day_assignments da JOIN places p ON p.id = da.place_id - WHERE da.day_id = ? ORDER BY da.order_index - `, dayId); + WHERE da.day_id = ? AND da.id != ? ORDER BY da.order_index + `, dayId, excludeId ?? -1); const later = rows.find(row => row.at !== null && row.at > checkIn); return later?.order_index; } @@ -275,22 +308,80 @@ export class AccommodationsService { return !laterAhead && !earlierBehind; } + /** + * Type the place as lodging, unless the traveller already typed it themselves. + * + * 'hotel' is a service stop: it takes no number, stays out of the day's stop + * count and falls under the existing "show service stops in Days" switch. Without + * the stamp a booked night made here would look nothing like one booked in the + * road trip. Only ever filled in when it is empty: a type the traveller picked + * (a campsite, say) is theirs. + */ + private stampLodging(placeId: number): PlaceWithTags | null { + const place = this.db.get<{ stop_type: string | null }>('SELECT stop_type FROM places WHERE id = ?', placeId); + if (!place || place.stop_type) return null; + this.db.run("UPDATE places SET stop_type = 'hotel' WHERE id = ?", placeId); + return this.db.getPlaceWithTags(placeId); + } + + /** + * Carry the booking's own stop to where the booking now is, in place. + * + * A day stop is more than a (day, place) pair. Its participants and any road-trip + * day boundary hang off its id by ON DELETE CASCADE, and its note, its hour and its + * end-of-day flag live in its own columns. Deleting the row and inserting a fresh + * one loses every bit of that, and correcting a booking's date is not a request to + * strip the stop the traveller built on it. + * + * Null when the row cannot simply move, because the target day already holds that + * place under a stop of its own: then ours has to go rather than stand beside it. + * + * Runs inside the caller's transaction. + */ + private relocateOwnStop( + stop: { id: number; day_id: number; order_index: number }, + placeId: number, + dayId: number, + checkIn?: string | null, + ): MirroredAssignment | null { + if (this.db.get('SELECT id FROM day_assignments WHERE day_id = ? AND place_id = ? AND id != ?', dayId, placeId, stop.id)) { + return null; + } + + // Close the gap the row leaves behind. A no-op when it is not changing days, + // because it is put back into that same numbering two statements down. + this.db.run( + 'UPDATE day_assignments SET order_index = order_index - 1 WHERE day_id = ? AND order_index > ?', + stop.day_id, stop.order_index, + ); + + // Park it at the end of the target day first, then seat it by the check-in the + // same way a fresh insert would. Two steps, because the index it should get is + // read off the chain it is not part of yet. + const max = this.db.get<{ max: number | null }>( + 'SELECT MAX(order_index) AS max FROM day_assignments WHERE day_id = ? AND id != ?', dayId, stop.id)!; + const end = (max.max !== null ? max.max : -1) + 1; + this.db.run('UPDATE day_assignments SET day_id = ?, place_id = ?, order_index = ? WHERE id = ?', dayId, placeId, end, stop.id); + + const seat = this.positionForCheckIn(dayId, checkIn, stop.id); + if (seat !== undefined && seat < end) { + this.db.run( + 'UPDATE day_assignments SET order_index = order_index + 1 WHERE day_id = ? AND order_index >= ? AND id != ?', + dayId, seat, stop.id, + ); + this.db.run('UPDATE day_assignments SET order_index = ? WHERE id = ?', seat, stop.id); + } + + return this.assignments.getAssignmentWithPlace(stop.id); + } + private mirrorStay(accommodationId: number, placeId: number | null, dayId: number, checkIn?: string | null): AccommodationMirror { const mirror = noMirror(); // A stay can outlive its place (place_id is ON DELETE SET NULL) and the booking // form writes stays that never had one. Nothing to put on the map then. if (!placeId) return mirror; - // 'hotel' is a service stop: it takes no number, stays out of the day's stop - // count and falls under the existing "show service stops in Days" switch. Without - // the stamp a booked night made here would look nothing like one booked in the - // road trip. Only ever filled in when it is empty: a type the traveller picked - // (a campsite, say) is theirs. - const place = this.db.get<{ stop_type: string | null }>('SELECT stop_type FROM places WHERE id = ?', placeId); - if (place && !place.stop_type) { - this.db.run("UPDATE places SET stop_type = 'hotel' WHERE id = ?", placeId); - mirror.stamped = this.db.getPlaceWithTags(placeId); - } + mirror.stamped = this.stampLodging(placeId); // The road-trip flow assigns the place to the day and only then books the night. // Claiming that row would make cancelling the booking delete a stop the traveller @@ -340,6 +431,11 @@ export class AccommodationsService { for (const stop of this.ownStops(accommodationId)) { if (opts.keepStop) { this.db.run('UPDATE day_assignments SET accommodation_id = NULL WHERE id = ?', stop.id); + // The stop stays, but it is the traveller's now. Days hides a stop whose + // accommodation_id is set, so a client left holding the old row keeps the + // place invisible on a day it is standing on. + const released = this.assignments.getAssignmentWithPlace(stop.id); + if (released) mirror.updated.push(released); continue; } this.db.run('DELETE FROM day_assignments WHERE id = ?', stop.id); @@ -371,6 +467,20 @@ export class AccommodationsService { } const mirror = noMirror(); + + // One stop is the ordinary case, and it can be carried across rather than + // rebuilt. Everything hanging off the row survives that: its participants, its + // note, its hour, its end-of-day flag and the road-trip day boundary anchored + // on its id, all of which a DELETE takes with it. + if (own.length === 1 && placeId) { + const moved = this.relocateOwnStop(own[0], placeId, dayId, checkIn); + if (moved) { + mirror.moved = { assignment: moved, oldDayId: own[0].day_id }; + mirror.stamped = this.stampLodging(placeId); + return mirror; + } + } + for (const stop of own) { this.db.run('DELETE FROM day_assignments WHERE id = ?', stop.id); mirror.removed.push({ id: stop.id, dayId: stop.day_id }); diff --git a/server/src/nest/assignments/assignments.service.ts b/server/src/nest/assignments/assignments.service.ts index 5e534aba3..b0f7d516c 100644 --- a/server/src/nest/assignments/assignments.service.ts +++ b/server/src/nest/assignments/assignments.service.ts @@ -53,7 +53,12 @@ export class AssignmentsService { try { this.journey.reconcileTripSkeletons(Number(tripId), socketId); } catch { /* non-fatal */ } } - private getAssignmentWithPlace(assignmentId: number | bigint) { + /** + * One stop, shaped the way every assignment event and REST answer carries it. + * Public because the accommodation mirror moves a stop in place and has to hand + * the moved row back in exactly this shape. + */ + getAssignmentWithPlace(assignmentId: number | bigint) { const a = this.dbs.get(` SELECT da.*, p.id as place_id, p.name as place_name, p.description as place_description, p.lat, p.lng, p.address, p.category_id, p.price, p.currency as place_currency, diff --git a/server/src/nest/places/places.controller.ts b/server/src/nest/places/places.controller.ts index 7b16818f3..e151c5fce 100644 --- a/server/src/nest/places/places.controller.ts +++ b/server/src/nest/places/places.controller.ts @@ -321,11 +321,17 @@ export class PlacesController { for (const id of scoped) this.places.onDeleted(id); // Read the linked expenses before the delete — afterwards the link is gone (#1298). const expenseIds = this.places.linkedExpenseIds(tripId, scoped); - const deleted = await this.places.removeMany(tripId, ids); + const { deleted, cancelled } = await this.places.removeMany(tripId, ids); for (const id of deleted) { this.places.broadcast(tripId, 'place:deleted', { placeId: id }, socketId); } - for (const itemId of expenseIds) { + // A night booked at this place went with it, and took its partner booking and + // that booking's expense along. Neither is covered by place:deleted, and an + // expense linked by reservation_id is not one linkedExpenseIds finds. + for (const reservationId of cancelled.reservationIds) { + this.places.broadcast(tripId, 'reservation:deleted', { reservationId }, socketId); + } + for (const itemId of [...expenseIds, ...cancelled.budgetItemIds]) { this.places.broadcast(tripId, 'budget:deleted', { itemId }, socketId); } return { deleted, count: deleted.length }; @@ -489,11 +495,18 @@ export class PlacesController { } this.places.onDeleted(Number(id)); const expenseIds = this.places.linkedExpenseIds(tripId, [id]); - if (!(await this.places.remove(tripId, id))) { + const { deleted, cancelled } = await this.places.remove(tripId, id); + if (!deleted) { throw new HttpException({ error: 'Place not found' }, 404); } this.places.broadcast(tripId, 'place:deleted', { placeId: Number(id) }, socketId); - for (const itemId of expenseIds) { + // A night booked at this place went with it, and took its partner booking and + // that booking's expense along. Neither is covered by place:deleted, and an + // expense linked by reservation_id is not one linkedExpenseIds finds. + for (const reservationId of cancelled.reservationIds) { + this.places.broadcast(tripId, 'reservation:deleted', { reservationId }, socketId); + } + for (const itemId of [...expenseIds, ...cancelled.budgetItemIds]) { this.places.broadcast(tripId, 'budget:deleted', { itemId }, socketId); } return { success: true }; diff --git a/server/src/nest/places/places.mcp.ts b/server/src/nest/places/places.mcp.ts index 2672b6c64..531a2939e 100644 --- a/server/src/nest/places/places.mcp.ts +++ b/server/src/nest/places/places.mcp.ts @@ -258,10 +258,14 @@ export class PlacesMcp { try { this.journey.onPlaceDeleted(placeId); } catch { /* non-fatal */ } // sync journeys before the row is gone // The link is gone once the place is, so read it first (#1298). const expenseIds = this.places.linkedExpenseIds(tripId, [placeId]); - const deleted = await this.places.remove(String(tripId), String(placeId)); + const { deleted, cancelled } = await this.places.remove(String(tripId), String(placeId)); if (!deleted) return { content: [{ type: 'text' as const, text: 'Place not found.' }], isError: true }; this.guards.safeBroadcast(tripId, 'place:deleted', { placeId }); - for (const itemId of expenseIds) this.guards.safeBroadcast(tripId, 'budget:deleted', { itemId }); + // A night booked at this place went with it, and took its partner booking and + // that booking's expense along. Neither is covered by place:deleted, and an + // expense linked by reservation_id is not one linkedExpenseIds finds. + for (const reservationId of cancelled.reservationIds) this.guards.safeBroadcast(tripId, 'reservation:deleted', { reservationId }); + for (const itemId of [...expenseIds, ...cancelled.budgetItemIds]) this.guards.safeBroadcast(tripId, 'budget:deleted', { itemId }); return ok({ success: true }); } @@ -431,9 +435,13 @@ export class PlacesMcp { } // The link is gone once the places are, so read it first (#1298). const expenseIds = this.places.linkedExpenseIds(tripId, scoped); - const deleted = await this.places.removeMany(String(tripId), placeIds); + const { deleted, cancelled } = await this.places.removeMany(String(tripId), placeIds); for (const id of deleted) this.guards.safeBroadcast(tripId, 'place:deleted', { placeId: id }); - for (const itemId of expenseIds) this.guards.safeBroadcast(tripId, 'budget:deleted', { itemId }); + // A night booked at this place went with it, and took its partner booking and + // that booking's expense along. Neither is covered by place:deleted, and an + // expense linked by reservation_id is not one linkedExpenseIds finds. + for (const reservationId of cancelled.reservationIds) this.guards.safeBroadcast(tripId, 'reservation:deleted', { reservationId }); + for (const itemId of [...expenseIds, ...cancelled.budgetItemIds]) this.guards.safeBroadcast(tripId, 'budget:deleted', { itemId }); return ok({ deleted, count: deleted.length }); } diff --git a/server/src/nest/places/places.rpc.ts b/server/src/nest/places/places.rpc.ts index 3eb831a77..ecb1710d8 100644 --- a/server/src/nest/places/places.rpc.ts +++ b/server/src/nest/places/places.rpc.ts @@ -105,12 +105,17 @@ export class PlacesRpc { // The link is gone once the place is, so read it first (#1298). const expenseIds = this.places.linkedExpenseIds(tripId, [placeId]); // remove is async (it deletes the place's storage object): await it so the - // refusal check sees the resolved boolean, not an always-truthy Promise. - if (!(await this.places.remove(String(tripId), String(placeId)))) { + // refusal check sees the resolved answer, not an always-truthy Promise. + const { deleted, cancelled } = await this.places.remove(String(tripId), String(placeId)); + if (!deleted) { throw new ForbiddenResource(`no place ${placeId} on trip ${tripId}`); } this.realtime.broadcast(tripId, 'place:deleted', { placeId }); - for (const itemId of expenseIds) this.realtime.broadcast(tripId, 'budget:deleted', { itemId }); + // A night booked at this place went with it, and took its partner booking and + // that booking's expense along. Neither is covered by place:deleted, and an + // expense linked by reservation_id is not one linkedExpenseIds finds. + for (const reservationId of cancelled.reservationIds) this.realtime.broadcast(tripId, 'reservation:deleted', { reservationId }); + for (const itemId of [...expenseIds, ...cancelled.budgetItemIds]) this.realtime.broadcast(tripId, 'budget:deleted', { itemId }); return { deleted: true }; } diff --git a/server/src/nest/places/places.service.ts b/server/src/nest/places/places.service.ts index d39fe94ac..8aa0752b4 100644 --- a/server/src/nest/places/places.service.ts +++ b/server/src/nest/places/places.service.ts @@ -29,6 +29,14 @@ import { reclaimPlaceImage } from './place-image'; import { JourneyDomainService } from '../journey/journey-domain.service'; import { StorageService } from '../storage/storage.service'; import { AccommodationsService } from '../accommodations/accommodations.service'; + +/** Rows a place delete took down with the nights booked there, for the caller to announce. */ +export interface CancelledStays { + reservationIds: number[]; + budgetItemIds: number[]; +} + +const noCancelledStays = (): CancelledStays => ({ reservationIds: [], budgetItemIds: [] }); import { ENRICH_CONCURRENCY, ADDRESS_BACKFILL_MAX_PLACES, @@ -439,33 +447,43 @@ export class PlacesService { * * Runs inside the caller's transaction. */ - private cancelStaysAt(tripId: string | number, placeId: string | number): void { + private cancelStaysAt(tripId: string | number, placeId: string | number, into: CancelledStays): void { const stays = this.dbs.all<{ id: number }>( 'SELECT id FROM day_accommodations WHERE trip_id = ? AND place_id = ?', tripId, placeId, ); - for (const stay of stays) this.accommodations.deleteAccommodation(stay.id); + for (const stay of stays) { + const gone = this.accommodations.deleteAccommodation(stay.id); + // What went down with the night is what the caller has to announce. The + // partner booking and its expense are rows the Bookings list and the Costs + // total are still holding; place:deleted says nothing about either, and a + // budget item linked by reservation_id is not found by linkedExpenseIds. + into.reservationIds.push(...gone.linkedReservationIds); + into.budgetItemIds.push(...gone.deletedBudgetItemIds); + } } - async remove(tripId: string, placeId: string): Promise { + async remove(tripId: string, placeId: string): Promise<{ deleted: boolean; cancelled: CancelledStays }> { const place = this.dbs.get<{ google_place_id: string | null; image_url: string | null }>( 'SELECT google_place_id, image_url FROM places WHERE id = ? AND trip_id = ?', placeId, tripId, ); - if (!place) return false; + const cancelled = noCancelledStays(); + if (!place) return { deleted: false, cancelled }; // The linked expense goes with the place, the same way a booking takes its // expense with it (#1298). One transaction, so a place can never survive // half-detached from its money. this.dbs.transaction(() => { - this.cancelStaysAt(tripId, placeId); + this.cancelStaysAt(tripId, placeId, cancelled); this.dbs.run('DELETE FROM budget_items WHERE trip_id = ? AND place_id = ?', tripId, placeId); this.dbs.run('DELETE FROM places WHERE id = ?', placeId); }); await reclaimPhotoCache(this.photoCache, place.google_place_id, place.image_url); await reclaimPlaceImage(this.storage, place.image_url); - return true; + return { deleted: true, cancelled }; } - async removeMany(tripId: string, ids: number[]): Promise { - if (ids.length === 0) return []; + async removeMany(tripId: string, ids: number[]): Promise<{ deleted: number[]; cancelled: CancelledStays }> { + const cancelled = noCancelledStays(); + if (ids.length === 0) return { deleted: [], cancelled }; const selectStmt = this.dbs.prepare('SELECT google_place_id, image_url FROM places WHERE id = ? AND trip_id = ?'); const deleteStmt = this.dbs.prepare('DELETE FROM places WHERE id = ?'); const deleteExpenseStmt = this.dbs.prepare('DELETE FROM budget_items WHERE trip_id = ? AND place_id = ?'); @@ -475,7 +493,7 @@ export class PlacesService { for (const id of ids) { const row = selectStmt.get(id, tripId) as { google_place_id: string | null; image_url: string | null } | undefined; if (!row) continue; - this.cancelStaysAt(tripId, id); + this.cancelStaysAt(tripId, id, cancelled); deleteExpenseStmt.run(tripId, id); deleteStmt.run(id); deleted.push(id); @@ -487,7 +505,7 @@ export class PlacesService { await reclaimPhotoCache(this.photoCache, row.google_place_id, row.image_url); await reclaimPlaceImage(this.storage, row.image_url); } - return deleted; + return { deleted, cancelled }; } /** diff --git a/server/src/nest/trips/trips.service.ts b/server/src/nest/trips/trips.service.ts index 0de1d230d..92ab51ed5 100644 --- a/server/src/nest/trips/trips.service.ts +++ b/server/src/nest/trips/trips.service.ts @@ -552,15 +552,15 @@ export class TripsService { INSERT INTO places (trip_id, name, description, lat, lng, address, category_id, price, currency, reservation_status, reservation_notes, reservation_datetime, place_time, end_time, duration_minutes, notes, image_url, google_place_id, google_ftid, website, phone, transport_mode, osm_id, - amap_poi_id, route_geometry, route_color, stop_type) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + amap_poi_id, route_geometry, route_color, stop_type, fill_percent) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) `); for (const p of oldPlaces) { const r = insertPlace.run(newTripId, p.name, p.description, p.lat, p.lng, p.address, p.category_id, p.price, p.currency, p.reservation_status, p.reservation_notes, p.reservation_datetime, p.place_time, p.end_time, p.duration_minutes, p.notes, p.image_url, p.google_place_id, p.google_ftid, p.website, p.phone, p.transport_mode, p.osm_id, p.amap_poi_id, p.route_geometry, - p.route_color, p.stop_type); + p.route_color, p.stop_type, p.fill_percent); placeMap.set(p.id, r.lastInsertRowid); } @@ -650,19 +650,31 @@ export class TripsService { const oldAccom = this.db.prepare('SELECT * FROM day_accommodations WHERE trip_id = ?').all(sourceTripId) as any[]; const accomMap = new Map(); const insertAccom = this.db.prepare(` - INSERT INTO day_accommodations (trip_id, place_id, start_day_id, end_day_id, check_in, check_out, confirmation, notes) - VALUES (?, ?, ?, ?, ?, ?, ?, ?) + INSERT INTO day_accommodations (trip_id, place_id, start_day_id, end_day_id, check_in, check_in_end, check_out, confirmation, notes) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) `); for (const a of oldAccom) { const newPlaceId = placeMap.get(a.place_id); const newStartDay = dayMap.get(a.start_day_id); const newEndDay = dayMap.get(a.end_day_id); if (newPlaceId && newStartDay && newEndDay) { - const r = insertAccom.run(newTripId, newPlaceId, newStartDay, newEndDay, a.check_in, a.check_out, a.confirmation, a.notes); + const r = insertAccom.run(newTripId, newPlaceId, newStartDay, newEndDay, a.check_in, a.check_in_end, a.check_out, a.confirmation, a.notes); accomMap.set(a.id, r.lastInsertRowid); } } + // A booked night's stop carries the booking that put it there. Left blank, the + // copy draws the hotel twice: once as the stop and once as the overnight block, + // which is the duplicate the mirror exists to remove. Stamped afterwards rather + // than at insert time, because the bookings are copied after the stops. + const stampCopiedStop = this.db.prepare('UPDATE day_assignments SET accommodation_id = ? WHERE id = ?'); + for (const a of oldAssignments) { + if (!a.accommodation_id) continue; + const newAssignmentId = assignmentMap.get(a.id); + const newAccomId = accomMap.get(a.accommodation_id); + if (newAssignmentId && newAccomId) stampCopiedStop.run(newAccomId, newAssignmentId); + } + const oldReservations = this.db.prepare('SELECT * FROM reservations WHERE trip_id = ?').all(sourceTripId) as any[]; // The external_* / sync_enabled columns are deliberately not copied: the // duplicate must not inherit the source's external sync identity. diff --git a/server/tests/unit/nest/accommodations.service.test.ts b/server/tests/unit/nest/accommodations.service.test.ts index 243a6ece7..961224818 100644 --- a/server/tests/unit/nest/accommodations.service.test.ts +++ b/server/tests/unit/nest/accommodations.service.test.ts @@ -342,7 +342,7 @@ describe('deleteAccommodation', () => { deletedBudgetItemId: budgetItemId, linkedReservationIds: [reservation.id], deletedBudgetItemIds: [budgetItemId], - mirror: { created: null, removed: [{ id: expect.any(Number), dayId: day.id }], stamped: null }, + mirror: { created: null, moved: null, updated: [], removed: [{ id: expect.any(Number), dayId: day.id }], stamped: null }, }); expect(testDb.prepare('SELECT id FROM budget_items WHERE id = ?').get(budgetItemId)).toBeUndefined(); }); @@ -503,7 +503,7 @@ describe('route-facing delegators', () => { deletedBudgetItemId: null, linkedReservationIds: [reservation.id], deletedBudgetItemIds: [], - mirror: { created: null, removed: [{ id: expect.any(Number), dayId: day.id }], stamped: null }, + mirror: { created: null, moved: null, updated: [], removed: [{ id: expect.any(Number), dayId: day.id }], stamped: null }, }); expect(svc.get(accom.id, trip.id)).toBeUndefined(); }); @@ -762,7 +762,7 @@ describe('the day stop a booking implies', () => { const { mirror } = book(trip.id, null, day.id, day.id); - expect(mirror).toEqual({ created: null, removed: [], stamped: null }); + expect(mirror).toEqual({ created: null, moved: null, updated: [], removed: [], stamped: null }); expect(stopsOn(day.id)).toHaveLength(0); }); @@ -779,9 +779,84 @@ describe('the day stop a booking implies', () => { const { mirror } = svc.updateAccommodation(accommodation.id, existing, { start_day_id: day2.id, end_day_id: day2.id }) as any; expect(stopsOn(day1.id)).toHaveLength(0); - expect(stopsOn(day2.id)).toEqual([expect.objectContaining({ place_id: place.id, accommodation_id: accommodation.id })]); - expect(mirror.removed).toEqual([{ id: before.id, dayId: day1.id }]); - expect(mirror.created).toMatchObject({ day_id: day2.id }); + // The same row, carried over. Not a delete and a fresh insert: everything the + // traveller hung on this stop is keyed by its id. + expect(stopsOn(day2.id)).toEqual([expect.objectContaining({ id: before.id, place_id: place.id, accommodation_id: accommodation.id })]); + expect(mirror.removed).toEqual([]); + expect(mirror.created).toBeNull(); + expect(mirror.moved).toMatchObject({ oldDayId: day1.id, assignment: { id: before.id, day_id: day2.id } }); + }); + + it('ACC-028b moving the booking keeps the note, the hour and the end-of-day flag on the stop', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day1 = createDay(testDb, trip.id); + const day2 = createDay(testDb, trip.id); + const place = createPlace(testDb, trip.id, { name: 'Hotel Adlon' }); + const { accommodation } = book(trip.id, place.id, day1.id, day1.id); + const stop = stopsOn(day1.id)[0]; + testDb.prepare('UPDATE day_assignments SET notes = ?, assignment_time = ?, end_day = 1 WHERE id = ?') + .run('ask for the quiet side', '15:30', stop.id); + + const existing = svc.getAccommodation(accommodation.id, trip.id)!; + svc.updateAccommodation(accommodation.id, existing, { start_day_id: day2.id, end_day_id: day2.id }); + + const after = testDb.prepare('SELECT * FROM day_assignments WHERE id = ?').get(stop.id) as Record; + expect(after).toMatchObject({ day_id: day2.id, notes: 'ask for the quiet side', assignment_time: '15:30', end_day: 1 }); + }); + + it('ACC-028c moving the booking keeps the people assigned to the stop', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day1 = createDay(testDb, trip.id); + const day2 = createDay(testDb, trip.id); + const place = createPlace(testDb, trip.id, { name: 'Hotel Adlon' }); + const { accommodation } = book(trip.id, place.id, day1.id, day1.id); + const stop = stopsOn(day1.id)[0]; + // assignment_participants.assignment_id is ON DELETE CASCADE, so a delete and + // re-insert would take these with it without a word. + testDb.prepare('INSERT INTO assignment_participants (assignment_id, user_id) VALUES (?, ?)').run(stop.id, user.id); + + const existing = svc.getAccommodation(accommodation.id, trip.id)!; + svc.updateAccommodation(accommodation.id, existing, { start_day_id: day2.id, end_day_id: day2.id }); + + const kept = testDb.prepare('SELECT user_id FROM assignment_participants WHERE assignment_id = ?').all(stop.id); + expect(kept).toEqual([{ user_id: user.id }]); + }); + + it('ACC-028d a move onto a day that already holds the place by hand drops the booking stop instead of doubling it', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day1 = createDay(testDb, trip.id); + const day2 = createDay(testDb, trip.id); + const place = createPlace(testDb, trip.id, { name: 'Hotel Adlon' }); + const { accommodation } = book(trip.id, place.id, day1.id, day1.id); + const own = stopsOn(day1.id)[0]; + const byHand = createDayAssignment(testDb, day2.id, place.id) as { id: number }; + + const existing = svc.getAccommodation(accommodation.id, trip.id)!; + const { mirror } = svc.updateAccommodation(accommodation.id, existing, { start_day_id: day2.id, end_day_id: day2.id }) as any; + + expect(stopsOn(day1.id)).toHaveLength(0); + expect(stopsOn(day2.id).map((row: { id: number }) => row.id)).toEqual([byHand.id]); + expect(mirror.removed).toEqual([{ id: own.id, dayId: day1.id }]); + expect(mirror.moved).toBeNull(); + }); + + it('ACC-028e the day the stop left closes the gap it made in the order', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day1 = createDay(testDb, trip.id); + const day2 = createDay(testDb, trip.id); + const hotel = createPlace(testDb, trip.id, { name: 'Hotel Adlon' }); + const museum = createPlace(testDb, trip.id, { name: 'Pergamon' }); + const { accommodation } = book(trip.id, hotel.id, day1.id, day1.id); + createDayAssignment(testDb, day1.id, museum.id); + + const existing = svc.getAccommodation(accommodation.id, trip.id)!; + svc.updateAccommodation(accommodation.id, existing, { start_day_id: day2.id, end_day_id: day2.id }); + + expect(stopsOn(day1.id).map((row: { order_index: number }) => row.order_index)).toEqual([0]); }); it('ACC-029 editing an unrelated field leaves the stop exactly where it is', () => { @@ -796,7 +871,7 @@ describe('the day stop a booking implies', () => { const { mirror } = svc.updateAccommodation(accommodation.id, existing, { check_in: '16:00' }) as any; expect(stopsOn(day.id)).toEqual(before); - expect(mirror).toEqual({ created: null, removed: [], stamped: null }); + expect(mirror).toEqual({ created: null, moved: null, updated: [], removed: [], stamped: null }); }); it('ACC-030 a stay whose stop belongs to the traveller does not get a second one', () => { @@ -887,10 +962,47 @@ describe('the day stop a booking implies', () => { const sent: string[] = []; svc.announceMirror(5, { created: { id: 78, day_id: 11 } as never, + moved: null, + updated: [], removed: [{ id: 77, dayId: 10 }], stamped: { id: 3 } as never, }, event => { sent.push(event); }); - expect(sent).toEqual(['assignment:deleted', 'assignment:created', 'place:updated']); + // The two days whose order changed follow, arrival first (see touchedDays). + expect(sent).toEqual([ + 'assignment:deleted', 'assignment:created', 'place:updated', + 'assignment:reordered', 'assignment:reordered', + ]); + }); + + it('ACC-033b announceMirror reports a move as one event, not a delete and a create', () => { + const sent: Array<{ event: string; payload: unknown }> = []; + svc.announceMirror(5, { + created: null, + moved: { assignment: { id: 78, day_id: 11 } as never, oldDayId: 10 }, + updated: [], + removed: [], + stamped: null, + }, (event, payload) => { sent.push({ event, payload }); }); + expect(sent.map(e => e.event)).toEqual(['assignment:moved', 'assignment:reordered', 'assignment:reordered']); + expect(sent[0].payload).toMatchObject({ oldDayId: 10, newDayId: 11 }); + }); + + it('ACC-033c a kept stop is announced as updated, so the day list stops hiding it', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id); + const place = createPlace(testDb, trip.id, { name: 'Hotel Adlon' }); + const { accommodation } = book(trip.id, place.id, day.id, day.id); + const stop = stopsOn(day.id)[0]; + + const { mirror } = svc.remove(accommodation.id, { keepStop: true }) as any; + + // Days hides a stop that carries an accommodation_id, so a client still holding + // the old row keeps the place invisible on a day it is standing on. + expect(mirror.updated).toEqual([expect.objectContaining({ id: stop.id, accommodation_id: null })]); + const sent: string[] = []; + svc.announceMirror(trip.id, mirror, event => { sent.push(event); }); + expect(sent).toContain('assignment:updated'); }); }); diff --git a/server/tests/unit/nest/places.service.test.ts b/server/tests/unit/nest/places.service.test.ts index 8b410cd2e..2462f38c2 100644 --- a/server/tests/unit/nest/places.service.test.ts +++ b/server/tests/unit/nest/places.service.test.ts @@ -406,14 +406,14 @@ describe('remove', () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const place = createPlace(testDb, trip.id, { name: 'To Delete' }) as any; - expect(await svc.remove(String(trip.id), String(place.id))).toBe(true); + expect((await svc.remove(String(trip.id), String(place.id))).deleted).toBe(true); expect(svc.get(String(trip.id), String(place.id))).toBeNull(); }); it('PLACE-SVC-018 — returns false for non-existent place', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); - expect(await svc.remove(String(trip.id), '99999')).toBe(false); + expect((await svc.remove(String(trip.id), '99999')).deleted).toBe(false); }); it('PLACE-SVC-019 — deleting one place does not remove others', async () => { @@ -474,7 +474,7 @@ describe('remove', () => { // Read the link before the delete — that is what the controller broadcasts. expect(svc.linkedExpenseIds(trip.id, [place.id])).toEqual([linked]); - expect(await svc.remove(String(trip.id), String(place.id))).toBe(true); + expect((await svc.remove(String(trip.id), String(place.id))).deleted).toBe(true); const rows = testDb.prepare('SELECT id FROM budget_items ORDER BY id').all() as { id: number }[]; expect(rows.map(r => r.id)).toEqual([untouched, standalone]); @@ -532,16 +532,41 @@ describe('removeMany', () => { const b = createPlace(testDb, trip.id, { name: 'B' }) as any; const foreign = createPlace(testDb, other.id, { name: 'Foreign' }) as any; - const deleted = await svc.removeMany(String(trip.id), [a.id, b.id, foreign.id, 99999]); + const { deleted } = await svc.removeMany(String(trip.id), [a.id, b.id, foreign.id, 99999]); expect(deleted.sort()).toEqual([a.id, b.id].sort()); expect(svc.get(String(other.id), String(foreign.id))).not.toBeNull(); }); + it('PLACE-SVC-057b — a place delete reports the booking and expense its cancelled night took down', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id) as any; + const hotel = createPlace(testDb, trip.id, { name: 'Hotel Adlon' }) as any; + // Booked through the accommodations domain, so it gets its partner hotel + // reservation the way the booking form writes one. + const { accommodation } = accommodations.createAccommodation(trip.id, { + place_id: hotel.id, start_day_id: day.id, end_day_id: day.id, + }) as { accommodation: { id: number } }; + const reservation = testDb.prepare('SELECT id FROM reservations WHERE accommodation_id = ?').get(accommodation.id) as { id: number }; + // An expense hung off the reservation rather than the place: linkedExpenseIds + // selects on budget_items.place_id and never finds this one. + const itemId = Number(testDb.prepare( + "INSERT INTO budget_items (trip_id, name, total_price, reservation_id) VALUES (?, 'Hotel stay', 240, ?)" + ).run(trip.id, reservation.id).lastInsertRowid); + + const { deleted, cancelled } = await svc.remove(String(trip.id), String(hotel.id)); + + expect(deleted).toBe(true); + expect(cancelled.reservationIds).toEqual([reservation.id]); + expect(cancelled.budgetItemIds).toEqual([itemId]); + expect(testDb.prepare('SELECT id FROM budget_items WHERE id = ?').get(itemId)).toBeUndefined(); + }); + it('PLACE-SVC-057 — returns [] for an empty id list', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); - expect(await svc.removeMany(String(trip.id), [])).toEqual([]); + expect((await svc.removeMany(String(trip.id), [])).deleted).toEqual([]); }); });