Feature/issue 65 final: CRUD endpoints for Programs and Program categories - #320
Conversation
… for certain test
|
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 52
🔭 Outside diff range comments (1)
VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/ImageSeeder/ImagesSeeder.cs (1)
11-29: Consider externalizing large base64 test dataThe hardcoded base64 image strings make the file difficult to maintain and increase the repository size. Consider storing test images as separate files or using smaller placeholder images for tests.
Consider creating a separate test data directory with actual image files and loading them dynamically:
-private static readonly List<Image> _images = -[ - new Image - { - Id = 1, - BlobName = "testname1", - Base64 = "data:image/jpeg;base64,/9j/4AAQ...", - MimeType = "image/jpg" - }, +private async Task<List<Image>> LoadTestImagesAsync() +{ + var testImagesPath = Path.Combine(AppContext.BaseDirectory, "TestData", "Images"); + var images = new List<Image>(); + + foreach (var file in Directory.GetFiles(testImagesPath, "*.jpg")) + { + var bytes = await File.ReadAllBytesAsync(file); + var base64 = Convert.ToBase64String(bytes); + images.Add(new Image + { + Id = images.Count + 1, + BlobName = Path.GetFileNameWithoutExtension(file), + Base64 = $"data:image/jpeg;base64,{base64}", + MimeType = "image/jpg" + }); + } + return images; +}
🧹 Nitpick comments (81)
VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Delete/DeleteProgramCategoryCommand.cs (1)
6-6: Follow C# naming conventions for parameter names.The parameter
idshould follow PascalCase naming convention for public properties in C# records.Apply this diff to correct the naming:
-public record DeleteProgramCategoryCommand(long id) : IRequest<Result<long>>; +public record DeleteProgramCategoryCommand(long Id) : IRequest<Result<long>>;VictoryCenter/VictoryCenter.BLL/Validators/ProgramCategories/UpdateProgramCategoryValidator.cs (1)
16-16: Minor formatting: Remove extra space before parameter.There's an extra space before
ProgramCategoryConstants.MaxNameLengthin the method call..WithMessage(ErrorMessagesConstants - .PropertyMustHaveAMaximumLengthOfNCharacters("Name", ProgramCategoryConstants.MaxNameLength)) + .PropertyMustHaveAMaximumLengthOfNCharacters("Name", ProgramCategoryConstants.MaxNameLength))VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Update/UpdateProgramCategoryCommand.cs (1)
7-8: Consider using PascalCase for the record parameter.The command structure follows CQRS patterns well and uses appropriate result handling with FluentResults. However, consider using PascalCase for the parameter name to align with C# naming conventions for public record parameters.
-public record UpdateProgramCategoryCommand(UpdateProgramCategoryDto updateProgramCategoryDto) +public record UpdateProgramCategoryCommand(UpdateProgramCategoryDto UpdateProgramCategoryDto) : IRequest<Result<ProgramCategoryDto>>;VictoryCenter/VictoryCenter.BLL/DTOs/Programs/UpdateProgramDto.cs (1)
3-6: Clean inheritance pattern with opportunity for validation enhancement.The design effectively extends
CreateProgramDtowith just the necessary identifier property, following DRY principles well. Consider adding validation attributes to ensure the Id property meets expected constraints:public class UpdateProgramDto : CreateProgramDto { + [Required] + [Range(1, long.MaxValue, ErrorMessage = "Id must be a positive number")] public long Id { get; set; } }This would provide consistent validation behavior across your update operations and prevent invalid identifiers from being processed.
VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetPublished/GetPublishedProgramsQuery.cs (1)
7-7: Consider adding pagination or filtering parameters for scalability.While the current parameterless design is simple and fits the immediate requirement for retrieving all published programs, consider whether this query might benefit from optional parameters for future scalability:
- Pagination (skip/take or page/pageSize)
- Category filtering
- Sorting options
- Search functionality
For now, the implementation aligns with the PR objectives, but these enhancements could improve performance as the dataset grows.
Example of how this could be extended:
-public record GetPublishedProgramsQuery : IRequest<Result<List<PublishedProgramDto>>>; +public record GetPublishedProgramsQuery( + int? Page = null, + int? PageSize = null, + long? CategoryId = null +) : IRequest<Result<List<PublishedProgramDto>>>;VictoryCenter/VictoryCenter.WebAPI/Controllers/ProgramCategories/ProgramCategoryController.cs (2)
14-18: Consider adding explicit route and response documentation.The create endpoint implementation follows good patterns with MediatR and async/await. However, consider enhancing it with explicit routing and response documentation for better API discoverability.
[HttpPost] +[Route("")] +[ProducesResponseType(typeof(ProgramCategoryDto), StatusCodes.Status201Created)] +[ProducesResponseType(StatusCodes.Status400BadRequest)] public async Task<IActionResult> CreateProgramCategory([FromBody] CreateProgramCategoryDto programCategoryDto)
27-31: Update endpoint could benefit from explicit route definition.The update implementation is solid, but consider adding an explicit empty route for consistency and clarity, especially when working with OpenAPI documentation tools.
[HttpPut] +[Route("")] public async Task<IActionResult> UpdateProgramCategory([FromBody] UpdateProgramCategoryDto updateProgramCategoryDto)VictoryCenter/VictoryCenter.WebAPI/Controllers/Public/ProgramsController.cs (1)
9-14: Consider scalability enhancements for the public endpoint.The implementation follows established patterns well with proper async handling and documentation. However, consider these enhancements for a public endpoint:
- Pagination support: Public endpoints may need to handle large datasets efficiently
- Caching strategy: Published programs are likely read frequently and change infrequently, making them good candidates for caching
- Response headers: Consider adding cache-control headers for client-side caching
Example enhancement for pagination:
[HttpGet("published")] [ProducesResponseType(StatusCodes.Status200OK, Type = typeof(List<PublishedProgramDto>))] -public async Task<IActionResult> GetPublishedPrograms() +public async Task<IActionResult> GetPublishedPrograms([FromQuery] int page = 1, [FromQuery] int pageSize = 10) { - return HandleResult(await Mediator.Send(new GetPublishedProgramsQuery())); + return HandleResult(await Mediator.Send(new GetPublishedProgramsQuery(page, pageSize))); }Consider adding response caching:
[HttpGet("published")] +[ResponseCache(Duration = 300, Location = ResponseCacheLocation.Any)] [ProducesResponseType(StatusCodes.Status200OK, Type = typeof(List<PublishedProgramDto>))]VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetById/GetProgramByIdHandler.cs (2)
27-28: Consider adding input validation for the request parameter.Based on the codebase patterns and retrieved learnings, MediatR handlers typically use FluentValidation with
ValidateAndThrowAsync()early in the Handle method. Consider adding validation for therequest.idparameter to ensure it's a valid identifier.public async Task<Result<ProgramDto>> Handle(GetProgramByIdQuery request, CancellationToken cancellationToken) { + await _validator.ValidateAndThrowAsync(request, cancellationToken); + var queryOptions = new QueryOptions<Program>This would require injecting an
IValidator<GetProgramByIdQuery>dependency and creating the corresponding validator class.
44-49: Consider adding error handling for blob storage operations.While the current implementation is solid, blob storage operations can fail due to network issues, missing files, or storage service problems. Consider wrapping the blob service call in a try-catch block to handle potential exceptions gracefully.
if (program.Image is not null) { - program.Image.Base64 = await _blobService.FindFileInStorageAsBase64Async( - program.Image.BlobName, - program.Image.MimeType); + try + { + program.Image.Base64 = await _blobService.FindFileInStorageAsBase64Async( + program.Image.BlobName, + program.Image.MimeType); + } + catch (Exception ex) + { + // Log the exception and continue without the image + // or return a specific error result based on requirements + program.Image.Base64 = null; + } }VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/Programs/UpdateProgramValidatorTests.cs (1)
64-95: Consider consistent test data setup for description validation.While the validation logic is correct, the test at line 70-76 creates a fully populated
UpdateProgramDtoobject, whereas other description tests only set theDescriptionproperty. For consistency and clarity, consider using minimal test data setup unless other properties are specifically needed for the validation being tested.var updateProgramDto = new UpdateProgramDto() { - Name = "TestName", - Status = Status.Published, Description = description, - CategoriesId = [1, 2] };VictoryCenter/VictoryCenter.BLL/Validators/ProgramCategories/CreateProgramCategoryValidator.cs (1)
9-19: Excellent validation rules implementation with minor formatting fix needed.The validation rules are comprehensive and follow FluentValidation best practices:
- Proper use of
NotEmpty(),MaximumLength(), andMinimumLength()- Consistent use of constants for maintainability
- Standardized error messages using
ErrorMessagesConstantsApply this diff to fix the extra space in the formatting:
- .PropertyMustHaveAMaximumLengthOfNCharacters("Name", ProgramCategoryConstants.MaxNameLength)) + .PropertyMustHaveAMaximumLengthOfNCharacters("Name", ProgramCategoryConstants.MaxNameLength))VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetById/GetProgramByIdQuery.cs (1)
7-7: Excellent use of modern C# patterns and MediatR conventions.The query record is well-designed using:
- Record type for immutability and value semantics
- Proper MediatR IRequest interface implementation
- FluentResults for robust error handling
- Appropriate parameter naming conventions
Consider adding parameter validation to ensure the
idis positive, though this could also be handled in the handler or through a custom validator.Example parameter validation approach:
-public record GetProgramByIdQuery(long id) : IRequest<Result<ProgramDto>>; +public record GetProgramByIdQuery(long id) : IRequest<Result<ProgramDto>> +{ + public GetProgramByIdQuery + { + if (id <= 0) + throw new ArgumentException("Program ID must be greater than zero.", nameof(id)); + } +}VictoryCenter/VictoryCenter.BLL/DTOs/Programs/ProgramFilterRequestDto.cs (3)
12-12: Consider renamingCategoryIdtoCategoryIdsfor clarity.Since this property accepts a list of category identifiers, using the plural form would better reflect its purpose and align with C# naming conventions for collections.
- public List<long>? CategoryId { get; init; } + public List<long>? CategoryIds { get; init; }
6-8: Add validation attributes for pagination parameters.Consider adding validation constraints to ensure valid pagination values. This will prevent potential issues with negative offsets or invalid limits.
+using System.ComponentModel.DataAnnotations; + public class ProgramFilterRequestDto { + [Range(0, int.MaxValue, ErrorMessage = "Offset must be non-negative")] public int? Offset { get; init; } + [Range(1, int.MaxValue, ErrorMessage = "Limit must be positive")] public int? Limit { get; init; }
4-13: Consider adding XML documentation for better API clarity.Since this DTO is used across multiple layers including the API, XML documentation comments would enhance developer experience and support automatic API documentation generation.
+/// <summary> +/// Data transfer object for filtering programs with pagination and category options. +/// </summary> public class ProgramFilterRequestDto { + /// <summary> + /// Gets or sets the number of records to skip for pagination. + /// </summary> public int? Offset { get; init; } + /// <summary> + /// Gets or sets the maximum number of records to return. + /// </summary> public int? Limit { get; init; } + /// <summary> + /// Gets or sets the status filter for programs. + /// </summary> public Status? Status { get; init; } + /// <summary> + /// Gets or sets the list of category IDs to filter programs by. + /// </summary> public List<long>? CategoryId { get; init; } }VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/ProgramCategories/UpdateProgramCategoryValidatorTests.cs (2)
10-10: Improve field naming convention.The field name
_validatorTestsshould follow standard naming conventions for private fields containing the system under test.- private readonly UpdateProgramCategoryValidator _validatorTests; + private readonly UpdateProgramCategoryValidator _validator;Update the constructor and all test methods accordingly:
- _validatorTests = new UpdateProgramCategoryValidator(); + _validator = new UpdateProgramCategoryValidator();- var result = _validatorTests.TestValidate(command); + var result = _validator.TestValidate(command);
29-42: Add boundary tests for minimum length validation.Consider adding test cases for the exact minimum length (5 characters based on the validation logic) to ensure boundary conditions are properly tested.
Add a test method for the valid minimum length:
[Fact] public void Validate_ShouldNotHaveError_When_Name_IsExactlyMinimumLength() { var name = new string('a', ProgramCategoryConstants.MinNameLength); var command = new UpdateProgramCategoryCommand(new UpdateProgramCategoryDto { Name = name }); var result = _validator.TestValidate(command); result.ShouldNotHaveValidationErrorFor(c => c.updateProgramCategoryDto.Name); }Similarly, add a test for the exact maximum length:
[Fact] public void Validate_ShouldNotHaveError_When_Name_IsExactlyMaximumLength() { var name = new string('a', ProgramCategoryConstants.MaxNameLength); var command = new UpdateProgramCategoryCommand(new UpdateProgramCategoryDto { Name = name }); var result = _validator.TestValidate(command); result.ShouldNotHaveValidationErrorFor(c => c.updateProgramCategoryDto.Name); }VictoryCenter/VictoryCenter.BLL/Validators/Programs/BaseProgramValidator.cs (3)
8-11: Consider the class naming and inheritance design.The class name
BaseProgramValidatorsuggests it's designed for inheritance, but it's tightly coupled toCreateProgramDto. If other validators likeUpdateProgramValidatorinherit from this, they'll be constrained to the same DTO type, which may not align with your intended design pattern.Consider either:
- Making this generic:
BaseProgramValidator<T>where T has the required properties- Renaming to
CreateProgramValidatorif inheritance isn't planned- Extracting common validation logic to shared methods
22-33: Consider consolidating Description validation rules.The validation logic is correct and the conditional requirement for Published status is well-implemented. However, the two separate
RuleForcalls on the same property can be consolidated for better readability.- RuleFor(x => x.Description) - .MaximumLength(ProgramConstants.MaxDescriptionLength) - .WithMessage(ErrorMessagesConstants - .PropertyMustHaveAMaximumLengthOfNCharacters("Description", ProgramConstants.MaxDescriptionLength)) - .MinimumLength(ProgramConstants.MinDescriptionLength) - .WithMessage(ErrorMessagesConstants - .PropertyMustHaveAMinimumLengthOfNCharacters("Description", ProgramConstants.MinDescriptionLength)); - - RuleFor(x => x.Description) - .NotEmpty() - .WithMessage(ErrorMessagesConstants.PropertyIsRequired("Description")) - .When(x => x.Status == Status.Published); + RuleFor(x => x.Description) + .MaximumLength(ProgramConstants.MaxDescriptionLength) + .WithMessage(ErrorMessagesConstants + .PropertyMustHaveAMaximumLengthOfNCharacters("Description", ProgramConstants.MaxDescriptionLength)) + .MinimumLength(ProgramConstants.MinDescriptionLength) + .WithMessage(ErrorMessagesConstants + .PropertyMustHaveAMinimumLengthOfNCharacters("Description", ProgramConstants.MinDescriptionLength)) + .NotEmpty() + .WithMessage(ErrorMessagesConstants.PropertyIsRequired("Description")) + .When(x => x.Status == Status.Published);
35-39: Consider enhancing CategoriesId validation.The current validation ensures the categories collection isn't empty, which is good. However, consider adding validation for:
- Individual category ID existence (database validation)
- Duplicate category IDs within the collection
- Valid category ID format (positive integers)
Example enhancement:
RuleFor(x => x.CategoriesId) - .NotEmpty().WithMessage(ErrorMessagesConstants.PropertyIsRequired("Categories-list")); + .NotEmpty().WithMessage(ErrorMessagesConstants.PropertyIsRequired("Categories-list")) + .Must(ids => ids.Distinct().Count() == ids.Count()) + .WithMessage("Duplicate category IDs are not allowed") + .ForEach(id => id.GreaterThan(0).WithMessage("Category ID must be positive"));Note: Database existence validation would typically be handled in a separate validator or business logic layer.
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/ProgramCategories/DeleteProgramCategoryTests.cs (1)
16-33: Minor consistency improvement for entity type references.The test data setup covers the necessary scenarios well. Consider using consistent type references for better readability.
Apply this diff to improve consistency:
- Programs = new List<DAL.Entities.Program>() + Programs = new List<Program>()- Programs = new List<DAL.Entities.Program> { new() } + Programs = new List<Program> { new Program() }VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/ProgramCategories/CreateProgramCategoryValidatorTests.cs (1)
11-15: Consider renaming the validator field for clarity.The field name
_validatorTestsis slightly confusing since it represents the validator instance, not tests. Consider renaming it to_validatorfor better clarity.- private readonly CreateProgramCategoryValidator _validatorTests; + private readonly CreateProgramCategoryValidator _validator;And update the constructor accordingly:
- _validatorTests = new CreateProgramCategoryValidator(); + _validator = new CreateProgramCategoryValidator();VictoryCenter/VictoryCenter.BLL/Commands/Programs/Create/CreateProgramHandler.cs (1)
59-59: Consider specific exception handlingThe generic
catch(Exception)might hide important blob service errors. Consider catching specific exceptions (likeBlobNotFoundException,UnauthorizedAccessException, etc.) to provide more meaningful error messages to users.- catch(Exception) + catch(BlobNotFoundException) { - return Result.Fail<ProgramDto>(TeamMemberConstants.FailedRetrievingMemberPhoto); + return Result.Fail<ProgramDto>(ProgramConstants.ImageNotFound); + } + catch(Exception ex) + { + // Log the exception for debugging + return Result.Fail<ProgramDto>(ProgramConstants.FailedToRetrieveImage); }VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/Programs/CreateProgramValidatorTests.cs (2)
31-43: Consider adding boundary test for minimum length validation.The tests cover lengths 1-4 characters, but according to the assertion on Line 42, the minimum length is
ProgramConstants.MinNameLength. Consider adding a test case for exactlyMinNameLength - 1to ensure the boundary condition is explicitly tested.[Theory] -[InlineData("t")] -[InlineData("te")] -[InlineData("tes")] -[InlineData("test")] +[InlineData("t")] +[InlineData("te")] +[InlineData("tes")] +[InlineData("test")] public void Validate_ShouldHaveError_When_Name_IsTooShort(string? name)Consider replacing the hardcoded strings with a dynamic approach that uses
ProgramConstants.MinNameLengthto generate test data at the boundary.
83-95: Description length tests should be more explicit about boundary conditions.Similar to the Name validation, these hardcoded lengths (3, 6, 9) don't clearly relate to
ProgramConstants.MinDescriptionLength. Consider using the constant to make the boundary condition explicit.[Theory] -[InlineData(3)] -[InlineData(6)] -[InlineData(9)] +[InlineData(1)] +[InlineData(5)] +[InlineData(9)] // Assuming MinDescriptionLength is 10 public void Validate_ShouldHaveError_When_Description_IsTooShort(int descriptionLength)Consider generating test data based on
ProgramConstants.MinDescriptionLength - 1to ensure clear boundary testing.VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Delete/DeleteProgramCategoryHandler.cs (1)
19-50: Consider using cancellation token in repository operationsWhile the cancellation token is available, it's not being passed to the repository operations. Consider passing it through for better cancellation support.
- var entityToDelete = await _repositoryWrapper.ProgramCategoriesRepository - .GetFirstOrDefaultAsync(queryOptions); + var entityToDelete = await _repositoryWrapper.ProgramCategoriesRepository + .GetFirstOrDefaultAsync(queryOptions, cancellationToken); - if (await _repositoryWrapper.SaveChangesAsync() > 0) + if (await _repositoryWrapper.SaveChangesAsync(cancellationToken) > 0)VictoryCenter/VictoryCenter.BLL/Queries/ProgramCategories/GetProgramCategoriesHandler.cs (1)
35-46: Consider performance implications of sequential blob fetching.The nested loops sequentially fetch blob content for each program image, which could impact performance with large datasets. Consider implementing batch blob retrieval or async parallel processing.
Here's an optimized approach using parallel processing:
- foreach (var category in mapped) - { - foreach (var program in category.Programs) - { - if (program.Image != null) - { - program.Image.Base64 = await _blobService.FindFileInStorageAsBase64Async( - program.Image.BlobName, - program.Image.MimeType); - } - } - } + var allProgramsWithImages = mapped + .SelectMany(c => c.Programs) + .Where(p => p.Image != null) + .ToList(); + + await Task.WhenAll(allProgramsWithImages.Select(async program => + { + program.Image.Base64 = await _blobService.FindFileInStorageAsBase64Async( + program.Image.BlobName, + program.Image.MimeType); + }));VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/ProgramCategories/GetProgramCategoriesTests.cs (1)
18-42: Consider adding more comprehensive test data.The test data setup is minimal and could benefit from more realistic scenarios. Consider including:
- Categories with different properties (descriptions, creation dates, etc.)
- Edge cases like empty names or special characters
- Categories with associated programs to test relationships
private readonly IEnumerable<ProgramCategory> _testProgramCategories = new List<ProgramCategory> { new() { Id = 1, - Name = "Test1" + Name = "Sports Programs", + Description = "Physical activity programs" }, new() { Id = 2, - Name = "Test2" + Name = "Educational Programs", + Description = "Learning and development programs" } };VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Programs/CreateProgramTests.cs (2)
87-100: Validation test has assertion weakness.The test only checks for "Validation failed" substring in the error message, which may not catch specific validation rule violations. Consider asserting more specific validation messages.
Make the assertion more specific:
- Assert.Contains("Validation failed", result.Errors[0].Message); + Assert.Contains("Name is required", result.Errors[0].Message);Or test each validation scenario separately to verify specific error messages match the validator rules.
126-131: Blob service setup could be more realistic.The setup always returns a successful result, but doesn't test the null image scenario where
ImageIdis null. Consider adding test coverage for programs without images.Add a test case for program creation without an image:
[Fact] public async Task Handle_ShouldCreateProgram_WithoutImage() { var dto = CreateValidProgramDto(); dto.ImageId = null; SetUpDependencies(); var handler = new CreateProgramHandler(_mapperMock.Object, _repositoryWrapperMock.Object, _validator, _blobServiceMock.Object); var result = await handler.Handle(new CreateProgramCommand(dto), CancellationToken.None); Assert.True(result.IsSuccess); // Verify blob service was not called _blobServiceMock.Verify(x => x.FindFileInStorageAsBase64Async(It.IsAny<string>(), It.IsAny<string>()), Times.Never); }VictoryCenter/VictoryCenter.BLL/DTOs/ProgramCategories/CreateProgramCategoryDto.cs (1)
3-6: Userequired+initto enforce presence & immutability ofName
CreateProgramCategoryDtois declared as a record, yet theNameproperty remains mutable (set;).
Switching torequired string Name { get; init; }:• Expresses that
Namemust be provided by callers (compile-time guarantee when nullable is enabled).
• Keeps the DTO immutable after construction, in line with the typical intent of C#recordtypes and prevents accidental mutation inside handlers/mappers.Example diff:
-public record CreateProgramCategoryDto -{ - public string Name { get; set; } -} +public record CreateProgramCategoryDto +{ + public required string Name { get; init; } +}(Requires C# 11 / .NET 7; if the target framework is older, consider at least using
init.)VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/ProgramCategories/ProgramCategoriesRepository.cs (1)
8-14: Consider sealing the repository and adding basic null-guardThe repository currently delivers only the base functionality and is unlikely to be subclassed. Marking it
sealedcommunicates that intent and enables some compiler-level optimisations.
While you’re touching the signature, you could also add a defensive null-check forcontext(the base ctor may already guard, but being explicit improves self-documentation).-public class ProgramCategoriesRepository : RepositoryBase<ProgramCategory>, IProgramCategoriesRepository +public sealed class ProgramCategoriesRepository : RepositoryBase<ProgramCategory>, IProgramCategoriesRepository { - public ProgramCategoriesRepository(VictoryCenterDbContext context) - : base(context) + public ProgramCategoriesRepository(VictoryCenterDbContext context) + : base(context ?? throw new ArgumentNullException(nameof(context))) { } }VictoryCenter/VictoryCenter.BLL/Constants/ProgramCategoryConstants.cs (2)
3-12: Mark the constants container asstaticand switch toconstfields
ProgramCategoryConstantsis intended purely as a container for compile-time values and should never be instantiated. Declaring the classstaticcommunicates that intent and prevents accidental instantiation or inheritance.
Likewise, the values are compile-time literals, soconstis more appropriate (and slightly faster) thanstatic readonly.-public class ProgramCategoryConstants +public static class ProgramCategoryConstants { - public static readonly string FailedToCreateCategory = "Failed to create category"; - public static readonly string CantDeleteProgramCategoryWhileAssociatedWithAnyProgram = - "Can't delete category while associated with any program"; - public static readonly string FailedToDeleteCategory = "Failed to delete category"; - public static readonly string FailedToUpdateCategory = "Failed to update category"; - public static readonly int MaxNameLength = 20; - public static readonly int MinNameLength = 5; + public const string FailedToCreateCategory = "Failed to create category"; + public const string CantDeleteProgramCategoryWhileAssociatedWithAnyProgram = + "Can't delete category while associated with any program"; + public const string FailedToDeleteCategory = "Failed to delete category"; + public const string FailedToUpdateCategory = "Failed to update category"; + public const int MaxNameLength = 20; + public const int MinNameLength = 5; }
6-7: Prefer formal tone in error messageContractions can look informal in API responses. Consider replacing “Can’t” with “Cannot” for a more professional tone:
- "Can't delete category while associated with any program"; + "Cannot delete category while associated with any program";VictoryCenter/VictoryCenter.DAL/Entities/ProgramCategory.cs (1)
8-8: PreferHashSet<Program>for navigation collectionsUsing
HashSet<T>communicates uniqueness semantics and gives O(1) look-ups, whereasList<T>allows duplicates and incurs O(n) contains checks. EF Core works fine withHashSet<T>.- public ICollection<Program> Programs { get; set; } = new List<Program>(); + public ICollection<Program> Programs { get; set; } = new HashSet<Program>();If you need ordering, keep
List<T>; otherwiseHashSet<T>is the usual choice for many-to-many joins.VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Programs/GetProgramByIdTests.cs (3)
20-35: Consider making test data more realistic and comprehensive.The test data setup is functional but could be enhanced for better test coverage:
private readonly DAL.Entities.Program _programEntity = new() { Id = 1, Name = "TestName", Description = "TestDescription", Status = Status.Draft, ImageId = 1, + Categories = new List<ProgramCategory>(), // Include categories for comprehensive testing + CreatedDate = DateTime.UtcNow, + UpdatedDate = DateTime.UtcNow }; private readonly ProgramDto _programDto = new() { + Id = 1, Name = "TestName", Description = "TestDescription", Status = Status.Draft, - Image = new ImageDTO() + Image = new ImageDTO { Base64Data = "mockedBase64" } };This ensures the test data aligns more closely with real-world scenarios and includes all relevant properties that might be used by the handler.
78-82: Verify QueryOptions setup includes proper filtering.The repository mock setup might need to verify that the correct query options are being passed:
private void SetUpRepositoryWrapper(DAL.Entities.Program program) { - _mockRepositoryWrapper.Setup(x => x.ProgramsRepository - .GetFirstOrDefaultAsync(It.IsAny<QueryOptions<DAL.Entities.Program>>())).ReturnsAsync(program); + _mockRepositoryWrapper.Setup(x => x.ProgramsRepository + .GetFirstOrDefaultAsync(It.Is<QueryOptions<DAL.Entities.Program>>( + opts => opts.Where != null && opts.Include != null))) + .ReturnsAsync(program); }This ensures the handler is properly setting up query options with filtering and includes, which is likely needed for fetching program categories and related data.
84-89: Consider making blob service mock more specific.The blob service mock could be more precise about the expected parameters:
private void SetUpBlobService() { _mockBlobService - .Setup(x => x.FindFileInStorageAsBase64Async(It.IsAny<string>(), It.IsAny<string>())) + .Setup(x => x.FindFileInStorageAsBase64Async( + It.IsAny<string>(), + It.Is<string>(fileName => !string.IsNullOrEmpty(fileName)))) .ReturnsAsync("mockedBase64"); }This provides better validation that the service is called with meaningful parameters.
VictoryCenter/VictoryCenter.BLL/DTOs/Programs/ProgramDto.cs (1)
17-18: Align DTO naming conventions (ImageDTOvsImageDto)Elsewhere (e.g.,
ProgramCategoryShortDto) the suffix uses “Dto”, whereas here the imported type isImageDTO(all-caps).
This inconsistency makes discoverability harder and violates common PascalCase guidelines.Consider renaming the image DTO (and its usages) to
ImageDtofor uniformity, or at minimum add an alias here to reduce friction:-using VictoryCenter.BLL.DTOs.Images; +using ImageDto = VictoryCenter.BLL.DTOs.Images.ImageDTO;…and adjust the property accordingly.
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Programs/GetPublishedProgramsTests.cs (1)
72-97: Well-structured helper methods with minor improvement opportunities.The dependency setup methods are well-organized and follow good separation of concerns. Consider these enhancements:
- Null handling in SetUpRepositoryWrapper: Add null coalescing for the programs parameter to make the intent clearer.
- More specific QueryOptions verification: Consider verifying that the repository is called with appropriate query options if that's important for the business logic.
private void SetUpRepositoryWrapper(List<DAL.Entities.Program> programs) { _mockRepositoryWrapper.Setup(x => x.ProgramsRepository - .GetAllAsync(It.IsAny<QueryOptions<DAL.Entities.Program>>())).ReturnsAsync(programs); + .GetAllAsync(It.IsAny<QueryOptions<DAL.Entities.Program>>())).ReturnsAsync(programs ?? new List<DAL.Entities.Program>()); }The current implementation works well for the test scenario, and the blob service mock appropriately returns a consistent value for testing purposes.
VictoryCenter/VictoryCenter.BLL/Constants/ProgramConstants.cs (1)
3-13: Optional: add XML summaries for IntelliSenseOne-line XML docs (e.g.,
/// <summary>Error message when program creation fails.</summary>) help callers quickly understand each constant, especially the length limits.VictoryCenter/VictoryCenter.BLL/DTOs/ProgramCategories/ProgramCategoryShortDto.cs (1)
3-7: Leveragerecordfor an idiomatic immutable DTODTOs are often pure data carriers. Converting to a
record(orrecord structif value-type semantics are desired) grants value equality, nice pattern-matching, and terser syntax:-public class ProgramCategoryShortDto -{ - public long Id { get; set; } - public required string Name { get; init; } -} +public readonly record struct ProgramCategoryShortDto(long Id, string Name);Adopt if this aligns with existing DTO conventions in the codebase.
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/ProgramCategories/CreateProgramCategoryTests.cs (5)
17-20: Inconsistent naming convention for validator field.The field
_validatorMockcontains an actual validator instance rather than a mock, which creates confusion. Consider renaming it to_validatorfor clarity.- private readonly IValidator<CreateProgramCategoryCommand> _validatorMock; + private readonly IValidator<CreateProgramCategoryCommand> _validator;And update the constructor accordingly:
- _validatorMock = new CreateProgramCategoryValidator(); + _validator = new CreateProgramCategoryValidator();
21-37: Test data setup contains unnecessary complexity.The
_programtest data includes aProgramscollection with nestedImageobjects that aren't used in most tests. This adds unnecessary complexity and potential confusion about what's being tested.Consider simplifying the test data to focus on what's actually needed:
private readonly ProgramCategory _program = new() { Id = 1, Name = "TestCategory", - CreatedAt = DateTime.UtcNow.AddMinutes(-10), - Programs = new List<DAL.Entities.Program> - { - new() - { - Image = new Image - { - BlobName = "someblob.jpg", - MimeType = "image/jpeg" - } - } - } + CreatedAt = DateTime.UtcNow.AddMinutes(-10) };
39-55: DTO test data mirrors unnecessary complexity.Similar to the entity, the DTO contains unused nested program data that doesn't contribute to testing the create functionality.
Simplify the DTO to match the simplified entity:
private readonly ProgramCategoryDto _programCategoryDto = new() { Id = 1, Name = "TestCategory", - CreatedAt = DateTime.UtcNow.AddMinutes(-10), - Programs = new List<ProgramDto> - { - new() - { - Image = new ImageDTO - { - BlobName = "someblob.jpg", - MimeType = "image/jpeg" - } - } - } + CreatedAt = DateTime.UtcNow.AddMinutes(-10) };
64-73: Test method follows good practices with minor improvements needed.The test correctly verifies both success status and returned data. Consider adding more specific assertions and using consistent field references.
[Fact] public async Task Handle_ShouldCreateProgramCategory() { SetupDependencies(); - var handler = new CreateProgramCategoryHandler(_mapperMock.Object, _repositoryWrapperMock.Object, _validatorMock); + var handler = new CreateProgramCategoryHandler(_mapperMock.Object, _repositoryWrapperMock.Object, _validator); var result = await handler .Handle(new CreateProgramCategoryCommand(new CreateProgramCategoryDto { Name = "TestCategory" }), CancellationToken.None); Assert.True(result.IsSuccess); - Assert.Equal(result.Value.Name, _programCategoryDto.Name); + Assert.Equal(_programCategoryDto.Name, result.Value.Name); + Assert.Equal(_programCategoryDto.Id, result.Value.Id); }
119-121: Repository mock setup has incomplete verification.The
CreateAsyncmethod setup doesn't verify that it's actually called, and there's no verification that the correct entity is passed to the repository.private void SetupRepositoryWrapper(int saveResult) { _repositoryWrapperMock.Setup(repo => repo.ProgramCategoriesRepository - .CreateAsync(It.IsAny<ProgramCategory>())); + .CreateAsync(It.IsAny<ProgramCategory>())) + .Verifiable(); _repositoryWrapperMock.Setup(repo => repo.SaveChangesAsync()).ReturnsAsync(saveResult); }Consider adding verification calls in your tests:
_repositoryWrapperMock.Verify(repo => repo.ProgramCategoriesRepository .CreateAsync(It.IsAny<ProgramCategory>()), Times.Once);VictoryCenter/VictoryCenter.BLL/DTOs/Programs/CreateProgramDto.cs (1)
4-11: Consider an immutablerecordfor DTO clarityDTOs are pure data carriers; defining them as
recordtypes cuts noise and enables value-based equality:-public class CreateProgramDto -{ - ... -} +public record CreateProgramDto( + required string Name, + string? Description, + Status Status, + long? ImageId, + List<long> CategoriesId);This is optional but improves readability and reduces mutation bugs.
VictoryCenter/VictoryCenter.BLL/Mapping/ProgramCategories/ProgramCategoriesProfile.cs (1)
7-16: AddReverseMap()where API returns DTO → entity conversionsIf any handler later needs to map
ProgramCategoryDtoback to the entity (e.g., for patching or cloning), chaining.ReverseMap()saves boilerplate and keeps configuration symmetric:-CreateMap<ProgramCategory, ProgramCategoryDto>(); +CreateMap<ProgramCategory, ProgramCategoryDto>() + .ReverseMap();Not mandatory today, but cheap to add and prevents future mapping misses.
VictoryCenter/VictoryCenter.DAL/Entities/Program.cs (1)
10-10: Store timestamps asDateTimeOffset(UTC) to avoid time-zone ambiguity
CreatedAtis currently a plainDateTime, which silently carries an unspecifiedKind. Persisting values with mixed or unknown kinds often leads to subtle bugs when data crosses service boundaries.- public DateTime CreatedAt { get; set; } + public DateTimeOffset CreatedAt { get; set; }If migration friction is a concern, at minimum consider enforcing
DateTimeKind.UtcinProgramConfig.VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/ProgramCategories/UpdateProgramCategoryTests.cs (2)
16-18: Consider making validator field readonly for consistency.The
_validatorfield should be marked as readonly like the other dependencies to maintain consistency and prevent accidental reassignment.- private IValidator<UpdateProgramCategoryCommand> _validator; + private readonly IValidator<UpdateProgramCategoryCommand> _validator;
76-76: Fix typo in test method name.There's a typo in the method name: "PtogramCategory" should be "ProgramCategory".
- public async Task Handle_ShouldUpdatePtogramCategory() + public async Task Handle_ShouldUpdateProgramCategory()VictoryCenter/VictoryCenter.BLL/Commands/Programs/Delete/DeleteProgramHandler.cs (1)
22-26: Consider the necessity of including Categories in the query.The query includes categories using
Include(p => p.Categories), but this is only used to clear the relationship on Line 34. Depending on your entity configuration, this eager loading might be unnecessary if EF Core can handle the cascade deletion automatically.Verify if the category relationship clearing is required based on your entity configuration:
#!/bin/bash # Description: Check entity configuration for Program-Category relationship to verify if manual clearing is needed # Expected: Find entity configurations showing relationship setup and cascade behavior ast-grep --pattern 'class Program$_Entity : $_' -A 20 ast-grep --pattern 'HasMany($_).WithMany($_)' -A 5 rg -A 5 "DeleteBehavior|OnDelete" --type csVictoryCenter/VictoryCenter.IntegrationTests/Utils/VictoryCenterWebApplicationFactory.cs (1)
111-124: Consider logging cleanup failures instead of silently ignoringWhile it's reasonable to not fail tests due to cleanup errors, completely swallowing exceptions makes debugging difficult when cleanup issues occur.
Consider at least tracing the errors:
private static void CleanupTestBlobDirectory() { try { if (Directory.Exists(TestBlobPath)) { Directory.Delete(TestBlobPath, true); } } - catch + catch (Exception ex) { - // Ignore cleanup errors in tests + // Log but don't fail tests on cleanup errors + System.Diagnostics.Trace.WriteLine($"Failed to cleanup test blob directory: {ex.Message}"); } }VictoryCenter/VictoryCenter.BLL/Validators/Programs/UpdateProgramValidator.cs (1)
10-10: Consider C# property naming conventions.The property name
updateProgramDtouses camelCase, but C# conventions typically use PascalCase for properties. Verify if this aligns with the actual property name inUpdateProgramCommand.If the property should follow PascalCase convention, update it to:
- RuleFor(x => x.updateProgramDto).SetValidator(baseProgramValidator); + RuleFor(x => x.UpdateProgramDto).SetValidator(baseProgramValidator);VictoryCenter/VictoryCenter.DAL/Data/EntityTypeConfigurations/ProgramCategoryConfig.cs (1)
19-21: Provide a DB-side default forCreatedAt
CreatedAtis required but nothing sets a default, so inserts that forget to populate it will fail.
Let SQL Server populate the timestamp and keep the application code simpler:- builder.Property(e => e.CreatedAt) - .IsRequired(); + builder.Property(e => e.CreatedAt) + .IsRequired() + .HasDefaultValueSql("GETUTCDATE()");If you prefer client-side timestamps, ensure all create commands set the value explicitly.
VictoryCenter/VictoryCenter.BLL/Mapping/Programs/ProgramsProfile.cs (1)
7-18: AddReverseMap()to reduce boilerplateFor most CRUD flows you’ll eventually need both directions (
Program ↔ ProgramDto). Adding.ReverseMap()to the existing mappings will save future effort and keep the configuration DRY.CreateMap<Program, ProgramDto>() … + .ReverseMap(); CreateMap<Program, PublishedProgramDto>() … + .ReverseMap();This has no runtime cost and avoids forgetting the reverse path later.
VictoryCenter/VictoryCenter.BLL/DTOs/Programs/PublishedProgramDto.cs (2)
13-13: Expose the category collection as read-onlyThe DTO intends
Categoriesto be an output list, but exposing a mutableList<T>with a public setter allows callers to replace the list entirely, breaking encapsulation. Returning anIReadOnlyCollection<ProgramCategoryShortDto>(and removing the setter) keeps the DTO immutable after mapping.- public List<ProgramCategoryShortDto> Categories { get; set; } = new List<ProgramCategoryShortDto>(); + public IReadOnlyCollection<ProgramCategoryShortDto> Categories { get; init; } = Array.Empty<ProgramCategoryShortDto>();
6-14: Consider making the DTO a C# 10 record for conciseness and immutabilityDTOs are pure data carriers; expressing them as
recordtypes provides value-based equality, a concise syntax, and encourages immutability. Example:-public class PublishedProgramDto -{ - public long Id { get; set; } - public string Name { get; set; } - public string Description { get; set; } - public DateTime CreatedAt { get; set; } - public ImageDTO? Image { get; set; } - public List<ProgramCategoryShortDto> Categories { get; set; } = new List<ProgramCategoryShortDto>(); -} +public record PublishedProgramDto( + long Id, + string Name, + string Description, + DateTimeOffset CreatedAt, + ImageDTO? Image, + IReadOnlyCollection<ProgramCategoryShortDto> Categories);Adopting records across new DTOs keeps the codebase modern and consistent.
VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/TeamMembersSeeder/TeamMemberUpdateSeeder.cs (1)
42-50: Consider making the Status enum casting more robust.The current implementation uses
(Status)(i % Enum.GetNames<Status>().Length)which assumes consecutive enum values starting from 0. Consider usingEnum.GetValues<Status>()for more reliable enum cycling.- Status = (Status)(i % Enum.GetNames<Status>().Length), + Status = Enum.GetValues<Status>()[i % Enum.GetValues<Status>().Length],VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/BaseSeeder.cs (1)
77-82: Consider refining the verification logic.The current verification
actual >= expectedallows for more entities than expected, which might mask issues if other seeders or existing data interfere. Consider tracking the initial count and verifying the exact increment.public virtual async Task<bool> VerifyAsync() { - var expected = _createdEntities.Count; - var actual = await _dbContext.Set<TEntity>().CountAsync(); - return actual >= expected; + var expectedIncrement = _createdEntities.Count; + if (expectedIncrement == 0) return true; + + var actualCount = await _dbContext.Set<TEntity>().CountAsync(); + // Verify we have at least the entities we created + return actualCount >= expectedIncrement; }VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/Delete/DeleteProgramTests.cs (1)
32-41: Comprehensive deletion test validationExcellent test implementation that verifies both the HTTP response and database state after deletion. The dual assertion on
response.IsSuccessStatusCode(lines 37 and 39) is redundant but harmless.response.EnsureSuccessStatusCode(); - -Assert.True(response.IsSuccessStatusCode);VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/TeamMembers/GetById/GetTeamMemberByIdTests.cs (1)
39-44: Remove debugging code once pipeline issue is resolved.The debugging output was likely added to investigate the pipeline failure. Consider removing this once the data seeding issue is resolved.
VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/CategoriesSeeder/CategoriesDataSeeder.cs (1)
47-49: Fix duplicate description for category 4Category 4 has the same description as category 3, which appears to be a copy-paste error.
new () { Name = "Test name4", - Description = "Test description3", + Description = "Test description4", CreatedAt = DateTime.UtcNow,VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/IntegrationTestsDatabaseSeeder.cs (2)
20-30: Consider dependency injection for seeder registrationThe hardcoded list of seeders reduces flexibility. Consider using dependency injection to register seeders, making it easier to add/remove seeders without modifying this class.
This would allow seeders to be registered in the test startup configuration, improving modularity and testability.
39-43: Optimize AddSeeder to avoid re-sortingRe-sorting the entire list after each addition is inefficient. Consider inserting at the correct position or sorting once after all seeders are added.
public void AddSeeder(ISeeder seeder) { - _seeders.Add(seeder); - _seeders = _seeders.OrderBy(s => s.Order).ToList(); + var index = _seeders.BinarySearch(seeder, Comparer<ISeeder>.Create((x, y) => x.Order.CompareTo(y.Order))); + if (index < 0) index = ~index; + _seeders.Insert(index, seeder); }VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Create/CreateProgramCategoryTests.cs (3)
29-29: Consider implementing cleanup in DisposeAsyncWhile returning
Task.CompletedTaskis valid, consider implementing cleanup logic to ensure test isolation. This could help prevent test pollution if subsequent tests don't properly initialize.- public Task DisposeAsync() => Task.CompletedTask; + public async Task DisposeAsync() + { + await _seederManager.DisposeAllAsync(); + }
38-41: Consider using HttpClient's built-in JSON methodsFor cleaner and more maintainable code, consider using
HttpClient's JSON extension methods instead of manual serialization.- var serializedDto = JsonConvert.SerializeObject(createProgramCategoryDto); - - var response = await _httpClient.PostAsync("/api/ProgramCategory/", new StringContent( - serializedDto, Encoding.UTF8, "application/json")); + var response = await _httpClient.PostAsJsonAsync("/api/ProgramCategory/", createProgramCategoryDto);Note: This requires adding
using System.Net.Http.Json;at the top of the file.
45-48: Add assertion for returned entity IDConsider verifying that the created category has a valid ID to ensure the entity was properly persisted.
Assert.Equal(HttpStatusCode.OK, response.StatusCode); Assert.NotNull(responseContent); Assert.Equal(createProgramCategoryDto.Name, responseContent.Name); + Assert.True(responseContent.Id > 0, "Created category should have a valid ID");VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/GetFiltered/GetByFilters.cs (3)
28-28: Consider implementing cleanup in DisposeAsyncAs with other test classes, consider adding cleanup logic for better test isolation.
- public Task DisposeAsync() => Task.CompletedTask; + public async Task DisposeAsync() + { + await _seederManager.DisposeAllAsync(); + }
36-42: Remove unused DTO creationThe
requestDtois created but never used. The query string is built manually instead. Either use the DTO or remove it for clarity.- ProgramFilterRequestDto requestDto = new() - { - Offset = offset, - Limit = limit, - Status = null, - CategoryId = null - }; - var query = new Dictionary<string, string?>
94-100: Remove redundant assertionLine 100 is redundant since
EnsureSuccessStatusCode()on line 94 already verifies success.response.EnsureSuccessStatusCode(); var content = await response.Content.ReadAsStringAsync(); var result = JsonConvert.DeserializeObject<ProgramsFilterResponseDto>(content); Assert.NotNull(result); - Assert.True(response.IsSuccessStatusCode);VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Programs/GetPrograms.cs (2)
131-131: Fix inconsistent spacing in constructor callMinor formatting inconsistency with extra spaces after
_blobService.Object.- var handler = new GetByFiltersHandler(_mockMapper.Object, _blobService.Object, _repositoryWrapper.Object); + var handler = new GetByFiltersHandler(_mockMapper.Object, _blobService.Object, _repositoryWrapper.Object);
166-166: Fix inconsistent spacing in constructor callSame formatting issue as line 131.
- var handler = new GetByFiltersHandler(_mockMapper.Object, _blobService.Object, _repositoryWrapper.Object); + var handler = new GetByFiltersHandler(_mockMapper.Object, _blobService.Object, _repositoryWrapper.Object);VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/Update/UpdateProgramTests.cs (1)
32-57: Add Status verification to update testThe test should verify that the program's status is preserved after the update, especially since Status is not included in the update DTO.
Assert.NotNull(responseContent); Assert.Equal(updateProgramDto.Name, responseContent.Name); Assert.Equal(updateProgramDto.Description, responseContent.Description); + Assert.Equal(Status.Published, responseContent.Status);VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/TeamMembersSeeder/TeamMemberSeeder.cs (1)
30-34: Add more descriptive error messageThe error message could be more helpful by indicating how many categories were actually found.
if (categories.Count < 2) { - throw new InvalidOperationException("At least 2 categories required to seed team members."); + throw new InvalidOperationException($"At least 2 categories required to seed team members, but found {categories.Count}."); }VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/Create/CreateProgramTests.cs (3)
11-12: Add class documentation for better test discoverabilityConsider adding XML documentation comments to describe the purpose of this test class and what functionality it covers. This helps other developers understand the test coverage at a glance.
+/// <summary> +/// Integration tests for the Program creation endpoint +/// </summary> [Collection("SharedIntegrationTests")] public class CreateProgramTests : IAsyncLifetime
24-28: Consider optimizing test data setup strategyThe current implementation disposes all seeded data and then re-seeds everything for each test. This could impact test performance as the test suite grows. Consider implementing a more targeted approach where only relevant data is reset.
public async Task InitializeAsync() { - await _seederManager.DisposeAllAsync(); - await _seederManager.SeedAllAsync(); + // Consider seeding only required data for Program tests + await _seederManager.DisposeAsync(typeof(ProgramSeeder)); + await _seederManager.SeedAsync(typeof(ProgramSeeder), typeof(ProgramCategoriesSeeder), typeof(ImagesDataSeeder)); }
41-41: Use collection expression for better readabilityConsider using the new C# 12 collection expression syntax for array initialization if your project supports it.
- CategoriesId = [1, 2] + CategoriesId = new[] { 1, 2 }VictoryCenter/VictoryCenter.WebAPI/Controllers/Programs/ProgramController.cs (1)
15-19: Consider adding OpenAPI documentation attributesAdd ProducesResponseType attributes to document possible response codes and types for better API documentation.
[HttpGet] +[ProducesResponseType(StatusCodes.Status200OK, Type = typeof(PagedResult<ProgramDto>))] +[ProducesResponseType(StatusCodes.Status400BadRequest)] public async Task<IActionResult> GetFilteredPrograms([FromQuery] ProgramFilterRequestDto requestDto) { return HandleResult(await Mediator.Send(new GetByFiltersQuery(requestDto))); }
| [Fact] | ||
| public async Task Handle_ShouldFindPrograms() | ||
| { | ||
| SetUpDependencies(_programEntities); | ||
| var handler = new GetPublishedProgramsHandler(_mapperMock.Object, _mockRepositoryWrapper.Object, _mockBlobService.Object); | ||
| var result = await handler.Handle(new GetPublishedProgramsQuery(), CancellationToken.None); | ||
| Assert.True(result.IsSuccess); | ||
| Assert.NotEmpty(result.Value); | ||
| Assert.NotNull(result); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test comprehensiveness and accuracy.
The test structure is good, but consider these improvements:
- More descriptive test name:
Handle_ShouldReturnSuccessWithPublishedProgramswould better describe what's being tested. - Remove redundant assertion: Line 69's
Assert.NotNull(result)is unnecessary since line 67 would fail first if result were null. - Add data verification: Consider asserting that the returned data matches expected values.
- Verify mock interactions: Add verification that repository and blob service methods were called appropriately.
[Fact]
-public async Task Handle_ShouldFindPrograms()
+public async Task Handle_ShouldReturnSuccessWithPublishedPrograms()
{
SetUpDependencies(_programEntities);
var handler = new GetPublishedProgramsHandler(_mapperMock.Object, _mockRepositoryWrapper.Object, _mockBlobService.Object);
var result = await handler.Handle(new GetPublishedProgramsQuery(), CancellationToken.None);
Assert.True(result.IsSuccess);
Assert.NotEmpty(result.Value);
- Assert.NotNull(result);
+ Assert.Equal(2, result.Value.Count());
}🤖 Prompt for AI Agents
In
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Programs/GetPublishedProgramsTests.cs
around lines 61 to 70, rename the test method to
Handle_ShouldReturnSuccessWithPublishedPrograms for clarity, remove the
redundant Assert.NotNull(result) since Assert.True(result.IsSuccess) already
ensures result is not null, add assertions to verify that the returned program
data matches expected values from the setup, and include verification calls to
confirm that the repository and blob service mocks were invoked as expected
during the handler execution.
| var response = await _httpClient.PutAsync("api/ProgramCategory/", new StringContent( | ||
| serializedDto, Encoding.UTF8, "application/json")); |
There was a problem hiding this comment.
Fix inconsistent API endpoint paths
The API endpoint paths are inconsistent across test methods. Line 41 uses "api/ProgramCategory/", line 67 uses "/api/ProgramCategory/", and line 86 uses "/api/ProgramCategories/" (plural).
- var response = await _httpClient.PutAsync("api/ProgramCategory/", new StringContent(
+ var response = await _httpClient.PutAsync("/api/ProgramCategory/", new StringContent(
serializedDto, Encoding.UTF8, "application/json"));And fix line 86:
- var response = await _httpClient.PutAsync("/api/ProgramCategories/", new StringContent(
+ var response = await _httpClient.PutAsync("/api/ProgramCategory/", new StringContent(
serializedDto, Encoding.UTF8, "application/json"));Also applies to: 67-68, 86-87
🤖 Prompt for AI Agents
In
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Update/UpdateProgramCategoryTests.cs
around lines 41-42, 67-68, and 86-87, the API endpoint paths are inconsistent in
their use of leading slashes and singular/plural forms. Standardize all endpoint
paths to use the same format, preferably "/api/ProgramCategory/" (singular with
leading slash), by updating the strings in the PutAsync calls to match this
consistent pattern.
| protected override async Task<List<Image>> GenerateEntitiesAsync() | ||
| { | ||
| foreach (var image in _images) | ||
| { | ||
| await service.SaveFileInStorageAsync(image.Base64, image.BlobName, image.MimeType); | ||
| await _blobService.SaveFileInStorageAsync(image.Base64, image.BlobName, image.MimeType); | ||
| } | ||
|
|
||
| return _images; | ||
| } |
There was a problem hiding this comment.
Add error handling for blob storage operations
The current implementation doesn't handle potential failures when saving to blob storage, which could cause tests to fail unexpectedly.
protected override async Task<List<Image>> GenerateEntitiesAsync()
{
+ var savedImages = new List<Image>();
foreach (var image in _images)
{
- await _blobService.SaveFileInStorageAsync(image.Base64, image.BlobName, image.MimeType);
+ try
+ {
+ await _blobService.SaveFileInStorageAsync(image.Base64, image.BlobName, image.MimeType);
+ savedImages.Add(image);
+ }
+ catch (Exception ex)
+ {
+ _logger.LogError(ex, "Failed to save image {BlobName} to storage", image.BlobName);
+ // Consider whether to continue or fail fast
+ throw;
+ }
}
- return _images;
+ return savedImages;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected override async Task<List<Image>> GenerateEntitiesAsync() | |
| { | |
| foreach (var image in _images) | |
| { | |
| await service.SaveFileInStorageAsync(image.Base64, image.BlobName, image.MimeType); | |
| await _blobService.SaveFileInStorageAsync(image.Base64, image.BlobName, image.MimeType); | |
| } | |
| return _images; | |
| } | |
| protected override async Task<List<Image>> GenerateEntitiesAsync() | |
| { | |
| var savedImages = new List<Image>(); | |
| foreach (var image in _images) | |
| { | |
| try | |
| { | |
| await _blobService.SaveFileInStorageAsync(image.Base64, image.BlobName, image.MimeType); | |
| savedImages.Add(image); | |
| } | |
| catch (Exception ex) | |
| { | |
| _logger.LogError(ex, "Failed to save image {BlobName} to storage", image.BlobName); | |
| // Consider whether to continue or fail fast | |
| throw; | |
| } | |
| } | |
| return savedImages; | |
| } |
🤖 Prompt for AI Agents
In
VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/ImageSeeder/ImagesSeeder.cs
around lines 44 to 52, the method SaveFileInStorageAsync is called without error
handling, risking unhandled exceptions during blob storage operations. Wrap the
await call to SaveFileInStorageAsync in a try-catch block to catch any
exceptions, log or handle the error appropriately, and decide whether to
continue or fail gracefully to prevent unexpected test failures.
| var selectedCategories = categories.Take(2).ToList(); | ||
|
|
||
| var teamMembers = new List<TeamMember>(); | ||
|
|
||
| for (int i = 0; i < TeamMemberCount; i++) | ||
| { | ||
| var teamMember = new TeamMember | ||
| var category = selectedCategories[i % selectedCategories.Count]; | ||
| teamMembers.Add(new TeamMember | ||
| { | ||
| FullName = $"FirstName{i} LastName{i}", | ||
| CategoryId = categories[i % (categories.Count - 1)].Id, | ||
| CategoryId = category.Id, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify impact on delete test isolation
The seeding strategy has changed from the previous implementation that intentionally excluded the last category (as noted in retrieved learnings). The new approach only uses the first 2 categories, which might affect delete tests that rely on having categories without team members.
Please verify this change doesn't break existing delete tests that depend on having an empty category.
🏁 Script executed:
#!/bin/bash
# Description: Check for category delete tests that might be affected by this seeding change
# Search for category delete tests
rg -A 10 "Delete.*Category" --glob "**/*Test*.cs"
# Search for tests that might rely on empty categories
rg -A 5 "category.*without.*member|empty.*category|no.*team.*member" --glob "**/*Test*.cs" -iLength of output: 48387
I've reviewed the integration‐test seeders for Categories and TeamMembers:
- CategorySeeder creates 5 categories (IDs 1–5).
- TeamMemberSeeder now only assigns members to categories 1 and 2 (first two), leaving 3–5 empty.
In the DeleteCategory controller tests, the code grabs the first category (ID 1) and deletes it, expecting success. Under the new seeding, category 1 still has team members, so deletion will fail (foreign‐key constraint or handler guard). To restore test isolation for delete scenarios, at least one seedable category—preferably the last—must remain free of team members.
Action items:
• In TeamMemberSeeder.cs, keep the previous strategy of excluding exactly one category (e.g. last) by using categories.Take(categories.Count - 1) or dynamically excluding the last ID – that ensures a deletable category exists.
• Alternatively, modify the delete integration test to select a category that you know has no members (e.g. the last category by ordering).
Please adjust the seeding logic or tests accordingly so DeleteCategoryTests continue to pass.
🤖 Prompt for AI Agents
In
VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/TeamMembersSeeder/TeamMemberSeeder.cs
around lines 36 to 46, the current seeding assigns team members only to the
first two categories, leaving others empty, which breaks the DeleteCategory
integration tests that expect at least one category without members. To fix
this, modify the seeding logic to assign team members to all but the last
category by using categories.Take(categories.Count - 1) instead of
categories.Take(2), ensuring the last category remains free of team members and
deletable. Alternatively, update the delete test to target a category known to
have no members, but adjusting the seeder is preferred for test isolation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryBase.cs (2)
20-21: CountAsync addition looks good; consider long counts and document semanticsSolid addition for pagination and total counts. Two improvements to future-proof and clarify:
- Very large tables: EF Core offers LongCountAsync. Consider adding a companion LongCountAsync to avoid potential int overflow on big data sets.
- Semantics: Count should ignore Include and OrderBy options and always be no-tracking. Document this to avoid accidental performance regressions.
Proposed interface updates:
public interface IRepositoryBase<T> where T : class { @@ - Task<int> CountAsync(QueryOptions<T>? queryOptions = null); + /// <summary> + /// Returns total number of entities matching the provided QueryOptions (filters). + /// Implementations should ignore Include/OrderBy for count and use AsNoTracking. + /// </summary> + Task<int> CountAsync(QueryOptions<T>? queryOptions = null); + + /// <summary> + /// Same as CountAsync but returns a 64-bit count for very large datasets. + /// </summary> + Task<long> LongCountAsync(QueryOptions<T>? queryOptions = null);
20-21: Cancellation tokens (follow-up suggestion for consistency, not a blocker)Long-running queries benefit from cancellation. If/when you introduce CancellationToken across the repository interface, add it here too for consistency. Recommend doing this as a cohesive follow-up across all async methods rather than only CountAsync.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
VictoryCenter/VictoryCenter.BLL/Commands/Programs/Create/CreateProgramHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Programs/Update/UpdateProgramHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Constants/ProgramConstants.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetByFilters/GetByFiltersHandler.cs(1 hunks)VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryBase.cs(1 hunks)VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryBase.cs(4 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Create/CreateProgramCategoryTests.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Delete/DeleteProgramCategoryTests.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/GetAll/GetAllProgramCategoriesTests.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Update/UpdateProgramCategoryTests.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/Create/CreateProgramTests.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/GetById/GetProgramById.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/GetFiltered/GetByFilters.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/GetPublished/GetPublishedPrograms.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/Update/UpdateProgramTests.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/ProgramSeeder/ProgramDataSeeder.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/Programs/CreateProgramValidatorTests.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/Programs/UpdateProgramValidatorTests.cs(1 hunks)VictoryCenter/VictoryCenter.WebAPI/Controllers/Programs/ProgramController.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/GetById/GetProgramById.cs
- VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/Create/CreateProgramTests.cs
- VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/Update/UpdateProgramTests.cs
- VictoryCenter/VictoryCenter.BLL/Commands/Programs/Update/UpdateProgramHandler.cs
- VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetByFilters/GetByFiltersHandler.cs
- VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/GetAll/GetAllProgramCategoriesTests.cs
- VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/GetFiltered/GetByFilters.cs
- VictoryCenter/VictoryCenter.BLL/Commands/Programs/Create/CreateProgramHandler.cs
- VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/ProgramSeeder/ProgramDataSeeder.cs
- VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/GetPublished/GetPublishedPrograms.cs
- VictoryCenter/VictoryCenter.BLL/Constants/ProgramConstants.cs
- VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryBase.cs
- VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/Programs/CreateProgramValidatorTests.cs
- VictoryCenter/VictoryCenter.WebAPI/Controllers/Programs/ProgramController.cs
- VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Delete/DeleteProgramCategoryTests.cs
- VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/Programs/UpdateProgramValidatorTests.cs
- VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Create/CreateProgramCategoryTests.cs
- VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Update/UpdateProgramCategoryTests.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and analyze
- GitHub Check: Build and analyze
| Task<int> CountAsync(QueryOptions<T>? queryOptions = null); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Quick check: ensure implementation ignores Includes and uses no-tracking
Please verify the concrete implementation aligns with the intended semantics (ignore Include/OrderBy, apply AsNoTracking). Script below searches for CountAsync implementation and looks for AsNoTracking usage.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate interface method"
rg -n "Task<\s*int\s*>\s*CountAsync\s*\(" -A 2 VictoryCenter
echo
echo "Locate repository implementation(s) of CountAsync"
rg -n "CountAsync\s*\(" -A 30 VictoryCenter | sed -n '1,200p'
echo
echo "Check AsNoTracking usage near CountAsync implementation"
rg -n "CountAsync" -A 30 VictoryCenter | rg -n "AsNoTracking" -n -A 0Length of output: 14667
Apply AsNoTracking in CountAsync
The CountAsync method in RepositoryBase<T> currently builds a tracking query and only applies filters—so it already ignores any Includes/OrderBy in QueryOptions<T>, but it omits no-tracking. To align with intended semantics and avoid unnecessary change-tracking overhead, update the initial query:
• File: VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryBase.cs (around line 58)
• Change:
- IQueryable<T> query = _dbContext.Set<T>();
+ IQueryable<T> query = _dbContext.Set<T>().AsNoTracking();This ensures only the filter is applied and the rest of the query remains untracked.
🤖 Prompt for AI Agents
In VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryBase.cs around
line 58, the CountAsync method builds a query that applies filters but does not
use AsNoTracking; update the initial query construction to call AsNoTracking()
so the count executes without change-tracking (preserving the semantics of
ignoring Includes/OrderBy and avoiding tracking overhead) and ensure only the
filter from QueryOptions<T> is applied before calling CountAsync.
There was a problem hiding this comment.
Actionable comments posted: 20
🔭 Outside diff range comments (2)
VictoryCenter/VictoryCenter.IntegrationTests/MiddlewareTests/ExceptionHandlingMiddlewareTests.cs (2)
45-64: Assert the log level to match the test’s intent (Critical level)The test name promises Critical-level logging but doesn’t assert it. Add an explicit check:
var log = _loggerProvider.Entries.Last(e => e.Category == categoryName); - Assert.Contains("Unhandled exception occured while processing request", log.Message); + Assert.Equal(LogLevel.Critical, log.Level); + Assert.Contains("Unhandled exception occured while processing request", log.Message);
20-43: Ensure Proper Disposal of Derived Factory and Strengthen Test AssertionsWe’ve confirmed that the switch to
fixture._factoryis consistently applied across the suite (no remainingfixture.Factoryreferences). Below are actionable improvements to prevent resource leaks and make your assertions more robust:• Prevent factory leaks
•WithWebHostBuilder(...)returns a disposableWebApplicationFactory<T>that you own—addIDisposableto your test class and dispose_customFactoryinDispose().
• Explicitly assert the intended log level
• Your test name mentions “Critical” logging—addAssert.Equal(LogLevel.Critical, log.LogLevel).
• Make log‐message assertions resilient
• Avoid exact matches on typos; useAssert.Contains("error occurred", log.Message, StringComparison.OrdinalIgnoreCase)or a regex.
• Improve readability with HTTP enums and content‐type checks
• UseHttpStatusCode.InternalServerErrorinstead of magic numbers.
• Optionally assertresponse.Content.Headers.ContentType.MediaType == "application/json".Locations to update
- VictoryCenter.IntegrationTests/MiddlewareTests/ExceptionHandlingMiddlewareTests.cs
- (If applicable) VictoryCenter.IntegrationTests/MiddlewareTests/RequestResponseLoggingMiddlewareTests.cs
Suggested diff for ExceptionHandlingMiddlewareTests.cs
-public class ExceptionHandlingMiddlewareTests +public class ExceptionHandlingMiddlewareTests : IDisposable { private readonly HttpClient _client; private readonly InMemoryLoggerProvider _loggerProvider; - private readonly IDisposable _customFactory; + private readonly WebApplicationFactory<Startup> _customFactory; public ExceptionHandlingMiddlewareTests(IntegrationTestDbFixture fixture) { var customFactory = fixture._factory.WithWebHostBuilder(builder => { builder.ConfigureLogging(logging => { logging.ClearProviders(); logging.AddProvider(new InMemoryLoggerProvider()); }); builder.ConfigureServices(services => { services .AddControllers() .AddApplicationPart(typeof(FakeErrorController).Assembly) .AddControllersAsServices(); }); }); _client = customFactory.CreateClient(); _loggerProvider = customFactory.Services .GetServices<ILoggerProvider>() .OfType<InMemoryLoggerProvider>() .Single(); _customFactory = customFactory; } + public void Dispose() + { + _customFactory.Dispose(); + } [Fact] public async Task MiddleWare_Should_Log_Critical_When_Error_Thrown() { - var response = await _client.GetAsync("/fake-error"); - var log = _loggerProvider.Logs.Single(); - Assert.Contains("Unhandled exception occured", log.Message); + var response = await _client.GetAsync("/fake-error"); + Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode); + Assert.Equal("application/json", response.Content.Headers.ContentType.MediaType); + + var log = _loggerProvider.Logs.Single(); + Assert.Equal(LogLevel.Critical, log.LogLevel); + Assert.Contains("error occurred", log.Message, StringComparison.OrdinalIgnoreCase); } }
♻️ Duplicate comments (6)
VictoryCenter/VictoryCenter.IntegrationTests/Utils/VictoryCenterWebApplicationFactory.cs (3)
14-14: Consider making TestBlobPath an instance field for better test isolationThe static
TestBlobPathfield could cause test interference when multiple test classes run in parallel. Each test class would share the same blob path, potentially leading to race conditions or data corruption.Consider making this an instance field to ensure each test has its own unique blob path:
- private static readonly string TestBlobPath = Path.Combine(Path.GetTempPath(), "VictoryCenter_IntegrationTests_Blobs", Guid.NewGuid().ToString()); + private readonly string TestBlobPath = Path.Combine(Path.GetTempPath(), "VictoryCenter_IntegrationTests_Blobs", Guid.NewGuid().ToString());
102-112: Fix memory leak by properly disposing efServiceProviderThe
efServiceProvidercreated at line 104 is never disposed, which could lead to memory leaks during test runs.Store and dispose it properly:
+ private ServiceProvider? _efServiceProvider; + private void AddTestDbContext(IServiceCollection services) { - var efServiceProvider = new ServiceCollection() + _efServiceProvider = new ServiceCollection() .AddEntityFrameworkInMemoryDatabase() .BuildServiceProvider(); services.AddDbContext<VictoryCenterDbContext>(options => { options.UseInMemoryDatabase(_databaseName) - .UseInternalServiceProvider(efServiceProvider); + .UseInternalServiceProvider(_efServiceProvider); }); }Then dispose it in the Dispose method:
protected override void Dispose(bool disposing) { if (disposing) { CleanupTestBlobDirectory(); + _efServiceProvider?.Dispose(); } base.Dispose(disposing); }
115-128: Update method signature if TestBlobPath becomes an instance fieldIf you implement the earlier suggestion to make
TestBlobPathan instance field, this method should also become an instance method:- private static void CleanupTestBlobDirectory() + private void CleanupTestBlobDirectory()VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Payments/PaymentsControllerTests.cs (1)
39-39: Apply consistent naming convention fixSame issue as noted in other files - accessing
_factorydirectly violates naming conventions.Once the property is added to
IntegrationTestDbFixture, update this line:- var client = _fixture._factory.WithWebHostBuilder(builder => + var client = _fixture.Factory.WithWebHostBuilder(builder =>VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/TeamMembers/Update/UpdateTeamMemberTests.cs (1)
45-59: Fix inconsistent seeder manager usageThere's inconsistent usage of seeder managers -
_seederManageron line 48 but_fixture.SeederManageron line 55. This could lead to unexpected behavior.Apply this diff to use consistent seeder manager instance:
-if (!await _fixture.SeederManager.SeedAllAsync()) +if (!await _seederManager.SeedAllAsync())VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Update/UpdateProgramCategoryTests.cs (1)
81-81: Fix incorrect API endpoint pathThe endpoint uses plural form "ProgramCategories" while other tests use singular "ProgramCategory". This inconsistency will cause the test to fail.
- var response = await _fixture.HttpClient.PutAsync("/api/ProgramCategories/", new StringContent( + var response = await _fixture.HttpClient.PutAsync("/api/ProgramCategory/", new StringContent(
🧹 Nitpick comments (8)
VictoryCenter/VictoryCenter.IntegrationTests/MiddlewareTests/ExceptionHandlingMiddlewareTests.cs (2)
63-64: Make the log message assertion resilient (avoid coupling to typo and exact text)Current assertion hard-codes a misspelling (“occured”). If the middleware message is corrected, this test will fail. Prefer a less brittle check:
- Assert.Contains("Unhandled exception occured while processing request", log.Message); + // Be resilient to typo fixes and minor text changes + Assert.Contains("Unhandled exception", log.Message);Optionally, assert EventId or state/exception presence if your InMemoryLogger captures them.
51-51: Use HttpStatusCode enum for readabilitySmall readability win:
- Assert.Equal(500, (int)response.StatusCode); + Assert.Equal(System.Net.HttpStatusCode.InternalServerError, response.StatusCode);VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Images/Create/CreateImageTests.cs (1)
20-20: Consider improving encapsulationAccessing
_blobEnvironmentVariablesdirectly from the fixture violates encapsulation principles. Consider exposing this through a public property instead of accessing the field directly.In the fixture class, expose the blob environment variables through a property:
-public readonly BlobEnvironmentVariables _blobEnvironmentVariables; +private readonly BlobEnvironmentVariables _blobEnvironmentVariables; +public BlobEnvironmentVariables BlobEnvironmentVariables => _blobEnvironmentVariables;Then update this line:
-_blobEnvironment = fixture._blobEnvironmentVariables; +_blobEnvironment = fixture.BlobEnvironmentVariables;VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/GetPublished/GetPublishedPrograms.cs (1)
1-1: Consider JSON serialization consistencyThe test uses
JsonConvert(Newtonsoft.Json) while other tests in the codebase useSystem.Text.Json. Consider standardizing on one JSON library across all tests for consistency.Also applies to: 30-30
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/TeamMembers/GetById/GetTeamMemberByIdTests.cs (1)
43-51: Consider removing or conditionalizing debug loggingThis extensive logging appears to be debug code that should be removed or wrapped in a conditional flag. It adds unnecessary noise to test output during normal test runs.
- var all = await _fixture.DbContext.TeamMembers - .Include(tm => tm.Category) - .ToListAsync(); - - _output.WriteLine($"Found {all.Count} team members:"); - foreach (var a in all) - { - _output.WriteLine($"ID: {a.Id}, Name: {a.FullName}, CategoryId: {a.CategoryId}"); - } + // Remove debug logging or wrap in conditional + // if (Environment.GetEnvironmentVariable("DEBUG_TESTS") == "true") + // { + // var all = await _fixture.DbContext.TeamMembers... + // }VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Delete/DeleteProgramCategoryTests.cs (1)
32-34: Remove redundant success checkLine 32 calls
EnsureSuccessStatusCode()which throws if unsuccessful, making the assertion on line 34 redundant.var response = await _fixture.HttpClient.DeleteAsync($"/api/ProgramCategory/{existingEntity.Id}"); - response.EnsureSuccessStatusCode(); Assert.True(response.IsSuccessStatusCode);VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/IntegrationTestsDatabaseSeeder.cs (2)
33-37: Optimize seeder reordering for better performance.The
AddSeedermethod reorders the entire collection on every addition, which has O(n log n) complexity. For multiple additions, this becomes inefficient.Consider batching operations or using a more efficient approach:
public void AddSeeder(ISeeder seeder) { + if (seeder == null) throw new ArgumentNullException(nameof(seeder)); _seeders.Add(seeder); - _seeders = _seeders.OrderBy(s => s.Order).ToList(); + // Sort only when needed, or provide a separate method to finalize order } + +public void FinalizeSeederOrder() +{ + _seeders = _seeders.OrderBy(s => s.Order).ToList(); +}Alternatively, consider using
SortedSet<ISeeder>with a custom comparer for automatic ordering.
62-69: Consider using a more maintainable approach for default seeders.The hardcoded seeder instantiation makes it difficult to modify the default seeder list and creates tight coupling with specific seeder implementations.
Consider using a factory pattern or configuration-based approach:
public IEnumerable<ISeeder> CreateDefaultSeeders() { - yield return new CategoriesSeeder.CategoriesSeeder(_dbContext, _loggerFactory.CreateLogger<CategoriesSeeder.CategoriesSeeder>(), _blobService); - yield return new TeamMembersSeeder.TeamMembersSeeder(_dbContext, _loggerFactory.CreateLogger<TeamMembersSeeder.TeamMembersSeeder>(), _blobService); - yield return new ImageSeeder.ImagesDataSeeder(_dbContext, _loggerFactory.CreateLogger<ImageSeeder.ImagesDataSeeder>(), _blobService); - yield return new ProgramSeeder.ProgramSeeder(_dbContext, _loggerFactory.CreateLogger<ProgramSeeder.ProgramSeeder>(), _blobService); - yield return new ProgramCategoriesSeeder.ProgramCategoriesSeeder(_dbContext, _loggerFactory.CreateLogger<ProgramCategoriesSeeder.ProgramCategoriesSeeder>(), _blobService); + var seederTypes = new[] + { + typeof(CategoriesSeeder.CategoriesSeeder), + typeof(TeamMembersSeeder.TeamMembersSeeder), + typeof(ImageSeeder.ImagesDataSeeder), + typeof(ProgramSeeder.ProgramSeeder), + typeof(ProgramCategoriesSeeder.ProgramCategoriesSeeder) + }; + + foreach (var seederType in seederTypes) + { + var logger = _loggerFactory.CreateLogger(seederType); + yield return (ISeeder)Activator.CreateInstance(seederType, _dbContext, logger, _blobService)!; + } }This approach makes it easier to configure seeders externally or add new ones without modifying this method.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
VictoryCenter/VictoryCenter.BLL/Queries/TeamMembers/GetById/GetTeamMemberByIdHandler.cs(0 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Base/IntegrationTestDbFixture.cs(4 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Categories/Create/CreateCategoryTests.cs(3 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Categories/Delete/DeleteCategoryTests.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Categories/Update/UpdateCategoryTests.cs(4 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Images/Create/CreateImageTests.cs(2 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Images/Delete/DeleteImageTests.cs(2 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Images/GetById/GetImageByIdTest.cs(2 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Images/GetByName/GetImageByNameTest.cs(2 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Images/Update/UpdateImageTest.cs(5 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Payments/PaymentsControllerTests.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Create/CreateProgramCategoryTests.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Delete/DeleteProgramCategoryTests.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/GetAll/GetAllProgramCategoriesTests.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Update/UpdateProgramCategoryTests.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/Delete/DeleteProgramTests.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/GetById/GetProgramById.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/GetPublished/GetPublishedPrograms.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/Update/UpdateProgramTests.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/TeamMembers/GetById/GetTeamMemberByIdTests.cs(2 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/TeamMembers/Update/UpdateTeamMemberTests.cs(3 hunks)VictoryCenter/VictoryCenter.IntegrationTests/MiddlewareTests/ExceptionHandlingMiddlewareTests.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/MiddlewareTests/RequestResponseLoggingMiddlewareTests.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/IntegrationTestsDatabaseSeeder.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/ProgramCategoriesSeeder/ProgramCategoriesDataSeeder.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/Utils/VictoryCenterWebApplicationFactory.cs(4 hunks)VictoryCenter/VictoryCenter.WebAPI/Extensions/ServicesConfiguration.cs(0 hunks)
💤 Files with no reviewable changes (2)
- VictoryCenter/VictoryCenter.WebAPI/Extensions/ServicesConfiguration.cs
- VictoryCenter/VictoryCenter.BLL/Queries/TeamMembers/GetById/GetTeamMemberByIdHandler.cs
🚧 Files skipped from review as they are similar to previous changes (5)
- VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/Delete/DeleteProgramTests.cs
- VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/Update/UpdateProgramTests.cs
- VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/ProgramCategoriesSeeder/ProgramCategoriesDataSeeder.cs
- VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Create/CreateProgramCategoryTests.cs
- VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/GetById/GetProgramById.cs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-17T20:32:16.009Z
Learnt from: VladimirSushinsky
PR: ita-social-projects/VictoryCenter-Back#114
File: VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/TeamMembersSeeder/TeamMemberSeeder.cs:20-24
Timestamp: 2025-06-17T20:32:16.009Z
Learning: In the VictoryCenter test seeder for TeamMember entities, the last category in the categories list is intentionally excluded from having team members assigned to it (using `categories[i % (categories.Count - 1)].Id`). This design ensures that the last category remains available for delete tests without foreign key constraint violations, as delete operations require categories with no related team members.
Applied to files:
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Categories/Update/UpdateCategoryTests.csVictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/GetAll/GetAllProgramCategoriesTests.csVictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Categories/Create/CreateCategoryTests.csVictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Categories/Delete/DeleteCategoryTests.csVictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/TeamMembers/Update/UpdateTeamMemberTests.csVictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Delete/DeleteProgramCategoryTests.cs
📚 Learning: 2025-06-27T08:50:52.032Z
Learnt from: NovickVitaliy
PR: ita-social-projects/VictoryCenter-Back#201
File: VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Base/IntegrationTestDbFixture.cs:48-48
Timestamp: 2025-06-27T08:50:52.032Z
Learning: In the VictoryCenter project integration tests, using an empty claims array for CreateAccessToken in the IntegrationTestDbFixture is sufficient for authentication purposes, as confirmed by the project maintainer.
Applied to files:
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Base/IntegrationTestDbFixture.cs
📚 Learning: 2025-07-02T13:57:29.340Z
Learnt from: NovickVitaliy
PR: ita-social-projects/VictoryCenter-Back#201
File: VictoryCenter/VictoryCenter.BLL/Commands/Auth/RefreshToken/RefreshTokenCommandHandler.cs:56-59
Timestamp: 2025-07-02T13:57:29.340Z
Learning: In the VictoryCenter codebase using ASP.NET Core Identity, the UserManager<Admin>.GetClaimsAsync() method does not return the email claim automatically. The email claim must be explicitly added when creating JWT tokens if it's needed, as shown in the RefreshTokenCommandHandler where the email claim is manually added to the claims array passed to CreateAccessToken.
Applied to files:
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Base/IntegrationTestDbFixture.cs
🧬 Code Graph Analysis (16)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Images/GetById/GetImageByIdTest.cs (3)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Base/IntegrationTestDbFixture.cs (6)
IntegrationTestDbFixture(20-146)IntegrationTestDbFixture(29-42)Task(48-51)Task(53-68)Task(70-109)Task(121-145)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Images/GetByName/GetImageByNameTest.cs (4)
Task(25-28)Task(30-30)Fact(32-46)Fact(48-57)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/IntegrationTestsDatabaseSeeder.cs (2)
Task(39-52)Task(54-60)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Update/UpdateProgramCategoryTests.cs (5)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Create/CreateProgramCategoryTests.cs (5)
Collection(9-66)Task(19-22)Task(24-24)Fact(26-46)Theory(48-65)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Delete/DeleteProgramCategoryTests.cs (5)
Collection(7-48)Task(17-20)Task(22-22)Fact(24-36)Theory(38-47)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/GetAll/GetAllProgramCategoriesTests.cs (4)
Collection(7-33)Task(16-19)Task(21-21)Fact(23-32)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/ProgramCategoriesSeeder/ProgramCategoriesDataSeeder.cs (2)
Task(17-20)Task(22-58)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/ProgramSeeder/ProgramDataSeeder.cs (2)
Task(20-23)Task(25-47)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/GetPublished/GetPublishedPrograms.cs (4)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Base/IntegrationTestDbFixture.cs (2)
IntegrationTestDbFixture(20-146)IntegrationTestDbFixture(29-42)VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetByFilters/GetByFiltersHandler.cs (1)
Task(29-72)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/IntegrationTestsDatabaseSeeder.cs (1)
IEnumerable(62-69)VictoryCenter/VictoryCenter.BLL/DTOs/Programs/ProgramDto.cs (1)
ProgramDto(7-19)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Images/GetByName/GetImageByNameTest.cs (3)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Base/IntegrationTestDbFixture.cs (6)
IntegrationTestDbFixture(20-146)IntegrationTestDbFixture(29-42)Task(48-51)Task(53-68)Task(70-109)Task(121-145)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Images/GetById/GetImageByIdTest.cs (4)
Task(25-28)Task(30-30)Fact(32-46)Fact(48-57)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/IntegrationTestsDatabaseSeeder.cs (2)
Task(39-52)Task(54-60)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Categories/Update/UpdateCategoryTests.cs (3)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Base/IntegrationTestDbFixture.cs (6)
IntegrationTestDbFixture(20-146)IntegrationTestDbFixture(29-42)Task(48-51)Task(53-68)Task(70-109)Task(121-145)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/TeamMembersSeeder/TeamMemberSeeder.cs (2)
Task(23-26)Task(28-54)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Categories/GetAll/GetAllCategoriesTests.cs (2)
Task(28-32)Task(34-34)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/GetAll/GetAllProgramCategoriesTests.cs (3)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Base/IntegrationTestDbFixture.cs (2)
IntegrationTestDbFixture(20-146)IntegrationTestDbFixture(29-42)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/ProgramCategoriesSeeder/ProgramCategoriesDataSeeder.cs (2)
Task(17-20)Task(22-58)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/IntegrationTestsDatabaseSeeder.cs (1)
IEnumerable(62-69)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Categories/Create/CreateCategoryTests.cs (1)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Base/IntegrationTestDbFixture.cs (6)
IntegrationTestDbFixture(20-146)IntegrationTestDbFixture(29-42)Task(48-51)Task(53-68)Task(70-109)Task(121-145)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Images/Delete/DeleteImageTests.cs (5)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Base/IntegrationTestDbFixture.cs (6)
IntegrationTestDbFixture(20-146)IntegrationTestDbFixture(29-42)Task(48-51)Task(53-68)Task(70-109)Task(121-145)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/IntegrationTestsDatabaseSeeder.cs (2)
Task(39-52)Task(54-60)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/TeamMembersSeeder/TeamMemberSeeder.cs (2)
Task(23-26)Task(28-54)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/CategoriesSeeder/CategoriesDataSeeder.cs (2)
Task(18-21)Task(23-55)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/ImageSeeder/ImagesSeeder.cs (2)
Task(44-52)Task(54-55)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Categories/Delete/DeleteCategoryTests.cs (1)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Base/IntegrationTestDbFixture.cs (2)
IntegrationTestDbFixture(20-146)IntegrationTestDbFixture(29-42)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/TeamMembers/Update/UpdateTeamMemberTests.cs (5)
VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/CategoriesSeeder/CategoriesDataSeeder.cs (2)
CategoriesSeeder(8-56)CategoriesSeeder(10-13)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/IntegrationTestsDatabaseSeeder.cs (6)
SeederManager(7-70)SeederManager(15-25)Task(39-52)Task(54-60)ClearSeeders(27-28)ConfigureSeeders(30-31)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Base/IntegrationTestDbFixture.cs (6)
IntegrationTestDbFixture(20-146)IntegrationTestDbFixture(29-42)Task(48-51)Task(53-68)Task(70-109)Task(121-145)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/TeamMembersSeeder/TeamMemberSeeder.cs (2)
Task(23-26)Task(28-54)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/TeamMembersSeeder/TeamMemberUpdateSeeder.cs (2)
TeamMemberUpdateSeeder(10-55)TeamMemberUpdateSeeder(14-17)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/TeamMembers/GetById/GetTeamMemberByIdTests.cs (2)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Base/IntegrationTestDbFixture.cs (2)
IntegrationTestDbFixture(20-146)IntegrationTestDbFixture(29-42)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/IntegrationTestsDatabaseSeeder.cs (2)
SeederManager(7-70)SeederManager(15-25)
VictoryCenter/VictoryCenter.IntegrationTests/Utils/VictoryCenterWebApplicationFactory.cs (2)
VictoryCenter/VictoryCenter.UnitTests/ServiceTests/BlobService.cs (1)
Dispose(100-106)VictoryCenter/VictoryCenter.WebAPI/Extensions/ServicesConfiguration.cs (1)
IServiceCollection(249-273)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Delete/DeleteProgramCategoryTests.cs (4)
VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryBase.cs (1)
Delete(18-18)VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryBase.cs (1)
Delete(75-78)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Base/IntegrationTestDbFixture.cs (2)
IntegrationTestDbFixture(20-146)IntegrationTestDbFixture(29-42)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/ProgramCategoriesSeeder/ProgramCategoriesDataSeeder.cs (2)
Task(17-20)Task(22-58)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Base/IntegrationTestDbFixture.cs (4)
VictoryCenter/VictoryCenter.IntegrationTests/Utils/VictoryCenterWebApplicationFactory.cs (3)
VictoryCenterWebApplicationFactory(11-129)VictoryCenterWebApplicationFactory(17-20)Dispose(64-72)VictoryCenter/VictoryCenter.DAL/Data/VictoryCenterDbContext.cs (2)
VictoryCenterDbContext(8-30)VictoryCenterDbContext(10-13)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/IntegrationTestsDatabaseSeeder.cs (2)
SeederManager(7-70)SeederManager(15-25)VictoryCenter/VictoryCenter.BLL/Services/TokenService/TokenService.cs (1)
CreateAccessToken(32-52)
VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/IntegrationTestsDatabaseSeeder.cs (7)
VictoryCenter/VictoryCenter.DAL/Data/VictoryCenterDbContext.cs (2)
VictoryCenterDbContext(8-30)VictoryCenterDbContext(10-13)VictoryCenter/VictoryCenter.IntegrationTests/Utils/InMemoryLoggerProvider.cs (1)
ILogger(10-11)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/TeamMembersSeeder/TeamMemberSeeder.cs (2)
TeamMembersSeeder(10-55)TeamMembersSeeder(14-17)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/CategoriesSeeder/CategoriesDataSeeder.cs (2)
CategoriesSeeder(8-56)CategoriesSeeder(10-13)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/ImageSeeder/ImagesSeeder.cs (2)
ImagesDataSeeder(9-56)ImagesDataSeeder(32-38)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/ProgramSeeder/ProgramDataSeeder.cs (2)
ProgramSeeder(9-48)ProgramSeeder(13-16)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/ProgramCategoriesSeeder/ProgramCategoriesDataSeeder.cs (2)
ProgramCategoriesSeeder(8-59)ProgramCategoriesSeeder(10-13)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Images/Update/UpdateImageTest.cs (4)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Base/IntegrationTestDbFixture.cs (6)
IntegrationTestDbFixture(20-146)IntegrationTestDbFixture(29-42)Task(48-51)Task(53-68)Task(70-109)Task(121-145)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/IntegrationTestsDatabaseSeeder.cs (2)
Task(39-52)Task(54-60)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/CategoriesSeeder/CategoriesDataSeeder.cs (2)
Task(18-21)Task(23-55)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/ImageSeeder/ImagesSeeder.cs (2)
Task(44-52)Task(54-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and analyze
- GitHub Check: Build and analyze
🔇 Additional comments (13)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Images/Update/UpdateImageTest.cs (1)
41-41: Good improvement using Path.Combine for cross-platform compatibilityThe change from string concatenation to
Path.Combinefor file path construction is a good improvement that ensures cross-platform compatibility.Also applies to: 58-58
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Base/IntegrationTestDbFixture.cs (1)
70-108: Well-implemented test isolation with database recreationThe
CreateFreshDatabasemethod provides excellent test isolation by:
- Creating a uniquely named database for each test
- Properly disposing previous resources
- Validating that seeding succeeded
- Maintaining authorization state across database recreations
This pattern ensures tests don't interfere with each other.
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Categories/Create/CreateCategoryTests.cs (2)
25-30: Good implementation of test lifecycle managementThe implementation of
IAsyncLifetimewithCreateFreshDatabase()ensures proper test isolation. Each test runs with a clean database state, preventing test interdependencies.
35-36: Good addition of edge case test dataAdding test cases for whitespace-only (" ") and valid description values improves test coverage for boundary conditions.
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Categories/Update/UpdateCategoryTests.cs (2)
11-24: Good adoption of the IAsyncLifetime patternThe refactoring to use
IntegrationTestDbFixtureand implementIAsyncLifetimeimproves test isolation and consistency across the test suite. This ensures each test runs with a clean database state.
26-31: Clean test initialization patternThe
InitializeAsyncmethod properly ensures a fresh database for each test run, preventing test interdependencies. TheDisposeAsyncreturningTask.CompletedTaskis correct since the fixture handles resource cleanup.VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Categories/Delete/DeleteCategoryTests.cs (2)
8-23: Consistent test infrastructure adoptionThe migration to
IntegrationTestDbFixtureandIAsyncLifetimemaintains consistency with other test classes, providing proper test isolation.
27-33: Appropriate entity selection for delete testUsing
LastOrDefaultAsync()to select the last category aligns with the seeding strategy where the last category is intentionally kept without team members, avoiding foreign key constraint violations during deletion.VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Images/Create/CreateImageTests.cs (1)
46-46: Good use of Path.CombineUsing
Path.Combineinstead of string concatenation improves cross-platform compatibility and handles path separators correctly.VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/GetPublished/GetPublishedPrograms.cs (1)
24-33: Well-structured test for published programs endpointThe test properly validates the published programs endpoint. Good placement of
EnsureSuccessStatusCode()before deserialization, which prevents errors when the response contains error content.VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/GetAll/GetAllProgramCategoriesTests.cs (1)
23-32: Clean implementation of program categories testThe test properly validates the program categories endpoint with correct error handling and assertions. Good adherence to the established test patterns.
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/TeamMembers/Update/UpdateTeamMemberTests.cs (1)
72-72: Clarify the entity selection strategyThe test uses
LastOrDefaultAsync()for valid update tests butFirstOrDefaultAsync()for the invalid name test. If this is intentional (e.g., to test different entities), consider adding a comment to explain the reasoning.If the mixed usage is unintentional, consider using consistent entity selection across all tests.
Also applies to: 106-106, 143-143
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/TeamMembers/GetById/GetTeamMemberByIdTests.cs (1)
22-33: Good defensive programming with seeding validationThe initialization properly validates that test data was seeded successfully before proceeding with tests. The logging of the count also aids in debugging test failures.
| public VictoryCenterWebApplicationFactory<Program> _factory; | ||
| public readonly IBlobService _blobService; | ||
| public readonly BlobEnvironmentVariables _blobEnvironmentVariables; |
There was a problem hiding this comment.
Fix naming convention violations for public fields
Public fields should not have underscore prefixes. This violates C# naming conventions and makes the API confusing.
Consider either making these fields private with public properties, or removing the underscore prefix:
- public VictoryCenterWebApplicationFactory<Program> _factory;
- public readonly IBlobService _blobService;
- public readonly BlobEnvironmentVariables _blobEnvironmentVariables;
+ private VictoryCenterWebApplicationFactory<Program> _factory;
+ private readonly IBlobService _blobService;
+ private readonly BlobEnvironmentVariables _blobEnvironmentVariables;
+
+ public VictoryCenterWebApplicationFactory<Program> Factory => _factory;
+ public IBlobService BlobService => _blobService;
+ public BlobEnvironmentVariables BlobEnvironmentVariables => _blobEnvironmentVariables;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public VictoryCenterWebApplicationFactory<Program> _factory; | |
| public readonly IBlobService _blobService; | |
| public readonly BlobEnvironmentVariables _blobEnvironmentVariables; | |
| @@ lines 22-24 of IntegrationTestDbFixture.cs | |
| - public VictoryCenterWebApplicationFactory<Program> _factory; | |
| - public readonly IBlobService _blobService; | |
| - public readonly BlobEnvironmentVariables _blobEnvironmentVariables; | |
| + private VictoryCenterWebApplicationFactory<Program> _factory; | |
| + private readonly IBlobService _blobService; | |
| + private readonly BlobEnvironmentVariables _blobEnvironmentVariables; | |
| + | |
| + public VictoryCenterWebApplicationFactory<Program> Factory => _factory; | |
| + public IBlobService BlobService => _blobService; | |
| + public BlobEnvironmentVariables BlobEnvironmentVariables => _blobEnvironmentVariables; |
🤖 Prompt for AI Agents
In
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Base/IntegrationTestDbFixture.cs
around lines 22 to 24, the public fields use underscore prefixes which violates
C# naming conventions; either rename the public fields to PascalCase without the
underscores (e.g., VictoryCenterWebApplicationFactory<Program> Factory;
IBlobService BlobService; BlobEnvironmentVariables BlobEnvironmentVariables) and
update all usages, or make the fields private readonly with underscore prefixes
and add public PascalCase properties that expose them; ensure all references in
the project are updated to the new names and keep accessibility consistent
(public properties or private fields) to satisfy conventions.
| var count = await _fixture.DbContext.TeamMembers.CountAsync(); | ||
|
|
||
| if (count == 0) | ||
| { | ||
| PropertyNameCaseInsensitive = true | ||
| }; | ||
| throw new InvalidOperationException("No TeamMembers were seeded. Check your seeder implementation."); | ||
| } |
There was a problem hiding this comment.
Validation checks wrong entity type
The initialization validates TeamMembers count but this test is for Images. This appears to be a copy-paste error.
- var count = await _fixture.DbContext.TeamMembers.CountAsync();
+ var count = await _fixture.DbContext.Images.CountAsync();
if (count == 0)
{
- throw new InvalidOperationException("No TeamMembers were seeded. Check your seeder implementation.");
+ throw new InvalidOperationException("No Images were seeded. Check your seeder implementation.");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var count = await _fixture.DbContext.TeamMembers.CountAsync(); | |
| if (count == 0) | |
| { | |
| PropertyNameCaseInsensitive = true | |
| }; | |
| throw new InvalidOperationException("No TeamMembers were seeded. Check your seeder implementation."); | |
| } | |
| var count = await _fixture.DbContext.Images.CountAsync(); | |
| if (count == 0) | |
| { | |
| throw new InvalidOperationException("No Images were seeded. Check your seeder implementation."); | |
| } |
🤖 Prompt for AI Agents
In
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Images/Delete/DeleteImageTests.cs
around lines 22 to 27, the validation is checking
_fixture.DbContext.TeamMembers.CountAsync() even though this test targets
Images; replace the check to count the Images DbSet (e.g., await
_fixture.DbContext.Images.CountAsync()) and update the thrown
InvalidOperationException message to reference "No Images were seeded. Check
your seeder implementation." so the validation matches the test entity.
| } | ||
| } | ||
|
|
||
| public Task DisposeAsync() => Task.CompletedTask; |
There was a problem hiding this comment.
Missing seeder disposal
DisposeAsync should dispose seeders for proper cleanup, consistent with other tests.
- public Task DisposeAsync() => Task.CompletedTask;
+ public async Task DisposeAsync()
+ {
+ await _fixture.SeederManager.DisposeAllAsync();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Task DisposeAsync() => Task.CompletedTask; | |
| public async Task DisposeAsync() | |
| { | |
| await _fixture.SeederManager.DisposeAllAsync(); | |
| } |
🤖 Prompt for AI Agents
In
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Images/Delete/DeleteImageTests.cs
around line 30, the DisposeAsync method currently just returns
Task.CompletedTask and does not dispose any test seeders; change the method to
be async Task DisposeAsync() and await disposal of any seeders created by the
test class (for example call await seeder.DisposeAsync() or iterate over a
seeder collection and await each DisposeAsync()), ensuring all seeders are
disposed before completing; keep the method signature async Task and propagate
any necessary fields (null-checks) when calling DisposeAsync on each seeder.
| _httpClient = fixture.HttpClient; | ||
| _dbContext = fixture.DbContext; | ||
| _fixture = fixture; | ||
| _blobService = fixture._blobService; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve encapsulation for fixture fields
Direct access to _blobService and _factory fields violates encapsulation. These should be exposed through properties if external access is needed.
Consider exposing these through properties in the fixture:
-_blobService = fixture._blobService;
+_blobService = fixture.BlobService;
-new CategoriesSeeder(_fixture.DbContext, _fixture._factory.Services.GetRequiredService<ILogger<CategoriesSeeder>>(), _blobService),
+new CategoriesSeeder(_fixture.DbContext, _fixture.GetService<ILogger<CategoriesSeeder>>(), _blobService),Also applies to: 52-53
🤖 Prompt for AI Agents
In
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/TeamMembers/Update/UpdateTeamMemberTests.cs
around lines 35 and 52-53, the test is directly assigning fixture fields
(_blobService and _factory) which breaks encapsulation; modify the fixture class
to make those fields private and expose them via public read-only properties
(e.g., BlobService and Factory), then update this test file to reference the new
properties instead of the underscored fields; ensure the fixture still
initializes the backing members and the property accessors return the
initialized instances.
| public RequestResponseLoggingMiddlewareTests(IntegrationTestDbFixture fixture) | ||
| { | ||
| var customFactory = fixture.Factory.WithWebHostBuilder(builder => | ||
| var customFactory = fixture._factory.WithWebHostBuilder(builder => |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider exposing _factory through a proper public property
Accessing _factory directly violates C# naming conventions for public members. Fields prefixed with underscore are typically private.
Consider exposing this through a proper public property in IntegrationTestDbFixture:
// In IntegrationTestDbFixture.cs
- public VictoryCenterWebApplicationFactory<Program> _factory;
+ private VictoryCenterWebApplicationFactory<Program> _factory;
+ public VictoryCenterWebApplicationFactory<Program> Factory => _factory;Then update this line:
- var customFactory = fixture._factory.WithWebHostBuilder(builder =>
+ var customFactory = fixture.Factory.WithWebHostBuilder(builder =>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var customFactory = fixture._factory.WithWebHostBuilder(builder => | |
| var customFactory = fixture.Factory.WithWebHostBuilder(builder => |
🤖 Prompt for AI Agents
In
VictoryCenter/VictoryCenter.IntegrationTests/MiddlewareTests/RequestResponseLoggingMiddlewareTests.cs
around line 18, the test accesses the fixture's internal field `_factory` which
violates C# public member naming and encapsulation; add a public property on
IntegrationTestDbFixture (e.g., public WebApplicationFactory<T> Factory { get;
}) that returns the existing factory instance (make the backing field private or
remove it), then update this line to use the new property
(fixture.Factory.WithWebHostBuilder(...)) so the test uses a properly exposed
public member.
| public SeederManager(VictoryCenterDbContext dbContext, ILoggerFactory loggerFactory, IBlobService blobService, IEnumerable<ISeeder>? seeders = null) | ||
| { | ||
| _dbContext = dbContext; | ||
| _loggerFactory = loggerFactory; | ||
| _logger = loggerFactory.CreateLogger<SeederManager>(); | ||
| _blobService = blobService; | ||
|
|
||
| _seeders = (seeders ?? CreateDefaultSeeders()) | ||
| .OrderBy(s => s.Order) | ||
| .ToList(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null validation for required dependencies.
The constructor accepts critical dependencies but doesn't validate them for null values, which could lead to runtime exceptions when the seeder manager is used.
public SeederManager(VictoryCenterDbContext dbContext, ILoggerFactory loggerFactory, IBlobService blobService, IEnumerable<ISeeder>? seeders = null)
{
+ _dbContext = dbContext ?? throw new ArgumentNullException(nameof(dbContext));
+ _loggerFactory = loggerFactory ?? throw new ArgumentNullException(nameof(loggerFactory));
+ _blobService = blobService ?? throw new ArgumentNullException(nameof(blobService));
- _dbContext = dbContext;
- _loggerFactory = loggerFactory;
- _logger = loggerFactory.CreateLogger<SeederManager>();
- _blobService = blobService;
+ _logger = _loggerFactory.CreateLogger<SeederManager>();
_seeders = (seeders ?? CreateDefaultSeeders())
.OrderBy(s => s.Order)
.ToList();
}🤖 Prompt for AI Agents
In
VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/IntegrationTestsDatabaseSeeder.cs
around lines 15 to 25, the constructor accepts critical dependencies but doesn't
validate them for null; add defensive null checks at the start of the
constructor and throw ArgumentNullException for required params (dbContext,
loggerFactory, blobService) before using them, then proceed to assign fields;
leave optional seeders handling as-is (use null-coalescing for
CreateDefaultSeeders()) so only mandatory dependencies are validated.
| public async Task<bool> SeedAllAsync() | ||
| { | ||
| foreach (var seeder in _seeders) | ||
| { | ||
| var result = await seeder.SeedAsync(); | ||
| if (!result.Success) | ||
| { | ||
| await DisposeAllAsync(); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and logging in SeedAllAsync.
The method fails fast on the first seeder error but doesn't provide sufficient logging context about which seeder failed or why.
public async Task<bool> SeedAllAsync()
{
+ _logger.LogInformation("Starting seeding process with {SeederCount} seeders", _seeders.Count);
+
foreach (var seeder in _seeders)
{
+ _logger.LogDebug("Executing seeder: {SeederType}", seeder.GetType().Name);
var result = await seeder.SeedAsync();
if (!result.Success)
{
+ _logger.LogError("Seeder {SeederType} failed: {ErrorMessage}", seeder.GetType().Name, result.ErrorMessage);
await DisposeAllAsync();
return false;
}
+ _logger.LogDebug("Seeder {SeederType} completed successfully", seeder.GetType().Name);
}
+ _logger.LogInformation("All seeders completed successfully");
return true;
}| public async Task DisposeAllAsync() | ||
| { | ||
| CategoriesDataSeeder.SeedData(dbContext); | ||
| TeamMemberSeeder.SeedData(dbContext, dbContext.Categories.ToList()); | ||
| ImagesDataSeeder.SeedData(dbContext, blobService ); | ||
| foreach (var seeder in _seeders.OrderByDescending(s => s.Order)) | ||
| { | ||
| await seeder.DisposeAsync(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to DisposeAllAsync.
The disposal method doesn't handle exceptions that might occur during individual seeder disposal, which could leave resources in an inconsistent state.
public async Task DisposeAllAsync()
{
+ var exceptions = new List<Exception>();
+
foreach (var seeder in _seeders.OrderByDescending(s => s.Order))
{
- await seeder.DisposeAsync();
+ try
+ {
+ await seeder.DisposeAsync();
+ }
+ catch (Exception ex)
+ {
+ _logger.LogWarning(ex, "Failed to dispose seeder {SeederType}", seeder.GetType().Name);
+ exceptions.Add(ex);
+ }
}
+
+ if (exceptions.Count > 0)
+ {
+ throw new AggregateException("One or more seeders failed to dispose properly", exceptions);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async Task DisposeAllAsync() | |
| { | |
| CategoriesDataSeeder.SeedData(dbContext); | |
| TeamMemberSeeder.SeedData(dbContext, dbContext.Categories.ToList()); | |
| ImagesDataSeeder.SeedData(dbContext, blobService ); | |
| foreach (var seeder in _seeders.OrderByDescending(s => s.Order)) | |
| { | |
| await seeder.DisposeAsync(); | |
| } | |
| } | |
| public async Task DisposeAllAsync() | |
| { | |
| var exceptions = new List<Exception>(); | |
| foreach (var seeder in _seeders.OrderByDescending(s => s.Order)) | |
| { | |
| try | |
| { | |
| await seeder.DisposeAsync(); | |
| } | |
| catch (Exception ex) | |
| { | |
| _logger.LogWarning(ex, "Failed to dispose seeder {SeederType}", seeder.GetType().Name); | |
| exceptions.Add(ex); | |
| } | |
| } | |
| if (exceptions.Count > 0) | |
| { | |
| throw new AggregateException("One or more seeders failed to dispose properly", exceptions); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/IntegrationTestsDatabaseSeeder.cs
around lines 54 to 60, DisposeAllAsync currently awaits each
seeder.DisposeAsync() without error handling; wrap each call in a try/catch to
ensure one failing seeder doesn't stop others from disposing, log the exception
with identifying seeder information (e.g., type or Order) and continue the loop,
and optionally collect exceptions into a list and after the loop throw an
AggregateException if any were captured so callers can observe failures; use the
existing test logger or an injected ILogger for logging.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/ProgramCategoriesSeeder/ProgramCategoriesDataSeeder.cs (1)
15-16: Name property now matches class — good fix.This resolves the earlier inconsistency and keeps logs/readability aligned.
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Programs/GetProgramByIdTests.cs (1)
44-59: Enhanced test implementation addresses previous feedback.The test now includes comprehensive assertions validating the result content (Name, Description, Status) and verifies that mocked dependencies are called as expected. This provides thorough validation of the handler's behavior.
🧹 Nitpick comments (5)
VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/ProgramCategoriesSeeder/ProgramCategoriesDataSeeder.cs (2)
22-58: Stabilize CreatedAt for deterministic tests.Capture UtcNow once to avoid per-item timestamp drift and make time-based assertions predictable.
protected override Task<List<ProgramCategory>> GenerateEntitiesAsync() { - var programCategories = new List<ProgramCategory> + var seededAt = DateTime.UtcNow; + var programCategories = new List<ProgramCategory> { new() { Id = 1, Name = "TestName1", - CreatedAt = DateTime.UtcNow + CreatedAt = seededAt }, new() { Id = 2, Name = "TestName2", - CreatedAt = DateTime.UtcNow + CreatedAt = seededAt }, new() { Id = 3, Name = "TestName3", - CreatedAt = DateTime.UtcNow + CreatedAt = seededAt }, new() { Id = 4, Name = "TestName4", - CreatedAt = DateTime.UtcNow + CreatedAt = seededAt }, new() { Id = 5, Name = "TestName5", - CreatedAt = DateTime.UtcNow + CreatedAt = seededAt } }; return Task.FromResult(programCategories); }
24-55: Nit: consider more descriptive test names.Optional: “TestCategory1..5” reads clearer in logs than “TestName1..5”.
- Name = "TestName1", + Name = "TestCategory1", ... - Name = "TestName2", + Name = "TestCategory2", ... - Name = "TestName3", + Name = "TestCategory3", ... - Name = "TestName4", + Name = "TestCategory4", ... - Name = "TestName5", + Name = "TestCategory5",VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Programs/GetProgramByIdTests.cs (3)
72-77: Consider making the parameter nullable for clarity.The
programparameter should be explicitly nullable to match its usage pattern wherenullis passed to simulate the "not found" scenario.Apply this diff to improve parameter clarity:
-private void SetUpDependencies(DAL.Entities.Program program = null) +private void SetUpDependencies(DAL.Entities.Program? program = null)
84-88: Parameter should be nullable to match usage.The
programparameter is used in scenarios wherenullis passed, so it should be explicitly nullable.Apply this diff to improve type safety:
-private void SetUpRepositoryWrapper(DAL.Entities.Program program) +private void SetUpRepositoryWrapper(DAL.Entities.Program? program)
90-95: Verify blob service interaction in success test.The blob service setup is configured but not verified in the success test. Consider adding verification to ensure the blob service is called when an image is present.
Add this verification to the success test after line 58:
_mockRepositoryWrapper.Verify(x => x.ProgramsRepository.GetFirstOrDefaultAsync(It.IsAny<QueryOptions<DAL.Entities.Program>>()), Times.Once); _mapperMock.Verify(x => x.Map<ProgramDto>(It.IsAny<DAL.Entities.Program>()), Times.Once); +_mockBlobService.Verify(x => x.FindFileInStorageAsBase64Async(It.IsAny<string>(), It.IsAny<string>()), Times.Once);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryBase.cs(4 hunks)VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/ProgramCategoriesSeeder/ProgramCategoriesDataSeeder.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Programs/GetProgramByIdTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryBase.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-17T20:32:16.009Z
Learnt from: VladimirSushinsky
PR: ita-social-projects/VictoryCenter-Back#114
File: VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/TeamMembersSeeder/TeamMemberSeeder.cs:20-24
Timestamp: 2025-06-17T20:32:16.009Z
Learning: In the VictoryCenter test seeder for TeamMember entities, the last category in the categories list is intentionally excluded from having team members assigned to it (using `categories[i % (categories.Count - 1)].Id`). This design ensures that the last category remains available for delete tests without foreign key constraint violations, as delete operations require categories with no related team members.
Applied to files:
VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/ProgramCategoriesSeeder/ProgramCategoriesDataSeeder.cs
🧬 Code Graph Analysis (2)
VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/ProgramCategoriesSeeder/ProgramCategoriesDataSeeder.cs (1)
VictoryCenter/VictoryCenter.DAL/Entities/ProgramCategory.cs (1)
ProgramCategory(3-9)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Programs/GetProgramByIdTests.cs (5)
VictoryCenter/VictoryCenter.BLL/DTOs/Programs/ProgramDto.cs (1)
ProgramDto(7-19)VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryBase.cs (5)
Task(21-36)Task(38-50)Task(52-56)Task(58-69)Task(81-96)VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetById/GetProgramByIdHandler.cs (3)
Task(27-53)GetProgramByIdHandler(14-54)GetProgramByIdHandler(20-25)VictoryCenter/VictoryCenter.DAL/Repositories/Options/QueryOptions.cs (1)
QueryOptions(6-16)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (1)
ErrorMessagesConstants(3-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and analyze
- GitHub Check: Build and analyze
🔇 Additional comments (3)
VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/ProgramCategoriesSeeder/ProgramCategoriesDataSeeder.cs (3)
16-16: Seeding Order Verified
ProgramCategoriesSeeder (Order = 3) now runs before ProgramSeeder (Order = 4), satisfying the FK constraint. No further changes needed.
- VictoryCenter.IntegrationTests/Utils/Seeder/ProgramCategoriesSeeder/ProgramCategoriesDataSeeder.cs: Order ⇒ 3
- VictoryCenter.IntegrationTests/Utils/Seeder/ProgramSeeder/ProgramDataSeeder.cs: Order ⇒ 4
22-58: Test seeding uses InMemory provider – explicit IDs are safeThe integration tests configure the DbContext with
UseInMemoryDatabase(see VictoryCenterWebApplicationFactory.cs), so EF Core’s in-memory store will honor any explicitly assigned key values without needing IDENTITY_INSERT or sequence reseeding. Since ProgramCategoriesDataSeeder and BaseSeeder are only used against the in-memory provider in tests, no additional toggling or reseeding logic is required. Keep the explicit Id assignments as is.
24-56: ProgramsSeeder already reserves the last category for delete testsThe
GenerateEntitiesAsyncin ProgramDataSeeder (Order 4) uses:var categories = await _dbContext.ProgramCategories.Take(4).ToListAsync();Since 5 categories (Ids 1–5) are seeded, only the first 4 are ever assigned to new programs—Id 5 remains unassigned and available for delete tests. No changes required.
| protected override Task<bool> ShouldSkipAsync() | ||
| { | ||
| return Task.FromResult(_dbContext.ProgramCategories.Any()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid sync EF call in an async method; use AnyAsync + AsNoTracking.
Prevents blocking and aligns with EF Core async best practices.
- protected override Task<bool> ShouldSkipAsync()
- {
- return Task.FromResult(_dbContext.ProgramCategories.Any());
- }
+ protected override async Task<bool> ShouldSkipAsync()
+ {
+ return await _dbContext.ProgramCategories
+ .AsNoTracking()
+ .AnyAsync();
+ }Add this import at the top of the file:
using Microsoft.EntityFrameworkCore;🤖 Prompt for AI Agents
In
VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/ProgramCategoriesSeeder/ProgramCategoriesDataSeeder.cs
around lines 17 to 20, the async method ShouldSkipAsync is calling the
synchronous EF method Any(), which can block; change it to use await
_dbContext.ProgramCategories.AsNoTracking().AnyAsync() and return the boolean
result, and add the required using Microsoft.EntityFrameworkCore; at the top of
the file. Ensure the method is marked async and awaits the AnyAsync call so the
EF query runs asynchronously and without tracking.
| private readonly DAL.Entities.Program _programEntity = new() | ||
| { | ||
| Id = 1, | ||
| Name = "TestName", | ||
| Description = "TestDescription", | ||
| Status = Status.Draft, | ||
| ImageId = 1, | ||
| }; | ||
|
|
||
| private readonly ProgramDto _programDto = new() | ||
| { | ||
| Name = "TestName", | ||
| Description = "TestDescription", | ||
| Status = Status.Draft, | ||
| Image = new ImageDTO() | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing Id field in _programDto affects test validation.
The _programEntity has Id = 1 but the _programDto doesn't include an Id field. This creates an inconsistency between the test data and may lead to incomplete validation in tests.
Apply this diff to align the test data:
private readonly ProgramDto _programDto = new()
{
+ Id = 1,
Name = "TestName",
Description = "TestDescription",
Status = Status.Draft,
Image = new ImageDTO()
};🤖 Prompt for AI Agents
In
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Programs/GetProgramByIdTests.cs
around lines 20 to 35, the test ProgramDto fixture is missing the Id value
present on the _programEntity; add an Id = 1 property to the _programDto
initialization so the DTO and entity IDs match, ensuring test validation covers
the Id field consistently.
| [Fact] | ||
| public async Task Handle_ShouldFailFindProgram() | ||
| { | ||
| SetUpDependencies(); | ||
| var handler = | ||
| new GetProgramByIdHandler(_mapperMock.Object, _mockRepositoryWrapper.Object, _mockBlobService.Object); | ||
| var result = await handler.Handle(new GetProgramByIdQuery(_programEntity.Id), CancellationToken.None); | ||
| Assert.False(result.IsSuccess); | ||
| Assert.Equal(ErrorMessagesConstants.NotFound(_programEntity.Id, typeof(Program)), result.Errors[0].Message); | ||
| } |
There was a problem hiding this comment.
Incorrect type reference in error message assertion.
Line 69 references typeof(Program) but should reference typeof(DAL.Entities.Program) to match the actual entity type used in the handler implementation.
Apply this diff to fix the type reference:
-Assert.Equal(ErrorMessagesConstants.NotFound(_programEntity.Id, typeof(Program)), result.Errors[0].Message);
+Assert.Equal(ErrorMessagesConstants.NotFound(_programEntity.Id, typeof(DAL.Entities.Program)), result.Errors[0].Message);🤖 Prompt for AI Agents
In
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Programs/GetProgramByIdTests.cs
around lines 61 to 70, the assertion uses typeof(Program) which is the wrong
type; update the test to reference the actual entity type by replacing
typeof(Program) with typeof(DAL.Entities.Program) so the expected error message
matches the handler's entity type.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetById/GetProgramByIdHandler.cs (1)
44-52: Avoid mutating the EF entity; map first and handle blob fetch failures (align with GetByFilters)Two improvements:
- Don’t set Base64 on the EF entity; set it on the DTO post-mapping to avoid mutating domain entities with transport-only data.
- Handle blob retrieval exceptions gracefully (consistent with GetByFiltersHandler) to prevent 500s when the blob is missing or inaccessible.
Apply this diff:
- if (program.Image is not null) - { - program.Image.Base64 = await _blobService.FindFileInStorageAsBase64Async( - program.Image.BlobName, - program.Image.MimeType); - } - - ProgramDto responseDto = _mapper.Map<ProgramDto>(program); - return Result.Ok(responseDto); + ProgramDto responseDto = _mapper.Map<ProgramDto>(program); + if (responseDto.Image is not null) + { + try + { + responseDto.Image.Base64 = await _blobService.FindFileInStorageAsBase64Async( + responseDto.Image.BlobName, + responseDto.Image.MimeType); + } + catch (BlobStorageException) + { + responseDto.Image.Base64 = string.Empty; + } + } + + return Result.Ok(responseDto);Notes:
- If
BlobStorageExceptionis in a different namespace here, add the appropriate using or fully-qualify it.- Optionally guard against missing metadata before calling storage (e.g., skip if BlobName/MimeType are null/empty) to avoid unnecessary calls.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Create/CreateProgramCategoryHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Delete/DeleteProgramCategoryHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Update/UpdateProgramCategoryHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Programs/Create/CreateProgramHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Programs/Delete/DeleteProgramHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Programs/Update/UpdateProgramHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Queries/ProgramCategories/GetProgramCategoriesHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetByFilters/GetByFiltersHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetById/GetProgramByIdHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetPublished/GetPublishedProgramsHandler.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Delete/DeleteProgramCategoryHandler.cs
- VictoryCenter/VictoryCenter.BLL/Commands/Programs/Create/CreateProgramHandler.cs
- VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetByFilters/GetByFiltersHandler.cs
- VictoryCenter/VictoryCenter.BLL/Queries/ProgramCategories/GetProgramCategoriesHandler.cs
- VictoryCenter/VictoryCenter.BLL/Commands/Programs/Delete/DeleteProgramHandler.cs
- VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetPublished/GetPublishedProgramsHandler.cs
- VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Create/CreateProgramCategoryHandler.cs
- VictoryCenter/VictoryCenter.BLL/Commands/Programs/Update/UpdateProgramHandler.cs
- VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Update/UpdateProgramCategoryHandler.cs
🧰 Additional context used
🧬 Code Graph Analysis (1)
VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetById/GetProgramByIdHandler.cs (6)
VictoryCenter/VictoryCenter.BLL/DTOs/Programs/ProgramDto.cs (1)
ProgramDto(7-19)VictoryCenter/VictoryCenter.BLL/Commands/Programs/Delete/DeleteProgramHandler.cs (1)
Task(20-43)VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetPublished/GetPublishedProgramsHandler.cs (1)
Task(27-50)VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetByFilters/GetByFiltersHandler.cs (1)
Task(29-72)VictoryCenter/VictoryCenter.DAL/Repositories/Options/QueryOptions.cs (1)
QueryOptions(6-16)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (1)
ErrorMessagesConstants(3-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and analyze
🔇 Additional comments (3)
VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetById/GetProgramByIdHandler.cs (3)
14-25: LGTM: Solid handler structure and DIClass shape, dependencies, and result wrapping are clean and aligned with the rest of the codebase patterns.
31-31: No change needed for identifier casing
TheGetProgramByIdQueryrecord is declared aspublic record GetProgramByIdQuery(long id), which generates a lowercaseidproperty. Usingrequest.idis therefore correct, and switching torequest.Idwould fail to compile.Likely an incorrect or invalid review comment.
37-37: CancellationToken propagation not supported by repository/blob APIsI’ve verified that
GetFirstOrDefaultAsyncinIRepositoryBaseonly takes aQueryOptions<T>parameter (noCancellationTokenoverload) and there is no overload ofFindFileInStorageAsBase64Asyncaccepting a cancellation token. Since neither API supports cancellation tokens, no changes are required here.
# Conflicts: # VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Base/IntegrationTestDbFixture.cs # VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Categories/Create/CreateCategoryTests.cs # VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Categories/Delete/DeleteCategoryTests.cs # VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Categories/GetAll/GetAllCategoriesTests.cs # VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Categories/Update/UpdateCategoryTests.cs # VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Images/Create/CreateImageTests.cs # VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Images/Delete/DeleteImageTests.cs # VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Images/GetById/GetImageByIdTest.cs # VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Images/GetByName/GetImageByNameTest.cs # VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Images/Update/UpdateImageTest.cs # VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Team/GetPublished/GetPublishedTeamMembersTests.cs # VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/TeamMembers/Create/CreateTeamMemberTest.cs # VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/TeamMembers/Delete/DeleteTeamMemberTests.cs # VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/TeamMembers/GetById/GetTeamMemberByIdTests.cs # VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/TeamMembers/GetFiltered/GetFilteredTeamMembersTests.cs # VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/TeamMembers/Reorder/ReorderTeamMemberTests.cs # VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/TeamMembers/Update/UpdateTeamMemberTests.cs # VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/CategoriesSeeder/CategoriesDataSeeder.cs # VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/IntegrationTestsDatabaseSeeder.cs # VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/TeamMembersSeeder/TeamMemberSeeder.cs # VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeders/Images/ImagesSeeder.cs # VictoryCenter/VictoryCenter.IntegrationTests/Utils/VictoryCenterWebApplicationFactory.cs
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Update/UpdateProgramCategoryTests.cs (3)
19-22: Validate seeding before tests to fail fast on environment issuesAdd a quick guard to ensure ProgramCategories are present; otherwise fail with a clear message. This prevents false negatives when the seeder fails.
public async Task InitializeAsync() { await _fixture.CreateFreshWebApplication(); + var count = await _fixture.DbContext.ProgramCategories.CountAsync(); + if (count == 0) + { + throw new InvalidOperationException("No ProgramCategories were seeded. Check your seeder implementation."); + } }
24-24: Dispose seeders/fixtures to free resources between testsIf seeders allocate resources, dispose them via the fixture to keep isolation and avoid leaks.
- public Task DisposeAsync() => Task.CompletedTask; + public async Task DisposeAsync() + { + await _fixture.SeederManager.DisposeAllAsync(); + }
78-80: Fix endpoint path: singular vs plural mismatch breaks routingThe controller route is
api/ProgramCategory/{id}, but the test usesapi/ProgramCategories/{id}here, causing 404/NotFound unrelated to handler logic.- HttpResponseMessage response = await _fixture.HttpClient.PutAsync($"/api/ProgramCategories/{id}", new StringContent( + HttpResponseMessage response = await _fixture.HttpClient.PutAsync($"/api/ProgramCategory/{id}", new StringContent( serializedDto, Encoding.UTF8, "application/json"));
🧹 Nitpick comments (10)
VictoryCenter/VictoryCenter.BLL/DTOs/Programs/UpdateProgramDto.cs (2)
3-3: DTO reuse for update is fine; confirm intended “full replace” semanticsInheriting from CreateProgramDto implies the update requires the same fields as create (full replacement). If partial updates (patch semantics) are expected now or in the future, consider a dedicated UpdateProgramDto with optional fields or a PATCH endpoint to avoid over-posting/mandatory-field friction.
I can propose a minimal Patch DTO or JSON Patch approach if you plan to support partial updates.
3-3: Optionally seal the record to prevent unintended inheritanceNot critical, but sealing DTOs helps avoid accidental inheritance and keeps the surface tight.
Apply this diff:
-public record UpdateProgramDto : CreateProgramDto; +public sealed record UpdateProgramDto : CreateProgramDto;VictoryCenter/VictoryCenter.BLL/Commands/Programs/Update/UpdateProgramCommand.cs (1)
7-7: Prefer clearer, PascalCased property names for record parametersCurrent positional properties will be named updateProgramDto and id (camelCase). Consider PascalCase and a more explicit name for id to aid discoverability at call sites.
Apply this diff (non-breaking in concept, but will require minor call-site renames):
-public record UpdateProgramCommand(UpdateProgramDto updateProgramDto, long id) : IRequest<Result<ProgramDto>>; +public sealed record UpdateProgramCommand(UpdateProgramDto Dto, long ProgramId) : IRequest<Result<ProgramDto>>;If renaming across the codebase is undesirable now, at least consider Id → ProgramId for clarity.
VictoryCenter/VictoryCenter.WebAPI/Controllers/ProgramCategories/ProgramCategoryController.cs (4)
20-25: Fold route template into the HTTP verb attribute and bindidexplicitly from routeSmall cleanup for consistency with other controllers (see ProgramController) and to make binding explicit.
- [HttpDelete] - [Route("{id:long}")] - public async Task<IActionResult> DeleteProgramCategory(long id) + [HttpDelete("{id:long}")] + public async Task<IActionResult> DeleteProgramCategory([FromRoute] long id) { return HandleResult(await Mediator.Send(new DeleteProgramCategoryCommand(id))); }
27-32: Same here: inline the route on the verb attribute and markidas[FromRoute]Keeps attribute usage concise and parameter binding explicit.
- [HttpPut] - [Route("{id:long}")] - public async Task<IActionResult> UpdateProgramCategory([FromBody] UpdateProgramCategoryDto updateProgramCategoryDto, long id) + [HttpPut("{id:long}")] + public async Task<IActionResult> UpdateProgramCategory([FromBody] UpdateProgramCategoryDto updateProgramCategoryDto, [FromRoute] long id) { return HandleResult(await Mediator.Send(new UpdateProgramCategoryCommand(updateProgramCategoryDto, id))); }
27-32: Pass CancellationToken through to MediatR (apply similarly to all actions)Propagate request abort/cancellation to handlers to avoid wasted work under client disconnects.
- public async Task<IActionResult> UpdateProgramCategory([FromBody] UpdateProgramCategoryDto updateProgramCategoryDto, long id) + public async Task<IActionResult> UpdateProgramCategory([FromBody] UpdateProgramCategoryDto updateProgramCategoryDto, long id, CancellationToken cancellationToken) { - return HandleResult(await Mediator.Send(new UpdateProgramCategoryCommand(updateProgramCategoryDto, id))); + return HandleResult(await Mediator.Send(new UpdateProgramCategoryCommand(updateProgramCategoryDto, id), cancellationToken)); }If you agree, mirror this change for Create, Delete, and Get endpoints.
34-38: Consider documenting response types for API discoverabilityAdding ProducesResponseType annotations (200/400/404) helps swagger and consumers.
Example:
- [ProducesResponseType(StatusCodes.Status200OK, Type = typeof(List))]
- [ProducesResponseType(StatusCodes.Status400BadRequest)]
- [ProducesResponseType(StatusCodes.Status404NotFound)]
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Update/UpdateProgramCategoryTests.cs (3)
29-37: Avoid hard-coded id; fetch an existing ProgramCategory to make the test robustRelying on id = 1 is brittle against seeding changes. Query the DbContext and use the found id.
public async Task UpdateProgramCategory_ShouldUpdateProgramCategory() { var updateProgramDto = new UpdateProgramCategoryDto { Name = "UpdatedName" }; var serializedDto = JsonConvert.SerializeObject(updateProgramDto); - HttpResponseMessage response = await _fixture.HttpClient.PutAsync("/api/ProgramCategory/1", new StringContent( + var existingEntity = await _fixture.DbContext.ProgramCategories.FirstOrDefaultAsync(); + Assert.NotNull(existingEntity); + + HttpResponseMessage response = await _fixture.HttpClient.PutAsync($"/api/ProgramCategory/{existingEntity!.Id}", new StringContent( serializedDto, Encoding.UTF8, "application/json")); response.EnsureSuccessStatusCode();Additionally, ensure EF Core is imported at the top of the file:
using System.Net; using System.Text; using Newtonsoft.Json; +using Microsoft.EntityFrameworkCore; using VictoryCenter.BLL.DTOs.ProgramCategories; using VictoryCenter.IntegrationTests.ControllerTests.DbFixture;
70-71: Align test parameter type with controller route (long)Minor consistency fix; keeps types aligned with controller signatures and avoids implicit conversions.
- public async Task ProgramCategory_ShouldNotUpdateProgramCategory_NotFound(int id) + public async Task ProgramCategory_ShouldNotUpdateProgramCategory_NotFound(long id)
1-6: Unify JSON approach across integration tests (optional)Other integration tests use System.Text.Json and/or HttpClient.PutAsJsonAsync. Consider aligning to reduce dependencies and verbosity.
Example refactor:
- Replace manual serialization with: await _fixture.HttpClient.PutAsJsonAsync(..., updateProgramDto);
- Deserialize with System.Text.Json and shared options (case-insensitive).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Update/UpdateProgramCategoryCommand.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Update/UpdateProgramCategoryHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Programs/Update/UpdateProgramCommand.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Programs/Update/UpdateProgramHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/DTOs/ProgramCategories/UpdateProgramCategoryDto.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/DTOs/Programs/CreateProgramDto.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/DTOs/Programs/UpdateProgramDto.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Update/UpdateProgramCategoryTests.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/Update/UpdateProgramTests.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/ProgramCategories/UpdateProgramCategoryTests.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Programs/UpdateProgramTests.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/ProgramCategories/UpdateProgramCategoryValidatorTests.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/Programs/UpdateProgramValidatorTests.cs(1 hunks)VictoryCenter/VictoryCenter.WebAPI/Controllers/ProgramCategories/ProgramCategoryController.cs(1 hunks)VictoryCenter/VictoryCenter.WebAPI/Controllers/Programs/ProgramController.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Update/UpdateProgramCategoryCommand.cs
- VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/Programs/UpdateProgramValidatorTests.cs
- VictoryCenter/VictoryCenter.BLL/DTOs/Programs/CreateProgramDto.cs
- VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/ProgramCategories/UpdateProgramCategoryTests.cs
- VictoryCenter/VictoryCenter.BLL/DTOs/ProgramCategories/UpdateProgramCategoryDto.cs
- VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Programs/UpdateProgramTests.cs
- VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Update/UpdateProgramCategoryHandler.cs
- VictoryCenter/VictoryCenter.BLL/Commands/Programs/Update/UpdateProgramHandler.cs
- VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/Update/UpdateProgramTests.cs
- VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/ProgramCategories/UpdateProgramCategoryValidatorTests.cs
- VictoryCenter/VictoryCenter.WebAPI/Controllers/Programs/ProgramController.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-17T20:32:16.009Z
Learnt from: VladimirSushinsky
PR: ita-social-projects/VictoryCenter-Back#114
File: VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/TeamMembersSeeder/TeamMemberSeeder.cs:20-24
Timestamp: 2025-06-17T20:32:16.009Z
Learning: In the VictoryCenter test seeder for TeamMember entities, the last category in the categories list is intentionally excluded from having team members assigned to it (using `categories[i % (categories.Count - 1)].Id`). This design ensures that the last category remains available for delete tests without foreign key constraint violations, as delete operations require categories with no related team members.
Applied to files:
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Update/UpdateProgramCategoryTests.cs
🧬 Code Graph Analysis (3)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Update/UpdateProgramCategoryTests.cs (2)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Categories/Update/UpdateCategoryTests.cs (1)
Collection(10-125)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/ProgramCategories/UpdateProgramCategoryTests.cs (2)
UpdateProgramCategoryTests(15-110)UpdateProgramCategoryTests(40-45)
VictoryCenter/VictoryCenter.BLL/Commands/Programs/Update/UpdateProgramCommand.cs (1)
VictoryCenter/VictoryCenter.BLL/DTOs/Programs/ProgramDto.cs (1)
ProgramDto(7-19)
VictoryCenter/VictoryCenter.WebAPI/Controllers/ProgramCategories/ProgramCategoryController.cs (6)
VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryBase.cs (4)
Delete(76-79)Task(21-36)Task(38-50)Task(52-56)VictoryCenter/VictoryCenter.WebAPI/Controllers/Programs/ProgramController.cs (6)
Authorize(12-47)HttpPost(21-25)HttpDelete(27-32)HttpPut(34-39)HttpGet(15-19)HttpGet(41-46)VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Update/UpdateProgramCategoryHandler.cs (1)
Task(26-61)VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Delete/DeleteProgramCategoryHandler.cs (1)
Task(20-51)VictoryCenter/VictoryCenter.BLL/Queries/ProgramCategories/GetProgramCategoriesHandler.cs (1)
Task(27-50)VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Create/CreateProgramCategoryHandler.cs (1)
Task(25-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and analyze
- GitHub Check: Build and analyze
🔇 Additional comments (4)
VictoryCenter/VictoryCenter.BLL/Commands/Programs/Update/UpdateProgramCommand.cs (1)
7-7: Command definition looks correctA concise MediatR command with Result return type. Matches the handler/validator flow described in the PR summary.
VictoryCenter/VictoryCenter.WebAPI/Controllers/ProgramCategories/ProgramCategoryController.cs (2)
11-13: Confirm role/permission scope for Admin-only endpointsThe controller is protected with [Authorize], but PR scope says these are Admin endpoints. If a stricter policy/role is required, consider applying it at the controller level.
Examples:
- [Authorize(Roles = "Admin")]
- [Authorize(Policy = "AdminOnly")]
Please confirm your auth setup; happy to suggest the exact attribute once we know your role/claim model.
14-18: LGTM on create wiring and mediator usageCommand wiring looks correct; model binding and DTO shape match the handler/validator.
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/ProgramCategories/Update/UpdateProgramCategoryTests.cs (1)
35-45: Good: success check precedes deserializationCalling EnsureSuccessStatusCode before deserializing avoids parsing error payloads into DTOs.
milrusy
left a comment
milrusy
left a comment
There was a problem hiding this comment.
pay attention to the comments and coderabbit suggestions, overall, good job!
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
VictoryCenter/VictoryCenter.BLL/Validators/Programs/BaseProgramValidator.cs (1)
14-17: Previous feedback addressed: nameof used instead of string literalThanks for switching to nameof(ProgramDto.Name) in messages—this avoids magic strings.
VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryBase.cs (1)
20-21: Document CountAsync(QueryOptions) semantics (ignore Include/OrderBy/Pagination, no-tracking)Please document that CountAsync(QueryOptions) should:
- Ignore Include/OrderBy/Selector and pagination (Offset/Limit).
- Always run as no-tracking.
This avoids accidental “page-sized” counts and unnecessary change tracking. See prior comment on the RepositoryBase implementation to ensure AsNoTracking is applied at query construction time.
Proposed XML doc (for discoverability):
- Task<int> CountAsync(QueryOptions<T>? queryOptions = null); + /// <summary> + /// Returns the total number of entities matching the provided filter. + /// Implementation must ignore Include, OrderBy, Selector, Offset, and Limit, + /// and execute as no-tracking for performance. + /// </summary> + Task<int> CountAsync(QueryOptions<T>? queryOptions = null);
🧹 Nitpick comments (10)
VictoryCenter/VictoryCenter.BLL/Validators/Programs/BaseProgramValidator.cs (4)
12-20: Harden Name validation: trim and prevent whitespace-only valuesToday " " or " a " could bypass intent because length checks count spaces. Trim first and optionally stop cascading after the first failure for clearer messages.
Apply this diff:
RuleFor(x => x.Name) - .NotEmpty() + .Cascade(CascadeMode.Stop) + .Transform(s => s?.Trim()) + .NotEmpty() .WithMessage(ErrorMessagesConstants.PropertyIsRequired(nameof(ProgramDto.Name))) .MaximumLength(ProgramConstants.MaxNameLength) .WithMessage( ErrorMessagesConstants.PropertyMustHaveAMaximumLengthOfNCharacters(nameof(ProgramDto.Name), ProgramConstants.MaxNameLength)) .MinimumLength(ProgramConstants.MinNameLength) .WithMessage( ErrorMessagesConstants.PropertyMustHaveAMinimumLengthOfNCharacters(nameof(ProgramDto.Name), ProgramConstants.MinNameLength));
38-39: Align error message with the actual request contract and validate each category idThe rule validates CategoriesId, but the message names Categories, which can confuse API consumers. Also, guard against non-positive ids.
Apply this diff:
- RuleFor(x => x.CategoriesId) - .NotEmpty().WithMessage(ErrorMessagesConstants.PropertyIsRequired(nameof(ProgramDto.Categories))); + RuleFor(x => x.CategoriesId) + .NotEmpty().WithMessage(ErrorMessagesConstants.PropertyIsRequired(nameof(CreateProgramDto.CategoriesId))); + RuleForEach(x => x.CategoriesId) + .GreaterThan(0) + .WithMessage(ErrorMessagesConstants.PropertyMustBePositive("CategoryId"));
35-36: Optional: unify error message style for enum validationMost messages come from ErrorMessagesConstants; consider using the same source for Status for consistency.
Apply this diff if you prefer consistent formatting:
- RuleFor(x => x.Status) - .IsInEnum().WithMessage(ProgramConstants.UnknownStatus); + RuleFor(x => x.Status) + .IsInEnum() + .WithMessage(ErrorMessagesConstants.PropertyMustBeValidEnum(nameof(ProgramDto.Status)));
8-11: Consider making this validator reusable across Create/Update via a common contractIf UpdateProgramValidator needs identical rules, extract a shared interface (e.g., IProgramUpsertDto with Name, Description, Status, CategoriesId) and change this to AbstractValidator. Then both Create/Update validators can SetValidator(new BaseProgramValidator()) without duplicating rules.
I can sketch the interface and refactors if you want to pursue this.
VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryBase.cs (1)
20-26: Unify Count semantics and return types to avoid confusion and overflow trapsYou now have two overloads named CountAsync with different return types: one returns int (with QueryOptions) and another returns long (with filter). This is easy to misuse and forces call sites to remember which overload returns what. Also, large tables can overflow int silently.
Recommend:
- Keep a single CountAsync that returns long, or
- Keep CountAsync returning int and add LongCountAsync for long counts, marking the long-returning CountAsync obsolete, or
- Provide both CountAsync(QueryOptions) and CountAsync(Expression<...>) returning the same type.
Given ProgramsFilterResponseDto.ProgramCount is int today, the least disruptive path is to:
- Rename the long-returning overload to LongCountAsync and keep the new QueryOptions-based CountAsync returning int.
Apply this interface-level diff (rename long overload; optionally add Obsolete later in a follow-up):
- Task<long> CountAsync(Expression<Func<T, bool>> filter); + Task<long> LongCountAsync(Expression<Func<T, bool>> filter);If you choose to mark the old signature obsolete in the same PR, add the attribute:
using System;- Task<long> CountAsync(Expression<Func<T, bool>> filter); + [Obsolete("Use CountAsync(QueryOptions<T>) for int or LongCountAsync for long results.")] + Task<long> LongCountAsync(Expression<Func<T, bool>> filter);VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetByFilters/GetProgramsByFiltersHandler.cs (2)
31-37: Filter composition is clear; consider minor readability tweakThe combined Status and Category filter is correct and EF-translatable. For readability, you could precompute the “has categories” boolean and use it in the expression to reduce nesting.
Example:
var hasCategories = programCategories is { Count: > 0 }; Expression<Func<Program, bool>> filter = p => (status == null || p.Status == status) && (!hasCategories || p.Categories.Any(c => programCategories!.Contains(c.Id)));
51-63: Image loading is resilient; consider micro-optimizations only if profiling indicates a hotspotThe parallel Base64 fetch with BlobStorageException suppression is solid. If this becomes a hot path:
- Batch or cache by BlobName to avoid duplicate fetches across programs.
- Consider returning a signed URL instead of Base64 for large payloads.
Optional and non-blocking.
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Programs/GetPrograms.cs (3)
175-201: Repository mock only handles GetAllAsync; add CountAsync setup and optional argument validationWithout CountAsync setup, handler’s ProgramCount can be 0 unnoticed. Also, validate the QueryOptions passed to GetAllAsync match expected Offset/Limit.
Apply:
private void SetUpRepositoryWrapper(List<DAL.Entities.Program> programs) { _repositoryWrapper.Setup(r => r.ProgramsRepository .GetAllAsync(It.IsAny<QueryOptions<DAL.Entities.Program>>())) .ReturnsAsync(programs); + + _repositoryWrapper.Setup(r => r.ProgramsRepository + .CountAsync(It.IsAny<QueryOptions<DAL.Entities.Program>>())) + .ReturnsAsync(programs.Count); }Optionally, validate pagination inputs per test case:
_repositoryWrapper.Verify(r => r.ProgramsRepository.GetAllAsync( It.Is<QueryOptions<DAL.Entities.Program>>(q => q.Offset == pageNumber * pageLimit && q.Limit == pageLimit)), Times.Once);Note: Adjust verification depending on your final “Offset” semantics (absolute offset vs. page index).
18-66: Consider adding a category filter test to exercise t.Categories.Any(...)Current fixtures don’t populate Categories, so the category filter path isn’t covered. Add a test that assigns category IDs to programs and verifies filtering by multiple category IDs.
If helpful, I can draft a self-contained test that initializes Categories for two programs and asserts handler returns only those when CategoryId = [ids].
68-105: Add an image-loading test to cover Base64 population and exception fallbackNo test currently sets ProgramDto.Image, so the parallel blob load path isn’t exercised.
Suggested additions:
- One test where ProgramDto.Image is non-null and blob service returns a value; assert Base64 gets set.
- One test where blob service throws BlobStorageException; assert Base64 is set to empty string.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetByFilters/GetProgramsByFiltersHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetByFilters/GetProgramsByFiltersQuery.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Validators/Programs/BaseProgramValidator.cs(1 hunks)VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryBase.cs(1 hunks)VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryBase.cs(4 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Programs/GetPrograms.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/ProgramCategories/CreateProgramCategoryValidatorTests.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/ProgramCategories/UpdateProgramCategoryValidatorTests.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/Programs/CreateProgramValidatorTests.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/Programs/UpdateProgramValidatorTests.cs(1 hunks)VictoryCenter/VictoryCenter.WebAPI/Controllers/Programs/ProgramController.cs(1 hunks)VictoryCenter/VictoryCenter.WebAPI/Extensions/ServicesConfiguration.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/Programs/CreateProgramValidatorTests.cs
- VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/ProgramCategories/UpdateProgramCategoryValidatorTests.cs
- VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/ProgramCategories/CreateProgramCategoryValidatorTests.cs
- VictoryCenter/VictoryCenter.WebAPI/Extensions/ServicesConfiguration.cs
- VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/Programs/UpdateProgramValidatorTests.cs
- VictoryCenter/VictoryCenter.WebAPI/Controllers/Programs/ProgramController.cs
- VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryBase.cs
🧰 Additional context used
🧬 Code graph analysis (4)
VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetByFilters/GetProgramsByFiltersQuery.cs (2)
VictoryCenter/VictoryCenter.BLL/DTOs/Programs/ProgramFilterRequestDto.cs (1)
ProgramFilterRequestDto(4-13)VictoryCenter/VictoryCenter.BLL/DTOs/Programs/ProgramsFilterResponseDto.cs (1)
ProgramsFilterResponseDto(3-7)
VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryBase.cs (1)
VictoryCenter/VictoryCenter.DAL/Repositories/Options/QueryOptions.cs (1)
QueryOptions(6-16)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Programs/GetPrograms.cs (5)
VictoryCenter/VictoryCenter.BLL/DTOs/Programs/ProgramDto.cs (1)
ProgramDto(7-19)VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetByFilters/GetProgramsByFiltersHandler.cs (3)
Task(29-72)GetProgramsByFiltersHandler(16-73)GetProgramsByFiltersHandler(22-27)VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryBase.cs (6)
Task(21-36)Task(38-50)Task(52-56)Task(58-69)Task(81-96)Task(98-101)VictoryCenter/VictoryCenter.BLL/DTOs/Programs/ProgramFilterRequestDto.cs (1)
ProgramFilterRequestDto(4-13)VictoryCenter/VictoryCenter.DAL/Repositories/Options/QueryOptions.cs (1)
QueryOptions(6-16)
VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetByFilters/GetProgramsByFiltersHandler.cs (3)
VictoryCenter/VictoryCenter.BLL/DTOs/Programs/ProgramsFilterResponseDto.cs (1)
ProgramsFilterResponseDto(3-7)VictoryCenter/VictoryCenter.DAL/Repositories/Options/QueryOptions.cs (1)
QueryOptions(6-16)VictoryCenter/VictoryCenter.BLL/DTOs/Programs/ProgramDto.cs (1)
ProgramDto(7-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and analyze
- GitHub Check: Build and analyze
🔇 Additional comments (2)
VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetByFilters/GetProgramsByFiltersQuery.cs (1)
7-7: LGTM: Query envelope looks clean and focusedThe record encapsulates the filter DTO and aligns with MediatR/Results usage. No concerns.
VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetByFilters/GetProgramsByFiltersHandler.cs (1)
38-46: Clarify pagination semantics in GetProgramsByFiltersHandlerI wasn’t able to find any existing unit tests that reference
.Offset, callSkip(...), or exercise this handler, so it’s unclear whetherOffsetis intended as an absolute row offset or as a zero-based page index. Let’s make the intent explicit and keep code and tests in sync:• Confirm intended semantics for
RequestDto.Offset:
– Absolute row offset (i.e. skip N rows)
– Page index (i.e. page 0, 1, 2…)
• If it’s absolute offset:
– Leave handler logic as-is
– Adjust existing/new tests to passOffset = pageIndex * pageSizefor each scenario
• If it’s a page index:
– Translate to an absolute offset here before passing to the repository
– e.g. computecomputedOffset = pageIndex * limit, falling back to rawOffsetif no validLimitFile to update:
• VictoryCenter.BLL/Queries/Programs/GetByFilters/GetProgramsByFiltersHandler.csSuggested change for page-index semantics:
- var queryOptions = new QueryOptions<Program> - { - Offset = request.RequestDto?.Offset is not null and > 0 ? (int)request.RequestDto.Offset : 0, - Limit = request.RequestDto?.Limit is not null and > 0 ? (int)request.RequestDto.Limit : 0, - Filter = filter, - Include = p => p.Include(x => x.Image).Include(x => x.Categories) - }; + // Determine whether Offset is a page index or an absolute row offset. + int computedOffset = 0; + if (request.RequestDto?.Offset is int page && page >= 0 && + request.RequestDto.Limit is int size && size > 0) + { + // Page-index semantics: multiply page index by page size. + computedOffset = page * size; + } + else + { + // Fallback to absolute offset (rows to skip). + computedOffset = request.RequestDto?.Offset ?? 0; + } + + var queryOptions = new QueryOptions<Program> + { + Offset = computedOffset, + Limit = request.RequestDto?.Limit is int l and > 0 ? l : 0, + Filter = filter, + Include = p => p.Include(x => x.Image).Include(x => x.Categories) + };
| IEnumerable<Program> programs = await _repositoryWrapper.ProgramsRepository.GetAllAsync(queryOptions); | ||
| var totalCount = await _repositoryWrapper.ProgramsRepository.CountAsync(queryOptions); | ||
| var programDto = _mapper.Map<IEnumerable<ProgramDto>>(programs).ToList(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a filter-only QueryOptions for CountAsync to avoid pagination/includes affecting totals
Passing the same QueryOptions risks page-sized counts if the implementation accidentally applies Offset/Limit, and it pays Include cost unnecessarily. Safer to pass filter-only options.
- IEnumerable<Program> programs = await _repositoryWrapper.ProgramsRepository.GetAllAsync(queryOptions);
- var totalCount = await _repositoryWrapper.ProgramsRepository.CountAsync(queryOptions);
+ IEnumerable<Program> programs = await _repositoryWrapper.ProgramsRepository.GetAllAsync(queryOptions);
+ var countOptions = new QueryOptions<Program>
+ {
+ Filter = filter,
+ AsNoTracking = true
+ };
+ var totalCount = await _repositoryWrapper.ProgramsRepository.CountAsync(countOptions);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| IEnumerable<Program> programs = await _repositoryWrapper.ProgramsRepository.GetAllAsync(queryOptions); | |
| var totalCount = await _repositoryWrapper.ProgramsRepository.CountAsync(queryOptions); | |
| var programDto = _mapper.Map<IEnumerable<ProgramDto>>(programs).ToList(); | |
| IEnumerable<Program> programs = await _repositoryWrapper.ProgramsRepository.GetAllAsync(queryOptions); | |
| var countOptions = new QueryOptions<Program> | |
| { | |
| Filter = filter, | |
| AsNoTracking = true | |
| }; | |
| var totalCount = await _repositoryWrapper.ProgramsRepository.CountAsync(countOptions); | |
| var programDto = _mapper.Map<IEnumerable<ProgramDto>>(programs).ToList(); |
🤖 Prompt for AI Agents
In
VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetByFilters/GetProgramsByFiltersHandler.cs
around lines 48 to 50, replace the current call that reuses the full
queryOptions for CountAsync with a filter-only QueryOptions instance (i.e., a
copy of the incoming options with Offset/Limit removed and Includes cleared) so
the count is computed using only filter criteria and does not pay include cost
or get limited by pagination; construct the new QueryOptions from the original
filters, pass that to CountAsync, and keep the original queryOptions (with
includes/paging) for GetAllAsync.
| RuleFor(x => x.Description) | ||
| .MaximumLength(ProgramConstants.MaxDescriptionLength) | ||
| .WithMessage(ErrorMessagesConstants | ||
| .PropertyMustHaveAMaximumLengthOfNCharacters(nameof(ProgramDto.Description), ProgramConstants.MaxDescriptionLength)) | ||
| .MinimumLength(ProgramConstants.MinDescriptionLength) | ||
| .WithMessage(ErrorMessagesConstants | ||
| .PropertyMustHaveAMinimumLengthOfNCharacters(nameof(ProgramDto.Description), ProgramConstants.MinDescriptionLength)); | ||
|
|
||
| RuleFor(x => x.Description) | ||
| .NotEmpty() | ||
| .WithMessage(ErrorMessagesConstants.PropertyIsRequired(nameof(ProgramDto.Description))) | ||
| .When(x => x.Status == Status.Published); |
There was a problem hiding this comment.
Draft/Unpublished Description currently fails validation due to unconditional MinimumLength
As written, MinimumLength is applied even when Description is null/empty, which will block creating Draft/Unpublished programs without a description. Keep the length rules only when a non-empty description is provided; separately enforce NotEmpty for Published.
Apply this diff to fix the conditional logic and trim before validation:
- RuleFor(x => x.Description)
- .MaximumLength(ProgramConstants.MaxDescriptionLength)
- .WithMessage(ErrorMessagesConstants
- .PropertyMustHaveAMaximumLengthOfNCharacters(nameof(ProgramDto.Description), ProgramConstants.MaxDescriptionLength))
- .MinimumLength(ProgramConstants.MinDescriptionLength)
- .WithMessage(ErrorMessagesConstants
- .PropertyMustHaveAMinimumLengthOfNCharacters(nameof(ProgramDto.Description), ProgramConstants.MinDescriptionLength));
-
- RuleFor(x => x.Description)
- .NotEmpty()
- .WithMessage(ErrorMessagesConstants.PropertyIsRequired(nameof(ProgramDto.Description)))
- .When(x => x.Status == Status.Published);
+ // If provided, enforce length limits (works for both Draft/Unpublished and Published)
+ RuleFor(x => x.Description)
+ .Transform(s => s?.Trim())
+ .MaximumLength(ProgramConstants.MaxDescriptionLength)
+ .WithMessage(ErrorMessagesConstants
+ .PropertyMustHaveAMaximumLengthOfNCharacters(nameof(ProgramDto.Description), ProgramConstants.MaxDescriptionLength))
+ .MinimumLength(ProgramConstants.MinDescriptionLength)
+ .WithMessage(ErrorMessagesConstants
+ .PropertyMustHaveAMinimumLengthOfNCharacters(nameof(ProgramDto.Description), ProgramConstants.MinDescriptionLength))
+ .When(x => !string.IsNullOrWhiteSpace(x.Description));
+
+ // Published programs must have a non-empty description
+ RuleFor(x => x.Description)
+ .Transform(s => s?.Trim())
+ .NotEmpty()
+ .WithMessage(ErrorMessagesConstants.PropertyIsRequired(nameof(ProgramDto.Description)))
+ .When(x => x.Status == Status.Published);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RuleFor(x => x.Description) | |
| .MaximumLength(ProgramConstants.MaxDescriptionLength) | |
| .WithMessage(ErrorMessagesConstants | |
| .PropertyMustHaveAMaximumLengthOfNCharacters(nameof(ProgramDto.Description), ProgramConstants.MaxDescriptionLength)) | |
| .MinimumLength(ProgramConstants.MinDescriptionLength) | |
| .WithMessage(ErrorMessagesConstants | |
| .PropertyMustHaveAMinimumLengthOfNCharacters(nameof(ProgramDto.Description), ProgramConstants.MinDescriptionLength)); | |
| RuleFor(x => x.Description) | |
| .NotEmpty() | |
| .WithMessage(ErrorMessagesConstants.PropertyIsRequired(nameof(ProgramDto.Description))) | |
| .When(x => x.Status == Status.Published); | |
| // If provided, enforce length limits (works for both Draft/Unpublished and Published) | |
| RuleFor(x => x.Description) | |
| .Transform(s => s?.Trim()) | |
| .MaximumLength(ProgramConstants.MaxDescriptionLength) | |
| .WithMessage(ErrorMessagesConstants | |
| .PropertyMustHaveAMaximumLengthOfNCharacters(nameof(ProgramDto.Description), ProgramConstants.MaxDescriptionLength)) | |
| .MinimumLength(ProgramConstants.MinDescriptionLength) | |
| .WithMessage(ErrorMessagesConstants | |
| .PropertyMustHaveAMinimumLengthOfNCharacters(nameof(ProgramDto.Description), ProgramConstants.MinDescriptionLength)) | |
| .When(x => !string.IsNullOrWhiteSpace(x.Description)); | |
| // Published programs must have a non-empty description | |
| RuleFor(x => x.Description) | |
| .Transform(s => s?.Trim()) | |
| .NotEmpty() | |
| .WithMessage(ErrorMessagesConstants.PropertyIsRequired(nameof(ProgramDto.Description))) | |
| .When(x => x.Status == Status.Published); |
🤖 Prompt for AI Agents
VictoryCenter/VictoryCenter.BLL/Validators/Programs/BaseProgramValidator.cs
lines 22-33: The MinimumLength/MaximumLength checks are currently unconditional
and run even for null/empty Description; trim the Description before validating
and apply length rules only when a non-empty value exists, while keeping
NotEmpty enforced only for Published status. Specifically, add a Transform/Trim
step (e.g., .Transform(x => x?.Trim())) or otherwise trim Description before
running validators, and wrap the .MaximumLength and .MinimumLength rules with a
.When(x => !string.IsNullOrWhiteSpace(x.Description)) (or use .Matches/Custom
when trimmed) so they only run for non-empty descriptions; leave the separate
RuleFor(...).NotEmpty().When(x => x.Status == Status.Published) unchanged.
| [Theory] | ||
| [InlineData(0, 0)] | ||
| [InlineData(0, 2)] | ||
| [InlineData(1, 2)] | ||
| public async Task Handle_ShouldReturnAllPrograms_NoFilters(int pageNumber, int pageLimit) | ||
| { | ||
| var programResponseDto = _responseDto | ||
| .Skip(pageNumber * pageLimit) | ||
| .Take(pageLimit) | ||
| .ToList(); | ||
|
|
||
| var programEntities = _programs | ||
| .Skip(pageNumber * pageLimit) | ||
| .Take(pageLimit) | ||
| .ToList(); | ||
|
|
||
| SetUpDependencies(programResponseDto, programEntities); | ||
|
|
||
| var handler = new GetProgramsByFiltersHandler(_mockMapper.Object, _blobService.Object, _repositoryWrapper.Object); | ||
|
|
||
| ProgramFilterRequestDto requestDto = new() | ||
| { | ||
| Offset = pageNumber, | ||
| Limit = pageLimit, | ||
| Status = null, | ||
| CategoryId = null | ||
| }; | ||
|
|
||
| var result = await handler | ||
| .Handle(new GetProgramsByFiltersQuery(requestDto), CancellationToken.None); | ||
|
|
||
| Assert.True(result.IsSuccess); | ||
| Assert.NotNull(result.Value); | ||
| Assert.Equal(programResponseDto, result.Value.Programs); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test name vs behavior mismatch; strengthen assertions and Count setup
- InlineData (0, 0) yields Take(0) => empty list, which contradicts “ShouldReturnAllPrograms_NoFilters”.
- Tests don’t assert ProgramCount and never set up CountAsync, so handler’s total can silently be 0.
Update:
- Rename the test to reflect pagination behavior.
- Set up CountAsync to return the filtered total.
- Assert ProgramCount matches expectation.
- public async Task Handle_ShouldReturnAllPrograms_NoFilters(int pageNumber, int pageLimit)
+ public async Task Handle_ShouldPaginatePrograms(int pageNumber, int pageLimit)
@@
- SetUpDependencies(programResponseDto, programEntities);
+ SetUpDependencies(programResponseDto, programEntities);
+ _repositoryWrapper.Setup(r => r.ProgramsRepository
+ .CountAsync(It.IsAny<QueryOptions<DAL.Entities.Program>>()))
+ .ReturnsAsync(_programs.Count); // total (no filters)
@@
- Assert.Equal(programResponseDto, result.Value.Programs);
+ Assert.Equal(programResponseDto, result.Value.Programs);
+ Assert.Equal(_programs.Count, result.Value.ProgramCount);
+
+ // Ensure count is computed without pagination
+ _repositoryWrapper.Verify(r => r.ProgramsRepository.CountAsync(
+ It.Is<QueryOptions<DAL.Entities.Program>>(q => q.Offset == 0 && q.Limit == 0)), Times.Once);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [Theory] | |
| [InlineData(0, 0)] | |
| [InlineData(0, 2)] | |
| [InlineData(1, 2)] | |
| public async Task Handle_ShouldReturnAllPrograms_NoFilters(int pageNumber, int pageLimit) | |
| { | |
| var programResponseDto = _responseDto | |
| .Skip(pageNumber * pageLimit) | |
| .Take(pageLimit) | |
| .ToList(); | |
| var programEntities = _programs | |
| .Skip(pageNumber * pageLimit) | |
| .Take(pageLimit) | |
| .ToList(); | |
| SetUpDependencies(programResponseDto, programEntities); | |
| var handler = new GetProgramsByFiltersHandler(_mockMapper.Object, _blobService.Object, _repositoryWrapper.Object); | |
| ProgramFilterRequestDto requestDto = new() | |
| { | |
| Offset = pageNumber, | |
| Limit = pageLimit, | |
| Status = null, | |
| CategoryId = null | |
| }; | |
| var result = await handler | |
| .Handle(new GetProgramsByFiltersQuery(requestDto), CancellationToken.None); | |
| Assert.True(result.IsSuccess); | |
| Assert.NotNull(result.Value); | |
| Assert.Equal(programResponseDto, result.Value.Programs); | |
| } | |
| [Theory] | |
| [InlineData(0, 0)] | |
| [InlineData(0, 2)] | |
| [InlineData(1, 2)] | |
| public async Task Handle_ShouldPaginatePrograms(int pageNumber, int pageLimit) | |
| { | |
| var programResponseDto = _responseDto | |
| .Skip(pageNumber * pageLimit) | |
| .Take(pageLimit) | |
| .ToList(); | |
| var programEntities = _programs | |
| .Skip(pageNumber * pageLimit) | |
| .Take(pageLimit) | |
| .ToList(); | |
| SetUpDependencies(programResponseDto, programEntities); | |
| _repositoryWrapper.Setup(r => r.ProgramsRepository | |
| .CountAsync(It.IsAny<QueryOptions<DAL.Entities.Program>>())) | |
| .ReturnsAsync(_programs.Count); // total (no filters) | |
| var handler = new GetProgramsByFiltersHandler(_mockMapper.Object, _blobService.Object, _repositoryWrapper.Object); | |
| ProgramFilterRequestDto requestDto = new() | |
| { | |
| Offset = pageNumber, | |
| Limit = pageLimit, | |
| Status = null, | |
| CategoryId = null | |
| }; | |
| var result = await handler | |
| .Handle(new GetProgramsByFiltersQuery(requestDto), CancellationToken.None); | |
| Assert.True(result.IsSuccess); | |
| Assert.NotNull(result.Value); | |
| Assert.Equal(programResponseDto, result.Value.Programs); | |
| Assert.Equal(_programs.Count, result.Value.ProgramCount); | |
| // Ensure count is computed without pagination | |
| _repositoryWrapper.Verify(r => r.ProgramsRepository.CountAsync( | |
| It.Is<QueryOptions<DAL.Entities.Program>>(q => q.Offset == 0 && q.Limit == 0)), Times.Once); | |
| } |
🤖 Prompt for AI Agents
In
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Programs/GetPrograms.cs
around lines 114-148, the test name and data are misleading (InlineData(0,0)
produces an empty page) and the test never configures or asserts the repository
total count; update the test to (1) rename it to reflect pagination (e.g.
Handle_ShouldReturnPagedPrograms_WithPaginationParameters), (2) remove or
replace the (0,0) case with a valid pageLimit (>0) or adjust expected results
accordingly, (3) extend SetUpDependencies to mock the repository CountAsync to
return the full filtered total (e.g. _programs.Count or expected total before
paging), and (4) add an assertion that result.Value.ProgramCount equals that
mocked total while keeping the existing assertion that Programs equals the paged
list.
| [Theory] | ||
| [InlineData(Status.Published)] | ||
| [InlineData(Status.Draft)] | ||
| public async Task Handle_ShouldFilterByStatus(Status status) | ||
| { | ||
| var programEntities = _programs.Where(p => p.Status == status).ToList(); | ||
| var programResponseDtos = _responseDto.Where(p => p.Status == status).ToList(); | ||
|
|
||
| SetUpDependencies(programResponseDtos, programEntities); | ||
|
|
||
| ProgramFilterRequestDto requestDto = new() | ||
| { | ||
| Offset = 0, | ||
| Status = status, | ||
| CategoryId = null | ||
| }; | ||
|
|
||
| var handler = new GetProgramsByFiltersHandler(_mockMapper.Object, _blobService.Object, _repositoryWrapper.Object); | ||
| var result = await handler.Handle(new GetProgramsByFiltersQuery(requestDto), CancellationToken.None); | ||
|
|
||
| Assert.True(result.IsSuccess); | ||
| Assert.NotNull(result.Value); | ||
| Assert.Equal(programResponseDtos, result.Value.Programs); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add Count setup/assertions and verify filter-only counting for status tests
Mirrors the pagination test gap. Also verifies that CountAsync receives non-paginated options.
@@
- SetUpDependencies(programResponseDtos, programEntities);
+ SetUpDependencies(programResponseDtos, programEntities);
+ _repositoryWrapper.Setup(r => r.ProgramsRepository
+ .CountAsync(It.IsAny<QueryOptions<DAL.Entities.Program>>()))
+ .ReturnsAsync(programEntities.Count); // filtered total
@@
- Assert.Equal(programResponseDtos, result.Value.Programs);
+ Assert.Equal(programResponseDtos, result.Value.Programs);
+ Assert.Equal(programEntities.Count, result.Value.ProgramCount);
+
+ _repositoryWrapper.Verify(r => r.ProgramsRepository.CountAsync(
+ It.Is<QueryOptions<DAL.Entities.Program>>(q => q.Offset == 0 && q.Limit == 0)), Times.Once);🤖 Prompt for AI Agents
In
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Programs/GetPrograms.cs
around lines 150 to 173, the test validates filtered results but does not set up
or assert the total count nor verify that the repository's CountAsync is called
with non-paginated options; update the test to mock the repository CountAsync to
return programEntities.Count, assert the handler's returned total count matches
that value, and add a mock verification that CountAsync was invoked with filter
options that do not include Offset/Limit (i.e., no pagination) so the test
ensures counting uses only the filter criteria.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Update/UpdateProgramCategoryHandler.cs (1)
57-60: Good: aggregate and return all validation errorsThis matches the codebase preference to surface all validation issues in one response. Keep it consistent across handlers (UpdateProgramHandler currently returns a single message).
If you want, I can prep a quick sweep to normalize handlers to return all validation errors.
#!/bin/bash # Find places catching FluentValidation exceptions and how they construct failures rg -n --type=cs -C2 'catch\s*\(\s*ValidationException\s+ex\s*\)'VictoryCenter/VictoryCenter.BLL/Commands/Programs/Update/UpdateProgramHandler.cs (1)
51-56: Validate that all requested category IDs exist (silently dropping missing IDs today)If some IDs aren’t found, they’re omitted and the program’s categories are unintentionally altered without a clear error. Please fail fast with the missing IDs.
IEnumerable<ProgramCategory> newCategories = await _repositoryWrapper.ProgramCategoriesRepository.GetAllAsync( new QueryOptions<ProgramCategory> { Filter = category => request.updateProgramDto.CategoriesId.Contains(category.Id), AsNoTracking = false }); + +// Ensure all requested categories exist +var requestedIds = request.updateProgramDto.CategoriesId.ToHashSet(); +var foundIds = newCategories.Select(c => c.Id).ToHashSet(); +if (requestedIds.Count != foundIds.Count) +{ + var missing = requestedIds.Except(foundIds); + return Result.Fail<ProgramDto>($"Categories not found: {string.Join(", ", missing)}"); +}
🧹 Nitpick comments (12)
VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Update/UpdateProgramCategoryHandler.cs (4)
32-36: Use tracked query for updates (AsNoTracking = false) to align with other handlersFetching the entity with tracking simplifies updates and matches UpdateProgramHandler. It also keeps you future-proof for concurrency tokens and EF change tracking behavior.
- .GetFirstOrDefaultAsync(new QueryOptions<ProgramCategory> - { - Filter = programCategory => programCategory.Id == request.id - }); + .GetFirstOrDefaultAsync(new QueryOptions<ProgramCategory> + { + Filter = programCategory => programCategory.Id == request.id, + AsNoTracking = false + });
44-52: Map in place; avoid the extra variable and explicitly preserve CreatedAtThis keeps the code simpler and avoids accidental loss of CreatedAt if the mapping profile changes later.
- ProgramCategory entityToUpdate = _mapper.Map(request.updateProgramCategoryDto, programCategoryEntity); - entityToUpdate.CreatedAt = programCategoryEntity.CreatedAt; - - _repositoryWrapper.ProgramCategoriesRepository.Update(entityToUpdate); - - if (await _repositoryWrapper.SaveChangesAsync() > 0) - { - ProgramCategoryDto responseDto = _mapper.Map<ProgramCategoryDto>(entityToUpdate); - return Result.Ok(responseDto); - } + var createdAt = programCategoryEntity.CreatedAt; + _mapper.Map(request.updateProgramCategoryDto, programCategoryEntity); + programCategoryEntity.CreatedAt = createdAt; + + _repositoryWrapper.ProgramCategoriesRepository.Update(programCategoryEntity); + + if (await _repositoryWrapper.SaveChangesAsync() > 0) + { + ProgramCategoryDto responseDto = _mapper.Map<ProgramCategoryDto>(programCategoryEntity); + return Result.Ok(responseDto); + }
55-56: Consider idempotent “no-op update” behaviorReturning Fail when SaveChangesAsync == 0 may treat “no changes” as an error. If your API semantics prefer idempotency, consider returning Ok with the current state when the entity exists but nothing changed.
26-61: Enhance Name Validation for Trimming and UniquenessI verified that
UpdateProgramCategoryValidatorin
VictoryCenter.BLL/Validators/ProgramCategories/UpdateProgramCategoryValidator.cs
only appliesNotEmpty()andMaximumLength(...)toupdateProgramCategoryDto.Name(lines 11–14). It doesn’t currently trim leading/trailing whitespace or guard against duplicate names, which could lead to inconsistent or conflicting entries. As an optional refactoring to improve data consistency and prevent duplicate categories, consider:• Trim the Name before other checks:
RuleFor(cmd => cmd.updateProgramCategoryDto.Name) .Transform(name => name?.Trim()) .NotEmpty() .WithMessage(…) .MaximumLength(ProgramCategoryConstants.MaxNameLength) .WithMessage(…);• Enforce case- and whitespace-insensitive uniqueness (excluding the current record):
RuleFor(cmd => cmd.updateProgramCategoryDto.Name) // …existing rules… .MustAsync(async (cmd, name, ct) => { var normalized = name.Trim().ToLowerInvariant(); var existing = await _repositoryWrapper.ProgramCategoriesRepository .GetFirstOrDefaultAsync(new QueryOptions<ProgramCategory> { Filter = pc => pc.Name.ToLower() == normalized && pc.Id != cmd.id }); return existing is null; }) .WithMessage("A program category with this name already exists.");These changes are optional but will help ensure that names are stored in a canonical form and remain unique across updates.
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/GetFiltered/GetProgramsByFilters.cs (5)
39-41: DRY the query-string construction.
Factor into a helper to avoid duplication and improve readability. Alternatively, use QueryHelpers.AddQueryString.- var queryString = string.Join("&", query - .Where(kv => kv.Value is not null) - .Select(kv => $"{kv.Key}={Uri.EscapeDataString(kv.Value!)}")); + var queryString = BuildQueryString(query);- var queryString = string.Join("&", query - .Where(kv => kv.Value is not null) - .Select(kv => $"{kv.Key}={Uri.EscapeDataString(kv.Value!)}")); + var queryString = BuildQueryString(query);Add inside the class:
private static string BuildQueryString(Dictionary<string, string?> query) { var nonNull = query.Where(kv => kv.Value is not null) .ToDictionary(kv => kv.Key, kv => kv.Value!); return string.Join("&", nonNull.Select(kv => $"{kv.Key}={Uri.EscapeDataString(kv.Value)}")); }Also applies to: 67-69
54-57: Cover all enum values without manual InlineData.
Use MemberData to iterate every Status; keeps tests in sync when enum grows.- [InlineData(Status.Draft)] - [InlineData(Status.Published)] + [MemberData(nameof(AllStatuses))]Add inside the class:
public static IEnumerable<object[]> AllStatuses => Enum.GetValues<Status>().Select(s => new object[] { s });
25-29: Add deeper paging checks.
Optionally call the endpoint twice (same limit, offset vs. offset+1) and assert deterministic paging if the API defines a default sort; otherwise, request explicit sort to avoid flakiness.
58-65: Add coverage for categoryId and combined filters.
Propose resilient tests that derive a valid category from existing data to avoid brittle seed assumptions.Example to add:
[Fact] public async Task GetPrograms_ShouldReturnPrograms_FilteredByCategory() { // Discover a valid category from data var firstPage = await _fixture.HttpClient.GetAsync("/api/Program?offset=0&limit=1"); firstPage.EnsureSuccessStatusCode(); var dto = JsonConvert.DeserializeObject<ProgramsFilterResponseDto>( await firstPage.Content.ReadAsStringAsync())!; Assert.NotNull(dto.Programs); if (dto.Programs.Count == 0) return; // nothing to assert var categoryId = dto.Programs[0].CategoryId; var response = await _fixture.HttpClient.GetAsync($"/api/Program?offset=0&limit=10&categoryId={categoryId}"); response.EnsureSuccessStatusCode(); var filtered = JsonConvert.DeserializeObject<ProgramsFilterResponseDto>( await response.Content.ReadAsStringAsync())!; Assert.All(filtered.Programs, p => Assert.Equal(categoryId, p.CategoryId)); }
23-24: Confirm environment reset strategy.
InitializeAsync runs once per class instance, not per [Theory] case. If future tests mutate data, consider per-test reset or a fixture method to clear DB state.VictoryCenter/VictoryCenter.BLL/Commands/Programs/Update/UpdateProgramHandler.cs (3)
37-44: Consider including Image in the initial query IncludeYou later manipulate Image; eagerly loading it keeps the tracked graph consistent and avoids surprises with navigation fix-up.
- Include = program => program.Include(p => p.Categories), + Include = program => program + .Include(p => p.Categories) + .Include(p => p.Image),
84-91: Treat “no changes” as success (idempotent update)If the payload matches current state, SaveChangesAsync returns 0 and you currently fail the operation. Prefer returning the current state as success.
- if (await _repositoryWrapper.SaveChangesAsync() > 0) - { - ProgramDto responseDto = _mapper.Map<ProgramDto>(programToUpdate); - return Result.Ok(responseDto); - } - - return Result.Fail<ProgramDto>(ProgramConstants.FailedToUpdateProgram); + await _repositoryWrapper.SaveChangesAsync(); + ProgramDto responseDto = _mapper.Map<ProgramDto>(programToUpdate); + return Result.Ok(responseDto);
40-41: Naming Refactor Suggestion: PascalCase Command PropertiesThe
UpdateProgramCommandrecord currently declares its primary‐constructor parameters with camel-case identifiers (updateProgramDto,id), which become public properties of the same casing. In C#, public members should follow PascalCase. Aligning these names will improve consistency and readability across handlers, validators, and tests.Key locations to update:
- VictoryCenter.BLL.Commands.Programs.Update.UpdateProgramCommand.cs
- VictoryCenter.BLL.Commands.Programs.Update.UpdateProgramHandler.cs (and any other usages)
Proposed diff for
UpdateProgramCommand.cs:-public record UpdateProgramCommand(UpdateProgramDto updateProgramDto, long id) : IRequest<Result<ProgramDto>>; +public record UpdateProgramCommand(UpdateProgramDto UpdateProgramDto, long Id) : IRequest<Result<ProgramDto>>;Then in
UpdateProgramHandler.cs(around lines 40–41), update usages:- Filter = program => program.Id == request.id, + Filter = program => program.Id == request.Id, - Include = program => program.Include(p => p.Categories), + Include = program => program.Include(p => p.Categories), - // mapping and other code referencing: - _mapper.Map(request.updateProgramDto, entity); + _mapper.Map(request.UpdateProgramDto, entity);• Don’t forget to update any validators, tests, controller actions, and other handlers that reference
request.idorrequest.updateProgramDto.
• This rename is a breaking change to the command’s shape—be sure to update all call sites accordingly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
VictoryCenter/VictoryCenter.DAL/Migrations/20250827213904_RenameTable.Designer.csis excluded by!**/Migrations/**,!**/*.Designer.csVictoryCenter/VictoryCenter.DAL/Migrations/20250827213904_RenameTable.csis excluded by!**/Migrations/**VictoryCenter/VictoryCenter.DAL/Migrations/VictoryCenterDbContextModelSnapshot.csis excluded by!**/Migrations/**
📒 Files selected for processing (9)
VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Create/CreateProgramCategoryHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Delete/DeleteProgramCategoryHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Update/UpdateProgramCategoryHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Programs/Create/CreateProgramHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Programs/Update/UpdateProgramHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Queries/ProgramCategories/GetProgramCategoriesHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetPublished/GetPublishedProgramsHandler.cs(1 hunks)VictoryCenter/VictoryCenter.DAL/Data/EntityTypeConfigurations/ProgramConfig.cs(1 hunks)VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/GetFiltered/GetProgramsByFilters.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Delete/DeleteProgramCategoryHandler.cs
- VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Create/CreateProgramCategoryHandler.cs
- VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetPublished/GetPublishedProgramsHandler.cs
- VictoryCenter/VictoryCenter.BLL/Queries/ProgramCategories/GetProgramCategoriesHandler.cs
- VictoryCenter/VictoryCenter.BLL/Commands/Programs/Create/CreateProgramHandler.cs
- VictoryCenter/VictoryCenter.DAL/Data/EntityTypeConfigurations/ProgramConfig.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-20T18:22:51.823Z
Learnt from: Oleh-Bashtovyi
PR: ita-social-projects/VictoryCenter-Back#179
File: VictoryCenter/VictoryCenter.BLL/Commands/TeamMembers/Reorder/ReorderTeamMembersHandler.cs:27-28
Timestamp: 2025-06-20T18:22:51.823Z
Learning: In the VictoryCenter codebase, FluentValidation is used for input validation in MediatR handlers. The handlers call ValidateAndThrowAsync() early in the Handle method, and validation exceptions are caught and converted to Result.Fail responses. This means validation logic should be kept in the validator classes rather than duplicated in the handlers.
Applied to files:
VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Update/UpdateProgramCategoryHandler.csVictoryCenter/VictoryCenter.BLL/Commands/Programs/Update/UpdateProgramHandler.cs
📚 Learning: 2025-06-17T20:32:16.009Z
Learnt from: VladimirSushinsky
PR: ita-social-projects/VictoryCenter-Back#114
File: VictoryCenter/VictoryCenter.IntegrationTests/Utils/Seeder/TeamMembersSeeder/TeamMemberSeeder.cs:20-24
Timestamp: 2025-06-17T20:32:16.009Z
Learning: In the VictoryCenter test seeder for TeamMember entities, the last category in the categories list is intentionally excluded from having team members assigned to it (using `categories[i % (categories.Count - 1)].Id`). This design ensures that the last category remains available for delete tests without foreign key constraint violations, as delete operations require categories with no related team members.
Applied to files:
VictoryCenter/VictoryCenter.BLL/Commands/Programs/Update/UpdateProgramHandler.cs
🧬 Code graph analysis (3)
VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Update/UpdateProgramCategoryHandler.cs (8)
VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Create/CreateProgramCategoryHandler.cs (1)
Task(25-47)VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Delete/DeleteProgramCategoryHandler.cs (1)
Task(20-49)VictoryCenter/VictoryCenter.BLL/Commands/Programs/Update/UpdateProgramHandler.cs (1)
Task(31-100)VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryBase.cs (2)
Task(21-36)Task(38-50)VictoryCenter/VictoryCenter.DAL/Entities/ProgramCategory.cs (1)
ProgramCategory(3-9)VictoryCenter/VictoryCenter.DAL/Repositories/Options/QueryOptions.cs (1)
QueryOptions(6-16)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (1)
ErrorMessagesConstants(3-66)VictoryCenter/VictoryCenter.BLL/Constants/ProgramCategoryConstants.cs (1)
ProgramCategoryConstants(3-12)
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/GetFiltered/GetProgramsByFilters.cs (1)
VictoryCenter/VictoryCenter.BLL/DTOs/Programs/ProgramsFilterResponseDto.cs (1)
ProgramsFilterResponseDto(3-7)
VictoryCenter/VictoryCenter.BLL/Commands/Programs/Update/UpdateProgramHandler.cs (7)
VictoryCenter/VictoryCenter.BLL/DTOs/Programs/ProgramDto.cs (1)
ProgramDto(7-19)VictoryCenter/VictoryCenter.BLL/Commands/Programs/Create/CreateProgramHandler.cs (1)
Task(30-80)VictoryCenter/VictoryCenter.BLL/Queries/Programs/GetPublished/GetPublishedProgramsHandler.cs (1)
Task(28-58)VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryBase.cs (6)
Task(21-36)Task(38-50)Task(52-56)Task(58-69)Task(81-96)Task(98-101)VictoryCenter/VictoryCenter.DAL/Repositories/Options/QueryOptions.cs (1)
QueryOptions(6-16)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (1)
ErrorMessagesConstants(3-66)VictoryCenter/VictoryCenter.BLL/Constants/ProgramConstants.cs (1)
ProgramConstants(3-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and analyze
- GitHub Check: Build and analyze
🔇 Additional comments (9)
VictoryCenter/VictoryCenter.BLL/Commands/ProgramCategories/Update/UpdateProgramCategoryHandler.cs (2)
49-49: Pass CancellationToken to persistence if supportedIf IRepositoryWrapper.SaveChangesAsync has an overload with token, pass cancellationToken to enable cooperative cancellation during shutdowns/timeouts.
Would you confirm the repository signature? If supported, update to:
await _repositoryWrapper.SaveChangesAsync(cancellationToken) > 0
30-31: Validation placement and timing look rightValidate early with ValidateAndThrowAsync and let the handler flow handle errors—consistent with our FluentValidation pattern.
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/GetFiltered/GetProgramsByFilters.cs (3)
1-5: Add xUnit import (if not globally included).
If there’s no global using for xUnit, this file won’t compile (attributes/interfaces unresolved).+using Xunit; using Newtonsoft.Json;
71-74: Verify model binding for Status query (string vs. int).
You send status as enum name (e.g., "Draft"). If the API expects numeric values, this will fail silently in future refactors.If needed, switch to numeric: ["status"] = ((int)status).ToString().
75-80: Enhance filter assertions and trim redundancyPlease extend the test to verify that every returned
ProgramDtoactually has the expected status and that the counts are consistent, and drop the unnecessary success‐status assertion.• Remove
Assert.True(response.IsSuccessStatusCode);– the framework will already fail the test on non-2xx responses.
• After deserializing, assert the payload is well-formed:Assert.NotNull(result); Assert.NotNull(result!.Programs);• Confirm filter effect:
Assert.All(result.Programs, p => Assert.Equal(status, p.Status));– Please verify that
ProgramDtoindeed exposes aStatusproperty of typeVictoryCenter.DAL.Enums.Status.
• Check pagination/count invariants:Assert.InRange(result.Programs.Count, 0, expectedPageSize); Assert.True(result.ProgramCount >= result.Programs.Count);This will ensure your integration test not only succeeds, but also validates both the filter behavior and the consistency of the returned counts.
VictoryCenter/VictoryCenter.BLL/Commands/Programs/Update/UpdateProgramHandler.cs (4)
31-36: Good: validation happens first thing in handlerEarly ValidateAndThrowAsync aligns with existing patterns and keeps handler logic lean.
45-49: Good: consistent NotFound error formatTyped NotFound message via ErrorMessagesConstants keeps API errors uniform.
75-80: Good: explicit category resetClearing and re-adding categories is straightforward for many-to-many updates and matches EF Core guidance.
58-59: Verified: AutoMapper mapping ignores Categories on update
Confirmed that in VictoryCenter.BLL/Mapping/Programs/ProgramsProfile.cs (lines 15–16), theCreateMap<UpdateProgramDto, Program>()call includes.ForMember(dest => dest.Categories, opt => opt.Ignore());so
_mapper.Map(request.updateProgramDto, programToUpdate)will not overwrite the existing navigation property.
| IEnumerable<ProgramCategory> newCategories = await _repositoryWrapper.ProgramCategoriesRepository.GetAllAsync( | ||
| new QueryOptions<ProgramCategory> | ||
| { | ||
| Filter = category => request.updateProgramDto.CategoriesId.Contains(category.Id), | ||
| AsNoTracking = false | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Guard against null CategoriesId to avoid NullReferenceException
If updateProgramDto.CategoriesId can be null (partial updates), the Contains() lambda will throw. Either ensure NotNull in the validator or treat null as “do not change categories”.
If you choose to handle it here:
-Filter = category => request.updateProgramDto.CategoriesId.Contains(category.Id),
+Filter = category => (request.updateProgramDto.CategoriesId ?? Array.Empty<long>())
+ .Contains(category.Id),If you rely on validation, confirm the rule exists:
🏁 Script executed:
#!/bin/bash
# Verify CategoriesId is validated as NotNull (UpdateProgramValidator on UpdateProgramCommand)
fd -a 'UpdateProgramValidator.cs'
rg -nP -C3 --type=cs 'class\s+UpdateProgramValidator\b'
rg -nP -C2 --type=cs '\.RuleFor\(\s*x\s*=>\s*x\.updateProgramDto\.CategoriesId\s*\)\.NotNull\(\)'Length of output: 1062
Ensure CategoriesId Is Always Guarded Against Null
It looks like UpdateProgramValidator does not include a .NotNull() rule for updateProgramDto.CategoriesId, so if that list comes through as null, the .Contains(category.Id) call in your handler will throw a NullReferenceException. Please address this by choosing one of the following fixes:
– Add a NotNull rule in VictoryCenter.BLL/Validators/Programs/UpdateProgramValidator.cs (mandatory fix):
public UpdateProgramValidator(BaseProgramValidator baseProgramValidator)
{
// existing rules…
RuleFor(x => x.updateProgramDto.CategoriesId)
.NotNull().WithMessage("CategoriesId must not be null.")
// you can also enforce at least one category if desired:
.Must(ids => ids.Any()).WithMessage("At least one category must be provided.");
}– Or guard against null in VictoryCenter.BLL/Commands/Programs/Update/UpdateProgramHandler.cs (handler-level fallback):
- Filter = category => request.updateProgramDto.CategoriesId.Contains(category.Id),
+ Filter = category =>
+ (request.updateProgramDto.CategoriesId ?? Array.Empty<long>())
+ .Contains(category.Id),Files to update:
- VictoryCenter.BLL/Validators/Programs/UpdateProgramValidator.cs
- VictoryCenter.BLL/Commands/Programs/Update/UpdateProgramHandler.cs (around the
Filterlambda)
Addressing this will prevent runtime crashes when CategoriesId is omitted in partial updates.
| if (programToUpdate.ImageId != null) | ||
| { | ||
| Image? newImage = await _repositoryWrapper.ImageRepository.GetFirstOrDefaultAsync(new QueryOptions<Image> | ||
| { | ||
| Filter = image => image.Id == request.updateProgramDto.ImageId, | ||
| AsNoTracking = false | ||
| }); | ||
| if (newImage is not null) | ||
| { | ||
| newImage.Base64 = await _blobService.FindFileInStorageAsBase64Async(newImage.BlobName, newImage.MimeType); | ||
| } | ||
|
|
||
| programToUpdate.Image = newImage; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align condition and retrieval on the same ImageId; clear navigation when removing image
Using programToUpdate.ImageId for the condition but request.updateProgramDto.ImageId for retrieval is brittle. Also, when ImageId is null, explicitly null the navigation to keep FK and nav consistent.
-if (programToUpdate.ImageId != null)
+var imageId = request.updateProgramDto.ImageId;
+if (imageId != null)
{
- Image? newImage = await _repositoryWrapper.ImageRepository.GetFirstOrDefaultAsync(new QueryOptions<Image>
+ Image? newImage = await _repositoryWrapper.ImageRepository.GetFirstOrDefaultAsync(new QueryOptions<Image>
{
- Filter = image => image.Id == request.updateProgramDto.ImageId,
+ Filter = image => image.Id == imageId,
AsNoTracking = false
});
if (newImage is not null)
{
newImage.Base64 = await _blobService.FindFileInStorageAsBase64Async(newImage.BlobName, newImage.MimeType);
}
programToUpdate.Image = newImage;
}
+else
+{
+ // Explicitly detach image when DTO requests removal
+ programToUpdate.Image = null;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (programToUpdate.ImageId != null) | |
| { | |
| Image? newImage = await _repositoryWrapper.ImageRepository.GetFirstOrDefaultAsync(new QueryOptions<Image> | |
| { | |
| Filter = image => image.Id == request.updateProgramDto.ImageId, | |
| AsNoTracking = false | |
| }); | |
| if (newImage is not null) | |
| { | |
| newImage.Base64 = await _blobService.FindFileInStorageAsBase64Async(newImage.BlobName, newImage.MimeType); | |
| } | |
| programToUpdate.Image = newImage; | |
| } | |
| var imageId = request.updateProgramDto.ImageId; | |
| if (imageId != null) | |
| { | |
| Image? newImage = await _repositoryWrapper.ImageRepository.GetFirstOrDefaultAsync(new QueryOptions<Image> | |
| { | |
| Filter = image => image.Id == imageId, | |
| AsNoTracking = false | |
| }); | |
| if (newImage is not null) | |
| { | |
| newImage.Base64 = await _blobService.FindFileInStorageAsBase64Async(newImage.BlobName, newImage.MimeType); | |
| } | |
| programToUpdate.Image = newImage; | |
| } | |
| else | |
| { | |
| // Explicitly detach image when DTO requests removal | |
| programToUpdate.Image = null; | |
| } |
🤖 Prompt for AI Agents
In
VictoryCenter/VictoryCenter.BLL/Commands/Programs/Update/UpdateProgramHandler.cs
around lines 60 to 73, the code checks programToUpdate.ImageId but then
retrieves the image using request.updateProgramDto.ImageId which can diverge;
change the logic to use the requested ImageId consistently (use
request.updateProgramDto.ImageId for both the null check and the repository
QueryOptions.Filter) and when the requested ImageId is null explicitly set
programToUpdate.Image = null (and ensure programToUpdate.ImageId is set
accordingly) so the FK and navigation property remain consistent; also keep the
blob-fetch and assignment only when a non-null image is found.
| catch (BlobStorageException) | ||
| { | ||
| return Result.Fail<ProgramDto>(ProgramConstants.FailedToUpdateProgram); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return a photo-retrieval-specific error on blob failure (consistent with Create handler)
CreateProgramHandler returns FailedRetrievingProgramPhoto; Update should do the same for consistency and clearer diagnostics.
-catch (BlobStorageException)
-{
- return Result.Fail<ProgramDto>(ProgramConstants.FailedToUpdateProgram);
-}
+catch (BlobStorageException)
+{
+ return Result.Fail<ProgramDto>(ProgramConstants.FailedRetrievingProgramPhoto);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| catch (BlobStorageException) | |
| { | |
| return Result.Fail<ProgramDto>(ProgramConstants.FailedToUpdateProgram); | |
| } | |
| catch (BlobStorageException) | |
| { | |
| return Result.Fail<ProgramDto>(ProgramConstants.FailedRetrievingProgramPhoto); | |
| } |
🤖 Prompt for AI Agents
In
VictoryCenter/VictoryCenter.BLL/Commands/Programs/Update/UpdateProgramHandler.cs
around lines 96-99, the catch for BlobStorageException currently returns
ProgramConstants.FailedToUpdateProgram; change it to return
ProgramConstants.FailedRetrievingProgramPhoto so the Update handler mirrors
CreateProgramHandler and provides a photo-retrieval-specific error. Replace the
return inside the BlobStorageException catch with
Result.Fail<ProgramDto>(ProgramConstants.FailedRetrievingProgramPhoto) and keep
the exception type handling unchanged.
| var content = await response.Content.ReadAsStringAsync(); | ||
| ProgramsFilterResponseDto? result = JsonConvert.DeserializeObject<ProgramsFilterResponseDto>(content); | ||
|
|
||
| Assert.NotNull(result); | ||
| Assert.True(response.IsSuccessStatusCode); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen assertions for pagination invariants.
Validate shape and basic paging guarantees; drop redundant IsSuccessStatusCode assertion.
- var content = await response.Content.ReadAsStringAsync();
- ProgramsFilterResponseDto? result = JsonConvert.DeserializeObject<ProgramsFilterResponseDto>(content);
-
- Assert.NotNull(result);
- Assert.True(response.IsSuccessStatusCode);
+ var content = await response.Content.ReadAsStringAsync();
+ var result = JsonConvert.DeserializeObject<ProgramsFilterResponseDto>(content);
+
+ Assert.NotNull(result);
+ Assert.NotNull(result!.Programs);
+ Assert.InRange(result.Programs.Count, 0, limit);
+ Assert.True(result.ProgramCount >= result.Programs.Count);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var content = await response.Content.ReadAsStringAsync(); | |
| ProgramsFilterResponseDto? result = JsonConvert.DeserializeObject<ProgramsFilterResponseDto>(content); | |
| Assert.NotNull(result); | |
| Assert.True(response.IsSuccessStatusCode); | |
| } | |
| var content = await response.Content.ReadAsStringAsync(); | |
| var result = JsonConvert.DeserializeObject<ProgramsFilterResponseDto>(content); | |
| Assert.NotNull(result); | |
| Assert.NotNull(result!.Programs); | |
| Assert.InRange(result.Programs.Count, 0, limit); | |
| Assert.True(result.ProgramCount >= result.Programs.Count); | |
| } |
🤖 Prompt for AI Agents
In
VictoryCenter/VictoryCenter.IntegrationTests/ControllerTests/Programs/GetFiltered/GetProgramsByFilters.cs
around lines 47-52, replace the redundant
Assert.True(response.IsSuccessStatusCode) with stronger assertions that validate
the response shape and basic paging invariants: keep Assert.NotNull(result);
assert the list of programs/items is not null and that its Count is <= the
paging PageSize; assert the paging object is not null and that PageNumber >= 1
and PageSize > 0; assert TotalCount >= items.Count and TotalPages >= 1 (or
compute total pages from TotalCount and PageSize if that property is absent).
Remove the IsSuccessStatusCode assertion and instead rely on the validated
result shape and paging invariants.
commented
Aug 28, 2025
|
7 participants
dev
JIRA
Code reviewers
Second Level Review
Summary of issue
Implemented backend endpoints for managing Programs and Program Categories on the Admin side, along with a public endpoint for fetching published programs.
Summary of change
Added implementation for Admin-side endpoints to manage Programs and Program Categories:
Implemented a public endpoint for visitors:
Included necessary DTOs, handlers, controller actions, and validators to support new functionality. Covered everything with unit and integration tests.
Testing approach
ToDo
CHECK LIST
Summary by CodeRabbit
New Features
Bug Fixes