Skip to content

feat: add project filtering with fix for multi-filter queries#5614

Open
rajohnson90 wants to merge 9 commits intomainfrom
OPS-5241/filter-projects
Open

feat: add project filtering with fix for multi-filter queries#5614
rajohnson90 wants to merge 9 commits intomainfrom
OPS-5241/filter-projects

Conversation

@rajohnson90
Copy link
Copy Markdown
Contributor

@rajohnson90 rajohnson90 commented May 1, 2026

What changed

This PR adds comprehensive project filtering to the Projects list page and fixes a critical backend bug that prevented multiple filters from working together correctly.

Backend query fix (backend/ops_api/ops/services/projects.py)

Replaced LEFT OUTER JOIN-based filtering with EXISTS subqueries in _get_research_projects_query and _get_administrative_and_support_projects_query. The original approach required a single joined row to satisfy all filter conditions simultaneously, which failed when:

  • One agreement had budget line items in the target fiscal year
  • A different agreement on the same project matched the agreement search term

The new approach uses independent EXISTS subqueries for each filter (portfolio, fiscal year, agreement search), allowing each filter to match different related records while still applying AND logic at the project level.

Frontend filtering UI (frontend/src/)

  • ProjectFilterButton/: New modal-based filter component with multi-select dropdowns for fiscal year, portfolio, project title, project type, and agreement name
  • ProjectFilterTags/: Displays active filters as removable tags with clear visual feedback
  • ProjectTypeComboBox/: Reusable project type selector component
  • Updated ProjectsList.jsx to integrate filtering UI and handle filter state

API enhancement

Agreement names in the /projects/filters endpoint now return {id, name} objects instead of plain strings, aligning with other filter options and enabling better frontend handling.

Tests

  • 260 lines of Cypress E2E tests covering all filter combinations
  • New backend test for combined fiscal year + agreement filtering scenario

Issue

#5241

How to test

Backend tests:

cd backend/ops_api
pipenv run pytest tests/ops/project/test_project.py::test_agreement_and_fiscal_year_filter -xvs
pipenv run pytest tests/ops/project/test_project.py::TestProjectFilterOptions -xvs

Frontend E2E tests:

cd frontend
npm run cypress:open
# Run projectsList.cy.js - new tests starting at line 360

Manual verification:

  1. Navigate to /projects
  2. Click the "Filter" button
  3. Select "FY 2044" from fiscal year dropdown
  4. Select "AA here's a note #1: Fathers and Continuous Learning (FCL)" from agreement dropdown
  5. Click "Apply"
  6. ✅ "Annual Performance Plans and Reports" project should appear (previously returned no results)
  7. Verify filter tags display: "FY 2044" and "AA here's a note #1: Fathers and Continuous Learning (FCL)"
  8. Click the X on a filter tag to remove it
  9. Click "Filter" → "Reset" to clear all filters
  10. Test other filter combinations (portfolio, project type, project title)

A11y impact

  • Color contrast verified for filter tags and buttons
  • Keyboard navigation tested in filter modal
  • Screen reader announces filter application and removal
  • Focus management when opening/closing modal

Screenshots

The image below shows the UI element
Screenshot 2026-05-01 at 11 57 56 AM

Definition of Done Checklist

  • OESA: Code refactored for clarity
  • OESA: Dependency rules followed
  • Automated unit tests updated and passed
  • Automated integration tests updated and passed
  • Automated quality tests updated and passed
  • Automated load tests updated and passed
  • Automated a11y tests updated and passed
  • Automated security tests updated and passed
  • 90%+ Code coverage achieved
  • Form validations updated

Links

Related context:

  • Backend uses EXISTS subqueries pattern similar to Agreement filtering
  • Frontend filter patterns follow existing CAN and Agreement list filter implementations
  • OpenAPI schema updated to match filter response structure

@rajohnson90 rajohnson90 self-assigned this May 1, 2026
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Pagination not reset when filters change

ProjectsList.jsx:69-71 — the useEffect that resets pagination only depends on sort/fiscal year.

filters is missing from the dependency array. If a user is on page 3 and applies a filter that returns fewer results, they'll see an empty page. filters needs to be added here.

Comment thread frontend/src/pages/projects/list/ProjectFilterTags/ProjectFilterTags.hooks.js Outdated
@rajohnson90 rajohnson90 requested a review from weimiao67 May 4, 2026 14:09
Copy link
Copy Markdown
Contributor

@johndeange johndeange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overview

Adds project filtering UI (modal + tags) and fixes a backend bug where combining fiscal year + agreement filters returned empty results. The EXISTS-subquery approach is sound and matches patterns used elsewhere in the codebase. Two blocking issues before merge.


