fix: application user - #55
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces factory methods to ApplicationUser for creating doctor and patient users, makes properties immutable with private setters, and updates command handlers to use these factory methods instead of direct object initialization. A query handler also adds eager loading for Doctor.ApplicationUser relationships. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/Booking/Booking.Domain/Entities/ApplicationUser.cs (2)
15-15: Address nullable warnings for EF Core constructor.The private parameterless constructor leaves non-nullable
FirstNameandLastNameuninitialized. While this is expected for EF Core, you can suppress the warnings by initializing withnull!or adding#pragma warning disable.♻️ Proposed fix to suppress warnings
- public string FirstName { get; private set; } - public string LastName { get; private set; } + public string FirstName { get; private set; } = null!; + public string LastName { get; private set; } = null!;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Booking/Booking.Domain/Entities/ApplicationUser.cs` at line 15, The private parameterless constructor ApplicationUser() leaves the non-nullable properties FirstName and LastName uninitialized and triggers nullable warnings; modify the ApplicationUser() constructor to suppress warnings by initializing those properties with the null-forgiving operator (e.g., assign FirstName = null! and LastName = null!) or alternatively add a scoped pragma disable/restore for nullable warnings around the constructor, ensuring the changes reference the ApplicationUser class and its parameterless constructor.
27-42: Consider adding input validation to factory methods.Neither
CreateDoctornorCreatePatientvalidate their inputs. IfUpdatePersonalInforequires non-empty names, the factory methods should enforce the same invariants to maintain consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Booking/Booking.Domain/Entities/ApplicationUser.cs` around lines 27 - 42, The factory methods CreateDoctor and CreatePatient currently accept any inputs; add the same input validation/invariants used by UpdatePersonalInfo: check email, firstName and lastName are not null/empty (and validate email format if UpdatePersonalInfo enforces it), and validate phoneNumber if required; throw ArgumentNullException/ArgumentException on invalid values before constructing the ApplicationUser so the factories preserve class invariants (update ApplicationUser.CreateDoctor and ApplicationUser.CreatePatient and add/adjust unit tests accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/Booking/Booking.Application/Identity/Commands/RegisterUser/RegisterUserCommandHandler.cs`:
- Around line 23-30: The call to ApplicationUser.CreatePatient in
RegisterUserCommandHandler passes request.PhoneNumber and request.Address which
may be null while CreatePatient requires non-nullable strings; fix by either
validating these fields in the handler (e.g., ensure PhoneNumber/Address are
present and return/throw a validation error) or pass safe fallbacks (e.g.,
coalesce to string.Empty or a default value) before calling CreatePatient;
alternatively, if null is acceptable domain-wise, update
ApplicationUser.CreatePatient signature to accept string? for phoneNumber and
address and adjust its internals to handle nulls. Ensure you update the
RegisterUserCommandHandler call or the CreatePatient signature consistently and
keep all references aligned.
In `@src/Booking/Booking.Domain/Entities/ApplicationUser.cs`:
- Around line 49-52: In ApplicationUser (ApplicationUser.cs) the validation
logic is inverted: change the conditions to throw when name or lastname are
null/empty (use string.IsNullOrEmpty(name) and string.IsNullOrEmpty(lastname))
instead of the current negated checks, and throw ArgumentException with a clear
parameter name/message (e.g., "Name cannot be empty." and "Last name cannot be
empty." or use nameof(name)/nameof(lastname)) in the constructor or setter that
currently contains those lines.
- Around line 59-64: The public factory method ApplicationUser.CreatePatient has
a typo in its parameter name "adress" — rename the parameter to "address" in the
CreatePatient signature and update all uses inside the ApplicationUser class
(and any internal parameter forwarding) to use the corrected name; then update
all call sites that pass that argument to use "address" to avoid breaking the
public API surface while keeping behavior unchanged.
---
Duplicate comments:
In
`@src/Booking/Booking.Application/Doctors/Command/UpdateDoctor/UpdateDoctorCommandHandler.cs`:
- Around line 48-51: The call to doctor.ApplicationUser.UpdatePersonalInfo in
UpdateDoctorCommandHandler.cs fails because UpdatePersonalInfo in
ApplicationUser.cs has inverted validation (it throws for valid Name/Lastname
and allows invalid ones); open ApplicationUser.cs, locate the UpdatePersonalInfo
method and invert the validation conditions so it only throws when inputs are
invalid (e.g., null/empty, wrong length or characters) rather than when they are
valid; ensure the method accepts the request.Name, request.Lastname,
request.PhoneNumber parameters and only raises exceptions on real validation
failures so UpdateDoctorCommandHandler's call succeeds.
---
Nitpick comments:
In `@src/Booking/Booking.Domain/Entities/ApplicationUser.cs`:
- Line 15: The private parameterless constructor ApplicationUser() leaves the
non-nullable properties FirstName and LastName uninitialized and triggers
nullable warnings; modify the ApplicationUser() constructor to suppress warnings
by initializing those properties with the null-forgiving operator (e.g., assign
FirstName = null! and LastName = null!) or alternatively add a scoped pragma
disable/restore for nullable warnings around the constructor, ensuring the
changes reference the ApplicationUser class and its parameterless constructor.
- Around line 27-42: The factory methods CreateDoctor and CreatePatient
currently accept any inputs; add the same input validation/invariants used by
UpdatePersonalInfo: check email, firstName and lastName are not null/empty (and
validate email format if UpdatePersonalInfo enforces it), and validate
phoneNumber if required; throw ArgumentNullException/ArgumentException on
invalid values before constructing the ApplicationUser so the factories preserve
class invariants (update ApplicationUser.CreateDoctor and
ApplicationUser.CreatePatient and add/adjust unit tests accordingly).
Comment on lines
+23
to
+30
| var user = ApplicationUser.CreatePatient | ||
| ( | ||
| firstName: request.UserName, | ||
| lastName: request.UserSurname, | ||
| email: request.UserEmail, | ||
| phoneNumber: request.PhoneNumber, | ||
| adress: request.Address | ||
| ); |
Contributor
There was a problem hiding this comment.
Handle potential null values for PhoneNumber and Address.
Static analysis warns that request.PhoneNumber and request.Address may be null, but CreatePatient expects non-nullable strings. Either validate these fields are required or update the factory method to accept nullable parameters.
🛡️ Option 1: Provide fallback values
var user = ApplicationUser.CreatePatient
(
firstName: request.UserName,
lastName: request.UserSurname,
email: request.UserEmail,
- phoneNumber: request.PhoneNumber,
- adress: request.Address
+ phoneNumber: request.PhoneNumber ?? string.Empty,
+ adress: request.Address ?? string.Empty
);Alternatively, update CreatePatient to accept string? for these parameters.
📝 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.
Suggested change
| var user = ApplicationUser.CreatePatient | |
| ( | |
| firstName: request.UserName, | |
| lastName: request.UserSurname, | |
| email: request.UserEmail, | |
| phoneNumber: request.PhoneNumber, | |
| adress: request.Address | |
| ); | |
| var user = ApplicationUser.CreatePatient | |
| ( | |
| firstName: request.UserName, | |
| lastName: request.UserSurname, | |
| email: request.UserEmail, | |
| phoneNumber: request.PhoneNumber ?? string.Empty, | |
| adress: request.Address ?? string.Empty | |
| ); |
🧰 Tools
🪛 GitHub Check: build
[warning] 29-29:
Possible null reference argument for parameter 'adress' in 'ApplicationUser ApplicationUser.CreatePatient(string firstName, string lastName, string email, string phoneNumber, string adress)'.
[warning] 28-28:
Possible null reference argument for parameter 'phoneNumber' in 'ApplicationUser ApplicationUser.CreatePatient(string firstName, string lastName, string email, string phoneNumber, string adress)'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/Booking/Booking.Application/Identity/Commands/RegisterUser/RegisterUserCommandHandler.cs`
around lines 23 - 30, The call to ApplicationUser.CreatePatient in
RegisterUserCommandHandler passes request.PhoneNumber and request.Address which
may be null while CreatePatient requires non-nullable strings; fix by either
validating these fields in the handler (e.g., ensure PhoneNumber/Address are
present and return/throw a validation error) or pass safe fallbacks (e.g.,
coalesce to string.Empty or a default value) before calling CreatePatient;
alternatively, if null is acceptable domain-wise, update
ApplicationUser.CreatePatient signature to accept string? for phoneNumber and
address and adjust its internals to handle nulls. Ensure you update the
RegisterUserCommandHandler call or the CreatePatient signature consistently and
keep all references aligned.
Comment on lines
+49
to
+52
| if (!string.IsNullOrEmpty(name)) | ||
| throw new ArgumentException("Name cannot be empty."); | ||
| if (!string.IsNullOrEmpty(lastname)) | ||
| throw new ArgumentException("Last name cannot be empty."); |
Contributor
There was a problem hiding this comment.
Critical bug: Validation logic is inverted.
The conditions throw when name and lastname are not empty, which is backwards. Valid inputs will cause exceptions, while null/empty values will be accepted.
🐛 Proposed fix to correct the validation logic
public void UpdatePersonalInfo(
string name,
string lastname,
string? phoneNumber)
{
- if (!string.IsNullOrEmpty(name))
+ if (string.IsNullOrEmpty(name))
throw new ArgumentException("Name cannot be empty.");
- if (!string.IsNullOrEmpty(lastname))
+ if (string.IsNullOrEmpty(lastname))
throw new ArgumentException("Last name cannot be empty.");
FirstName = name;
LastName = lastname;
PhoneNumber = phoneNumber;
}📝 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.
Suggested change
| if (!string.IsNullOrEmpty(name)) | |
| throw new ArgumentException("Name cannot be empty."); | |
| if (!string.IsNullOrEmpty(lastname)) | |
| throw new ArgumentException("Last name cannot be empty."); | |
| public void UpdatePersonalInfo( | |
| string name, | |
| string lastname, | |
| string? phoneNumber) | |
| { | |
| if (string.IsNullOrEmpty(name)) | |
| throw new ArgumentException("Name cannot be empty."); | |
| if (string.IsNullOrEmpty(lastname)) | |
| throw new ArgumentException("Last name cannot be empty."); | |
| FirstName = name; | |
| LastName = lastname; | |
| PhoneNumber = phoneNumber; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Booking/Booking.Domain/Entities/ApplicationUser.cs` around lines 49 - 52,
In ApplicationUser (ApplicationUser.cs) the validation logic is inverted: change
the conditions to throw when name or lastname are null/empty (use
string.IsNullOrEmpty(name) and string.IsNullOrEmpty(lastname)) instead of the
current negated checks, and throw ArgumentException with a clear parameter
name/message (e.g., "Name cannot be empty." and "Last name cannot be empty." or
use nameof(name)/nameof(lastname)) in the constructor or setter that currently
contains those lines.
Comment on lines
+59
to
+64
| public static ApplicationUser CreatePatient( | ||
| string firstName, | ||
| string lastName, | ||
| string email, | ||
| string phoneNumber, | ||
| string adress) |
Contributor
There was a problem hiding this comment.
Typo in parameter name: adress should be address.
This typo in the public API surface will propagate to all callers.
✏️ Proposed fix
public static ApplicationUser CreatePatient(
string firstName,
string lastName,
string email,
string phoneNumber,
- string adress)
+ string address)
{
return new ApplicationUser
{
UserName = email,
Email = email,
FirstName = firstName,
LastName = lastName,
PhoneNumber = phoneNumber,
- Address = adress,
+ Address = address,
EmailConfirmed = 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.
Suggested change
| public static ApplicationUser CreatePatient( | |
| string firstName, | |
| string lastName, | |
| string email, | |
| string phoneNumber, | |
| string adress) | |
| public static ApplicationUser CreatePatient( | |
| string firstName, | |
| string lastName, | |
| string email, | |
| string phoneNumber, | |
| string address) | |
| { | |
| return new ApplicationUser | |
| { | |
| UserName = email, | |
| Email = email, | |
| FirstName = firstName, | |
| LastName = lastName, | |
| PhoneNumber = phoneNumber, | |
| Address = address, | |
| EmailConfirmed = true | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Booking/Booking.Domain/Entities/ApplicationUser.cs` around lines 59 - 64,
The public factory method ApplicationUser.CreatePatient has a typo in its
parameter name "adress" — rename the parameter to "address" in the CreatePatient
signature and update all uses inside the ApplicationUser class (and any internal
parameter forwarding) to use the corrected name; then update all call sites that
pass that argument to use "address" to avoid breaking the public API surface
while keeping behavior unchanged.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Update application user domain; add some DDD architecture. Implement code base to handler
Summary by CodeRabbit
Bug Fixes
Performance