Feature/issue 620 - Added BaseHandler #653
Conversation
WalkthroughThis PR implements a systematic refactoring of command handlers across the codebase, replacing MediatR's Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Specific areas requiring attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
VictoryCenter/VictoryCenter.BLL/Queries/Admin/Images/GetByName/GetImageByNameHandler.cs (1)
19-23: Remove unused constructor parameter.The
IBlobService blobServiceparameter is received in the constructor but never assigned to a field or used anywhere in the class. This creates unnecessary coupling and maintenance overhead.Apply this diff to remove the unused parameter:
-public GetImageByNameHandler(IRepositoryWrapper repositoryWrapper, IMapper mapper, IBlobService blobService) +public GetImageByNameHandler(IRepositoryWrapper repositoryWrapper, IMapper mapper) { _repositoryWrapper = repositoryWrapper; _mapper = mapper; }VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Images/DeleteImage.cs (1)
49-49: Update tests to match refactored handler API.The
DeleteImageHandlerhas been refactored toBaseHandler<DeleteImageCommand, long>with aHandleRequestmethod that returnslongdirectly and uses exception-based error handling. However, the tests still call the oldHandle()method and expectResult-based responses with.IsSuccessand.Errorsproperties, which no longer exist.Update all three test methods (lines 49, 72, 101) to:
- Call
handler.HandleRequest(command, CancellationToken.None)instead ofhandler.Handle()- Assert on the returned
longvalue directly or wrap calls in try-catch blocks to verify exceptions are thrown for error cases- Remove assertions on
.IsSuccess,.Value, and.ErrorspropertiesVictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Update/UpdateProgramHandler.cs (1)
55-63: Use request.ImageId for the condition.
Currently gated by the old value; new image won’t be loaded when adding one.- if (programToUpdate.ImageId != null) + if (request.UpdateProgramDto.ImageId != null) { Image? newImage = await _repositoryWrapper.ImageRepository.GetFirstOrDefaultAsync(new QueryOptions<Image> { - Filter = image => image.Id == request.UpdateProgramDto.ImageId, + Filter = image => image.Id == request.UpdateProgramDto.ImageId, AsNoTracking = false }); programToUpdate.Image = newImage; }VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.cs (1)
43-71: Missing TransactionScope.Complete(); changes will be rolled back.
This causes the insert to be aborted despite SaveChangesAsync succeeding.TeamMemberDto? result = _mapper.Map<TeamMemberDto>(entity); if (entity.ImageId != null) { Image? imageResult = await _repositoryWrapper.ImageRepository.GetFirstOrDefaultAsync( new QueryOptions<Image> { Filter = i => i.Id == entity.ImageId }); result.Image = _mapper.Map<ImageDto>(imageResult); } - return result; + scope.Complete(); + return result;VictoryCenter/VictoryCenter.BLL/Commands/Admin/Images/Update/UpdateImageHandler.cs (1)
63-71: Bug: Updated BlobName not persisted (DB can drift from storage).You set
imageEntity.BlobName = updatedBlobNamebut never save it. IfUpdateFileInStorageAsyncreturns a different name (e.g., MIME-driven extension), the DB won't reflect it. Persist within the same transaction.Apply:
if (await _repositoryWrapper.SaveChangesAsync() <= 0) { throw new DbUpdateException(ErrorMessagesConstants.FailedToUpdateEntity(typeof(Image))); } var updatedBlobName = await _blobService.UpdateFileInStorageAsync( imageEntity.BlobName, previousType, request.UpdateImageDto.Base64!, imageEntity.BlobName, request.UpdateImageDto.MimeType!); imageEntity.BlobName = updatedBlobName; + + // Persist the possibly changed blob name within the same transaction + _repositoryWrapper.ImageRepository.Update(imageEntity); + if (await _repositoryWrapper.SaveChangesAsync() <= 0) + { + throw new DbUpdateException(ErrorMessagesConstants.FailedToUpdateEntityInDatabase(typeof(Image))); + } ImageDto resultDto = _mapper.Map<Image, ImageDto>(imageEntity); transaction.Complete(); return resultDto; } catch (BlobStorageException e) { - throw new Exception(ErrorMessagesConstants.BlobStorageError(e.Message)); + // Preserve the original exception as InnerException for diagnostics + throw new Exception(ErrorMessagesConstants.BlobStorageError(e.Message), e); }Also applies to: 76-81
🧹 Nitpick comments (43)
VictoryCenter/VictoryCenter.BLL/Validators/TeamMembers/SearchTeamMemberValidator.cs (1)
10-18: Tighten validation: guard null DTO, trim, and stop on first failure.
- Add a NotNull guard for SearchTeamMemberDto for clearer errors.
- Trim FullName before length checks.
- Use Cascade(CascadeMode.Stop) to avoid redundant messages/work.
public class SearchTeamMemberValidator : AbstractValidator<SearchTeamMemberQuery> { public SearchTeamMemberValidator() { + RuleFor(x => x.SearchTeamMemberDto) + .NotNull() + .WithMessage(ErrorMessagesConstants.PropertyIsRequired(nameof(SearchTeamMemberQuery.SearchTeamMemberDto))); + - RuleFor(x => x.SearchTeamMemberDto.FullName) + RuleFor(x => x.SearchTeamMemberDto.FullName) + .Cascade(CascadeMode.Stop) + .Transform(s => s?.Trim()) .NotEmpty().WithMessage(ErrorMessagesConstants.PropertyIsRequired(nameof(SearchTeamMemberDto.FullName))) .MinimumLength(FullNameMinLength).WithMessage(ErrorMessagesConstants .PropertyMustHaveAMinimumLengthOfNCharacters(nameof(SearchTeamMemberDto.FullName), FullNameMinLength)) .MaximumLength(FullNameMaxLength).WithMessage(ErrorMessagesConstants .PropertyMustHaveAMaximumLengthOfNCharacters(nameof(SearchTeamMemberDto.FullName), FullNameMaxLength)); }VictoryCenter/VictoryCenter.BLL/Validators/Images/CreateImageValidator.cs (1)
13-17: Harden image validation: NotNull DTO, cascade-stop, and case‑insensitive MIME check.
- Use NotNull() for DTO (clearer intent for reference types).
- Add Cascade(CascadeMode.Stop) to Base64/MimeType rules to short‑circuit.
- MIME types are case‑insensitive per RFC; use a HashSet with OrdinalIgnoreCase.
Optional (no diff): consider a max Base64 length cap to prevent large allocations/DoS.
-using FluentValidation; +using FluentValidation; +using System.Collections.Generic; public class CreateImageValidator : AbstractValidator<CreateImageCommand> { - private static readonly string[] AllowedMimeTypes = { "image/jpeg", "image/jpg", "image/png", "image/webp" }; + private static readonly string[] AllowedMimeTypes = { "image/jpeg", "image/jpg", "image/png", "image/webp" }; + private static readonly HashSet<string> AllowedMimeTypesSet = + new(StringComparer.OrdinalIgnoreCase) { "image/jpeg", "image/jpg", "image/png", "image/webp" }; public CreateImageValidator() { - RuleFor(x => x.CreateImageDto).NotEmpty().WithMessage(ErrorMessagesConstants.PropertyIsRequired(nameof(CreateImageCommand.CreateImageDto))); - RuleFor(x => x.CreateImageDto.Base64) + RuleFor(x => x.CreateImageDto) + .NotNull().WithMessage(ErrorMessagesConstants.PropertyIsRequired(nameof(CreateImageCommand.CreateImageDto))); + RuleFor(x => x.CreateImageDto.Base64) + .Cascade(CascadeMode.Stop) .NotEmpty().WithMessage(ErrorMessagesConstants.PropertyIsRequired(nameof(CreateImageDto.Base64))) .Must(IsValidBase64).WithMessage(ImageConstants.Base64ValidationError); - RuleFor(x => x.CreateImageDto.MimeType) + RuleFor(x => x.CreateImageDto.MimeType) + .Cascade(CascadeMode.Stop) .NotEmpty().WithMessage(ErrorMessagesConstants.PropertyIsRequired(nameof(CreateImageDto.MimeType))) - .Must(mimeType => AllowedMimeTypes.Contains(mimeType)) + .Must(mimeType => mimeType is not null && AllowedMimeTypesSet.Contains(mimeType)) .WithMessage(ImageConstants.MimeTypeValidationError(AllowedMimeTypes)); }Also applies to: 18-21
VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/FaqQuestions/BaseFaqQuestionValidatorTests.cs (1)
35-41: Add boundary tests for min/max lengths (and consider whitespace).Current tests cover max; add exact boundary cases to lock inclusive constraints and prevent regressions. Also consider a whitespace‑only case if the validator enforces non‑whitespace.
Example additions:
[Fact] public void Validate_QuestionTextAtMin_ShouldNotHaveError() { var model = new CreateFaqQuestionDto { QuestionText = new('Q', BaseFaqQuestionValidator.QuestionTextMinLength), AnswerText = _validAnswerText, PageIds = [1] }; _validator.TestValidate(model).ShouldNotHaveValidationErrorFor(x => x.QuestionText); } [Fact] public void Validate_QuestionTextAtMax_ShouldNotHaveError() { var model = new CreateFaqQuestionDto { QuestionText = new('Q', BaseFaqQuestionValidator.QuestionTextMaxLength), AnswerText = _validAnswerText, PageIds = [1] }; _validator.TestValidate(model).ShouldNotHaveValidationErrorFor(x => x.QuestionText); } [Fact] public void Validate_AnswerTextAtMin_ShouldNotHaveError() { var model = new CreateFaqQuestionDto { QuestionText = _validQuestionText, AnswerText = new('A', BaseFaqQuestionValidator.AnswerTextMinLength), PageIds = [1] }; _validator.TestValidate(model).ShouldNotHaveValidationErrorFor(x => x.AnswerText); } [Fact] public void Validate_AnswerTextAtMax_ShouldNotHaveError() { var model = new CreateFaqQuestionDto { QuestionText = _validQuestionText, AnswerText = new('A', BaseFaqQuestionValidator.AnswerTextMaxLength), PageIds = [1] }; _validator.TestValidate(model).ShouldNotHaveValidationErrorFor(x => x.AnswerText); } // Optional if validator treats whitespace as invalid: [Fact] public void Validate_QuestionTextIsWhitespace_ShouldHaveError() { var model = new CreateFaqQuestionDto { QuestionText = " ", AnswerText = _validAnswerText, PageIds = [1] }; _validator.TestValidate(model).ShouldHaveValidationErrorFor(x => x.QuestionText); }Also applies to: 44-50, 62-68, 71-77
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/FaqQuestions/ReorderFaqQuestionsTests.cs (1)
151-153: Prefer explicit DbUpdateException overload and preserve inner causeUse the (message, innerException) overload to avoid version-specific ctor surprises and keep the causal chain.
- _mockRepoWrapper.Setup(x => x.SaveChangesAsync()) - .ThrowsAsync(new DbUpdateException(ErrorMessagesConstants.FailedToUpdateEntityInDatabase(typeof(FaqQuestion)))); + _mockRepoWrapper.Setup(x => x.SaveChangesAsync()) + .ThrowsAsync(new DbUpdateException( + ErrorMessagesConstants.FailedToUpdateEntityInDatabase(typeof(FaqQuestion)), + new Exception("Simulated DB failure")));VictoryCenter/VictoryCenter.BLL/Commands/Admin/ProgramCategories/Delete/DeleteProgramCategoryHandler.cs (1)
35-38: Avoid loading full collection when checking associationsInstead of Include + Count, prefer a query that checks existence (e.g., Programs.Any()) to reduce I/O and memory.
Example:
- Query only the flag: select new { HasPrograms = programCategory.Programs.Any() } or use a repository method that projects to a boolean.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Categories/Delete/DeleteCategoryHandler.cs (2)
19-26: Use specific exception types and lighter existence checks.
- Throwing plain Exception for NotFound/association violation makes downstream mapping harder. Prefer domain-specific exceptions (e.g., NotFoundException, BusinessRuleException) or ArgumentException/InvalidOperationException. Improves observability and HTTP mapping.
- Loading TeamMembers via Include just to check associations can be heavy. If repository supports it, prefer an existence query (Any) without Include.
Example (conceptual):
- Include = query => query.Include(x => x.TeamMembers) + // No Include; defer to a targeted existence checkand later:
- if (entityToDelete.TeamMembers.Count != 0) + if (await _repositoryWrapper.TeamMembersRepository + .AnyAsync(new QueryOptions<TeamMember> { Filter = tm => tm.CategoryId == request.Id })) { - throw new Exception(CategoryConstants.CantDeleteCategoryWhileAssociatedWithAnyTeamMember); + throw new BusinessRuleException(CategoryConstants.CantDeleteCategoryWhileAssociatedWithAnyTeamMember); }If TeamMembersRepository isn’t available, at least switch Count to Any on the loaded collection for clarity.
Please confirm whether IRepositoryWrapper exposes TeamMembersRepository and whether SaveChangesAsync/GetFirstOrDefaultAsync accept CancellationToken so we can pass it through.
Also applies to: 28-36
40-46: Propagate cancellation tokens to persistence calls.If signatures support it, pass cancellationToken into SaveChangesAsync (and earlier queries) to enable cooperative cancellation.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Images/Delete/DeleteImageHandler.cs (2)
55-60: Preserve the original BlobStorageException as inner exception.Re-throwing a new Exception loses the stack/context. Preserve it as InnerException.
- throw new Exception(ErrorMessagesConstants.BlobStorageError(e.Message)); + throw new Exception(ErrorMessagesConstants.BlobStorageError(e.Message), e);
24-47: Validate transaction semantics and cancellation tokens.
- You’re using TransactionScope for DB work and then an external blob delete. The current order is fine (DB change enlists, rollbacks if blob delete fails), but add a brief comment to document the intent to avoid orphaned DB rows if storage fails.
- If available, pass cancellationToken to repository calls.
- if (await _repositoryWrapper.SaveChangesAsync() <= 0) + if (await _repositoryWrapper.SaveChangesAsync(/*cancellationToken*/) <= 0)Confirm BeginTransaction returns a TransactionScope that enlists the EF context; if it returns IDbContextTransaction instead, use using var tx = await Db.Database.BeginTransactionAsync(cancellationToken).
Also applies to: 48-51
VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Reorder/ReorderFaqQuestionsHandler.cs (4)
49-56: Message mismatch: checking priorities, but error says “IdsAreNonConsecutive”.You validate that existing priorities are contiguous; the message implies ID consecutiveness. Rename the constant or change the message for clarity (e.g., SelectionMustBeContiguousByPriority).
- throw new Exception(FaqConstants.IdsAreNonConsecutive); + throw new Exception(FaqConstants.SelectionMustBeContiguousByPriority);If changing the constant isn’t feasible now, at least adjust the message string.
If there’s a unique constraint on (PageId, Priority), keep this contiguity check; otherwise, this guard can be relaxed.
62-69: Reduce O(n^2) lookups and ensure temporary priorities are strictly negative.
- Repeated Single(...) inside the loop is O(n^2). Use a dictionary.
- If any Priority is 0,
-faq.Prioritystays 0; ensure negative values.-foreach (var faq in questionsToReorder) -{ - faq.Priority = -faq.Priority; -} +foreach (var faq in questionsToReorder) +{ + faq.Priority = -(Math.Abs(faq.Priority) + 1); // guarantee strictly negative +} ... -foreach (var questionId in orderedIds) -{ - questionsToReorder.Single(e => e.QuestionId == questionId).Priority = minPriorityToSet++; -} +var map = questionsToReorder.ToDictionary(e => e.QuestionId); +foreach (var questionId in orderedIds) +{ + map[questionId].Priority = minPriorityToSet++; +}Also applies to: 70-73
31-37: Pass cancellation tokens to repository calls.If supported, propagate cancellationToken into GetAllAsync/SaveChangesAsync for better request-abort behavior.
Also applies to: 75-77
38-47: Differentiate “page empty” from “IDs not found”.
questionsToReorder.Count == 0conflates “page not found/empty” with “IDs don’t belong to this page”. Consider a preliminary check to ensure the page exists and has FAQs, then a separate check for invalid IDs (you already havenotFoundIds). Improves UX and testability.VictoryCenter/VictoryCenter.BLL/Commands/Admin/Auth/Login/LoginCommandHandler.cs (2)
39-43: Leverage ValidationException to integrate with BaseHandler’s specific catch.You currently aggregate messages and throw Exception. Throw ValidationException (or use ValidateAndThrowAsync) so BaseHandler returns structured validation errors.
-var validationResult = await _validator.ValidateAsync(request, cancellationToken); -if (!validationResult.IsValid) -{ - throw new Exception(string.Join("; ", validationResult.Errors.Select(e => e.ErrorMessage))); -} +await _validator.ValidateAndThrowAsync(request, cancellationToken);
63-70: Cookie policy: confirm SameSite=Strict meets your client flows.Strict blocks cross-site top-level navigations in some flows (e.g., OAuth redirect chains). If your SPA/API runs on the same site it’s fine; otherwise consider Lax and document the rationale.
Do you support third-party redirects or cross-subdomain admin? If yes, reconsider SameSite.
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
12-31: Add logging and avoid leaking raw exception messages.
- Log exceptions here once, since you transform them to Result and swallow the stack.
- Consider mapping unexpected exceptions to a generic message to avoid leaking internals; let handlers craft user-safe messages (as they already do for DB ops).
Sketch:
-public async Task<Result<TResponse>> Handle(TRequest request, CancellationToken cancellationToken) +private readonly ILogger<BaseHandler<TRequest, TResponse>> _logger; +public BaseHandler(ILogger<BaseHandler<TRequest, TResponse>> logger) { _logger = logger; } +public async Task<Result<TResponse>> Handle(TRequest request, CancellationToken cancellationToken) { try { var response = await HandleRequest(request, cancellationToken); return Result.Ok(response); } catch (ValidationException ex) { + _logger.LogWarning(ex, "Validation failed for {RequestType}", typeof(TRequest).Name); return Result.Fail<TResponse>(ex.Errors.Select(e => e.ErrorMessage)); } catch (DbUpdateException ex) { + _logger.LogError(ex, "DB update failed for {RequestType}", typeof(TRequest).Name); - return Result.Fail<TResponse>(ex.Message); + return Result.Fail<TResponse>("Database operation failed"); } catch (OperationCanceledException) { + _logger.LogInformation("Operation canceled for {RequestType}", typeof(TRequest).Name); + return Result.Fail<TResponse>("Operation canceled"); } catch (Exception ex) { - return Result.Fail<TResponse>(ex.Message); + _logger.LogError(ex, "Unhandled error in {RequestType}", typeof(TRequest).Name); + return Result.Fail<TResponse>("Unexpected error"); } }VictoryCenter/VictoryCenter.BLL/Commands/Admin/Images/Create/CreateImageHandler.cs (3)
36-37: Use Guid "N" format instead of Replace.
Simpler and allocates less.- var fileName = Guid.NewGuid().ToString().Replace("-", ""); + var fileName = Guid.NewGuid().ToString("N");
53-57: Avoid nullable temp; return after completing the transaction.
Keeps nullability consistent with the non-null return type.- ImageDto? response = _mapper.Map<ImageDto>(createdImage); - - transaction.Complete(); - - return response; + ImageDto response = _mapper.Map<ImageDto>(createdImage); + transaction.Complete(); + return response;
59-62: Preserve original exception (type + stack).
Wrapping into System.Exception loses signal. Keep BlobStorageException and set inner.- throw new Exception(ErrorMessagesConstants.BlobStorageError(e.Message)); + throw new BlobStorageException(ErrorMessagesConstants.BlobStorageError(e.Message), e);VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Update/UpdateProgramHandler.cs (3)
41-43: Throw a more specific exception for NotFound.
Improves upstream handling and logging.- throw new Exception(ErrorMessagesConstants - .NotFound(request.Id, typeof(Program))); + throw new KeyNotFoundException( + ErrorMessagesConstants.NotFound(request.Id, typeof(Program)));
20-25: Remove unused dependency.
IBlobService isn’t used; trim ctor param (and DI registration) to reduce coupling.- public UpdateProgramHandler(IMapper mapper, IRepositoryWrapper repositoryWrapper, IValidator<UpdateProgramCommand> validator, IBlobService blobService) + public UpdateProgramHandler(IMapper mapper, IRepositoryWrapper repositoryWrapper, IValidator<UpdateProgramCommand> validator) { _mapper = mapper; _repositoryWrapper = repositoryWrapper; _validator = validator; }
79-81: Tiny nit: return directly.
Optional micro‑cleanup.- ProgramDto responseDto = _mapper.Map<ProgramDto>(programToUpdate); - return responseDto; + return _mapper.Map<ProgramDto>(programToUpdate);VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Delete/DeleteTeamMemberHandler.cs (1)
31-33: Prefer a specific NotFound exception.
Improves error routing and observability.- throw new Exception( - ErrorMessagesConstants.NotFound(request.Id, typeof(TeamMember))); + throw new KeyNotFoundException( + ErrorMessagesConstants.NotFound(request.Id, typeof(TeamMember)));VictoryCenter/VictoryCenter.BLL/Commands/Admin/Auth/Logout/LogoutCommandHandler.cs (1)
27-28: Use specific exception for unauthorized.
Clearer intent and easier policy mapping.- throw new Exception(AuthConstants.Unauthorized); + throw new UnauthorizedAccessException(AuthConstants.Unauthorized);VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.cs (2)
58-70: Avoid nullable temp; align with non‑nullable return type.- TeamMemberDto? result = _mapper.Map<TeamMemberDto>(entity); + TeamMemberDto result = _mapper.Map<TeamMemberDto>(entity);
46-50: Priority assignment can race under concurrency.
Two concurrent creates in the same category can pick the same next priority.Consider:
- DB unique index on (CategoryId, Priority) and retry on conflict, or
- Compute priority via a single SQL statement or stored proc that locks the range, or
- Use SERIALIZABLE isolation within the transaction if supported by your provider.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Create/CreateProgramHandler.cs (1)
20-25: Remove unused IBlobService from ctor (DI hygiene).
blobServiceis injected but never stored/used. Drop it to avoid confusing DI wiring.- public CreateProgramHandler(IMapper mapper, IRepositoryWrapper repositoryWrapper, IValidator<CreateProgramCommand> validator, IBlobService blobService) + public CreateProgramHandler(IMapper mapper, IRepositoryWrapper repositoryWrapper, IValidator<CreateProgramCommand> validator) { _mapper = mapper; _repositoryWrapper = repositoryWrapper; _validator = validator; }VictoryCenter/VictoryCenter.BLL/Commands/Admin/Auth/RefreshToken/RefreshTokenCommandHandler.cs (1)
80-83: Harden identity error handling.
First()can throw ifErrorsis empty. Usestring.Joinfor robustness and consistency with Login handler.- return result.Succeeded - ? new AuthResponseDto(accessToken) - : throw new Exception(result.Errors.First().Description); + return result.Succeeded + ? new AuthResponseDto(accessToken) + : throw new Exception(string.Join("; ", result.Errors.Select(e => e.Description)));VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Reorder/ReorderTeamMembersHandler.cs (1)
55-64: Optional: check SaveChanges results and batch updates.
- After the temp negative update and final assignment, consider verifying
SaveChangesAsync() > 0and throwing a DbUpdateException on failure for parity with other handlers.- Use
UpdateRange(allCategoryMembers)instead of per-entity updates.- foreach (var member in allCategoryMembers) - { - member.Priority = tempPriority--; - _repositoryWrapper.TeamMembersRepository.Update(member); - } - - await _repositoryWrapper.SaveChangesAsync(); + foreach (var member in allCategoryMembers) + { + member.Priority = tempPriority--; + } + _repositoryWrapper.TeamMembersRepository.UpdateRange(allCategoryMembers); + if (await _repositoryWrapper.SaveChangesAsync() <= 0) + { + throw new DbUpdateException(ErrorMessagesConstants.FailedToUpdateEntity(typeof(TeamMember))); + } ... - await _repositoryWrapper.SaveChangesAsync(); + if (await _repositoryWrapper.SaveChangesAsync() <= 0) + { + throw new DbUpdateException(ErrorMessagesConstants.FailedToUpdateEntity(typeof(TeamMember))); + }Also applies to: 79-83
VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Create/CreateFaqQuestionHandler.cs (2)
35-41: Pre-validate pages and dedupe PageIds.Avoid O(N*M) lookups and duplicate placements by de-duplicating and validating in bulk.
- foreach (var pageId in request.CreateFaqQuestionDto.PageIds) + var distinctPageIds = request.CreateFaqQuestionDto.PageIds.Distinct().ToList(); + var existingPageIds = allPages.Select(p => p.Id).ToHashSet(); + var missing = distinctPageIds.Where(id => !existingPageIds.Contains(id)).ToList(); + if (missing.Count > 0) + { + throw new Exception(ErrorMessagesConstants.ReorderingContainsInvalidIds(typeof(VisitorPage), missing)); + } + foreach (var pageId in distinctPageIds) { - if (!allPages.Any(p => p.Id == pageId)) - { - throw new Exception(ErrorMessagesConstants.NotFound(pageId, typeof(VisitorPage))); - } + // validated above
57-60: Consistent failure message helper.Elsewhere you sometimes use a dedicated “in database” message. If that helper exists (
FailedToCreateEntityInDatabase), consider aligning for consistency.- throw new DbUpdateException(ErrorMessagesConstants.FailedToCreateEntity(typeof(FaqQuestion))); + throw new DbUpdateException(ErrorMessagesConstants.FailedToCreateEntity(typeof(FaqQuestion))); + // or: ErrorMessagesConstants.FailedToCreateEntityInDatabase(typeof(FaqQuestion))VictoryCenter/VictoryCenter.BLL/Commands/Admin/Images/Update/UpdateImageHandler.cs (1)
46-49: Prefer specific exception types for NotFound.Throwing
Exceptionblurs intent and handling. Consider a dedicatedNotFoundException(orKeyNotFoundException) to keep BaseHandler mapping clear and enable targeted catch paths later.VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Delete/DeleteFaqQuestionHandler.cs (3)
49-56: Avoid int cast and Skip-by-index; filter by Priority instead.
Priorityislong. Casting tointrisks overflow andSkip((int)question.Priority)assumes 1..N contiguous indexing. Filter by value for clarity and safety.Apply:
- var question = placementsToDelete.Single(q => q.PageId == id); - var group = pageGroups - .Single(g => g.Key == id) - .OrderBy(q => q.Priority) - .Skip((int)question.Priority) - .ToList(); + var question = placementsToDelete.Single(q => q.PageId == id); + var group = pageGroups + .Single(g => g.Key == id) + .Where(q => q.Priority > question.Priority) + .OrderBy(q => q.Priority) + .ToList();
72-79: Check the final delete result explicitly.Aggregating into
affectedRowscan mask a failed final delete if earlier updates succeeded. Validate the delete SaveChanges independently.Apply:
- _repositoryWrapper.FaqQuestionsRepository.Delete(questionToDelete); - affectedRows += await _repositoryWrapper.SaveChangesAsync(); - - if (affectedRows <= 0) + _repositoryWrapper.FaqQuestionsRepository.Delete(questionToDelete); + var deletedRows = await _repositoryWrapper.SaveChangesAsync(); + + if (deletedRows <= 0) { throw new DbUpdateException(ErrorMessagesConstants.FailedToDeleteEntity(typeof(FaqQuestion))); }
58-70: Optional: persist the “flip back to positive” immediately.Today you rely on the subsequent SaveChanges (during delete) to persist the final positive priorities. Saving right after the flip mirrors the pattern used elsewhere and is clearer.
_repositoryWrapper.FaqPlacementsRepository.UpdateRange(group); affectedRows += await _repositoryWrapper.SaveChangesAsync(); foreach (var faq in group) { faq.Priority = -faq.Priority; } + affectedRows += await _repositoryWrapper.SaveChangesAsync();VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Delete/DeleteProgramHandler.cs (1)
33-36: Optional: wrap clear+delete in a transaction.Not required (single SaveChanges is atomic), but a
TransactionScopealigns with other handlers and future-proofs multi-step changes.VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Update/UpdateFaqQuestionHandler.cs (4)
82-90: Use Priority filter instead of int cast + Skip.Same reasoning as Delete handler: avoid
(int)cast and index-based skipping.- var question = questionPlacements.Single(q => q.PageId == id && q.QuestionId == request.Id); - var group = pageGroups - .Single(g => g.Key == id) - .OrderBy(q => q.Priority) - .Skip((int)question.Priority) - .ToList(); + var question = questionPlacements.Single(q => q.PageId == id && q.QuestionId == request.Id); + var group = pageGroups + .Single(g => g.Key == id) + .Where(q => q.Priority > question.Priority) + .OrderBy(q => q.Priority) + .ToList();Also applies to: 91-97, 99-106
108-116: Report which PageIds are invalid when adding.Replace per-id checks with a single diff and include the missing ids in the error. This improves observability and client UX.
- if (addedPageIds.Count > 0) - { - foreach (var addedId in addedPageIds) - { - if (!allPageIds.Contains(addedId)) - { - throw new Exception(FaqConstants.SomePagesNotFound); - } - } + if (addedPageIds.Count > 0) + { + var missingPageIds = addedPageIds.Except(allPageIds).ToList(); + if (missingPageIds.Count > 0) + { + throw new Exception(ErrorMessagesConstants.ReorderingContainsInvalidIds(typeof(VisitorPage), missingPageIds)); + }Also applies to: 118-124, 125-141
30-33: Pass cancellation tokens to repository calls (where supported).You pass
cancellationTokento validation; propagate it to repos to enable cooperative cancellation under request aborts.Also applies to: 54-63, 64-68, 144-147
149-157: Null-forgiving on result fetch.
resultEntity!can hide a genuine fetch issue. Consider guarding and throwing a targeted exception if null, or map from the tracked aggregate directly.VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Update/UpdateTeamMemberHandler.cs (3)
75-94: Consider idempotent updates (0 rows shouldn’t be an error).
SaveChangesAsync() > 0treats no-op updates as failures. Many APIs accept idempotent updates as success. Optionally return current state when 0 rows affected.- if (await _repositoryWrapper.SaveChangesAsync() > 0) + var rows = await _repositoryWrapper.SaveChangesAsync(); + if (rows >= 0) // accept no-op as success { if (entityToUpdate.ImageId != null) { Image? image = await _repositoryWrapper.ImageRepository.GetFirstOrDefaultAsync( new QueryOptions<Image> { Filter = i => i.Id == entityToUpdate.ImageId }); entityToUpdate.Image = image; } TeamMemberDto? resultDto = _mapper.Map<TeamMember, TeamMemberDto>(entityToUpdate); scope.Complete(); return resultDto; } - throw new DbUpdateException(ErrorMessagesConstants.FailedToUpdateEntity(typeof(TeamMember))); + throw new DbUpdateException(ErrorMessagesConstants.FailedToUpdateEntity(typeof(TeamMember)));
77-85: Optional: validate Image existence when ImageId provided.If
ImageIdis invalid, you silently return withImage = null. Consider throwing a NotFound to match your Category check.
34-43: Prefer specific exception types for NotFound.Use a domain
NotFoundException(orKeyNotFoundException) instead ofExceptionfor clarity and consistent handling.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Auth/Login/LoginCommandHandler.cs
Show resolved
Hide resolved
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Auth/Logout/LogoutCommandHandler.cs
Show resolved
Hide resolved
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Auth/RefreshToken/RefreshTokenCommandHandler.cs
Show resolved
Hide resolved
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Categories/Update/UpdateCategoryHandler.cs
Outdated
Show resolved
Hide resolved
...ryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Reorder/ReorderFaqQuestionsHandler.cs
Show resolved
Hide resolved
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Create/CreateProgramHandler.cs
Outdated
Show resolved
Hide resolved
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Create/CreateProgramHandler.cs
Outdated
Show resolved
Hide resolved
...oryCenter.BLL/Commands/Admin/HippotherapyPrograms/Delete/DeleteHippotherapyProgramHandler.cs
Show resolved
Hide resolved
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Update/UpdateProgramHandler.cs
Outdated
Show resolved
Hide resolved
VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Reorder/ReorderTeamMembersHandler.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Update/UpdateProgramHandler.cs (1)
54-63: Validate that the image exists before assignment.If
request.UpdateProgramDto.ImageIdis not null but the corresponding image doesn't exist in the database,newImagewill be null and will silently clear the program's image. This differs from the validation inCreateProgramHandler, which throws an exception when an image is not found.Apply this diff to add proper validation:
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 null) + { + throw new Exception(ErrorMessagesConstants.NotFound(request.UpdateProgramDto.ImageId.Value, typeof(Image))); + } programToUpdate.Image = newImage; }
♻️ Duplicate comments (1)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Reorder/ReorderTeamMembersHandler.cs (1)
44-47: Duplicate ID validation successfully implemented.Excellent! This check prevents duplicate IDs from causing issues downstream (particularly at line 57's
ToDictionarycall). The implementation matches the pattern used inReorderFaqQuestionsHandlerand addresses the previous review feedback.
🧹 Nitpick comments (6)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Update/UpdateProgramHandler.cs (2)
20-25: Remove unusedblobServiceparameter.The constructor accepts an
IBlobService blobServiceparameter that is neither stored nor used anywhere in the class. This appears to be leftover from a previous implementation.Apply this diff to remove the unused parameter:
- public UpdateProgramHandler(IMapper mapper, IRepositoryWrapper repositoryWrapper, IValidator<UpdateProgramCommand> validator, IBlobService blobService) + public UpdateProgramHandler(IMapper mapper, IRepositoryWrapper repositoryWrapper, IValidator<UpdateProgramCommand> validator) { _mapper = mapper; _repositoryWrapper = repositoryWrapper; _validator = validator; }
39-43: Consider using a more specific exception type.The code throws a generic
Exceptionfor the not-found scenario. While this works with the currentBaseHandlerimplementation, consider introducing and using a more specific exception type (e.g.,NotFoundException) to improve clarity and enable more granular exception handling in the future.VictoryCenter/VictoryCenter.BLL/Commands/Admin/Auth/RefreshToken/RefreshTokenCommandHandler.cs (1)
36-40: Consider storing HttpContext in a local variable.After the null check on lines 31-34, you could store HttpContext in a local variable to eliminate the need for the null-forgiving operator on line 36 and improve readability.
Apply this diff:
if (_httpContextAccessor.HttpContext is null) { throw new Exception(AuthConstants.Unauthorized); } + var httpContext = _httpContextAccessor.HttpContext; - var refreshTokenRetrieved = _httpContextAccessor.HttpContext!.Request.Cookies.TryGetValue(AuthConstants.RefreshTokenCookieName, out var refreshToken); + var refreshTokenRetrieved = httpContext.Request.Cookies.TryGetValue(AuthConstants.RefreshTokenCookieName, out var refreshToken); if (!refreshTokenRetrieved || string.IsNullOrWhiteSpace(refreshToken)) { throw new Exception(AuthConstants.RefreshTokenIsNotPresent); }Then update line 71 to use the local variable:
- _httpContextAccessor.HttpContext?.Response.Cookies.Append(AuthConstants.RefreshTokenCookieName, newRefreshToken, new CookieOptions + httpContext.Response.Cookies.Append(AuthConstants.RefreshTokenCookieName, newRefreshToken, new CookieOptionsVictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Reorder/ReorderFaqQuestionsHandler.cs (3)
30-38: Move duplicate OrderedIds validation into the FluentValidation validatorDuplicate checking belongs in the validator to keep handlers lean and consistent with project practice. Remove this block and add a validator rule.
[suggested changes]
- Handler: remove lines 30–38.
- Validator example:
RuleFor(x => x.ReorderFaqQuestionsDto.OrderedIds) .Must(ids => ids.Distinct().Count() == ids.Count) .WithMessage(ErrorMessagesConstants.CollectionMustContainUniqueValues(nameof(ReorderFaqQuestionsCommand.ReorderFaqQuestionsDto.OrderedIds)));Based on learnings
35-38: Prefer typed exceptions for user/input errors to leverage BaseHandler’s mappingThrow
ValidationExceptionfor duplicate IDs, invalid IDs, and non‑consecutive IDs; optionally a domainNotFoundExceptionfor missing/empty page. BaseHandler already treatsValidationExceptionspecially.[example]
throw new ValidationException(new[]{ new ValidationFailure("OrderedIds", ErrorMessagesConstants.CollectionMustContainUniqueValues(nameof(request.ReorderFaqQuestionsDto.OrderedIds)))});Also applies to: 49-50, 55-56, 63-65
67-85: Optional: Mirror the TeamMembers reorder algorithm for consistencyConsider reusing the pattern from ReorderTeamMembersHandler: temporary negatives, assign new priorities for the selected block, then proceed. Consistency eases maintenance and reduces edge‑case drift between features.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Auth/Login/LoginCommandHandler.cs(3 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Auth/RefreshToken/RefreshTokenCommandHandler.cs(4 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Reorder/ReorderFaqQuestionsHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/ProgramCategories/Create/CreateProgramCategoryHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Create/CreateProgramHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Update/UpdateProgramHandler.cs(4 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Reorder/ReorderTeamMembersHandler.cs(2 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Auth/LoginTests.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Auth/LogoutTests.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/Admin/TeamMembers/Reorder/ReorderTeamMembersHandler.csVictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Reorder/ReorderFaqQuestionsHandler.cs
📚 Learning: 2025-09-09T19:00:28.697Z
Learnt from: roman-stozhuk
PR: ita-social-projects/VictoryCenter-Back#349
File: VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Create/CreateFaqQuestionHandler.cs:37-46
Timestamp: 2025-09-09T19:00:28.697Z
Learning: In the VictoryCenter project, duplicate validation for collections like PageIds is handled in FluentValidation validators using .Must(collection => collection.Distinct().Count() == collection.Count) with appropriate error messages from ErrorMessagesConstants.
Applied to files:
VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Reorder/ReorderFaqQuestionsHandler.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.BLL/Commands/Admin/Auth/RefreshToken/RefreshTokenCommandHandler.cs
🧬 Code graph analysis (9)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Auth/LoginTests.cs (1)
VictoryCenter/VictoryCenter.BLL/Constants/AuthConstants.cs (1)
AuthConstants(3-16)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/ProgramCategories/Create/CreateProgramCategoryHandler.cs (4)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Categories/Create/CreateCategoryHandler.cs (1)
Task(28-44)VictoryCenter/VictoryCenter.DAL/Entities/ProgramCategory.cs (1)
ProgramCategory(3-9)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToCreateEntity(19-24)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Auth/LogoutTests.cs (1)
VictoryCenter/VictoryCenter.DAL/Entities/AdminUser.cs (1)
AdminUser(5-10)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Reorder/ReorderTeamMembersHandler.cs (4)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (3)
BaseHandler(8-34)Task(12-31)Task(33-33)VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Reorder/ReorderFaqQuestionsHandler.cs (1)
Task(24-88)VictoryCenter/VictoryCenter.BLL/Constants/TeamMemberConstants.cs (1)
TeamMemberConstants(3-7)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (3)
ErrorMessagesConstants(3-129)CollectionMustContainUniqueValues(110-113)ReorderingContainsInvalidIds(61-66)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Create/CreateProgramHandler.cs (7)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (3)
BaseHandler(8-34)Task(12-31)Task(33-33)VictoryCenter/VictoryCenter.BLL/Commands/Admin/ProgramCategories/Create/CreateProgramCategoryHandler.cs (1)
Task(25-40)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Update/UpdateProgramHandler.cs (1)
Task(27-81)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Images/Create/CreateImageHandler.cs (1)
Task(30-63)VictoryCenter/VictoryCenter.DAL/Entities/ProgramCategory.cs (1)
ProgramCategory(3-9)VictoryCenter/VictoryCenter.DAL/Entities/Program.cs (1)
Program(5-15)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (3)
ErrorMessagesConstants(3-129)ReorderingContainsInvalidIds(61-66)FailedToCreateEntity(19-24)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Auth/Login/LoginCommandHandler.cs (5)
VictoryCenter/VictoryCenter.BLL/Services/TokenService/TokenService.cs (2)
TokenService(17-106)TokenService(24-30)VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Auth/RefreshToken/RefreshTokenCommandHandler.cs (1)
Task(29-88)VictoryCenter/VictoryCenter.WebAPI/Extensions/ServicesConfiguration.cs (4)
Task(135-175)Task(177-182)Task(184-208)Task(210-245)VictoryCenter/VictoryCenter.BLL/Constants/AuthConstants.cs (1)
AuthConstants(3-16)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Update/UpdateProgramHandler.cs (3)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Create/CreateProgramHandler.cs (1)
Task(27-75)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToUpdateEntity(33-38)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Reorder/ReorderFaqQuestionsHandler.cs (7)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Reorder/ReorderTeamMembersHandler.cs (1)
Task(24-89)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (3)
ErrorMessagesConstants(3-129)CollectionMustContainUniqueValues(110-113)ReorderingContainsInvalidIds(61-66)VictoryCenter/VictoryCenter.DAL/Entities/FaqPlacement.cs (1)
FaqPlacement(3-14)VictoryCenter/VictoryCenter.BLL/Constants/FaqQuestionConstants.cs (1)
FaqConstants(3-8)VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryBase.cs (2)
UpdateRange(22-22)UpdateRange(24-24)VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryBase.cs (2)
UpdateRange(67-70)UpdateRange(72-75)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Auth/RefreshToken/RefreshTokenCommandHandler.cs (4)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Auth/Login/LoginCommandHandler.cs (1)
Task(37-80)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Auth/Logout/LogoutCommandHandler.cs (1)
Task(22-48)VictoryCenter/VictoryCenter.BLL/Constants/AuthConstants.cs (1)
AuthConstants(3-16)
🔇 Additional comments (33)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/ProgramCategories/Create/CreateProgramCategoryHandler.cs (4)
3-4: LGTM! Imports support the new BaseHandler pattern.The added imports are necessary for the centralized exception handling approach.
Microsoft.EntityFrameworkCoreprovidesDbUpdateException, andVictoryCenter.BLL.Commands.Baseprovides theBaseHandlerbase class.
12-12: LGTM! Class signature aligns with the centralized exception handling pattern.The migration from
IRequestHandlertoBaseHandler<CreateProgramCategoryCommand, ProgramCategoryDto>is consistent with the PR objectives and matches the pattern used in other handlers throughout the codebase.
25-25: LGTM! Method signature correctly implements the BaseHandler pattern.The method signature change from
HandletoHandleRequestwith a direct return ofProgramCategoryDtois correct. The baseHandlemethod inBaseHandlerwraps the response and catches exceptions, eliminating repetitive try/catch blocks in child handlers.
27-39: LGTM! Implementation follows the correct pattern and addresses previous feedback.The implementation correctly:
- Validates input before any side effects (line 27)
- Maps and persists the entity with appropriate timestamp (lines 29-31)
- Uses
<= 0boundary check forSaveChangesAsync(line 33), which correctly addresses the previous review feedback and treats both zero and negative return values as failures- Throws
DbUpdateExceptionwith an appropriate error message (line 35)- Returns the mapped DTO (lines 38-39)
The pattern is consistent with other handlers in the codebase, such as
CreateCategoryHandler.VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Update/UpdateProgramHandler.cs (2)
27-29: LGTM!The method signature correctly overrides
HandleRequestfromBaseHandler, and validation properly usesValidateAndThrowAsyncfor exception-based error handling.
74-77: SaveChanges check fixed, but consider the error message from past review.The condition has been correctly updated to
<= 0(addressing the past review comment). However, the past review also suggested usingFailedToUpdateEntityInDatabaseinstead ofFailedToUpdateEntityto be more specific about where the failure occurred.While
CreateProgramHandleruses the non-database-specific version (FailedToCreateEntity), consider whether the more specific message provides value for debugging. Both methods exist inErrorMessagesConstants.If you'd like to apply the past review suggestion:
- throw new DbUpdateException(ErrorMessagesConstants.FailedToUpdateEntity(typeof(Program))); + throw new DbUpdateException(ErrorMessagesConstants.FailedToUpdateEntityInDatabase(typeof(Program)));VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Reorder/ReorderTeamMembersHandler.cs (6)
3-3: Migration to BaseHandler looks solid.The handler successfully migrates to the new BaseHandler pattern, centralizing exception handling. The method signature change from
HandlereturningTask<Result<Unit>>toHandleRequestreturningTask<Unit>is consistent with the base class contract and aligns with other handlers in this PR.Also applies to: 11-11, 24-24
33-42: Category validation is implemented correctly.The logic properly validates that the category exists and contains team members before proceeding with reordering operations. The error message is clear and uses the appropriate constant.
49-54: ID existence validation is thorough.This check ensures all provided IDs are valid members of the category. The error message helpfully includes the list of invalid IDs for debugging.
56-82: Priority assignment logic is well-structured.The two-phase priority update (temporary negative values, then final positions) correctly avoids unique constraint violations. The logic splits members into reordered and unchanged groups, then assigns priorities sequentially.
However, I notice that
ReorderFaqQuestionsHandlerincludes a validation check to ensure the items being reordered have consecutive priorities:var prioritiesFound = questionsToReorder.Select(q => q.Priority).OrderBy(p => p).ToList(); for (var i = 1; i < prioritiesFound.Count; i++) { if (prioritiesFound[i] - prioritiesFound[i - 1] != 1) { throw new Exception(FaqConstants.IdsAreNonConsecutive); } }Is this check intentionally omitted for team members, or should it be included to match the FAQ handler's behavior? The current implementation allows reordering any subset of members regardless of their current priority positions.
84-88: Transaction completion is handled correctly.The transaction is properly completed after all changes are saved. Returning
defaultforUnitis the correct approach.
26-26: Validation approach follows established pattern.The early call to
ValidateAndThrowAsyncaligns with the codebase convention. TheBaseHandlerwill catchValidationExceptionand convert it to aResult.Failresponse appropriately.Based on learnings
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Create/CreateProgramHandler.cs (8)
1-4: LGTM!The import additions (Microsoft.EntityFrameworkCore and VictoryCenter.BLL.Commands.Base) align with the BaseHandler pattern migration and DbUpdateException usage.
14-14: LGTM!The class now derives from BaseHandler<CreateProgramCommand, ProgramDto>, consistent with the broader refactoring pattern across this PR that centralizes exception handling.
27-27: LGTM!The method signature correctly overrides HandleRequest and returns Task directly, eliminating the Result wrapper in favor of exception-based error signaling.
29-29: LGTM!Validation uses ValidateAndThrowAsync correctly. The BaseHandler catches ValidationException and converts it to a Result.Fail response.
31-36: Great fix for category validation!The handler now validates that all requested CategoryIds exist and fails fast with a clear error message listing the missing IDs. This addresses the past review concern about silently dropping missing categories.
Also applies to: 56-60
40-54: Great fix for image validation!The handler now validates that the provided ImageId exists and throws a clear exception if not found. This addresses the past review concern about silently assigning null for missing images.
67-70: Great fix for SaveChangesAsync check!The condition now correctly uses
<= 0to detect when no rows were written. This addresses the past review concern that< 0would never be true in EF Core.
62-65: LGTM!The entity creation, persistence, and DTO mapping flow is correct and consistent with the BaseHandler pattern.
Also applies to: 72-75
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Auth/LogoutTests.cs (1)
97-97: LGTM!The RefreshToken value change from "refresh_token" to "refreshToken" is a minor test data adjustment that doesn't affect test behavior or assertions.
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Auth/LoginTests.cs (1)
97-97: LGTM!The test now expects AuthConstants.IncorrectPassword when the admin doesn't exist, correctly validating the security enhancement that prevents user enumeration by returning the same error message for both "user not found" and "incorrect password" scenarios.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Auth/RefreshToken/RefreshTokenCommandHandler.cs (4)
5-5: LGTM!The class now derives from BaseHandler<RefreshTokenCommand, AuthResponseDto>, consistent with the centralized exception handling pattern.
Also applies to: 14-14
29-34: Great fix for HttpContext null check!The explicit null check for HttpContext before accessing it addresses the past review concern about using the null-forgiving operator. This properly handles the edge case where HttpContext might be null.
37-63: LGTM!The token validation, admin lookup, and refresh token expiry checks are correctly implemented with exception-based error handling, consistent with the BaseHandler pattern.
65-88: LGTM!The token creation, cookie management, and admin persistence flow is correct and includes proper security settings (HttpOnly, Secure, SameSite).
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Auth/Login/LoginCommandHandler.cs (6)
6-6: LGTM!The class now derives from BaseHandler<LoginCommand, AuthResponseDto> with appropriate imports, consistent with the centralized exception handling pattern.
Also applies to: 10-10, 15-15
37-43: LGTM!The method signature and validation logic correctly use exception-based error handling, which BaseHandler catches and converts to Result.Fail.
45-55: Great security enhancement!The handler now returns the same error message (AuthConstants.IncorrectPassword) for both "user not found" and "incorrect password" scenarios, preventing user enumeration. This addresses the past review concern about revealing email validity through distinct error messages.
57-70: LGTM!The token creation and cookie management flow is correct, including proper security settings (HttpOnly, Secure, SameSite, and appropriate expiry).
72-73: LGTM!The admin entity is correctly updated with the new refresh token and expiry timestamp.
75-80: LGTM!The persistence and return flow correctly handles update failures with exception-based error signaling.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Reorder/ReorderFaqQuestionsHandler.cs (1)
11-25: Good adoption of BaseHandler patternConstructor DI plus
HandleRequestoverride look consistent with the new base-handler flow. Validations are invoked upfront.
...ryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Reorder/ReorderFaqQuestionsHandler.cs
Outdated
Show resolved
Hide resolved
...ryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Reorder/ReorderFaqQuestionsHandler.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.cs (1)
20-20: Remove unused field.The
_reorderServicefield is declared but never initialized (the constructor no longer accepts it as a parameter starting at Line 25). This is dead code and should be removed.Apply this diff:
- private readonly IReorderService _reorderService;VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/DeleteTeamMemberTests.cs (2)
18-18: Remove unused mock field.The
_mockReorderServicefield is no longer used since theDeleteTeamMemberHandlerconstructor no longer acceptsIReorderService(see production handler at VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Delete/DeleteTeamMemberHandler.cs, lines 12-15).Apply this diff:
- private readonly Mock<IReorderService> _mockReorderService;Also remove the initialization at Line 35:
- _mockReorderService = new Mock<IReorderService>();
105-109: Remove unused helper method.The
SetupReorderServicemethod is no longer used by any tests since the handler no longer depends onIReorderService.Apply this diff:
- private void SetupReorderService() - { - _mockReorderService.Setup(r => r.RenumberPriorityAsync<TeamMember>(It.IsAny<Expression<Func<TeamMember, bool>>>())) - .Returns(Task.CompletedTask); - }
🧹 Nitpick comments (5)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.cs (1)
8-8: Remove unused import.With the removal of
IReorderServicedependency, the importusing VictoryCenter.BLL.Interfaces.ReorderService;at Line 8 is no longer needed.Apply this diff:
-using VictoryCenter.BLL.Interfaces.ReorderService;VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/DeleteTeamMemberTests.cs (1)
7-7: Remove unused import.With the removal of
IReorderServicedependency from tests, the importusing VictoryCenter.BLL.Interfaces.ReorderService;at Line 7 is no longer needed.Apply this diff:
-using VictoryCenter.BLL.Interfaces.ReorderService;VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Update/UpdateTeamMemberHandler.cs (1)
71-71: Minor: rowsAffected initialization could be simplified.The
rowsAffectedvariable is initialized to 0 at Line 71 but is immediately reassigned at Line 82. Consider initializing it directly with the firstSaveChangesAsyncresult for clarity.Apply this diff:
- var rowsAffected = 0; - _mapper.Map(request.UpdateTeamMemberDto, entityToUpdate); if (categoryChanged) { entityToUpdate.Priority = await _indexReorderService.GetNextDisplayOrderAsync<TeamMember>( groupSelector: tm => tm.CategoryId == newCategoryId); } _repositoryWrapper.TeamMembersRepository.Update(entityToUpdate); - rowsAffected += await _repositoryWrapper.SaveChangesAsync(); + var rowsAffected = await _repositoryWrapper.SaveChangesAsync();VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Create/CreateFaqQuestionHandler.cs (1)
40-45: Use a more specific exception type for not-found scenarios.Throwing a generic
Exceptionreduces the ability to handle specific error cases. Consider using a domain-specific exception (e.g.,EntityNotFoundException) or at minimumInvalidOperationException.Apply this pattern for better exception handling:
- throw new Exception(ErrorMessagesConstants.NotFound(pageId, typeof(VisitorPage))); + throw new InvalidOperationException(ErrorMessagesConstants.NotFound(pageId, typeof(VisitorPage)));Note: If a custom
EntityNotFoundExceptionexists in the codebase, prefer that instead.VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Delete/DeleteFaqQuestionHandler.cs (1)
21-31: Use a more specific exception type for not-found scenarios.Similar to the Create handler, throwing a generic
Exceptionlimits error handling capabilities. Consider using a domain-specific exception orInvalidOperationException.Apply this pattern:
- throw new Exception(ErrorMessagesConstants.NotFound(request.Id, typeof(FaqQuestion))); + throw new InvalidOperationException(ErrorMessagesConstants.NotFound(request.Id, typeof(FaqQuestion)));Note: If a custom
EntityNotFoundExceptionexists in the codebase, prefer that for consistency across handlers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Create/CreateFaqQuestionHandler.cs(3 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Delete/DeleteFaqQuestionHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.cs(4 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Update/UpdateTeamMemberHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Validators/Images/CreateImageValidator.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Validators/TeamMembers/BaseTeamMembersValidator.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Validators/TeamMembers/ReorderTeamMembersValidator.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/FaqQuestions/CreateFaqQuestionTests.cs(2 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/CreateTeamMemberTests.cs(8 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/DeleteTeamMemberTests.cs(4 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/UpdateTeamMemberTests.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/TeamMembers/BaseTeamMemberValidatorTests.cs(1 hunks)VictoryCenter/VictoryCenter.WebAPI/Extensions/ServicesConfiguration.cs(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/TeamMembers/BaseTeamMemberValidatorTests.cs
- VictoryCenter/VictoryCenter.BLL/Validators/Images/CreateImageValidator.cs
- VictoryCenter/VictoryCenter.BLL/Validators/TeamMembers/BaseTeamMembersValidator.cs
- VictoryCenter/VictoryCenter.WebAPI/Extensions/ServicesConfiguration.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- VictoryCenter/VictoryCenter.BLL/Validators/TeamMembers/ReorderTeamMembersValidator.cs
- VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/FaqQuestions/CreateFaqQuestionTests.cs
🧰 Additional context used
🧬 Code graph analysis (7)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Delete/DeleteFaqQuestionHandler.cs (9)
VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryBase.cs (5)
Delete(26-26)DeleteRange(28-28)DeleteRange(30-30)UpdateRange(22-22)UpdateRange(24-24)VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryBase.cs (5)
Delete(77-80)DeleteRange(82-85)DeleteRange(87-90)UpdateRange(67-70)UpdateRange(72-75)VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Update/UpdateFaqQuestionHandler.cs (1)
Task(30-158)VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Reorder/ReorderFaqQuestionsHandler.cs (1)
Task(24-88)VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryWrapper.cs (1)
Task(34-34)VictoryCenter/VictoryCenter.DAL/Entities/FaqQuestion.cs (1)
FaqQuestion(5-18)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (4)
ErrorMessagesConstants(3-129)NotFound(7-10)NotFound(12-17)FailedToDeleteEntity(47-52)VictoryCenter/VictoryCenter.DAL/Entities/FaqPlacement.cs (1)
FaqPlacement(5-16)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/UpdateTeamMemberTests.cs (2)
VictoryCenter/VictoryCenter.BLL/Exceptions/ReorderExceptions/ReorderException.cs (3)
ReorderException(3-14)ReorderException(5-8)ReorderException(10-13)VictoryCenter/VictoryCenter.BLL/Constants/ReorderConstants.cs (2)
ReorderConstants(3-23)ErrorWithReordering(9-12)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Create/CreateFaqQuestionHandler.cs (8)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.cs (1)
Task(32-78)VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryWrapper.cs (2)
Task(34-34)TransactionScope(36-36)VictoryCenter/VictoryCenter.DAL/Entities/FaqQuestion.cs (1)
FaqQuestion(5-18)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (4)
ErrorMessagesConstants(3-129)NotFound(7-10)NotFound(12-17)FailedToCreateEntity(19-24)VictoryCenter/VictoryCenter.DAL/Entities/VisitorPage.cs (1)
VisitorPage(3-14)VictoryCenter/VictoryCenter.DAL/Entities/FaqPlacement.cs (1)
FaqPlacement(5-16)VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryWrapper.cs (1)
TransactionScope(75-78)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.cs (6)
VictoryCenter/VictoryCenter.BLL/Services/ReorderService/ReorderService.cs (2)
ReorderService(14-162)ReorderService(18-21)VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (3)
BaseHandler(8-34)Task(12-31)Task(33-33)VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryWrapper.cs (1)
Task(34-34)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (4)
ErrorMessagesConstants(3-129)NotFound(7-10)NotFound(12-17)FailedToCreateEntityInDatabase(26-31)VictoryCenter/VictoryCenter.DAL/Entities/Category.cs (1)
Category(3-14)VictoryCenter/VictoryCenter.DAL/Entities/TeamMember.cs (1)
TeamMember(6-29)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Update/UpdateTeamMemberHandler.cs (6)
VictoryCenter/VictoryCenter.BLL/Services/ReorderService/ReorderService.cs (2)
ReorderService(14-162)ReorderService(18-21)VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (3)
BaseHandler(8-34)Task(12-31)Task(33-33)VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.cs (1)
Task(32-78)VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Delete/DeleteTeamMemberHandler.cs (1)
Task(18-46)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToUpdateEntity(33-38)VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryWrapper.cs (1)
TransactionScope(36-36)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/DeleteTeamMemberTests.cs (4)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Delete/DeleteTeamMemberHandler.cs (2)
DeleteTeamMemberHandler(9-47)DeleteTeamMemberHandler(13-16)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (3)
ErrorMessagesConstants(3-129)FailedToDeleteEntityInDatabase(54-59)FailedToDeleteEntity(47-52)VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryWrapper.cs (1)
TransactionScope(36-36)VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryWrapper.cs (1)
TransactionScope(75-78)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/CreateTeamMemberTests.cs (2)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.cs (2)
CreateTeamMemberHandler(15-79)CreateTeamMemberHandler(22-30)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToCreateEntityInDatabase(26-31)
⏰ 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 (16)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/UpdateTeamMemberTests.cs (1)
268-268: Test correctly validates standardized error message format.The change to wrap the exception message with
ReorderConstants.ErrorWithReordering()ensures consistency between the thrown exception and the expected error message in the assertion (line 284). This validates that the BaseHandler correctly converts exceptions to Result errors while preserving the standardized message format.VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.cs (2)
52-55: Inline priority calculation replaces ReorderService usage.The handler now calculates the next priority inline instead of using
IReorderService.GetNextDisplayOrderAsync. This duplicates the logic fromReorderService.GetNextDisplayOrderAsync(which performs the sameMaxAsync+ 1 calculation). While this approach is acceptable given the broader refactor to remove the dependency, be aware that this pattern is repeated across multiple handlers.Consider documenting this decision in an ADR if not already captured, as it affects maintainability when priority calculation logic needs to evolve.
44-44: Exception handling migration looks correct.The handler properly throws exceptions for error conditions (NotFound at Line 44, DbUpdateException at Line 61) instead of returning
Result.Fail. This aligns with the new BaseHandler pattern where exceptions are centrally caught and converted to Result objects.Also applies to: 61-61
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/DeleteTeamMemberTests.cs (1)
79-79: Error message updates align with centralized constants.The test assertions now use
ErrorMessagesConstants.FailedToDeleteEntityInDatabase(Line 79) andErrorMessagesConstants.FailedToDeleteEntity(Line 90), which is consistent with the migration to centralized error handling.Also applies to: 90-90
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/CreateTeamMemberTests.cs (3)
126-127: Proper test setup for inline priority calculation.The tests now mock
MaxAsyncon theTeamMembersRepositoryto support the handler's inline priority calculation (which replaced theIReorderService.GetNextDisplayOrderAsynccall). This is the correct approach to ensure tests remain isolated and deterministic.Also applies to: 172-173
86-86: Error message assertions updated correctly.The test assertions now use
ErrorMessagesConstants.FailedToCreateEntityInDatabase(typeof(TeamMember))consistently across all failure scenarios (Lines 86, 125, 144), aligning with the handler's new exception-based error signaling.Also applies to: 125-125, 144-144
73-73: Handler instantiation reflects simplified constructor.All test instantiations of
CreateTeamMemberHandlerhave been correctly updated to exclude theIReorderServiceparameter, matching the production handler's new constructor signature.Also applies to: 89-89, 108-108, 137-137
VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Update/UpdateTeamMemberHandler.cs (3)
15-15: Appropriate retention of IReorderService for updates.Unlike the Create and Delete handlers,
UpdateTeamMemberHandlercorrectly retains theIReorderServicedependency (Lines 20, 31) because it's needed when a TeamMember's category changes (Lines 77-78, 86-87). The service manages priority reordering in both the old and new categories.Also applies to: 20-20, 31-31
34-48: Exception-based error handling migration.The
HandleRequestmethod now throws exceptions for validation failures (Line 36 viaValidateAndThrowAsync) and not-found scenarios (Line 47), replacing the previous Result-wrapping pattern. This aligns with the BaseHandler's centralized exception-to-Result mapping.
69-112: Transaction scope and error handling look correct.The transaction scope usage is appropriate:
- Begins at Line 69
- Performs update and saves changes (Lines 73-82)
- Conditionally reorders priorities if category changed (Lines 84-90)
- Completes transaction on success (Line 106)
- Throws
DbUpdateExceptionif no rows affected (Line 111)The exception will be caught by
BaseHandlerand converted to aResult.Fail.VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Create/CreateFaqQuestionHandler.cs (3)
1-14: LGTM! BaseHandler pattern correctly adopted.The class now properly inherits from
BaseHandler<CreateFaqQuestionCommand, FaqQuestionDto>with appropriate imports for the transaction support and base handler infrastructure.
35-35: LGTM! Validation correctly delegates to base handler.Using
ValidateAndThrowAsyncproperly throwsValidationExceptionon validation failure, which theBaseHandlercatches and converts to a failedResult.
57-57: No changes needed—the code is correct for the current entity type.The
FaqQuestion.CreatedAtproperty is defined asDateTime, notDateTimeOffset. UsingDateTime.UtcNowis the correct approach here. The suggestion to switch toDateTimeOffset.UtcNowwould create a type mismatch and cause a compilation error.If consistency with
DateTimeOffsetis desired across handlers, it would require refactoring the entity property definition itself—a broader change beyond this handler. However, the current implementation is type-safe and appropriate for the entity schema.Likely an incorrect or invalid review comment.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Delete/DeleteFaqQuestionHandler.cs (3)
9-9: LGTM! BaseHandler pattern correctly adopted.The class properly inherits from
BaseHandler<DeleteFaqQuestionCommand, long>, returning the ID of the deleted entity.
49-70: Priority reordering logic appears sound.The use of negative priorities (lines 58-60, 66-69) as a temporary mechanism to avoid unique constraint violations is consistent with
ReorderFaqQuestionsHandler.cs. TheSkip((int)question.Priority)operation correctly skips items that don't need reordering after the deletion creates a gap in priorities.
75-78: Verify appropriateness ofaffectedRows <= 0check.The check accumulates affected rows across placement deletions, priority updates, and the question deletion. While this seems reasonable, consider edge cases:
- If the question has no placements,
affectedRowsshould still be > 0 from the question deletion itself (line 73)- If
affectedRows <= 0, it suggests a transaction consistency issue or the entity wasn't properly trackedThe logic appears sound, but ensure this aligns with expected EF Core behavior for delete operations.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Create/CreateFaqQuestionHandler.cs
Show resolved
Hide resolved
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/DeleteTeamMemberTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/FaqQuestions/UpdateFaqQuestionTests.cs (1)
190-203: Add explicit exception handling to align with established codebase patterns.UpdateFaqQuestionHandler throws
DbUpdateExceptionwithFailedToUpdateEntity, but unlike similar handlers (e.g.,UpdateSupportOptionsHandler,UpdateCorrespondentBankDetailsHandler), it lacks a try-catch block to explicitly handleDbUpdateException. Other handlers in the codebase catch this exception locally and return a custom error message (FailedToUpdateEntityInDatabase), rather than relying onBaseHandlerto pass through the exception message. Add a try-catch block matching this established pattern for consistency and clearer error semantics:
- Scenario 1 (silent failure): SaveChangesAsync returns -1 → throw with
FailedToUpdateEntity- Scenario 2 (database exception): Catch
DbUpdateException→ return withFailedToUpdateEntityInDatabaseThis makes error handling explicit and ensures the custom message is always used in production, not just in mocked tests.
♻️ Duplicate comments (1)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Reorder/ReorderFaqQuestionsHandler.cs (1)
35-45: Ambiguous error conflates "page not found" with "invalid IDs".Counting placements filtered by both
pageIdandorderedIdsproduces a zero count for three distinct scenarios:
- The page doesn't exist
- The page exists but contains no questions
- The page exists with questions, but none of the provided
orderedIdsare validThe generic error message
PageNotFoundOrContainsNoFaqQuestionsdoesn't help callers distinguish these cases. Following the pattern inReorderTeamMembersHandler, load all placements for the page first, validate that everyorderedIdexists in that set, and throw a specificReorderingContainsInvalidIdserror if any are missing.Apply this diff to clarify error handling:
- var questionsToReorderCount = await _repositoryWrapper.FaqPlacementsRepository.CountAsync( - new QueryOptions<FaqPlacement> + var allPagePlacements = (await _repositoryWrapper.FaqPlacementsRepository.GetAllAsync( + new QueryOptions<FaqPlacement> { - Filter = e => e.PageId == pageId && orderedIds.Contains(e.QuestionId), - OrderByASC = e => e.Priority + Filter = e => e.PageId == pageId }); - if (questionsToReorderCount == 0) + if (!allPagePlacements.Any()) { throw new Exception(FaqConstants.PageNotFoundOrContainsNoFaqQuestions); } + + var notFoundIds = orderedIds.Except(allPagePlacements.Select(f => f.QuestionId)).ToList(); + if (notFoundIds.Any()) + { + throw new Exception(ErrorMessagesConstants.ReorderingContainsInvalidIds(typeof(FaqQuestion), notFoundIds)); + }
🧹 Nitpick comments (8)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Delete/DeleteFaqQuestionHandler.cs (1)
31-34: Consider using a more specific exception type for not-found scenarios.The generic
Exceptionworks functionally but lacks semantic clarity. If aNotFoundExceptionor similar domain-specific exception type is available, consider using it here for better error categorization.Example:
-throw new Exception(ErrorMessagesConstants.NotFound(request.Id, typeof(FaqQuestion))); +throw new NotFoundException(ErrorMessagesConstants.NotFound(request.Id, typeof(FaqQuestion)));VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Update/UpdateFaqQuestionHandler.cs (4)
45-48: Consider using a specific exception type for not-found scenarios.Throwing a generic
Exceptionmakes it difficult for callers or middleware to handle different error cases specifically. Consider introducing aNotFoundExceptionor similar domain-specific exception.Example:
if (entityToUpdate is null) { - throw new Exception(ErrorMessagesConstants.NotFound(request.Id, typeof(FaqQuestion))); + throw new NotFoundException(ErrorMessagesConstants.NotFound(request.Id, typeof(FaqQuestion))); }Note: This pattern is also used in other handlers (CreateFaqQuestionHandler, DeleteFaqQuestionHandler) and should be addressed consistently across the codebase.
59-78: Placement removal logic is correct; consider efficiency improvement.The logic correctly handles removed placements and priority renumbering. However, fetching all pages (line 60) could be inefficient if there are many pages in the system.
Consider fetching only the necessary page IDs:
-var allPageIds = (await _repositoryWrapper.VisitorPagesRepository.GetAllAsync()).Select(p => p.Id).ToList(); +// Fetch only when needed for validation in the addedPageIds blockAlternatively, if you need to validate page existence, consider a more targeted query that checks only the specific page IDs being added.
82-87: Use a specific exception type for validation failures.Similar to the not-found check earlier, throwing a generic
Exceptionfor validation failures reduces code clarity and makes error handling less precise.Consider using a domain-specific exception:
if (missingPageIds.Any()) { - throw new Exception(FaqConstants.SomePagesNotFound); + throw new ValidationException(FaqConstants.SomePagesNotFound); }Note: Ensure consistency with FluentValidation exceptions if that's the validation framework in use.
107-121: Refetching the entity may be unnecessary.After updating
entityToUpdateand its placements within the transaction, refetching the entity (lines 109-114) adds an extra database round-trip. Since all changes are already tracked inentityToUpdate, you could map it directly.Consider returning the already-updated entity:
if (affectedRows > 0) { - FaqQuestion? resultEntity = await _repositoryWrapper.FaqQuestionsRepository.GetFirstOrDefaultAsync( - new QueryOptions<FaqQuestion> - { - Filter = entity => entity.Id == request.Id, - Include = e => e.Include(q => q.Placements), - }); - scope.Complete(); - FaqQuestionDto? resultDto = _mapper.Map<FaqQuestion, FaqQuestionDto>(resultEntity!); + FaqQuestionDto? resultDto = _mapper.Map<FaqQuestion, FaqQuestionDto>(entityToUpdate); return resultDto; }However, if you need to ensure the entity state is refreshed from the database, the current approach is valid.
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/FaqQuestions/UpdateFaqQuestionTests.cs (2)
162-163: Consider removing the development comment.The Ukrainian comment explains a change but will become outdated context noise post-merge. Consider removing it or replacing with a more permanent explanation if needed.
- // ЗМІНЕНО: Додано reorderService в конструктор var handler = new UpdateFaqQuestionHandler(_mockMapper.Object, _mockRepositoryWrapper.Object, _validator, _mockReorderService.Object);
308-313: Consider removing the development comment.Similar to line 162, this Ukrainian comment explains a development change but adds unnecessary noise post-merge.
- // ДОДАНО: Setup для DeleteRange та CreateRangeAsync _mockRepositoryWrapper.Setup(x => x.FaqPlacementsRepository.DeleteRange(It.IsAny<IEnumerable<FaqPlacement>>()));VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Reorder/ReorderFaqQuestionsHandler.cs (1)
55-55: Prefer explicitUnit.Valueoverdefaultfor clarity.Returning
defaultforUnitis technically correct but less idiomatic. Usereturn Unit.Value;to match conventions in other handlers.Apply this diff:
- return default; + return Unit.Value;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Delete/DeleteFaqQuestionHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Reorder/ReorderFaqQuestionsHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Update/UpdateFaqQuestionHandler.cs(3 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Reorder/ReorderTeamMembersHandler.cs(2 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/FaqQuestions/DeleteFaqQuestionTests.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/FaqQuestions/ReorderFaqQuestionsTests.cs(2 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/FaqQuestions/UpdateFaqQuestionTests.cs(2 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/ReorderTeamMembersTests.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Reorder/ReorderTeamMembersHandler.cs
- VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/FaqQuestions/ReorderFaqQuestionsTests.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-09T19:00:28.697Z
Learnt from: roman-stozhuk
PR: ita-social-projects/VictoryCenter-Back#349
File: VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Create/CreateFaqQuestionHandler.cs:37-46
Timestamp: 2025-09-09T19:00:28.697Z
Learning: In the VictoryCenter project, duplicate validation for collections like PageIds is handled in FluentValidation validators using .Must(collection => collection.Distinct().Count() == collection.Count) with appropriate error messages from ErrorMessagesConstants.
Applied to files:
VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Reorder/ReorderFaqQuestionsHandler.cs
📚 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/Admin/FaqQuestions/Reorder/ReorderFaqQuestionsHandler.cs
🧬 Code graph analysis (4)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Update/UpdateFaqQuestionHandler.cs (5)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Delete/DeleteFaqQuestionHandler.cs (1)
Task(22-63)VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Create/CreateFaqQuestionHandler.cs (1)
Task(33-69)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (4)
ErrorMessagesConstants(3-129)NotFound(7-10)NotFound(12-17)FailedToUpdateEntity(33-38)VictoryCenter/VictoryCenter.BLL/Constants/FaqQuestionConstants.cs (1)
FaqConstants(3-8)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/FaqQuestions/DeleteFaqQuestionTests.cs (2)
VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToDeleteEntityInDatabase(54-59)VictoryCenter/VictoryCenter.DAL/Entities/FaqQuestion.cs (1)
FaqQuestion(5-18)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/FaqQuestions/UpdateFaqQuestionTests.cs (2)
VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToUpdateEntityInDatabase(40-45)VictoryCenter/VictoryCenter.DAL/Entities/FaqQuestion.cs (1)
FaqQuestion(5-18)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/ReorderTeamMembersTests.cs (2)
VictoryCenter/VictoryCenter.BLL/Exceptions/ReorderExceptions/ReorderException.cs (3)
ReorderException(3-14)ReorderException(5-8)ReorderException(10-13)VictoryCenter/VictoryCenter.BLL/Constants/ReorderConstants.cs (2)
ReorderConstants(3-23)ErrorWithReordering(9-12)
⏰ 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.UnitTests/MediatRHandlersTests/FaqQuestions/DeleteFaqQuestionTests.cs (1)
102-102: LGTM - Test mock now matches real-world exception behavior.The addition of the specific error message to the
DbUpdateExceptionensures the mock accurately simulates a database failure scenario and aligns with the assertion at line 115.VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Delete/DeleteFaqQuestionHandler.cs (4)
10-10: LGTM - Handler correctly refactored to use BaseHandler pattern.The inheritance change centralizes exception handling as intended by issue #620, eliminating repetitive try/catch blocks.
22-22: LGTM - Method signature correctly updated.The
HandleRequestoverride now returnslongdirectly, delegating Result wrapping and exception handling toBaseHandler.
62-62: Error message correctly differentiates logical failure from database exceptions.Using
FailedToDeleteEntityhere (whenaffectedRows == 0) appropriately distinguishes logical failures from database-level exceptions (which would useFailedToDeleteEntityInDatabaseand be thrown bySaveChangesAsyncitself). This dual-message approach provides clear failure context.
56-62: Validate each SaveChangesAsync operation individually to prevent partial success scenarios.The codebase shows this accumulation pattern (
affectedRows +=) is unique to the FaqQuestions handlers (Delete and Update). While the final checkif (affectedRows > 0)confirms at least one operation succeeded, it cannot distinguish between:
- An intermediate save returning 0 because nothing was actually modified (legitimate edge case)
- An intermediate save returning 0 due to a silent failure
Consider validating each
SaveChangesAsyncresult separately. For instance, if placement deletion is critical, verify its result before proceeding with renumbering. The transaction scope ensures atomicity, but intermediate validation would make intent explicit and prevent disguised partial failures.// Current pattern allows ambiguity affectedRows += await _repositoryWrapper.SaveChangesAsync(); // Could be 0 legitimately // Consider intermediate validation if each operation is critical var deletionRows = await _repositoryWrapper.SaveChangesAsync(); if (deletionRows == 0 && placementsToDelete.Any()) { throw new DbUpdateException("Failed to delete placements"); } affectedRows += deletionRows;VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Update/UpdateFaqQuestionHandler.cs (3)
1-1: LGTM: Necessary imports for the refactoring.The new imports support the BaseHandler pattern and transaction management, aligning with the PR objectives.
Also applies to: 5-5
15-15: LGTM: Proper inheritance from BaseHandler.The handler correctly adopts the new BaseHandler pattern, which centralizes exception handling as intended by this PR.
50-58: LGTM: Proper transaction management and update flow.The handler correctly initiates a transaction scope and tracks affected rows for validation.
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/FaqQuestions/UpdateFaqQuestionTests.cs (2)
1-1: LGTM: BOM/whitespace cleanup.The change to this line aligns with the PR's removal of BOM/hidden-character artifacts in using directives.
224-225: Test correctly verifies exception-to-Result mapping.The mock setup properly tests the handler's behavior when
DbUpdateExceptionis thrown. The test verifies that the exception is caught and converted to a failed Result with the expected error message, aligning with the centralized exception handling pattern introduced in this PR.VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/ReorderTeamMembersTests.cs (1)
165-176: Test correctly validates the standardized exception message format.The update to throw
ReorderExceptionwithReorderConstants.ErrorWithReordering(reorderExceptionMessage)properly tests the new exception-based flow. The assertion on line 176 correctly expects the wrapped message format. This aligns well with the BaseHandler migration.VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Reorder/ReorderFaqQuestionsHandler.cs (2)
30-30: Duplicate ID validation is already correctly implemented.The
ReorderFaqQuestionsValidatorvalidatesOrderedIdsfor uniqueness at line 24 using.Must(ids => ids.Distinct().Count() == ids.Count)with an appropriate error message fromErrorMessagesConstants. No action required.
49-54: Inconsistency confirmed: align SaveChangesAsync patterns between handlers.The review comment accurately identifies a real pattern divergence. SwapElementsAsync internally calls SaveChangesAsync twice—once after assigning temporary priorities (line 98) and again after final priorities (line 110). However,
ReorderTeamMembersHandleradds an explicit third call afterSwapElementsAsynccompletes (line 40), whereasReorderFaqQuestionsHandlerdoes not.Within a transaction context, this difference is architecturally meaningful. Determine whether:
- The extra
SaveChangesAsyncinReorderTeamMembersHandlerserves a specific purpose or is redundant- Both handlers should follow the same pattern for consistency and clarity
Align the implementations once the intent is confirmed.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Update/UpdateFaqQuestionHandler.cs
Show resolved
Hide resolved
VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Update/UpdateFaqQuestionHandler.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (27)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Delete/DeleteForeignBankDetailsHandler.cs (1)
26-30: Use a typed NotFoundException instead of generic Exception.Typed exceptions preserve semantics (e.g., 404 mapping) and make BaseHandler discrimination possible later.
Apply:
- throw new Exception(ErrorMessagesConstants - .NotFound(request.Id, typeof(Entities.ForeignBankDetails))); + throw new NotFoundException(ErrorMessagesConstants + .NotFound(request.Id, typeof(Entities.ForeignBankDetails)));Add once (new file):
// VictoryCenter.BLL/Exceptions/NotFoundException.cs namespace VictoryCenter.BLL.Exceptions { public sealed class NotFoundException : Exception { public NotFoundException(string message) : base(message) { } } }And extend BaseHandler to catch NotFoundException distinctly if desired.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Delete/DeleteCorrespondentBankDetailsHandler.cs (1)
26-30: Prefer NotFoundException for not-found cases.Keeps error semantics explicit and future-proofs BaseHandler mapping.
- throw new Exception(ErrorMessagesConstants - .NotFound(request.Id, typeof(Entities.CorrespondentBankDetails))); + throw new NotFoundException(ErrorMessagesConstants + .NotFound(request.Id, typeof(Entities.CorrespondentBankDetails)));VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Update/UpdateForeignBankDetailsHandler.cs (2)
35-39: Use NotFoundException to keep not-found distinct.Aligns with other handlers and improves error handling consistency.
- throw new Exception(ErrorMessagesConstants - .NotFound(request.Id, typeof(Entities.ForeignBankDetails))); + throw new NotFoundException(ErrorMessagesConstants + .NotFound(request.Id, typeof(Entities.ForeignBankDetails)));
45-52: Propagate CancellationToken to SaveChangesAsync (if supported).Improves resiliency on timeouts/shutdowns.
- if (await _repositoryWrapper.SaveChangesAsync() > 0) + if (await _repositoryWrapper.SaveChangesAsync(cancellationToken) > 0) return responseDto;VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Create/CreateCorrespondentBankDetailsHandler.cs (2)
35-39: Throw NotFoundException when FK parent is missing.Explicit type helps map to 404 and improves diagnostics.
- throw new Exception(ErrorMessagesConstants - .NotFound(request.CreateCorrespondentBankDetailsDto.ForeignBankDetailsId, typeof(Entities.ForeignBankDetails))); + throw new NotFoundException(ErrorMessagesConstants + .NotFound(request.CreateCorrespondentBankDetailsDto.ForeignBankDetailsId, typeof(Entities.ForeignBankDetails)));
44-50: Pass CancellationToken to SaveChangesAsync (if available).Keeps behavior consistent with other handlers.
- if (await _repositoryWrapper.SaveChangesAsync() > 0) + if (await _repositoryWrapper.SaveChangesAsync(cancellationToken) > 0) return responseDto;VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Create/CreateForeignBankDetailsHandler.cs (2)
4-4: Centralization via BaseHandler is good; consider hardening BaseHandler’s catch-all.Right now, BaseHandler returns ex.Message for unknown exceptions. Prefer logging the exception and returning a generic message to avoid leaking internals; add a dedicated catch for NotFoundException.
Also applies to: 11-12
31-37: Use CancellationToken in SaveChangesAsync (if supported).Standardizes cooperative cancellation across commands.
- if (await _repositoryWrapper.SaveChangesAsync() > 0) + if (await _repositoryWrapper.SaveChangesAsync(cancellationToken) > 0) return responseDto;Also consider a small helper (e.g., EnsureSavedAsync) to DRY the “SaveChangesAsync>0 or throw” pattern across handlers.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Update/UpdateCorrespondentBankDetailsHandler.cs (4)
35-39: Use a specific NotFound exception instead of generic ExceptionThrow KeyNotFoundException (or a domain NotFoundException) to preserve semantics and enable differentiated handling upstream.
- throw new Exception(ErrorMessagesConstants - .NotFound(request.Id, typeof(Entities.CorrespondentBankDetails))); + throw new KeyNotFoundException(ErrorMessagesConstants + .NotFound(request.Id, typeof(Entities.CorrespondentBankDetails)));
41-44: Map onto the tracked entity; avoid creating a new variableMapping directly into correspondentBankDetailsEntity improves clarity and reduces risk with change tracking; Update(...) may be unnecessary if the context tracks the entity.
- Entities.CorrespondentBankDetails entityToUpdate = _mapper.Map(request.UpdateCorrespondentBankDetailsDto, correspondentBankDetailsEntity); - - _repositoryWrapper.CorrespondentBankDetailsRepository.Update(entityToUpdate); + _mapper.Map(request.UpdateCorrespondentBankDetailsDto, correspondentBankDetailsEntity); + // If your repository requires explicit Update marking, keep this call; otherwise you can remove it. + _repositoryWrapper.CorrespondentBankDetailsRepository.Update(correspondentBankDetailsEntity);
45-52: Treat no-op updates as success; align error message; pass CancellationTokenSaveChangesAsync can return 0 when the incoming values equal the current ones. That shouldn’t be a DB failure. Also prefer the “InDatabase” message variant and propagate cancellationToken.
- if (await _repositoryWrapper.SaveChangesAsync() > 0) - { - CorrespondentBankDetailsDto responseDto = _mapper.Map<CorrespondentBankDetailsDto>(entityToUpdate); - return responseDto; - } - - throw new DbUpdateException(ErrorMessagesConstants.FailedToUpdateEntity(typeof(Entities.CorrespondentBankDetails))); + await _repositoryWrapper.SaveChangesAsync(cancellationToken); + return _mapper.Map<CorrespondentBankDetailsDto>(correspondentBankDetailsEntity); + // If you must signal a real DB failure, use the more specific message: + // throw new DbUpdateException(ErrorMessagesConstants.FailedToUpdateEntityInDatabase(typeof(Entities.CorrespondentBankDetails)));
29-33: Consider adding CancellationToken support to repository methods for proper request cancellation handlingThe
GetFirstOrDefaultAsyncandSaveChangesAsyncrepository methods currently don't accept aCancellationTokenparameter, even though handlers have cancellation tokens available. While this is a recommended practice for async operations to prevent stuck I/O during request aborts, implementing it would require updating the repository interface and all implementing classes. This is a valuable improvement for production resilience but falls outside the scope of this change.VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/SupportOptions/Create/CreateSupportOptionsHandler.cs (1)
31-38: Prefer “InDatabase” message and propagate CancellationTokenUse the more precise error message when persistence fails and pass the token to SaveChangesAsync.
- if (await _repositoryWrapper.SaveChangesAsync() > 0) + if (await _repositoryWrapper.SaveChangesAsync(cancellationToken) > 0) { SupportOptionsDto responseDto = _mapper.Map<SupportOptionsDto>(entity); return responseDto; } - - throw new DbUpdateException(ErrorMessagesConstants.FailedToCreateEntity(typeof(Entities.SupportOptions))); + throw new DbUpdateException(ErrorMessagesConstants.FailedToCreateEntityInDatabase(typeof(Entities.SupportOptions)));VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/SupportOptions/Update/UpdateSupportOptionsHandler.cs (3)
35-39: Prefer KeyNotFoundException (or domain NotFoundException) over generic ExceptionImproves clarity and allows upstream mapping to 404 when desired.
- throw new Exception(ErrorMessagesConstants + throw new KeyNotFoundException(ErrorMessagesConstants .NotFound(request.Id, typeof(Entities.SupportOptions)));
41-44: Map into the tracked instance; Update() may be optionalKeeps change tracking straightforward; only call Update(...) if your repo expects explicit state change.
- Entities.SupportOptions entityToUpdate = _mapper.Map(request.UpdateSupportOptionsDto, supportOptionsEntity); - - _repositoryWrapper.SupportOptionsRepository.Update(entityToUpdate); + _mapper.Map(request.UpdateSupportOptionsDto, supportOptionsEntity); + _repositoryWrapper.SupportOptionsRepository.Update(supportOptionsEntity);
45-52: No-op updates shouldn’t be treated as DB failures; align message and pass tokenConsider not failing when SaveChangesAsync returns 0; also favor the “InDatabase” message and pass cancellationToken.
- if (await _repositoryWrapper.SaveChangesAsync() > 0) + await _repositoryWrapper.SaveChangesAsync(cancellationToken); { - SupportOptionsDto responseDto = _mapper.Map<SupportOptionsDto>(entityToUpdate); - return responseDto; + return _mapper.Map<SupportOptionsDto>(supportOptionsEntity); } - - throw new DbUpdateException(ErrorMessagesConstants.FailedToUpdateEntity(typeof(Entities.SupportOptions))); + // If differentiating true DB failures: + // throw new DbUpdateException(ErrorMessagesConstants.FailedToUpdateEntityInDatabase(typeof(Entities.SupportOptions)));VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/SupportOptions/Delete/DeleteSupportOptionsHandler.cs (2)
26-30: Use a more specific NotFound exceptionPrefer KeyNotFoundException (or a custom NotFoundException) over Exception for clearer intent.
- throw new Exception(ErrorMessagesConstants + throw new KeyNotFoundException(ErrorMessagesConstants .NotFound(request.Id, typeof(Entities.SupportOptions)));
34-39: Propagate token and use “InDatabase” message variant on delete failureCommunicates the exact failure point and cooperates with cancellation.
- if (await _repositoryWrapper.SaveChangesAsync() > 0) + if (await _repositoryWrapper.SaveChangesAsync(cancellationToken) > 0) { return entityToDelete.Id; } - - throw new DbUpdateException(ErrorMessagesConstants.FailedToDeleteEntity(typeof(Entities.SupportOptions))); + throw new DbUpdateException(ErrorMessagesConstants.FailedToDeleteEntityInDatabase(typeof(Entities.SupportOptions)));VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/UahBankDetails/Update/UpdateUahBankDetailsHandler.cs (4)
35-39: Use a specific exception for not‑found casesThrow a domain‑appropriate exception (e.g., KeyNotFoundException) instead of a generic Exception to enable precise handling and mapping upstream.
+using System.Collections.Generic; ... - throw new Exception(ErrorMessagesConstants - .NotFound(request.Id, typeof(Entities.UahBankDetails))); + throw new KeyNotFoundException(ErrorMessagesConstants + .NotFound(request.Id, typeof(Entities.UahBankDetails)));
41-44: Avoid Update(entity) if the fetched entity is trackedIf GetFirstOrDefaultAsync returns a tracked entity (typical for EF), mapping onto it is enough; calling Update forces all fields to Modified and can overwrite unchanged values/concurrency columns.
- _repositoryWrapper.UahBankDetailsRepository.Update(entityToUpdate); + // If the entity is tracked, no explicit Update call is needed. + // _repositoryWrapper.UahBankDetailsRepository.Update(entityToUpdate);
12-12: Centralized handling: consider refining BaseHandler mapping/loggingTo avoid leaking DB internals and to map not‑found distinctly, adjust BaseHandler to:
- catch KeyNotFoundException and map to a not‑found Result;
- sanitize DbUpdateException messages (return a generic message, log details);
- log unexpected exceptions.
Example (outside this file):
public async Task<Result<TResponse>> Handle(TRequest request, CancellationToken ct) { try { var response = await HandleRequest(request, ct); return Result.Ok(response); } catch (ValidationException ex) { return Result.Fail<TResponse>(ex.Errors.Select(e => e.ErrorMessage)); } catch (KeyNotFoundException ex) { return Result.Fail<TResponse>(ErrorMessagesConstants.NotFound()); // generic } catch (DbUpdateException ex) { // log ex safely; avoid returning raw DB message return Result.Fail<TResponse>(ErrorMessagesConstants.FailedToUpdateEntityInDatabase(typeof(object))); } catch (Exception) { // log ex return Result.Fail<TResponse>("An unexpected error occurred"); } }
29-33: Add CancellationToken support to repository methods (architectural prerequisite)The handler receives
cancellationTokenand already uses it for validation, but cannot propagate it toGetFirstOrDefaultAsync()orSaveChangesAsync()since these methods lackCancellationTokenparameters. To implement this recommendation, update the repository layer signatures inIRepositoryBaseandIRepositoryWrapperalong with their implementations to accept and pass through the cancellation token.VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/UahBankDetails/Delete/DeleteUahBankDetailsHandler.cs (3)
26-30: Throw a specific not‑found exceptionReplace generic Exception with KeyNotFoundException (or a project‑specific NotFoundException) to improve upstream handling.
+using System.Collections.Generic; ... - throw new Exception(ErrorMessagesConstants - .NotFound(request.Id, typeof(Entities.UahBankDetails))); + throw new KeyNotFoundException(ErrorMessagesConstants + .NotFound(request.Id, typeof(Entities.UahBankDetails)));
34-40: Align error message with DB operation phaseFailure here occurs on SaveChangesAsync; use the “…InDatabase” message variant for clarity.
- throw new DbUpdateException(ErrorMessagesConstants.FailedToDeleteEntity(typeof(Entities.UahBankDetails))); + throw new DbUpdateException(ErrorMessagesConstants.FailedToDeleteEntityInDatabase(typeof(Entities.UahBankDetails)));
20-24: Codebase-wide refactoring: Add CancellationToken support to repository methodsThe current repository interfaces don't expose
CancellationTokenparameters, preventing propagation to underlying Entity Framework Core operations. While Entity Framework Core async LINQ methods always have an overload accepting a cancellation token, the custom repository wrappers block this:
IRepositoryBase.GetFirstOrDefaultAsync()– no CancellationToken parameterIRepositoryWrapper.SaveChangesAsync()– no CancellationToken parameterTo follow best practices, add CancellationToken support to these base interfaces and implementations (RepositoryBase, RepositoryWrapper), then propagate tokens through all handlers. This is a best-practice enhancement rather than a bug fix and involves multiple files across the DAL and BLL layers.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/UahBankDetails/Create/CreateUahBankDetailsHandler.cs (2)
37-37: Use the database‑phase error message for clarityFailure happens during SaveChangesAsync; use FailedToCreateEntityInDatabase.
- throw new DbUpdateException(ErrorMessagesConstants.FailedToCreateEntity(typeof(Entities.UahBankDetails))); + throw new DbUpdateException(ErrorMessagesConstants.FailedToCreateEntityInDatabase(typeof(Entities.UahBankDetails)));
28-32: Consider adding CancellationToken support to repository methods for cooperative cancellationCurrently,
CreateAsyncandSaveChangesAsyncdo not accept CancellationToken parameters. While the underlyingDbContext.SaveChangesAsync()supports it, propagating cancellation through the repository layer would enable graceful cancellation of long-running operations.This requires updating:
IRepositoryBase<T>.CreateAsync(T entity)→ addCancellationToken cancellationToken = defaultIRepositoryWrapper.SaveChangesAsync()→ addCancellationToken cancellationToken = default- All implementations and call sites across the BLL handlers
This is a broader architectural enhancement; prioritize based on your cancellation handling strategy.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Create/CreateCorrespondentBankDetailsHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Delete/DeleteCorrespondentBankDetailsHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Update/UpdateCorrespondentBankDetailsHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Create/CreateForeignBankDetailsHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Delete/DeleteForeignBankDetailsHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Update/UpdateForeignBankDetailsHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/SupportOptions/Create/CreateSupportOptionsHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/SupportOptions/Delete/DeleteSupportOptionsHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/SupportOptions/Update/UpdateSupportOptionsHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/UahBankDetails/Create/CreateUahBankDetailsHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/UahBankDetails/Delete/DeleteUahBankDetailsHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/UahBankDetails/Update/UpdateUahBankDetailsHandler.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Delete/DeleteCorrespondentBankDetailsHandler.cs (5)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Delete/DeleteForeignBankDetailsHandler.cs (1)
Task(18-40)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/SupportOptions/Delete/DeleteSupportOptionsHandler.cs (1)
Task(18-40)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/UahBankDetails/Delete/DeleteUahBankDetailsHandler.cs (1)
Task(18-40)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToDeleteEntity(47-52)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/UahBankDetails/Delete/DeleteUahBankDetailsHandler.cs (5)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Delete/DeleteCorrespondentBankDetailsHandler.cs (1)
Task(18-40)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Delete/DeleteForeignBankDetailsHandler.cs (1)
Task(18-40)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/SupportOptions/Delete/DeleteSupportOptionsHandler.cs (1)
Task(18-40)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToDeleteEntity(47-52)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/UahBankDetails/Create/CreateUahBankDetailsHandler.cs (4)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Create/CreateForeignBankDetailsHandler.cs (1)
Task(24-38)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/UahBankDetails/Delete/DeleteUahBankDetailsHandler.cs (1)
Task(18-40)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToCreateEntity(19-24)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Update/UpdateForeignBankDetailsHandler.cs (3)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Update/UpdateCorrespondentBankDetailsHandler.cs (1)
Task(25-52)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Create/CreateForeignBankDetailsHandler.cs (1)
Task(24-38)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/SupportOptions/Create/CreateSupportOptionsHandler.cs (3)
VictoryCenter/VictoryCenter.DAL/Entities/SupportOptions.cs (1)
SupportOptions(4-10)VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToCreateEntity(19-24)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Update/UpdateCorrespondentBankDetailsHandler.cs (3)
VictoryCenter/VictoryCenter.DAL/Entities/CorrespondentBankDetails.cs (1)
CorrespondentBankDetails(2-11)VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToUpdateEntity(33-38)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/SupportOptions/Delete/DeleteSupportOptionsHandler.cs (3)
VictoryCenter/VictoryCenter.DAL/Entities/SupportOptions.cs (1)
SupportOptions(4-10)VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToDeleteEntity(47-52)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Delete/DeleteForeignBankDetailsHandler.cs (3)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Delete/DeleteCorrespondentBankDetailsHandler.cs (1)
Task(18-40)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToDeleteEntity(47-52)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/UahBankDetails/Update/UpdateUahBankDetailsHandler.cs (3)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/UahBankDetails/Create/CreateUahBankDetailsHandler.cs (1)
Task(24-38)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToUpdateEntity(33-38)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/SupportOptions/Update/UpdateSupportOptionsHandler.cs (5)
VictoryCenter/VictoryCenter.DAL/Entities/SupportOptions.cs (1)
SupportOptions(4-10)VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/SupportOptions/Create/CreateSupportOptionsHandler.cs (1)
Task(24-38)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/SupportOptions/Delete/DeleteSupportOptionsHandler.cs (1)
Task(18-40)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToUpdateEntity(33-38)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Create/CreateForeignBankDetailsHandler.cs (3)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Create/CreateCorrespondentBankDetailsHandler.cs (1)
Task(25-51)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/UahBankDetails/Create/CreateUahBankDetailsHandler.cs (1)
Task(24-38)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Create/CreateCorrespondentBankDetailsHandler.cs (7)
VictoryCenter/VictoryCenter.DAL/Entities/CorrespondentBankDetails.cs (1)
CorrespondentBankDetails(2-11)VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Delete/DeleteCorrespondentBankDetailsHandler.cs (1)
Task(18-40)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Update/UpdateCorrespondentBankDetailsHandler.cs (1)
Task(25-52)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Create/CreateForeignBankDetailsHandler.cs (1)
Task(24-38)VictoryCenter/VictoryCenter.DAL/Entities/ForeignBankDetails.cs (1)
ForeignBankDetails(4-14)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToCreateEntity(19-24)
⏰ 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 (14)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Update/UpdateForeignBankDetailsHandler.cs (2)
25-27: Good: validate early with FluentValidation.Early failure reduces DB roundtrips and centralizes error messages.
41-44: No action needed—Id field is already properly guarded.The mapping configuration in
VictoryCenter/VictoryCenter.BLL/Mapping/Donate/ForeignBankDetailsProfile.cs(lines 12–14) already includes.ForMember(dest => dest.Id, opt => opt.Ignore()), which prevents the Id from being remapped during updates. The code follows the recommended safeguard and is correctly configured.VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Create/CreateCorrespondentBankDetailsHandler.cs (1)
25-28: Nice: validate request before any DB work.VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Create/CreateForeignBankDetailsHandler.cs (1)
24-27: Good: request validation before persistence.VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Delete/DeleteForeignBankDetailsHandler.cs (2)
34-39: CurrentSaveChangesAsync()signature does not supportCancellationToken.The verification confirms that
IRepositoryWrapper.SaveChangesAsync()is defined asTask<int> SaveChangesAsync()without aCancellationTokenparameter. Since the condition stated in the review comment—"If IRepositoryWrapper exposes token-aware APIs"—is not met, the suggested change cannot be applied.Any modifications to support cancellation tokens would require updating the repository wrapper interface and all call sites across the codebase, which is beyond the scope of this handler.
20-24: Cascade delete confirmed — no change required.Entity relationship ForeignBankDetails -> CorrespondentBankDetails is configured with OnDelete(DeleteBehavior.Cascade) in the EF Core model and recorded in migrations, so deleting the parent will cascade to dependents; the Delete handler (remove + SaveChanges) is safe as‑is. See: VictoryCenter.DAL/Data/EntityTypeConfigurations/ForeignBankDetailsConfig.cs (HasMany(...).OnDelete(DeleteBehavior.Cascade)) and CorrespondentBankDetailsConfig.cs; migrations/model snapshot (e.g. 20250923143353_AddDonateEntities.cs and 20251015152033_DonateMigrationFix*.Designer.cs / VictoryCenterDbContextModelSnapshot.cs) also show .OnDelete(DeleteBehavior.Cascade).
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Delete/DeleteCorrespondentBankDetailsHandler.cs (1)
34-39: Repository abstraction layer would need to be updated first to support CancellationToken propagation.The suggestion is technically sound—Entity Framework and MediatR both support cooperative cancellation—but implementing it requires updating the
IRepositoryWrapperinterface andRepositoryWrapperimplementation to accept and forward the token. This affects 40+ call sites across the Commands and Services layers.Since the handler currently cannot pass
cancellationTokentoSaveChangesAsync()due to the repository's signature, consider this a good candidate for a future architectural enhancement rather than addressing it in this PR. The current implementation is functionally correct; refactoring to propagate cancellation tokens would be a codebase-wide concern better handled as a separate effort.VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Update/UpdateCorrespondentBankDetailsHandler.cs (1)
12-12: Migration to BaseHandler looks goodThe shift to BaseHandler<TRequest,TResponse> aligns with the PR goal of centralizing exception handling.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/SupportOptions/Create/CreateSupportOptionsHandler.cs (1)
11-12: Adoption of BaseHandler is consistent and cleanThe override-based HandleRequest flow removes per-handler try/catch clutter.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/SupportOptions/Update/UpdateSupportOptionsHandler.cs (2)
12-12: Pattern alignment looks goodExtends BaseHandler and keeps validation-first flow.
29-33: CancellationToken propagation requires infrastructure updates—the suggested diff won't compile as-isThe review comment proposes a sound best practice, but the suggested diff cannot be applied because the repository infrastructure does not currently support
CancellationTokenparameters.Current state:
GetFirstOrDefaultAsyncsignature does not acceptCancellationTokenSaveChangesAsyncalso lacksCancellationTokensupport- Attempting to pass
cancellationTokento these methods will result in a compilation errorWhat's needed:
To implement the recommendation properly, you must first update:
IRepositoryBase<T>interface — addCancellationTokenparameter toGetFirstOrDefaultAsync()RepositoryBase<T>implementation — accept and propagateCancellationTokentoDbContextqueries- Consider updating
SaveChangesAsync()to also acceptCancellationTokenfor consistency- Update all ~60+ call sites across handlers and queries to pass
cancellationTokenRecommendation:
File this as a separate architectural improvement task rather than a single-file fix. The current review comment's diff is premature without these foundational changes.VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/SupportOptions/Delete/DeleteSupportOptionsHandler.cs (2)
9-10: Deriving from BaseHandler is consistent with the new strategyKeeps exception handling centralized.
20-24: Cancellation token support unavailable—refactor not applicableThe
GetFirstOrDefaultAsyncmethod does not currently accept acancellationTokenparameter. The method signature isTask<T?> GetFirstOrDefaultAsync(QueryOptions<T>? queryOptions = null), and all 40+ call sites across the codebase invoke it without cancellation support. To implement this would require modifying the interface and base repository implementation—a broader architectural change beyond the scope of this handler. This suggestion cannot be applied as written.Likely an incorrect or invalid review comment.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/UahBankDetails/Create/CreateUahBankDetailsHandler.cs (1)
24-35: Overall flow looks goodValidation, mapping, persistence, and return shape align with the new BaseHandler pattern.
Please ensure tests cover:
- create happy path;
- validation failure (ValidationException);
- DB failure path mapped by BaseHandler.
...VictoryCenter.BLL/Commands/Admin/Donate/SupportOptions/Create/CreateSupportOptionsHandler.cs
Show resolved
Hide resolved
...VictoryCenter.BLL/Commands/Admin/Donate/UahBankDetails/Update/UpdateUahBankDetailsHandler.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Images/Create/CreateImageHandler.cs (1)
32-63: Remove redundant try-catch block—BaseHandler already provides exception handling.The try-catch block here is unnecessary because BaseHandler.Handle already wraps HandleRequest with comprehensive exception handling (catching ValidationException, DbUpdateException, and Exception). The inner try-catch defeats the purpose of centralizing exception handling.
Additionally, converting BlobStorageException to a generic Exception at line 61 loses valuable exception type information that BaseHandler could handle appropriately.
Apply this diff to remove redundant exception handling:
public override async Task<ImageDto> HandleRequest(CreateImageCommand request, CancellationToken cancellationToken) { - try - { - await _validator.ValidateAndThrowAsync(request, cancellationToken); + await _validator.ValidateAndThrowAsync(request, cancellationToken); - var fileName = Guid.NewGuid().ToString().Replace("-", ""); + var fileName = Guid.NewGuid().ToString().Replace("-", ""); - using TransactionScope transaction = _repositoryWrapper.BeginTransaction(); + using TransactionScope transaction = _repositoryWrapper.BeginTransaction(); - Image image = _mapper.Map<Image>(request.CreateImageDto); - image.BlobName = fileName; - image.CreatedAt = DateTimeOffset.UtcNow; + Image image = _mapper.Map<Image>(request.CreateImageDto); + image.BlobName = fileName; + image.CreatedAt = DateTimeOffset.UtcNow; - Image createdImage = await _repositoryWrapper.ImageRepository.CreateAsync(image, cancellationToken); + Image createdImage = await _repositoryWrapper.ImageRepository.CreateAsync(image, cancellationToken); - if (await _repositoryWrapper.SaveChangesAsync() <= 0) - { - throw new DbUpdateException(ImageConstants.FailToSaveImageInDatabase); - } + if (await _repositoryWrapper.SaveChangesAsync() <= 0) + { + throw new DbUpdateException(ImageConstants.FailToSaveImageInDatabase); + } - await _blobService.SaveFileInStorageAsync(request.CreateImageDto.Base64, fileName, request.CreateImageDto.MimeType); + await _blobService.SaveFileInStorageAsync(request.CreateImageDto.Base64, fileName, request.CreateImageDto.MimeType); - ImageDto? response = _mapper.Map<ImageDto>(createdImage); + ImageDto? response = _mapper.Map<ImageDto>(createdImage); - transaction.Complete(); + transaction.Complete(); - return response; - } - catch (BlobStorageException e) - { - throw new Exception(ErrorMessagesConstants.BlobStorageError(e.Message)); - } + return response; }If BlobStorageException requires special message formatting, consider adding it as a specific catch clause in BaseHandler.Handle instead.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.cs (1)
49-77: Addscope.Complete()before returning in CreateTeamMemberHandler.The transaction opened at line 49 lacks the required
Complete()call. Without it, theTransactionScoperolls back on disposal, negating the database insert despiteSaveChangesAsync()succeeding. This causes silent data loss.All other handlers in the codebase properly call
Complete()(verified across FaqQuestions, Images, and TeamMembers handlers), so this file is an isolated gap.Add
scope.Complete();immediately before thereturn result;statement at line 77:using (var scope = _repositoryWrapper.BeginTransaction()) { // ... create, save, map ... var result = _mapper.Map<TeamMemberDto>(entity); if (entity.ImageId != null) { // ... load image ... result.Image = _mapper.Map<ImageDto>(imageResult); } + scope.Complete(); return result; }VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Create/CreateProgramHandler.cs (1)
20-25: Update CreateProgramHandler constructor and related tests to remove unused IBlobService dependency.The
IBlobServicedependency is injected but never used. Removing it requires updating three test instantiations that currently pass_blobServiceMock.Object:
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Create/CreateProgramHandler.cs: Remove theIBlobService blobServiceparameter and the corresponding using statementVictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Programs/CreateProgramTests.cslines 82, 97, 107: Remove_blobServiceMock.Objectargument from eachCreateProgramHandlerinstantiation
♻️ Duplicate comments (3)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Create/CreateFaqQuestionHandler.cs (1)
61-64: Use database-specific failure message after SaveChangesAsyncThis is a DB persistence failure path; switch to the “InDatabase” variant for consistency.
- throw new DbUpdateException(ErrorMessagesConstants.FailedToCreateEntity(typeof(FaqQuestion))); + throw new DbUpdateException(ErrorMessagesConstants.FailedToCreateEntityInDatabase(typeof(FaqQuestion)));VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Update/UpdateFaqQuestionHandler.cs (1)
126-126: Zero-row update shouldn’t be a DbUpdateException (treat as no-op or use a specific domain error).If values are unchanged,
affectedRowscan be 0. Either return the current entity (no-op) or throw a domain-specific error, notDbUpdateException.Example no-op success:
- throw new DbUpdateException(ErrorMessagesConstants.FailedToUpdateEntity(typeof(FaqQuestion))); + scope.Complete(); + return _mapper.Map<FaqQuestionDto>(entityToUpdate);VictoryCenter/VictoryCenter.BLL/Commands/Admin/ProgramCategories/Create/CreateProgramCategoryHandler.cs (1)
33-36: Good fix: treating 0 affected rows as failureUsing
<= 0correctly handles “no rows saved” as a creation failure, aligning with other handlers. Thanks for addressing the earlier feedback.
🧹 Nitpick comments (20)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Donate/UahBankDetails/CreateUahBankDetailsTests.cs (1)
133-133: Consider usingIt.IsAny<CancellationToken>()for more flexible mocking.The mock setup currently uses
new CancellationToken(), which creates a specific default token. UsingIt.IsAny<CancellationToken>()would make the mock more flexible and better aligned with common mocking practices.Apply this diff:
- .CreateAsync(It.IsAny<Entities.UahBankDetails>(), new CancellationToken())); + .CreateAsync(It.IsAny<Entities.UahBankDetails>(), It.IsAny<CancellationToken>()));VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Images/CreateImage.cs (1)
67-68: Avoid brittle Moq matches for CancellationTokenMatch the actual token instead of using a hardcoded
new CancellationToken(). UseIt.IsAny<CancellationToken>()(or capture the token passed tohandler.Handle) to keep tests resilient.Apply this diff to the shown lines:
- _mockRepositoryWrapper.Setup(x => x.ImageRepository.CreateAsync(It.IsAny<Image>(), new CancellationToken())) + _mockRepositoryWrapper.Setup(x => x.ImageRepository.CreateAsync(It.IsAny<Image>(), It.IsAny<CancellationToken>())) ... - _mockRepositoryWrapper.Verify(x => x.ImageRepository.CreateAsync(It.IsAny<Image>(), new CancellationToken()), Times.Once); + _mockRepositoryWrapper.Verify(x => x.ImageRepository.CreateAsync(It.IsAny<Image>(), It.IsAny<CancellationToken>()), Times.Once); ... - _mockRepositoryWrapper.Setup(x => x.ImageRepository.CreateAsync(It.IsAny<Image>(), new CancellationToken())) + _mockRepositoryWrapper.Setup(x => x.ImageRepository.CreateAsync(It.IsAny<Image>(), It.IsAny<CancellationToken>())) ... - _mockRepositoryWrapper.Setup(x => x.ImageRepository.CreateAsync(It.IsAny<Image>(), new CancellationToken())).ReturnsAsync(_testImage); + _mockRepositoryWrapper.Setup(x => x.ImageRepository.CreateAsync(It.IsAny<Image>(), It.IsAny<CancellationToken>())).ReturnsAsync(_testImage);Also applies to: 100-100, 138-139, 165-165
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Categories/CreateCategoryTests.cs (2)
88-90: Assert across errors, not at a fixed indexError ordering can vary. Assert via predicate over the collection to avoid brittle indexing.
- Assert.False(result.IsSuccess); - Assert.Contains(ErrorMessagesConstants.PropertyIsRequired(nameof(Category.Name)), result.Errors[0].Message); + Assert.False(result.IsSuccess); + Assert.Contains(result.Errors, e => e.Message == ErrorMessagesConstants.PropertyIsRequired(nameof(Category.Name)));
125-125: Use flexible token matching in MoqReplace the hardcoded
new CancellationToken()withIt.IsAny<CancellationToken>()to future‑proof the setup.- _repositoryWrapperMock.Setup(repo => repo.CategoriesRepository.CreateAsync(It.IsAny<Category>(), new CancellationToken())); + _repositoryWrapperMock.Setup(repo => repo.CategoriesRepository.CreateAsync(It.IsAny<Category>(), It.IsAny<CancellationToken>()));VictoryCenter/VictoryCenter.BLL/Commands/Admin/Categories/Create/CreateCategoryHandler.cs (1)
37-40: Use database-specific failure message after SaveChangesAsyncThis branch indicates a persistence failure. Prefer
FailedToCreateEntityInDatabasefor consistency and clarity.- throw new DbUpdateException(ErrorMessagesConstants.FailedToCreateEntity(typeof(Category))); + throw new DbUpdateException(ErrorMessagesConstants.FailedToCreateEntityInDatabase(typeof(Category)));VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Create/CreateForeignBankDetailsHandler.cs (1)
31-38: Prefer “FailedToCreateEntityInDatabase” when SaveChangesAsync failsAlign error semantics with a database write failure.
- throw new DbUpdateException(ErrorMessagesConstants.FailedToCreateEntity(typeof(Entities.ForeignBankDetails))); + throw new DbUpdateException(ErrorMessagesConstants.FailedToCreateEntityInDatabase(typeof(Entities.ForeignBankDetails)));VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Create/CreateFaqQuestionHandler.cs (1)
37-55: Batch-validate page IDs to avoid loading all pages and N×M scansFetch only requested pages, detect missing IDs once, then build placements. This reduces memory and improves query efficiency.
- var allPages = await _repositoryWrapper.VisitorPagesRepository.GetAllAsync(); - FaqQuestion entity = _mapper.Map<FaqQuestion>(request.CreateFaqQuestionDto); - - foreach (var pageId in request.CreateFaqQuestionDto.PageIds.Distinct()) - { - if (!allPages.Any(p => p.Id == pageId)) - { - throw new Exception(ErrorMessagesConstants.NotFound(pageId, typeof(VisitorPage))); - } - - var priority = await _reorderService.GetNextDisplayOrderAsync<FaqPlacement>( - groupSelector: fp => fp.PageId == pageId); - - entity.Placements.Add(new FaqPlacement - { - PageId = pageId, - Priority = priority - }); - } + var pageIds = request.CreateFaqQuestionDto.PageIds.Distinct().ToList(); + var existingPages = await _repositoryWrapper.VisitorPagesRepository.GetAllAsync( + new QueryOptions<VisitorPage> { Filter = p => pageIds.Contains(p.Id) }); + var existingIds = existingPages.Select(p => p.Id).ToHashSet(); + var missingIds = pageIds.Except(existingIds).ToList(); + if (missingIds.Count > 0) + { + // Consider throwing a domain-specific NotFound exception type if available + throw new Exception(ErrorMessagesConstants.NotFound(missingIds.First(), typeof(VisitorPage))); + } + + FaqQuestion entity = _mapper.Map<FaqQuestion>(request.CreateFaqQuestionDto); + foreach (var pageId in pageIds) + { + var priority = await _reorderService.GetNextDisplayOrderAsync<FaqPlacement>( + groupSelector: fp => fp.PageId == pageId); + entity.Placements.Add(new FaqPlacement { PageId = pageId, Priority = priority }); + }Optional: if supported, replace the looped
Addwithentity.Placements.AddRange(newPlacements)to reduce change-tracker overhead.VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.cs (2)
52-56: Optional: centralize priority calculation via IReorderService.To keep ordering logic consistent with the rest of the codebase, prefer the shared service:
- var maxPriority = await _repositoryWrapper.TeamMembersRepository.MaxAsync( - u => u.Priority, - u => u.CategoryId == entity.CategoryId); - entity.Priority = (maxPriority ?? 0) + 1; + entity.Priority = await _reorderService.GetNextDisplayOrderAsync<TeamMember>( + groupSelector: u => u.CategoryId == entity.CategoryId);Requires injecting/assigning
IReorderService(see earlier comment).
47-58: Validate ImageId existence before persisting to avoid FK violations.If
entity.ImageIdis set but the image is missing, the insert may fail at commit. Validate upfront:var entity = _mapper.Map<TeamMember>(request.CreateTeamMemberDto); using (var scope = _repositoryWrapper.BeginTransaction()) { + if (entity.ImageId is not null) + { + var image = await _repositoryWrapper.ImageRepository.GetFirstOrDefaultAsync( + new QueryOptions<Image> { Filter = i => i.Id == entity.ImageId }); + if (image is null) + { + throw new Exception(ErrorMessagesConstants.NotFound(entity.ImageId, typeof(Image))); + } + }Also applies to: 65-74
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/CreateTeamMemberTests.cs (3)
175-181: Consider verifying transaction commit semantics.Current tests can’t detect missing
scope.Complete(). If feasible, wrapTransactionScopein an interface returned byIRepositoryWrapper.BeginTransaction()so you can mock and assertComplete()was called.
175-176: Minor: use 0 to represent failure from SaveChangesAsync.EF Core never returns negative values. Prefer
0to model a failed write.
124-127: Refactor Moq setups to useIt.IsAny<CancellationToken>()for resilient token matching across the test suite.The review identifies a valid pattern issue: hardcoding
new CancellationToken()in Moq setups creates brittle tests that depend on instance equality. UsingIt.IsAny<CancellationToken>()matches any token value and is more resilient to changes.The pattern confirmed across 11+ test files, including the specified locations at CreateTeamMemberTests.cs:124 and :169, plus similar instances in:
- CreateCategoryTests.cs, CreateProgramTests.cs, CreateImage.cs, CreateFaqQuestionTests.cs, CreateProgramCategoryTests.cs, and donation-related test files.
Apply the suggested refactor consistently across all these test files.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Create/CreateCorrespondentBankDetailsHandler.cs (1)
35-39: Prefer a domain-specific NotFound exception.Throwing a generic
Exceptionobscures intent. ANotFoundException(or similar) would allow BaseHandler or higher layers to map to clearer responses.VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Update/UpdateFaqQuestionHandler.cs (2)
83-87: Include missing page IDs in the error for easier diagnostics.Instead of a generic message, include the actual IDs:
- throw new Exception(FaqConstants.SomePagesNotFound); + throw new Exception(ErrorMessagesConstants.ReorderingContainsInvalidIds(typeof(VisitorPage), missingPageIds));
52-58: Optional: reduce SaveChanges calls.You can batch deletions/reordering/additions and call
SaveChangesAsyncfewer times to cut round trips, while keeping the transaction.Also applies to: 65-78, 103-105
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Create/CreateProgramHandler.cs (1)
67-70: Unify create failure message with the rest of the codebase.Consider switching to the same constant used in other create handlers:
- throw new DbUpdateException(ErrorMessagesConstants.FailedToCreateEntity(typeof(Program))); + throw new DbUpdateException(ErrorMessagesConstants.FailedToCreateEntityInDatabase(typeof(Program)));VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/ProgramCategories/CreateProgramCategoryTests.cs (3)
70-74: Swap expected/actual in Assert.Equal for clearer failure diffsImprove readability by placing expected first.
- Assert.Equal(result.Value.Name, _programCategoryDto.Name); + Assert.Equal(_programCategoryDto.Name, result.Value.Name);
80-91: Assert no persistence calls on validation failureSince validation fails early, the repository should not be invoked. Add verifies.
Assert.False(result.IsSuccess); Assert.NotEmpty(result.Errors); + + _repositoryWrapperMock.Verify(w => + w.ProgramCategoriesRepository.CreateAsync(It.IsAny<ProgramCategory>(), It.IsAny<CancellationToken>()), + Times.Never); + _repositoryWrapperMock.Verify(w => w.SaveChangesAsync(), Times.Never);
66-74: Also verify interactions on happy-pathEnsure CreateAsync and SaveChangesAsync are called once.
Assert.True(result.IsSuccess); Assert.Equal(_programCategoryDto.Name, result.Value.Name); + _repositoryWrapperMock.Verify(w => + w.ProgramCategoriesRepository.CreateAsync(It.IsAny<ProgramCategory>(), It.IsAny<CancellationToken>()), + Times.Once); + _repositoryWrapperMock.Verify(w => w.SaveChangesAsync(), Times.Once);VictoryCenter/VictoryCenter.BLL/Commands/Admin/ProgramCategories/Create/CreateProgramCategoryHandler.cs (1)
25-32: Consistent cancellation support on repository callsNice use of
cancellationTokenin validation andCreateAsync. Consider propagating cancellation toSaveChangesAsyncin the repository interface in a follow-up for full cancellation support.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Categories/Create/CreateCategoryHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Create/CreateCorrespondentBankDetailsHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Create/CreateForeignBankDetailsHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/SupportOptions/Create/CreateSupportOptionsHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/UahBankDetails/Create/CreateUahBankDetailsHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Create/CreateFaqQuestionHandler.cs(3 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Update/UpdateFaqQuestionHandler.cs(3 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Images/Create/CreateImageHandler.cs(5 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/ProgramCategories/Create/CreateProgramCategoryHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Create/CreateProgramHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.cs(4 hunks)VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryBase.cs(1 hunks)VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryBase.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Categories/CreateCategoryTests.cs(2 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Donate/CorrespondentBankDetails/CreateCorrespondentBankDetailsTests.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Donate/ForeignBankDetails/CreateForeignBankDetailsTests.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Donate/SupportOptions/CreateSupportOptionsTests.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Donate/UahBankDetails/CreateUahBankDetailsTests.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/FaqQuestions/CreateFaqQuestionTests.cs(4 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Images/CreateImage.cs(5 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/ProgramCategories/CreateProgramCategoryTests.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Programs/CreateProgramTests.cs(2 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/CreateTeamMemberTests.cs(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/Admin/Images/Create/CreateImageHandler.cs
🧬 Code graph analysis (18)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/ProgramCategories/CreateProgramCategoryTests.cs (1)
VictoryCenter/VictoryCenter.DAL/Entities/ProgramCategory.cs (1)
ProgramCategory(3-9)
VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryBase.cs (3)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Create/CreateFaqQuestionHandler.cs (1)
Task(33-69)VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryBase.cs (7)
Task(10-10)Task(12-12)Task(14-14)Task(16-16)Task(18-18)Task(32-33)Task(35-35)VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryWrapper.cs (1)
Task(70-73)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/UahBankDetails/Create/CreateUahBankDetailsHandler.cs (5)
VictoryCenter/VictoryCenter.DAL/Entities/UahBankDetails.cs (1)
UahBankDetails(2-10)VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Categories/Create/CreateCategoryHandler.cs (1)
Task(28-44)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Create/CreateCorrespondentBankDetailsHandler.cs (1)
Task(25-51)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Create/CreateForeignBankDetailsHandler.cs (1)
Task(24-38)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Images/Create/CreateImageHandler.cs (4)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Categories/Create/CreateCategoryHandler.cs (1)
Task(28-44)VictoryCenter/VictoryCenter.BLL/Constants/ImageConstants.cs (1)
ImageConstants(3-37)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)BlobStorageError(125-128)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Donate/UahBankDetails/CreateUahBankDetailsTests.cs (1)
VictoryCenter/VictoryCenter.DAL/Entities/UahBankDetails.cs (1)
UahBankDetails(2-10)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Create/CreateFaqQuestionHandler.cs (9)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Categories/Create/CreateCategoryHandler.cs (1)
Task(28-44)VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Update/UpdateFaqQuestionHandler.cs (1)
Task(34-127)VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryBase.cs (7)
Task(10-10)Task(12-12)Task(14-14)Task(16-16)Task(18-18)Task(32-33)Task(35-35)VictoryCenter/VictoryCenter.DAL/Entities/FaqQuestion.cs (1)
FaqQuestion(5-18)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (4)
ErrorMessagesConstants(3-129)NotFound(7-10)NotFound(12-17)FailedToCreateEntity(19-24)VictoryCenter/VictoryCenter.DAL/Entities/VisitorPage.cs (1)
VisitorPage(3-14)VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryWrapper.cs (1)
TransactionScope(36-36)VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryWrapper.cs (1)
TransactionScope(75-78)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Update/UpdateFaqQuestionHandler.cs (9)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Create/CreateFaqQuestionHandler.cs (1)
Task(33-69)VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Delete/DeleteFaqQuestionHandler.cs (1)
Task(22-63)VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Update/UpdateTeamMemberHandler.cs (1)
Task(34-113)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (4)
ErrorMessagesConstants(3-129)NotFound(7-10)NotFound(12-17)FailedToUpdateEntity(33-38)VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryWrapper.cs (1)
TransactionScope(36-36)VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryWrapper.cs (1)
TransactionScope(75-78)VictoryCenter/VictoryCenter.DAL/Entities/FaqPlacement.cs (1)
FaqPlacement(5-16)VictoryCenter/VictoryCenter.BLL/Constants/FaqQuestionConstants.cs (1)
FaqConstants(3-8)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Categories/CreateCategoryTests.cs (2)
VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)PropertyIsRequired(83-86)VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Categories/CategoriesRepository.cs (2)
CategoriesRepository(8-14)CategoriesRepository(10-13)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Create/CreateProgramHandler.cs (6)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (3)
BaseHandler(8-34)Task(12-31)Task(33-33)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Images/Create/CreateImageHandler.cs (1)
Task(30-63)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Update/UpdateProgramHandler.cs (1)
Task(27-81)VictoryCenter/VictoryCenter.DAL/Entities/ProgramCategory.cs (1)
ProgramCategory(3-9)VictoryCenter/VictoryCenter.DAL/Entities/Program.cs (1)
Program(5-15)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (3)
ErrorMessagesConstants(3-129)ReorderingContainsInvalidIds(61-66)FailedToCreateEntity(19-24)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/CreateTeamMemberTests.cs (3)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.cs (2)
CreateTeamMemberHandler(15-79)CreateTeamMemberHandler(22-30)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToCreateEntityInDatabase(26-31)VictoryCenter/VictoryCenter.DAL/Entities/TeamMember.cs (1)
TeamMember(6-29)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Categories/Create/CreateCategoryHandler.cs (4)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryBase.cs (3)
Task(10-10)Task(12-12)Task(14-14)VictoryCenter/VictoryCenter.DAL/Entities/Category.cs (1)
Category(3-14)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToCreateEntity(19-24)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.cs (6)
VictoryCenter/VictoryCenter.BLL/Services/ReorderService/ReorderService.cs (2)
ReorderService(14-162)ReorderService(18-21)VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (3)
BaseHandler(8-34)Task(12-31)Task(33-33)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Categories/Create/CreateCategoryHandler.cs (1)
Task(28-44)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Images/Create/CreateImageHandler.cs (1)
Task(30-63)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (4)
ErrorMessagesConstants(3-129)NotFound(7-10)NotFound(12-17)FailedToCreateEntityInDatabase(26-31)VictoryCenter/VictoryCenter.DAL/Entities/TeamMember.cs (1)
TeamMember(6-29)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Programs/CreateProgramTests.cs (1)
VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)PropertyIsRequired(83-86)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Donate/SupportOptions/CreateSupportOptionsTests.cs (1)
VictoryCenter/VictoryCenter.DAL/Entities/SupportOptions.cs (1)
SupportOptions(4-10)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Create/CreateCorrespondentBankDetailsHandler.cs (7)
VictoryCenter/VictoryCenter.DAL/Entities/CorrespondentBankDetails.cs (1)
CorrespondentBankDetails(2-11)VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Create/CreateForeignBankDetailsHandler.cs (1)
Task(24-38)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Delete/DeleteCorrespondentBankDetailsHandler.cs (1)
Task(18-40)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Update/UpdateCorrespondentBankDetailsHandler.cs (1)
Task(25-52)VictoryCenter/VictoryCenter.DAL/Entities/ForeignBankDetails.cs (1)
ForeignBankDetails(4-14)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToCreateEntity(19-24)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/ProgramCategories/Create/CreateProgramCategoryHandler.cs (3)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Categories/Create/CreateCategoryHandler.cs (1)
Task(28-44)VictoryCenter/VictoryCenter.DAL/Entities/ProgramCategory.cs (1)
ProgramCategory(3-9)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Create/CreateForeignBankDetailsHandler.cs (4)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Categories/Create/CreateCategoryHandler.cs (1)
Task(28-44)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Create/CreateCorrespondentBankDetailsHandler.cs (1)
Task(25-51)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToCreateEntity(19-24)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/SupportOptions/Create/CreateSupportOptionsHandler.cs (4)
VictoryCenter/VictoryCenter.DAL/Entities/SupportOptions.cs (1)
SupportOptions(4-10)VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Categories/Create/CreateCategoryHandler.cs (1)
Task(28-44)VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/ForeignBankDetails/Create/CreateForeignBankDetailsHandler.cs (1)
Task(24-38)
⏰ 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 (18)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/UahBankDetails/Create/CreateUahBankDetailsHandler.cs (1)
24-38: Implementation looks good and follows the BaseHandler pattern correctly.The handler properly:
- Validates the request and throws
ValidationExceptionon failure- Maps the DTO to entity and creates it via repository
- Propagates the
CancellationTokenthroughout the async chain- Throws
DbUpdateExceptionwhen persistence fails- Returns the mapped DTO on success
The implementation is consistent with other handlers in the codebase (e.g.,
CreateForeignBankDetailsHandler,CreateCorrespondentBankDetailsHandler).VictoryCenter/VictoryCenter.BLL/Commands/Admin/Images/Create/CreateImageHandler.cs (1)
15-15: LGTM! BaseHandler pattern correctly applied.The handler successfully migrates to the new BaseHandler pattern, which centralizes exception handling as intended by the PR objectives.
VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryBase.cs (1)
14-14: LGTM! CancellationToken support added to CreateAsync.The signature change follows .NET async best practices by enabling cancellation support. All callers across the codebase have been updated to pass the CancellationToken parameter.
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Programs/CreateProgramTests.cs (2)
100-100: LGTM! More precise error assertion.The test now validates the exact error message from ErrorMessagesConstants.PropertyIsRequired, which is more precise than the previous generic "Validation failed" check.
141-141: LGTM! Test updated to match new repository signature.The CreateAsync call correctly passes CancellationToken to align with the updated repository interface.
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Donate/ForeignBankDetails/CreateForeignBankDetailsTests.cs (1)
139-139: LGTM! Test updated to match new repository signature.The CreateAsync mock setup correctly includes CancellationToken to align with the updated repository interface.
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Donate/SupportOptions/CreateSupportOptionsTests.cs (1)
92-92: LGTM! Test updated to match new repository signature.The CreateAsync mock setup correctly includes CancellationToken to align with the updated repository interface.
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Donate/CorrespondentBankDetails/CreateCorrespondentBankDetailsTests.cs (1)
174-174: LGTM! Test updated to match new repository signature.The CreateAsync mock setup correctly includes CancellationToken to align with the updated repository interface.
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/FaqQuestions/CreateFaqQuestionTests.cs (3)
1-1: LGTM! BOM artifact cleaned up.The byte order mark (BOM) has been removed from the using directive, improving code cleanliness.
125-125: LGTM! Tests updated to match new repository signature.Both CreateAsync calls (in test setup and mock configuration) correctly include CancellationToken to align with the updated repository interface.
Also applies to: 168-168
135-135: LGTM! More accurate error assertion.The test now asserts against the actual exception message (testMessage) thrown by DbUpdateException, which is more precise than checking against a constant.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Update/UpdateFaqQuestionHandler.cs (1)
116-123: Nice defensive re-fetch/Null check.Adding a null check before mapping prevents NREs on concurrent deletes.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Programs/Create/CreateProgramHandler.cs (1)
40-51: Good: image existence is validated before attach.The explicit NotFound on missing ImageId prevents dangling FKs and aligns with other handlers.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/CorrespondentBankDetails/Create/CreateCorrespondentBankDetailsHandler.cs (1)
50-51: The review comment's premise is incorrect based on codebase analysis.The suggestion to change
FailedToCreateEntitytoFailedToCreateEntityInDatabasecontradicts the actual pattern in the codebase:
- 8 out of 10 create handlers use
FailedToCreateEntity(Programs, ProgramCategories, FaqQuestions, UahBankDetails, CorrespondentBankDetails, Categories, ForeignBankDetails, SupportOptions)- 1 out of 10 uses
FailedToCreateEntityInDatabase(TeamMembers)The CreateCorrespondentBankDetailsHandler is already aligned with the predominant pattern. If consistency is a goal, the outlier (TeamMembers) might warrant investigation instead. The current implementation requires no changes.
Likely an incorrect or invalid review comment.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/Donate/SupportOptions/Create/CreateSupportOptionsHandler.cs (1)
31-38: The review comment misidentifies the inconsistency; SupportOptions is already standardized with the dominant convention.Evidence shows that 8 of 10 Create handlers use
FailedToCreateEntity, including the file under review (CreateSupportOptionsHandler.cs:37). Only CreateTeamMemberHandler.cs deviates withFailedToCreateEntityInDatabase. Since SupportOptions already aligns with the established pattern across the codebase, no changes are needed here. If standardization is desired, the focus should be on bringing CreateTeamMemberHandler.cs in line with the others, not adjusting SupportOptions.Likely an incorrect or invalid review comment.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.cs (1)
8-9: Remove the unused readonly field and using statement—they prevent compilation.The field
_reorderServiceat line 20 is declared but never injected in the constructor (lines 22-26) and never used throughout the class. Since C# requires readonly fields to be assigned in the constructor or at declaration, this code will not compile. Removing the unused dependency (Option A from the review) is the straightforward solution here.VictoryCenter/VictoryCenter.BLL/Commands/Admin/ProgramCategories/Create/CreateProgramCategoryHandler.cs (2)
38-40: LGTM: mapping entity → DTO after persistenceReturning the mapped DTO post-
SaveChangesAsyncis correct and keeps handler slim under BaseHandler.
29-31: No changes required—property and usage are correct and consistent.Verification confirms that
CreateProgramCategoryCommandhas a property namedProgramCategoryDto(of typeCreateProgramCategoryDto), the handler correctly readsrequest.ProgramCategoryDto, and tests construct the command asnew CreateProgramCategoryCommand(new CreateProgramCategoryDto { ... }). The naming is intentional and consistent throughout the codebase—there is no confusion or mismatch.
| public async Task<T> CreateAsync(T entity, CancellationToken cancellationToken) | ||
| { | ||
| return (await DbContext.Set<T>().AddAsync(entity)).Entity; | ||
| } |
There was a problem hiding this comment.
Pass cancellationToken to AddAsync for proper cancellation support.
The cancellationToken parameter is accepted but not used. Entity Framework's AddAsync supports cancellation, so the token should be passed through to enable proper cancellation support during value generation or other async operations.
Apply this diff:
public async Task<T> CreateAsync(T entity, CancellationToken cancellationToken)
{
- return (await DbContext.Set<T>().AddAsync(entity)).Entity;
+ return (await DbContext.Set<T>().AddAsync(entity, cancellationToken)).Entity;
}📝 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<T> CreateAsync(T entity, CancellationToken cancellationToken) | |
| { | |
| return (await DbContext.Set<T>().AddAsync(entity)).Entity; | |
| } | |
| public async Task<T> CreateAsync(T entity, CancellationToken cancellationToken) | |
| { | |
| return (await DbContext.Set<T>().AddAsync(entity, cancellationToken)).Entity; | |
| } |
🤖 Prompt for AI Agents
In
VictoryCenter/VictoryCenter.DAL/Repositories/Realizations/Base/RepositoryBase.cs
around lines 47 to 50, the CreateAsync method accepts a CancellationToken but
does not pass it to DbContext.Set<T>().AddAsync; update the call to pass the
cancellationToken parameter into AddAsync so EF Core can observe cancellation
during the async add operation and return the entity as before.
| private void SetupRepositoryWrapper(int saveResult) | ||
| { | ||
| _repositoryWrapperMock.Setup(repo => repo.ProgramCategoriesRepository | ||
| .CreateAsync(It.IsAny<ProgramCategory>())); | ||
| .CreateAsync(It.IsAny<ProgramCategory>(), new CancellationToken())); | ||
| _repositoryWrapperMock.Setup(repo => repo.SaveChangesAsync()).ReturnsAsync(saveResult); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Stabilize Moq setup: nested repo mock + return CompletedTask + flexible token match
Current setup risks awaiting a null Task and NREs, and may rely on recursive mocks for the nested property. Provide an explicit repository mock, return a completed Task, and relax the token matcher.
Apply:
- private void SetupRepositoryWrapper(int saveResult)
- {
- _repositoryWrapperMock.Setup(repo => repo.ProgramCategoriesRepository
- .CreateAsync(It.IsAny<ProgramCategory>(), new CancellationToken()));
- _repositoryWrapperMock.Setup(repo => repo.SaveChangesAsync()).ReturnsAsync(saveResult);
- }
+ private void SetupRepositoryWrapper(int saveResult)
+ {
+ var programCategoriesRepoMock = new Mock<IProgramCategoriesRepository>(MockBehavior.Strict);
+ programCategoriesRepoMock
+ .Setup(r => r.CreateAsync(It.IsAny<ProgramCategory>(), It.IsAny<CancellationToken>()))
+ .Returns(Task.CompletedTask);
+
+ _repositoryWrapperMock
+ .SetupGet(repo => repo.ProgramCategoriesRepository)
+ .Returns(programCategoriesRepoMock.Object);
+
+ _repositoryWrapperMock
+ .Setup(repo => repo.SaveChangesAsync())
+ .ReturnsAsync(saveResult);
+ }Optional: if you prefer not to introduce a nested mock, at least change the matcher and return a completed Task:
- _repositoryWrapperMock.Setup(repo => repo.ProgramCategoriesRepository
- .CreateAsync(It.IsAny<ProgramCategory>(), new CancellationToken()));
+ _repositoryWrapperMock.Setup(repo => repo.ProgramCategoriesRepository
+ .CreateAsync(It.IsAny<ProgramCategory>(), It.IsAny<CancellationToken>()))
+ .Returns(Task.CompletedTask);To verify repository-wide mocking defaults aren’t masking this, run:
🏁 Script executed:
#!/bin/bash
# Look for recursive-mock/default-value configs that could hide NREs
rg -n --type cs -C2 'DefaultValue|DefaultValueProvider|MockBehavior|SetReturnsDefault|CallBase|Recursive' VictoryCenter.UnitTestsLength of output: 63
I need to explore the actual repository structure. Let me search for the test file and examine the codebase directly.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the test file and explore directory structure
fd -type f -name "*.csproj" | head -20
echo "---"
fd -type f -name "CreateProgramCategoryTests.cs"
Length of output: 345
Let me correct the fd command syntax:
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the test file and explore directory structure
fd -t f -name "*.csproj" | head -20
echo "---"
fd -t f -name "CreateProgramCategoryTests.cs"
echo "---"
# List top-level directories
ls -la | head -20
Length of output: 1320
Let me try a simpler approach to locate and examine the test file:
---
🏁 Script executed:
```shell
#!/bin/bash
# Direct path check and exploration
ls -la VictoryCenter/
echo "---"
find . -name "CreateProgramCategoryTests.cs" -type f
Length of output: 921
Now let me examine the actual test file:
---
🏁 Script executed:
```shell
#!/bin/bash
# View the test file
cat ./VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/ProgramCategories/CreateProgramCategoryTests.cs
Length of output: 4478
Now let me check the handler implementation and interface definitions to verify the concerns:
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the handler implementation
find . -name "CreateProgramCategoryHandler.cs" -type f
echo "---"
# Search for IProgramCategoriesRepository and IRepositoryWrapper definitions
find . -name "*.cs" -type f -path "*/Interfaces/*" | grep -i "repository"
Length of output: 1434
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the handler implementation
cat ./VictoryCenter/VictoryCenter.BLL/Commands/Admin/ProgramCategories/Create/CreateProgramCategoryHandler.cs
echo "---HANDLER_END---"
# Check the interface definitions
cat ./VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryWrapper.cs
echo "---WRAPPER_END---"
cat ./VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/ProgramCategories/IProgramCategoriesRepository.cs
Length of output: 3577
---
🏁 Script executed:
```shell
#!/bin/bash
# Check the IRepositoryBase interface to see CreateAsync return type
cat ./VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryBase.cs
Length of output: 1029
Let me check if there are any other mock configurations that might mask this issue:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for any Moq default configurations or recursive mock settings in test files
grep -rn "MockBehavior\|SetReturnsDefault\|CallBase\|DefaultValue" ./VictoryCenter/VictoryCenter.UnitTests/ --include="*.cs" | head -20
echo "---"
# Check if the test actually compiles and what happens with null Task
grep -rn "CreateAsync" ./VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/ProgramCategories/ --include="*.cs" -B2 -A2
Length of output: 1000
Fix the mock setup to return an awaitable Task and use flexible token matching
The concerns in the original review are valid and critical. The mock setup for CreateAsync is missing a return value, which causes Moq (with default loose behavior) to return null. Since the handler code actually awaits this result on line 34 of CreateProgramCategoryHandler.cs, this creates a real NRE risk. Additionally, the token matcher uses new CancellationToken() which won't match the cancellationToken parameter passed by the handler, causing the setup to be skipped and the loose mock to create a recursive mock that also returns null.
Apply the recommended fix to explicitly set up the nested repository mock with a completed Task return and flexible token matching:
private void SetupRepositoryWrapper(int saveResult)
{
var programCategoriesRepoMock = new Mock<IProgramCategoriesRepository>(MockBehavior.Strict);
programCategoriesRepoMock
.Setup(r => r.CreateAsync(It.IsAny<ProgramCategory>(), It.IsAny<CancellationToken>()))
.Returns(Task.CompletedTask);
_repositoryWrapperMock
.SetupGet(repo => repo.ProgramCategoriesRepository)
.Returns(programCategoriesRepoMock.Object);
_repositoryWrapperMock
.Setup(repo => repo.SaveChangesAsync())
.ReturnsAsync(saveResult);
}Alternatively, if you prefer to avoid the nested mock, at minimum change the matcher to It.IsAny<CancellationToken>() and return a completed Task:
private void SetupRepositoryWrapper(int saveResult)
{
_repositoryWrapperMock.Setup(repo => repo.ProgramCategoriesRepository
.CreateAsync(It.IsAny<ProgramCategory>(), It.IsAny<CancellationToken>()))
.Returns(Task.CompletedTask);
_repositoryWrapperMock.Setup(repo => repo.SaveChangesAsync()).ReturnsAsync(saveResult);
}📝 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.
| private void SetupRepositoryWrapper(int saveResult) | |
| { | |
| _repositoryWrapperMock.Setup(repo => repo.ProgramCategoriesRepository | |
| .CreateAsync(It.IsAny<ProgramCategory>())); | |
| .CreateAsync(It.IsAny<ProgramCategory>(), new CancellationToken())); | |
| _repositoryWrapperMock.Setup(repo => repo.SaveChangesAsync()).ReturnsAsync(saveResult); | |
| } | |
| private void SetupRepositoryWrapper(int saveResult) | |
| { | |
| var programCategoriesRepoMock = new Mock<IProgramCategoriesRepository>(MockBehavior.Strict); | |
| programCategoriesRepoMock | |
| .Setup(r => r.CreateAsync(It.IsAny<ProgramCategory>(), It.IsAny<CancellationToken>())) | |
| .Returns(Task.CompletedTask); | |
| _repositoryWrapperMock | |
| .SetupGet(repo => repo.ProgramCategoriesRepository) | |
| .Returns(programCategoriesRepoMock.Object); | |
| _repositoryWrapperMock | |
| .Setup(repo => repo.SaveChangesAsync()) | |
| .ReturnsAsync(saveResult); | |
| } |
| private void SetupRepositoryWrapper(int saveResult) | |
| { | |
| _repositoryWrapperMock.Setup(repo => repo.ProgramCategoriesRepository | |
| .CreateAsync(It.IsAny<ProgramCategory>())); | |
| .CreateAsync(It.IsAny<ProgramCategory>(), new CancellationToken())); | |
| _repositoryWrapperMock.Setup(repo => repo.SaveChangesAsync()).ReturnsAsync(saveResult); | |
| } | |
| private void SetupRepositoryWrapper(int saveResult) | |
| { | |
| _repositoryWrapperMock.Setup(repo => repo.ProgramCategoriesRepository | |
| .CreateAsync(It.IsAny<ProgramCategory>(), It.IsAny<CancellationToken>())) | |
| .Returns(Task.CompletedTask); | |
| _repositoryWrapperMock.Setup(repo => repo.SaveChangesAsync()).ReturnsAsync(saveResult); | |
| } |
🤖 Prompt for AI Agents
In
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/ProgramCategories/CreateProgramCategoryTests.cs
around lines 118-123, the mock for CreateAsync doesn't return an awaitable Task
and uses a concrete new CancellationToken() matcher which won't match the
handler's token; update the setup so the nested
ProgramCategoriesRepository.CreateAsync call returns a completed Task (e.g.,
Task.CompletedTask) and use a flexible token matcher
(It.IsAny<CancellationToken>()) so the setup is hit, or alternatively set up the
repository wrapper to return a properly configured nested mock that itself
returns Task.CompletedTask for CreateAsync with It.IsAny<CancellationToken>().
There was a problem hiding this comment.
I really don't like the current concept of result handling, especially the part where we return internal error details like return Result.Fail(ex.Message); for a DbUpdateException or even a general Exception.
This approach has two major flaws:
- It leaks internal server details to the client, which is a security risk.
- It misuses FluentResults. We've included this library specifically to avoid this pattern.
We should be using typed errors for expected failures. For example, we should define custom errors like this:
public class NotFoundError : Error
{
public NotFoundError(string resourceName, object resourceId)
: base(ErrorMessagesConstants.NotFound(resourceName, resourceId))
{
Metadata.Add("ResourceType", resourceName);
Metadata.Add("ResourceId", resourceId);
}
}And then return these expected errors directly from our MediatR handlers instead of throwing exceptions:
if (newImage is null)
{
// OLD: throw new Exception(ErrorMessagesConstants.NotFound(...));
// NEW: Return a typed error
return Result.Fail(new NotFoundError(nameof(Image), request.CreateProgramDto.ImageId.Value));
}This allows our BaseApiController to handle results in a clean, type-safe way, without fragile string-matching:
if (result.HasError<NotFoundError>(out var notFoundErrors))
{
var firstError = notFoundErrors.First();
var notFoundDetails = ProblemsFactory.CreateProblemDetails(
HttpContext,
statusCode: StatusCodes.Status404NotFound,
detail: firstError.Message);
return NotFound(notFoundDetails);
}For unexpected errors (like a real database update failure), we should let them throw.
We already have an ExceptionHandlingMiddleware specifically for this purpose. Its job is to catch these unhandled exceptions, log them, and return a proper 500 Internal Server Error to the client. The current BaseHandler completely bypasses this by catching these exceptions and masking them as a 400 Bad Request.
If a developer really thinks a specific database error needs to be handled manually, they can create another custom error (e.g., UpdateFailedError) and return Result.Fail(new UpdateFailedError(...)). This can then be handled cleanly in the controller, just like the NotFoundError.
We need to refactor BaseHandler to remove this try-catch logic and let our middleware and FluentResults do their jobs correctly.
And other question is what should be in BaseHandler? We can still have this abstract class but with try-catch block for ValidationException to continue using ValidateAndThrowAsync method in MediatR handlers.
public abstract class BaseHandler<TRequest, TResponse>
: IRequestHandler<TRequest, Result<TResponse>>
where TRequest : IRequest<Result<TResponse>>
{
public async Task<Result<TResponse>> Handle(TRequest request, CancellationToken cancellationToken)
{
try
{
var response = await HandleRequest(request, cancellationToken);
return Result.Ok(response);
}
catch (ValidationException ex)
{
return Result.Fail<TResponse>(ex.Errors.Select(e => e.ErrorMessage));
}
}
public abstract Task<TResponse> HandleRequest(TRequest request, CancellationToken cancellationToken);
}
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Donate/UahBankDetails/CreateUahBankDetailsTests.cs (3)
77-96: Validation test should expect exceptions instead of failed Results.With the new
BaseHandlerpattern, validation failures should throw exceptions rather than returning failedResult<T>objects. This test should be rewritten to:
- Use
Assert.ThrowsAsync<ValidationException>()or equivalent- Verify exception details instead of checking
result.ErrorsBased on learnings from this codebase, focus on testing the handler's core responsibility (validation triggering appropriate exception) rather than the Result pattern that's being removed.
99-117: Error handling test should expect exceptions with BaseHandler pattern.The test checks for
result.IsSuccess == falseand verifies an error message inresult.Errors[0].Message. With centralized exception handling inBaseHandler, repository failures should throw exceptions. Rewrite this test to:
- Expect an exception when
SaveChangesAsyncreturns -1- Verify the exception message contains the expected error text
- Remove Result assertions
47-66: Refactor tests to match refactored handler—Result pattern removed.The handler has been updated to inherit from
BaseHandler<CreateUahBankDetailsCommand, UahBankDetailsDto>(removing theResult<T>wrapper), but these tests still expect the old pattern:
handler.Handle()should behandler.HandleRequest()(or equivalent)- Tests assert on
result.IsSuccessandresult.Value, but the handler now returnsUahBankDetailsDtodirectly- Invalid inputs should raise exceptions instead of returning failed Results
Update all test methods (lines 47–66, 77–96, 99–117) to:
- Await the handler method directly without unpacking
Result<T>- Use
Assert.Throws<>or equivalents for validation/error cases instead of checkingresult.Errors- Compare the returned
UahBankDetailsDtoproperties directlyVictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.cs (2)
20-20: Remove unused field.The
_reorderServicefield is declared but never initialized or used. The constructor (line 25) doesn't accept anIReorderServiceparameter.Apply this diff to remove the dead code:
private readonly IMapper _mapper; private readonly IRepositoryWrapper _repositoryWrapper; private readonly IValidator<CreateTeamMemberCommand> _validator; - private readonly IReorderService _reorderService; public CreateTeamMemberHandler(
48-76: CRITICAL: Missing transaction scope completion.The transaction scope created on line 48 is never completed with
scope.Complete()before the method returns. This will cause the transaction to rollback when the scope is disposed, preventing any team members from being created.Apply this diff to fix the transaction:
result.Image = _mapper.Map<ImageDto>(imageResult); } + scope.Complete(); + return result; } }This pattern is correctly implemented in
UpdateTeamMemberHandler.cs(line 106) and should be followed here.VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyPrograms/Update/UpdateHippotherapyProgramHandler.cs (2)
44-69: Prevent silent loss of invalid category IDs.When the request contains a category ID that does not exist,
newCategoriessimply omits it and the subsequentforeachreassigns only the found categories. The client’s intent is silently ignored and the missing ID is dropped. The create handler already guards against this by throwingReorderingContainsInvalidIds; please mirror that check here so we fail fast with a clear message.IEnumerable<HippotherapyProgramCategory> newCategories = await _repositoryWrapper.HippotherapyProgramCategoriesRepository.GetAllAsync( new QueryOptions<HippotherapyProgramCategory> { Filter = category => request.UpdateProgramDto.CategoryIds.Contains(category.Id), AsNoTracking = false }); + var missingCategoryIds = request.UpdateProgramDto.CategoryIds + .Except(newCategories.Select(category => category.Id)) + .ToList(); + + if (missingCategoryIds.Count > 0) + { + throw new Exception(ErrorMessagesConstants.ReorderingContainsInvalidIds( + typeof(HippotherapyProgramCategory), missingCategoryIds)); + } + _mapper.Map(request.UpdateProgramDto, programToUpdate);
55-62: Return NotFound when the new image is missing.If the requested image ID does not exist,
newImagebecomes null and we still proceed. That leavesprogramToUpdate.ImageIdset whileprogramToUpdate.Imageis null, and SaveChanges will throw a low-level FK violation instead of the user-friendly not-found error we emit during creation. Please add the explicit guard.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 null) + { + throw new Exception(ErrorMessagesConstants.NotFound( + request.UpdateProgramDto.ImageId!.Value, typeof(Image))); + } + programToUpdate.Image = newImage; }
♻️ Duplicate comments (1)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyPrograms/Delete/DeleteHippotherapyProgramHandler.cs (1)
36-39: Ensure SaveChangesAsync detects zero-row deletes.
SaveChangesAsync()never returns a negative number. If no rows are affected it returns0, and the current< 0comparison will silently treat that as success, leaving the program undeleted. Please adjust the check so that zero rows also trigger the failure path.- if (await _repositoryWrapper.SaveChangesAsync() < 0) + if (await _repositoryWrapper.SaveChangesAsync() <= 0) { throw new DbUpdateException(ErrorMessagesConstants.FailedToDeleteEntity(typeof(HippotherapyProgram))); }
🧹 Nitpick comments (8)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/CreateTeamMemberTests.cs (2)
124-127: Verify CancellationToken usage in mocks.The mock setup now includes
new CancellationToken()as a parameter matcher. While functional, usingIt.IsAny<CancellationToken>()would be more idiomatic for Moq and would match any CancellationToken instance, not just the default.Consider applying this diff for more idiomatic Moq usage:
_repositoryWrapperMock .Setup(repositoryWrapperMock => - repositoryWrapperMock.TeamMembersRepository.CreateAsync(It.IsAny<TeamMember>(), new CancellationToken())) + repositoryWrapperMock.TeamMembersRepository.CreateAsync(It.IsAny<TeamMember>(), It.IsAny<CancellationToken>())) .ThrowsAsync(new DbUpdateException(ErrorMessagesConstants.FailedToCreateEntityInDatabase(typeof(TeamMember))));The same pattern applies to line 169 in
SetupRepositoryWrapper.
169-173: CancellationToken mock setup should use It.IsAny().Similar to the previous comment, using
It.IsAny<CancellationToken>()instead ofnew CancellationToken()would make the mock more flexible and align with Moq best practices.Apply this diff:
_repositoryWrapperMock.Setup(repositoryWrapper => repositoryWrapper.TeamMembersRepository - .CreateAsync(It.IsAny<TeamMember>(), new CancellationToken())) + .CreateAsync(It.IsAny<TeamMember>(), It.IsAny<CancellationToken>())) .ReturnsAsync(teamMember);VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Delete/DeleteTeamMemberHandler.cs (2)
22-27: Including TeamCategory but then nulling it seems unnecessary.The query includes
TeamCategorynavigation property (line 26), but then the handler explicitly sets it tonull!(line 35). This suggests either:
- The Include is unnecessary if you're detaching the relationship anyway
- There's an EF Core configuration issue that requires this workaround
Consider one of these approaches:
Option 1: Remove the Include if you don't need the category data:
var entityToDelete = await _repositoryWrapper.TeamMembersRepository.GetFirstOrDefaultAsync( new QueryOptions<TeamMember> { - Filter = entity => entity.Id == request.Id, - Include = query => query.Include(tm => tm.TeamCategory) + Filter = entity => entity.Id == request.Id });Option 2: If the Include is needed to prevent cascade issues, add a comment explaining why:
+ // Include and clear TeamCategory to prevent cascade delete issues entityToDelete.TeamCategory = null!;
35-45: Consider renumbering priorities after deletion.Unlike the
UpdateTeamMemberHandlerwhich callsRenumberPriorityAsyncwhen the category changes (lines 86-88 in UpdateTeamMemberHandler.cs), this delete handler doesn't renumber the remaining team members' priorities after deletion. This could leave gaps in the priority sequence.If maintaining sequential priorities is important, consider adding reordering logic:
_repositoryWrapper.TeamMembersRepository.Delete(entityToDelete); + var deletedId = entityToDelete.Id; + var categoryId = entityToDelete.CategoryId; + var changes = await _repositoryWrapper.SaveChangesAsync(); if (changes <= 0) { throw new DbUpdateException(ErrorMessagesConstants.FailedToDeleteEntityInDatabase(typeof(TeamMember))); } + // Renumber priorities in the category after deletion + await _indexReorderService.RenumberPriorityAsync<TeamMember>( + groupSelector: tm => tm.CategoryId == categoryId); + await _repositoryWrapper.SaveChangesAsync(); + - return entityToDelete.Id; + return deletedId;Note: This would require re-adding
IReorderServiceto the constructor if this pattern should match the Update handler.VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.cs (1)
51-54: Consider using IReorderService for consistency.The manual priority calculation here duplicates the logic from
IReorderService.GetNextDisplayOrderAsync(see VictoryCenter.BLL/Services/ReorderService/ReorderService.cs lines 96-132). TheUpdateTeamMemberHandlerstill usesIReorderServicefor priority calculations, creating an inconsistency.For consistency with
UpdateTeamMemberHandlerand to avoid code duplication, consider:
- Re-add
IReorderServiceto the constructor:public CreateTeamMemberHandler( IRepositoryWrapper repositoryWrapper, IMapper mapper, - IValidator<CreateTeamMemberCommand> validator) + IValidator<CreateTeamMemberCommand> validator, + IReorderService reorderService) { _repositoryWrapper = repositoryWrapper; _mapper = mapper; _validator = validator; + _reorderService = reorderService; }
- Replace the manual calculation:
entity.CreatedAt = DateTimeOffset.UtcNow; - var maxPriority = await _repositoryWrapper.TeamMembersRepository.MaxAsync( - u => u.Priority, - u => u.CategoryId == entity.CategoryId); - entity.Priority = (maxPriority ?? 0) + 1; + entity.Priority = await _reorderService.GetNextDisplayOrderAsync<TeamMember>( + groupSelector: tm => tm.CategoryId == entity.CategoryId);This maintains consistency across handlers and centralizes the priority calculation logic.
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/HippotherapyPrograms/UpdateHippotherapyProgramTests.cs (1)
101-101: Check all validation errors instead of only the first.
To keep the test resilient if additional validators are added (or their order changes), let's search the whole error collection rather than assuming the Name violation will always be first.- Assert.Contains(ErrorMessagesConstants.PropertyIsRequired(nameof(HippotherapyProgramDto.Name)), result.Errors[0].Message); + Assert.Contains( + result.Errors, + error => error.Message.Contains(ErrorMessagesConstants.PropertyIsRequired(nameof(HippotherapyProgramDto.Name))));VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/HippotherapyPrograms/CreateHippotherapyProgramTests.cs (1)
100-100: Search the entire error list when asserting validation.
This avoids coupling the test to the ordering of validation failures.- Assert.Contains(ErrorMessagesConstants.PropertyIsRequired(nameof(HippotherapyProgramDto.Name)), result.Errors[0].Message); + Assert.Contains( + result.Errors, + error => error.Message.Contains(ErrorMessagesConstants.PropertyIsRequired(nameof(HippotherapyProgramDto.Name))));VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyPrograms/Create/CreateHippotherapyProgramHandler.cs (1)
71-72: Fix typo in response variable.
responceDtois misspelled. Please rename it toresponseDtofor clarity and consistency with the rest of the codebase.- HippotherapyProgramDto responceDto = _mapper.Map<HippotherapyProgramDto>(entity); - return responceDto; + HippotherapyProgramDto responseDto = _mapper.Map<HippotherapyProgramDto>(entity); + return responseDto;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyProgramCategories/Create/CreateHippotherapyProgramCategoryHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyProgramCategories/Delete/DeleteHippotherapyProgramCategoryHandler.cs(3 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyProgramCategories/Update/UpdateHippotherapyProgramCategoryHandler.cs(3 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyPrograms/Create/CreateHippotherapyProgramHandler.cs(2 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyPrograms/Delete/DeleteHippotherapyProgramHandler.cs(3 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyPrograms/Update/UpdateHippotherapyProgramHandler.cs(4 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamCategories/Create/CreateTeamCategoryHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamCategories/Delete/DeleteTeamCategoryHandler.cs(3 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.cs(4 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Delete/DeleteTeamMemberHandler.cs(1 hunks)VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Update/UpdateTeamMemberHandler.cs(2 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Donate/UahBankDetails/CreateUahBankDetailsTests.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/FaqQuestions/UpdateFaqQuestionTests.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/HippotherapyProgramCategories/CreateHippotherapyProgramCategoryTests.cs(2 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/HippotherapyPrograms/CreateHippotherapyProgramTests.cs(2 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/HippotherapyPrograms/UpdateHippotherapyProgramTests.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamCategories/CreateCategoryTests.cs(2 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamCategories/UpdateCategoryTests.cs(1 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/CreateTeamMemberTests.cs(7 hunks)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/UpdateTeamMemberTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/UpdateTeamMemberTests.cs
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: roman-stozhuk
PR: ita-social-projects/VictoryCenter-Back#349
File: VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/FaqQuestions/GetPublishedFaqQuestionsBySlugTests.cs:15-42
Timestamp: 2025-09-15T16:24:58.158Z
Learning: In unit tests for MediatR handlers, focus on testing the handler's core responsibilities (calling repositories with correct parameters, mapping results, error handling) rather than testing repository behavior like sorting and filtering, which should be mocked and tested separately in repository or integration tests.
📚 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.UnitTests/MediatRHandlersTests/FaqQuestions/UpdateFaqQuestionTests.csVictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyPrograms/Create/CreateHippotherapyProgramHandler.csVictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyPrograms/Delete/DeleteHippotherapyProgramHandler.csVictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/HippotherapyPrograms/UpdateHippotherapyProgramTests.csVictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyProgramCategories/Create/CreateHippotherapyProgramCategoryHandler.csVictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyPrograms/Update/UpdateHippotherapyProgramHandler.csVictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyProgramCategories/Update/UpdateHippotherapyProgramCategoryHandler.csVictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.csVictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyProgramCategories/Delete/DeleteHippotherapyProgramCategoryHandler.csVictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamCategories/Delete/DeleteTeamCategoryHandler.csVictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Update/UpdateTeamMemberHandler.csVictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Delete/DeleteTeamMemberHandler.cs
📚 Learning: 2025-09-15T16:24:58.158Z
Learnt from: roman-stozhuk
PR: ita-social-projects/VictoryCenter-Back#349
File: VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/FaqQuestions/GetPublishedFaqQuestionsBySlugTests.cs:15-42
Timestamp: 2025-09-15T16:24:58.158Z
Learning: In unit tests for MediatR handlers, focus on testing the handler's core responsibilities (calling repositories with correct parameters, mapping results, error handling) rather than testing repository behavior like sorting and filtering, which should be mocked and tested separately in repository or integration tests.
Applied to files:
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/FaqQuestions/UpdateFaqQuestionTests.csVictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/HippotherapyPrograms/CreateHippotherapyProgramTests.csVictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/HippotherapyProgramCategories/CreateHippotherapyProgramCategoryTests.csVictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyPrograms/Delete/DeleteHippotherapyProgramHandler.csVictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/HippotherapyPrograms/UpdateHippotherapyProgramTests.csVictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamCategories/CreateCategoryTests.csVictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Donate/UahBankDetails/CreateUahBankDetailsTests.csVictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/CreateTeamMemberTests.csVictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyProgramCategories/Delete/DeleteHippotherapyProgramCategoryHandler.csVictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamCategories/Delete/DeleteTeamCategoryHandler.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.UnitTests/MediatRHandlersTests/TeamCategories/UpdateCategoryTests.csVictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/HippotherapyProgramCategories/CreateHippotherapyProgramCategoryTests.csVictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamCategories/CreateCategoryTests.csVictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/CreateTeamMemberTests.csVictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamCategories/Create/CreateTeamCategoryHandler.csVictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.csVictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamCategories/Delete/DeleteTeamCategoryHandler.cs
📚 Learning: 2025-09-09T19:00:28.697Z
Learnt from: roman-stozhuk
PR: ita-social-projects/VictoryCenter-Back#349
File: VictoryCenter/VictoryCenter.BLL/Commands/Admin/FaqQuestions/Create/CreateFaqQuestionHandler.cs:37-46
Timestamp: 2025-09-09T19:00:28.697Z
Learning: In the VictoryCenter project, duplicate validation for collections like PageIds is handled in FluentValidation validators using .Must(collection => collection.Distinct().Count() == collection.Count) with appropriate error messages from ErrorMessagesConstants.
Applied to files:
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamCategories/UpdateCategoryTests.csVictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/HippotherapyPrograms/UpdateHippotherapyProgramTests.cs
📚 Learning: 2025-06-20T18:50:30.605Z
Learnt from: VladimirSushinsky
PR: ita-social-projects/VictoryCenter-Back#177
File: VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/DeleteTeamMemberTests.cs:47-47
Timestamp: 2025-06-20T18:50:30.605Z
Learning: In VictoryCenter project, when writing unit tests for DeleteTeamMemberCommand, the parameter types should use `long` to match the command constructor and entity ID type. InlineData attributes should provide `long` literals (e.g., -1L, 0L) rather than int values.
Applied to files:
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/CreateTeamMemberTests.csVictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamCategories/Delete/DeleteTeamCategoryHandler.csVictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Delete/DeleteTeamMemberHandler.cs
🧬 Code graph analysis (16)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/FaqQuestions/UpdateFaqQuestionTests.cs (2)
VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToUpdateEntityInDatabase(40-45)VictoryCenter/VictoryCenter.DAL/Entities/FaqQuestion.cs (1)
FaqQuestion(6-15)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/HippotherapyPrograms/CreateHippotherapyProgramTests.cs (1)
VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)PropertyIsRequired(83-86)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamCategories/UpdateCategoryTests.cs (1)
VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)PropertyIsRequired(83-86)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyPrograms/Create/CreateHippotherapyProgramHandler.cs (4)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyProgramCategories/Create/CreateHippotherapyProgramCategoryHandler.cs (1)
Task(25-40)VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyPrograms/Update/UpdateHippotherapyProgramHandler.cs (1)
Task(26-80)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (3)
ErrorMessagesConstants(3-129)ReorderingContainsInvalidIds(61-66)FailedToCreateEntity(19-24)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/HippotherapyProgramCategories/CreateHippotherapyProgramCategoryTests.cs (2)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamCategories/CreateCategoryTests.cs (1)
SetupRepositoryWrapper(123-127)VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/CreateTeamMemberTests.cs (1)
SetupRepositoryWrapper(166-186)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyPrograms/Delete/DeleteHippotherapyProgramHandler.cs (3)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyProgramCategories/Delete/DeleteHippotherapyProgramCategoryHandler.cs (1)
Task(19-48)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToDeleteEntity(47-52)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/HippotherapyPrograms/UpdateHippotherapyProgramTests.cs (1)
VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)PropertyIsRequired(83-86)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamCategories/CreateCategoryTests.cs (1)
VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)PropertyIsRequired(83-86)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyProgramCategories/Create/CreateHippotherapyProgramCategoryHandler.cs (2)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyPrograms/Create/CreateHippotherapyProgramHandler.cs (1)
Task(26-73)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyPrograms/Update/UpdateHippotherapyProgramHandler.cs (3)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyProgramCategories/Update/UpdateHippotherapyProgramCategoryHandler.cs (1)
Task(26-53)VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyPrograms/Create/CreateHippotherapyProgramHandler.cs (1)
Task(26-73)
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/CreateTeamMemberTests.cs (4)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.cs (3)
CreateTeamMemberHandler(15-78)CreateTeamMemberHandler(22-30)Task(32-77)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (4)
ErrorMessagesConstants(3-129)FailedToCreateEntityInDatabase(26-31)NotFound(7-10)NotFound(12-17)VictoryCenter/VictoryCenter.DAL/Repositories/Interfaces/Base/IRepositoryBase.cs (7)
Task(10-10)Task(12-12)Task(14-14)Task(16-16)Task(18-18)Task(32-33)Task(35-35)VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (2)
Task(12-31)Task(33-33)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyProgramCategories/Update/UpdateHippotherapyProgramCategoryHandler.cs (2)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToUpdateEntity(33-38)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyProgramCategories/Delete/DeleteHippotherapyProgramCategoryHandler.cs (3)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyPrograms/Create/CreateHippotherapyProgramHandler.cs (1)
Task(26-73)VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyPrograms/Delete/DeleteHippotherapyProgramHandler.cs (1)
Task(19-42)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamCategories/Delete/DeleteTeamCategoryHandler.cs (4)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyProgramCategories/Delete/DeleteHippotherapyProgramCategoryHandler.cs (1)
Task(19-48)VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyPrograms/Delete/DeleteHippotherapyProgramHandler.cs (1)
Task(19-42)VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Delete/DeleteTeamMemberHandler.cs (1)
Task(18-46)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Update/UpdateTeamMemberHandler.cs (3)
VictoryCenter/VictoryCenter.BLL/Services/ReorderService/ReorderService.cs (2)
ReorderService(14-162)ReorderService(18-21)VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (3)
BaseHandler(8-34)Task(12-31)Task(33-33)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToUpdateEntity(33-38)
VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Delete/DeleteTeamMemberHandler.cs (7)
VictoryCenter/VictoryCenter.BLL/Commands/Base/BaseHandler.cs (1)
BaseHandler(8-34)VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyProgramCategories/Delete/DeleteHippotherapyProgramCategoryHandler.cs (1)
Task(19-48)VictoryCenter/VictoryCenter.BLL/Commands/Admin/HippotherapyPrograms/Delete/DeleteHippotherapyProgramHandler.cs (1)
Task(19-42)VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Create/CreateTeamMemberHandler.cs (1)
Task(32-77)VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Update/UpdateTeamMemberHandler.cs (1)
Task(34-113)VictoryCenter/VictoryCenter.DAL/Entities/TeamMember.cs (1)
TeamMember(7-25)VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs (2)
ErrorMessagesConstants(3-129)FailedToDeleteEntityInDatabase(54-59)
⏰ 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.UnitTests/MediatRHandlersTests/FaqQuestions/UpdateFaqQuestionTests.cs (1)
206-235: Test correctly reflects production exception handling behavior.Verification confirms the BaseHandler properly catches
DbUpdateExceptionand preserves the exception's message in the result (lines 23-26 of BaseHandler.cs):catch (DbUpdateException ex) { return Result.Fail<TResponse>(ex.Message); }The test accurately validates this behavior by:
- Setting up the mock to throw
DbUpdateExceptionwith the error message fromErrorMessagesConstants.FailedToUpdateEntityInDatabase(typeof(FaqQuestion))- Verifying the exception's message flows through to
result.Errors[0].MessageThe test change aligns perfectly with the PR's centralized exception handling pattern and provides valuable coverage of database-level exceptions alongside logical failures tested in the adjacent test method.
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/Donate/UahBankDetails/CreateUahBankDetailsTests.cs (1)
133-138: CancellationToken addition is correct but incomplete without full test refactor.The repository mock correctly includes
CancellationTokenin theCreateAsyncsignature, aligning with the broader codebase changes. However, updating only this line while leaving the tests in the oldResult<T>pattern creates an inconsistent state. Once the handler is fully migrated toBaseHandler, ensure all test assertions and expectations are updated accordingly.VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamMembers/CreateTeamMemberTests.cs (1)
73-73: LGTM: Handler instantiation correctly updated.The handler instantiation has been properly updated across all test methods to remove the
IReorderServicedependency, aligning with the handler refactoring.Also applies to: 89-89, 108-108, 137-137
VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamMembers/Update/UpdateTeamMemberHandler.cs (1)
34-112: LGTM: Comprehensive update logic with proper transaction management.The handler properly implements the BaseHandler pattern with:
- Validation using
ValidateAndThrowAsync- Entity existence checking with appropriate exception throwing
- Category validation when changed
- Transaction scope with proper completion
- Priority reordering when category changes
- Image loading for the response DTO
The implementation follows the established patterns from other handlers in the codebase and properly handles the complex scenario of category changes with priority reordering.
VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamCategories/Create/CreateTeamCategoryHandler.cs (1)
37-37: Good catch on cancellation token propagation. PassingcancellationTokenintoCreateAsynckeeps the repository call cancellable and aligned with the updated signature.VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamCategories/UpdateCategoryTests.cs (1)
99-101: Sharper validation assertion. Verifying the concretePropertyIsRequiredmessage locks the test to the expected validator output and prevents message regressions.VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/TeamCategories/CreateCategoryTests.cs (2)
88-90: Clearer failure expectation. Asserting againstPropertyIsRequiredmakes the negative path explicit and keeps the validator contract front and center.
125-125: Mock updated for new signature. Expecting theCancellationTokenargument ensures the mock tracks the repository’s revised API shape.VictoryCenter/VictoryCenter.BLL/Commands/Admin/TeamCategories/Delete/DeleteTeamCategoryHandler.cs (1)
10-45: Base handler migration looks solid. The override cleanly raises typed exceptions for not-found and in-use cases, reuses existing constants, and surfaces the deleted Id on success—exactly what the shared BaseHandler pattern expects.
Issue ticket link
Summary of issue
Summary of change
Testing approach
Expected result
Check List
Summary by CodeRabbit
Release Notes