diff --git a/client/src/components/Files/fileActions.test.ts b/client/src/components/Files/fileActions.test.ts index 0de70ac24..2b3bac87a 100644 --- a/client/src/components/Files/fileActions.test.ts +++ b/client/src/components/Files/fileActions.test.ts @@ -1,10 +1,16 @@ -// FE-FILES-ACTIONS-001 to FE-FILES-ACTIONS-010: the paste and link rules the desktop file -// manager and the phone's files tab and link sheet share. +// FE-FILES-ACTIONS-001 to FE-FILES-ACTIONS-012: the paste and link rules the desktop file +// manager and the phone's files tab and link sheet share, with the id reading each keeps. import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { buildTripFile } from '../../../tests/helpers/factories'; import { filesApi } from '../../api/client'; -import { filesFromClipboard, planFileLinkToggle, runFileLinkRecordStep, toggleFileLink } from './fileActions'; +import { + PHONE_FILE_LINK_RULES, + filesFromClipboard, + planFileLinkToggle, + runFileLinkRecordStep, + toggleFileLink, +} from './fileActions'; function clipboard(items: { kind: string; file: File | null }[]): DataTransfer { return { items: items.map((i) => ({ kind: i.kind, getAsFile: () => i.file })) } as unknown as DataTransfer; @@ -54,6 +60,12 @@ describe('planFileLinkToggle', () => { expect(planFileLinkToggle(file, 'place_id', 5)).toEqual({ kind: 'removeLink' }); expect(planFileLinkToggle(file, 'reservation_id', 7)).toEqual({ kind: 'removeLink' }); }); + + it('FE-FILES-ACTIONS-011: a zero column is free on desktop and taken on the phone', () => { + const file = buildTripFile({ place_id: 0 }); + expect(planFileLinkToggle(file, 'place_id', 5)).toEqual({ kind: 'update', data: { place_id: 5 } }); + expect(planFileLinkToggle(file, 'place_id', 5, PHONE_FILE_LINK_RULES)).toEqual({ kind: 'addLink' }); + }); }); describe('running a link toggle', () => { @@ -80,9 +92,9 @@ describe('running a link toggle', () => { expect(filesApi.addLink).toHaveBeenNthCalledWith(2, 1, 9, { reservation_id: 7 }); }); - it('FE-FILES-ACTIONS-008: removing a record looks it up by the target, numeric or not', async () => { - await runFileLinkRecordStep(1, 9, 'place_id', 5, 'removeLink'); - await runFileLinkRecordStep(1, 9, 'reservation_id', 7, 'removeLink'); + it('FE-FILES-ACTIONS-008: on the phone a record is looked up by the numeric target', async () => { + await runFileLinkRecordStep(1, 9, 'place_id', 5, 'removeLink', PHONE_FILE_LINK_RULES); + await runFileLinkRecordStep(1, 9, 'reservation_id', 7, 'removeLink', PHONE_FILE_LINK_RULES); expect(filesApi.getLinks).toHaveBeenCalledWith(1, 9); expect(filesApi.removeLink).toHaveBeenNthCalledWith(1, 1, 9, 50); expect(filesApi.removeLink).toHaveBeenNthCalledWith(2, 1, 9, 70); @@ -95,13 +107,31 @@ describe('running a link toggle', () => { expect(filesApi.removeLink).not.toHaveBeenCalled(); }); + it('FE-FILES-ACTIONS-012: on desktop a record is looked up by the target as it comes', async () => { + await runFileLinkRecordStep(1, 9, 'place_id', 5, 'removeLink'); + await runFileLinkRecordStep(1, 9, 'reservation_id', 7, 'removeLink'); + expect(filesApi.removeLink).toHaveBeenCalledTimes(1); + expect(filesApi.removeLink).toHaveBeenCalledWith(1, 9, 70); + }); + it('FE-FILES-ACTIONS-010: toggleFileLink writes the column or the record, and rethrows', async () => { - await toggleFileLink(1, buildTripFile({ id: 9, place_id: null }), 'place_id', 5); + await toggleFileLink(1, buildTripFile({ id: 9, place_id: null }), 'place_id', 5, PHONE_FILE_LINK_RULES); expect(filesApi.update).toHaveBeenCalledWith(1, 9, { place_id: 5 }); - await toggleFileLink(1, buildTripFile({ id: 9, place_id: 1 }), 'place_id', 5); + await toggleFileLink(1, buildTripFile({ id: 9, place_id: 1 }), 'place_id', 5, PHONE_FILE_LINK_RULES); expect(filesApi.addLink).toHaveBeenCalledWith(1, 9, { place_id: 5 }); + await toggleFileLink(1, buildTripFile({ id: 9, place_id: 1, linked_place_ids: [5] }), 'place_id', 5); + expect(filesApi.removeLink).not.toHaveBeenCalled(); + await toggleFileLink( + 1, + buildTripFile({ id: 9, place_id: 1, linked_place_ids: [5] }), + 'place_id', + 5, + PHONE_FILE_LINK_RULES + ); + expect(filesApi.removeLink).toHaveBeenCalledWith(1, 9, 50); + vi.mocked(filesApi.update).mockRejectedValue(new Error('nope')); await expect(toggleFileLink(1, buildTripFile({ id: 9, reservation_id: 7 }), 'reservation_id', 7)).rejects.toThrow( 'nope' diff --git a/client/src/components/Files/fileActions.ts b/client/src/components/Files/fileActions.ts index 6f1df531c..1705dc03a 100644 --- a/client/src/components/Files/fileActions.ts +++ b/client/src/components/Files/fileActions.ts @@ -30,19 +30,37 @@ export type FileLinkStep = | { kind: 'addLink' } | { kind: 'removeLink' }; +/** + * How a caller reads the ids, which differs between the views. The phone link sheet treats + * only a null column as free and finds a link record by the numeric value of its id; the + * desktop manager treats any empty column as free and compares the record id as it comes. + */ +export interface FileLinkRules { + freeOnlyWhenNull?: boolean; + matchRecordsByNumber?: boolean; +} + +export const PHONE_FILE_LINK_RULES: FileLinkRules = { freeOnlyWhenNull: true, matchRecordsByNumber: true }; + function linkedIds(file: TripFile, field: FileLinkField): number[] { const own = file[field]; const extra = field === 'place_id' ? file.linked_place_ids : file.linked_reservation_ids; return [...(own != null ? [own] : []), ...(extra || []).filter((id) => id != null)]; } -export function planFileLinkToggle(file: TripFile, field: FileLinkField, targetId: number): FileLinkStep { +export function planFileLinkToggle( + file: TripFile, + field: FileLinkField, + targetId: number, + rules: FileLinkRules = {} +): FileLinkStep { const column = (value: number | null) => (field === 'place_id' ? { place_id: value } : { reservation_id: value }); if (linkedIds(file, field).includes(targetId)) { if (file[field] === targetId) return { kind: 'update', data: column(null) }; return { kind: 'removeLink' }; } - if (!file[field]) return { kind: 'update', data: column(targetId) }; + const free = rules.freeOnlyWhenNull ? file[field] == null : !file[field]; + if (free) return { kind: 'update', data: column(targetId) }; return { kind: 'addLink' }; } @@ -58,7 +76,8 @@ export async function runFileLinkRecordStep( fileId: number, field: FileLinkField, targetId: number, - step: 'addLink' | 'removeLink' + step: 'addLink' | 'removeLink', + rules: FileLinkRules = {} ): Promise { if (step === 'addLink') { await filesApi.addLink( @@ -69,7 +88,9 @@ export async function runFileLinkRecordStep( return; } const linksRes = (await filesApi.getLinks(tripId, fileId)) as { links?: FileLinkRecord[] }; - const link = (linksRes.links || []).find((l) => Number(l[field]) === targetId); + const link = (linksRes.links || []).find((l) => + rules.matchRecordsByNumber ? Number(l[field]) === targetId : l[field] === targetId + ); if (link) await filesApi.removeLink(tripId, fileId, link.id); } @@ -78,9 +99,10 @@ export async function toggleFileLink( tripId: number, file: TripFile, field: FileLinkField, - targetId: number + targetId: number, + rules: FileLinkRules = {} ): Promise { - const step = planFileLinkToggle(file, field, targetId); + const step = planFileLinkToggle(file, field, targetId, rules); if (step.kind === 'update') await filesApi.update(tripId, file.id, step.data); - else await runFileLinkRecordStep(tripId, file.id, field, targetId, step.kind); + else await runFileLinkRecordStep(tripId, file.id, field, targetId, step.kind, rules); } diff --git a/client/src/mobile/screens/trip/tabs/MFileLinkSheet.tsx b/client/src/mobile/screens/trip/tabs/MFileLinkSheet.tsx index 8200d3ceb..4d623e4e6 100644 --- a/client/src/mobile/screens/trip/tabs/MFileLinkSheet.tsx +++ b/client/src/mobile/screens/trip/tabs/MFileLinkSheet.tsx @@ -2,7 +2,7 @@ import { useRef, useState } from 'react' import { Check, Link2, Loader2, MapPin, Ticket, TrainFront } from 'lucide-react' import type { LucideIcon } from 'lucide-react' import MSheet from '../../../components/MSheet' -import { toggleFileLink, type FileLinkField } from '../../../../components/Files/fileActions' +import { PHONE_FILE_LINK_RULES, toggleFileLink, type FileLinkField } from '../../../../components/Files/fileActions' import type { TripFile } from '../../../../types' import type { TripPlanner } from '../MTripShell' import { Eyebrow, TileHeader } from '../sheets/MTripSheetUi' @@ -48,7 +48,7 @@ export default function MFileLinkSheet({ planner, file, onClose }: MFileLinkShee const key = `${field === 'place_id' ? 'p' : 'r'}${targetId}` setBusyKey(key) try { - await toggleFileLink(tripId, shown, field, targetId) + await toggleFileLink(tripId, shown, field, targetId, PHONE_FILE_LINK_RULES) refresh() } catch { toast.error(t('files.toast.assignError'))