Skip to content
4 changes: 2 additions & 2 deletions backend/core/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@

urlpatterns = [
path("admin/", admin.site.urls),
# API Routes - Uncomment when ready to use
# path('api/', include('users.urls')),
# API Routes
path("api/", include("users.urls")),
# path('api/inventory/', include('inventory.urls')),
# path('api/', include('requests.urls')),
]
Expand Down
46 changes: 17 additions & 29 deletions backend/users/serializers.py
Original file line number Diff line number Diff line change
@@ -1,33 +1,21 @@
from rest_framework import serializers
from .models import VolunteerApplication

# from .models import User

# Create your serializers here.
class VolunteerApplicationSerializer(serializers.ModelSerializer):
"""
Serializer for the VolunteerApplication model.
Handles creating new volunteer applications upon submission.
"""

# Example: User Serializer
# Uncomment and modify as needed
#
# class UserSerializer(serializers.ModelSerializer):
# """
# Serializer for the User model.
# Controls what fields are exposed in the API.
# """
# class Meta:
# model = User
# fields = ['id', 'username', 'email', 'first_name', 'last_name', 'date_joined']
# read_only_fields = ['id', 'date_joined']
#
#
# class UserCreateSerializer(serializers.ModelSerializer):
# """
# Serializer for creating new users with password handling.
# """
# password = serializers.CharField(write_only=True, min_length=8, style={'input_type': 'password'})
#
# class Meta:
# model = User
# fields = ['username', 'email', 'password', 'first_name', 'last_name']
#
# def create(self, validated_data):
# """Create user with encrypted password."""
# return User.objects.create_user(**validated_data)
reviewed_by = serializers.StringRelatedField(read_only=True)

class Meta:
model = VolunteerApplication
fields = ["id", "name", "email", "motivation_text", "status", "created_at", "reviewed_at", "reviewed_by"]
read_only_fields = ["id", "created_at", "reviewed_at", "reviewed_by"]
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The status field is exposed as writable in the serializer, allowing public users to submit applications with any status (including "APPROVED" or "REJECTED") by including the status in the POST request body. Even though the create() method forces status to "PENDING", the field should be added to read_only_fields to make the API contract clearer and prevent status manipulation on updates.

Suggested change
read_only_fields = ["id", "created_at", "reviewed_at", "reviewed_by"]
read_only_fields = ["id", "created_at", "reviewed_at", "reviewed_by", "status"]

Copilot uses AI. Check for mistakes.

def create(self, validated_data):
"""Create a new volunteer application with PENDING status."""
validated_data["status"] = "PENDING"
return super().create(validated_data)
51 changes: 51 additions & 0 deletions backend/users/tests.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import pytest
from django.test import Client
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

Import of 'reverse' is not used.

Copilot uses AI. Check for mistakes.
from django.contrib.auth import get_user_model

from .models import VolunteerApplication


User = get_user_model()


Expand All @@ -11,3 +15,50 @@ def test_create_user():
assert user.email == "test@example.com"
assert user.name == "Test User"
assert user.check_password("testpass123")


@pytest.mark.django_db
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

Missing test coverage for the public POST endpoint (/api/volunteer-applications/) that allows unauthenticated users to submit volunteer applications. This is a core feature mentioned in the PR description.

Add a test that:

  • POSTs a new application without authentication
  • Verifies the response status is 201
  • Confirms the application is created with status="PENDING"
  • Ensures reviewed_at and reviewed_by are null for new submissions

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

Missing test coverage for important edge cases:

  1. Approving an application when a User with that email already exists (should not create duplicate user)
  2. Multiple status changes on the same application (ensure reviewed_at and reviewed_by are only set on first review)
  3. Changing status from PENDING to PENDING (should not trigger review metadata or user creation)

Add tests for these scenarios to ensure the logic in _handle_review_metadata and _handle_volunteer_user_creation handles edge cases correctly.

Copilot uses AI. Check for mistakes.
def test_approve_volunteer_application():
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

Missing test coverage for the public POST endpoint to create volunteer applications. Add a test that verifies: 1) Anyone can submit an application via POST, 2) The application is created with status="PENDING", and 3) The response includes the expected fields.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

Missing test coverage for the admin-only GET list endpoint. Add a test that verifies: 1) Unauthenticated users receive 403 when accessing GET /api/volunteer-applications/, and 2) Admin users can successfully list applications.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

