feat: show pull reviewer decisions (Closes #1334)
This commit is contained in:
1 parent
f8ab6977ed
commit
bd96c3caa3
7 files changed
+275
-10
No files matched your search
+52
-1
@@ -7629,6 +7629,8 @@ const controller = createPullSheet({{
|
||||
return new Promise(resolve => {{ finish = resolve; }});
|
||||
}}
|
||||
}});
|
||||
const notified = [];
|
||||
controller.onReviewRequested = result => notified.push(result);
|
||||
const item = {{repository:'stackchain/api',number:7}};
|
||||
(async () => {{
|
||||
const candidates = await controller.loadReviewCandidates(item);
|
||||
@@ -7636,7 +7638,7 @@ const item = {{repository:'stackchain/api',number:7}};
|
||||
const duplicate = controller.requestReview(item, 'casey', 'abc1234');
|
||||
finish({{reviewer:'casey',requested_reviewers:['sam','casey']}});
|
||||
const results = await Promise.all([first, duplicate]);
|
||||
process.stdout.write(JSON.stringify({{calls,candidates,results,same:first===duplicate}}));
|
||||
process.stdout.write(JSON.stringify({{calls,candidates,results,same:first===duplicate,notified}}));
|
||||
}})();
|
||||
"""
|
||||
output = json.loads(subprocess.run(
|
||||
@@ -7656,6 +7658,7 @@ const item = {{repository:'stackchain/api',number:7}};
|
||||
{"reviewer": "casey", "requested_reviewers": ["sam", "casey"]},
|
||||
],
|
||||
"same": True,
|
||||
"notified": [{"reviewer": "casey", "requested_reviewers": ["sam", "casey"]}],
|
||||
}
|
||||
|
||||
|
||||
@@ -7675,6 +7678,22 @@ def test_pull_sheet_exposes_touch_safe_review_request_flow():
|
||||
assert "pullController.setReviewDetail(detail)" in dashboard
|
||||
|
||||
|
||||
def test_pull_sheet_exposes_mobile_reviewer_status_and_updated_review_action():
|
||||
root = PULL_SHEET.parents[1]
|
||||
html = (root / "frontend" / "index.html").read_text()
|
||||
css = (root / "frontend" / "dashboard.css").read_text()
|
||||
dashboard = (root / "frontend" / "dashboard.js").read_text()
|
||||
|
||||
assert 'id="pull-reviewer-statuses"' in html
|
||||
assert 'id="request-updated-pull-review"' in html
|
||||
assert 'id="pull-reviewer-status" class="small" aria-live="polite"' in html
|
||||
assert ".pull-reviewer-status" in css
|
||||
assert "overflow-wrap:anywhere" in css
|
||||
assert "#request-updated-pull-review { min-height:44px;" in css
|
||||
assert "createPullSheet.review(detail, pullReviewState, document)" in dashboard
|
||||
assert "reviewers.some(reviewer => reviewer.status === 'outdated')" in PULL_SHEET.read_text()
|
||||
|
||||
|
||||
def test_pull_sheet_surfaces_pending_merge_confirmation_guidance():
|
||||
script = f"""
|
||||
const createPullSheet = require({json.dumps(str(PULL_SHEET))});
|
||||
@@ -7721,6 +7740,38 @@ process.stdout.write(JSON.stringify(states.map(createPullSheet.mergeEligibility)
|
||||
assert output[3]["allowed"] is False and "conflict" in output[3]["reason"].lower()
|
||||
|
||||
|
||||
def test_pull_sheet_renders_reviewer_decisions_and_blocks_unresolved_requests():
|
||||
script = f"""
|
||||
const createPullSheet = require({json.dumps(str(PULL_SHEET))});
|
||||
const base = {{state:'open', draft:false, mergeable:true, merged:false, ci_state:'success', head_sha:'new'}};
|
||||
const states = [
|
||||
[{{login:'casey', status:'waiting', head_sha:'new', blocking:true}}],
|
||||
[{{login:'sam', status:'changes_requested', head_sha:'new', blocking:true}}],
|
||||
[{{login:'lee', status:'outdated', head_sha:'old', blocking:true}}],
|
||||
[{{login:'pat', status:'approved', head_sha:'new', blocking:false}}],
|
||||
];
|
||||
process.stdout.write(JSON.stringify({{
|
||||
eligibility:states.map(reviewers => createPullSheet.mergeEligibility({{...base, reviewers}})),
|
||||
html:createPullSheet.renderReviewerStatuses(states.flat(), value => String(value)
|
||||
.replaceAll('&', '&').replaceAll('<', '<').replaceAll('>', '>')),
|
||||
}}));
|
||||
"""
|
||||
output = json.loads(subprocess.run(
|
||||
["node", "-e", script], check=True, capture_output=True, text=True
|
||||
).stdout)
|
||||
|
||||
assert output["eligibility"] == [
|
||||
{"allowed": False, "reason": "Waiting for @casey’s review"},
|
||||
{"allowed": False, "reason": "@sam requested changes"},
|
||||
{"allowed": False, "reason": "@lee’s review is outdated after new commits"},
|
||||
{"allowed": True, "reason": "Ready to merge"},
|
||||
]
|
||||
assert "@casey" in output["html"] and "Waiting for review" in output["html"]
|
||||
assert "Changes requested" in output["html"]
|
||||
assert "Outdated after new commits" in output["html"]
|
||||
assert "Approved current head" in output["html"]
|
||||
|
||||
|
||||
def test_pull_sheet_persists_head_scoped_file_review_and_gates_merge():
|
||||
script = f"""
|
||||
const createPullSheet = require({json.dumps(str(PULL_SHEET))});
|
||||
|
||||
+77
-1
@@ -211,8 +211,33 @@ async def test_assigned_pull_review_endpoint_loads_review_payload_on_demand(monk
|
||||
]
|
||||
|
||||
|
||||
def test_reviewer_statuses_distinguish_waiting_current_and_outdated_decisions():
|
||||
pull = {
|
||||
"requested_reviewers": [{"login": "casey"}],
|
||||
"head": {"sha": "new-head"},
|
||||
}
|
||||
reviews = [
|
||||
{"id": 1, "user": {"login": "casey"}, "state": "APPROVED", "commit_id": "old-head"},
|
||||
{"id": 2, "user": {"login": "sam"}, "state": "REQUEST_CHANGES", "commit_id": "new-head"},
|
||||
{"id": 3, "user": {"login": "lee"}, "state": "COMMENT", "commit_id": "new-head"},
|
||||
{"id": 4, "user": {"login": "pat"}, "state": "APPROVED", "commit_id": "new-head"},
|
||||
]
|
||||
|
||||
assert gitea_proxy._normalize_reviewer_statuses(pull, reviews, "new-head") == [
|
||||
{"login": "casey", "status": "waiting", "head_sha": "new-head", "blocking": True},
|
||||
{"login": "lee", "status": "commented", "head_sha": "new-head", "blocking": False},
|
||||
{"login": "pat", "status": "approved", "head_sha": "new-head", "blocking": False},
|
||||
{"login": "sam", "status": "changes_requested", "head_sha": "new-head", "blocking": True},
|
||||
]
|
||||
|
||||
pull["requested_reviewers"] = []
|
||||
assert gitea_proxy._normalize_reviewer_statuses(pull, reviews[:1], "new-head") == [
|
||||
{"login": "casey", "status": "outdated", "head_sha": "old-head", "blocking": True},
|
||||
]
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_gitea_pull_check_status_skips_files_reviews_and_diff():
|
||||
async def test_gitea_pull_check_status_refreshes_reviewer_decisions_without_files_or_diff():
|
||||
requests = []
|
||||
|
||||
async def handler(request):
|
||||
@@ -221,7 +246,10 @@ async def test_gitea_pull_check_status_skips_files_reviews_and_diff():
|
||||
return httpx.Response(200, json={
|
||||
"state": "open", "draft": False, "mergeable": True, "merged": False,
|
||||
"head": {"sha": "abc123"},
|
||||
"requested_reviewers": [{"login": "casey"}],
|
||||
})
|
||||
if request.url.path.endswith("/pulls/7/reviews"):
|
||||
return httpx.Response(200, json=[])
|
||||
if request.url.path.endswith("/commits/abc123/status"):
|
||||
return httpx.Response(200, json={
|
||||
"state": "success",
|
||||
@@ -238,12 +266,16 @@ async def test_gitea_pull_check_status_skips_files_reviews_and_diff():
|
||||
assert requests == [
|
||||
("GET", "/api/v1/repos/stackchain/api/pulls/7"),
|
||||
("GET", "/api/v1/repos/stackchain/api/commits/abc123/status"),
|
||||
("GET", "/api/v1/repos/stackchain/api/pulls/7/reviews"),
|
||||
]
|
||||
assert status == {
|
||||
"repository": "stackchain/api", "number": 7, "head_sha": "abc123",
|
||||
"state": "open", "draft": False, "mergeable": True, "merged": False,
|
||||
"ci_state": "success",
|
||||
"checks": [{"name": "tests", "state": "success", "description": "Passed", "url": ""}],
|
||||
"reviewers": [
|
||||
{"login": "casey", "status": "waiting", "head_sha": "abc123", "blocking": True},
|
||||
],
|
||||
}
|
||||
|
||||
|
||||
@@ -285,7 +317,12 @@ async def test_gitea_assigned_pull_review_includes_bounded_diff_previews():
|
||||
"state": "open",
|
||||
"mergeable": True,
|
||||
"head": {"sha": "abc123"},
|
||||
"requested_reviewers": [{"login": "casey"}],
|
||||
})
|
||||
if path.endswith("/pulls/7/reviews"):
|
||||
return httpx.Response(200, json=[
|
||||
{"id": 8, "user": {"login": "sam"}, "state": "APPROVED", "commit_id": "abc123"},
|
||||
])
|
||||
if path.endswith("/pulls/7/files"):
|
||||
return httpx.Response(200, json=[
|
||||
{"filename": "src/api.py", "status": "modified", "additions": 1, "deletions": 1},
|
||||
@@ -315,6 +352,10 @@ async def test_gitea_assigned_pull_review_includes_bounded_diff_previews():
|
||||
assert "+new" in detail["files"][0]["diff_lines"]
|
||||
assert detail["files"][1]["diff_binary"] is True
|
||||
assert detail["files"][1]["diff_available"] is False
|
||||
assert detail["reviewers"] == [
|
||||
{"login": "casey", "status": "waiting", "head_sha": "abc123", "blocking": True},
|
||||
{"login": "sam", "status": "approved", "head_sha": "abc123", "blocking": False},
|
||||
]
|
||||
assert "conversation" not in detail
|
||||
|
||||
|
||||
@@ -678,6 +719,39 @@ async def test_gitea_merge_rejects_failed_ci_without_upstream_mutation():
|
||||
]
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
@pytest.mark.parametrize("requested,reviews", [
|
||||
([{"login": "casey"}], []),
|
||||
([], [{"id": 9, "user": {"login": "casey"}, "state": "REQUEST_CHANGES", "commit_id": "abc123"}]),
|
||||
])
|
||||
async def test_gitea_merge_rejects_unresolved_reviewer_status_without_mutation(requested, reviews):
|
||||
requests = []
|
||||
|
||||
async def handler(request):
|
||||
requests.append((request.method, request.url.path))
|
||||
if request.url.path.endswith("/pulls/7"):
|
||||
return httpx.Response(200, json={
|
||||
"number": 7, "state": "open", "draft": False, "mergeable": True,
|
||||
"merged": False, "head": {"sha": "abc123"},
|
||||
"requested_reviewers": requested,
|
||||
})
|
||||
if request.url.path.endswith("/commits/abc123/status"):
|
||||
return httpx.Response(200, json={"state": "success"})
|
||||
if request.url.path.endswith("/pulls/7/reviews"):
|
||||
return httpx.Response(200, json=reviews)
|
||||
raise AssertionError(f"unexpected mutation: {request.method} {request.url.path}")
|
||||
|
||||
gitea_proxy.start_client(transport=httpx.MockTransport(handler))
|
||||
try:
|
||||
with pytest.raises(gitea_proxy.PullNotMergeableError):
|
||||
await gitea_proxy.merge_assigned_pull("stackchain/api", 7, "abc123")
|
||||
finally:
|
||||
await gitea_proxy.stop_client()
|
||||
|
||||
assert requests[-1] == ("GET", "/api/v1/repos/stackchain/api/pulls/7/reviews")
|
||||
assert all(method == "GET" for method, _path in requests)
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_assigned_pull_ownership_endpoints_list_handoff_and_release(monkeypatch):
|
||||
calls = []
|
||||
@@ -920,6 +994,8 @@ async def test_gitea_merge_returns_the_exact_merge_commit_for_release_tracking()
|
||||
})
|
||||
if request.url.path.endswith("/commits/abc123/status"):
|
||||
return httpx.Response(200, json={"state": "success"})
|
||||
if request.url.path.endswith("/pulls/7/reviews"):
|
||||
return httpx.Response(200, json=[])
|
||||
if request.url.path.endswith("/pulls/7/merge"):
|
||||
return httpx.Response(200, json={"merged": True, "sha": "merge456"})
|
||||
raise AssertionError(request.url.path)
|
||||
|
||||
Reference in new issue
Block a user