-
Notifications
You must be signed in to change notification settings - Fork 1
Refactor services and enhance user activity logging #123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,36 +16,25 @@ public class Metadata | |||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| public class VerificationService(IDiditApi diditApi, ISecretService secretService, IAuthService authService, | ||||||||||||||||||||||||||
| IUserService userService, IMailService mailService) : IVerificationService | ||||||||||||||||||||||||||
| public class VerificationService( | ||||||||||||||||||||||||||
| IDiditApi diditApi, | ||||||||||||||||||||||||||
| ISecretService secretService, | ||||||||||||||||||||||||||
| IAuthService authService, | ||||||||||||||||||||||||||
| IUserService userService, | ||||||||||||||||||||||||||
| IUserActivityService userActivityService, | ||||||||||||||||||||||||||
| IMailService mailService) : IVerificationService | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| private readonly IDiditApi _diditApi = diditApi; | ||||||||||||||||||||||||||
| private readonly ISecretService _secretService = secretService; | ||||||||||||||||||||||||||
| private readonly IAuthService _authService = authService; | ||||||||||||||||||||||||||
| private readonly IUserService _userService = userService; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| private static Dictionary<string, KycStatus> MapVerificationStatusToEnum() | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| return new Dictionary<string, KycStatus> | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| { "approved", KycStatus.Approved }, | ||||||||||||||||||||||||||
| { "declined", KycStatus.Rejected }, | ||||||||||||||||||||||||||
| { "pending", KycStatus.Pending } | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| public async Task<CreateSessionResponse> StartSessionAsync(string userId) | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| var user = await _authService.GetById(userId); | ||||||||||||||||||||||||||
| var domainUser = await _userService.GetByIdAsync(userId); | ||||||||||||||||||||||||||
| var user = await authService.GetById(userId); | ||||||||||||||||||||||||||
| var domainUser = await userService.GetByIdAsync(userId); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if (domainUser.KycStatus.Equals(KycStatus.NotSubmitted.ToString())) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| throw new BadActionException("KYC is not submitted yet"); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if (domainUser.KycStatus.Equals(KycStatus.Approved.ToString())) | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| throw new BadActionException("Your account is Already Active"); | ||||||||||||||||||||||||||
|
|
@@ -54,7 +43,7 @@ public async Task<CreateSessionResponse> StartSessionAsync(string userId) | |||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| var request = new CreateSessionRequest | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| WorkflowId = _secretService.GetSecret("DiditWorkflowId"), | ||||||||||||||||||||||||||
| WorkflowId = secretService.GetSecret("DiditWorkflowId"), | ||||||||||||||||||||||||||
| VendorData = userId, | ||||||||||||||||||||||||||
| Callback = "https://dentizone.vercel.app/auth/kyc/status", | ||||||||||||||||||||||||||
| Metadata = JsonConvert.SerializeObject(new Metadata() | ||||||||||||||||||||||||||
|
|
@@ -72,38 +61,27 @@ public async Task<CreateSessionResponse> StartSessionAsync(string userId) | |||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| var session = await _diditApi.CreateSessionAsync(request, _secretService.GetSecret("DiditApi")); | ||||||||||||||||||||||||||
| await _userService.SetKycStatusAsync(userId, KycStatus.NotSubmitted); | ||||||||||||||||||||||||||
| var session = await diditApi.CreateSessionAsync(request, secretService.GetSecret("DiditApi")); | ||||||||||||||||||||||||||
| await userService.SetKycStatusAsync(userId, KycStatus.NotSubmitted); | ||||||||||||||||||||||||||
| await mailService.Send(user.Email!, "Dentizone: Verification Started", | ||||||||||||||||||||||||||
| "Thank you for starting the email verification process." + | ||||||||||||||||||||||||||
| " You can use this url to verify your identity" + | ||||||||||||||||||||||||||
| $" <a href=\"{session.Url}\">Verify Now</a>" | ||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| return session; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| public async Task<SessionDecisionResponse> GetVerificationStatusAsync(string sessionId) | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| return await _diditApi.GetSessionDecisionAsync(sessionId, _secretService.GetSecret("DiditApi")); | ||||||||||||||||||||||||||
| return await diditApi.GetSessionDecisionAsync(sessionId, secretService.GetSecret("DiditApi")); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| public async Task<UserView> UpdateUserVerificationState(string userId, string status) | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| if (!MapVerificationStatusToEnum().TryGetValue(status.ToLower(), out var kycStatus)) | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| throw new ArgumentException($"Invalid verification status: {status}"); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| var output = await _userService.SetKycStatusAsync(userId, kycStatus); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| return output; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| 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; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
82
to
86
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -26,6 +26,7 @@ internal class OrderService( | |||||
| IPaymentService paymentService, | ||||||
| ICartService cartService, | ||||||
| IHttpContextAccessor accessor, | ||||||
| IUserActivityService userActivityService, | ||||||
| AppDbContext dbContext) | ||||||
| : BaseService(accessor), IOrderService | ||||||
| { | ||||||
|
|
@@ -84,6 +85,7 @@ await mailService.Send(seller.Email, "Order Cancelled", | |||||
|
|
||||||
|
|
||||||
| var dto = mapper.Map<OrderViewDto>(order); | ||||||
| await userActivityService.CreateAsync(UserActivities.OrderCancelled, DateTime.UtcNow, order.BuyerId); | ||||||
| return dto; | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -188,6 +190,8 @@ await paymentService.CreateSaleTransaction( | |||||
| await cartService.ClearCartAsync(buyerId); | ||||||
|
|
||||||
| await transaction.CommitAsync(); | ||||||
| await userActivityService.CreateAsync(UserActivities.OrderPlaced, new DateTime(), buyerId); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix DateTime usage for user activity logging. The - await userActivityService.CreateAsync(UserActivities.OrderPlaced, new DateTime(), buyerId);
+ await userActivityService.CreateAsync(UserActivities.OrderPlaced, DateTime.UtcNow, buyerId);📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
|
|
||||||
| return result.Id; | ||||||
| } | ||||||
| catch (Exception) | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,7 +16,8 @@ public class QaService( | |||||||||||||||||||||||||||
| IMapper mapper, | ||||||||||||||||||||||||||||
| IAnswerRepository answerRepository, | ||||||||||||||||||||||||||||
| IQuestionRepository questionRepository, | ||||||||||||||||||||||||||||
| IBackgroundJobService _backgroundJob) | ||||||||||||||||||||||||||||
| IBackgroundJobService _backgroundJob, | ||||||||||||||||||||||||||||
| IUserActivityService userActivity) | ||||||||||||||||||||||||||||
| : IQaService | ||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||
| public async Task<AnswerViewDto> AnswerQuestionAsync(string questionId, CreateAnswerDto dto, string responderId) | ||||||||||||||||||||||||||||
|
|
@@ -50,7 +51,8 @@ public async Task<AnswerViewDto> AnswerQuestionAsync(string questionId, CreateAn | |||||||||||||||||||||||||||
| await questionRepository.UpdateAsync(question); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| _backgroundJob.Enqueue<IMonitorJob>(job => | ||||||||||||||||||||||||||||
| job.ReviewAnswerAsync(answer.Id, answer.Text)); | ||||||||||||||||||||||||||||
| job.ReviewAnswerAsync(answer.Id, answer.Text)); | ||||||||||||||||||||||||||||
| await userActivity.CreateAsync(UserActivities.AnsweredQuestion, DateTime.UtcNow, responderId); | ||||||||||||||||||||||||||||
|
Comment on lines
+54
to
+55
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||
| return mapper.Map<AnswerViewDto>(answer); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
@@ -62,7 +64,9 @@ public async Task<QuestionViewDto> AskQuestionAsync(CreateQuestionDto dto, strin | |||||||||||||||||||||||||||
| await questionRepository.CreateAsync(question); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| _backgroundJob.Enqueue<IMonitorJob>(job => | ||||||||||||||||||||||||||||
| job.ReviewQuestionAsync(question.Id, question.Text)); | ||||||||||||||||||||||||||||
| job.ReviewQuestionAsync(question.Id, question.Text)); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| await userActivity.CreateAsync(UserActivities.AskedQuestion, DateTime.UtcNow, askerId); | ||||||||||||||||||||||||||||
|
Comment on lines
+67
to
+69
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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 |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| return mapper.Map<QuestionViewDto>(question); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
@@ -107,13 +111,13 @@ public async Task DeleteQuestionAsync(string questionId) | |||||||||||||||||||||||||||
| public async Task<IEnumerable<QuestionViewDto>> GetQuestionsForPostAsync(string postId) | ||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||
| var includes = new Expression<Func<Question, object>>[] | ||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||
| q => q.Answer | ||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||
| q => q.Answer | ||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| var questions = await questionRepository.FindAllBy( | ||||||||||||||||||||||||||||
| q => q.PostId == postId && !q.IsDeleted, | ||||||||||||||||||||||||||||
| includes); | ||||||||||||||||||||||||||||
| q => q.PostId == postId && !q.IsDeleted, | ||||||||||||||||||||||||||||
| includes); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| return mapper.Map<IEnumerable<QuestionViewDto>>(questions); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,10 +8,15 @@ public enum UserActivities | |
| PasswordReset, | ||
| OrderPlaced, | ||
| OrderCancelled, | ||
| OrderCompleted, | ||
| ProfileUpdated, | ||
| AccountVerified, | ||
| Lockdout, | ||
| EmailConfirmed | ||
| Lockedout, | ||
| EmailConfirmed, | ||
| AnsweredQuestion, | ||
| AskedQuestion, | ||
| ReviewedOrder, | ||
| WithdrawalRequestCreated, | ||
| Registered, | ||
| EmailVerificationSent, | ||
| PasswordResetRequested | ||
|
Comment on lines
+13
to
+20
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Address naming inconsistencies and potential duplicates. The new activity additions are valuable for comprehensive user tracking, but there are some naming concerns:
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 |
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused dependency injection detected.
The
IUserActivityService userActivityServiceis 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
🤖 Prompt for AI Agents