Missing test coverage for edge cases: 1) Approving an application when a user with the same email already exists (should not create duplicate), 2) Attempting to change status from APPROVED back to PENDING or REJECTED (should review metadata behavior), 3) Directly updating to APPROVED without going through PENDING first.

Copilot uses AI. Check for mistakes.
client = Client()

application = VolunteerApplication.objects.create(
name="Test Volunteer",
email="volunteer@example.com",
motivation_text="I want to help",
status="PENDING",
)

url = f"/api/volunteer-applications/{application.id}/"

response = client.patch(url, {"status": "APPROVED"}, content_type="application/json")
assert response.status_code in (200, 202)

application.refresh_from_db()
assert application.status == "APPROVED"
assert application.reviewed_at is not None

Comment on lines +36 to +39
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The test verifies that reviewed_at is set, but doesn't verify that reviewed_by is also set when an application is approved. According to the PR description and the _handle_review_metadata implementation, both fields should be populated when an authenticated admin reviews an application.

Add an assertion to verify application.reviewed_by is set to the reviewing user (or None if unauthenticated, which highlights the permission issue).

Copilot uses AI. Check for mistakes.
user = User.objects.get(email="volunteer@example.com")
assert user.role == "VOLUNTEER"
Comment on lines +20 to +41
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The test for approving applications doesn't verify that reviewed_by is set. According to the implementation in views.py, reviewed_by should be set when an authenticated user reviews an application. Add an assertion to check application.reviewed_by is set correctly (or is None if the request is unauthenticated).

Copilot uses AI. Check for mistakes.


@pytest.mark.django_db
def test_reject_application():
client = Client()

application = VolunteerApplication.objects.create(
name="Test Volunteer 2",
email="reject@example.com",
motivation_text="I want to help",
status="PENDING",
)

url = f"/api/volunteer-applications/{application.id}/"

response = client.patch(url, {"status": "REJECTED"}, content_type="application/json")
assert response.status_code in (200, 202)

application.refresh_from_db()
assert application.status == "REJECTED"
assert application.reviewed_at is not None

Comment on lines +60 to +63
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

Similar to the approval test, this test should also verify that reviewed_by is set when an application is rejected. The _handle_review_metadata method sets both reviewed_at and reviewed_by for any status change to APPROVED or REJECTED.

Add an assertion to verify application.reviewed_by is properly set.

Copilot uses AI. Check for mistakes.
assert not User.objects.filter(email="reject@example.com").exists()
32 changes: 12 additions & 20 deletions backend/users/urls.py
Original file line number Diff line number Diff line change
@@ -1,27 +1,19 @@
from django.urls import path, include
from rest_framework.routers import DefaultRouter

# from .views import UserViewSet
from .views import VolunteerApplicationAPIView

# Create your URL patterns here.

# Example: Using DRF Router for ViewSets
# Uncomment and modify as needed
#
# router = DefaultRouter()
# router.register(r'users', UserViewSet, basename='user')
#
# urlpatterns = [
# path('', include(router.urls)),
# ]

# This will create the following endpoints:
# GET /api/users/ - List all users
# POST /api/users/ - Create a new user
# GET /api/users/{id}/ - Retrieve a specific user
# PUT /api/users/{id}/ - Update a user
# PATCH /api/users/{id}/ - Partial update
# DELETE /api/users/{id}/ - Delete a user
# GET /api/users/me/ - Custom action (if defined in viewset)
router = DefaultRouter()
router.register(r"volunteer-applications", VolunteerApplicationAPIView, basename="volunteer-application")

urlpatterns = []
urlpatterns = [
path("", include(router.urls)),
]

# This will create the following endpoints:
# POST /api/volunteer-applications/ - Create a new application (PENDING)
# GET /api/volunteer-applications/ - List all applications
# GET /api/volunteer-applications/{id}/ - Retrieve a specific application (admin only)
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The comment states "Retrieve a specific application (admin only)" but there's no admin-only restriction implemented for the retrieve endpoint. The current implementation only protects the list endpoint. Either update the comment to reflect the actual implementation or add the missing admin protection to the retrieve endpoint.

Suggested change
# GET /api/volunteer-applications/{id}/ - Retrieve a specific application (admin only)
# GET /api/volunteer-applications/{id}/ - Retrieve a specific application

Copilot uses AI. Check for mistakes.
# PATCH /api/volunteer-applications/{id}/ - Partial update
107 changes: 76 additions & 31 deletions backend/users/views.py
Original file line number Diff line number Diff line change
@@ -1,35 +1,80 @@
from rest_framework import viewsets, permissions, status
from rest_framework.decorators import action
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The action decorator is imported but never used in this file. Remove this unused import to keep the code clean.

