Staff photos#470
Conversation
- Updated AboutController to include ImageService for handling image uploads and resizing. - Introduced CreateStaffViewModel and EditStaffViewModel for managing staff data, including image URLs. - Added ImageUrl property to Staff entity and updated ApplicationDbContextModelSnapshot. - Initialized Staff, Board, and SteeringCommittee lists in AboutUsModel. - Registered ImageService in Program.cs for dependency injection. - Updated CreateStaff and EditStaff views to support file uploads and validation. - Enhanced About.cshtml layout to display staff with images using Bootstrap cards. - Added PeopleController for CRUD operations on staff with authorization checks. - Created migration files to add ImageUrl column to People table. - Added FormFileFromStream and ImageService classes for image processing. - Created StaffViewModels for managing staff creation and editing. - Updated/Create views (Create.cshtml, Delete.cshtml, Details.cshtml, Edit.cshtml, Index.cshtml) to support new functionality.
Reduced #heading top margin and switched to em units for consistency. Removed #pc2Logo padding-bottom to eliminate extra space below the logo on large screens.
Moved Staff, Board, and Steering Committee CRUD actions from AboutController to a new PeopleController. Updated dependency injection for image services. Staff photo upload now requires a file (no URL option). Rewrote and reorganized all related Razor views, removing generic People views. Updated navigation to use new controller routes. Cleaned up legacy code and improved UI consistency.
…nto StaffPhotos
ImageUrl is now defined in the People base class, allowing all People entities to have an associated image URL. The property retains its DataType attribute for URL validation.
Refactored the management of Staff, Board, and Steering Committee members into a single, generic CRUD interface using a new PersonType enum and PersonViewModel. Replaced all separate controller actions and views with unified, type-driven implementations. Updated navigation and removed old, redundant ViewModels. This reduces duplication, simplifies maintenance, and provides a consistent UI for all people management.
Relaxed staff photo upload validation in controller and views. Generalized image filename generation for all person types. Added client-side image preview to Create/Edit views. Show current image in Delete view. Updated Edit view to clear preview when removing photo. Removed "Required for staff" note from Create view.
Renamed .staff-avatar to .person-avatar for consistency. Board and steering committee members with images now use card layouts; those without images are listed in tables. Section headers adjust dynamically based on image presence. Index page adds avatar column with image or placeholder icon for each person. Enhances visual clarity and consistency across the site.
Changed PriorityOrder input to hidden for SteeringCommittee (set to 1). For other types, replaced number input with select options. Staff shows "Director" and "Other"; others show "Board Chair", "Treasurer", and "Other".
Replaced console error output with ILogger-based logging for image upload and deletion errors. Updated image resizing to use explicit 250x250 dimensions during upload. Injected ILogger<PeopleController> via constructor.
Updated the PeopleController to resize uploaded images to 350x350 pixels instead of 250x250 before uploading. This change ensures higher resolution profile images.
Deleted staff image section in About.cshtml and removed all associated image files from the project.
Added calls to RemovePersonPhoto for Staff, Board, and Steering Committee members to ensure their photos are deleted from storage before their database records are removed.
There was a problem hiding this comment.
Pull request overview
This pull request implements photo management capabilities for staff, board, and steering committee members, addressing issue #370. The changes consolidate the previously separate CRUD operations for different member types into a unified PeopleController, add image upload/resize/delete functionality using Azure Blob Storage, and update the public-facing About page to display member photos in a modern card-based layout.
Changes:
- Consolidated staff/board/steering committee CRUD operations into a new
PeopleControllerwith a unifiedPersonViewModel - Added photo upload functionality with automatic resizing using ImageSharp library
- Updated the About page to display member photos using Bootstrap cards
- Added database migration to support ImageUrl storage for all person types
Reviewed changes
Copilot reviewed 29 out of 40 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| PC2/Controllers/PeopleController.cs | New unified controller for managing all person types with photo upload/delete functionality |
| PC2/Controllers/AboutController.cs | Removed staff/board/committee CRUD methods (migrated to PeopleController) |
| PC2/Models/ViewModels/PersonViewModel.cs | New unified view model for all person types with photo support |
| PC2/Models/People.cs | Added ImageUrl property to base People class and PersonType enum |
| PC2/Models/FormFileFromStream.cs | Helper class to wrap streams as IFormFile for Azure upload |
| PC2/Services/ImageService.cs | New service for image processing, resizing, and validation |
| PC2/Views/People/*.cshtml | New consolidated CRUD views for all person types |
| PC2/Views/Home/About.cshtml | Updated to display photos in card layout with fallback for members without photos |
| PC2/Views/Shared/_Layout.cshtml | Updated navigation to use new PeopleController routes |
| PC2/Views/About/*.cshtml | Removed old separate CRUD views for staff/board/committee |
| PC2/Data/Migrations/20250807162448_AddImageUrlToStaff.cs | Database migration adding ImageUrl column to People table |
| PC2/Program.cs | Registered ImageService in DI container |
| PC2/PC2.csproj | Added SixLabors.ImageSharp package dependency |
| PC2/wwwroot/css/site.css | Minor CSS adjustments for layout |
Files not reviewed (1)
- PC2/Data/Migrations/20250807162448_AddImageUrlToStaff.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await image.SaveAsJpegAsync(outputStream, new JpegEncoder | ||
| { | ||
| Quality = 85 | ||
| }); |
There was a problem hiding this comment.
The image is always converted to JPEG format regardless of the input format (line 60). While this ensures consistency, it means that PNG images with transparency will lose their alpha channel and may have unexpected visual results. Consider either preserving the original format or documenting this behavior clearly in the UI helper text.
| /// <summary> | ||
| /// Validates if the uploaded file is a valid image | ||
| /// </summary> | ||
| public static bool IsValidImageFile(IFormFile file) | ||
| { | ||
| if (file == null || file.Length == 0) | ||
| return false; | ||
|
|
||
| var allowedMimeTypes = new[] | ||
| { | ||
| "image/jpeg", | ||
| "image/jpg", | ||
| "image/png", | ||
| "image/gif", | ||
| "image/bmp", | ||
| "image/webp" | ||
| }; | ||
|
|
||
| return allowedMimeTypes.Contains(file.ContentType?.ToLower()); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets a safe filename for uploaded images | ||
| /// </summary> | ||
| public static string GetSafeImageFileName(string originalFileName, int personId) |
There was a problem hiding this comment.
The image validation only checks the MIME type from the Content-Type header, which can be easily spoofed by attackers. This could potentially allow malicious files to be uploaded. Consider adding additional validation such as checking the actual file signature (magic bytes) or using ImageSharp to attempt to load the image and catching any exceptions, which would verify it's actually a valid image file.
| /// <summary> | |
| /// Validates if the uploaded file is a valid image | |
| /// </summary> | |
| public static bool IsValidImageFile(IFormFile file) | |
| { | |
| if (file == null || file.Length == 0) | |
| return false; | |
| var allowedMimeTypes = new[] | |
| { | |
| "image/jpeg", | |
| "image/jpg", | |
| "image/png", | |
| "image/gif", | |
| "image/bmp", | |
| "image/webp" | |
| }; | |
| return allowedMimeTypes.Contains(file.ContentType?.ToLower()); | |
| } | |
| /// <summary> | |
| /// Gets a safe filename for uploaded images | |
| /// </summary> | |
| public static string GetSafeImageFileName(string originalFileName, int personId) | |
| /// <summary> | |
| /// Validates if the uploaded file is a valid image | |
| /// </summary> | |
| public static bool IsValidImageFile(IFormFile file) | |
| { | |
| if (file == null || file.Length == 0) | |
| return false; | |
| var allowedMimeTypes = new[] | |
| { | |
| "image/jpeg", | |
| "image/jpg", | |
| "image/png", | |
| "image/gif", | |
| "image/bmp", | |
| "image/webp" | |
| }; | |
| var contentType = file.ContentType?.ToLowerInvariant(); | |
| if (string.IsNullOrWhiteSpace(contentType) || !allowedMimeTypes.Contains(contentType)) | |
| return false; | |
| // Attempt to load the file as an image using ImageSharp to verify it is a valid image file | |
| try | |
| { | |
| using var stream = file.OpenReadStream(); | |
| using var image = Image.Load(stream); | |
| return true; | |
| } | |
| catch (UnknownImageFormatException) | |
| { | |
| return false; | |
| } | |
| catch (ImageFormatException) | |
| { | |
| return false; | |
| } | |
| catch | |
| { | |
| // Any error while loading the image indicates an invalid or corrupted image file | |
| return false; | |
| } | |
| } | |
| /// <summary> | |
| /// Gets a safe filename for uploaded images | |
| /// </summary> | |
| public static string GetSafeImageFileName(string originalFileName, int personId) |
| [Display(Name = "Photo")] | ||
| public IFormFile? PhotoFile { get; set; } |
There was a problem hiding this comment.
The PhotoFile property lacks file size validation attributes. Without a maximum file size limit, users could upload extremely large files which could cause performance issues, timeout errors, or consume excessive bandwidth and storage. Consider adding a file size validation attribute or documenting the maximum acceptable file size.
| await RemovePersonPhoto(person); | ||
|
|
||
| var safeFileName = ImageService.GetSafeImageFileName(photoFile.FileName, personId ?? 0); | ||
| using var resizedImageStream = await _imageService.ResizeImageAsync(photoFile.OpenReadStream(), 350, 350); |
There was a problem hiding this comment.
The resizedImageStream is wrapped in a using statement, but it's then wrapped in a FormFileFromStream and passed to UploadFileAsync. The UploadFileAsync method calls OpenReadStream() which returns the same stream instance, and then disposes it in another using statement (line 43-46 of AzureBlobUploader.cs). This means the stream is disposed twice - once inside UploadFileAsync and once when the HandlePhotoUpload method's using block exits. This could cause ObjectDisposedException or other unexpected behavior. Consider either removing the using statement from HandlePhotoUpload or ensuring the stream isn't disposed in UploadFileAsync.
| using var resizedImageStream = await _imageService.ResizeImageAsync(photoFile.OpenReadStream(), 350, 350); | |
| var resizedImageStream = await _imageService.ResizeImageAsync(photoFile.OpenReadStream(), 350, 350); |
| /// <summary> | ||
| /// URL to the person's photo/image. | ||
| /// </summary> | ||
| [DataType(DataType.Url)] |
There was a problem hiding this comment.
The ImageUrl property has a DataType.Url attribute for display purposes but lacks a [Url] validation attribute. This means invalid URL formats could be stored in the database and rendered in views, potentially causing broken images or, in edge cases, unexpected behavior. Consider adding a [Url] validation attribute to ensure only valid URLs are stored.
| [DataType(DataType.Url)] | |
| [DataType(DataType.Url)] | |
| [Url] |
| namespace PC2.Models | ||
| { | ||
| /// <summary> | ||
| /// Implementation of IFormFile that wraps a Stream | ||
| /// Used for creating IFormFile from resized image streams | ||
| /// </summary> | ||
| public class FormFileFromStream : IFormFile | ||
| { | ||
| private readonly Stream _stream; | ||
| private readonly string _name; | ||
| private readonly string _contentType; | ||
|
|
||
| public FormFileFromStream(Stream stream, string name, string contentType) | ||
| { | ||
| _stream = stream; | ||
| _name = name; | ||
| _contentType = contentType; | ||
| } | ||
|
|
||
| public string ContentType => _contentType; | ||
| public string ContentDisposition => $"form-data; name=\"{Name}\"; filename=\"{FileName}\""; | ||
| public IHeaderDictionary Headers => new HeaderDictionary(); | ||
| public long Length => _stream.Length; | ||
| public string Name => _name; | ||
| public string FileName => _name; | ||
|
|
||
| public void CopyTo(Stream target) | ||
| { | ||
| _stream.CopyTo(target); | ||
| } | ||
|
|
||
| public Task CopyToAsync(Stream target, CancellationToken cancellationToken = default) | ||
| { | ||
| return _stream.CopyToAsync(target, cancellationToken); | ||
| } | ||
|
|
||
| public Stream OpenReadStream() | ||
| { | ||
| _stream.Position = 0; | ||
| return _stream; | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
The FormFileFromStream class wraps a Stream but doesn't implement IDisposable. The wrapped stream is never disposed, which could lead to resource leaks. Since IFormFile doesn't inherit from IDisposable, the consuming code may not know to dispose the underlying stream. Consider either implementing IDisposable or documenting that the caller is responsible for disposing the original stream that was passed to the constructor.
| [HttpPost, ActionName("Delete")] | ||
| public async Task<IActionResult> ConfirmDelete(int id, PersonType type) | ||
| { | ||
| switch (type) | ||
| { | ||
| case PersonType.Staff: | ||
| var staff = await StaffDB.GetStaffMember(_context, id); | ||
| if (staff == null) return NotFound(); | ||
| await RemovePersonPhoto(staff); | ||
| await StaffDB.Delete(_context, staff); | ||
| break; | ||
|
|
||
| case PersonType.Board: | ||
| var board = await BoardDB.GetBoardMember(_context, id); | ||
| if (board == null) return NotFound(); | ||
| await RemovePersonPhoto(board); | ||
| await BoardDB.Delete(_context, board); | ||
| break; | ||
|
|
||
| case PersonType.SteeringCommittee: | ||
| var sc = await SteeringCommitteeDB.GetSteeringCommitteeMember(_context, id); | ||
| if (sc == null) return NotFound(); | ||
| await RemovePersonPhoto(sc); | ||
| await SteeringCommitteeDB.Delete(_context, sc); | ||
| break; | ||
| } | ||
| return RedirectToAction(nameof(Index), new { type }); |
There was a problem hiding this comment.
If RemovePersonPhoto fails during person deletion (line 191, 198, or 205), the exception is caught and logged in RemovePersonPhoto (line 253-256), but the person is still deleted from the database. This results in orphaned blobs in Azure storage that can never be cleaned up since the reference to them is lost. Consider either: 1) Propagating the exception to prevent database deletion if blob deletion fails, or 2) Implementing a separate cleanup process for orphaned blobs, or 3) At minimum, logging a warning that manual cleanup may be needed.
| using Microsoft.EntityFrameworkCore.Migrations; | ||
|
|
||
| #nullable disable | ||
|
|
||
| namespace PC2.Data.Migrations | ||
| { | ||
| /// <inheritdoc /> | ||
| public partial class AddImageUrlToStaff : Migration | ||
| { | ||
| /// <inheritdoc /> | ||
| protected override void Up(MigrationBuilder migrationBuilder) | ||
| { | ||
| migrationBuilder.AddColumn<string>( | ||
| name: "ImageUrl", | ||
| table: "People", | ||
| type: "nvarchar(max)", | ||
| nullable: true); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| protected override void Down(MigrationBuilder migrationBuilder) | ||
| { | ||
| migrationBuilder.DropColumn( | ||
| name: "ImageUrl", | ||
| table: "People"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The migration is named "AddImageUrlToStaff" but it actually adds the ImageUrl column to the People table (the base table in the TPH hierarchy), which affects all person types including Staff, Board, and SteeringCommittee. While functionally correct, the name is misleading. Consider a more accurate name like "AddImageUrlToPeople" to better reflect what the migration actually does.
| public async Task<MemoryStream> ResizeImageAsync(Stream imageStream, int maxWidth = 800, int maxHeight = 600) | ||
| { | ||
| try | ||
| { | ||
| using var image = await Image.LoadAsync(imageStream); | ||
|
|
||
| // Calculate new dimensions while maintaining aspect ratio | ||
| (int newWidth, int newHeight) = CalculateResizeDimensions(image.Width, image.Height, maxWidth, maxHeight); | ||
|
|
||
| // If image is already smaller than max dimensions, return original | ||
| if (newWidth == image.Width && newHeight == image.Height) | ||
| { | ||
| var originalStream = new MemoryStream(); | ||
| imageStream.Position = 0; | ||
| await imageStream.CopyToAsync(originalStream); | ||
| originalStream.Position = 0; | ||
| return originalStream; | ||
| } | ||
|
|
||
| // Resize the image | ||
| image.Mutate(x => x.Resize(new ResizeOptions | ||
| { | ||
| Size = new Size(newWidth, newHeight), | ||
| Mode = ResizeMode.Max, // Ensure it fits within max dimensions | ||
| Sampler = KnownResamplers.Lanczos3 // High quality resampling (more CPU intensive) | ||
| })); | ||
|
|
||
| // Save to memory stream | ||
| var outputStream = new MemoryStream(); | ||
|
|
||
| // Use JPEG format with good quality for most cases | ||
| await image.SaveAsJpegAsync(outputStream, new JpegEncoder | ||
| { | ||
| Quality = 85 | ||
| }); | ||
|
|
||
| outputStream.Position = 0; | ||
| return outputStream; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error resizing image"); | ||
| throw new InvalidOperationException("Failed to resize image", ex); | ||
| } | ||
| } |
There was a problem hiding this comment.
There is no file size validation before attempting to resize the image. A user could upload an extremely large file (e.g., several hundred MB or GB), which would consume significant memory and CPU during the ImageSharp processing, potentially causing timeouts or out-of-memory errors. Consider adding a maximum file size check (e.g., 10-20 MB) before processing the image.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Refactored PeopleController for clarity and maintainability: moved [Authorize] to class level, used constructor DI, split CRUD actions into separate GET/POST methods, and improved type handling with switch expressions. - Improved photo upload and deletion logic with better error handling and logging. - Promoted FormFileFromStream and PersonViewModel to top-level classes with cleaner formatting and documentation. - Cleaned up About.cshtml and removed an unused using from AboutController.
…nto StaffPhotos
Removed using statement around file stream in AzureBlobUploader. The method no longer disposes the stream after upload; instead, the caller is now responsible for managing the stream's lifetime. This change clarifies ownership and prevents premature disposal.
Wrap Create, Edit, and Delete POST actions in try-catch blocks to log exceptions and display user-friendly error messages. Simplify HandlePhotoUpload by removing its internal try-catch, allowing errors to propagate. Ensure RemovePersonPhoto logs and rethrows exceptions. These changes improve reliability and user feedback.
Changed the staff image class from 'rounded-circle' to 'rounded' in About.cshtml, so images now have slightly rounded corners instead of being fully circular. This improves visual consistency with other image styles on the site.
Closes #370
Summary
Add the ability for the client to manage photos for the staff members
Copilot Summary
This pull request introduces a major refactor for managing staff, board, and steering committee members by consolidating their CRUD operations into a new, unified
PeopleController. Additionally, it adds support for storing and managing profile images for these members, including database schema changes and file handling logic. Several redundant methods are removed fromAboutController, and the code is streamlined for clarity and maintainability.Refactor and consolidation of person management:
PeopleControllerthat centralizes CRUD operations for staff, board, and steering committee members, replacing multiple methods previously inAboutController. This controller also introduces unified view models and type-specific validation. (PC2/Controllers/PeopleController.cs)AboutController, shifting their responsibilities toPeopleController. (PC2/Controllers/AboutController.cs)Profile image support:
PC2/Controllers/PeopleController.cs)ImageUrlcolumn to thePeopletable, enabling storage of image links for staff, board, and steering committee members. (PC2/Data/Migrations/20250807162448_AddImageUrlToStaff.cs)ImageUrlcolumn in thePeopletable. (PC2/Data/Migrations/ApplicationDbContextModelSnapshot.cs)Minor code clean-up:
AboutController. (PC2/Controllers/AboutController.cs) [1] [2] [3] [4]PC2/Controllers/AboutController.cs) [1] [2]