Bounty #427: Add account workflow shortcuts#483
Conversation
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughThis PR inserts an "Account shortcuts" actions block into the account page template (activity search and account-scoped JSON endpoints) and updates tests to assert those links and verify URL-encoding of account path segments. ChangesAccount Shortcuts
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
GHX5T-SOL
left a comment
There was a problem hiding this comment.
I found one blocker in the new account shortcuts.
The activity link correctly URL-encodes the account as a query parameter, but the two API links insert account.account directly into path segments. For non-prefixed account IDs containing URL-significant characters, those shortcuts point at the wrong account.
Repro on current head b7df70de3d1c781e1e92dfb25ccc436ac53760dc:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python - <<'PY'
from fastapi.testclient import TestClient
from app.db import create_schema
from app.main import create_app
import pathlib
import tempfile
path = pathlib.Path(tempfile.mkdtemp()) / "db.sqlite3"
url = f"sqlite:///{path}"
create_schema(url)
client = TestClient(create_app(database_url=url, webhook_secret="secret"))
resp = client.get("/accounts/foo%23bar")
print(resp.status_code)
for frag in [
'href="/activity?q=foo%23bar"',
'href="/api/v1/accounts/foo%23bar/accepted-work"',
'href="/api/v1/accounts/foo%23bar"',
'href="/api/v1/accounts/foo#bar/accepted-work"',
]:
print(frag, frag in resp.text)
PYObserved:
200
href="/activity?q=foo%23bar" True
href="/api/v1/accounts/foo%23bar/accepted-work" False
href="/api/v1/accounts/foo%23bar" False
href="/api/v1/accounts/foo#bar/accepted-work" True
In a browser, #bar is a fragment, so the API shortcut navigates to /api/v1/accounts/foo..., not account foo#bar. Please URL-encode the account value in both API shortcut path segments and add a regression around /accounts/foo%23bar.
Validation I ran:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_account_routes.py -q -> 3 passed
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest -q -> 414 passed
uv run --extra dev ruff check tests/test_account_routes.py -> passed
uv run --extra dev ruff format --check tests/test_account_routes.py -> 1 file already formatted
uv run --extra dev python -m mypy app -> success
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python scripts/docs_smoke.py -> docs smoke ok
git diff --check origin/main...HEAD -> clean
gitleaks detect --no-banner --redact --source . --log-opts origin/main..HEAD --exit-code 1 -> no leaks found
|
Thanks for the precise repro. Addressed in Changes made:
Validation rerun: |
GHX5T-SOL
left a comment
There was a problem hiding this comment.
Reviewed the updated head 61cfbc4f4e78329de92c734cf39a8d059fe418a0.
The account shortcut blocker from my earlier review is fixed. The current template URL-encodes the account value in both API shortcut path segments, so an account like foo#bar renders /api/v1/accounts/foo%23bar and /api/v1/accounts/foo%23bar/accepted-work instead of fragment-form foo#bar URLs.
Validation on this head:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_account_routes.py -q -> 4 passed
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_account_routes.py tests/test_account_validation.py tests/test_api_mcp.py::test_explorer_links_ledger_proof_and_account tests/test_bounty_pages.py tests/test_wallet_api.py -q -> 69 passed
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest -q -> 415 passed
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python scripts/docs_smoke.py -> docs smoke ok
uv run --extra dev ruff check tests/test_account_routes.py -> passed
uv run --extra dev ruff format --check tests/test_account_routes.py -> 1 file already formatted
uv run --extra dev python -m mypy app -> success
git diff --check origin/main...HEAD -> clean
gitleaks detect --no-banner --redact --source . --log-opts origin/main..HEAD --exit-code 1 -> no leaks found
GitHub CI and CodeRabbit are also successful on the current head. I do not see a remaining blocker.
jtc268
left a comment
There was a problem hiding this comment.
Approved.
I checked the account shortcut additions and the tests now cover URL-encoded account path segments like foo#bar. Validation I ran:
- focused account shortcut tests: 2 passed
- tests/test_account_routes.py: 4 passed
- ruff check/format on tests/test_account_routes.py passed
- mypy app/accounts.py passed
- docs smoke passed
- git diff --check clean
One local note: I initially ran ruff against the Jinja template, which is not a Python target; rerunning the Python-only ruff checks passed.
eliasx45
left a comment
There was a problem hiding this comment.
Reviewed current head 61cfbc4 against refreshed origin/main.
Verdict: request changes.
The account shortcut behavior itself still validates locally: the account page renders activity/account JSON/accepted-work JSON shortcuts, API path segments are URL-encoded, and the focused account/public route tests pass. However, the branch is no longer merge-ready against current main because merge-tree reports a content conflict in tests/test_account_routes.py.
Validation on this head:
- PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ..venv\Scripts\python.exe -m pytest tests\test_account_routes.py tests\test_account_validation.py tests\test_api_mcp.py::test_explorer_links_ledger_proof_and_account tests\test_bounty_pages.py tests\test_wallet_api.py -q -> 69 passed
- ..venv\Scripts\python.exe -m ruff check tests\test_account_routes.py -> passed
- ..venv\Scripts\python.exe -m ruff format --check tests\test_account_routes.py -> 1 file already formatted
- PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ..venv\Scripts\python.exe scripts\docs_smoke.py -> docs smoke ok
- git diff --check origin/main...HEAD -> clean
- git merge-tree --write-tree origin/main HEAD -> conflict in tests/test_account_routes.py
Required follow-up: rebase/merge current main, resolve tests/test_account_routes.py, and rerun the focused account-route tests. I do not see a remaining account-shortcut behavior blocker beyond the stale/conflicting test file.
…-navigation # Conflicts: # tests/test_account_routes.py
|
Thanks for the refreshed-main check. I merged current Validation after the update:
GitHub now reports the PR as mergeable/clean on the updated head. |
eliasx45
left a comment
There was a problem hiding this comment.
Re-reviewed current head d2499dbfa8d5bc74635768c17922024af5b62d60 after the branch was refreshed against current main.
Verdict: approve.
The previous blocker was merge readiness only; that is resolved now. Validation on this head:
git diff --check origin/main...HEAD
# clean
git merge-tree --write-tree origin/main HEAD
# 8363d73bb91651613578ca30aed75b02a34c2b5a
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python -m pytest tests/test_account_routes.py -q
# 5 passed
python -m py_compile app/accounts.py
# passed
python -m ruff check tests/test_account_routes.py
# All checks passed
python -m ruff format --check tests/test_account_routes.py
# 1 file already formatted
The account-shortcut behavior remains correct: activity and API shortcuts URL-encode account values in both query and path positions, and the /accounts/foo%23bar regression protects against fragment-form links.
Summary
/activityRefs #427.
Before / After
Before: contributors and maintainers viewing an account page had to manually copy the account id into
/activityor type API URLs to inspect the account and its proof-backed accepted-work rows.After: the account detail header has focused shortcuts for the common follow-up actions: search activity, open accepted-work JSON, and open account JSON. The links use the normalized account id and URL-encode both the activity query and account API path segments.
Distinctness
This targets the account-detail workflow. It is distinct from the existing #427 work I found on bounty list styling/back navigation, bounty empty states, wallet private-key copy, bounty sorting, bounty detail next steps/attempts, activity empty states, and proof JSON disclosure.
Validation
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ~/.local/bin/uv run --extra dev python -m pytest -q-> 414 passedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_account_routes.py -q-> 4 passedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev ruff check tests/test_account_routes.py-> passedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev ruff format --check tests/test_account_routes.py-> 1 file already formattedgit diff --check-> cleanSummary by CodeRabbit
New Features
Tests