Move "For me" filter to server-side to fix table pagination#384
Move "For me" filter to server-side to fix table pagination#384frenzymadness wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR migrates event eligibility filtering from client-side state to server-side computation. The backend now identifies events with unoccupied spots matching the user's qualifications, applies filtering to the index query, and passes the state to the template. The template propagates ChangesFor Me Event Filter
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. 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
🧹 Nitpick comments (1)
tests/test_events.py (1)
1795-1810: ⚡ Quick winAssert pagination totals in
for_metests to lock the original regression.These tests verify visible rows, but they don’t assert the total/count output that issue
#326was about. Adding a total-count assertion will prevent regressions where rows are filtered but pagination metadata is wrong.Suggested test additions
def test_for_me_shows_only_eligible_events(self, app, client): @@ assert b"Eligible Event" in resp.data assert b"Ineligible Event" not in resp.data + assert "z celkem 1 akcí".encode() in resp.data def test_for_me_off_shows_all_events(self, app, client): @@ assert b"Eligible Event" in resp.data assert b"Ineligible Event" in resp.data + assert "z celkem 2 akcí".encode() in resp.data🤖 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_events.py` around lines 1795 - 1810, Add assertions in the two tests to validate the pagination total/count rendered in the response HTML so the pagination metadata matches the filtered rows: in test_for_me_shows_only_eligible_events (after calling resp = client.get("/events/?statuses=ASSIGNMENTS_OPEN&for_me=1")) assert the response contains the expected total count for only eligible events (e.g., "1" or the exact total string your UI renders for totals), and in test_for_me_off_shows_all_events (after resp = client.get("/events/?statuses=ASSIGNMENTS_OPEN")) assert the response contains the expected total count for all events (e.g., "2" or the exact total string); locate these assertions near the existing checks that look for "Eligible Event" / "Ineligible Event" using the resp variable and the test function names to find the right places.
🤖 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 `@app/routes/events/crud.py`:
- Around line 174-193: The function _eligible_event_ids_for_user currently
returns events with any unoccupied eligible spot but doesn’t enforce that the
event is in ASSIGNMENTS_OPEN (which _build_eligible_spot_map expects for
"for_me"); update the DB query in _eligible_event_ids_for_user to join Event and
add a where clause restricting Event.state == ASSIGNMENTS_OPEN (use the same
enum/constant used elsewhere), still filter out deleted quals and only include
qualifications in fillable_ids, and apply the same change to the analogous logic
around lines 205-207 so both places consistently consider only claimable events.
---
Nitpick comments:
In `@tests/test_events.py`:
- Around line 1795-1810: Add assertions in the two tests to validate the
pagination total/count rendered in the response HTML so the pagination metadata
matches the filtered rows: in test_for_me_shows_only_eligible_events (after
calling resp = client.get("/events/?statuses=ASSIGNMENTS_OPEN&for_me=1")) assert
the response contains the expected total count for only eligible events (e.g.,
"1" or the exact total string your UI renders for totals), and in
test_for_me_off_shows_all_events (after resp =
client.get("/events/?statuses=ASSIGNMENTS_OPEN")) assert the response contains
the expected total count for all events (e.g., "2" or the exact total string);
locate these assertions near the existing checks that look for "Eligible Event"
/ "Ineligible Event" using the resp variable and the test function names to find
the right places.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aa3cd18d-fe5c-49c0-a25c-ad19817d6ba3
📒 Files selected for processing (4)
app/routes/events/crud.pyapp/static/js/events-index.jsapp/templates/events/index.htmltests/test_events.py
| def _eligible_event_ids_for_user(user: UserAccount) -> list[int]: | ||
| """Return a list of event IDs where the user has at least one unoccupied, fillable spot.""" | ||
| fillable_ids = user_fillable_qual_ids(user) | ||
|
|
||
| # Find deleted qual IDs so we can ignore them (match existing eligibility logic) | ||
| deleted_qual_ids = { | ||
| q.id for q in db.session.scalars(db.select(Qualification).where(Qualification.is_deleted.is_(True))).all() | ||
| } | ||
|
|
||
| # Fetch all (spot_id, event_id, qual_id) for unoccupied spots | ||
| rows = db.session.execute( | ||
| db.select( | ||
| EventSpot.id.label("spot_id"), | ||
| EventSpot.event_id, | ||
| spot_qualifications.c.qualification_id, | ||
| ) | ||
| .outerjoin(Assignment, Assignment.spot_id == EventSpot.id) | ||
| .outerjoin(spot_qualifications, spot_qualifications.c.spot_id == EventSpot.id) | ||
| .where(Assignment.id.is_(None)) | ||
| ).all() |
There was a problem hiding this comment.
Align for_me eligibility with claimable-event status.
_eligible_event_ids_for_user() currently considers all events with unoccupied eligible spots, but the rest of the page contract (notably _build_eligible_spot_map) treats eligibility as claimable spots in ASSIGNMENTS_OPEN. This can surface non-claimable events under “Pro mě”.
Suggested fix
def _eligible_event_ids_for_user(user: UserAccount) -> list[int]:
@@
rows = db.session.execute(
db.select(
EventSpot.id.label("spot_id"),
EventSpot.event_id,
spot_qualifications.c.qualification_id,
)
+ .join(Event, Event.id == EventSpot.event_id)
.outerjoin(Assignment, Assignment.spot_id == EventSpot.id)
.outerjoin(spot_qualifications, spot_qualifications.c.spot_id == EventSpot.id)
- .where(Assignment.id.is_(None))
+ .where(
+ Assignment.id.is_(None),
+ Event.status == EventStatus.ASSIGNMENTS_OPEN,
+ )
).all()Also applies to: 205-207
🤖 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 `@app/routes/events/crud.py` around lines 174 - 193, The function
_eligible_event_ids_for_user currently returns events with any unoccupied
eligible spot but doesn’t enforce that the event is in ASSIGNMENTS_OPEN (which
_build_eligible_spot_map expects for "for_me"); update the DB query in
_eligible_event_ids_for_user to join Event and add a where clause restricting
Event.state == ASSIGNMENTS_OPEN (use the same enum/constant used elsewhere),
still filter out deleted quals and only include qualifications in fillable_ids,
and apply the same change to the analogous logic around lines 205-207 so both
places consistently consider only claimable events.
SummaryMoves eligibility filtering from client-side to server-side to fix pagination count mismatches. Changes Reviewed
Full review of all changes. Code Review🐛 Bugs & CorrectnessThe eligibility logic in 🔒 SecurityPermission checks are in place: the filter only applies if ⚡ PerformanceThe database query in 🏗️ Code QualityThe refactoring is clean — removes localStorage/client-side logic, centralizes filtering on the server. URL generation is consistent across all filter macros. The button-to-link change aligns with server-side routing. One minor note: the ✅ Tests & DocumentationTest coverage is strong:
All critical scenarios are covered. No docstrings needed for simple filter logic. Actionable SuggestionsNone — the implementation is solid and the refactoring fixes the pagination issue while improving consistency. Overall AssessmentLGTM ✅ Server-side eligibility filtering is now correctly integrated, fixing the pagination count issue while maintaining security and test coverage. Reviewed by MedCover Reviewer AI · Full review |
|
Can we move all those for loops down to the DB level so it's faster and cleaner? |
SummaryMigrates the "For me" (eligibility) filter from client-side JavaScript to server-side filtering, enabling accurate pagination when filtering is applied. Changes ReviewedFull diff: 5 files (app/routes/events/crud.py, app/static/js/events-index.js, app/templates/events/index.html, tests/test_events.py) Code Review🐛 Bugs & CorrectnessNo issues found. The
🔒 SecurityNo issues found. Permission check ( ⚡ PerformanceNo issues found. The eligibility query is optimized with appropriate joins. 🏗️ Code QualityNo issues found. Client-side filtering code is cleanly removed; server-side implementation is clear. ✅ Tests & DocumentationNo issues found. Tests cover:
Actionable SuggestionsNo changes needed. Overall AssessmentLGTM ✅ Reviewed by MedCover Reviewer AI · Full review |
SummaryMoves client-side filtering logic to server-side to fix table pagination issues. Changes Reviewed4 files modified - full review Code Review🐛 Bugs & CorrectnessNo issues found. 🔒 SecurityNo issues found. ⚡ PerformanceNo issues found. 🏗️ Code QualityNo issues found. ✅ Tests & DocumentationNo issues found. Actionable SuggestionsNone — this is a straightforward refactoring moving filter logic server-side to resolve pagination. Overall AssessmentLGTM ✅ Reviewed by MedCover Reviewer AI · Full review |
Changes Reviewedapp/routes/events/crud.py, app/static/js/events-index.js, app/templates/events/index.html, tests/test_events.py — Moves "For me" filter from client-side to server-side for proper pagination. Code Review🐛 Bugs & CorrectnessNo issues found. Server-side filtering logic is sound: 🔒 SecurityNo issues found. Query properly checks ⚡ PerformanceGood optimization. Moving to server-side fixes the pagination issue (client-side filtering breaks pagination semantics). Query uses 🏗️ Code QualityNo issues found. JavaScript simplification removes ~50 lines of local filtering/state management — good cleanup. ✅ Tests & DocumentationNo issues found. Server-side filter integration tested. Actionable SuggestionsNone. Overall AssessmentLGTM ✅ Solid refactor that fixes pagination correctness by moving filter logic to the server, with cleaner client code as a bonus. Reviewed by MedCover Reviewer AI · Full review |
2 participants
Fixes: #326
Summary by CodeRabbit
New Features
Tests