m/paliad
1
0
Fork 0
Code Issues 146 Pull Requests 8 Actions 1 Packages Projects Releases Wiki Activity

docs(approvals): t-paliad-216 — fold m's decisions, rewrite §3 implementation

Browse Source 
§0a captures m's locked picks across all 8 questions. Two divergences from
inventor recommendations reshape the model:

- Q4: hybrid — approver edits the proposed values (counter-payload) AND/OR
  leaves free-text in decision_note. Adds counter_payload jsonb column.
- Q6/Q7: the counter is treated as a NEW pending approval_request authored
  by the approver, not an "edit and resubmit" CTA on the requester side.
  Original requester sees the old row as changes_requested ("Abgelehnt mit
  Vorschlag") and the new row as pending — they can approve it themselves
  if eligible (they're no longer the requested_by). 4-Augen still holds.

§3 implementation sketch rewritten: SuggestChanges atomically closes the
old row, applyRevert's the entity, spawns a new pending row with
counter_payload as payload + previous_request_id linking back, re-applies
the counter via write-then-approve, emits both *_approval_changes_suggested
and *_approval_requested events. Migration 103 adds the CHECK value plus
counter_payload jsonb + previous_request_id FK + index. Slice plan trimmed
to backend / frontend-modal / Verlauf-integration.
This commit is contained in:
mAi
2026-05-20 09:28:08 +02:00
parent 2fa47278ce
commit c01f3f2db8
1 changed files with 141 additions and 100 deletions
Download Patch File Download Diff File Expand all files Collapse all files

241
docs/design-approval-suggest-changes-2026-05-19.md
View File

@@ -10,11 +10,35 @@
## 0. TL;DR
Add a fourth action **"Änderungen vorschlagen"** ("Suggest changes") to the approval flow, alongside Approve / Reject / Revoke. Use case: the approver doesn't want to accept the proposed change as-is, but doesn't want to reject outright — they want the requester to tweak something and re-submit.
Add a fourth action **"Änderungen vorschlagen"** ("Suggest changes") to the approval flow, alongside Approve / Reject / Revoke. Use case: the approver doesn't want to accept the proposed change as-is, but doesn't want to reject outright — they edit the proposed values into a counter-proposal and submit it back into the same approval flow.
Pattern in the wild: GitHub PR "request changes". The reviewer leaves a note explaining what they want changed; the author edits, force-pushes, the reviewer re-reviews.
**Mental model (m, 2026-05-19):** suggest-changes is not "ping the requester to fix it" — it's the approver **authoring a counter-proposal** that gets re-injected into the approval flow as a fresh `pending` row. The original requester (now potentially an eligible approver of the counter, since they're no longer the requested_by) sees:
- the **old row** in their /inbox as `changes_requested` ("Abgelehnt mit Vorschlag" / "Declined with changes") — historical record of their original attempt;
- the **new row** in /inbox as `pending` — the counter, which they can approve, reject, revoke (n/a, not theirs), or suggest changes back on. Everyone else eligible sees the new row too. 4-Augen still holds: the counter's requested_by (the approver who suggested it) cannot self-approve.
Conceptually this is **a soft-reject with a hint**. The proposed mutation is undone (entity reverts to pre_image, same as Reject), the approval_requests row terminates in a new status `changes_requested`, and the approver's note travels with the row. The requester sees the row in /inbox under `a_role=self_requested` with status `changes_requested` + the note + an "Edit + resubmit" CTA that opens the entity edit form pre-populated with the original payload. Saving the form fires a fresh `Submit*` → new `approval_requests` row → new `pending` cycle. The old row is linked from the new one via `previous_request_id` FK so the audit chain is reconstructable.
Click flow:
1. Approver opens an editable modal on the pending row showing the requester's proposed values. Edits any field. Writes a free-text note ("Bitte den Termin um 9:00 statt 8:00, weil der Raum sonst kollidiert").
2. POST `/api/approval-requests/{id}/suggest-changes` with `{note, counter_payload}`.
3. Server, in one tx: closes the old row (`changes_requested`, `decision_note=note`), reverts the entity from `pre_image`, then immediately inserts a **new** `pending` approval_requests row authored by the approver with `payload=counter_payload`, re-applies the counter to the entity, marks `pending_request_id` to the new row, emits two events (`*_approval_changes_suggested` + `*_approval_requested`). `previous_request_id` FK links new → old for chain traversal.
The pending audience for the new row is the same as any fresh `Submit*` — the existing notification + visibility plumbing handles it without special-casing.
---
## 0a. m's decisions (2026-05-19)
| # | Header | m picked | Reasoning note (when different from recommendation) |
|---|---|---|---|
| Q1 | State machine | **(a) New status `changes_requested`.** | As recommended. |
| Q2 | Entity state | **(a) Reverts to pre_image, same as Reject.** | As recommended. The counter is then re-applied in the same tx by the new approval row's write-then-approve cycle. |
| Q3 | Chain depth | **(a) Yes, across chained rows.** | As recommended. |
| Q4 | Note shape | **Hybrid: approver can edit the proposed values (counter-proposal) AND/OR leave free-text in `decision_note`.** | Differs from (a). Inventor picked free-text-only; m's twist: the suggestion should ALSO carry concrete edits. This adds a `counter_payload jsonb` column on `approval_requests` and turns "suggest-changes" into an action that authors a real counter-proposal, not just a hint. |
| Q5 | Surface | **(a) /inbox only — v1.** | As recommended. Email + entity-detail badge are Phase 2. |
| Q6 | Requester actions | **Different model: the counter is a NEW pending approval_request row, not an "edit + resubmit" CTA on the requester side.** | Differs from (a). m's reframing: instead of routing back to the requester to act on, the suggestion IS the next request. Original requester sees the old row as `changes_requested` (status pill "Abgelehnt mit Vorschlag" or similar). Original requester then sees the NEW row in /inbox like any pending — and **may approve it themselves**, because they are no longer the row's requested_by (the suggesting approver is). Everyone else eligible sees it too. Cleaner workflow, removes the "edit-and-resubmit CTA" from the requester role entirely. |
| Q7 | Notifications | **(b) Notify all eligible approvers + the original requester for the NEW pending row.** | Consistent with Q6. The counter is a fresh `pending` request, so the existing Submit*-notification audience applies. The original requester needs the ping because they're now an eligible approver of the counter — no special-case path. |
| Q8 | Audit shape | **(a) New event_type `*_approval_changes_suggested` per entity.** | As recommended. The new row also emits a normal `*_approval_requested` event, so the Verlauf chronology naturally captures the chain. |
The decisions above lock the design. §3 has been rewritten to reflect them; §2 (open questions) is retained as the historical record of what was open before the decisions.
---
@@ -109,7 +133,7 @@ Recommend (a). Each transition gets its own event row — that's how the existin
---
## 3. Implementation sketch (if recommendations stand)
## 3. Implementation sketch (decisions-locked, see §0a)
### 3.1 Migration `103_approval_suggest_changes.up.sql`
@@ -121,7 +145,15 @@ ALTER TABLE paliad.approval_requests
ADD CONSTRAINT approval_requests_status_check
CHECK (status IN ('pending', 'approved', 'rejected', 'revoked', 'superseded', 'changes_requested'));
-- 2. Add previous_request_id FK for chain reconstruction.
-- 2. Add counter_payload — the approver's edited values, becomes the
-- `payload` of the NEW pending row spawned in the same tx as the
-- suggest-changes call. Stored on the OLD (now changes_requested) row
-- too so the audit chain can show "approver edited X, Y, Z" without
-- joining to the next row.
ALTER TABLE paliad.approval_requests
ADD COLUMN counter_payload jsonb NULL;
-- 3. Add previous_request_id FK so the new row links back to its origin.
ALTER TABLE paliad.approval_requests
ADD COLUMN previous_request_id uuid NULL
REFERENCES paliad.approval_requests(id) ON DELETE SET NULL;
@@ -131,86 +163,94 @@ CREATE INDEX approval_requests_previous_idx
WHERE previous_request_id IS NOT NULL;
```
`.down.sql`: drop the index + column, restore the original CHECK (which would reject any rows currently in `changes_requested` — the down would fail if we've already used the status; that's normal for breaking-change downs).
`.down.sql`: drop the index + columns, restore the original CHECK (would reject existing `changes_requested` rows — that's normal for a breaking-change down).
### 3.2 Service layer
Add to `internal/services/approval_service.go`:
`SuggestChanges` is the only new public method on `ApprovalService`. It runs in **one transaction** and does five things:
```go
// SuggestChanges flips a pending request to 'changes_requested', reverts
// the entity from pre_image (same as Reject), and emits the
// *_approval_changes_suggested event. Runs in its own transaction.
const RequestStatusChangesRequested = "changes_requested"
var ErrSuggestionRequiresChange = errors.New("suggestion_requires_change")
// SuggestChanges closes the pending request as `changes_requested`,
// reverts the entity, then immediately inserts a new pending
// approval_request authored by the caller carrying `counterPayload` as
// its new payload. The new row enters the standard pending flow — anyone
// eligible (including the original requester) can approve, reject,
// suggest-changes-again, etc.
//
// Authorization: caller must satisfy canApprove (same gate as Approve /
// Reject — peer / admin_override / derived_peer). The note is required
// (we enforce non-empty server-side; a suggestion without text is
// indistinguishable from a reject).
func (s *ApprovalService) SuggestChanges(ctx context.Context, requestID, callerID uuid.UUID, note string) error {
if strings.TrimSpace(note) == "" {
return ErrSuggestionNoteRequired
}
return s.decide(ctx, requestID, callerID, RequestStatusChangesRequested, note)
// Authorization: caller satisfies canApprove on the OLD row (same gate
// as Approve / Reject). For the NEW row, the caller is the requested_by
// — self-approval is blocked by the standard 3-layer guard. Deadlock
// check (qualified-approver-exists-other-than-caller) runs on the new
// row to avoid spawning an unapprovable request.
//
// counterPayload must differ from the old row's payload OR a non-empty
// note must be present. A no-op suggest (same values, no note) is
// indistinguishable from "I have no opinion" and gets rejected with
// ErrSuggestionRequiresChange.
func (s *ApprovalService) SuggestChanges(
ctx context.Context,
requestID, callerID uuid.UUID,
counterPayload []byte, // jsonb-marshaled
note string,
) (newRequestID *uuid.UUID, err error) {
// 1. Begin tx, lock old row, validate status=pending + canApprove.
// 2. Validate: counterPayload differs from old payload OR note != "".
// 3. Update old row: status='changes_requested', decided_by=callerID,
// decision_note=note, counter_payload=counterPayload.
// 4. applyRevert on the entity (uses old row's pre_image).
// 5. Deadlock-check on the new row's required_role + projectID,
// excluding callerID.
// 6. INSERT new approval_requests row: requested_by=callerID,
// pre_image=<entity-state-as-just-reverted> (= old.pre_image),
// payload=counterPayload, required_role=old.required_role,
// lifecycle_event=old.lifecycle_event, entity_type=old.entity_type,
// entity_id=old.entity_id, status='pending',
// previous_request_id=requestID.
// 7. Re-apply the new payload to the entity (write-then-approve):
// apply the counter_payload's field updates + mark
// approval_status='pending' + pending_request_id=newRequestID.
// 8. Emit *_approval_changes_suggested project_events row
// (metadata: note, counter_payload diff vs original).
// 9. Emit *_approval_requested project_events row for the new
// request (same shape Submit* normally emits).
// 10. Commit.
}
```
Extend `decide()` to handle the new final status:
Steps 6 + 7 reuse the existing `Submit*` plumbing structurally — the cleanest implementation factors out an "insert approval row + apply payload to entity" helper that both `Submit*` and `SuggestChanges` call. **decide()** does not need to know about `changes_requested` because suggest-changes is not a decision-kernel transition — it's its own end-to-end action.
```go
case RequestStatusChangesRequested:
kind, err := s.canApprove(ctx, tx, callerID, req)
if err != nil {
return err
}
decisionKind = kind
```
And in the lifecycle outcome switch, treat `RequestStatusChangesRequested` like `RequestStatusRejected` (call `applyRevert`).
Audit verlaufKind: `"changes_suggested"`.
New constants:
```go
const RequestStatusChangesRequested = "changes_requested"
var ErrSuggestionNoteRequired = errors.New("suggestion_note_required")
```
### 3.3 Resubmit linking
The new field `previous_request_id` is set by the entity service's `Submit*` path when it inserts the new `approval_requests` row. Detection: look up the entity's most-recent `changes_requested` request for the same `(entity_type, entity_id, lifecycle_event)` and link to it.
```go
// In SubmitUpdate (etc.) — before INSERT new approval_requests row:
var prevID *uuid.UUID
err := tx.Get(&prevID, `
SELECT id FROM paliad.approval_requests
WHERE entity_type = $1 AND entity_id = $2 AND lifecycle_event = $3
AND status = 'changes_requested'
ORDER BY decided_at DESC LIMIT 1`, entityType, entityID, lifecycleEvent)
// then on the INSERT: pass previous_request_id = prevID (may be NULL).
```
Soft chain. If nothing matches, FK stays NULL — that's fine (this is just a normal first-submission).
### 3.4 HTTP layer
### 3.3 HTTP layer
```
POST /api/approval-requests/{id}/suggest-changes
Body: {"note": "..."}
Returns: 200 {} on success
Body: {
"counter_payload": { ...same shape as Submit*'s payload... },
"note": "free-text explanation, optional iff counter_payload differs from original"
}
Returns: 200 { "new_request_id": "uuid" }
Errors:
400 "note_required" — empty note
403 "self_approval_blocked" — caller == requester
403 "not_authorized" — caller doesn't satisfy canApprove
404 — request not found / not visible
409 "request_not_pending" — already decided
400 "suggestion_requires_change" — counter_payload == old payload AND note empty
400 "invalid_counter_payload" — schema validation failure
403 "self_approval_blocked" — caller == old row's requested_by
403 "not_authorized" — caller doesn't satisfy canApprove
404 — request not found / not visible
409 "request_not_pending" — old row already decided
409 "no_qualified_approver" — deadlock on the new row (only caller is eligible)
```
Register in `internal/handlers/handlers.go` alongside the existing three.
Register in `internal/handlers/handlers.go` alongside the existing three:
### 3.5 Frontend
```go
protected.HandleFunc("POST /api/approval-requests/{id}/suggest-changes", handleSuggestChangesApprovalRequest)
```
`frontend/src/client/views/shape-list.ts` — add a fourth button to the pending-row action group:
### 3.4 Frontend
`frontend/src/client/views/shape-list.ts` — extend the pending-row action group to four buttons:
```ts
actions.appendChild(approvalActionBtn("approve", detail));
@@ -219,65 +259,66 @@ actions.appendChild(approvalActionBtn("reject", detail));
actions.appendChild(approvalActionBtn("revoke", detail));
```
Update the `action` union type to include `"suggest_changes"`. The disabled-reason logic is identical to approve/reject (`viewer_can_approve` gate).
The `action` union type gains `"suggest_changes"`. Disabled-reason logic is identical to approve/reject (`viewer_can_approve` gate). i18n: `approvals.action.suggest_changes` → DE "Änderungen vorschlagen" / EN "Suggest changes".
`frontend/src/client/inbox.ts` — extend the click handler:
`frontend/src/client/inbox.ts` — clicking the suggest-changes button opens a **modal**, not a `window.prompt` (the existing reject prompt is OK because reject only needs a note; suggest-changes needs an editable form). The modal:
- Renders the same fields the entity edit form would show, pre-populated from `detail.payload` (the requester's proposed values).
- Adds a free-text "Vorschlagskommentar" textarea at the bottom (the note).
- On submit: POST `/api/approval-requests/{id}/suggest-changes` with `{counter_payload: {...editedFields}, note}`.
- On success: refresh the bar — the old row flips to `changes_requested`, the new row appears as `pending`.
```ts
if (action === "reject" || action === "suggest_changes") {
note = window.prompt(t("approvals.note.placeholder")) || "";
if (action === "suggest_changes" && !note.trim()) {
// Server enforces too; client-side guard for UX.
return;
}
}
const url = action === "suggest_changes"
? `/api/approval-requests/${id}/suggest-changes`
: `/api/approval-requests/${id}/${action}`;
```
Where the modal's field-editor lives: a new `client/components/approval-edit-modal.ts` that takes `entity_type` + `payload` + `pre_image` and returns the edited payload. For v1 it can be a thin wrapper over the existing entity-edit form components (Frist date picker, Termin start/end pickers). Don't build a generic field-editor framework — just deadlines + appointments, hard-coded fields per entity_type.
Status pill: add `"approvals.status.changes_requested"` to `frontend/src/client/i18n.ts` + `frontend/src/i18n-keys.ts`. DE: "Änderungen vorgeschlagen". EN: "Changes suggested".
**Status pill for `changes_requested`** — i18n keys + colour:
- `approvals.status.changes_requested` → DE "Abgelehnt mit Vorschlag" / EN "Declined with changes"
- Reuse the existing `approval-pill--historic` style; no new colour token needed for v1.
Edit-and-resubmit CTA on the row when status === "changes_requested" AND viewer_is_requester: a single button "Bearbeiten und erneut einreichen" that links to the entity's edit page pre-populated with the original payload. (The entity-edit page already supports prefill via query params for some flows; if not, that's a small adjacent change.)
**The "Edit and resubmit" CTA on the requester's row is NOT needed** (m's Q6 reframing) — the requester just sees the new pending row in /inbox, same as any other.
### 3.6 Inbox visibility
### 3.5 Inbox filter
The existing /inbox `approval_status` filter chip cluster has values: pending / approved / rejected / revoked / superseded (frontend) — we add `changes_requested`. The `self_requested` viewer-role default already includes terminal statuses (so the requester sees their changes_requested row without changing the default filter).
The /inbox `approval_status` filter chip cluster gains `changes_requested`. The `self_requested` viewer-role default already includes terminal statuses, so the original requester sees their `changes_requested` row without changing the default filter.
### 3.6 Linkage from old row to new row in /inbox
When showing a `changes_requested` row in /inbox, add a small "→ Neuer Vorschlag von {approver}" link below the note that scrolls / filters to the new pending row (it'll be visible to anyone eligible, including the original requester). The new row has `previous_request_id` pointing at the old one — so the API response for the old row can hydrate `next_request_id` (computed: `SELECT id FROM approval_requests WHERE previous_request_id = $1 LIMIT 1`).
### 3.7 Email notification (Phase 2 — defer until v1 ships)
Reuse `mail.MailService` from the existing reminder path. New template `email-approval-changes-suggested-de.html` / `-en.html`. Trigger: emit a `mail_jobs` row in the same tx as the `SuggestChanges` commit; the existing mailer cron picks it up.
The new row triggers the existing `*_approval_requested` notification path (whatever that is for Submit*) — same audience, same template. No new code. The old row's transition to `changes_requested` doesn't need its own mail; the new-row mail already tells the audience "X suggested changes to your earlier submission" through the body.
Out of scope for v1 if m wants to ship fast — /inbox visibility is enough on day one.
Out of scope for v1: a bespoke "your submission was declined with a counter-proposal" email aimed at the original requester. The new-row mail covers it functionally.
---
## 4. Slice plan
The whole thing is small enough for one PR, but breaking it into 3 commits keeps each reviewable:
Three reviewable slices, each one PR. Combined scope is small/medium.
1. **Slice A — backend.** Migration 103 + `SuggestChanges` service method + `decide()` switch + HTTP handler + tests.
2. **Slice B — frontend.** 4th button on /inbox + status pill + "Bearbeiten und erneut einreichen" CTA + i18n keys.
3. **Slice C — chain linking (`previous_request_id`).** Hook into `Submit*` paths to populate the FK. Lightweight, testable in isolation.
1. **Slice A — backend.** Migration 103 (CHECK extension + `counter_payload jsonb` + `previous_request_id` FK + index) + `SuggestChanges` service method + HTTP handler + service tests (happy path, no-op-suggestion guard, deadlock on new row, self-approval block, request_not_pending). Migration is non-blocking on Postgres; safe for live deploy.
2. **Slice B — frontend.** 4th button on /inbox + the edit modal (deadline-fields variant + appointment-fields variant) + status pill `changes_requested` ("Abgelehnt mit Vorschlag") + i18n keys (DE + EN) + the "→ Neuer Vorschlag" link from old row to new row. End-to-end browser smoke test via Playwright.
3. **Slice C — Verlauf integration.** Make sure the `*_approval_changes_suggested` event renders on the project / deadline / appointment Verlauf timeline alongside the existing 4 approval event types. May or may not need code change depending on how generic the Verlauf row renderer is — likely just an i18n key + an icon mapping.
Each slice is one merge to main. Total: small/medium PR.
Don't ship a chain-traversal UI in v1. The `previous_request_id` FK is captured so the data is there; surfacing the full chain history (n hops back) is a Phase-2 polish.
---
## 5. Risks / open considerations
- **Chain depth.** Nothing stops a runaway "I keep suggesting / they keep resubmitting" loop. Probably fine — same risk as comment threads on GitHub PRs. Out of scope to cap.
- **Concurrent suggestions.** Two approvers click "suggest changes" simultaneously? Same `getRequestForUpdate` row-lock as Approve/Reject handles this — second caller gets `ErrRequestNotPending`.
- **Migration safety.** Adding a value to a CHECK constraint is non-blocking on Postgres (no table rewrite). Adding a NULLable FK column is also non-blocking. Safe for live deploy.
- **`previous_request_id` lookup race.** If two `Submit*` fire concurrently after one `changes_requested`, both could link to the same `previous_request_id`. That's harmless (the chain is a soft tree), but for cleanliness the lookup could be enforced as "no two pending+previous_request_id pointing at the same previous" via a partial unique index. Defer unless this becomes a real problem.
- **i18n coverage.** New strings on every supported lang (DE + EN). Trivial.
- **What about a checklist-style suggestion (option Q4c, structured per-field)?** Could be a Phase 2 enhancement layered on top: parse a structured-JSON note shape opportunistically; fall back to free-text. Don't block v1 on it.
- **Chain depth runaway.** Nothing stops an "I keep suggesting / they keep counter-suggesting" loop. Same risk as comment threads on GitHub PRs. Out of scope to cap; the social pressure (each round is a 4-Augen action with a name attached) is the natural brake.
- **Concurrent suggestions on the same pending row.** Two approvers click "suggest changes" at the same time? The existing `getRequestForUpdate` row-lock serialises them; the second caller gets `ErrRequestNotPending` (the first already flipped it). Same guarantee as Approve/Reject today.
- **Deadlock on the new row.** If the suggesting approver is the only qualified approver other than the original requester, the new row's deadlock check returns "no qualified approver" — because the original requester IS now eligible (they're no longer the requested_by), but might not have a high-enough role. The check needs to recognise: caller's pool = "anyone other than the new requester who can canApprove". Original requester counts if they hit the required-role bar. This is just the existing deadlock predicate run against the new (requester, role) tuple; no special-case logic. Surfaced as `409 "no_qualified_approver"` to the suggesting approver, with the standard global_admin override path still available.
- **Counter-payload schema validation.** Server must validate `counter_payload` against the same schema as a normal `Submit*` for that entity_type + lifecycle_event. Otherwise a malicious approver could write garbage values via the suggestion path that wouldn't fly through `Submit*`. Reuse the existing payload-schema validator from the entity services; don't write a parallel.
- **No-op suggestion guard.** Approver clicks suggest-changes but doesn't actually edit anything AND leaves the note empty? Server rejects with `ErrSuggestionRequiresChange`. UI guards too (the submit button stays disabled until either the form is dirty OR the note has text).
- **Migration safety.** Non-blocking. Adding a value to a CHECK constraint is a metadata-only change; adding a NULLable column + a NULLable FK is also metadata-only.
- **What about a structured per-field suggestion (Q4c)?** The `counter_payload` jsonb IS structured — each entity_type has fixed fields. There's no need for a separate "{field, current, suggested}" shape because the diff is computable from `pre_image → counter_payload` on the new row.
- **What about thread-of-notes (Q4b)?** Implicit in the chain — each row's `decision_note` is one "note" by one author; following `previous_request_id` backwards reconstructs the full back-and-forth. A future "thread view" UI is layered on top of this without schema change.
---
## 6. m's decisions (to be filled by AskUserQuestion phase)
## 6. m's decisions
_This section will be filled in after m answers the AskUserQuestion batches. Each line will read: `Q{N} ({header}): m picked {answer}. {reasoning if different from recommendation}`._
See §0a (decisions table) — filled in after the AskUserQuestion phase on 2026-05-19.
---