Added CreateThread endpoint#31
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds full thread and thread-like features: new API controllers, DTOs, service interfaces and implementations, DI registrations, repository paging/count methods, enum additions, comment endpoints, and package/AutoMapper registration changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant API as ThreadController
participant USvc as IUserService
participant Svc as IThreadService
participant DAL as Repository/UoW
rect #F5F8FF
note over User,API: Create Thread
User->>API: POST /api/threads/thread
API->>USvc: GetCurrentUserAsync()
USvc-->>API: current user
API->>Svc: AddThread(askerId, CreateThreadDto)
Svc->>DAL: Validate users, BeginTx, Save Thread
DAL-->>Svc: persisted thread
Svc->>DAL: CommitTx
Svc-->>API: ServiceResult<ThreadResponseDto>
API-->>User: 200 / 400
end
rect #F5FFF5
note over User,API: Answer Thread
User->>API: PUT /api/threads/{id}/answer
API->>USvc: GetCurrentUserAsync()
API->>Svc: AnswerThread(id, userId, AnswerDto)
Svc->>DAL: Load thread, authorize, update, CommitTx
Svc-->>API: ServiceResult<ThreadResponseDto>
API-->>User: 200 / 400
end
sequenceDiagram
autonumber
actor User
participant API as ThreadLikeController
participant USvc as IUserService
participant LikeSvc as IThreadLikeService
participant DAL as Repository/UoW
rect #FFF8F5
note over User,API: Like / Unlike / Get Likes
User->>API: POST /api/threads/{id}/likes
API->>USvc: GetCurrentUserAsync()
API->>LikeSvc: AddLike(threadId, userId)
LikeSvc->>DAL: Load thread+likes, upsert like, CommitTx
LikeSvc-->>API: ServiceResult<ThreadLikeResponseDto>
API-->>User: 200 / 400
User->>API: GET /api/threads/{id}/likes
API->>LikeSvc: GetLikes(id)
LikeSvc->>DAL: Load likes
LikeSvc-->>API: ServiceResult<List<...>>
API-->>User: 200 / 400
User->>API: DELETE /api/threads/{id}/likes
API->>USvc: GetCurrentUserAsync()
API->>LikeSvc: RemoveLike(id, userId)
LikeSvc->>DAL: Remove like, CommitTx
LikeSvc-->>API: ServiceResult<bool>
API-->>User: 200 / 400
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
AskFm/AskFm.BLL/Services/ThreadService.cs (1)
13-18: Inject the logger with the correct categoryLines 13-16 currently request
ILogger<CommentLikeService>, so any future log entries from this class would be tagged under the wrong category. Switching toILogger<ThreadService>keeps logging metadata accurate and avoids coupling this service to another one.- private readonly ILogger<CommentLikeService> _logger; + private readonly ILogger<ThreadService> _logger; - public ThreadService(IUnitOfWork unitOfWork, ILogger<CommentLikeService> logger) - { - _unitOfWork = unitOfWork; - _logger = logger; - } + public ThreadService(IUnitOfWork unitOfWork, ILogger<ThreadService> logger) + { + _unitOfWork = unitOfWork; + _logger = logger; + }AskFm/AskFm.API/Controllers/ThreadController.cs (1)
19-26: Inject logger with the correct categoryThe constructor requests
ILogger<CommentController>, so every log entry from this controller will be tagged with the wrong category. Switch it toILogger<ThreadController>to keep log routing consistent.Apply this diff:
- ILogger<CommentController> logger, + ILogger<ThreadController> logger,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
AskFm/AskFm.API/Controllers/ThreadController.cs(1 hunks)AskFm/AskFm.API/Program.cs(1 hunks)AskFm/AskFm.BLL/DTO/CreateThreadDto.cs(1 hunks)AskFm/AskFm.BLL/DTO/ThreadResponseDto.cs(1 hunks)AskFm/AskFm.BLL/Services/IThreadService.cs(1 hunks)AskFm/AskFm.BLL/Services/ThreadService.cs(1 hunks)
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
AskFm/AskFm.API/Controllers/ThreadLikeController.cs (1)
13-24: Inject the correct logger categoryLine 13 injects
ILogger<CommentController>, so every log fromThreadLikeControlleris misclassified. Swap the generic argument to match this controller.- private readonly ILogger<CommentController> _logger; + private readonly ILogger<ThreadLikeController> _logger; @@ - ILogger<CommentController> logger, + ILogger<ThreadLikeController> logger,AskFm/AskFm.BLL/Services/ThreadService.cs (1)
12-19: Log ThreadService events under the right categoryLine 12 asks for
ILogger<CommentLikeService>, so all ThreadService logs end up under the wrong category. RequestILogger<ThreadService>instead.- private readonly ILogger<CommentLikeService> _logger; + private readonly ILogger<ThreadService> _logger; @@ - public ThreadService(IUnitOfWork unitOfWork, ILogger<CommentLikeService> logger) + public ThreadService(IUnitOfWork unitOfWork, ILogger<ThreadService> logger)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
AskFm/AskFm.API/Controllers/ThreadController.cs(1 hunks)AskFm/AskFm.API/Controllers/ThreadLikeController.cs(1 hunks)AskFm/AskFm.BLL/DTO/ThreadAnswerDto.cs(1 hunks)AskFm/AskFm.BLL/DTO/ThreadResponseDto.cs(1 hunks)AskFm/AskFm.BLL/Services/IThreadService.cs(1 hunks)AskFm/AskFm.BLL/Services/ThreadService.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- AskFm/AskFm.BLL/DTO/ThreadResponseDto.cs
- AskFm/AskFm.API/Controllers/ThreadController.cs
- AskFm/AskFm.BLL/Services/IThreadService.cs
🔇 Additional comments (1)
AskFm/AskFm.BLL/Services/ThreadService.cs (1)
1-19: Remove the unused_mapperfield to keep the build greenLine 13 still declares the readonly
_mapperfield without ever assigning it in the constructor, so the class will not compile (CS0191). Drop the field (and the now-unneeded AutoMapper using) unless you plan to inject and use it.-using AutoMapper; @@ - private readonly IMapper _mapper; - - public ThreadService(IUnitOfWork unitOfWork, ILogger<CommentLikeService> logger) + public ThreadService(IUnitOfWork unitOfWork, ILogger<CommentLikeService> logger)
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
AskFm/AskFm.API/Controllers/ThreadLikeController.cs (1)
15-27: Log under the correct categoryInjecting
ILogger<CommentController>here writes logs under the wrong category. Swap it forILogger<ThreadLikeController>so diagnostics stay discoverable.AskFm/AskFm.API/Controllers/ThreadController.cs (1)
15-27: Use the correct logger typeThe controller still asks for
ILogger<CommentController>, so all log entries show up underCommentController. Switch toILogger<ThreadController>for accurate categorisation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
AskFm/AskFm.API/Controllers/ThreadController.cs(1 hunks)AskFm/AskFm.API/Controllers/ThreadLikeController.cs(1 hunks)AskFm/AskFm.API/Program.cs(1 hunks)AskFm/AskFm.BLL/DTO/ThreadAnswerDto.cs(1 hunks)AskFm/AskFm.BLL/DTO/ThreadLikeResponseDto.cs(1 hunks)AskFm/AskFm.BLL/Services/IThreadLikeService.cs(1 hunks)AskFm/AskFm.BLL/Services/ThreadLikeService.cs(1 hunks)AskFm/AskFm.BLL/Services/ThreadService.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- AskFm/AskFm.API/Program.cs
| [Route("api/[controller]")] | ||
| [Authorize(AuthenticationSchemes = "Bearer")] | ||
| public class ThreadLikeController : ControllerBase | ||
| { | ||
| private readonly ILogger<CommentController> _logger; | ||
| private readonly IUserService _userService; | ||
| private readonly IThreadLikeService _threadLikeService; | ||
|
|
||
| public ThreadLikeController( | ||
| ILogger<CommentController> logger, | ||
| IUserService userService, | ||
| IThreadLikeService threadService) | ||
| { | ||
| _logger = logger; | ||
| _userService = userService; | ||
| _threadLikeService = threadService; | ||
| } | ||
|
|
||
|
|
||
| // POST api/threads/{id}/likes - Add a Like to the thread with id = {id} | ||
| [HttpPost] | ||
| [Route("threads/{id}/likes")] | ||
| public async Task<IActionResult> LikeThread([FromRoute] int id) | ||
| { | ||
| var user = await _userService.GetCurrentUserAsync(); | ||
| if (!user.success) | ||
| { | ||
| return BadRequest(user.Errors); | ||
| } | ||
| var res = await _threadLikeService.AddLike(id, user.Data.Id); | ||
| if (!res.success) | ||
| { | ||
| return BadRequest(res.Errors); | ||
| } | ||
| return Ok(res.Data); | ||
|
|
||
| } | ||
|
|
||
|
|
||
| // GET api/threads/{id}/likes - Get all the Likes to the thread with id = {id} | ||
| [HttpGet] | ||
| [Route("threads/{id}/likes")] | ||
| public async Task<IActionResult> GetLikes([FromRoute] int id) | ||
| { | ||
| var likes = await _threadLikeService.GetLikes(id); | ||
| if (!likes.success) | ||
| { | ||
| return BadRequest(likes.Errors); | ||
| } | ||
| return Ok(likes.Data); | ||
| } | ||
|
|
||
| // DELETE api/threads/{id}/likes - Unlike to the thread with id = {id} | ||
| [HttpDelete] | ||
| [Route("threads/{id}/likes")] | ||
| public async Task<IActionResult> UnlikeThread([FromRoute] int id) |
There was a problem hiding this comment.
Fix the route templates
The base route [Route("api/[controller]")] resolves to api/ThreadLike, so these actions register as api/ThreadLike/threads/{id}/likes, not the documented api/threads/{id}/likes. Clients calling the documented paths will hit 404s. Please either change the controller route to api/threads or make each action route absolute (e.g., [Route("/api/threads/{id}/likes")]).
🤖 Prompt for AI Agents
In AskFm/AskFm.API/Controllers/ThreadLikeController.cs around lines 11 to 66 the
controller is annotated with [Route("api/[controller]")] which causes endpoints
to be registered under api/ThreadLike/threads/{id}/likes instead of the intended
api/threads/{id}/likes; fix by either changing the controller-level route to
[Route("api/threads")] so action routes become relative to api/threads, or make
each action route absolute (e.g., [Route("/api/threads/{id}/likes")]) — pick one
approach and update the attributes consistently for POST, GET and DELETE so the
routes match the documented api/threads/{id}/likes.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
AskFm/AskFm.API/AskFm.API.csproj (1)
10-26: Resolve conflicting Swashbuckle package versions.
Swashbuckle.AspNetCore.SwaggerUIis declared twice in this project file—Line 26 keeps 9.0.4 while Line 53 downgrades to 9.0.3. MSBuild rejects multiple<PackageReference>entries for the same package with different versions, sodotnet restorewill fail until one reference is removed or the versions are aligned.- <PackageReference Include="Swashbuckle.AspNetCore.SwaggerUI" Version="9.0.4" /> + <PackageReference Include="Swashbuckle.AspNetCore.SwaggerUI" Version="9.0.3" /> @@ - <PackageReference Include="Swashbuckle.AspNetCore.SwaggerUI" Version="9.0.3" />Also applies to: 53-54
AskFm/AskFm.BLL/Services/CommentLikeService.cs (2)
111-128: Reconsider updatingCreatedAton re-like.Updating
CreatedAtwhen a user re-likes a comment (after previously unliking it) is semantically questionable. TheCreatedAtfield typically represents the original creation timestamp and should be immutable. This change effectively makes it represent "last liked at" instead.Consider one of the following approaches:
- Keep the original
CreatedAtvalue to maintain historical accuracy- Add a separate
UpdatedAtorLastLikedAtfield if you need to track when the like was last activatedIf you decide to keep the original timestamp, apply this diff:
existingLike.IsDeleted = false; comment.LikeCount++; -existingLike.CreatedAt = DateTime.Now; _unitOfWork.Comments.Update(comment);
178-178: Fix typo in error message.The error message contains a stray
$character at the beginning.Apply this diff:
-"$User didn't like this comment" +"User didn't like this comment"
♻️ Duplicate comments (4)
AskFm/AskFm.BLL/Services/ThreadLikeService.cs (1)
57-64: Fix the timestamp update ordering.The code sets
existingLike.CreatedAt = DateTime.Nowon line 59 after callingAddon line 60,Updateon line 61, and beforeSaveAsyncon line 62. However, line 60 re-adds an entity that's already in the collection, which is unnecessary. More critically, settingCreatedAtafter adding it back might not persist correctly depending on EF tracking state.Apply this diff to fix the flow:
// otherwise , the user liked the comment , then unliked it , and then wants to like it again existingLike.IsDeleted = false; existingLike.CreatedAt = DateTime.Now; -thread.ThreadLikes.Add(existingLike); _unitOfWork.Threads.Update(thread); await _unitOfWork.SaveAsync(); await transaction.CommitAsync();The
Addis redundant becauseexistingLikeis already inthread.ThreadLikes(found viaFirstOrDefaulton line 43). Simply updating the properties and callingUpdateon the parent entity is sufficient.AskFm/AskFm.BLL/Services/ThreadService.cs (2)
234-235: Avoid unsafe nullable access on AskerId.Lines 234-235 use different patterns for null handling: line 234 safely uses
thread.AskerId ?? 0, but line 235 inconsistently checks anonymous status. Additionally, past review comments flagged unsafe navigation property access (thread.Asker.Nameinstead ofthread.Asker?.Name).The current code at lines 234-237 should consistently null-check navigation properties:
AskerId = thread.AskerId ?? 0, -AskerName = thread.isAnonymous ? "Anonymous" : thread.Asker?.Name, +AskerName = thread.isAnonymous ? "Anonymous" : (thread.Asker?.Name ?? "Unknown"), AskedId = thread.AskedId, -AskedName = thread.Asked?.Name +AskedName = thread.Asked?.Name ?? "Unknown"This ensures that when navigation properties are null, a fallback value is provided instead of returning null.
100-100: Guard against null Asker navigation property.Line 100 uses
thread.Asker?.Name ?? "Unknown", which is correct. However, line 99 accessesthread.AskerId.Valuewithout checking ifAskerIdis null. IfAskerIdis null, this throwsInvalidOperationException.Apply this diff:
-AskerName = thread.Asker?.Name ?? "Unknown", +AskerId = thread.AskerId ?? 0, +AskerName = thread.isAnonymous ? "Anonymous" : (thread.Asker?.Name ?? "Unknown"), AskedId = thread.AskedId, AskedName = thread.Asked?.Name ?? "Unknown"This also adds the anonymous check for consistency with other methods.
AskFm/AskFm.API/Controllers/ThreadController.cs (1)
11-11: Fix the controller base route to align with endpoint routes.The controller route uses
[controller]which producesapi/Thread(singular, capital T), but all individual endpoint routes and comments usethreads(plural, lowercase). This creates inconsistent URLs likeapi/Thread/threads/{id}instead of the expectedapi/threads/{id}.Apply this diff:
-[Route("api/[controller]")] +[Route("api/threads")]
🧹 Nitpick comments (11)
AskFm/AskFm.DAL/Interfaces/IRepository.cs (1)
2-2: Remove the unused using directive.The
using Microsoft.EntityFrameworkCore;directive is unnecessary in this interface file. None of the interface method signatures reference EntityFrameworkCore types—IQueryablecomes fromSystem.Linq, and the other types are from core .NET namespaces.Apply this diff to remove the unused using:
using System.Linq.Expressions; -using Microsoft.EntityFrameworkCore;AskFm/AskFm.BLL/Services/CommentLikeService.cs (2)
1-1: Remove unused import.The
System.Runtime.InteropServices.JavaScriptnamespace is imported but not used anywhere in the file.Apply this diff to remove the unused import:
-using System.Runtime.InteropServices.JavaScript; using AskFm.BLL.DTO;
15-16: Remove or initialize unused_mapperfield.The
_mapperfield is declared but never initialized in the constructor or used anywhere in the class. Either remove it or injectIMapperin the constructor and utilize AutoMapper for DTO mappings.Apply this diff to remove the unused field:
private IUnitOfWork _unitOfWork; private readonly ILogger<CommentLikeService> _logger; -private readonly IMapper _mapper; public CommentLikeService(IUnitOfWork unitOfWork, ILogger<CommentLikeService> logger)AskFm/AskFm.BLL/DTO/AnswerThreadDto.cs (1)
5-5: Consider adding validation for AnswerContent.The
AnswerContentproperty lacks validation. Since answering a thread with empty content is likely invalid, consider adding the[Required]attribute to enforce this at the model binding level.Add the validation attribute:
+using System.ComponentModel.DataAnnotations; + namespace AskFm.BLL.DTO; public class AnswerThreadDto { + [Required(ErrorMessage = "Answer content is required")] public string AnswerContent { get; set; } }AskFm/AskFm.BLL/DTO/PagedResponseDto.cs (1)
3-10: Initialize the Items collection to prevent null reference issues.The
Itemsproperty is not initialized, which can lead to null reference exceptions when consumers iterate over it without checking for null. Initialize it to an empty list by default.Apply this diff:
public class PagedResponseDto<T> { - public List<T> Items { get; set; } + public List<T> Items { get; set; } = new List<T>(); public int PageNumber { get; set; } public int PageSize { get; set; } public int TotalCount { get; set; } public int TotalPages { get; set; } }AskFm/AskFm.DAL/Enums/ThreadStatus.cs (1)
2-10: Standardize enum member naming to PascalCase.The enum uses inconsistent casing:
PRIVATE,PUBLIC,PRIVATEQUESTION(UPPERCASE) mixed withClosed,Answered,Pending(PascalCase). C# conventions recommend PascalCase for all enum members.Apply this diff to standardize naming:
public enum ThreadStatus { - PRIVATE, - PUBLIC, + Private, + Public, Closed, - PRIVATEQUESTION, + PrivateQuestion, Answered, Pending }Note: This is a breaking change that will require updates throughout the codebase. If the enum values are persisted or exposed via API, ensure backward compatibility or coordinate the migration carefully.
AskFm/AskFm.BLL/Services/ThreadLikeService.cs (1)
188-188: Use async commit to match the transaction pattern.Line 188 calls
transaction.Commit()(synchronous), but the rest of the codebase usesawait transaction.CommitAsync()(lines 63, 95). For consistency and to avoid blocking, use the async version.-transaction.Commit(); +await transaction.CommitAsync();AskFm/AskFm.BLL/Services/ThreadService.cs (1)
137-137: Ensure consistent anonymous handling.Line 137 checks
thread.isAnonymous ? "Anonymous"but line 100 (AddThread) doesn't include this check. For consistency, either all methods should apply the anonymous check or none should. Based on the business logic, whenisAnonymousis true, the asker's identity should be hidden.Verify the business requirement: should
AskerNamealways respect theisAnonymousflag? If yes, ensure the AutoMapper profile (suggested in the previous comment) or all manual mappings include this check consistently.AskFm/AskFm.API/Controllers/ThreadController.cs (3)
104-104: Validate pagination parameters.
pageandpageSizequery parameters default to 1 and 10 but accept any integer value. Negative or zero values could cause service errors or unexpected behavior.Apply input validation before calling services:
public async Task<IActionResult> GetThreads([FromQuery] int page = 1, [FromQuery] int pageSize = 10) { + if (page < 1 || pageSize < 1) + { + return BadRequest(new { message = "Page and pageSize must be greater than 0" }); + } + var threads = await _threadService.GetThreads(page, pageSize);Apply similar validation to
GetFeed(line 137) andGetSavedThreads(line 197).Also applies to: 137-137, 197-197
15-15: Consider using the injected logger.The logger is injected but never used throughout the controller. Adding structured logging for key operations (thread creation, deletion, errors) improves observability.
Example usage:
_logger.LogInformation("Creating thread for user {UserId}", userId); _logger.LogError("Failed to create thread: {Errors}", string.Join(", ", result.Errors));Also applies to: 24-24
36-41: Extract repeated user retrieval into a helper method.The pattern of fetching the current user and checking success is repeated in 7 methods. Extracting this into a private helper reduces duplication and improves maintainability.
Add a helper method:
private async Task<(bool success, int userId, IActionResult? errorResult)> GetCurrentUserIdAsync() { var user = await _userService.GetCurrentUserAsync(); if (!user.success) { return (false, 0, BadRequest(user.Errors)); } return (true, user.Data.Id, null); }Then simplify each method:
var (success, userId, errorResult) = await GetCurrentUserIdAsync(); if (!success) return errorResult!; // Continue with userId...Also applies to: 86-90, 119-123, 139-143, 159-163, 179-183, 199-203
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
AskFm/AskFm.API/AskFm.API.csproj(1 hunks)AskFm/AskFm.API/Controllers/ThreadController.cs(1 hunks)AskFm/AskFm.API/Program.cs(1 hunks)AskFm/AskFm.BLL/AskFm.BLL.csproj(1 hunks)AskFm/AskFm.BLL/DTO/AnswerThreadDto.cs(1 hunks)AskFm/AskFm.BLL/DTO/PagedResponseDto.cs(1 hunks)AskFm/AskFm.BLL/DTO/ThreadLikeResponseDto.cs(1 hunks)AskFm/AskFm.BLL/DTO/ThreadResponseDto.cs(1 hunks)AskFm/AskFm.BLL/Services/CommentLikeService.cs(1 hunks)AskFm/AskFm.BLL/Services/IThreadService.cs(1 hunks)AskFm/AskFm.BLL/Services/ThreadLikeService.cs(1 hunks)AskFm/AskFm.BLL/Services/ThreadService.cs(1 hunks)AskFm/AskFm.DAL/Enums/ThreadStatus.cs(1 hunks)AskFm/AskFm.DAL/Interfaces/IRepository.cs(1 hunks)AskFm/AskFm.DAL/Repositories/Repository.cs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- AskFm/AskFm.API/Program.cs
🧰 Additional context used
🪛 GitHub Check: build-and-test
AskFm/AskFm.DAL/Repositories/Repository.cs
[warning] 72-72:
Cannot convert null literal to non-nullable reference type.
[warning] 57-57:
Cannot convert null literal to non-nullable reference type.
AskFm/AskFm.DAL/Interfaces/IRepository.cs
[warning] 22-22:
Cannot convert null literal to non-nullable reference type.
[warning] 21-21:
Cannot convert null literal to non-nullable reference type.
[warning] 17-17:
Cannot convert null literal to non-nullable reference type.
[warning] 17-17:
Cannot convert null literal to non-nullable reference type.
[warning] 16-16:
Cannot convert null literal to non-nullable reference type.
🔇 Additional comments (10)
AskFm/AskFm.DAL/Interfaces/IRepository.cs (2)
36-36: LGTM! Proper nullable handling for the count method.The
CountAsyncmethod signature correctly uses nullable syntax for the optional predicate parameter, improving upon the nullable reference handling in some pre-existing methods in this interface.
38-44: LGTM! Required ordering ensures deterministic pagination.The
GetPagedAsyncmethod signature is well-designed for reliable pagination. Notably, theorderByparameter is required (non-nullable), which is a best practice—deterministic ordering is essential for consistent paging results across requests. The nullable syntax for optional parameters (predicate,includes) is properly applied.AskFm/AskFm.DAL/Repositories/Repository.cs (1)
114-124: LGTM!The
CountAsyncmethod implementation is clean and correct. It properly handles the optional predicate and follows the existing code patterns in the repository.AskFm/AskFm.BLL/DTO/ThreadResponseDto.cs (1)
5-20: LGTM!The DTO structure is well-designed with appropriate nullable types for optional fields (
AnswerContent,AskerId,AskerName,AskedName,SavedAt) and consistent PascalCase naming throughout. The properties accurately represent the thread response data needed across the API surface.AskFm/AskFm.BLL/Services/ThreadLikeService.cs (2)
16-21: LGTM!The constructor properly injects and assigns all dependencies (
IUnitOfWork,ILogger,IMapper). Dependencies are correctly stored for use throughout the service.
137-143: Update property references after DTO refactoring.After renaming
ThreadLikeResponseDtoproperties to PascalCase (see separate comment on that file), update the mappings here to useUserId,ThreadId, andCreatedAtinstead ofuserId,threadId, andcreatedAt.Verify that this change is coordinated with the DTO refactoring. Once the DTO is updated, apply this diff:
var likes = thread.ThreadLikes.Select(like => new ThreadLikeResponseDto { - threadId = like.ThreadId, - userId = like.UserId, + ThreadId = like.ThreadId, + UserId = like.UserId, UserName = like.User?.Name ?? "Unknown", - createdAt = like.CreatedAt + CreatedAt = like.CreatedAt }).ToList();AskFm/AskFm.BLL/Services/ThreadService.cs (2)
17-22: LGTM!The constructor properly injects and assigns all dependencies. The
IMapperinstance is now correctly wired up through dependency injection.
340-395: Validate that followed users exist before querying threads.The
GetFeedmethod constructsfollowedUserIdsby querying follows and adding the currentuserId. If the user has no follows, this results in a feed containing only their own threads. While this might be intentional, consider whether an empty follow list should return an empty feed instead or provide a helpful message.Clarify the business requirement: should a user with no follows see only their own threads or an empty feed? If the latter, add a check:
if (followedUserIds.Count == 1 && followedUserIds[0] == userId) { // Return empty feed or informational message return await ServiceResult<PagedResponseDto<ThreadResponseDto>>.Success( new PagedResponseDto<ThreadResponseDto> { Items = new List<ThreadResponseDto>(), PageNumber = page, PageSize = pageSize, TotalCount = 0, TotalPages = 0 }); }AskFm/AskFm.API/Controllers/ThreadController.cs (2)
136-136: Potential route conflict: literal "feed" vs route parameter {id}.The route
threads/feed(line 136) could conflict withthreads/{id}(line 69) depending on the routing engine's evaluation order. ASP.NET Core typically matches literal segments before parameters, but explicitly ordering the routes (via route constraint or placingGetFeedbeforeGetThreadWithIdin code) makes intent clearer and prevents future ambiguity.Verify that ASP.NET Core routes the literal "feed" correctly and doesn't interpret it as an ID. If you encounter routing issues, consider:
- Adding an
[Order(0)]attribute to the GetFeed action to ensure it's evaluated first- Adding a route constraint to GetThreadWithId like
[Route("threads/{id:int}")]
57-65: Verify authorization for resource access and modification.
GetAllThreadsretrieves threads for any user ID without checking if the current user has permission to view them. Similarly,DeleteThreadcalls the service without controller-level checks ensuring the current user owns the thread or has admin rights. If authorization is entirely delegated to the service layer, this is acceptable; otherwise, sensitive operations should have explicit controller-level guards.Confirm that
IThreadService.DeleteThreadand other methods enforce ownership/permission checks internally. If not, add authorization logic at the controller level or via a policy-based authorization attribute.Also applies to: 117-132
| <ProjectReference Include="..\Shared\Shared.csproj"/> | ||
| </ItemGroup> | ||
| <ItemGroup> | ||
| <PackageReference Include="AutoMapper" Version="13.0.1" /> |
There was a problem hiding this comment.
Keep AutoMapper aligned at 15.0.1
This line downgrades us from AutoMapper 15.0.1 to 13.0.1. The rest of the solution still targets 15.0.1, so this will either surface NU1605 downgrade warnings during restore or, worse, load the wrong assembly version at runtime. Please revert to 15.0.1 (or bump every consumer to 13.0.1 if you truly need the downgrade) so the package versions stay consistent.
- <PackageReference Include="AutoMapper" Version="13.0.1" />
+ <PackageReference Include="AutoMapper" Version="15.0.1" />📝 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.
| <PackageReference Include="AutoMapper" Version="13.0.1" /> | |
| <PackageReference Include="AutoMapper" Version="15.0.1" /> |
🤖 Prompt for AI Agents
In AskFm/AskFm.BLL/AskFm.BLL.csproj around line 12, the PackageReference pins
AutoMapper to 13.0.1 which downgrades the solution from the expected 15.0.1;
change the Version attribute back to 15.0.1 so the project matches the rest of
the solution, save the csproj, and run a package restore/build to verify no
NU1605 warnings remain.
| var responseDto = new ThreadResponseDto | ||
| { | ||
| Id = thread.Id, | ||
| QuestionContent = thread.QuestionContent, | ||
| Status = thread.Status, | ||
| IsAnonymous = thread.isAnonymous, | ||
| CreatedAt = thread.CreatedAt, | ||
| AskerId = thread.AskerId.Value, | ||
| AskerName = thread.Asker?.Name ?? "Unknown", | ||
| AskedId = thread.AskedId, | ||
| AskedName = thread.Asked?.Name ?? "Unknown" | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract ThreadResponseDto mapping to reduce duplication.
This DTO mapping pattern is duplicated across at least 7 methods (lines 92-103, 128-142, 168-182, 226-238, 267-281, 363-377, 497-512). Since IMapper is already injected, leverage it to eliminate this code smell.
Create an AutoMapper profile:
public class ThreadMappingProfile : Profile
{
public ThreadMappingProfile()
{
CreateMap<Thread, ThreadResponseDto>()
.ForMember(dest => dest.AskerName,
opt => opt.MapFrom(src => src.isAnonymous ? "Anonymous" : (src.Asker != null ? src.Asker.Name : null)))
.ForMember(dest => dest.AskedName,
opt => opt.MapFrom(src => src.Asked != null ? src.Asked.Name : null))
.ForMember(dest => dest.LikesCount,
opt => opt.MapFrom(src => src.ThreadLikes != null ? src.ThreadLikes.Count : 0))
.ForMember(dest => dest.CommentsCount,
opt => opt.MapFrom(src => src.Comments != null ? src.Comments.Count : 0));
}
}Then replace all manual mappings with:
-var responseDto = new ThreadResponseDto
-{
- Id = thread.Id,
- QuestionContent = thread.QuestionContent,
- ...
-};
+var responseDto = _mapper.Map<ThreadResponseDto>(thread);This reduces cognitive complexity, eliminates duplication, and ensures consistency across all mapping locations.
🤖 Prompt for AI Agents
In AskFm/AskFm.BLL/Services/ThreadService.cs around lines 92-103, the manual
construction of ThreadResponseDto should be replaced by AutoMapper to remove
duplicated mapping logic; create a ThreadMappingProfile (register it) that maps
Thread to ThreadResponseDto handling AskerName (return "Anonymous" when
isAnonymous, otherwise Asker?.Name or "Unknown"), AskedName (Asked?.Name or
"Unknown"), LikesCount and CommentsCount, then replace the manual new
ThreadResponseDto { ... } usages with _mapper.Map<ThreadResponseDto>(thread)
across all occurrences (lines 92-103, 128-142, 168-182, 226-238, 267-281,
363-377, 497-512) and ensure the profile is added to DI so the injected IMapper
resolves correctly.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
AskFm/AskFm.BLL/Services/UserIdentityService/UserService.cs (1)
143-144: Remove duplicate assignment.Line 144 duplicates the assignment from line 143. The second
followExist.IsDeleted = true;is redundant.Apply this diff to remove the duplicate:
{ followExist.IsDeleted = true; - followExist.IsDeleted = true; followExist.IsActive = false;
♻️ Duplicate comments (1)
AskFm/AskFm.API/Controllers/ThreadController.cs (1)
10-117: Align ThreadController routes with the /api/threads contractUsing
[Route("api/[controller]")]registers everything under/api/Thread/..., yet the XML comments and consumers expect/api/threads/.... This mismatch breaks documented clients. Please set the controller route toapi/threadsand remove the redundantthread/threadsprefixes on the action templates so the endpoints land exactly where intended.-[Route("api/[controller]")] +[Route("api/threads")] ... - [HttpPost] - [Route("thread")] + [HttpPost] ... - [HttpGet] - [Route("thread/{id}")] + [HttpGet("users/{id}")] ... - [HttpGet] - [Route("threads/{id}")] + [HttpGet("{id}")] ... - [HttpGet] - [Route("threads")] + [HttpGet]Apply the same cleanup to the other actions (
feed,save,saved) as needed so their paths become/api/threads/feed,/api/threads/{id}/save, etc.Also applies to: 101-212
🧹 Nitpick comments (6)
AskFm/AskFm.BLL/Services/UserIdentityService/UserService.cs (2)
154-154: Use async commit for consistency.This method uses the synchronous
transaction.Commit()while other transaction commits in this file useawait transaction.CommitAsync()(lines 109, 176). Using the async version maintains consistency and avoids blocking I/O.Apply this diff:
- transaction.Commit(); + await transaction.CommitAsync();
256-258: Remove commented-out code.The commented-out code on lines 256-258 should be removed. If this logic is needed for future implementation, track it in an issue or add a clear TODO comment instead.
Apply this diff:
var res = await CheckNullObjectAsync<ReadUserDTO, ApplicationUser>(userApp); if(!res.success) return res; - //var emailResult = _userManager.GenerateChangeEmailTokenAsync(); - - // throw new NotImplementedException();AskFm/AskFm.BLL/Services/ThreadService.cs (4)
13-13: Make_unitOfWorkreadonly.The
_unitOfWorkfield is never reassigned after construction, so it should be markedreadonlyfor consistency with_loggerand_mapper.Apply this diff:
- private IUnitOfWork _unitOfWork; + private readonly IUnitOfWork _unitOfWork;
263-263: Simplify the always-true predicate.The predicate
t => trueat line 263 matches all threads. Consider omitting it or using an overload ofGetPagedAsyncthat doesn't require a predicate, if available.
92-103: Eliminate ThreadResponseDto mapping duplication.The same manual DTO mapping logic is repeated in 7 different methods (AddThread, GetThreadById, GetAllThreads, AnswerThread, GetThreads, GetFeed, GetSavedThreads). Since
IMapperis already injected, leverage it to eliminate this duplication.Create an AutoMapper profile:
public class ThreadMappingProfile : Profile { public ThreadMappingProfile() { CreateMap<Thread, ThreadResponseDto>() .ForMember(dest => dest.AskerName, opt => opt.MapFrom(src => src.isAnonymous ? "Anonymous" : src.Asker?.Name ?? "Unknown")) .ForMember(dest => dest.AskedName, opt => opt.MapFrom(src => src.Asked?.Name ?? "Unknown")) .ForMember(dest => dest.AskerId, opt => opt.MapFrom(src => src.AskerId ?? 0)) .ForMember(dest => dest.LikesCount, opt => opt.MapFrom(src => src.ThreadLikes != null ? src.ThreadLikes.Count : 0)) .ForMember(dest => dest.CommentsCount, opt => opt.MapFrom(src => src.Comments != null ? src.Comments.Count : 0)); CreateMap<SavedThreads, ThreadResponseDto>() .ForMember(dest => dest.Id, opt => opt.MapFrom(src => src.Thread.Id)) .ForMember(dest => dest.QuestionContent, opt => opt.MapFrom(src => src.Thread.QuestionContent)) .ForMember(dest => dest.AnswerContent, opt => opt.MapFrom(src => src.Thread.AnswerContent)) .ForMember(dest => dest.Status, opt => opt.MapFrom(src => src.Thread.Status)) .ForMember(dest => dest.IsAnonymous, opt => opt.MapFrom(src => src.Thread.isAnonymous)) .ForMember(dest => dest.CreatedAt, opt => opt.MapFrom(src => src.Thread.CreatedAt)) .ForMember(dest => dest.AskerId, opt => opt.MapFrom(src => src.Thread.AskerId ?? 0)) .ForMember(dest => dest.AskerName, opt => opt.MapFrom(src => src.Thread.isAnonymous ? "Anonymous" : src.Thread.Asker?.Name ?? "Unknown")) .ForMember(dest => dest.AskedId, opt => opt.MapFrom(src => src.Thread.AskedId)) .ForMember(dest => dest.AskedName, opt => opt.MapFrom(src => src.Thread.Asked?.Name ?? "Unknown")) .ForMember(dest => dest.LikesCount, opt => opt.MapFrom(src => src.Thread.ThreadLikes != null ? src.Thread.ThreadLikes.Count : 0)) .ForMember(dest => dest.CommentsCount, opt => opt.MapFrom(src => src.Thread.Comments != null ? src.Thread.Comments.Count : 0)) .ForMember(dest => dest.SavedAt, opt => opt.MapFrom(src => src.CreatedAt)); } }Then register it in your DI configuration and replace all manual mappings:
For Thread to ThreadResponseDto:
-var responseDto = new ThreadResponseDto -{ - Id = thread.Id, - QuestionContent = thread.QuestionContent, - ... -}; +var responseDto = _mapper.Map<ThreadResponseDto>(thread);For SavedThreads to ThreadResponseDto:
-var threadDtos = savedThreads.Select(st => new ThreadResponseDto -{ - Id = st.Thread.Id, - ... -}).ToList(); +var threadDtos = _mapper.Map<List<ThreadResponseDto>>(savedThreads);This will improve maintainability and ensure consistency across all mapping locations. Based on learnings
Also applies to: 128-142, 168-182, 226-238, 267-281, 363-377, 497-512
84-87: Remove redundant manual navigation updates
EF Core’s ThreadConfiguration already tracks Asker/Asked relationships via AskerId/AskedId, so you can drop theAskUser.AskedThreads?.Add(thread),AskedUser.ReceivedThreads?.Add(thread)and the subsequentUpdateAsynccalls—just add the Thread entity and save.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
AskFm/AskFm.API/Controllers/CommentController.cs(5 hunks)AskFm/AskFm.API/Controllers/ThreadController.cs(1 hunks)AskFm/AskFm.API/Controllers/ThreadLikeController.cs(1 hunks)AskFm/AskFm.API/Controllers/WeatherForecastController.cs(1 hunks)AskFm/AskFm.BLL/DTO/CommentResponseDto.cs(1 hunks)AskFm/AskFm.BLL/DTO/CreateCommentDto.cs(1 hunks)AskFm/AskFm.BLL/DTO/RefreshTokenDto.cs(1 hunks)AskFm/AskFm.BLL/DTO/ThreadLikeResponseDto.cs(1 hunks)AskFm/AskFm.BLL/Services/CommentLikeService.cs(1 hunks)AskFm/AskFm.BLL/Services/CommentService.cs(1 hunks)AskFm/AskFm.BLL/Services/ICommentService.cs(1 hunks)AskFm/AskFm.BLL/Services/IThreadService.cs(1 hunks)AskFm/AskFm.BLL/Services/ThreadLikeService.cs(1 hunks)AskFm/AskFm.BLL/Services/ThreadService.cs(1 hunks)AskFm/AskFm.BLL/Services/UserIdentityService/UserService.cs(1 hunks)AskFm/AskFm.DAL/Repositories/Repository.cs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- AskFm/AskFm.BLL/Services/ThreadLikeService.cs
- AskFm/AskFm.API/Controllers/ThreadLikeController.cs
- AskFm/AskFm.BLL/Services/CommentLikeService.cs
🧰 Additional context used
🪛 GitHub Check: build-and-test
AskFm/AskFm.DAL/Repositories/Repository.cs
[warning] 66-66:
Cannot convert null literal to non-nullable reference type.
[warning] 51-51:
Cannot convert null literal to non-nullable reference type.
🔇 Additional comments (4)
AskFm/AskFm.BLL/Services/UserIdentityService/UserService.cs (1)
173-173: LGTM! UTC timestamp alignment.Changing from
DateTime.NowtoDateTime.UtcNowis a best practice that ensures consistent timestamp storage regardless of server timezone. This aligns with the UTC-aware timestamp logic being introduced elsewhere in the codebase.AskFm/AskFm.BLL/Services/ThreadService.cs (3)
301-338: LGTM!The delete authorization logic correctly allows both the asker and asked user to delete the thread, and the transaction handling is appropriate.
397-444: LGTM!The duplicate save check and transaction handling are appropriate.
446-477: LGTM!The unsave logic and transaction handling are correct.
| if (user == null || !user.success) | ||
| return BadRequest(user.Errors); | ||
|
|
||
|
|
||
|
|
||
| userId = user.Data.Id; | ||
| var comment = await _commentService.GetCommentAsync(id); | ||
|
|
||
| if (comment == null || !user.success) | ||
| return BadRequest(user.Errors); |
There was a problem hiding this comment.
Fix comment existence check in DeleteLike
This block reuses the user result when validating the comment, so a missing comment slips through and user.Errors is returned even when the user lookup succeeded. It also risks a null dereference if user is null. Validate comment.success and surface its errors instead.
- if (user == null || !user.success)
- return BadRequest(user.Errors);
+ if (user == null || !user.success)
+ {
+ return BadRequest(user?.Errors ?? new[] { "Unable to resolve current user." });
+ }
userId = user.Data.Id;
var comment = await _commentService.GetCommentAsync(id);
- if (comment == null || !user.success)
- return BadRequest(user.Errors);
+ if (comment == null || !comment.success)
+ {
+ return BadRequest(comment?.Errors ?? new[] { "Comment not found." });
+ }🤖 Prompt for AI Agents
In AskFm/AskFm.API/Controllers/CommentController.cs around lines 110 to 117, the
code incorrectly reuses the user result when validating the comment; replace the
second conditional so it verifies the comment result instead of user (i.e.,
check comment == null || !comment.success) and return comment.Errors; also
ensure you don't dereference user in that branch so the null-safety check is
correct.
| // POST api/threads/{id}/comments - Add a comment to the thread with id = {id} | ||
| [HttpPost] | ||
| [Route("threads/{id}/comments")] | ||
| public async Task<IActionResult> AddComment([FromRoute] int id, [FromBody] CreateCommentDto createCommentDto) | ||
| { | ||
| var user = await _userService.GetCurrentUserAsync(); | ||
| if (!user.success) | ||
| { | ||
| return BadRequest(user.Errors); | ||
| } | ||
|
|
||
| var result = await _commentService.AddComment(id, user.Data.Id, createCommentDto); | ||
| if (!result.success) | ||
| { | ||
| return BadRequest(result.Errors); | ||
| } | ||
|
|
||
| return Ok(result.Data); | ||
| } | ||
|
|
||
| // GET api/threads/{id}/comments - Get all comments for the thread with id = {id} | ||
| [HttpGet] | ||
| [Route("threads/{id}/comments")] | ||
| public async Task<IActionResult> GetComments([FromRoute] int id, [FromQuery] int page = 1, [FromQuery] int pageSize = 10) | ||
| { | ||
| var result = await _commentService.GetCommentsByThreadId(id, page, pageSize); | ||
| if (!result.success) | ||
| { | ||
| return BadRequest(result.Errors); | ||
| } | ||
|
|
||
| return Ok(result.Data); | ||
| } | ||
|
|
||
| // DELETE api/threads/{threadId}/comments/{commentId} - Delete a comment with id = {commentId} | ||
| [HttpDelete] | ||
| [Route("threads/{threadId}/comments/{commentId}")] | ||
| public async Task<IActionResult> DeleteComment([FromRoute] int threadId, [FromRoute] int commentId) | ||
| { | ||
| var user = await _userService.GetCurrentUserAsync(); | ||
| if (!user.success) | ||
| { | ||
| return BadRequest(user.Errors); | ||
| } | ||
|
|
||
| var result = await _commentService.DeleteComment(threadId, commentId, user.Data.Id); | ||
| if (!result.success) | ||
| { | ||
| return BadRequest(result.Errors); | ||
| } | ||
|
|
||
| return Ok(new { message = "Comment deleted successfully" }); | ||
| } |
There was a problem hiding this comment.
Expose comment endpoints under /api/threads as documented
With [Route("api/[controller]")], these actions currently register at /api/comment/threads/..., not the advertised /api/threads/{...}/comments. Use absolute route templates (prefixed with /) or restructure the base route so clients hit the intended paths.
- [HttpPost]
- [Route("threads/{id}/comments")]
+ [HttpPost("/api/threads/{id}/comments")]
public async Task<IActionResult> AddComment([FromRoute] int id, [FromBody] CreateCommentDto createCommentDto)
...
- [HttpGet]
- [Route("threads/{id}/comments")]
+ [HttpGet("/api/threads/{id}/comments")]
public async Task<IActionResult> GetComments([FromRoute] int id, [FromQuery] int page = 1, [FromQuery] int pageSize = 10)
...
- [HttpDelete]
- [Route("threads/{threadId}/comments/{commentId}")]
+ [HttpDelete("/api/threads/{threadId}/comments/{commentId}")]
public async Task<IActionResult> DeleteComment([FromRoute] int threadId, [FromRoute] int commentId)Alternatively, move these actions to a controller whose base route is api/threads and adjust the templates accordingly.
🤖 Prompt for AI Agents
In AskFm/AskFm.API/Controllers/CommentController.cs around lines 145-197 the
comment-related actions are being registered under /api/comment/threads/...
because the controller base route is api/[controller]; update the route
attributes to use absolute paths (prefix each route template with a leading '/')
so they register as /api/threads/{id}/comments (e.g.
"/api/threads/{id}/comments", "/api/threads/{id}/comments" for GET/POST and
"/api/threads/{threadId}/comments/{commentId}" for DELETE), or alternatively
change the controller's base route to "api/threads" and adjust the action
templates to relative paths ("{id}/comments", "{threadId}/comments/{commentId}")
so the endpoints match the documented /api/threads/... paths.
| namespace AskFm.BLL.DTO; | ||
|
|
||
| public class CreateCommentDto | ||
| { | ||
| public string Content { get; set; } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Protect the endpoint from null/empty comment bodies
CreateCommentDto.Content is optional right now, so a client can POST without a body. That null flows into the service and hits the database, which is very likely to reject it (non-null column) and return a 500 instead of a clean 400. Mark the property as required so model validation blocks bad requests up front.
Add the data-annotation and import:
+using System.ComponentModel.DataAnnotations;
+
namespace AskFm.BLL.DTO;
public class CreateCommentDto
{
- public string Content { get; set; }
+ [Required]
+ public string Content { get; set; }
}🤖 Prompt for AI Agents
In AskFm/AskFm.BLL/DTO/CreateCommentDto.cs lines 1-6, the Content property is
currently optional allowing null/empty bodies; add a using
System.ComponentModel.DataAnnotations; at the top and decorate the Content
property with [Required] (optionally also [MinLength(1)] if you want to reject
empty strings) so model validation returns 400 for missing/empty comments
instead of letting null flow to the service/database.
| public async Task<ServiceResult<PagedResponseDto<CommentResponseDto>>> GetCommentsByThreadId(int threadId, int page, int pageSize) | ||
| { | ||
| try | ||
| { | ||
| // Check if thread exists | ||
| var thread = await _unitOfWork.Threads.FindAsync(t => t.Id == threadId); | ||
| if (thread == null) | ||
| { | ||
| return await ServiceResult<PagedResponseDto<CommentResponseDto>>.Failure(new List<string>() { "Thread not found" }); | ||
| } | ||
|
|
||
| // Get total count of comments for this thread | ||
| var totalCount = await _unitOfWork.Comments.CountAsync(c => c.ThreadId == threadId); | ||
|
|
||
| // Get paginated comments | ||
| var comments = await _unitOfWork.Comments.GetPagedAsync( | ||
| skip: (page - 1) * pageSize, | ||
| take: pageSize, | ||
| orderBy: c => c.CreatedAt, | ||
| ascending: false, // Newest first | ||
| predicate: c => c.ThreadId == threadId, | ||
| includes: new[] { "User", "CommentLikes" } | ||
| ); | ||
|
|
||
| var commentDtos = comments.Select(c => new CommentResponseDto | ||
| { | ||
| Id = c.Id, | ||
| Content = c.Content, | ||
| UserId = c.UserId, | ||
| UserName = c.User?.Name ?? "Unknown", | ||
| ThreadId = c.ThreadId, | ||
| CreatedAt = c.CreatedAt, | ||
| LikesCount = c.CommentLikes?.Count ?? 0, | ||
| }).ToList(); | ||
|
|
||
| // Create paged response | ||
| var response = new PagedResponseDto<CommentResponseDto> | ||
| { | ||
| Items = commentDtos, | ||
| PageNumber = page, | ||
| PageSize = pageSize, | ||
| TotalCount = totalCount, | ||
| TotalPages = (int)Math.Ceiling((double)totalCount / pageSize) | ||
| }; |
There was a problem hiding this comment.
Validate pagination inputs to prevent runtime faults
page or pageSize ≤ 0 leads to negative skip values and a divide-by-zero when computing TotalPages, turning a bad client request into a 500. Reject invalid inputs up front.
public async Task<ServiceResult<PagedResponseDto<CommentResponseDto>>> GetCommentsByThreadId(int threadId, int page, int pageSize)
{
try
{
+ if (page <= 0 || pageSize <= 0)
+ {
+ return await ServiceResult<PagedResponseDto<CommentResponseDto>>.Failure(
+ new List<string> { "Both page and pageSize must be greater than zero." });
+ }
+
// Check if thread exists
var thread = await _unitOfWork.Threads.FindAsync(t => t.Id == threadId);📝 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<ServiceResult<PagedResponseDto<CommentResponseDto>>> GetCommentsByThreadId(int threadId, int page, int pageSize) | |
| { | |
| try | |
| { | |
| // Check if thread exists | |
| var thread = await _unitOfWork.Threads.FindAsync(t => t.Id == threadId); | |
| if (thread == null) | |
| { | |
| return await ServiceResult<PagedResponseDto<CommentResponseDto>>.Failure(new List<string>() { "Thread not found" }); | |
| } | |
| // Get total count of comments for this thread | |
| var totalCount = await _unitOfWork.Comments.CountAsync(c => c.ThreadId == threadId); | |
| // Get paginated comments | |
| var comments = await _unitOfWork.Comments.GetPagedAsync( | |
| skip: (page - 1) * pageSize, | |
| take: pageSize, | |
| orderBy: c => c.CreatedAt, | |
| ascending: false, // Newest first | |
| predicate: c => c.ThreadId == threadId, | |
| includes: new[] { "User", "CommentLikes" } | |
| ); | |
| var commentDtos = comments.Select(c => new CommentResponseDto | |
| { | |
| Id = c.Id, | |
| Content = c.Content, | |
| UserId = c.UserId, | |
| UserName = c.User?.Name ?? "Unknown", | |
| ThreadId = c.ThreadId, | |
| CreatedAt = c.CreatedAt, | |
| LikesCount = c.CommentLikes?.Count ?? 0, | |
| }).ToList(); | |
| // Create paged response | |
| var response = new PagedResponseDto<CommentResponseDto> | |
| { | |
| Items = commentDtos, | |
| PageNumber = page, | |
| PageSize = pageSize, | |
| TotalCount = totalCount, | |
| TotalPages = (int)Math.Ceiling((double)totalCount / pageSize) | |
| }; | |
| public async Task<ServiceResult<PagedResponseDto<CommentResponseDto>>> GetCommentsByThreadId(int threadId, int page, int pageSize) | |
| { | |
| try | |
| { | |
| if (page <= 0 || pageSize <= 0) | |
| { | |
| return await ServiceResult<PagedResponseDto<CommentResponseDto>>.Failure( | |
| new List<string> { "Both page and pageSize must be greater than zero." }); | |
| } | |
| // Check if thread exists | |
| var thread = await _unitOfWork.Threads.FindAsync(t => t.Id == threadId); | |
| if (thread == null) | |
| { | |
| return await ServiceResult<PagedResponseDto<CommentResponseDto>>.Failure( | |
| new List<string> { "Thread not found" }); | |
| } | |
| // Get total count of comments for this thread | |
| var totalCount = await _unitOfWork.Comments.CountAsync(c => c.ThreadId == threadId); | |
| // Get paginated comments | |
| var comments = await _unitOfWork.Comments.GetPagedAsync( | |
| skip: (page - 1) * pageSize, | |
| take: pageSize, | |
| orderBy: c => c.CreatedAt, | |
| ascending: false, // Newest first | |
| predicate: c => c.ThreadId == threadId, | |
| includes: new[] { "User", "CommentLikes" } | |
| ); | |
| var commentDtos = comments.Select(c => new CommentResponseDto | |
| { | |
| Id = c.Id, | |
| Content = c.Content, | |
| UserId = c.UserId, | |
| UserName = c.User?.Name ?? "Unknown", | |
| ThreadId = c.ThreadId, | |
| CreatedAt = c.CreatedAt, | |
| LikesCount = c.CommentLikes?.Count ?? 0, | |
| }).ToList(); | |
| // Create paged response | |
| var response = new PagedResponseDto<CommentResponseDto> | |
| { | |
| Items = commentDtos, | |
| PageNumber = page, | |
| PageSize = pageSize, | |
| TotalCount = totalCount, | |
| TotalPages = (int)Math.Ceiling((double)totalCount / pageSize) | |
| }; | |
| return await ServiceResult<PagedResponseDto<CommentResponseDto>>.Success(response); | |
| } | |
| catch (Exception ex) | |
| { | |
| // existing error handling... | |
| } | |
| } |
🤖 Prompt for AI Agents
In AskFm/AskFm.BLL/Services/CommentService.cs around lines 124 to 167, validate
that both page and pageSize are greater than zero before using them; if either
is <= 0 return a failed ServiceResult with a clear message (e.g., "Invalid
pagination parameters") so you avoid negative skip and divide-by-zero when
computing TotalPages, and perform this check immediately after confirming the
thread exists and before calling CountAsync/GetPagedAsync.
| public Task<ServiceResult<ThreadResponseDto>> AddThread(int askerId, CreateThreadDto createThreadDto); | ||
| public Task<ServiceResult<ThreadResponseDto>> GetThreadById(int id); | ||
| public Task<ServiceResult<List<ThreadResponseDto>>> GetAllThreads(int askedId); | ||
| public Task<ServiceResult<ThreadResponseDto>> AnswerThread(int threadId, int userId, AnswerThreadDto answerDto); | ||
| public Task<ServiceResult<PagedResponseDto<ThreadResponseDto>>> GetThreads(int page, int pageSize); | ||
| public Task<ServiceResult<bool>> DeleteThread(int threadId, int userId); | ||
| public Task<ServiceResult<PagedResponseDto<ThreadResponseDto>>> GetFeed(int userId, int page, int pageSize); | ||
| public Task<ServiceResult<bool>> SaveThread(int threadId, int userId); | ||
| public Task<ServiceResult<bool>> UnsaveThread(int threadId, int userId); | ||
| public Task<ServiceResult<PagedResponseDto<ThreadResponseDto>>> GetSavedThreads(int userId, int page, int pageSize); |
There was a problem hiding this comment.
Remove access modifiers inside the interface
Interface members must not carry explicit access modifiers; public here causes the project to fail compilation. Drop the public keyword from each signature.
- public Task<ServiceResult<ThreadResponseDto>> AddThread(int askerId, CreateThreadDto createThreadDto);
- public Task<ServiceResult<ThreadResponseDto>> GetThreadById(int id);
- public Task<ServiceResult<List<ThreadResponseDto>>> GetAllThreads(int askedId);
- public Task<ServiceResult<ThreadResponseDto>> AnswerThread(int threadId, int userId, AnswerThreadDto answerDto);
- public Task<ServiceResult<PagedResponseDto<ThreadResponseDto>>> GetThreads(int page, int pageSize);
- public Task<ServiceResult<bool>> DeleteThread(int threadId, int userId);
- public Task<ServiceResult<PagedResponseDto<ThreadResponseDto>>> GetFeed(int userId, int page, int pageSize);
- public Task<ServiceResult<bool>> SaveThread(int threadId, int userId);
- public Task<ServiceResult<bool>> UnsaveThread(int threadId, int userId);
- public Task<ServiceResult<PagedResponseDto<ThreadResponseDto>>> GetSavedThreads(int userId, int page, int pageSize);
+ Task<ServiceResult<ThreadResponseDto>> AddThread(int askerId, CreateThreadDto createThreadDto);
+ Task<ServiceResult<ThreadResponseDto>> GetThreadById(int id);
+ Task<ServiceResult<List<ThreadResponseDto>>> GetAllThreads(int askedId);
+ Task<ServiceResult<ThreadResponseDto>> AnswerThread(int threadId, int userId, AnswerThreadDto answerDto);
+ Task<ServiceResult<PagedResponseDto<ThreadResponseDto>>> GetThreads(int page, int pageSize);
+ Task<ServiceResult<bool>> DeleteThread(int threadId, int userId);
+ Task<ServiceResult<PagedResponseDto<ThreadResponseDto>>> GetFeed(int userId, int page, int pageSize);
+ Task<ServiceResult<bool>> SaveThread(int threadId, int userId);
+ Task<ServiceResult<bool>> UnsaveThread(int threadId, int userId);
+ Task<ServiceResult<PagedResponseDto<ThreadResponseDto>>> GetSavedThreads(int userId, int page, int pageSize);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Task<ServiceResult<ThreadResponseDto>> AddThread(int askerId, CreateThreadDto createThreadDto); | |
| public Task<ServiceResult<ThreadResponseDto>> GetThreadById(int id); | |
| public Task<ServiceResult<List<ThreadResponseDto>>> GetAllThreads(int askedId); | |
| public Task<ServiceResult<ThreadResponseDto>> AnswerThread(int threadId, int userId, AnswerThreadDto answerDto); | |
| public Task<ServiceResult<PagedResponseDto<ThreadResponseDto>>> GetThreads(int page, int pageSize); | |
| public Task<ServiceResult<bool>> DeleteThread(int threadId, int userId); | |
| public Task<ServiceResult<PagedResponseDto<ThreadResponseDto>>> GetFeed(int userId, int page, int pageSize); | |
| public Task<ServiceResult<bool>> SaveThread(int threadId, int userId); | |
| public Task<ServiceResult<bool>> UnsaveThread(int threadId, int userId); | |
| public Task<ServiceResult<PagedResponseDto<ThreadResponseDto>>> GetSavedThreads(int userId, int page, int pageSize); | |
| Task<ServiceResult<ThreadResponseDto>> AddThread(int askerId, CreateThreadDto createThreadDto); | |
| Task<ServiceResult<ThreadResponseDto>> GetThreadById(int id); | |
| Task<ServiceResult<List<ThreadResponseDto>>> GetAllThreads(int askedId); | |
| Task<ServiceResult<ThreadResponseDto>> AnswerThread(int threadId, int userId, AnswerThreadDto answerDto); | |
| Task<ServiceResult<PagedResponseDto<ThreadResponseDto>>> GetThreads(int page, int pageSize); | |
| Task<ServiceResult<bool>> DeleteThread(int threadId, int userId); | |
| Task<ServiceResult<PagedResponseDto<ThreadResponseDto>>> GetFeed(int userId, int page, int pageSize); | |
| Task<ServiceResult<bool>> SaveThread(int threadId, int userId); | |
| Task<ServiceResult<bool>> UnsaveThread(int threadId, int userId); | |
| Task<ServiceResult<PagedResponseDto<ThreadResponseDto>>> GetSavedThreads(int userId, int page, int pageSize); |
🤖 Prompt for AI Agents
In AskFm/AskFm.BLL/Services/IThreadService.cs around lines 7 to 16, the
interface methods incorrectly include the 'public' access modifier which
prevents compilation; remove the 'public' keyword from each method signature so
they are declared with only the return type and method name/signature. Ensure
all listed methods (AddThread, GetThreadById, GetAllThreads, AnswerThread,
GetThreads, DeleteThread, GetFeed, SaveThread, UnsaveThread, GetSavedThreads)
are updated accordingly and the file compiles.
|
|
||
| try | ||
| { | ||
| // check if the Asked user exite or not |
There was a problem hiding this comment.
Fix typo in comment.
The word "exite" should be "exist".
Apply this diff:
- // check if the Asked user exite or not
+ // check if the Asked user exists or not📝 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.
| // check if the Asked user exite or not | |
| // check if the Asked user exists or not |
🤖 Prompt for AI Agents
In AskFm/AskFm.BLL/Services/ThreadService.cs around line 30, the inline comment
contains a typo "exite" — update the comment to read "exist" (e.g., change "//
check if the Asked user exite or not" to "// check if the Asked user exist or
not" or better "// check if the asked user exists or not") to correct spelling
and grammar.
| IsAnonymous = thread.isAnonymous, | ||
| CreatedAt = thread.CreatedAt, | ||
| AskerId = thread.AskerId.Value, | ||
| AskerName = thread.Asker?.Name ?? "Unknown", |
There was a problem hiding this comment.
Inconsistent anonymous user handling.
Line 100 always shows the asker's name (or "Unknown" if null), but doesn't check thread.isAnonymous like the other methods do (e.g., line 137, 177, 235). This creates an inconsistency where AddThread exposes the asker's identity even when the thread is anonymous.
Apply this diff to align with the pattern used elsewhere:
- AskerName = thread.Asker?.Name ?? "Unknown",
+ AskerName = thread.isAnonymous ? "Anonymous" : thread.Asker?.Name ?? "Unknown",📝 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.
| AskerName = thread.Asker?.Name ?? "Unknown", | |
| AskerName = thread.isAnonymous | |
| ? "Anonymous" | |
| : thread.Asker?.Name ?? "Unknown", |
🤖 Prompt for AI Agents
In AskFm/AskFm.BLL/Services/ThreadService.cs around line 100, the AskerName
assignment in AddThread always reveals the asker's name (or "Unknown") and must
be aligned with other methods; change it to first check thread.IsAnonymous and
return a consistent anonymous label (e.g., "Anonymous") when true, otherwise
preserve the existing fallback to thread.Asker?.Name ?? "Unknown".
Mahmoud-Ayman-Saleh
left a comment
Mahmoud-Ayman-Saleh
left a comment
There was a problem hiding this comment.
i've reviewed the next endpoints:
- AddThread
- GetThreadById
- GetAllThreads
- AnswerThread
- GetThreads
@hadeer-r if you have a time please review the remaining endpoints, most of them needs just a validation for soft-delete and other minor validations or modifications
| // check if the Asked user exite or not | ||
| var askedId = createThreadDto.AskedId; | ||
|
|
||
| var askedUser = await _unitOfWork.Users.GetByIdAsync(askedId); |
There was a problem hiding this comment.
you have to check here if the askedUser is not deleted or not and the same with askerUser
var askedUser = await _unitOfWork.Users.FindAsync(
u => u.Id == createThreadDto.AskedId && !u.IsDeleted);| isAnonymous = createThreadDto.isAnonymous, | ||
| CreatedAt = DateTime.UtcNow, | ||
| }; | ||
|
|
There was a problem hiding this comment.
Status should always be Pending: The DTO allows setting any status, but a new question should always start as Pending
// Create thread - FORCE status to Pending
Thread thread = new Thread
{
AskedId = createThreadDto.AskedId,
AskerId = askerId,
QuestionContent = createThreadDto.QuestionContent.Trim(),
AnswerContent = string.Empty,
Status = ThreadStatus.Pending, // Always pending for new threads
isAnonymous = createThreadDto.isAnonymous,
CreatedAt = DateTime.UtcNow,
UpdatedAt = DateTime.UtcNow,
IsDeleted = false
};| await _unitOfWork.Users.UpdateAsync(askedUser); | ||
| await _unitOfWork.Users.UpdateAsync(askerUser); | ||
| await _unitOfWork.SaveAsync(); | ||
|
|
There was a problem hiding this comment.
why you use UpdateAsync twice ?? i see that you should remove both of them
|
|
||
| public bool isAnonymous { get; set; } | ||
|
|
||
| } No newline at end of file |
There was a problem hiding this comment.
- you should remove Status, the user should not set the status, the service will do this.
- if he creates new thread then the status will be Pending, if he answers threads then the status will be Answered
- maybe you can add some validations for improvements
public class CreateThreadDto
{
[Required(ErrorMessage = "Asked user ID is required")]
public int AskedId { get; set; }
[Required(ErrorMessage = "Question content is required")]
[StringLength(1000, MinimumLength = 2, ErrorMessage = "Question must be between 2 and 1000 characters")]
public string QuestionContent { get; set; }
public bool IsAnonymous { get; set; } // Fixed naming
}| pageSize, | ||
| t => t.CreatedAt, | ||
| false, | ||
| t => true, |
There was a problem hiding this comment.
fix this
t => !t.IsDeleted && t.Status == ThreadStatus.Answered| Status = thread.Status, | ||
| IsAnonymous = thread.isAnonymous, | ||
| CreatedAt = thread.CreatedAt, | ||
| AskerId = thread.AskerId ?? 0, |
There was a problem hiding this comment.
hide if Anonymous
AskerId = thread.isAnonymous ? null : thread.AskerId
| [HttpGet] | ||
| [Route("threads/{id}")] | ||
| public async Task<IActionResult> GetThreadWithId([FromRoute] int id) | ||
| { |
There was a problem hiding this comment.
add validation for id
if (id <= 0) return BadRequest(new { message = "Invalid thread ID" });| AnswerContent = thread.AnswerContent, | ||
| Status = thread.Status, | ||
| IsAnonymous = thread.isAnonymous, | ||
| CreatedAt = thread.CreatedAt, |
There was a problem hiding this comment.
add UodatedAt
UpdatedAt = thread.UpdatedAt,
|
@Mahmoud-Ayman-Saleh Thanks, I will do it |
hadeer-r
left a comment
hadeer-r
left a comment
There was a problem hiding this comment.
Great work! I've reviewed the SaveThread endpoints and the ThreadLike Service and Controller.
I suggest validation attributes directly to the DTOs (for required fields and other validation logic). This will help us avoid redundant validation code in the service layer and keep things DRY.
| } | ||
|
|
||
| // Check if user is authorized to delete this thread (either asker or asked) | ||
| if (thread.AskerId != userId && thread.AskedId != userId) |
| var user = await _userService.GetCurrentUserAsync(); | ||
| if (!user.success) | ||
| { | ||
| return BadRequest(user.Errors); |
There was a problem hiding this comment.
I think Unauthorized is better here, as it means there is no logged in user
|
|
||
| followedUserIds.Add(userId); | ||
|
|
||
| var totalCount = await _unitOfWork.Threads.CountAsync(t => |
There was a problem hiding this comment.
There is no need for the totalCount of Threads in Feed. Also, it's an overhead on the database .
The user in the feed page doesn't care about how many answered questions there, just like on Facebook, you don't care how many of the posts you have in the feed page
- removed the totalCount and totalPages from pagedResponseDto as
thier uses is so overhead on the database.
- instead, replaced them with HasMore, meaning are there any
remaining pages left here to make the client or the frontend use
as a flage.
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores