Skip to content

Require N approvals before a change set is approved - #2668

Merged
pournasserian merged 8 commits into
v2from
2645-required-approvals
Oct 26, 2026
Merged

pournasserian merged 8 commits into
v2from
2645-required-approvals

Conversation

@pournasserian

@pournasserian pournasserian commented Oct 26, 2026 •

Copy link
Copy Markdown
Member

Closes #2645

What

The first half of RTE-05 (F-RTE #2421): a set in review needs N approvals, by N different people, before it is approved. N defaults to 1 and is configurable.

  • Core (FluentCMS.Core.ChangeSets):
    • ChangeSet.RequiredApprovals (1 to 10) and ChangeSet.Approvals. Each ChangeSetApproval names the person and the time.
    • EnterReview(n) moves a set into review, needing n approvals.
    • Approve(person, at) records an approval. A second approval by the same person is refused with approval_duplicate. The N-th approval moves the set to approved.
    • TransitionTo(Approved) from awaiting review is refused with approvals_needed, so nothing approves a set in review except its approvals. The move from checks running to approved (routing's auto-publish) is unchanged.
  • Directory: IChangeSetDirectory.ApproveAsync(changeSetId), and TransitionAsync(id, Approved) on a set in review goes through it.
    • Each approval appends change_set.approval_recorded. The one that completes the count also appends change_set.approved, in the same unit of work.
    • The move into review takes N from Approvals:Required (validated at start, 1 to 10, default 1).
    • Agents and engines are refused as before (MCP-06, ADR-0012). The refusal is logged by the same permission check.
  • Infrastructure: ChangeSets.RequiredApprovals (default 1, with a check constraint from 1 to 10), and the ChangeSetApprovals table keyed by set and person. One migration for each provider.
  • Desk:
    • The review panel shows "1 of 2 approvals" and who approved.
    • Approve is hidden from a person who already approved. Its form stays on the page, so a post from an older tab still reaches its handler.
    • An approval that does not complete the count keeps the reviewer on the set and says how many more it needs.
  • MCP: get_change_set_review lists requiredApprovals and the approvals, each with who and when.

Why

RTE-05 says required approvals default to 1 and are configurable (N). The epic's acceptance has "a set whose level requires 2 does not publish after one approval". Routing (the next half of RTE-05) sets N from a matched review rule, and the scorer (RTE-02) from the risk level. Both need the count and the approvals on the set first.

Choices worth a look:

  • Approvals never need clearing. A set reaches review once: it leaves only for approved or rejected, and a reopened set comes from checks failed. So all of a set's approvals belong to its one review, keyed by set and person.
  • The person who opened a set may approve it. M1 has no per-area approvers, and any administrator counts toward N (USR-02 and USR-03 are M2). On a one-person install a set could otherwise never publish.
  • Concurrent approvals serialise on the set's revision. Two people approving at the same moment write over the same revision. One of them gets change_set_conflict and approves again on the fresh page.
  • The option is Approvals:Required, not under Routing. Routing will pass its own N for a matched rule, and this is the fallback for everything else. The issue now says so.
  • approval_recorded is not a move in ChangeSetMoves, so a set's "state since" and the desk's queues are unchanged by a partial approval.

Follow-ups from the review, each in its own issue because this PR does not introduce them: #2670 (a person can still approve a set whose checks are running, through the move routing's auto-publish uses) and #2671 (Awaiting me still lists sets the reviewer already approved).

Tests

Unit:

  • A set needing 2 stays in review after one approval. A repeat by the same person is refused, a direct move to approved is refused with the count in the message, and a second person's approval approves the set. An approval after that is refused.
  • EnterReview takes only 1 to 10 and changes nothing otherwise.
  • The transition table test approves its awaiting review → approved row instead of moving it.
  • The set view shows the count and the approvers, and offers Approve only to someone who has not approved. Every action's form is still on the page.

Integration, each on SQLite and PostgreSQL:

  • With the default of 1, an approval approves the set and appends change_set.approval_recorded then change_set.approved.
  • With Approvals:Required at 2, the first approval leaves the set in review, a repeat is refused and stores nothing, and a second person approves it. Two approval rows are stored, and the events are recorded, recorded, approved.
  • An approval of an approved set or an unknown set is refused and appends nothing.
  • get_change_set_review returns the approval and the count to the agent.
  • The table-driven agent and transition tests still pass through the new path. DatabaseTests reverts and reapplies the new migration.
  • DeskQueueTests' check that every move appends exactly one known move now leaves out change_set.approval_recorded, as it already leaves out change_set.operation_*: an approval is recorded next to the move it completes, not as a move.

dotnet build -warnaserror and the full suite pass locally on SQLite and against a postgres:17 container. No new package and no new project reference.

Checklist

  • Linked to an issue a maintainer has confirmed
  • Targets v2
  • Tests or docs updated where behaviour changed
  • No tool-generated co-author trailers or footers

@pournasserian pournasserian added this to the M1 Governance core milestone Oct 26, 2026
@pournasserian pournasserian added type:feature New capability v2 FluentCMS v2 (v2 branch) area:routing Risk routing and sampling (RTE) labels Oct 26, 2026
@pournasserian
pournasserian merged commit 614a77c into v2 Oct 26, 2026
2 checks passed
@pournasserian
pournasserian deleted the 2645-required-approvals branch October 26, 2026 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:routing Risk routing and sampling (RTE) type:feature New capability v2 FluentCMS v2 (v2 branch)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Require N approvals before a change set is approved

1 participant