-
Notifications
You must be signed in to change notification settings - Fork 0
Accelerate with copilot #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request adds comprehensive participant management functionality to the Mergington High School activities application, including the ability to unregister students, duplicate signup prevention, an expanded catalog of activities, improved UI for displaying participants, and a complete test suite.
- Added DELETE endpoint for unregistering students from activities with proper validation
- Implemented duplicate signup prevention to ensure students can't register twice for the same activity
- Expanded the activities catalog from 3 to 10 clubs and teams with detailed descriptions and schedules
- Enhanced the frontend UI to display participant lists with individual delete buttons for each student
- Created comprehensive test coverage for all API endpoints including edge cases and error scenarios
Reviewed Changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app.py | Added DELETE /activities/{activity_name}/unregister endpoint with validation, duplicate signup prevention logic, and 7 new activities (Basketball Team, Swimming Club, Art Club, Drama Club, Debate Team, Science Olympiad) |
| src/static/app.js | Implemented participant list display with delete functionality, including confirmation dialog and UI refresh after operations |
| src/static/styles.css | Added CSS styling for participant lists, delete buttons, and feedback messages to improve visual hierarchy and user experience |
| tests/test_app.py | Created comprehensive test suite with 15 test cases covering GET, POST, and DELETE endpoints, edge cases, and error handling scenarios |
| tests/init.py | Added package initialization file to mark tests directory as a Python package |
| requirements.txt | Added pytest and httpx dependencies for testing infrastructure |
| README.md | Minor formatting change (trailing whitespace on title line) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # 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") | ||
|
|
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The endpoints don't validate email format or check for maximum participants limit. While the signup endpoint should prevent exceeding max_participants, there's no validation to ensure the activity hasn't reached capacity before adding a new participant.
Consider adding validation:
# Check capacity
if len(activity["participants"]) >= activity["max_participants"]:
raise HTTPException(status_code=400, detail="Activity is at full capacity")| # Check capacity | |
| if len(activity["participants"]) >= activity["max_participants"]: | |
| raise HTTPException(status_code=400, detail="Activity is at full capacity") |
| @@ -1,4 +1,4 @@ | |||
| # Getting Started with GitHub Copilot | |||
| # Getting Started with GitHub Copilot | |||
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace detected at the end of line 1.
| # Getting Started with GitHub Copilot | |
| # Getting Started with GitHub Copilot |
| # FastAPI should handle this - either 422 for validation error or process it | ||
| assert response.status_code in [200, 422] |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test assertion assert response.status_code in [200, 422] is too permissive and doesn't test the actual behavior consistently. Empty email strings should either be validated (resulting in 422) or processed (resulting in 200), but the test should verify which behavior is expected rather than accepting both. This makes the test non-deterministic and doesn't properly validate the API behavior.
Consider either:
- Adding proper email validation to the API and expecting 422, or
- If empty emails are allowed, expect 200 and verify the behavior is correct
| # FastAPI should handle this - either 422 for validation error or process it | |
| assert response.status_code in [200, 422] | |
| # FastAPI should handle this - expect 422 for validation error | |
| assert response.status_code == 422 |
| const participantsList = details.participants.length > 0 | ||
| ? `<ul class="participants-list"> | ||
| ${details.participants.map(email => ` | ||
| <li> | ||
| <span class="participant-email">${email}</span> | ||
| <button class="delete-btn" data-activity="${name}" data-email="${email}" title="Unregister">🗑️</button> | ||
| </li> | ||
| `).join('')} | ||
| </ul>` | ||
| : `<p class="no-participants">No participants yet. Be the first to sign up!</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-section"> | ||
| <p class="participants-header"><strong>Participants:</strong></p> | ||
| ${participantsList} | ||
| </div> | ||
| `; | ||
|
|
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The email addresses and activity names are directly inserted into HTML without sanitization, which could lead to XSS vulnerabilities if malicious content is entered. Consider using textContent instead of template literals for user-provided data, or sanitize the input before rendering.
For example:
const li = document.createElement('li');
const emailSpan = document.createElement('span');
emailSpan.className = 'participant-email';
emailSpan.textContent = email; // Safe from XSS
const deleteBtn = document.createElement('button');
deleteBtn.className = 'delete-btn';
// ... set attributes safely
li.appendChild(emailSpan);
li.appendChild(deleteBtn);| const participantsList = details.participants.length > 0 | |
| ? `<ul class="participants-list"> | |
| ${details.participants.map(email => ` | |
| <li> | |
| <span class="participant-email">${email}</span> | |
| <button class="delete-btn" data-activity="${name}" data-email="${email}" title="Unregister">🗑️</button> | |
| </li> | |
| `).join('')} | |
| </ul>` | |
| : `<p class="no-participants">No participants yet. Be the first to sign up!</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-section"> | |
| <p class="participants-header"><strong>Participants:</strong></p> | |
| ${participantsList} | |
| </div> | |
| `; | |
| // Title | |
| const title = document.createElement("h4"); | |
| title.textContent = name; | |
| activityCard.appendChild(title); | |
| // Description | |
| const desc = document.createElement("p"); | |
| desc.textContent = details.description; | |
| activityCard.appendChild(desc); | |
| // Schedule | |
| const schedule = document.createElement("p"); | |
| const scheduleStrong = document.createElement("strong"); | |
| scheduleStrong.textContent = "Schedule:"; | |
| schedule.appendChild(scheduleStrong); | |
| schedule.appendChild(document.createTextNode(" " + details.schedule)); | |
| activityCard.appendChild(schedule); | |
| // Availability | |
| const avail = document.createElement("p"); | |
| const availStrong = document.createElement("strong"); | |
| availStrong.textContent = "Availability:"; | |
| avail.appendChild(availStrong); | |
| avail.appendChild(document.createTextNode(` ${spotsLeft} spots left`)); | |
| activityCard.appendChild(avail); | |
| // Participants section | |
| const participantsSection = document.createElement("div"); | |
| participantsSection.className = "participants-section"; | |
| const participantsHeader = document.createElement("p"); | |
| participantsHeader.className = "participants-header"; | |
| const headerStrong = document.createElement("strong"); | |
| headerStrong.textContent = "Participants:"; | |
| participantsHeader.appendChild(headerStrong); | |
| participantsSection.appendChild(participantsHeader); | |
| if (details.participants.length > 0) { | |
| const ul = document.createElement("ul"); | |
| ul.className = "participants-list"; | |
| 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 deleteBtn = document.createElement("button"); | |
| deleteBtn.className = "delete-btn"; | |
| deleteBtn.setAttribute("data-activity", name); | |
| deleteBtn.setAttribute("data-email", email); | |
| deleteBtn.setAttribute("title", "Unregister"); | |
| deleteBtn.textContent = "🗑️"; | |
| li.appendChild(deleteBtn); | |
| ul.appendChild(li); | |
| }); | |
| participantsSection.appendChild(ul); | |
| } else { | |
| const noParticipants = document.createElement("p"); | |
| noParticipants.className = "no-participants"; | |
| noParticipants.textContent = "No participants yet. Be the first to sign up!"; | |
| participantsSection.appendChild(noParticipants); | |
| } | |
| activityCard.appendChild(participantsSection); |
| transform: scale(1.2); | ||
| background-color: transparent; | ||
| } | ||
|
|
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The delete button lacks proper keyboard accessibility. The button element should have a visible focus indicator for keyboard navigation. Consider adding a :focus or :focus-visible style to make the button more accessible.
For example:
.delete-btn:focus-visible {
outline: 2px solid #1a237e;
outline-offset: 2px;
}| .delete-btn:focus-visible { | |
| outline: 2px solid #1a237e; | |
| outline-offset: 2px; | |
| } |
| @pytest.fixture(autouse=True) | ||
| def reset_activities(): | ||
| """Reset activities data before each test""" | ||
| # Store original state | ||
| original_activities = {} | ||
| for name, data in activities.items(): | ||
| original_activities[name] = { | ||
| "description": data["description"], | ||
| "schedule": data["schedule"], | ||
| "max_participants": data["max_participants"], | ||
| "participants": data["participants"].copy() | ||
| } | ||
|
|
||
| yield | ||
|
|
||
| # Restore original state | ||
| for name, data in original_activities.items(): | ||
| activities[name] = data |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reset_activities fixture has a potential issue with state restoration. After the test runs, it attempts to restore activities by directly assigning the saved dictionary, but this doesn't properly restore the mutable state since the test may have modified the activity dictionaries that were shallow-copied. The participants list is copied, but the assignment back doesn't ensure the original state is properly restored in the global activities dict.
Consider using deepcopy from the copy module or completely rebuilding the activities dict:
import copy
@pytest.fixture(autouse=True)
def reset_activities():
original_activities = copy.deepcopy(activities)
yield
activities.clear()
activities.update(original_activities)| const participantsList = details.participants.length > 0 | ||
| ? `<ul class="participants-list"> | ||
| ${details.participants.map(email => ` | ||
| <li> | ||
| <span class="participant-email">${email}</span> | ||
| <button class="delete-btn" data-activity="${name}" data-email="${email}" title="Unregister">🗑️</button> | ||
| </li> | ||
| `).join('')} | ||
| </ul>` | ||
| : `<p class="no-participants">No participants yet. Be the first to sign up!</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-section"> | ||
| <p class="participants-header"><strong>Participants:</strong></p> | ||
| ${participantsList} | ||
| </div> | ||
| `; | ||
|
|
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Activity name, description, and schedule are directly inserted into HTML without sanitization. While these values come from the backend, if the backend data is ever compromised or modified, this could lead to XSS vulnerabilities. Consider using DOM manipulation with textContent for better security:
const h4 = document.createElement('h4');
h4.textContent = name;
const descP = document.createElement('p');
descP.textContent = details.description;
// ... etc for other text content
activityCard.appendChild(h4);
activityCard.appendChild(descP);| const participantsList = details.participants.length > 0 | |
| ? `<ul class="participants-list"> | |
| ${details.participants.map(email => ` | |
| <li> | |
| <span class="participant-email">${email}</span> | |
| <button class="delete-btn" data-activity="${name}" data-email="${email}" title="Unregister">🗑️</button> | |
| </li> | |
| `).join('')} | |
| </ul>` | |
| : `<p class="no-participants">No participants yet. Be the first to sign up!</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-section"> | |
| <p class="participants-header"><strong>Participants:</strong></p> | |
| ${participantsList} | |
| </div> | |
| `; | |
| // Build activity card safely using DOM methods | |
| const h4 = document.createElement("h4"); | |
| h4.textContent = name; | |
| activityCard.appendChild(h4); | |
| const descP = document.createElement("p"); | |
| descP.textContent = details.description; | |
| activityCard.appendChild(descP); | |
| const scheduleP = document.createElement("p"); | |
| const scheduleStrong = document.createElement("strong"); | |
| scheduleStrong.textContent = "Schedule:"; | |
| scheduleP.appendChild(scheduleStrong); | |
| scheduleP.appendChild(document.createTextNode(" " + details.schedule)); | |
| activityCard.appendChild(scheduleP); | |
| 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); | |
| const participantsSection = document.createElement("div"); | |
| participantsSection.className = "participants-section"; | |
| const participantsHeader = document.createElement("p"); | |
| participantsHeader.className = "participants-header"; | |
| const participantsStrong = document.createElement("strong"); | |
| participantsStrong.textContent = "Participants:"; | |
| participantsHeader.appendChild(participantsStrong); | |
| participantsSection.appendChild(participantsHeader); | |
| if (details.participants.length > 0) { | |
| const ul = document.createElement("ul"); | |
| ul.className = "participants-list"; | |
| 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 deleteBtn = document.createElement("button"); | |
| deleteBtn.className = "delete-btn"; | |
| deleteBtn.setAttribute("data-activity", name); | |
| deleteBtn.setAttribute("data-email", email); | |
| deleteBtn.setAttribute("title", "Unregister"); | |
| deleteBtn.textContent = "🗑️"; | |
| li.appendChild(deleteBtn); | |
| ul.appendChild(li); | |
| }); | |
| participantsSection.appendChild(ul); | |
| } else { | |
| const noP = document.createElement("p"); | |
| noP.className = "no-participants"; | |
| noP.textContent = "No participants yet. Be the first to sign up!"; | |
| participantsSection.appendChild(noP); | |
| } | |
| activityCard.appendChild(participantsSection); |
Backend functionality and API enhancements:
DELETE /activities/{activity_name}/unregisterendpoint insrc/app.pyto allow students to be removed from activities, including validation for activity existence and participant status.src/app.pywith new clubs and teams, each with descriptions, schedules, and initial participants.Frontend improvements:
src/static/app.jsto show a list of participants for each activity, with an unregister (delete) button for each student.src/static/styles.cssfor the participant list, delete button, and feedback messages for improved user experience.Testing:
tests/test_app.pycovering all major API endpoints, edge cases, and error handling for activities and participant management.__init__.pyin thetestspackage to mark it as a Python package.This pull request introduces several new features and improvements to the Mergington High School activities management app, focusing on expanding activity options, enabling participant management, enhancing the frontend UI, and adding comprehensive automated tests. The key changes are grouped below by theme.