From a910c2a837bcebfbcec7935732c3d55b416cceca Mon Sep 17 00:00:00 2001 From: Maurice Date: Wed, 23 Sep 2026 17:39:25 +0200 Subject: [PATCH] fix: follow-ups from the review of the price, Immich, moving day and import fixes Saved place price (#2471) - A price above zero that would be left without a currency is refused with a 400, over REST and through update_collection_place alike. A list has no base currency, so such an amount was read in each viewer's own default currency and went into a trip as the trip's. A price sent alone keeps the stored currency, and a zero price or clearing both fields still goes through. The check sits in the service because the MCP tool spreads the contract's shape, where a refine would never arrive. - An older row that holds an amount without a currency shows it as a bare number instead of in the viewer's own currency. - The Hungarian hint uses the informal register of its file, which is now prettier clean. Immich certificate switch (#2475) - The switch trusts one server. Saving a different URL without sending it turns it off; saving the same URL keeps it as before. The wiki says so. Moving day (#2476) - Google Maps and CoMaps are left out only when there is nothing to hand over. A day with a single stop opens it as a pin again, on the desktop and on the phone, as it did before. - Desktop: a moving day whose flight or train has no stations shows no route tools, since there is nothing left to draw. With stations they stay. - Phone: the drive between two stays on a day without stops or bookings shows once at the top instead of again at the bottom. - The export helpers take the carrier flags as one object instead of two booleans in a row. Import card (#2477) - The success and error marks use the theme tokens. - The e2e test names its upload with a made-up booking number. Tests: COLLECTIONS-SVC-132 and 133, COLLECTIONS-E2E-088, update_collection_place with a price and no currency, FE-COMP-COLEXTRAS-013 (011 adjusted), IMMICH-TLS-010, IMMICH-103 extended, FE-PLANNER-DAYPLAN-222 adjusted and 224, FE-MOB-PLTL-051, FE-MOB-PTLM-047, FE-W4BGT-023 and 024 check the mark colours. --- .../BackgroundTasksWidget.extra.test.tsx | 5 ++- .../BackgroundTasks/BackgroundTasksWidget.tsx | 4 +- .../useCollectionPlaceExtras.test.ts | 16 +++++-- .../Collections/useCollectionPlaceExtras.ts | 13 +++++- .../Planner/DayPlanSidebar.test.tsx | 30 +++++++++++-- .../src/components/Planner/DayPlanSidebar.tsx | 26 +++++++----- .../mobile/screens/trip/lib/dayRoute.test.ts | 16 +++---- .../src/mobile/screens/trip/lib/dayRoute.ts | 42 +++++++++++-------- .../screens/trip/plan/planTimelineModel.ts | 7 +++- .../screens/trip/plan/useMPlanTimeline.ts | 33 +++++++-------- .../mobile/screens/trip/sheets/MDaySheet.tsx | 4 +- .../mobile/trip/planTimelineModel.test.ts | 10 ++++- .../unit/mobile/trip/useMPlanTimeline.test.ts | 20 ++++++++- .../nest/collections/collections.service.ts | 18 ++++++++ server/src/nest/memories/immich.service.ts | 22 +++++----- server/tests/e2e/collections.e2e.test.ts | 11 +++++ .../tests/integration/memories-immich.test.ts | 8 +++- .../tests/unit/mcp/tools-collections.test.ts | 19 +++++++++ .../unit/nest/collections.service.test.ts | 25 +++++++++++ server/tests/unit/nest/immich.service.test.ts | 17 ++++++++ shared/src/i18n/hu/collection.ts | 5 ++- wiki/Photo-Providers.md | 2 +- 22 files changed, 269 insertions(+), 84 deletions(-) diff --git a/client/src/components/BackgroundTasks/BackgroundTasksWidget.extra.test.tsx b/client/src/components/BackgroundTasks/BackgroundTasksWidget.extra.test.tsx index 7c3928794..b48fe3221 100644 --- a/client/src/components/BackgroundTasks/BackgroundTasksWidget.extra.test.tsx +++ b/client/src/components/BackgroundTasks/BackgroundTasksWidget.extra.test.tsx @@ -290,7 +290,7 @@ describe('BackgroundTasksWidget: the mark on a finished job (#2477)', () => { const { baseElement } = render() expect(screen.getByText('No reservations could be extracted from the uploaded files.')).toBeInTheDocument() - expect(baseElement.querySelector('svg.lucide-alert-triangle')).not.toBeNull() + expect(baseElement.querySelector('svg.lucide-alert-triangle')).toHaveAttribute('stroke', 'var(--warning)') expect(baseElement.querySelector('svg.lucide-check-circle2')).toBeNull() expect(baseElement.querySelector('.animate-spin')).toBeNull() }) @@ -299,7 +299,8 @@ describe('BackgroundTasksWidget: the mark on a finished job (#2477)', () => { useBackgroundTasksStore.setState({ tasks: [task({ items: [{ id: 1 }] as never, warnings: ['could not locate "X"'] })] }) const { baseElement } = render() - expect(baseElement.querySelector('svg.lucide-check-circle2')).not.toBeNull() + // The theme's own success colour, so it follows light and dark. + expect(baseElement.querySelector('svg.lucide-check-circle2')).toHaveAttribute('stroke', 'var(--success)') expect(baseElement.querySelector('svg.lucide-alert-triangle')).toBeNull() }) }) diff --git a/client/src/components/BackgroundTasks/BackgroundTasksWidget.tsx b/client/src/components/BackgroundTasks/BackgroundTasksWidget.tsx index 6db1ea821..d6de54f34 100644 --- a/client/src/components/BackgroundTasks/BackgroundTasksWidget.tsx +++ b/client/src/components/BackgroundTasks/BackgroundTasksWidget.tsx @@ -141,12 +141,12 @@ export default function BackgroundTasksWidget() { >
{(task.status === 'running' || (task.status === 'done' && task.items === undefined)) && } - {task.status === 'done' && task.items !== undefined && task.items.length > 0 && } + {task.status === 'done' && task.items !== undefined && task.items.length > 0 && } {/* A parse that found nothing is not a success, whatever the job status says: the same card used to show a green tick next to "no reservations could be extracted" (#2477). */} {task.status === 'done' && task.items?.length === 0 && } - {task.status === 'error' && } + {task.status === 'error' && }
diff --git a/client/src/components/Collections/useCollectionPlaceExtras.test.ts b/client/src/components/Collections/useCollectionPlaceExtras.test.ts index c9be71dca..ca7a5a32d 100644 --- a/client/src/components/Collections/useCollectionPlaceExtras.test.ts +++ b/client/src/components/Collections/useCollectionPlaceExtras.test.ts @@ -1,4 +1,4 @@ -// FE-COMP-COLEXTRAS-001 to FE-COMP-COLEXTRAS-012: price, website and phone of a saved place (#2471). +// FE-COMP-COLEXTRAS-001 to FE-COMP-COLEXTRAS-013: price, website and phone of a saved place (#2471). import { act, renderHook } from '@testing-library/react' import { useSettingsStore } from '../../store/settingsStore' import { resetAllStores } from '../../../tests/helpers/store' @@ -8,6 +8,7 @@ import { extrasPatch, parsePriceInput, phoneHref, + priceLabel, useCollectionPlaceExtras, type CollectionPlaceExtrasDraft, } from './useCollectionPlaceExtras' @@ -87,6 +88,15 @@ describe('phoneHref', () => { }) }) +describe('priceLabel', () => { + it('FE-COMP-COLEXTRAS-013: names the stored currency, and without one shows the bare amount whatever the viewer uses', () => { + expect(priceLabel(2000, 'JPY', 'en-US')).toBe(formatMoney(2000, 'JPY', 'en-US')) + expect(priceLabel(2000, null, 'en-US')).toBe('2,000') + expect(priceLabel(12.5, '', 'de-DE')).toBe('12,5') + expect(priceLabel(12.5, undefined, 'de-DE')).not.toMatch(/€|\$|EUR|USD/) + }) +}) + describe('useCollectionPlaceExtras', () => { it('FE-COMP-COLEXTRAS-011: formats the stored price for read mode, only above zero, and guards the website', () => { withDefaultCurrency('CHF') @@ -96,9 +106,9 @@ describe('useCollectionPlaceExtras', () => { expect(paid.result.current.phone).toBe('+41 44') expect(paid.result.current.phoneHref).toBe('tel:+4144') - // No currency stored: the user's own default names it. + // No currency stored: a bare amount, not the viewer's own default currency. const bare = renderHook(() => useCollectionPlaceExtras({ price: 8, currency: null })) - expect(bare.result.current.priceLabel).toBe(formatMoney(8, 'CHF', 'en-US')) + expect(bare.result.current.priceLabel).toBe('8') const free = renderHook(() => useCollectionPlaceExtras({ price: 0, currency: 'EUR', website: 'javascript:alert(1)', phone: ' ' })) expect(free.result.current.priceLabel).toBeNull() diff --git a/client/src/components/Collections/useCollectionPlaceExtras.ts b/client/src/components/Collections/useCollectionPlaceExtras.ts index cc25fc391..140fb69b4 100644 --- a/client/src/components/Collections/useCollectionPlaceExtras.ts +++ b/client/src/components/Collections/useCollectionPlaceExtras.ts @@ -69,6 +69,17 @@ export function extrasPatch(initial: CollectionPlaceExtrasDraft, draft: Collecti return patch } +/** + * The read-mode price. An older row can hold an amount without a currency (a + * trip place saved before it had one); it shows as a bare number rather than + * in the viewer's own default currency, which would name a different currency + * for every member of a shared list. + */ +export function priceLabel(price: number, currency: string | null | undefined, locale: string): string { + if (currency) return formatMoney(price, currency, locale) + return new Intl.NumberFormat(locale || undefined, { maximumFractionDigits: 2 }).format(price) +} + /** A tel: link keeps the digits and a leading plus, nothing a dialer would choke on. */ export function phoneHref(phone: string): string { return `tel:${phone.replace(/[^\d+]/g, '')}` @@ -115,7 +126,7 @@ export function useCollectionPlaceExtras(place: ExtrasSource | null | undefined) options: currenciesWith(draft.currency).map(c => ({ value: c, label: SYMBOLS[c] ? `${c} ${SYMBOLS[c]}` : c })), }, // Read mode: a free place shows no price, the same rule the trip inspector follows. - priceLabel: price > 0 ? formatMoney(price, place?.currency || defaultCurrency || 'EUR', locale) : null, + priceLabel: price > 0 ? priceLabel(price, place?.currency, locale) : null, websiteHref: safeHttpUrl(place?.website), phone, phoneHref: phone ? phoneHref(phone) : null, diff --git a/client/src/components/Planner/DayPlanSidebar.test.tsx b/client/src/components/Planner/DayPlanSidebar.test.tsx index 2b77772de..bcd440d15 100644 --- a/client/src/components/Planner/DayPlanSidebar.test.tsx +++ b/client/src/components/Planner/DayPlanSidebar.test.tsx @@ -3959,9 +3959,12 @@ describe('DayPlanSidebar', () => { const { unmount } = render() - // The route itself can still be shown; only the hand-offs, which could only - // ever describe a drive from Munich to Hamburg, are gone. - expect(screen.getByRole('button', { name: 'Route' })).toBeInTheDocument() + // With its airports the route to and from them can still be shown; without + // them there is nothing left to draw, so the tools go as a whole instead of + // standing there dead. The hand-offs, which could only ever describe a drive + // from Munich to Hamburg, are gone either way. + if (located) expect(screen.getByRole('button', { name: 'Route' })).toBeInTheDocument() + else expect(screen.queryByRole('button', { name: 'Route' })).not.toBeInTheDocument() expect(screen.queryByRole('button', { name: 'Open in Google Maps' })).not.toBeInTheDocument() expect(screen.queryByRole('button', { name: 'Open in CoMaps' })).not.toBeInTheDocument() unmount() @@ -3978,6 +3981,27 @@ describe('DayPlanSidebar', () => { expect(pairs).toContainEqual([{ lat: 53.63, lng: 9.988 }, { lat: 53.551, lng: 9.993 }]) expect(pairs).not.toContainEqual([{ lat: 48.137, lng: 11.575 }, { lat: 53.551, lng: 9.993 }]) }) + + it('FE-PLANNER-DAYPLAN-224: a day with a single located stop still hands it over as a pin', async () => { + // Two stops, one of them without coordinates, and no stay: the route tools show, + // and the hand-offs open the one stop the way they always did. + const user = userEvent.setup() + const { generateGoogleMapsUrl } = await import('../Map/RouteCalculator') + const openSpy = vi.spyOn(window, 'open').mockReturnValue(null) + const day = buildDay({ id: 10, date: '2026-11-03', title: 'Day 1' }) + const assignments = { + '10': [ + buildAssignment({ id: 1, day_id: 10, order_index: 0, place: buildPlace({ id: 1, name: 'Elbphilharmonie', lat: 53.541, lng: 9.984 }) }), + buildAssignment({ id: 2, day_id: 10, order_index: 1, place: buildPlace({ id: 2, name: 'Somewhere nice', lat: null, lng: null }) }), + ], + } + render() + + expect(screen.getByRole('button', { name: 'Open in CoMaps' })).toBeInTheDocument() + await user.click(screen.getByRole('button', { name: 'Open in Google Maps' })) + expect(vi.mocked(generateGoogleMapsUrl)).toHaveBeenCalledWith([{ lat: 53.541, lng: 9.984, name: 'Elbphilharmonie' }]) + openSpy.mockRestore() + }) }) it('FE-PLANNER-DAYPLAN-169: picking a whole-day travel mode persists it and redraws the map', async () => { diff --git a/client/src/components/Planner/DayPlanSidebar.tsx b/client/src/components/Planner/DayPlanSidebar.tsx index bd4f23bc5..bff3a255b 100644 --- a/client/src/components/Planner/DayPlanSidebar.tsx +++ b/client/src/components/Planner/DayPlanSidebar.tsx @@ -1628,15 +1628,19 @@ const DayPlanSidebar = React.memo(function DayPlanSidebar(props: DayPlanSidebarP // distinct bookend hotels you actually slept in / sleep in tonight — so the route // tools appear when you click the day (#1297). A same-hotel rest day or a plain // arrival/departure day has morning === evening and stays excluded. With a flight - // or train booked on it the map drops that leg and keeps only the drives to and - // from located stations, and the exports below have nothing to hand over (#2476). + // or train booked on it the map drops that leg (#2476): it keeps only the drives + // to and from the booking's located stations, and a booking without any leaves + // nothing to draw, so the tools stay away instead of sitting there dead. const transferMorning = routeBookends?.morning const transferEvening = routeBookends?.evening + const dayCarriers = (mergedItemsMap[day.id] || []).filter(i => i.type === 'transport' && isCarrierTransport(i.data)) + const dayHasLocatedCarrier = dayCarriers.some(i => hasCarrierEndpointOnDay(i.data, day.id)) const hasHotelTransfer = !!( routeBookends?.morningIsSleptHere && routeBookends?.eveningIsOvernight && transferMorning?.place_lat != null && transferMorning?.place_lng != null && transferEvening?.place_lat != null && transferEvening?.place_lng != null && - (transferMorning.place_lat !== transferEvening.place_lat || transferMorning.place_lng !== transferEvening.place_lng) + (transferMorning.place_lat !== transferEvening.place_lat || transferMorning.place_lng !== transferEvening.place_lng) && + (dayCarriers.length === 0 || dayHasLocatedCarrier) ) const routeToolsRoutable = da.length >= 2 || (loc != null && hasRouteBookend) || hasHotelTransfer /** @@ -1652,16 +1656,15 @@ const DayPlanSidebar = React.memo(function DayPlanSidebar(props: DayPlanSidebarP // A flight, train, ferry or coach on a day without stops is the move itself, // located or not: the hotels at either end are joined by it, not by a road // worth handing to a map app (#2476). - if (dayStops.length === 0 && (mergedItemsMap[day.id] || []).some(i => i.type === 'transport' && isCarrierTransport(i.data))) return [] + if (dayStops.length === 0 && dayCarriers.length > 0) return [] const stops = dayStops.map(a => ({ lat: a.place!.lat!, lng: a.place!.lng!, name: a.place!.name })) const first = dayStops[0] ? { isPlace: true, time: dayStops[0].place?.place_time ?? null, lat: dayStops[0].place!.lat!, lng: dayStops[0].place!.lng! } : undefined const lastAssignment = dayStops[dayStops.length - 1] const last = lastAssignment ? { isPlace: true, time: lastAssignment.place?.place_time ?? null, lat: lastAssignment.place!.lat!, lng: lastAssignment.place!.lng! } : undefined // Same carrier gate as the drawn route (#2157): the exported link must not // start at a hotel you only reach tonight or lead back to one you left. - const dayHasCarrier = (mergedItemsMap[day.id] || []).some(i => i.type === 'transport' && hasCarrierEndpointOnDay(i.data, day.id)) - const drawMorning = !!routeBookends && shouldDrawMorningLeg(routeBookends, day, first, dayHasCarrier) - const drawEvening = !!routeBookends && shouldDrawEveningLeg(routeBookends, day, last, dayHasCarrier) + const drawMorning = !!routeBookends && shouldDrawMorningLeg(routeBookends, day, first, dayHasLocatedCarrier) + const drawEvening = !!routeBookends && shouldDrawEveningLeg(routeBookends, day, last, dayHasLocatedCarrier) const morning = drawMorning && routeBookends?.morning?.place_lat != null && routeBookends?.morning?.place_lng != null ? { lat: routeBookends.morning.place_lat, lng: routeBookends.morning.place_lng, name: routeBookends.morning.place_name } : null const evening = drawEvening && routeBookends?.evening?.place_lat != null && routeBookends?.evening?.place_lng != null @@ -1669,8 +1672,9 @@ const DayPlanSidebar = React.memo(function DayPlanSidebar(props: DayPlanSidebarP return [...(morning ? [morning] : []), ...stops, ...(evening ? [evening] : [])] } const showRouteTools = (isSelected || (showRouteToolsWhenExpanded && isExpanded)) && routeToolsRoutable - // Built once, for the day the tools show on. A route needs two ends: with - // fewer the hand-offs would open a lone pin or nothing at all (#2476). + // Built once, for the day the tools show on. With no stop to hand over the + // hand-offs would open nothing, so they are left out; a single stop still + // opens as a pin (#2476). const exportStops = showRouteTools ? dayExportStops() : [] // Is this day's inline route currently on? Mobile toggles it per day (its // own expandedRouteDayIds entry); desktop uses the global Route toggle on @@ -2875,7 +2879,7 @@ const DayPlanSidebar = React.memo(function DayPlanSidebar(props: DayPlanSidebarP {!narrowPanel && t('dayplan.route')} {/* Open the day's stops as a route in Google Maps (planned order). #1255 */} - {exportStops.length >= 2 && + {exportStops.length > 0 &&