From 789a95de92ebd53a4b2fdf6de8ba9ce27efcff3b Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 8 Oct 2026 12:10:20 +0200 Subject: [PATCH] fix(planner): the place dialog and the reorder time removal go through the visit repo The planner's two time edits of a visit called the time route straight: saving the place dialog opened from a visit, and removing a visit's time before a reorder. They skipped the visit repo's check for an older write to the same visit still in the queue, so with such a write parked or backing off the new time landed first and Try again (or the next flush) replayed the older time over it, with no version token to refuse it. Both now use the store's setAssignmentTimes, which writes through assignmentRepo.setTimes, queues behind the older write when there is one and keeps the optimistic update and rollback the other time edits have. The dialog no longer reloads the days afterwards, since the store already holds the new time and a reload would show the server's older one while the new one waits in the queue; a changed day note is set on the visit in place instead. The retry docs drop the exception they made for these two paths. --- client/scripts/size-baseline.json | 2 +- .../Planner/DayPlanSidebar.test.tsx | 23 +++++---- .../src/components/Planner/DayPlanSidebar.tsx | 13 ++--- .../pages/tripPlanner/usePlaceEdits.test.ts | 50 ++++++++++++++++++- client/src/pages/tripPlanner/usePlaceEdits.ts | 21 ++++++-- .../pages/tripPlanner/useTripPlanner.test.tsx | 10 ++-- client/src/sync/mutationQueue.ts | 3 +- 7 files changed, 90 insertions(+), 32 deletions(-) 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