docs(approvals): t-paliad-216 — design doc for "Suggest changes" action
Inventor draft of the fourth approval action alongside approve / reject / revoke. Open questions in §2 will be resolved via AskUserQuestion before any coder work. Recommendations folded in inline. Verified live state before designing: status enum already carries an unused 'superseded' value; entity approval_status is approved/pending/legacy only; decision_note exists as free text; the existing decide() kernel handles approve / reject / revoke with a single switch.
This commit is contained in:
291
docs/design-approval-suggest-changes-2026-05-19.md
Normal file
291
docs/design-approval-suggest-changes-2026-05-19.md
Normal file
@@ -0,0 +1,291 @@
|
||||
# Design — "Suggest changes" action on approval flow
|
||||
|
||||
**Author:** hertz (inventor)
|
||||
**Date:** 2026-05-19
|
||||
**Task:** t-paliad-216 (m/paliad in-flight)
|
||||
**Branch:** `mai/hertz/inventor-suggest-changes`
|
||||
**Status:** DESIGN — open questions await m before any coder shift.
|
||||
|
||||
---
|
||||
|
||||
## 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.
|
||||
|
||||
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.
|
||||
|
||||
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.
|
||||
|
||||
---
|
||||
|
||||
## 1. Context — what's already in the code (verified 2026-05-19)
|
||||
|
||||
- **State machine** in `internal/services/approval_service.go`:
|
||||
- `paliad.approval_requests.status` CHECK is already `('pending', 'approved', 'rejected', 'revoked', 'superseded')` — the `superseded` value is defined as a Go constant `RequestStatusSuperseded` but never written by the live service (reserved).
|
||||
- `paliad.{deadlines,appointments}.approval_status` CHECK is `('approved', 'pending', 'legacy')` — three values only.
|
||||
- Shared kernel `decide(requestID, callerID, finalStatus, note)` powers Approve / Reject / Revoke. Approve invokes `applyApproved`; Reject + Revoke invoke `applyRevert` (restores entity from `pre_image`).
|
||||
- Self-approval blocked at 3 layers: `canApprove` Go gate, `approval_requests_no_self_approval` DB CHECK, deadlock-check excludes requester from pool.
|
||||
- **Handlers** in `internal/handlers/approvals.go`:
|
||||
- `POST /api/approval-requests/{id}/approve`
|
||||
- `POST /api/approval-requests/{id}/reject`
|
||||
- `POST /api/approval-requests/{id}/revoke`
|
||||
- `GET /api/approval-requests/{id}` — single hydrated request
|
||||
- **Per-viewer flags** (t-paliad-202, shipped): every row carries `viewer_can_approve` + `viewer_is_requester` resolved server-side so the UI can grey out buttons the server would reject. Server still enforces — the flags are a UX hint.
|
||||
- **Frontend**:
|
||||
- `frontend/src/client/inbox.ts` wires three buttons per pending row (approve/reject/revoke). Reject opens `window.prompt()` for the note; approve+revoke don't.
|
||||
- `frontend/src/client/views/shape-list.ts` (row_action="approve") stamps the row with action buttons + diff + `decision_note` display if present.
|
||||
- **Audit**: event types `*_approval_requested`, `*_approval_approved`, `*_approval_rejected`, `*_approval_revoked` emitted to `paliad.project_events` (one per entity_type prefix).
|
||||
- **Decision note**: `paliad.approval_requests.decision_note text` — a single free-text column, last-write-wins. Already populated on Reject (Approve also accepts an optional note).
|
||||
|
||||
---
|
||||
|
||||
## 2. Design questions (the open list — see §6 for answered)
|
||||
|
||||
Pre-recommendations from inventor. m will pick via AskUserQuestion.
|
||||
|
||||
### State machine
|
||||
|
||||
**Q1 — Where does "suggest changes" sit on the lifecycle?**
|
||||
- **(a) New status `changes_requested` (RECOMMENDED).** The approval_requests row transitions pending → changes_requested. Sibling of approved/rejected/revoked/superseded. The row is terminal in that status; a re-submit creates a fresh row (linked via `previous_request_id`).
|
||||
- (b) Reuse `rejected` with `is_revisable=true` flag. Cheap, but conflates two semantically distinct outcomes ("we'll never want this" vs. "tweak X and try again").
|
||||
- (c) Auto-revoke the current row, mark the entity for edit, requester creates a new approval row when ready. Reuses existing plumbing — but loses the approver's note as a first-class thing (it'd just be a comment on the project_events row).
|
||||
- (d) Other (you'll tell us).
|
||||
|
||||
Recommend (a) — keeps the audit lifecycle clear, gives us a clean place to hang the suggestion note, and is the smallest schema change (one new value in a CHECK constraint).
|
||||
|
||||
**Q2 — What happens to the entity (deadline/appointment) while in "changes requested"?**
|
||||
- **(a) Entity reverts to pre_image — same as Reject (RECOMMENDED).** approval_status flips back to `approved`. The requester edits the entity in the normal flow; saving fires a fresh `Submit*` cycle.
|
||||
- (b) Entity stays at `approval_status=pending` carrying the proposed values; requester edits "in place" through a new "amend the pending request" endpoint that mutates the same approval_request row + entity fields.
|
||||
- (c) Entity goes to a new `approval_status=draft` (would require a new value on the entity-level CHECK + UI work to handle a third entity state).
|
||||
|
||||
Recommend (a) — minimum schema change, reuses every existing path (entity edit, Submit*, applyRevert, project_events emission). The trade-off is one extra approval_requests row per cycle; we link via `previous_request_id` so the chain stays inspectable.
|
||||
|
||||
**Q3 — Can the approver suggest changes multiple times (across a chain)?**
|
||||
- **(a) Yes, across chained rows (RECOMMENDED).** Each row is terminal after suggest-changes; the requester resubmits → new pending row → approver can suggest changes again. Chain depth unbounded.
|
||||
- (b) No — one chance per entity-lifecycle; if the requester comes back, the only options are approve or reject (the suggest-changes button is hidden for the second submission).
|
||||
|
||||
Recommend (a) — bounded by the requester's patience, not by the system. Multi-round review is the norm in legal-doc workflows.
|
||||
|
||||
**Q4 — Note shape on the suggestion**
|
||||
- **(a) Free-text — reuse `decision_note` (RECOMMENDED).** Same column the existing Reject path already populates. Last-write-wins per row (but rows are terminal after suggest-changes, so there's no real "last write").
|
||||
- (b) Thread of notes — new `paliad.approval_notes` table, ordered, multi-author. Lets the requester respond inline, the approver clarify, etc.
|
||||
- (c) Structured per-field suggestions (`[{"field": "due_date", "current": "...", "suggested": "..."}]`) — a "diff-style" view.
|
||||
|
||||
Recommend (a) — matches the existing Reject UX, no new schema. (b) is right if the team wants to discuss; (c) is over-engineered for v1.
|
||||
|
||||
### UX
|
||||
|
||||
**Q5 — Where does the requester see the suggestion?**
|
||||
- **(a) /inbox under `a_role=self_requested` (RECOMMENDED for v1).** Same surface they already use to see rejected. New status pill "Änderungen vorgeschlagen" + the note + a CTA "Bearbeiten und erneut einreichen".
|
||||
- (b) A new badge on the entity's detail page (e.g. on the deadline detail page itself).
|
||||
- (c) Email + push notification.
|
||||
- (d) All of the above.
|
||||
|
||||
Recommend (a) for v1. Email reminder is a natural Phase-2 add-on (it'd reuse the existing reminder-mail plumbing). The entity-detail badge is nice but the user is already seeing the row in /inbox.
|
||||
|
||||
**Q6 — What action(s) does the requester have on a `changes_requested` row?**
|
||||
- **(a) Edit and resubmit (RECOMMENDED).** Primary action. Opens the entity's edit form pre-populated with the original `payload`. Saving fires `Submit*` → new pending request with `previous_request_id` linking back.
|
||||
- (b) Withdraw (= dismiss the row from inbox, no DB change). Mostly UI-only — the row is already terminal; "withdraw" would just be a "mark as not-pursuing" toggle.
|
||||
- (c) Both.
|
||||
|
||||
Recommend (a). The row is already terminal once status=`changes_requested`; the requester either acts on the suggestion (a) or lets the row sit in their inbox history (no action needed). Adding a "dismiss" button is a UI nice-to-have but doesn't change the data model; can defer.
|
||||
|
||||
### Notifications
|
||||
|
||||
**Q7 — Who gets notified when "suggest changes" fires?**
|
||||
- **(a) Just the requester (RECOMMENDED for v1).** Email-reminder path is reused: requester gets a mail "X hat Änderungen vorgeschlagen für …" with the note inline + a link to /inbox.
|
||||
- (b) Requester + any other potential approvers (they need to know the request is closed, not pending).
|
||||
- (c) Requester + approval-policy-defined watchers (would require a new `approval_policies.watchers` column).
|
||||
|
||||
Recommend (a). The request is terminal so other approvers don't need a "this is now your problem" ping — they wouldn't have anything to act on. They see it in /inbox under "Alle sichtbaren" anyway if curious.
|
||||
|
||||
### Audit
|
||||
|
||||
**Q8 — Audit row shape on `project_events`**
|
||||
- **(a) New event_type `*_approval_changes_suggested` per entity (RECOMMENDED).** Parallel to the existing 4 (requested/approved/rejected/revoked). Two new event types: `deadline_approval_changes_suggested`, `appointment_approval_changes_suggested`. Note text goes in metadata.
|
||||
- (b) Bundle with the resubmission — single composite event "approved-with-revisions" when the chain eventually approves.
|
||||
|
||||
Recommend (a). Each transition gets its own event row — that's how the existing audit chain already works (one event per state change). It also gives the Verlauf timeline a row to render the approver's note.
|
||||
|
||||
---
|
||||
|
||||
## 3. Implementation sketch (if recommendations stand)
|
||||
|
||||
### 3.1 Migration `103_approval_suggest_changes.up.sql`
|
||||
|
||||
```sql
|
||||
-- 1. Extend approval_requests.status CHECK to allow 'changes_requested'.
|
||||
ALTER TABLE paliad.approval_requests
|
||||
DROP CONSTRAINT IF EXISTS approval_requests_status_check;
|
||||
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.
|
||||
ALTER TABLE paliad.approval_requests
|
||||
ADD COLUMN previous_request_id uuid NULL
|
||||
REFERENCES paliad.approval_requests(id) ON DELETE SET NULL;
|
||||
|
||||
CREATE INDEX approval_requests_previous_idx
|
||||
ON paliad.approval_requests (previous_request_id)
|
||||
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).
|
||||
|
||||
### 3.2 Service layer
|
||||
|
||||
Add to `internal/services/approval_service.go`:
|
||||
|
||||
```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.
|
||||
//
|
||||
// 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)
|
||||
}
|
||||
```
|
||||
|
||||
Extend `decide()` to handle the new final status:
|
||||
|
||||
```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
|
||||
|
||||
```
|
||||
POST /api/approval-requests/{id}/suggest-changes
|
||||
Body: {"note": "..."}
|
||||
Returns: 200 {} on success
|
||||
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
|
||||
```
|
||||
|
||||
Register in `internal/handlers/handlers.go` alongside the existing three.
|
||||
|
||||
### 3.5 Frontend
|
||||
|
||||
`frontend/src/client/views/shape-list.ts` — add a fourth button to the pending-row action group:
|
||||
|
||||
```ts
|
||||
actions.appendChild(approvalActionBtn("approve", detail));
|
||||
actions.appendChild(approvalActionBtn("suggest_changes", detail));
|
||||
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).
|
||||
|
||||
`frontend/src/client/inbox.ts` — extend the click handler:
|
||||
|
||||
```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}`;
|
||||
```
|
||||
|
||||
Status pill: add `"approvals.status.changes_requested"` to `frontend/src/client/i18n.ts` + `frontend/src/i18n-keys.ts`. DE: "Änderungen vorgeschlagen". EN: "Changes suggested".
|
||||
|
||||
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.)
|
||||
|
||||
### 3.6 Inbox visibility
|
||||
|
||||
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).
|
||||
|
||||
### 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.
|
||||
|
||||
Out of scope for v1 if m wants to ship fast — /inbox visibility is enough on day one.
|
||||
|
||||
---
|
||||
|
||||
## 4. Slice plan
|
||||
|
||||
The whole thing is small enough for one PR, but breaking it into 3 commits keeps each reviewable:
|
||||
|
||||
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.
|
||||
|
||||
Each slice is one merge to main. Total: small/medium PR.
|
||||
|
||||
---
|
||||
|
||||
## 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.
|
||||
|
||||
---
|
||||
|
||||
## 6. m's decisions (to be filled by AskUserQuestion phase)
|
||||
|
||||
_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}`._
|
||||
|
||||
---
|
||||
|
||||
## 7. Out of scope for this design
|
||||
|
||||
- Email + push notifications (Phase 2; see §3.7).
|
||||
- Structured per-field suggestion shape (Phase 2 enhancement).
|
||||
- Approval-policy `watchers` column for notification fan-out.
|
||||
- "Dismiss this row from my inbox" UI toggle (UX-only, not a data-model change).
|
||||
- Cross-entity suggest-changes (e.g. project, party). Same as the original approval scope — deadlines + appointments only.
|
||||
|
||||
Reference in New Issue
Block a user