Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces a complete voucher management feature for the backend, including a REST API controller with create and update endpoints, DTOs, service layer implementation with business logic, repository layer for data access, and validation rules using FluentValidation. The feature spans from API controllers through the application and infrastructure layers with dependency injection setup. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as VoucherController
participant Service as VoucherService
participant Repository as VoucherRepository
participant Database as AppDbContext
Client->>Controller: POST /api/admin/vouchers<br/>(CreateVoucherDto)
Controller->>Service: CreateVoucherAsync(dto, token)
Service->>Service: Map DTO to Voucher entity<br/>Apply defaults
Service->>Repository: Add(voucher)
Service->>Database: SaveChangesAsync(token)
Database-->>Service: Persisted
Service-->>Controller: ErrorOr<Created>
Controller-->>Client: 201 Created / 400 BadRequest
sequenceDiagram
participant Client
participant Controller as VoucherController
participant Service as VoucherService
participant Repository as VoucherRepository
participant Database as AppDbContext
Client->>Controller: PUT /api/admin/vouchers/{guid}<br/>(UpdateVoucherDto)
Controller->>Service: UpdateVoucherAsync(guid, dto, token)
Service->>Repository: GetVoucherByGuid(guid)
Repository->>Database: Query Vouchers by PublicId
Database-->>Repository: Voucher entity (or null)
Repository-->>Service: IQueryable<Voucher>
Service->>Service: FirstOrDefaultAsync()
alt Voucher not found
Service-->>Controller: Error.NotFound
Controller-->>Client: 404 NotFound
else Voucher found
Service->>Service: Update entity fields<br/>Set UpdatedAt = UtcNow
Service->>Repository: Update(voucher)
Service->>Database: SaveChangesAsync(token)
Database-->>Service: Persisted
Service-->>Controller: ErrorOr<Updated>
Controller-->>Client: 200 OK / 400 BadRequest
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🧹 Nitpick comments (8)
Backend/src/ShoeStore.Application/Interface/VoucherInterface/IVoucherService.cs (1)
8-11: Remove commented-out interface members.Keep the public service contract focused on implemented operations; track the future CRUD methods in an issue instead of commented stubs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend/src/ShoeStore.Application/Interface/VoucherInterface/IVoucherService.cs` around lines 8 - 11, In IVoucherService remove the four commented-out interface members (GetVoucherByGuidAsync, GetAllVouchersAsync, UpdateVoucherAsync, DeleteVoucherAsync) so the public contract only contains implemented methods; delete those commented lines from the IVoucherService interface file and, if you want to track them for later, create an issue/ticket instead of leaving commented stubs in the codebase.Backend/src/ShoeStore.Application/DTOs/VoucherDtos/UpdateVoucherDto.cs (1)
8-14: Keep partial-update fields nullable and audit timestamps server-owned.
MaxPriceDiscount = 0,IsDeleted = false, and client-settableUpdateAtmake it easy to overwrite existing data when a field is simply omitted. Set audit values in the service layer and keep optional update fields nullable.♻️ Suggested DTO adjustment
- public decimal MaxPriceDiscount { get; set; } - public DateTime UpdateAt { get; set; } = DateTime.UtcNow; + public decimal? MaxPriceDiscount { get; set; } public DateTime? ValidFrom { get; set; } public DateTime? ValidTo { get; set; } public int? MaxUsagePerUser { get; set; } public int? TotalQuantity { get; set; } - public bool? IsDeleted { get; set; } = false; + public bool? IsDeleted { get; set; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend/src/ShoeStore.Application/DTOs/VoucherDtos/UpdateVoucherDto.cs` around lines 8 - 14, Update the UpdateVoucherDto so optional update fields are nullable and audit timestamps are server-owned: change MaxPriceDiscount to decimal? (nullable) and IsDeleted to bool? (nullable) and remove the client-settable UpdateAt (either remove the property or make it internal/handled only by the service), and remove any default values (e.g., = false or = DateTime.UtcNow) from the DTO; then ensure the service layer (e.g., Voucher update handler) sets UpdateAt = DateTime.UtcNow and applies partial updates only when nullable DTO fields (MaxPriceDiscount, IsDeleted, ValidFrom, ValidTo, MaxUsagePerUser, TotalQuantity) are non-null.Backend/src/ShoeStore.Application/DTOs/VoucherDtos/ResponseVoucherDto.cs (1)
7-10: Avoid shipping an empty public response DTO.If this DTO is returned later, clients will only receive
{}. Either add the voucher response fields now or remove the DTO until an endpoint actually needs it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend/src/ShoeStore.Application/DTOs/VoucherDtos/ResponseVoucherDto.cs` around lines 7 - 10, The ResponseVoucherDto class is empty; either populate it with the actual response properties or remove it until needed. Fix by updating the ResponseVoucherDto DTO to include the voucher response fields used by your API (e.g., properties such as Id, Code, DiscountAmount or Percentage, ValidFrom, ValidTo, IsActive or whatever the domain requires) so clients receive meaningful JSON, or delete ResponseVoucherDto and refactor any controllers/services that reference it to use the correct existing DTO/entity until you define the final response shape.Backend/src/ShoeStore.Application/Interface/VoucherInterface/IVoucherRepository.cs (1)
8-9: These methods are currently unused; if implemented, prefer async materialized results overIQueryable.
GetAllVouchersandGetVoucherByGuidare defined but not called anywhere in the codebase. When they're implemented, follow the established async pattern across other repositories:Task<IReadOnlyList<Voucher>> GetAllVouchersAsync(CancellationToken token)andTask<Voucher?> GetVoucherByGuidAsync(Guid voucherGuid, CancellationToken token). ReturningIQueryabledefers execution, cancellation, and DbContext lifetime handling to callers; materialized results provide cleaner encapsulation and align withICategoryRepository,IColorRepository,ISizeRepository, and similar patterns in the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend/src/ShoeStore.Application/Interface/VoucherInterface/IVoucherRepository.cs` around lines 8 - 9, The interface IVoucherRepository currently exposes IQueryable methods GetAllVouchers and GetVoucherByGuid; change these to async, materialized signatures to match the repository pattern used elsewhere by replacing GetAllVouchers() with Task<IReadOnlyList<Voucher>> GetAllVouchersAsync(CancellationToken token) and GetVoucherByGuid(Guid voucherGuid) with Task<Voucher?> GetVoucherByGuidAsync(Guid voucherGuid, CancellationToken token), and update implementations to materialize queries with cancellation support (e.g., ToListAsync/FirstOrDefaultAsync) so callers don't handle IQueryable/DbContext lifetime.Backend/src/ShoeStore.Application/Services/VoucherService.cs (2)
44-62: Remove commented-out stubs.These commented
DeleteVoucherAsync/GetAllVouchersAsync/GetVoucherByGuidAsync/UpdateVoucherAsyncblocks are dead code. Track unimplemented methods via an issue/TODO rather than committed commented code; source control preserves history.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend/src/ShoeStore.Application/Services/VoucherService.cs` around lines 44 - 62, Remove the commented-out stub methods in VoucherService.cs (the commented DeleteVoucherAsync, GetAllVouchersAsync, GetVoucherByGuidAsync, and UpdateVoucherAsync blocks); delete those commented blocks entirely and, if you need to track the missing implementations, replace them with a single TODO comment or create a ticket/issue and reference its ID in a short TODO line (e.g., "// TODO: implement voucher operations - ISSUE-1234") so history is preserved by source control while keeping the codebase clean.
22-36: Silent defaulting of nullable required fields masks validation failures.
VoucherName ?? string.Empty,Discount ?? 0,TotalQuantity ?? 0,MinOrderPrice ?? 0will happily create a voucher with an empty name or zero discount/quantity if validation is ever bypassed (e.g., validator not wired, or a new code path). SinceCreateVoucherDtomodels these as nullable, the service should either reject nulls explicitly or rely on strongly-typed (non-nullable) DTO properties. Recommended: make required fields non-nullable in the DTO so the compiler/validator enforces presence and this service doesn't need to coalesce.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend/src/ShoeStore.Application/Services/VoucherService.cs` around lines 22 - 36, The current object initializer in VoucherService that builds the Voucher from voucherCreateDto silently coalesces nullable fields (voucherCreateDto.VoucherName, Discount, TotalQuantity, MinOrderPrice) to defaults; instead either make those properties non-nullable on CreateVoucherDto so the compiler/validation enforces them, or add explicit null/invalid checks in VoucherService (e.g., in the method that constructs the Voucher) and throw a clear exception (ArgumentException/ValidationException) when VoucherName is null/empty or numeric fields are null/invalid before creating the Voucher instance; locate the initialization in the VoucherService method that references voucherCreateDto and update it to validate and reject nulls rather than using ?? defaults.Backend/src/ShoeStore.Infrastructure/Repositories/VoucherRepository.cs (1)
14-17: Consider returning a single entity for GUID lookup.
GetVoucherByGuidreturnsIQueryable<Voucher>for what is logically a single-entity lookup (PublicId should be unique). Callers have to callFirstOrDefaultAsyncthemselves, and theIQueryableshape leaks EF concerns into the application layer. ConsiderTask<Voucher?> GetVoucherByGuidAsync(Guid voucherGuid, CancellationToken token)instead. Also, style-wiseAsNoTracking()is typically applied beforeWhere(...)(functionally equivalent here but more conventional).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend/src/ShoeStore.Infrastructure/Repositories/VoucherRepository.cs` around lines 14 - 17, Change the repository method GetVoucherByGuid to return a single entity asynchronously: replace the IQueryable-returning GetVoucherByGuid with Task<Voucher?> GetVoucherByGuidAsync(Guid voucherGuid, CancellationToken cancellationToken) that queries context.Vouchers.AsNoTracking().Where(v => v.PublicId == voucherGuid).FirstOrDefaultAsync(cancellationToken); update any callers to await the new async method and remove any .FirstOrDefaultAsync usage there; keep AsNoTracking() applied before Where for style/convention and ensure you include the CancellationToken in the EF Core call.Backend/src/ShoeStore.Api/Controllers/VoucherController.cs (1)
47-53:Createdresponse lacks a resource location and identifier.
Created("", new { message = ... })emits a 201 with an emptyLocationheader and does not return the newly created voucher's identifier (e.g.,PublicId). Consumers cannot discover the created resource. Consider returning the new voucher'sPublicIdfromCreateVoucherAsync(e.g.,ErrorOr<Guid>or a small result DTO) and usingCreatedAtAction(nameof(GetVoucherByGuid), new { id = ... }, ...)once a GET endpoint exists, or at minimum include thePublicIdin the response body.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend/src/ShoeStore.Api/Controllers/VoucherController.cs` around lines 47 - 53, The Created response in VoucherController currently uses Created("", ...) which sends an empty Location header and omits the created voucher identifier; update the CreateVoucherAsync flow (or the method returning result) to return the new voucher PublicId (e.g., change CreateVoucherAsync to return ErrorOr<Guid> or a DTO containing PublicId), then replace Created("", ...) with CreatedAtAction(nameof(GetVoucherByGuid), new { id = publicId }, new { message = "Voucher created successfully", publicId }) or at minimum include publicId in the response body so consumers can discover the resource.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Backend/src/ShoeStore.Application/DTOs/VoucherDtos/CreateVoucherDto.cs`:
- Around line 13-24: The setters for ValidFrom and ValidTo currently use
DateTime.SpecifyKind which only labels the value as UTC without converting the
instant; update the setters in CreateVoucherDto (properties ValidFrom/ValidTo,
backing fields _validFrom/_validTo) to convert incoming non-null DateTime values
to UTC using ToUniversalTime() (e.g., set _validFrom = value.HasValue ?
value.Value.ToUniversalTime() : null) so timestamps are actually normalized to
UTC while preserving null handling.
In `@Backend/src/ShoeStore.Application/Services/VoucherService.cs`:
- Around line 27-28: The CreateVoucherDtoValidation is missing enum validation,
so unvalidated int values are being cast into enums in VoucherService when
assigning VoucherScope and DiscountType; update CreateVoucherDtoValidation to
add IsInEnum() rules for the VoucherScope and DiscountType properties (e.g.,
RuleFor(x => x.VoucherScope).IsInEnum().WithMessage(...); RuleFor(x =>
x.DiscountType).IsInEnum().WithMessage(...)) to ensure invalid ints are rejected
before VoucherService performs the casts.
In
`@Backend/src/ShoeStore.Application/Validations/VoucherValidation/CreateVoucherDtoValidation.cs`:
- Around line 10-19: Add validations for enum and numeric constraints: ensure
VoucherScope and DiscountType use IsInEnum() (e.g., RuleFor(x =>
x.VoucherScope).IsInEnum().WithMessage(...), RuleFor(x =>
x.DiscountType).IsInEnum().WithMessage(...)); add a conditional rule that when
DiscountType == DiscountType.Percentage the Discount value is <= 100 (RuleFor(x
=> x.Discount).LessThanOrEqualTo(100).When(x => x.DiscountType ==
DiscountType.Percentage).WithMessage(...)); and enforce non-negative constraints
for MaxPriceDiscount, MinOrderPrice and MaxUsagePerUser (e.g., RuleFor(x =>
x.MaxPriceDiscount).GreaterThanOrEqualTo(0)..., RuleFor(x =>
x.MinOrderPrice).GreaterThanOrEqualTo(0)..., RuleFor(x =>
x.MaxUsagePerUser).GreaterThanOrEqualTo(0)...). Use the
CreateVoucherDtoValidation class and the property names VoucherScope,
DiscountType, Discount, MaxPriceDiscount, MinOrderPrice, and MaxUsagePerUser to
locate where to add these rules.
---
Nitpick comments:
In `@Backend/src/ShoeStore.Api/Controllers/VoucherController.cs`:
- Around line 47-53: The Created response in VoucherController currently uses
Created("", ...) which sends an empty Location header and omits the created
voucher identifier; update the CreateVoucherAsync flow (or the method returning
result) to return the new voucher PublicId (e.g., change CreateVoucherAsync to
return ErrorOr<Guid> or a DTO containing PublicId), then replace Created("",
...) with CreatedAtAction(nameof(GetVoucherByGuid), new { id = publicId }, new {
message = "Voucher created successfully", publicId }) or at minimum include
publicId in the response body so consumers can discover the resource.
In `@Backend/src/ShoeStore.Application/DTOs/VoucherDtos/ResponseVoucherDto.cs`:
- Around line 7-10: The ResponseVoucherDto class is empty; either populate it
with the actual response properties or remove it until needed. Fix by updating
the ResponseVoucherDto DTO to include the voucher response fields used by your
API (e.g., properties such as Id, Code, DiscountAmount or Percentage, ValidFrom,
ValidTo, IsActive or whatever the domain requires) so clients receive meaningful
JSON, or delete ResponseVoucherDto and refactor any controllers/services that
reference it to use the correct existing DTO/entity until you define the final
response shape.
In `@Backend/src/ShoeStore.Application/DTOs/VoucherDtos/UpdateVoucherDto.cs`:
- Around line 8-14: Update the UpdateVoucherDto so optional update fields are
nullable and audit timestamps are server-owned: change MaxPriceDiscount to
decimal? (nullable) and IsDeleted to bool? (nullable) and remove the
client-settable UpdateAt (either remove the property or make it internal/handled
only by the service), and remove any default values (e.g., = false or =
DateTime.UtcNow) from the DTO; then ensure the service layer (e.g., Voucher
update handler) sets UpdateAt = DateTime.UtcNow and applies partial updates only
when nullable DTO fields (MaxPriceDiscount, IsDeleted, ValidFrom, ValidTo,
MaxUsagePerUser, TotalQuantity) are non-null.
In
`@Backend/src/ShoeStore.Application/Interface/VoucherInterface/IVoucherRepository.cs`:
- Around line 8-9: The interface IVoucherRepository currently exposes IQueryable
methods GetAllVouchers and GetVoucherByGuid; change these to async, materialized
signatures to match the repository pattern used elsewhere by replacing
GetAllVouchers() with Task<IReadOnlyList<Voucher>>
GetAllVouchersAsync(CancellationToken token) and GetVoucherByGuid(Guid
voucherGuid) with Task<Voucher?> GetVoucherByGuidAsync(Guid voucherGuid,
CancellationToken token), and update implementations to materialize queries with
cancellation support (e.g., ToListAsync/FirstOrDefaultAsync) so callers don't
handle IQueryable/DbContext lifetime.
In
`@Backend/src/ShoeStore.Application/Interface/VoucherInterface/IVoucherService.cs`:
- Around line 8-11: In IVoucherService remove the four commented-out interface
members (GetVoucherByGuidAsync, GetAllVouchersAsync, UpdateVoucherAsync,
DeleteVoucherAsync) so the public contract only contains implemented methods;
delete those commented lines from the IVoucherService interface file and, if you
want to track them for later, create an issue/ticket instead of leaving
commented stubs in the codebase.
In `@Backend/src/ShoeStore.Application/Services/VoucherService.cs`:
- Around line 44-62: Remove the commented-out stub methods in VoucherService.cs
(the commented DeleteVoucherAsync, GetAllVouchersAsync, GetVoucherByGuidAsync,
and UpdateVoucherAsync blocks); delete those commented blocks entirely and, if
you need to track the missing implementations, replace them with a single TODO
comment or create a ticket/issue and reference its ID in a short TODO line
(e.g., "// TODO: implement voucher operations - ISSUE-1234") so history is
preserved by source control while keeping the codebase clean.
- Around line 22-36: The current object initializer in VoucherService that
builds the Voucher from voucherCreateDto silently coalesces nullable fields
(voucherCreateDto.VoucherName, Discount, TotalQuantity, MinOrderPrice) to
defaults; instead either make those properties non-nullable on CreateVoucherDto
so the compiler/validation enforces them, or add explicit null/invalid checks in
VoucherService (e.g., in the method that constructs the Voucher) and throw a
clear exception (ArgumentException/ValidationException) when VoucherName is
null/empty or numeric fields are null/invalid before creating the Voucher
instance; locate the initialization in the VoucherService method that references
voucherCreateDto and update it to validate and reject nulls rather than using ??
defaults.
In `@Backend/src/ShoeStore.Infrastructure/Repositories/VoucherRepository.cs`:
- Around line 14-17: Change the repository method GetVoucherByGuid to return a
single entity asynchronously: replace the IQueryable-returning GetVoucherByGuid
with Task<Voucher?> GetVoucherByGuidAsync(Guid voucherGuid, CancellationToken
cancellationToken) that queries context.Vouchers.AsNoTracking().Where(v =>
v.PublicId == voucherGuid).FirstOrDefaultAsync(cancellationToken); update any
callers to await the new async method and remove any .FirstOrDefaultAsync usage
there; keep AsNoTracking() applied before Where for style/convention and ensure
you include the CancellationToken in the EF Core call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fde2b7b4-ad97-42e0-9f47-77eb870f4d83
📒 Files selected for processing (11)
Backend/src/ShoeStore.Api/Controllers/VoucherController.csBackend/src/ShoeStore.Application/DTOs/VoucherDtos/CreateVoucherDto.csBackend/src/ShoeStore.Application/DTOs/VoucherDtos/ResponseVoucherDto.csBackend/src/ShoeStore.Application/DTOs/VoucherDtos/UpdateVoucherDto.csBackend/src/ShoeStore.Application/Interface/VoucherInterface/IVoucherRepository.csBackend/src/ShoeStore.Application/Interface/VoucherInterface/IVoucherService.csBackend/src/ShoeStore.Application/Services/VoucherService.csBackend/src/ShoeStore.Application/ShoeStore.Application.csprojBackend/src/ShoeStore.Application/Validations/VoucherValidation/CreateVoucherDtoValidation.csBackend/src/ShoeStore.Infrastructure/DependencyInjection/DependencyInjection.csBackend/src/ShoeStore.Infrastructure/Repositories/VoucherRepository.cs
| public DateTime? ValidFrom | ||
| { | ||
| get => _validFrom; | ||
| set => _validFrom = value.HasValue ? DateTime.SpecifyKind(value.Value, DateTimeKind.Utc) : null; | ||
| } | ||
|
|
||
| private DateTime? _validTo; | ||
| public DateTime? ValidTo | ||
| { | ||
| get => _validTo; | ||
| set => _validTo = value.HasValue ? DateTime.SpecifyKind(value.Value, DateTimeKind.Utc) : null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Microsoft .NET DateTime.SpecifyKind documentation: does SpecifyKind convert the DateTime value to UTC or only set the Kind property?
💡 Result:
DateTime.SpecifyKind does not convert the DateTime value to UTC. It only sets the Kind property while keeping the same number of ticks (time value) unchanged. The official documentation explicitly states: "The returned DateTime value does not represent the same instant in time as the value parameter, and SpecifyKind is not a time zone conversion method. Instead, it leaves the time specified by the value parameter unchanged, and sets the Kind property to kind." For time zone conversions, use methods like ToUniversalTime or ToLocalTime.
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/system.datetime.specifykind?view=net-10.0
- 2: https://msdn.microsoft.com/en-us/library/system.datetime.specifykind
- 3: https://msdn.microsoft.com/library/system.datetime.specifykind
🏁 Script executed:
# First, locate and inspect the CreateVoucherDto.cs file
find . -name "CreateVoucherDto.cs" -type fRepository: tqha1011/ShoeStore
Length of output: 134
🏁 Script executed:
# Search for DateTime.SpecifyKind usage across the codebase
rg "SpecifyKind" -t cs -C 3Repository: tqha1011/ShoeStore
Length of output: 5521
🏁 Script executed:
# Search for similar DateTime assignment patterns in DTOs
rg "DateTime\.SpecifyKind|DateTimeKind\.Utc" -t cs --glob "**/*Dto.cs"Repository: tqha1011/ShoeStore
Length of output: 419
Use ToUniversalTime() to convert non-UTC times, not SpecifyKind().
Lines 16 and 23 use DateTime.SpecifyKind(), which only sets metadata without converting the timestamp. If clients send local or offset-based times, they get relabeled as UTC without any actual conversion, causing vouchers to expire at the wrong instant.
🛠️ Proper UTC normalization
+ private static DateTime? NormalizeUtc(DateTime? value)
+ {
+ if (!value.HasValue)
+ {
+ return null;
+ }
+
+ return value.Value.Kind == DateTimeKind.Unspecified
+ ? DateTime.SpecifyKind(value.Value, DateTimeKind.Utc)
+ : value.Value.ToUniversalTime();
+ }
+
private DateTime? _validFrom;
public DateTime? ValidFrom
{
get => _validFrom;
- set => _validFrom = value.HasValue ? DateTime.SpecifyKind(value.Value, DateTimeKind.Utc) : null;
+ set => _validFrom = NormalizeUtc(value);
}
private DateTime? _validTo;
public DateTime? ValidTo
{
get => _validTo;
- set => _validTo = value.HasValue ? DateTime.SpecifyKind(value.Value, DateTimeKind.Utc) : null;
+ set => _validTo = NormalizeUtc(value);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Backend/src/ShoeStore.Application/DTOs/VoucherDtos/CreateVoucherDto.cs`
around lines 13 - 24, The setters for ValidFrom and ValidTo currently use
DateTime.SpecifyKind which only labels the value as UTC without converting the
instant; update the setters in CreateVoucherDto (properties ValidFrom/ValidTo,
backing fields _validFrom/_validTo) to convert incoming non-null DateTime values
to UTC using ToUniversalTime() (e.g., set _validFrom = value.HasValue ?
value.Value.ToUniversalTime() : null) so timestamps are actually normalized to
UTC while preserving null handling.
| VoucherScope = (VoucherScope)voucherCreateDto.VoucherScope, | ||
| DiscountType = (DiscountType)voucherCreateDto.DiscountType, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd CreateVoucherDtoValidation.cs --exec cat {}
fd CreateVoucherDto.cs --exec cat {}Repository: tqha1011/ShoeStore
Length of output: 2141
Unvalidated int→enum casts persist invalid enum values.
Verified: CreateVoucherDtoValidation contains no validation for VoucherScope or DiscountType. Both fields lack IsInEnum() rules, allowing any int to silently cast to an enum on lines 27–28, corrupting voucher records.
Add to CreateVoucherDtoValidation:
RuleFor(x => x.VoucherScope)
.IsInEnum().WithMessage("Invalid VoucherScope value.");
RuleFor(x => x.DiscountType)
.IsInEnum().WithMessage("Invalid DiscountType value.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Backend/src/ShoeStore.Application/Services/VoucherService.cs` around lines 27
- 28, The CreateVoucherDtoValidation is missing enum validation, so unvalidated
int values are being cast into enums in VoucherService when assigning
VoucherScope and DiscountType; update CreateVoucherDtoValidation to add
IsInEnum() rules for the VoucherScope and DiscountType properties (e.g.,
RuleFor(x => x.VoucherScope).IsInEnum().WithMessage(...); RuleFor(x =>
x.DiscountType).IsInEnum().WithMessage(...)) to ensure invalid ints are rejected
before VoucherService performs the casts.
| RuleFor(x => x.VoucherName) | ||
| .NotEmpty().WithMessage("Voucher name is required") | ||
| .MaximumLength(100).WithMessage("Voucher name must not exceed 100 characters"); | ||
|
|
||
| RuleFor(x => x.Discount) | ||
| .NotNull().WithMessage("Discount is required") | ||
| .GreaterThanOrEqualTo(0).WithMessage("Discount must be greater than or equal to 0"); | ||
|
|
||
| RuleFor(x => x.TotalQuantity) | ||
| .GreaterThan(0).WithMessage("Total quantity must be greater than 0"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find the domain/entity definitions and usages for voucher scope/type/value fields.
# Expected: Use the discovered allowed values to replace the placeholder Must(...) checks above.
rg -n -C3 --type=cs '\b(VoucherScope|DiscountType|MaxPriceDiscount|MinOrderPrice|MaxUsagePerUser)\b'Repository: tqha1011/ShoeStore
Length of output: 50374
🏁 Script executed:
fd -n "CreateVoucherDto" --type fRepository: tqha1011/ShoeStore
Length of output: 292
🏁 Script executed:
fd "CreateVoucherDto" --type fRepository: tqha1011/ShoeStore
Length of output: 226
🏁 Script executed:
cat -n "Backend/src/ShoeStore.Application/DTOs/VoucherDtos/CreateVoucherDto.cs"Repository: tqha1011/ShoeStore
Length of output: 1334
🏁 Script executed:
fd "Voucher" --type f | grep -E "(Entity|Domain|Model)" | head -20Repository: tqha1011/ShoeStore
Length of output: 268
🏁 Script executed:
cat -n "Backend/src/ShoeStore.Domain/Entities/Voucher.cs"Repository: tqha1011/ShoeStore
Length of output: 1532
🏁 Script executed:
cat -n "Backend/src/ShoeStore.Domain/Enum/VoucherScope.cs"Repository: tqha1011/ShoeStore
Length of output: 207
🏁 Script executed:
fd "DiscountType" --type fRepository: tqha1011/ShoeStore
Length of output: 111
🏁 Script executed:
cat -n "Backend/src/ShoeStore.Domain/Enum/DiscountType.cs"Repository: tqha1011/ShoeStore
Length of output: 213
🏁 Script executed:
cat -n "Backend/src/ShoeStore.Application/Validations/VoucherValidation/CreateVoucherDtoValidation.cs"Repository: tqha1011/ShoeStore
Length of output: 1253
🏁 Script executed:
cat -n "Backend/src/ShoeStore.Application/Validations/VoucherValidation/CreateVoucherDtoValidation.cs" | head -30Repository: tqha1011/ShoeStore
Length of output: 1253
Add missing validation rules for voucher enum fields and economic constraints.
The validation is incomplete. VoucherScope and DiscountType accept arbitrary integers, percentage discounts can exceed 100%, and negative values are allowed for MaxPriceDiscount, MinOrderPrice, and MaxUsagePerUser. These should be rejected at the DTO boundary.
🛡️ Add missing validation rules
+ RuleFor(x => x.VoucherScope)
+ .Must(value => value is 1 or 2)
+ .WithMessage("Voucher scope must be 1 (Product) or 2 (Shipping)");
+
+ RuleFor(x => x.DiscountType)
+ .Must(value => value is 1 or 2)
+ .WithMessage("Discount type must be 1 (FixedAmount) or 2 (Percentage)");
+
RuleFor(x => x.Discount)
.NotNull().WithMessage("Discount is required")
.GreaterThanOrEqualTo(0).WithMessage("Discount must be greater than or equal to 0");
+
+ RuleFor(x => x.Discount)
+ .LessThanOrEqualTo(100).WithMessage("Percentage discount must not exceed 100")
+ .When(x => x.DiscountType == 2);
+
+ RuleFor(x => x.MaxPriceDiscount)
+ .GreaterThanOrEqualTo(0).WithMessage("Max price discount must be greater than or equal to 0");
+
+ RuleFor(x => x.MaxUsagePerUser)
+ .GreaterThan(0).WithMessage("Max usage per user must be greater than 0")
+ .When(x => x.MaxUsagePerUser.HasValue);
+
+ RuleFor(x => x.MinOrderPrice)
+ .GreaterThanOrEqualTo(0).WithMessage("Minimum order price must be greater than or equal to 0")
+ .When(x => x.MinOrderPrice.HasValue);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@Backend/src/ShoeStore.Application/Validations/VoucherValidation/CreateVoucherDtoValidation.cs`
around lines 10 - 19, Add validations for enum and numeric constraints: ensure
VoucherScope and DiscountType use IsInEnum() (e.g., RuleFor(x =>
x.VoucherScope).IsInEnum().WithMessage(...), RuleFor(x =>
x.DiscountType).IsInEnum().WithMessage(...)); add a conditional rule that when
DiscountType == DiscountType.Percentage the Discount value is <= 100 (RuleFor(x
=> x.Discount).LessThanOrEqualTo(100).When(x => x.DiscountType ==
DiscountType.Percentage).WithMessage(...)); and enforce non-negative constraints
for MaxPriceDiscount, MinOrderPrice and MaxUsagePerUser (e.g., RuleFor(x =>
x.MaxPriceDiscount).GreaterThanOrEqualTo(0)..., RuleFor(x =>
x.MinOrderPrice).GreaterThanOrEqualTo(0)..., RuleFor(x =>
x.MaxUsagePerUser).GreaterThanOrEqualTo(0)...). Use the
CreateVoucherDtoValidation class and the property names VoucherScope,
DiscountType, Discount, MaxPriceDiscount, MinOrderPrice, and MaxUsagePerUser to
locate where to add these rules.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Backend/src/ShoeStore.Api/Controllers/VoucherController.cs (1)
48-69: Avoid mapping all non-404 service errors to 400.Both endpoints collapse service failures into
BadRequest, which hides conflicts or unexpected server errors and diverges from the existing controller patterns that map byErrorType/code.Proposed direction
return result.Match<IActionResult>( _ => Created("", new { message = "Voucher created successfully" }), - errors => BadRequest(new - { - message = "Failed to create voucher", - details = errors - })); + errors => errors[0].Type switch + { + ErrorType.Conflict => Conflict(new { message = "Failed to create voucher", details = errors }), + ErrorType.Validation => BadRequest(new { message = "Failed to create voucher", details = errors }), + _ => StatusCode(StatusCodes.Status500InternalServerError, + new { message = "Failed to create voucher", details = errors }) + });Apply the same pattern to
UpdateVoucher, keepingErrorType.NotFound => NotFound(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend/src/ShoeStore.Api/Controllers/VoucherController.cs` around lines 48 - 69, The UpdateVoucher endpoint currently maps all non-NotFound errors to BadRequest; change the error handling for voucherService.UpdateVoucherAsync in UpdateVoucher (and mirror CreateVoucher if applicable) to map ErrorType values explicitly (e.g., if any error.Type == ErrorType.NotFound return NotFound(...); if any error.Type == ErrorType.Conflict return Conflict(...); otherwise return StatusCode(500, ...) or BadRequest depending on ErrorType) so conflicts and server errors are not misclassified; use the existing errors collection and ErrorType enum to decide the correct IActionResult and include error details where appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Backend/src/ShoeStore.Api/Controllers/VoucherController.cs`:
- Around line 14-17: The controller has its [Authorize(Roles = "Admin")]
attribute commented out, exposing admin voucher endpoints; re-enable the
attribute by restoring [Authorize(Roles = "Admin")] on the VoucherController
(class VoucherController) or alternatively apply it to the specific mutation
actions (e.g., Create/Update methods in VoucherController) so that only users in
the Admin role can call the admin voucher endpoints.
In `@Backend/src/ShoeStore.Application/Services/VoucherService.cs`:
- Around line 64-78: The update logic in VoucherService.cs is overwriting
existing voucher fields with DTO defaults because fields like VoucherScope,
DiscountType, and MaxPriceDiscount are non-nullable on voucherUpdateDto and are
always written; also Discount is never updated. Make the corresponding
properties on the update DTO nullable (e.g., int? VoucherScope, int?
DiscountType, decimal? MaxPriceDiscount, and include a nullable Discount
property if intended), then change VoucherService.Update mapping to only assign
when the DTO value.HasValue or DTO string is non-null (e.g., check
voucherUpdateDto.VoucherScope.HasValue before casting and assigning to
voucher.VoucherScope), and add mapping for Discount from voucherUpdateDto when
provided; this preserves existing values on partial updates.
- Around line 52-54: In UpdateVoucherAsync, the database read call using
repository.GetVoucherByGuid(...).FirstOrDefaultAsync() does not pass the
CancellationToken parameter, so the read cannot be cancelled; update the call to
pass the provided CancellationToken token into FirstOrDefaultAsync(token) (or
the appropriate overload) so the cancellation token is honored during the query
before you later call SaveChangesAsync(token).
---
Nitpick comments:
In `@Backend/src/ShoeStore.Api/Controllers/VoucherController.cs`:
- Around line 48-69: The UpdateVoucher endpoint currently maps all non-NotFound
errors to BadRequest; change the error handling for
voucherService.UpdateVoucherAsync in UpdateVoucher (and mirror CreateVoucher if
applicable) to map ErrorType values explicitly (e.g., if any error.Type ==
ErrorType.NotFound return NotFound(...); if any error.Type == ErrorType.Conflict
return Conflict(...); otherwise return StatusCode(500, ...) or BadRequest
depending on ErrorType) so conflicts and server errors are not misclassified;
use the existing errors collection and ErrorType enum to decide the correct
IActionResult and include error details where appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1bea080a-5a90-4a81-8bba-d018b0f15727
📒 Files selected for processing (4)
Backend/src/ShoeStore.Api/Controllers/VoucherController.csBackend/src/ShoeStore.Application/DTOs/VoucherDtos/UpdateVoucherDto.csBackend/src/ShoeStore.Application/Interface/VoucherInterface/IVoucherService.csBackend/src/ShoeStore.Application/Services/VoucherService.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Backend/src/ShoeStore.Application/Interface/VoucherInterface/IVoucherService.cs
- Backend/src/ShoeStore.Application/DTOs/VoucherDtos/UpdateVoucherDto.cs
| [ApiController] | ||
| [Route("api/admin/vouchers")] | ||
| // [Authorize(Roles = "Admin")] | ||
| public class VoucherController(IVoucherService voucherService) : ControllerBase |
There was a problem hiding this comment.
Re-enable Admin authorization on voucher mutations.
The route and docs say this is Admin-only, but [Authorize(Roles = "Admin")] is commented out, making voucher creation/update publicly reachable if the app has no other gate in front of this controller.
Proposed fix
[ApiController]
[Route("api/admin/vouchers")]
-// [Authorize(Roles = "Admin")]
+[Authorize(Roles = "Admin")]
public class VoucherController(IVoucherService voucherService) : ControllerBase📝 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.
| [ApiController] | |
| [Route("api/admin/vouchers")] | |
| // [Authorize(Roles = "Admin")] | |
| public class VoucherController(IVoucherService voucherService) : ControllerBase | |
| [ApiController] | |
| [Route("api/admin/vouchers")] | |
| [Authorize(Roles = "Admin")] | |
| public class VoucherController(IVoucherService voucherService) : ControllerBase |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Backend/src/ShoeStore.Api/Controllers/VoucherController.cs` around lines 14 -
17, The controller has its [Authorize(Roles = "Admin")] attribute commented out,
exposing admin voucher endpoints; re-enable the attribute by restoring
[Authorize(Roles = "Admin")] on the VoucherController (class VoucherController)
or alternatively apply it to the specific mutation actions (e.g., Create/Update
methods in VoucherController) so that only users in the Admin role can call the
admin voucher endpoints.
| var voucher = await repository | ||
| .GetVoucherByGuid(voucherGuid) | ||
| .FirstOrDefaultAsync(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify no-token EF async query calls in C# files.
# Expected: voucher query uses FirstOrDefaultAsync(token) or an equivalent CancellationToken overload.
rg -nP --type cs -C2 '\.FirstOrDefaultAsync\s*\(\s*\)'Repository: tqha1011/ShoeStore
Length of output: 545
🏁 Script executed:
cat -n Backend/src/ShoeStore.Application/Services/VoucherService.cs | head -100Repository: tqha1011/ShoeStore
Length of output: 3990
🏁 Script executed:
rg -n 'FirstOrDefaultAsync\s*\(\s*\)' Backend/src/ShoeStore.Application/Services/VoucherService.csRepository: tqha1011/ShoeStore
Length of output: 104
Pass the cancellation token to the database query.
The UpdateVoucherAsync method accepts a CancellationToken token parameter but doesn't pass it to FirstOrDefaultAsync() on line 54. Without the token, the database read operation cannot be cancelled, even though the save operation on line 82 correctly uses it.
Proposed fix
var voucher = await repository
.GetVoucherByGuid(voucherGuid)
- .FirstOrDefaultAsync();
+ .FirstOrDefaultAsync(token);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Backend/src/ShoeStore.Application/Services/VoucherService.cs` around lines 52
- 54, In UpdateVoucherAsync, the database read call using
repository.GetVoucherByGuid(...).FirstOrDefaultAsync() does not pass the
CancellationToken parameter, so the read cannot be cancelled; update the call to
pass the provided CancellationToken token into FirstOrDefaultAsync(token) (or
the appropriate overload) so the cancellation token is honored during the query
before you later call SaveChangesAsync(token).
| // Update logic | ||
| voucher.VoucherDescription = voucherUpdateDto.VoucherDescription ?? voucher.VoucherDescription; | ||
|
|
||
| voucher.VoucherScope = (VoucherScope)voucherUpdateDto.VoucherScope; | ||
| voucher.DiscountType = (DiscountType)voucherUpdateDto.DiscountType; | ||
|
|
||
| voucher.MaxPriceDiscount = voucherUpdateDto.MaxPriceDiscount; | ||
|
|
||
| voucher.ValidFrom = voucherUpdateDto.ValidFrom ?? voucher.ValidFrom; | ||
| voucher.ValidTo = voucherUpdateDto.ValidTo ?? voucher.ValidTo; | ||
|
|
||
| voucher.MaxUsagePerUser = voucherUpdateDto.MaxUsagePerUser ?? voucher.MaxUsagePerUser; | ||
| voucher.TotalQuantity = voucherUpdateDto.TotalQuantity ?? voucher.TotalQuantity; | ||
| voucher.MinOrderPrice = voucherUpdateDto.MinOrderPrice ?? voucher.MinOrderPrice; | ||
|
|
There was a problem hiding this comment.
Fix update mapping before it corrupts existing voucher fields.
Discount is never updated, while omitted non-nullable fields like VoucherScope, DiscountType, and MaxPriceDiscount are written with DTO defaults. A description-only update can silently reset existing values.
Proposed direction
// Update logic
voucher.VoucherDescription = voucherUpdateDto.VoucherDescription ?? voucher.VoucherDescription;
+ voucher.Discount = voucherUpdateDto.Discount ?? voucher.Discount;
- voucher.VoucherScope = (VoucherScope)voucherUpdateDto.VoucherScope;
- voucher.DiscountType = (DiscountType)voucherUpdateDto.DiscountType;
+ if (voucherUpdateDto.VoucherScope.HasValue)
+ voucher.VoucherScope = (VoucherScope)voucherUpdateDto.VoucherScope.Value;
- voucher.MaxPriceDiscount = voucherUpdateDto.MaxPriceDiscount;
+ if (voucherUpdateDto.DiscountType.HasValue)
+ voucher.DiscountType = (DiscountType)voucherUpdateDto.DiscountType.Value;
+
+ voucher.MaxPriceDiscount = voucherUpdateDto.MaxPriceDiscount ?? voucher.MaxPriceDiscount;This also requires making the update DTO value fields nullable, for example:
public int? VoucherScope { get; set; }
public int? DiscountType { get; set; }
public decimal? MaxPriceDiscount { get; set; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Backend/src/ShoeStore.Application/Services/VoucherService.cs` around lines 64
- 78, The update logic in VoucherService.cs is overwriting existing voucher
fields with DTO defaults because fields like VoucherScope, DiscountType, and
MaxPriceDiscount are non-nullable on voucherUpdateDto and are always written;
also Discount is never updated. Make the corresponding properties on the update
DTO nullable (e.g., int? VoucherScope, int? DiscountType, decimal?
MaxPriceDiscount, and include a nullable Discount property if intended), then
change VoucherService.Update mapping to only assign when the DTO value.HasValue
or DTO string is non-null (e.g., check voucherUpdateDto.VoucherScope.HasValue
before casting and assigning to voucher.VoucherScope), and add mapping for
Discount from voucherUpdateDto when provided; this preserves existing values on
partial updates.
|
| [ProducesResponseType(typeof(object), StatusCodes.Status404NotFound)] | ||
| [ProducesResponseType(typeof(object), StatusCodes.Status500InternalServerError)] | ||
| [HttpGet("user/{userGuid}")] | ||
| public async Task<IActionResult> GetVouchersForUser(Guid userGuid, CancellationToken token) |
There was a problem hiding this comment.
cái này k cần phải là tham số, lấy từ jwt token dùng find first value
|
|
||
| namespace ShoeStore.Application.Services | ||
| { | ||
| public class VoucherService : IVoucherService, IUserVoucherService |
| }) | ||
| .ToListAsync(token); | ||
|
|
||
| if(vouchers == null || !vouchers.Any()) |
2 participants
Summary by CodeRabbit