Audit entries rework #19345
Open
Audit entries rework #19345
Conversation
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
- Moved logic related to the IAuditEntryRepository from the AuditService to the new service - Introduced new Async methods - Using ids (for easier transition from the previous Write method) - Using keys - Moved and updated integration tests related to the audit entries to a new test class `AuditEntryServiceTests` - Added unit tests class `AuditEntryServiceTests` and added a few unit tests - Added migration to add columns for `performingUserKey` and `affectedUserKey` and convert existing user ids - Adjusted usages of the old AuditService.Write method to use the new one (mostly notification handlers)
lauraneto
commented
May 19, 2025
Zeegaan
requested changes
Jun 2, 2025
Comment on lines
377
to
+382
| private string FormatEmail(IMember? member) => | ||
| member == null ? string.Empty : member.Email.IsNullOrWhiteSpace() ? string.Empty : $"<{member.Email}>"; | ||
|
|
||
| private string FormatEmail(IUser user) => user == null ? string.Empty : user.Email.IsNullOrWhiteSpace() ? string.Empty : $"<{user.Email}>"; | ||
| private string FormatEmail(IUser? user) => user is not null && !user.Email.IsNullOrWhiteSpace() | ||
| ? $"<{user.Email}>" | ||
| : string.Empty; |
There was a problem hiding this comment.
Might be pedantic here, but couldn't we combine these methods into one with:
private string FormatEmail(string? email) => email is not null && !email.IsNullOrWhiteSpace() ? $"<{email}>" : string.Empty;
And then just pass in the user?.Email and member?.Email respectively
Comment on lines
+58
to
+66
| async Task<int> GetUserId(Guid userKey) | ||
| { | ||
| return userKey switch | ||
| { | ||
| _ when userKey == Constants.Security.UnknownUserKey => Constants.Security.UnknownUserId, | ||
| _ when await _userIdKeyResolver.TryGetAsync(userKey) is { Success: true } attempt => attempt.Result, | ||
| _ => Constants.Security.UnknownUserId, | ||
| }; | ||
| } |
There was a problem hiding this comment.
I'm personally not a fan of inline methods 🙈 I'd rather it be in its own method, and maybe even have a unit test or 2 covering the behavior 🫡
Also at least for me personally (this might be a old man take), I feel like the IF syntax here is a lot more readable as a human, and IIRC, it's not really a performance difference, as the compiler will optimize it.
private async Task<int> GetUserId(Guid userKey)
{
if (userKey == Constants.Security.UnknownUserKey)
{
return Constants.Security.UnknownUserId;
}
if (await _userIdKeyResolver.TryGetAsync(userKey) is {Success: true} attempt)
{
return attempt.Result;
}
return Constants.Security.UnknownUserId;
}
Comment on lines
+116
to
+124
| async Task<Guid?> GetUserKey(int userId) | ||
| { | ||
| return userId switch | ||
| { | ||
| Constants.Security.UnknownUserId => Constants.Security.UnknownUserKey, | ||
| _ when await _userIdKeyResolver.TryGetAsync(userId) is { Success: true } attempt => attempt.Result, | ||
| _ => Constants.Security.UnknownUserKey, | ||
| }; | ||
| } |
| @@ -264,115 +280,29 @@ public async Task<PagedModel<IAuditItem>> GetPagedItemsByUserAsync( | |||
| } | |||
|
|
|||
| /// <inheritdoc /> | |||
| public IAuditEntry Write(int performingUserId, string perfomingDetails, string performingIp, DateTime eventDateUtc, int affectedUserId, string? affectedDetails, string eventType, string eventDetails) | |||
| [Obsolete("Use AuditEntryService.WriteAsync() instead. Scheduled for removal in Umbraco 18.")] | |||
There was a problem hiding this comment.
Why can we remove the method in 18, but the constructor in 19 🤔
I think everything we obsolete in this PR, should be scheduled for the same major (19) 😁
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
IAuditEntryRepository(umbracoAudit) from theAuditServiceto the new serviceperformingUserKeyandaffectedUserKeyand convert existing user idsAuditEntryServiceTestsAuditEntryServiceTestsand added a few unit testsTask<Attempt<Guid>> TryGetAsync(int id)andTask<Attempt<int>> TryGetAsync(Guid key)methods toIUserIdKeyResolverAffected notification handlers that trigger audit logs
UserLoginSuccessNotificationUserLogoutSuccessNotificationUserLoginFailedNotificationUserForgotPasswordRequestedNotificationUserForgotPasswordChangedNotificationUserPasswordChangedNotificationUserPasswordResetNotificationMemberSavedNotificationMemberDeletedNotificationAssignedMemberRolesNotificationRemovedMemberRolesNotificationExportedMemberNotificationUserSavedNotificationUserDeletedNotificationUserGroupWithUsersSavedNotificationAssignedUserGroupPermissionsNotificationTesting
Run the project in the main branch and do a few user operations. Then, switch to this branch, perform the update and do additional user operations.
Check the database to ensure that:
❓ Pending questions ❓
SYSTEM,Unknown)?00000000-0000-0000-0000-000000000000or do we want it to be null? (both in the database and code)nullis preferred, some changes might be needed in theAddAsyncmethods (at the moment if the id is 0 or the overload accepting id is used and it cannot convert/find that id, it is saving as empty guid)