mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(ceo-approve): require substantive notes; panel collects them
ceo-approve was bound to QANotes (notes required), so the panel's
one-click approve (posts {}) 422'd. The wrong fix is to waive notes —
that empties the audit record for a production merge. Instead require
substantive notes (>=20 chars, mirroring pass-qa) and make the panel
COLLECT them: a new CeoApproveDialog (mirrors the reject dialog) gates
the 'Approve & Merge' action, and the dashboard approval-queue enforces
the same before POSTing. The CEO sign-off note is now always captured.
This commit is contained in:
@@ -7,6 +7,7 @@ import { useCreateBranch, useCreatePR } from "@/hooks/use-git";
|
|||||||
import { TaskHeader, TaskMetadata, TaskTabs } from "@/components/tasks/task-detail";
|
import { TaskHeader, TaskMetadata, TaskTabs } from "@/components/tasks/task-detail";
|
||||||
import {
|
import {
|
||||||
EscalateToCeoDialog,
|
EscalateToCeoDialog,
|
||||||
|
CeoApproveDialog,
|
||||||
CeoRejectDialog,
|
CeoRejectDialog,
|
||||||
CreateBranchDialog,
|
CreateBranchDialog,
|
||||||
CreatePRDialog,
|
CreatePRDialog,
|
||||||
@@ -32,6 +33,7 @@ export default function TaskDetailPage({ params }: TaskDetailPageProps) {
|
|||||||
|
|
||||||
// Dialog states
|
// Dialog states
|
||||||
const [escalateDialogOpen, setEscalateDialogOpen] = useState(false);
|
const [escalateDialogOpen, setEscalateDialogOpen] = useState(false);
|
||||||
|
const [approveDialogOpen, setApproveDialogOpen] = useState(false);
|
||||||
const [rejectDialogOpen, setRejectDialogOpen] = useState(false);
|
const [rejectDialogOpen, setRejectDialogOpen] = useState(false);
|
||||||
const [branchDialogOpen, setBranchDialogOpen] = useState(false);
|
const [branchDialogOpen, setBranchDialogOpen] = useState(false);
|
||||||
const [prDialogOpen, setPrDialogOpen] = useState(false);
|
const [prDialogOpen, setPrDialogOpen] = useState(false);
|
||||||
@@ -103,9 +105,8 @@ export default function TaskDetailPage({ params }: TaskDetailPageProps) {
|
|||||||
toast.success("Submitted for PM review");
|
toast.success("Submitted for PM review");
|
||||||
break;
|
break;
|
||||||
case "ceo-approve":
|
case "ceo-approve":
|
||||||
await lifecycle.ceoApprove.mutateAsync({ taskId: task.id });
|
setApproveDialogOpen(true);
|
||||||
toast.success("Task approved and completed");
|
return; // Don't refetch yet — dialog collects the required note
|
||||||
break;
|
|
||||||
case "ceo-reject":
|
case "ceo-reject":
|
||||||
setRejectDialogOpen(true);
|
setRejectDialogOpen(true);
|
||||||
return; // Don't refetch yet, dialog will handle it
|
return; // Don't refetch yet, dialog will handle it
|
||||||
@@ -167,6 +168,19 @@ export default function TaskDetailPage({ params }: TaskDetailPageProps) {
|
|||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
|
const handleCeoApprove = async (notes: string) => {
|
||||||
|
if (!task) return;
|
||||||
|
try {
|
||||||
|
await lifecycle.ceoApprove.mutateAsync({ taskId: task.id, notes });
|
||||||
|
toast.success("Task approved and completed");
|
||||||
|
setApproveDialogOpen(false);
|
||||||
|
refetch();
|
||||||
|
} catch (err) {
|
||||||
|
toast.error("Failed to approve task");
|
||||||
|
console.error(err);
|
||||||
|
}
|
||||||
|
};
|
||||||
|
|
||||||
const handleCreateBranch = async (branchType: string) => {
|
const handleCreateBranch = async (branchType: string) => {
|
||||||
if (!task || !project) return;
|
if (!task || !project) return;
|
||||||
try {
|
try {
|
||||||
@@ -280,6 +294,13 @@ export default function TaskDetailPage({ params }: TaskDetailPageProps) {
|
|||||||
isPending={lifecycle.escalateToCeo.isPending}
|
isPending={lifecycle.escalateToCeo.isPending}
|
||||||
/>
|
/>
|
||||||
|
|
||||||
|
<CeoApproveDialog
|
||||||
|
open={approveDialogOpen}
|
||||||
|
onOpenChange={setApproveDialogOpen}
|
||||||
|
onConfirm={handleCeoApprove}
|
||||||
|
isPending={lifecycle.ceoApprove.isPending}
|
||||||
|
/>
|
||||||
|
|
||||||
<CeoRejectDialog
|
<CeoRejectDialog
|
||||||
open={rejectDialogOpen}
|
open={rejectDialogOpen}
|
||||||
onOpenChange={setRejectDialogOpen}
|
onOpenChange={setRejectDialogOpen}
|
||||||
|
|||||||
@@ -83,7 +83,13 @@ export function CeoApprovalQueue({ className }: CeoApprovalQueueProps) {
|
|||||||
if (!selectedTask) return;
|
if (!selectedTask) return;
|
||||||
|
|
||||||
if (actionType === "approve") {
|
if (actionType === "approve") {
|
||||||
approveMutation.mutate({ taskId: selectedTask.id, notes: notes || undefined });
|
// The approval note is the audit record for merging to production —
|
||||||
|
// required and substantive (>= 20 chars), matching the server gate.
|
||||||
|
if (notes.trim().length < 20) {
|
||||||
|
toast.error("Approval notes are required (>= 20 characters)");
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
approveMutation.mutate({ taskId: selectedTask.id, notes: notes.trim() });
|
||||||
} else if (actionType === "reject") {
|
} else if (actionType === "reject") {
|
||||||
if (!notes.trim()) {
|
if (!notes.trim()) {
|
||||||
toast.error("Rejection reason is required");
|
toast.error("Rejection reason is required");
|
||||||
|
|||||||
@@ -139,6 +139,72 @@ export function CeoRejectDialog({
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// CEO Approve Dialog — the sign-off note is the audit record for merging to
|
||||||
|
// production, so it is REQUIRED and must be substantive (>= 20 chars), matching
|
||||||
|
// the server's CEO_NOTES_REQUIRED gate.
|
||||||
|
const _CEO_NOTES_MIN = 20;
|
||||||
|
|
||||||
|
interface CeoApproveDialogProps {
|
||||||
|
open: boolean;
|
||||||
|
onOpenChange: (open: boolean) => void;
|
||||||
|
onConfirm: (notes: string) => void;
|
||||||
|
isPending?: boolean;
|
||||||
|
}
|
||||||
|
|
||||||
|
export function CeoApproveDialog({
|
||||||
|
open,
|
||||||
|
onOpenChange,
|
||||||
|
onConfirm,
|
||||||
|
isPending,
|
||||||
|
}: CeoApproveDialogProps) {
|
||||||
|
const [notes, setNotes] = useState("");
|
||||||
|
const tooShort = notes.trim().length < _CEO_NOTES_MIN;
|
||||||
|
|
||||||
|
const handleConfirm = () => {
|
||||||
|
if (!tooShort) {
|
||||||
|
onConfirm(notes.trim());
|
||||||
|
setNotes("");
|
||||||
|
}
|
||||||
|
};
|
||||||
|
|
||||||
|
return (
|
||||||
|
<Dialog open={open} onOpenChange={onOpenChange}>
|
||||||
|
<DialogContent>
|
||||||
|
<DialogHeader>
|
||||||
|
<DialogTitle>Approve & Merge</DialogTitle>
|
||||||
|
<DialogDescription>
|
||||||
|
Record why this work is approved for production. This note is the
|
||||||
|
permanent audit record for the merge and is required.
|
||||||
|
</DialogDescription>
|
||||||
|
</DialogHeader>
|
||||||
|
<div className="grid gap-4 py-4">
|
||||||
|
<div className="grid gap-2">
|
||||||
|
<Label htmlFor="ceo-approve-notes">Approval notes</Label>
|
||||||
|
<Textarea
|
||||||
|
id="ceo-approve-notes"
|
||||||
|
value={notes}
|
||||||
|
onChange={(e) => setNotes(e.target.value)}
|
||||||
|
placeholder="Verified against acceptance criteria; approving for production because..."
|
||||||
|
rows={4}
|
||||||
|
/>
|
||||||
|
<p className="text-xs text-muted-foreground">
|
||||||
|
{notes.trim().length}/{_CEO_NOTES_MIN} characters minimum
|
||||||
|
</p>
|
||||||
|
</div>
|
||||||
|
</div>
|
||||||
|
<DialogFooter>
|
||||||
|
<Button variant="outline" onClick={() => onOpenChange(false)}>
|
||||||
|
Cancel
|
||||||
|
</Button>
|
||||||
|
<Button onClick={handleConfirm} disabled={tooShort || isPending}>
|
||||||
|
{isPending ? "Approving..." : "Approve & Merge"}
|
||||||
|
</Button>
|
||||||
|
</DialogFooter>
|
||||||
|
</DialogContent>
|
||||||
|
</Dialog>
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
// Create Branch Dialog
|
// Create Branch Dialog
|
||||||
interface CreateBranchDialogProps {
|
interface CreateBranchDialogProps {
|
||||||
open: boolean;
|
open: boolean;
|
||||||
|
|||||||
@@ -1266,8 +1266,21 @@ async def ceo_approve_task(
|
|||||||
status_code=status.HTTP_404_NOT_FOUND, detail="Task not found"
|
status_code=status.HTTP_404_NOT_FOUND, detail="Task not found"
|
||||||
)
|
)
|
||||||
|
|
||||||
notes = data.notes if data else None
|
# The CEO sign-off note is the audit record for merging to production —
|
||||||
task = await service.ceo_approve(task_id, notes)
|
# it must be present and substantive. An approval with no rationale leaves
|
||||||
|
# the audit trail empty, so reject it (the panel collects the note before
|
||||||
|
# POSTing). Order mirrors pass-qa: 404 before the notes gate.
|
||||||
|
if not data or not data.notes or len(data.notes.strip()) < _MIN_NOTES_CHARS:
|
||||||
|
raise HTTPException(
|
||||||
|
status_code=status.HTTP_400_BAD_REQUEST,
|
||||||
|
detail=(
|
||||||
|
"CEO_NOTES_REQUIRED: CEO approval must include notes (>=20 "
|
||||||
|
"chars) recording why the work is approved for production. "
|
||||||
|
"POST /api/tasks/{id}/ceo-approve with notes='...'."
|
||||||
|
),
|
||||||
|
)
|
||||||
|
|
||||||
|
task = await service.ceo_approve(task_id, data.notes)
|
||||||
if not task:
|
if not task:
|
||||||
raise HTTPException(
|
raise HTTPException(
|
||||||
status_code=status.HTTP_400_BAD_REQUEST,
|
status_code=status.HTTP_400_BAD_REQUEST,
|
||||||
|
|||||||
@@ -2434,7 +2434,7 @@ async def test_ceo_approve_service_returns_none(ceo_client: dict) -> None:
|
|||||||
await ceo_client["db"].flush()
|
await ceo_client["db"].flush()
|
||||||
response = await ceo_client["client"].post(
|
response = await ceo_client["client"].post(
|
||||||
f"/api/tasks/{task.id}/ceo-approve",
|
f"/api/tasks/{task.id}/ceo-approve",
|
||||||
json={"notes": "approved"},
|
json={"notes": "Reviewed and approved for production release."},
|
||||||
headers=_HDR,
|
headers=_HDR,
|
||||||
)
|
)
|
||||||
assert response.status_code == HTTPStatus.BAD_REQUEST
|
assert response.status_code == HTTPStatus.BAD_REQUEST
|
||||||
@@ -2452,12 +2452,31 @@ async def test_ceo_approve_success(ceo_client: dict) -> None:
|
|||||||
mock_factory.return_value = instance
|
mock_factory.return_value = instance
|
||||||
response = await ceo_client["client"].post(
|
response = await ceo_client["client"].post(
|
||||||
f"/api/tasks/{task.id}/ceo-approve",
|
f"/api/tasks/{task.id}/ceo-approve",
|
||||||
json={"notes": "approved"},
|
json={"notes": "Verified against all acceptance criteria; approved."},
|
||||||
headers=_HDR,
|
headers=_HDR,
|
||||||
)
|
)
|
||||||
assert response.status_code == HTTPStatus.OK
|
assert response.status_code == HTTPStatus.OK
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_ceo_approve_without_notes_rejected(ceo_client: dict) -> None:
|
||||||
|
"""Audit: a CEO approval with no/thin notes leaves no record of WHY the
|
||||||
|
work shipped, so the endpoint must reject it (>= 20 chars required). The
|
||||||
|
panel collects the note before POSTing."""
|
||||||
|
task = _seed_task_ceo(ceo_client)
|
||||||
|
await ceo_client["db"].flush()
|
||||||
|
for body in ({}, {"notes": ""}, {"notes": "lgtm"}):
|
||||||
|
response = await ceo_client["client"].post(
|
||||||
|
f"/api/tasks/{task.id}/ceo-approve",
|
||||||
|
json=body,
|
||||||
|
headers=_HDR,
|
||||||
|
)
|
||||||
|
assert response.status_code in (
|
||||||
|
HTTPStatus.BAD_REQUEST,
|
||||||
|
HTTPStatus.UNPROCESSABLE_ENTITY,
|
||||||
|
), (body, response.status_code)
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_ceo_reject_task_not_found(ceo_client: dict) -> None:
|
async def test_ceo_reject_task_not_found(ceo_client: dict) -> None:
|
||||||
response = await ceo_client["client"].post(
|
response = await ceo_client["client"].post(
|
||||||
|
|||||||
Reference in New Issue
Block a user