Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds participant management functionality to the high school activities system, enabling users to view current participants and unregister them from activities.
- Added a new DELETE endpoint for unregistering participants from activities
- Enhanced the UI to display participant lists with delete icons for each participant
- Added validation to prevent duplicate signups
- Included comprehensive test coverage for the new unregister functionality
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app.py | Added unregister endpoint, duplicate signup validation, and 6 new activity entries to the database |
| src/static/app.js | Implemented participant list display with delete functionality and automatic refresh after signup/unregister |
| src/static/styles.css | Added styling for participant lists and delete icons |
| tests/test_app.py | Added comprehensive test suite covering signup, unregister, and error scenarios |
| requirements.txt | Added pytest and httpx dependencies for testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .participants-list { | ||
| margin: 10px 0 10px 20px; | ||
| padding: 0; | ||
| padding-left: 0; |
There was a problem hiding this comment.
Redundant padding declaration. Lines 79-80 both set padding and padding-left to 0, which is already set by the .participants-list.no-bullets class below. Consider removing either padding-left: 0; from line 80 or simplifying the padding declaration.
| padding-left: 0; |
| let participantsHTML = ''; | ||
| if (details.participants.length > 0) { | ||
| participantsHTML = ` | ||
| <p><strong>Participants:</strong></p> | ||
| <ul class="participants-list no-bullets"> | ||
| ${details.participants.map(email => ` | ||
| <li> | ||
| <span class="participant-email">${email}</span> | ||
| <span class="delete-icon" title="Unregister" data-activity="${name}" data-email="${email}">🗑</span> | ||
| </li> | ||
| `).join('')} | ||
| </ul> | ||
| `; | ||
| } else { | ||
| participantsHTML = '<p><strong>Participants:</strong> <em>Be the first to sign up!</em></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> | ||
| ${participantsHTML} | ||
| `; | ||
|
|
There was a problem hiding this comment.
Potential XSS vulnerability. The email and activity name from details.participants and name are inserted into the HTML without sanitization or escaping. If user-controlled data can contain special characters like <, >, ", or ', this could lead to cross-site scripting attacks. Consider using textContent instead of template literals, or properly escape the values before insertion.
| let participantsHTML = ''; | |
| if (details.participants.length > 0) { | |
| participantsHTML = ` | |
| <p><strong>Participants:</strong></p> | |
| <ul class="participants-list no-bullets"> | |
| ${details.participants.map(email => ` | |
| <li> | |
| <span class="participant-email">${email}</span> | |
| <span class="delete-icon" title="Unregister" data-activity="${name}" data-email="${email}">🗑</span> | |
| </li> | |
| `).join('')} | |
| </ul> | |
| `; | |
| } else { | |
| participantsHTML = '<p><strong>Participants:</strong> <em>Be the first to sign up!</em></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> | |
| ${participantsHTML} | |
| `; | |
| // Build participants list safely | |
| let participantsSection; | |
| if (details.participants.length > 0) { | |
| participantsSection = document.createElement("div"); | |
| const participantsTitle = document.createElement("p"); | |
| const strong = document.createElement("strong"); | |
| strong.textContent = "Participants:"; | |
| participantsTitle.appendChild(strong); | |
| participantsSection.appendChild(participantsTitle); | |
| const ul = document.createElement("ul"); | |
| ul.className = "participants-list no-bullets"; | |
| details.participants.forEach(email => { | |
| const li = document.createElement("li"); | |
| const emailSpan = document.createElement("span"); | |
| emailSpan.className = "participant-email"; | |
| emailSpan.textContent = email; | |
| li.appendChild(emailSpan); | |
| const deleteIcon = document.createElement("span"); | |
| deleteIcon.className = "delete-icon"; | |
| deleteIcon.title = "Unregister"; | |
| deleteIcon.setAttribute("data-activity", name); | |
| deleteIcon.setAttribute("data-email", email); | |
| deleteIcon.innerHTML = "🗑"; // Unicode trash can | |
| li.appendChild(deleteIcon); | |
| ul.appendChild(li); | |
| }); | |
| participantsSection.appendChild(ul); | |
| } else { | |
| participantsSection = document.createElement("p"); | |
| const strong = document.createElement("strong"); | |
| strong.textContent = "Participants:"; | |
| participantsSection.appendChild(strong); | |
| const em = document.createElement("em"); | |
| em.textContent = "Be the first to sign up!"; | |
| participantsSection.appendChild(document.createTextNode(" ")); | |
| participantsSection.appendChild(em); | |
| } | |
| // Build activity card safely | |
| activityCard.innerHTML = ""; // Clear any previous content | |
| const h4 = document.createElement("h4"); | |
| h4.textContent = name; | |
| activityCard.appendChild(h4); | |
| const descP = document.createElement("p"); | |
| descP.textContent = details.description; | |
| activityCard.appendChild(descP); | |
| const schedP = document.createElement("p"); | |
| const schedStrong = document.createElement("strong"); | |
| schedStrong.textContent = "Schedule:"; | |
| schedP.appendChild(schedStrong); | |
| schedP.appendChild(document.createTextNode(" " + details.schedule)); | |
| activityCard.appendChild(schedP); | |
| const availP = document.createElement("p"); | |
| const availStrong = document.createElement("strong"); | |
| availStrong.textContent = "Availability:"; | |
| availP.appendChild(availStrong); | |
| availP.appendChild(document.createTextNode(` ${spotsLeft} spots left`)); | |
| activityCard.appendChild(availP); | |
| activityCard.appendChild(participantsSection); |
|
|
||
| # Get the specific activity | ||
| activity = activities[activity_name] | ||
|
|
There was a problem hiding this comment.
Missing validation for activity capacity. The signup endpoint checks if a student is already signed up but doesn't verify if the activity has reached its max_participants limit. This could allow more students to sign up than the activity can accommodate. Consider adding a check like: if len(activity["participants"]) >= activity["max_participants"]: raise HTTPException(status_code=400, detail="Activity is full").
| # Check if activity is full | |
| if len(activity["participants"]) >= activity["max_participants"]: | |
| raise HTTPException(status_code=400, detail="Activity is full") |
| @app.delete("/activities/{activity_name}/unregister") | ||
| def unregister_participant(activity_name: str, email: str): | ||
| """Remove a participant from an activity""" | ||
| if activity_name not in activities: | ||
| raise HTTPException(status_code=404, detail="Activity not found") | ||
| if email not in activities[activity_name]["participants"]: | ||
| raise HTTPException(status_code=400, detail="Student is not registered for this activity") | ||
| activities[activity_name]["participants"].remove(email) | ||
| return {"message": f"Unregistered {email} from {activity_name}"} |
There was a problem hiding this comment.
Missing input validation for email parameter. Both the signup and unregister endpoints accept any string as an email without validation. Consider adding email format validation (e.g., using Pydantic's EmailStr type or regex validation) to ensure valid email addresses are provided before processing.
| @@ -0,0 +1,62 @@ | |||
| import pytest | |||
There was a problem hiding this comment.
Import of 'pytest' is not used.
| import pytest |
This pull request adds several new features and improvements to the school activities signup app, focusing on expanding available activities, enabling participant management from the UI, and strengthening backend validation and test coverage. The most important changes include adding new activities, implementing participant unregister functionality, updating the frontend to allow participant removal, improving the display of participant lists, and introducing backend tests for signup and unregister flows.
New activities and backend features:
Soccer Team,Basketball Club,Art Club,Drama Club,Debate Team,Science Club) to theactivitiesdictionary insrc/app.py, each with their own schedule, description, and initial participants.unregister_participantendpoint insrc/app.pyto allow participants to be removed from activities, with proper validation for registration status.Frontend enhancements:
src/static/app.jsto display participant lists for each activity, including a delete icon next to each participant for easy removal.src/static/app.jsfor the delete icons, enabling users to unregister participants directly from the UI, with confirmation dialogs and feedback messages.src/static/styles.css, including custom icons and visual feedback for delete actions.Testing and reliability:
tests/test_app.pyto cover activity retrieval, signup, duplicate signup prevention, not-found errors, unregister flow, and edge cases for participant removal.Other improvements: