From 4436b6f6735a58a17bb5d582c027492133ce8698 Mon Sep 17 00:00:00 2001 From: "Julien G." <66769052+jubnl@users.noreply.github.com> Date: Thu, 23 Apr 2026 10:53:32 +0200 Subject: [PATCH] fix(journey,pdf): journey reorder sort_order + PDF multi-day transport (#848) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(journey): make sort_order authoritative for within-day entry ordering Reorder buttons appeared broken because the server ORDER BY put entry_time before sort_order, so entries synced from trip places with differing times would always sort by time regardless of sort_order writes. The client store mirrored the same comparator, making even the optimistic update invisible. - Change ORDER BY to (entry_date, sort_order, id) in getJourneyFull and listEntries - Fix syncTripPlaces and onPlaceCreated to assign MAX+1 sort_order per day instead of day_number/0 - Update client store comparator to match - Add DB migration to backfill sort_order using old effective key (entry_time, id) so existing journeys retain their visual order - Add tests: JOURNEY-SVC-089–093, FE-STORE-JOURNEY-018–019 Closes #846 * fix(pdf): include multi-day transport return/arrival in PDF itinerary (#847) Reservations were matched to days by pickup date only, so the end-day card (e.g. car Return, flight Arrival) was silently dropped from the PDF. Add span-aware helpers mirroring DayPlanSidebar logic: match by day_id/end_day_id span, show reservation_end_time on end days, prefix title with phase label (Return/Arrival/etc.), and use per-day position for sort order. * test(pdf): add missing day_id to transport reservation fixture --- client/src/components/PDF/TripPDF.test.ts | 1 + client/src/components/PDF/TripPDF.tsx | 55 +++++++-- client/src/store/journeyStore.test.ts | 31 +++++ client/src/store/journeyStore.ts | 6 +- server/src/db/migrations.ts | 23 ++++ server/src/services/journeyService.ts | 23 +++- .../unit/services/journeyService.test.ts | 106 ++++++++++++++++++ 7 files changed, 228 insertions(+), 17 deletions(-) diff --git a/client/src/components/PDF/TripPDF.test.ts b/client/src/components/PDF/TripPDF.test.ts index 46549188..77e33eac 100644 --- a/client/src/components/PDF/TripPDF.test.ts +++ b/client/src/components/PDF/TripPDF.test.ts @@ -78,6 +78,7 @@ const transportReservation = { id: 400, title: 'Flight to Rome', type: 'flight', + day_id: 10, reservation_time: '2025-06-01T14:30:00', confirmation_number: 'ABC123', metadata: JSON.stringify({ airline: 'Air Italia', flight_number: 'AI123', departure_airport: 'CDG', arrival_airport: 'FCO' }), diff --git a/client/src/components/PDF/TripPDF.tsx b/client/src/components/PDF/TripPDF.tsx index 040bb711..1a5a3316 100644 --- a/client/src/components/PDF/TripPDF.tsx +++ b/client/src/components/PDF/TripPDF.tsx @@ -140,23 +140,58 @@ export async function downloadTripPDF({ trip, days, places, assignments, categor const totalCost = Object.values(assignments || {}) .flatMap(a => a).reduce((s, a) => s + (parseFloat(a.place?.price) || 0), 0) + // Span helpers for multi-day transport (mirrors DayPlanSidebar logic) + const pdfGetDayOrder = (d: Day) => d.day_number + const pdfGetSpanPhase = (r: any, dayId: number): 'single' | 'start' | 'middle' | 'end' => { + const startId = r.day_id + const endId = r.end_day_id ?? startId + if (!startId || startId === endId) return 'single' + if (dayId === startId) return 'start' + if (dayId === endId) return 'end' + return 'middle' + } + const pdfGetDisplayTime = (r: any, dayId: number): string | null => { + const phase = pdfGetSpanPhase(r, dayId) + if (phase === 'end') return r.reservation_end_time || null + if (phase === 'middle') return null + return r.reservation_time || null + } + const pdfGetSpanLabel = (r: any, phase: string): string | null => { + if (phase === 'single') return null + if (r.type === 'flight') return tr(`reservations.span.${phase === 'start' ? 'departure' : phase === 'end' ? 'arrival' : 'inTransit'}`) + if (r.type === 'car') return tr(`reservations.span.${phase === 'start' ? 'pickup' : phase === 'end' ? 'return' : 'active'}`) + return tr(`reservations.span.${phase === 'start' ? 'start' : phase === 'end' ? 'end' : 'ongoing'}`) + } + const pdfGetTransportForDay = (dayId: number) => (reservations || []).filter(r => { + if (r.type === 'hotel') return false + const startId = r.day_id + const endId = r.end_day_id ?? startId + if (startId == null) return false + if (endId !== startId) { + const startDay = sorted.find(d => d.id === startId) + const endDay = sorted.find(d => d.id === endId) + const thisDay = sorted.find(d => d.id === dayId) + if (!startDay || !endDay || !thisDay) return false + return pdfGetDayOrder(thisDay) >= pdfGetDayOrder(startDay) && pdfGetDayOrder(thisDay) <= pdfGetDayOrder(endDay) + } + return startId === dayId + }) + // Build day HTML const daysHtml = sorted.map((day, di) => { const assigned = assignments[String(day.id)] || [] const notes = (dayNotes || []).filter(n => n.day_id === day.id) const cost = dayCost(assignments, day.id, loc) - // Reservations for this day (hotel rendered via accommodations block) - const dayReservations = (reservations || []).filter(r => { - if (!r.reservation_time || r.type === 'hotel') return false - return day.date && r.reservation_time.split('T')[0] === day.date - }) + // Reservations for this day (hotel rendered via accommodations block; car middle-phase rendered in sidebar header only) + const dayReservations = pdfGetTransportForDay(day.id) + .filter(r => !(r.type === 'car' && pdfGetSpanPhase(r, day.id) === 'middle')) const merged = [] assigned.forEach(a => merged.push({ type: 'place', k: a.order_index ?? a.sort_order ?? 0, data: a })) notes.forEach(n => merged.push({ type: 'note', k: n.sort_order ?? 0, data: n })) dayReservations.forEach(r => { - const pos = r.day_plan_position ?? (merged.length > 0 ? Math.max(...merged.map(m => m.k)) + 0.5 : 0.5) + const pos = r.day_positions?.[day.id] ?? r.day_positions?.[String(day.id)] ?? r.day_plan_position ?? (merged.length > 0 ? Math.max(...merged.map(m => m.k)) + 0.5 : 0.5) merged.push({ type: 'reservation', k: pos, data: r }) }) merged.sort((a, b) => a.k - b.k) @@ -177,13 +212,17 @@ export async function downloadTripPDF({ trip, days, places, assignments, categor else if (r.type === 'event') subtitle = [meta.venue].filter(Boolean).join(' · ') else if (r.type === 'tour') subtitle = [meta.operator].filter(Boolean).join(' · ') const locationLine = r.location || meta.location || '' - const time = r.reservation_time?.includes('T') ? r.reservation_time.split('T')[1]?.substring(0, 5) : '' + const phase = pdfGetSpanPhase(r, day.id) + const spanLabel = pdfGetSpanLabel(r, phase) + const displayTime = pdfGetDisplayTime(r, day.id) + const time = displayTime?.includes('T') ? displayTime.split('T')[1]?.substring(0, 5) : '' + const titleHtml = `${spanLabel ? escHtml(spanLabel) + ': ' : ''}${escHtml(r.title)}` return `
${icon}
-
${escHtml(r.title)}${time ? ` ${time}` : ''}
+
${titleHtml}${time ? ` ${time}` : ''}
${subtitle ? `
${escHtml(subtitle)}
` : ''} ${locationLine ? `
${escHtml(locationLine)}
` : ''} ${r.confirmation_number ? `
Code: ${escHtml(r.confirmation_number)}
` : ''} diff --git a/client/src/store/journeyStore.test.ts b/client/src/store/journeyStore.test.ts index 564969ec..d5c7a2a3 100644 --- a/client/src/store/journeyStore.test.ts +++ b/client/src/store/journeyStore.test.ts @@ -355,6 +355,37 @@ describe('journeyStore', () => { expect(useJourneyStore.getState().loading).toBe(false); }); + // ── reorderEntries ─────────────────────────────────────────────────────── + + it('FE-STORE-JOURNEY-018: reorderEntries reorders by sort_order not entry_time', async () => { + const a = buildEntry({ id: 201, entry_date: '2026-04-01', entry_time: '09:00', sort_order: 0 }); + const b = buildEntry({ id: 202, entry_date: '2026-04-01', entry_time: '11:00', sort_order: 1 }); + const c = buildEntry({ id: 203, entry_date: '2026-04-01', entry_time: '14:00', sort_order: 2 }); + const detail = buildJourneyDetail({ id: 55, entries: [a, b, c] }); + useJourneyStore.setState({ current: detail }); + + server.use( + http.put('/api/journeys/55/entries/reorder', () => HttpResponse.json({ success: true })) + ); + await useJourneyStore.getState().reorderEntries(55, [202, 201, 203]); + const ids = useJourneyStore.getState().current?.entries.map(e => e.id); + expect(ids).toEqual([202, 201, 203]); + }); + + it('FE-STORE-JOURNEY-019: reorderEntries rolls back on API failure', async () => { + const a = buildEntry({ id: 211, entry_date: '2026-04-01', sort_order: 0 }); + const b = buildEntry({ id: 212, entry_date: '2026-04-01', sort_order: 1 }); + const detail = buildJourneyDetail({ id: 56, entries: [a, b] }); + useJourneyStore.setState({ current: detail }); + + server.use( + http.put('/api/journeys/56/entries/reorder', () => HttpResponse.json({}, { status: 403 })) + ); + await expect(useJourneyStore.getState().reorderEntries(56, [212, 211])).rejects.toBeTruthy(); + const ids = useJourneyStore.getState().current?.entries.map(e => e.id); + expect(ids).toEqual([211, 212]); + }); + // ── clear ──────────────────────────────────────────────────────────────── it('FE-STORE-JOURNEY-015: clear resets state', () => { diff --git a/client/src/store/journeyStore.ts b/client/src/store/journeyStore.ts index 47b75971..c2edfa69 100644 --- a/client/src/store/journeyStore.ts +++ b/client/src/store/journeyStore.ts @@ -223,10 +223,8 @@ export const useJourneyStore = create((set, get) => ({ ) entries.sort((a, b) => { if (a.entry_date !== b.entry_date) return a.entry_date.localeCompare(b.entry_date) - const atime = a.entry_time || '' - const btime = b.entry_time || '' - if (atime !== btime) return atime.localeCompare(btime) - return (a.sort_order || 0) - (b.sort_order || 0) + if (a.sort_order !== b.sort_order) return (a.sort_order || 0) - (b.sort_order || 0) + return a.id - b.id }) return { current: { ...s.current, entries } } }) diff --git a/server/src/db/migrations.ts b/server/src/db/migrations.ts index 46f48e8c..29640339 100644 --- a/server/src/db/migrations.ts +++ b/server/src/db/migrations.ts @@ -2107,6 +2107,29 @@ function runMigrations(db: Database.Database): void { != substr(reservations.reservation_time, 1, 10) `); }, + // #846: make sort_order authoritative within a day. Previous ORDER BY put + // entry_time before sort_order, silently ignoring reorder clicks when two + // same-date entries had different times. Backfill renumbers using the old + // effective key (entry_time ASC, id ASC) so existing journeys retain their + // current visual order. + () => { + db.exec(` + WITH ranked AS ( + SELECT id, + ROW_NUMBER() OVER ( + PARTITION BY journey_id, entry_date + ORDER BY entry_time ASC, id ASC + ) - 1 AS rn + FROM journey_entries + ) + UPDATE journey_entries + SET sort_order = (SELECT rn FROM ranked WHERE ranked.id = journey_entries.id) + `); + db.exec( + 'CREATE INDEX IF NOT EXISTS idx_journey_entries_order ' + + 'ON journey_entries(journey_id, entry_date, sort_order)' + ); + }, ]; if (currentVersion < migrations.length) { diff --git a/server/src/services/journeyService.ts b/server/src/services/journeyService.ts index a03d1fea..afa6c7b3 100644 --- a/server/src/services/journeyService.ts +++ b/server/src/services/journeyService.ts @@ -120,7 +120,7 @@ export function getJourneyFull(journeyId: number, userId: number) { if (!journey) return null; const entries = db.prepare( - 'SELECT * FROM journey_entries WHERE journey_id = ? ORDER BY entry_date ASC, entry_time ASC, sort_order ASC' + 'SELECT * FROM journey_entries WHERE journey_id = ? ORDER BY entry_date ASC, sort_order ASC, id ASC' ).all(journeyId) as JourneyEntry[]; const photos = db.prepare( @@ -306,12 +306,21 @@ export function syncTripPlaces(journeyId: number, tripId: number, authorId: numb ).all(journeyId, tripId) as { source_place_id: number }[]; const existingPlaceIds = new Set(existing.map(e => e.source_place_id)); + // Track next sort_order per date so synced skeletons get unique, sequential positions. + const dateMaxOrder = new Map(); + const maxRows = db.prepare( + 'SELECT entry_date, COALESCE(MAX(sort_order), -1) AS m FROM journey_entries WHERE journey_id = ? GROUP BY entry_date' + ).all(journeyId) as { entry_date: string; m: number }[]; + for (const row of maxRows) dateMaxOrder.set(row.entry_date, row.m); + for (const place of places) { if (existingPlaceIds.has(place.id)) continue; existingPlaceIds.add(place.id); const entryDate = place.day_date || new Date().toISOString().split('T')[0]; const entryTime = place.assignment_time || place.place_time || null; + const nextOrder = (dateMaxOrder.get(entryDate) ?? -1) + 1; + dateMaxOrder.set(entryDate, nextOrder); db.prepare(` INSERT INTO journey_entries (journey_id, source_trip_id, source_place_id, author_id, type, title, entry_date, entry_time, location_name, location_lat, location_lng, sort_order, created_at, updated_at) @@ -320,7 +329,7 @@ export function syncTripPlaces(journeyId: number, tripId: number, authorId: numb journeyId, tripId, place.id, authorId, place.name, entryDate, entryTime, place.address || place.name, place.lat || null, place.lng || null, - place.day_number || 0, now, now + nextOrder, now, now ); } } @@ -367,15 +376,19 @@ export function onPlaceCreated(tripId: number, placeId: number) { const journey = db.prepare('SELECT user_id FROM journeys WHERE id = ?').get(link.journey_id) as { user_id: number }; const entryDate = place.day_date; + const maxOrder = db.prepare( + 'SELECT MAX(sort_order) AS m FROM journey_entries WHERE journey_id = ? AND entry_date = ?' + ).get(link.journey_id, entryDate) as { m: number | null }; + const nextOrder = (maxOrder?.m ?? -1) + 1; db.prepare(` INSERT INTO journey_entries (journey_id, source_trip_id, source_place_id, author_id, type, title, entry_date, entry_time, location_name, location_lat, location_lng, sort_order, created_at, updated_at) - VALUES (?, ?, ?, ?, 'skeleton', ?, ?, ?, ?, ?, ?, 0, ?, ?) + VALUES (?, ?, ?, ?, 'skeleton', ?, ?, ?, ?, ?, ?, ?, ?, ?) `).run( link.journey_id, tripId, placeId, journey.user_id, place.name, entryDate, place.assignment_time || place.place_time || null, place.address || place.name, place.lat || null, place.lng || null, - now, now + nextOrder, now, now ); } } @@ -451,7 +464,7 @@ export function listEntries(journeyId: number, userId: number) { if (!canAccessJourney(journeyId, userId)) return null; const entries = db.prepare( - 'SELECT * FROM journey_entries WHERE journey_id = ? ORDER BY entry_date ASC, entry_time ASC, sort_order ASC' + 'SELECT * FROM journey_entries WHERE journey_id = ? ORDER BY entry_date ASC, sort_order ASC, id ASC' ).all(journeyId) as JourneyEntry[]; const photos = db.prepare( diff --git a/server/tests/unit/services/journeyService.test.ts b/server/tests/unit/services/journeyService.test.ts index 950eff04..82b14d55 100644 --- a/server/tests/unit/services/journeyService.test.ts +++ b/server/tests/unit/services/journeyService.test.ts @@ -68,6 +68,7 @@ import { removeContributor, getSuggestions, syncTripPlaces, + reorderEntries, onPlaceCreated, onPlaceUpdated, onPlaceDeleted, @@ -1465,3 +1466,108 @@ describe('addProviderPhoto — passphrase', () => { expect(row?.passphrase).not.toBe('secret-pp'); }); }); + +// -- reorderEntries (#846) ---------------------------------------------------- + +function insertEntry(journeyId: number, authorId: number, opts: { entry_date: string; entry_time?: string | null; sort_order?: number }): { id: number } { + const now = Date.now(); + const res = testDb.prepare(` + INSERT INTO journey_entries (journey_id, author_id, type, entry_date, entry_time, sort_order, visibility, created_at, updated_at) + VALUES (?, ?, 'entry', ?, ?, ?, 'private', ?, ?) + `).run(journeyId, authorId, opts.entry_date, opts.entry_time ?? null, opts.sort_order ?? 0, now, now); + return { id: Number(res.lastInsertRowid) }; +} + +describe('reorderEntries', () => { + it('JOURNEY-SVC-089: reorder persists and listEntries returns requested order regardless of entry_time', () => { + const { user } = createUser(testDb); + const journey = createJourney(testDb, user.id); + const e1 = insertEntry(journey.id, user.id, { entry_date: '2026-08-01', entry_time: '09:00', sort_order: 0 }); + const e2 = insertEntry(journey.id, user.id, { entry_date: '2026-08-01', entry_time: '14:00', sort_order: 1 }); + + const ok = reorderEntries(journey.id, user.id, [e2.id, e1.id]); + expect(ok).toBe(true); + + const entries = listEntries(journey.id, user.id)!; + const dayEntries = entries.filter(e => e.entry_date === '2026-08-01'); + expect(dayEntries.map(e => e.id)).toEqual([e2.id, e1.id]); + }); + + it('JOURNEY-SVC-090: reorderEntries rejects ids from another journey', () => { + const { user } = createUser(testDb); + const j1 = createJourney(testDb, user.id); + const j2 = createJourney(testDb, user.id); + const entry = createJourneyEntry(testDb, j2.id, user.id, { entry_date: '2026-08-02' }); + + const ok = reorderEntries(j1.id, user.id, [entry.id]); + expect(ok).toBe(false); + }); + + it('JOURNEY-SVC-091: reorderEntries does not affect entries on other days', () => { + const { user } = createUser(testDb); + const journey = createJourney(testDb, user.id); + const day1a = insertEntry(journey.id, user.id, { entry_date: '2026-08-01', sort_order: 0 }); + const day1b = insertEntry(journey.id, user.id, { entry_date: '2026-08-01', sort_order: 1 }); + const day2 = insertEntry(journey.id, user.id, { entry_date: '2026-08-02', sort_order: 0 }); + + reorderEntries(journey.id, user.id, [day1b.id, day1a.id]); + + const entries = listEntries(journey.id, user.id)!; + const day2Entry = entries.find(e => e.id === day2.id)!; + expect(day2Entry.sort_order).toBe(0); + }); +}); + +describe('syncTripPlaces sort_order', () => { + it('JOURNEY-SVC-092: assigns unique sequential sort_order per date for same-day places', () => { + const { user } = createUser(testDb); + const journey = createJourney(testDb, user.id); + const trip = createTrip(testDb, user.id, { + title: 'Order Trip', + start_date: '2026-09-01', + end_date: '2026-09-02', + }); + const day = testDb.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY date ASC LIMIT 1').get(trip.id) as { id: number }; + const p1 = createPlace(testDb, trip.id, { name: 'Place A' }); + const p2 = createPlace(testDb, trip.id, { name: 'Place B' }); + const p3 = createPlace(testDb, trip.id, { name: 'Place C' }); + createDayAssignment(testDb, day.id, p1.id); + createDayAssignment(testDb, day.id, p2.id); + createDayAssignment(testDb, day.id, p3.id); + + syncTripPlaces(journey.id, trip.id, user.id); + + const rows = testDb.prepare( + 'SELECT sort_order FROM journey_entries WHERE journey_id = ? ORDER BY sort_order ASC' + ).all(journey.id) as { sort_order: number }[]; + const orders = rows.map(r => r.sort_order); + expect(new Set(orders).size).toBe(orders.length); + expect(orders).toEqual([0, 1, 2]); + }); +}); + +describe('onPlaceCreated sort_order', () => { + it('JOURNEY-SVC-093: assigns MAX+1 sort_order when entries already exist on the target date', () => { + const { user } = createUser(testDb); + const journey = createJourney(testDb, user.id); + const trip = createTrip(testDb, user.id, { + title: 'Append Trip', + start_date: '2026-10-01', + end_date: '2026-10-02', + }); + addTripToJourney(journey.id, trip.id, user.id); + + const day = testDb.prepare('SELECT id, date FROM days WHERE trip_id = ? ORDER BY date ASC LIMIT 1').get(trip.id) as { id: number; date: string }; + insertEntry(journey.id, user.id, { entry_date: day.date, sort_order: 5 }); + + const place = createPlace(testDb, trip.id, { name: 'Late Addition' }); + createDayAssignment(testDb, day.id, place.id); + onPlaceCreated(trip.id, place.id); + + const newEntry = testDb.prepare( + 'SELECT sort_order FROM journey_entries WHERE journey_id = ? AND source_place_id = ?' + ).get(journey.id, place.id) as { sort_order: number } | undefined; + expect(newEntry).toBeDefined(); + expect(newEntry!.sort_order).toBe(6); + }); +});