Skip to content

feat: filter opportunities by skill - #892

Merged
arturasmckwcz merged 3 commits into
need4deed-org:developfrom
ivannissimrch:ivannissimrch/fix-opportunity-filters-overwrite
Aug 22, 2026
Merged

feat: filter opportunities by skill#892
arturasmckwcz merged 3 commits into
need4deed-org:developfrom
ivannissimrch:ivannissimrch/fix-opportunity-filters-overwrite

Conversation

@ivannissimrch

@ivannissimrch ivannissimrch commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Description

  • filter[skill] was already accepted by the querystring schema and the handler type, but getOpportunityWhere never read it, so the param was silently ignored and the filter did nothing
  • Adds the dealSkill branch so skill actually constrains the query. This is the BE half of fe#905
  • Adding it surfaced an existing bug: language, district, and activity each wrote the same deal key from separate spreads, so the object spread replaced the previous constraint and only the last one reached the query. Combining an activity with a non-matching language returned the activity count instead of 0. Measured locally:
    activity alone 1, activity plus non-matching language still 1
  • Fixed by giving all four constraints one shared deal object as optional properties, so they accumulate instead of overwriting. Skill drops in as one more property rather than a fourth competing spread
  • Unblocks fe 936, which needs a skill branch added to this same function. It now slots in as one more property instead of a fourth competing spread.

Related Issues

Closes #888

Checklist

  • WITHIN THE SCOPE OF AN ISSUE; No unnecessary files included
  • Tests added/updated
  • Documentation updated
  • CI passes

@ivannissimrch ivannissimrch changed the title fix: keep every opportunity deal filter when several are combined feat: filter opportunities by skill Aug 20, 2026
@nadavosa

Copy link
Copy Markdown
Collaborator

Reviewed. The fix itself is solid and worth landing:

  • Real bug, correctly diagnosed: language/district/activity each spread onto the same deal key, so combining filters silently dropped all but the last one (verified the repro described in the PR body against the pre-fix code). Merging them into one shared deal object so they accumulate as properties instead of competing spreads is the right fix.
  • Test coverage is good: covers the combined-filters regression case, the In([...]) OR-multiple-values behavior, and the single-filter-still-works case.
  • dealSkill slots in cleanly as one more property on the same object, matching the existing dealLanguage/dealDistrict/dealActivity shape — consistent with the rest of the file.

On the conflict: this isn't a superficial one to auto-resolve. develop has moved since this branched — get-opportunity-where.ts's getOpportunityWhere picked up a second appointment parameter and had its type/date-range handling extracted into getTypeWhere/getAppointmentDateWhere helpers (#893/#894), and get-opportunity-where.test.ts was newly created on develop by that same PR with entirely different test cases (type/status/search/appointment-date/excludeAccompanying) — so both branches added a new file at the same path with unrelated content, not overlapping edits to an existing one.

Resolving this needs someone to:

  1. Keep develop's getTypeWhere/getAppointmentDateWhere extraction and the appointment param as-is.
  2. Re-apply this PR's fix — the shared deal object with dealLanguage/dealDistrict/dealActivity/dealSkill as properties — inside that restructured function.
  3. Merge the two test files rather than picking one — combine this PR's 3 new cases with develop's existing appointment-date/excludeAccompanying suite in the same describe block.

@ivannissimrch — since this is a substantive merge (not just adjacent lines), can you take a pass at resolving it against current develop?

…annissimrch/fix-opportunity-filters-overwrite
@ivannissimrch

Copy link
Copy Markdown
Contributor Author

I resolved against the current develop, kept getTypeWhere/getAppointmentDateWhere and the appointment param untouched, re-applied the shared deal object inside the new shape, and merged both test files into one describe block

…sion test

Code review follow-up on this PR:
- The four deal-filter spreads are now built via a getDealWhere()
  accumulator (mirroring get-volunteer-where.ts's identical dealFilter
  pattern) instead of inline nested spreads gated by a redundant OR
  condition. Behaviorally identical (same output shape for every existing
  test), but structurally immune to the exact key-collision bug this PR
  fixes — a 5th filter can't accidentally reintroduce it by copying the old
  shape.
- Added a test that encodes the issue's own literal repro (activity alone,
  then activity + a non-matching language) as a direct regression case,
  on top of the existing generic "all four combined" coverage.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@need4deed need4deed left a comment

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.

Reviewed independently (three finder angles + a TypeORM-source-level check on a candidate finding). The core fix is correct and well-tested — no correctness bugs in the diff. Pushed two small follow-ups on top:

  • Extracted the four deal-filter branches into a getDealWhere() accumulator, mirroring get-volunteer-where.ts's existing identical pattern — same output shape, but structurally immune to the exact key-collision bug this PR fixes (a future 5th filter can't accidentally reintroduce it).
  • Added a test encoding the issue's own literal repro (activity alone, then activity + a non-matching language) as a direct regression case.

One pre-existing, unrelated finding for the record (not from this PR, not blocking): opportunityListQuerySchema's additionalProperties: false in querystring.ts is mis-nested and doesn't actually restrict anything, which is why skill/activity validate at all today. Worth its own issue if not already tracked.

Thanks for the fix — this was a real, cleanly-diagnosed bug.

@arturasmckwcz
arturasmckwcz merged commit 35d0dde into need4deed-org:develop Aug 22, 2026
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.

Add skill filter and fix Opportunity filters overwrite each other

4 participants