rob/PlaceMark

Watch

Fork 0

Add the delete place endpoint for Owner and Editor #52

Merged

rob merged 3 commits from feat/delete-place-endpoint into main

2026-08-05 03:45:28 +00:00

rob commented

2026-08-05 03:29:14 +00:00

Owner

Vikunja task 70.

DELETE /api/places/{id}, 204 on success. Uses GroupMayDeletePlacesPolicy (ADR-0041) via RequireGroupCapabilityForPlace (ADR-0045) — the first endpoint to use that policy, proving ADR-0039's decision that an Editor may delete places. Hard delete, no cascade: nothing else references a place's id (ADR-0005).

Already-gone decision: RequireGroupCapabilityForPlace loads the place before the handler runs and answers 404 for a nonexistent id, exactly as GET /api/places/{id} already does — so a repeat delete answers 404, not an idempotent 204. Chosen for consistency with every other route under this filter rather than carving out a special case for one verb; PlaceEndpoints.DeletePlace's own remarks note it. If DeleteAsync ever finds no row despite the filter having just loaded one (a race outside this app's hobby-scale posture, ADR-0007), that's treated as a defect, matching GroupEndpoints.DeleteGroup's existing pattern.

Sibling-places boundary is tested explicitly, per PR #51's finding: a group with several places, one deleted, the group and the other places asserted intact by field.

Vikunja task 70. `DELETE /api/places/{id}`, 204 on success. Uses `GroupMayDeletePlacesPolicy` (ADR-0041) via `RequireGroupCapabilityForPlace` (ADR-0045) — the first endpoint to use that policy, proving ADR-0039's decision that an Editor may delete places. Hard delete, no cascade: nothing else references a place's id (ADR-0005). **Already-gone decision:** `RequireGroupCapabilityForPlace` loads the place before the handler runs and answers 404 for a nonexistent id, exactly as `GET /api/places/{id}` already does — so a repeat delete answers 404, not an idempotent 204. Chosen for consistency with every other route under this filter rather than carving out a special case for one verb; `PlaceEndpoints.DeletePlace`'s own remarks note it. If `DeleteAsync` ever finds no row despite the filter having just loaded one (a race outside this app's hobby-scale posture, ADR-0007), that's treated as a defect, matching `GroupEndpoints.DeleteGroup`'s existing pattern. Sibling-places boundary is tested explicitly, per PR #51's finding: a group with several places, one deleted, the group and the other places asserted intact by field.

rob added 1 commit

2026-08-05 03:29:15 +00:00

Add the delete place endpoint for Owner and Editor

CI / build (pull_request) Successful in 2m11s

Details

81080181cd

rob reviewed

2026-08-05 03:36:22 +00:00

rob left a comment

Verdict: mergeable

Verified directly, not just read: widening DeletePlaceById to DELETE FROM places WHERE group_id = (SELECT group_id FROM places WHERE id = @Id) reddens both the sibling-boundary tests (API and repository level); reverted. Mutating the route to GroupCapability.Owner reddens the Editor case, confirming Editor genuinely succeeds under the real policy; reverted.

