From a44df6cf1106de3471c4bd9bfb0a691d532d7de1 Mon Sep 17 00:00:00 2001 From: Maurice Date: Tue, 22 Sep 2026 10:53:54 +0200 Subject: [PATCH] fix(planner): place edit and delete controls follow place_edit, not day_edit With places set to "Trip owner" a member who may edit days still saw Edit and Delete on a place in the day plan's context menu, the inspector's action row, its inline rename and the phone timeline's edit circle, and every one of them ended in the server's 403. The day plan gates the two entries on place_edit (taking a place off the day stays a day right), the inspector drops its edit, delete and update handlers without the right the way the places pool already does, the phone timeline offers the pencil only with it, and openPlaceEditor and handleDeletePlace guard like their siblings. Closes #2446 --- .../Planner/DayPlanSidebar.test.tsx | 20 ++++++++++++++++--- .../src/components/Planner/DayPlanSidebar.tsx | 9 +++++++-- .../Planner/PlaceInspector.test.tsx | 20 +++++++++++++++++++ .../src/components/Planner/PlaceInspector.tsx | 15 ++++++++++++-- .../screens/trip/plan/MPlanTimeline.tsx | 3 ++- .../screens/trip/plan/MPlanTimelineRows.tsx | 10 ++++++---- .../pages/tripPlanner/useTripPlanner.test.tsx | 15 ++++++++++++++ .../src/pages/tripPlanner/useTripPlanner.ts | 6 ++++-- .../unit/mobile/trip/MPlanTimeline.test.tsx | 13 ++++++++++++ 9 files changed, 97 insertions(+), 14 deletions(-) diff --git a/client/src/components/Planner/DayPlanSidebar.test.tsx b/client/src/components/Planner/DayPlanSidebar.test.tsx index 7a6b3f884..dc9f6c591 100644 --- a/client/src/components/Planner/DayPlanSidebar.test.tsx +++ b/client/src/components/Planner/DayPlanSidebar.test.tsx @@ -101,14 +101,15 @@ vi.mock('../shared/Toast', () => ({ // ── Permissions mock ──────────────────────────────────────────────────────── -// Flipped per test to render the read-only variants of the day rows. -const mockPermissions = vi.hoisted(() => ({ canEdit: true })) +// Flipped per test to render the read-only variants of the day rows; `denied` +// takes single actions away from an otherwise editing member. +const mockPermissions = vi.hoisted(() => ({ canEdit: true, denied: new Set() })) vi.mock('../../store/permissionsStore', async (importOriginal) => { const actual = await importOriginal() as any return { ...actual, - useCanDo: () => () => mockPermissions.canEdit, + useCanDo: () => (action: string) => mockPermissions.canEdit && !mockPermissions.denied.has(action), } }) @@ -182,6 +183,7 @@ beforeEach(() => { resetAllStores() vi.clearAllMocks() mockPermissions.canEdit = true + mockPermissions.denied.clear() // clearAllMocks keeps implementations, so tests that swap the router out would // otherwise leak into the ones after them. vi.mocked(calculateRouteWithLegs).mockImplementation(waypoints => Promise.resolve({ @@ -938,6 +940,18 @@ describe('DayPlanSidebar', () => { expect(onEditPlace).toHaveBeenCalledWith(place, assignment.id) }) + it('FE-PLANNER-DAYPLAN-214: without place_edit the menu still removes from the day but neither edits nor deletes the place (#2446)', () => { + mockPermissions.denied.add('place_edit') + const place = buildPlace({ id: 42, name: 'Louvre Museum' }) + const day = buildDay({ id: 10, date: '2025-06-01', title: 'Day 1' }) + const assignment = buildAssignment({ id: 99, day_id: 10, order_index: 0, place }) + render() + fireEvent.contextMenu(screen.getByText('Louvre Museum')) + expect(screen.getByText(/Remove from day/i)).toBeInTheDocument() + expect(screen.queryByText('Edit')).not.toBeInTheDocument() + expect(screen.queryByText('Delete')).not.toBeInTheDocument() + }) + // ── Arrow reorder buttons ──────────────────────────────────────────────── it('FE-PLANNER-DAYPLAN-041: arrow down button reorders day assignments', async () => { diff --git a/client/src/components/Planner/DayPlanSidebar.tsx b/client/src/components/Planner/DayPlanSidebar.tsx index f0296f8d3..749b8b561 100644 --- a/client/src/components/Planner/DayPlanSidebar.tsx +++ b/client/src/components/Planner/DayPlanSidebar.tsx @@ -188,6 +188,9 @@ function useDayPlanSidebar(props: DayPlanSidebarProps) { const tripActions = useRef(useTripStore.getState()).current const can = useCanDo() const canEditDays = can('day_edit', trip) + // Editing or deleting the place itself is a place right; taking it off the + // day stays a day right (#2446). + const canEditPlaces = can('place_edit', trip) // The calendar subscription hands out a link that reads the trip without an // account, so it sits behind the same permission as the public share link. const canManageShare = can('share_manage', trip) @@ -1148,6 +1151,7 @@ function useDayPlanSidebar(props: DayPlanSidebarProps) { tripActions, can, canEditDays, + canEditPlaces, canManageShare, noteUi, setNoteUi, @@ -1348,6 +1352,7 @@ const DayPlanSidebar = React.memo(function DayPlanSidebar(props: DayPlanSidebarP tripActions, can, canEditDays, + canEditPlaces, canManageShare, noteUi, setNoteUi, @@ -2097,13 +2102,13 @@ const DayPlanSidebar = React.memo(function DayPlanSidebar(props: DayPlanSidebarP // does not exist here. const navTargets = getNavigationTargets(place) ctxMenu.open(e, [ - canEditDays && onEditPlace && { label: t('common.edit'), icon: Pencil, onClick: () => onEditPlace(place, assignment.id) }, + canEditPlaces && onEditPlace && { label: t('common.edit'), icon: Pencil, onClick: () => onEditPlace(place, assignment.id) }, canEditDays && onRemoveAssignment && { label: t('planner.removeFromDay'), icon: Trash2, onClick: () => onRemoveAssignment(day.id, assignment.id) }, safeHttpUrl(place.website) && { label: t('inspector.website'), icon: ExternalLink, onClick: () => window.open(safeHttpUrl(place.website)!, '_blank', 'noopener,noreferrer') }, ...navTargets.map(target => ({ label: target.label, icon: Navigation, onClick: () => openNavigationTarget(target) })), collectionsEnabled && { label: t('inspector.saveToCollection'), icon: Bookmark, onClick: () => useSaveToCollectionStore.getState().open(placeToSaveTarget(place)) }, { divider: true }, - canEditDays && onDeletePlace && { label: t('common.delete'), icon: Trash2, danger: true, onClick: () => onDeletePlace(place.id) }, + canEditPlaces && onDeletePlace && { label: t('common.delete'), icon: Trash2, danger: true, onClick: () => onDeletePlace(place.id) }, ]) }} onMouseEnter={e => { diff --git a/client/src/components/Planner/PlaceInspector.test.tsx b/client/src/components/Planner/PlaceInspector.test.tsx index f3478d578..facc0317b 100644 --- a/client/src/components/Planner/PlaceInspector.test.tsx +++ b/client/src/components/Planner/PlaceInspector.test.tsx @@ -8,6 +8,7 @@ import { useSettingsStore } from '../../store/settingsStore'; import { useAddonStore } from '../../store/addonStore'; import { usePluginStore } from '../../store/pluginStore'; import { useSaveToCollectionStore } from '../../store/saveToCollectionStore'; +import { usePermissionsStore } from '../../store/permissionsStore'; import { http, HttpResponse } from 'msw'; import { server } from '../../../tests/helpers/msw/server'; import type { AssignmentsMap } from '../../types'; @@ -1478,6 +1479,25 @@ describe('PlaceInspector', () => { expect(screen.getByText('Museum Ticket').closest('[role="button"]')).toHaveAttribute('data-no-press'); }); + it('FE-PLANNER-INSPECTOR-102: a member without place_edit gets neither Edit nor Delete nor the inline rename (#2446)', () => { + // buildUser() is a plain member and buildTrip({ id: 1 }) belongs to somebody else. + usePermissionsStore.setState({ permissions: { place_edit: 'trip_owner' } }); + const onEdit = vi.fn(); + const onDelete = vi.fn(); + const onUpdatePlace = vi.fn(); + const member = render(); + expect(screen.queryByText('Edit')).not.toBeInTheDocument(); + expect(screen.queryByText('Delete')).not.toBeInTheDocument(); + fireEvent.doubleClick(screen.getByText(place.name)); + expect(screen.queryByRole('textbox')).not.toBeInTheDocument(); + member.unmount(); + // the same trip owned by this member shows them again + seedStore(useTripStore, { trip: buildTrip({ id: 1, user_id: useAuthStore.getState().user!.id }) }); + render(); + expect(screen.getByText('Edit')).toBeInTheDocument(); + expect(screen.getByText('Delete')).toBeInTheDocument(); + }); + it('FE-PLANNER-INSPECTOR-017b: a stop a booked night wrote is neither removed nor doubled here', () => { // Taking it off the day would leave the booking behind with nothing on the drive and diff --git a/client/src/components/Planner/PlaceInspector.tsx b/client/src/components/Planner/PlaceInspector.tsx index 692276803..aed903a36 100644 --- a/client/src/components/Planner/PlaceInspector.tsx +++ b/client/src/components/Planner/PlaceInspector.tsx @@ -32,6 +32,7 @@ import type { Place, Category, Day, Assignment, Reservation, TripFile, Assignmen import type { CollectionStatus } from '@trek/shared' import { splitReservationDateTime, formatTime, formatMoney } from '../../utils/formatters' import { useTripStore } from '../../store/tripStore' +import { useCanDo } from '../../store/permissionsStore' import { formatDistance, formatElevation } from '../../utils/units' import { getNavigationTargets, openNavigationTarget } from './placeNavigation' import { TRANSPORT_TYPES, getAssignmentReservations } from '../../utils/dayMerge' @@ -184,11 +185,21 @@ interface PlaceInspectorProps { export default function PlaceInspector({ place, categories, mode = 'trip', days = [], selectedDayId = null, selectedAssignmentId = null, assignments = {}, reservations = [], onEditTransport, onEditReservation, - onClose, onEdit, onDelete, onAssignToDay, onRemoveAssignment, - files = [], onFileUpload, tripMembers = [], onSetParticipants, onUpdatePlace, onUploadImage, onRate, + onClose, onEdit: editPlace, onDelete: deletePlace, onAssignToDay, onRemoveAssignment, + files = [], onFileUpload, tripMembers = [], onSetParticipants, onUpdatePlace: updatePlace, onUploadImage, onRate, leftWidth = 0, rightWidth = 0, collectionStatus, onCopyToTrip, onSetStatus, onRemoveFromList, roadtripEndDay, roadtripStay, roadtripActive, }: PlaceInspectorProps) { + // Editing the place is a place right. The planner hands the handlers over + // regardless, and a member without the right saw Edit, Delete and the inline + // rename and got the server's 403 for each (#2446). Collection mode gates + // its own actions. + const can = useCanDo() + const trip = useTripStore(s => s.trip) + const mayEditPlace = mode !== 'trip' || can('place_edit', trip) + const onEdit = mayEditPlace ? editPlace : undefined + const onDelete = mayEditPlace ? deletePlace : undefined + const onUpdatePlace = mayEditPlace ? updatePlace : undefined // Plugins that declared a place-detail slot mount at the bottom of this panel, // scoped to the open place (trip mode only). Inline-filter like the other sites. const placeDetailPlugins = usePluginStore((s) => s.plugins).filter((p) => p.type === 'widget' && p.slot === 'place-detail') diff --git a/client/src/mobile/screens/trip/plan/MPlanTimeline.tsx b/client/src/mobile/screens/trip/plan/MPlanTimeline.tsx index 2baed3029..a118a2b30 100644 --- a/client/src/mobile/screens/trip/plan/MPlanTimeline.tsx +++ b/client/src/mobile/screens/trip/plan/MPlanTimeline.tsx @@ -43,6 +43,7 @@ export default function MPlanTimeline({ planner, shell }: MPlanTimelineProps) { const { t, trip, can } = planner const canEdit = can('day_edit', trip) const editing = shell.mode === 'edit' && canEdit + const canEditPlaces = can('place_edit', trip) // Per-segment travel mode (#1281): tap a connector → pick the leg's mode. const legMenu = useContextMenu() const modeIcon = (key: string) => (key === 'walking' ? Footprints : key.startsWith('plugin:') ? Zap : Car) @@ -202,7 +203,7 @@ export default function MPlanTimeline({ planner, shell }: MPlanTimelineProps) { reorder={reorderFor(row.item)} drag={dragFor(row)} onOpen={() => openPlace(row.assignment)} - onEdit={() => tl.editAssignment(row.assignment)} + onEdit={canEditPlaces ? () => tl.editAssignment(row.assignment) : undefined} onRemove={() => tl.removeAssignment(row.assignment)} /> {dayScheduleFor('assignment', row.assignment.id)} diff --git a/client/src/mobile/screens/trip/plan/MPlanTimelineRows.tsx b/client/src/mobile/screens/trip/plan/MPlanTimelineRows.tsx index 1ea35adc9..b86e12e14 100644 --- a/client/src/mobile/screens/trip/plan/MPlanTimelineRows.tsx +++ b/client/src/mobile/screens/trip/plan/MPlanTimelineRows.tsx @@ -124,7 +124,7 @@ export function PlaceRow({ assignment, fullPlace, linkedReservations, chrome, re reorder: ReactNode drag?: RowDrag onOpen: () => void - onEdit: () => void + onEdit?: () => void onRemove: () => void }) { const { t } = chrome @@ -210,9 +210,11 @@ export function PlaceRow({ assignment, fullPlace, linkedReservations, chrome, re {chrome.editing && ( - - - + {onEdit && ( + + + + )} diff --git a/client/src/pages/tripPlanner/useTripPlanner.test.tsx b/client/src/pages/tripPlanner/useTripPlanner.test.tsx index eb3037fd5..c6428278b 100644 --- a/client/src/pages/tripPlanner/useTripPlanner.test.tsx +++ b/client/src/pages/tripPlanner/useTripPlanner.test.tsx @@ -2092,6 +2092,21 @@ describe('useTripPlanner — misc state', () => { expect(result.current.prefillCoords).toBeNull() }) + it('FE-TP-HOOK-120: without place_edit rights the place editor and the delete dialog stay shut (#2446)', async () => { + seedStore(useAuthStore, { user: buildUser({ id: 2, role: 'user' }) }) + usePermissionsStore.setState({ permissions: { place_edit: 'admin' } }) + const place = buildPlace({ id: 1, lat: 1, lng: 2 }) + seedTrip({ places: [place], assignments: { '7': [buildAssignment({ id: 10, day_id: 7, place })] } }) + + const { result } = await renderPlanner() + act(() => { result.current.openPlaceEditor(place, 10) }) + act(() => { result.current.handleDeletePlace(1) }) + + expect(result.current.showPlaceForm).toBe(false) + expect(result.current.editingPlace).toBeNull() + expect(result.current.deletePlaceId).toBeNull() + }) + it('FE-TP-HOOK-102: a member roster refresh replaces the cached list', async () => { seedTrip() const { result } = await renderPlanner() diff --git a/client/src/pages/tripPlanner/useTripPlanner.ts b/client/src/pages/tripPlanner/useTripPlanner.ts index 243422e90..d6bd740fd 100644 --- a/client/src/pages/tripPlanner/useTripPlanner.ts +++ b/client/src/pages/tripPlanner/useTripPlanner.ts @@ -2095,6 +2095,7 @@ export function useTripPlanner() { // place's lone assignment to hydrate & persist its times; with 0 or 2+ // assignments the time is ambiguous and the modal hides the fields (#1247). const openPlaceEditor = useCallback((place: Place, preferredAssignmentId: number | null = null) => { + if (!can('place_edit', trip)) return if (roadtripActive && (isServiceStopType(place.stop_type) || tripAccommodations.some(stay => stay.place_id === place.id)) && typeof place.lat === 'number' && typeof place.lng === 'number') { const visitId = preferredAssignmentId ?? resolvePoolAssignmentId(assignments, place.id) const entry = Object.entries(assignments).find(([, visits]) => visits.some(visit => visit.id === visitId)) @@ -2119,7 +2120,7 @@ export function useTripPlanner() { setPlaceFormDayId(null) setServiceStopForm(false) setShowPlaceForm(true) - }, [assignments, roadtripActive, tripAccommodations, days, overnightOptions, roadtripRoutes.days]) + }, [can, trip, assignments, roadtripActive, tripAccommodations, days, overnightOptions, roadtripRoutes.days]) /** * How long the drive stands here, for every stop alike. @@ -2134,8 +2135,9 @@ export function useTripPlanner() { }, []) const handleDeletePlace = useCallback((placeId) => { + if (!can('place_edit', trip)) return setDeletePlaceId(placeId) - }, []) + }, [can, trip]) const confirmDeletePlace = useCallback(async () => { if (!deletePlaceId) return diff --git a/client/tests/unit/mobile/trip/MPlanTimeline.test.tsx b/client/tests/unit/mobile/trip/MPlanTimeline.test.tsx index f458b9fbe..33319c6a0 100644 --- a/client/tests/unit/mobile/trip/MPlanTimeline.test.tsx +++ b/client/tests/unit/mobile/trip/MPlanTimeline.test.tsx @@ -622,6 +622,19 @@ describe('MPlanTimeline', () => { expect(mocks.tl.removeAssignment).toHaveBeenCalledWith(MUSEUM) }) + it('FE-MOB-PLTL-048: without place_edit a place row keeps remove and reorder but loses its edit circle (#2446)', () => { + renderTimeline({}, { can: vi.fn((action: string) => action !== 'place_edit') as TripPlanner['can'] }, { mode: 'edit' }) + + // the place row keeps its remove circle and loses the edit one; the + // transport and note rows keep theirs, editing those is a day right + const row = screen.getByText('Museum').closest('[role="button"]') as HTMLElement + expect(within(row).getByLabelText('planner.removeFromDay')).toBeInTheDocument() + expect(within(row).queryByLabelText('common.edit')).not.toBeInTheDocument() + fireEvent.click(within(row).getByLabelText('planner.removeFromDay')) + expect(mocks.tl.removeAssignment).toHaveBeenCalledWith(MUSEUM) + expect(mocks.tl.editAssignment).not.toHaveBeenCalled() + }) + it('FE-MOB-PLTL-037: the transit row opens the journey view from its edit circle', () => { renderTimeline({}, {}, { mode: 'edit' })