Skip to content

feat(grants): add filters to shortlist page (needs_funds_for_travel, need_visa, need_accommodation, country_type)#4651

Merged
estyxx merged 2 commits into
mainfrom
feature/grants-shortlist-filters
May 21, 2026
Merged

feat(grants): add filters to shortlist page (needs_funds_for_travel, need_visa, need_accommodation, country_type)#4651
estyxx merged 2 commits into
mainfrom
feature/grants-shortlist-filters

Conversation

@estyxx
Copy link
Copy Markdown
Member

@estyxx estyxx commented May 21, 2026

Summary

  • Adds four new client-side filters to the grants review shortlist page at /admin/reviews/reviewsession/<id>/review/shortlist/
  • Needs funds for travel – Yes / No checkboxes
  • Need visa – Yes / No checkboxes
  • Need accommodation – Yes / No checkboxes
  • Country type – Italy / Europe / Extra EU / Unknown checkboxes (choices driven from Grant.CountryType via the context)
  • Exposes country_type_choices in GrantsReviewAdapter.get_shortlist_context so the template can render the country-type options dynamically
  • Adds data-needs-funds-for-travel, data-need-visa, data-need-accommodation, data-country-type HTML data-* attributes on each <tr class="grant-item"> row
  • Refactors the old status-only filter JS into a single unified applyFilters() function that ANDs all five filter groups together; all checkboxes start checked (show all), unchecking hides matching rows

Test plan

  • Open the grants shortlist page for a review session with grants
  • Verify all four new filter groups appear in the Filters panel, all checkboxes pre-checked
  • Uncheck "Yes" on Needs funds for travel → rows where needs_funds_for_travel=True hide; re-check → they reappear
  • Repeat for Need visa and Need accommodation
  • Uncheck individual Country type options (e.g. "Europe") → grants from European countries hide
  • Confirm the existing status filter still works correctly in combination with the new filters
  • Confirm the bottom-bar stats, decision radios, and sorting are unaffected

…ortlist

Add four new client-side filters to the grants review shortlist page:
needs_funds_for_travel, need_visa, need_accommodation, and country_type.
Refactors the existing status-only filter JS into a unified applyFilters()
function that ANDs all active filter groups together.
@vercel
Copy link
Copy Markdown

vercel Bot commented May 21, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
pycon Error Error May 21, 2026 2:21pm

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 21, 2026

Adds client-side filters (needs_funds_for_travel, need_visa, need_accommodation, country_type) to the grants shortlist page, refactoring per-field status filter JS into a unified applyFilters() function.

i18n inconsistency

backend/reviews/adapters.py line 470: The appended "Unknown" sentinel is a plain Python string, inconsistent with all other CountryType.choices labels which use lazy translation strings (_()).

Behavior change in status filter not documented

backend/reviews/templates/grants-shortlist.html lines 461–462: The new matchesFilter semantics treat an empty checked-set as "ignore this filter / show all." This is a behavior change for the pre-existing status filter (previously unchecking all would hide all rows). Not documented in code or PR description.

Incomplete test coverage

backend/reviews/tests/test_admin.py: need_visa and need_accommodation have no backend tests verifying their boolean→template string pipeline, even though an equivalent test was written for needs_funds_for_travel.

Cryptic test failure on missing grant

backend/reviews/tests/test_admin.py line 1003: next(item for item in context["items"] if item.id == grant.id) raises StopIteration (not AssertionError) if the grant is absent. Prefer next(..., None) with an explicit assert for a readable failure message.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 21, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.52%. Comparing base (e3a604e) to head (5a6991b).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4651   +/-   ##
=======================================
  Coverage   92.52%   92.52%           
=======================================
  Files         359      359           
  Lines       10791    10791           
  Branches      821      821           
=======================================
  Hits         9984     9984           
  Misses        696      696           
  Partials      111      111           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

- Remove dead fields from grantsById JS object; filters read from data-*
  attributes only, so the object fields were a false source-of-truth
- Move the null country_type sentinel ("", "Unknown") into the adapter
  context so the template loop handles all options uniformly
- Call applyFilters() on page load to sync rows with bfcache-restored state
- Treat all-unchecked filter group as "show all" via matchesFilter helper,
  preventing silent blank tables when every option in a group is unchecked
- Add three pytest tests: country_type_choices present in context, grant
  with no departure_country maps to the empty-string sentinel, and
  needs_funds_for_travel boolean is correctly exposed on queryset items
@estyxx
Copy link
Copy Markdown
Member Author

estyxx commented May 21, 2026

@claude do the code review again.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 21, 2026

Claude finished @estyxx's task in 1m 49s —— View job


Code Review - Re-review

  • Understand the request
  • Read changed files
  • Review the code changes
  • Provide feedback

Summary: The feedback from my previous review has been addressed well. The code is now in good shape for merging.

Issues Fixed ✅

All five issues from my previous review have been addressed:

  1. Dead code in grantsById — Removed the filter-related fields that were never read; only the fields actually used by updateBottomBarUI() remain.

  2. Hardcoded "Unknown" checkbox — Now handled uniformly via country_type_choices=[*Grant.CountryType.choices, ("", "Unknown")] in the adapter (adapters.py:470), so the template loop renders all options consistently.

  3. applyFilters() not called on page load — Fixed at grants-shortlist.html:496. Rows now sync correctly with any bfcache-restored checkbox state.

  4. All-unchecked produces silent blank table — The new matchesFilter() helper (grants-shortlist.html:461-462) treats an empty checked-list as "show all", preventing the blank-table footgun.

  5. Missing tests — Three new tests added:

    • test_grants_review_get_shortlist_context_includes_country_type_choices — verifies the sentinel is present
    • test_grants_review_get_shortlist_context_grant_with_no_departure_country — verifies null country_type maps to empty string
    • test_grants_review_get_shortlist_context_needs_funds_for_travel — verifies boolean exposure on queryset

Minor Observations (Non-blocking)

  1. Mixed window load handler patterns (grants-shortlist.html): The file uses both window.addEventListener("load", ...) (lines 351, 599) and direct window.onload = ... (line 416). Both work correctly, but using addEventListener consistently would be cleaner.

  2. Test coverage for need_visa and need_accommodation: The needs_funds_for_travel boolean is explicitly tested, but the other two booleans rely on the same pattern. Adding similar tests would provide extra confidence, though the current coverage is reasonable.

Verdict

LGTM 👍 — The PR addresses the review feedback thoroughly and the implementation is solid. Ready to merge.


|

@estyxx estyxx merged commit 3042be5 into main May 21, 2026
7 of 8 checks passed
@estyxx estyxx deleted the feature/grants-shortlist-filters branch May 21, 2026 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant