diff --git a/client/scripts/size-baseline.json b/client/scripts/size-baseline.json index 701e25c7f..5dcf30138 100644 --- a/client/scripts/size-baseline.json +++ b/client/scripts/size-baseline.json @@ -6,7 +6,7 @@ "components/Map/MapViewGL.tsx": 2572, "components/Map/RouteCalculator.ts": 1190, "components/PDF/TripPDF.tsx": 1025, - "components/Planner/DayPlanSidebar.tsx": 3058, + "components/Planner/DayPlanSidebar.tsx": 3051, "components/Planner/PlaceFormModal.tsx": 1633, "components/Planner/PlaceInspector.tsx": 1273, "components/Planner/TransportModal.tsx": 1199, diff --git a/client/src/components/Planner/DayPlanSidebar.test.tsx b/client/src/components/Planner/DayPlanSidebar.test.tsx index b0c2ecde6..e01d95c77 100644 --- a/client/src/components/Planner/DayPlanSidebar.test.tsx +++ b/client/src/components/Planner/DayPlanSidebar.test.tsx @@ -156,6 +156,11 @@ function stubTripActions(actions: Record) { useTripStore.setState(actions as unknown as Partial) } +/** The stand-in for the store's visit-time action that beforeEach puts in place. */ +function setAssignmentTimesStub() { + return vi.mocked(useTripStore.getState().setAssignmentTimes) +} + function dayHeader(title: string) { return screen.getByText(title).closest('[style*="cursor: pointer"]') as HTMLElement } @@ -194,6 +199,8 @@ const emptyDataTransfer = { setData: vi.fn(), effectAllowed: '', getData: vi.fn( beforeEach(() => { resetAllStores() vi.clearAllMocks() + // The time removal before a reorder goes through the store's visit-time action. + stubTripActions({ setAssignmentTimes: vi.fn(async () => undefined) }) Element.prototype.scrollTo = vi.fn() mockPermissions.canEdit = true mockPermissions.denied.clear() @@ -1928,7 +1935,6 @@ describe('DayPlanSidebar', () => { it('FE-PLANNER-DAYPLAN-081: clicking Confirm in time modal calls confirmTimeRemoval (updates assignment time)', async () => { const user = userEvent.setup() - const { assignmentsApi } = await import('../../api/client') const placeA = buildPlace({ id: 1, name: 'Morning Place', place_time: '08:00' }) const placeB = buildPlace({ id: 2, name: 'Afternoon Place', place_time: '14:00' }) const day = buildDay({ id: 10, date: '2025-06-01', title: 'Day 1' }) @@ -1954,7 +1960,7 @@ describe('DayPlanSidebar', () => { const confirmBtn = screen.getByRole('button', { name: /confirm/i }) await user.click(confirmBtn) - await waitFor(() => expect((assignmentsApi as any).updateTime).toHaveBeenCalled()) + await waitFor(() => expect(setAssignmentTimesStub()).toHaveBeenCalled()) }) // ── applyMergedOrder with notes in list (noteUpdates branch) ────────────── @@ -2207,7 +2213,6 @@ describe('DayPlanSidebar', () => { it('FE-PLANNER-DAYPLAN-093: arrow-reorder timed place shows modal then confirm removes time', async () => { const user = userEvent.setup() - const { assignmentsApi } = await import('../../api/client') as any const onReorder = vi.fn().mockResolvedValue(undefined) const placeA = buildPlace({ id: 1, name: 'Early Place', place_time: '08:00' }) const placeB = buildPlace({ id: 2, name: 'Later Place', place_time: '14:00' }) @@ -2230,7 +2235,7 @@ describe('DayPlanSidebar', () => { // Click Confirm const confirmBtn = screen.getByRole('button', { name: /confirm/i }) await user.click(confirmBtn) - await waitFor(() => expect(assignmentsApi.updateTime).toHaveBeenCalled()) + await waitFor(() => expect(setAssignmentTimesStub()).toHaveBeenCalled()) } }) @@ -3207,8 +3212,7 @@ describe('DayPlanSidebar', () => { it('FE-PLANNER-DAYPLAN-127: a failing time removal aborts the reorder and reports the error', async () => { const user = userEvent.setup() - const { assignmentsApi } = await import('../../api/client') - vi.mocked(assignmentsApi.updateTime).mockRejectedValueOnce(new Error('locked')) + setAssignmentTimesStub().mockRejectedValueOnce(new Error('locked')) const placeA = buildPlace({ id: 1, name: 'Morning Place', place_time: '08:00' }) const placeB = buildPlace({ id: 2, name: 'Afternoon Place', place_time: '14:00' }) const day = buildDay({ id: 10, date: '2025-06-01', title: 'Day 1' }) @@ -4658,7 +4662,6 @@ describe('DayPlanSidebar', () => { it('FE-PLANNER-DAYPLAN-180: the chronology check spans notes and bookings, not just stops', async () => { const user = userEvent.setup() - const { assignmentsApi } = await import('../../api/client') const day = buildDay({ id: 10, date: '2025-06-01', title: 'Day 1' }) const placeA = buildPlace({ id: 1, name: 'Morning Place', place_time: '08:00' }) const placeB = buildPlace({ id: 2, name: 'Afternoon Place', place_time: '14:00' }) @@ -4678,7 +4681,7 @@ describe('DayPlanSidebar', () => { fireEvent.drop(dragRow(screen.getByText('Morning Place')), { dataTransfer: { getData: vi.fn(() => '') } }) await waitFor(() => expect(screen.getByText('Remove time?')).toBeInTheDocument()) await user.click(screen.getByRole('button', { name: /confirm/i })) - await waitFor(() => expect(vi.mocked(assignmentsApi.updateTime)).toHaveBeenCalledWith(1, 12, { place_time: null, end_time: null })) + await waitFor(() => expect(setAssignmentTimesStub()).toHaveBeenCalledWith(1, 10, 12, { place_time: null, end_time: null })) await waitFor(() => expect(onReorder).toHaveBeenCalledWith(10, [12, 11])) // The booking keeps its slot in the rebuilt order. expect(screen.getByText('Midday bus')).toBeInTheDocument() @@ -4686,7 +4689,7 @@ describe('DayPlanSidebar', () => { it('FE-PLANNER-DAYPLAN-181: an arrow reorder across a booking drops the time and re-slots the booking', async () => { const user = userEvent.setup() - const { assignmentsApi, reservationsApi } = await import('../../api/client') + const { reservationsApi } = await import('../../api/client') const day = buildDay({ id: 10, date: '2025-06-01', title: 'Day 1' }) const placeA = buildPlace({ id: 1, name: 'Morning Place', place_time: '08:00' }) const placeB = buildPlace({ id: 2, name: 'Evening Place', place_time: '18:00' }) @@ -4706,7 +4709,7 @@ describe('DayPlanSidebar', () => { await user.click(upBtn) await waitFor(() => expect(screen.getByText('Remove time?')).toBeInTheDocument()) await user.click(screen.getByRole('button', { name: /confirm/i })) - await waitFor(() => expect(vi.mocked(assignmentsApi.updateTime)).toHaveBeenCalledWith(1, 12, { place_time: null, end_time: null })) + await waitFor(() => expect(setAssignmentTimesStub()).toHaveBeenCalledWith(1, 10, 12, { place_time: null, end_time: null })) await waitFor(() => expect(onReorder).toHaveBeenCalledWith(10, [11, 12])) // The stop really moves past the booking: the bus lands behind both places // instead of keeping its old slot between them. diff --git a/client/src/components/Planner/DayPlanSidebar.tsx b/client/src/components/Planner/DayPlanSidebar.tsx index edb6be637..9990bb092 100644 --- a/client/src/components/Planner/DayPlanSidebar.tsx +++ b/client/src/components/Planner/DayPlanSidebar.tsx @@ -1080,17 +1080,10 @@ function useDayPlanSidebar(props: DayPlanSidebarProps) { const { dayId, fromId, fromType, toType, toId, insertAfter, toLegIndex } = saved setTimeConfirm(null) - // Remove time from assignment + // Remove time from assignment, through the store and the visit's repo so a time + // still waiting in the queue for this visit goes out first. try { - await assignmentsApi.updateTime(tripId, fromId, { place_time: null, end_time: null }) - const key = String(dayId) - const currentAssignments = { ...assignments } - if (currentAssignments[key]) { - currentAssignments[key] = currentAssignments[key].map(a => - a.id === fromId ? { ...a, place: { ...a.place, place_time: null, end_time: null } } : a - ) - tripActions.setAssignments(currentAssignments) - } + await tripActions.setAssignmentTimes(tripId, dayId, fromId, { place_time: null, end_time: null }) } catch (err) { toast.error(err instanceof Error ? err.message : t('common.unknownError')) return diff --git a/client/src/pages/tripPlanner/usePlaceEdits.test.ts b/client/src/pages/tripPlanner/usePlaceEdits.test.ts index 573afd207..64332db55 100644 --- a/client/src/pages/tripPlanner/usePlaceEdits.test.ts +++ b/client/src/pages/tripPlanner/usePlaceEdits.test.ts @@ -5,7 +5,12 @@ // and every write goes to a recorded stand-in for the store's actions, so each case // can read what was written and what the undo would put back. import { act, renderHook } from '@testing-library/react' +import { http, HttpResponse } from 'msw' import { assignmentsApi } from '../../api/client' +import { offlineDb, clearAll } from '../../db/offlineDb' +import { mutationQueue } from '../../sync/mutationQueue' +import { setAuthed } from '../../sync/authGate' +import { server } from '../../../tests/helpers/msw/server' import { useTripStore, type TripStoreState } from '../../store/tripStore' import { resetAllStores } from '../../../tests/helpers/store' import { buildAssignment, buildDay, buildPlace, buildReservation, buildTrip } from '../../../tests/helpers/factories' @@ -35,6 +40,7 @@ function makeActions() { addPlace: vi.fn(async (): Promise<{ id: number } | null> => ({ id: 900 })), assignPlaceToDay: vi.fn(async () => ({ id: 555 })), refreshDays: vi.fn(async () => undefined), + setAssignmentTimes: vi.fn(async () => undefined), addFile: vi.fn(async () => undefined), deletePlace: vi.fn(async (): Promise<{ tourPlaceIds?: number[] } | undefined> => undefined), deletePlacesMany: vi.fn(async (): Promise<{ tourPlaceIds?: number[] } | undefined> => undefined), @@ -170,6 +176,7 @@ describe('usePlaceEdits', () => { const updateNotes = vi.spyOn(assignmentsApi, 'updateNotes').mockResolvedValue({} as never) actions.addFile.mockRejectedValueOnce(new Error('files.tooLarge')).mockResolvedValueOnce(undefined) const file = new File(['x'], 'a.pdf') + useTripStore.setState(state => ({ assignments: { ...state.assignments, 10: [{ ...state.assignments[10][0], notes: 'old' }] } })) const { result } = renderEdits({ editingPlace: lake, editingAssignmentId: 101 }) let saved: unknown @@ -181,9 +188,13 @@ describe('usePlaceEdits', () => { expect(saved).toEqual({ id: 1 }) expect(actions.updatePlace).toHaveBeenCalledWith(42, 1, { name: 'Lake' }) - expect(updateTime).toHaveBeenCalledWith(42, 101, { place_time: '09:00', end_time: null }) + // The times go through the store and the visit's repo, never straight to the API. + expect(actions.setAssignmentTimes).toHaveBeenCalledWith(42, 10, 101, { place_time: '09:00', end_time: null }) + expect(updateTime).not.toHaveBeenCalled() expect(updateNotes).toHaveBeenCalledWith(42, 101, { notes: null }) - expect(actions.refreshDays).toHaveBeenCalledWith(42) + // The note lands on the visit without reloading the days over a queued time. + expect(useTripStore.getState().assignments[10][0].notes).toBeNull() + expect(actions.refreshDays).not.toHaveBeenCalled() expect(actions.addFile).toHaveBeenCalledTimes(2) expect(toast.error).toHaveBeenCalledWith('files.uploadError') expect(toast.success).toHaveBeenCalledWith('trip.toast.placeUpdated') @@ -196,10 +207,45 @@ describe('usePlaceEdits', () => { await act(async () => { await result.current.handleSavePlace({ name: 'Castle', place_time: '10:00' }) }) expect(actions.updatePlace).toHaveBeenCalledWith(42, 2, { name: 'Castle' }) + expect(actions.setAssignmentTimes).not.toHaveBeenCalled() expect(updateTime).not.toHaveBeenCalled() expect(actions.refreshDays).not.toHaveBeenCalled() }) + it('FE-TP-PLACEEDITS-006b: a time saved from the dialog waits behind a parked time of the same visit', async () => { + // The older time was parked as failed; the one saved now online must not land first, + // or Try again would replay the older time over it. + await clearAll() + mutationQueue._resetFlushing() + setAuthed(true) + onTestFinished(() => setAuthed(false)) + await offlineDb.mutationQueue.put({ + id: 'parked', tripId: 42, method: 'PUT', url: '/trips/42/assignments/101/time', + body: { place_time: '07:00', end_time: null }, createdAt: 1, status: 'failed', attempts: 8, + lastError: 'boom', resource: 'assignments', entityId: 101, + }) + const sent: string[] = [] + server.use(http.put('/api/trips/42/assignments/101/time', async ({ request }) => { + const body = await request.json() as { place_time: string } + sent.push(body.place_time) + return HttpResponse.json({ assignment: buildAssignment({ id: 101, day_id: 10, place: lake, assignment_time: body.place_time }) }) + })) + const { setAssignmentTimes } = useTripStore.getState() + const { result } = renderEdits({ + editingPlace: lake, editingAssignmentId: 101, + tripActions: { ...fixed.tripActions, setAssignmentTimes } as TripStoreState, + }) + + await act(async () => { await result.current.handleSavePlace({ name: 'Lake', place_time: '09:00', end_time: '' }) }) + await new Promise(resolve => setTimeout(resolve, 20)) + expect(sent).toEqual([]) + expect(useTripStore.getState().assignments[10][0].assignment_time).toBe('09:00') + + await mutationQueue.retryFailed() + expect(sent).toEqual(['07:00', '09:00']) + expect(await offlineDb.mutationQueue.count()).toBe(0) + }) + it('FE-TP-PLACEEDITS-008: a service stop the drawn card does not show still lands on the day and position it named', async () => { const { result } = renderEdits() diff --git a/client/src/pages/tripPlanner/usePlaceEdits.ts b/client/src/pages/tripPlanner/usePlaceEdits.ts index f8c78d757..506618b15 100644 --- a/client/src/pages/tripPlanner/usePlaceEdits.ts +++ b/client/src/pages/tripPlanner/usePlaceEdits.ts @@ -102,13 +102,28 @@ export function usePlaceEdits(options: PlaceEditsOptions) { await tripActions.updatePlace(tripId, editingPlace.id, placeData) // If editing from assignment context, save time per-assignment if (editingAssignmentId) { - await assignmentsApi.updateTime(tripId, editingAssignmentId, { place_time: place_time || null, end_time: end_time || null }) + // Through the store and the visit's repo, so a time still waiting in the + // queue for this visit goes out first and a retry cannot land over this one. + const assignments = useTripStore.getState().assignments + const dayKey = Object.keys(assignments).find(key => assignments[key].some(a => a.id === editingAssignmentId)) + if (dayKey) { + await tripActions.setAssignmentTimes(tripId, Number(dayKey), editingAssignmentId, { + place_time: place_time || null, end_time: end_time || null, + }) + } // The form only includes assignment_notes when the user changed it, so // an untouched note never produces a PUT (#2163). '' clears like null. + // The note is set on the visit in place: reloading the days would show + // the server's time while the new one still waits in the queue. if (assignment_notes !== undefined) { - await assignmentsApi.updateNotes(tripId, editingAssignmentId, { notes: assignment_notes || null }) + const notes = assignment_notes || null + await assignmentsApi.updateNotes(tripId, editingAssignmentId, { notes }) + useTripStore.setState(state => ({ + assignments: Object.fromEntries(Object.entries(state.assignments).map(([key, items]) => [ + key, items.map(a => (a.id === editingAssignmentId ? { ...a, notes } : a)), + ])), + })) } - await tripActions.refreshDays(tripId) } // Upload pending files with place_id if (pendingFiles?.length > 0) { diff --git a/client/src/pages/tripPlanner/useTripPlanner.test.tsx b/client/src/pages/tripPlanner/useTripPlanner.test.tsx index 51cfbb35b..077ca54f0 100644 --- a/client/src/pages/tripPlanner/useTripPlanner.test.tsx +++ b/client/src/pages/tripPlanner/useTripPlanner.test.tsx @@ -94,6 +94,7 @@ interface PlannerActions { loadBudgetItems: ReturnType loadFiles: ReturnType refreshDays: ReturnType + setAssignmentTimes: ReturnType addPlace: ReturnType updatePlace: ReturnType deletePlace: ReturnType @@ -123,6 +124,7 @@ function makeActions(): PlannerActions { loadBudgetItems: vi.fn(async () => undefined), loadFiles: vi.fn(async () => undefined), refreshDays: vi.fn(async () => undefined), + setAssignmentTimes: vi.fn(async () => undefined), addPlace: vi.fn(async () => ({ id: 900, name: 'New' })), updatePlace: vi.fn(async () => undefined), deletePlace: vi.fn(async () => undefined), @@ -1221,8 +1223,8 @@ describe('useTripPlanner — place CRUD', () => { }) expect(actions.updatePlace).toHaveBeenCalledWith(42, 1, { name: 'Nara' }) - expect(assignmentsApi.updateTime).toHaveBeenCalledWith(42, 10, { place_time: '09:00', end_time: '10:00' }) - expect(actions.refreshDays).toHaveBeenCalledWith(42) + expect(actions.setAssignmentTimes).toHaveBeenCalledWith(42, 7, 10, { place_time: '09:00', end_time: '10:00' }) + expect(assignmentsApi.updateTime).not.toHaveBeenCalled() }) it('FE-TP-HOOK-053b: a changed assignment note is stripped off the place and PUT per assignment (#2163)', async () => { @@ -1241,7 +1243,7 @@ describe('useTripPlanner — place CRUD', () => { expect(actions.updatePlace).toHaveBeenCalledWith(42, 1, { name: 'Nara' }) expect(assignmentsApi.updateNotes).toHaveBeenCalledWith(42, 10, { notes: 'Book the 10:00 entry' }) - expect(actions.refreshDays).toHaveBeenCalledWith(42) + expect(useTripStore.getState().assignments['7'][0].notes).toBe('Book the 10:00 entry') }) it('FE-TP-HOOK-053c: without assignment_notes in the payload no notes write happens; an empty string clears (#2163)', async () => { @@ -1278,7 +1280,7 @@ describe('useTripPlanner — place CRUD', () => { await result.current.handleSavePlace({ name: 'Nara', _pendingFiles: [new File(['x'], 'a.pdf')] }) }) - expect(assignmentsApi.updateTime).not.toHaveBeenCalled() + expect(actions.setAssignmentTimes).not.toHaveBeenCalled() expect(actions.addFile).toHaveBeenCalledTimes(1) }) diff --git a/client/src/sync/mutationQueue.ts b/client/src/sync/mutationQueue.ts index a2193cdcc..ad5f02ba0 100644 --- a/client/src/sync/mutationQueue.ts +++ b/client/src/sync/mutationQueue.ts @@ -584,8 +584,7 @@ export const mutationQueue = { * so any newer version on the server, a collaborator's or one saved here * online, refuses it (409) and it becomes a conflict. A write without a token * overwrites what someone else changed on the server meanwhile, as every - * replayed offline write of those resources does, and so do writes made here - * that skip the repos (the planner's two direct time edits of a visit). + * replayed offline write of those resources does. */ async retryFailed(): Promise { await offlineDb.mutationQueue