Add admin webhook status summary#331
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a prioritized, case-insensitive summary of webhook delivery outcomes to the admin page: defines a scan-order mapping, computes counts grouped by normalized ChangesWebhook status summary
Sequence Diagram(s)sequenceDiagram
participant AdminRoute
participant webhook_status_summary
participant DB
AdminRoute->>webhook_status_summary: call with Session
webhook_status_summary->>DB: query WebhookEvent.processed_status
DB-->>webhook_status_summary: rows (case-variant statuses)
webhook_status_summary-->>AdminRoute: sorted [{processed_status,count}, ...]
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
TateLyman
left a comment
TateLyman
left a comment
There was a problem hiding this comment.
The summary implementation looks behaviorally focused, but I would clean up two merge/validation issues before accepting it.
Evidence checked:
- Inspected
app/main.py,app/templates/admin.html, andtests/test_security.pyon current PR head617d03080a6ecee23e3cf0c03a789a073cd33233. - Verified the admin route still uses the existing
/admincookie flow before querying webhook data. - Verified the added summary groups
processed_statuscase-insensitively and renders only aggregate labels/counts, not payload bodies. - Ran the focused admin rendering test locally:
uv run --extra dev python -m pytest tests/test_security.py::test_admin_page_renders_safe_webhook_events_for_cookie_admin -q-> 1 passed. - Ran Python-only validation:
uv run --extra dev ruff check app/main.py tests/test_security.py-> passed;uv run --extra dev ruff format --check app/main.py tests/test_security.py-> 2 files already formatted;uv run --extra dev python -m mypy app/main.py-> success;git diff --check origin/main...HEAD-> clean.
Actionable notes:
- GitHub currently reports this PR as
DIRTY, so it needs a rebase/update before maintainers can merge even though the local diff-check is clean. - The PR description's Ruff command includes
app/templates/admin.html. Running that exact command locally fails because Ruff parses the Jinja template as Python. Please update the validation notes to either run Ruff on Python files only or use a template-aware check foradmin.html.
LubuSeb
force-pushed
the
codex/mergework-admin-webhook-summary
branch
from
617d030 to
00ca4f2
Compare
|
Updated. I rebased the branch onto current I also updated the PR description with the post-rebase validation and kept Ruff scoped to Python files only:
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_security.py`:
- Around line 200-210: Add assertions in tests/test_security.py that verify the
summary priority ordering by checking string positions in the rendered summary
HTML: use all_events.text.find(...) (or .index(...)) to assert that
"exhausted_bounty" appears before "missing_submitter" and that
"missing_submitter" appears before "paid" (i.e., assert idx_exhausted <
idx_missing < idx_paid), and similarly assert ordering for filtered.text (e.g.,
that "delivery-missing-submitter" appears before
"delivery-missing-submitter-uppercase") so the scan-order contract is enforced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 86af19d9-f6e5-4f3f-8edd-117ac864b2ae
📒 Files selected for processing (3)
app/main.pyapp/templates/admin.htmltests/test_security.py
|
Rechecked after the rebase/force-push. My two original blockers are resolved now:
Local validation on current head
The remaining CodeRabbit note about asserting summary/display ordering looks valid but lower-severity; it would improve regression coverage for the ordering contract but is not the same merge/validation blocker I originally flagged. |
|
Addressed the CodeRabbit ordering test suggestion in I added a rendered admin-page assertion that locks the intended status summary priority order: Validation after the follow-up commit:
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_security.py (1)
204-210:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winScope status-order assertions to the summary block only.
Using
all_events.text.index(...)here allows matches from the table, so Line 205-Line 208 can still pass even if a summary item regresses (for example,paidmissing from the summary but present in rows). Extract.bounty-list-summaryfirst, then assert order within that substring.Suggested patch
- status_summary_positions = [ - all_events.text.index("<code>missing_submitter</code>"), - all_events.text.index("<code>bounty_not_found</code>"), - all_events.text.index("<code>exhausted_bounty</code>"), - all_events.text.index("<code>paid</code>"), - ] + summary_match = re.search( + r'<div class="bounty-list-summary"[^>]*>.*?</div>', all_events.text, flags=re.S + ) + assert summary_match is not None + summary_html = summary_match.group(0) + status_summary_positions = [ + summary_html.index("<code>missing_submitter</code>"), + summary_html.index("<code>bounty_not_found</code>"), + summary_html.index("<code>exhausted_bounty</code>"), + summary_html.index("<code>paid</code>"), + ] assert status_summary_positions == sorted(status_summary_positions)As per coding guidelines,
tests/**/*.py: Add or update tests for changed behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_security.py` around lines 204 - 210, The current order check uses all_events.text.index(...) which can match items outside the summary; instead extract only the bounty summary HTML/text first (the .bounty-list-summary block from all_events.text), then compute status_summary_positions by calling .index(...) on that summary substring and assert those positions are sorted; update uses of status_summary_positions and all_events to operate on the extracted summary string so the order assertion only checks the summary block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@tests/test_security.py`:
- Around line 204-210: The current order check uses all_events.text.index(...)
which can match items outside the summary; instead extract only the bounty
summary HTML/text first (the .bounty-list-summary block from all_events.text),
then compute status_summary_positions by calling .index(...) on that summary
substring and assert those positions are sorted; update uses of
status_summary_positions and all_events to operate on the extracted summary
string so the order assertion only checks the summary block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1feefb12-e173-482a-9110-7bbfb2235fb9
📒 Files selected for processing (1)
tests/test_security.py
|
Updated again in Validation after this commit:
|
ramimbo
added
mrwk:accepted
3 participants
Refs #316
What changed:
/adminwebhook outcomes section.missing_submitter,bounty_not_found,exhausted_bounty, duplicate delivery, already-paid, and paid./admin; no admin-token-only API path or mutation control is added.Why this is distinct:
Post-rebase validation:
C:\Users\sebas\Documents\BountyBar\work\mergework-281\.venv\Scripts\python.exe -m pytest -q-> 318 passed.C:\Users\sebas\Documents\BountyBar\work\mergework-281\.venv\Scripts\python.exe -m ruff check app/main.py tests/test_security.py-> passed.C:\Users\sebas\Documents\BountyBar\work\mergework-281\.venv\Scripts\python.exe -m ruff format --check app/main.py tests/test_security.py-> passed.C:\Users\sebas\Documents\BountyBar\work\mergework-281\.venv\Scripts\python.exe -m mypy app-> success.git diff --check origin/main...HEAD-> clean.Summary by CodeRabbit
New Features
Tests