Skip to content

fix(archival): place the Awb legal hold that has never once been placed - #693

Merged
rubenvdlinde merged 1 commit into
developmentfrom
fix/legal-hold-case-lookup
Aug 2, 2026
Merged

fix(archival): place the Awb legal hold that has never once been placed#693
rubenvdlinde merged 1 commit into
developmentfrom
fix/legal-hold-case-lookup

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

The exposure

BezwaarLegalHoldListener is the control that suspends a case from destruction while it is under bezwaar or beroep. It has never once placed a hold. Cases under an Awb proceeding have been destruction-eligible throughout.

On the development instance: 22 bezwaar proceedings, 0 legal holds.

Root cause

resolveCaseObject() and caseIdOfObjection() both called $objectMapper->findByUuid($uuid).

MagicMapper has no findByUuid() and no __call(), so every invocation raised a fatal \Error. Each call site wraps the lookup in catch (\Throwable), which swallowed it and returned null / ''. applyHold() then hit an early return that was completely silent.

The result is the worst shape a compliance control can have: no hold, no exception, no log line, and nothing that distinguishes it from a healthy listener.

Verified by reflection in the running container, with a positive control rather than a grep:

class_exists(OCA\OpenRegister\Db\MagicMapper): true
  find         hasMethod=true     <- positive control
  findAll      hasMethod=true     <- positive control
  findByUuid   hasMethod=false
  __call       hasMethod=false

procest#692 fixed the slug comparison in the guard above this. This is a second, independent defect downstream of that guard — which is why the end-to-end behaviour did not change after #692.

The fix

Both call sites now use find(identifier:, _rbac: false, _multitenancy: false).

RBAC and multitenancy are disabled deliberately, not incidentally: this runs inside an event handler where there is no session user and no active organisation, so an organisation-scoped read would match nothing and silently reopen the same hole.

Both swallowing catches now log a warning, and the previously-silent "collaborator or case unresolved" early return in applyHold() now records why a hold was not applied.

Positive control (live instance)

Both fixed call sites are exercised.

Place — create case, then bezwaar referencing it:

{"legalHold": {"active": true,
  "reason": "Awb-procedure (bezwaar) geregistreerd — archivering opgeschort",
  "placedBy": "admin", "placedDate": "2026-08-01T18:15:41+00:00"}}

Release — create bezwaarDecision (this path goes through caseIdOfObjection(), the second findByUuid() site):

{"legalHold": {"active": false, "history": [{"active": true,
  "placedDate": "2026-08-01T18:15:41+00:00",
  "releasedBy": "admin", "releasedDate": "2026-08-01T18:16:43+00:00",
  "releaseReason": "Awb-procedure (bezwaarDecision) afgehandeld — archivering hervat"}]}}

Before this change the same sequence produced no retention data at all. The new hold is the only one in a table of 348 cases — which is simultaneously the positive control and the negative control.

Checks

phpcs clean on the touched file (2 pre-existing //end try errors my longer catch blocks triggered are fixed here).

Not covered

  • No unit test yet — the defect is a missing method on a collaborator, which a mock-based test would not have caught (a getMockBuilder(stdClass)->addMethods(['findByUuid']) double would have made the broken call pass). A structural check that asserts the called method exists on the real class would be the right regression guard; worth a follow-up.
  • The 21 historical bezwaar proceedings that never got a hold are not backfilled by this PR. They need a remediation sweep — filing separately.

Refs procest#692

BezwaarLegalHoldListener resolved the case via $objectMapper->findByUuid().
MagicMapper has no findByUuid() and no __call(), so every invocation raised a
fatal \Error that the surrounding `catch (\Throwable)` swallowed and turned into
a `return null` — after which applyHold() returned silently. The control was
dead for its entire life: no hold, no exception, no log line.

Verified by reflection in the running container rather than by grep: on
OCA\OpenRegister\Db\MagicMapper, find() and findAll() are present, findByUuid()
and __call() are not.

The exposure this leaves is not cosmetic. Cases under bezwaar/beroep are
supposed to be suspended from destruction; instead they stayed
destruction-eligible. On the development instance 22 bezwaar proceedings had
produced exactly zero legal holds.

Both call sites now use find(identifier:, _rbac: false, _multitenancy: false).
RBAC and multitenancy are disabled deliberately: this runs inside an event
handler with no session user and no active organisation, so an
organisation-scoped read would match nothing and silently reopen the same hole.

The two swallowing catches now log a warning, and the "collaborator or case
unresolved" early return in applyHold() — previously completely silent — logs
the reason a hold was not applied. A dead compliance control must not be able
to look identical to a healthy one again.

Positive control on the live instance (both fixed call sites):
  - create case + bezwaar -> case._retention.legalHold.active = true
  - create bezwaarDecision -> active = false, with the placement recorded in
    legalHold.history (this exercises caseIdOfObjection(), the second
    findByUuid() site)
Before this change that same sequence produced no retention data at all, and
the new hold is the only one in a table of 348 cases.

Refs procest#692
@rubenvdlinde
rubenvdlinde merged commit d8fc894 into development Aug 2, 2026
4 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/legal-hold-case-lookup branch August 2, 2026 07:20
rubenvdlinde added a commit that referenced this pull request Aug 2, 2026
…laced (#695)

BezwaarLegalHoldListener was inert for its entire life (#693): it called
MagicMapper::findByUuid(), which does not exist, so every invocation raised a
fatal \Error that the surrounding catch (\Throwable) swallowed — no hold, no
exception, no log line. #693 fixes it forward; this repairs the backlog.

Adds `occ procest:legal-hold:backfill`:

- Dry-run by DEFAULT; writes only with --apply.
- Idempotent — a case already carrying an active hold is skipped, so it is
  safe to re-run and safe to run after the listener is live.
- Additive only. It places holds; it never releases one, and never deletes or
  range-updates anything.
- Only cases with at least one OPEN proceeding are held. A proceeding with a
  terminal decision is concluded, and back-dating a hold onto it would
  fabricate retention history rather than repair it.
- The reason string names the remediation and states the hold is placed at
  repair time, not back-dated, so it can never be mistaken in an audit for a
  contemporaneous hold.
- beroep is treated as hold-opening even though the listener itself does not
  act on it: an appeal suspends destruction exactly as an objection does.

Two failure modes are deliberately made loud rather than silent, because
silence is what hid the original defect: a schema scan that throws is reported
as `[scan failed]` instead of being swallowed into an empty result, and the
count of already-concluded proceedings is printed per schema. That counter
caught a real bug during development — an empty uuid had silently disabled the
open-proceeding filter, which would have held every concluded case too.

Verified live on the development instance: 12 live bezwaar + 1 beroep scanned,
2 concluded correctly excluded, 1 case held, and the hold read back from
storage. A second --apply run wrote nothing (held=0, already held=2).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant