Extract account explorer helpers#358
Conversation
|
Warning Review limit reached
More reviews will be available in 3 minutes and 33 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR factors account- and explorer-related helper logic into dedicated modules, updates the FastAPI app to use them, and adds tests for the new helper behavior.
Changes:
- Added
app.accountsfor account normalization + API summary helpers and refactoredapp.mainto use it. - Added
app.explorerfor proof/transaction query helpers and refactored account/wallet pages + ledger API to use it. - Added tests covering new account normalization helpers and explorer query helpers; hardened
_safe_next_pathagainst percent-encoded unsafe paths.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_explorer.py | Adds tests for explorer helper functions (proof lookup + transaction query behavior). |
| tests/test_accounts.py | Adds tests for account normalization and account summary helper output. |
| app/main.py | Refactors endpoints/pages to use new helpers; strengthens _safe_next_path. |
| app/ledger/service.py | Simplifies update execution typing/casts around SQLAlchemy execute(). |
| app/explorer.py | Introduces reusable explorer query helpers for proofs and account transactions. |
| app/accounts.py | Introduces reusable account normalization + presentation helpers (summary/status/github login). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rows = ledger_transactions_for_account(session, "github:alice") | ||
| limited = ledger_transactions_for_account(session, "github:alice", limit=1) | ||
|
|
||
| assert [row["sequence"] for row in rows] == [manual.sequence, proof.ledger_sequence] | ||
| assert rows[0]["proof_hash"] is None | ||
| assert rows[1]["proof_hash"] == proof.hash | ||
| assert [row["sequence"] for row in limited] == [manual.sequence] |
| account_data = api_account(account) | ||
| accepted_summary = safe_account_accepted_summary(session, account) | ||
| entries = session.scalars( | ||
| select(LedgerEntry) | ||
| .where(or_(LedgerEntry.from_account == account, LedgerEntry.to_account == account)) | ||
| .order_by(LedgerEntry.sequence.desc()) | ||
| .limit(100) | ||
| ).all() | ||
| proofs = _proof_hashes_by_sequence(session, [entry.sequence for entry in entries]) | ||
| transactions = [ledger_to_dict(entry, proofs.get(entry.sequence)) for entry in entries] | ||
| transactions = ledger_transactions_for_account(session, account) | ||
| accepted_work = safe_accepted_work_for_account(session, account) |
tolga-tom-nook
left a comment
tolga-tom-nook
left a comment
There was a problem hiding this comment.
No blockers found on PR #358 at head 3bb705c.
Evidence checked:
- Inspected
app/accounts.pyextraction for account normalization, GitHub login parsing, transfer-status text, and account API summary shape; it preserves the existing HTTP 400 wrapping through_normalized_account(). - Inspected
app/explorer.pyextraction for proof-hash lookup and account/wallet ledger transaction rows; the shared helper keeps descending sequence order, 100-row default limit, and optional proof hashes. - Checked the
app/main.pycall sites for/api/v1/accounts/{account}, account pages, wallet pages, and/api/v1/ledger; the account page reassigns the normalized account before querying transactions, so the normalized value is used afterapi_account(). - Reviewed the new tests in
tests/test_accounts.pyandtests/test_explorer.py, including account canonicalization/rejection, API summary shape, empty proof lookup, proof attachment, ordering, and limit behavior.
Commands run locally:
./.venv/bin/python -m pytest tests/test_accounts.py tests/test_explorer.py tests/test_wallet_api.py tests/test_activity.py -q→ 47 passed./.venv/bin/python -m ruff check app/accounts.py app/explorer.py app/main.py tests/test_accounts.py tests/test_explorer.py→ passed./.venv/bin/python -m ruff format --check app/accounts.py app/explorer.py app/main.py tests/test_accounts.py tests/test_explorer.py→ passed./.venv/bin/python -m mypy app→ passedgit diff --check origin/main...HEAD→ passed
newmattock
left a comment
newmattock
left a comment
There was a problem hiding this comment.
No blockers found on current head 3bb705c.
Evidence checked:
- Inspected
app/explorer.py,app/accounts.py, account/wallet page delegation inapp/main.py, andtests/test_explorer.py. - Verified
proof_hashes_by_sequence()preserves the shared proof lookup behavior used by ledger/account/wallet views and returns an empty mapping for empty sequence lists. - Verified
ledger_transactions_for_account()preserves the account and wallet transaction query boundary: matches eitherfrom_accountorto_account, orders newest-first by ledger sequence, honors the limit argument, and attaches bounty-payment proof hashes throughledger_to_dict(). - Rechecked the account API shape and malformed-proof fallback path because this branch also moves account normalization/summary helpers.
Validation run locally:
uv run --extra dev python -m pytest tests/test_explorer.py tests/test_accounts.py tests/test_api_mcp.py::test_explorer_links_ledger_proof_and_account tests/test_api_mcp.py::test_account_api_keeps_schema_when_accepted_work_proof_is_malformed tests/test_wallet_api.py::test_github_login_stores_safe_default_for_backslash_next -q-> 17 passed.uv run --extra dev python -m mypy app/accounts.py app/explorer.py app/main.py-> success.uv run --extra dev python -m ruff check app/accounts.py app/explorer.py app/main.py tests/test_accounts.py tests/test_explorer.py-> passed.uv run --extra dev python -m ruff format --check app/accounts.py app/explorer.py app/main.py tests/test_accounts.py tests/test_explorer.py-> 5 files already formatted.git diff --check origin/main...HEAD-> clean.
|
Maintainer note after #320 filled: this PR still references closed bounty #320. If you want it considered for the current code-health round, rebase against main, update the bounty reference to #377, and keep the scope distinct from already accepted #328, #337, #364, #375, and #376. This is not accepted or paid as-is. |
Refs #320
Summary
app/accounts.py.app/explorer.py, then reuse it from ledger, account, and wallet page routes.mypy appand by sanitizing percent-decoded OAuthnextpaths covered by the existing OAuth regression test.Complexity reduced
app/main.pyno longer owns account canonicalization rules, account API response shaping, proof hash lookup, or duplicated account/wallet transaction queries. Those responsibilities now live in small modules with direct tests, while route handlers remain thin wrappers around request/session/template concerns.Tests
python -m pytest -q-> 349 passedpython -m pytest tests/test_accounts.py tests/test_explorer.py tests/test_account_validation.py tests/test_wallet_api.py::test_wallet_pages_expose_transfer_and_github_claim_flows tests/test_api_mcp.py::test_explorer_links_ledger_proof_and_account -q-> 41 passedpython -m pytest tests/test_explorer.py tests/test_account_validation.py tests/test_wallet_api.py::test_wallet_pages_expose_transfer_and_github_claim_flows tests/test_api_mcp.py::test_explorer_links_ledger_proof_and_account tests/test_ledger.py -q-> 59 passedpython -m ruff format --check app/accounts.py app/explorer.py app/main.py app/ledger/service.py tests/test_accounts.py tests/test_explorer.pypython -m ruff check app/accounts.py app/explorer.py app/main.py app/ledger/service.py tests/test_accounts.py tests/test_explorer.pypython -m mypy app