Feature/215/create an ability to change user password#242
Conversation
weazy12
requested review from
Markian-Grybok,
Mykola-Rad,
Skiper29,
VladysMoroz,
denys-pavskyi and
oleksandrhofner
WalkthroughAdds change-password capability: request/response DTOs, a MediatR command and handler, FluentValidation validators, an AuthController change-password endpoint, and unit tests for handler and validators. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant AuthController
participant Mediator
participant Handler
participant UserManager
Client->>AuthController: POST /api/auth/change-password (ChangePasswordRequestDto)
AuthController->>Mediator: Send(ChangePasswordCommand)
Mediator->>Handler: Handle(command)
Handler->>UserManager: Users.FirstOrDefault(u => u.Email == dto.Email)
alt user not found
Handler-->>Mediator: Result.Fail("User not found")
else user found
Handler->>UserManager: ChangePasswordAsync(user, oldPassword, newPassword)
alt change failed
Handler-->>Mediator: Result.Fail("Error! Can't change password")
else success
Handler-->>Mediator: Result.Ok(ChangePasswordResponseDto{IsSuccess=true, Message="Password changed successfully"})
end
end
Mediator-->>AuthController: Result
alt success
AuthController-->>Client: 200 OK (response DTO)
else failure
AuthController-->>Client: 400 Bad Request (errors)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)Streetcode/Streetcode.WebApi/Controllers/Auth/AuthController.cs (2)
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (20)
Streetcode/Streetcode.XUnitTest/BLL/Validators/Users/ChangeUserPassword/ChangeUserPasswordCommandValidatorTests.cs (2)
31-40: Positive path covered; consider asserting no child errors too.Optionally add
result.ShouldNotHaveAnyValidationErrors()to ensure no nested rules fire when DTO is valid.
42-51: Keep test comments in English.Replace the inline non-English comment to keep consistency across tests.
- var dto = new ChangePasswordRequestDto { NewPassword = "abc" }; // короткий пароль + var dto = new ChangePasswordRequestDto { NewPassword = "abc" }; // short passwordStreetcode/Streetcode.BLL/DTO/Users/ChangePassword/ChangePasswordResponseDto.cs (1)
9-13: DTO is fine; consider immutability.Using a record with init-only properties makes the response safer and clearer.
- public class ChangePasswordResponseDto - { - public bool IsSuccess { get; set; } - public string? Message { get; set; } - } + public sealed record ChangePasswordResponseDto + { + public bool IsSuccess { get; init; } + public string? Message { get; init; } + }Streetcode/Streetcode.BLL/DTO/Users/ChangePassword/ChangePasswordRequestDto.cs (1)
9-14: Consider adding compile-time guarantees.If targeting C# 11+, mark properties as
requiredto prevent partially-initialized DTOs.- public string Email { get; set; } = string.Empty; - public string OldPassword { get; set; } = string.Empty; - public string NewPassword { get; set; } = string.Empty; + public required string Email { get; set; } + public required string OldPassword { get; set; } + public required string NewPassword { get; set; }Streetcode/Streetcode.XUnitTest/BLL/Validators/Users/ChangeUserPassword/ChangeUserPasswordDtoValidatorTests.cs (2)
46-52: Boundary test overshoots; optional tighten.
new string('A', 21) + "1a"produces length 23; 21 is enough to fail >20.- var model = new ChangePasswordRequestDto { NewPassword = new string('A', 21) + "1a" }; + var model = new ChangePasswordRequestDto { NewPassword = new string('A', 19) + "1a" }; // exactly 21 chars
78-84: Add min/max boundary positives (optional).Consider tests for exactly 6 and exactly 20 chars to lock boundaries.
Streetcode/Streetcode.BLL/MediatR/Users/ChangePassword/ChangePasswordHandler.cs (5)
28-36: Prefer UserManager API for lookup; avoid querying Users directly.Use
FindByEmailAsyncfor normalization and provider-optimized lookup.- var user = _userManager.Users.FirstOrDefault(u => u.Email == request.changePasswordRequestDto.Email); + var user = await _userManager.FindByEmailAsync(request.changePasswordRequestDto.Email);If you rename the command property to PascalCase, update access accordingly.
38-45: Propagate Identity errors internally; keep client message generic (optional).Current generic message is fine for clients; consider logging
result.Errorsdetails.- _loggerService.LogError(request, errorMsg); + _loggerService.LogError(request, errorMsg); + // Optionally log individual identity errors here + // foreach (var e in result.Errors) _loggerService.LogError(request, e.Description);
9-14: Remove unused usings.
Streetcode.BLL.DTO.FeedbackandStreetcode.BLL.Resourcesare unused.-using Streetcode.BLL.DTO.Feedback; -using Streetcode.BLL.Resources;
28-52: Validate more than NewPassword (recommendation).Add checks ensuring Email and OldPassword are present/valid and that NewPassword != OldPassword. This belongs either here or in the command validator.
47-52: Wrap success in Result.Ok(...) and fix the apostropheReturn an explicit Result.Ok for the successful response and replace the backtick in the error message with an apostrophe; update the test that asserts the error string.
- return new ChangePasswordResponseDto - { - IsSuccess = true, - Message = "Password changed successfully" - }; + return Result.Ok(new ChangePasswordResponseDto + { + IsSuccess = true, + Message = "Password changed successfully" + });Change error string:
- "Error! Can`t change password" → "Error! Can't change password"
Files to update:
- Streetcode/Streetcode.BLL/MediatR/Users/ChangePassword/ChangePasswordHandler.cs
- Streetcode/Streetcode.XUnitTest/BLL/MediatR/Users/ChangePassword/ChangeUserPasswordHandlerTests.cs (update assertion for the error message)
Streetcode/Streetcode.BLL/Validators/Users/ChangeUserPasswordValidator/ChangeUserPasswordDtoValidator.cs (1)
16-23: Add missing validations for Email/OldPassword and strengthen password rule (optional).Currently only NewPassword is validated.
public ChangeUserPasswordDtoValidator() { - RuleFor(x => x.NewPassword) + RuleFor(x => x.Email) + .NotEmpty().WithMessage("Email is required") + .EmailAddress().WithMessage("Invalid email format"); + + RuleFor(x => x.OldPassword) + .NotEmpty().WithMessage("Old password is required"); + + RuleFor(x => x.NewPassword) .NotEmpty().WithMessage("Password is required") .MinimumLength(MinPasswordLength).WithMessage($"Password must be at least {MinPasswordLength} characters long") .MaximumLength(20).WithMessage("Password must not exceed 20 characters") .Matches("[A-Z]").WithMessage("Password must contain at least one uppercase letter") .Matches("[a-z]").WithMessage("Password must contain at least one lowercase letter") - .Matches("[0-9]").WithMessage("Password must contain at least one digit"); + .Matches("[0-9]").WithMessage("Password must contain at least one digit") + .Must(p => p is not null && !p.Contains(' ')).WithMessage("Password must not contain spaces"); + + RuleFor(x => x) + .Must(x => !string.Equals(x.NewPassword, x.OldPassword, StringComparison.Ordinal)) + .WithMessage("New password must differ from old password"); }Note: add/update tests if you adopt these rules.
Streetcode/Streetcode.BLL/Validators/Users/ChangeUserPasswordValidator/ChangeUserPasswordCommandValidator.cs (4)
1-5: Remove unused System usingsThese namespaces aren’t used here. Trim to reduce noise.
-using System; -using System.Collections.Generic; -using System.Linq; -using System.Text; -using System.Threading.Tasks; using FluentValidation; using Streetcode.BLL.MediatR.Users.ChangePassword; using Streetcode.BLL.Resources; using Streetcode.BLL.Util.Extensions; using Streetcode.BLL.Validators.Users.ChangeUserPasswordValidator;
12-13: Align namespace with sibling validatorsConsider consolidating under a consistent namespace (e.g., Streetcode.BLL.Validators.Users.ChangeUserPassword) to match ChangeUserPasswordDtoValidator and improve discoverability.
18-21: Avoid hard-coded token in validation messageUse nameof to prevent string drift if the property name changes.
- .WithMessage(Errors_Validation.IsRequiredData.FormatWith("ChangePasswordRequestDto")); + .WithMessage(Errors_Validation.IsRequiredData.FormatWith(nameof(ChangePasswordCommand.changePasswordRequestDto)));
22-26: Use DependentRules instead of When + nested RuleForThis keeps the null check and clearly scopes child validation to run only if NotNull passes.
- When(x => x.changePasswordRequestDto != null, () => - { - RuleFor(x => x.changePasswordRequestDto) - .SetValidator(new ChangeUserPasswordDtoValidator()); - }); + RuleFor(x => x.changePasswordRequestDto) + .NotNull() + .WithMessage(Errors_Validation.IsRequiredData.FormatWith(nameof(ChangePasswordCommand.changePasswordRequestDto))) + .DependentRules(() => + { + RuleFor(x => x.changePasswordRequestDto) + .SetValidator(new ChangeUserPasswordDtoValidator()); + });Streetcode/Streetcode.XUnitTest/BLL/MediatR/Users/ChangePassword/ChangeUserPasswordHandlerTests.cs (4)
16-21: Consider a test helper for UserManager setupA small factory/helper for Mock<UserManager> will cut duplication/noise across tests.
32-48: Also assert ChangePasswordAsync is not invoked when user is missingStrengthens behavior by ensuring no change attempt occurs without a user.
Assert.False(result.IsSuccess); Assert.Contains("User not found", result.Errors.Select(e => e.Message)); _mockLoggerService.Verify(l => l.LogError(command, "User not found"), Times.Once); + _mockUserManager.Verify( + u => u.ChangePasswordAsync(It.IsAny<User>(), It.IsAny<string>(), It.IsAny<string>()), + Times.Never);
51-70: Verify ChangePasswordAsync is called exactly once with expected argsEnsures correct invocation when the user exists but operation fails.
Assert.False(result.IsSuccess); Assert.Contains("Error! Can`t change password", result.Errors.Select(e => e.Message)); _mockLoggerService.Verify(l => l.LogError(command, "Error! Can`t change password"), Times.Once); + _mockUserManager.Verify( + u => u.ChangePasswordAsync(user, "oldPass123", "newPass123"), + Times.Once);
72-92: On success, assert no error logs and exactly one password-change callTightens the positive-path contract.
Assert.True(result.IsSuccess); Assert.Equal("Password changed successfully", result.Value.Message); + _mockUserManager.Verify( + u => u.ChangePasswordAsync(user, "oldPass123", "newPass123"), + Times.Once); + _mockLoggerService.Verify( + l => l.LogError(It.IsAny<object>(), It.IsAny<string>()), + Times.Never);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Streetcode/Streetcode.BLL/DTO/Users/ChangePassword/ChangePasswordRequestDto.cs(1 hunks)Streetcode/Streetcode.BLL/DTO/Users/ChangePassword/ChangePasswordResponseDto.cs(1 hunks)Streetcode/Streetcode.BLL/MediatR/Users/ChangePassword/ChangePasswordCommand.cs(1 hunks)Streetcode/Streetcode.BLL/MediatR/Users/ChangePassword/ChangePasswordHandler.cs(1 hunks)Streetcode/Streetcode.BLL/Validators/Users/ChangeUserPasswordValidator/ChangeUserPasswordCommandValidator.cs(1 hunks)Streetcode/Streetcode.BLL/Validators/Users/ChangeUserPasswordValidator/ChangeUserPasswordDtoValidator.cs(1 hunks)Streetcode/Streetcode.WebApi/Controllers/Auth/AuthController.cs(2 hunks)Streetcode/Streetcode.XUnitTest/BLL/MediatR/Users/ChangePassword/ChangeUserPasswordHandlerTests.cs(1 hunks)Streetcode/Streetcode.XUnitTest/BLL/Validators/Users/ChangeUserPassword/ChangeUserPasswordCommandValidatorTests.cs(1 hunks)Streetcode/Streetcode.XUnitTest/BLL/Validators/Users/ChangeUserPassword/ChangeUserPasswordDtoValidatorTests.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
Streetcode/Streetcode.BLL/Validators/Users/ChangeUserPasswordValidator/ChangeUserPasswordDtoValidator.cs (1)
Streetcode/Streetcode.BLL/DTO/Users/ChangePassword/ChangePasswordRequestDto.cs (1)
ChangePasswordRequestDto(9-14)
Streetcode/Streetcode.WebApi/Controllers/Auth/AuthController.cs (2)
Streetcode/Streetcode.BLL/MediatR/Users/ChangePassword/ChangePasswordHandler.cs (1)
Task(28-52)Streetcode/Streetcode.BLL/DTO/Users/ChangePassword/ChangePasswordRequestDto.cs (1)
ChangePasswordRequestDto(9-14)
Streetcode/Streetcode.XUnitTest/BLL/Validators/Users/ChangeUserPassword/ChangeUserPasswordCommandValidatorTests.cs (3)
Streetcode/Streetcode.XUnitTest/BLL/MediatR/Users/ChangePassword/ChangeUserPasswordHandlerTests.cs (3)
Fact(31-48)Fact(50-70)Fact(72-92)Streetcode/Streetcode.XUnitTest/BLL/Validators/Users/ChangeUserPassword/ChangeUserPasswordDtoValidatorTests.cs (8)
Fact(22-28)Fact(30-36)Fact(38-44)Fact(46-52)Fact(54-60)Fact(62-68)Fact(70-76)Fact(78-84)Streetcode/Streetcode.BLL/DTO/Users/ChangePassword/ChangePasswordRequestDto.cs (1)
ChangePasswordRequestDto(9-14)
Streetcode/Streetcode.BLL/MediatR/Users/ChangePassword/ChangePasswordCommand.cs (2)
Streetcode/Streetcode.BLL/DTO/Users/ChangePassword/ChangePasswordRequestDto.cs (1)
ChangePasswordRequestDto(9-14)Streetcode/Streetcode.BLL/DTO/Users/ChangePassword/ChangePasswordResponseDto.cs (1)
ChangePasswordResponseDto(9-13)
Streetcode/Streetcode.BLL/Validators/Users/ChangeUserPasswordValidator/ChangeUserPasswordCommandValidator.cs (2)
Streetcode/Streetcode.BLL/Util/Extensions/ResourceExtensions.cs (1)
FormatWith(5-8)Streetcode/Streetcode.BLL/Validators/Users/ChangeUserPasswordValidator/ChangeUserPasswordDtoValidator.cs (2)
ChangeUserPasswordDtoValidator(11-24)ChangeUserPasswordDtoValidator(14-23)
Streetcode/Streetcode.XUnitTest/BLL/Validators/Users/ChangeUserPassword/ChangeUserPasswordDtoValidatorTests.cs (2)
Streetcode/Streetcode.XUnitTest/BLL/Validators/Users/ChangeUserPassword/ChangeUserPasswordCommandValidatorTests.cs (3)
Fact(23-29)Fact(31-40)Fact(42-51)Streetcode/Streetcode.BLL/DTO/Users/ChangePassword/ChangePasswordRequestDto.cs (1)
ChangePasswordRequestDto(9-14)
Streetcode/Streetcode.BLL/MediatR/Users/ChangePassword/ChangePasswordHandler.cs (2)
Streetcode/Streetcode.BLL/DTO/Users/ChangePassword/ChangePasswordResponseDto.cs (1)
ChangePasswordResponseDto(9-13)Streetcode/Streetcode.BLL/Services/Logging/LoggerService.cs (1)
LogError(35-40)
Streetcode/Streetcode.XUnitTest/BLL/MediatR/Users/ChangePassword/ChangeUserPasswordHandlerTests.cs (3)
Streetcode/Streetcode.BLL/MediatR/Users/ChangePassword/ChangePasswordHandler.cs (3)
ChangePasswordHandler(17-54)ChangePasswordHandler(22-26)Task(28-52)Streetcode/Streetcode.BLL/DTO/Users/ChangePassword/ChangePasswordRequestDto.cs (1)
ChangePasswordRequestDto(9-14)Streetcode/Streetcode.BLL/Services/Logging/LoggerService.cs (1)
LogError(35-40)
🔇 Additional comments (3)
Streetcode/Streetcode.XUnitTest/BLL/Validators/Users/ChangeUserPassword/ChangeUserPasswordCommandValidatorTests.cs (1)
23-29: Good coverage for null DTO case.Asserts the not-null rule at the command level. LGTM.
Streetcode/Streetcode.XUnitTest/BLL/Validators/Users/ChangeUserPassword/ChangeUserPasswordDtoValidatorTests.cs (1)
22-28: Covers null case well.LGTM.
Streetcode/Streetcode.BLL/Validators/Users/ChangeUserPasswordValidator/ChangeUserPasswordCommandValidator.cs (1)
18-26: Confirm DTO validation also covers Email/OldPassword and old≠newDTO validator snippet only enforces NewPassword complexity. Ensure it also:
- Requires Email and OldPassword (and validates email format).
- Ensures NewPassword != OldPassword.
|
3 participants
dev
JIRA
Code reviewers
Second Level Review
Summary of issue
closed #215
Summary of change
ToDo
Testing approach
ToDo
CHECK LIST
Summary by CodeRabbit
New Features
Validation
Tests