Skip to content

Add approve, reject, publish and revert to the set view - #2581

Merged
pournasserian merged 1 commit into
v2from
2578-review-actions
Oct 19, 2026
Merged

pournasserian merged 1 commit into
v2from
2578-review-actions

Conversation

@pournasserian

Copy link
Copy Markdown
Member

Closes #2578

What

The third slice of F-DSK (#2422): the review actions on the set view (DSK-03). A reviewer can approve, reject with a reason (CHG-08), publish an approved set and revert a published one (CHG-06). Each action is saved when it is made.

  • Review panel on /sets/{id}, offering only what the state allows:
    • Awaiting review: Approve and Reject.
    • Checks failed: Reject.
    • Approved: Publish.
    • Published: Revert.
    • Any other state: no action, and the panel says why.
  • Reject opens the existing Dialog with a required reason. The directory refuses a blank one, and the stored reason shows on the view.
  • Revert is one click (S3). It opens the set that undoes this one, with the rationale "Revert change set {id}: {rationale}", and redirects to it. The new set is reviewed like any other.
  • Each action is a posted form with an anti-forgery token on the static page. It calls TransitionAsync, RejectAsync, PublishAsync or RevertAsync on IChangeSetDirectory, which appends the event with the signed-in person as initiator. Then the page shows the set as it is now, with a status line or the refusal.
  • Moved-on guard: every form carries Seen, the id of the last move event of the set the page showed. If the set has moved since, the action is refused with "The set has moved on since this page was shown." Every action's form is always on the page, hidden when not offered, so a post from an older page reaches its handler and gets that message instead of a 400.

Why

  • Forms on the static page, not circuit handlers. ADR-0006 asks that review decisions survive a lost circuit. A posted form needs no circuit, and the request carries the signed-in person, so ICurrentInitiator needs no circuit-side variant.
  • The moved-on guard. Without it, a page left open could approve a set that was reopened, changed and resubmitted in the meantime. The last move event is already read for the page's times, so the guard costs no query.
  • Approve moves the set to approved. Approvals per reviewer and N approvals (RTE-05) are F-RTE's next slice. Approve then records an approval instead.
  • Not in this PR: edit in place (CHG-07, needs F-FRM), the sample selector (RTE-04), "propose a rule" after a rejection or revert (RUL-08, needs F-RUL), and keyboard shortcuts (next slice).

Tests

  • bUnit ChangeSetViewTests:
    • For each of the eight states, the panel offers exactly the allowed actions and explains itself. All four forms are on the page, the ones not offered are hidden, and each Seen is the last move's id.
    • The reject dialog has a required reason.
  • Integration, host:
    • A reviewer approves and then publishes a set. Each page shows the new state and the next action. Both events name the reviewer, and the set moves to recently published.
    • A blank reason is refused and leaves the set awaiting review. A real reason rejects the set and shows on the view, and no action is left.
    • Revert redirects to a new open set that reverts the original and links to it.
    • A post with the Seen of a page shown before another reviewer approved the set is refused, and nothing is appended.
    • An anonymous post goes to sign-in, and a post without the anti-forgery token answers 400 and changes nothing.
    • Awaiting review offers approve and reject, checks failed offers reject, and checks running offers nothing.

dotnet build -warnaserror and the full suite pass locally against a postgres:17 container as well as SQLite. 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 19, 2026
@pournasserian pournasserian added type:feature New capability v2 FluentCMS v2 (v2 branch) area:desk labels Oct 19, 2026
@pournasserian
pournasserian merged commit ed00e18 into v2 Oct 19, 2026
2 checks passed
@pournasserian
pournasserian deleted the 2578-review-actions branch October 19, 2026 04:42
pournasserian added a commit that referenced this pull request Oct 20, 2026
Closes #2624

## What

Follow-ups from the independent review of #2581 (F-DSK, #2422).

- **Each review action is taken only from a state the panel offers it
in,** whatever a post claims:
  - submit from open;
  - approve from awaiting review;
  - publish from approved;
  - reject from awaiting review, checks failed or approved;
  - revert from published.

A post for any other state gets the "moved on" refusal and changes
nothing, so a set can't be approved past its checks.
- **Revert is idempotent.** Before opening a revert, the page looks for
a revert of this set still in progress (open, checks running, checks
failed, awaiting review or approved). If there is one, a second click
lands on it. `ChangeSetListQuery.Reverts` filters the list by the set a
revert undoes.
- Two posts at the very same instant can still both open a set. A unique
index belongs to the change sets lane, and #2646 asks for it.
- **The diff island reads through a scope of its own.** The new
PostgreSQL review tests exposed it: on PostgreSQL an async read can
finish at once, so the set view sometimes renders `ChangeSetItems`
before its own reads are done. The island's prerendered diff then ran on
the same request `DbContext` and failed the page with a 500 or a stall.
This is the #2642 rule applied to the last component that loads data
beside its page. It is also the likely cause of the intermittent
`SignedInPageTests` hang on v2.

## Tests

- `ReviewActionTests` runs on SQLite and PostgreSQL.
- The approve test asserts the stored state is approved.
- Each of these posts carries the set's current `Seen` and is refused,
leaving the state unchanged:
- an approve for a set in checks running, checks failed, open or
published;
  - a publish from awaiting review;
  - a revert from approved;
  - a submit from checks failed;
  - a reject from open.
- A second revert of the same published set lands on the first, and only
one revert set exists.
- Every action's form is refused without the anti-forgery token, and an
anonymous post of each goes to sign-in.
- Without the guards, the refusal and revert tests fail.

`dotnet build -warnaserror`, the unit and bUnit suite and the review,
list and queue tests on SQLite and PostgreSQL pass locally. No new
package and no new project reference.

## Checklist

- [x] Linked to an issue a maintainer has confirmed
- [x] Targets `v2`
- [x] Tests or docs updated where behaviour changed
- [x] No tool-generated co-author trailers or footers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:feature New capability v2 FluentCMS v2 (v2 branch)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Review actions on the set view: approve, reject with a reason, publish and revert

1 participant