From d7c7a01824e8c8310e98105d8dacaa6da9cd54c1 Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Mon, 24 Aug 2026 19:38:31 +0200 Subject: [PATCH 1/3] feat: manage group member fields as group-scoped resources (#135) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A group member field has a ChurchTools id but belongs to exactly one group and is not globally reusable, so it is modelled as an owned sub-resource of the group rather than a registry type: declared inside `ct.group({ memberFields: [...] })`, folded per field into `memberField:` pseudo-fields, and applied inline with its group. Identity is the managed group key plus the local field key (`ojbp_2026_27_praktikum_1::wahl`). No ChurchTools field id ever reaches authored config or an adopted blueprint — declaring one is a config error. - adopt: `ct adopt group --with-member-fields` (opt-in only) - plan: per-field creates and updates; the actual side is narrowed to the declared properties so a clean apply re-plans as a no-op - apply: creates fields after the owning group exists, updates in place by identity (PATCH, PUT fallback on 405/501) - delete: never implicit. A dropped field produces no desired diff key at all; it is reported as a DELETE CANDIDATE and removed only by the explicit `ct destroy --member-field ::`, which inherits the owning group's backup, typed confirmation and preventDestroy guardrails - rulesets: `ref.groupMemberField(group, field)` resolves per host; a reference to a field the target group does not declare fails at config-eval time, and one into an adopted group is hard-errored at plan time — both before apply - ordering: synthetic fields are applied in registration order (parents → memberField:* → dynamic), and synthetic pending refs are resolved immediately before each write, so a ruleset can name a field created in the same run Also fixes a related fold gate: a config whose only dynamic group was created by that same run never folded its desired `dynamic` block, so its ruleset silently did not apply on the first run. Closes #135. Claude-Session: https://claude.ai/code/session_018JShVZYNLaRb4hF5KbHCXG --- README.md | 17 +- docs/api-coverage.md | 3 + docs/handbuch/blueprints.md | 21 +- docs/handbuch/dynamic-groups.md | 36 ++- docs/handbuch/group-member-fields.md | 209 ++++++++++++ docs/handbuch/index.md | 4 + docs/handbuch/mkdocs.yml | 1 + docs/handbuch/permissions.md | 11 +- src/commands/adopt-group.ts | 55 ++++ src/commands/destroy.ts | 184 ++++++++++- src/commands/state.ts | 3 + src/config/context.ts | 161 +++++++++- src/engine/execute.ts | 36 ++- src/engine/member-fields.ts | 205 ++++++++++++ src/engine/synthetic.ts | 228 ++++++++++++- src/engine/types.ts | 11 + src/resolve/refs.ts | 55 +++- src/resolve/resolver.ts | 112 +++++++ src/state/state.ts | 11 + tests/adopt-group-command.test.ts | 78 +++++ tests/destroy-command.test.ts | 63 ++++ tests/member-fields.test.ts | 463 +++++++++++++++++++++++++++ tests/synthetic-dynamic.test.ts | 6 + 23 files changed, 1939 insertions(+), 34 deletions(-) create mode 100644 docs/handbuch/group-member-fields.md create mode 100644 src/engine/member-fields.ts create mode 100644 tests/member-fields.test.ts diff --git a/README.md b/README.md index e58d589..802066c 100644 --- a/README.md +++ b/README.md @@ -159,14 +159,15 @@ confirmation — and a declaration marked `preventDestroy: true` blocks even tha ## What it manages -| Resource | DSL | Guide | -| -------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------- | ------------------------------------------------- | -| Campuses | `ct.campus` | [config guide](docs/configuration.md) | -| Groups (fields, campus, multi-parent hierarchy) | `ct.group` | [config guide](docs/configuration.md) | -| Group types, roles, age/target groups, relationship types, person statuses | `ct.groupType`, `ct.roleDefinition`, `ct.ageGroup`, `ct.targetGroup`, `ct.relationshipType`, `ct.personStatus` | reusable building blocks | -| Permissions (group-role, group-type-role, person-status) | `ct.groupRole`, `ct.groupTypeRole`, `ct.status` | [permissions](docs/handbuch/permissions.md) | -| Auto-groups (dynamic groups) | the `dynamic` block on a group | [dynamic groups](docs/handbuch/dynamic-groups.md) | -| Repeated structure, parametrized | a plain function over the DSL | [blueprints](docs/handbuch/blueprints.md) | +| Resource | DSL | Guide | +| -------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------- | +| Campuses | `ct.campus` | [config guide](docs/configuration.md) | +| Groups (fields, campus, multi-parent hierarchy) | `ct.group` | [config guide](docs/configuration.md) | +| Group types, roles, age/target groups, relationship types, person statuses | `ct.groupType`, `ct.roleDefinition`, `ct.ageGroup`, `ct.targetGroup`, `ct.relationshipType`, `ct.personStatus` | reusable building blocks | +| Permissions (group-role, group-type-role, person-status) | `ct.groupRole`, `ct.groupTypeRole`, `ct.status` | [permissions](docs/handbuch/permissions.md) | +| Auto-groups (dynamic groups) | the `dynamic` block on a group | [dynamic groups](docs/handbuch/dynamic-groups.md) | +| Group member field definitions (group-scoped, never deleted implicitly) | the `memberFields` block on a group | [group member fields](docs/handbuch/group-member-fields.md) | +| Repeated structure, parametrized | a plain function over the DSL | [blueprints](docs/handbuch/blueprints.md) | Read-only by design: the person master-data model, security levels and custom-field _definitions_ (`ct get person-masterdata`, `ct get data-fields`) — diff --git a/docs/api-coverage.md b/docs/api-coverage.md index 385fc0a..bdbde1d 100644 --- a/docs/api-coverage.md +++ b/docs/api-coverage.md @@ -32,6 +32,8 @@ Methods marked only if they actually exist on the matched path. "Update" = PUT o | 13 | `ct_person_status` | `/statuses`, `/statuses/{id}` | list ✅ / by-id ✅ | ✅ | ✅ PUT | ✅ | **Full CRUD, live-verified 2026-08-13** (eqrm prod, CT 3.135.2, read from the instance OpenAPI spec): `/statuses` → GET, POST; `/statuses/{id}` → GET, PUT, DELETE. `POST` requires `name`, `shorty`, `isMember`; `PUT` requires ALL of `name`, `shorty`, `isMember`, `isSearchable`, `sortKey`, `securityLevelId` — uniquely strict among managed types (every other managed PUT declares no required fields), and since PUT is a full replace the registry manages all six rather than a subset. The person/community **Status** master data (tag `Status`): "0 - First", "3 - Group Active", …, the domain a `ct.status` permission grant hangs off. Adoptable since #96, which is what makes a config using that domain self-sufficient across hosts. Do NOT conflate with `ct_group_status` (row 8, no endpoint at all) or `/group/memberstatus` (member statuses, string ids). Master data — never a person record; the people guard is unaffected. | | 14 | `ct_department` | `/departments` | list ✅ / by-id ❌ | ❌ | ❌ | ❌ | **No REST write path — a ref catalog here, not a managed resource.** Live-probed 2026-08-13 (eqrm prod, CT 3.135.2): `GET /departments` returns `[{id, name, nameTranslated, sortKey, shorty}]`; no `POST`/`PUT`/`DELETE` on `/departments` exists in the spec, and there is no `/departments/{id}` path at all. Bereiche are the `cdb_bereich` permission scope dimension (`churchdb:view alldata`), so `ct` resolves them BY NAME for a `scope: [{ department: "…" }]` reference (#98) and surfaces them via `ct get departments` — but `ct.department` declares one and `ct adopt department ` adopts one. **Managed since #108** — `ct` writes Bereiche through the legacy `POST /index.php?q=churchdb/ajax` `func=saveMasterData` interface the admin UI uses (create/update/delete all verified live on eqrm-dev 2026-08-14). It appears in no OpenAPI spec, which is why an OpenAPI-only audit could never have found it (#111). Note there is no `/departments/{id}` at all, so a single Bereich is read by filtering the collection. | +| 15 | `ct_group_member_field` | `/groups/{groupId}/memberfields` (GET), `/groups/{groupId}/memberfields/group` (POST), `/groups/{groupId}/memberfields/group/{groupMemberFieldId}` (PATCH/PUT/DELETE) | list ✅ (per group) / by-id ❌ | ✅ | ✅ PATCH (PUT fallback) | ✅ | **Full CRUD, but GROUP-SCOPED — not a standalone resource (#135).** A member field belongs to exactly one group and is not globally reusable, so it has no collection path of its own and no entry in the resource registry: it is a _synthetic owned sub-resource_ of `ct_group`, declared inside `ct.group({ memberFields: [...] })`, exactly like hierarchy `parents` and the dynamic ruleset. Its portable identity is the managed group key plus a local field key (`ojbp_2026_27_praktikum_1::wahl`); a ChurchTools field id never reaches authored config or an adopted blueprint. Reads are narrowed to rows the `/memberfields/group` endpoints own — the same GET also returns fields sourced from person master data / group-type defaults, which are neither adoptable nor writable here. `apply` NEVER deletes one (a field dropped from config produces no desired diff key at all); removal is the explicit `ct destroy --member-field ::`. Update uses `PATCH` (partial, so unmanaged siblings survive) and falls back to `PUT` on a 405/501. Definitions only — never a per-person value; `assertNotPeople` still guards every path. Distinct from the group CUSTOM fields of #48/#60 (`/dbfields`, `fieldCategory.table == "cdb_gruppe"`), which describe the group RECORD rather than what its members are asked. | + > **Create-time required fields (#73).** The "POST ✅" marks above were spec-derived, not live-exercised for > create. CT's POST validators require fields the tool does not manage for diffing: `group-type` needs > `namePlural`/`shorty`/`color`/`permissionDepth`/`isLeaderNecessary`/`availableForNewPerson` (+ `sortKey`/`postsEnabled`), @@ -61,6 +63,7 @@ No collection POST; state is set/removed through PUT/DELETE on the item path. Mo - `ct_group_hierarchy` — manage parent links via `PUT`/`DELETE /groups/{groupId}/parents/{parentGroupId}` - `ct_permission` — assign/revoke via `PUT`/`DELETE /permissions/{domainType}/{domainId}` +- `ct_group_member_field` — full CRUD verbs, but only ever **under a group** (#135), so it is a synthetic owned sub-resource rather than a registry type: declared as `memberFields` on `ct.group(...)`, applied inline with that group (after it exists, before its ruleset), and deleted only by the explicit `ct destroy --member-field ::` ### Partial — 1 resource diff --git a/docs/handbuch/blueprints.md b/docs/handbuch/blueprints.md index 2190cf4..f25dcf8 100644 --- a/docs/handbuch/blueprints.md +++ b/docs/handbuch/blueprints.md @@ -4,7 +4,7 @@ sources: - src/config/context.ts - src/engine/graph.ts - src/engine/hierarchy.ts -sources_hash: 37db6b8730470f73 +sources_hash: a1b74a4f10325270 reviewed: 2026-08-17 --- @@ -201,12 +201,15 @@ A blueprint doesn't need to worry about sequencing: `dependsOn`), so `orderKeys` always places `${campus}_kids_lead` before `${campus}_kids_0_3`/`_4_6`/`_checkin`/`_all` — the topological sort guarantees it structurally, not by declaration order. -- **Hierarchy and auto-group state ride along with their group.** `parents` - and `dynamic` are _synthetic fields_ on the `group` resource itself (see - `SYNTHETIC_FIELDS` in [`src/engine/synthetic.ts`](https://github.com/eqrm/ct-cli/blob/main/src/engine/synthetic.ts)), +- **Hierarchy, member fields and auto-group state ride along with their + group.** `parents`, `memberFields` and `dynamic` are _synthetic fields_ on + the `group` resource itself (see `SYNTHETIC_FIELDS` in + [`src/engine/synthetic.ts`](https://github.com/eqrm/ct-cli/blob/main/src/engine/synthetic.ts)), not separate resources with their own tier — they're diffed and applied as part of that same group's create/update, once the group (and, for - `parents`, its referenced parent groups) already exist. + `parents`, its referenced parent groups) already exist. Within one group + they are written in that registered order, so a group's member fields exist + before a ruleset that references them is installed. - **Permissions apply after the structural plan.** `ct.groupTypeRole` / `ct.groupRole` declarations aren't part of `orderKeys`'s dependency graph at all — they go through a separate plan/apply pass @@ -238,6 +241,14 @@ Group "berlin_kids_0_3" declares hierarchy parent "berlin_kids_laed", which is n Managed parents must reference a group by its key (omit unmanaged parents entirely). ``` +The same pass validates **group member field references** (#135): a +`ref.groupMemberField("", "")` anywhere in a declaration — a +dynamic ruleset included — must name a group declared in this config that +declares that field, or `evaluateConfig` throws before any plan runs. Member +fields are the surface where this matters most in a blueprint, because a field +is owned by exactly one group: a blueprint instantiated twice creates two +independent `wahl` fields, and a reference must say _which group's_. + This matters more in a blueprint than in a hand-written flat config, because the `${campus}_`-prefixed key is itself computed (`` `${campus}_kids_lead` ``, not a literal string) — a copy-paste slip in diff --git a/docs/handbuch/dynamic-groups.md b/docs/handbuch/dynamic-groups.md index 7919861..43b8278 100644 --- a/docs/handbuch/dynamic-groups.md +++ b/docs/handbuch/dynamic-groups.md @@ -6,7 +6,7 @@ sources: - src/engine/dynamic.ts - src/engine/synthetic.ts - src/commands/adopt-group.ts -sources_hash: 13e4a54533b0e8a2 +sources_hash: 69a1413762039f5c reviewed: 2026-08-17 --- @@ -67,6 +67,36 @@ the group's own tier-1 apply, after the group itself has been created/updated — so `ct apply` always writes against the group's real (possibly just-created) id. +That holds on the very first run too, including when the group being made +dynamic is created by that same run and is the _only_ dynamic group in the +config (fixed alongside #135): the desired `dynamic` block is folded whenever +any group declares one, not only when there is already an adopted dynamic group +to read. + +The group's own synthetic fields are written **in declaration order**, and that +order is fixed by the synthetic-field registry in `src/engine/synthetic.ts`: +hierarchy `parents`, then `memberField:*`, then `dynamic`. So a group's +[member fields](group-member-fields.md) are created before the ruleset that may +reference them. + +### Referencing a group member field portably (#135) + +A ruleset that names one of a group's member fields must not carry that host's +numeric field id — it is not portable, and ChurchTools validates none of the ids +inside a ruleset, so the wrong id applies cleanly and silently computes the +wrong membership. Use the group-scoped reference instead: + +```ts +ref.groupMemberField("ojbp_2026_27_praktikum_1", "wahl"); +``` + +The resolver maps it to this host's id from +`GET /groups/{groupId}/memberfields`. A field this config declares but that does +not exist on the host yet resolves to a pending marker and is completed during +apply, right after the create that minted it. A reference to a field the target +group does not declare fails at config-eval time, before any network call. See +[Group member fields](group-member-fields.md). + ## Supplying a ruleset — three ways 1. **Inline `RuleSet` object literal** — write the object by hand (e.g. @@ -140,6 +170,10 @@ ct adopt group --with-dynamic # portablized (the d ct adopt group --with-dynamic --no-portable-rulesets # verbatim, this host's ids ``` +`--with-member-fields` is the sibling opt-in for a group's own +[member-field definitions](group-member-fields.md); combine the two to capture a +whole structure — group, fields, ruleset — in one pass. + > **A re-adopt refreshes the snapshot, not the key (#123).** Re-running this over > a list of ids is the documented way to refresh rulesets once their scope > targets become managed — and it is the one mode where `-k` is rejected ("only diff --git a/docs/handbuch/group-member-fields.md b/docs/handbuch/group-member-fields.md new file mode 100644 index 0000000..2fa946e --- /dev/null +++ b/docs/handbuch/group-member-fields.md @@ -0,0 +1,209 @@ +--- +sources_hash: 43f62be625184052 +title: Group member fields +sources: + - src/engine/member-fields.ts + - src/engine/synthetic.ts + - src/config/context.ts + - src/commands/adopt-group.ts + - src/commands/destroy.ts +reviewed: 2026-08-24 +--- + +# Group member fields (#135) + +A ChurchTools group can ask its members for extra information — "Wahl", +"Praktikumsplatz", a free-text note. Those are **group member fields**: field +_definitions_ that belong to one group. + +`ct` manages them as **group-scoped owned resources**: they are declared inside +the group that owns them, adopted with the group, created after it, and never +deleted implicitly. + +> Definitions only, never people. A member field says _what a group asks_. The +> value a specific person answered is a per-record value and is permanently out +> of scope, like every other person field (see +> [Field definitions](field-definitions.md)). `assertNotPeople` still guards +> every write. + +This is a **different surface** from the group custom fields of #48/#60, which +live on `/dbfields` (`fieldCategory.table == "cdb_gruppe"`) and describe the +group _record_ rather than what its members are asked. + +## Why the identity is `::` + +A member field has a ChurchTools id, but it belongs to exactly one group and is +**not globally reusable**. Two groups declaring a field called `wahl` are two +independent resources with two different ids. + +So the portable identity is the **managed group key plus the local field key**: + +```text +ojbp_2026_27_praktikum_1::wahl +``` + +That is what makes the blueprint use case work. Running the same function for +25/26 and for 26/27 creates new groups and new fields; no 25/26 field id takes +part in resolving 26/27. + +**A ChurchTools field id never appears in authored config or in an adopted +blueprint.** Declaring `id`, `groupMemberFieldId` or `groupId` on a member field +is a config error, not a warning — a host-specific id in a config that is meant +to stand up any host is the one defect the whole tool exists to prevent. + +## Declaring them + +```ts +ct.group({ + key: "ojbp_2026_27_praktikum_1", + name: "OJBP 1. Praktikum 26/27", + groupTypeId: 5, + memberFields: [ + { + key: "wahl", + name: "Wahl", + fieldTypeCode: "text", + requiredInRegistrationForm: true, + }, + ], +}); +``` + +`memberFields` is **opt-in**, exactly like `parents` and `dynamic`: + +| Declaration | Meaning | +| --------------------- | ---------------------------------------------------------------- | +| omitted | the group's member fields are not managed at all | +| `memberFields: []` | managed, none declared — still never deletes an existing field | +| `memberFields: [ … ]` | these fields are created/updated; anything else is left in place | + +`key` is the group-local key and is unique within the group. Everything else is +a ChurchTools member-field property. + +### Managed properties + +`name`, `fieldTypeCode`, `defaultValue`, `options`, `nameInSignupForm`, `note`, +`noteInSignupForm`, `requiredInRegistrationForm`, `useInRegistrationForm`, +`securityLevel`, `sortKey`. + +Two readable/writable properties are deliberately **not** managed: + +- **`id`** — host-specific; see above. +- **`referenceName`** — this _is_ the local identity, not a diffable property. + It is what a create sends and what every later run matches on, so managing it + would let a rename silently re-key the resource and re-create the field + instead of updating it. (A field created in the ChurchTools UI, where CT may + mint its own `referenceName`, is matched by its slugged `name` as a fallback.) + +A property outside the managed list still passes through to ChurchTools +unchanged — it only earns a warning, and it is never diffed. + +## Adoption is opt-in — `--with-member-fields` + +```bash +ct adopt group 4711 --with-member-fields +ct adopt group --children-of ojbp_2025_26 --with-member-fields +``` + +The emitted snippet carries a `memberFields:` block with every group-scoped +field and **no ChurchTools ids** — paste it, re-key it for the next year, and +`ct plan` proposes fresh groups and fresh fields. + +Rows the group's member form shows but that are not group-scoped (person master +data, group-type defaults) are not emitted: only `/memberfields/group` rows can +be created or updated. + +!!! note "Why opt-in, and not the default" +Whether an owned child resource is adopted automatically is a project-wide +contract, not a per-feature choice. That decision lives on +[ct-cli#141](https://github.com/eqrm/ct-cli/issues/141); until it lands, +member fields are captured only when you ask for them. + +## Plan and apply + +`plan` diffs **per field**, under the pseudo-field `memberField:`: + +```text + + group.ojbp_2026_27_praktikum_1 + name: "OJBP 1. Praktikum 26/27" + memberField:wahl: {"name":"Wahl","fieldTypeCode":"text"} + + ~ group.ojbp_2025_26_praktikum_1 (#4711) + memberField:wahl: {"name":"Wahl"} -> {"name":"Wahl (neu)"} +``` + +The actual side is narrowed to exactly the properties the declaration names, so +a server default ChurchTools returns can never make the two sides differ +forever: **a clean apply re-plans as a no-op.** + +`apply` creates a field only after its owning group exists (the group's own +create runs first, then its owned sub-resources), and updates an existing field +in place — matched by identity, never re-created. + +If the member-field read fails with anything other than a 404, the group is +reported as `fetch-failed` and the plan says it is INCOMPLETE. It never +manufactures "create every field" out of a transient error. + +## Nothing is ever deleted implicitly + +Removing a field from `memberFields` does **not** delete it. The diff walks the +declared side, so a removal produces no key, no change and no write — it is +structurally impossible for the engine to propose it. + +The live field is still surfaced, every plan, as a **delete candidate**: + +```text +! group "ojbp_2025_26_praktikum_1": member field + "ojbp_2025_26_praktikum_1::alt" exists in ChurchTools but is not declared — + DELETE CANDIDATE, left untouched. ct never removes a member field because it + vanished from config; remove it explicitly with + `ct destroy --member-field ojbp_2025_26_praktikum_1::alt`. +``` + +Removing one is an explicit, separate operation: + +```bash +ct destroy --member-field ojbp_2025_26_praktikum_1::alt +``` + +It carries the same guardrails as any other destroy: a pre-delete backup of the +field definition, a typed confirmation, and the owning group's `preventDestroy` +— protecting a group protects the fields it owns. + +## Referencing a field from a dynamic ruleset + +A ruleset must not freeze one host's field id into a file that is applied +elsewhere, so a member field is referenced by its portable identity: + +```ts +ref.groupMemberField("ojbp_2026_27_praktikum_1", "wahl"); +``` + +Three things follow: + +1. **A reference to a field that is not declared for the target group fails at + config-eval time** — offline, before any network call — naming what the group + does declare. ChurchTools validates none of the ids inside a ruleset, so + without this the mistake would apply cleanly and simply compute the wrong + membership. +2. A reference into a group that is merely _adopted_ (in state, not declared) is + checked against the live `GET /groups/{id}/memberfields` at plan time, and + hard-errors there — still before apply writes anything. +3. **Ordering is guaranteed within an apply.** A group's member fields are + written before that group's ruleset is installed, and a reference from + another group adds a dependency edge onto the owning group. A field created + in the same run resolves to the id the create just minted. + +## ChurchTools endpoints + +| Operation | Path | +| --------- | ------------------------------------------------------- | +| read | `GET /groups/{groupId}/memberfields` | +| create | `POST /groups/{groupId}/memberfields/group` | +| update | `PATCH /groups/{groupId}/memberfields/group/{fieldId}` | +| delete | `DELETE /groups/{groupId}/memberfields/group/{fieldId}` | + +`PATCH` is used because it is a partial update, so unmanaged sibling properties +are left alone. An instance whose endpoint implements only `PUT` answers +405/501, and the update falls back to `PUT` rather than failing an apply over a +verb. diff --git a/docs/handbuch/index.md b/docs/handbuch/index.md index 4b894d3..b907e51 100644 --- a/docs/handbuch/index.md +++ b/docs/handbuch/index.md @@ -38,6 +38,10 @@ each page marks explicitly what was **verified** and what is still an - **[Field definitions](field-definitions.md)** — the person master-data model, security levels, and person/group custom-field _definitions_: what is readable over REST, what is writable, and where the boundary to per-record values runs. +- **[Group member fields](group-member-fields.md)** — the field definitions a + group asks its members for: why their identity is scoped by the owning group, + how they are adopted portably (no ChurchTools ids), and why nothing is ever + deleted just because it left the config. - **[Blueprints](blueprints.md)** — describing a repeated structure (e.g. one campus's area scaffold) once and instantiating it per campus. diff --git a/docs/handbuch/mkdocs.yml b/docs/handbuch/mkdocs.yml index 8fa7ae7..f0d8558 100644 --- a/docs/handbuch/mkdocs.yml +++ b/docs/handbuch/mkdocs.yml @@ -79,4 +79,5 @@ nav: - Permissions: permissions.md - Dynamic groups: dynamic-groups.md - Field definitions: field-definitions.md + - Group member fields: group-member-fields.md - Blueprints: blueprints.md diff --git a/docs/handbuch/permissions.md b/docs/handbuch/permissions.md index 1144be5..f3a7d8c 100644 --- a/docs/handbuch/permissions.md +++ b/docs/handbuch/permissions.md @@ -5,7 +5,7 @@ sources: - src/resolve/resolver.ts - src/resolve/refs.ts - src/config/context.ts -sources_hash: 87fe6c847d70d89c +sources_hash: 8201657847a7964a reviewed: 2026-08-17 --- @@ -413,6 +413,15 @@ the rest of the DSL uses — so both spellings are interchangeable. | `cc_securitylevel` | `{ securityLevel: "" }` | managed security levels, then `GET /securitylevels` (#110) | | `cdb_comment_viewer` | `{ commentViewer: "" }` | `GET /person/commentviewers` **only** (#102) | +Two `Ref` kinds are deliberately **not** in that table because no permission +dimension scopes by them: `group-type-role` (a `groupTypeRoleId`, addressed by +its `(group type, role name)` pair — #76) and `group-member-field` (a +group-scoped member-field definition, addressed by its portable +`(group key, local field key)` pair — #135). They share this file's resolver and +its "managed state first, then live lookup, else a hard error at plan time" +rules, but they are referenced from **dynamic-group rulesets**, not from grant +scopes. See [Group member fields](group-member-fields.md). + **Why this matters:** campus ids are host-specific — Mainz is `0` on eqrm prod and `6` on eqrm dev. A campus-scoped grant written as a numeric literal is therefore a cross-environment misgrant, and because declaring a domain makes diff --git a/src/commands/adopt-group.ts b/src/commands/adopt-group.ts index e3be69e..d8618f7 100644 --- a/src/commands/adopt-group.ts +++ b/src/commands/adopt-group.ts @@ -16,6 +16,12 @@ import { CtApiError, type CtClient } from "../api/ctClient.js"; import { resolveConfig } from "../config.js"; import { prepareEnv } from "../env/context.js"; import { normalizeRuleset } from "../engine/dynamic.js"; +import { + groupScopedRows, + localKeyOf, + MEMBER_FIELD_PROPS, + memberFieldsReadPath, +} from "../engine/member-fields.js"; import type { DynamicStatus } from "../engine/types.js"; import { RESOURCES, configSnippet, fromInformation, slug } from "../resources/registry.js"; import { ReverseResolver, type RoleCatalogEntry } from "../resolve/reverse.js"; @@ -32,6 +38,8 @@ interface AdoptGroupOptions { type?: string; childrenOf?: string; withDynamic?: boolean; + /** Opt in to capturing the group's group-scoped member-field definitions (#135). Never the default. */ + withMemberFields?: boolean; /** Opt in to changing an already-managed group's logical key (#123). Never the default. */ rekey?: boolean; /** Commander's negatable `--no-portable-rulesets`: true unless the flag was passed (#101). */ @@ -186,6 +194,42 @@ interface DynamicCapture { normalizedRuleset: Record; } +/** + * Capture a group's group-scoped member-field DEFINITIONS as portable declarations (#135). + * + * Emits `{ key, ...managed properties }` per field and **never a ChurchTools id** — the field's + * identity in config is the group key plus its local key (`ojbp_2026_27_praktikum_1::wahl`), so the + * same blueprint applied to another group, or another host, mints its own fields rather than + * resolving against this host's numbering. The local key comes from CT's own `referenceName` + * (slugged), falling back to the slugged name for a field created in the ChurchTools UI. + * + * Opt-in only (`--with-member-fields`). Whether owned child resources are adopted by default is a + * project-wide contract, deliberately left to #141. + */ +async function captureMemberFields( + id: number, + client: Pick, +): Promise> | undefined> { + let raw: unknown; + try { + raw = await client.get(memberFieldsReadPath(id)); + } catch (err) { + // A group whose member fields cannot be read is not a reason to abort a bulk adoption of a whole + // subtree — say so and adopt the group without them. + if (err instanceof CtApiError && err.status === 404) return undefined; + throw err; + } + const rows = groupScopedRows(raw); + if (rows.length === 0) return undefined; + return rows.map((row) => { + const declaration: Record = { key: localKeyOf(row) }; + for (const prop of MEMBER_FIELD_PROPS) { + if (row[prop] !== undefined) declaration[prop] = row[prop]; + } + return declaration; + }); +} + /** Fetch + normalize a group's ruleset and status. `undefined` (never throws) if the group isn't dynamic. */ async function captureDynamic( id: number, @@ -228,6 +272,11 @@ export function adoptGroupCommand(): Command { "--with-dynamic", "also capture each dynamic group's ruleset to rulesets/.json and emit the dynamic: block", ) + .option( + "--with-member-fields", + "also capture each group's group-scoped member-field definitions and emit the memberFields: " + + "block (portable: no ChurchTools field ids are ever emitted)", + ) .option( "--portable-rulesets", "(deprecated — this is the default since #101) rewrite managed entity ids into portable logical refs", @@ -350,6 +399,12 @@ export function adoptGroupCommand(): Command { // captured `dynamic` block (if any) is appended AFTER, so it is not treated as an id field. const { fields: sugared, todos } = await reverse.sugarFields(fields); const snippetFields: Record = sugared; + // Emitted BEFORE `dynamic` so the snippet reads in apply order — the fields a ruleset may + // reference are declared above the ruleset that references them (#135). + if (opts.withMemberFields) { + const memberFields = await captureMemberFields(id, client); + if (memberFields) snippetFields.memberFields = memberFields; + } if (opts.withDynamic) { const captured = await captureDynamic(id, client); if (captured) { diff --git a/src/commands/destroy.ts b/src/commands/destroy.ts index 0bdc763..c2cc476 100644 --- a/src/commands/destroy.ts +++ b/src/commands/destroy.ts @@ -11,12 +11,22 @@ import { fetchActual } from "../engine/build.js"; import { parentIdsByGroupId, managedParentKeys, type HierarchyEntry } from "../engine/hierarchy.js"; import type { DesiredResource } from "../engine/types.js"; import { writeBackup } from "../engine/backup.js"; +import { + groupScopedRows, + matchesLocalKey, + memberFieldItemPath, + memberFieldRowId, + memberFieldsReadPath, + parseMemberFieldIdentity, +} from "../engine/member-fields.js"; import { resolveBackupDir } from "./apply.js"; import { confirmTyped, confirmEnv } from "../ui/prompt.js"; import { info, warn, success, error, formatError } from "../ui.js"; interface DestroyOptions { target?: string[]; + /** Group-scoped member fields to delete, by their portable `::` identity (#135). */ + memberField?: string[]; state?: string; env?: string; confirmEnv?: string; @@ -105,6 +115,130 @@ async function fetchParentEdges( * `person-status`, whose deletion mutates every person carrying it), so an unattended `--force` * teardown is exactly the run that should stop and ask. */ +/** One `--member-field` target, resolved against state. */ +export interface MemberFieldTarget { + /** The portable `::` identity, as typed. */ + identity: string; + groupKey: string; + fieldKey: string; + groupId: number; +} + +/** + * Resolve `--member-field ::` targets against the state file (#135). + * + * This is the EXPLICIT destructive operation a group member field can only ever be removed by. + * `apply` never deletes one — a field dropped from config produces no desired key at all, so the + * diff engine is structurally unable to propose it (see engine/synthetic.ts) — and `ct destroy + * --target` addresses whole managed resources, which a member field is not: it has no state entry + * of its own because it belongs to exactly one group. + * + * Guardrails are the group's: the owning group must be managed, and `preventDestroy` on it blocks + * its fields too — protecting a group protects what it owns. + */ +export function resolveMemberFieldTargets(state: State, raw: string[]): MemberFieldTarget[] { + const out: MemberFieldTarget[] = []; + for (const identity of parseTargets(raw)) { + const parsed = parseMemberFieldIdentity(identity); + if (!parsed) { + throw new Error( + `"${identity}" is not a group member field identity. Use "::" ` + + `(e.g. "ojbp_2026_27_praktikum_1::wahl").`, + ); + } + const managed = state.resources[parsed.group]; + if (!managed || managed.type !== "group") { + throw new Error( + `"${identity}": no managed group "${parsed.group}" in the state file. A member field can only ` + + `be destroyed through the group that owns it.`, + ); + } + if (managed.preventDestroy) { + throw new Error( + `preventDestroy is set (in state) for group "${parsed.group}", which owns "${identity}". ` + + `Clear the protection first — protecting a group protects its member fields too.`, + ); + } + out.push({ + identity, + groupKey: parsed.group, + fieldKey: parsed.field, + groupId: managed.id, + }); + } + return out; +} + +/** + * Delete each resolved member field, dropping its id from the owning group's state entry and saving + * after every success. A field that is already gone in ChurchTools (no live row, or a 404 on the + * DELETE) is success-with-note, mirroring `runDeleteLoop`; any other error stops the run with state + * saved up to that point. + */ +export async function runMemberFieldDeleteLoop(ctx: { + client: Pick; + state: State; + statePath: string; + targets: MemberFieldTarget[]; + save?: (path: string, state: State) => Promise; +}): Promise { + const { client, state, statePath, targets } = ctx; + const save = ctx.save ?? saveState; + for (const target of targets) { + const forget = async (): Promise => { + const managed = state.resources[target.groupKey]; + if (managed?.memberFields && target.fieldKey in managed.memberFields) { + const rest = { ...managed.memberFields }; + delete rest[target.fieldKey]; + if (Object.keys(rest).length > 0) managed.memberFields = rest; + else delete managed.memberFields; + } + await save(statePath, state); + }; + let fieldId: number | undefined; + try { + const rows = groupScopedRows(await client.get(memberFieldsReadPath(target.groupId))); + const matches = rows.filter((row) => matchesLocalKey(row, target.fieldKey)); + if (matches.length > 1) { + error( + `${target.identity}: ${matches.length} member fields on group #${target.groupId} answer to ` + + `"${target.fieldKey}" — refusing to guess which one to delete. Rename one in ChurchTools first.`, + ); + process.exitCode = 1; + continue; + } + fieldId = matches.length === 1 ? memberFieldRowId(matches[0]!) : undefined; + } catch (err) { + error(`Stopped at ${target.identity}: ${formatError(err)}. Nothing further was deleted.`); + process.exitCode = 1; + return; + } + if (fieldId === undefined) { + await forget(); + success(`${target.identity} already absent in ChurchTools — nothing to delete`); + continue; + } + const path = memberFieldItemPath(target.groupId, fieldId); + assertNotPeople(path); + try { + await client.request("DELETE", path); + } catch (err) { + if (err instanceof CtApiError && err.status === 404) { + await forget(); + success(`${target.identity} (#${fieldId}) already deleted in ChurchTools`); + continue; + } + error( + `Stopped at ${target.identity}: ${formatError(err)}. State saved up to this point — re-run to resume.`, + ); + process.exitCode = 1; + return; + } + await forget(); + success(`Destroyed group member field ${target.identity} (#${fieldId})`); + } +} + export function destroyWarnings(state: State, keys: string[]): string[] { const out: string[] = []; for (const key of keys) { @@ -118,7 +252,12 @@ export function destroyWarnings(state: State, keys: string[]): string[] { export function destroyCommand(): Command { return new Command("destroy") .description("Explicitly delete managed resources (protected; never implicit)") - .requiredOption("--target ", "logical key(s) to destroy (repeatable or comma-separated)") + .option("--target ", "logical key(s) to destroy (repeatable or comma-separated)") + .option( + "--member-field ", + "group member field(s) to destroy, by portable identity :: (#135) — the " + + "ONLY way one is ever deleted; apply never removes a field that vanished from config", + ) .option("-s, --state ", "state file (or set CT_STATE)") .option("-e, --env ", "environment profile from ct.envs.json (host + state + token)") .option("--confirm-env ", "confirm a protected env non-interactively (must match --env exactly)") @@ -129,8 +268,9 @@ export function destroyCommand(): Command { ) .action(async (opts: DestroyOptions) => { const targets = parseTargets(opts.target ?? []); - if (targets.length === 0) { - throw new Error("No --target given. Destroy never deletes implicitly."); + const memberFieldArgs = parseTargets(opts.memberField ?? []); + if (targets.length === 0 && memberFieldArgs.length === 0) { + throw new Error("No --target or --member-field given. Destroy never deletes implicitly."); } const cmdEnv = await prepareEnv(opts); @@ -157,6 +297,10 @@ export function destroyCommand(): Command { ); } + // Member fields (#135) are resolved against state BEFORE any network call, so a malformed + // identity, an unmanaged group or a preventDestroy'd owner stops the run without touching CT. + const memberFieldTargets = resolveMemberFieldTargets(state, memberFieldArgs); + const { client } = await authedSession(); const parentEdges = await fetchParentEdges(client, state, targets); @@ -177,10 +321,26 @@ export function destroyCommand(): Command { process.exitCode = 1; return; } + // Member-field definitions go into the SAME backup, under their portable identity, so an + // explicit teardown of one is as recoverable as any managed resource. + for (const target of memberFieldTargets) { + try { + const rows = groupScopedRows(await client.get(memberFieldsReadPath(target.groupId))); + const match = rows.find((row) => matchesLocalKey(row, target.fieldKey)); + if (match) actual.set(target.identity, match); + } catch (err) { + error( + `Backup fetch failed for ${target.identity}: ${formatError(err)}. Nothing was deleted — ` + + `resolve the error and re-run.`, + ); + process.exitCode = 1; + return; + } + } const backupPath = await writeBackup(resolveBackupDir(opts.backupDir, statePath), config.host, actual); info(`Backup written: ${backupPath}`); - warn(`About to DELETE: ${ordered.join(", ")}`); + warn(`About to DELETE: ${[...ordered, ...memberFieldTargets.map((t) => t.identity)].join(", ")}`); // Type-level risk (see `destroyWarnings`): surfaced before the prompt, and it takes `--force` // away for this run so the delete cannot go through unattended. const risky = destroyWarnings(state, ordered); @@ -192,7 +352,12 @@ export function destroyCommand(): Command { } // Protected env (#22): typed confirmation of the env NAME is mandatory and --force does NOT bypass // it (--confirm-env substitutes in CI). Otherwise the usual per-target typed confirmation. - const expected = targets.length === 1 ? targets[0]! : "destroy"; + const expected = + targets.length === 1 && memberFieldTargets.length === 0 + ? targets[0]! + : targets.length === 0 && memberFieldTargets.length === 1 + ? memberFieldTargets[0]!.identity + : "destroy"; const ok = cmdEnv.protected ? await confirmEnv(cmdEnv.name!, { confirmFlag: opts.confirmEnv }) : await confirmTyped(expected, { force: opts.force && risky.length === 0 }); @@ -206,7 +371,14 @@ export function destroyCommand(): Command { return; } - await runDeleteLoop({ client, state, statePath, ordered }); + // Member fields FIRST: they are owned by their group, so deleting the group would take them + // with it and the explicit per-field record would be lost. + if (memberFieldTargets.length > 0) { + await runMemberFieldDeleteLoop({ client, state, statePath, targets: memberFieldTargets }); + } + if (ordered.length > 0) { + await runDeleteLoop({ client, state, statePath, ordered }); + } }); } diff --git a/src/commands/state.ts b/src/commands/state.ts index 6f1c7d5..7683511 100644 --- a/src/commands/state.ts +++ b/src/commands/state.ts @@ -138,6 +138,9 @@ async function declaredKeys(configOpt: string | undefined): Promise const addRef = (r: Ref): void => { if (r.kind === "group-role") keys.add(r.group); else if (r.kind === "group-type-role") keys.add(r.groupType); + // A group-scoped member field (#135) is owned by a group, so the key it keeps alive is that + // group's — the field itself has no state entry of its own. + else if (r.kind === "group-member-field") keys.add(r.group); else keys.add(r.key); }; for (const ref of collectRefs(permissions)) addRef(ref); diff --git a/src/config/context.ts b/src/config/context.ts index ae15873..c8a32c8 100644 --- a/src/config/context.ts +++ b/src/config/context.ts @@ -13,7 +13,13 @@ */ import { basename } from "node:path"; import { fileURLToPath } from "node:url"; -import type { DesiredResource, DynamicSpec, DynamicStatus } from "../engine/types.js"; +import type { DesiredResource, DynamicSpec, DynamicStatus, MemberFieldSpec } from "../engine/types.js"; +import { + isManagedMemberFieldProp, + MEMBER_FIELD_FORBIDDEN_PROPS, + MEMBER_FIELD_PROPS, +} from "../engine/member-fields.js"; +import { collectRefs } from "../resolve/refs.js"; import type { DomainType } from "../permissions/grants.js"; import type { DesiredPermission, Grant, PreserveUnknown } from "../permissions/types.js"; import { KNOWN_SCOPE_FIELDS } from "../permissions/catalog.js"; @@ -87,6 +93,16 @@ function relocate(err: unknown, location: string | undefined): unknown { */ export type DynamicInput = boolean | string | { status: DynamicStatus; ruleset: unknown }; +/** + * One group-scoped member field in a `ct.group({ memberFields: [...] })` declaration (#135). + * `key` is the group-local key; every other property is a ChurchTools member-field property + * (see {@link MEMBER_FIELD_PROPS}) and is diffed and written as one unit. + */ +export interface MemberFieldInput { + key: string; + [prop: string]: unknown; +} + export interface ResourceInput { key: string; /** Ordering hint: apply this resource after `parent`. A dependency edge only — NOT managed hierarchy. */ @@ -99,6 +115,26 @@ export interface ResourceInput { * in the same config. */ parents?: string[]; + /** + * Group-scoped member-field DEFINITIONS this group owns (#135). Opt-in, exactly like `parents` + * and `dynamic`: omit to leave the group's member fields unmanaged. + * + * ```ts + * ct.group({ + * key: "ojbp_2026_27_praktikum_1", + * name: "OJBP 1. Praktikum 26/27", + * memberFields: [{ key: "wahl", name: "Wahl", fieldTypeCode: "text" }], + * }); + * ``` + * + * `key` is the LOCAL key — the field's portable identity is the group key plus this one + * (`ojbp_2026_27_praktikum_1::wahl`), because a member field belongs to exactly one group and is + * not globally reusable. Two groups declaring `wahl` stay independent fields with different + * ChurchTools ids. A declaration may never carry a ChurchTools field id. + * + * A field dropped from this list is NEVER deleted — see `ct destroy --member-field`. + */ + memberFields?: MemberFieldInput[]; dependsOn?: string[]; /** * Block `ct destroy` for this resource. Mirrored to the state entry on `apply`, @@ -350,6 +386,78 @@ function desugarDynamic(type: string, key: string, dynamic: unknown): DynamicSpe return { status: d.status as DynamicStatus, ruleset: d.ruleset }; } +/** + * Validate and normalise a group's `memberFields` declaration (#135) into {@link MemberFieldSpec}s. + * + * `undefined` passes straight through (opt-in: member fields are not managed for this group). Every + * failure is an EVAL-time error rather than a warning, because each one is a portability or + * identity defect that would otherwise only surface as a wrong write: + * - a declared ChurchTools field id would freeze this host's numbering into a config that is meant + * to stand up any host — the single guarantee the whole tool rests on; + * - a duplicate local key would silently collapse two fields into one pseudo-field; + * - a missing `name` would reach CT as an HTTP 400 halfway through an unattended apply. + * A property outside the managed set only WARNS and still passes through — the same escape hatch + * unrecognised resource fields have. + */ +function normalizeMemberFields( + type: string, + key: string, + input: unknown, + location?: string, +): MemberFieldSpec[] | undefined { + if (input === undefined) return undefined; + if (type !== "group") throw new Error(`${type} "${key}": "memberFields" is only valid on a group.`); + if (!Array.isArray(input)) { + throw new Error(`group "${key}": "memberFields" must be an array of { key, name, ... } objects.`); + } + const specs: MemberFieldSpec[] = []; + const seen = new Set(); + for (const raw of input) { + if (raw === null || typeof raw !== "object" || Array.isArray(raw)) { + throw new Error(`group "${key}": each entry of "memberFields" must be an object with a "key".`); + } + const { key: localKey, ...props } = raw as MemberFieldInput; + if (typeof localKey !== "string" || localKey.length === 0) { + throw new Error(`group "${key}": each entry of "memberFields" needs a non-empty string "key".`); + } + if (seen.has(localKey)) { + throw new Error( + `group "${key}": duplicate member field key "${localKey}". Local keys must be unique within ` + + `a group — they are half of the portable "${key}::${localKey}" identity.`, + ); + } + seen.add(localKey); + for (const forbidden of MEMBER_FIELD_FORBIDDEN_PROPS) { + if (props[forbidden] !== undefined) { + throw new Error( + `group "${key}" member field "${localKey}": "${forbidden}" must never appear in config. A ` + + `group member field is identified by "${key}::${localKey}", not by a ChurchTools id — ` + + `declaring one would pin this config to a single host.`, + ); + } + } + if (typeof props.name !== "string" || props.name.length === 0) { + throw new Error( + `group "${key}" member field "${localKey}": a non-empty "name" is required (ChurchTools ` + + `rejects a member field without one).`, + ); + } + for (const prop of Object.keys(props)) { + if (!isManagedMemberFieldProp(prop)) { + warn( + located( + location, + `group "${key}" member field "${localKey}": unknown property "${prop}" (sent as-is, never ` + + `diffed against ChurchTools; managed properties: ${MEMBER_FIELD_PROPS.join(", ")})`, + ), + ); + } + } + specs.push({ key: localKey, props }); + } + return specs; +} + function toDesired(type: string, input: ResourceInput, location?: string): DesiredResource { const { key, @@ -358,6 +466,7 @@ function toDesired(type: string, input: ResourceInput, location?: string): Desir dependsOn = [], preventDestroy, dynamic, + memberFields, allowDuplicateName, ...fields } = input; @@ -468,6 +577,9 @@ function toDesired(type: string, input: ResourceInput, location?: string): Desir // `dynamic` is a synthetic field for auto-groups, handled separately from the plain diffed // field bag. Opt-in: `undefined` means "not a dynamic group" (mirrors `parents`). const dynamicSpec = desugarDynamic(type, key, dynamic); + // Group-scoped member fields (#135) — likewise synthetic, likewise opt-in, and destructured out + // above so they never reach the plain field bag (and never trip the unknown-field warning). + const memberFieldSpecs = normalizeMemberFields(type, key, memberFields, location); // `parent` is an ordering hint only — a dependency edge, never a diffed/managed field // (its pre-hierarchy meaning; a `parent` may point at a campus). Group hierarchy is // managed opt-in via `parents`: `undefined` → unmanaged, `[]` → managed with no parents. @@ -483,6 +595,7 @@ function toDesired(type: string, input: ResourceInput, location?: string): Desir parent: parentKey, parents: parentKeys, dynamic: dynamicSpec, + memberFields: memberFieldSpecs, dependsOn: edges, preventDestroy, allowDuplicateName, @@ -513,6 +626,52 @@ function validateReferences(resources: DesiredResource[]): void { } } } + validateMemberFieldRefs(resources, byKey); +} + +/** + * Every `ref.groupMemberField(group, field)` a config emits must name a group DECLARED in this + * config that DECLARES that member field (#135). + * + * This is the plan-time ruleset check the issue asks for, pulled all the way forward to config + * evaluation: a dynamic ruleset naming a field that is not declared for its target group fails here, + * offline, before any network call — never as a mid-apply surprise. ChurchTools treats a ruleset as + * opaque JSON and validates none of the ids inside it, so without this the mistake would apply + * cleanly and simply compute the wrong membership. + * + * Deliberately limited to groups this config OWNS. A ref into a group that is merely adopted (in + * state, not declared) cannot be checked offline — the resolver checks it against the live + * `GET /groups/{id}/memberfields` instead, and hard-errors there, still before apply writes. + */ +function validateMemberFieldRefs(resources: DesiredResource[], byKey: Map): void { + for (const r of resources) { + // The ruleset lives on the synthetic `dynamic` spec, id fields on the plain bag — walk both. + for (const ref of collectRefs([r.fields, r.dynamic?.ruleset])) { + if (ref.kind !== "group-member-field") continue; + const target = byKey.get(ref.group); + if (!target) continue; // adopted-but-not-declared: the resolver checks it live (see above) + if (target.type !== "group") { + throw new Error( + `"${r.key}" references member field "${ref.group}::${ref.field}", but "${ref.group}" is a ` + + `${target.type}, not a group.`, + ); + } + if (target.memberFields === undefined) { + throw new Error( + `"${r.key}" references member field "${ref.group}::${ref.field}", but group "${ref.group}" ` + + `does not manage member fields. Add a "memberFields" list declaring "${ref.field}" to it.`, + ); + } + if (!target.memberFields.some((f) => f.key === ref.field)) { + const declared = target.memberFields.map((f) => f.key).join(", "); + throw new Error( + `"${r.key}" references member field "${ref.group}::${ref.field}", which group "${ref.group}" ` + + `does not declare${declared ? ` (it declares: ${declared})` : ""}. A member field belongs ` + + `to exactly one group, so it can only be named through the group that owns it.`, + ); + } + } + } } export function createContext(): { diff --git a/src/engine/execute.ts b/src/engine/execute.ts index 7cdecea..7a807ef 100644 --- a/src/engine/execute.ts +++ b/src/engine/execute.ts @@ -93,15 +93,28 @@ function snapshotFromChanges(base: Record, changes: FieldChange return snap; } +/** + * Apply every synthetic sub-resource field of one item, IN CHANGE ORDER. + * + * Each change's pending references are re-resolved immediately BEFORE that change is applied, not + * once up front (#135). That matters because synthetic fields on one resource can depend on each + * other: a group's member fields are created here, and the same group's dynamic ruleset — the next + * change in the same loop — may name one of them by its portable `::` identity. Up-front + * re-resolution would look for an id that the very next statement is about to mint. The write BODY is + * still built from the up-front re-resolved changes in `executePlan`; only synthetic writes are late-bound. + */ async function applySyntheticFields( client: Pick, state: State, id: number, + key: string, changes: FieldChange[], ): Promise { for (const c of changes) { const f = syntheticField(c.field); - if (f) await f.apply({ client, state, id, change: c }); + if (!f) continue; + const change = hasPendingRef(c.to) ? { ...c, to: reresolvePendingValue(c.to, state) } : c; + await f.apply({ client, state, id, key, change }); } } @@ -112,8 +125,17 @@ export async function executePlan(plan: Plan, deps: ExecuteDeps): Promise - changes.map((c) => (hasPendingRef(c.to) ? { ...c, to: reresolvePendingValue(c.to, state) } : c)); + changes.map((c) => + !isSyntheticField(c.field) && hasPendingRef(c.to) + ? { ...c, to: reresolvePendingValue(c.to, state) } + : c, + ); const now = deps.now ?? (() => new Date().toISOString()); const save = deps.save ?? saveState; const created: string[] = []; @@ -190,7 +212,12 @@ export async function executePlan(plan: Plan, deps: ExecuteDeps): Promise::` (#135). */ +export function memberFieldIdentity(groupKey: string, localKey: string): string { + return `${groupKey}::${localKey}`; +} + +/** + * Split a `::` identity. Returns `undefined` unless both halves are non-empty, + * so a caller (e.g. `ct destroy --member-field`) can reject a malformed target with its own message. + */ +export function parseMemberFieldIdentity(raw: string): { group: string; field: string } | undefined { + const at = raw.indexOf("::"); + if (at <= 0) return undefined; + const group = raw.slice(0, at).trim(); + const field = raw.slice(at + 2).trim(); + if (!group || !field) return undefined; + return { group, field }; +} + +/** + * The properties this tool MANAGES on a member field — the ones #135 verified as readable AND + * writable, minus the two that must never be managed: + * + * - `id` — host-specific, and the entire portability guarantee is that it never appears in config. + * - `referenceName` — this IS the local identity (see {@link matchesLocalKey}), not a diffable + * property. Managing it would let a plain rename silently re-key the resource, and the field + * would then be re-created rather than updated on the next apply. + * + * A declared property outside this list still passes through to ChurchTools untouched (the escape + * hatch, mirroring the registry's unknown-field behaviour) — it only earns a warning. + */ +export const MEMBER_FIELD_PROPS = [ + "name", + "fieldTypeCode", + "defaultValue", + "options", + "nameInSignupForm", + "note", + "noteInSignupForm", + "requiredInRegistrationForm", + "useInRegistrationForm", + "securityLevel", + "sortKey", +] as const; + +const MANAGED_PROPS = new Set(MEMBER_FIELD_PROPS); + +/** Properties a declaration may never carry — see {@link MEMBER_FIELD_PROPS} for why. */ +export const MEMBER_FIELD_FORBIDDEN_PROPS = ["id", "groupMemberFieldId", "groupId"] as const; + +export function isManagedMemberFieldProp(name: string): boolean { + return MANAGED_PROPS.has(name); +} + +/** One declared member field: its local key plus the property bag that gets diffed and written. */ +export interface MemberFieldSpec { + /** Local key, unique within the owning group. Half of the portable `::` identity. */ + key: string; + /** The declared properties — never an id, never `referenceName`. Diffed as one unit. */ + props: Record; +} + +/** A row as ChurchTools returns it from `GET /groups/{groupId}/memberfields`. */ +export type MemberFieldRow = Record; + +/** + * Is this live row a GROUP-SCOPED custom member field — i.e. one of the rows the + * `/memberfields/group` write endpoints own? + * + * `GET /groups/{groupId}/memberfields` answers with every field the group's signup/member form + * shows, which on a real instance mixes group-scoped custom fields with rows sourced elsewhere + * (person master data, group-type defaults). Only the first kind can be created or updated through + * `/memberfields/group`, so only the first kind is adoptable or manageable here. + * + * The discriminator is read leniently, because CT has spelled this differently across versions: + * a `type`/`fieldCategory`/`source` of `"group"` marks a group-scoped row. A row that carries NO + * discriminator at all is treated as group-scoped — the endpoint is the group's own, so "we cannot + * tell" errs towards showing the field rather than silently hiding one an author needs. + */ +export function isGroupScopedMemberField(row: MemberFieldRow): boolean { + for (const name of ["type", "fieldCategory", "source", "fieldSource"]) { + const value = row[name]; + if (typeof value === "string") return value.toLowerCase() === "group"; + } + return true; +} + +/** + * The local key a live row answers to. `referenceName` is CT's own stable, non-numeric handle + * within the group and is what a create sends, so it wins; `name` is the fallback for a row created + * in the ChurchTools UI, where CT may mint its own referenceName. Slugged on both sides so a key + * derived from a German name (`Wahl` → `wahl`) matches either spelling. + */ +export function localKeyOf(row: MemberFieldRow): string { + const reference = row.referenceName; + if (typeof reference === "string" && reference.length > 0) return slug(reference); + const name = row.name; + return typeof name === "string" ? slug(name) : ""; +} + +/** Does this live row carry the declared local key? (See {@link localKeyOf}.) */ +export function matchesLocalKey(row: MemberFieldRow, localKey: string): boolean { + const wanted = slug(localKey); + if (localKeyOf(row) === wanted) return true; + const name = row.name; + return typeof name === "string" && slug(name) === wanted; +} + +/** The numeric ChurchTools id of a live row, or `undefined` when it carries none. */ +export function memberFieldRowId(row: MemberFieldRow): number | undefined { + for (const name of ["id", "groupMemberFieldId"]) { + const value = row[name]; + if (typeof value === "number" && Number.isFinite(value)) return value; + } + return undefined; +} + +/** + * The ACTUAL side of one member field's diff: the live row narrowed to exactly the properties the + * declaration names. + * + * Narrowing is what makes a clean apply round-trip to a no-op. The pseudo-field's value is one + * object compared with `deepEqual`, so an unmanaged sibling property CT happens to return (a + * server-defaulted `sortKey`, a `nameTranslated`, an echoed `groupId`) would otherwise make the two + * sides structurally unequal forever — a plan that proposes the same update on every run. Declared + * keys only, one level deeper than `diffFields` does at the resource level, with the same meaning: + * what the config does not mention is not managed. + */ +export function actualMemberFieldProps( + row: MemberFieldRow, + declaredProps: readonly string[], +): Record { + const out: Record = {}; + for (const name of declaredProps) out[name] = row[name]; + return out; +} + +/** `GET`/`POST` path for a group's group-scoped member fields. */ +export function memberFieldsReadPath(groupId: number): string { + return `/groups/${groupId}/memberfields`; +} + +export function memberFieldsCreatePath(groupId: number): string { + return `/groups/${groupId}/memberfields/group`; +} + +export function memberFieldItemPath(groupId: number, fieldId: number): string { + return `/groups/${groupId}/memberfields/group/${fieldId}`; +} + +/** Normalise CT's list envelope (bare array or `{ data: [...] }`) to the group-scoped rows only. */ +export function groupScopedRows(raw: unknown): MemberFieldRow[] { + const list = Array.isArray(raw) + ? raw + : Array.isArray((raw as { data?: unknown } | null)?.data) + ? ((raw as { data: unknown[] }).data as unknown[]) + : []; + return list + .filter((row): row is MemberFieldRow => row !== null && typeof row === "object" && !Array.isArray(row)) + .filter(isGroupScopedMemberField); +} diff --git a/src/engine/synthetic.ts b/src/engine/synthetic.ts index 6919163..dbc46ce 100644 --- a/src/engine/synthetic.ts +++ b/src/engine/synthetic.ts @@ -16,6 +16,22 @@ import { mapConcurrent } from "../util/concurrency.js"; import { info, warn, formatError } from "../ui.js"; import { normalizeDynamic, normalizeRuleset, putRulesetBody, resolveRulesetRef } from "./dynamic.js"; import { formatPortablizeWarnings, scanUnportablized } from "../config/query-refs.js"; +import { slug } from "../resources/registry.js"; +import { + actualMemberFieldProps, + groupScopedRows, + localKeyOf, + matchesLocalKey, + memberFieldIdentity, + memberFieldItemPath, + memberFieldLocalKey, + memberFieldPseudo, + memberFieldRowId, + memberFieldsCreatePath, + memberFieldsReadPath, + MEMBER_FIELD_PREFIX, + type MemberFieldRow, +} from "./member-fields.js"; import type { DynamicStatus } from "./types.js"; /** How many dynamic groups to fetch (ruleset + status) from ChurchTools at once. Mirrors build.ts. */ @@ -40,6 +56,12 @@ export interface SyntheticApplyCtx { client: Pick; state: State; id: number; + /** + * The owning resource's LOGICAL key. Needed by any field whose write produces state a later + * change has to read back — today the group-scoped member fields (#135), which record each + * created field's id on the group's state entry so a same-run ruleset reference can resolve. + */ + key: string; change: FieldChange; } export interface SyntheticPostApplyCtx { @@ -67,6 +89,14 @@ export interface SyntheticFoldResult { export interface SyntheticField { field: string; + /** + * Marks this entry as owning a FAMILY of pseudo-fields sharing a prefix, rather than one fixed + * name (#135). Group-scoped member fields need it: one declared field folds into one pseudo-field + * (`memberField:wahl`), so the plan diffs and renders per field instead of as one opaque array — + * and, crucially, a field dropped from config simply has no desired key, which is what makes + * `diffFields` (desired-driven) structurally incapable of turning a removal into a delete. + */ + prefix?: string; fold(ctx: SyntheticFoldCtx): Promise; apply(ctx: SyntheticApplyCtx): Promise; /** @@ -128,6 +158,179 @@ const parentsField: SyntheticField = { }, }; +/** How many groups' member-field lists to read from ChurchTools at once. Mirrors the dynamic fold. */ +const MEMBER_FIELD_FETCH_CONCURRENCY = 8; + +/** Read one group's group-scoped member fields through a read-only client. */ +async function readMemberFields(client: Pick, groupId: number): Promise { + return groupScopedRows(await client.get(memberFieldsReadPath(groupId))); +} + +/** Read them through the write client (apply only has `request`; `GET` goes through it just fine). */ +async function readMemberFieldsForWrite( + client: Pick, + groupId: number, +): Promise { + return groupScopedRows(await client.request("GET", memberFieldsReadPath(groupId))); +} + +/** + * `memberField:`: group-scoped member-field DEFINITIONS (#135), one pseudo-field per + * declared field on the owning group. + * + * Three properties of this shape are load-bearing rather than incidental: + * + * 1. **Never deletes.** `diffFields` walks the DESIRED side, so a field dropped from config + * produces no key, no change and no write — it is structurally impossible for a removal to + * become a delete here. The live field is still surfaced as a DELETE CANDIDATE (a warning + * naming its portable identity and the explicit `ct destroy --member-field` that removes it), + * so "left alone" never means "unnoticed". + * 2. **Round-trips to a no-op.** The actual side is narrowed to exactly the properties the + * declaration names (`actualMemberFieldProps`), so a server-defaulted sibling CT returns can + * never make the two sides unequal forever. + * 3. **Ordered before `dynamic`.** This entry is registered ahead of the dynamic field, so the + * folded `memberField:*` keys land in the desired bag before `dynamic`, `diffFields` emits them + * in that order, and `applySyntheticFields` therefore creates the fields before installing a + * ruleset that may reference them. + */ +const memberFieldsField: SyntheticField = { + field: MEMBER_FIELD_PREFIX, + prefix: MEMBER_FIELD_PREFIX, + async fold({ client, state, desired, actual }) { + const optedIn = desired.filter((d) => d.type === "group" && d.memberFields !== undefined); + if (optedIn.length === 0) return { desired, errors: [] }; + + // Only groups that are ALREADY managed and were fetched have an actual side to read. A group + // this run creates has neither — its fields are created by its own apply item — and a vanished + // one is handled as a recreate by the plain plan. + const readable = optedIn.filter((d) => { + const managed = state.resources[d.key]; + return managed?.type === "group" && actual.get(d.key) !== undefined; + }); + const outcomes = await mapConcurrent(readable, MEMBER_FIELD_FETCH_CONCURRENCY, async (d) => { + const managed = state.resources[d.key]!; + try { + return { key: d.key, rows: await readMemberFields(client, managed.id), errors: [] as string[] }; + } catch (err) { + // Same honesty rule as the dynamic fold (#126): an unread actual is NOT a known-absent one, + // so the desired side stays unfolded and the group is reported unreadable rather than having + // "create every member field" manufactured out of a transient 429. + return { + key: d.key, + rows: undefined, + errors: [`member fields ${d.key} (#${managed.id}): ${formatError(err)}`], + }; + } + }); + const errors = outcomes.flatMap((o) => o.errors); + const unreadable = outcomes.filter((o) => o.rows === undefined).map((o) => o.key); + const unreadableKeys = new Set(unreadable); + const rowsByKey = new Map(outcomes.filter((o) => o.rows !== undefined).map((o) => [o.key, o.rows!])); + + const augmented = desired.map((d) => { + if (d.type !== "group" || d.memberFields === undefined) return d; + if (unreadableKeys.has(d.key)) return d; + const rows = rowsByKey.get(d.key); + const a = actual.get(d.key); + const fields = { ...d.fields }; + for (const spec of d.memberFields) { + const pseudo = memberFieldPseudo(spec.key); + fields[pseudo] = spec.props; + if (!a || !rows) continue; + const matches = rows.filter((row) => matchesLocalKey(row, spec.key)); + // >1 live match means the local key is ambiguous on this host — a blind update would pick + // one arbitrarily, so leave the actual side absent and let the ambiguity surface where it + // can be acted on (the apply path refuses it by name). + if (matches.length === 1) { + a[pseudo] = actualMemberFieldProps(matches[0]!, Object.keys(spec.props)); + } + } + if (rows) { + const declared = new Set(d.memberFields.map((f) => slug(f.key))); + for (const row of rows) { + const local = localKeyOf(row); + if (!local || declared.has(local)) continue; + warn( + `group "${d.key}": member field "${memberFieldIdentity(d.key, local)}" exists in ` + + `ChurchTools but is not declared — DELETE CANDIDATE, left untouched. ct never removes a ` + + `member field because it vanished from config; remove it explicitly with ` + + `\`ct destroy --member-field ${memberFieldIdentity(d.key, local)}\`.`, + ); + } + } + return { ...d, fields }; + }); + return { desired: augmented, errors, unreadable }; + }, + async apply({ client, state, id, key, change }) { + const local = memberFieldLocalKey(change.field); + if (local === undefined) return; + const props = change.to as Record | undefined; + // No desired value = the field is not declared. `diffFields` cannot even produce such a change + // (it walks the desired side), so this is belt-and-braces: apply never deletes a member field. + if (props === undefined || props === null) return; + + const rows = await readMemberFieldsForWrite(client, id); + const matches = rows.filter((row) => matchesLocalKey(row, local)); + if (matches.length > 1) { + throw new Error( + `group member field "${memberFieldIdentity(key, local)}": ${matches.length} fields on group ` + + `#${id} answer to the local key "${local}". Rename one in ChurchTools so the key is unique ` + + `within the group, then re-apply.`, + ); + } + + const record = (fieldId: number): void => { + // Record the per-host id on the OWNING GROUP's state entry, so a same-run `::` + // reference in a ruleset applied later in this very item can be completed (#135). + const managed = state.resources[key]; + if (!managed) return; + managed.memberFields = { ...managed.memberFields, [local]: fieldId }; + }; + + if (matches.length === 1) { + const fieldId = memberFieldRowId(matches[0]!); + if (fieldId === undefined) { + throw new Error( + `group member field "${memberFieldIdentity(key, local)}": the matching ChurchTools row ` + + `carries no numeric id, so it cannot be updated.`, + ); + } + const path = memberFieldItemPath(id, fieldId); + assertNotPeople(path); + // PATCH is a partial update, so unmanaged sibling properties are left alone (the same reason + // groups are PATCHed). Instances whose member-field endpoint only implements PUT answer 405/501 + // — fall back rather than fail an apply over a verb. + try { + await client.request("PATCH", path, props); + } catch (err) { + if (!(err instanceof CtApiError && (err.status === 405 || err.status === 501))) throw err; + await client.request("PUT", path, props); + } + record(fieldId); + return; + } + + const createPath = memberFieldsCreatePath(id); + assertNotPeople(createPath); + // `referenceName` is the local key, not a managed property: it is CT's own stable, non-numeric + // handle within the group, and sending it at create is what lets every later run find this + // field by its portable identity instead of by a host-specific id. + const created = await client.request<{ id?: number } | undefined>("POST", createPath, { + ...props, + referenceName: local, + }); + const newId = created?.id; + if (typeof newId !== "number") { + throw new Error( + `group member field "${memberFieldIdentity(key, local)}": create returned no numeric id ` + + `(got ${JSON.stringify(created)}).`, + ); + } + record(newId); + }, +}; + /** `dynamic`: dynamic-group ruleset + status, reconciled as one synthetic field. */ const dynamicField: SyntheticField = { field: "dynamic", @@ -143,7 +346,14 @@ const dynamicField: SyntheticField = { if (!a) return []; // vanished from CT → handled as a recreate by the plain plan return [{ managed, a }]; }); - if (targets.length === 0) return { desired, errors: [] }; + // Gate on the DESIRED opt-in, not on the readable targets (#135, mirroring `parents`). Returning + // early whenever nothing is readable ALSO skipped folding the desired side of a group this run + // CREATES — so a config whose only dynamic group was brand new silently applied no ruleset on + // its first run, while the same config next to an already-adopted dynamic group applied it fine. + // With no readable targets there is simply nothing to fetch; the desired-side fold below still runs. + if (!desired.some((d) => d.type === "group" && d.dynamic !== undefined)) { + return { desired, errors: [] }; + } // Fetch each group's (ruleset, status) concurrently — 2N serial round-trips otherwise dominate // plan/apply latency on a config with many dynamic groups. Within a group the two GETs stay // sequential: the status GET must run only after the ruleset GET succeeds (a 404 there means @@ -270,14 +480,24 @@ const dynamicField: SyntheticField = { }, }; -export const SYNTHETIC_FIELDS: SyntheticField[] = [parentsField, dynamicField]; +/** + * Registration ORDER IS APPLY ORDER (#135). Each fold appends its pseudo-fields to the desired bag, + * `diffFields` walks that bag in insertion order, and `applySyntheticFields` walks the resulting + * changes in order — so member fields are created before the same group's dynamic ruleset is + * installed, which is exactly what lets a ruleset reference a field created in the same run. + */ +export const SYNTHETIC_FIELDS: SyntheticField[] = [parentsField, memberFieldsField, dynamicField]; const BY_FIELD = new Map(SYNTHETIC_FIELDS.map((f) => [f.field, f])); +/** Entries owning a whole prefix family (`memberField:`) rather than one fixed field name. */ +const BY_PREFIX = SYNTHETIC_FIELDS.filter( + (f): f is SyntheticField & { prefix: string } => typeof f.prefix === "string", +); export function isSyntheticField(field: string): boolean { - return BY_FIELD.has(field); + return syntheticField(field) !== undefined; } export function syntheticField(field: string): SyntheticField | undefined { - return BY_FIELD.get(field); + return BY_FIELD.get(field) ?? BY_PREFIX.find((f) => field.startsWith(f.prefix)); } /** diff --git a/src/engine/types.ts b/src/engine/types.ts index 419d8ed..250a14a 100644 --- a/src/engine/types.ts +++ b/src/engine/types.ts @@ -3,6 +3,10 @@ * plan produced by diffing desired vs state vs actual. */ +import type { MemberFieldSpec } from "./member-fields.js"; + +export type { MemberFieldSpec }; + export type DynamicStatus = "active" | "inactive" | "manual" | "none"; export interface DynamicSpec { @@ -22,6 +26,13 @@ export interface DesiredResource { parents?: string[]; /** Auto-group / dynamic-group config. Opt-in: `undefined` = not a dynamic group. Only valid on a group. */ dynamic?: DynamicSpec; + /** + * Group-scoped member-field DEFINITIONS owned by this group (#135). Opt-in, mirroring `parents` + * and `dynamic`: `undefined` = member fields are not managed for this group; `[]` = managed with + * none declared (which still never deletes an existing one — see engine/synthetic.ts). Only valid + * on a group, because a member field belongs to exactly one group and is not globally reusable. + */ + memberFields?: MemberFieldSpec[]; /** Logical keys this resource must be applied after (includes `parent`/`parents`). */ dependsOn: string[]; /** Lifecycle flag: block `ct destroy` for this resource. Never diffed or sent to the API. */ diff --git a/src/resolve/refs.ts b/src/resolve/refs.ts index e94f781..f6c63d9 100644 --- a/src/resolve/refs.ts +++ b/src/resolve/refs.ts @@ -29,7 +29,8 @@ export type RefKind = | "role-def" | "group" | "group-role" - | "group-type-role"; + | "group-type-role" + | "group-member-field"; /** * Shared explanation for why a group-status reference can never be resolved by name (#67): @@ -95,7 +96,28 @@ export interface GroupTypeRoleRef { role: string; } -export type Ref = SimpleRef | GroupRoleRef | GroupTypeRoleRef; +/** + * Compound reference: a GROUP-SCOPED MEMBER FIELD, addressed by its portable + * `(group key, local field key)` pair (#135) — `ojbp_2026_27_praktikum_1::wahl`. + * + * It has to be compound for the same reason {@link GroupTypeRoleRef} does, only more strongly: a + * member field belongs to exactly ONE group and is not globally reusable, so its local key alone + * names nothing. Two groups declaring `wahl` are two independent fields with two different + * ChurchTools ids, and that is the whole point of the blueprint use case — running the same + * function for 25/26 and 26/27 must mint fresh fields per group, with no id from the other year + * participating in resolution. + * + * This is what lets a dynamic-group ruleset name a member field portably instead of freezing this + * host's numeric field id into a file that is then applied somewhere else. + */ +export interface GroupMemberFieldRef { + __ctRef: true; + kind: "group-member-field"; + group: string; + field: string; +} + +export type Ref = SimpleRef | GroupRoleRef | GroupTypeRoleRef | GroupMemberFieldRef; function requireKey(kind: RefKind, value: unknown): string { if (typeof value !== "string" || value.length === 0) { @@ -230,6 +252,19 @@ export const ref = { groupType: requireKey("group-type-role", groupType), role: requireKey("group-type-role", role), }), + /** + * A group-scoped member FIELD (#135), by its portable `(group key, local field key)` pair — the + * `ojbp_2026_27_praktikum_1::wahl` identity. The resolver maps it to this host's numeric field id + * from `GET /groups/{groupId}/memberfields`; a field the same config declares but that does not + * exist on this host yet resolves to a {@link PendingRef} and is completed during apply, after the + * owning group's fields have been created. See {@link GroupMemberFieldRef}. + */ + groupMemberField: (group: string, field: string): GroupMemberFieldRef => ({ + __ctRef: true, + kind: "group-member-field", + group: requireKey("group-member-field", group), + field: requireKey("group-member-field", field), + }), }; export function isRef(value: unknown): value is Ref { @@ -243,6 +278,8 @@ export function refKey(r: Ref): string { return `group-role:${r.group} ${r.role}`; case "group-type-role": return `group-type-role:${r.groupType} ${r.role}`; + case "group-member-field": + return `group-member-field:${r.group}::${r.field}`; default: return `${r.kind}:${r.key}`; } @@ -255,6 +292,10 @@ export function refLabel(r: Ref): string { return `group-role(group=${r.group}, role=${r.role})`; case "group-type-role": return `group-type-role(groupType=${r.groupType}, role=${r.role})`; + case "group-member-field": + // The portable `::` identity itself — the same string the docs, the plan and + // `ct destroy --member-field` all use, so one identity is spoken everywhere (#135). + return `group-member-field:${r.group}::${r.field}`; default: return `${r.kind}:${r.key}`; } @@ -330,9 +371,15 @@ export function collectPendingRefKeys(value: unknown): string[] { const walk = (v: unknown): void => { if (isPendingRef(v)) { const r = v.__pendingRef; + // A pending group-scoped member field (#135) DOES belong here, and its ordering dependency is + // the OWNING GROUP: the field is created by that group's own apply item (before its dynamic + // ruleset — see engine/synthetic.ts), so sequencing the group first is exactly what makes a + // ruleset on another group able to name a field created in the same run. A self-reference (the + // ruleset and the field on the same group) is dropped by buildPlan's `k !== d.key` filter. + if (r.kind === "group-member-field") out.push(r.group); // Only the simple, single-`key` kinds contribute an apply-order dependency here — see the note - // above for why the compound ones cannot reach this walk. - if (r.kind !== "group-role" && r.kind !== "group-type-role") out.push(r.key); + // above for why the other compound ones cannot reach this walk. + else if (r.kind !== "group-role" && r.kind !== "group-type-role") out.push(r.key); return; } if (Array.isArray(v)) { diff --git a/src/resolve/resolver.ts b/src/resolve/resolver.ts index 2c03127..a28bbcb 100644 --- a/src/resolve/resolver.ts +++ b/src/resolve/resolver.ts @@ -29,6 +29,12 @@ import type { CtClient } from "../api/ctClient.js"; import type { State } from "../state/state.js"; import type { DesiredResource } from "../engine/types.js"; import { slug } from "../resources/registry.js"; +import { + groupScopedRows, + matchesLocalKey, + memberFieldRowId, + memberFieldsReadPath, +} from "../engine/member-fields.js"; import { collectRefs, deepMapRefs, @@ -38,6 +44,7 @@ import { pendingRef, refKey, refLabel, + type GroupMemberFieldRef, type GroupRoleRef, type GroupTypeRoleRef, type PendingRef, @@ -201,6 +208,14 @@ export class Resolver { * Ref resolved lazily at check time. */ private readonly declaredRoleDefTypes = new Map(); + /** + * Group key → the slugged LOCAL member-field keys that group declares (#135). A member field is + * group-scoped, so "does this config declare it?" can only be answered per group — the same local + * key on two groups is two independent fields. Drives the pending decision below. + */ + private readonly declaredMemberFields = new Map>(); + /** Per-group member-field list cache, keyed by group id, fetched at most once per run. */ + private readonly memberFieldLists = new Map[]>>(); constructor(deps: ResolverDeps) { this.client = deps.client; @@ -223,6 +238,9 @@ export class Resolver { else this.declaredRoleDefTypes.set(name, [gt]); } } + if (d.type === "group" && d.memberFields !== undefined) { + this.declaredMemberFields.set(d.key, new Set(d.memberFields.map((f) => slug(f.key)))); + } } } @@ -230,6 +248,7 @@ export class Resolver { async resolve(r: Ref, site: string, opts: ResolveOptions = {}): Promise { if (r.kind === "group-role") return this.resolveGroupRole(r, site, opts); if (r.kind === "group-type-role") return this.resolveGroupTypeRole(r, site); + if (r.kind === "group-member-field") return this.resolveGroupMemberField(r, site); // (1) managed desired ∪ state by logical key const type = REF_KIND_TYPE[r.kind]; if (type !== undefined) { @@ -456,6 +475,82 @@ export class Resolver { ); } + /** + * Resolve a group-scoped member field (#135) by its portable `(group, field)` pair to this host's + * numeric field id — the reference a dynamic-group ruleset uses so it never freezes one host's + * field id into a file that is applied elsewhere. + * + * Three outcomes, in order: + * 1. The owning group is managed AND the live `GET /groups/{id}/memberfields` carries the field → + * its numeric id. (>1 match is a hard error: the local key would be ambiguous on this host.) + * 2. The field is DECLARED by this config for that group but does not exist on this host yet → + * a {@link PendingRef}. Fields are created by the owning group's own apply item, before that + * group's dynamic ruleset is installed, so the pending marker is completed from state during + * apply (see `pendingIdFromState` and engine/synthetic.ts). + * 3. Neither → a hard error naming the portable identity. This is the check #135 asks for: a + * ruleset naming a field that does not exist and is not declared FAILS AT PLAN TIME, before + * anything is written. + */ + private async resolveGroupMemberField(r: GroupMemberFieldRef, site: string): Promise { + const declaresField = this.declaredMemberFields.get(r.group)?.has(slug(r.field)) === true; + const managed = this.state.resources[r.group]; + if (!managed || managed.type !== "group") { + if (declaresField) return pendingRef(r); + const declaresGroup = this.declaredByType.get("group")?.has(r.group) === true; + throw new Error( + `Cannot resolve ${refLabel(r)} referenced at ${site} on ${this.host}: ` + + (declaresGroup + ? `group "${r.group}" is declared but does not declare a member field "${r.field}". ` + + `Add it to that group's "memberFields" — a member field belongs to exactly one group, ` + + `so it can only be named through the group that owns it.` + : `no managed group named "${r.group}" is declared or adopted. Declare/adopt it, or fix the key.`), + ); + } + const rows = await this.memberFieldList(managed.id); + const matches = rows.filter((row) => matchesLocalKey(row, r.field)); + if (matches.length > 1) { + const list = matches + .map((row) => `${JSON.stringify(row.name ?? row.referenceName)} (#${String(memberFieldRowId(row))})`) + .join(", "); + throw new Error( + `Ambiguous ${refLabel(r)} referenced at ${site} on ${this.host}: ${matches.length} member ` + + `fields on group #${managed.id} match — ${list}. Rename one in ChurchTools so the local key ` + + `is unique within the group.`, + ); + } + if (matches.length === 1) { + const id = memberFieldRowId(matches[0]!); + if (id === undefined) { + throw new Error( + `Cannot resolve ${refLabel(r)} referenced at ${site} on ${this.host}: the matched member ` + + `field row carries no numeric id.`, + ); + } + return id; + } + if (declaresField) return pendingRef(r); + const available = rows + .map((row) => (typeof row.name === "string" ? JSON.stringify(row.name) : "?")) + .join(", "); + throw new Error( + `Cannot resolve ${refLabel(r)} referenced at ${site} on ${this.host}: group #${managed.id} has ` + + `no member field "${r.field}"${available ? ` (available: ${available})` : ""}, and this config ` + + `does not declare one. Declare it in that group's "memberFields", or fix the key.`, + ); + } + + /** One group's member-field list, fetched at most once per run (several refs may name one group). */ + private memberFieldList(groupId: number): Promise[]> { + let p = this.memberFieldLists.get(groupId); + if (!p) { + p = this.client + .get(memberFieldsReadPath(groupId)) + .then((raw) => groupScopedRows(raw)) as Promise[]>; + this.memberFieldLists.set(groupId, p); + } + return p; + } + private groupRoleList(groupId: number): Promise { let p = this.groupRoleLists.get(groupId); if (!p) { @@ -651,6 +746,23 @@ function pendingIdFromState(r: Ref, state: State): number { `live /groups/{id}/roles fetch, so it can only be completed by applyPermissionPlan (#106).`, ); } + if (r.kind === "group-member-field") { + // Completed from the OWNING GROUP's state entry (#135): the member-field apply records each + // field's freshly minted id under `memberFields` there, and it runs before the same group's + // dynamic ruleset (engine/synthetic.ts orders the pseudo-fields ahead of `dynamic`, and + // `applySyntheticFields` re-resolves each change immediately before applying it). So by the time + // a ruleset carrying this marker is written, the id is already in state. + const group = state.resources[r.group]; + const id = group?.memberFields?.[r.field]; + if (typeof id !== "number") { + throw new Error( + `Pending ${refLabel(r)} did not resolve after its group applied — no member field "${r.field}" ` + + `was created for group "${r.group}" in this run. This usually means the group's create, or ` + + `that field's create, failed earlier.`, + ); + } + return id; + } if (r.kind === "group-type-role") { // A group-type-role ref resolves to a concrete /group/roles id at plan time (the catalog id exists // independently of any same-run apply), so a pending one should never reach apply either (#76). diff --git a/src/state/state.ts b/src/state/state.ts index 105c950..5311b83 100644 --- a/src/state/state.ts +++ b/src/state/state.ts @@ -30,6 +30,17 @@ export interface ManagedResource { * (the exact moment it becomes a destroy candidate). Missing = not protected. */ preventDestroy?: boolean; + /** + * Groups only (#135): local member-field key → this host's ChurchTools member-field id. + * + * NOT a managed-field snapshot — it is never diffed and never written to ChurchTools. It exists + * because a group-scoped member field has no logical key of its own in `resources` (it belongs to + * exactly one group and is not globally reusable), yet a same-run reference to a field created in + * THIS apply — a dynamic ruleset naming `::` — has to be completed from somewhere + * once the field exists. `ct destroy --member-field` reads it too, so an explicit teardown does + * not need a live lookup to find the id it is about to delete. + */ + memberFields?: Record; } export interface State { diff --git a/tests/adopt-group-command.test.ts b/tests/adopt-group-command.test.ts index 45fd446..cc235bb 100644 --- a/tests/adopt-group-command.test.ts +++ b/tests/adopt-group-command.test.ts @@ -72,6 +72,23 @@ function makeClient(childrenResponse: ChildrenResponse = "array") { }, }; const statuses: Record = { 30: "active", 33: "active" }; + // Group-scoped member fields (#135). Group 31 has two custom fields plus a row sourced elsewhere + // (`type: "person"`), which must NOT be adopted — only `/memberfields/group` rows are manageable. + const memberFields: Record>> = { + 31: [ + { + id: 701, + type: "group", + referenceName: "wahl", + name: "Wahl", + fieldTypeCode: "text", + requiredInRegistrationForm: true, + sortKey: 1, + }, + { id: 702, type: "group", referenceName: "notiz", name: "Notiz", fieldTypeCode: "textarea" }, + { id: 703, type: "person", referenceName: "vorname", name: "Vorname", fieldTypeCode: "text" }, + ], + }; const get = vi.fn(async (path: string): Promise => { let m = /^\/groups\/(\d+)$/.exec(path); @@ -99,6 +116,8 @@ function makeClient(childrenResponse: ChildrenResponse = "array") { } return rows; } + m = /^\/groups\/(\d+)\/memberfields$/.exec(path); + if (m) return memberFields[Number(m[1])] ?? []; if (path === "/group/grouptypes") return groupTypes; if (path === "/campuses") return campuses; if (path === "/group/roles") return roles; @@ -535,3 +554,62 @@ describe("ct adopt group — idiomatic snippet round-trips to a no-op (#52 item expect(plan.items.every((i) => i.action === "no-op")).toBe(true); }); }); + +describe("ct adopt group --with-member-fields (#135)", () => { + /** Grab the `--dry-run` payload, which carries the generated config snippet verbatim. */ + async function snippetFor(args: string[]): Promise { + const writes: string[] = []; + const spy = vi.spyOn(process.stdout, "write").mockImplementation((s) => { + writes.push(String(s)); + return true; + }); + try { + await run(args); + } finally { + spy.mockRestore(); + } + const payload = JSON.parse(writes.join("")) as { config?: string } | Array<{ config?: string }>; + return Array.isArray(payload) ? payload.map((p) => p.config).join("\n") : (payload.config ?? ""); + } + + it("is opt-in: a plain adopt emits no memberFields block and never reads the endpoint", async () => { + const snippet = await snippetFor(["group", "31", "--dry-run", "--state", statePath]); + expect(snippet).not.toContain("memberFields"); + expect(client.get).not.toHaveBeenCalledWith("/groups/31/memberfields"); + }); + + it("emits every group-scoped field WITHOUT any ChurchTools id", async () => { + const snippet = await snippetFor([ + "group", + "31", + "--with-member-fields", + "--dry-run", + "--state", + statePath, + ]); + expect(snippet).toContain("memberFields:"); + expect(snippet).toContain('key: "wahl"'); + expect(snippet).toContain('name: "Wahl"'); + expect(snippet).toContain('fieldTypeCode: "text"'); + expect(snippet).toContain("requiredInRegistrationForm: true"); + expect(snippet).toContain('key: "notiz"'); + // The whole portability guarantee: no host-specific field id anywhere in the emitted config. + expect(snippet).not.toContain("701"); + expect(snippet).not.toContain("702"); + expect(snippet).not.toMatch(/\bid:/); + // …and a row that is not group-scoped is not manageable through /memberfields/group. + expect(snippet).not.toContain("vorname"); + }); + + it("emits no memberFields block for a group that has none", async () => { + const snippet = await snippetFor([ + "group", + "10", + "--with-member-fields", + "--dry-run", + "--state", + statePath, + ]); + expect(snippet).not.toContain("memberFields"); + }); +}); diff --git a/tests/destroy-command.test.ts b/tests/destroy-command.test.ts index c919be8..fb29574 100644 --- a/tests/destroy-command.test.ts +++ b/tests/destroy-command.test.ts @@ -138,3 +138,66 @@ describe("ct destroy (command level)", () => { } }); }); + +/** + * The EXPLICIT destructive operation for a group member field (#135). `apply` can never delete one + * — a field dropped from config produces no desired key, so the diff engine is structurally unable + * to propose it — so this command is the only path, and it inherits the group's guardrails. + */ +describe("ct destroy --member-field (#135)", () => { + const rows = [{ id: 701, type: "group", referenceName: "wahl", name: "Wahl", fieldTypeCode: "text" }]; + + beforeEach(() => { + getMock.mockImplementation((async (path: string) => { + if (path === "/groups/hierarchies") return []; + if (path === "/groups/1/memberfields") return rows; + return { name: path }; + }) as never); + }); + + it("deletes the field through its group-scoped path and drops it from state", async () => { + const state = emptyState(HOST); + state.resources.area = group("area", 1, { memberFields: { wahl: 701 } }); + await saveState(statePath, state); + + await runDestroy(["--member-field", "area::wahl", "--state", statePath, "--force"]); + + expect(calls).toEqual([{ method: "DELETE", path: "/groups/1/memberfields/group/701" }]); + const after = await loadState(statePath, HOST); + // The owning GROUP survives — only the field it owns was destroyed. + expect(after.resources.area).toBeDefined(); + expect(after.resources.area!.memberFields).toBeUndefined(); + }); + + it("is refused when the owning group is protected — protecting a group protects its fields", async () => { + const state = emptyState(HOST); + state.resources.area = group("area", 1, { preventDestroy: true }); + await saveState(statePath, state); + + await expect( + runDestroy(["--member-field", "area::wahl", "--state", statePath, "--force"]), + ).rejects.toThrow(/preventDestroy is set \(in state\) for group "area"/); + expect(calls).toEqual([]); + }); + + it("rejects a malformed identity and an unmanaged group before any network call", async () => { + const state = emptyState(HOST); + state.resources.area = group("area", 1); + await saveState(statePath, state); + + await expect(runDestroy(["--member-field", "wahl", "--state", statePath, "--force"])).rejects.toThrow( + /not a group member field identity/, + ); + await expect( + runDestroy(["--member-field", "nope::wahl", "--state", statePath, "--force"]), + ).rejects.toThrow(/no managed group "nope"/); + expect(calls).toEqual([]); + }); + + it("still refuses to delete anything with neither --target nor --member-field", async () => { + await saveState(statePath, emptyState(HOST)); + await expect(runDestroy(["--state", statePath, "--force"])).rejects.toThrow( + /Destroy never deletes implicitly/, + ); + }); +}); diff --git a/tests/member-fields.test.ts b/tests/member-fields.test.ts new file mode 100644 index 0000000..d4b8201 --- /dev/null +++ b/tests/member-fields.test.ts @@ -0,0 +1,463 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { CtApiError, type CtClient } from "../src/api/ctClient.js"; +import { buildPlan } from "../src/engine/build.js"; +import { executePlan } from "../src/engine/execute.js"; +import { renderPlan } from "../src/engine/render.js"; +import { createContext, evaluateConfig, ref } from "../src/config/context.js"; +import { emptyState, type State } from "../src/state/state.js"; +import type { DesiredResource } from "../src/engine/types.js"; +import { memberFieldPseudo } from "../src/engine/member-fields.js"; + +const HOST = "https://mychurch.church.tools"; + +/** + * An in-memory ChurchTools double for group member fields (#135). + * + * Deliberately models the ONE property that makes them interesting: a member field lives under its + * group (`/groups/{id}/memberfields`) and gets a globally unique id when created, so two groups can + * hold a field with the same local key and still be two independent resources. + */ +function makeCt(seed: { groups?: Record> } = {}) { + const groups: Record> = seed.groups ?? { + 100: { id: 100, name: "Praktikum 1", information: { groupTypeId: 5, groupStatusId: 1 } }, + 101: { id: 101, name: "Praktikum 2", information: { groupTypeId: 5, groupStatusId: 1 } }, + }; + const memberFields: Record[]> = {}; + const rulesets: Record = {}; + const statuses: Record = {}; + let nextGroupId = 900; + let nextFieldId = 500; + const calls: string[] = []; + + const get = async (path: string): Promise => { + calls.push(`GET ${path}`); + let m = /^\/groups\/(\d+)$/.exec(path); + if (m) { + const g = groups[Number(m[1])]; + if (!g) throw new CtApiError("not found", 404, null); + return g; + } + m = /^\/groups\/(\d+)\/memberfields$/.exec(path); + if (m) return memberFields[Number(m[1])] ?? []; + m = /^\/dynamicgroups\/(\d+)\/ruleset$/.exec(path); + if (m) { + const rs = rulesets[Number(m[1])]; + if (rs === undefined) throw new CtApiError("not found", 404, null); + return rs; + } + m = /^\/dynamicgroups\/(\d+)\/status$/.exec(path); + if (m) return { dynamicGroupStatus: statuses[Number(m[1])] ?? "none" }; + throw new CtApiError(`unmocked GET ${path}`, 404, null); + }; + + const request = async (method: string, path: string, body?: unknown): Promise => { + if (method === "GET") return get(path); + calls.push(`${method} ${path}`); + if (method === "POST" && path === "/groups") { + const id = nextGroupId++; + groups[id] = { id, ...(body as Record) }; + return { id }; + } + let m = /^\/groups\/(\d+)\/memberfields\/group$/.exec(path); + if (m && method === "POST") { + const groupId = Number(m[1]); + const id = nextFieldId++; + const row = { id, type: "group", ...(body as Record) }; + memberFields[groupId] = [...(memberFields[groupId] ?? []), row]; + return { id }; + } + m = /^\/groups\/(\d+)\/memberfields\/group\/(\d+)$/.exec(path); + if (m) { + const groupId = Number(m[1]); + const fieldId = Number(m[2]); + const rows = memberFields[groupId] ?? []; + const i = rows.findIndex((r) => r.id === fieldId); + if (i < 0) throw new CtApiError("not found", 404, null); + if (method === "DELETE") rows.splice(i, 1); + else rows[i] = { ...rows[i], ...(body as Record) }; + return undefined; + } + m = /^\/dynamicgroups\/(\d+)\/ruleset$/.exec(path); + if (m) { + if (method === "DELETE") delete rulesets[Number(m[1])]; + else rulesets[Number(m[1])] = (body as { dynamicGroupRuleSet: unknown[] }).dynamicGroupRuleSet[0]; + return undefined; + } + m = /^\/dynamicgroups\/(\d+)\/status$/.exec(path); + if (m) { + statuses[Number(m[1])] = (body as { dynamicGroupStatus: string }).dynamicGroupStatus; + return undefined; + } + m = /^\/groups\/(\d+)$/.exec(path); + if (m) { + groups[Number(m[1])] = { ...groups[Number(m[1])], ...(body as Record) }; + return undefined; + } + throw new CtApiError(`unmocked ${method} ${path}`, 404, null); + }; + + const getFn = vi.fn(get); + const requestFn = vi.fn(request); + return { + get: getFn, + request: requestFn, + /** The same doubles, typed as the client surface `buildPlan`/`executePlan` expect. */ + client: { get: getFn, request: requestFn } as unknown as Pick, + groups, + memberFields, + rulesets, + statuses, + calls, + }; +} + +function stateWith(entries: Record }>) { + const state: State = emptyState(HOST); + for (const [key, e] of Object.entries(entries)) { + state.resources[key] = { + ...e, + key, + adoptedAt: "2026-01-01T00:00:00.000Z", + updatedAt: "2026-01-01T00:00:00.000Z", + }; + } + return state; +} + +/** The OJBP blueprint from #135, parametrised — the point being it can be called twice. */ +function praktikum(key: string, name: string): DesiredResource { + const { ct, resources } = createContext(); + ct.group({ + key, + name, + groupTypeId: 5, + groupStatusId: 1, + memberFields: [{ key: "wahl", name: "Wahl", fieldTypeCode: "text" }], + }); + return resources[0]!; +} + +let warnings: string[]; +beforeEach(() => { + warnings = []; + vi.spyOn(process.stderr, "write").mockImplementation((chunk: unknown) => { + warnings.push(String(chunk)); + return true; + }); +}); + +describe("group member fields — DSL and identity (#135)", () => { + it("scopes identity by the managed group, so two groups sharing a local key stay independent", async () => { + const ct = makeCt(); + const state = stateWith({}); + const desired = [praktikum("praktikum_1", "Praktikum 1"), praktikum("praktikum_2", "Praktikum 2")]; + + const { plan } = await buildPlan(ct.client, state, desired); + expect(plan.items.map((i) => `${i.action} ${i.key}`)).toEqual([ + "create praktikum_1", + "create praktikum_2", + ]); + + await executePlan(plan, { client: ct.client, state, statePath: "s.json", save: async () => {} }); + + const g1 = state.resources.praktikum_1!.id; + const g2 = state.resources.praktikum_2!.id; + expect(g1).not.toBe(g2); + // Two ChurchTools fields, two ids, no state-key collision: the identity is the GROUP key plus + // the local key, and each group's own state entry carries its own id mapping. + const id1 = state.resources.praktikum_1!.memberFields!.wahl; + const id2 = state.resources.praktikum_2!.memberFields!.wahl; + expect(id1).toBeTypeOf("number"); + expect(id2).toBeTypeOf("number"); + expect(id1).not.toBe(id2); + expect(ct.memberFields[g1]!.map((r) => r.referenceName)).toEqual(["wahl"]); + expect(ct.memberFields[g2]!.map((r) => r.referenceName)).toEqual(["wahl"]); + }); + + it("refuses a declaration carrying a ChurchTools field id — the portability guarantee", () => { + const { ct } = createContext(); + expect(() => + ct.group({ + key: "g", + name: "G", + memberFields: [{ key: "wahl", name: "Wahl", id: 42 }], + }), + ).toThrow(/"id" must never appear in config/); + }); + + it("rejects a duplicate local key within one group", () => { + const { ct } = createContext(); + expect(() => + ct.group({ + key: "g", + name: "G", + memberFields: [ + { key: "wahl", name: "Wahl" }, + { key: "wahl", name: "Wahl 2" }, + ], + }), + ).toThrow(/duplicate member field key "wahl"/); + }); + + it("rejects memberFields on a non-group declaration", () => { + const { ct } = createContext(); + expect(() => ct.campus({ key: "mz", name: "Mainz", memberFields: [] })).toThrow(/only valid on a group/); + }); +}); + +describe("group member fields — plan (#135)", () => { + it("pins the rendered plan for a CREATE", async () => { + const ct = makeCt(); + const state = stateWith({}); + const { plan } = await buildPlan(ct.client, state, [praktikum("praktikum_1", "Praktikum 1")]); + const rendered = renderPlan(plan); + expect(rendered).toContain("+ group.praktikum_1"); + expect(rendered).toContain('memberField:wahl: {"name":"Wahl","fieldTypeCode":"text"}'); + expect(rendered).toContain("Plan: 1 to create, 0 to update, 0 to delete."); + }); + + it("pins the rendered plan for an UPDATE to a field definition", async () => { + const ct = makeCt(); + ct.memberFields[100] = [ + { id: 501, type: "group", referenceName: "wahl", name: "Wahl", fieldTypeCode: "text" }, + ]; + const state = stateWith({ + praktikum_1: { + type: "group", + id: 100, + fields: { name: "Praktikum 1", groupTypeId: 5, groupStatusId: 1 }, + }, + }); + const { ct: dsl, resources } = createContext(); + dsl.group({ + key: "praktikum_1", + name: "Praktikum 1", + groupTypeId: 5, + groupStatusId: 1, + memberFields: [{ key: "wahl", name: "Wahl (neu)", fieldTypeCode: "text" }], + }); + const { plan } = await buildPlan(ct.client, state, resources); + const rendered = renderPlan(plan); + expect(rendered).toContain("~ group.praktikum_1 (#100)"); + expect(rendered).toContain( + 'memberField:wahl: {"name":"Wahl","fieldTypeCode":"text"} -> {"name":"Wahl (neu)","fieldTypeCode":"text"}', + ); + expect(rendered).toContain("Plan: 0 to create, 1 to update, 0 to delete."); + }); + + it("ignores a live property the declaration does not name, so a clean apply is a no-op", async () => { + const ct = makeCt(); + const state = stateWith({}); + const desired = [praktikum("praktikum_1", "Praktikum 1")]; + + const { plan } = await buildPlan(ct.client, state, desired); + await executePlan(plan, { client: ct.client, state, statePath: "s.json", save: async () => {} }); + + // ChurchTools echoes back more than was declared (server defaults, the id, the referenceName). + const gid = state.resources.praktikum_1!.id; + ct.memberFields[gid] = ct.memberFields[gid]!.map((r) => ({ ...r, sortKey: 7, nameTranslated: null })); + + const again = await buildPlan(ct.client, state, [praktikum("praktikum_1", "Praktikum 1")]); + expect(renderPlan(again.plan)).toBe("No changes. Desired state matches ChurchTools."); + }); + + it("surfaces an undeclared live field as a DELETE CANDIDATE and never plans a delete", async () => { + const ct = makeCt(); + ct.memberFields[100] = [ + { id: 501, type: "group", referenceName: "wahl", name: "Wahl", fieldTypeCode: "text" }, + { id: 502, type: "group", referenceName: "alt", name: "Alt", fieldTypeCode: "text" }, + ]; + const state = stateWith({ + praktikum_1: { + type: "group", + id: 100, + fields: { name: "Praktikum 1", groupTypeId: 5, groupStatusId: 1 }, + }, + }); + // `alt` was dropped from config — the exact scenario that must NOT delete anything. + const { plan } = await buildPlan(ct.client, state, [ + { ...praktikum("praktikum_1", "Praktikum 1"), key: "praktikum_1" }, + ]); + expect(plan.items.every((i) => i.action !== "delete")).toBe(true); + expect(renderPlan(plan)).toBe("No changes. Desired state matches ChurchTools."); + expect(warnings.join("\n")).toMatch( + /praktikum_1::alt.*DELETE CANDIDATE.*ct destroy --member-field praktikum_1::alt/s, + ); + + // …and applying the plan issues no DELETE at all. + await executePlan(plan, { client: ct.client, state, statePath: "s.json", save: async () => {} }); + expect(ct.calls.some((c) => c.startsWith("DELETE"))).toBe(false); + expect(ct.memberFields[100]).toHaveLength(2); + }); + + it("does not fabricate creates when the member-field read fails (plan stays honest)", async () => { + const ct = makeCt(); + const realGet = ct.get.getMockImplementation()!; + ct.get.mockImplementation(async (path: string) => { + if (path === "/groups/100/memberfields") throw new CtApiError("boom", 429, null); + return realGet(path); + }); + const state = stateWith({ + praktikum_1: { + type: "group", + id: 100, + fields: { name: "Praktikum 1", groupTypeId: 5, groupStatusId: 1 }, + }, + }); + const { plan } = await buildPlan(ct.client, state, [praktikum("praktikum_1", "Praktikum 1")]); + const item = plan.items.find((i) => i.key === "praktikum_1")!; + expect(item.action).toBe("no-op"); + expect(item.note).toBe("fetch-failed"); + expect(renderPlan(plan)).toContain("INCOMPLETE"); + }); +}); + +describe("group member fields — apply (#135)", () => { + it("creates fields only after the owning group exists, and updates existing ones idempotently", async () => { + const ct = makeCt(); + const state = stateWith({}); + const { plan } = await buildPlan(ct.client, state, [praktikum("praktikum_1", "Praktikum 1")]); + await executePlan(plan, { client: ct.client, state, statePath: "s.json", save: async () => {} }); + + const groupCreate = ct.calls.indexOf("POST /groups"); + const fieldCreate = ct.calls.findIndex((c) => c.includes("/memberfields/group")); + expect(groupCreate).toBeGreaterThanOrEqual(0); + expect(fieldCreate).toBeGreaterThan(groupCreate); + + // Second run: rename the field. The existing row is PATCHed, never re-created. + const { ct: dsl, resources } = createContext(); + const gid = state.resources.praktikum_1!.id; + dsl.group({ + key: "praktikum_1", + name: "Praktikum 1", + groupTypeId: 5, + groupStatusId: 1, + memberFields: [{ key: "wahl", name: "Wahl (neu)", fieldTypeCode: "text" }], + }); + const second = await buildPlan(ct.client, state, resources); + await executePlan(second.plan, { client: ct.client, state, statePath: "s.json", save: async () => {} }); + expect(ct.memberFields[gid]).toHaveLength(1); + expect(ct.memberFields[gid]![0]!.name).toBe("Wahl (neu)"); + expect(ct.calls.filter((c) => c.startsWith("PATCH /groups/") && c.includes("memberfields"))).toHaveLength( + 1, + ); + }); + + it("orders field creation before the dependent dynamic ruleset is installed", async () => { + const ct = makeCt(); + const state = stateWith({}); + const { ct: dsl, resources } = createContext(); + dsl.group({ + key: "praktikum_1", + name: "Praktikum 1", + groupTypeId: 5, + groupStatusId: 1, + memberFields: [{ key: "wahl", name: "Wahl", fieldTypeCode: "text" }], + dynamic: { + status: "active", + ruleset: { + description: "wahl", + query: { "==": [{ var: "memberfield.id" }, ref.groupMemberField("praktikum_1", "wahl")] }, + process: {}, + }, + }, + }); + const { plan } = await buildPlan(ct.client, state, resources); + const changes = plan.items[0]!.changes.map((c) => c.field); + expect(changes.indexOf("memberField:wahl")).toBeLessThan(changes.indexOf("dynamic")); + + await executePlan(plan, { client: ct.client, state, statePath: "s.json", save: async () => {} }); + const fieldCreate = ct.calls.findIndex((c) => c.includes("/memberfields/group")); + const rulesetPut = ct.calls.findIndex((c) => c.startsWith("PUT /dynamicgroups/")); + expect(fieldCreate).toBeGreaterThanOrEqual(0); + expect(rulesetPut).toBeGreaterThan(fieldCreate); + + // The pending `::` reference was completed with the id the create just minted — + // never a host-specific id frozen at authoring time. + const gid = state.resources.praktikum_1!.id; + const stored = ct.rulesets[gid] as { query: { "==": unknown[] } }; + expect(stored.query["=="][1]).toBe(state.resources.praktikum_1!.memberFields!.wahl); + }); +}); + +describe("group member fields — ruleset validation (#135)", () => { + it("fails at config-eval time when a ruleset names a field the target group does not declare", async () => { + await expect( + evaluateConfig((ct) => { + ct.group({ key: "a", name: "A", memberFields: [{ key: "wahl", name: "Wahl" }] }); + ct.group({ + key: "b", + name: "B", + dynamic: { + status: "active", + ruleset: { query: { "==": [{ var: "x" }, ref.groupMemberField("a", "nope")] } }, + }, + }); + }), + ).rejects.toThrow(/does not declare.*it declares: wahl/s); + }); + + it("fails when the referenced group does not manage member fields at all", async () => { + await expect( + evaluateConfig((ct) => { + ct.group({ key: "a", name: "A" }); + ct.group({ + key: "b", + name: "B", + dynamic: { + status: "active", + ruleset: { query: { "==": [{ var: "x" }, ref.groupMemberField("a", "wahl")] } }, + }, + }); + }), + ).rejects.toThrow(/does not manage member fields/); + }); + + it("accepts a reference to a field the target group does declare", async () => { + const { resources } = await evaluateConfig((ct) => { + ct.group({ key: "a", name: "A", memberFields: [{ key: "wahl", name: "Wahl" }] }); + ct.group({ + key: "b", + name: "B", + dynamic: { + status: "active", + ruleset: { query: { "==": [{ var: "x" }, ref.groupMemberField("a", "wahl")] } }, + }, + }); + }); + expect(resources).toHaveLength(2); + }); + + it("hard-errors at plan time for a ref into an ADOPTED group that has no such field", async () => { + const ct = makeCt(); + ct.memberFields[100] = [{ id: 501, type: "group", referenceName: "wahl", name: "Wahl" }]; + const state = stateWith({ + praktikum_1: { + type: "group", + id: 100, + fields: { name: "Praktikum 1", groupTypeId: 5, groupStatusId: 1 }, + }, + other: { type: "group", id: 101, fields: { name: "Praktikum 2", groupTypeId: 5, groupStatusId: 1 } }, + }); + const { ct: dsl, resources } = createContext(); + dsl.group({ + key: "other", + name: "Praktikum 2", + groupTypeId: 5, + groupStatusId: 1, + dynamic: { + status: "active", + ruleset: { query: { "==": [{ var: "x" }, ref.groupMemberField("praktikum_1", "nope")] } }, + }, + }); + await expect(buildPlan(ct.client, state, resources)).rejects.toThrow( + /group #100 has no member field "nope"/, + ); + }); +}); + +describe("memberFieldPseudo", () => { + it("is the diff key a declaration folds into", () => { + expect(memberFieldPseudo("wahl")).toBe("memberField:wahl"); + }); +}); diff --git a/tests/synthetic-dynamic.test.ts b/tests/synthetic-dynamic.test.ts index 40f5b9c..bb76c2d 100644 --- a/tests/synthetic-dynamic.test.ts +++ b/tests/synthetic-dynamic.test.ts @@ -243,6 +243,7 @@ describe("dynamic synthetic field — apply", () => { client: { request } as unknown as Pick, state, id: 5, + key: "g", change: { field: "dynamic", from: undefined, @@ -266,6 +267,7 @@ describe("dynamic synthetic field — apply", () => { client: { request } as unknown as Pick, state, id: 5, + key: "g", change: { field: "dynamic", from: { status: "active", ruleset }, // same ruleset, only the status flips @@ -286,6 +288,7 @@ describe("dynamic synthetic field — apply", () => { client: { request } as unknown as Pick, state, id: 5, + key: "g", change: { field: "dynamic", from: { status: "active", ruleset: { description: "old", query: {}, process: {} } }, @@ -307,6 +310,7 @@ describe("dynamic synthetic field — apply", () => { client: { request } as unknown as Pick, state, id: 5, + key: "g", change: { field: "dynamic", from: { status: "active", ruleset: {} }, @@ -330,6 +334,7 @@ describe("dynamic synthetic field — apply", () => { client: { request } as unknown as Pick, state, id: 5, + key: "g", change: { field: "dynamic", from: undefined, to: { status: "none", ruleset: {} } }, }), ).resolves.toBeUndefined(); // 404 swallowed — apply does not abort @@ -350,6 +355,7 @@ describe("dynamic synthetic field — apply", () => { client: { request } as unknown as Pick, state, id: 5, + key: "g", change: { field: "dynamic", from: undefined, to: { status: "none", ruleset: {} } }, }), ).rejects.toThrow(/Server Error/); From 861f9fac8ad560ec4c9c1d86d18c22878df7e34c Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Mon, 24 Aug 2026 20:21:45 +0200 Subject: [PATCH 2/3] fix(member-fields): abort the group destroy on a failed field delete, and use one spelling of identity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on #135: - `ct destroy --member-field g::f --target g` deleted the group even when the field's DELETE had just failed — taking the field with it, right after the run printed "Nothing further was deleted". The loop now reports success and the caller holds back the group deletes. - The `memberFields` state map was written under the raw declaration key and read under the raw ref key, while every live-row comparison slugs. A `wahl` declaration plus a `ref.groupMemberField(g, "Wahl")` therefore hard-failed mid-apply, after the group and field had been created. `memberFieldStateKey` is now the one canonical spelling — writer, reader, `ct destroy`'s state cleanup, the declaration-uniqueness check and the ruleset-ref check all use it. - `isGroupScopedMemberField` returned on the FIRST string among type/fieldCategory/source/fieldSource, so a row carrying a field TYPE in `type` ("text") would exclude every row: adopt emits nothing and apply POSTs a duplicate field on every run. Rows are now classified by value — "group" includes, a known non-group source excludes, anything unrecognised includes. - A group's N member-field changes issued N identical `GET /groups/{id}/memberfields`. They share one per-item read cache now. - `--with-member-fields` swallowed only a 404, so a 403 or a 429 on one group aborted a `--children-of` adoption partway, with earlier groups already in state. Every read failure is now warned about and the group adopted without its fields, as the comment always claimed. Also states in the flag's help text (and the handbook) that the opt-in is transitional, per docs/adoption-contract.md (#141). Claude-Session: https://claude.ai/code/session_018JShVZYNLaRb4hF5KbHCXG --- docs/handbuch/group-member-fields.md | 17 +++-- src/commands/adopt-group.ts | 28 ++++++-- src/commands/destroy.ts | 38 +++++++++-- src/config/context.ts | 16 +++-- src/engine/execute.ts | 6 +- src/engine/member-fields.ts | 69 ++++++++++++++++++-- src/engine/synthetic.ts | 36 ++++++++-- src/resolve/resolver.ts | 3 +- tests/adopt-group-command.test.ts | 29 ++++++++ tests/destroy-command.test.ts | 36 ++++++++++ tests/member-fields.test.ts | 98 +++++++++++++++++++++++++++- 11 files changed, 340 insertions(+), 36 deletions(-) diff --git a/docs/handbuch/group-member-fields.md b/docs/handbuch/group-member-fields.md index 2fa946e..68bea24 100644 --- a/docs/handbuch/group-member-fields.md +++ b/docs/handbuch/group-member-fields.md @@ -113,11 +113,20 @@ Rows the group's member form shows but that are not group-scoped (person master data, group-type defaults) are not emitted: only `/memberfields/group` rows can be created or updated. -!!! note "Why opt-in, and not the default" +!!! note "Why opt-in, and not the default — and why that is temporary" Whether an owned child resource is adopted automatically is a project-wide -contract, not a per-feature choice. That decision lives on -[ct-cli#141](https://github.com/eqrm/ct-cli/issues/141); until it lands, -member fields are captured only when you ask for them. +contract, not a per-feature choice. That contract now exists +([ct-cli#141](https://github.com/eqrm/ct-cli/issues/141)), and it classes +member fields as an **owned structural child** — so `--with-member-fields` +is a **transitional** opt-in, not a statement that member fields are +optional. The flip to default-on is a separate release: the flag keeps +working as a no-op, and `--no-member-fields` ships in the same release as +the flip. Nothing about an existing config changes when that happens — an +absent `memberFields:` block still means "unmanaged". + +Rows that could not be read (a 403, a rate-limited 429) are reported and the +group is adopted **without** them, so one unreadable group never aborts a +bulk adoption — re-run with `--with-member-fields` once the read succeeds. ## Plan and apply diff --git a/src/commands/adopt-group.ts b/src/commands/adopt-group.ts index d8618f7..876696e 100644 --- a/src/commands/adopt-group.ts +++ b/src/commands/adopt-group.ts @@ -28,7 +28,7 @@ import { ReverseResolver, type RoleCatalogEntry } from "../resolve/reverse.js"; import type { RefKind } from "../resolve/refs.js"; import { formatPortablizeWarnings, portablizeRuleset, scanUnportablized } from "../config/query-refs.js"; import { chooseAdoptKey, loadState, saveState, upsert, type State } from "../state/state.js"; -import { success, info, warn, out } from "../ui.js"; +import { success, info, warn, out, formatError } from "../ui.js"; interface AdoptGroupOptions { key?: string; @@ -203,8 +203,10 @@ interface DynamicCapture { * resolving against this host's numbering. The local key comes from CT's own `referenceName` * (slugged), falling back to the slugged name for a field created in the ChurchTools UI. * - * Opt-in only (`--with-member-fields`). Whether owned child resources are adopted by default is a - * project-wide contract, deliberately left to #141. + * Opt-in only (`--with-member-fields`) — and that opt-in is TRANSITIONAL, not a statement that + * member fields are optional: they are a category-2 owned structural child, so the flip to + * default-on is a follow-up governed by the promotion policy in docs/adoption-contract.md (the flag + * survives as a no-op, and `--no-member-fields` ships in the same release as the flip). */ async function captureMemberFields( id: number, @@ -215,9 +217,18 @@ async function captureMemberFields( raw = await client.get(memberFieldsReadPath(id)); } catch (err) { // A group whose member fields cannot be read is not a reason to abort a bulk adoption of a whole - // subtree — say so and adopt the group without them. - if (err instanceof CtApiError && err.status === 404) return undefined; - throw err; + // subtree — say so and adopt the group without them. That holds for EVERY failure, not just the + // 404: a 403 on one group the token may not read the fields of, or a transient 429, would + // otherwise abort `--children-of` partway with the earlier groups already written to state. + // Silence is the one thing that is not allowed here, because "no member fields" and "could not + // read them" produce the same config. + if (!(err instanceof CtApiError && err.status === 404)) { + warn( + `group #${id}: member fields could not be read (${formatError(err)}) — adopted WITHOUT them. ` + + `Re-run \`ct adopt group ${id} --with-member-fields\` once the read succeeds.`, + ); + } + return undefined; } const rows = groupScopedRows(raw); if (rows.length === 0) return undefined; @@ -275,7 +286,10 @@ export function adoptGroupCommand(): Command { .option( "--with-member-fields", "also capture each group's group-scoped member-field definitions and emit the memberFields: " + - "block (portable: no ChurchTools field ids are ever emitted)", + "block (portable: no ChurchTools field ids are ever emitted). TRANSITIONAL: member fields " + + "are an owned part of a group, so this becomes the default in a later release (with a " + + "--no-member-fields escape hatch, and this flag kept working as a no-op) — see " + + "docs/adoption-contract.md", ) .option( "--portable-rulesets", diff --git a/src/commands/destroy.ts b/src/commands/destroy.ts index c2cc476..4400399 100644 --- a/src/commands/destroy.ts +++ b/src/commands/destroy.ts @@ -13,6 +13,7 @@ import type { DesiredResource } from "../engine/types.js"; import { writeBackup } from "../engine/backup.js"; import { groupScopedRows, + memberFieldStateKey, matchesLocalKey, memberFieldItemPath, memberFieldRowId, @@ -174,6 +175,11 @@ export function resolveMemberFieldTargets(state: State, raw: string[]): MemberFi * after every success. A field that is already gone in ChurchTools (no live row, or a 404 on the * DELETE) is success-with-note, mirroring `runDeleteLoop`; any other error stops the run with state * saved up to that point. + * + * Returns `false` if ANY target failed — including the ones that only skip ahead to the next target + * — so the caller can hold back the group deletes that follow. That gate matters: the groups being + * destroyed are the very groups these fields belong to, so carrying on would delete a field the run + * just reported it could not delete, and the printed "Nothing further was deleted" would be a lie. */ export async function runMemberFieldDeleteLoop(ctx: { client: Pick; @@ -181,15 +187,20 @@ export async function runMemberFieldDeleteLoop(ctx: { statePath: string; targets: MemberFieldTarget[]; save?: (path: string, state: State) => Promise; -}): Promise { +}): Promise { const { client, state, statePath, targets } = ctx; const save = ctx.save ?? saveState; + let ok = true; for (const target of targets) { const forget = async (): Promise => { const managed = state.resources[target.groupKey]; - if (managed?.memberFields && target.fieldKey in managed.memberFields) { + // Slugged, exactly as the apply that wrote it keyed the entry (`memberFieldStateKey`) and as + // `matchesLocalKey` matched the live row — otherwise `--member-field g::Wahl` deletes the + // field in ChurchTools but leaves `memberFields.wahl` pointing at the id it just destroyed. + const stateKey = memberFieldStateKey(target.fieldKey); + if (managed?.memberFields && stateKey in managed.memberFields) { const rest = { ...managed.memberFields }; - delete rest[target.fieldKey]; + delete rest[stateKey]; if (Object.keys(rest).length > 0) managed.memberFields = rest; else delete managed.memberFields; } @@ -205,13 +216,14 @@ export async function runMemberFieldDeleteLoop(ctx: { `"${target.fieldKey}" — refusing to guess which one to delete. Rename one in ChurchTools first.`, ); process.exitCode = 1; + ok = false; continue; } fieldId = matches.length === 1 ? memberFieldRowId(matches[0]!) : undefined; } catch (err) { error(`Stopped at ${target.identity}: ${formatError(err)}. Nothing further was deleted.`); process.exitCode = 1; - return; + return false; } if (fieldId === undefined) { await forget(); @@ -232,11 +244,12 @@ export async function runMemberFieldDeleteLoop(ctx: { `Stopped at ${target.identity}: ${formatError(err)}. State saved up to this point — re-run to resume.`, ); process.exitCode = 1; - return; + return false; } await forget(); success(`Destroyed group member field ${target.identity} (#${fieldId})`); } + return ok; } export function destroyWarnings(state: State, keys: string[]): string[] { @@ -374,7 +387,20 @@ export function destroyCommand(): Command { // Member fields FIRST: they are owned by their group, so deleting the group would take them // with it and the explicit per-field record would be lost. if (memberFieldTargets.length > 0) { - await runMemberFieldDeleteLoop({ client, state, statePath, targets: memberFieldTargets }); + const fieldsDone = await runMemberFieldDeleteLoop({ + client, + state, + statePath, + targets: memberFieldTargets, + }); + if (!fieldsDone) { + // A field that could not be deleted must not be deleted anyway as collateral of its + // owning group's destroy — and the user was just told nothing further would happen. + if (ordered.length > 0) { + error(`Not destroying ${ordered.join(", ")} — a member field on it could not be deleted first.`); + } + return; + } } if (ordered.length > 0) { await runDeleteLoop({ client, state, statePath, ordered }); diff --git a/src/config/context.ts b/src/config/context.ts index c8a32c8..76db652 100644 --- a/src/config/context.ts +++ b/src/config/context.ts @@ -18,6 +18,7 @@ import { isManagedMemberFieldProp, MEMBER_FIELD_FORBIDDEN_PROPS, MEMBER_FIELD_PROPS, + memberFieldStateKey, } from "../engine/member-fields.js"; import { collectRefs } from "../resolve/refs.js"; import type { DomainType } from "../permissions/grants.js"; @@ -420,13 +421,18 @@ function normalizeMemberFields( if (typeof localKey !== "string" || localKey.length === 0) { throw new Error(`group "${key}": each entry of "memberFields" needs a non-empty string "key".`); } - if (seen.has(localKey)) { + // Compared in the CANONICAL spelling, because that is what identity means here: `matchesLocalKey` + // slugs both sides, so "Wahl" and "wahl" would resolve to the same live row and two declarations + // would fight over it — each apply PATCHing the row the other just wrote. + const canonical = memberFieldStateKey(localKey); + if (seen.has(canonical)) { throw new Error( `group "${key}": duplicate member field key "${localKey}". Local keys must be unique within ` + - `a group — they are half of the portable "${key}::${localKey}" identity.`, + `a group — they are half of the portable "${key}::${localKey}" identity, and are compared ` + + `in their normalised form ("Wahl" and "wahl" are the same key).`, ); } - seen.add(localKey); + seen.add(canonical); for (const forbidden of MEMBER_FIELD_FORBIDDEN_PROPS) { if (props[forbidden] !== undefined) { throw new Error( @@ -662,7 +668,9 @@ function validateMemberFieldRefs(resources: DesiredResource[], byKey: Map f.key === ref.field)) { + // Normalised on both sides, exactly as the live row is matched (`matchesLocalKey`) and as the + // created id is keyed in state (`memberFieldStateKey`) — one spelling of identity everywhere. + if (!target.memberFields.some((f) => memberFieldStateKey(f.key) === memberFieldStateKey(ref.field))) { const declared = target.memberFields.map((f) => f.key).join(", "); throw new Error( `"${r.key}" references member field "${ref.group}::${ref.field}", which group "${ref.group}" ` + diff --git a/src/engine/execute.ts b/src/engine/execute.ts index 7a807ef..0b217dd 100644 --- a/src/engine/execute.ts +++ b/src/engine/execute.ts @@ -110,11 +110,15 @@ async function applySyntheticFields( key: string, changes: FieldChange[], ): Promise { + // One read cache per ITEM, so a group's N member-field changes share a single + // `GET /groups/{id}/memberfields` instead of issuing N identical ones. Deliberately not longer- + // lived than the item: the next apply must see the rows this one wrote. + const reads = new Map>(); for (const c of changes) { const f = syntheticField(c.field); if (!f) continue; const change = hasPendingRef(c.to) ? { ...c, to: reresolvePendingValue(c.to, state) } : c; - await f.apply({ client, state, id, key, change }); + await f.apply({ client, state, id, key, change, reads }); } } diff --git a/src/engine/member-fields.ts b/src/engine/member-fields.ts index f02f784..8f91863 100644 --- a/src/engine/member-fields.ts +++ b/src/engine/member-fields.ts @@ -107,6 +107,29 @@ export interface MemberFieldSpec { /** A row as ChurchTools returns it from `GET /groups/{groupId}/memberfields`. */ export type MemberFieldRow = Record; +/** Row keys that have, across CT versions, carried the field's scope/source. */ +const SCOPE_KEYS = ["type", "fieldCategory", "source", "fieldSource"] as const; + +/** + * Values that positively mark a row as sourced OUTSIDE the group — the only thing that excludes a + * row. Kept as an explicit vocabulary so an unfamiliar value (a field type, a new CT spelling) + * falls through to "group-scoped" rather than silently emptying the list. + */ +const NON_GROUP_SCOPES = new Set([ + "person", + "persons", + "personmasterdata", + "master", + "masterdata", + "db", + "dbfield", + "grouptype", + "group_type", + "grouptypedefault", + "global", + "system", +]); + /** * Is this live row a GROUP-SCOPED custom member field — i.e. one of the rows the * `/memberfields/group` write endpoints own? @@ -116,17 +139,49 @@ export type MemberFieldRow = Record; * (person master data, group-type defaults). Only the first kind can be created or updated through * `/memberfields/group`, so only the first kind is adoptable or manageable here. * - * The discriminator is read leniently, because CT has spelled this differently across versions: - * a `type`/`fieldCategory`/`source` of `"group"` marks a group-scoped row. A row that carries NO - * discriminator at all is treated as group-scoped — the endpoint is the group's own, so "we cannot - * tell" errs towards showing the field rather than silently hiding one an author needs. + * The discriminator is read leniently, because CT has spelled this differently across versions — + * and because the candidate keys are not all unambiguously about SCOPE. `type` in particular may + * carry a field TYPE (`"text"`, `"date"`) rather than a source on some versions; the managed + * property for that is `fieldTypeCode`, but a live row is not obliged to agree. So the row is + * classified by VALUE, not by which key happened to appear first: + * + * - any candidate saying `"group"` → group-scoped; + * - otherwise, a candidate naming a known non-group SOURCE ({@link NON_GROUP_SCOPES}) → excluded; + * - otherwise (no discriminator, or an unrecognised value like a field type) → group-scoped. + * + * The last rule is the important one. The endpoint is the group's own, so "we cannot tell" errs + * towards showing the field rather than silently hiding one an author needs — and an unrecognised + * value must never be read as "not a group field", because that would make `groupScopedRows` empty + * for every group: adopt would emit nothing, and apply would find no match and POST a fresh field + * on every single run, duplicating the group's member fields. Over-inclusion fails loudly instead + * (a write against a row `/memberfields/group` does not own is rejected by CT). + * + * Worth pinning against a live response when one is at hand; until then this errs on the side of + * the loud failure. */ export function isGroupScopedMemberField(row: MemberFieldRow): boolean { - for (const name of ["type", "fieldCategory", "source", "fieldSource"]) { + let sawNonGroup = false; + for (const name of SCOPE_KEYS) { const value = row[name]; - if (typeof value === "string") return value.toLowerCase() === "group"; + if (typeof value !== "string") continue; + const normalized = value.toLowerCase().replace(/[\s-]/g, ""); + if (normalized === "group") return true; + if (NON_GROUP_SCOPES.has(normalized)) sawNonGroup = true; } - return true; + return !sawNonGroup; +} + +/** + * The key a member-field id is stored under in the owning group's `memberFields` state map. + * + * SLUGGED, because every other comparison in this feature is: `matchesLocalKey` slugs both sides, + * so a declaration keyed `wahl` and a `ref.groupMemberField("g", "Wahl")` are deliberately the same + * field. If the state map were keyed by the raw string instead, the write (`wahl`) and the pending + * read (`Wahl`) would miss each other and apply would hard-fail mid-run — after the group and the + * field had already been created. One canonical spelling, used by writer and reader alike (#135). + */ +export function memberFieldStateKey(localKey: string): string { + return slug(localKey); } /** diff --git a/src/engine/synthetic.ts b/src/engine/synthetic.ts index dbc46ce..6b9a02d 100644 --- a/src/engine/synthetic.ts +++ b/src/engine/synthetic.ts @@ -20,6 +20,7 @@ import { slug } from "../resources/registry.js"; import { actualMemberFieldProps, groupScopedRows, + memberFieldStateKey, localKeyOf, matchesLocalKey, memberFieldIdentity, @@ -63,6 +64,13 @@ export interface SyntheticApplyCtx { */ key: string; change: FieldChange; + /** + * Per-ITEM read cache, keyed by request path. One apply item is one resource, so a group with N + * declared member fields would otherwise issue N identical `GET /groups/{id}/memberfields` — one + * per pseudo-field change — against an API that rate-limits (#145). Scoped to the item rather + * than the process on purpose: a later apply in the same process must see its own live rows. + */ + reads?: Map>; } export interface SyntheticPostApplyCtx { client: Pick; @@ -166,12 +174,30 @@ async function readMemberFields(client: Pick, groupId: number): return groupScopedRows(await client.get(memberFieldsReadPath(groupId))); } -/** Read them through the write client (apply only has `request`; `GET` goes through it just fine). */ +/** + * Read them through the write client (apply only has `request`; `GET` goes through it just fine), + * reusing the item's cached read when one is offered (see {@link SyntheticApplyCtx.reads}). + * + * Sharing one list across an item's member-field changes is safe because each change matches it by + * its OWN local key, and a local key is unique within the group: a row created for one key can + * never be the row another key was looking for. + */ async function readMemberFieldsForWrite( client: Pick, groupId: number, + reads?: Map>, ): Promise { - return groupScopedRows(await client.request("GET", memberFieldsReadPath(groupId))); + const path = memberFieldsReadPath(groupId); + const cached = reads?.get(path); + if (cached) return groupScopedRows(await cached); + const pending = client.request("GET", path); + reads?.set(path, pending); + try { + return groupScopedRows(await pending); + } catch (err) { + reads?.delete(path); // a failed read must not be remembered as this item's answer + throw err; + } } /** @@ -262,7 +288,7 @@ const memberFieldsField: SyntheticField = { }); return { desired: augmented, errors, unreadable }; }, - async apply({ client, state, id, key, change }) { + async apply({ client, state, id, key, change, reads }) { const local = memberFieldLocalKey(change.field); if (local === undefined) return; const props = change.to as Record | undefined; @@ -270,7 +296,7 @@ const memberFieldsField: SyntheticField = { // (it walks the desired side), so this is belt-and-braces: apply never deletes a member field. if (props === undefined || props === null) return; - const rows = await readMemberFieldsForWrite(client, id); + const rows = await readMemberFieldsForWrite(client, id, reads); const matches = rows.filter((row) => matchesLocalKey(row, local)); if (matches.length > 1) { throw new Error( @@ -285,7 +311,7 @@ const memberFieldsField: SyntheticField = { // reference in a ruleset applied later in this very item can be completed (#135). const managed = state.resources[key]; if (!managed) return; - managed.memberFields = { ...managed.memberFields, [local]: fieldId }; + managed.memberFields = { ...managed.memberFields, [memberFieldStateKey(local)]: fieldId }; }; if (matches.length === 1) { diff --git a/src/resolve/resolver.ts b/src/resolve/resolver.ts index a28bbcb..f2e6c20 100644 --- a/src/resolve/resolver.ts +++ b/src/resolve/resolver.ts @@ -31,6 +31,7 @@ import type { DesiredResource } from "../engine/types.js"; import { slug } from "../resources/registry.js"; import { groupScopedRows, + memberFieldStateKey, matchesLocalKey, memberFieldRowId, memberFieldsReadPath, @@ -753,7 +754,7 @@ function pendingIdFromState(r: Ref, state: State): number { // `applySyntheticFields` re-resolves each change immediately before applying it). So by the time // a ruleset carrying this marker is written, the id is already in state. const group = state.resources[r.group]; - const id = group?.memberFields?.[r.field]; + const id = group?.memberFields?.[memberFieldStateKey(r.field)]; if (typeof id !== "number") { throw new Error( `Pending ${refLabel(r)} did not resolve after its group applied — no member field "${r.field}" ` + diff --git a/tests/adopt-group-command.test.ts b/tests/adopt-group-command.test.ts index cc235bb..30676a0 100644 --- a/tests/adopt-group-command.test.ts +++ b/tests/adopt-group-command.test.ts @@ -601,6 +601,35 @@ describe("ct adopt group --with-member-fields (#135)", () => { expect(snippet).not.toContain("vorname"); }); + it("a 403 on one group's fields does not abort a bulk adoption — it warns and adopts without them", async () => { + const warnings: string[] = []; + const spy = vi.spyOn(process.stderr, "write").mockImplementation((chunk) => { + warnings.push(String(chunk)); + return true; + }); + const original = client.get.getMockImplementation()!; + client.get.mockImplementation((async (path: string) => { + if (path === "/groups/31/memberfields") throw new CtApiError("forbidden", 403, null); + return original(path); + }) as never); + try { + const snippet = await snippetFor([ + "group", + "31", + "--with-member-fields", + "--dry-run", + "--state", + statePath, + ]); + expect(snippet).toContain('key: "'); // the group itself was still adopted + expect(snippet).not.toContain("memberFields"); + expect(warnings.join("")).toMatch(/member fields could not be read/); + } finally { + client.get.mockImplementation(original as never); + spy.mockRestore(); + } + }); + it("emits no memberFields block for a group that has none", async () => { const snippet = await snippetFor([ "group", diff --git a/tests/destroy-command.test.ts b/tests/destroy-command.test.ts index fb29574..cfc9eb6 100644 --- a/tests/destroy-command.test.ts +++ b/tests/destroy-command.test.ts @@ -9,6 +9,7 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { rm } from "node:fs/promises"; +import { CtApiError } from "../src/api/ctClient.js"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -194,6 +195,41 @@ describe("ct destroy --member-field (#135)", () => { expect(calls).toEqual([]); }); + it("a field that could not be deleted holds back its owning group's destroy", async () => { + // Otherwise the group destroy takes the field with it as collateral — right after the run + // printed "Nothing further was deleted" — and the failure the user was told about becomes an + // irreversible delete they were told did not happen. + const state = emptyState(HOST); + state.resources.area = group("area", 1, { memberFields: { wahl: 701 } }); + await saveState(statePath, state); + requestMock.mockImplementationOnce((async (method: string, path: string) => { + calls.push({ method, path }); + throw new CtApiError("boom", 500, null); + }) as never); + + await runDestroy(["--member-field", "area::wahl", "--target", "area", "--state", statePath, "--force"]); + + expect(calls).toEqual([{ method: "DELETE", path: "/groups/1/memberfields/group/701" }]); + expect(process.exitCode).toBe(1); + const after = await loadState(statePath, HOST); + expect(after.resources.area).toBeDefined(); // the group was NOT destroyed + process.exitCode = 0; + }); + + it("matches the state entry in its normalised spelling, so no stale id is left behind", async () => { + const state = emptyState(HOST); + state.resources.area = group("area", 1, { memberFields: { wahl: 701 } }); + await saveState(statePath, state); + + // `--member-field area::Wahl` matches the live row (matchesLocalKey slugs), so it must also + // match the state entry the apply wrote under the slugged key. + await runDestroy(["--member-field", "area::Wahl", "--state", statePath, "--force"]); + + expect(calls).toEqual([{ method: "DELETE", path: "/groups/1/memberfields/group/701" }]); + const after = await loadState(statePath, HOST); + expect(after.resources.area!.memberFields).toBeUndefined(); + }); + it("still refuses to delete anything with neither --target nor --member-field", async () => { await saveState(statePath, emptyState(HOST)); await expect(runDestroy(["--state", statePath, "--force"])).rejects.toThrow( diff --git a/tests/member-fields.test.ts b/tests/member-fields.test.ts index d4b8201..c5b2eaf 100644 --- a/tests/member-fields.test.ts +++ b/tests/member-fields.test.ts @@ -6,7 +6,7 @@ import { renderPlan } from "../src/engine/render.js"; import { createContext, evaluateConfig, ref } from "../src/config/context.js"; import { emptyState, type State } from "../src/state/state.js"; import type { DesiredResource } from "../src/engine/types.js"; -import { memberFieldPseudo } from "../src/engine/member-fields.js"; +import { memberFieldPseudo, isGroupScopedMemberField } from "../src/engine/member-fields.js"; const HOST = "https://mychurch.church.tools"; @@ -380,6 +380,79 @@ describe("group member fields — apply (#135)", () => { }); }); +describe("group member fields — one spelling of identity (#135 review)", () => { + it("resolves a differently-cased ref against the declared key, end to end", async () => { + // `matchesLocalKey` slugs both sides, so `wahl` and `Wahl` are deliberately the SAME field. + // Everything downstream has to agree: if the id were recorded under the raw declaration key and + // read back under the raw ref key, apply would hard-fail here — after the group and the field + // had already been created in ChurchTools. + const ct = makeCt(); + const state = stateWith({}); + const { ct: dsl, resources } = createContext(); + dsl.group({ + key: "praktikum_1", + name: "Praktikum 1", + groupTypeId: 5, + groupStatusId: 1, + memberFields: [{ key: "wahl", name: "Wahl", fieldTypeCode: "text" }], + dynamic: { + status: "active", + ruleset: { + description: "wahl", + query: { "==": [{ var: "memberfield.id" }, ref.groupMemberField("praktikum_1", "Wahl")] }, + process: {}, + }, + }, + }); + const { plan } = await buildPlan(ct.client, state, resources); + await executePlan(plan, { client: ct.client, state, statePath: "s.json", save: async () => {} }); + + const gid = state.resources.praktikum_1!.id; + const stored = ct.rulesets[gid] as { query: { "==": unknown[] } }; + expect(stored.query["=="][1]).toBe(ct.memberFields[gid]![0]!.id); + expect(state.resources.praktikum_1!.memberFields).toEqual({ wahl: ct.memberFields[gid]![0]!.id }); + }); + + it("rejects two declarations that differ only in case, since both name the same live row", async () => { + await expect( + evaluateConfig((ct) => { + ct.group({ + key: "a", + name: "A", + memberFields: [ + { key: "wahl", name: "Wahl" }, + { key: "Wahl", name: "Wahl again" }, + ], + }); + }), + ).rejects.toThrow(/duplicate member field key/); + }); + + it("reads a group's member fields ONCE per apply, however many fields it declares", async () => { + const ct = makeCt(); + const state = stateWith({}); + const { ct: dsl, resources } = createContext(); + dsl.group({ + key: "praktikum_1", + name: "Praktikum 1", + groupTypeId: 5, + groupStatusId: 1, + memberFields: [ + { key: "wahl", name: "Wahl", fieldTypeCode: "text" }, + { key: "alt", name: "Alt", fieldTypeCode: "text" }, + { key: "dritte", name: "Dritte", fieldTypeCode: "text" }, + ], + }); + const { plan } = await buildPlan(ct.client, state, resources); + ct.calls.length = 0; + await executePlan(plan, { client: ct.client, state, statePath: "s.json", save: async () => {} }); + + const reads = ct.calls.filter((c) => c === `GET /groups/${state.resources.praktikum_1!.id}/memberfields`); + expect(reads).toHaveLength(1); // not one per declared field (#145: the API rate-limits) + expect(ct.memberFields[state.resources.praktikum_1!.id]).toHaveLength(3); + }); +}); + describe("group member fields — ruleset validation (#135)", () => { it("fails at config-eval time when a ruleset names a field the target group does not declare", async () => { await expect( @@ -461,3 +534,26 @@ describe("memberFieldPseudo", () => { expect(memberFieldPseudo("wahl")).toBe("memberField:wahl"); }); }); + +describe("isGroupScopedMemberField (#135 review)", () => { + it("keeps a row whose only string discriminator is a field TYPE, not a scope", () => { + // The failure this guards against is silent and compounding: read `type: "text"` as "not a + // group field" and EVERY group's list comes back empty — adopt emits nothing, and apply finds + // no match and POSTs a brand-new field on every single run, duplicating the group's fields. + expect(isGroupScopedMemberField({ id: 1, type: "text", name: "Wahl" })).toBe(true); + expect(isGroupScopedMemberField({ id: 1, type: "date", fieldTypeCode: "date" })).toBe(true); + }); + + it("keeps a row with no discriminator at all, and one that says group in any key", () => { + expect(isGroupScopedMemberField({ id: 1, name: "Wahl" })).toBe(true); + expect(isGroupScopedMemberField({ id: 1, type: "group" })).toBe(true); + expect(isGroupScopedMemberField({ id: 1, fieldCategory: "Group" })).toBe(true); + expect(isGroupScopedMemberField({ id: 1, source: "group", type: "text" })).toBe(true); + }); + + it("drops a row that positively names a source outside the group", () => { + expect(isGroupScopedMemberField({ id: 1, type: "person" })).toBe(false); + expect(isGroupScopedMemberField({ id: 1, source: "masterdata" })).toBe(false); + expect(isGroupScopedMemberField({ id: 1, fieldSource: "group-type" })).toBe(false); + }); +}); From c7c74eabf42efdadbdedc2d5a6deb76e421d5a2d Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Mon, 24 Aug 2026 20:25:17 +0200 Subject: [PATCH 3/3] test: compare the pinned plans without ANSI, and re-sign the handbook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pinned-plan assertions matched only locally: picocolors enables colour when the ambient CI env var is set, so every runner saw escape codes in `renderPlan` output. Strip them at the assertion — colour is render.test.ts's subject. Handbook pages re-read against the sources this branch changed; the member-field and dynamic-group pages gained the destroy gate, the normalised-key rule and the tolerant `--with-member-fields` read. Claude-Session: https://claude.ai/code/session_018JShVZYNLaRb4hF5KbHCXG --- docs/handbuch/blueprints.md | 2 +- docs/handbuch/dynamic-groups.md | 6 ++++-- docs/handbuch/group-member-fields.md | 13 ++++++++++++- docs/handbuch/permissions.md | 2 +- tests/member-fields.test.ts | 16 ++++++++++++---- 5 files changed, 30 insertions(+), 9 deletions(-) diff --git a/docs/handbuch/blueprints.md b/docs/handbuch/blueprints.md index f25dcf8..d0447a8 100644 --- a/docs/handbuch/blueprints.md +++ b/docs/handbuch/blueprints.md @@ -4,7 +4,7 @@ sources: - src/config/context.ts - src/engine/graph.ts - src/engine/hierarchy.ts -sources_hash: a1b74a4f10325270 +sources_hash: ed5171960e9206fa reviewed: 2026-08-17 --- diff --git a/docs/handbuch/dynamic-groups.md b/docs/handbuch/dynamic-groups.md index 43b8278..8a008d0 100644 --- a/docs/handbuch/dynamic-groups.md +++ b/docs/handbuch/dynamic-groups.md @@ -6,7 +6,7 @@ sources: - src/engine/dynamic.ts - src/engine/synthetic.ts - src/commands/adopt-group.ts -sources_hash: 69a1413762039f5c +sources_hash: a0be8585fb6a48dc reviewed: 2026-08-17 --- @@ -94,7 +94,9 @@ The resolver maps it to this host's id from `GET /groups/{groupId}/memberfields`. A field this config declares but that does not exist on the host yet resolves to a pending marker and is completed during apply, right after the create that minted it. A reference to a field the target -group does not declare fails at config-eval time, before any network call. See +group does not declare fails at config-eval time, before any network call — the +declared key and the referenced one being compared in their normalised form, so +`"Wahl"` finds a field declared as `wahl`. See [Group member fields](group-member-fields.md). ## Supplying a ruleset — three ways diff --git a/docs/handbuch/group-member-fields.md b/docs/handbuch/group-member-fields.md index 68bea24..3159af3 100644 --- a/docs/handbuch/group-member-fields.md +++ b/docs/handbuch/group-member-fields.md @@ -1,5 +1,5 @@ --- -sources_hash: 43f62be625184052 +sources_hash: 0dd7d04768dda2d2 title: Group member fields sources: - src/engine/member-fields.ts @@ -179,6 +179,11 @@ It carries the same guardrails as any other destroy: a pre-delete backup of the field definition, a typed confirmation, and the owning group's `preventDestroy` — protecting a group protects the fields it owns. +If a field cannot be deleted, the run stops there **and holds back any +`--target` groups in the same command**. Deleting the owning group would take +the field with it as collateral, so a run that just reported it could not delete +a field must not delete it anyway. + ## Referencing a field from a dynamic ruleset A ruleset must not freeze one host's field id into a file that is applied @@ -188,6 +193,12 @@ elsewhere, so a member field is referenced by its portable identity: ref.groupMemberField("ojbp_2026_27_praktikum_1", "wahl"); ``` +Local keys are compared in their **normalised** form throughout — `"Wahl"` and +`"wahl"` are the same field, whether they appear in a declaration, in a +reference, in `ct destroy --member-field`, or as ChurchTools' own +`referenceName` on the live row. Two declarations in one group that differ only +in case are therefore rejected as duplicates. + Three things follow: 1. **A reference to a field that is not declared for the target group fails at diff --git a/docs/handbuch/permissions.md b/docs/handbuch/permissions.md index f3a7d8c..522e13a 100644 --- a/docs/handbuch/permissions.md +++ b/docs/handbuch/permissions.md @@ -5,7 +5,7 @@ sources: - src/resolve/resolver.ts - src/resolve/refs.ts - src/config/context.ts -sources_hash: 8201657847a7964a +sources_hash: 3ce80118edd98c48 reviewed: 2026-08-17 --- diff --git a/tests/member-fields.test.ts b/tests/member-fields.test.ts index c5b2eaf..3639348 100644 --- a/tests/member-fields.test.ts +++ b/tests/member-fields.test.ts @@ -10,6 +10,14 @@ import { memberFieldPseudo, isGroupScopedMemberField } from "../src/engine/membe const HOST = "https://mychurch.church.tools"; +/** + * `renderPlan` colours its output, and picocolors decides that at module load: on a CI runner the + * ambient `CI` env var forces colour ON, so a pinned plan string only matches locally. Compare the + * plain text — the colours are render.test.ts's subject, not this file's. + */ +// eslint-disable-next-line no-control-regex +const plain = (s: string): string => s.replace(/\u001b\[[0-9;]*m/g, ""); + /** * An in-memory ChurchTools double for group member fields (#135). * @@ -210,7 +218,7 @@ describe("group member fields — plan (#135)", () => { const ct = makeCt(); const state = stateWith({}); const { plan } = await buildPlan(ct.client, state, [praktikum("praktikum_1", "Praktikum 1")]); - const rendered = renderPlan(plan); + const rendered = plain(renderPlan(plan)); expect(rendered).toContain("+ group.praktikum_1"); expect(rendered).toContain('memberField:wahl: {"name":"Wahl","fieldTypeCode":"text"}'); expect(rendered).toContain("Plan: 1 to create, 0 to update, 0 to delete."); @@ -237,7 +245,7 @@ describe("group member fields — plan (#135)", () => { memberFields: [{ key: "wahl", name: "Wahl (neu)", fieldTypeCode: "text" }], }); const { plan } = await buildPlan(ct.client, state, resources); - const rendered = renderPlan(plan); + const rendered = plain(renderPlan(plan)); expect(rendered).toContain("~ group.praktikum_1 (#100)"); expect(rendered).toContain( 'memberField:wahl: {"name":"Wahl","fieldTypeCode":"text"} -> {"name":"Wahl (neu)","fieldTypeCode":"text"}', @@ -258,7 +266,7 @@ describe("group member fields — plan (#135)", () => { ct.memberFields[gid] = ct.memberFields[gid]!.map((r) => ({ ...r, sortKey: 7, nameTranslated: null })); const again = await buildPlan(ct.client, state, [praktikum("praktikum_1", "Praktikum 1")]); - expect(renderPlan(again.plan)).toBe("No changes. Desired state matches ChurchTools."); + expect(plain(renderPlan(again.plan))).toBe("No changes. Desired state matches ChurchTools."); }); it("surfaces an undeclared live field as a DELETE CANDIDATE and never plans a delete", async () => { @@ -279,7 +287,7 @@ describe("group member fields — plan (#135)", () => { { ...praktikum("praktikum_1", "Praktikum 1"), key: "praktikum_1" }, ]); expect(plan.items.every((i) => i.action !== "delete")).toBe(true); - expect(renderPlan(plan)).toBe("No changes. Desired state matches ChurchTools."); + expect(plain(renderPlan(plan))).toBe("No changes. Desired state matches ChurchTools."); expect(warnings.join("\n")).toMatch( /praktikum_1::alt.*DELETE CANDIDATE.*ct destroy --member-field praktikum_1::alt/s, );