-
Notifications
You must be signed in to change notification settings - Fork 0
Implement filter_by_role API function #49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
65db821
38550cd
6bc18e1
f20928c
0dc2eca
ccc9f4a
ca06416
5266bf9
cd9bf44
f71c6fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to implement |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| import { NextRequest, NextResponse } from "next/server"; | ||
| import { | ||
| getVolunteersByRoles, | ||
| isAllStrings, | ||
| isValidOperator, | ||
| } from "@/lib/api/getVolunteersByRoles"; | ||
|
|
||
| export async function GET(request: NextRequest) { | ||
| const { searchParams } = request.nextUrl; | ||
| const operator = searchParams.get("operator"); | ||
| const roleParams = searchParams.get("roles"); | ||
|
|
||
| if (!operator || !roleParams) { | ||
| return NextResponse.json({ | ||
| status: 400, | ||
| error: "Missing operator or role filter values", | ||
| }); | ||
| } | ||
|
|
||
| const roles = roleParams.split(","); | ||
|
|
||
| if (!isValidOperator(operator)) { | ||
| return { status: 400, error: "Operator is not AND or OR" }; | ||
| } | ||
|
|
||
| if (!isAllStrings(roles)) { | ||
| return { | ||
| status: 400, | ||
| error: "Roles to filter by are not all strings", | ||
| }; | ||
LeandroHamaguchi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| const response = await getVolunteersByRoles(operator as "OR" | "AND", roles); | ||
|
|
||
| return NextResponse.json(response, { | ||
| status: response.status, | ||
| }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,108 @@ | ||||||||||||||||||||||
| import { createClient } from "../client/supabase/server"; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| type Volunteer = { | ||||||||||||||||||||||
| id: number; | ||||||||||||||||||||||
| created_at: string; | ||||||||||||||||||||||
| updated_at: string; | ||||||||||||||||||||||
| email: string | null; | ||||||||||||||||||||||
| phone: string | null; | ||||||||||||||||||||||
| name_org: string; | ||||||||||||||||||||||
| notes: string | null; | ||||||||||||||||||||||
| opt_in_communication: boolean | null; | ||||||||||||||||||||||
| position: string | null; | ||||||||||||||||||||||
| pronouns: string | null; | ||||||||||||||||||||||
| pseudonym: string | null; | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| type Operator = "OR" | "AND"; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| export function isAllStrings(arr: unknown[]): arr is string[] { | ||||||||||||||||||||||
| return arr.every((item) => typeof item === "string"); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| export function isValidOperator(operator: string) { | ||||||||||||||||||||||
| return ( | ||||||||||||||||||||||
| typeof operator === "string" && | ||||||||||||||||||||||
| ["OR", "AND"].includes(operator.toUpperCase()) | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| export async function getVolunteersByRoles( | ||||||||||||||||||||||
| operator: Operator, | ||||||||||||||||||||||
| filters: string[] | ||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should also check for the case that
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the case that filters is empty, should it return an error or an empty array? Right now, when an empty array is passed, Supabase returns an empty list of rows since there are no filters to match rows by
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Returning an empty array should be good |
||||||||||||||||||||||
| ) { | ||||||||||||||||||||||
| if (!isValidOperator(operator)) { | ||||||||||||||||||||||
| return { status: 400, error: "Operator is not AND or OR" }; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if (!isAllStrings(filters)) { | ||||||||||||||||||||||
| return { | ||||||||||||||||||||||
| status: 400, | ||||||||||||||||||||||
| error: "Roles to filter by are not all strings", | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
Comment on lines
+34
to
+44
|
||||||||||||||||||||||
| if (!isValidOperator(operator)) { | |
| return { status: 400, error: "Operator is not AND or OR" }; | |
| } | |
| if (!isAllStrings(filters)) { | |
| return { | |
| status: 400, | |
| error: "Roles to filter by are not all strings", | |
| }; | |
| } |
Copilot
AI
Nov 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using loose equality (==) instead of strict equality (===). JavaScript best practices recommend using === to avoid type coercion issues.
| if (operator.toUpperCase() == "OR") { | |
| if (operator.toUpperCase() === "OR") { |
Copilot
AI
Nov 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AND operator logic is incorrect. This checks if the number of matched roles equals the filter count, but doesn't verify that ALL requested roles are present. For example, if filters are ["Role 1", "Role 2"] and a volunteer has ["Role 1", "Role 1"] (counted once in the Set), they would incorrectly match since size is 1 but filters.length is 2 and they don't match. More critically, if a volunteer has ["Role 1", "Role 2", "Role 3"] and you filter for ["Role 1", "Role 2"], they would match (size 3 ≠ filters.length 2) incorrectly.
The correct logic should verify that all filter roles are present in the volunteer's roles:
if (filters.every(filter => volunteer.roleNames.has(filter))) {| if (volunteer.roleNames.size == filters.length) { | |
| if (filters.every(filter => volunteer.roleNames.has(filter))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Copilot on this, you are only checking the count, not the actual content itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, I have the select from the db set up so that I join Roles to Volunteers, and I filter for rows such that the role name is in the filters.
More critically, if a volunteer has ["Role 1", "Role 2", "Role 3"] and you filter for ["Role 1", "Role 2"], they would match (size 3 ≠ filters.length 2) incorrectly.
I don't think a case like this would occur since the rows I would get from the db would consist of rows where the role name is in the filters (ie. either Role 1 or Role 2).
For example, if filters are ["Role 1", "Role 2"] and a volunteer has ["Role 1", "Role 1"] (counted once in the Set), they would incorrectly match since size is 1 but filters.length is 2 and they don't match
And for this case, if the operator was OR, then the volunteer would be matched. If the operator was AND, the set of role names would only contain Role 1 and the length of the set would be 1, so that volunteer would not be matched.
I might be wrong about this though and I can add logic to check for the contents of the roles. Also, if the logic for the filtering is too confusing, I can edit the code to make it clearer and add comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right. Let's create tests for these cases to be sure
1michhu1 marked this conversation as resolved.
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,3 +1,8 @@ | ||||||
| // Add any exports from your API files here | ||||||
|
|
||||||
| export { getExample } from "./getExample"; | ||||||
| export { | ||||||
| getVolunteersByRoles, | ||||||
| isAllStrings, | ||||||
| isValidOperator, | ||||||
|
Comment on lines
+6
to
+7
|
||||||
| isAllStrings, | |
| isValidOperator, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with Copilot here. We don't need to export isAllStrings and isValidOperator here
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good job doing the tests ahead of time! We will be running tests against an instance of the official database, so the test syntax will likely need to change a bit. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| // Example test for getExample function | ||
| // This test is not meaningful as is, but serves as a template | ||
| // You should modify it to fit your actual implementation and testing needs | ||
|
|
||
| import { describe, it, expect, vi, beforeEach } from "vitest"; | ||
| import { getVolunteersByRoles } from "@/lib/api/getVolunteersByRoles"; | ||
| import { createClient } from "@/lib/client/supabase/server"; | ||
|
|
||
| const Roles1 = [ | ||
| { | ||
| Roles: { name: "Role 1" }, | ||
| Volunteers: { | ||
| id: 1, | ||
| }, | ||
| }, | ||
| { | ||
| Roles: { name: "Role 1" }, | ||
| Volunteers: { | ||
| id: 3, | ||
| }, | ||
| }, | ||
| ]; | ||
|
|
||
| const Roles2 = [ | ||
| { | ||
| Roles: { name: "Role 2" }, | ||
| Volunteers: { | ||
| id: 2, | ||
| }, | ||
| }, | ||
| { | ||
| Roles: { name: "Role 2" }, | ||
| Volunteers: { | ||
| id: 3, | ||
| }, | ||
| }, | ||
| ]; | ||
|
|
||
| // Mock the Supabase client | ||
| vi.mock("@/lib/client/supabase/server", () => ({ | ||
| createClient: vi.fn(), | ||
| })); | ||
|
|
||
| describe("getVolunteersByRoles", () => { | ||
| const mockIn = vi.fn(); | ||
| const mockSelect = vi.fn(() => ({ in: mockIn })); | ||
| const mockFrom = vi.fn(() => ({ select: mockSelect })); | ||
| const mockClient = { from: mockFrom }; | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| // @ts-expect-error - Partial mock of SupabaseClient for testing | ||
| vi.mocked(createClient).mockResolvedValue(mockClient); | ||
| }); | ||
|
|
||
| it("returns error response for an invalid operator", async () => { | ||
| const result = await getVolunteersByRoles("INVALID", ["Role 1"]); | ||
| expect(result.status).toBe(400); | ||
| expect(result).toEqual({ status: 400, error: "Operator is not AND or OR" }); | ||
| }); | ||
|
|
||
| it("returns error response if the filters array is malformed", async () => { | ||
| const result = await getVolunteersByRoles("OR", [1]); | ||
| expect(result.status).toBe(400); | ||
| expect(result).toEqual({ | ||
| status: 400, | ||
| error: "Roles to filter by are not all strings", | ||
| }); | ||
| }); | ||
|
|
||
| it("returns all volunteers with Role 1 or Role 2", async () => { | ||
| mockIn.mockResolvedValueOnce({ data: [...Roles1, ...Roles2], error: null }); | ||
|
|
||
| const result = await getVolunteersByRoles("OR", ["Role 1", "Role 2"]); | ||
| expect(result.status).toBe(200); | ||
| expect(result).toHaveProperty("data"); | ||
|
|
||
| const ids = result.data?.map((volunteer) => volunteer.id).sort(); | ||
| expect(ids).toEqual([1, 2, 3]); | ||
| }); | ||
|
|
||
| it("returns all volunteers with Role 1", async () => { | ||
| mockIn.mockResolvedValueOnce({ data: Roles1, error: null }); | ||
|
|
||
| const result = await getVolunteersByRoles("OR", ["Role 1"]); | ||
| expect(result.status).toBe(200); | ||
| expect(result).toHaveProperty("data"); | ||
|
|
||
| const ids = result.data?.map((volunteer) => volunteer.id).sort(); | ||
| expect(ids).toEqual([1, 3]); | ||
| }); | ||
|
|
||
| it("returns all volunteers with Role 1 AND Role2", async () => { | ||
| mockIn.mockResolvedValueOnce({ data: [...Roles1, ...Roles2], error: null }); | ||
|
|
||
| const result = await getVolunteersByRoles("AND", ["Role 1", "Role 2"]); | ||
| expect(result.status).toBe(200); | ||
| expect(result).toHaveProperty("data"); | ||
|
|
||
| const ids = result.data?.map((volunteer) => volunteer.id).sort(); | ||
| expect(ids).toEqual([3]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding
"type": "module"to package.json was not mentioned in the PR description and doesn't appear to be related to the filter_by_role feature. This change affects how Node.js interprets JavaScript files throughout the entire project. Please clarify why this change is necessary for this PR, or if it should be removed or submitted as a separate PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Copilot here. Is this necessary?