Suggested change
from rest_framework.decorators import action

Copilot uses AI. Check for mistakes.
from rest_framework.response import Response
from django.contrib.auth import get_user_model
from django.utils import timezone
from django.db import transaction
import secrets

# from .models import User
# from .serializers import UserSerializer, UserCreateSerializer

# Create your views here.

# Example: User ViewSet
# Uncomment and modify as needed
#
# class UserViewSet(viewsets.ModelViewSet):
# """
# ViewSet for managing users.
# Provides CRUD operations: list, create, retrieve, update, delete
# """
# queryset = User.objects.all()
# serializer_class = UserSerializer
# permission_classes = [permissions.IsAuthenticatedOrReadOnly]
#
# def get_serializer_class(self):
# """Return appropriate serializer based on action."""
# if self.action == 'create':
# return UserCreateSerializer
# return UserSerializer
#
# @action(detail=False, methods=['get'], permission_classes=[permissions.IsAuthenticated])
# def me(self, request):
# """
# Custom endpoint: GET /api/users/me/
# Returns the current authenticated user.
# """
# serializer = self.get_serializer(request.user)
# return Response(serializer.data)
from .models import VolunteerApplication
from .serializers import VolunteerApplicationSerializer

User = get_user_model()


class VolunteerApplicationAPIView(viewsets.ModelViewSet):
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The class name VolunteerApplicationAPIView is misleading. The suffix APIView is typically used for DRF's APIView class, not ModelViewSet. Consider renaming to VolunteerApplicationViewSet to follow Django REST Framework naming conventions.

Suggested change
class VolunteerApplicationAPIView(viewsets.ModelViewSet):
class VolunteerApplicationViewSet(viewsets.ModelViewSet):

Copilot uses AI. Check for mistakes.
"""
API endpoint for submitting volunteer applications.
Currently contains:
POST /api/volunteer-applications/
Comment on lines +17 to +20
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The docstring is incomplete and doesn't accurately describe the full functionality of this ViewSet. It only mentions the POST endpoint, but the class also implements:

  • GET /api/volunteer-applications/ (admin-only list)
  • GET /api/volunteer-applications/{id}/ (retrieve)
  • PATCH /api/volunteer-applications/{id}/ (approve/reject)

Update the docstring to document all available endpoints and their access restrictions.

Suggested change
API endpoint for submitting volunteer applications.
Currently contains:
POST /api/volunteer-applications/
API endpoint for managing volunteer applications.
Endpoints:
- POST /api/volunteer-applications/
Submit a new volunteer application. (Open to all)
- GET /api/volunteer-applications/
List all volunteer applications. (Admin only)
- GET /api/volunteer-applications/{id}/
Retrieve a specific volunteer application. (Admin only)
- PATCH /api/volunteer-applications/{id}/
Approve or reject a volunteer application. (Admin only)
Access:
- POST: Any user (no authentication required)
- GET (list/retrieve), PATCH: Admin users only (role == "ADMIN")

Copilot uses AI. Check for mistakes.
"""

queryset = VolunteerApplication.objects.all()
serializer_class = VolunteerApplicationSerializer
permission_classes = [permissions.AllowAny]
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The AllowAny permission class is too permissive for a ModelViewSet. This allows unrestricted access to all CRUD operations (GET, POST, PUT, PATCH, DELETE) on all volunteer applications, including retrieving and modifying individual applications. Consider using DRF's get_permissions() method to return different permission classes based on the action (e.g., AllowAny for create, but require authentication/admin for list, retrieve, update, destroy).

Copilot uses AI. Check for mistakes.

def get_serializer_class(self):
"""Return appropriate serializer based on action."""
if self.action == "create":
return VolunteerApplicationSerializer
return VolunteerApplicationSerializer # Can update this later, just for formality

Comment on lines +27 to +32
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The get_serializer_class() method always returns the same serializer regardless of action, making it redundant. Either remove this method entirely (and rely on the serializer_class attribute) or add a meaningful distinction between actions if different serializers are needed in the future.

Suggested change
def get_serializer_class(self):
"""Return appropriate serializer based on action."""
if self.action == "create":
return VolunteerApplicationSerializer
return VolunteerApplicationSerializer # Can update this later, just for formality

