add skills filter to the opportunity list - #936
Conversation
…filter-opportunities-by-skills
nadavosa
left a comment
There was a problem hiding this comment.
Reviewed the diff and checked it out locally.
FE code looks correct. The skill filter mirrors the activity filter pattern exactly across types.ts, constants.ts, helpers.ts (both the Filters one and the parent one), FiltersContent.tsx, and Opportunities.tsx. I grepped every EntityTableName.ACTIVITY usage in src/components/Dashboard/Opportunities and confirmed a matching SKILL counterpart was added at each spot, nothing missed.
Confirmed EntityTableName.SKILL already exists in the published need4deed-sdk (dist/types/core.d.ts), so the "no SDK change needed" claim in the description holds. tsc --noEmit and lint both pass clean locally too.
One thing worth deciding before merge: since this ships without the BE getOpportunityWhere skill branch, a user can select a skill filter and it will silently do nothing to the results, no error, just no effect. Is that an acceptable gap to ship now, or should this wait and land together with the backend change so the filter isn't visibly inert in the meantime?
Separate from this PR: the pre-push test failures you hit (day-off-by-one date assertions assuming a European timezone, plus the flaky swagger.test.ts timeout) sound like a real problem blocking non-UTC-negative contributors from opening BE PRs at all. Might be worth its own BE issue so it doesn't keep biting people.
No test added here, but that matches the existing sibling filters (activity/language/district are untested too), so not blocking on that basis.
nadavosa
left a comment
There was a problem hiding this comment.
Update: checked the BE repo, this is already handled. need4deed-org/be#892 merged to develop on 2026-08-22, adds the dealSkill branch to getOpportunityWhere and also fixes the same-key-overwrite bug you flagged (language/district/activity/skill now accumulate into one shared deal object instead of clobbering each other, with tests added).
So my earlier concern about shipping a silently inert filter doesn't apply, the BE half is already merged. This should just work end to end once develop picks up both. No blocker from me.
arturasmckwcz
left a comment
There was a problem hiding this comment.
Automated review pass on this PR. One thing worth double-checking before merge: the PR description says the backend skill where-clause hasn't landed and that the BE PR couldn't be opened yet ("nothing is filtered until that lands"). But be's develop at commit ee963e60 (2026-08-22, same author) already added the skill branch to getOpportunityWhere, and this branch's merge-base with origin/develop includes that commit. So the filter is likely already fully live against current develop, not inert — worth confirming and testing end-to-end rather than merging on the assumption it's a no-op.
Two smaller inline notes below.
| Object.entries(filter.skill).forEach(([key, value]) => { | ||
| if (value === true) { | ||
| const paramValue = | ||
| (options?.serializeToIDs && options.apiFilterOptions?.skill?.find((d) => d.title === key)?.id) || key; |
There was a problem hiding this comment.
find(...)?.id || key treats a skill id === 0 as falsy and falls back to sending the title string instead of 0, silently breaking that filter for such an id. Same latent bug pattern already present for activity/district/language, now duplicated here for skill.
There was a problem hiding this comment.
this is just for logic purity, id never comes as 0 in anyway
| (key) => key, | ||
| ); | ||
|
|
||
| const skillFilters = generateNestedFilterControlItems( |
There was a problem hiding this comment.
Adding this filter required copy-pasting the same wiring across ~6 files (types, constants, this item-builder, the flat-array builder, option loading, serialize/deserialize) instead of a shared data-driven config for district/language/activity/skill. Likely how the falsy-0 bug above slipped in — the next filter (or a fix) has to be replicated by hand across all 6 sites with no compiler check for a missed spot.
There was a problem hiding this comment.
Understood, I'll work on it. Thank you.
|
Tested end to end and it works, ticking a skill narrows the list. Description updated, be#892 landed Aug 22, so the inert note no longer applies. |
| } | ||
| }); | ||
|
|
||
| params.delete(EntityTableName.SKILL); |
There was a problem hiding this comment.
NIT: serializeOpportunityFilters now has a 4th near-verbatim copy-pasted block (district/language/activity/skill) instead of a shared helper parameterized by filter key. Worth generalizing so a future filter/fix doesn't need manual replication across all blocks.
| newFilter.activity[l] = true; | ||
| }); | ||
|
|
||
| const querySkills = searchParams.getAll(EntityTableName.SKILL); |
There was a problem hiding this comment.
NIT: deserializeOpportunityFilters similarly gains a 4th near-verbatim copy-pasted block (searchParams.getAll + forEach + set true) instead of a shared helper. Same generalization opportunity as the serialize side.
|
I did the serialize and deserialize half here in fccd07f. Both loop a shared key list now. |
|
|
Backlogged as #970 |
Update 2026-08-26
be#892 landed on Aug 22 and added the
skillbranch togetOpportunityWhere, so the note below about the filter being inert no longer applies. Tested end to end: ticking a skill narrows the list and the param round-trips through the URL.Description
Adds a skills filter to the opportunity list, following the existing activity filter pattern.
This needs a backend change first.
getOpportunityWherehas noskillbranch. So the filter renders and reaches the URL. Still, nothing is filtered until that lands.filter.skillis already declared inQuerystringOpportunityFilteringand the list route already eager-loadsdeal.dealSkill.skill, so only the where-clause is missing. No SDK change needed.I have the backend side working locally with a unit test, verified end-to-end against my own backend, but I could not open the BE PR. The pre-push hook runs the full test suite, and it does not pass on my machine:
getDateObjexpects day 11 and I get 10, and the onetimer test expects 2026-08-01, and I get 2026-07-31. Both are one day off because I am in UTC-5, and those tests assume a European time zone.swagger.test.tsalso times out at 5s intermittently. None of them touch my change.One more thing worth knowing. While adding the skill branch, I found a separate bug in the same function:
language,districtandactivityall write the samedealkey, and object spread replaces rather than merges on a repeated key, so combining two of them silently drops one. Measured: one activity alone returns 1, and that same activity plus a language that does not apply still returns 1 instead of 0. It is ondevelopnow and unrelated to this feature.And when you test this: the skills dropdown depends on
/optionreturning skills. If an environment has nooptionrows withitem_type='skill', the accordion renders empty. My local database had none until I seeded them.Related Issues
Closes #905
Changes
skilladded toOpportunityCardsFilteranddefaultOpportunityCardsFilterskillFiltersincreateOpportunityFilterItems, plus the selected-filters flat arrayAccordionFilterinFiltersContent.tsxskillstranslation key in en and deScreenshots / Demos
Checklist