From 98d31e6d619d70e6d02e406e4f74e6606c39ae22 Mon Sep 17 00:00:00 2001 From: ivannissimrch Date: Wed, 19 Aug 2026 21:06:34 -0400 Subject: [PATCH 1/2] fix: keep every opportunity deal filter when several are combined --- .../utils/data/get-opportunity-where.ts | 50 +++++++++--------- .../utils/data/get-opportunity-where.test.ts | 52 +++++++++++++++++++ 2 files changed, 77 insertions(+), 25 deletions(-) create mode 100644 src/test/server/utils/data/get-opportunity-where.test.ts diff --git a/src/server/utils/data/get-opportunity-where.ts b/src/server/utils/data/get-opportunity-where.ts index 27ab6ed8..ff5fb852 100644 --- a/src/server/utils/data/get-opportunity-where.ts +++ b/src/server/utils/data/get-opportunity-where.ts @@ -24,36 +24,36 @@ export function getOpportunityWhere( title: ILike(`%${filter.search}%`), } : {}), - ...(filter?.language + // language, district, activity and skill all constrain the same `deal` relation, so they must share a + // single `deal` key. + // if we use two spreads with the same key, the second replaces the first, and the earlier + // constraint is silently dropped. + ...(filter?.language || + filter?.district || + filter?.activity || + filter?.skill ? { deal: { - dealLanguage: { - language: { - id: normalizeStringArrayInput(filter.language), + ...(filter?.language && { + dealLanguage: { + language: { id: normalizeStringArrayInput(filter.language) }, }, - }, - }, - } - : {}), - ...(filter?.district - ? { - deal: { - dealDistrict: { - district: { - id: normalizeStringArrayInput(filter.district), + }), + ...(filter?.district && { + dealDistrict: { + district: { id: normalizeStringArrayInput(filter.district) }, }, - }, - }, - } - : {}), - ...(filter?.activity - ? { - deal: { - dealActivity: { - activity: { - id: normalizeStringArrayInput(filter.activity), + }), + ...(filter?.activity && { + dealActivity: { + activity: { id: normalizeStringArrayInput(filter.activity) }, + }, + }), + ...(filter?.skill && { + dealSkill: { + skill: { id: normalizeStringArrayInput(filter.skill) }, }, - }, + }), }, } : {}), diff --git a/src/test/server/utils/data/get-opportunity-where.test.ts b/src/test/server/utils/data/get-opportunity-where.test.ts new file mode 100644 index 00000000..c7a4d562 --- /dev/null +++ b/src/test/server/utils/data/get-opportunity-where.test.ts @@ -0,0 +1,52 @@ +import { In } from "typeorm"; +import { describe, expect, it } from "vitest"; +import { QuerystringOpportunityFiltering } from "../../../../server/types"; +import { getOpportunityWhere } from "../../../../server/utils"; + +describe("getOpportunityWhere", () => { + it("returns an empty object when no filters are provided", () => { + expect(getOpportunityWhere(undefined)).toEqual({}); + }); + + it("keeps every deal constraint when several are combined", () => { + const where = getOpportunityWhere({ + type: "", + status: "", + language: "1", + district: "2", + activity: "3", + skill: "4", + }); + + expect(where.deal).toEqual({ + dealLanguage: { language: { id: "1" } }, + dealDistrict: { district: { id: "2" } }, + dealActivity: { activity: { id: "3" } }, + dealSkill: { skill: { id: "4" } }, + }); + }); + + // Multiple selections arrive as an array at runtime (?language=3&language=4) + // and become In([...]), which TypeORM ORs. The querystring type declares + // every filter as `string`, hence the cast, see the + // `// TODO: what about arrays?` above QuerystringOpportunityFiltering. + it("ORs multiple values within a single filter", () => { + const where = getOpportunityWhere({ + type: "", + status: "", + language: ["3", "4"], + } as unknown as QuerystringOpportunityFiltering["filter"]); + + expect(where.deal).toEqual({ + dealLanguage: { language: { id: In(["3", "4"]) } }, + }); + }); + + it("applies only the language constraint when nothing else is selected", () => { + const where = getOpportunityWhere({ type: "", status: "", language: "3" }); + + expect(where.deal).toEqual({ + dealLanguage: { language: { id: "3" } }, + }); + }); +}); From ee963e60523d8b79dc5f0300f455a1bc6908bece Mon Sep 17 00:00:00 2001 From: arturasmckwcz Date: Sat, 22 Aug 2026 18:08:15 +0000 Subject: [PATCH 2/2] refactor: extract getDealWhere() accumulator; add be#888 repro regression test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../utils/data/get-opportunity-where.ts | 68 ++++++++++--------- .../utils/data/get-opportunity-where.test.ts | 27 ++++++++ 2 files changed, 62 insertions(+), 33 deletions(-) diff --git a/src/server/utils/data/get-opportunity-where.ts b/src/server/utils/data/get-opportunity-where.ts index 0d0abb4b..ebed45de 100644 --- a/src/server/utils/data/get-opportunity-where.ts +++ b/src/server/utils/data/get-opportunity-where.ts @@ -101,12 +101,46 @@ function getTypeWhere( : {}; } +// language, district, activity and skill all constrain the same `deal` +// relation, so they must accumulate onto one shared `deal` key rather than +// each writing their own top-level spread — two spreads with the same key +// have the second replace the first, silently dropping the earlier +// constraint. Mirrors get-volunteer-where.ts's identical dealFilter. +function getDealWhere( + filter: QuerystringOpportunityFiltering["filter"], +): Record { + const dealFilter: Record = {}; + if (filter?.language) { + dealFilter.dealLanguage = { + language: { id: normalizeStringArrayInput(filter.language) }, + }; + } + if (filter?.district) { + dealFilter.dealDistrict = { + district: { id: normalizeStringArrayInput(filter.district) }, + }; + } + if (filter?.activity) { + dealFilter.dealActivity = { + activity: { id: normalizeStringArrayInput(filter.activity) }, + }; + } + if (filter?.skill) { + dealFilter.dealSkill = { + skill: { id: normalizeStringArrayInput(filter.skill) }, + }; + } + return dealFilter; +} + // SECURITY (#666): filters run on unmasked DB columns, so a non-privileged // caller can infer PII masked in the response by probing which rows match. export function getOpportunityWhere( filter: QuerystringOpportunityFiltering["filter"], appointment?: OpportunityAppointmentFilter, ): FindOptionsWhere { + const dealFilter = getDealWhere(filter); + return { ...getTypeWhere(filter, appointment?.excludeAccompanying), ...getAppointmentDateWhere(appointment), @@ -120,38 +154,6 @@ export function getOpportunityWhere( title: ILike(`%${filter.search}%`), } : {}), - // language, district, activity and skill all constrain the same `deal` relation, so they must share a - // single `deal` key. - // if we use two spreads with the same key, the second replaces the first, and the earlier - // constraint is silently dropped. - ...(filter?.language || - filter?.district || - filter?.activity || - filter?.skill - ? { - deal: { - ...(filter?.language && { - dealLanguage: { - language: { id: normalizeStringArrayInput(filter.language) }, - }, - }), - ...(filter?.district && { - dealDistrict: { - district: { id: normalizeStringArrayInput(filter.district) }, - }, - }), - ...(filter?.activity && { - dealActivity: { - activity: { id: normalizeStringArrayInput(filter.activity) }, - }, - }), - ...(filter?.skill && { - dealSkill: { - skill: { id: normalizeStringArrayInput(filter.skill) }, - }, - }), - }, - } - : {}), + ...(Object.keys(dealFilter).length ? { deal: dealFilter } : {}), } as FindOptionsWhere; } diff --git a/src/test/server/utils/data/get-opportunity-where.test.ts b/src/test/server/utils/data/get-opportunity-where.test.ts index 9ffd870b..dff06dd8 100644 --- a/src/test/server/utils/data/get-opportunity-where.test.ts +++ b/src/test/server/utils/data/get-opportunity-where.test.ts @@ -183,4 +183,31 @@ describe("getOpportunityWhere", () => { dealLanguage: { language: { id: "3" } }, }); }); + + // be#888's own repro: activity alone matched 1 row; activity plus a + // non-matching language still matched that same 1, because the language + // spread silently overwrote the activity spread on the shared `deal` key. + // Asserting both constraints survive together is the unit-level proof that + // no longer happens. + it("keeps both activity and language when combined, matching be#888's repro", () => { + const activityOnly = getOpportunityWhere({ + type: "", + status: "", + activity: "3", + }); + expect(activityOnly.deal).toEqual({ + dealActivity: { activity: { id: "3" } }, + }); + + const activityPlusLanguage = getOpportunityWhere({ + type: "", + status: "", + activity: "3", + language: "9", + }); + expect(activityPlusLanguage.deal).toEqual({ + dealActivity: { activity: { id: "3" } }, + dealLanguage: { language: { id: "9" } }, + }); + }); });