fix(jira,tests): clarify manager-resolved disputes in Jira and the Summary tab
Aegis CI / lint-and-test (push) Has been cancelled
Snyk Security Scan / Python vulnerabilities (backend) (push) Has been cancelled
Snyk Security Scan / npm vulnerabilities (frontend) (push) Has been cancelled
Snyk Security Scan / Docker image vulnerabilities (backend) (push) Has been cancelled
Aegis CI / lint-and-test (push) Has been cancelled
Snyk Security Scan / Python vulnerabilities (backend) (push) Has been cancelled
Snyk Security Scan / npm vulnerabilities (frontend) (push) Has been cancelled
Snyk Security Scan / Docker image vulnerabilities (backend) (push) Has been cancelled
The validated/rejected Jira comment always said 'by both leads' and never carried the manager's own notes, even when a manager had just overridden two disagreeing votes — misleading, since that combination of statuses is only reachable via a manager decision. Now detected from the disagreement itself and worded accordingly, with the manager's notes included via the existing extra-data mechanism. Also reworks the Summary tab: Final Result moves to the top (was the very last thing on the page) and now shows Containment alongside Detection — previously the only place Containment appeared at all was inside the editable Blue tab. Both team cards now show the validating lead's notes, not just the approved/rejected badge, and flag when a manager's decision resolved a disagreement between the two leads.
This commit is contained in:
@@ -516,8 +516,17 @@ def _build_state_comment(
|
|||||||
lines += ["", "h4. Remediation Steps", test.remediation_steps]
|
lines += ["", "h4. Remediation Steps", test.remediation_steps]
|
||||||
|
|
||||||
elif new_state == "validated":
|
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 += [
|
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"*Red Lead Status:* {test.red_validation_status or 'N/A'}",
|
||||||
f"*Blue Lead Status:* {test.blue_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}"]
|
lines += ["", f"*Blue Lead Notes:* {test.blue_validation_notes}"]
|
||||||
|
|
||||||
elif new_state == "rejected":
|
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 += [
|
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"*Red Lead Status:* {test.red_validation_status or 'N/A'}",
|
||||||
f"*Blue Lead Status:* {test.blue_validation_status or 'N/A'}",
|
f"*Blue Lead Status:* {test.blue_validation_status or 'N/A'}",
|
||||||
|
|||||||
@@ -1147,9 +1147,14 @@ def check_dual_validation(db: Session, test: Test) -> Test:
|
|||||||
|
|
||||||
# Define function _dispatch_dual_validation_effects
|
# Define function _dispatch_dual_validation_effects
|
||||||
def _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:
|
) -> 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:
|
for event in entity.events:
|
||||||
# Check: event.name == "dual_validation_approved"
|
# Check: event.name == "dual_validation_approved"
|
||||||
if event.name == "dual_validation_approved":
|
if event.name == "dual_validation_approved":
|
||||||
@@ -1178,7 +1183,7 @@ def _dispatch_dual_validation_effects(
|
|||||||
if actor:
|
if actor:
|
||||||
try:
|
try:
|
||||||
from app.services.jira_service import push_test_event
|
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:
|
except Exception as e:
|
||||||
logger.warning("Jira push failed for test %s: %s", test.id, e, exc_info=True)
|
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:
|
if actor:
|
||||||
try:
|
try:
|
||||||
from app.services.jira_service import push_test_event
|
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:
|
except Exception as e:
|
||||||
logger.warning("Jira push failed for test %s: %s", test.id, e, exc_info=True)
|
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},
|
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
|
# Let both leads know the manager made the final call, not just whichever
|
||||||
# side "won" — the losing side needs to know just as much.
|
# side "won" — the losing side needs to know just as much.
|
||||||
|
|||||||
@@ -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)
|
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.has_admin_jira_configured", return_value=True)
|
||||||
@patch("app.services.jira_service.get_admin_jira_client")
|
@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):
|
def test_push_test_event_red_review_comment_includes_round_data(mock_get_client, mock_configured, db):
|
||||||
|
|||||||
@@ -698,8 +698,51 @@ export default function TeamTabs({
|
|||||||
|
|
||||||
// ── Summary Tab ──────────────────────────────────────────────────
|
// ── 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 (
|
||||||
<div className="space-y-6">
|
<div className="space-y-6">
|
||||||
|
{/* Final Result — shown first: the single most important thing to
|
||||||
|
know at a glance, especially right after a manager's decision. */}
|
||||||
|
<div className="rounded-lg border border-gray-700 bg-gray-800/50 p-4">
|
||||||
|
<h3 className="mb-2 text-sm font-semibold text-gray-300">Final Result</h3>
|
||||||
|
<div className="flex flex-wrap items-center gap-4">
|
||||||
|
<div className="flex items-center gap-2">
|
||||||
|
<span className="text-xs text-gray-500">State:</span>
|
||||||
|
<span className="text-sm font-medium capitalize text-gray-200">
|
||||||
|
{test.state.replace(/_/g, " ")}
|
||||||
|
</span>
|
||||||
|
</div>
|
||||||
|
<div className="flex items-center gap-2">
|
||||||
|
<span className="text-xs text-gray-500">Detection:</span>
|
||||||
|
<span className="text-sm font-medium capitalize text-gray-200">
|
||||||
|
{test.detection_result ? test.detection_result.replace(/_/g, " ") : "Not evaluated"}
|
||||||
|
</span>
|
||||||
|
</div>
|
||||||
|
<div className="flex items-center gap-2">
|
||||||
|
<span className="text-xs text-gray-500">Containment:</span>
|
||||||
|
<span className="text-sm font-medium capitalize text-gray-200">
|
||||||
|
{test.containment_result ? test.containment_result.replace(/_/g, " ") : "Not evaluated"}
|
||||||
|
</span>
|
||||||
|
</div>
|
||||||
|
</div>
|
||||||
|
{managerResolved && (
|
||||||
|
<p className="mt-2 text-xs text-purple-300">
|
||||||
|
Resolved by a manager's decision — the leads disagreed (Red: {test.red_validation_status}, Blue: {test.blue_validation_status}).
|
||||||
|
</p>
|
||||||
|
)}
|
||||||
|
</div>
|
||||||
|
|
||||||
{/* Side-by-side comparison */}
|
{/* Side-by-side comparison */}
|
||||||
<div className="grid gap-6 md:grid-cols-2">
|
<div className="grid gap-6 md:grid-cols-2">
|
||||||
{/* Red Side */}
|
{/* Red Side */}
|
||||||
@@ -752,6 +795,9 @@ export default function TeamTabs({
|
|||||||
<><XCircle className="h-3.5 w-3.5 text-red-400" /><span className="text-red-400">Rejected</span></>
|
<><XCircle className="h-3.5 w-3.5 text-red-400" /><span className="text-red-400">Rejected</span></>
|
||||||
)}
|
)}
|
||||||
</dd>
|
</dd>
|
||||||
|
{test.red_validation_notes && (
|
||||||
|
<dd className="mt-1"><MarkdownText content={test.red_validation_notes} className="text-xs text-gray-400 italic" /></dd>
|
||||||
|
)}
|
||||||
</div>
|
</div>
|
||||||
)}
|
)}
|
||||||
</dl>
|
</dl>
|
||||||
@@ -783,6 +829,28 @@ export default function TeamTabs({
|
|||||||
)}
|
)}
|
||||||
</dd>
|
</dd>
|
||||||
</div>
|
</div>
|
||||||
|
{(test.detection_result === "detected" || test.detection_result === "partially_detected") && (
|
||||||
|
<div>
|
||||||
|
<dt className="text-xs font-medium uppercase text-gray-500">Containment Result</dt>
|
||||||
|
<dd className="mt-0.5 text-sm">
|
||||||
|
{test.containment_result ? (
|
||||||
|
<span
|
||||||
|
className={`inline-flex rounded-full border px-2 py-0.5 text-xs font-medium ${
|
||||||
|
test.containment_result === "contained"
|
||||||
|
? "border-green-500/30 bg-green-900/50 text-green-400"
|
||||||
|
: test.containment_result === "not_contained"
|
||||||
|
? "border-red-500/30 bg-red-900/50 text-red-400"
|
||||||
|
: "border-yellow-500/30 bg-yellow-900/50 text-yellow-400"
|
||||||
|
}`}
|
||||||
|
>
|
||||||
|
{test.containment_result.replace(/_/g, " ")}
|
||||||
|
</span>
|
||||||
|
) : (
|
||||||
|
<span className="text-gray-500">Not evaluated</span>
|
||||||
|
)}
|
||||||
|
</dd>
|
||||||
|
</div>
|
||||||
|
)}
|
||||||
<div>
|
<div>
|
||||||
<dt className="text-xs font-medium uppercase text-gray-500">Summary</dt>
|
<dt className="text-xs font-medium uppercase text-gray-500">Summary</dt>
|
||||||
<dd className="mt-0.5"><MarkdownText content={test.blue_summary || "N/A"} className="text-sm text-gray-300" /></dd>
|
<dd className="mt-0.5"><MarkdownText content={test.blue_summary || "N/A"} className="text-sm text-gray-300" /></dd>
|
||||||
@@ -803,6 +871,9 @@ export default function TeamTabs({
|
|||||||
<><XCircle className="h-3.5 w-3.5 text-red-400" /><span className="text-red-400">Rejected</span></>
|
<><XCircle className="h-3.5 w-3.5 text-red-400" /><span className="text-red-400">Rejected</span></>
|
||||||
)}
|
)}
|
||||||
</dd>
|
</dd>
|
||||||
|
{test.blue_validation_notes && (
|
||||||
|
<dd className="mt-1"><MarkdownText content={test.blue_validation_notes} className="text-xs text-gray-400 italic" /></dd>
|
||||||
|
)}
|
||||||
</div>
|
</div>
|
||||||
)}
|
)}
|
||||||
{test.system_gaps && (
|
{test.system_gaps && (
|
||||||
@@ -814,29 +885,9 @@ export default function TeamTabs({
|
|||||||
</dl>
|
</dl>
|
||||||
</div>
|
</div>
|
||||||
</div>
|
</div>
|
||||||
|
|
||||||
{/* Final Result */}
|
|
||||||
<div className="rounded-lg border border-gray-700 bg-gray-800/50 p-4">
|
|
||||||
<h3 className="mb-2 text-sm font-semibold text-gray-300">Final Result</h3>
|
|
||||||
<div className="flex items-center gap-4">
|
|
||||||
<div className="flex items-center gap-2">
|
|
||||||
<span className="text-xs text-gray-500">State:</span>
|
|
||||||
<span className="text-sm font-medium capitalize text-gray-200">
|
|
||||||
{test.state.replace(/_/g, " ")}
|
|
||||||
</span>
|
|
||||||
</div>
|
|
||||||
{test.detection_result && (
|
|
||||||
<div className="flex items-center gap-2">
|
|
||||||
<span className="text-xs text-gray-500">Detection:</span>
|
|
||||||
<span className="text-sm font-medium capitalize text-gray-200">
|
|
||||||
{test.detection_result.replace(/_/g, " ")}
|
|
||||||
</span>
|
|
||||||
</div>
|
|
||||||
)}
|
|
||||||
</div>
|
|
||||||
</div>
|
|
||||||
</div>
|
</div>
|
||||||
);
|
);
|
||||||
|
};
|
||||||
|
|
||||||
// ── Timeline Tab ─────────────────────────────────────────────────
|
// ── Timeline Tab ─────────────────────────────────────────────────
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user