Implement filter_by_role API function #49

1michhu1 · 2025-11-23T01:20:49Z

Implemented the function handler for filtering volunteers by role(s).

Notes:
-Currently, the response object contains the column values in the Volunteers table for each filtered volunteers, as well as an additional field named filtered_rows representing the roles that that volunteer was filtered on. So for instance, if volunteer1 has roles 1 and 2 and I filter for volunteers with role 1, then filtered_rows contains only role 1 for volunteer 1. This can be changed if necessary
-Due to the limited filtering options with the Supabase JS client, I currently filter out non-matching rows in the function itself. I was thinking if this isn't acceptable for performance, I could try paginating it instead with range or adding a Postgres function?
-Added a non-exhaustive set of simple unit tests

Test Cases:

Volunteers Table:

VolunteerRoles Table

Roles Table

Test 1: Filter for Role 1 with operator

Test 2: Filter for Role 1 and Role 2 with OR operator

Test 3: Filter for Role 1 and Role 2 with AND operator

…kend/filter-by-role-api

… role

…teers by role" This reverts commit 6bc18e1.

vercel · 2025-11-23T01:20:54Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
trcc	Error			Nov 23, 2025 8:50pm

Copilot

Pull request overview

This PR implements a new API endpoint for filtering volunteers by role(s) with support for AND/OR operators. The implementation includes the API function, route handler, and basic unit tests. However, the PR contains several issues including a critical bug in the AND operator logic, code duplication across two nearly identical files, and unrelated package changes that need clarification.

Key Changes:

New API function getVolunteersByRoles that queries the VolunteerRoles junction table to filter volunteers by role names
Route handler at /volunteers/filter_by_role supporting query parameters for operator (AND/OR) and roles
Unit tests covering basic scenarios including validation errors and OR/AND filtering

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
`src/lib/api/getVolunteersByRoles.ts`	Main API function implementing role-based volunteer filtering with validation and map-based aggregation
`src/lib/api/getRolesByFilter.ts`	Duplicate implementation with minor variations (should be removed)
`src/app/volunteers/filter_by_role/route.ts`	GET endpoint handler for the filter API with request parameter parsing
`tests/lib/api/getVolunteersByRoles.test.ts`	Unit tests for validation and basic OR/AND filtering scenarios
`src/lib/api/index.ts`	Export additions for new API function and helper functions
`package.json`	Added "type": "module" field (unrelated to feature)
`package-lock.json`	Supabase CLI upgrade and license field cleanup (unrelated to feature)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-23T20:54:36Z

src/lib/api/getVolunteersByRoles.ts

