003 event admin UI - #2
Conversation
- Created migration to add TimeSlot and related entities (event_series, time_slots, tracks) to the database schema. - Implemented TimeSlot entity with properties such as Id, EventId, Name, StartTime, EndTime, Type, IsEventLevel, CreatedAt, and UpdatedAt. - Added IScheduleService interface with method to copy day schedule for events. - Developed ScheduleService class to handle the logic for copying time slots from one date to another within the same event. - Removed obsolete columns from events table and updated foreign key relationships.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 152 out of 166 changed files in this pull request and generated 5 comments.
Files not reviewed (5)
- src/PrivateApp/package-lock.json: Language not supported
- src/PublicApp/package-lock.json: Language not supported
- src/Shared/EventManagement/Data/Migrations/20260104185946_AddFoundationalEntities.Designer.cs: Language not supported
- src/Shared/EventManagement/Data/Migrations/20260105113600_AddTimeSlot.Designer.cs: Language not supported
- src/Shared/EventManagement/Data/Migrations/20260105115202_AddSessionAndAssignment.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Injectable({ | ||
| providedIn: 'root', | ||
| }) | ||
| export class Event { |
There was a problem hiding this comment.
The class name 'Event' conflicts with the built-in DOM Event class and the Event interface defined in event.model.ts. Rename this service class to avoid confusion (e.g., 'EventService' or remove this file if it's not needed).
| export class Event { | |
| export class EventService { |
| Published = 1, | ||
| Cancelled = 2, | ||
| Completed = 3, | ||
| Postponed = 4 |
There was a problem hiding this comment.
EventStatus enum values don't match the backend C# enum (which includes Active and Archived). This mismatch will cause runtime errors. Ensure the TypeScript enum matches the backend definition exactly.
| Published = 1, | |
| Cancelled = 2, | |
| Completed = 3, | |
| Postponed = 4 | |
| Active = 1, | |
| Archived = 2, | |
| Cancelled = 3, | |
| Completed = 4, | |
| Postponed = 5 |
antonrademaker
left a comment
antonrademaker
left a comment
There was a problem hiding this comment.
I have addressed the latest review comments:
- Duplicate CSS: Removed duplicate rules in
schedule-grid.component.scss. - Unused File: Deleted
src/PrivateApp/src/app/event-management/services/event.ts. - Spec Import: Fixed
EventList->EventListComponentimport/usage inevent-list.spec.ts. - Enum Sync: Aligned
EventStatusinevent.model.tswith the C# backend (Active,Archivedadded;Postponedremoved). - Log Injection: Sanitized the
Slugparameter inEventService.cslogging.
Regarding Session.cs and [Table(..., Schema = ...)]: The property Schema in C#'s TableAttribute is PascalCase. Changing it to lowercase would cause a compilation error. I have left it as is.
antonrademaker
left a comment
antonrademaker
left a comment
There was a problem hiding this comment.
I have also reinforced the log sanitization in SearchEventsAsync to explicitly replace \r and \n instead of relying on Environment.NewLine, ensuring stricter security compliance across all logging points.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 151 out of 165 changed files in this pull request and generated 3 comments.
Files not reviewed (5)
- src/PrivateApp/package-lock.json: Language not supported
- src/PublicApp/package-lock.json: Language not supported
- src/Shared/EventManagement/Data/Migrations/20260104185946_AddFoundationalEntities.Designer.cs: Language not supported
- src/Shared/EventManagement/Data/Migrations/20260105113600_AddTimeSlot.Designer.cs: Language not supported
- src/Shared/EventManagement/Data/Migrations/20260105115202_AddSessionAndAssignment.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case EventStatus.Published: return 'Published'; | ||
| case EventStatus.Cancelled: return 'Cancelled'; | ||
| case EventStatus.Completed: return 'Completed'; | ||
| case EventStatus.Postponed: return 'Postponed'; |
There was a problem hiding this comment.
The EventStatus.Postponed case is handled but this status was removed from the EventStatus enum (as stated in the PR description). This will cause a compilation error since Postponed is no longer a valid enum value.
| case EventStatus.Postponed: return 'Postponed'; |
| 'bg-blue-200 text-blue-800': event.status === EventStatus.Completed, | ||
| 'bg-yellow-200 text-yellow-800': event.status === EventStatus.Postponed |
There was a problem hiding this comment.
Reference to EventStatus.Postponed should be removed as this status no longer exists in the EventStatus enum.
| 'bg-blue-200 text-blue-800': event.status === EventStatus.Completed, | |
| 'bg-yellow-200 text-yellow-800': event.status === EventStatus.Postponed | |
| 'bg-blue-200 text-blue-800': event.status === EventStatus.Completed |
| import { TestBed } from '@angular/core/testing'; | ||
|
|
||
| import { Event } from './event'; | ||
|
|
||
| describe('Event', () => { | ||
| let service: Event; | ||
|
|
||
| beforeEach(() => { | ||
| TestBed.configureTestingModule({}); | ||
| service = TestBed.inject(Event); | ||
| }); | ||
|
|
||
| it('should be created', () => { | ||
| expect(service).toBeTruthy(); | ||
| }); |
There was a problem hiding this comment.
This test imports from './event' which doesn't exist. The description mentions deleting 'src/PrivateApp/src/app/event-management/services/event.ts', but this test file still references it. Should import from '../models/event.model' instead or the test file should be deleted.
| import { TestBed } from '@angular/core/testing'; | |
| import { Event } from './event'; | |
| describe('Event', () => { | |
| let service: Event; | |
| beforeEach(() => { | |
| TestBed.configureTestingModule({}); | |
| service = TestBed.inject(Event); | |
| }); | |
| it('should be created', () => { | |
| expect(service).toBeTruthy(); | |
| }); | |
| import { Event } from '../models/event.model'; | |
| describe('Event', () => { | |
| it('should create an instance', () => { | |
| expect(new Event()).toBeTruthy(); | |
| }); |
antonrademaker
left a comment
antonrademaker
left a comment
There was a problem hiding this comment.
I have addressed recent review comments:
- Removed
EventStatus.Postponedusage inevent-list.tsandevent-list.html. - Deleted
event.spec.tsas the corresponding service was deleted.
The build checks should pass now.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 151 out of 165 changed files in this pull request and generated 4 comments.
Files not reviewed (5)
- src/PrivateApp/package-lock.json: Language not supported
- src/PublicApp/package-lock.json: Language not supported
- src/Shared/EventManagement/Data/Migrations/20260104185946_AddFoundationalEntities.Designer.cs: Language not supported
- src/Shared/EventManagement/Data/Migrations/20260105113600_AddTimeSlot.Designer.cs: Language not supported
- src/Shared/EventManagement/Data/Migrations/20260105115202_AddSessionAndAssignment.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div *cdkDragPreview class="session-preview"> | ||
| {{ session.title }} | ||
| </div> |
There was a problem hiding this comment.
The *cdkDragPreview directive is being used, but Angular CDK recommends using the structural directive syntax <ng-template cdkDragPreview> instead of *cdkDragPreview on a regular div. The current approach may not work as expected.
| <div *cdkDragPreview class="session-preview"> | |
| {{ session.title }} | |
| </div> | |
| <ng-template cdkDragPreview> | |
| <div class="session-preview"> | |
| {{ session.title }} | |
| </div> | |
| </ng-template> |
|
|
||
| forkJoin({ | ||
| slots: this.timeSlotService.getTimeSlots(this.eventId), | ||
| tracks: this.trackService.getTracks(this.eventId), |
There was a problem hiding this comment.
The eventId is checked for null before calling loadData, but TypeScript's strict null checking means this.eventId could still be null when passed to the services. Use the non-null assertion operator (this.eventId!) or proper null handling.
| private snackBar: MatSnackBar | ||
| ) { } | ||
|
|
||
| handleDrop(event: CdkDragDrop<Session[] | any>, eventId: string, trackId?: string, timeSlotId?: string): void { |
There was a problem hiding this comment.
Using any type for the generic parameter defeats the purpose of TypeScript's type safety. Consider defining a proper type for the drop container data.
| var firstAssignment = await _context.Set<SessionAssignment>() | ||
| .FirstOrDefaultAsync(a => a.SessionId == firstSessionId && a.Session.EventId == eventId, cancellationToken); | ||
|
|
||
| var secondAssignment = await _context.Set<SessionAssignment>() | ||
| .FirstOrDefaultAsync(a => a.SessionId == secondSessionId && a.Session.EventId == eventId, cancellationToken); |
There was a problem hiding this comment.
This performs two separate database queries for the session assignments. Consider using a single query with a Where clause that filters both sessions to reduce database round trips.
| var firstAssignment = await _context.Set<SessionAssignment>() | |
| .FirstOrDefaultAsync(a => a.SessionId == firstSessionId && a.Session.EventId == eventId, cancellationToken); | |
| var secondAssignment = await _context.Set<SessionAssignment>() | |
| .FirstOrDefaultAsync(a => a.SessionId == secondSessionId && a.Session.EventId == eventId, cancellationToken); | |
| var assignments = await _context.Set<SessionAssignment>() | |
| .Where(a => | |
| a.Session.EventId == eventId && | |
| (a.SessionId == firstSessionId || a.SessionId == secondSessionId)) | |
| .ToListAsync(cancellationToken); | |
| var firstAssignment = assignments.FirstOrDefault(a => a.SessionId == firstSessionId); | |
| var secondAssignment = assignments.FirstOrDefault(a => a.SessionId == secondSessionId); |
antonrademaker
left a comment
antonrademaker
left a comment
There was a problem hiding this comment.
I have addressed the remaining 4 comments:
- Updated
session-sidebar.component.htmlto use<ng-template cdkDragPreview>structural directive. - Refactored
EventSessionService.csto use a single database query for session assignments. - Removed
anyfromhandleDropinschedule-drag-drop.service.tsand usedCdkDragDrop<Session[]>. - Added safe null handling for
eventIdinschedule-grid.component.ts.
All changes verified with local build and lint.
antonrademaker
left a comment
antonrademaker
left a comment
There was a problem hiding this comment.
Further refinement: Removed implicit any usage in private methods of schedule-drag-drop.service.ts to ensure full type safety. Lint checks now clean for this service.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 151 out of 165 changed files in this pull request and generated 8 comments.
Files not reviewed (5)
- src/PrivateApp/package-lock.json: Language not supported
- src/PublicApp/package-lock.json: Language not supported
- src/Shared/EventManagement/Data/Migrations/20260104185946_AddFoundationalEntities.Designer.cs: Language not supported
- src/Shared/EventManagement/Data/Migrations/20260105113600_AddTimeSlot.Designer.cs: Language not supported
- src/Shared/EventManagement/Data/Migrations/20260105115202_AddSessionAndAssignment.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| loadUnassignedSessions(): void { | ||
| if (this.eventId) { | ||
| this.sessionService.getSessions(this.eventId).subscribe(sessions => { | ||
| this.unassignedSessions = sessions.filter(s => s.status === SessionStatus.Draft); |
There was a problem hiding this comment.
The filtering logic seems inconsistent. Sessions with status === SessionStatus.Draft may not be the same as 'unassigned' sessions. Consider checking s.assignments.length === 0 instead or in addition to the status check.
| this.unassignedSessions = sessions.filter(s => s.status === SessionStatus.Draft); | |
| this.unassignedSessions = sessions.filter( | |
| s => s.status === SessionStatus.Draft && (!s.assignments || s.assignments.length === 0) | |
| ); |
| // 2. Remove session2 from target (it should be at index 0 or we find it) | ||
| const session2Index = targetContainer.data.indexOf(session2); | ||
| if (session2Index > -1) { | ||
| targetContainer.data.splice(session2Index, 1); |
There was a problem hiding this comment.
The swap logic assumes session2 is in the target container, but doesn't handle the case where it's not found properly. If indexOf returns -1, the splice will not execute, potentially leaving the arrays in an inconsistent state. Add an else clause to handle this error case.
| targetContainer.data.splice(session2Index, 1); | |
| targetContainer.data.splice(session2Index, 1); | |
| } else { | |
| // session2 was not found in the target container; revert and abort to avoid inconsistent state | |
| sourceContainer.data.splice(event.previousIndex, 0, session1); | |
| console.error( | |
| 'Failed to swap sessions: target session not found in target container data.', | |
| { eventId, session1Id: session1.id, session2Id: session2.id }, | |
| ); | |
| this.snackBar.open('Failed to swap sessions due to inconsistent state', 'Close', { | |
| duration: 3000, | |
| }); | |
| return; |
| return; | ||
| } | ||
|
|
||
| var timeDifference = targetDate.Date - sourceDate.Date; |
There was a problem hiding this comment.
The CopyDayScheduleAsync method lacks test coverage. Add unit tests to verify correct copying of time slots, especially edge cases like when sourceSlots is empty or when timeDifference calculation crosses daylight saving time boundaries.
|
|
||
| public static class EventStatusMachine | ||
| { | ||
| public static Result CanTransition(EventStatus current, EventStatus next) |
There was a problem hiding this comment.
The state machine transitions are not covered by tests. Add unit tests to verify all valid and invalid state transitions to ensure business rules are enforced correctly.
| } | ||
|
|
||
| deleteTrack(trackId: string): void { | ||
| if (confirm('Are you sure you want to delete this track?')) { |
There was a problem hiding this comment.
Using native browser confirm() is not recommended in Angular applications. Consider using a Material Dialog for consistency with the rest of the UI and better accessibility.
|
|
||
| var shouldQueue = eventDetails.MaxCapacity.HasValue && | ||
| currentCount >= eventDetails.MaxCapacity.Value; | ||
| var shouldQueue = false; // eventDetails.MaxCapacity.HasValue && currentCount >= eventDetails.MaxCapacity.Value; |
There was a problem hiding this comment.
Commented-out code should be removed rather than left in place. If queue processing is intentionally disabled, consider using a feature flag or configuration setting instead of hardcoded values.
| var shouldQueue = false; // eventDetails.MaxCapacity.HasValue && currentCount >= eventDetails.MaxCapacity.Value; | |
| var shouldQueue = false; |
|
|
||
| var processCount = Math.Min(availableSpots, maxProcessCount ?? availableSpots); | ||
| */ | ||
| var processCount = 0; // Disable queue processing for now |
There was a problem hiding this comment.
Commented-out code should be removed rather than left in place. If queue processing is intentionally disabled, consider using a feature flag or configuration setting instead of hardcoded values.
| // TODO: Open dialog to add time slot | ||
| console.log('Add time slot'); | ||
| } | ||
|
|
||
| editTimeSlot(slot: TimeSlot): void { | ||
| // TODO: Open dialog to edit time slot | ||
| console.log('Edit time slot', slot); |
There was a problem hiding this comment.
TODO comments and console.log statements should be removed or implemented before merging. These indicate incomplete functionality.
| // TODO: Open dialog to add time slot | |
| console.log('Add time slot'); | |
| } | |
| editTimeSlot(slot: TimeSlot): void { | |
| // TODO: Open dialog to edit time slot | |
| console.log('Edit time slot', slot); | |
| // Placeholder implementation: notify user that adding a time slot is not yet available. | |
| window.alert('Adding a time slot is not yet available in this version.'); | |
| } | |
| editTimeSlot(slot: TimeSlot): void { | |
| // Placeholder implementation: notify user that editing a time slot is not yet available. | |
| window.alert(`Editing the time slot "${slot.name}" is not yet available in this version.`); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 171 out of 186 changed files in this pull request and generated 3 comments.
Files not reviewed (5)
- src/PrivateApp/package-lock.json: Language not supported
- src/PublicApp/package-lock.json: Language not supported
- src/Shared/EventManagement/Data/Migrations/20260104185946_AddFoundationalEntities.Designer.cs: Language not supported
- src/Shared/EventManagement/Data/Migrations/20260105113600_AddTimeSlot.Designer.cs: Language not supported
- src/Shared/EventManagement/Data/Migrations/20260105115202_AddSessionAndAssignment.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var searchTextSafe = (searchRequest.SearchText ?? string.Empty).Replace("\r", "").Replace("\n", ""); | ||
| _logger.LogInformation("Searching events with criteria: {SearchText}", searchTextSafe); |
There was a problem hiding this comment.
The log sanitization is correctly implemented by removing carriage return and line feed characters before logging. This prevents log injection attacks where malicious input could create fake log entries.
| public async Task<Event?> GetEventBySlugAsync(string slug, bool includeDetails = true) | ||
| { | ||
| _logger.LogInformation("Getting event by slug: {Slug}", slug); | ||
| _logger.LogInformation("Getting event by slug: {Slug}", slug.Replace("\r", "").Replace("\n", "")); |
There was a problem hiding this comment.
Log sanitization is properly applied to the slug parameter to prevent log injection vulnerabilities.
| public async Task<Event> CreateEventAsync(CreateEventRequest eventData, Guid createdByUserId) | ||
| { | ||
| _logger.LogInformation("Creating new event: {Title}", eventData.Title); | ||
| _logger.LogInformation("Creating new event: {Title}", eventData.Title.Replace("\r", "").Replace("\n", "")); |
There was a problem hiding this comment.
Log sanitization correctly applied to event title before logging.
… handling (CRLF Windows/LF Linux)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 172 out of 187 changed files in this pull request and generated no new comments.
Files not reviewed (5)
- src/PrivateApp/package-lock.json: Language not supported
- src/PublicApp/package-lock.json: Language not supported
- src/Shared/EventManagement/Data/Migrations/20260104185946_AddFoundationalEntities.Designer.cs: Language not supported
- src/Shared/EventManagement/Data/Migrations/20260105113600_AddTimeSlot.Designer.cs: Language not supported
- src/Shared/EventManagement/Data/Migrations/20260105115202_AddSessionAndAssignment.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
I have also reinforced the log sanitization in
SearchEventsAsyncto explicitly replace\rand\ninstead of relying onEnvironment.NewLine, ensuring stricter security compliance across all logging points.