Repository navigation
fix(dynamic): keep groupmembership stereotype wrappers when normalizing rulesets - #192
Merged
Merged
Conversation
…ng rulesets
stripCosmeticLabels dropped every dterm label, including
{ stereotype: ["groupmembership"] }. That label is part of the query:
it makes ChurchTools evaluate a group condition per person instead of
per membership row. Because apply PUTs the normalized ruleset, applying
a ruleset with a negated group condition silently turned
!(member of X) into a no-op (MHfsH in eqrm/ct-structure kept adding
unsubscribed people).
Verified read-only on prod (CT 3.137) via POST /churchquery/debug/export
over all 72 active rulesets: keeping only stereotype labels reproduces
every live result; stripping them changes 18.
…ls verbatim Review follow-ups on keeping groupmembership wrappers: - plan warns when the declared ruleset has fewer non-cosmetic dterm wrappers than the live one. Without it, upgrading turned a silent no-op into a PUT that strips every wrapper a pre-fix adopt missed. - isCosmeticLabel is an allowlist (string or title-only label); any other label key is kept rather than assumed decorative. - coerceScalars leaves a kept label verbatim, so a numeric-looking title is not retyped to a number on write-back; labels are copied, not shared. - q.memberOf emits the wrapper from the typed builder. - Handbook: neutral example with a portable group ref, and the recovery path for groups an earlier apply already stripped live (re-adopt cannot bring those back). Re-signed. - Fixture test asserts real wrapper counts instead of a vacuous every().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
stripCosmeticLabels(src/engine/dynamic.ts) treated everydterm: [label, expr]wrapper as cosmetic. A label withstereotype: ["groupmembership"]is not cosmetic: it makes ChurchTools evaluate the wrapped group condition per person ("is / is not a member of group X") instead of per membership row.applyPUTs the normalized ruleset, so applying one silently removed these wrappers. For a negated group condition that is fatal:!(member of X)stays true for anyone who has any other membership row, so it excludes no one.Evidence (read-only,
POST /churchquery/debug/export, CT 3.137)Change
isCosmeticLabelis an allowlist: string and{ title }-only labels are stripped; anything else (notablystereotype) is kept verbatim, its body normalized recursively. Idempotent.coerceScalarsleaves a kept label untouched (a title"2024"stays a string); labels are copied, not shared.ct planwarns when the declared ruleset has fewer stereotype wrappers than the live one (applying would remove N "stereotype" dterm wrapper(s)). Rulesets adopted before this fix lack them; without the warning, the first apply after upgrading would strip every wrapper still live.q.memberOf(node)emits the wrapper from the typed builder.dynamic-groups.md: normalization section, authoring example with a portable group ref, and recovery for groups an earlier apply already stripped live (re-adopt can't bring those back). Re-signed.Review
/code-review high: 10 findings, 8 fixed here (upgrade-path guard, recovery doc, DSL helper, numeric-title coercion, allowlist predicate, org names in docs/tests, vacuous fixture assertion, shared label reference). Refuted: "kept title diffs forever" — a declared title that differs from live is a real diff in a declarative config.Ran the build against dev (
ct plan --env dev, read-only) with a consumer config whose rulesets carry the wrappers: 44 updates, every one differing only by restoring wrappers that earlier applies had stripped; no removal warnings.Follow-up (consumers)
Re-adopt
rulesets/*.json(or add the wrappers) before applying with this version; the new plan warning lists every group affected.