Add course waitlist functionality with position tracking and notifications #12
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nd API endpoints Co-authored-by: Hemavathi15sg <224925058+Hemavathi15sg@users.noreply.github.com>
…nt tracking Co-authored-by: Hemavathi15sg <224925058+Hemavathi15sg@users.noreply.github.com>
Co-authored-by: Hemavathi15sg <224925058+Hemavathi15sg@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Add course waitlist feature for full classes
Add course waitlist functionality with position tracking and notifications
Nov 23, 2025
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive waitlist feature for course registration, allowing students to join waitlists when courses reach maximum enrollment. The implementation adds position tracking, notification preferences, and admin management capabilities. However, several critical concurrency issues, security vulnerabilities, and design concerns need to be addressed before merging.
Key Changes:
- New
WaitlistEntrydomain entity with position tracking and notification preferences - Service layer with
WaitlistServicefor waitlist operations andNotificationServicefor student notifications - Nine new REST API endpoints for waitlist management (join, leave, query, admin operations)
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| WaitlistEntry.cs | New domain entity with student/course relationships and notification tracking |
| NotificationType.cs | Enum defining notification channels (Email, InApp, Both) |
| Course.cs | Added MaxEnrollment property (default 30) and IsFull computed property |
| Student.cs | Added WaitlistEntries navigation collection |
| IWaitlistRepository.cs | Repository interface for waitlist queries and position management |
| WaitlistRepository.cs | Implementation with active waitlist queries and auto-reordering |
| CourseRepository.cs | Added eager loading of Registrations and WaitlistEntries for computed properties |
| CourseRegistrationDbContext.cs | Configuration for WaitlistEntry entity with indexes and relationships |
| IUnitOfWork.cs & UnitOfWork.cs | Added Waitlists repository property |
| WaitlistService.cs | Core business logic for joining, leaving, and managing waitlists |
| NotificationService.cs | Abstraction for email/in-app notifications (currently logs only) |
| RegistrationService.cs | Modified to check course capacity and trigger waitlist notifications on cancellation |
| WaitlistDtos.cs | DTOs for creating, updating, and returning waitlist entries |
| CourseDtos.cs | Added MaxEnrollment, IsFull, and WaitlistCount properties |
| MappingProfile.cs | AutoMapper configurations for waitlist entities and updated course mappings |
| IWaitlistService.cs & INotificationService.cs | Service interfaces for waitlist and notification operations |
| WaitlistController.cs | Nine REST endpoints for waitlist CRUD and admin operations |
| Program.cs | DI registration for new repositories and services |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
Comment on lines
+17
to
+26
| private readonly IWaitlistService _waitlistService; | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the RegistrationService | ||
| /// </summary> | ||
| public RegistrationService(IUnitOfWork unitOfWork, IMapper mapper) | ||
| public RegistrationService(IUnitOfWork unitOfWork, IMapper mapper, IWaitlistService waitlistService) | ||
| { | ||
| _unitOfWork = unitOfWork ?? throw new ArgumentNullException(nameof(unitOfWork)); | ||
| _mapper = mapper ?? throw new ArgumentNullException(nameof(mapper)); | ||
| _waitlistService = waitlistService ?? throw new ArgumentNullException(nameof(waitlistService)); |
There was a problem hiding this comment.
Circular dependency detected: RegistrationService depends on IWaitlistService, but WaitlistService (through its operations) may need to check registration status through the repository. This creates a potential circular dependency pattern. Consider:
- Moving the
NotifyNextStudentAsync()call to a domain event pattern whereRegistrationServicepublishes aRegistrationCancelledevent - Having a separate orchestration service that coordinates between registrations and waitlists
- Using the repository directly in
RegistrationServiceinstead of injectingIWaitlistService
The current implementation works because WaitlistService doesn't depend on IRegistrationService, but this tight coupling makes the services harder to maintain and test independently.
Comment on lines
+118
to
+124
| // Check if course is full | ||
| if (course.IsFull) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"Course is full (max enrollment: {course.MaxEnrollment}). " + | ||
| "Please join the waitlist instead."); | ||
| } |
There was a problem hiding this comment.
Race condition risk: Between checking course.IsFull (line 119) and creating the registration (lines 126-131), another concurrent request could register for the same course, causing enrollment to exceed MaxEnrollment. The IsFull check relies on a computed property that queries the Registrations collection loaded earlier, but doesn't use database-level constraints or locking.
Consider:
- Adding a unique constraint at the database level that enforces max enrollment
- Using optimistic concurrency with a version field on
Course - Wrapping the check and insert in a transaction with appropriate isolation level
- Using a database-level check constraint or trigger to enforce the limit
Comment on lines
+73
to
+82
| var nextPosition = await _unitOfWork.Waitlists.GetNextPositionAsync(createWaitlistEntryDto.CourseId); | ||
|
|
||
| // Create waitlist entry | ||
| var waitlistEntry = _mapper.Map<WaitlistEntry>(createWaitlistEntryDto); | ||
| waitlistEntry.Position = nextPosition; | ||
| waitlistEntry.JoinedAt = DateTime.UtcNow; | ||
| waitlistEntry.IsActive = true; | ||
|
|
||
| await _unitOfWork.Waitlists.AddAsync(waitlistEntry); | ||
| await _unitOfWork.SaveChangesAsync(); |
There was a problem hiding this comment.
Race condition in position assignment: Between calling GetNextPositionAsync() (line 73) and saving the entry (line 82), concurrent requests could receive the same position number. The repository method queries active entries and calculates Max(Position) + 1, but without locking or a database sequence, multiple simultaneous requests will get identical positions.
Solutions:
- Use a database sequence or identity column for position
- Implement row-level locking when reading the max position
- Use optimistic concurrency and retry logic
- Assign positions in a database transaction with serializable isolation
Comment on lines
+110
to
+111
| .Include(c => c.Registrations) | ||
| .Include(c => c.WaitlistEntries) |
There was a problem hiding this comment.
Eager loading added unconditionally: These changes force eager loading of Registrations and WaitlistEntries for every course query, even when the caller doesn't need enrollment information. This can significantly impact performance, especially for list operations.
Consider:
- Creating separate methods like
GetByIdWithEnrollmentAsync()for when this data is needed - Using projection (
.Select()) to load only required fields - Allowing callers to specify includes via a parameter
- Loading these collections only when specifically needed by business logic
Comment on lines
+163
to
+198
| // Handle position change if provided (admin reordering) | ||
| if (updateDto.Position.HasValue && updateDto.Position.Value != existingEntry.Position) | ||
| { | ||
| var oldPosition = existingEntry.Position; | ||
| var newPosition = updateDto.Position.Value; | ||
|
|
||
| // Get all active waitlist entries for the course | ||
| var allEntries = await _unitOfWork.Waitlists.GetActiveWaitlistForCourseAsync(existingEntry.CourseId); | ||
| var entriesList = allEntries.ToList(); | ||
|
|
||
| // Validate new position | ||
| if (newPosition < 1 || newPosition > entriesList.Count) | ||
| { | ||
| throw new InvalidOperationException("Invalid position specified."); | ||
| } | ||
|
|
||
| // Reorder entries | ||
| if (newPosition < oldPosition) | ||
| { | ||
| // Moving up in the list | ||
| foreach (var entry in entriesList.Where(e => e.Position >= newPosition && e.Position < oldPosition)) | ||
| { | ||
| entry.Position++; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| // Moving down in the list | ||
| foreach (var entry in entriesList.Where(e => e.Position > oldPosition && e.Position <= newPosition)) | ||
| { | ||
| entry.Position--; | ||
| } | ||
| } | ||
|
|
||
| existingEntry.Position = newPosition; | ||
| } |
There was a problem hiding this comment.
Unsafe admin reordering operation: The single-entry position update (lines 163-198) doesn't prevent creating duplicate positions or gaps in the sequence. While it attempts to shift other entries, if the update fails partway through (e.g., exception after updating some entries), the waitlist will be in an inconsistent state.
Issues:
- No transaction boundary wrapping the multi-entry update
- The logic only shifts entries between old and new positions, but doesn't validate final state
- If multiple admins reorder simultaneously, race conditions could corrupt positions
Consider:
- Wrapping the entire operation in an explicit transaction
- Using the bulk
ReorderWaitlistAsync()method (line 250) instead - Adding validation after updates to ensure positions are sequential and unique
Comment on lines
+37
to
+39
| .ForMember(dest => dest.CurrentEnrollment, opt => opt.MapFrom(src => src.CurrentEnrollment)) | ||
| .ForMember(dest => dest.IsFull, opt => opt.MapFrom(src => src.IsFull)) | ||
| .ForMember(dest => dest.WaitlistCount, opt => opt.MapFrom(src => src.WaitlistEntries.Count(w => w.IsActive))); |
There was a problem hiding this comment.
N+1 query problem: The WaitlistCount mapping executes src.WaitlistEntries.Count(w => w.IsActive) on the already-loaded collection. However, if WaitlistEntries is not eagerly loaded (which could happen in some query paths), this will trigger a separate database query for each course when mapping multiple courses.
While lines 110-111 and 125-126 in CourseRepository.cs do add eager loading, this mapping should be defensive. Consider either:
- Always ensuring
WaitlistEntriesis loaded before mapping - Using a database-level projection that counts in SQL
- Adding a computed column or cached count field
Comment on lines
+79
to
+82
| /// <summary> | ||
| /// Maximum number of students allowed | ||
| /// </summary> | ||
| public int MaxEnrollment { get; set; } = 30; |
There was a problem hiding this comment.
Missing validation: The MaxEnrollment field in UpdateCourseDto also lacks validation. Additionally, when updating MaxEnrollment on an existing course, there's no check to prevent setting it below the current enrollment count, which would create an inconsistent state where a course is "over capacity."
Add:
- Range validation attribute as with
CreateCourseDto - Business logic in
CourseService.UpdateCourseAsync()to validate:if (updateDto.MaxEnrollment < course.CurrentEnrollment) throw new InvalidOperationException( $"Cannot set max enrollment below current enrollment ({course.CurrentEnrollment})");
Comment on lines
+30
to
+35
| var body = $"Dear {studentName},\n\n" + | ||
| $"A spot has become available in the course '{courseName}'.\n" + | ||
| $"You are currently in position {position} on the waitlist.\n\n" + | ||
| $"Please log in to your account to register for the course.\n\n" + | ||
| $"Best regards,\n" + | ||
| $"Course Registration System"; |
There was a problem hiding this comment.
Misleading notification message: The notification sent to the waitlist student states "You are currently in position {position} on the waitlist" where position is their position before anyone registered. However, the message says "A spot has become available", implying they should be able to register immediately.
If position is 1, the message makes sense. But if position is 5, the message is confusing—a spot opened but they're still in position 5.
Consider:
- Only notifying position 1 when a spot opens
- Clarifying the message: "A spot may become available soon. You are currently in position {position}."
- Sending different messages based on position
Suggested change
| var body = $"Dear {studentName},\n\n" + | |
| $"A spot has become available in the course '{courseName}'.\n" + | |
| $"You are currently in position {position} on the waitlist.\n\n" + | |
| $"Please log in to your account to register for the course.\n\n" + | |
| $"Best regards,\n" + | |
| $"Course Registration System"; | |
| string body; | |
| if (position == 1) | |
| { | |
| body = $"Dear {studentName},\n\n" + | |
| $"A spot has become available in the course '{courseName}'.\n" + | |
| $"You are first in line on the waitlist and may now register for the course.\n\n" + | |
| $"Please log in to your account to register for the course.\n\n" + | |
| $"Best regards,\n" + | |
| $"Course Registration System"; | |
| } | |
| else | |
| { | |
| body = $"Dear {studentName},\n\n" + | |
| $"A spot may become available soon in the course '{courseName}'.\n" + | |
| $"You are currently in position {position} on the waitlist.\n\n" + | |
| $"We will notify you when you are eligible to register.\n\n" + | |
| $"Best regards,\n" + | |
| $"Course Registration System"; | |
| } |
Comment on lines
+1
to
+287
| using Microsoft.AspNetCore.Mvc; | ||
| using CourseRegistration.Application.DTOs; | ||
| using CourseRegistration.Application.Interfaces; | ||
|
|
||
| namespace CourseRegistration.API.Controllers; | ||
|
|
||
| /// <summary> | ||
| /// Controller for waitlist management operations | ||
| /// </summary> | ||
| [ApiController] | ||
| [Route("api/[controller]")] | ||
| [Produces("application/json")] | ||
| public class WaitlistController : ControllerBase | ||
| { | ||
| private readonly IWaitlistService _waitlistService; | ||
| private readonly ILogger<WaitlistController> _logger; | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the WaitlistController | ||
| /// </summary> | ||
| public WaitlistController(IWaitlistService waitlistService, ILogger<WaitlistController> logger) | ||
| { | ||
| _waitlistService = waitlistService ?? throw new ArgumentNullException(nameof(waitlistService)); | ||
| _logger = logger ?? throw new ArgumentNullException(nameof(logger)); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Adds a student to a course waitlist | ||
| /// </summary> | ||
| /// <param name="createWaitlistEntryDto">Waitlist entry creation data</param> | ||
| /// <returns>Created waitlist entry</returns> | ||
| [HttpPost] | ||
| [ProducesResponseType(typeof(ApiResponseDto<WaitlistEntryDto>), StatusCodes.Status201Created)] | ||
| [ProducesResponseType(typeof(ApiResponseDto<object>), StatusCodes.Status400BadRequest)] | ||
| public async Task<ActionResult<ApiResponseDto<WaitlistEntryDto>>> JoinWaitlist([FromBody] CreateWaitlistEntryDto createWaitlistEntryDto) | ||
| { | ||
| _logger.LogInformation("Student {StudentId} attempting to join waitlist for course {CourseId}", | ||
| createWaitlistEntryDto.StudentId, createWaitlistEntryDto.CourseId); | ||
|
|
||
| try | ||
| { | ||
| var waitlistEntry = await _waitlistService.JoinWaitlistAsync(createWaitlistEntryDto); | ||
| return CreatedAtAction( | ||
| nameof(GetWaitlistEntry), | ||
| new { id = waitlistEntry.WaitlistEntryId }, | ||
| new ApiResponseDto<WaitlistEntryDto> | ||
| { | ||
| Success = true, | ||
| Message = $"Successfully joined waitlist at position {waitlistEntry.Position}", | ||
| Data = waitlistEntry | ||
| }); | ||
| } | ||
| catch (InvalidOperationException ex) | ||
| { | ||
| _logger.LogWarning("Failed to join waitlist: {Message}", ex.Message); | ||
| return BadRequest(new ApiResponseDto<object> | ||
| { | ||
| Success = false, | ||
| Message = ex.Message | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets a specific waitlist entry by ID | ||
| /// </summary> | ||
| /// <param name="id">Waitlist entry ID</param> | ||
| /// <returns>Waitlist entry details</returns> | ||
| [HttpGet("{id}")] | ||
| [ProducesResponseType(typeof(ApiResponseDto<WaitlistEntryDto>), StatusCodes.Status200OK)] | ||
| [ProducesResponseType(typeof(ApiResponseDto<object>), StatusCodes.Status404NotFound)] | ||
| public async Task<ActionResult<ApiResponseDto<WaitlistEntryDto>>> GetWaitlistEntry(Guid id) | ||
| { | ||
| _logger.LogInformation("Getting waitlist entry {Id}", id); | ||
|
|
||
| var waitlistEntry = await _waitlistService.GetWaitlistEntryAsync(id); | ||
|
|
||
| if (waitlistEntry == null) | ||
| { | ||
| return NotFound(new ApiResponseDto<object> | ||
| { | ||
| Success = false, | ||
| Message = "Waitlist entry not found" | ||
| }); | ||
| } | ||
|
|
||
| return Ok(new ApiResponseDto<WaitlistEntryDto> | ||
| { | ||
| Success = true, | ||
| Data = waitlistEntry | ||
| }); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets active waitlist entries for a specific course | ||
| /// </summary> | ||
| /// <param name="courseId">Course ID</param> | ||
| /// <returns>List of waitlist entries for the course</returns> | ||
| [HttpGet("course/{courseId}")] | ||
| [ProducesResponseType(typeof(ApiResponseDto<IEnumerable<WaitlistEntryDto>>), StatusCodes.Status200OK)] | ||
| public async Task<ActionResult<ApiResponseDto<IEnumerable<WaitlistEntryDto>>>> GetCourseWaitlist(Guid courseId) | ||
| { | ||
| _logger.LogInformation("Getting waitlist for course {CourseId}", courseId); | ||
|
|
||
| var waitlistEntries = await _waitlistService.GetCourseWaitlistAsync(courseId); | ||
|
|
||
| return Ok(new ApiResponseDto<IEnumerable<WaitlistEntryDto>> | ||
| { | ||
| Success = true, | ||
| Data = waitlistEntries, | ||
| Message = $"Found {waitlistEntries.Count()} entries in waitlist" | ||
| }); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets active waitlist entries for a specific student | ||
| /// </summary> | ||
| /// <param name="studentId">Student ID</param> | ||
| /// <returns>List of waitlist entries for the student</returns> | ||
| [HttpGet("student/{studentId}")] | ||
| [ProducesResponseType(typeof(ApiResponseDto<IEnumerable<WaitlistEntryDto>>), StatusCodes.Status200OK)] | ||
| public async Task<ActionResult<ApiResponseDto<IEnumerable<WaitlistEntryDto>>>> GetStudentWaitlists(Guid studentId) | ||
| { | ||
| _logger.LogInformation("Getting waitlists for student {StudentId}", studentId); | ||
|
|
||
| var waitlistEntries = await _waitlistService.GetStudentWaitlistsAsync(studentId); | ||
|
|
||
| return Ok(new ApiResponseDto<IEnumerable<WaitlistEntryDto>> | ||
| { | ||
| Success = true, | ||
| Data = waitlistEntries, | ||
| Message = $"Student is on {waitlistEntries.Count()} waitlist(s)" | ||
| }); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Removes a student from a waitlist | ||
| /// </summary> | ||
| /// <param name="id">Waitlist entry ID</param> | ||
| /// <returns>Success status</returns> | ||
| [HttpDelete("{id}")] | ||
| [ProducesResponseType(typeof(ApiResponseDto<object>), StatusCodes.Status200OK)] | ||
| [ProducesResponseType(typeof(ApiResponseDto<object>), StatusCodes.Status404NotFound)] | ||
| public async Task<ActionResult<ApiResponseDto<object>>> LeaveWaitlist(Guid id) | ||
| { | ||
| _logger.LogInformation("Attempting to remove waitlist entry {Id}", id); | ||
|
|
||
| var result = await _waitlistService.LeaveWaitlistAsync(id); | ||
|
|
||
| if (!result) | ||
| { | ||
| return NotFound(new ApiResponseDto<object> | ||
| { | ||
| Success = false, | ||
| Message = "Waitlist entry not found or already inactive" | ||
| }); | ||
| } | ||
|
|
||
| return Ok(new ApiResponseDto<object> | ||
| { | ||
| Success = true, | ||
| Message = "Successfully left the waitlist" | ||
| }); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Updates a waitlist entry (admin function) | ||
| /// </summary> | ||
| /// <param name="id">Waitlist entry ID</param> | ||
| /// <param name="updateDto">Update data</param> | ||
| /// <returns>Updated waitlist entry</returns> | ||
| [HttpPatch("{id}")] | ||
| [ProducesResponseType(typeof(ApiResponseDto<WaitlistEntryDto>), StatusCodes.Status200OK)] | ||
| [ProducesResponseType(typeof(ApiResponseDto<object>), StatusCodes.Status404NotFound)] | ||
| [ProducesResponseType(typeof(ApiResponseDto<object>), StatusCodes.Status400BadRequest)] | ||
| public async Task<ActionResult<ApiResponseDto<WaitlistEntryDto>>> UpdateWaitlistEntry( | ||
| Guid id, | ||
| [FromBody] UpdateWaitlistEntryDto updateDto) | ||
| { | ||
| _logger.LogInformation("Updating waitlist entry {Id}", id); | ||
|
|
||
| try | ||
| { | ||
| var updatedEntry = await _waitlistService.UpdateWaitlistEntryAsync(id, updateDto); | ||
|
|
||
| if (updatedEntry == null) | ||
| { | ||
| return NotFound(new ApiResponseDto<object> | ||
| { | ||
| Success = false, | ||
| Message = "Waitlist entry not found" | ||
| }); | ||
| } | ||
|
|
||
| return Ok(new ApiResponseDto<WaitlistEntryDto> | ||
| { | ||
| Success = true, | ||
| Message = "Waitlist entry updated successfully", | ||
| Data = updatedEntry | ||
| }); | ||
| } | ||
| catch (InvalidOperationException ex) | ||
| { | ||
| _logger.LogWarning("Failed to update waitlist entry: {Message}", ex.Message); | ||
| return BadRequest(new ApiResponseDto<object> | ||
| { | ||
| Success = false, | ||
| Message = ex.Message | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Clears the entire waitlist for a course (admin function) | ||
| /// </summary> | ||
| /// <param name="courseId">Course ID</param> | ||
| /// <returns>Success status</returns> | ||
| [HttpDelete("course/{courseId}/clear")] | ||
| [ProducesResponseType(typeof(ApiResponseDto<object>), StatusCodes.Status200OK)] | ||
| public async Task<ActionResult<ApiResponseDto<object>>> ClearWaitlist(Guid courseId) | ||
| { | ||
| _logger.LogInformation("Clearing waitlist for course {CourseId}", courseId); | ||
|
|
||
| await _waitlistService.ClearWaitlistAsync(courseId); | ||
|
|
||
| return Ok(new ApiResponseDto<object> | ||
| { | ||
| Success = true, | ||
| Message = "Waitlist cleared successfully" | ||
| }); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Notifies the next student on the waitlist (admin/system function) | ||
| /// </summary> | ||
| /// <param name="courseId">Course ID</param> | ||
| /// <returns>Success status</returns> | ||
| [HttpPost("course/{courseId}/notify-next")] | ||
| [ProducesResponseType(typeof(ApiResponseDto<object>), StatusCodes.Status200OK)] | ||
| public async Task<ActionResult<ApiResponseDto<object>>> NotifyNextStudent(Guid courseId) | ||
| { | ||
| _logger.LogInformation("Notifying next student on waitlist for course {CourseId}", courseId); | ||
|
|
||
| await _waitlistService.NotifyNextStudentAsync(courseId); | ||
|
|
||
| return Ok(new ApiResponseDto<object> | ||
| { | ||
| Success = true, | ||
| Message = "Next student notified successfully" | ||
| }); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Reorders waitlist entries for a course (admin function) | ||
| /// </summary> | ||
| /// <param name="courseId">Course ID</param> | ||
| /// <param name="newPositions">Dictionary of waitlist entry IDs and their new positions</param> | ||
| /// <returns>Success status</returns> | ||
| [HttpPut("course/{courseId}/reorder")] | ||
| [ProducesResponseType(typeof(ApiResponseDto<object>), StatusCodes.Status200OK)] | ||
| [ProducesResponseType(typeof(ApiResponseDto<object>), StatusCodes.Status400BadRequest)] | ||
| public async Task<ActionResult<ApiResponseDto<object>>> ReorderWaitlist( | ||
| Guid courseId, | ||
| [FromBody] Dictionary<Guid, int> newPositions) | ||
| { | ||
| _logger.LogInformation("Reordering waitlist for course {CourseId}", courseId); | ||
|
|
||
| try | ||
| { | ||
| await _waitlistService.ReorderWaitlistAsync(courseId, newPositions); | ||
|
|
||
| return Ok(new ApiResponseDto<object> | ||
| { | ||
| Success = true, | ||
| Message = "Waitlist reordered successfully" | ||
| }); | ||
| } | ||
| catch (InvalidOperationException ex) | ||
| { | ||
| _logger.LogWarning("Failed to reorder waitlist: {Message}", ex.Message); | ||
| return BadRequest(new ApiResponseDto<object> | ||
| { | ||
| Success = false, | ||
| Message = ex.Message | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing authorization: All waitlist endpoints lack authorization checks. Critical operations that should be restricted:
Admin-only operations (no authorization implemented):
ClearWaitlist()- line 220: Any user can clear entire waitlistsReorderWaitlist()- line 262: Any user can manipulate positionsNotifyNextStudent()- line 240: Could be abused to spam notificationsUpdateWaitlistEntry()- line 176: Should only allow admins to change positions, or students to update their own preferences
Student-owned operations (no ownership verification):
LeaveWaitlist()- line 144: A student could remove another student's entryUpdateWaitlistEntry()- line 176: Should verify the student owns the entry
Add role-based authorization attributes like [Authorize(Roles = "Admin")] and implement ownership checks in the service layer.
Comment on lines
+99
to
+113
| [HttpGet("course/{courseId}")] | ||
| [ProducesResponseType(typeof(ApiResponseDto<IEnumerable<WaitlistEntryDto>>), StatusCodes.Status200OK)] | ||
| public async Task<ActionResult<ApiResponseDto<IEnumerable<WaitlistEntryDto>>>> GetCourseWaitlist(Guid courseId) | ||
| { | ||
| _logger.LogInformation("Getting waitlist for course {CourseId}", courseId); | ||
|
|
||
| var waitlistEntries = await _waitlistService.GetCourseWaitlistAsync(courseId); | ||
|
|
||
| return Ok(new ApiResponseDto<IEnumerable<WaitlistEntryDto>> | ||
| { | ||
| Success = true, | ||
| Data = waitlistEntries, | ||
| Message = $"Found {waitlistEntries.Count()} entries in waitlist" | ||
| }); | ||
| } |
There was a problem hiding this comment.
Potential data exposure: The GetCourseWaitlist() endpoint returns all waitlist entries including student email addresses and personal information to any caller. This violates privacy principles—students on a waitlist shouldn't necessarily see other students' email addresses.
Consider:
- Adding authorization to restrict this endpoint to admins only
- Creating a separate public endpoint that returns only aggregated data (e.g., total count, student's own position)
- Removing sensitive fields like email from the response for non-admin users
- Having students query their own position via
GetStudentWaitlists()instead
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements waitlist management for courses at max enrollment. Students join waitlists with position tracking and receive notifications when spots open.
Domain Changes
MaxEnrollment(default 30) andIsFullcomputed property toCourseWaitlistEntryentity withPosition,NotificationPreference, andNotifiedAttrackingNotificationTypeenum: Email, InApp, BothApplication Layer
WaitlistService: Join/leave waitlist, position management, admin reorderingNotificationService: Email/in-app notification abstraction (currently logs)RegistrationService.CancelRegistrationAsync()to triggerNotifyNextStudentAsync()on confirmed cancellationscourse.IsFulland throws with waitlist suggestionInfrastructure
WaitlistRepository: Active waitlist queries, position calculation, auto-reordering on removalCourseRepository.GetByIdAsync()andGetPagedAsync()to eager-loadRegistrationsandWaitlistEntriesfor computed propertiesAPI
Nine new endpoints on
/api/waitlist:/- Join waitlist/{id},/course/{courseId},/student/{studentId}- Read operations/{id}- Leave waitlist (auto-reorders positions)/{id}- Update preferences/notes/course/{courseId}/clear, PUT/course/{courseId}/reorder, POST/course/{courseId}/notify-nextExample Usage
Validations
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.