Copilot uses AI. Check for mistakes.
def list(self, request, *args, **kwargs):
"""List applications; restrict to admin users using role PLACEHOLDER."""
user = getattr(request, "user", None)
if not (user is not None and getattr(user, "is_authenticated", False) and self._is_admin(user)):
return Response({"detail": "Admin only"}, status=status.HTTP_403_FORBIDDEN)

return super().list(request, *args, **kwargs)
Comment on lines +33 to +39
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The list() method is protected by admin check, but the retrieve(), update(), partial_update(), and destroy() methods inherited from ModelViewSet remain unprotected due to AllowAny permissions. Anyone can view, modify, or delete individual applications. Override these methods with appropriate permission checks or use get_permissions() to enforce proper authorization.

Copilot uses AI. Check for mistakes.
Comment on lines +33 to +39
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

Only the list() method has admin-only protection, but other methods like retrieve(), update(), partial_update(), and destroy() are not protected. This means:

  • Anyone can GET individual applications (potentially exposing PII)
  • Anyone can update/delete applications (due to AllowAny permissions)

Override these methods or use get_permissions() to ensure proper access control for all actions. Typically, only admins should be able to view individual applications and update their status.

Copilot uses AI. Check for mistakes.

def _is_admin(self, user):
"""PLACEHOLDER check for admin users based on role."""
return getattr(user, "role", None) == "ADMIN"

def _handle_review_metadata(self, application):
"""Set reviewed_at and reviewed_by when an application is reviewed."""
if application.reviewed_at is None:
application.reviewed_at = timezone.now()

user = getattr(self.request, "user", None)
if user is not None and getattr(user, "is_authenticated", False) and application.reviewed_by is None:
application.reviewed_by = user

application.save()
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The _handle_review_metadata() method calls application.save() which saves the application outside the serializer's save flow. Since this method is called after serializer.save() in perform_update(), it performs a second database write. This can cause issues with the transaction and the fields might not be properly updated through the serializer. Instead, set the fields before calling serializer.save() or pass them to the serializer's save method.

Copilot uses AI. Check for mistakes.

Comment on lines +54 to +55
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The application.save() call causes the application to be saved twice within the same transaction: once by serializer.save() (line 76) and again here. This is inefficient and could lead to race conditions or unexpected behavior.

Instead, modify the fields without calling save() and let the transaction commit handle the single save operation. The @transaction.atomic decorator will ensure all changes are committed together.

Suggested change
application.save()

Copilot uses AI. Check for mistakes.
def _handle_volunteer_user_creation(self, application):
"""Create a VOLUNTEER user when an application is approved, if needed."""
if application.status != "APPROVED":
return

exists = User.objects.filter(email=application.email).exists()
if exists:
return

temp_password = secrets.token_urlsafe(12)
User.objects.create_user(
email=application.email,
name=application.name,
password=temp_password,
role="VOLUNTEER",
)
Comment on lines +61 to +71
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

There's a potential race condition between checking if a user exists and creating the user. If two applications with the same email are approved simultaneously, both threads could pass the exists check before either creates the user, resulting in an IntegrityError due to the unique email constraint. Use get_or_create() or handle the IntegrityError gracefully to prevent crashes.

Suggested change
exists = User.objects.filter(email=application.email).exists()
if exists:
return
temp_password = secrets.token_urlsafe(12)
User.objects.create_user(
email=application.email,
name=application.name,
password=temp_password,
role="VOLUNTEER",
)
temp_password = secrets.token_urlsafe(12)
user, created = User.objects.get_or_create(
email=application.email,
defaults={
"name": application.name,
"role": "VOLUNTEER",
}
)
if created:
user.set_password(temp_password)
user.save()

Copilot uses AI. Check for mistakes.

Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The temporary password generated using secrets.token_urlsafe(12) is created but never stored, logged, or sent to the user. This means the created volunteer user account will be inaccessible since no one knows the password. Consider storing this password securely for later retrieval (e.g., encrypted in the database) or implementing an email invitation flow to allow users to set their own password.

Suggested change
# Store the temporary password in the application for later retrieval/notification
application.temp_password = temp_password
application.save(update_fields=["temp_password"])

Copilot uses AI. Check for mistakes.
@transaction.atomic
def perform_update(self, serializer):
old_status = serializer.instance.status
application = serializer.save()

if old_status != application.status and application.status in {"APPROVED", "REJECTED"}:
self._handle_review_metadata(application)
self._handle_volunteer_user_creation(application)