Merge: t-paliad-202 — inbox grey-out illegal actions (replace alert-after-click with server-tagged viewer_can_approve / viewer_is_requester flags)

2026-05-17 12:45:32 +02:00
parent 4f66feffce aa82434af9
commit 3c9ecabf17
7 changed files with 275 additions and 70 deletions
--- a/frontend/src/client/i18n.ts
+++ b/frontend/src/client/i18n.ts
@@ -2201,6 +2201,9 @@ const translations: Record<Lang, Record<string, string>> = {
    "approvals.error.concurrent_pending": "Es liegt bereits eine Genehmigungsanfrage auf diesem Eintrag vor.",
    "approvals.error.awaiting_approval": "Diese Anforderung wartet auf Genehmigung.",
    "approvals.error.request_not_pending": "Diese Anfrage ist nicht mehr offen.",
+    "approvals.disabled.self_approval": "Du kannst eigene Anträge nicht genehmigen",
+    "approvals.disabled.not_authorized": "Du hast keine Genehmigungsberechtigung für diesen Antrag",
+    "approvals.disabled.revoke_not_requester": "Nur der Antragsteller kann zurückziehen",
    "approvals.pending.badge": "Wartet auf Genehmigung",
    "approvals.withdraw.cta": "Genehmigungsanfrage zurückziehen",
    "approvals.withdraw.confirm": "Genehmigungsanfrage wirklich zurückziehen?",
@@ -4746,6 +4749,9 @@ const translations: Record<Lang, Record<string, string>> = {
    "approvals.error.concurrent_pending": "Another approval request is already in flight on this entity.",
    "approvals.error.awaiting_approval": "This entity is awaiting approval.",
    "approvals.error.request_not_pending": "This request is no longer open.",
+    "approvals.disabled.self_approval": "You cannot approve your own requests",
+    "approvals.disabled.not_authorized": "You are not authorized to approve this request",
+    "approvals.disabled.revoke_not_requester": "Only the requester can withdraw",
    "approvals.pending.badge": "Awaiting approval",
    "approvals.withdraw.cta": "Withdraw approval request",
    "approvals.withdraw.confirm": "Withdraw the approval request?",
--- a/frontend/src/client/views/shape-list.ts
+++ b/frontend/src/client/views/shape-list.ts
@@ -196,6 +196,12 @@ interface ApprovalDetail {
  requester_kind?: "user" | "agent";
  decider_name?: string;
  decision_note?: string;
+  // Per-viewer eligibility flags resolved server-side against the caller
+  // (t-paliad-202). Used to grey out actions the server would reject.
+  // Optional so an older payload still renders — falsy means "treat as
+  // disabled" for the safety side (no false enables).
+  viewer_can_approve?: boolean;
+  viewer_is_requester?: boolean;
 }

 function renderApprovalList(rows: ViewRow[]): HTMLElement {
@@ -256,13 +262,15 @@ function renderApprovalList(rows: ViewRow[]): HTMLElement {
    actions.className = "inbox-row-actions";

    if (detail.status === "pending") {
-      // The bar's approval_viewer_role distinguishes which actions are
-      // appropriate. The surface inspects the active role and decides
-      // which buttons to keep — but for default rendering we stamp all
-      // three with role-class hints and let the surface filter.
-      actions.appendChild(actionBtn("approve"));
-      actions.appendChild(actionBtn("reject"));
-      actions.appendChild(actionBtn("revoke"));
+      // All three actions are stamped on every pending row; the per-viewer
+      // viewer_can_approve / viewer_is_requester flags (resolved server-side)
+      // decide which are enabled vs. greyed out with a tooltip. m's ask
+      // (2026-05-17): show what's possible but disable what isn't, rather
+      // than alert-after-click. The server still enforces — disabled buttons
+      // are a UI hint, not a security gate.
+      actions.appendChild(approvalActionBtn("approve", detail));
+      actions.appendChild(approvalActionBtn("reject", detail));
+      actions.appendChild(approvalActionBtn("revoke", detail));
    } else if (detail.status) {
      const pill = document.createElement("span");
      pill.className = "approval-pill approval-pill--historic";
@@ -312,16 +320,39 @@ function renderDiff(detail: ApprovalDetail): HTMLElement | null {
  return wrap;
 }

-function actionBtn(action: "approve" | "reject" | "revoke"): HTMLButtonElement {
+function approvalActionBtn(
+  action: "approve" | "reject" | "revoke",
+  detail: ApprovalDetail,
+): HTMLButtonElement {
  const btn = document.createElement("button");
  btn.type = "button";
  btn.dataset.action = action;
  const cls = action === "approve" ? "btn-primary" : action === "reject" ? "btn-danger" : "btn-secondary";
  btn.className = `btn ${cls} inbox-row-action views-approval-action`;
  btn.textContent = t(("approvals.action." + action) as I18nKey);
+
+  // approve / reject share the eligibility gate; revoke is requester-only.
+  const reason = disabledReasonFor(action, detail);
+  if (reason) {
+    btn.disabled = true;
+    btn.title = t(reason);
+  }
  return btn;
 }

+function disabledReasonFor(
+  action: "approve" | "reject" | "revoke",
+  detail: ApprovalDetail,
+): I18nKey | null {
+  if (action === "revoke") {
+    return detail.viewer_is_requester ? null : "approvals.disabled.revoke_not_requester";
+  }
+  // approve + reject — same gate as the server's canApprove.
+  if (detail.viewer_can_approve) return null;
+  if (detail.viewer_is_requester) return "approvals.disabled.self_approval";
+  return "approvals.disabled.not_authorized";
+}
+
 function formatRelativeTime(iso: string): string {
  const t0 = Date.parse(iso);
  if (isNaN(t0)) return iso;
--- a/frontend/src/i18n-keys.ts
+++ b/frontend/src/i18n-keys.ts
@@ -591,6 +591,9 @@ export type I18nKey =
  | "approvals.decision_kind.peer"
  | "approvals.diff.after"
  | "approvals.diff.before"
+  | "approvals.disabled.not_authorized"
+  | "approvals.disabled.revoke_not_requester"
+  | "approvals.disabled.self_approval"
  | "approvals.empty.mine"
  | "approvals.empty.pending_mine"
  | "approvals.entity.appointment"
--- a/frontend/src/styles/global.css
+++ b/frontend/src/styles/global.css
@@ -11456,6 +11456,24 @@ dialog.quick-add-sheet::backdrop {
    font-size: 13px;
 }

+/* Greyed-out variant for actions the current viewer can't grant —
+ * approve/reject without authority, revoke when not the requester
+ * (t-paliad-202). Click is suppressed by the disabled attribute; the
+ * tooltip on `title` explains why. The neutral background/colour pair
+ * overrides .btn-primary/.btn-danger/.btn-secondary so all three
+ * variants look the same when disabled. */
+.inbox-row-action:disabled {
+    cursor: not-allowed;
+    opacity: 0.55;
+    background: var(--color-surface-2);
+    color: var(--color-text-muted);
+    border: 1px solid var(--color-border);
+}
+.inbox-row-action:disabled:hover {
+    background: var(--color-surface-2);
+    border-color: var(--color-border);
+}
+
 .inbox-row-decided {
    color: var(--fg-muted);
    font-size: 12px;
--- a/internal/handlers/approvals.go
+++ b/internal/handlers/approvals.go
@@ -281,7 +281,8 @@ func handleGetApprovalRequest(w http.ResponseWriter, r *http.Request) {
 	if !requireDB(w) {
 		return
 	}
-	if _, ok := requireUser(w, r); !ok {
+	uid, ok := requireUser(w, r)
+	if !ok {
 		return
 	}
 	requestID, err := uuid.Parse(r.PathValue("id"))
@@ -289,7 +290,7 @@ func handleGetApprovalRequest(w http.ResponseWriter, r *http.Request) {
 		writeJSON(w, http.StatusBadRequest, map[string]string{"error": "invalid request id"})
 		return
 	}
-	row, err := dbSvc.approval.GetRequest(r.Context(), requestID)
+	row, err := dbSvc.approval.GetRequest(r.Context(), uid, requestID)
 	if err != nil {
 		writeServiceError(w, err)
 		return
--- a/internal/services/approval_service.go
+++ b/internal/services/approval_service.go
@@ -809,16 +809,67 @@ func marshalJSONOrNull(m map[string]any) ([]byte, error) {
 // ApprovalRequestView is the inbox-friendly projection of an approval
 // request: the bare ApprovalRequest plus the contextual labels the inbox
 // needs to render a row without further fetches.
+//
+// ViewerCanApprove + ViewerIsRequester are per-viewer eligibility flags
+// computed against the $1 callerID bound at query time (t-paliad-202).
+// The frontend uses them to grey out the action buttons it knows the
+// server would reject, replacing the previous click-then-alert UX.
 type ApprovalRequestView struct {
 	models.ApprovalRequest
-	ProjectTitle    string  `db:"project_title"     json:"project_title"`
-	EntityTitle     *string `db:"entity_title"      json:"entity_title,omitempty"`
-	RequesterName   string  `db:"requester_name"    json:"requester_name"`
-	RequesterEmail  string  `db:"requester_email"   json:"requester_email"`
-	DeciderName     *string `db:"decider_name"      json:"decider_name,omitempty"`
-	DeciderEmail    *string `db:"decider_email"     json:"decider_email,omitempty"`
+	ProjectTitle      string  `db:"project_title"       json:"project_title"`
+	EntityTitle       *string `db:"entity_title"        json:"entity_title,omitempty"`
+	RequesterName     string  `db:"requester_name"      json:"requester_name"`
+	RequesterEmail    string  `db:"requester_email"     json:"requester_email"`
+	DeciderName       *string `db:"decider_name"        json:"decider_name,omitempty"`
+	DeciderEmail      *string `db:"decider_email"       json:"decider_email,omitempty"`
+	ViewerCanApprove  bool    `db:"viewer_can_approve"  json:"viewer_can_approve"`
+	ViewerIsRequester bool    `db:"viewer_is_requester" json:"viewer_is_requester"`
 }

+// approvalEligibilitySQL is the SELECT-and-WHERE-compatible boolean
+// expression that returns true iff the user bound to $1 is qualified to
+// approve the approval_requests row aliased `ar` on the project aliased
+// `p` (i.e. the SELECT must include `paliad.approval_requests ar JOIN
+// paliad.projects p ON p.id = ar.project_id`). The three eligibility
+// branches mirror canApprove (line 484):
+//
+//   - $1 is global_admin, OR
+//   - $1 has direct/ancestor project_teams membership with responsibility
+//     ∈ {lead, member} AND a profession at or above the threshold
+//     (t-paliad-148 tuple-with-gate), OR
+//   - $1 has partner-unit-derived authority (t-paliad-139).
+//
+// Self-authorship is NOT subtracted here — callers add the
+// `ar.requested_by <> $1` predicate when they want the strict
+// "can approve" semantics (the inbox WHERE) or fold it into the
+// SELECT (viewer_can_approve column). Keeping the two predicates
+// separate lets the same fragment serve both ListPendingForApprover's
+// filter and the per-row viewer flag without duplicating SQL.
+const approvalEligibilitySQL = `(
+    EXISTS (SELECT 1 FROM paliad.users u WHERE u.id = $1 AND u.global_role = 'global_admin')
+    OR EXISTS (
+        SELECT 1 FROM paliad.project_teams pt
+          JOIN paliad.users u ON u.id = pt.user_id
+         WHERE pt.user_id = $1
+           AND pt.project_id = ANY(string_to_array(p.path, '.')::uuid[])
+           AND pt.responsibility IN ('lead', 'member')
+           AND paliad.approval_role_level(u.profession) >= paliad.approval_role_level(ar.required_role)
+    )
+    OR EXISTS (
+        SELECT 1 FROM paliad.project_partner_units ppu
+          JOIN paliad.partner_unit_members pum ON pum.partner_unit_id = ppu.partner_unit_id
+         WHERE pum.user_id = $1
+           AND ppu.project_id = ANY(string_to_array(p.path, '.')::uuid[])
+           AND ppu.derive_grants_authority = true
+           AND pum.unit_role = ANY(ppu.derive_unit_roles)
+           AND paliad.approval_role_level(
+                   paliad.approval_role_from_unit_role(pum.unit_role)
+               ) >= paliad.approval_role_level(ar.required_role)
+    )
+)`
+
+// approvalRequestViewColumns binds $1 = callerID via the two viewer_*
+// flags. Every caller must pass the caller's UUID as the first arg.
 const approvalRequestViewColumns = `
    ar.id, ar.project_id, ar.entity_type, ar.entity_id, ar.lifecycle_event,
    ar.pre_image, ar.payload, ar.requested_by, ar.requested_at, ar.required_role,
@@ -832,7 +883,9 @@ const approvalRequestViewColumns = `
    COALESCE(ru.display_name, ru.email) AS requester_name,
    ru.email                            AS requester_email,
    du.display_name                     AS decider_name,
-    du.email                            AS decider_email`
+    du.email                            AS decider_email,
+    (ar.status = 'pending' AND ar.requested_by <> $1 AND ` + approvalEligibilitySQL + `) AS viewer_can_approve,
+    (ar.requested_by = $1)              AS viewer_is_requester`

 const approvalRequestViewJoins = `
    paliad.approval_requests ar
@@ -860,34 +913,10 @@ func (s *ApprovalService) ListPendingForApprover(ctx context.Context, callerID u
 	conds := []string{
 		"ar.status = 'pending'",
 		"ar.requested_by <> $1",
-		// Eligibility (any one branch suffices):
-		//  - caller is global_admin, OR
-		//  - caller has direct/ancestor project_teams membership with
-		//    responsibility ∈ {lead, member} AND profession at or above
-		//    the threshold (t-paliad-148 tuple-with-gate), OR
-		//  - caller is a partner-unit-derived member with derive_grants_authority=true
-		//    on an attachment in the project's path, and the unit_role maps to a
-		//    profession at or above the threshold (t-paliad-139).
-		`(EXISTS (SELECT 1 FROM paliad.users u WHERE u.id = $1 AND u.global_role = 'global_admin')
-		  OR EXISTS (
-		      SELECT 1 FROM paliad.project_teams pt
-		        JOIN paliad.users u ON u.id = pt.user_id
-		       WHERE pt.user_id = $1
-		         AND pt.project_id = ANY(string_to_array(p.path, '.')::uuid[])
-		         AND pt.responsibility IN ('lead', 'member')
-		         AND paliad.approval_role_level(u.profession) >= paliad.approval_role_level(ar.required_role)
-		  )
-		  OR EXISTS (
-		      SELECT 1 FROM paliad.project_partner_units ppu
-		        JOIN paliad.partner_unit_members pum ON pum.partner_unit_id = ppu.partner_unit_id
-		       WHERE pum.user_id = $1
-		         AND ppu.project_id = ANY(string_to_array(p.path, '.')::uuid[])
-		         AND ppu.derive_grants_authority = true
-		         AND pum.unit_role = ANY(ppu.derive_unit_roles)
-		         AND paliad.approval_role_level(
-		                 paliad.approval_role_from_unit_role(pum.unit_role)
-		             ) >= paliad.approval_role_level(ar.required_role)
-		  ))`,
+		// Eligibility predicate (the three branches mirror canApprove and
+		// the viewer_can_approve SELECT expression — same fragment, single
+		// source of truth).
+		approvalEligibilitySQL,
 	}
 	args := []any{callerID}
 	if filter.ProjectID != nil {
@@ -946,13 +975,15 @@ func (s *ApprovalService) ListSubmittedByUser(ctx context.Context, callerID uuid
 }

 // GetRequest returns one approval request hydrated for the inbox detail
-// view. Visibility is gated upstream by the handler (anyone with project
-// access can see the request).
-func (s *ApprovalService) GetRequest(ctx context.Context, requestID uuid.UUID) (*ApprovalRequestView, error) {
-	q := fmt.Sprintf(`SELECT %s FROM %s WHERE ar.id = $1`,
+// view, with viewer_can_approve / viewer_is_requester resolved for
+// callerID. Visibility is gated upstream by the handler (anyone with
+// project access can see the request).
+func (s *ApprovalService) GetRequest(ctx context.Context, callerID, requestID uuid.UUID) (*ApprovalRequestView, error) {
+	// $1 = callerID (binds the viewer_* flags); $2 = requestID.
+	q := fmt.Sprintf(`SELECT %s FROM %s WHERE ar.id = $2`,
 		approvalRequestViewColumns, approvalRequestViewJoins)
 	var v ApprovalRequestView
-	if err := s.db.GetContext(ctx, &v, q, requestID); err != nil {
+	if err := s.db.GetContext(ctx, &v, q, callerID, requestID); err != nil {
 		if errors.Is(err, sql.ErrNoRows) {
 			return nil, nil
 		}
@@ -974,26 +1005,7 @@ func (s *ApprovalService) PendingCountForUser(ctx context.Context, callerID uuid
 	        JOIN paliad.projects p ON p.id = ar.project_id
 	       WHERE ar.status = 'pending'
 	         AND ar.requested_by <> $1
-	         AND (EXISTS (SELECT 1 FROM paliad.users u WHERE u.id = $1 AND u.global_role = 'global_admin')
-	              OR EXISTS (
-	                  SELECT 1 FROM paliad.project_teams pt
-	                    JOIN paliad.users u ON u.id = pt.user_id
-	                   WHERE pt.user_id = $1
-	                     AND pt.project_id = ANY(string_to_array(p.path, '.')::uuid[])
-	                     AND pt.responsibility IN ('lead', 'member')
-	                     AND paliad.approval_role_level(u.profession) >= paliad.approval_role_level(ar.required_role)
-	              )
-	              OR EXISTS (
-	                  SELECT 1 FROM paliad.project_partner_units ppu
-	                    JOIN paliad.partner_unit_members pum ON pum.partner_unit_id = ppu.partner_unit_id
-	                   WHERE pum.user_id = $1
-	                     AND ppu.project_id = ANY(string_to_array(p.path, '.')::uuid[])
-	                     AND ppu.derive_grants_authority = true
-	                     AND pum.unit_role = ANY(ppu.derive_unit_roles)
-	                     AND paliad.approval_role_level(
-	                             paliad.approval_role_from_unit_role(pum.unit_role)
-	                         ) >= paliad.approval_role_level(ar.required_role)
-	              ))`
+	         AND ` + approvalEligibilitySQL
 	var n int
 	if err := s.db.GetContext(ctx, &n, q, callerID); err != nil {
 		return 0, fmt.Errorf("pending count: %w", err)
--- a/internal/services/approval_service_test.go
+++ b/internal/services/approval_service_test.go
@@ -812,3 +812,137 @@ func TestApprovalService_ListSubmittedByUser_PendingVisible(t *testing.T) {
 		t.Errorf("other user: len(rows) = %d, want 0 — must scope by requested_by", len(rows))
 	}
 }
+
+// TestApprovalService_ViewerFlags pins the per-viewer eligibility flags on
+// ApprovalRequestView (t-paliad-202). Drives /inbox grey-out of
+// Genehmigen/Ablehnen/Zurückziehen instead of click-then-error.
+//
+// Matrix (one pending request, four viewers):
+//
+//	viewer            viewer_can_approve  viewer_is_requester
+//	requester (self)  false               true                 → only Zurückziehen
+//	approver (peer)   true                false                → Genehmigen + Ablehnen
+//	other (no team)   false               false                → all three disabled
+//	global_admin      true                false                → Genehmigen + Ablehnen
+func TestApprovalService_ViewerFlags(t *testing.T) {
+	env := setupApprovalTest(t)
+	defer env.cleanup()
+	ctx := context.Background()
+
+	// Profession + global_role tuning: the live-DB seed gives every user
+	// global_role='standard' + profession=NULL, which means nobody is
+	// eligible by default. Promote requester→associate (matches threshold)
+	// and approver→partner (above threshold), and create a fourth user
+	// with global_role='global_admin' (the override branch).
+	if _, err := env.pool.ExecContext(ctx,
+		`UPDATE paliad.users SET profession = 'associate' WHERE id = $1`, env.requester); err != nil {
+		t.Fatalf("set requester profession: %v", err)
+	}
+	if _, err := env.pool.ExecContext(ctx,
+		`UPDATE paliad.users SET profession = 'partner' WHERE id = $1`, env.approver); err != nil {
+		t.Fatalf("set approver profession: %v", err)
+	}
+	adminID := uuid.New()
+	if _, err := env.pool.ExecContext(ctx,
+		`INSERT INTO auth.users (id, email) VALUES ($1, $1::text || '@test.local')
+		 ON CONFLICT (id) DO NOTHING`, adminID); err != nil {
+		t.Logf("skip auth.users seed for admin: %v (continuing)", err)
+	}
+	if _, err := env.pool.ExecContext(ctx,
+		`INSERT INTO paliad.users (id, email, display_name, office, global_role)
+		 VALUES ($1, $1::text || '@test.local', 'Admin', 'munich', 'global_admin')
+		 ON CONFLICT (id) DO UPDATE SET global_role = 'global_admin'`, adminID); err != nil {
+		t.Fatalf("seed admin: %v", err)
+	}
+	defer func() {
+		ctx := context.Background()
+		env.pool.ExecContext(ctx, `DELETE FROM paliad.users WHERE id = $1`, adminID)
+		env.pool.ExecContext(ctx, `DELETE FROM auth.users   WHERE id = $1`, adminID)
+	}()
+
+	env.seedPolicy(EntityTypeDeadline, LifecycleCreate, "associate")
+	deadlineID := env.seedDeadline(time.Now().AddDate(0, 0, 14))
+
+	tx, err := env.pool.BeginTxx(ctx, nil)
+	if err != nil {
+		t.Fatalf("begin: %v", err)
+	}
+	reqID, err := env.approvals.SubmitCreate(ctx, tx, env.projectID, deadlineID, env.requester, EntityTypeDeadline, nil)
+	if err != nil {
+		tx.Rollback()
+		t.Fatalf("SubmitCreate: %v", err)
+	}
+	if reqID == nil {
+		tx.Rollback()
+		t.Fatal("SubmitCreate returned nil request id")
+	}
+	if err := tx.Commit(); err != nil {
+		t.Fatalf("commit: %v", err)
+	}
+
+	cases := []struct {
+		name           string
+		viewer         uuid.UUID
+		wantCanApprove bool
+		wantIsRequester bool
+	}{
+		{"self_authored", env.requester, false, true},
+		{"eligible_approver", env.approver, true, false},
+		{"non_eligible_viewer", env.other, false, false},
+		{"global_admin", adminID, true, false},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			row, err := env.approvals.GetRequest(ctx, c.viewer, *reqID)
+			if err != nil {
+				t.Fatalf("GetRequest: %v", err)
+			}
+			if row == nil {
+				t.Fatal("GetRequest returned nil — request should exist")
+			}
+			if row.ViewerCanApprove != c.wantCanApprove {
+				t.Errorf("viewer_can_approve = %v, want %v",
+					row.ViewerCanApprove, c.wantCanApprove)
+			}
+			if row.ViewerIsRequester != c.wantIsRequester {
+				t.Errorf("viewer_is_requester = %v, want %v",
+					row.ViewerIsRequester, c.wantIsRequester)
+			}
+		})
+	}
+
+	// ListPendingForApprover stamps the same flags. The approver runs the
+	// query; they should see one row with viewer_can_approve=true,
+	// viewer_is_requester=false.
+	pending, err := env.approvals.ListPendingForApprover(ctx, env.approver, InboxFilter{})
+	if err != nil {
+		t.Fatalf("ListPendingForApprover: %v", err)
+	}
+	if len(pending) != 1 {
+		t.Fatalf("len(pending) = %d, want 1", len(pending))
+	}
+	if !pending[0].ViewerCanApprove {
+		t.Error("ListPendingForApprover: viewer_can_approve = false, want true")
+	}
+	if pending[0].ViewerIsRequester {
+		t.Error("ListPendingForApprover: viewer_is_requester = true, want false")
+	}
+
+	// ListSubmittedByUser carries them too. Requester runs the query; the
+	// one row must have viewer_can_approve=false (self-approval blocked)
+	// and viewer_is_requester=true.
+	mine, err := env.approvals.ListSubmittedByUser(ctx, env.requester, InboxFilter{})
+	if err != nil {
+		t.Fatalf("ListSubmittedByUser: %v", err)
+	}
+	if len(mine) != 1 {
+		t.Fatalf("len(mine) = %d, want 1", len(mine))
+	}
+	if mine[0].ViewerCanApprove {
+		t.Error("ListSubmittedByUser: viewer_can_approve = true on self-authored row, want false")
+	}
+	if !mine[0].ViewerIsRequester {
+		t.Error("ListSubmittedByUser: viewer_is_requester = false on self-authored row, want true")
+	}
+}