[PM-26376] Emergency Access Delete Command #6857
Conversation
…UserFeatures space.
…single responsibility.
|
Claude finished @ike-kottlowski's task in 2m 48s —— View job Code Review: PM-26376 Emergency Access Delete CommandReviewing PR #6857 - Re-review following latest commit (d603a87) SummaryThis PR implements a Previous Findings Status
Latest Commit Review (d603a87)The latest commit removes the explicit
Implementation QualitySecurity
Correctness
Test Coverage
Minor Observations (Non-blocking)
Files Reviewed: 21 changed files across command, interface, repository, mail templates, SQL, and tests Status: All previous issues resolved. PR is ready for merge. Generated with Claude Code |
src/Core/Auth/UserFeatures/EmergencyAccess/Commands/DeleteEmergencyAccessCommand.cs
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6857 +/- ##
==========================================
+ Coverage 56.05% 60.02% +3.96%
==========================================
Files 1967 1969 +2
Lines 86901 87007 +106
Branches 7737 7749 +12
==========================================
+ Hits 48714 52222 +3508
+ Misses 36386 32899 -3487
- Partials 1801 1886 +85 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Great job! No new security vulnerabilities introduced in this pull request |
src/Core/Auth/UserFeatures/EmergencyAccess/Commands/DeleteEmergencyAccessCommand.cs
Outdated
Show resolved
Hide resolved
test/Core.Test/Auth/UserFeatures/EmergencyAccess/DeleteEmergencyAccessCommandTests.cs
Show resolved
Hide resolved
test: add database tests for new SP
…ntors. Modified tests to validate.
| SET @BatchSize = @@ROWCOUNT | ||
|
|
||
| END | ||
| END |
There was a problem hiding this comment.
❓ Does the account revision date need to be bumped after the deletes? Some of our other "DeleteMany" procs do a revision bump and some don't..
| var (grantorEmails, granteeEmails) = await DeleteEmergencyAccessAsync(emergencyAccessDetails); | ||
|
|
||
| // Send notification email to grantor | ||
| await SendEmergencyAccessRemoveGranteesEmailAsync(grantorEmails, granteeEmails); ; |
There was a problem hiding this comment.
⛏️ Double semicolon
| var emergencyAccessDetails = await _emergencyAccessRepository.GetManyDetailsByGrantorIdAsync(grantorId); | ||
|
|
||
| // if there is nothing return an empty array and do not send an email | ||
| if (emergencyAccessDetails == null || emergencyAccessDetails.Count == 0) |
There was a problem hiding this comment.
⛏️ Unnecessary null check. You have a test to make sure that no results comes back as an empty list anywho, emergencyAccessDetails can't ever be null. There are 2 places in this file this check is done and can be removed.
There was a problem hiding this comment.
Couple of small nitpicks, nothing blocking 👍
🎟️ Tracking
PM-26376
📔 Objective
On our path to accomplishing the CQRS ADR we are taking a pit stop at the Emergency Access feature. This work is to enable the Admin Console team to enforce an Organization policy that will un-enroll users from the emergency access feature.
I have added a simple
DeleteManyByIdstored procedure that batches, if need be, the records for deletion.Things to note:
GranteeIdwe loop over the grantor emails sending one at a time, so we don't accidentlyccall the grantors for a given grantee.nullablefor classes that felt like low hanging fruit, which is why other teams are tagged.⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes