diff --git a/backend/app/services/jira_service.py b/backend/app/services/jira_service.py index 4fe77ae..1599b7e 100644 --- a/backend/app/services/jira_service.py +++ b/backend/app/services/jira_service.py @@ -516,8 +516,17 @@ def _build_state_comment( lines += ["", "h4. Remediation Steps", test.remediation_steps] elif new_state == "validated": + # A disputed test resolved by a manager keeps the leads' original, + # disagreeing votes (one approved, one rejected) — that combination + # is otherwise unreachable, since normal dual-validation requires + # both leads to agree. Use it to tell the two paths apart. + manager_resolved = ( + test.red_validation_status and test.blue_validation_status + and test.red_validation_status != test.blue_validation_status + ) lines += [ - "Test has been *validated* by both leads.", + "Test has been *validated* by a manager's decision on a disputed test." + if manager_resolved else "Test has been *validated* by both leads.", "", f"*Red Lead Status:* {test.red_validation_status or 'N/A'}", f"*Blue Lead Status:* {test.blue_validation_status or 'N/A'}", @@ -528,8 +537,13 @@ def _build_state_comment( lines += ["", f"*Blue Lead Notes:* {test.blue_validation_notes}"] elif new_state == "rejected": + manager_resolved = ( + test.red_validation_status and test.blue_validation_status + and test.red_validation_status != test.blue_validation_status + ) lines += [ - "Test has been *rejected* and must be reworked.", + "Test has been *rejected* by a manager's decision on a disputed test." + if manager_resolved else "Test has been *rejected* and must be reworked.", "", f"*Red Lead Status:* {test.red_validation_status or 'N/A'}", f"*Blue Lead Status:* {test.blue_validation_status or 'N/A'}", diff --git a/backend/app/services/test_workflow_service.py b/backend/app/services/test_workflow_service.py index b5f3386..0615bac 100644 --- a/backend/app/services/test_workflow_service.py +++ b/backend/app/services/test_workflow_service.py @@ -1147,9 +1147,14 @@ def check_dual_validation(db: Session, test: Test) -> Test: # Define function _dispatch_dual_validation_effects def _dispatch_dual_validation_effects( - db: Session, test: Test, entity: TestEntity, actor: User | None = None + db: Session, test: Test, entity: TestEntity, actor: User | None = None, + extra: dict | None = None, ) -> None: - """Dispatch side effects (notifications, cache, Jira) based on domain events.""" + """Dispatch side effects (notifications, cache, Jira) based on domain events. + + ``extra`` is forwarded onto the Jira comment (e.g. a manager's decision + notes) — see ``push_test_event``'s ``extra`` kwarg. + """ for event in entity.events: # Check: event.name == "dual_validation_approved" if event.name == "dual_validation_approved": @@ -1178,7 +1183,7 @@ def _dispatch_dual_validation_effects( if actor: try: from app.services.jira_service import push_test_event - push_test_event(db, test, actor, "validated") + push_test_event(db, test, actor, "validated", extra=extra) except Exception as e: logger.warning("Jira push failed for test %s: %s", test.id, e, exc_info=True) @@ -1198,7 +1203,7 @@ def _dispatch_dual_validation_effects( if actor: try: from app.services.jira_service import push_test_event - push_test_event(db, test, actor, "rejected") + push_test_event(db, test, actor, "rejected", extra=extra) except Exception as e: logger.warning("Jira push failed for test %s: %s", test.id, e, exc_info=True) @@ -1364,7 +1369,8 @@ def resolve_dispute_by_manager(db: Session, test: Test, user: User, outcome: str details={"outcome": outcome, "notes": notes, "test_name": test.name}, ) - _dispatch_dual_validation_effects(db, test, entity, actor=user) + jira_extra = {"Manager Decision": f"{user.username}" + (f" — {notes}" if notes else "")} + _dispatch_dual_validation_effects(db, test, entity, actor=user, extra=jira_extra) # Let both leads know the manager made the final call, not just whichever # side "won" — the losing side needs to know just as much. diff --git a/backend/tests/test_jira_service.py b/backend/tests/test_jira_service.py index b3b3de8..620ea60 100644 --- a/backend/tests/test_jira_service.py +++ b/backend/tests/test_jira_service.py @@ -233,6 +233,68 @@ def test_push_test_event_maps_every_state_to_jira_status( mock_jira.set_issue_status.assert_called_once_with(link.jira_issue_key, expected_status) +@patch("app.services.jira_service.has_admin_jira_configured", return_value=True) +@patch("app.services.jira_service.get_admin_jira_client") +def test_push_test_event_validated_says_both_leads_when_they_agree(mock_get_client, mock_configured, db): + mock_jira = MagicMock() + mock_get_client.return_value = mock_jira + test = _make_test(red_validation_status="approved", blue_validation_status="approved") + link = _make_link(test.id) + db.add(link) + db.commit() + + jira_service.push_test_event(db, test, MagicMock(username="alice", jira_account_id=None), "validated") + + comment = mock_jira.issue_add_comment.call_args[0][1] + assert "validated* by both leads" in comment + assert "manager" not in comment.lower() + + +@patch("app.services.jira_service.has_admin_jira_configured", return_value=True) +@patch("app.services.jira_service.get_admin_jira_client") +def test_push_test_event_validated_flags_manager_override_when_leads_disagree(mock_get_client, mock_configured, db): + """Regression: a disputed test resolved by a manager keeps the leads' + original disagreeing votes — the comment must not claim 'both leads' + agreed, and must carry the manager's own decision/notes.""" + mock_jira = MagicMock() + mock_get_client.return_value = mock_jira + test = _make_test(red_validation_status="approved", blue_validation_status="rejected") + link = _make_link(test.id) + db.add(link) + db.commit() + + jira_service.push_test_event( + db, test, MagicMock(username="sergio", jira_account_id=None), "validated", + extra={"Manager Decision": "sergio — test rejected"}, + ) + + comment = mock_jira.issue_add_comment.call_args[0][1] + assert "manager's decision" in comment.lower() + assert "both leads" not in comment + assert "Manager Decision:* sergio — test rejected" in comment + + +@patch("app.services.jira_service.has_admin_jira_configured", return_value=True) +@patch("app.services.jira_service.get_admin_jira_client") +def test_push_test_event_rejected_flags_manager_override_when_leads_disagree(mock_get_client, mock_configured, db): + mock_jira = MagicMock() + mock_get_client.return_value = mock_jira + test = _make_test(red_validation_status="rejected", blue_validation_status="approved") + link = _make_link(test.id) + db.add(link) + db.commit() + + jira_service.push_test_event( + db, test, MagicMock(username="sergio", jira_account_id=None), "rejected", + extra={"Manager Decision": "sergio — needs full rework"}, + ) + + comment = mock_jira.issue_add_comment.call_args[0][1] + assert "manager's decision" in comment.lower() + assert "must be reworked" not in comment + assert "Manager Decision:* sergio — needs full rework" in comment + + @patch("app.services.jira_service.has_admin_jira_configured", return_value=True) @patch("app.services.jira_service.get_admin_jira_client") def test_push_test_event_red_review_comment_includes_round_data(mock_get_client, mock_configured, db): diff --git a/frontend/src/components/test-detail/TeamTabs.tsx b/frontend/src/components/test-detail/TeamTabs.tsx index 9e5091c..1ac7c10 100644 --- a/frontend/src/components/test-detail/TeamTabs.tsx +++ b/frontend/src/components/test-detail/TeamTabs.tsx @@ -698,8 +698,51 @@ export default function TeamTabs({ // ── Summary Tab ────────────────────────────────────────────────── - const renderSummaryTab = () => ( + const renderSummaryTab = () => { + // A disputed test resolved by a manager keeps the leads' original, + // disagreeing votes (one approved, one rejected) — that combination is + // otherwise unreachable, since normal dual-validation requires both + // leads to agree. Use it to tell the two paths apart, same signal used + // for the Jira comment. + const managerResolved = + !!test.red_validation_status && + !!test.blue_validation_status && + test.red_validation_status !== test.blue_validation_status && + (test.state === "validated" || test.state === "rejected"); + + return (
+ {/* Final Result — shown first: the single most important thing to + know at a glance, especially right after a manager's decision. */} +
+

Final Result

+
+
+ State: + + {test.state.replace(/_/g, " ")} + +
+
+ Detection: + + {test.detection_result ? test.detection_result.replace(/_/g, " ") : "Not evaluated"} + +
+
+ Containment: + + {test.containment_result ? test.containment_result.replace(/_/g, " ") : "Not evaluated"} + +
+
+ {managerResolved && ( +

+ Resolved by a manager's decision — the leads disagreed (Red: {test.red_validation_status}, Blue: {test.blue_validation_status}). +

+ )} +
+ {/* Side-by-side comparison */}
{/* Red Side */} @@ -752,6 +795,9 @@ export default function TeamTabs({ <>Rejected )} + {test.red_validation_notes && ( +
+ )}
)} @@ -783,6 +829,28 @@ export default function TeamTabs({ )}
+ {(test.detection_result === "detected" || test.detection_result === "partially_detected") && ( +
+
Containment Result
+
+ {test.containment_result ? ( + + {test.containment_result.replace(/_/g, " ")} + + ) : ( + Not evaluated + )} +
+
+ )}
Summary
@@ -803,6 +871,9 @@ export default function TeamTabs({ <>Rejected )} + {test.blue_validation_notes && ( +
+ )}
)} {test.system_gaps && ( @@ -814,29 +885,9 @@ export default function TeamTabs({ - - {/* Final Result */} -
-

Final Result

-
-
- State: - - {test.state.replace(/_/g, " ")} - -
- {test.detection_result && ( -
- Detection: - - {test.detection_result.replace(/_/g, " ")} - -
- )} -
-
- ); + ); + }; // ── Timeline Tab ─────────────────────────────────────────────────