From 61e6037e972ba800b04c744052ac20d7e9693a8a Mon Sep 17 00:00:00 2001 From: kitos Date: Wed, 3 Jun 2026 12:21:47 +0200 Subject: [PATCH] feat(tests): disputed state + fix timestamps on reopen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. New 'disputed' state — one lead approved, the other rejected: - Both approved → validated (unchanged) - Both rejected → rejected (unchanged) - One approves + one rejects → disputed (new) - DB: ALTER TYPE teststate ADD VALUE 'disputed' - Notification sent to the approving lead explaining the conflict with the rejection notes 2. Disputed UI in TestDetailHeader: - Amber banner showing conflict + rejection reason from notes - 'Change Vote to Rejected' button for the lead who approved - Validation indicators shown for disputed state too 3. Fix timestamps on reopen (rejected → draft): - Keep red_started_at, blue_started_at etc. as historical record - Only clear paused_at defensively - Timestamps naturally update when test is re-executed 4. disputed badge (amber) added to all badge color maps Co-Authored-By: Claude Sonnet 4.6 --- .../versions/b046_add_disputed_test_state.py | 22 ++++++ backend/app/domain/enums.py | 1 + backend/app/domain/test_entity.py | 32 +++++---- backend/app/services/test_workflow_service.py | 68 +++++++++++++++++-- .../src/components/AddTestToCampaignModal.tsx | 1 + .../test-detail/TestDetailHeader.tsx | 54 ++++++++++++++- frontend/src/pages/CampaignDetailPage.tsx | 1 + frontend/src/pages/DashboardPage.tsx | 1 + frontend/src/pages/TechniqueDetailPage.tsx | 1 + frontend/src/pages/TestsPage.tsx | 3 + frontend/src/types/models.ts | 3 +- 11 files changed, 166 insertions(+), 21 deletions(-) create mode 100644 backend/alembic/versions/b046_add_disputed_test_state.py diff --git a/backend/alembic/versions/b046_add_disputed_test_state.py b/backend/alembic/versions/b046_add_disputed_test_state.py new file mode 100644 index 0000000..e86eeac --- /dev/null +++ b/backend/alembic/versions/b046_add_disputed_test_state.py @@ -0,0 +1,22 @@ +"""Add 'disputed' value to teststate enum. + +Revision ID: b046 +Revises: b045 +Create Date: 2026-06-03 +""" + +from alembic import op + +revision = "b046" +down_revision = "b045" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.execute("ALTER TYPE teststate ADD VALUE IF NOT EXISTS 'disputed'") + + +def downgrade() -> None: + # PostgreSQL does not support removing enum values; downgrade is a no-op. + pass diff --git a/backend/app/domain/enums.py b/backend/app/domain/enums.py index 0f8405a..820f6be 100644 --- a/backend/app/domain/enums.py +++ b/backend/app/domain/enums.py @@ -24,6 +24,7 @@ class TestState(str, enum.Enum): in_review = "in_review" validated = "validated" rejected = "rejected" + disputed = "disputed" # one lead approved, the other rejected class TeamSide(str, enum.Enum): diff --git a/backend/app/domain/test_entity.py b/backend/app/domain/test_entity.py index 182d64b..d9ae907 100644 --- a/backend/app/domain/test_entity.py +++ b/backend/app/domain/test_entity.py @@ -314,21 +314,21 @@ class TestEntity: def check_dual_validation(self) -> None: """Evaluate both leads' votes and advance state if appropriate. - - Both **approved** -> ``validated`` - - Either **rejected** -> ``rejected`` - - Otherwise no change (waiting for the other lead). + Rules (v2 — consensus required): + - Both **approved** -> ``validated`` + - Both **rejected** -> ``rejected`` + - One approved + one rejected -> ``disputed`` (conflict, needs discussion) + - Otherwise (one or both still pending) -> no change Called automatically by :meth:`validate_red` and :meth:`validate_blue`. - Also available as a standalone entry point for backward compatibility - when validation fields are set externally. """ self._check_dual_validation() def _assert_in_review(self, side: str) -> None: - if self.state != TestState.in_review: + if self.state not in (TestState.in_review, TestState.disputed): raise InvalidOperationError( f"Cannot validate {side} side while test is in " - f"'{self.state.value}' state (must be in_review)" + f"'{self.state.value}' state (must be in_review or disputed)" ) @staticmethod @@ -339,11 +339,19 @@ class TestEntity: ) def _check_dual_validation(self) -> None: - """If both leads have voted, advance to validated or rejected.""" + """Advance the test state once both leads have voted.""" r, b = self.red_validation_status, self.blue_validation_status - if r == "rejected" or b == "rejected": - self.state = TestState.rejected - self._events.append(DomainEvent("dual_validation_rejected")) - elif r == "approved" and b == "approved": + + if r == "approved" and b == "approved": self.state = TestState.validated self._events.append(DomainEvent("dual_validation_approved")) + + elif r == "rejected" and b == "rejected": + # Full consensus to reject + self.state = TestState.rejected + self._events.append(DomainEvent("dual_validation_rejected")) + + elif (r == "approved" and b == "rejected") or (r == "rejected" and b == "approved"): + # Conflict: one approves, one rejects → needs discussion + self.state = TestState.disputed + self._events.append(DomainEvent("dual_validation_disputed")) diff --git a/backend/app/services/test_workflow_service.py b/backend/app/services/test_workflow_service.py index 352102e..330fd80 100644 --- a/backend/app/services/test_workflow_service.py +++ b/backend/app/services/test_workflow_service.py @@ -37,7 +37,8 @@ VALID_TRANSITIONS: dict[TestState, list[TestState]] = { TestState.draft: [TestState.red_executing], TestState.red_executing: [TestState.blue_evaluating], TestState.blue_evaluating: [TestState.in_review], - TestState.in_review: [TestState.validated, TestState.rejected], + TestState.in_review: [TestState.validated, TestState.rejected, TestState.disputed], + TestState.disputed: [TestState.validated, TestState.rejected], TestState.rejected: [TestState.draft], TestState.validated: [], # terminal state } @@ -529,6 +530,60 @@ def _dispatch_dual_validation_effects( except Exception as e: logger.warning("Jira push failed for test %s: %s", test.id, e, exc_info=True) + elif event.name == "dual_validation_disputed": + # Notify the lead who APPROVED asking them to review the rejection + _notify_validation_conflict(db, test, actor) + + +def _notify_validation_conflict(db: Session, test: Test, actor: User | None) -> None: + """Notify the lead who APPROVED about the other lead's rejection. + + Tells them: 'The other lead rejected. Review their notes and either + change your vote to rejected or discuss with them to resolve.' + """ + from app.models.user import User as UserModel + + red_approved = test.red_validation_status == "approved" + blue_approved = test.blue_validation_status == "approved" + + # Identify who approved (they need to be notified) + approver_id = None + rejector_role = None + rejection_notes = None + + if red_approved and test.blue_validation_status == "rejected": + approver_id = test.red_validated_by + rejector_role = "Blue Lead" + rejection_notes = test.blue_validation_notes + elif blue_approved and test.red_validation_status == "rejected": + approver_id = test.blue_validated_by + rejector_role = "Red Lead" + rejection_notes = test.red_validation_notes + + if not approver_id: + return + + notes_snippet = f': "{rejection_notes[:200]}"' if rejection_notes else "" + try: + create_notification( + db, + user_id=approver_id, + type="validation_conflict", + title="Validation conflict — action required", + message=( + f"{rejector_role} rejected test '{test.name}' while you approved it{notes_snippet}. " + f"Review their reason and either change your decision to 'rejected' " + f"or contact {rejector_role} to resolve the disagreement." + ), + entity_type="test", + entity_id=str(test.id), + ) + except Exception as e: + logger.warning( + "Failed to send conflict notification for test %s: %s", + test.id, e, exc_info=True, + ) + def handle_remediation_completed(db: Session, test: Test, user: User) -> Test | None: """Create a re-test when remediation is completed. @@ -681,13 +736,12 @@ def reopen_test(db: Session, test: Test, user: User) -> Test: test.blue_validated_at = None # test.blue_validation_notes → KEEP (rejection reason / clarification needed) - # Reset phase timing so the new execution starts fresh - test.red_started_at = None - test.blue_started_at = None - test.blue_work_started_at = None + # Phase timing: kept as historical record of the previous attempt. + # When the team presses "Start Execution" again, red_started_at will be + # overwritten with the new timestamp — no manual reset needed. + # Only the active-pause marker is cleared (it should never be set on a + # rejected test, but clear it defensively to avoid a stuck timer). test.paused_at = None - test.red_paused_seconds = 0 - test.blue_paused_seconds = 0 try: from app.services.jira_service import push_test_event diff --git a/frontend/src/components/AddTestToCampaignModal.tsx b/frontend/src/components/AddTestToCampaignModal.tsx index bc65309..069dcf9 100644 --- a/frontend/src/components/AddTestToCampaignModal.tsx +++ b/frontend/src/components/AddTestToCampaignModal.tsx @@ -26,6 +26,7 @@ const stateBadge: Record = { in_review: "bg-blue-900/50 text-blue-400 border-blue-500/30", validated: "bg-green-900/50 text-green-400 border-green-500/30", rejected: "bg-red-900/50 text-red-400 border-red-500/30", + disputed: "bg-amber-900/50 text-amber-400 border-amber-500/30", }; const SEVERITY_COLORS: Record = { diff --git a/frontend/src/components/test-detail/TestDetailHeader.tsx b/frontend/src/components/test-detail/TestDetailHeader.tsx index 537a0fa..17eed31 100644 --- a/frontend/src/components/test-detail/TestDetailHeader.tsx +++ b/frontend/src/components/test-detail/TestDetailHeader.tsx @@ -8,6 +8,8 @@ import { Loader2, Shield, ShieldCheck, + AlertTriangle, + MessageSquare, } from "lucide-react"; import type { Test, TestState, User } from "../../types/models"; import LiveTimer from "./LiveTimer"; @@ -40,6 +42,7 @@ const STATE_BADGE: Record = { in_review: "bg-blue-900/50 text-blue-400 border-blue-500/30", validated: "bg-green-900/50 text-green-400 border-green-500/30", rejected: "bg-red-900/50 text-red-400 border-red-500/30", + disputed: "bg-amber-900/50 text-amber-400 border-amber-500/30", }; // ── Props ────────────────────────────────────────────────────────── @@ -212,6 +215,30 @@ export default function TestDetailHeader({ ); } + // Disputed: the lead who approved can change their vote to rejected + if (test.state === "disputed") { + const canChangeVote = + (role === "red_lead" && test.red_validation_status === "approved") || + (role === "blue_lead" && test.blue_validation_status === "approved") || + role === "admin"; + + if (canChangeVote) { + const side = role === "red_lead" ? "red" : role === "blue_lead" ? "blue" : null; + if (side || role === "admin") { + buttons.push( + , + ); + } + } + } + // Leads/admin on rejected -> Reopen if ( test.state === "rejected" && @@ -238,7 +265,7 @@ export default function TestDetailHeader({ // ── Dual validation indicators ─────────────────────────────────── const renderValidationIndicators = () => { - if (test.state !== "in_review" && test.state !== "validated" && test.state !== "rejected") { + if (!["in_review", "validated", "rejected", "disputed"].includes(test.state)) { return null; } @@ -347,6 +374,31 @@ export default function TestDetailHeader({ + {/* Disputed conflict banner */} + {test.state === "disputed" && ( +
+ +
+

