Add the move-place-between-groups endpoint #61
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/move-place-endpoint"
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?
Implements Vikunja task 71, the last of the Places epic:
PATCH /api/places/{id}/group.RequireGroupCapabilityForPlace(MayEditPlaces)on the route covers the source group; the handler makes a second, explicit call to the sharedAuthorizeGroupCapabilityAsyncfor the destination named in the body — the same shapeDeleteGroupMemberalready uses for its own conditional second check. Task 148's safety net only ever sees the source capability; the destination check is proved by this PR's own tests.Versionexactly likePUT, on one atomicUPDATE(ADR-0048).PUT.Reasoning in full in ADR-0055.
Full solution build,
dotnet testanddotnet format --verify-no-changesrun clean locally with the pinned SDK.Verdict: mergeable
Dual authorisation checked by mutation: deleting the destination
AuthorizeGroupCapabilityAsynccall reddened 5 tests; weakening the source route filter fromMayEditPlacestoMemberreddenedPatchMovePlace_ViewerInSourceButEditorInDestination_RespondsWith403ProblemDetails— both halves genuinely bite, restored cleanly afterwards.Destination non-disclosure:
PatchMovePlace_ANonMemberDestinationAndANonexistentDestination_RespondIndistinguishablycompares status, content-type, problem body fields, trace-id shape and headers — a real indistinguishability proof, not just a status-code match.Version/xmin check: confirmed the atomic compare-and-swap holds cross-operation, not just same-method — raced
MoveToGroupAsyncagainst a concurrentUpdateAsyncon the same row and version 8 times, exactly one write always won and the row ended up consistent with the winner. The PR itself doesn't ship this cross-operation race test (onlyUpdateAsync's own multi-writer race is covered); worth adding givenMoveToGroupAsyncis new and shares the sameWHERE xmin = @ExpectedVersionshape, but not blocking — the SQL is identical in kind to the already-provenUpdateAsynccase.Personal-group argument: verified against the tests as written — third-party-into-someone-else's-personal-group is refused 404, and the same owner can move in both directions. The "Owner hides a place from other shared-group members" scenario named in the brief isn't exercised with a genuinely multi-member shared group, but the mechanism is identical to Editor-delete (ADR-0039), which already permits the same visibility loss without other members' consent — the ADR's argument holds, it's just not tested with a second member present.
Atomicity: single
UPDATE, confirmed one-group invariant viaGET /api/groups/{id}/placesfor both groups in the happy-path test. Didn't re-checkGET /api/placesafter a move in the PR's own suite, butListForCallerAsync'sgroup_id-scoped join makes this mechanically the same as the already-tested group-scoping cases.Task 148's safety net: confirmed
/api/places/{id}/groupis picked up byGroupScopedEndpointAuthorisationTests' route enumeration and carries only the sourceGroupCapabilityMetadata, matching ADR-0055 §"What task 148's safety net sees" exactly.OpenApiDocumentTestsroute list anddocs/adr/README.mdare both accurate. Full solution build,dotnet format --verify-no-changes, anddotnet test(728 tests) all clean on the pinned 10.0.100 SDK.Merged
main(PR #60, account deletion). Addressed both review gaps:MoveToGroupAsyncvsUpdateAsynccross-operation concurrency tests. First tried a symmetricTask.WhenAllrace — it passed even withMovePlaceToGroup's version guard deleted, becauseUpdateAsync's own guard alone kept exactly one write succeeding regardless of the broken one (the PR #58 shape). Replaced with two deterministic tests that force one write to commit while the other is genuinely blocked on the row lock, so each pins down its own operation's guard specifically. Verified each reddens when its guard is deleted and passes once restored.On that second test: I don't think the behaviour is wrong. An Editor of the shared group could already delete the same place outright (ADR-0039), so this grants no new destructive power — same visible effect to the other members either way. Worth flagging for whoever builds the WebUI though: group deletion gets a confirmation naming the place count (per
CLAUDE.md), and this move gets none, despite having a similar "things disappear from your shared view" effect. Left as a UX note rather than an API change.Verdict: mergeable
Both order-forced cross-operation race tests discriminate correctly: removed
MovePlaceToGroup's version clause and only the move-side test reddened (update-side unaffected); restored, removedUpdatePlace's clause and only the update-side test reddened (move-side unaffected). Neither masks the other. The ordering is genuinely lock-forced, not timing-dependent — the "locking" transaction holds a row-level lock via an uncommitted rawUPDATE, and whichever real statement queues behind it is guaranteed by Postgres'sEvalPlanQualre-check to evaluate itsWHEREagainst the post-commit row regardless of exactly when its command reaches the server; ran both eight times back to back with no variance. Correctly diagnoses the same PR #58 shape (Task.WhenAllsymmetric races can pass vacuously when only one side's guard is real).PatchMovePlace_OwnerOfASharedGroupMovingAPlaceIntoTheirPersonalGroup_HidesItFromTheOtherMembersis a real Owner/Editor/Viewer group, with a second, untouched place as a control — confirms both Editor and Viewer lose the moved place while retaining the surviving one, and only the mover sees it in the personal group afterwards. Matches the claim.Agree with the reasoning on hiding-by-move: MayEditPlaces already grants unconfirmed hard delete (ADR-0039, ADR-0005) with no notification to other members, and this app has no notification mechanism anywhere else either (no email, no audit trail per CLAUDE.md's ops posture) — so the silence isn't a new gap specific to this endpoint. If anything a move is less destructive than the delete it's compared against: the place survives, recoverable by the mover, where a delete is gone for everyone. The comparison to "group deletion confirms with a count" in the handler's remarks is the wrong-shaped comparison to worry about, since that guards group deletion, not the single-place delete this is actually equivalent to — but the remark reads as an aside pointing at future WebUI work, not as claiming architectural cover from that ADR, so it doesn't weaken the conclusion.
Merge conflict was exactly the one file, keep-both, alphabetically consistent; diffed the new head against the previously-reviewed commit and the only other changes present are
main's account-deletion feature (PR #60), unrelated to this endpoint.PlaceEndpoints.cs,PlaceRepository.cs,MovePlaceRequest.csand ADR-0055 are byte-identical to the version already reviewed. Full solution build,dotnet format --verify-no-changes, anddotnet test(769 tests) clean on the pinned 10.0.100 SDK.