Reject transaction filters on account accepted-work API - #1167
Conversation
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
qingfeng312
left a comment
There was a problem hiding this comment.
I found one blocking issue before this is merge-ready.
The intended behavior change is narrow and looks correct: the accepted-work API now rejects type and tx_type, and the added assertions cover both query parameters.
Blocking issue:
The patch rewrites both changed files with CRLF line endings. At head 54b5a3f609226bea3b01a21050d6345ef763a046, app/accounts.py has 231 CRLF line endings and tests/test_account_routes.py has 411 CRLF line endings. A standard whitespace check reports every rewritten line as trailing whitespace, for 642 whitespace errors. It also makes the review surface look like a full-file rewrite; ignoring end-of-line whitespace shows the real semantic change is only the Request parameter, the accepted-work reject_unsupported_query_params(...) call, and the two regression assertions.
Please normalize the two files back to LF-only and keep the semantic diff small.
Validation:
- Inspected PR #1167 at head
54b5a3f609226bea3b01a21050d6345ef763a046. - Confirmed the semantic diff is narrowly scoped when end-of-line whitespace is ignored.
- Confirmed GitHub CI is green, but the line-ending rewrite still leaves a repository-level whitespace defect.
Refs #1009
|
@qingfeng312 — proactive CRLF cleanup on this branch. Normalized LF line endings (no functional changes) in:
Should pass |
54b5a3f to
f078b19
Compare
|
@qingfeng312 — normalized |
f078b19 to
3ba64c2
Compare
|
@qingfeng312 — CI fully green on latest head. PR #1167 (
Please recheck when convenient. Merge-ready. Wallet: |
taherdhanera
left a comment
There was a problem hiding this comment.
Reviewed current head 3ba64c2c52489c95a235d004c2c6b9990e004fd5.
Approved. The previous blocker was the full-file CRLF rewrite; the current committed/index state is LF (git ls-files --eol reports i/lf for both changed files), git diff --check origin/main...HEAD is clean, and the semantic diff is now narrow: accepted-work receives the Request, rejects type / tx_type through the existing reject_unsupported_query_params(...) helper, and the regression test asserts both 400 responses while preserving the unfiltered 200 path.
Verification:
- Hosted CI is green on
3ba64c2c52489c95a235d004c2c6b9990e004fd5: https://github.com/ramimbo/mergework/actions/runs/28634201770 python -m py_compile app/accounts.py tests/test_account_routes.pypassed locally.git diff --check origin/main...HEADpassed locally.python -m pytest tests/test_account_routes.py -qcould not run in this local environment because FastAPI is not installed; I relied on the green hosted quality/readiness workflow for runtime test coverage.
|
@qingfeng312 — Account accepted-work filter rejection covered on Wallet: |
|
@qingfeng312 Follow-up: normalized both changed files back to LF-only. Semantic diff unchanged — only the accepted-work query guard and regression tests. Ready for re-review. |
3ba64c2 to
fe2f3da
Compare
piaigmt
left a comment
There was a problem hiding this comment.
APPROVE — reviewed current head fe2f3da6962f765440ca7e4adedc9508bdc6bf5f for PR #1167.
The earlier CRLF line-ending blocker from a prior review is resolved on this head — both changed files are now LF-only (file reports ASCII/UTF-8 text, no CRLF). The patch is narrowly scoped: 2 files, +20/-1 lines.
The change adds reject_unsupported_query_params for type and tx_type on the /api/v1/accounts/{account}/accepted-work endpoint, matching the existing guard on the sibling /api/v1/accounts/{account} route (lines 194–198). The pattern is consistent: Request param added, same query params rejected, same context string convention with the route-specific suffix.
Evidence checked:
- Diff is limited to
app/accounts.pyandtests/test_account_routes.py. - The accepted-work route previously ignored all query params silently; now
typeandtx_typereturn HTTP 400 with a descriptive error, matching the sibling account detail route behavior. - Tests assert both
typeandtx_typereturn 400 with the correct error message on the accepted-work endpoint. - The existing 200-path assertions for the accepted-work response (status, summary, submission URL) remain intact.
Local validation (Python 3.12 venv):
pytest tests/test_account_routes.py -q→ 9 passedpytest tests/ -q→ 905 passedruff check app/accounts.py tests/test_account_routes.py→ all checks passedruff format --check app/accounts.py tests/test_account_routes.py→ 2 files already formattedmypy app/accounts.py→ success, no issues- Hosted CI:
Quality, readiness, docs, and image checks→ pass
No further blocker found on this head.
Summary
Implements proposed work for #1114.
Test plan
Related to #1114