Let an Owner or Editor delete a place from the detail panel #78

Merged

rob merged 3 commits from feat/delete-place-ui into main 2026-08-06 03:57:53 +00:00

Conversation 3 Commits 3 Files changed 9 +665 -14

rob commented 2026-08-06 03:24:44 +00:00

Owner

Copy link

Closes the hole task 95 named: DELETE /api/places/{id} has existed since task 70 with nothing in the UI calling it.

• PlaceDetailPanel gains a Delete button, gated on CanDelete (courtesy only — the API's own GroupMayDeletePlacesPolicy still decides, and a stale-role 403 is caught and shown rather than assumed away, the pattern PlaceForm/PR #69 established). Home passes the same caller-role check it already uses for CanEdit.
• Confirming is the inline swap GroupMemberRow already established for removal/leaving (ADR-0065, ADR-0068): a prompt naming the deletion as permanent, a focused confirm button, Escape to cancel.
• PlaceDetailPanel is now self-contained for this mutation (injects PlaceMarkApiClient/MapPlacesState directly), the same shape GroupMembersPanel.LeaveAsync uses: on success it calls MapPlacesState.RemovePlace and closes with no reload.
• MapPlacesState.RemovePlace mirrors RemoveGroup's shape — drops one place, clears the selection if it named the removed place. Tested with several places across several groups to prove only the one requested is removed (the PR #70/#76 over/under-removal class of bug).

No new ADR: this only applies patterns already decided (ADR-0039, ADR-0065, ADR-0068, ADR-0067's local-patch shape).

Not verified in a browser (task 156), as with every WebUI change so far: focus landing on the confirm button and what a screen reader announces are asserted only as the component's own request to the browser, not observed.

Closes the hole task 95 named: `DELETE /api/places/{id}` has existed since task 70 with nothing in the UI calling it. - `PlaceDetailPanel` gains a `Delete` button, gated on `CanDelete` (courtesy only — the API's own `GroupMayDeletePlacesPolicy` still decides, and a stale-role 403 is caught and shown rather than assumed away, the pattern `PlaceForm`/PR #69 established). `Home` passes the same caller-role check it already uses for `CanEdit`. - Confirming is the inline swap `GroupMemberRow` already established for removal/leaving (ADR-0065, ADR-0068): a prompt naming the deletion as permanent, a focused confirm button, Escape to cancel. - `PlaceDetailPanel` is now self-contained for this mutation (injects `PlaceMarkApiClient`/`MapPlacesState` directly), the same shape `GroupMembersPanel.LeaveAsync` uses: on success it calls `MapPlacesState.RemovePlace` and closes with no reload. - `MapPlacesState.RemovePlace` mirrors `RemoveGroup`'s shape — drops one place, clears the selection if it named the removed place. Tested with several places across several groups to prove only the one requested is removed (the PR #70/#76 over/under-removal class of bug). No new ADR: this only applies patterns already decided (ADR-0039, ADR-0065, ADR-0068, ADR-0067's local-patch shape). **Not verified in a browser** (task 156), as with every WebUI change so far: focus landing on the confirm button and what a screen reader announces are asserted only as the component's own request to the browser, not observed.

rob added 1 commit 2026-08-06 03:24:44 +00:00

Let an Owner or Editor delete a place from the detail panel 5f912df5d1

rob reviewed 2026-08-06 03:31:36 +00:00

rob left a comment

Copy link

Verdict: changes needed

Real bug: a stale expanded-group id can blank the map after deleting the only place in that group. MapPlacesState.RemovePlace (src/PlaceMark.WebUI/Maps/MapPlacesState.cs) never removes the group from _expandedGroupIds the way RemoveGroup does. If a caller expands group A (its one place), then deletes that place, Groups rebuilds without A (so the accordion's whole A section, including its toggle, disappears) — but _expandedGroupIds still contains A. VisiblePlaces then filters to _expandedGroupIds (count != 0) and matches nothing, so the map goes blank even though the caller still has places in other, non-expanded groups. Worse, there is no longer any control in the UI to un-expand A, since its accordion section is gone — the caller is stuck until they expand a different group by chance. Reproduced directly: add state.ExpandedGroupIds.ShouldNotContain(_groupA) after toggling A and deleting its only place, and it reddens. RemovePlace should drop the group id from _expandedGroupIds once the group has no places left (or unconditionally, mirroring RemoveGroup), the same way it already clears the selection.

Everything else checks out:

• RemovePlace itself removes exactly one place — proved multi-group, and a mutation to over-remove (matching by group instead of id) reddens two tests immediately.
• Stale selection: RemovePlace clears _selectedPlaceId when it names the removed place, which is what lets Home's own @if stop rendering PlaceDetailPanel with no explicit OnClose call — proved end-to-end in HomeTests.Home_DeleteConfirmed_RemovesTheMarkerAndClosesThePanelWithoutReload.
• Confirmation wording states permanence ("This can't be undone — the place will be permanently removed").
• Authorisation: Home.CanManage gates the button for Owner/Editor (courtesy), and PlaceDetailPanel_DeleteRefusedWithA403_ShowsTheErrorAndKeepsThePlace proves a stale-role 403 is caught and shown, not assumed away — both halves verified.
• Accessibility: mutated the Escape handler and the focus-request flag directly — both regressions reddened the corresponding tests, so the assertions genuinely bite, not just pass vacuously.
• Build and full PlaceMark.WebUI.Tests/PlaceMark.Architecture.Tests runs are clean on the pinned 10.0.100 SDK.

On the design point (injecting PlaceMarkApiClient/MapPlacesState into PlaceDetailPanel directly): I'd have put this in the host rather than the panel. The precedent cited (GroupMembersPanel.LeaveAsync) is real, but it's the wrong precedent to reach for here — GroupMembersPanel is a management surface with no separate "detail" mode, whereas PlaceDetailPanel already had an established internal convention before this PR: Close and Edit both bubble as events, and the host (Home) decides what they mean, including owning every other mutation this page makes (PlaceFormPanel patches MapPlacesState.UpsertPlace itself, but it's a distinct component Home explicitly hosts for that reason). Delete is now the one action on this component that skips that bubble-to-host shape and reaches into DI directly, which is an asymmetry future readers of this one file have to notice and remember, not a pattern they can infer from the other two buttons next to it. It also taxes every future test of this component: the class's own doc comment admits RegisterPlacesState now has to run "even [for tests] that never touch delete." I'd have kept PlaceDetailPanel presentational — confirmation UI only, no I/O — and raised an event for Home to handle the same way it already handles OnEdit. Not a blocker; it's a genuine judgement call and the precedent isn't unreasonable, but I don't think it's the right one for this specific component.

Not verifiable without a browser (task 156, consistent with every prior WebUI PR): whether focus visibly lands on the confirm button, what a screen reader actually announces from aria-describedby, and whether the panel reads sensibly disappearing off a narrow viewport are all unconfirmed — bUnit only proves the component asks the browser to do the right thing.

Verdict: changes needed **Real bug: a stale expanded-group id can blank the map after deleting the only place in that group.** `MapPlacesState.RemovePlace` (`src/PlaceMark.WebUI/Maps/MapPlacesState.cs`) never removes the group from `_expandedGroupIds` the way `RemoveGroup` does. If a caller expands group A (its one place), then deletes that place, `Groups` rebuilds without A (so the accordion's whole A section, including its toggle, disappears) — but `_expandedGroupIds` still contains A. `VisiblePlaces` then filters to `_expandedGroupIds` (count != 0) and matches nothing, so the map goes blank even though the caller still has places in other, non-expanded groups. Worse, there is no longer any control in the UI to un-expand A, since its accordion section is gone — the caller is stuck until they expand a different group by chance. Reproduced directly: add `state.ExpandedGroupIds.ShouldNotContain(_groupA)` after toggling A and deleting its only place, and it reddens. `RemovePlace` should drop the group id from `_expandedGroupIds` once the group has no places left (or unconditionally, mirroring `RemoveGroup`), the same way it already clears the selection. Everything else checks out: - `RemovePlace` itself removes exactly one place — proved multi-group, and a mutation to over-remove (matching by group instead of id) reddens two tests immediately. - Stale selection: `RemovePlace` clears `_selectedPlaceId` when it names the removed place, which is what lets `Home`'s own `@if` stop rendering `PlaceDetailPanel` with no explicit `OnClose` call — proved end-to-end in `HomeTests.Home_DeleteConfirmed_RemovesTheMarkerAndClosesThePanelWithoutReload`. - Confirmation wording states permanence ("This can't be undone — the place will be permanently removed"). - Authorisation: `Home.CanManage` gates the button for Owner/Editor (courtesy), and `PlaceDetailPanel_DeleteRefusedWithA403_ShowsTheErrorAndKeepsThePlace` proves a stale-role 403 is caught and shown, not assumed away — both halves verified. - Accessibility: mutated the Escape handler and the focus-request flag directly — both regressions reddened the corresponding tests, so the assertions genuinely bite, not just pass vacuously. - Build and full `PlaceMark.WebUI.Tests`/`PlaceMark.Architecture.Tests` runs are clean on the pinned 10.0.100 SDK. **On the design point** (injecting `PlaceMarkApiClient`/`MapPlacesState` into `PlaceDetailPanel` directly): I'd have put this in the host rather than the panel. The precedent cited (`GroupMembersPanel.LeaveAsync`) is real, but it's the wrong precedent to reach for here — `GroupMembersPanel` is a management surface with no separate "detail" mode, whereas `PlaceDetailPanel` already had an established *internal* convention before this PR: `Close` and `Edit` both bubble as events, and the host (`Home`) decides what they mean, including owning every other mutation this page makes (`PlaceFormPanel` patches `MapPlacesState.UpsertPlace` itself, but it's a distinct component `Home` explicitly hosts for that reason). Delete is now the one action on this component that skips that bubble-to-host shape and reaches into DI directly, which is an asymmetry future readers of this one file have to notice and remember, not a pattern they can infer from the other two buttons next to it. It also taxes every future test of this component: the class's own doc comment admits `RegisterPlacesState` now has to run "even [for tests] that never touch delete." I'd have kept `PlaceDetailPanel` presentational — confirmation UI only, no I/O — and raised an event for `Home` to handle the same way it already handles `OnEdit`. Not a blocker; it's a genuine judgement call and the precedent isn't unreasonable, but I don't think it's the right one for this specific component. **Not verifiable without a browser** (task 156, consistent with every prior WebUI PR): whether focus visibly lands on the confirm button, what a screen reader actually announces from `aria-describedby`, and whether the panel reads sensibly disappearing off a narrow viewport are all unconfirmed — bUnit only proves the component asks the browser to do the right thing.

rob added 1 commit 2026-08-06 03:38:58 +00:00

Fix blanked map on last-place delete and keep delete UI presentational 6c9e22c689

rob reviewed 2026-08-06 03:48:11 +00:00

rob left a comment

Copy link

Verdict: changes needed

Home.razor's DeleteError="_deleteError" is missing its @ prefix, and this is not cosmetic. DeleteError is string?, and a bare identifier is a syntactically valid string literal, so Razor's component-attribute parser treats it as the literal text "_deleteError" rather than a reference to the field — unlike IsDeleting="_isDeleting" two lines above, which is forced into an expression because a bare identifier can't type-check as bool. The practical effect: PlaceDetailPanel's ErrorNotification renders unconditionally on Message is not null, and the literal string is never null — so every place selected shows a permanent error banner reading the literal text "_deleteError", whether or not a delete was ever attempted, and dismissing it does nothing observable since the bound value never changes. Confirmed directly: added a bUnit test that selects a place and asserts no .error-notification-message renders — it fails, showing that exact literal text. Confirmed the fix too — changing to DeleteError="@_deleteError" makes that test pass and also makes a 403-refusal test (also added, since none existed) correctly show the real API detail instead. Both scratch tests were reverted after confirming; neither is committed. This is exactly the shape the "error path gets dropped a level up" concern anticipated, and it's a bigger break than a dropped error path — it's a defect visible on the ordinary, successful path too.

Everything else in this delta holds up:

• Expansion-state pruning. The regression test (RemovePlace_TheLastPlaceInAnExpandedGroup_DropsItFromExpandedGroupIdsRatherThanBlankingTheMap) reddens against the unfixed code — confirmed by removing the PruneExpandedGroupIdsWithNoPlaces() call from RemovePlace. RemoveGroup's pre-existing behaviour is unchanged by the shared helper — confirmed by removing the call from RemoveGroup instead and watching its own existing RemoveGroup_GroupWasExpanded_DropsItFromExpandedGroupIdsToo test redden.
• Pruning is not too eager. RemovePlace_APlaceRemainsInTheExpandedGroup_LeavesItExpanded proves a group with a place still in it stays expanded — confirmed by mutating the helper to unconditionally clear _expandedGroupIds, which reddens exactly that test. The UpsertPlace interaction is moot in practice: an empty group never has an accordion section (Groups only ever contains groups with at least one place), so there is no UI path to expand one before a place exists in it.
• The presentational refactor itself is right. PlaceDetailPanel injects nothing now — confirmed by the test class's own setup, which registers no API client or state service anywhere. Home.DeleteSelectedPlaceAsync owns the call, the try/catch (ApiException) on the stale-role 403 case is intact in principle (just not correctly wired to the UI, per above), and IsDeleting threads through correctly since its type forces expression evaluation.
• Accessibility assertions still bite post-refactor. Re-ran the Escape and focus-request mutations against the new code; both still redden the corresponding tests.
• Full build and PlaceMark.WebUI.Tests/PlaceMark.Architecture.Tests are clean on the pinned SDK — unsurprising, since nothing in the current suite renders a PlaceDetailPanel and checks for the absence of an error notification, which is exactly the gap that let this through.

Verdict: changes needed **`Home.razor`'s `DeleteError="_deleteError"` is missing its `@` prefix, and this is not cosmetic.** `DeleteError` is `string?`, and a bare identifier is a syntactically valid string literal, so Razor's component-attribute parser treats it as the literal text `"_deleteError"` rather than a reference to the field — unlike `IsDeleting="_isDeleting"` two lines above, which is forced into an expression because a bare identifier can't type-check as `bool`. The practical effect: `PlaceDetailPanel`'s `ErrorNotification` renders unconditionally on `Message is not null`, and the literal string is never null — so **every place selected shows a permanent error banner reading the literal text "_deleteError", whether or not a delete was ever attempted**, and dismissing it does nothing observable since the bound value never changes. Confirmed directly: added a bUnit test that selects a place and asserts no `.error-notification-message` renders — it fails, showing that exact literal text. Confirmed the fix too — changing to `DeleteError="@_deleteError"` makes that test pass and also makes a 403-refusal test (also added, since none existed) correctly show the real API detail instead. Both scratch tests were reverted after confirming; neither is committed. This is exactly the shape the "error path gets dropped a level up" concern anticipated, and it's a bigger break than a dropped error path — it's a defect visible on the ordinary, successful path too. Everything else in this delta holds up: - **Expansion-state pruning.** The regression test (`RemovePlace_TheLastPlaceInAnExpandedGroup_DropsItFromExpandedGroupIdsRatherThanBlankingTheMap`) reddens against the unfixed code — confirmed by removing the `PruneExpandedGroupIdsWithNoPlaces()` call from `RemovePlace`. `RemoveGroup`'s pre-existing behaviour is unchanged by the shared helper — confirmed by removing the call from `RemoveGroup` instead and watching its own existing `RemoveGroup_GroupWasExpanded_DropsItFromExpandedGroupIdsToo` test redden. - **Pruning is not too eager.** `RemovePlace_APlaceRemainsInTheExpandedGroup_LeavesItExpanded` proves a group with a place still in it stays expanded — confirmed by mutating the helper to unconditionally clear `_expandedGroupIds`, which reddens exactly that test. The `UpsertPlace` interaction is moot in practice: an empty group never has an accordion section (`Groups` only ever contains groups with at least one place), so there is no UI path to expand one before a place exists in it. - **The presentational refactor itself is right.** `PlaceDetailPanel` injects nothing now — confirmed by the test class's own setup, which registers no API client or state service anywhere. `Home.DeleteSelectedPlaceAsync` owns the call, the `try`/`catch (ApiException)` on the stale-role 403 case is intact in principle (just not correctly wired to the UI, per above), and `IsDeleting` threads through correctly since its type forces expression evaluation. - **Accessibility assertions still bite post-refactor.** Re-ran the Escape and focus-request mutations against the new code; both still redden the corresponding tests. - Full build and `PlaceMark.WebUI.Tests`/`PlaceMark.Architecture.Tests` are clean on the pinned SDK — unsurprising, since nothing in the current suite renders a `PlaceDetailPanel` and checks for the *absence* of an error notification, which is exactly the gap that let this through.

rob added 1 commit 2026-08-06 03:52:40 +00:00

Fix missing @ on DeleteError binding in Home 2a5f43f836

rob reviewed 2026-08-06 03:57:26 +00:00

rob left a comment

Copy link

Verdict: mergeable

• Regression test genuinely discriminates: removed the @ from DeleteError="@_deleteError" again and Home_PlaceSelectedWithNoDeleteAttempted_ShowsNoErrorNotification reddens (finds a stray .error-notification div), passes clean with it restored.
• Independently swept PlaceMark.WebUI for every [Parameter] (all 12 files that declare one, all in code-behind — no inline .razor parameters to miss) and every string/string?-typed one's call sites: ErrorNotification.Message/AdditionalCssClass are @-prefixed everywhere; FieldErrors.Field/Id are bare but every call site is a genuine literal (kebab-case DOM ids, lowercase field-name keys), not a variable reference; PlaceDetailPanel.DeleteError has exactly one call site, now fixed. No RenderFragment, RenderFragment<T>, or MarkupString parameter exists anywhere in this project, so that class of the bug doesn't apply here. The author's report holds.
• Diff is exactly the two files reported (Home.razor, HomeTests.cs, 20 lines) — confirmed by diffing against the prior commit directly. MapPlacesState.cs, PlaceDetailPanel.razor/.razor.cs, and the Maps test files are byte-for-byte unchanged, so the pruning fix and the presentational refactor from the last two rounds are undisturbed.
• Full build and PlaceMark.WebUI.Tests/PlaceMark.Architecture.Tests clean on the pinned SDK.

Nothing further to act on.

Verdict: mergeable - Regression test genuinely discriminates: removed the `@` from `DeleteError="@_deleteError"` again and `Home_PlaceSelectedWithNoDeleteAttempted_ShowsNoErrorNotification` reddens (finds a stray `.error-notification` div), passes clean with it restored. - Independently swept `PlaceMark.WebUI` for every `[Parameter]` (all 12 files that declare one, all in code-behind — no inline `.razor` parameters to miss) and every `string`/`string?`-typed one's call sites: `ErrorNotification.Message`/`AdditionalCssClass` are `@`-prefixed everywhere; `FieldErrors.Field`/`Id` are bare but every call site is a genuine literal (kebab-case DOM ids, lowercase field-name keys), not a variable reference; `PlaceDetailPanel.DeleteError` has exactly one call site, now fixed. No `RenderFragment`, `RenderFragment<T>`, or `MarkupString` parameter exists anywhere in this project, so that class of the bug doesn't apply here. The author's report holds. - Diff is exactly the two files reported (`Home.razor`, `HomeTests.cs`, 20 lines) — confirmed by diffing against the prior commit directly. `MapPlacesState.cs`, `PlaceDetailPanel.razor`/`.razor.cs`, and the Maps test files are byte-for-byte unchanged, so the pruning fix and the presentational refactor from the last two rounds are undisturbed. - Full build and `PlaceMark.WebUI.Tests`/`PlaceMark.Architecture.Tests` clean on the pinned SDK. Nothing further to act on.

rob merged commit f29aa4a5f9 into main 2026-08-06 03:57:53 +00:00

rob deleted branch feat/delete-place-ui 2026-08-06 03:57:53 +00:00

rob referenced this pull request from a commit 2026-08-06 03:57:54 +00:00

Let an Owner or Editor delete a place (#78)

rob referenced this pull request 2026-08-20 06:47:31 +00:00

Show the reader's current location on the map and in the add-place form #198

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

rob/PlaceMark!78

No description provided.