feat(apply): migrate a group's type through POST /groups/{id}/grouptype - #190
Merged
Merged
Conversation
Closes #171. A `groupTypeId` change was planned as an ordinary field update and sent on the regular group update, which ChurchTools always rejects: PATCH /groups/1182 -> 400 groupTypeId: validation.always.invalid So the plan looked applicable and the apply died partway through. CT migrates a group's type through a dedicated endpoint instead, taking a mapping from every role of the old type onto a role of the new one: POST /groups/{id}/grouptype { groupTypeId, roleMapping: { <oldRoleId>: <newRoleId> } } Verified live (CT 3.137.0-RC21, 2026-09-28) by migrating a group and migrating it back: both sides of `roleMapping` are groupTypeRoleIds from /group/roles, keyed by the roles of the group's CURRENT type; the endpoint answers 204 with no body. That mapping decides where existing MEMBERSHIPS land, so it is never guessed. It is derived only where the answer cannot cost anyone their role: name exactly one role of the target type carries the same name empty-role the old role holds no members in this group, so nothing can move declared a `roleMapping` on the group — always wins A role that holds members and has no same-named target is refused at PLAN time, naming the role, its member count and the target type's roles. Nothing is written before that refusal, which keeps `plan` honest: it no longer renders an applicable-looking diff that is guaranteed to fail. The plan spells the migration out under the field rather than showing a bare `groupTypeId: 5 -> 4`, because applying it moves people: ~ group.team_academy_first_year (#1182) groupTypeId: 5 -> 4 via POST /groups/1182/grouptype — role mapping (members follow their role): Mitglied -> Mitglied (7 members, matched by name) Supporter -> Mitglied (0 members, matched by empty-role) Apply POSTs the migration first, then PATCHes the remaining fields with `groupTypeId` withheld — so a refused migration leaves every other field untouched, and state still records the new type, so a re-plan is a no-op. Costs nothing when no type changes: the resolver issues no request at all. When one does it reads /group/roles once plus one members page per migrating group; the members read is what makes an empty role safe to map, so it is not optional. Verified against a live instance: the two groups that could never converge now render a full derived mapping and plan cleanly. Claude-Session: https://claude.ai/code/session_016XVmiQY44pjx1u9Fu4iSDP
…tions - read /group/roles and group members with getAll: a role whose holders sit past page 1 counted as empty and was mapped away by empty-role - refuse a groupTypeId change whose target is a pending ref (was silently planned as the PATCH CT always rejects) - refuse roleMapping keys that name no current-type role, and occupied roles the catalog does not list under the current type - name a failed DECLARED target in the refusal instead of asking for it again - render the role mapping in the markdown plan - replace real group identifiers in docs/tests with generic ones
Member
Author
Review triage (
|
| # | Finding | Verdict |
|---|---|---|
| 1 | /group/roles read with plain get |
Fixed (getAll). Not reproducible on dev (plain get returned all 72 rows), but #101 documents the 10-row truncation, so it follows the repo convention |
| 2 | Members read as one ?limit=200 page |
Fixed + regression test. Real: a role whose holders are all past page 1 counted as empty and was mapped away by empty-role. Also added a refusal when members hold a role the catalog does not list under the current type |
| 3 | Pending-ref / non-numeric type change skipped → falls back to the PATCH CT rejects | Fixed: refused at plan time with instructions |
| 4 | Markdown plan omits the mapping | Fixed + test |
| 5 | Declared roleMapping key matching no role is silently ignored |
Fixed: refused, naming the current type's roles |
| 7 | Refusal message wrong for a failed declared target | Fixed |
| 10 | Real group key/id/name in docs, tests and this description (public repo) | Fixed: replaced with generic identifiers |
| 6 | ref.groupRole grants resolved against pre-migration roles |
Not fixed: needs a type change and a grant on the same group in one run. A follow-up if it ever matters |
| 8 | Duplicate catalog fetch / sequential member reads | Skipped: one extra request, and only when something changes type; sequential is deliberate given rate limits |
| 9 | Executor/renderer special-case groupTypeId |
Skipped: a registry hook for a single field isn't worth it yet |
Every behavioural test fails before and passes after. npm test 1230 passed; typecheck, eslint, build and the docs gate are clean. Read-only ct plan --env dev with this build renders both real migrations with full mappings and no refusals.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #171.
The bug
A
groupTypeIdchange was planned as an ordinary field update and sent on the regular group update, which ChurchTools always rejects:So
ct planrendered an applicable-looking diff andct applydied partway through it. CT migrates a group's type through a dedicated endpoint instead:Verified live (CT 3.137.0-RC21, 2026-09-28) by migrating a group and migrating it back: both sides of
roleMappingaregroupTypeRoleIds from/group/roles, keyed by the roles of the group's current type, and the endpoint answers204with an empty body.Why the mapping is never guessed
That mapping decides where existing memberships land — whoever holds a role mapping to
XholdsXafterwards. #171 is explicit that memberships must not be silently dropped, so a mapping is derived only where the answer cannot cost anyone their role:nameempty-roledeclaredroleMappingon the group — always winsA role that holds members with no same-named target is refused at plan time, naming the role, its member count and the target type's roles:
Nothing is written before that refusal — which is the point.
planstays honest rather than rendering a diff guaranteed to fail.Ambiguity is refused too: a name match only counts when exactly one target role carries the name, so "one of the two Leiter roles" is never picked for you.
The plan says what it will do
A bare
groupTypeId: 5 -> 4under-reports an operation that moves people, so the mapping is rendered under the field:applysends exactly that mapping — resolution happens once, duringbuildPlan, so the rendered plan and the executed call cannot drift apart.Apply ordering
The migration POSTs first, then the remaining fields PATCH with
groupTypeIdwithheld. A failed migration therefore leaves every other field untouched, and when the type is the only change no PATCH is sent at all. State records the new type, so a re-plan is a no-op — the round-trip rule inCONTRIBUTING.md.Cost
Zero when nothing changes type: the resolver issues no request. When something does, it reads
/group/rolesonce plus one members page per migrating group. That members read is what makes an empty role safe to map automatically, so it is not optional.New config surface
Names, not ids, so one config stays portable across hosts; slug-compared. Like
allowDuplicateNameit is never a managed field — not diffed, not in state, never adopted, read only when the type actually changes.Verification
npm test— 1224 passed, 5 skipped (23 new)npm run typecheck— cleannpx eslint src tests— cleannpm run build— cleanNew tests cover the derivation table (incl. real role catalogs read from a live instance), the ambiguity and occupied-role refusals, declared mappings winning over name matches, the rendered plan, and the executor's two calls — that the migration is POSTed with the rendered mapping, that
groupTypeIdnever reaches the PATCH, the ordering, and the state round-trip.Note on lint
npm run lintreports 7 errors in.claude/worktrees/.../dist/index.js, a stray untracked build artefact in my checkout — not part of the repo and not from this change.npx eslint src testsis clean. Worth a.eslintignore/ignoresentry for.claude/**separately if others hit it.https://claude.ai/code/session_016XVmiQY44pjx1u9Fu4iSDP