🔴 Blocking

1. Broken assertion in new backend test

backend/ops_api/tests/ops/project/test_project.py:360

assert response_research == 200

response_research is a TestResponse object, not an int — this comparison is always False. Should be:

assert response_research.status_code == 200

This test is the whole motivation for the backend fix, so it needs to actually execute. Consider also adding a negative case (agreement matches but FY doesn't → empty result).

2. Frontend/backend contract mismatch on `agreement_names`

Backend now returns `agreement_names: [{id, name}]`, but `frontend/src/api/opsAPI.js:486` reads `.title`:

queryParams.push(\`agreement_search=\${encodeURIComponent(agreement.title)}\`);

If `AgreementNameComboBox` transforms `{id, name}` → items with `title` before they land in `agreementSearch` state, this works — but the transform isn't visible in this diff and is easy to break. Please verify end-to-end and ideally add a test that asserts the produced query string. The mocked combobox in `ProjectFilterButton.test.jsx` uses `{title: "Agreement 1"}`, so the unit tests wouldn't catch a shape mismatch.


Important

Inconsistent shape for `projectType` filter

`opsAPI.js:500` sends `type.name`, but `ProjectFilterTags.hooks.js:111` removes by `type.title`, and tests use both `{title: "RESEARCH"}` and `{name: PROJECT_TYPE_RESEARCH}`. Pick one key (recommend `name` since it's an enum value) and apply everywhere.

`Modal.setAppElement("#root")` inside render

`ProjectFilterButton.jsx:109` — runs on every render. Move to module level or a `useEffect` with empty deps.

`useTagsList` is more complex than needed

`ProjectFilterTags.hooks.js` has 5 `useEffect`s, a `useCallback`, and three near-duplicate branches that differ only by `item.name` vs `item.title`. This can collapse to a single `useMemo` derived from `filters` — no state, no effects.

const tagsList = useMemo(() => {
    const map = { portfolio: 'name', fiscalYear: 'title', projectSearch: 'title', agreementSearch: 'title', projectType: 'title' };
    return Object.entries(map).flatMap(([key, prop]) =>
        (filters[key] ?? []).map(item => ({ tagText: item[prop], filter: key }))
    );
}, [filters]);

`useProjectFilterButton` sync pattern

The 5 `useEffect`s that copy `filters.X` → local state have a subtle UX quirk: if the user opens the modal, makes selections, closes without Apply, the internal state persists on re-open. Confirm this matches the design.


Minor

  • `console.warn` in `removeFilter` default case — unreachable with current types, remove it.
  • `ProjectFilterTypes.d.ts` defines `Filters` but `ProjectFilterTags.hooks.js` redefines it via JSDoc — consolidate.
  • `ProjectTitleComboBox.jsx:13-14` JSDoc typo: "IS the data for" → "Is the data for".
  • `.distinct(ResearchProject.id)` is kept but no longer needed now that outer joins are gone — can be removed.
  • PR description's A11y checklist is all unchecked, and "Screenshots" section appears twice.
  • Cypress `cy.wait(1000)` calls — prefer asserting on a changed-state condition rather than a fixed wait.
  • `selectinload` chains are duplicated verbatim between the two query builders — candidate for a shared helper if we want to DRY further.

Strengths

  • EXISTS-subquery fix is correct and addresses a real user-facing bug.
  • Good Cypress coverage across filter combinations.
  • Thorough unit tests on the filter button modal.
  • No injection concerns — all values go through SQLAlchemy parameter binding.

Requesting changes for the two blocking items; the rest are polish.

@rajohnson90 rajohnson90 requested a review from johndeange May 5, 2026 13:12
Copy link
Copy Markdown
Contributor

@johndeange johndeange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review after "respond to pr comments" commit

Both blocking issues from my previous review are resolved:

  • assert response_research.status_code == 200 fixed, negative case added
  • Modal.setAppElement moved to useEffect
  • useTagsList collapsed to a single useMemo (much cleaner)
  • console.warn removed, type defs consolidated

Minor items for follow-up

  1. Pagination reset on filter change — Wei flagged that the pagination useEffect doesn't depend on filters. Please confirm this is handled (here or follow-up).
  2. agreement_search data shape — pipeline works (E2E proves it), but the {id, name}{id, title} transform in AgreementNameComboBox isn't visible in this diff. A brief comment would help the next dev.
  3. projectType naming asymmetryopsAPI.js sends type.id, removeFilter filters by type.title. Works because both exist on each item, but naming is inconsistent across the filter lifecycle.

None of these are blocking. Approving — nice work on the EXISTS-subquery fix and the filter UI. 👍

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.

4 participants