Skip to content

Reject transaction filters on account accepted-work API - #1167

Open
yanyishuai wants to merge 1 commit into
ramimbo:mainfrom
yanyishuai:bounty-1114-accepted-work-query-guard
Open

Reject transaction filters on account accepted-work API#1167
yanyishuai wants to merge 1 commit into
ramimbo:mainfrom
yanyishuai:bounty-1114-accepted-work-query-guard

Conversation

@yanyishuai

Copy link
Copy Markdown

Summary

Implements proposed work for #1114.

  • Reject ype and x_type query parameters on GET /api/v1/accounts/{account}/accepted-work with the same bounded 400 behavior used by the parent account JSON route.
  • Add regression coverage in ests/test_account_routes.py.

Test plan

  • Accepted-work API returns 400 for ype / x_type filters
  • Unfiltered accepted-work JSON still returns 200

Related to #1114

@coderabbitai

coderabbitai Bot commented Jun 28, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@yanyishuai, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 53 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 74653178-6fc8-4af7-a704-baa591fceb91

📥 Commits

Reviewing files that changed from the base of the PR and between 3bc87d2 and fe2f3da.

📒 Files selected for processing (2)
  • app/accounts.py
  • tests/test_account_routes.py

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@qingfeng312 qingfeng312 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@yanyishuai

Copy link
Copy Markdown
Author

@qingfeng312 — proactive CRLF cleanup on this branch.

Normalized LF line endings (no functional changes) in:

  • app/accounts.py
  • tests/test_account_routes.py

Should pass git diff --check / trailing-whitespace gates on Windows-authored patches.

@yanyishuai
yanyishuai force-pushed the bounty-1114-accepted-work-query-guard branch from 54b5a3f to f078b19 Compare July 3, 2026 02:06
@yanyishuai

Copy link
Copy Markdown
Author

@qingfeng312 — normalized app/accounts.py and tests/test_account_routes.py to LF-only; semantic diff unchanged.

@yanyishuai
yanyishuai force-pushed the bounty-1114-accepted-work-query-guard branch from f078b19 to 3ba64c2 Compare July 3, 2026 02:21
@yanyishuai

Copy link
Copy Markdown
Author

@qingfeng312CI fully green on latest head.

PR #1167 (3ba64c2c) — reject transaction filters on account accepted-work API

  • LF-normalized (app/accounts.py + tests); semantic diff unchanged
  • full CI green

Please recheck when convenient. Merge-ready.

Wallet: Do4v7foHJvRJLpRRoGaVPWX6DDEjX3yTK7J91gpwUQpE

@taherdhanera taherdhanera left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.py passed locally.
  • git diff --check origin/main...HEAD passed locally.
  • python -m pytest tests/test_account_routes.py -q could not run in this local environment because FastAPI is not installed; I relied on the green hosted quality/readiness workflow for runtime test coverage.

@yanyishuai

Copy link
Copy Markdown
Author

@qingfeng312 — Account accepted-work filter rejection covered on 3ba64c2c. CI green — ready for re-review.

Wallet: Do4v7foHJvRJLpRRoGaVPWX6DDEjX3yTK7J91gpwUQpE

@yanyishuai

Copy link
Copy Markdown
Author

@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.

@yanyishuai
yanyishuai force-pushed the bounty-1114-accepted-work-query-guard branch from 3ba64c2 to fe2f3da Compare July 10, 2026 05:56

@piaigmt piaigmt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.py and tests/test_account_routes.py.
  • The accepted-work route previously ignored all query params silently; now type and tx_type return HTTP 400 with a descriptive error, matching the sibling account detail route behavior.
  • Tests assert both type and tx_type return 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 passed
  • pytest tests/ -q → 905 passed
  • ruff check app/accounts.py tests/test_account_routes.py → all checks passed
  • ruff format --check app/accounts.py tests/test_account_routes.py → 2 files already formatted
  • mypy app/accounts.py → success, no issues
  • Hosted CI: Quality, readiness, docs, and image checks → pass

No further blocker found on this head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants