Skip to content

delete volunteers api #42

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.

Sign up for GitHub

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

Open
BrandonT310442 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

delete volunteers api #42

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@
# next.js
/.next/
/out/

.claude
Documentation
# production
/build

Expand Down
71 changes: 71 additions & 0 deletions src/lib/api/deleteVolunteer.ts
View file
Open in desktop
Copy link
Contributor

@notjackl3 notjackl3 Nov 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

You should change the branch name to from remove_volunteer_api to backend/remove-volunteer-api.

Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import { createClient } from "../client/supabase/server";

export async function deleteVolunteer(id: number) {
// Validate input
if (!id || typeof id !== "number" || id <= 0) {
return {
error: {
message: "Invalid volunteer ID. ID must be a positive number.",
Comment on lines +5 to +8
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

The validation !id will incorrectly reject id: 0, but id <= 0 already covers this case. While id: 0 is invalid per the business logic, the !id check is redundant because:

  • If id is 0, the id <= 0 check will catch it
  • The !id check adds unnecessary complexity

Consider simplifying to:

if (typeof id !== "number" || id <= 0 || !Number.isInteger(id)) {

Also consider adding !Number.isInteger(id) to prevent decimal IDs like 1.5 from passing validation.

Suggested change
if (!id || typeof id !== "number" || id <= 0) {
return {
error: {
message: "Invalid volunteer ID. ID must be a positive number.",
if (typeof id !== "number" || id <= 0 || !Number.isInteger(id)) {
return {
error: {
message: "Invalid volunteer ID. ID must be a positive integer.",

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@notjackl3 notjackl3 Nov 26, 2025

Choose a reason for hiding this comment

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

Copilot is right, no need for !id here because we are already checking typeof.

status: 400,
},
data: null,
};
}

try {
const client = await createClient();

// Check if volunteer exists before deleting
const { data: existingVolunteer, error: fetchError } = await client
.from("Volunteers")
.select("id")
.eq("id", id)
.single();

if (fetchError || !existingVolunteer) {
return {
error: {
message: "Volunteer not found.",
status: 404,
},
data: null,
};
}

// Delete the volunteer
const { error: deleteError } = await client
.from("Volunteers")
.delete()
.eq("id", id);

if (deleteError) {
return {
error: {
message: deleteError.message || "Failed to delete volunteer.",
status: 500,
Comment on lines +18 to +45
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

Potential race condition: The check-then-delete pattern (lines 19-39) is vulnerable to race conditions. If another process deletes the volunteer between the existence check and the actual delete operation, the delete will succeed without error, but you've already verified the volunteer exists.

While this may not cause a critical issue in this specific case, it represents a Time-of-check to time-of-use (TOCTOU) problem. Consider either:

  1. Removing the existence check and relying on the delete operation's return data to determine if a row was affected
  2. If you need the existence check for business logic, document this known limitation

For option 1, Supabase delete operations don't return the deleted data by default, but you can check if the operation succeeded without errors as an indicator.

Suggested change
// Check if volunteer exists before deleting
const { data: existingVolunteer, error: fetchError } = await client
.from("Volunteers")
.select("id")
.eq("id", id)
.single();
if (fetchError || !existingVolunteer) {
return {
error: {
message: "Volunteer not found.",
status: 404,
},
data: null,
};
}
// Delete the volunteer
const { error: deleteError } = await client
.from("Volunteers")
.delete()
.eq("id", id);
if (deleteError) {
return {
error: {
message: deleteError.message || "Failed to delete volunteer.",
status: 500,
// Attempt to delete the volunteer and return the deleted id
const { data: deletedVolunteer, error: deleteError } = await client
.from("Volunteers")
.delete()
.eq("id", id)
.select("id")
if (deleteError) {
return {
error: {
message: deleteError.message || "Failed to delete volunteer.",
status: 500,
},
data: null,
};
}
// If no volunteer was deleted, it did not exist
if (!deletedVolunteer || deletedVolunteer.length === 0) {
return {
error: {
message: "Volunteer not found.",
status: 404,

Copilot uses AI. Check for mistakes.
Copy link
Member

@LeandroHamaguchi LeandroHamaguchi Dec 4, 2025

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. Can we rely on the client's return value to determine whether the volunteer was actually deleted?

One example of how this might be an issue is if the user spams the delete button.

},
data: null,
};
}

return {
error: null,
data: {
message: "Volunteer deleted successfully.",
id,
},
status: 200,
};
Comment on lines +51 to +58
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

Inconsistent return structure: The success response has status at the top level, but error responses have status nested inside the error object. This inconsistency makes it harder for API consumers to handle responses uniformly.

Consider either:

  1. Moving status inside the data object for success responses, or
  2. Always placing status at the top level for both success and error cases

Example of consistent structure (option 2):

return {
  status: 200,
  error: null,
  data: {
    message: "Volunteer deleted successfully.",
    id,
  },
};

Copilot uses AI. Check for mistakes.
} catch (error) {
return {
error: {
message:
error instanceof Error
? error.message
: "An unexpected error occurred.",
status: 500,
},
data: null,
};
}
}
1 change: 1 addition & 0 deletions src/lib/api/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// Add any exports from your API files here

export { getExample } from "./getExample";
export { deleteVolunteer } from "./deleteVolunteer";
Loading