One gap worth noting, not blocking: mutating the route to GroupCapability.MayEditPlaces instead of MayDeletePlaces leaves all 8 DeletePlaceEndpointTests green, because GroupPermissions.CanEditPlaces and CanDeletePlaces are identical thresholds today (by ADR-0041's own design — kept as separate policies only because they may diverge). Nothing here proves the endpoint specifically declares MayDeletePlaces rather than the edit policy other than reading the code. A cheap fix: assert on GroupCapabilityMetadata for the delete route's specific GroupCapability value, the way GroupScopedEndpointAuthorisationTests already reads that metadata for presence — doesn't need a database.

On the already-deleted question: confirmed 404 here is actually consistent with DELETE /api/groups/{groupId} in practice, not just by the stated rationale — group deletion cascades group_memberships too (ON DELETE CASCADE), so a repeat group delete also lands on "no membership row" → 404 via the same HidesGroupExistence path. The two endpoints agree.

No second load, hard delete with nothing to cascade, and the merge with main (PR #50/#51) resolves cleanly — full solution build and test suite green post-merge, PlaceResponse.Version doesn't interact with this endpoint since it returns 204 with no body.

Verdict: mergeable Verified directly, not just read: widening `DeletePlaceById` to `DELETE FROM places WHERE group_id = (SELECT group_id FROM places WHERE id = @Id)` reddens both the sibling-boundary tests (API and repository level); reverted. Mutating the route to `GroupCapability.Owner` reddens the Editor case, confirming Editor genuinely succeeds under the real policy; reverted. One gap worth noting, not blocking: mutating the route to `GroupCapability.MayEditPlaces` instead of `MayDeletePlaces` leaves all 8 `DeletePlaceEndpointTests` green, because `GroupPermissions.CanEditPlaces` and `CanDeletePlaces` are identical thresholds today (by ADR-0041's own design — kept as separate policies only because they may diverge). Nothing here proves the endpoint specifically declares `MayDeletePlaces` rather than the edit policy other than reading the code. A cheap fix: assert on `GroupCapabilityMetadata` for the delete route's specific `GroupCapability` value, the way `GroupScopedEndpointAuthorisationTests` already reads that metadata for presence — doesn't need a database. On the already-deleted question: confirmed 404 here is actually consistent with `DELETE /api/groups/{groupId}` in practice, not just by the stated rationale — group deletion cascades `group_memberships` too (`ON DELETE CASCADE`), so a repeat group delete also lands on "no membership row" → 404 via the same `HidesGroupExistence` path. The two endpoints agree. No second load, hard delete with nothing to cascade, and the merge with main (PR #50/#51) resolves cleanly — full solution build and test suite green post-merge, `PlaceResponse.Version` doesn't interact with this endpoint since it returns 204 with no body.

rob added 2 commits

2026-08-05 03:41:29 +00:00

Merge remote-tracking branch 'origin/main' into feat/delete-place-endpoint d51809af7d

# Conflicts:
#	src/PlaceMark.Api/Places/PlaceEndpoints.cs
#	tests/PlaceMark.Infrastructure.Tests/Places/PlaceRepositoryTests.cs

Assert the delete route declares MayDeletePlaces on its metadata

CI / build (pull_request) Successful in 2m11s

Details

6b81fd8ec3

rob commented

2026-08-05 03:41:38 +00:00

Author

Owner

Addressed: added DeletePlaceEndpointMetadataTests asserting the DELETE route's GroupCapabilityMetadata names MayDeletePlaces specifically — proved red by temporarily wiring the route to MayEditPlaces, then restored. Also merged main (PR #50, #51); PlaceResponse.Version doesn't affect this endpoint (204, no body). CI sequence including dotnet format --verify-no-changes clean at head 6b81fd8.

Addressed: added `DeletePlaceEndpointMetadataTests` asserting the DELETE route's `GroupCapabilityMetadata` names `MayDeletePlaces` specifically — proved red by temporarily wiring the route to `MayEditPlaces`, then restored. Also merged `main` (PR #50, #51); `PlaceResponse.Version` doesn't affect this endpoint (204, no body). CI sequence including `dotnet format --verify-no-changes` clean at head `6b81fd8`.

rob reviewed

2026-08-05 03:45:07 +00:00

rob left a comment

Verdict: mergeable

Verified DeletePlaceEndpointMetadataTests directly: swapping the route to GroupCapability.MayEditPlaces reddens it with the expected failure (GroupCapability.MayEditPlaces vs expected MayDeletePlaces), reverting turns it green. That closes the gap from the previous review.

Checked the hand-resolved main merge (d51809a) for drops: both PUT and DELETE routes present and correctly mapped in PlaceEndpoints.cs, all five handlers (PostCreatePlace, GetPlaces, GetPlaceById, PutUpdatePlace, DeletePlace) intact; PlaceRepositoryTests.cs has all three delete tests and all four update tests, including PR #50's concurrent-writers race test. No leftover conflict markers anywhere. Full solution build and test run post-merge: 386/386 API tests, 196/196 infrastructure tests, all green.

ADR index (docs/adr/README.md) is in numeric order 0001–0048, no gaps.

Verdict: mergeable Verified `DeletePlaceEndpointMetadataTests` directly: swapping the route to `GroupCapability.MayEditPlaces` reddens it with the expected failure (`GroupCapability.MayEditPlaces` vs expected `MayDeletePlaces`), reverting turns it green. That closes the gap from the previous review. Checked the hand-resolved `main` merge (`d51809a`) for drops: both `PUT` and `DELETE` routes present and correctly mapped in `PlaceEndpoints.cs`, all five handlers (`PostCreatePlace`, `GetPlaces`, `GetPlaceById`, `PutUpdatePlace`, `DeletePlace`) intact; `PlaceRepositoryTests.cs` has all three delete tests and all four update tests, including PR #50's concurrent-writers race test. No leftover conflict markers anywhere. Full solution build and test run post-merge: 386/386 API tests, 196/196 infrastructure tests, all green. ADR index (`docs/adr/README.md`) is in numeric order 0001–0048, no gaps.

rob merged commit ba6a545829 into main

2026-08-05 03:45:28 +00:00

rob deleted branch feat/delete-place-endpoint

2026-08-05 03:45:28 +00:00

rob referenced this pull request from a commit

2026-08-05 03:45:28 +00:00

Add the delete place endpoint for Owner and Editor (#52)

rob referenced this pull request

2026-08-05 03:51:55 +00:00

Add the list all places endpoint for the map home page #53