Add is_extended_promotional filter to components and search#131
Add is_extended_promotional filter to components and search#131yuliuyi717-ux wants to merge 1 commit intotscircuit:mainfrom
Conversation
| const allComponents = allRes.data.components as Array<{ | ||
| basic: number | ||
| preferred: number | ||
| is_extended_promotional: number | ||
| }> | ||
| const filteredComponents = filteredRes.data.components as Array<{ | ||
| basic: number | ||
| preferred: number | ||
| is_extended_promotional: number | ||
| }> | ||
|
|
||
| for (const component of allComponents) { | ||
| const expected = component.preferred === 1 && component.basic === 0 ? 1 : 0 | ||
| expect(component.is_extended_promotional).toBe(expected) | ||
| } | ||
|
|
||
| for (const component of filteredComponents) { | ||
| expect(component.is_extended_promotional).toBe(1) | ||
| expect(component.preferred).toBe(1) | ||
| expect(component.basic).toBe(0) | ||
| } |
There was a problem hiding this comment.
Type mismatch: tests expect numeric values (0/1) but API returns booleans. The route transforms all flags with Boolean() (line 81-93 in search.tsx), so is_extended_promotional, basic, and preferred will be true/false in responses, not 0/1. This causes test failures:
// Current (broken):
component.preferred === 1 // false === 1 → false
component.basic === 0 // true === 0 → false
// Should be:
component.preferred === true
component.basic === false
component.is_extended_promotional === true
// And type should be:
type Component = {
basic: boolean
preferred: boolean
is_extended_promotional: boolean
}| const allComponents = allRes.data.components as Array<{ | |
| basic: number | |
| preferred: number | |
| is_extended_promotional: number | |
| }> | |
| const filteredComponents = filteredRes.data.components as Array<{ | |
| basic: number | |
| preferred: number | |
| is_extended_promotional: number | |
| }> | |
| for (const component of allComponents) { | |
| const expected = component.preferred === 1 && component.basic === 0 ? 1 : 0 | |
| expect(component.is_extended_promotional).toBe(expected) | |
| } | |
| for (const component of filteredComponents) { | |
| expect(component.is_extended_promotional).toBe(1) | |
| expect(component.preferred).toBe(1) | |
| expect(component.basic).toBe(0) | |
| } | |
| const allComponents = allRes.data.components as Array<{ | |
| basic: boolean | |
| preferred: boolean | |
| is_extended_promotional: boolean | |
| }> | |
| const filteredComponents = filteredRes.data.components as Array<{ | |
| basic: boolean | |
| preferred: boolean | |
| is_extended_promotional: boolean | |
| }> | |
| for (const component of allComponents) { | |
| const expected = component.preferred === true && component.basic === false | |
| expect(component.is_extended_promotional).toBe(expected) | |
| } | |
| for (const component of filteredComponents) { | |
| expect(component.is_extended_promotional).toBe(true) | |
| expect(component.preferred).toBe(true) | |
| expect(component.basic).toBe(false) | |
| } | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
This comment came from an experimental review—please leave feedback if it was helpful/unhelpful. Learn more about experimental comments here.
| const allComponents = allRes.data.components as Array<{ | ||
| basic: number | ||
| preferred: number | ||
| is_extended_promotional: number | ||
| }> | ||
| const filteredComponents = filteredRes.data.components as Array<{ | ||
| basic: number | ||
| preferred: number | ||
| is_extended_promotional: number | ||
| }> | ||
|
|
||
| for (const component of allComponents) { | ||
| const expected = component.preferred === 1 && component.basic === 0 ? 1 : 0 | ||
| expect(component.is_extended_promotional).toBe(expected) | ||
| } | ||
|
|
||
| for (const component of filteredComponents) { | ||
| expect(component.is_extended_promotional).toBe(1) | ||
| expect(component.preferred).toBe(1) | ||
| expect(component.basic).toBe(0) | ||
| } |
There was a problem hiding this comment.
Type mismatch: tests expect numeric values (0/1) but API returns booleans. The route transforms all flags with Boolean() (line 83-94 in list.tsx), so is_extended_promotional, basic, and preferred will be true/false in responses, not 0/1. This causes test failures:
// Current (broken):
component.preferred === 1 // false === 1 → false
component.basic === 0 // true === 0 → false
// Should be:
component.preferred === true
component.basic === false
component.is_extended_promotional === true
// And type should be:
type Component = {
basic: boolean
preferred: boolean
is_extended_promotional: boolean
}| const allComponents = allRes.data.components as Array<{ | |
| basic: number | |
| preferred: number | |
| is_extended_promotional: number | |
| }> | |
| const filteredComponents = filteredRes.data.components as Array<{ | |
| basic: number | |
| preferred: number | |
| is_extended_promotional: number | |
| }> | |
| for (const component of allComponents) { | |
| const expected = component.preferred === 1 && component.basic === 0 ? 1 : 0 | |
| expect(component.is_extended_promotional).toBe(expected) | |
| } | |
| for (const component of filteredComponents) { | |
| expect(component.is_extended_promotional).toBe(1) | |
| expect(component.preferred).toBe(1) | |
| expect(component.basic).toBe(0) | |
| } | |
| const allComponents = allRes.data.components as Array<{ | |
| basic: boolean | |
| preferred: boolean | |
| is_extended_promotional: boolean | |
| }> | |
| const filteredComponents = filteredRes.data.components as Array<{ | |
| basic: boolean | |
| preferred: boolean | |
| is_extended_promotional: boolean | |
| }> | |
| for (const component of allComponents) { | |
| const expected = component.preferred === true && component.basic === false ? true : false | |
| expect(component.is_extended_promotional).toBe(expected) | |
| } | |
| for (const component of filteredComponents) { | |
| expect(component.is_extended_promotional).toBe(true) | |
| expect(component.preferred).toBe(true) | |
| expect(component.basic).toBe(false) | |
| } | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
This comment came from an experimental review—please leave feedback if it was helpful/unhelpful. Learn more about experimental comments here.
|
Quick follow-up on this PR. I can make any adjustments immediately if there are review notes. If the current approach looks good, a merge when convenient would be appreciated. |
|
Quick follow-up: this PR is still clean and mergeable. If you want any naming/query-shape tweaks before merge, I can update right away. |
e4e3cca to
afa4f6d
Compare
| test("GET /api/search supports is_extended_promotional filter", async () => { | ||
| const { axios } = await getTestServer() | ||
| const allRes = await axios.get("/api/search?full=true&limit=200") | ||
| const filteredRes = await axios.get( | ||
| "/api/search?full=true&limit=200&is_extended_promotional=true", | ||
| ) | ||
|
|
||
| const allComponents = allRes.data.components as Array<{ | ||
| basic: number | ||
| preferred: number | ||
| is_extended_promotional: number | ||
| }> | ||
| const filteredComponents = filteredRes.data.components as Array<{ | ||
| basic: number | ||
| preferred: number | ||
| is_extended_promotional: number | ||
| }> | ||
|
|
||
| for (const component of allComponents) { | ||
| const expected = component.preferred === 1 && component.basic === 0 ? 1 : 0 | ||
| expect(component.is_extended_promotional).toBe(expected) | ||
| } | ||
|
|
||
| for (const component of filteredComponents) { | ||
| expect(component.is_extended_promotional).toBe(1) | ||
| expect(component.preferred).toBe(1) | ||
| expect(component.basic).toBe(0) | ||
| } | ||
| }) |
There was a problem hiding this comment.
This test file violates the rule that a *.test.ts file may have AT MOST one test(...) function. The file already contains multiple test functions, and this modification adds another test function starting at line 112. According to the rule, after the first test, additional tests should be split into multiple, numbered files (e.g., search1.test.ts, search2.test.ts). To fix this, create a new file like search2.test.ts and move this new test function there.
Spotted by Graphite (based on custom rule: Custom rule)
Is this helpful? React 👍 or 👎 to let us know.
|
Gentle follow-up on #131. The filter support is ready for review; happy to revise quickly if you want changes before merge. Thanks in advance. |
|
Clarification to my last note: this PR adds the |
|
Friendly bump on this one—I added the is_extended_promotional filter for components and search in #131. When you have a moment, I’d really appreciate a review. Thanks! |
Implements
is_extended_promotionalsupport for issue #92 using existing source fields (preferred+basic) without adding a new DB migration.Proposed changes
is_extended_promotionalquery param to:/components/list/api/searchis_extended_promotionalaspreferred = 1 AND basic = 0is_extended_promotionalin API response payloadsis_extended_promotional=truein both routes(preferred == 1 && basic == 0)Proof
Local validation run:
bunx tsc --noEmitbunx biome check routes/api/search.tsx routes/components/list.tsx tests/routes/api/search.test.ts tests/routes/components/list.test.tsbun test tests/routes/components/list.test.tsbun test tests/routes/api/search.test.ts -t "supports is_extended_promotional filter"/claim #92