diff --git a/client/scripts/offline-baseline.json b/client/scripts/offline-baseline.json index 91cbfddd0..41dd224bd 100644 --- a/client/scripts/offline-baseline.json +++ b/client/scripts/offline-baseline.json @@ -206,7 +206,6 @@ "src/pages/settings/useSettings.ts", "src/pages/sharedTrip/useSharedTrip.ts", "src/pages/tripPlanner/useBookingEdits.ts", - "src/pages/tripPlanner/usePlaceEdits.ts", "src/pages/tripPlanner/usePlaceFormOpeners.ts", "src/pages/tripPlanner/usePlannerDialogs.ts", "src/pages/tripPlanner/useRoadtripStopEdits.ts", diff --git a/client/src/pages/tripPlanner/usePlaceEdits.test.ts b/client/src/pages/tripPlanner/usePlaceEdits.test.ts index 64332db55..bb2b45830 100644 --- a/client/src/pages/tripPlanner/usePlaceEdits.test.ts +++ b/client/src/pages/tripPlanner/usePlaceEdits.test.ts @@ -41,6 +41,7 @@ function makeActions() { assignPlaceToDay: vi.fn(async () => ({ id: 555 })), refreshDays: vi.fn(async () => undefined), setAssignmentTimes: vi.fn(async () => undefined), + setAssignmentNotes: 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), @@ -191,9 +192,10 @@ describe('usePlaceEdits', () => { // 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 }) - // The note lands on the visit without reloading the days over a queued time. - expect(useTripStore.getState().assignments[10][0].notes).toBeNull() + // The note goes through the store and the visit's repo too, which caches it on the day, + // and the days are not reloaded over a queued time. + expect(actions.setAssignmentNotes).toHaveBeenCalledWith(42, 10, 101, null) + expect(updateNotes).not.toHaveBeenCalled() expect(actions.refreshDays).not.toHaveBeenCalled() expect(actions.addFile).toHaveBeenCalledTimes(2) expect(toast.error).toHaveBeenCalledWith('files.uploadError') @@ -208,6 +210,7 @@ describe('usePlaceEdits', () => { expect(actions.updatePlace).toHaveBeenCalledWith(42, 2, { name: 'Castle' }) expect(actions.setAssignmentTimes).not.toHaveBeenCalled() + expect(actions.setAssignmentNotes).not.toHaveBeenCalled() expect(updateTime).not.toHaveBeenCalled() expect(actions.refreshDays).not.toHaveBeenCalled() }) diff --git a/client/src/pages/tripPlanner/usePlaceEdits.ts b/client/src/pages/tripPlanner/usePlaceEdits.ts index 506618b15..de1ac9b4c 100644 --- a/client/src/pages/tripPlanner/usePlaceEdits.ts +++ b/client/src/pages/tripPlanner/usePlaceEdits.ts @@ -1,5 +1,4 @@ import { useCallback, useMemo, useState } from 'react' -import { assignmentsApi } from '../../api/client' import { roadtripInsertion } from '../../components/Roadtrip/dayWindow' import { reanchorAfterInsert } from '../../components/Roadtrip/roadtripModel' import type { usePlaceSelection } from '../../hooks/usePlaceSelection' @@ -110,19 +109,12 @@ export function usePlaceEdits(options: PlaceEditsOptions) { 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) { - 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)), - ])), - })) + // The form only includes assignment_notes when the user changed it, so + // an untouched note never produces a PUT (#2163). '' clears like null. + // Through the visit's repo as well, so the cached day keeps the new note. + if (assignment_notes !== undefined) { + await tripActions.setAssignmentNotes(tripId, Number(dayKey), editingAssignmentId, assignment_notes || null) + } } } // Upload pending files with place_id diff --git a/client/src/pages/tripPlanner/useTripPlanner.test.tsx b/client/src/pages/tripPlanner/useTripPlanner.test.tsx index 077ca54f0..2b14ecde1 100644 --- a/client/src/pages/tripPlanner/useTripPlanner.test.tsx +++ b/client/src/pages/tripPlanner/useTripPlanner.test.tsx @@ -203,7 +203,10 @@ beforeEach(() => { vi.spyOn(tripsApi, 'getMembers').mockResolvedValue({ owner: null, members: [] }) vi.spyOn(accommodationsApi, 'list').mockResolvedValue({ accommodations: [] }) vi.spyOn(assignmentsApi, 'updateTime').mockResolvedValue({}) - vi.spyOn(assignmentsApi, 'updateNotes').mockResolvedValue({}) + // The note route answers with the saved visit, which the repo caches on the day. + vi.spyOn(assignmentsApi, 'updateNotes').mockImplementation(async (_tripId, id, { notes }) => ({ + assignment: buildAssignment({ id, day_id: 7, place: buildPlace({ id: 1, lat: 1, lng: 2 }), notes }), + })) vi.spyOn(airtrailApi, 'sync').mockResolvedValue({ changed: 0 }) vi.spyOn(mapsApi, 'reverse').mockResolvedValue({ name: '', address: '' } as never) vi.spyOn(mapsApi, 'search').mockResolvedValue({ places: [] } as never) diff --git a/client/src/repo/assignmentRepo.test.ts b/client/src/repo/assignmentRepo.test.ts index 91682effa..6de6f5b08 100644 --- a/client/src/repo/assignmentRepo.test.ts +++ b/client/src/repo/assignmentRepo.test.ts @@ -11,7 +11,7 @@ import type { Assignment, Day } from '../types' vi.mock('../api/assignmentEndDay', () => ({ saveAssignmentEndDay: vi.fn() })) vi.mock('../sync/networkMode', () => ({ isEffectivelyOffline: vi.fn(() => true) })) vi.mock('../sync/authGate', () => ({ isAuthed: () => true })) -vi.mock('../api/client', () => ({ apiClient: { request: vi.fn() }, assignmentsApi: { updateTime: vi.fn(), clearDay: vi.fn() } })) +vi.mock('../api/client', () => ({ apiClient: { request: vi.fn() }, assignmentsApi: { updateTime: vi.fn(), updateNotes: vi.fn(), clearDay: vi.fn() } })) const assignment = { id: 7, day_id: 1, place_id: 2, order_index: 0, assignment_time: '07:00', place: { id: 2, name: 'Berlin' } } as Assignment beforeEach(async () => { @@ -80,6 +80,34 @@ describe('assignment time persistence', () => { }) }) +describe('assignment note persistence', () => { + it('saves through the note route online and caches the saved row on the day', async () => { + vi.mocked(isEffectivelyOffline).mockReturnValue(false) + vi.mocked(assignmentsApi.updateNotes).mockResolvedValue({ assignment: { ...assignment, notes: 'Bring cash' } }) + const saved = await assignmentRepo.setNotes(9, assignment, 'Bring cash') + expect(assignmentsApi.updateNotes).toHaveBeenCalledWith(9, 7, { notes: 'Bring cash' }) + expect(saved.notes).toBe('Bring cash') + expect((await offlineDb.days.get(1))?.assignments?.[0]).toMatchObject({ notes: 'Bring cash', assignment_time: '07:00' }) + expect(await offlineDb.mutationQueue.count()).toBe(0) + }) + + it('stores the offline note with a replayable write', async () => { + const saved = await assignmentRepo.setNotes(9, assignment, null) + expect(saved.notes).toBeNull() + expect((await offlineDb.days.get(1))?.assignments?.[0].notes).toBeNull() + expect(await offlineDb.mutationQueue.toArray()).toEqual([expect.objectContaining({ + url: '/trips/9/assignments/7/notes', method: 'PUT', body: { notes: null }, resource: 'assignments', entityId: 7, + })]) + }) + + it('keeps the cached visit unchanged after a refused save', async () => { + vi.mocked(isEffectivelyOffline).mockReturnValue(false) + vi.mocked(assignmentsApi.updateNotes).mockRejectedValue(new Error('Denied')) + await expect(assignmentRepo.setNotes(9, assignment, 'x')).rejects.toThrow('Denied') + expect((await offlineDb.days.get(1))?.assignments?.[0]).toEqual(assignment) + }) +}) + describe('an online edit of a visit with an older write still queued', () => { const first = { place_time: '07:00', end_time: null } const second = { place_time: '09:00', end_time: null } diff --git a/client/src/repo/assignmentRepo.ts b/client/src/repo/assignmentRepo.ts index 72f8d008f..6741e7d5b 100644 --- a/client/src/repo/assignmentRepo.ts +++ b/client/src/repo/assignmentRepo.ts @@ -98,4 +98,28 @@ export const assignmentRepo = { mutationQueue.sendSoon() return updated }, + /** + * The note a visit carries on its day (#2163). Saved through here rather than straight + * to the API so the cached day holds the new note too: the tab never sees its own + * `assignment:updated` echo, and a trip reopened offline reads its days from the cache. + * A write of the same visit still waiting in the queue keeps this one behind it. + */ + async setNotes(tripId: number | string, assignment: Assignment, notes: string | null): Promise { + if (!(await mutationQueue.mustQueue('assignments', assignment.id))) { + const saved = assignmentSchema.parse((await assignmentsApi.updateNotes(tripId, assignment.id, { notes })).assignment) + await cacheAssignment(saved) + return saved + } + const updated = { ...assignment, notes } + await offlineDb.transaction('rw', offlineDb.days, offlineDb.mutationQueue, async () => { + await mutationQueue.enqueue({ + id: generateUUID(), tripId: Number(tripId), method: 'PUT', + url: `/trips/${tripId}/assignments/${assignment.id}/notes`, + body: { notes }, resource: 'assignments', entityId: assignment.id, + }) + await cacheAssignment(updated) + }) + mutationQueue.sendSoon() + return updated + }, } diff --git a/client/src/store/slices/assignmentsSlice.ts b/client/src/store/slices/assignmentsSlice.ts index bdf2d9e3c..abf7ae547 100644 --- a/client/src/store/slices/assignmentsSlice.ts +++ b/client/src/store/slices/assignmentsSlice.ts @@ -13,6 +13,7 @@ export interface AssignmentsSlice { setAssignmentEndDay: (tripId: number | string, dayId: number, assignmentId: number, endDay: boolean) => Promise setAssignmentRouteExcluded: (tripId: number | string, dayId: number, assignmentId: number, excluded: boolean) => Promise setAssignmentTimes: (tripId: number | string, dayId: number, assignmentId: number, times: AssignmentTimes) => Promise + setAssignmentNotes: (tripId: number | string, dayId: number, assignmentId: number, notes: string | null) => Promise assignPlaceToDay: (tripId: number | string, dayId: number | string, placeId: number | string, position?: number | null) => Promise removeAssignment: (tripId: number | string, dayId: number | string, assignmentId: number) => Promise clearDayAssignments: (tripId: number | string, dayId: number) => Promise @@ -77,6 +78,25 @@ export const createAssignmentsSlice = (set: SetState, get: GetState): Assignment throw err } }, + setAssignmentNotes: async (tripId, dayId, assignmentId, notes) => { + const assignment = get().assignments[String(dayId)]?.find(a => a.id === assignmentId) + if (!assignment || assignmentId < 0) return + // Only the note is patched: the saved row carries the place as the server has it, + // and the visit's times may still wait in the queue ahead of this write. + const apply = (value: string | null) => set(state => ({ + assignments: Object.fromEntries(Object.entries(state.assignments).map(([key, items]) => [ + key, items.map(a => a.id === assignmentId ? { ...a, notes: value } : a), + ])), + })) + apply(notes) + try { + const saved = await assignmentRepo.setNotes(tripId, assignment, notes) + apply(saved.notes ?? null) + } catch (err: unknown) { + apply(assignment.notes ?? null) + throw err + } + }, assignPlaceToDay: async (tripId, dayId, placeId, position) => { const state = get() const place = state.places.find(p => p.id === Number.parseInt(String(placeId))) diff --git a/client/tests/unit/slices/assignmentsSlice.test.ts b/client/tests/unit/slices/assignmentsSlice.test.ts index eed11a9f5..04ced9bc0 100644 --- a/client/tests/unit/slices/assignmentsSlice.test.ts +++ b/client/tests/unit/slices/assignmentsSlice.test.ts @@ -427,6 +427,46 @@ describe('assignmentsSlice', () => { }); }); + describe('setAssignmentNotes', () => { + const visit = () => buildAssignment({ id: 5, day_id: 1, place_id: 10, notes: 'old', assignment_time: '09:00' }); + + it('FE-ASSIGN-030: shows the note at once and keeps the one the server saved', async () => { + seedStore(useTripStore, { assignments: { '1': [visit()] } }); + let seen: unknown; + server.use( + http.put('/api/trips/1/assignments/5/notes', async ({ request }) => { + seen = await request.json(); + expect(useTripStore.getState().assignments['1'][0].notes).toBe('Bring cash'); + return HttpResponse.json({ assignment: { ...visit(), notes: 'Bring cash' } }); + }), + ); + + await useTripStore.getState().setAssignmentNotes(1, 1, 5, 'Bring cash'); + + expect(seen).toEqual({ notes: 'Bring cash' }); + expect(useTripStore.getState().assignments['1'][0]).toMatchObject({ notes: 'Bring cash', assignment_time: '09:00' }); + }); + + it('FE-ASSIGN-031: a refused write puts the old note back', async () => { + seedStore(useTripStore, { assignments: { '1': [visit()] } }); + server.use(http.put('/api/trips/1/assignments/5/notes', () => HttpResponse.json({ error: 'no' }, { status: 403 }))); + + await expect(useTripStore.getState().setAssignmentNotes(1, 1, 5, null)).rejects.toBeTruthy(); + + expect(useTripStore.getState().assignments['1'][0].notes).toBe('old'); + }); + + it('FE-ASSIGN-032: a visit it cannot find, or one not yet saved, is left alone', async () => { + const unsaved = { ...visit(), id: -3 }; + seedStore(useTripStore, { assignments: { '1': [unsaved] } }); + + await useTripStore.getState().setAssignmentNotes(1, 1, 5, 'x'); + await useTripStore.getState().setAssignmentNotes(1, 1, -3, 'x'); + + expect(useTripStore.getState().assignments['1'][0]).toEqual(unsaved); + }); + }); + describe('setAssignmentTimes', () => { // The pool place still carries an End of its own, which the visit shows once its // own override is gone, exactly as a fresh read of the day would.