+ Validation Conflict — Leads Disagree +

+

+ One lead approved and the other rejected this test. + {test.red_validation_status === "rejected" && test.red_validation_notes && ( + <> Red Lead's reason: "{test.red_validation_notes}" + )} + {test.blue_validation_status === "rejected" && test.blue_validation_notes && ( + <> Blue Lead's reason: "{test.blue_validation_notes}" + )} +

+

+ + The lead who approved should review the rejection reason and either change their vote or discuss with the other lead to resolve the disagreement. +

+
+
+ )} + {/* Progress bar */} {test.state !== "rejected" && (
diff --git a/frontend/src/pages/CampaignDetailPage.tsx b/frontend/src/pages/CampaignDetailPage.tsx index f63fa31..d84a2eb 100644 --- a/frontend/src/pages/CampaignDetailPage.tsx +++ b/frontend/src/pages/CampaignDetailPage.tsx @@ -55,6 +55,7 @@ const testStateColors: Record = { in_review: "bg-blue-900/50 text-blue-400 border-blue-500/30", validated: "bg-green-900/50 text-green-400 border-green-500/30", rejected: "bg-red-900/50 text-red-400 border-red-500/30", + disputed: "bg-amber-900/50 text-amber-400 border-amber-500/30", }; export default function CampaignDetailPage() { diff --git a/frontend/src/pages/DashboardPage.tsx b/frontend/src/pages/DashboardPage.tsx index 0217992..fb20f32 100644 --- a/frontend/src/pages/DashboardPage.tsx +++ b/frontend/src/pages/DashboardPage.tsx @@ -54,6 +54,7 @@ const testStateBadgeColors: Record = { in_review: "bg-blue-900/50 text-blue-400 border-blue-500/30", validated: "bg-green-900/50 text-green-400 border-green-500/30", rejected: "bg-red-900/50 text-red-400 border-red-500/30", + disputed: "bg-amber-900/50 text-amber-400 border-amber-500/30", }; const testStateLabels: Record = { diff --git a/frontend/src/pages/TechniqueDetailPage.tsx b/frontend/src/pages/TechniqueDetailPage.tsx index 5e72721..b36c9e3 100644 --- a/frontend/src/pages/TechniqueDetailPage.tsx +++ b/frontend/src/pages/TechniqueDetailPage.tsx @@ -45,6 +45,7 @@ const testStateBadgeColors: Record = { in_review: "bg-blue-900/50 text-blue-400 border-blue-500/30", validated: "bg-green-900/50 text-green-400 border-green-500/30", rejected: "bg-red-900/50 text-red-400 border-red-500/30", + disputed: "bg-amber-900/50 text-amber-400 border-amber-500/30", }; const testResultBadgeColors: Record = { diff --git a/frontend/src/pages/TestsPage.tsx b/frontend/src/pages/TestsPage.tsx index 210b64b..c891d2b 100644 --- a/frontend/src/pages/TestsPage.tsx +++ b/frontend/src/pages/TestsPage.tsx @@ -32,6 +32,7 @@ const testStateBadgeColors: Record = { in_review: "bg-blue-900/50 text-blue-400 border-blue-500/30", validated: "bg-green-900/50 text-green-400 border-green-500/30", rejected: "bg-red-900/50 text-red-400 border-red-500/30", + disputed: "bg-amber-900/50 text-amber-400 border-amber-500/30", }; const testStateLabels: Record = { @@ -41,6 +42,7 @@ const testStateLabels: Record = { in_review: "In Review", validated: "Validated", rejected: "Rejected", + disputed: "Disputed", }; const ALL_STATES: TestState[] = [ @@ -50,6 +52,7 @@ const ALL_STATES: TestState[] = [ "in_review", "validated", "rejected", + "disputed", ]; /* ── Helper: which team "owns" the current state ────────────────────── */ diff --git a/frontend/src/types/models.ts b/frontend/src/types/models.ts index f8e68f1..302c3f0 100644 --- a/frontend/src/types/models.ts +++ b/frontend/src/types/models.ts @@ -43,7 +43,8 @@ export type TestState = | "blue_evaluating" | "in_review" | "validated" - | "rejected"; + | "rejected" + | "disputed"; // one lead approved, the other rejected — needs discussion export type TestResult = "detected" | "not_detected" | "partially_detected";