fix(tours): prevent duplicate day assignments

2026-10-18 21:16:47 +00:00 · 2026-10-05 23:53:11 +02:00
parent a9f9a23908
commit af7dbce105
14 changed files with 313 additions and 15 deletions
@@ -129,6 +129,36 @@ describe('ToursSidebar', () => {
    expect(onToursChanged).not.toHaveBeenCalled()
  })

+  it('disables already-assigned days and refreshes availability when the picker is reopened', async () => {
+    const dayOne = { id: 7, trip_id: 1, day_number: 1, title: 'Day one', date: null } as never
+    const dayTwo = { id: 8, trip_id: 1, day_number: 2, title: 'Day two', date: null } as never
+    const dayThree = { id: 9, trip_id: 1, day_number: 3, title: 'Day three', date: null } as never
+    useTripStore.setState({ assignments: { '7': [{ id: 1, day_id: 7, place_id: tour.place_id }] } as never })
+    const onAssignToDay = vi.fn((placeId: number, dayId: number) => {
+      useTripStore.setState(state => ({
+        assignments: { ...state.assignments, [String(dayId)]: [{ id: 2, day_id: dayId, place_id: placeId }] } as never,
+      }))
+      return true
+    })
+    render(<ToursSidebar canAssign tripId={1} days={[dayOne, dayTwo, dayThree]} tours={[tour]} onAssignToDay={onAssignToDay} onSelectTour={vi.fn()} />)
+
+    const openPicker = () => fireEvent.click(screen.getByRole('button', { name: /Add to day|Zum Tag hinzufügen/ }))
+    openPicker()
+    expect(screen.getByRole('button', { name: /Day one/ })).toBeDisabled()
+    expect(screen.getByRole('button', { name: /Day two/ })).toBeEnabled()
+    fireEvent.click(screen.getByRole('button', { name: /Day two/ }))
+    expect(onAssignToDay).toHaveBeenCalledTimes(1)
+    expect(onAssignToDay).toHaveBeenCalledWith(tour.place_id, 8)
+
+    openPicker()
+    expect(screen.getByRole('button', { name: /Day one/ })).toBeDisabled()
+    expect(screen.getByRole('button', { name: /Day two/ })).toBeDisabled()
+    expect(screen.getByRole('button', { name: /Day three/ })).toBeEnabled()
+    fireEvent.click(screen.getByRole('button', { name: /Day three/ }))
+    expect(onAssignToDay).toHaveBeenCalledTimes(2)
+    expect(onAssignToDay).toHaveBeenLastCalledWith(tour.place_id, 9)
+  })
+
  it('neither refreshes nor reports success when assignment fails', async () => {
    toast.success.mockClear()
    const onToursChanged = vi.fn()
@@ -178,7 +178,7 @@ export default function ToursSidebar({ tripId, days, tours, loading = false, sel
                tour={tour}
                selected={tour.place_id === selectedPlaceId}
                onSelect={(selectedTour, opener) => onSelectTour?.(selectedTour, opener)}
-                action={<TourDayMenu days={days} label={t('tours.addToDay')} disabled={!canAssign} onPick={(day, index) => { void handleAssign(tour, day, index) }} />}
+                action={<TourDayMenu days={days} placeId={tour.place_id} label={t('tours.addToDay')} disabled={!canAssign} onPick={(day, index) => { void handleAssign(tour, day, index) }} />}
              />
            ))}
          </ul>
@@ -471,6 +471,7 @@ export function TourPlannerToursRail({ planner, tours, days, loading, onAssignTo
                variant="button"
                primary
                days={days}
+                placeId={planner.saveOutcome.place_id}
                label={t('tours.planner.assignToDay')}
                disabled={!canAssign || assigning}
                onPick={day => { void assignSavedTour(day.id) }}
@@ -498,6 +499,7 @@ export function TourPlannerToursRail({ planner, tours, days, loading, onAssignTo
                    action={canAssign && days.length > 0 ? (
                      <TourDayMenu
                        days={days}
+                        placeId={tour.place_id}
                        label={`${t('tours.addToDay')}: ${tour.name}`}
                        onPick={day => { if (canAssign) void onAssignToDay(tour.place_id, day.id) }}
                      />
@@ -10,6 +10,7 @@ import { formatDate } from '../../utils/formatters'
 import { ContextMenu, useContextMenu } from '../shared/ContextMenu'
 import { DialogButton, fs } from '../shared/DialogShell'
 import { Tooltip } from '../shared/Tooltip'
+import { useTripStore } from '../../store/tripStore'
 import { BOX, Eyebrow } from '../Planner/bookings/bookingParts'
 import { SoftPill } from '../Planner/planParts'
 import { hikeSourceBadgeLabel } from './tourPresentation'
@@ -73,8 +74,9 @@ function dayMenuLabel(day: Day, index: number, t: Translate, locale: string): st
 * Putting a tour on a day: a round "+" on a row, or a footer button with a
 * chevron, both opening the trip's days as a menu.
 */
-export function TourDayMenu({ days, onPick, disabled = false, label, variant = 'round', primary = false }: {
+export function TourDayMenu({ days, placeId, onPick, disabled = false, label, variant = 'round', primary = false }: {
  days: Day[]
+  placeId?: number
  onPick: (day: Day, index: number) => void
  disabled?: boolean
  label: string
@@ -83,9 +85,16 @@ export function TourDayMenu({ days, onPick, disabled = false, label, variant = '
 }) {
  const { t, locale } = useTranslation()
  const menu = useContextMenu()
-  const items = days.map((day, index) => ({ label: dayMenuLabel(day, index, t, locale), onClick: () => onPick(day, index) }))
+  const assignments = useTripStore(state => state.assignments ?? {})
+  const assigned = (dayId: number) => placeId != null && (assignments[String(dayId)] ?? []).some(assignment => assignment.place_id === placeId)
+  const items = days.map((day, index) => ({
+    label: dayMenuLabel(day, index, t, locale),
+    disabled: assigned(day.id),
+    onClick: () => onPick(day, index),
+  }))
+  const hasAvailableDay = items.some(item => !item.disabled)
  const open = (event: React.MouseEvent<HTMLButtonElement>) => {
-    if (disabled || items.length === 0) return
+    if (disabled || !hasAvailableDay) return
    if (menu.menu) { event.stopPropagation(); menu.close(); return }
    menu.open(event, items, true)
  }
@@ -93,13 +102,13 @@ export function TourDayMenu({ days, onPick, disabled = false, label, variant = '
    <>
      {variant === 'round' ? (
        <Tooltip label={label} disabled={!!menu.menu}>
-          <button type="button" onClick={open} disabled={disabled || items.length === 0} aria-label={label} aria-haspopup="menu" aria-expanded={!!menu.menu}
+          <button type="button" onClick={open} disabled={disabled || !hasAvailableDay} aria-label={label} aria-haspopup="menu" aria-expanded={!!menu.menu}
            className={`grid h-[26px] w-[26px] flex-none place-items-center rounded-full shadow-sm ring-1 transition-colors disabled:cursor-default disabled:opacity-40 ${menu.menu ? 'bg-accent text-accent-text ring-transparent' : 'bg-surface-card text-content-muted ring-edge-faint enabled:hover:bg-accent enabled:hover:text-accent-text enabled:hover:ring-transparent'}`}>
            <Plus size={13} strokeWidth={2.4} />
          </button>
        </Tooltip>
      ) : (
-        <DialogButton variant={primary ? 'primary' : 'secondary'} onClick={open} disabled={disabled || items.length === 0}
+        <DialogButton variant={primary ? 'primary' : 'secondary'} onClick={open} disabled={disabled || !hasAvailableDay}
          aria-haspopup="menu" aria-expanded={!!menu.menu} icon={<Plus size={14} strokeWidth={2} />}>
          {label}
          <ChevronDown size={13} strokeWidth={2.2} className={`transition-transform ${menu.menu ? 'rotate-180' : ''}`} />
@@ -79,4 +79,13 @@ describe('ContextMenu', () => {
    });
    expect(onClose).toHaveBeenCalledOnce();
  });
+
+  it('does not activate a disabled menu item', () => {
+    const onClick = vi.fn()
+    render(<ContextMenu menu={makeMenu(100, 200, [{ label: 'Already assigned', onClick, disabled: true }])} onClose={onClose} />)
+    const item = screen.getByRole('button', { name: 'Already assigned' })
+    expect(item).toBeDisabled()
+    fireEvent.click(item)
+    expect(onClick).not.toHaveBeenCalled()
+  })
 });
@@ -6,6 +6,7 @@ interface MenuItem {
  label?: string
  icon?: LucideIcon
  onClick?: () => void
+  disabled?: boolean
  danger?: boolean
  divider?: boolean
 }
@@ -84,15 +85,15 @@ export function ContextMenu({ menu, onClose }: ContextMenuProps) {
        if (item.divider) return <div key={i} style={{ height: 1, background: 'var(--border-faint)', margin: '3px 6px' }} />
        const Icon = item.icon
        return (
-          <button type="button" key={i} onClick={() => { item.onClick?.(); onClose() }} style={{
+          <button type="button" key={i} disabled={item.disabled} onClick={() => { if (item.disabled) return; item.onClick?.(); onClose() }} style={{
            display: 'flex', alignItems: 'center', gap: 8, width: '100%',
            padding: '7px 10px', borderRadius: 7, border: 'none',
-            background: 'none', cursor: 'pointer', fontFamily: 'inherit',
+            background: 'none', cursor: item.disabled ? 'default' : 'pointer', fontFamily: 'inherit',
            fontSize: 'calc(12px * var(--fs-scale-body, 1))', fontWeight: 500, textAlign: 'left',
-            color: item.danger ? '#ef4444' : 'var(--text-primary)',
+            color: item.danger ? '#ef4444' : 'var(--text-primary)', opacity: item.disabled ? 0.45 : 1,
            transition: 'background 0.1s',
          }}
-            onMouseEnter={e => e.currentTarget.style.background = item.danger ? 'rgba(239,68,68,0.08)' : 'var(--bg-hover)'}
+            onMouseEnter={e => { if (!item.disabled) e.currentTarget.style.background = item.danger ? 'rgba(239,68,68,0.08)' : 'var(--bg-hover)' }}
            onMouseLeave={e => e.currentTarget.style.background = 'none'}
          >
            {Icon && <Icon size={13} style={{ flexShrink: 0, color: item.danger ? '#ef4444' : 'var(--text-faint)' }} />}
@@ -31,6 +31,7 @@ export default function MBrowseActionsSheet({ planner, shell }: MTripSheetsProps
  const openSavePicker = useSaveToCollectionStore(s => s.open)

  const [daysOpen, setDaysOpen] = useState(false)
+  const pendingTourDayIds = useRef(new Set<string>())
  useEffect(() => { setDaysOpen(open && Boolean(payload.dayPicker)) }, [open, payload.dayPicker])

  // Hold the last place so the card content survives the exit animation.
@@ -57,9 +58,20 @@ export default function MBrowseActionsSheet({ planner, shell }: MTripSheetsProps
    openSavePicker(collectionTargetFromPlace(place))
  }

+  const tourAlreadyAssignedToDay = (dayId: number) => isTourPlace(place.id)
+    && (planner.assignments[String(dayId)] ?? []).some(assignment => assignment.place_id === place.id)
+
  const assignToDay = async (dayId: number) => {
-    const assigned = await planner.handleAssignToDay(place.id, dayId)
-    if (assigned !== false) shell.closeSheet()
+    const isTour = isTourPlace(place.id)
+    const pendingKey = `${place.id}:${dayId}`
+    if (isTour && (tourAlreadyAssignedToDay(dayId) || pendingTourDayIds.current.has(pendingKey))) return
+    if (isTour) pendingTourDayIds.current.add(pendingKey)
+    try {
+      const assigned = await planner.handleAssignToDay(place.id, dayId)
+      if (assigned !== false) shell.closeSheet()
+    } finally {
+      if (isTour) pendingTourDayIds.current.delete(pendingKey)
+    }
  }

  const deletePlace = () => {
@@ -112,8 +124,9 @@ export default function MBrowseActionsSheet({ planner, shell }: MTripSheetsProps
                  <button
                    key={d.id}
                    type="button"
+                    disabled={tourAlreadyAssignedToDay(d.id) || (isTourPlace(place.id) && pendingTourDayIds.current.has(`${place.id}:${d.id}`))}
                    onClick={() => { void assignToDay(d.id) }}
-                    className={`flex w-full items-center gap-2 px-3 py-[10px] text-left ${i > 0 ? 'border-t border-[color:var(--m-rowbr)]' : ''}`}
+                    className={`flex w-full items-center gap-2 px-3 py-[10px] text-left disabled:cursor-default disabled:opacity-40 ${i > 0 ? 'border-t border-[color:var(--m-rowbr)]' : ''}`}
                  >
                    <span className="min-w-0 flex-1 truncate text-[0.78125rem] font-semibold">
                      {/* A day_number of 0 is as unusable as a missing one — both take the row position. */}
@@ -1567,6 +1567,37 @@ describe('useTripPlanner — place CRUD', () => {
 })

 describe('useTripPlanner — day plan CRUD', () => {
+  it('does not write a Tour assignment twice to one day, but allows another day and preserves Place writes', async () => {
+    const tour: TourListItem = {
+      place_id: 1, name: 'Ridge walk', tour_type: 'hike', distance: 4, elevation_gain: 100,
+      elevation_loss: 80, duration: null, difficulty: null, wanderer_ref: null, match_confidence: 1,
+      tour_group_id: null, max_hiking_difficulty: 2, planned: false, caution: false,
+    }
+    const tourPlace = buildPlace({ id: 1, name: 'Ridge walk', tour_place_id: 1 })
+    const ordinaryPlace = buildPlace({ id: 2, name: 'Ordinary place' })
+    useAddonStore.setState({ addons: [{ id: 'tours', enabled: true } as never], loaded: true })
+    vi.mocked(addonsApi.enabled).mockResolvedValue({ addons: [{ id: 'tours' }] })
+    vi.spyOn(toursApi, 'list').mockResolvedValue({ tours: [tour] })
+    seedTrip({
+      places: [tourPlace, ordinaryPlace],
+      assignments: { '7': [buildAssignment({ id: 70, day_id: 7, place: tourPlace })] },
+    })
+
+    const { result } = await renderPlanner()
+    await waitFor(() => expect(result.current.tours).toEqual([tour]))
+    actions.assignPlaceToDay.mockClear()
+
+    let repeated!: boolean
+    await act(async () => { repeated = await result.current.handleAssignToDay(tourPlace.id, 7) })
+    expect(repeated).toBe(false)
+    expect(actions.assignPlaceToDay).not.toHaveBeenCalled()
+
+    await act(async () => { await result.current.handleAssignToDay(tourPlace.id, 8) })
+    await act(async () => { await result.current.handleAssignToDay(ordinaryPlace.id, 7) })
+    expect(actions.assignPlaceToDay).toHaveBeenNthCalledWith(1, 42, 8, tourPlace.id, undefined)
+    expect(actions.assignPlaceToDay).toHaveBeenNthCalledWith(2, 42, 7, ordinaryPlace.id, undefined)
+  })
+
  it('refreshes Tours only after a successful Tour assignment write', async () => {
    const unplanned: TourListItem = {
      place_id: 1, name: 'Ridge walk', tour_type: 'hike', distance: 4, elevation_gain: 100,
@@ -2527,6 +2527,7 @@ export function useTripPlanner() {
  const handleAssignToDay = useCallback(async (placeId: number, dayId?: number, position?: number) => {
    const target = dayId || selectedDayId
    if (!target) { toast.error(t('trip.toast.selectDay')); return false }
+    if (isTourPlace(placeId) && (storedAssignments[String(target)] ?? []).some(assignment => assignment.place_id === placeId)) return false
    const place = places.find(p => p.id === placeId)
    // A place with a start of its own is drawn by it, so it is stored there too, the
    // way a stop moved over from another day is. Without one it goes where it was put.
@@ -0,0 +1,109 @@
+import { describe, expect, it, vi } from 'vitest';
+import type { TripPlanner } from '../../../../src/mobile/screens/trip/MTripShell';
+import MBrowseActionsSheet from '../../../../src/mobile/screens/trip/sheets/MBrowseActionsSheet';
+import type { Place } from '../../../../src/types';
+import { buildPlanner, buildShell } from '../../../helpers/mobileTrip';
+import { act, fireEvent, render, screen, waitFor } from '../../../helpers/render';
+import { resetAllStores } from '../../../helpers/store';
+
+function tourPlace(): Place {
+  return {
+    id: 42,
+    trip_id: 1,
+    name: 'Ridge walk',
+    address: null,
+    description: null,
+    category_id: null,
+    lat: 48,
+    lng: 11,
+    image_url: null,
+    google_place_id: null,
+    osm_id: null,
+    route_geometry: '[[48,11],[49,12]]',
+    route_color: null,
+    tour_place_id: 42,
+  } as unknown as Place;
+}
+
+const days = [
+  { id: 7, trip_id: 1, day_number: 1, title: 'Day one', date: null },
+  { id: 8, trip_id: 1, day_number: 2, title: 'Day two', date: null },
+  { id: 9, trip_id: 1, day_number: 3, title: 'Day three', date: null },
+] as never;
+
+function mobilePlanner(overrides: Partial<TripPlanner> = {}) {
+  const place = tourPlace();
+  return buildPlanner({
+    places: [place],
+    days,
+    assignments: { '7': [{ id: 70, day_id: 7, place_id: place.id }] } as never,
+    isTourPlace: vi.fn((placeId: number) => placeId === place.id),
+    ...overrides,
+  } as Partial<TripPlanner>);
+}
+
+function renderDayPicker(planner = mobilePlanner()) {
+  const shell = buildShell({ sheet: { id: 'bract', payload: { placeId: 42, dayPicker: true } } });
+  const view = render(<MBrowseActionsSheet planner={planner} shell={shell} />);
+  return { ...view, planner, shell };
+}
+
+describe('MBrowseActionsSheet Tour assignment picker', () => {
+  it('disables assigned Tour days, blocks repeat clicks, and refreshes on reopen', async () => {
+    resetAllStores();
+    let finishAssignment!: (assigned: boolean) => void;
+    const handleAssignToDay = vi.fn(
+      () =>
+        new Promise<boolean>((resolve) => {
+          finishAssignment = resolve;
+        })
+    );
+    const planner = mobilePlanner({ handleAssignToDay });
+    const { rerender, shell } = renderDayPicker(planner);
+
+    const dayOne = screen.getByRole('button', { name: /Day one/ });
+    const dayTwo = screen.getByRole('button', { name: /Day two/ });
+    expect(dayOne).toBeDisabled();
+    expect(dayTwo).toBeEnabled();
+    fireEvent.click(dayOne);
+    expect(handleAssignToDay).not.toHaveBeenCalled();
+
+    fireEvent.click(dayTwo);
+    fireEvent.click(dayTwo);
+    expect(handleAssignToDay).toHaveBeenCalledTimes(1);
+    expect(handleAssignToDay).toHaveBeenCalledWith(42, 8);
+
+    await act(async () => {
+      finishAssignment(true);
+    });
+    await waitFor(() => expect(shell.closeSheet).toHaveBeenCalledOnce());
+
+    const reopened = mobilePlanner({
+      assignments: {
+        '7': [{ id: 70, day_id: 7, place_id: 42 }],
+        '8': [{ id: 80, day_id: 8, place_id: 42 }],
+      } as never,
+    });
+    rerender(<MBrowseActionsSheet planner={reopened} shell={shell} />);
+    expect(screen.getByRole('button', { name: /Day one/ })).toBeDisabled();
+    expect(screen.getByRole('button', { name: /Day two/ })).toBeDisabled();
+    expect(screen.getByRole('button', { name: /Day three/ })).toBeEnabled();
+  });
+
+  it('leaves ordinary Place day choices enabled and preserves their assignment write behavior', () => {
+    resetAllStores();
+    const place = { ...tourPlace(), tour_place_id: undefined };
+    const handleAssignToDay = vi.fn().mockResolvedValue(true);
+    const planner = mobilePlanner({
+      places: [place],
+      isTourPlace: vi.fn(() => false),
+      assignments: { '7': [{ id: 70, day_id: 7, place_id: place.id }] } as never,
+      handleAssignToDay,
+    });
+    renderDayPicker(planner);
+    const dayOne = screen.getByRole('button', { name: /Day one/ });
+    expect(dayOne).toBeEnabled();
+    fireEvent.click(dayOne);
+    expect(handleAssignToDay).toHaveBeenCalledWith(place.id, 7);
+  });
+});