Improve student activity reg system #113
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the student activity registration system by adding participant visibility and unregistration functionality. The changes enable students to see who is enrolled in each activity and remove themselves from activities they no longer wish to attend.
Key changes:
- Added a new DELETE endpoint for unregistering students from activities
- Implemented participant list display with delete buttons in the UI
- Added duplicate signup validation to prevent students from registering twice
- Expanded the activities database with 6 additional activities
- Added comprehensive test suite with pytest and httpx dependencies
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app.py | Added 6 new activities, implemented unregister endpoint, and added duplicate signup validation |
| src/static/app.js | Added participant display with delete functionality and refresh logic after signup/unregister |
| src/static/styles.css | Added styling for participant list display and delete buttons |
| tests/test_app.py | New comprehensive test suite covering activities, signup, unregister, and edge cases |
| tests/conftest.py | New pytest fixtures for test client and activity state reset |
| requirements.txt | Added pytest and httpx for testing support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Validate student is not already signed up for the activity | ||
| if email in activity["participants"]: | ||
| raise HTTPException(status_code=400, detail="Already signed up for this activity") | ||
|
|
There was a problem hiding this comment.
The signup endpoint does not validate whether the activity has reached its maximum capacity before adding a new participant. This allows more participants to sign up than max_participants allows.
Add a validation check:
if len(activity["participants"]) >= activity["max_participants"]:
raise HTTPException(status_code=400, detail="Activity is full")| # Validate activity has not reached maximum capacity | |
| if len(activity["participants"]) >= activity["max_participants"]: | |
| raise HTTPException(status_code=400, detail="Activity is full") |
| # Validate student is not already signed up for the activity | ||
| if email in activity["participants"]: | ||
| raise HTTPException(status_code=400, detail="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.
The new unregister functionality lacks test coverage for the scenario where an activity reaches maximum capacity. There should be a test to verify that signups are rejected when len(participants) >= max_participants.
Add a test case like:
def test_signup_activity_full(self, reset_activities):
"""Test that signup fails when activity is at capacity"""
# Fill up Chess Club (max 12)
# Try to add 13th participant and verify it fails with 400| const spotsLeft = details.max_participants - details.participants.length; | ||
| const participantsMarkup = details.participants?.length | ||
| ? `<ul class="participants-list"> | ||
| ${details.participants.map((p) => `<li>${p} <button class="delete-btn" data-activity="${name}" data-email="${p}" aria-label="Delete ${p}">✕</button></li>`).join("")} |
There was a problem hiding this comment.
The aria-label attribute also uses unsanitized user input (email), which could lead to attribute injection attacks. An attacker could use an email like test" onclick="alert('xss')" data-x="@test.com to inject malicious attributes.
Ensure proper escaping or use a safer method to construct DOM elements with createElement() and set properties individually rather than using innerHTML.
| const participantsMarkup = details.participants?.length | ||
| ? `<ul class="participants-list"> | ||
| ${details.participants.map((p) => `<li>${p} <button class="delete-btn" data-activity="${name}" data-email="${p}" aria-label="Delete ${p}">✕</button></li>`).join("")} | ||
| </ul>` | ||
| : `<p class="participants-empty">Be the first to join this activity!</p>`; | ||
|
|
||
| activityCard.innerHTML = ` | ||
| <h4>${name}</h4> | ||
| <p>${details.description}</p> | ||
| <p><strong>Schedule:</strong> ${details.schedule}</p> | ||
| <p><strong>Availability:</strong> ${spotsLeft} spots left</p> | ||
| <div class="participants"> | ||
| <div class="participants-header"> | ||
| <span class="participants-title">Participants</span> | ||
| <span class="participants-count">${details.participants.length}</span> | ||
| </div> | ||
| ${participantsMarkup} | ||
| </div> |
There was a problem hiding this comment.
The name variable (activity name) is also directly interpolated into HTML without sanitization in multiple places. While activity names are currently hardcoded in the backend, if this ever becomes user-editable, it would introduce XSS vulnerabilities.
Consider sanitizing or escaping the name variable as well, or use DOM manipulation methods instead of innerHTML.
| // Add event listeners for delete buttons using event delegation | ||
| activitiesList.addEventListener("click", async (event) => { | ||
| if (event.target.classList.contains("delete-btn")) { | ||
| const activityName = event.target.dataset.activity; | ||
| const email = event.target.dataset.email; | ||
|
|
||
| if (confirm(`Remove ${email} from ${activityName}?`)) { | ||
| try { | ||
| const response = await fetch( | ||
| `/activities/${encodeURIComponent(activityName)}/unregister?email=${encodeURIComponent(email)}`, | ||
| { | ||
| method: "DELETE", | ||
| } | ||
| ); | ||
|
|
||
| if (response.ok) { | ||
| // Refresh the activities list | ||
| fetchActivities(); | ||
| } else { | ||
| const result = await response.json(); | ||
| alert(result.detail || "Failed to unregister participant"); | ||
| } | ||
| } catch (error) { | ||
| alert("Failed to unregister participant"); | ||
| console.error("Error unregistering:", error); | ||
| } | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
The event listener for delete buttons is added inside fetchActivities(), which means a new listener is attached every time the activities list is refreshed. This creates duplicate listeners that will cause the unregister action to be triggered multiple times.
Move the event listener registration outside of fetchActivities() so it's only registered once when the page loads.
| const participantsMarkup = details.participants?.length | ||
| ? `<ul class="participants-list"> | ||
| ${details.participants.map((p) => `<li>${p} <button class="delete-btn" data-activity="${name}" data-email="${p}" aria-label="Delete ${p}">✕</button></li>`).join("")} |
There was a problem hiding this comment.
Participant emails are directly interpolated into the HTML without sanitization, creating a potential XSS vulnerability. If a malicious email address containing HTML/JavaScript is stored (e.g., <script>alert('xss')</script>@test.com), it will be executed when the page is rendered.
Use textContent instead of template literals for user-generated content, or properly escape the email addresses before inserting them into the HTML.
1 participant
Summary
Changes
Closes:
Task list