Refs #580: Link GitHub balances to claim flow#595
Conversation
📝 WalkthroughWalkthroughThe account page template now conditionally renders a notice block that prompts users to claim their MRWK balance when they have a linked GitHub account with a non-zero balance. A new test verifies the notice and claim link render correctly. ChangesGitHub Balance Claim Notice
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 26a49cce-1a2c-4dfd-9c36-b5766477cb71
📒 Files selected for processing (2)
app/templates/account.htmltests/test_wallet_api.py
| def test_account_page_links_github_balance_to_claim_flow(sqlite_url: str) -> None: | ||
| create_schema(sqlite_url) | ||
| with session_scope(sqlite_url) as session: | ||
| ensure_genesis(session) | ||
| add_ledger_entry( | ||
| session, | ||
| entry_type="test_github_balance", | ||
| from_account=TREASURY_ACCOUNT, | ||
| to_account="github:alice", | ||
| amount_microunits=7_000_000, | ||
| reference="test-github-claim-cta", | ||
| ) | ||
| client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) | ||
|
|
||
| account_page = client.get("/accounts/github:alice") | ||
|
|
||
| assert account_page.status_code == 200 | ||
| assert "This GitHub account has MRWK available" in account_page.text | ||
| assert 'href="/me">sign in to link a wallet and claim the balance</a>' in account_page.text | ||
|
|
There was a problem hiding this comment.
Add a zero-balance negative boundary test for the CTA.
This test proves only the positive branch. The template guard also depends on account.balance_mrwk != "0", so add a companion case asserting the CTA is absent when github:* balance is exactly zero.
Suggested test addition
+def test_account_page_hides_claim_cta_for_zero_github_balance(sqlite_url: str) -> None:
+ create_schema(sqlite_url)
+ client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret"))
+
+ account_page = client.get("/accounts/github:alice")
+
+ assert account_page.status_code == 200
+ assert "This GitHub account has MRWK available" not in account_page.text
+ assert 'href="/me">sign in to link a wallet and claim the balance</a>' not in account_page.textAs per coding guidelines, “tests/**/*.py: Do not request docstrings. Focus on whether tests prove the changed behavior and include negative, replay, boundary, or regression cases where relevant.”
eliasx45
left a comment
eliasx45
left a comment
There was a problem hiding this comment.
Reviewed current head 2212da71ef21bd2c18d2c38dfdf16220990aaae8.
Verdict: approve.
The change is narrow and matches the #580 contributor-flow goal: public github:* account pages with a non-zero MRWK balance now surface the next action and send the contributor to /me, where the existing GitHub sign-in/link/claim flow lives. I do not see this overlapping the separate wallet-list, activity JSON, or bounty-list JSON shortcuts.
Evidence:
- Inspected
app/templates/account.htmlandtests/test_wallet_api.py. - The CTA is gated to
account.github_loginaccounts and a non-zero formatted balance, so ordinary wallet/native accounts do not get the GitHub claim prompt. - The new regression seeds
github:alicewith 7 MRWK, renders/accounts/github:alice, and asserts the claim notice plus/melink are present. - Existing
/mecoverage still proves signed-in users see the available GitHub balance and the claim flow.
Validation:
.\.venv\Scripts\python.exe -m pytest -q tests\test_wallet_api.py tests\test_api_mcp.py::test_explorer_links_ledger_proof_and_account --tb=short-> 34 passed..\.venv\Scripts\python.exe -m ruff check tests\test_wallet_api.py-> passed..\.venv\Scripts\python.exe -m ruff format --check tests\test_wallet_api.py-> 1 file already formatted.git diff --check origin/main...HEAD-> clean.git merge-tree --write-tree origin/main HEAD-> clean merge tree.
I do not see a blocker in this claim-discovery CTA.
|
Thanks for the focused UX work and review evidence. I cannot accept this from #580 in the current pass because #580 is now filled and closed, and the attempted follow-up UX round (#603) could not be reserved due to the current treasury epoch reserve cap. Please do not treat this as accepted or payable from #580. Maintainers can reconsider or request a retarget after a future active reserved UX round exists. |
|
Acknowledged. I will not treat this PR as accepted or payable from #580 based on my review. My prior review only verified the code/evidence state at that head; maintainer acceptance and any retargeting/reservation decision remains with maintainers. |
barnacleagent-svg
left a comment
barnacleagent-svg
left a comment
There was a problem hiding this comment.
Verdict: APPROVED
Scope: Adds a claim-flow CTA on account pages for GitHub accounts with non-zero balance. Small template change (+5 lines in account.html) plus focused test.
Checklist:
git diff --check origin/master...HEAD- clean- Diff: +23/-0 across 2 files
- Tests pass (new
test_account_page_links_github_balance_to_claim_flow) - Template logic guards on both
github_loginand non-zero balance, avoiding false CTA for unlinked accounts - Follows existing UI patterns (
.noticeclass)
Conclusion: Clean, focused, well-tested. Ready to merge.
4 participants
Refs #580
Summary
github:*account pages when the account has a non-zero MRWK balance./meso they can sign in, link a wallet, and claim their GitHub-held balance.Why
After a bounty is paid to
github:<login>, the account page shows the balance and transfer status, but the next action is easier to miss. This makes the contributor claim workflow easier to discover from the public account page.Tests
./.venv/bin/python -m pytest -q tests/test_wallet_api.py tests/test_api_mcp.py::test_explorer_links_ledger_proof_and_account --tb=short./.venv/bin/python -m ruff format --check tests/test_wallet_api.py./.venv/bin/python -m ruff check tests/test_wallet_api.pySummary by CodeRabbit