mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-06-19 13:21:46 +00:00
fix(journey,pdf): journey reorder sort_order + PDF multi-day transport (#848)
* 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
This commit is contained in:
@@ -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' }),
|
||||
|
||||
@@ -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 `
|
||||
<div class="note-card" style="border-left: 3px solid ${color};">
|
||||
<div class="note-line" style="background: ${color};"></div>
|
||||
<span class="note-icon">${icon}</span>
|
||||
<div class="note-body">
|
||||
<div class="note-text" style="font-weight: 600;">${escHtml(r.title)}${time ? ` <span style="color:#6b7280;font-weight:400;font-size:10px;">${time}</span>` : ''}</div>
|
||||
<div class="note-text" style="font-weight: 600;">${titleHtml}${time ? ` <span style="color:#6b7280;font-weight:400;font-size:10px;">${time}</span>` : ''}</div>
|
||||
${subtitle ? `<div class="note-time">${escHtml(subtitle)}</div>` : ''}
|
||||
${locationLine ? `<div class="note-time">${escHtml(locationLine)}</div>` : ''}
|
||||
${r.confirmation_number ? `<div class="note-time" style="font-size:9px;">Code: ${escHtml(r.confirmation_number)}</div>` : ''}
|
||||
|
||||
@@ -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', () => {
|
||||
|
||||
@@ -223,10 +223,8 @@ export const useJourneyStore = create<JourneyState>((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 } }
|
||||
})
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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<string, number>();
|
||||
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(
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user