Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds activity unregistration functionality, enhances the frontend with participant management UI, expands the activity database, and adds comprehensive API testing to the High School Management System. However, the PR contains several critical bugs that prevent it from functioning correctly, including structural issues in tests, improper code placement, duplicate imports, and security vulnerabilities.
Changes:
- Added
/activities/{activity_name}/unregisterPOST endpoint to remove participants from activities - Enhanced frontend JavaScript to display participant lists with delete functionality for each participant
- Expanded activity database with six new activities (Basketball Team, Soccer Club, Drama Club, Art Workshop, Math Olympiad, Science Club)
- Added duplicate signup prevention and comprehensive API tests
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_app.py | New test file with API tests, but contains critical structural issues with nested function definitions |
| src/static/styles.css | Added CSS styling for the participants section UI |
| src/static/app.js | Enhanced to display participants with delete icons and refresh activities after changes, but has XSS vulnerabilities |
| src/app.py | Added unregister endpoint, expanded activities database, duplicate signup prevention, but has import issues and missing capacity validation |
| requirements.txt | Added pytest, httpx, and pytest-cov for testing infrastructure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/static/app.js
Outdated
| (email) => | ||
| `<div class="participant-item"> | ||
| <span class="participant-email">${email}</span> | ||
| <span class="delete-icon" title="Remove participant" data-activity="${name}" data-email="${email}">🗑</span> | ||
| </div>` |
There was a problem hiding this comment.
XSS vulnerability: The activity name and email addresses are directly interpolated into HTML without proper escaping. If user-provided data (like email addresses) contains HTML/JavaScript, it will be executed in the browser. Use textContent for user-provided data or properly escape HTML entities before inserting into innerHTML.
src/app.py
Outdated
| app.mount("/static", StaticFiles(directory=os.path.join(Path(__file__).parent, "static")), name="static") | ||
| # Unregister endpoint | ||
| @app.post("/activities/{activity_name}/unregister") | ||
| async def unregister_from_activity(activity_name: str, email: str): |
There was a problem hiding this comment.
Missing email validation: The unregister endpoint accepts any string as an email parameter without validation. This should validate email format for consistency with the signup endpoint.
tests/test_app.py
Outdated
| def test_max_participants_limit(): | ||
| activity = "Art Workshop" | ||
| emails = [f"user{i}@example.com" for i in range(1, 22)] | ||
| # Fill up the activity | ||
| for email in emails[:20]: | ||
| resp = client.post(f"/activities/{activity}/signup?email={email}") | ||
| assert resp.status_code == 200 or resp.status_code == 400 | ||
| # Try to add one more | ||
| resp = client.post(f"/activities/{activity}/signup?email={emails[20]}") | ||
| assert resp.status_code == 400 | ||
| assert resp.json()["detail"] == "Activity is full" |
There was a problem hiding this comment.
Test isolation issue: The test_max_participants_limit test fills up "Art Workshop" with 20 participants but doesn't clean up afterwards. This could cause other tests to fail if they depend on Art Workshop having available slots. Tests should either use unique test data, clean up after themselves, or use fixtures to reset state.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: pmca31 <6774003+pmca31@users.noreply.github.com>
Co-authored-by: pmca31 <6774003+pmca31@users.noreply.github.com>
Co-authored-by: pmca31 <6774003+pmca31@users.noreply.github.com>
Co-authored-by: pmca31 <6774003+pmca31@users.noreply.github.com>
Co-authored-by: pmca31 <6774003+pmca31@users.noreply.github.com>
Co-authored-by: pmca31 <6774003+pmca31@users.noreply.github.com>
Fix Python execution order: move unregister endpoint after activities dict
Fix XSS vulnerability in activity card rendering
Add email validation for empty and whitespace-only values
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/app.py:16
- Top of file has duplicate imports and a placeholder comment (
# ...existing code...) plus unused imports (Request,JSONResponse) and an unused variable (current_dir). This adds noise and can confuse readers; remove duplicates/placeholders and keep only the imports actually used in this module.
from fastapi import FastAPI, HTTPException, Request
from fastapi.responses import JSONResponse
# ...existing code...
"""
High School Management System API
A super simple FastAPI application that allows students to view and sign up
for extracurricular activities at Mergington High School.
"""
from fastapi import FastAPI, HTTPException
from fastapi.staticfiles import StaticFiles
from fastapi.responses import RedirectResponse
import os
from pathlib import Path
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .participants-list { | ||
| margin-top: 6px; | ||
| margin-left: 18px; | ||
| list-style-type: disc; |
There was a problem hiding this comment.
.participants-list is styled with list-style-type: disc, but the JS creates it as a <div> containing <div> items, so list bullets won’t render. Use semantic <ul>/<li> in the markup (and update CSS selectors accordingly), or remove list-style-related CSS and implement bullets another way.
| list-style-type: disc; |
| const participantsList = document.createElement("div"); | ||
| participantsList.className = "participants-list"; | ||
|
|
||
| details.participants.forEach((email) => { | ||
| const participantItem = document.createElement("div"); |
There was a problem hiding this comment.
Participants markup uses <div class="participants-list"> / <div class="participant-item">, but the CSS suggests a list with bullets. Consider rendering participants as a <ul> with <li> items (or adjust CSS) so the UI matches the intended styling and improves semantics for assistive tech.
| const participantsList = document.createElement("div"); | |
| participantsList.className = "participants-list"; | |
| details.participants.forEach((email) => { | |
| const participantItem = document.createElement("div"); | |
| const participantsList = document.createElement("ul"); | |
| participantsList.className = "participants-list"; | |
| details.participants.forEach((email) => { | |
| const participantItem = document.createElement("li"); |
| # Get the specific activity | ||
| activity = activities[activity_name] | ||
|
|
||
| # Validate student is not already signed up | ||
| if email in activity["participants"]: | ||
| raise HTTPException(status_code=400, detail="Student already signed up for this activity") | ||
|
|
||
|
|
||
| # Add student | ||
| activity["participants"].append(email) | ||
| return {"message": f"Signed up {email} for {activity_name}"} |
There was a problem hiding this comment.
signup_for_activity no longer enforces max_participants capacity before appending to participants. This makes it possible to overfill an activity and will break the expected 400 "Activity is full" behavior (see tests). Add a check comparing len(activity["participants"]) against activity["max_participants"] and raise HTTPException(400, "Activity is full") before appending.
Co-authored-by: pmca31 <6774003+pmca31@users.noreply.github.com>
Remove async from unregister_from_activity
This pull request introduces several new features and improvements to the High School Management System, focusing on activity management, user experience enhancements, and test coverage. The most significant changes include the addition of an activity unregistration feature, improved frontend participant management, and expanded backend activity data. Automated tests have also been added to ensure correct API behavior.
Backend enhancements:
/activities/{activity_name}/unregisterPOST endpoint insrc/app.pyto allow participants to be removed from activities, with appropriate error handling for missing activities or participants.activitiesdatabase insrc/app.pyto include more activities such as Basketball Team, Soccer Club, Drama Club, Art Workshop, Math Olympiad, and Science Club, each with their own details.src/app.pyto prevent duplicate signups by checking if a student is already registered before adding them.Frontend improvements:
src/static/app.jsto display a list of participants for each activity, including a delete icon for each participant that allows removal via the new unregistration endpoint. The participant list refreshes automatically after changes. [1] [2]src/static/styles.cssto style the participants section, making it visually distinct and user-friendly.Testing:
tests/test_app.pyto cover activity retrieval, signup, unregistration, duplicate signup prevention, non-existent activities, participant removal, activity capacity limits, and static file serving.