+    }
+  } else {
+    for (const volunteer of volunteerRoleMap.values()) {
+      if (volunteer.roleNames.size == filters.length) {


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))) {

Suggested change

if (volunteer.roleNames.size == filters.length) {

if (filters.every(filter => volunteer.roleNames.has(filter))) {

I agree with Copilot on this, you are only checking the count, not the actual content itself.

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?

I think you're right. Let's create tests for these cases to be sure

src/lib/api/getVolunteersByRoles.ts

Copilot · 2025-11-23T20:54:36Z

src/lib/api/getVolunteersByRoles.ts

+  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",
+    };
+  }
+


This validation is redundant. The route handler already validates the operator and filters before calling getVolunteersByRoles. Since the function signature requires operator: Operator (which is "OR" | "AND"), and filters: string[], these checks are unnecessary in the API function. Consider removing them or removing the validation from the route handler to avoid duplication.

Suggested change

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 · 2025-11-23T20:54:37Z

package.json

  "name": "trcc",
  "version": "0.1.0",
  "private": true,
+  "type": "module",


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.

Suggested change

"type": "module",

I agree with Copilot here. Is this necessary?

Copilot · 2025-11-23T20:54:37Z

src/lib/api/getVolunteersByRoles.ts

+  }
+
+  const filteredVolunteers = [];
+  if (operator.toUpperCase() == "OR") {


Using loose equality (==) instead of strict equality (===). JavaScript best practices recommend using === to avoid type coercion issues.

Suggested change

if (operator.toUpperCase() == "OR") {

if (operator.toUpperCase() === "OR") {

src/app/volunteers/filter_by_role/route.ts

Copilot · 2025-11-23T20:54:37Z

src/lib/api/index.ts

+  isAllStrings,
+  isValidOperator,


Exporting internal validation helper functions (isAllStrings, isValidOperator) in the public API index is unusual. These are implementation details that consumers of the API module shouldn't need. Consider keeping them as internal utilities or moving them to a separate utilities file if they need to be shared.

Suggested change

isAllStrings,

isValidOperator,

Agree with Copilot here. We don't need to export isAllStrings and isValidOperator here

notjackl3 · 2025-11-27T00:33:54Z

src/app/volunteers/filter_by_role/route.ts

No need to implement route.ts file.

notjackl3 · 2025-11-27T00:38:35Z

src/lib/api/getVolunteersByRoles.ts

+  position: string | null;
+  pronouns: string | null;
+  pseudonym: string | null;
+};


I think the Volunteer type has already been defined in the types.ts, so you can just import it instead of creating a new type:

type Volunteer = Database["public"]["Tables"]["Volunteers"]["Row"];

notjackl3 · 2025-11-27T00:40:13Z

src/lib/api/getVolunteersByRoles.ts

+
+export async function getVolunteersByRoles(
+  operator: Operator,
+  filters: string[]


You should also check for the case that filters is an empty array.

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

Returning an empty array should be good

LeandroHamaguchi · 2025-12-02T03:03:14Z

package.json

  "name": "trcc",
  "version": "0.1.0",
  "private": true,
+  "type": "module",


I agree with Copilot here. Is this necessary?

LeandroHamaguchi · 2025-12-02T03:08:48Z

src/lib/api/getVolunteersByRoles.ts

+
+export async function getVolunteersByRoles(
+  operator: Operator,
+  filters: string[]


Returning an empty array should be good

LeandroHamaguchi · 2025-12-02T03:20:06Z

src/lib/api/getVolunteersByRoles.ts

+    }
+  } else {
+    for (const volunteer of volunteerRoleMap.values()) {
+      if (volunteer.roleNames.size == filters.length) {


I think you're right. Let's create tests for these cases to be sure

LeandroHamaguchi · 2025-12-02T03:22:27Z

src/lib/api/index.ts

+  isAllStrings,
+  isValidOperator,


Agree with Copilot here. We don't need to export isAllStrings and isValidOperator here

LeandroHamaguchi · 2025-12-02T03:24:28Z

tests/lib/api/getVolunteersByRoles.test.ts

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.

1michhu1 added 9 commits November 22, 2025 14:02

created filter by role endpoint handler

65db821

Merge branch 'main' of https://github.com/uoftblueprint/trcc into bac…

38550cd

…kend/filter-by-role-api

made file name and function more specific for filtering volunteers by…

6bc18e1

… role

Revert "made file name and function more specific for filtering volun…

f20928c

…teers by role" This reverts commit 6bc18e1.

renamed filter file to something more specific

0dc2eca

added type: module so that unit tests can run w/o error

ccc9f4a

creating test file

ca06416

moved filter exports to correct index file

5266bf9

linted and formatted

cd9bf44

1michhu1 linked an issue Nov 23, 2025 that may be closed by this pull request

BE 1.5 - Implement filter_by_role API function #28

Open

1michhu1 added the backend this ticket is part of the backend label Nov 23, 2025

1michhu1 self-assigned this Nov 23, 2025

1zhaohel requested a review from Copilot November 23, 2025 20:49

deleted duplicate file

f71c6fc

Copilot started reviewing on behalf of 1zhaohel November 23, 2025 20:50 View session

vercel bot had a problem deploying to Preview November 23, 2025 20:50 Failure

Copilot finished reviewing on behalf of 1zhaohel November 23, 2025 20:53

Copilot AI reviewed Nov 23, 2025

View reviewed changes

notjackl3 reviewed Nov 27, 2025

View reviewed changes

src/app/volunteers/filter_by_role/route.ts

Copy link

Contributor

notjackl3 Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to implement route.ts file.

1michhu1 reacted with thumbs up emoji

notjackl3 reviewed Nov 27, 2025

View reviewed changes

LeandroHamaguchi requested changes Dec 2, 2025

View reviewed changes

This was referenced Dec 4, 2025

volunteer update api #43

Open

delete volunteers api #42

Open

	if (volunteer.roleNames.size == filters.length) {
	if (filters.every(filter => volunteer.roleNames.has(filter))) {

	if (operator.toUpperCase() == "OR") {
	if (operator.toUpperCase() === "OR") {

Implement filter_by_role API function #49

Are you sure you want to change the base?

Implement filter_by_role API function #49

Conversation

1michhu1 commented Nov 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Nov 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1michhu1 Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

1michhu1 commented Nov 23, 2025 •

edited

Loading

vercel bot commented Nov 23, 2025 •

edited

Loading

1michhu1 Nov 29, 2025 •

edited

Loading