Skip to content
This repository was archived by the owner on May 29, 2026. It is now read-only.

Specs: entity-relation-grondslagen (#1435)#1494

Merged
rjzondervan merged 2 commits into
developmentfrom
feat/1435/entity-relation-grondslagen
May 12, 2026
Merged

Specs: entity-relation-grondslagen (#1435)#1494
rjzondervan merged 2 commits into
developmentfrom
feat/1435/entity-relation-grondslagen

Conversation

@rjzondervan

Copy link
Copy Markdown
Member

Summary

Adds the OpenSpec change directory for entity-relation-grondslagen — extending EntityRelation with an optional bases JSON column so the anonymise endpoint can persist legal-basis (Woo Art. 5 grondslag) references per entity. Bases are stripped from the payload before forwarding to OpenAnonymiser, so the upstream service contract is unchanged.

Backwards-compatible: callers that omit bases see identical behaviour.

Tightly scoped — this PR is specs only, no implementation. It also does not retrofit the broader EntityRelation surface, which is currently uncovered by an OpenSpec capability.

Refs: #1435
Pair: ConductionNL/docudesk#135 — DocuDesk's anonymisation-grondslagen-summary (hard dep) and anonymisation-grondslagen-and-prohibition-gate (soft dep) depend on this.

Test plan

  • Review openspec/changes/entity-relation-grondslagen/{proposal,design,tasks}.md and the entity-relation-grondslagen/spec.md delta
  • Confirm Requirements / Scenarios match the DocuDesk-side consumer expectations in PR check isset items type #135
  • openspec validate (when the implementation PR lands)

Adds the spec set for extending EntityRelation with an optional bases
JSON column. The anonymise endpoint accepts bases per entity, persists
them on the matching relation row, and strips them from the payload
before forwarding to OpenAnonymiser. Backwards-compatible — callers
that omit bases see identical behaviour.

Tightly scoped: this does not retrofit the broader EntityRelation
surface (currently uncovered by an OpenSpec capability).

Refs: #1435
Pair: ConductionNL/docudesk#135 (anonymisation-grondslagen-summary +
anonymisation-grondslagen-and-prohibition-gate hard/soft dep).
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 0634d13

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 153/153
npm ✅ 598/598
PHPUnit
Newman ⏭️
Playwright ⏭️

Quality workflow — 2026-05-12 09:20 UTC

Download the full PDF report from the workflow artifacts.

@WilcoLouwerse WilcoLouwerse left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(inline-only comment review — verdict follows in a separate REQUEST_CHANGES review)

Comment thread openspec/changes/entity-relation-grondslagen/tasks.md

@WilcoLouwerse WilcoLouwerse left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 blockers require fixes — no audit-trail requirement for bases writes (the feature exists for Woo compliance; without ADR-022 audit linkage it defeats its own purpose), and no authorization requirement for who may set bases (any authenticated user could fabricate audit-trail entries without an ADR-005/ADR-023 anchor in the spec). Four concerns (spec ↔ tasks.md validation contradiction, retry idempotency scenario lacks a Requirement, no Woo Art. 5 / canonical UUID references, and spec format vs writing-specs.md — repo-wide drift, surfaced for visibility). Two minor polish items. The strip-before-forward contract, backwards-compatibility story, and persistence-before-forward ordering are all well-specified — main gaps are the missing compliance/security requirements that this feature literally exists to provide. CI Vue Quality (eslint) and PHPUnit are failing but unrelated to this spec-only PR.

Blockers:
- B1: add audit-trail Requirement (ADR-022 / Woo compliance). Every
  set/update of bases produces an audit entry with previousBases,
  newBases, actor UID (per ADR-005, not display name), timestamp, and
  row identifier. Reads do not produce audit entries.
- B2: add authorization-inheritance Requirement (ADR-005 / ADR-023).
  Bases writes inherit the anonymise endpoint's existing per-object
  authorization; no extra check is added in this change, and the
  absence is documented explicitly so reviewers do not mistake it
  for oversight.

Concerns:
- C1: add endpoint-shape-validation Requirement that distinguishes
  endpoint-layer shape check (rejects non-array / non-string elements
  with 400) from mapper-layer content acceptance (any string array
  persisted verbatim). Reconciles spec ↔ tasks.md.
- C2: add retry-idempotency Requirement. Three caller intents are
  distinguished: field absent (reuse persisted), present-null (clear,
  audit-logged), present-empty-array (set to [], audit-logged).
- C3: add Notes section with Woo Art. 5 references and the six
  canonical DocuDesk base seed slugs.

Minors:
- M1: apply MUST to THEN/AND assertions throughout all Scenarios.

Tasks: update 3.1 to clarify two-layer shape vs content split; add
3.5 (audit-trail wiring), 3.6 (retry semantics), 3.7 (auth confirm);
add corresponding 4.4–4.7 test tasks.

Skipped: C4 (spec-format vs writing-specs.md) and M2 (tasks.md
spec_ref / files / acceptance_criteria) — both flag repo-wide format
drift that Wilco explicitly says does not block merge and needs
maintainer coordination.
@rjzondervan

Copy link
Copy Markdown
Member Author

@WilcoLouwerse — thanks for the thorough review. Addressed in 61caaf72a:

Blockers

  • 🔴 B1 — added Requirement "bases writes MUST be recorded in OpenRegister's audit trail (ADR-022 / Woo compliance)" with three Scenarios (first-time set, update with previous → new transition, reads do not audit). tasks.md 3.5 wires it through OR's existing immutable-audit-trail subsystem.
  • 🔴 B2 — added Requirement "bases writes MUST inherit the anonymise endpoint's existing authorization (ADR-005 / ADR-023)" explicitly stating that no extra group/role check is added in this change, and that the absence is intentional (not oversight). Two Scenarios (unauthorized rejected, authorized succeeds). tasks.md 3.7 confirms during implementation.

Concerns

  • 🟡 C1 — added Requirement "endpoint MUST validate the SHAPE… but MUST NOT validate the CONTENT" with three Scenarios distinguishing endpoint-layer shape rejection (HTTP 400 with offending entity index) from mapper-layer content acceptance. tasks.md 3.1 reworded to match the two-layer split.
  • 🟡 C2 — added Requirement "A retry that omits bases MUST reuse the persisted values" with three Scenarios covering the three caller intents: absent → reuse, present-null → clear (audit-logged), present-[] → set empty (audit-logged). tasks.md 3.6 implements.
  • 🟡 C3 — added ## Notes section with Woo Art. 5 sub-article references and a table of the six canonical DocuDesk seed slugs (persoonsgegevens, bijzondere-persoonsgegevens, strafrechtelijk, bedrijfs-fabricagegegevens, onevenredige-benadeling, nationale-veiligheid). Notes also state that consumers MUST handle unresolvable UUIDs gracefully.

Minor

  • 🟢 M1 — applied MUST to all THEN/AND assertions in every Scenario.

Skipped (intentionally)

  • 🟡 C4 (spec format vs writing-specs.md) and 🟢 M2 (spec_ref/files/acceptance_criteria on tasks) — both flag repo-wide drift that you marked as not blocking and needing maintainer coordination on whether OpenRegister adopts the global convention (and retrofits existing specs) or writing-specs.md is amended. Happy to address in a follow-up once that decision is made.

New test tasks 4.4–4.7 cover the new behaviours (shape validation, audit trail, retry, authorization). Ready for re-review.

@WilcoLouwerse WilcoLouwerse left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 prior findings fully resolved in 61caaf72B1 audit trail, B2 authorization, C1 two-layer validation, C2 retry idempotency, C3 Woo Art. 5 notes. C4 and M2 remain deferred (non-blocking per prior review). Newman CI is non-required.

@rjzondervan rjzondervan merged commit e081043 into development May 12, 2026
1 check failed
@rjzondervan rjzondervan deleted the feat/1435/entity-relation-grondslagen branch May 12, 2026 11:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants