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

Merged

rob merged 2 commits from feat/list-all-places-endpoint into main

2026-08-05 03:59:19 +00:00

rob commented

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

Owner

Implements task 144: GET /api/places, every place the caller can see across every group they hold an accepted membership of (personal group included), each carrying its group id and name so the map can build its accordion without a request per group.

Scoped entirely in SQL (PlaceRepository.ListForCallerAsync, joining places → group_memberships → groups, filtered on status = 'accepted') — never fetches then filters. PlaceForCaller is the join projection, mirroring GroupForCaller.
Stable paging: ordered by created_at, id, tested with several places sharing a created_at across two of the caller's groups.
Pending invitation and a removed (hard-deleted) membership row both tested directly and excluded; another user's places never appear.

Authorisation: no RequireGroupCapability/RequireGroupCapabilityForPlace — this route spans every group the caller belongs to, not one named by the route, so it doesn't fit either filter. The WHERE clause scoping to the caller's own accepted memberships is the enforcement, the same shape GET /api/groups already takes. Task 148's safety net (GroupScopedEndpointAuthorisationTests) does not cover this route — it enumerates routes under /api/groups/{groupId} and /api/places/, and this one matches neither pattern (mapped directly at exact /api/places, deliberately outside the places group, to avoid a trailing-slash routing artefact that would otherwise sweep it in as if it named one place).

Pagination (ADR-0049): reuses the exact PagedResponse<T> shape and page-size cap (50) every other list endpoint already uses, rather than a bigger bound or no bound at all. The map is expected to keep requesting pages until it has read TotalCount places, accumulating markers — never truncated, just possibly more than one round trip. Argued in full in the ADR, including why a bigger cap specific to this endpoint was rejected.

Took ADR number 0049 — checked against main (currently at PR #52) immediately before pushing; no open PRs at push time.

Merged origin/main in before pushing to pick up PR #52 (delete place), which touched the same files; one test-file conflict, resolved by keeping both independent test methods.

Implements task 144: `GET /api/places`, every place the caller can see across every group they hold an *accepted* membership of (personal group included), each carrying its group id and name so the map can build its accordion without a request per group. - Scoped entirely in SQL (`PlaceRepository.ListForCallerAsync`, joining `places` → `group_memberships` → `groups`, filtered on `status = 'accepted'`) — never fetches then filters. `PlaceForCaller` is the join projection, mirroring `GroupForCaller`. - Stable paging: ordered by `created_at, id`, tested with several places sharing a `created_at` across two of the caller's groups. - Pending invitation and a removed (hard-deleted) membership row both tested directly and excluded; another user's places never appear. **Authorisation:** no `RequireGroupCapability`/`RequireGroupCapabilityForPlace` — this route spans every group the caller belongs to, not one named by the route, so it doesn't fit either filter. The `WHERE` clause scoping to the caller's own accepted memberships *is* the enforcement, the same shape `GET /api/groups` already takes. **Task 148's safety net (`GroupScopedEndpointAuthorisationTests`) does not cover this route** — it enumerates routes under `/api/groups/{groupId}` and `/api/places/`, and this one matches neither pattern (mapped directly at exact `/api/places`, deliberately outside the `places` group, to avoid a trailing-slash routing artefact that would otherwise sweep it in as if it named one place). **Pagination (ADR-0049):** reuses the exact `PagedResponse<T>` shape and page-size cap (50) every other list endpoint already uses, rather than a bigger bound or no bound at all. The map is expected to keep requesting pages until it has read `TotalCount` places, accumulating markers — never truncated, just possibly more than one round trip. Argued in full in the ADR, including why a bigger cap specific to this endpoint was rejected. Took ADR number **0049** — checked against `main` (currently at PR #52) immediately before pushing; no open PRs at push time. Merged `origin/main` in before pushing to pick up PR #52 (delete place), which touched the same files; one test-file conflict, resolved by keeping both independent test methods.

rob added 2 commits

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

Add the list all places endpoint for the map home page 65d6b33b29

Merge origin/main into feat/list-all-places-endpoint

CI / build (pull_request) Successful in 2m2s

Details

a067aa7ccf

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

rob reviewed

2026-08-05 03:59:00 +00:00

rob left a comment

Verdict: mergeable

Verified the trailing-slash claim directly rather than taking it on trust: mapping GetAllPlaces inside the places group (places.MapGet("", ...)) does produce route text /api/places/, which does trip GroupScopedEndpointAuthorisationTests' StartsWith($"{ByIdRoutePrefix}/") check — and since the route is genuinely not group-scoped, that flag is a false positive, not a real gap the test is right to catch. Reverted, then added a new group-scoped route under /api/places/... with no capability requirement to confirm the net still catches it — it does. The workaround is also recorded durably in code (the routing comment above the MapGet call and in GetAllPlaces' own XML doc), not only in the PR description. No action needed here.

Mutated SelectPlacesForCaller/CountPlacesForCaller to drop gm.status = 'accepted' — ListForCallerAsync_APendingInvitation_ExcludesItsGroupsPlaces reddens as expected, confirming the scoping is load-bearing in the SQL, not incidental. Personal-group inclusion, removed-membership and cross-user exclusion are all covered at both repository and endpoint level. Ordering tie-break on (created_at, id) is tested across groups, not just within one.

Merge conflict resolution in PlaceRepositoryTests.cs kept the full union of both branches' tests (ListForCallerAsync_* from this branch, DeleteAsync_* from #52) — nothing dropped.

Entities stay off the wire: PlaceForCaller → PlaceForCallerResponse mapping is explicit in the handler.

ADR-0049's cap is argued honestly, including the consequence that a caller's map waits on as many requests as it takes to reach TotalCount — it doesn't pin an exact ceiling on round trips, but the "tens to low hundreds per caller" estimate is a direct, reasonable inference from ADR-0007's existing app-wide numbers, not a new assumption, and the ADR names its own revisit trigger. Not blocking.

Full solution builds clean and all tests pass under the pinned SDK (10.0.100).

Verdict: mergeable Verified the trailing-slash claim directly rather than taking it on trust: mapping `GetAllPlaces` inside the `places` group (`places.MapGet("", ...)`) does produce route text `/api/places/`, which does trip `GroupScopedEndpointAuthorisationTests`' `StartsWith($"{ByIdRoutePrefix}/")` check — and since the route is genuinely not group-scoped, that flag is a false positive, not a real gap the test is right to catch. Reverted, then added a new group-scoped route under `/api/places/...` with no capability requirement to confirm the net still catches it — it does. The workaround is also recorded durably in code (the routing comment above the `MapGet` call and in `GetAllPlaces`' own XML doc), not only in the PR description. No action needed here. Mutated `SelectPlacesForCaller`/`CountPlacesForCaller` to drop `gm.status = 'accepted'` — `ListForCallerAsync_APendingInvitation_ExcludesItsGroupsPlaces` reddens as expected, confirming the scoping is load-bearing in the SQL, not incidental. Personal-group inclusion, removed-membership and cross-user exclusion are all covered at both repository and endpoint level. Ordering tie-break on `(created_at, id)` is tested across groups, not just within one. Merge conflict resolution in `PlaceRepositoryTests.cs` kept the full union of both branches' tests (`ListForCallerAsync_*` from this branch, `DeleteAsync_*` from #52) — nothing dropped. Entities stay off the wire: `PlaceForCaller` → `PlaceForCallerResponse` mapping is explicit in the handler. ADR-0049's cap is argued honestly, including the consequence that a caller's map waits on as many requests as it takes to reach `TotalCount` — it doesn't pin an exact ceiling on round trips, but the "tens to low hundreds per caller" estimate is a direct, reasonable inference from ADR-0007's existing app-wide numbers, not a new assumption, and the ADR names its own revisit trigger. Not blocking. Full solution builds clean and all tests pass under the pinned SDK (10.0.100).

rob merged commit afddee9252 into main

2026-08-05 03:59:19 +00:00

rob deleted branch feat/list-all-places-endpoint

2026-08-05 03:59:19 +00:00

rob referenced this pull request from a commit

2026-08-05 03:59:19 +00:00

Add the list all visible places endpoint (#53)