User Endpoints#25
Conversation
- Update User - Delete User - Follow User - UnFollow User - Update Password Tests: - Update User Tests TODO: - reset Password - reset Email - Decide how deleted endpoint will behave with relations
|
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 profile-focused user endpoints to the API, implements full user business logic with transactional follow/unfollow and password update in UserService, adjusts DTOs and auth behaviors (removing Logout and standardizing error responses), adds UnitOfWork transaction support, configures Swagger for JWT, and introduces unit tests and test dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as UserController
participant BLL as UserService
participant UoW as UnitOfWork
participant Repo as UserRepository
participant IdMgr as UserManager
rect rgb(235,245,255)
note over Client,API: Update profile flow
Client->>API: POST /profile/update/{userId} (UpdateUserDTO)
API->>API: _checkCurrentUser(userId)
API->>BLL: UpdateUserAsync(userId, dto)
BLL->>UoW: GetByIdAsync(userId)
UoW->>Repo: GetByIdAsync
Repo-->>UoW: ApplicationUser
UoW-->>BLL: ApplicationUser
BLL->>UoW: UpdateAsync(user)
BLL->>UoW: SaveAsync()
BLL-->>API: ServiceResult<bool>
API-->>Client: 200 OK / 400 BadRequest (result.Errors)
end
sequenceDiagram
autonumber
actor Client
participant API as UserController
participant BLL as UserService
participant UoW as UnitOfWork
participant Users as UsersRepo
participant Follows as FollowsRepo
rect rgb(240,255,240)
note over Client,API: Follow user flow (transactional)
Client->>API: POST /profile/{followerId}/follow/{targetUserId}
API->>API: validate & auth check
API->>BLL: FollowUserAsync(followerId, targetUserId)
BLL->>UoW: BeginTransactionAsync()
BLL->>Users: GetByIdAsync(followerId)
BLL->>Users: GetByIdAsync(targetUserId)
BLL->>Follows: GetRelation(followerId,targetUserId)
alt relation exists
BLL->>Follows: Reactivate relation
else
BLL->>Follows: Create relation
end
BLL->>Users: Update follower/following counts
BLL->>UoW: SaveAsync()
BLL->>UoW: Commit()
BLL-->>API: ServiceResult<bool>
API-->>Client: 200 OK / 400 BadRequest (result.Errors)
end
sequenceDiagram
autonumber
actor Client
participant API as AuthController
participant Auth as AuthService
rect rgb(255,250,235)
note over Client,API: Refresh token handling
Client->>API: POST /auth/refresh
API->>Auth: RefreshTokenAsync()
alt success
API->>API: setRefreshToken(cookie)
API-->>Client: 200 OK (token data)
else failure
API-->>Client: 400 BadRequest (result.Errors)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
@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: 13
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/Controllers/UserController.cs (2)
17-17: Make _userService field private.The
_userServicefield should be private to maintain proper encapsulation.- public IUserService _userService; + private IUserService _userService;
27-37: GetAllUsers method lacks pagination and may cause performance issues.Loading all users without pagination can cause performance problems as the user base grows.
Would you like me to help implement pagination for this endpoint?
AskFm/AskFm.BLL/Services/UserIdentityService/UserService.cs (1)
199-209: Null-claims NRE risk in GetCurrentUserAsyncAccessing
.Valuewithout checking the claim orHttpContextcan throw.- string email = _httpContextAccessor.HttpContext.User.FindFirst(ClaimTypes.Email).Value; - if (string.IsNullOrEmpty(email)) + var emailClaim = _httpContextAccessor.HttpContext?.User?.FindFirst(ClaimTypes.Email); + if (emailClaim == null || string.IsNullOrEmpty(emailClaim.Value)) { var errors = new List<string>() { - "Can't Access Current user" + "Cannot access current user" }; return await ServiceResult<ApplicationUser>.Failure(errors); } - var currentAppUser = await _userManager.FindByEmailAsync(email); + var currentAppUser = await _userManager.FindByEmailAsync(emailClaim.Value);
🧹 Nitpick comments (16)
AskFm/Tests/Tests.csproj (1)
14-14: Mark Moq as test-only to avoid transitive flow.Add PrivateAssets to keep it scoped to tests.
- <PackageReference Include="Moq" Version="4.20.72" /> + <PackageReference Include="Moq" Version="4.20.72" PrivateAssets="all" />AskFm/AskFm.API/Program.cs (1)
49-75: Minor cleanup: remove duplicate AddControllers and unify auth scheme.
- You call AddControllers twice (Lines 28 and 47). Keep one.
- Prefer JwtBearerDefaults.AuthenticationScheme for both DefaultAuthenticateScheme and DefaultChallengeScheme.
Outside-diff tweaks:
// Remove the second call: // builder.Services.AddControllers(); // (Line 47) // Auth defaults: builder.Services.AddAuthentication(options => { options.DefaultAuthenticateScheme = JwtBearerDefaults.AuthenticationScheme; options.DefaultChallengeScheme = JwtBearerDefaults.AuthenticationScheme; })AskFm/AskFm.BLL/Services/UserIdentityService/IAuthService.cs (1)
9-9: Remove commented-out API from the interface.Use an issue/TODO tracker instead of commented signatures.
- // public Task<ServiceResult<bool>> ResetPasswordAsync(string Email);AskFm/AskFm.DAL/Interfaces/IUnitOfWork.cs (1)
20-21: Consider CT/isolation overloads for transactions.Add CancellationToken and optional IsolationLevel to future-proof transaction usage.
Example:
Task<IDbContextTransaction> BeginTransactionAsync( IsolationLevel isolationLevel = IsolationLevel.ReadCommitted, CancellationToken cancellationToken = default);AskFm/AskFm.BLL/Services/UserIdentityService/IUserService.cs (1)
8-8: Consider returning the updated user data instead of just a boolean.The change from
Task<ServiceResult<UpdateUserDTO>>toTask<ServiceResult<bool>>reduces the information returned to the client. Consider returning the updated user data for better client experience and to avoid an additional API call.AskFm/Tests/UserTests.cs (2)
29-29: Fix typo in comment.- // uesr repo setup + // user repo setup
68-69: Unnecessary Task.CompletedTask and Task.FromResult usage.The mock setup can be simplified:
- _mockUnitOfWork.Setup(u => u.Users.UpdateAsync(It.IsAny<ApplicationUser>())).Returns(Task.CompletedTask); - _mockUnitOfWork.Setup(u => u.SaveAsync()).Returns(Task.FromResult(1)); + _mockUnitOfWork.Setup(u => u.Users.UpdateAsync(It.IsAny<ApplicationUser>())).Returns(Task.CompletedTask); + _mockUnitOfWork.Setup(u => u.SaveAsync()).ReturnsAsync(1);AskFm/AskFm.API/Controllers/UserController.cs (3)
41-59: Code duplication in GetCurrentUserAsync.The manual mapping to
ReadUserDTOduplicates logic that likely exists in the service layer.Consider having the service return a properly mapped DTO directly, or use AutoMapper to avoid manual mapping.
75-75: UpdateUserDTO should be from request body.Add
[FromBody]attribute to explicitly indicate the source of the DTO.- public async Task<IActionResult> UpdateUserAsync(int userId, UpdateUserDTO updatedUser) + public async Task<IActionResult> UpdateUserAsync(int userId, [FromBody] UpdateUserDTO updatedUser)
177-181: Track TODO items mentioned in comments.The comments indicate pending work for email updates and user deletion checks during login.
Would you like me to create issues to track these TODO items: reset email functionality, email confirmation, and checking user deletion status during login?
AskFm/AskFm.BLL/Services/UserIdentityService/UserService.cs (6)
36-42: Avoid null-overwrites; return explicit true for consistency
- Only overwrite fields when provided (patch semantics).
- Align return value with other methods that return
Success(true).- AppUserToUpdate.Name = updatedUser.Name; - AppUserToUpdate.Bio = updatedUser.Bio; - AppUserToUpdate.AvatarPath = updatedUser.AvatarPath; + if (updatedUser.Name is not null) AppUserToUpdate.Name = updatedUser.Name; + if (updatedUser.Bio is not null) AppUserToUpdate.Bio = updatedUser.Bio; + if (updatedUser.AvatarPath is not null) AppUserToUpdate.AvatarPath = updatedUser.AvatarPath; @@ - return await ServiceResult<bool>.Success(); + return await ServiceResult<bool>.Success(true);
47-53: Prefer async repository calls and clarify delete behavior
- Use
GetByIdAsyncfor consistency with EF Core async usage.- Define deletion semantics (soft delete vs. hard delete + cascades), per PR TODO.
- var appUser = _unitOfWork.Users.GetById(userId); + var appUser = await _unitOfWork.Users.GetByIdAsync(userId);Please confirm: Should related data (questions, answers, follows) be soft-deleted, reassigned, or cascaded? I can draft the approach once decided.
61-62: Clarify error messageBe explicit to aid client handling.
- return await ServiceResult<bool>.Failure(new List<string> { "Invalid user" }); + return await ServiceResult<bool>.Failure(new List<string> { "Cannot follow yourself" });
75-104: Ensure uniqueness and counter integrity for follows
- Add a unique DB constraint on
(FollowerId, FollowedId)to prevent races/dupes.- Consider recalculating or validating counters periodically to guard against drift.
If you’d like, I can provide the EF Core Fluent config and migration to enforce the unique index.
182-194: Use async retrieval; verify DTO property casing
- Prefer
GetByIdAsync.- Check that
ReadUserDTOhasfollowerCount(lowercase f). If it’sFollowerCount, this won’t compile.- var user = _unitOfWork.Users.GetById(userId); + var user = await _unitOfWork.Users.GetByIdAsync(userId);Please confirm the exact property name on
ReadUserDTO.
241-251: Avoid throwing NotImplemented from a service methodReturn a structured failure until implemented; prevents 500s and keeps API contracts consistent.
- // - throw new NotImplementedException(); + // Not implemented yet + return await ServiceResult<ReadUserDTO>.Failure(new List<string> { "ResetEmail is not implemented yet." });I can wire this up with
GenerateChangeEmailTokenAsync, email sender, and a confirmation endpoint when ready.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
AskFm/AskFm.API/Controllers/AuthController.cs(4 hunks)AskFm/AskFm.API/Controllers/UserController.cs(2 hunks)AskFm/AskFm.API/Program.cs(1 hunks)AskFm/AskFm.BLL/DTO/UserDTOs/UpdateUserDTO.cs(0 hunks)AskFm/AskFm.BLL/Services/UserIdentityService/AuthService.cs(0 hunks)AskFm/AskFm.BLL/Services/UserIdentityService/IAuthService.cs(1 hunks)AskFm/AskFm.BLL/Services/UserIdentityService/IUserService.cs(1 hunks)AskFm/AskFm.BLL/Services/UserIdentityService/UserService.cs(3 hunks)AskFm/AskFm.DAL/Interfaces/IUnitOfWork.cs(2 hunks)AskFm/AskFm.DAL/Repositories/UserRepository.cs(1 hunks)AskFm/AskFm.DAL/UnitOfWork.cs(2 hunks)AskFm/Tests/Tests.csproj(2 hunks)AskFm/Tests/UserTests.cs(1 hunks)
💤 Files with no reviewable changes (2)
- AskFm/AskFm.BLL/DTO/UserDTOs/UpdateUserDTO.cs
- AskFm/AskFm.BLL/Services/UserIdentityService/AuthService.cs
🔇 Additional comments (10)
AskFm/AskFm.DAL/Repositories/UserRepository.cs (1)
19-19: No-op whitespace change — OK.No functional impact.
AskFm/Tests/Tests.csproj (2)
12-12: EF Core InMemory dependency — OK.Version aligns with net9.0.
23-26: Project references added — OK.Keeps tests close to BLL/DAL surfaces.
AskFm/AskFm.API/Program.cs (1)
49-75: JWT security in Swagger — LGTM.Security scheme + requirement are correctly wired.
AskFm/AskFm.DAL/Interfaces/IUnitOfWork.cs (1)
2-2: Import for transactions — OK.Required for IDbContextTransaction.
AskFm/AskFm.API/Controllers/AuthController.cs (2)
33-33: Good improvement in error response consistency.The consistent return of
result.Errorsinstead of the full result object improves API response uniformity and reduces information leakage.Also applies to: 50-50, 71-71, 91-91
52-53: LGTM! Simplified token refresh flow.The unconditional call to
setRefreshTokenafter success check simplifies the control flow and aligns with the updated authentication service behavior.Also applies to: 73-73
AskFm/AskFm.DAL/UnitOfWork.cs (2)
134-137: Good fix for async method implementation.Properly implementing the async method by awaiting
SaveChangesAsync()instead of returning the task directly ensures correct exception handling and stack traces.
139-139: Transaction support added correctly.The implementation of
BeginTransactionAsyncenables transactional operations across the data layer, which is essential for maintaining data consistency in complex operations.AskFm/AskFm.BLL/Services/UserIdentityService/IUserService.cs (1)
12-12: Good improvement in UpdateLastSeenAsync signature.Removing the
DateTime lastSeenparameter and letting the service determine the current time internally prevents client-side time inconsistencies.
| @@ -1,4 +1,5 @@ | |||
| using AskFm.DAL.Models; | |||
| using Microsoft.EntityFrameworkCore.Storage; | |||
| using Thread = System.Threading.Thread; | |||
There was a problem hiding this comment.
Critical: Repository typed to System.Threading.Thread, not your domain Thread.
The alias forces IRepository to mean System.Threading.Thread, likely breaking data access for threads.
Apply this diff within the using block:
-using Thread = System.Threading.Thread;
+using DalThread = AskFm.DAL.Models.Thread;Then update the property type (outside this diffed range):
IRepository<DalThread> Threads { get; }🤖 Prompt for AI Agents
In AskFm/AskFm.DAL/Interfaces/IUnitOfWork.cs around line 3, the file currently
aliases Thread to System.Threading.Thread which causes IRepository<Thread> to
refer to the framework Thread type instead of your domain entity; remove that
alias, add or ensure the using for your DAL thread entity namespace (so
DalThread is in scope), and update the interface property declaration (outside
the shown diff) to use IRepository<DalThread> Threads { get; }.
…sswords in paramaters
…ollow function userService
|
@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 (2)
AskFm/AskFm.BLL/Services/UserIdentityService/AuthService.cs (1)
223-238: Replace obsolete RNGCryptoServiceProvider.Use RandomNumberGenerator APIs; fewer allocations and no obsolete type warnings.
- private async Task<RefreshToken> generateRefreshToken() + private async Task<RefreshToken> generateRefreshToken() { - var randomNumber = new byte[32]; - - using var generator = new RNGCryptoServiceProvider(); - - generator.GetBytes(randomNumber); + var randomNumber = RandomNumberGenerator.GetBytes(32); return new RefreshToken { Token = Convert.ToBase64String(randomNumber), ExpireOn = DateTime.UtcNow.AddDays(10), CreatedOn = DateTime.UtcNow }; }AskFm/AskFm.BLL/Services/UserIdentityService/UserService.cs (1)
206-214: Null-safety for current-user claim lookupAvoid potential NRE when
HttpContext/User/claim is missing.- string email = _httpContextAccessor.HttpContext.User.FindFirst(ClaimTypes.Email).Value; + var email = _httpContextAccessor.HttpContext?.User?.FindFirst(ClaimTypes.Email)?.Value; if (string.IsNullOrEmpty(email))
♻️ Duplicate comments (2)
AskFm/AskFm.BLL/Services/UserIdentityService/UserService.cs (2)
173-176: Prefer UTC timestamps for LastSeen- appUser.LastSeen = DateTime.Now; + appUser.LastSeen = DateTime.UtcNow;
141-156: Fix duplicate flag assignment and use CommitAsync
IsDeletedis set twice.- Mixing async transaction with sync
Commit()risks incomplete commits.if (followExist != null) { - followExist.IsDeleted = true; - followExist.IsDeleted = true; + followExist.IsDeleted = true; followExist.IsActive = false; if (userFollower.FollowingCount > 0) userFollower.FollowingCount--; if (targetUser.FollowersCount > 0) targetUser.FollowersCount--; await _unitOfWork.Follows.UpdateAsync(followExist); await _unitOfWork.Users.UpdateAsync(userFollower); await _unitOfWork.Users.UpdateAsync(targetUser); await _unitOfWork.SaveAsync(); } - transaction.Commit(); + await transaction.CommitAsync();
🧹 Nitpick comments (19)
AskFm/AskFm.BLL/DTO/UserDTOs/UpdatePasswordDTO.cs (1)
3-7: Naming nit: “UpdatedPassword” reads awkwardly.Consider “NewPassword” for clarity across API and service layers. Low-cost rename now before external clients depend on it.
AskFm/AskFm.BLL/Services/UserIdentityService/AuthService.cs (2)
122-128: Use async repository calls and stronger token param validation.These paths likely hit the DB; prefer async and guard against whitespace tokens.
- if (refreshToken == null) + if (string.IsNullOrWhiteSpace(refreshToken)) { var errors = new List<string> { "Invalid Token." }; return await ServiceResult<AuthResponseDTO>.Failure(errors); } - var user = _unitOfWork.Users.GetById(id); + var user = await _unitOfWork.Users.GetByIdAsync(id);- if (!user.RefreshTokens.Any(r => r.Token == refreshToken)) + if (!user.RefreshTokens.Any(r => r.Token == refreshToken)) { var errors = new List<string> { "Invalid Token." }; return await ServiceResult<bool>.Failure(errors); } - var oldRefreshToken = user.RefreshTokens.Single(t => t.Token == refreshToken); + var oldRefreshToken = user.RefreshTokens.Single(t => t.Token == refreshToken); oldRefreshToken.RevokedOn = DateTime.UtcNow; - await _userManager.UpdateAsync(user); + await _userManager.UpdateAsync(user);Also applies to: 169-179
239-257: JWT hardening (optional).Consider adding jti and sub claims and including roles if you authorize by role later.
- Subject = new ClaimsIdentity(new Claim[] + Subject = new ClaimsIdentity(new Claim[] { new(ClaimTypes.Name, appUser.Name), new(ClaimTypes.Email, appUser.Email), new("UserId", appUser.Id.ToString()) + // new(JwtRegisteredClaimNames.Sub, appUser.Id.ToString()), + // new(JwtRegisteredClaimNames.Jti, Guid.NewGuid().ToString()), })AskFm/AskFm.BLL/Services/UserIdentityService/IUserService.cs (2)
8-8: Returning only bool forces extra fetches.Returning ReadUserDTO from UpdateUserAsync reduces an extra GetByIdAsync in the controller.
- Task<ServiceResult<bool>> UpdateUserAsync(int userId, UpdateUserDTO updatedUser); + Task<ServiceResult<ReadUserDTO>> UpdateUserAsync(int userId, UpdateUserDTO updatedUser);
12-16: Add CancellationToken to service methods.Improves request cancellation and scalability under load.
- Task<ServiceResult<bool>> UpdateLastSeenAsync(int userId); + Task<ServiceResult<bool>> UpdateLastSeenAsync(int userId, CancellationToken ct = default); - Task<ServiceResult<bool>> UpdatePassword(int userId, UpdatePasswordDTO updatePasswordDto); + Task<ServiceResult<bool>> UpdatePassword(int userId, UpdatePasswordDTO updatePasswordDto, CancellationToken ct = default); - Task<ServiceResult<ReadUserDTO>> ResetEmail(int userId, string updatedEmail); + Task<ServiceResult<ReadUserDTO>> ResetEmail(int userId, string updatedEmail, CancellationToken ct = default);AskFm/Tests/UserTests.cs (4)
49-80: Strengthen assertions to verify updated fields.Also assert passed entity contains new values, not just that UpdateAsync was called.
- _mockUnitOfWork.Verify(u => u.Users.UpdateAsync(It.Is<ApplicationUser>(u => u.Id == userId)), Times.Once); + _mockUnitOfWork.Verify(u => u.Users.UpdateAsync(It.Is<ApplicationUser>(u => + u.Id == userId && + u.Name == updatedUser.Name && + u.Bio == updatedUser.Bio && + u.AvatarPath == updatedUser.AvatarPath + )), Times.Once);
83-119: Fix typos in test names (“Faild” → “Failed”).Improves readability in test reports.
- public async Task UpdatedUser_ThePassedUserIsNull_UpdateFaild() + public async Task UpdatedUser_ThePassedUserIsNull_UpdateFailed() ... - public async Task UpdatedUser_ThePassedUserIsNotFound_UpdateFaild() + public async Task UpdatedUser_ThePassedUserIsNotFound_UpdateFailed()
125-138: Add coverage for follow/unfollow and update password.Current suite misses business-critical paths added in this PR. I can scaffold tests using Moq for transactions and ChangePasswordAsync.
Also applies to: 140-167
176-201: UserManager mock: consider strict setup for ChangePasswordAsync.You’ll need this when adding UpdatePassword tests.
_mockUserManager .Setup(um => um.ChangePasswordAsync(It.IsAny<ApplicationUser>(), It.IsAny<string>(), It.IsAny<string>())) .ReturnsAsync(IdentityResult.Success);AskFm/AskFm.API/Controllers/UserController.cs (6)
48-58: Consider 404 for missing users.If the service indicates “not found”, return NotFound instead of BadRequest.
- if (!result.success) - { - return BadRequest(result.Errors); - } + if (!result.success) + { + // if service distinguishes NotFound, surface it here + return NotFound(result.Errors); + }
78-93: Prefer 204 NoContent on successful delete and fix Forbid usage.- if (!await _checkCurrentUser(userId)) - { - return Forbid("Cannot Remove this user"); - } + if (!await _checkCurrentUser(userId)) + { + return Forbid(); + } ... - return Ok(); + return NoContent();
95-114: Status code semantics and Forbid usage.Self-follow is a business rule violation; BadRequest or Conflict is clearer than Forbid. Also remove string arg.
- if (await _checkCurrentUser(targetUserId)) - { - return Forbid("Cannot Follow the current user"); - } + if (await _checkCurrentUser(targetUserId)) + { + return BadRequest("Cannot follow yourself."); + } - if (!await _checkCurrentUser(followerId)) - { - return Forbid("User can't perform this follow"); - } + if (!await _checkCurrentUser(followerId)) + { + return Forbid(); + }
116-135: Mirror follow endpoint behavior and fix Forbid usage.- if (await _checkCurrentUser(targetUserId)) - { - return Forbid("Cannot unFollow the current user"); - } + if (await _checkCurrentUser(targetUserId)) + { + return BadRequest("Cannot unfollow yourself."); + } - if (!await _checkCurrentUser(followerId)) - { - return Forbid("User can't perform this unfollow"); - } + if (!await _checkCurrentUser(followerId)) + { + return Forbid(); + }
165-171: Outdated TODO.“check user not deleted in login” is implemented in AuthService.LoginAsync; remove or update the TODO.
60-66: Remove string arguments from Forbid() calls
- In AskFm/AskFm.API/Controllers/UserController.cs at lines 66, 84, 101, 105, 122, 126, 143: replace calls like
Forbid("…")withForbid()(or usereturn StatusCode(403, "…")if you need to include a response body).AskFm/AskFm.BLL/Services/UserIdentityService/UserService.cs (4)
241-245: Neutral/error message clarity- var errors = new List<string> { "Cannot Update Current Password." }; + var errors = new List<string> { "Cannot update password." };
36-38: Optional: patch semantics to avoid overwriting with nullsIf DTO fields are optional, guard assignments to prevent nulling existing values.
- AppUserToUpdate.Name = updatedUser.Name; - AppUserToUpdate.Bio = updatedUser.Bio; - AppUserToUpdate.AvatarPath = updatedUser.AvatarPath; + if (updatedUser.Name is not null) AppUserToUpdate.Name = updatedUser.Name; + if (updatedUser.Bio is not null) AppUserToUpdate.Bio = updatedUser.Bio; + if (updatedUser.AvatarPath is not null) AppUserToUpdate.AvatarPath = updatedUser.AvatarPath;
75-104: Add DB uniqueness and concurrency guards for follow relations
- Enforce unique index on
(FollowerId, FollowedId)to prevent duplicates under race.- Consider optimistic concurrency (rowversion) on user counters to avoid drift.
Also applies to: 136-151
250-260: ResetEmail stub — can help implement end-to-endToken generation, confirmation link, and
ChangeEmailAsyncflow are pending. I can provide a complete implementation wired to Identity options and email sender.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
AskFm/AskFm.API/Controllers/UserController.cs(1 hunks)AskFm/AskFm.BLL/DTO/UserDTOs/UpdatePasswordDTO.cs(1 hunks)AskFm/AskFm.BLL/Services/UserIdentityService/AuthService.cs(2 hunks)AskFm/AskFm.BLL/Services/UserIdentityService/IUserService.cs(1 hunks)AskFm/AskFm.BLL/Services/UserIdentityService/UserService.cs(3 hunks)AskFm/Tests/UserTests.cs(1 hunks)
🔇 Additional comments (2)
AskFm/AskFm.BLL/Services/UserIdentityService/AuthService.cs (1)
54-61: Good: Block login for soft-deleted users without leaking info.Returning the same generic error string prevents user enumeration. Looks solid.
AskFm/AskFm.BLL/Services/UserIdentityService/UserService.cs (1)
228-239: Good fix: validate current password and prevent reuse
| [HttpPost] | ||
| [Route("profile/update/{userId}")] | ||
| public async Task<IActionResult> UpdateUserAsync(int userId, UpdateUserDTO updatedUser) | ||
| { | ||
| if (!await _checkCurrentUser(userId)) | ||
| { | ||
| return Forbid("Cannot Update this user"); | ||
| } | ||
| var result = await _userService.UpdateUserAsync(userId, updatedUser); | ||
| if (!result.success) | ||
| { | ||
| return BadRequest(result.Errors); | ||
| } | ||
|
|
||
| var userRead = await _userService.GetUserByIdAsync(userId); | ||
| return Ok(userRead); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix response shape and Forbid usage.
- Forbid(string) treats the string as an auth scheme, not a message.
- Return the DTO, not the ServiceResult wrapper.
- if (!await _checkCurrentUser(userId))
- {
- return Forbid("Cannot Update this user");
- }
+ if (!await _checkCurrentUser(userId))
+ {
+ return Forbid();
+ }
var result = await _userService.UpdateUserAsync(userId, updatedUser);
if (!result.success)
{
return BadRequest(result.Errors);
}
-
- var userRead = await _userService.GetUserByIdAsync(userId);
- return Ok(userRead);
+ var userRead = await _userService.GetUserByIdAsync(userId);
+ return Ok(userRead.Data);📝 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.
| [HttpPost] | |
| [Route("profile/update/{userId}")] | |
| public async Task<IActionResult> UpdateUserAsync(int userId, UpdateUserDTO updatedUser) | |
| { | |
| if (!await _checkCurrentUser(userId)) | |
| { | |
| return Forbid("Cannot Update this user"); | |
| } | |
| var result = await _userService.UpdateUserAsync(userId, updatedUser); | |
| if (!result.success) | |
| { | |
| return BadRequest(result.Errors); | |
| } | |
| var userRead = await _userService.GetUserByIdAsync(userId); | |
| return Ok(userRead); | |
| } | |
| [HttpPost] | |
| [Route("profile/update/{userId}")] | |
| public async Task<IActionResult> UpdateUserAsync(int userId, UpdateUserDTO updatedUser) | |
| { | |
| if (!await _checkCurrentUser(userId)) | |
| { | |
| return Forbid(); | |
| } | |
| var result = await _userService.UpdateUserAsync(userId, updatedUser); | |
| if (!result.success) | |
| { | |
| return BadRequest(result.Errors); | |
| } | |
| var userRead = await _userService.GetUserByIdAsync(userId); | |
| return Ok(userRead.Data); | |
| } |
🤖 Prompt for AI Agents
In AskFm/AskFm.API/Controllers/UserController.cs around lines 60 to 76, change
the Forbid call to not pass a string (use Forbid() or return ForbidResult())
because Forbid(string) is treated as an auth scheme, and when returning the
updated user unwrap the service result and return the DTO itself (e.g., check
GetUserByIdAsync result for success and return Ok(result.Value or .Data) instead
of returning the ServiceResult wrapper); also handle a failed GetUserByIdAsync
by returning an appropriate error status.
| [HttpPost] | ||
| [Route("profile/update/pass/{userId}")] | ||
| public async Task<IActionResult> UpdatePassword(int userId, UpdatePasswordDTO udpatePasswordDto) | ||
| { | ||
| if (await _checkCurrentUser(userId)) | ||
| { | ||
| return Forbid("Cannot update password for another user"); | ||
| } | ||
|
|
||
| var result = await _userService.UpdatePassword(userId, udpatePasswordDto); | ||
| if (!result.success) | ||
| { | ||
| return BadRequest(result.Errors); | ||
| } | ||
|
|
||
| return Ok(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Critical: Authorization check inverted + parameter typo.
This currently forbids the current user from changing their own password and allows others. Also fix the variable name and Forbid usage.
- public async Task<IActionResult> UpdatePassword(int userId, UpdatePasswordDTO udpatePasswordDto)
+ public async Task<IActionResult> UpdatePassword(int userId, UpdatePasswordDTO updatePasswordDto)
{
- if (await _checkCurrentUser(userId))
- {
- return Forbid("Cannot update password for another user");
- }
+ if (!await _checkCurrentUser(userId))
+ {
+ return Forbid();
+ }
- var result = await _userService.UpdatePassword(userId, udpatePasswordDto);
+ var result = await _userService.UpdatePassword(userId, updatePasswordDto);
if (!result.success)
{
return BadRequest(result.Errors);
}
return Ok();
}📝 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.
| [HttpPost] | |
| [Route("profile/update/pass/{userId}")] | |
| public async Task<IActionResult> UpdatePassword(int userId, UpdatePasswordDTO udpatePasswordDto) | |
| { | |
| if (await _checkCurrentUser(userId)) | |
| { | |
| return Forbid("Cannot update password for another user"); | |
| } | |
| var result = await _userService.UpdatePassword(userId, udpatePasswordDto); | |
| if (!result.success) | |
| { | |
| return BadRequest(result.Errors); | |
| } | |
| return Ok(); | |
| } | |
| [HttpPost] | |
| [Route("profile/update/pass/{userId}")] | |
| public async Task<IActionResult> UpdatePassword(int userId, UpdatePasswordDTO updatePasswordDto) | |
| { | |
| if (!await _checkCurrentUser(userId)) | |
| { | |
| return Forbid(); | |
| } | |
| var result = await _userService.UpdatePassword(userId, updatePasswordDto); | |
| if (!result.success) | |
| { | |
| return BadRequest(result.Errors); | |
| } | |
| return Ok(); | |
| } |
🤖 Prompt for AI Agents
In AskFm/AskFm.API/Controllers/UserController.cs around lines 137 to 154, the
authorization check is inverted and the DTO parameter is misspelled; change the
method signature parameter from udpatePasswordDto to updatePasswordDto, and
invert the conditional so that if the current user is NOT the target user you
return Forbid(); also remove the message passed to Forbid (use Forbid() or
return ForbidResult) and use the corrected updatePasswordDto variable when
calling _userService.UpdatePassword.
| //------------------------------------------------------------------- | ||
| // Helper functions | ||
| private async Task<bool> _checkCurrentUser(int userId) | ||
| { | ||
| var current_user = await _userService.GetCurrentUserAsync(); | ||
| return current_user.Data.Id == userId; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against null current user to avoid NRE.
Return false if service failed or Data is null.
- private async Task<bool> _checkCurrentUser(int userId)
+ private async Task<bool> _checkCurrentUser(int userId)
{
- var current_user = await _userService.GetCurrentUserAsync();
- return current_user.Data.Id == userId;
+ var currentUser = await _userService.GetCurrentUserAsync();
+ return currentUser.success && currentUser.Data != null && currentUser.Data.Id == userId;
}📝 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.
| //------------------------------------------------------------------- | |
| // Helper functions | |
| private async Task<bool> _checkCurrentUser(int userId) | |
| { | |
| var current_user = await _userService.GetCurrentUserAsync(); | |
| return current_user.Data.Id == userId; | |
| } | |
| //------------------------------------------------------------------- | |
| // Helper functions | |
| private async Task<bool> _checkCurrentUser(int userId) | |
| { | |
| var currentUser = await _userService.GetCurrentUserAsync(); | |
| return currentUser.success | |
| && currentUser.Data != null | |
| && currentUser.Data.Id == userId; | |
| } |
🤖 Prompt for AI Agents
In AskFm/AskFm.API/Controllers/UserController.cs around lines 156-163, the
helper _checkCurrentUser assumes _userService.GetCurrentUserAsync() returns a
non-null response and Data is present; change it to guard against null or failed
service results by returning false when the service call fails or the returned
object or its Data is null (e.g., await the call, check for null response,
optional success flag and response.Data null, and then only compare Id when
safe), ensuring no null-reference exception.
| public string CurrentPassword { get; set; } | ||
| public string UpdatedPassword { get; set; } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add basic validation and avoid nullable strings in DTO.
Mark fields as required and set non-null defaults to prevent nulls from slipping through model binding.
+using System.ComponentModel.DataAnnotations;
+
public class UpdatePasswordDTO
{
- public string CurrentPassword { get; set; }
- public string UpdatedPassword { get; set; }
+ [Required, DataType(DataType.Password), MinLength(8)]
+ public string CurrentPassword { get; set; } = string.Empty;
+
+ [Required, DataType(DataType.Password), MinLength(8)]
+ public string UpdatedPassword { get; set; } = string.Empty;
}📝 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 string CurrentPassword { get; set; } | |
| public string UpdatedPassword { get; set; } | |
| using System.ComponentModel.DataAnnotations; | |
| public class UpdatePasswordDTO | |
| { | |
| [Required, DataType(DataType.Password), MinLength(8)] | |
| public string CurrentPassword { get; set; } = string.Empty; | |
| [Required, DataType(DataType.Password), MinLength(8)] | |
| public string UpdatedPassword { get; set; } = string.Empty; | |
| } |
🤖 Prompt for AI Agents
In AskFm/AskFm.BLL/DTO/UserDTOs/UpdatePasswordDTO.cs around lines 5-6, the
CurrentPassword and UpdatedPassword properties are nullable and lack validation;
update them to be non-nullable by initializing to string.Empty and add basic
DataAnnotations (e.g., [Required] on both and a [MinLength] constraint as
appropriate) and include the System.ComponentModel.DataAnnotations using
directive so model binding enforces presence and minimal length.
| await _unitOfWork.SaveAsync(); | ||
| return await ServiceResult<bool>.Success(); | ||
| } |
There was a problem hiding this comment.
Return Success(true) to reflect an actual update
Currently returns default bool (false). Align with other methods returning true on success.
- await _unitOfWork.SaveAsync();
- return await ServiceResult<bool>.Success();
+ await _unitOfWork.SaveAsync();
+ return await ServiceResult<bool>.Success(true);📝 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.
| await _unitOfWork.SaveAsync(); | |
| return await ServiceResult<bool>.Success(); | |
| } | |
| await _unitOfWork.SaveAsync(); | |
| return await ServiceResult<bool>.Success(true); | |
| } |
🤖 Prompt for AI Agents
In AskFm/AskFm.BLL/Services/UserIdentityService/UserService.cs around lines 41
to 43, the method currently returns ServiceResult<bool>.Success() which yields
the default bool (false); change the return to ServiceResult<bool>.Success(true)
so the method returns true on successful update to match other service methods
and reflect the actual success state.
| Follow follow = new Follow() | ||
| { | ||
| FollowerId = followerId, | ||
| FollowedId = targetUserId, | ||
|
|
||
| }; | ||
|
|
||
| userFollower.FollowingCount++; | ||
| targetUser.FollowersCount++; | ||
| await _unitOfWork.Follows.AddAsync(follow); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
New follow should be active (and not deleted) on insert
Without setting flags, queries relying on IsActive may ignore new relations.
if (followExist == null)
{
Follow follow = new Follow()
{
FollowerId = followerId,
FollowedId = targetUserId,
+ IsActive = true,
+ IsDeleted = false
};
userFollower.FollowingCount++;
targetUser.FollowersCount++;
await _unitOfWork.Follows.AddAsync(follow);
}📝 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.
| Follow follow = new Follow() | |
| { | |
| FollowerId = followerId, | |
| FollowedId = targetUserId, | |
| }; | |
| userFollower.FollowingCount++; | |
| targetUser.FollowersCount++; | |
| await _unitOfWork.Follows.AddAsync(follow); | |
| if (followExist == null) | |
| { | |
| Follow follow = new Follow() | |
| { | |
| FollowerId = followerId, | |
| FollowedId = targetUserId, | |
| IsActive = true, | |
| IsDeleted = false | |
| }; | |
| userFollower.FollowingCount++; | |
| targetUser.FollowersCount++; | |
| await _unitOfWork.Follows.AddAsync(follow); | |
| } |
🤖 Prompt for AI Agents
In AskFm/AskFm.BLL/Services/UserIdentityService/UserService.cs around lines 81
to 90, the new Follow entity is created without initializing its active/deleted
flags so queries that filter by IsActive may ignore it; set the follow's
IsActive = true (and optionally IsDeleted = false if that flag exists in the
model) before calling _unitOfWork.Follows.AddAsync(follow) so the relation is
immediately considered active, and keep the existing increments to
FollowingCount/FollowersCount as-is.
| var user = _unitOfWork.Users.GetById(userId); | ||
| var res = await CheckNullObjectAsync<ReadUserDTO, ApplicationUser>(user); | ||
| if (!res.success) return res; | ||
|
|
||
| return await ServiceResult<ReadUserDTO>.Success(new ReadUserDTO() | ||
| { | ||
| Name = user.Name, | ||
| Email = user.Email, | ||
| LastSeen = user.LastSeen, | ||
| Bio = user.Bio, | ||
| AvatarPath = user.AvatarPath, | ||
| followerCount = user.FollowersCount | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Use async repository call; verify DTO property casing
- var user = _unitOfWork.Users.GetById(userId);
+ var user = await _unitOfWork.Users.GetByIdAsync(userId);Also confirm ReadUserDTO uses followerCount (camelCase) vs FollowerCount (PascalCase).
🏁 Script executed:
#!/bin/bash
# Verify ReadUserDTO property casing and definition
rg -n -C2 -g '!**/bin/**' -g '!**/obj/**' 'class\s+ReadUserDTO|followerCount|FollowerCount' --type csLength of output: 2127
Use async repository call and rename followerCount to PascalCase
- In
UserService.cs, replacewithvar user = _unitOfWork.Users.GetById(userId);
and mark the method asvar user = await _unitOfWork.Users.GetByIdAsync(userId);
async. - In
ReadUserDTO.cs, changetopublic int followerCount { get; set; }
then update all initializers and references (in both BLL services and the API controller) to usepublic int FollowersCount { get; set; }
FollowersCount.
🤖 Prompt for AI Agents
In AskFm/AskFm.BLL/Services/UserIdentityService/UserService.cs around lines
189-201, the repository call is synchronous and the DTO property uses
non-PascalCase; change the method to use the async repository call (await
_unitOfWork.Users.GetByIdAsync(userId)) and mark the service method async so you
await the user retrieval and subsequent null check, and rename the ReadUserDTO
property followerCount to FollowersCount (update the DTO class) and update this
initializer and every reference across BLL services and the API controller to
use FollowersCount.
…mments, threads and notifications
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
2 participants
#22
Tests:
TODO:
Summary by CodeRabbit
New Features
Changes
Bug Fixes
Documentation
Tests