From 0fc2427f71bbb348d6d6d40812cef57cfed28832 Mon Sep 17 00:00:00 2001 From: kitos Date: Fri, 10 Jul 2026 16:19:14 +0200 Subject: [PATCH] feat(review): review queue for leads + manual reviewer reassignment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Leads get the same two-queue view operators already have: 'Available to Review' (pending reviews currently assigned to a peer lead) and 'My Assigned Reviews' (assigned to me), replacing the old single 'My Reviews' toggle. Since the load-balanced auto-assignment always picks a reviewer immediately, 'available' means peer-assigned reviews a lead could pick up if the assignee can't get to them, not unclaimed ones (there aren't any). POST /tests/{id}/assign now also accepts red_reviewer_assignee / blue_reviewer_assignee, validated against the matching lead role and synced to Jira the same way operator assignment already is. The AssigneeControl UI gained a 'reviewer' kind (leads-only picker) shown on the test detail header while a test sits in red_review/blue_review — this also fixes the reviewer assignment being invisible in Aegis even though it was already being pushed to Jira correctly. --- backend/app/routers/tests.py | 43 +++++-- backend/app/schemas/test.py | 4 +- backend/tests/test_assign_operators.py | 74 +++++++++++ frontend/src/api/tests.ts | 9 +- .../test-detail/AssigneeControl.tsx | 29 ++++- .../test-detail/TestDetailHeader.tsx | 33 ++++- frontend/src/pages/TestDetailPage.tsx | 10 +- frontend/src/pages/TestsPage.tsx | 120 +++++++++++------- 8 files changed, 250 insertions(+), 72 deletions(-) diff --git a/backend/app/routers/tests.py b/backend/app/routers/tests.py index d8d1a35..7399ad9 100644 --- a/backend/app/routers/tests.py +++ b/backend/app/routers/tests.py @@ -1280,44 +1280,65 @@ def assign_test_operators( db: Session = Depends(get_db), current_user: User = Depends(require_any_role_strict("manager", "red_lead", "blue_lead")), ): - """Assign red_tech and/or blue_tech operators to a test. Leads/managers only — not admin, who administers the site rather than coordinating operators.""" + """Assign red/blue tech operators and/or reviewers to a test. Leads/managers only — not admin, who administers the site rather than coordinating people.""" test = crud_get_test_or_raise(db, test_id) - newly_assigned: User | None = None + newly_assigned: list[User] = [] if payload.red_tech_assignee is not None: u = db.query(User).filter(User.id == payload.red_tech_assignee).first() if not u or u.role not in ("red_tech", "red_lead"): raise HTTPException(status_code=400, detail="Invalid red tech assignee") test.red_tech_assignee = payload.red_tech_assignee - newly_assigned = u + newly_assigned.append(u) if payload.blue_tech_assignee is not None: u = db.query(User).filter(User.id == payload.blue_tech_assignee).first() if not u or u.role not in ("blue_tech", "blue_lead"): raise HTTPException(status_code=400, detail="Invalid blue tech assignee") test.blue_tech_assignee = payload.blue_tech_assignee - newly_assigned = u + newly_assigned.append(u) + + if payload.red_reviewer_assignee is not None: + u = db.query(User).filter(User.id == payload.red_reviewer_assignee).first() + if not u or u.role != "red_lead": + raise HTTPException(status_code=400, detail="Invalid red reviewer — must be a red_lead") + test.red_reviewer_assignee = payload.red_reviewer_assignee + newly_assigned.append(u) + + if payload.blue_reviewer_assignee is not None: + u = db.query(User).filter(User.id == payload.blue_reviewer_assignee).first() + if not u or u.role != "blue_lead": + raise HTTPException(status_code=400, detail="Invalid blue reviewer — must be a blue_lead") + test.blue_reviewer_assignee = payload.blue_reviewer_assignee + newly_assigned.append(u) # Handle intentional null (clearing) — model_fields_set tracks which keys were sent if "red_tech_assignee" in payload.model_fields_set and payload.red_tech_assignee is None: test.red_tech_assignee = None if "blue_tech_assignee" in payload.model_fields_set and payload.blue_tech_assignee is None: test.blue_tech_assignee = None + if "red_reviewer_assignee" in payload.model_fields_set and payload.red_reviewer_assignee is None: + test.red_reviewer_assignee = None + if "blue_reviewer_assignee" in payload.model_fields_set and payload.blue_reviewer_assignee is None: + test.blue_reviewer_assignee = None log_action(db, current_user.id, "assign_test", str(test_id), { "red_tech_assignee": str(payload.red_tech_assignee) if payload.red_tech_assignee else None, "blue_tech_assignee": str(payload.blue_tech_assignee) if payload.blue_tech_assignee else None, + "red_reviewer_assignee": str(payload.red_reviewer_assignee) if payload.red_reviewer_assignee else None, + "blue_reviewer_assignee": str(payload.blue_reviewer_assignee) if payload.blue_reviewer_assignee else None, }) db.commit() db.refresh(test) - if newly_assigned is not None: - try: - from app.services.jira_service import push_assignee_update - push_assignee_update(db, test, newly_assigned) - db.commit() - except Exception: # nosec B110 - pass # jira_service already logs warnings internally + if newly_assigned: + from app.services.jira_service import push_assignee_update + for assignee in newly_assigned: + try: + push_assignee_update(db, test, assignee) + db.commit() + except Exception: # nosec B110 + pass # jira_service already logs warnings internally return test diff --git a/backend/app/schemas/test.py b/backend/app/schemas/test.py index d59e419..a06cfef 100644 --- a/backend/app/schemas/test.py +++ b/backend/app/schemas/test.py @@ -171,10 +171,12 @@ class TestRemediationUpdate(BaseModel): class TestAssign(BaseModel): - """Payload for assigning operators to a test.""" + """Payload for assigning operators or reviewers to a test.""" red_tech_assignee: uuid.UUID | None = None blue_tech_assignee: uuid.UUID | None = None + red_reviewer_assignee: uuid.UUID | None = None + blue_reviewer_assignee: uuid.UUID | None = None class TestHold(BaseModel): diff --git a/backend/tests/test_assign_operators.py b/backend/tests/test_assign_operators.py index 50c6f59..e2a8105 100644 --- a/backend/tests/test_assign_operators.py +++ b/backend/tests/test_assign_operators.py @@ -148,3 +148,77 @@ def test_assigning_operator_syncs_to_jira(client, db, red_lead_headers, red_lead mock_push.assert_called_once() assert captured["test_id"] == test.id assert captured["assignee_id"] == red_tech_user.id + + +def _make_second_red_lead(db): + from app.auth import hash_password + from app.models.user import User + u = User( + username="redlead2", email="redlead2@test.com", + hashed_password=hash_password("x"), role="red_lead", is_active=True, + must_change_password=False, + ) + db.add(u) + db.commit() + db.refresh(u) + return u + + +def test_lead_can_reassign_red_reviewer_to_peer(client, db, red_lead_headers, red_lead_user): + """A lead can hand a review off to a peer lead — e.g. if the currently + assigned reviewer can't get to it.""" + technique = _seed_technique(db) + test = _seed_test(db, technique, red_lead_user.id) + peer = _make_second_red_lead(db) + test.red_reviewer_assignee = red_lead_user.id + db.commit() + + resp = client.post( + f"/api/v1/tests/{test.id}/assign", + json={"red_reviewer_assignee": str(peer.id)}, + headers=red_lead_headers, + ) + assert resp.status_code == 200, resp.text + assert resp.json()["red_reviewer_assignee"] == str(peer.id) + + db.refresh(test) + assert test.red_reviewer_assignee == peer.id + + +def test_reviewer_reassign_rejects_non_lead(client, db, red_lead_headers, red_lead_user, red_tech_user): + """Only a red_lead can be the red reviewer — a red_tech is not eligible.""" + technique = _seed_technique(db) + test = _seed_test(db, technique, red_lead_user.id) + + resp = client.post( + f"/api/v1/tests/{test.id}/assign", + json={"red_reviewer_assignee": str(red_tech_user.id)}, + headers=red_lead_headers, + ) + assert resp.status_code == 400 + + +def test_reviewer_reassign_rejects_wrong_side(client, db, red_lead_headers, red_lead_user, blue_lead_user): + """A blue_lead can't be set as the red reviewer.""" + technique = _seed_technique(db) + test = _seed_test(db, technique, red_lead_user.id) + + resp = client.post( + f"/api/v1/tests/{test.id}/assign", + json={"red_reviewer_assignee": str(blue_lead_user.id)}, + headers=red_lead_headers, + ) + assert resp.status_code == 400 + + +def test_manager_can_reassign_blue_reviewer(client, db, manager_headers, manager_user, blue_lead_user): + technique = _seed_technique(db) + test = _seed_test(db, technique, manager_user.id) + + resp = client.post( + f"/api/v1/tests/{test.id}/assign", + json={"blue_reviewer_assignee": str(blue_lead_user.id)}, + headers=manager_headers, + ) + assert resp.status_code == 200, resp.text + assert resp.json()["blue_reviewer_assignee"] == str(blue_lead_user.id) diff --git a/frontend/src/api/tests.ts b/frontend/src/api/tests.ts index 92f4f40..b3355d0 100644 --- a/frontend/src/api/tests.ts +++ b/frontend/src/api/tests.ts @@ -179,10 +179,15 @@ export async function updateTestClassification( return data; } -/** Assign red_tech/blue_tech operators to a test (leads + managers only). */ +/** Assign operators or hand off a review to a peer lead (leads + managers only). */ export async function assignTestOperators( testId: string, - payload: { red_tech_assignee?: string | null; blue_tech_assignee?: string | null }, + payload: { + red_tech_assignee?: string | null; + blue_tech_assignee?: string | null; + red_reviewer_assignee?: string | null; + blue_reviewer_assignee?: string | null; + }, ): Promise { const { data } = await client.post(`/tests/${testId}/assign`, payload); return data; diff --git a/frontend/src/components/test-detail/AssigneeControl.tsx b/frontend/src/components/test-detail/AssigneeControl.tsx index bd0ea5e..ee528aa 100644 --- a/frontend/src/components/test-detail/AssigneeControl.tsx +++ b/frontend/src/components/test-detail/AssigneeControl.tsx @@ -11,6 +11,10 @@ interface Props { onAssign: (userId: string | null) => void; /** "sm" = compact pill (default), "lg" = full-size button matching the action bar. */ size?: "sm" | "lg"; + /** "operator" (default) = red_tech_assignee/blue_tech_assignee, picking from + * techs+leads. "reviewer" = red_reviewer_assignee/blue_reviewer_assignee, + * picking from leads only — for handing a review off to a peer lead. */ + kind?: "operator" | "reviewer"; } const SIDE_STYLE = { @@ -18,20 +22,31 @@ const SIDE_STYLE = { blue: "border-indigo-500/40 bg-indigo-900/20 text-indigo-400 hover:bg-indigo-900/40", }; -const SIDE_ROLES: Record<"red" | "blue", string[]> = { - red: ["red_tech", "red_lead"], - blue: ["blue_tech", "blue_lead"], +const ELIGIBLE_ROLES: Record<"operator" | "reviewer", Record<"red" | "blue", string[]>> = { + operator: { + red: ["red_tech", "red_lead"], + blue: ["blue_tech", "blue_lead"], + }, + reviewer: { + red: ["red_lead"], + blue: ["blue_lead"], + }, }; -/** Lead/manager picker for the red_tech_assignee / blue_tech_assignee fields. */ +const LABEL_PREFIX: Record<"operator" | "reviewer", Record<"red" | "blue", string>> = { + operator: { red: "RT", blue: "BT" }, + reviewer: { red: "Reviewer", blue: "Reviewer" }, +}; + +/** Lead/manager picker for operator assignment or reviewer hand-off. */ export default function AssigneeControl({ - side, assigneeId, operators, canEdit, isSaving, onAssign, size = "sm", + side, assigneeId, operators, canEdit, isSaving, onAssign, size = "sm", kind = "operator", }: Props) { const [expanded, setExpanded] = useState(false); const current = operators.find((o) => o.id === assigneeId); const label = current ? current.username : "Unassigned"; - const eligible = operators.filter((o) => SIDE_ROLES[side].includes(o.role)); - const sideLabel = side === "red" ? "RT" : "BT"; + const eligible = operators.filter((o) => ELIGIBLE_ROLES[kind][side].includes(o.role)); + const sideLabel = LABEL_PREFIX[kind][side]; const badgeClass = size === "lg" diff --git a/frontend/src/components/test-detail/TestDetailHeader.tsx b/frontend/src/components/test-detail/TestDetailHeader.tsx index 2d47054..ae13be4 100644 --- a/frontend/src/components/test-detail/TestDetailHeader.tsx +++ b/frontend/src/components/test-detail/TestDetailHeader.tsx @@ -93,7 +93,10 @@ interface TestDetailHeaderProps { onUpdateClassification: (value: DataClassification) => void; isUpdatingClassification: boolean; operators: OperatorOut[]; - onAssignOperator: (side: "red" | "blue", userId: string | null) => void; + onAssignOperator: ( + field: "red_tech_assignee" | "blue_tech_assignee" | "red_reviewer_assignee" | "blue_reviewer_assignee", + userId: string | null, + ) => void; isAssigningOperator: boolean; } @@ -551,13 +554,37 @@ export default function TestDetailHeader({
+ {test.state === "red_review" && ( + onAssignOperator("red_reviewer_assignee", userId)} + size="lg" + /> + )} + {test.state === "blue_review" && ( + onAssignOperator("blue_reviewer_assignee", userId)} + size="lg" + /> + )} onAssignOperator("red", userId)} + onAssign={(userId) => onAssignOperator("red_tech_assignee", userId)} size="lg" /> onAssignOperator("blue", userId)} + onAssign={(userId) => onAssignOperator("blue_tech_assignee", userId)} size="lg" />
diff --git a/frontend/src/pages/TestDetailPage.tsx b/frontend/src/pages/TestDetailPage.tsx index 295ca30..3d9121d 100644 --- a/frontend/src/pages/TestDetailPage.tsx +++ b/frontend/src/pages/TestDetailPage.tsx @@ -385,8 +385,12 @@ export default function TestDetailPage() { }); const assignOperatorMutation = useMutation({ - mutationFn: ({ side, userId }: { side: "red" | "blue"; userId: string | null }) => - assignTestOperators(testId!, side === "red" ? { red_tech_assignee: userId } : { blue_tech_assignee: userId }), + mutationFn: ({ + field, userId, + }: { + field: "red_tech_assignee" | "blue_tech_assignee" | "red_reviewer_assignee" | "blue_reviewer_assignee"; + userId: string | null; + }) => assignTestOperators(testId!, { [field]: userId }), onSuccess: () => { invalidateAll(); showToast("Assignment updated", "success"); @@ -559,7 +563,7 @@ export default function TestDetailPage() { onUpdateClassification={(value) => updateClassificationMutation.mutate(value)} isUpdatingClassification={updateClassificationMutation.isPending} operators={operators} - onAssignOperator={(side, userId) => assignOperatorMutation.mutate({ side, userId })} + onAssignOperator={(field, userId) => assignOperatorMutation.mutate({ field, userId })} isAssigningOperator={assignOperatorMutation.isPending} /> diff --git a/frontend/src/pages/TestsPage.tsx b/frontend/src/pages/TestsPage.tsx index 4d0dfea..cc237f6 100644 --- a/frontend/src/pages/TestsPage.tsx +++ b/frontend/src/pages/TestsPage.tsx @@ -149,8 +149,8 @@ export default function TestsPage() { const [platformFilter, setPlatformFilter] = useState(""); const [searchText, setSearchText] = useState(""); const [showMyTasks, setShowMyTasks] = useState(false); - const [showMyReviews, setShowMyReviews] = useState(false); const isReviewLead = user?.role === "red_lead" || user?.role === "blue_lead"; + const reviewQueueActive = isReviewLead && !showMyTasks; // ── Sort state ──────────────────────────────────────────────────── const [sortKey, setSortKey] = useState("created_at"); @@ -169,13 +169,7 @@ export default function TestsPage() { const filters = useMemo(() => { const f: TestListFilters = { limit: 200 }; - if (showMyReviews && user && isReviewLead) { - // "My reviews" — tests assigned to ME at the red_review/blue_review - // gate. Distinct from "My Tasks" below, which covers the later - // in_review manager-validation stage regardless of assignment. - f.state = user.role === "red_lead" ? "red_review" : "blue_review"; - f.reviewer_id = user.id; - } else if (showMyTasks && user) { + if (showMyTasks && user) { switch (user.role) { case "red_tech": f.created_by = user.id; @@ -200,7 +194,7 @@ export default function TestsPage() { if (platformFilter) f.platform = platformFilter; return f; - }, [stateFilter, platformFilter, showMyTasks, showMyReviews, isReviewLead, user]); + }, [stateFilter, platformFilter, showMyTasks, user]); const { data: allTests, @@ -361,6 +355,39 @@ export default function TestsPage() { return { availableTests: [] as typeof tests, myAssignedTests: [] as typeof tests }; }, [techRole, user, allTests, searchText, platformFilter]); + // ── Two-queue split for lead review gate (red_review / blue_review) ─ + // The auto load-balancer always assigns a reviewer immediately, so + // "available" here means "assigned to another lead" — visible so a + // lead can pick up a peer's review if they can't get to it. + const { availableReviews, myAssignedReviews } = useMemo(() => { + if (!isReviewLead || !user || !allTests || showMyTasks) { + return { availableReviews: [] as typeof tests, myAssignedReviews: [] as typeof tests }; + } + + let searchFiltered = allTests; + if (searchText.trim()) { + const q = searchText.toLowerCase(); + searchFiltered = searchFiltered.filter( + (t) => + t.name.toLowerCase().includes(q) || + (t.technique_mitre_id && t.technique_mitre_id.toLowerCase().includes(q)) || + (t.technique_name && t.technique_name.toLowerCase().includes(q)) + ); + } + if (platformFilter) { + searchFiltered = searchFiltered.filter((t) => + t.platform?.toLowerCase().includes(platformFilter.toLowerCase()) + ); + } + + const reviewState = user.role === "red_lead" ? "red_review" : "blue_review"; + const reviewerField = user.role === "red_lead" ? "red_reviewer_assignee" : "blue_reviewer_assignee"; + const inState = searchFiltered.filter((t) => t.state === reviewState); + const available = inState.filter((t) => t[reviewerField] !== user.id); + const mine = inState.filter((t) => t[reviewerField] === user.id); + return { availableReviews: available, myAssignedReviews: mine }; + }, [isReviewLead, user, allTests, searchText, platformFilter, showMyTasks]); + // ── Formatting helpers ───────────────────────────────────────────── const formatDate = (dateStr: string | null | undefined) => { if (!dateStr) return "-"; @@ -500,13 +527,14 @@ export default function TestsPage() { {/* ── Filters Bar ───────────────────────────────────────────────── */}
- {/* My tasks toggle — hidden for tech roles (they have the two-queue view below) */} + {/* My tasks toggle — hidden for tech roles (two-queue view below); + for leads this switches AWAY from the review two-queue view + to the in_review dual-validation stage, a distinct responsibility. */} {user?.role !== "admin" && user?.role !== "viewer" && !techRole && ( )} - {/* My reviews toggle — red_lead/blue_lead only, the red_review/blue_review - lead-review gate assignment queue (distinct from "My Tasks" above, - which covers the later in_review manager-validation stage). */} - {isReviewLead && ( - - )} - {/* State filter */}
@@ -590,14 +595,13 @@ export default function TestsPage() {
{/* Clear filters */} - {(stateFilter || platformFilter || searchText || showMyTasks || showMyReviews) && ( + {(stateFilter || platformFilter || searchText || showMyTasks) && (
{/* Active filter summary */} - {(stateFilter || showMyTasks || showMyReviews) && ( + {(stateFilter || showMyTasks) && (
Showing: - {showMyReviews && ( - - My Reviews - - )} {showMyTasks && ( {myTasksLabel} @@ -664,16 +663,47 @@ export default function TestsPage() {
+ ) : reviewQueueActive ? ( + /* Two-section review queue for red_lead/blue_lead */ +
+ {/* Available to Review */} +
+
+
+

Available to Review

+ + {availableReviews.length} + +
+ Assigned to other leads — pick one up if needed +
+ +
+ + {/* My Assigned Reviews */} +
+
+
+

My Assigned Reviews

+ + {myAssignedReviews.length} + +
+ Reviews assigned to you +
+ +
+
) : ( - /* Normal single-table view for leads, admin, viewer */ + /* Normal single-table view for admin, viewer, and leads viewing "My Tasks" */

- {showMyReviews ? "My Reviews" : showMyTasks ? myTasksLabel : "All Tests"} + {showMyTasks ? myTasksLabel : "All Tests"}

{tests.length} tests
- +
)}