diff --git a/README.md b/README.md index 67e3e02..d07c8d8 100644 --- a/README.md +++ b/README.md @@ -173,6 +173,24 @@ as a normal field update, and `ct adopt group ` captures it. Which group fields are managed vs. deliberately left to the CT UI is recorded in [`docs/group-field-decisions.md`](docs/group-field-decisions.md). +**Same-named groups (#75):** ChurchTools guards group creation by NAME, not by +this tool's logical key — `POST /groups` 400s (`forbidden.duplicate.group`) if a +group with that name already exists, even when the two are legitimately +distinct (e.g. an archived and an active "Kids Elternabend 2026" event signup). +Opt in per-declaration to create it anyway: + +```ts +ct.group({ key: "kids_2026_b", name: "Kids Elternabend 2026", groupTypeId: 2, allowDuplicateName: true }); +``` + +`allowDuplicateName` sends CT's `force: true` on the CREATE request only — it is +never a managed field (not diffed, not in state, not touched on update, and +never adopted). **Never set it as a default**; it exists for the rare +intentional-duplicate case. If a create 400s on this guard without the flag +set, `ct apply`'s stop message explains the likely cause (an unmanaged existing +group that should be adopted with `ct adopt group --key `) and the +opt-in as the alternative. + Machine-readable output goes to **stdout** (pipe/`jq` it); human status lines go to **stderr**. diff --git a/docs/group-field-decisions.md b/docs/group-field-decisions.md index 5d1bc89..90ff357 100644 --- a/docs/group-field-decisions.md +++ b/docs/group-field-decisions.md @@ -31,6 +31,7 @@ field — a plain top-level key. `campusId` is wired the same deliberate way as | **`campusId`** | **managed (new, #21)** | The campus link is the tool's core "instantiate this area per campus" requirement. Numeric escape hatch only — an existing CT campus id (or `null` to clear). A *logical* `campus: "key"` reference (resolving a same-run campus by key) is **deferred to [#20](https://github.com/eqrm/ct-cli/issues/20)**; the DSL rejects a `campus` field with a pointer to #20 so it can't slip through as an un-diffable phantom. | | `parents` (hierarchy) | **opt-in synthetic** | Group→group hierarchy is reconciled through its own endpoint, not the group body — see `src/engine/synthetic.ts`. Opt-in via `parents: [...]`. | | `dynamic` (auto-group ruleset) | **opt-in synthetic** | Ruleset + status live behind a dedicated endpoint (#14); opt-in via the `dynamic` block. See `docs/dynamic-groups.md`. | +| `allowDuplicateName` (CT's `force` create flag, #75) | **create-only, unmanaged** | Not a field on the live group at all — it's a request-only escape hatch for `POST /groups`' same-name guard (`forbidden.duplicate.group`). Opt-in per declaration; sent as `force: true` on CREATE only. Deliberately NOT a registry `managedFields` entry: it has no live value to diff against, so it is destructured out of the DSL input before the field bag (never triggers the unknown-field warning, never lands in state, never adopted, never touched on update). | | `visibility` | **out of scope** | Not rights-bearing structure; instance-/policy-specific and easily changed in the UI. No demand in #21 to manage it. Promote later only with its own registry entry + tests. | | `note` | **out of scope** | Free-text annotation, not structure. Managing it would fight human edits in the UI for no structural benefit. | | `autoAccept` / open-for-members settings | **out of scope** | Membership-request policy — adjacent to *who is in a group*, which the tool never manages (`assertNotPeople`, `src/engine/guard.ts`). Left to the UI. | diff --git a/src/config/context.ts b/src/config/context.ts index 560f93d..8151608 100644 --- a/src/config/context.ts +++ b/src/config/context.ts @@ -104,6 +104,15 @@ export interface ResourceInput { * setting `false` (or removing it) and re-applying before you destroy. */ preventDestroy?: boolean; + /** + * Group-only (#75): opt in to CT's `force` create flag so a group can be created with the same + * name as an existing one. `POST /groups` otherwise 400s (`forbidden.duplicate.group`) whenever + * a same-named group already exists — CT's guard is name-based, not key-based, so two + * legitimately same-named groups (e.g. an archived and an active event signup) need this to + * both be creatable. Create-time only: never diffed, never in state, never sent on update. + * NEVER force by default — omit (or `false`) to keep CT's guard on. + */ + allowDuplicateName?: boolean; [field: string]: unknown; } @@ -229,10 +238,21 @@ function desugarDynamic(type: string, key: string, dynamic: unknown): DynamicSpe } function toDesired(type: string, input: ResourceInput, location?: string): DesiredResource { - const { key, parent, parents, dependsOn = [], preventDestroy, dynamic, ...fields } = input; + const { key, parent, parents, dependsOn = [], preventDestroy, dynamic, allowDuplicateName, ...fields } = input; if (!key || typeof key !== "string") { throw new Error(`${type} declaration is missing a string "key".`); } + // Group-only opt-in (#75) into CT's `force` create flag — see ResourceInput.allowDuplicateName. + // Destructured out above (never reaches `fields`), so it is accepted but never diffed/managed/ + // adopted, and never trips the unknown-field warning below. + if (allowDuplicateName !== undefined) { + if (type !== "group") { + throw new Error(`${type} "${key}": "allowDuplicateName" is only valid on a group.`); + } + if (typeof allowDuplicateName !== "boolean") { + throw new Error(`${type} "${key}": "allowDuplicateName" must be a boolean.`); + } + } // A nullish/empty `parent` is "no parent", not an opt-in to managed-empty hierarchy. if (parent != null && typeof parent !== "string") { throw new Error(`${type} "${key}": "parent" must be a string key.`); @@ -316,6 +336,7 @@ function toDesired(type: string, input: ResourceInput, location?: string): Desir dynamic: dynamicSpec, dependsOn: edges, preventDestroy, + allowDuplicateName, }; } diff --git a/src/engine/execute.ts b/src/engine/execute.ts index b2148b9..5131d20 100644 --- a/src/engine/execute.ts +++ b/src/engine/execute.ts @@ -9,9 +9,10 @@ * their own dedicated endpoints, not the owning resource's body — see synthetic.ts. */ import type { CtClient } from "../api/ctClient.js"; +import { CtApiError } from "../api/ctClient.js"; import type { ManagedResource, State } from "../state/state.js"; import { upsert, saveState } from "../state/state.js"; -import type { FieldChange, Plan } from "./types.js"; +import type { FieldChange, Plan, PlanItem } from "./types.js"; import { RESOURCES } from "../resources/registry.js"; import { assertNotPeople } from "./guard.js"; import { isSyntheticField, syntheticField } from "./synthetic.js"; @@ -19,6 +20,50 @@ import { reresolvePendingValue } from "../resolve/resolver.js"; import { hasPendingRef } from "../resolve/refs.js"; import { formatError } from "../ui.js"; +/** + * The messageKey CT's `POST /groups` 400s with when a same-named group already exists and the + * body did not carry `force: true` (#75). Confirmed against the ChurchTools OpenAPI spec's + * analogous `POST /persons` duplicate-guard error envelope (`forbidden.duplicate.person`, the + * same `{ message, messageKey, translatedMessage, args, errors }` shape) and the live 400 text + * observed against a dev rehearsal instance (issue #75): "Duplicate found. Use force flag to + * create group with same name." `POST /groups` itself is undocumented beyond "Bad Request" in the + * spec, so both the messageKey AND a text fallback are checked below. + */ +const DUPLICATE_GROUP_MESSAGE_KEY = "forbidden.duplicate.group"; + +/** Detect CT's same-name group-creation guard (#75) from a caught create error. */ +function isDuplicateGroupNameError(err: unknown): boolean { + if (!(err instanceof CtApiError) || err.status !== 400) return false; + const body = err.body; + if (!body || typeof body !== "object") return false; + const b = body as Record; + if (b.messageKey === DUPLICATE_GROUP_MESSAGE_KEY) return true; + const message = b.message; + return ( + typeof message === "string" && + /duplicate/i.test(message) && + /force flag/i.test(message) && + /group/i.test(message) + ); +} + +/** + * Append actionable guidance (#75) to an otherwise-formatted stop message when a group create + * failed on CT's same-name guard without the `allowDuplicateName` opt-in: point at adopting the + * existing group (the usual accident) or opting in (the rare intentional-duplicate case). Reuses + * `formatError`'s output verbatim — never forks the HTTP status/body formatting. + */ +function withDuplicateGroupGuidance(message: string, item: PlanItem): string { + return ( + `${message}\n` + + `Guidance: a group named like "${item.key}" likely already exists in ChurchTools. If it should ` + + `be managed by this tool, adopt it instead of creating a new one: ` + + `\`ct adopt group --env --key ${item.key}\` (find its id with \`ct get groups\`). ` + + `If two groups sharing this name is intentional, set \`allowDuplicateName: true\` on this ` + + `group's declaration and re-apply` + ); +} + export interface ExecuteDeps { client: Pick; state: State; @@ -109,7 +154,10 @@ export async function executePlan(plan: Plan, deps: ExecuteDeps): Promise("POST", spec.collectionPath, createBody); if (typeof res.id !== "number") { @@ -155,11 +203,18 @@ export async function executePlan(plan: Plan, deps: ExecuteDeps): Promise { }); }); +describe("allowDuplicateName create-time opt-in (#75)", () => { + it("is carried on the resource but kept out of managed fields (not diffed/adopted)", () => { + const { ct, resources } = createContext(); + ct.group({ key: "kids_2026_b", name: "Kids Elternabend 2026", groupTypeId: 2, allowDuplicateName: true }); + expect(resources[0]?.allowDuplicateName).toBe(true); + expect(resources[0]?.fields).toEqual({ name: "Kids Elternabend 2026", groupTypeId: 2 }); + }); + + it("defaults to undefined when not declared", () => { + const { ct, resources } = createContext(); + ct.group({ key: "kids", name: "Kids" }); + expect(resources[0]?.allowDuplicateName).toBeUndefined(); + }); + + it("does not trip the unknown-field warning", () => { + const spy = vi.spyOn(process.stderr, "write").mockImplementation(() => true); + try { + const { ct } = createContext(); + ct.group({ key: "kids_2026_b", name: "Kids Elternabend 2026", allowDuplicateName: true }); + expect(spy).not.toHaveBeenCalled(); + } finally { + spy.mockRestore(); + } + }); + + it("rejects the flag on a non-group type", () => { + const { ct } = createContext(); + expect(() => ct.campus({ key: "c", name: "C", allowDuplicateName: true } as never)).toThrow( + /allowDuplicateName.*only valid on a group/i, + ); + }); + + it("rejects a non-boolean value", () => { + const { ct } = createContext(); + expect(() => ct.group({ key: "g", name: "G", allowDuplicateName: "yes" as never })).toThrow( + /allowDuplicateName.*must be a boolean/i, + ); + }); + + it("`allowDuplicateName: false` is accepted and still kept out of fields", () => { + const { ct, resources } = createContext(); + ct.group({ key: "g", name: "G", allowDuplicateName: false }); + expect(resources[0]?.allowDuplicateName).toBe(false); + expect(resources[0]?.fields).toEqual({ name: "G" }); + }); +}); + describe("permission declarations", () => { it("collects groupRole / groupTypeRole with validated grants", async () => { const mod = (ct: ConfigContext) => { diff --git a/tests/execute.test.ts b/tests/execute.test.ts index d2b6d31..dd8d9a3 100644 --- a/tests/execute.test.ts +++ b/tests/execute.test.ts @@ -541,4 +541,183 @@ describe("executePlan", () => { expect(result.failed?.message).toContain("HTTP 403"); expect(result.failed?.message).toContain("no permission to create group types"); }); + + describe("allowDuplicateName / force create opt-in (#75)", () => { + it("sends force: true on the CREATE POST body when a create item opts in", async () => { + const state = emptyState("h"); + const { client, calls } = recorder({ "POST /groups": { id: 7 } }); + const plan: Plan = { + items: [ + { + type: "group", + key: "kids_2026_b", + id: null, + action: "create", + changes: [ + { field: "name", from: undefined, to: "Kids Elternabend 2026" }, + { field: "groupTypeId", from: undefined, to: 2 }, + ], + allowDuplicateName: true, + }, + ], + }; + const result = await executePlan(plan, { client, state, statePath: "s.json", save: noSave, now: fixedNow }); + expect(result.failed).toBeUndefined(); + expect(calls[0]).toEqual({ + method: "POST", + path: "/groups", + body: { name: "Kids Elternabend 2026", groupTypeId: 2, force: true }, + }); + // force is create-body only — never stored as a managed field. + expect(state.resources.kids_2026_b?.fields).toEqual({ + name: "Kids Elternabend 2026", + groupTypeId: 2, + }); + }); + + it("omits force from the CREATE POST body when not opted in", async () => { + const state = emptyState("h"); + const { client, calls } = recorder({ "POST /groups": { id: 7 } }); + const plan: Plan = { + items: [ + { + type: "group", + key: "kids_2026_a", + id: null, + action: "create", + changes: [{ field: "name", from: undefined, to: "Kids Elternabend 2026" }], + }, + ], + }; + await executePlan(plan, { client, state, statePath: "s.json", save: noSave, now: fixedNow }); + expect(calls[0]).toEqual({ + method: "POST", + path: "/groups", + body: { name: "Kids Elternabend 2026" }, + }); + }); + + it("never sends force on the UPDATE path, even if a stale item somehow carried the flag", async () => { + const state = emptyState("h"); + state.resources.team = { + type: "group", + id: 9, + key: "team", + fields: { name: "Team" }, + adoptedAt: "t", + updatedAt: "t", + }; + const { client, calls } = recorder(); + const plan: Plan = { + items: [ + { + type: "group", + key: "team", + id: 9, + action: "update", + changes: [{ field: "name", from: "Team", to: "Team A" }], + actual: { name: "Team" }, + allowDuplicateName: true, + }, + ], + }; + await executePlan(plan, { client, state, statePath: "s.json", save: noSave, now: fixedNow }); + expect(calls[0]).toEqual({ method: "PATCH", path: "/groups/9", body: { name: "Team A" } }); + }); + + it("appends adoption/opt-in guidance to the stop message on a duplicate-name 400 (messageKey) without the flag", async () => { + const state = emptyState("h"); + const client = { + request: async (): Promise => { + throw new CtApiError("POST /groups failed", 400, { + message: "Duplicate found. Use force flag to create group with same name.", + messageKey: "forbidden.duplicate.group", + translatedMessage: "Duplikat gefunden. Nutze das force Flag um die Gruppe trotzdem anzulegen.", + args: [], + errors: [], + }); + }, + }; + const plan: Plan = { + items: [ + { + type: "group", + key: "kids_2026_b", + id: null, + action: "create", + changes: [{ field: "name", from: undefined, to: "Kids Elternabend 2026" }], + }, + ], + }; + const result = await executePlan(plan, { client, state, statePath: "s.json", save: noSave, now: fixedNow }); + expect(result.failed?.key).toBe("kids_2026_b"); + // The shared formatter's output is preserved verbatim (HTTP status + body)... + expect(result.failed?.message).toContain("HTTP 400"); + expect(result.failed?.message).toContain("forbidden.duplicate.group"); + // ...with one guidance line appended, naming both remedies. + expect(result.failed?.message).toContain("ct adopt group "); + expect(result.failed?.message).toContain("--key kids_2026_b"); + expect(result.failed?.message).toContain("allowDuplicateName: true"); + }); + + it("also recognises the duplicate-group guard from the 400 message text alone (no messageKey)", async () => { + const state = emptyState("h"); + const client = { + request: async (): Promise => { + throw new CtApiError("POST /groups failed", 400, { + message: "Duplicate found. Use force flag to create group with same name.", + }); + }, + }; + const plan: Plan = { + items: [ + { type: "group", key: "kids_2026_b", id: null, action: "create", changes: [{ field: "name", from: undefined, to: "K" }] }, + ], + }; + const result = await executePlan(plan, { client, state, statePath: "s.json", save: noSave, now: fixedNow }); + expect(result.failed?.message).toContain("allowDuplicateName: true"); + }); + + it("does NOT append guidance when the create already opted in with allowDuplicateName", async () => { + const state = emptyState("h"); + const client = { + request: async (): Promise => { + throw new CtApiError("POST /groups failed", 400, { + message: "Duplicate found. Use force flag to create group with same name.", + messageKey: "forbidden.duplicate.group", + }); + }, + }; + const plan: Plan = { + items: [ + { + type: "group", + key: "kids_2026_b", + id: null, + action: "create", + changes: [{ field: "name", from: undefined, to: "K" }], + allowDuplicateName: true, + }, + ], + }; + const result = await executePlan(plan, { client, state, statePath: "s.json", save: noSave, now: fixedNow }); + expect(result.failed?.message).not.toContain("Guidance:"); + }); + + it("does NOT append duplicate-group guidance for an unrelated 400 (e.g. a validation error)", async () => { + const state = emptyState("h"); + const client = { + request: async (): Promise => { + throw new CtApiError("POST /groups failed", 400, { message: "name must not be empty" }); + }, + }; + const plan: Plan = { + items: [ + { type: "group", key: "kids", id: null, action: "create", changes: [{ field: "name", from: undefined, to: "" }] }, + ], + }; + const result = await executePlan(plan, { client, state, statePath: "s.json", save: noSave, now: fixedNow }); + expect(result.failed?.message).not.toContain("Guidance:"); + }); + }); }); diff --git a/tests/plan.test.ts b/tests/plan.test.ts index 092cd0d..7bfe94d 100644 --- a/tests/plan.test.ts +++ b/tests/plan.test.ts @@ -309,3 +309,28 @@ describe("group campus assignment (#21)", () => { expect(plan.items[0]?.changes).toEqual([]); }); }); + +describe("allowDuplicateName threading onto create items (#75)", () => { + it("carries the flag onto a fresh create item", () => { + const plan = computePlan( + [{ type: "group", key: "kids_b", fields: { name: "Kids" }, dependsOn: [], allowDuplicateName: true }], + stateOf(), + new Map(), + ); + expect(plan.items[0]).toMatchObject({ action: "create", allowDuplicateName: true }); + }); + + it("carries the flag onto a recreate create item (managed but vanished from ChurchTools)", () => { + const plan = computePlan( + [{ type: "group", key: "kids_b", fields: { name: "Kids" }, dependsOn: [], allowDuplicateName: true }], + stateOf(managedT("group", "kids_b", 9, { name: "Kids" })), + new Map(), // vanished: no actual entry + ); + expect(plan.items[0]).toMatchObject({ action: "create", note: "recreate", allowDuplicateName: true }); + }); + + it("is undefined on a create item when not declared", () => { + const plan = computePlan([desired("mainz", { name: "Mainz" })], stateOf(), new Map()); + expect(plan.items[0]?.allowDuplicateName).toBeUndefined(); + }); +});