Improve student activity registration system#2
Conversation
There was a problem hiding this comment.
Pull request overview
This PR expands the student activities signup app by adding an unregister capability, surfacing participant lists in the UI, broadening the seeded activity set, and introducing backend tests to cover core API behaviors.
Changes:
- Added a
DELETE /activities/{activity_name}/signupendpoint to unregister a student, and added duplicate-signup validation. - Enhanced the frontend to display participants per activity and allow removing a participant from the UI.
- Added pytest-based backend tests and updated dependencies.
Show a summary per file
| File | Description |
|---|---|
src/app.py |
Adds more activities, duplicate signup check, and the unregister endpoint. |
src/static/app.js |
Renders participant lists, adds “remove participant” behavior, and refreshes activities after mutations. |
src/static/styles.css |
Styles participant list UI and remove button. |
tests/test_app.py |
Introduces API tests for listing activities, signup, and unregister flows. |
requirements.txt |
Adds pytest to dependencies and fixes formatting. |
tests/__init__.py |
Package marker (no functional changes). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (5)
tests/test_app.py:79
- This test interpolates an activity name with spaces directly into the URL path. URL-encode the
activity_namesegment (e.g., viaurllib.parse.quote) to avoid invalid URL parsing and to match how the frontend calls the API.
response = client.post(
f"/activities/{activity_name}/signup",
params={"email": email},
)
tests/test_app.py:95
- This test interpolates an activity name with spaces directly into the URL path. URL-encode the
activity_namesegment (e.g., viaurllib.parse.quote) to avoid invalid URL parsing and to match how the frontend calls the API.
response = client.post(
f"/activities/{activity_name}/signup",
params={"email": email},
)
tests/test_app.py:113
- This test interpolates an activity name with spaces directly into the URL path. URL-encode the
activity_namesegment (e.g., viaurllib.parse.quote) to avoid invalid URL parsing and to match how the frontend calls the API.
response = client.delete(
f"/activities/{activity_name}/signup",
params={"email": email},
)
tests/test_app.py:130
- This test interpolates an activity name with spaces directly into the URL path. URL-encode the
activity_namesegment (e.g., viaurllib.parse.quote) to avoid invalid URL parsing and to match how the frontend calls the API.
response = client.delete(
f"/activities/{activity_name}/signup",
params={"email": email},
)
tests/test_app.py:146
- This test interpolates an activity name with spaces directly into the URL path. URL-encode the
activity_namesegment (e.g., viaurllib.parse.quote) to avoid invalid URL parsing and to match how the frontend calls the API.
response = client.delete(
f"/activities/{activity_name}/signup",
params={"email": email},
)
- Files reviewed: 4/6 changed files
- Comments generated: 7
| const participantsList = details.participants | ||
| .map( | ||
| (p) => | ||
| `<li><span class="participant-email">${p}</span><button class="remove-btn" data-activity="${name}" data-email="${p}" title="Remove ${p}">✕</button></li>` | ||
| ) | ||
| .join(""); | ||
|
|
||
| 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-section"> | ||
| <strong>Participants:</strong> | ||
| <ul class="participants-list"> | ||
| ${participantsList || "<li class='no-participants'>No participants yet</li>"} | ||
| </ul> | ||
| </div> | ||
| `; | ||
|
|
There was a problem hiding this comment.
details.participants (and name) are interpolated directly into innerHTML and into HTML attributes. Since email comes from user input, this enables DOM/attribute injection (XSS) if a malicious value is stored. Build these list items with DOM APIs (createElement, textContent, dataset) or escape/sanitize values before inserting into HTML/attributes.
| const participantsList = details.participants | |
| .map( | |
| (p) => | |
| `<li><span class="participant-email">${p}</span><button class="remove-btn" data-activity="${name}" data-email="${p}" title="Remove ${p}">✕</button></li>` | |
| ) | |
| .join(""); | |
| 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-section"> | |
| <strong>Participants:</strong> | |
| <ul class="participants-list"> | |
| ${participantsList || "<li class='no-participants'>No participants yet</li>"} | |
| </ul> | |
| </div> | |
| `; | |
| const title = document.createElement("h4"); | |
| title.textContent = name; | |
| const description = document.createElement("p"); | |
| description.textContent = details.description; | |
| const schedule = document.createElement("p"); | |
| const scheduleLabel = document.createElement("strong"); | |
| scheduleLabel.textContent = "Schedule:"; | |
| schedule.appendChild(scheduleLabel); | |
| schedule.appendChild(document.createTextNode(` ${details.schedule}`)); | |
| const availability = document.createElement("p"); | |
| const availabilityLabel = document.createElement("strong"); | |
| availabilityLabel.textContent = "Availability:"; | |
| availability.appendChild(availabilityLabel); | |
| availability.appendChild(document.createTextNode(` ${spotsLeft} spots left`)); | |
| const participantsSection = document.createElement("div"); | |
| participantsSection.className = "participants-section"; | |
| const participantsLabel = document.createElement("strong"); | |
| participantsLabel.textContent = "Participants:"; | |
| const participantsUl = document.createElement("ul"); | |
| participantsUl.className = "participants-list"; | |
| if (details.participants.length > 0) { | |
| details.participants.forEach((p) => { | |
| const participantItem = document.createElement("li"); | |
| const participantEmail = document.createElement("span"); | |
| participantEmail.className = "participant-email"; | |
| participantEmail.textContent = p; | |
| const removeButton = document.createElement("button"); | |
| removeButton.className = "remove-btn"; | |
| removeButton.dataset.activity = name; | |
| removeButton.dataset.email = p; | |
| removeButton.title = `Remove ${p}`; | |
| removeButton.textContent = "✕"; | |
| participantItem.appendChild(participantEmail); | |
| participantItem.appendChild(removeButton); | |
| participantsUl.appendChild(participantItem); | |
| }); | |
| } else { | |
| const noParticipantsItem = document.createElement("li"); | |
| noParticipantsItem.className = "no-participants"; | |
| noParticipantsItem.textContent = "No participants yet"; | |
| participantsUl.appendChild(noParticipantsItem); | |
| } | |
| participantsSection.appendChild(participantsLabel); | |
| participantsSection.appendChild(participantsUl); | |
| activityCard.appendChild(title); | |
| activityCard.appendChild(description); | |
| activityCard.appendChild(schedule); | |
| activityCard.appendChild(availability); | |
| activityCard.appendChild(participantsSection); |
| if (response.ok) { | ||
| messageDiv.textContent = result.message; | ||
| messageDiv.className = "success"; | ||
| signupForm.reset(); | ||
| fetchActivities(); | ||
| } else { | ||
| messageDiv.textContent = result.detail || "An error occurred"; |
There was a problem hiding this comment.
fetchActivities() is now called after signup/removal, but fetchActivities() appends to the <select> without clearing existing options. This will accumulate duplicate activity options on each refresh. Clear/rebuild the select options (preserving the placeholder) before re-populating.
| const participantsList = details.participants | ||
| .map( | ||
| (p) => | ||
| `<li><span class="participant-email">${p}</span><button class="remove-btn" data-activity="${name}" data-email="${p}" title="Remove ${p}">✕</button></li>` |
There was a problem hiding this comment.
The remove control is rendered as an icon-only <button> and relies on title for labeling. For screen readers, add an aria-label (and consider type="button" to avoid accidental form submission if markup changes).
| `<li><span class="participant-email">${p}</span><button class="remove-btn" data-activity="${name}" data-email="${p}" title="Remove ${p}">✕</button></li>` | |
| `<li><span class="participant-email">${p}</span><button type="button" class="remove-btn" data-activity="${name}" data-email="${p}" aria-label="Remove ${p}" title="Remove ${p}">✕</button></li>` |
| # Get the specific activity | ||
| activity = activities[activity_name] | ||
|
|
||
| # Validate student is not already signed up |
There was a problem hiding this comment.
The "Validate student is not already signed up" comment is not indented with the rest of the function body. While it doesn’t break execution, it’s easy to misread as being outside the function—indent it to match surrounding code.
| # Validate student is not already signed up | |
| # Validate student is not already signed up |
| @app.delete("/activities/{activity_name}/signup") | ||
| def unregister_from_activity(activity_name: str, email: str): | ||
| """Unregister a student from an activity""" | ||
| if activity_name not in activities: | ||
| raise HTTPException(status_code=404, detail="Activity not found") | ||
|
|
||
| activity = activities[activity_name] | ||
|
|
||
| if email not in activity["participants"]: | ||
| raise HTTPException(status_code=400, detail="Student not signed up for this activity") |
There was a problem hiding this comment.
email is accepted as an unconstrained str and stored/echoed back to the UI where it’s displayed. To prevent malicious payloads and reduce inconsistent duplicates, validate/normalize it at the API boundary (e.g., strip whitespace, enforce email format and length; optionally enforce the school domain).
| .remove-btn:hover { | ||
| background-color: #ffcdd2; | ||
| } | ||
|
|
There was a problem hiding this comment.
.remove-btn adds custom styling for an interactive control but only defines a hover state. Add a visible :focus-visible style (e.g., outline/background) so keyboard users can see which remove button is focused.
| .remove-btn:focus-visible { | |
| background-color: #ffcdd2; | |
| outline: 2px solid #c62828; | |
| outline-offset: 2px; | |
| } |
| response = client.post( | ||
| f"/activities/{activity_name}/signup", | ||
| params={"email": email}, | ||
| ) |
There was a problem hiding this comment.
The activity name contains spaces (e.g., "Chess Club"), but the test builds the path using the raw string. HTTP clients typically require path segments to be URL-encoded; otherwise this can raise an invalid URL error. Encode activity_name when interpolating into the URL (as the frontend does) so the test matches real requests.
This issue also appears in the following locations of the same file:
- line 76
- line 92
- line 110
- line 127
- line 143
This pull request adds several new features and improvements to the student activities signup application. The main changes include introducing the ability for students to unregister from activities, displaying and managing participant lists in the UI, expanding the set of available activities, enhancing the frontend user experience, and adding comprehensive backend tests.