Create/edit place form with a map picker (task 94) #72
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/place-form"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes task 94. Took ADR-0064 (0063 is claimed by PR #71, which is untouched by this branch).
PlaceForm(src/PlaceMark.WebUI/Places/) creates or amends a place — name, description, latitude/longitude — validated againstCreatePlaceRequest/UpdatePlaceRequest's own annotations (ADR-0025), not a restated copy.One pair of fields for both input paths. Typing writes into
_latitude/_longitudedirectly; picking on the embedded map (LeafletMap.OnPick) writes into the same two fields. There is no second "picked" representation to fall out of step with a typed one, so a round trip through either path cannot drift.LeafletMap.Centreis computed once on init and never re-derived from the coordinate fields, so typing moves the marker without panning the map.Interop extension (ADR-0064).
LeafletMapgainsEnablePicking/OnPick: a map click or a marker drag now calls back into .NET through one new[JSInvokable]method. Off by default — the home map and group overview are unaffected. Full reasoning and alternatives in the ADR.409 handling.
Place.Versionround-trips intoUpdatePlaceRequest.Version(ADR-0048). A stale version is told apart from a validation failure by status, and rendered with prose naming what happened: someone else changed the place first, and it's their change a silent overwrite would have destroyed — not the caller's edit, which was not saved rather than lost.Group handling. Create mode offers a group selector filtered to the caller's Owner/Editor groups only (courtesy, not authority — the API still decides). Edit mode offers no group control at all:
UpdatePlaceRequestcarries nogroup_id, so it structurally cannot change here. Moving a place is task 71's own endpoint.Navigation. No navigation on success, following
GroupForm's own precedent (task 69) rather than the groomed criterion's "group detail page" — written before the map became the home page and before ADR-0062 replaced a place detail page with a panel. RaisesOnSaved; the host decides what happens next.What bUnit cannot show (task 156, no browser here): that a real click or drag actually reaches
OnMapPicked, that dragging feels responsive, or anything about how the picker renders. The round-trip tests prove the .NET-side channel is exact at full double precision by invokingOnMapPickeddirectly, not by driving a real pointer.Full CI sequence (restore, build, test,
dotnet format --verify-no-changes) run locally against the pinned SDK — 1187 tests pass.Verdict: changes needed
Stale
Versionon a second consecutive edit.PlaceFormreadsPlace.Versionlive off the[Parameter]inSubmitAsync, but nothing in the component ever updates it after a successful save —SaveAsyncinvokesOnSavedand stops. Confirmed empirically (added and ran a probe test, since the PR's own suite doesn't cover this): two edits submitted in a row against the same rendered instance, with no host-supplied refresh ofPlacebetween them, both send"version":42— even though the first response carriedVersion: 43. The doc comment's "round-tripped, never invented" claim only holds for the first submit; every submit after that silently depends on the host swapping in a freshPlaceparameter, which is neither enforced nor documented as a requirement, and the PR's own remarks explicitly leave "what happens next" (including "closing a panel" vs. keeping it open) to the host. This is exactly the confusing-409-on-a-second-edit scenario the ticket calls out. Fix: track the current version in a field seeded fromPlace.Versionand updated fromsaved.VersioninSaveAsync's success branch, and use that instead ofPlace.VersioninSubmitAsync; add a test that submits twice in a row.Clearing a coordinate field silently commits it to
0. Emptying#place-form-latitude(or longitude) sets_latitudeto0, not to an error state — standardBindConverterbehaviour for a non-nullable numeric@bind, but this is the first numeric-bound field in the codebase, so nothing established how to handle it.0is in range, soRangeAttributenever flags it, and the UI gives no indication the field changed — a caller who clears a field meaning to retype it, then submits without noticing, silently saves a wrong location. Worth a deliberate decision (nullable intermediate field with its own required-message, or otherwise visibly flag empty) rather than leaving the framework default as the behaviour.Everything else checked out:
HandlePicknever dispatches a DOM event or calls back into JS, andsetMarkersnever synthesises a click/dragend, so there's no feedback path to loop through.group_idis unchangeable on edit by construction, not just a hidden control —UpdatePlaceRequestcarries noGroupIdmember at all.ContractValidation.ValidateagainstCreatePlaceRequest/UpdatePlaceRequest), confirmed rather than assumed.CallerRoleis unset.OnMapPicked: as narrow as ADR-0064 claims. ADotNetObjectReferenceis only ever created and handed tomap.jswhenEnablePickingis true, so a non-pickingLeafletMap(home map, group overview) has no callable reference to reach at all. The method takes unvalidated primitives, but that's fine — the worst an unexpected caller could do is set a coordinate, which the visible fields already allow, still subject to the same downstream validation.dotnet format --verify-no-changesall pass clean under the pinned SDK (10.0.100).What bUnit cannot show, as the PR and ADR-0064 already say: a real click or drag reaching
OnMapPicked, drag responsiveness, or anything about how the picker actually renders. Nothing here adds to that.Both findings fixed, each with a regression test confirmed failing against the pre-fix code before the fix landed:
PlaceFormnow tracks its own_versionfield (seeded fromPlace.Versiononce, updated from the API's response after every successful save) instead of readingPlace.Versionlive at submit time.PlaceForm_EditedTwiceInARowWithNoInterveningReload_SendsTheVersionTheFirstSaveActuallyProduced.double?, notdouble. A cleared field parses tonull(not the type's default), checked explicitly before either request type is built and reported as a required-field message, same shape as a blank name.PlaceForm_ClearingACoordinateField_DoesNotPlotOrCommitTheEquator/PlaceForm_SubmittedWithAClearedCoordinateField_ShowsARequiredMessageAndDoesNotCallTheApi.Also merged
main(PR #71) — the form now uses the sharedErrorNotificationcomponent rather than its own paragraph, matchingGroupForm's updated shape.Full CI sequence (build, test across the whole solution,
dotnet format --verify-no-changes) run locally against the pinned SDK — 1207 tests pass.Verdict: mergeable
Re-reviewed at
29e3ad8. Both prior findings are fixed and I verified the fixes myself rather than taking the summary on trust:_versiontracking. Confirmed: two consecutive edits with no intervening reload now send42then43, and both succeed. I revertedPlaceForm.razor/.razor.csalone to the pre-fix commit and re-ranPlaceForm_EditedTwiceInARowWithNoInterveningReload_SendsTheVersionTheFirstSaveActuallyProducedand the two nullable-coordinate tests against it — all three genuinely fail (stale version, silent0, API called with a blank field) on the old code and pass on the new. The author's claim checks out.The swap edge case. Confirmed by rendering
PlaceFormagainst one place, then re-rendering the same instance with a differentPlace(different id, name, coordinates and version — no@key), then submitting: the request goes to the new place's route (Place.Idis read live) but carries the old place's name, coordinates and_version—OnInitializedseeds every field,_versionincluded, exactly once, and nothing reacts toPlacechanging under it. This isn't a fresh regression —_name/_latitude/_longitudealready had this property before this fix — but it's worth naming precisely because the fix changes_version's piece of it: previously a swap would have sent the new place's real version alongside the old place's stale content, which a matching version could let through as a silent overwrite of the wrong place with the wrong data; now_versionis stale too, so the mismatch almost certainly 409s instead — a safer failure mode, landing on the wrong side of "fail loud" by accident rather than by anyone deciding so. Nothing hostsPlaceFormyet, so there's no way to confirm a future host won't reuse the instance across a selection change without re-keying it. Worth a line in the remarks stating the@key="Place?.Id"requirement explicitly (the same way every other deliberate constraint here is written down), so whoever writes the first host reads it before discovering it.Nullable coordinates.
0,0.0and-0all parse as real values, plot a marker, show no field error, and submit correctly (-0serialises as-0in the JSON body, which is correct IEEE-754 behaviour, not a bug). An empty field is refused with a required message and never reaches the API. Matches the brief exactly.The
mainmerge.PlaceFormnow renders oneErrorNotificationfor its general/conflict error, matchingGroupForm's shape exactly — its own.form-errorparagraph is gone, and the remaining.form-errormarkup is only the pre-form Owner/Editor gating message, the same non-dismissible carve-out ADR-0063 documents forGroupForm's rename gate. No duplicate surface. Full suite (1207, matching the PR's own count), build anddotnet format --verify-no-changesall pass clean under the pinned SDK; no conflict markers or other stray changes anywhere in the merge.