Refactor services and enhance user activity logging#123
Conversation
- Removed `GetSubmittedReviews` from `IReviewService` and `UpdateUserVerificationState` from `IVerificationService`. - Updated role parsing in `AuthService` to ensure correct user role retrieval. - Corrected spelling of "Lockdout" to "Lockedout" in user activity logging. - Added user activity logging for registration, email verification, password reset, order actions, and QA interactions. - Refactored `VerificationService` for improved constructor handling and removed unnecessary methods. - Updated `UserActivityService` to set a default fingerprint token. - Enhanced `UserActivities` enum with new activity types and corrected spelling errors.
WalkthroughThis change removes specific methods from the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Service (Order/QA/Review/Withdrawal)
participant UserActivityService
User->>Service: Perform Action (e.g., Place Order, Ask Question)
Service->>UserActivityService: CreateAsync(activityType, timestamp, userId)
UserActivityService-->>Service: Log Activity
Service-->>User: Return Result
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (2)
Dentizone.Domain/Enums/UserActivity.cs (1)
6-20: Remove unusedRegisterenum; retain distinct password-reset events
UserActivities.Registerisn’t referenced anywhere in the codebase and can be removed to avoid dead code.UserActivities.Registeredis used byAuthServiceto log completed registrations—keep it.- Both
PasswordReset(when the reset actually occurs) andPasswordResetRequested(when the reset email is sent) are used in different contexts—retain both.Proposed diff:
--- a/Dentizone.Domain/Enums/UserActivity.cs +++ b/Dentizone.Domain/Enums/UserActivity.cs @@ enum UserActivities - Register, Logout, PasswordReset, OrderPlaced, OrderCancelled, AccountVerified, Lockedout, EmailConfirmed, AnsweredQuestion, AskedQuestion, ReviewedOrder, WithdrawalRequestCreated, Registered, EmailVerificationSent, PasswordResetRequestedDentizone.Application/Services/Authentication/VerificationService.cs (1)
27-74: Consider logging user activity for verification session creation.The method creates a verification session but doesn't log this important user activity. Given the PR's focus on enhancing user activity logging, this would be a good candidate for activity tracking.
Add user activity logging at the end of the method:
var session = await diditApi.CreateSessionAsync(request, secretService.GetSecret("DiditApi")); await userService.SetKycStatusAsync(userId, KycStatus.NotSubmitted); + await userActivityService.LogAsync(userId, UserActivities.VerificationStarted, "KYC verification session created"); await mailService.Send(user.Email!, "Dentizone: Verification Started", // ... rest of email sending );
🧹 Nitpick comments (1)
Dentizone.Application/Services/Authentication/VerificationService.cs (1)
76-79: Consider adding user activity logging for verification status checks.This method retrieves verification status, which could be valuable to track for audit purposes and user activity monitoring.
Add logging before returning the result:
public async Task<SessionDecisionResponse> GetVerificationStatusAsync(string sessionId) { var result = await diditApi.GetSessionDecisionAsync(sessionId, secretService.GetSecret("DiditApi")); + // Note: You may need to extract userId from sessionId or pass it as parameter + // await userActivityService.LogAsync(userId, UserActivities.VerificationStatusChecked, "Verification status checked"); return result; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Dentizone.Application/Interfaces/IReviewService.cs(0 hunks)Dentizone.Application/Interfaces/IVerificationService.cs(0 hunks)Dentizone.Application/Services/Authentication/AuthService.cs(7 hunks)Dentizone.Application/Services/Authentication/VerificationService.cs(3 hunks)Dentizone.Application/Services/OrderService.cs(3 hunks)Dentizone.Application/Services/QAService.cs(4 hunks)Dentizone.Application/Services/ReviewService.cs(3 hunks)Dentizone.Application/Services/UserActivityService.cs(1 hunks)Dentizone.Application/Services/WithdrawalService.cs(2 hunks)Dentizone.Domain/Enums/UserActivity.cs(1 hunks)
💤 Files with no reviewable changes (2)
- Dentizone.Application/Interfaces/IReviewService.cs
- Dentizone.Application/Interfaces/IVerificationService.cs
🧰 Additional context used
🧠 Learnings (2)
Dentizone.Application/Services/ReviewService.cs (3)
Learnt from: gitnasr
PR: dentizone/api#104
File: Dentizone.Presentaion/Controllers/ReviewController.cs:24-24
Timestamp: 2025-06-29T09:03:23.566Z
Learning: In the Dentizone application, users cannot edit reviews once they are submitted. Only admins can update or delete reviews, likely for moderation purposes. This means review updates and deletions should be restricted to admin-only authorization rather than ownership-based authorization.
Learnt from: gitnasr
PR: dentizone/api#104
File: Dentizone.Presentaion/Controllers/PostsController.cs:60-61
Timestamp: 2025-06-29T09:04:16.858Z
Learning: The Dentizone application uses global middleware to handle exceptions, which means controller methods don't need individual try-catch blocks. Service layer exceptions are centrally caught and translated to appropriate HTTP responses through this middleware.
Learnt from: gitnasr
PR: dentizone/api#16
File: Dentizone.Infrastructure/Persistence/Configurations/OrderConfiguration.cs:1-1
Timestamp: 2025-05-28T11:22:49.051Z
Learning: In the Dentizone.Infrastructure project, the IsDeleted property from IBaseEntity is handled through Entity Framework interceptors rather than explicit configuration in individual entity configuration files. This means entity configurations don't need to explicitly configure the IsDeleted property.
Dentizone.Application/Services/Authentication/AuthService.cs (1)
Learnt from: gitnasr
PR: dentizone/api#104
File: Dentizone.Presentaion/Controllers/ReviewController.cs:24-24
Timestamp: 2025-06-29T09:03:23.566Z
Learning: In the Dentizone application, users cannot edit reviews once they are submitted. Only admins can update or delete reviews, likely for moderation purposes. This means review updates and deletions should be restricted to admin-only authorization rather than ownership-based authorization.
🧬 Code Graph Analysis (1)
Dentizone.Application/Services/ReviewService.cs (1)
Dentizone.Application/Services/BaseService.cs (1)
BaseService(10-72)
🔇 Additional comments (18)
Dentizone.Domain/Enums/UserActivity.cs (1)
12-12: Good spelling correction.The typo fix from "Lockdout" to "Lockedout" improves code quality and consistency.
Dentizone.Application/Services/Authentication/VerificationService.cs (1)
19-25: Primary constructor syntax is supported in net9.0
All project files target<TargetFramework>net9.0</TargetFramework>, which defaults to C# 12 (and even C# 13) without an explicit<LangVersion>. The primary‐constructor syntax inVerificationServiceis therefore valid—no changes needed.Dentizone.Application/Services/UserActivityService.cs (1)
23-23: Verify the rationale for hardcoding the fingerprint token.The fingerprint token is now hardcoded to "NON" instead of being retrieved from the request context service. This change should be verified to ensure it's intentional and aligns with the application's security and tracking requirements.
Consider whether this is:
- A temporary workaround that should be documented
- A permanent change due to fingerprint tracking being disabled
- A default value for scenarios where fingerprint retrieval fails
If this is temporary, consider adding a TODO comment to track the intended future implementation.
Dentizone.Application/Services/WithdrawalService.cs (2)
20-20: LGTM: Proper dependency injection for user activity service.The constructor correctly includes the
IUserActivityServicedependency to enable activity logging.
50-51: LGTM: Appropriate user activity logging for withdrawal requests.The user activity logging is correctly placed after successful withdrawal request creation and uses the appropriate enum value with proper parameters.
Dentizone.Application/Services/ReviewService.cs (3)
6-6: LGTM: Necessary using statement for UserActivities enum.The using statement is properly added to access the
UserActivitiesenum used in the activity logging.
20-20: LGTM: Proper dependency injection for user activity service.The constructor correctly includes the
IUserActivityServicedependency to enable activity logging.
41-41: LGTM: Appropriate user activity logging for order reviews.The user activity logging is correctly placed after successful review creation and uses the appropriate enum value with proper parameters.
Dentizone.Application/Services/OrderService.cs (1)
29-29: LGTM: Proper dependency injection for user activity service.The constructor correctly includes the
IUserActivityServicedependency to enable activity logging.Dentizone.Application/Services/Authentication/AuthService.cs (7)
39-39: Verify safety of role parsing change.The change from
FirstOrDefault()toFirst()removes the null-coalescing fallback. While there's a null/empty check above (lines 34-37), ensure this change doesn't introduce potential exceptions if the collection state changes unexpectedly.The null/empty check above should prevent issues, but verify that all code paths leading to this method maintain this invariant.
74-74: LGTM: Typo correction and proper user activity logging.The typo "Lockdout" has been corrected to "Lockedout" and the user activity logging is properly implemented for lockout events.
114-114: Verify safety of role parsing change.The change from
LastOrDefault()toLast()removes the null-coalescing fallback. While there's a null/empty check above (lines 95-98), ensure this change doesn't introduce potential exceptions if the collection state changes unexpectedly.The null/empty check above should prevent issues, but verify that all code paths leading to this method maintain this invariant.
148-148: LGTM: Proper user activity logging for registration.The user activity logging is correctly placed after successful user registration and uses the appropriate enum value with proper parameters.
205-206: LGTM: Improved email formatting and user activity logging.The email link formatting has been improved and user activity logging is properly implemented for email verification events.
223-224: LGTM: Improved email formatting and user activity logging.The email link formatting has been improved and user activity logging is properly implemented for password reset request events.
256-256: LGTM: Proper user activity logging for password reset.The user activity logging is correctly placed after successful password reset and uses the appropriate enum value with proper parameters.
Dentizone.Application/Services/QAService.cs (2)
15-21: LGTM: Constructor properly updated with new dependency.The constructor correctly accepts the new
IUserActivityServicedependency, maintaining the primary constructor pattern used in the class.
113-120: LGTM: Formatting improvements maintain functionality.The formatting changes to the
includesarray andFindAllBymethod call improve readability without affecting the logic.
| EmailConfirmed, | ||
| AnsweredQuestion, | ||
| AskedQuestion, | ||
| ReviewedOrder, | ||
| WithdrawalRequestCreated, | ||
| Registered, | ||
| EmailVerificationSent, | ||
| PasswordResetRequested |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address naming inconsistencies and potential duplicates.
The new activity additions are valuable for comprehensive user tracking, but there are some naming concerns:
-
Potential duplicates:
Register(line 6) vsRegistered(line 18) - these appear to represent the same user registration activityPasswordReset(line 8) vsPasswordResetRequested(line 20) - these might also be duplicates
-
Naming convention inconsistency:
- Mix of past tense (
EmailConfirmed,AnsweredQuestion), present tense (Register), and descriptive forms (WithdrawalRequestCreated)
- Mix of past tense (
Consider standardizing the naming convention and removing duplicates to improve API clarity.
public enum UserActivities
{
Login,
- Register,
Logout,
- PasswordReset,
OrderPlaced,
OrderCancelled,
AccountVerified,
Lockedout,
EmailConfirmed,
AnsweredQuestion,
AskedQuestion,
ReviewedOrder,
WithdrawalRequestCreated,
Registered,
EmailVerificationSent,
PasswordResetRequested
}🤖 Prompt for AI Agents
In Dentizone.Domain/Enums/UserActivity.cs around lines 6 to 20, there are
duplicate enum entries for user registration and password reset activities, and
inconsistent naming conventions mixing past tense, present tense, and
descriptive forms. To fix this, remove the duplicate entries by choosing either
Register or Registered and either PasswordReset or PasswordResetRequested, then
standardize all enum names to use a consistent tense and style, preferably past
tense for completed actions, to improve clarity and maintainability.
| public async Task<UserView> UpdateUserNationalId(string userId, string nationalId) | ||
| { | ||
| var output = await _userService.SetNationalId(userId, nationalId); | ||
| var output = await userService.SetNationalId(userId, nationalId); | ||
| return output; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add user activity logging for national ID updates.
Updating a user's national ID is a significant action that should be logged for security and audit purposes.
Add activity logging after the successful update:
public async Task<UserView> UpdateUserNationalId(string userId, string nationalId)
{
var output = await userService.SetNationalId(userId, nationalId);
+ await userActivityService.LogAsync(userId, UserActivities.NationalIdUpdated, "National ID updated");
return output;
}📝 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<UserView> UpdateUserNationalId(string userId, string nationalId) | |
| { | |
| var output = await _userService.SetNationalId(userId, nationalId); | |
| var output = await userService.SetNationalId(userId, nationalId); | |
| return output; | |
| } | |
| public async Task<UserView> UpdateUserNationalId(string userId, string nationalId) | |
| { | |
| var output = await userService.SetNationalId(userId, nationalId); | |
| await userActivityService.LogAsync(userId, UserActivities.NationalIdUpdated, "National ID updated"); | |
| return output; | |
| } |
🤖 Prompt for AI Agents
In Dentizone.Application/Services/Authentication/VerificationService.cs around
lines 82 to 86, the UpdateUserNationalId method updates the user's national ID
but lacks logging of this significant action. After successfully updating the
national ID, add a call to the user activity logging service to record this
event, including relevant details such as userId and the action performed, to
ensure proper security and audit tracking.
| public class VerificationService( | ||
| IDiditApi diditApi, | ||
| ISecretService secretService, | ||
| IAuthService authService, | ||
| IUserService userService, | ||
| IUserActivityService userActivityService, | ||
| IMailService mailService) : IVerificationService |
There was a problem hiding this comment.
Unused dependency injection detected.
The IUserActivityService userActivityService is injected in the constructor but never used in any of the methods. This suggests either incomplete implementation or dead code.
Based on the PR objectives to "enhance user activity logging," this service should likely be used to log verification-related activities such as session starts, verification status checks, and national ID updates.
Consider adding user activity logging in the relevant methods:
public async Task<CreateSessionResponse> StartSessionAsync(string userId)
{
var user = await authService.GetById(userId);
var domainUser = await userService.GetByIdAsync(userId);
+ await userActivityService.LogAsync(userId, UserActivities.VerificationStarted, "KYC verification session started");
// ... rest of the method
}📝 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 class VerificationService( | |
| IDiditApi diditApi, | |
| ISecretService secretService, | |
| IAuthService authService, | |
| IUserService userService, | |
| IUserActivityService userActivityService, | |
| IMailService mailService) : IVerificationService | |
| public async Task<CreateSessionResponse> StartSessionAsync(string userId) | |
| { | |
| var user = await authService.GetById(userId); | |
| var domainUser = await userService.GetByIdAsync(userId); | |
| await userActivityService.LogAsync(userId, UserActivities.VerificationStarted, "KYC verification session started"); | |
| // ... rest of the method | |
| } |
🤖 Prompt for AI Agents
In Dentizone.Application/Services/Authentication/VerificationService.cs around
lines 19 to 25, the IUserActivityService is injected but not used. To fix this,
identify key verification-related methods such as session starts, verification
status checks, and national ID updates, and add calls to IUserActivityService to
log these user activities appropriately. This will ensure the injected service
is utilized as intended for enhanced user activity logging.
| await cartService.ClearCartAsync(buyerId); | ||
|
|
||
| await transaction.CommitAsync(); | ||
| await userActivityService.CreateAsync(UserActivities.OrderPlaced, new DateTime(), buyerId); |
There was a problem hiding this comment.
Fix DateTime usage for user activity logging.
The new DateTime() creates a default DateTime value (01/01/0001 00:00:00) instead of the current timestamp. This should be DateTime.UtcNow or DateTime.Now to properly track when the activity occurred.
- await userActivityService.CreateAsync(UserActivities.OrderPlaced, new DateTime(), buyerId);
+ await userActivityService.CreateAsync(UserActivities.OrderPlaced, DateTime.UtcNow, buyerId);📝 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 userActivityService.CreateAsync(UserActivities.OrderPlaced, new DateTime(), buyerId); | |
| await userActivityService.CreateAsync(UserActivities.OrderPlaced, DateTime.UtcNow, buyerId); |
🤖 Prompt for AI Agents
In Dentizone.Application/Services/OrderService.cs at line 193, replace the
incorrect usage of new DateTime() with DateTime.UtcNow or DateTime.Now to log
the current timestamp for user activity. This change ensures the activity time
reflects the actual order placement time instead of the default minimum DateTime
value.
| job.ReviewQuestionAsync(question.Id, question.Text)); | ||
|
|
||
| await userActivity.CreateAsync(UserActivities.AskedQuestion, DateTime.UtcNow, askerId); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider error handling for user activity logging.
Similar to the AnswerQuestionAsync method, the user activity logging lacks error handling and could potentially fail the entire question creation operation.
Apply the same error handling pattern as suggested for the AnswerQuestionAsync method.
_backgroundJob.Enqueue<IMonitorJob>(job =>
job.ReviewQuestionAsync(question.Id, question.Text));
-await userActivity.CreateAsync(UserActivities.AskedQuestion, DateTime.UtcNow, askerId);
+try
+{
+ await userActivity.CreateAsync(UserActivities.AskedQuestion, DateTime.UtcNow, askerId);
+}
+catch (Exception ex)
+{
+ // Log the error but don't fail the operation
+ // Consider using ILogger to log the exception
+}🤖 Prompt for AI Agents
In Dentizone.Application/Services/QAService.cs around lines 67 to 69, the call
to userActivity.CreateAsync for logging the AskedQuestion activity lacks error
handling, which may cause the entire question creation to fail if logging fails.
Wrap this call in a try-catch block similar to the pattern used in
AnswerQuestionAsync, catching exceptions and handling them appropriately (e.g.,
logging the error) without interrupting the main flow.
| job.ReviewAnswerAsync(answer.Id, answer.Text)); | ||
| await userActivity.CreateAsync(UserActivities.AnsweredQuestion, DateTime.UtcNow, responderId); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider error handling for user activity logging.
The user activity logging is added appropriately after the core business logic, but lacks error handling. If userActivity.CreateAsync fails, it could cause the entire operation to fail even though the question was successfully answered.
Consider wrapping the activity logging in a try-catch block or making it fire-and-forget to prevent it from affecting the primary operation.
_backgroundJob.Enqueue<IMonitorJob>(job =>
job.ReviewAnswerAsync(answer.Id, answer.Text));
-await userActivity.CreateAsync(UserActivities.AnsweredQuestion, DateTime.UtcNow, responderId);
+try
+{
+ await userActivity.CreateAsync(UserActivities.AnsweredQuestion, DateTime.UtcNow, responderId);
+}
+catch (Exception ex)
+{
+ // Log the error but don't fail the operation
+ // Consider using ILogger to log the exception
+}📝 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.
| job.ReviewAnswerAsync(answer.Id, answer.Text)); | |
| await userActivity.CreateAsync(UserActivities.AnsweredQuestion, DateTime.UtcNow, responderId); | |
| _backgroundJob.Enqueue<IMonitorJob>(job => | |
| job.ReviewAnswerAsync(answer.Id, answer.Text)); | |
| try | |
| { | |
| await userActivity.CreateAsync(UserActivities.AnsweredQuestion, DateTime.UtcNow, responderId); | |
| } | |
| catch (Exception ex) | |
| { | |
| // Log the error but don't fail the operation | |
| // Consider using ILogger to log the exception | |
| } |
🤖 Prompt for AI Agents
In Dentizone.Application/Services/QAService.cs around lines 54 to 55, the call
to userActivity.CreateAsync lacks error handling, which could cause the entire
operation to fail if logging fails. To fix this, wrap the
userActivity.CreateAsync call in a try-catch block to catch and handle any
exceptions without affecting the main operation, or alternatively, make the call
fire-and-forget by not awaiting it and handling exceptions internally.
Modified OrderService to use DateTime.UtcNow for user activity timestamp. Removed GetSubmittedReviews and GetReceivedReviews methods from ReviewController, retaining only GetAllReviews for admin access.
|
1 participant
GetSubmittedReviewsfromIReviewServiceandUpdateUserVerificationStatefromIVerificationService.AuthServiceto ensure correct user role retrieval.VerificationServicefor improved constructor handling and removed unnecessary methods.UserActivityServiceto set a default fingerprint token.UserActivitiesenum with new activity types and corrected spelling errors.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Removals