feat: submit pull reviews from mobile My Work (#155)
This commit is contained in:
timmy committed
1 parent
7f5e671f8d
commit
9b81dce8aa
7 files changed
+443
-11
No files matched your search
+86
-1
@@ -974,7 +974,12 @@ async def test_review_sheet_loads_details_and_preserves_safe_gitea_handoff():
|
||||
assert "review-files" in html
|
||||
assert "review-history" in html
|
||||
assert "open-review-gitea" in html
|
||||
assert "reviewController.submit" not in html
|
||||
assert 'id="submit-review"' in html
|
||||
assert 'id="review-submit-status"' in html and 'aria-live="assertive"' in html
|
||||
assert "reviewController.submit(selectedReview" in html
|
||||
assert "window.confirm" in html
|
||||
assert "expected_head_sha: selectedReviewHead" in html
|
||||
assert "await load()" in html
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
@@ -1003,6 +1008,86 @@ async def test_mobile_review_sheet_captures_and_safely_hands_off_feedback():
|
||||
assert "handoffWindow.opener = null" in html
|
||||
|
||||
|
||||
def test_review_controller_submits_once_while_request_is_in_flight():
|
||||
script = f"""
|
||||
const createReviewController = require({json.dumps(str(REVIEW_SHEET))});
|
||||
let resolveRequest;
|
||||
const calls = [];
|
||||
const controller = createReviewController({{ fetchJson: (url, options) => {{
|
||||
calls.push({{url, options}});
|
||||
return new Promise(resolve => {{ resolveRequest = resolve; }});
|
||||
}} }});
|
||||
const item = {{repository:'stackchain/api', number:7}};
|
||||
const payload = {{decision:'approve', body:'Looks good.', expected_head_sha:'abc123'}};
|
||||
const first = controller.submit(item, payload);
|
||||
const second = controller.submit(item, payload);
|
||||
resolveRequest({{id:91, state:'APPROVED'}});
|
||||
Promise.all([first, second]).then(results => process.stdout.write(JSON.stringify({{
|
||||
calls, results
|
||||
}})));
|
||||
"""
|
||||
result = subprocess.run(
|
||||
["node", "-e", script], check=True, capture_output=True, text=True
|
||||
)
|
||||
|
||||
payload = json.loads(result.stdout)
|
||||
assert len(payload["calls"]) == 1
|
||||
assert payload["calls"][0] == {
|
||||
"url": "api/v1/repos/stackchain/api/pulls/7/review",
|
||||
"options": {
|
||||
"method": "POST",
|
||||
"headers": {
|
||||
"Accept": "application/json",
|
||||
"Content-Type": "application/json",
|
||||
},
|
||||
"body": json.dumps(
|
||||
{
|
||||
"decision": "approve",
|
||||
"body": "Looks good.",
|
||||
"expected_head_sha": "abc123",
|
||||
},
|
||||
separators=(",", ":"),
|
||||
),
|
||||
},
|
||||
}
|
||||
assert payload["results"] == [
|
||||
{"id": 91, "state": "APPROVED"},
|
||||
{"id": 91, "state": "APPROVED"},
|
||||
]
|
||||
|
||||
|
||||
def test_successful_review_can_clear_head_scoped_draft_and_progress():
|
||||
script = f"""
|
||||
const reviewSheet = require({json.dumps(str(REVIEW_SHEET))});
|
||||
const values = new Map();
|
||||
const removed = [];
|
||||
const storage = {{
|
||||
getItem: key => values.get(key) || null,
|
||||
setItem: (key, value) => values.set(key, value),
|
||||
removeItem: key => {{ removed.push(key); values.delete(key); }},
|
||||
}};
|
||||
const options = {{storage, repository:'stackchain/api', number:7, headSha:'abc123', files:[{{filename:'a.py'}}]}};
|
||||
const draft = reviewSheet.createDraft(options);
|
||||
const progress = reviewSheet.createProgress(options);
|
||||
draft.setSummary('Looks good.');
|
||||
progress.markReviewed('a.py');
|
||||
draft.clear();
|
||||
progress.clear();
|
||||
process.stdout.write(JSON.stringify({{removed, draft:draft.snapshot(), progress:progress.snapshot()}}));
|
||||
"""
|
||||
result = subprocess.run(
|
||||
["node", "-e", script], check=True, capture_output=True, text=True
|
||||
)
|
||||
|
||||
payload = json.loads(result.stdout)
|
||||
assert payload["removed"] == [
|
||||
"stackchain.review-draft.v1:stackchain/api#7@abc123",
|
||||
"stackchain.review-progress.v1:stackchain/api#7@abc123",
|
||||
]
|
||||
assert payload["draft"] == {"notes": {}, "summary": "", "decision": "comment"}
|
||||
assert payload["progress"]["reviewedCount"] == 0
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_mobile_review_failure_offers_an_in_place_retry_for_the_same_item():
|
||||
html = await dashboard()
|
||||
|
||||
+193
-4
@@ -3,7 +3,7 @@ import asyncio
|
||||
import httpx
|
||||
import pytest
|
||||
|
||||
from src import main
|
||||
from src import gitea_proxy, main
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
@@ -76,13 +76,202 @@ async def test_review_detail_has_one_retryable_deadline_and_cancels_pending_work
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_public_review_endpoint_does_not_expose_service_token_mutations():
|
||||
async def test_review_submission_posts_validated_decision_for_requested_current_head(monkeypatch):
|
||||
calls = []
|
||||
|
||||
async def requested(repository, number):
|
||||
return (repository, number) == ("stackchain/api", 7)
|
||||
|
||||
async def submit(repository, number, expected_head_sha, decision, body):
|
||||
calls.append((repository, number, expected_head_sha, decision, body))
|
||||
return {"id": 91, "state": "APPROVED", "url": "https://forge.example/reviews/91"}
|
||||
|
||||
monkeypatch.setattr(main, "is_requested_review", requested)
|
||||
monkeypatch.setattr(main.gitea_proxy, "submit_pull_review", submit, raising=False)
|
||||
transport = httpx.ASGITransport(app=main.app)
|
||||
async with httpx.AsyncClient(transport=transport, base_url="http://test") as client:
|
||||
response = await client.post(
|
||||
"/api/v1/repos/stackchain/api/pulls/7/review",
|
||||
json={"action": "approve"},
|
||||
json={
|
||||
"decision": "approve",
|
||||
"body": "Looks good on mobile.",
|
||||
"expected_head_sha": "abc123",
|
||||
},
|
||||
)
|
||||
|
||||
assert response.status_code == 405
|
||||
assert response.status_code == 201
|
||||
assert response.headers["cache-control"] == "no-store"
|
||||
assert response.json() == {
|
||||
"id": 91,
|
||||
"state": "APPROVED",
|
||||
"url": "https://forge.example/reviews/91",
|
||||
}
|
||||
assert calls == [
|
||||
("stackchain/api", 7, "abc123", "approve", "Looks good on mobile.")
|
||||
]
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_gitea_review_submission_checks_head_then_maps_decision_upstream():
|
||||
requests = []
|
||||
|
||||
async def handler(request):
|
||||
requests.append(request)
|
||||
if request.method == "GET":
|
||||
return httpx.Response(200, json={"head": {"sha": "abc123"}})
|
||||
return httpx.Response(
|
||||
200,
|
||||
json={
|
||||
"id": 91,
|
||||
"state": "APPROVED",
|
||||
"html_url": "https://forge.example/reviews/91",
|
||||
},
|
||||
)
|
||||
|
||||
gitea_proxy.start_client(transport=httpx.MockTransport(handler))
|
||||
try:
|
||||
result = await gitea_proxy.submit_pull_review(
|
||||
"stackchain/api", 7, "abc123", "approve", "Looks good."
|
||||
)
|
||||
finally:
|
||||
await gitea_proxy.stop_client()
|
||||
|
||||
assert [(request.method, request.url.path) for request in requests] == [
|
||||
("GET", "/api/v1/repos/stackchain/api/pulls/7"),
|
||||
("POST", "/api/v1/repos/stackchain/api/pulls/7/reviews"),
|
||||
]
|
||||
assert requests[1].content == b'{"body":"Looks good.","event":"APPROVE","commit_id":"abc123"}'
|
||||
assert result == {
|
||||
"id": 91,
|
||||
"state": "APPROVED",
|
||||
"url": "https://forge.example/reviews/91",
|
||||
}
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_gitea_review_submission_rejects_changed_head_without_posting():
|
||||
methods = []
|
||||
|
||||
async def handler(request):
|
||||
methods.append(request.method)
|
||||
return httpx.Response(200, json={"head": {"sha": "new456"}})
|
||||
|
||||
gitea_proxy.start_client(transport=httpx.MockTransport(handler))
|
||||
try:
|
||||
with pytest.raises(gitea_proxy.StaleReviewError):
|
||||
await gitea_proxy.submit_pull_review(
|
||||
"stackchain/api", 7, "abc123", "request_changes", "Please revise."
|
||||
)
|
||||
finally:
|
||||
await gitea_proxy.stop_client()
|
||||
|
||||
assert methods == ["GET"]
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_review_submission_returns_conflict_when_pull_head_changed(monkeypatch):
|
||||
async def requested(repository, number):
|
||||
return True
|
||||
|
||||
async def submit(*args):
|
||||
raise gitea_proxy.StaleReviewError("changed")
|
||||
|
||||
monkeypatch.setattr(main, "is_requested_review", requested)
|
||||
monkeypatch.setattr(main.gitea_proxy, "submit_pull_review", submit)
|
||||
transport = httpx.ASGITransport(app=main.app)
|
||||
async with httpx.AsyncClient(transport=transport, base_url="http://test") as client:
|
||||
response = await client.post(
|
||||
"/api/v1/repos/stackchain/api/pulls/7/review",
|
||||
json={
|
||||
"decision": "request_changes",
|
||||
"body": "Please revise.",
|
||||
"expected_head_sha": "abc123",
|
||||
},
|
||||
)
|
||||
|
||||
assert response.status_code == 409
|
||||
assert response.json() == {
|
||||
"error": "New commits were pushed. Refresh the review before submitting."
|
||||
}
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_review_submission_rejects_unsupported_decision_before_upstream(monkeypatch):
|
||||
called = False
|
||||
|
||||
async def requested(repository, number):
|
||||
nonlocal called
|
||||
called = True
|
||||
return True
|
||||
|
||||
monkeypatch.setattr(main, "is_requested_review", requested)
|
||||
transport = httpx.ASGITransport(app=main.app)
|
||||
async with httpx.AsyncClient(transport=transport, base_url="http://test") as client:
|
||||
response = await client.post(
|
||||
"/api/v1/repos/stackchain/api/pulls/7/review",
|
||||
json={
|
||||
"decision": "merge",
|
||||
"body": "Ship it.",
|
||||
"expected_head_sha": "abc123",
|
||||
},
|
||||
)
|
||||
|
||||
assert response.status_code == 422
|
||||
assert called is False
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_review_submission_rejects_oversized_feedback_before_upstream(monkeypatch):
|
||||
called = False
|
||||
|
||||
async def requested(repository, number):
|
||||
nonlocal called
|
||||
called = True
|
||||
return True
|
||||
|
||||
monkeypatch.setattr(main, "is_requested_review", requested)
|
||||
transport = httpx.ASGITransport(app=main.app)
|
||||
async with httpx.AsyncClient(transport=transport, base_url="http://test") as client:
|
||||
response = await client.post(
|
||||
"/api/v1/repos/stackchain/api/pulls/7/review",
|
||||
json={
|
||||
"decision": "comment",
|
||||
"body": "x" * 10_001,
|
||||
"expected_head_sha": "abc123",
|
||||
},
|
||||
)
|
||||
|
||||
assert response.status_code == 422
|
||||
assert called is False
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_review_submission_deadline_cancels_request_check(monkeypatch):
|
||||
cancelled = asyncio.Event()
|
||||
|
||||
async def requested(repository, number):
|
||||
try:
|
||||
await asyncio.Event().wait()
|
||||
finally:
|
||||
cancelled.set()
|
||||
|
||||
monkeypatch.setattr(main, "is_requested_review", requested)
|
||||
monkeypatch.setattr(main, "REVIEW_DETAIL_TIMEOUT_SECONDS", 0.01)
|
||||
transport = httpx.ASGITransport(app=main.app)
|
||||
async with httpx.AsyncClient(transport=transport, base_url="http://test") as client:
|
||||
response = await asyncio.wait_for(
|
||||
client.post(
|
||||
"/api/v1/repos/stackchain/api/pulls/7/review",
|
||||
json={
|
||||
"decision": "comment",
|
||||
"body": "Review note.",
|
||||
"expected_head_sha": "abc123",
|
||||
},
|
||||
),
|
||||
timeout=0.2,
|
||||
)
|
||||
|
||||
assert response.status_code == 503
|
||||
assert response.headers["retry-after"] == "1"
|
||||
assert cancelled.is_set()
|
||||
|
||||
Reference in new issue
Block a user