[pm-15621] Refactor DeleteManagedOrganizationUserAccountCommand #5345
Conversation
...onsole/OrganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommand.cs
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5345 +/- ##
==========================================
+ Coverage 44.48% 44.52% +0.04%
==========================================
Files 1533 1533
Lines 71075 71082 +7
Branches 6377 6375 -2
==========================================
+ Hits 31618 31652 +34
+ Misses 38090 38067 -23
+ Partials 1367 1363 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
New Issues (7)Checkmarx found the following issues in this Pull Request
Fixed Issues (22)Great job! The following issues were fixed in this Pull Request
|
There was a problem hiding this comment.
Nice work @JimmyVo16 , I think this is an improvement. I also have some thoughts about the try/catch blocks here which I shared in our dev channel - I wanted to run it past the team before asking for it here, but let me know what you think.
...onsole/OrganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommand.cs
Outdated
Show resolved
Hide resolved
...onsole/OrganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommand.cs
Outdated
Show resolved
Hide resolved
...onsole/OrganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommand.cs
Outdated
Show resolved
Hide resolved
...onsole/OrganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommand.cs
Show resolved
Hide resolved
...onsole/OrganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommand.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Just a few minor items.
I think the userDeletionResults tuple makes sense (that is, we need to match these objects up and then pass them around together), but is very verbose and awkward. Definitely a case for better/stronger typed ValidationResult classes, which we should experiment with in the future (whoever gets there first, really).
...onsole/OrganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommand.cs
Outdated
Show resolved
Hide resolved
...onsole/OrganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommand.cs
Outdated
Show resolved
Hide resolved
...onsole/OrganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommand.cs
Outdated
Show resolved
Hide resolved
| private async Task LogDeletedOrganizationUsersAsync( | ||
| IEnumerable<OrganizationUser> orgUsers, | ||
| IEnumerable<(Guid OrgUserId, string? ErrorMessage)> results) | ||
| private async Task EnsureUserIsNotSoleOrganizationOwnerAsync(User user) |
There was a problem hiding this comment.
This overlaps with PreventOrganizationSoleOwnerDeletion (maybe what you meant to refer to in your comment above?)
PreventOrganizationSoleOwnerDeletionprevents you from deleting the sole owner of this organizationEnsureUserIsNotSoleOrganizationOwnerAsyncprevents you from deleting the sole owner of any organization
I suggest we can just rely on the latter given that it obviously includes the former. Both also require database calls, and we don't want to hit the db unnecessarily.
I also question whether PreventOrganizationSoleOwnerDeletion would ever be tripped, given that you can't delete yourself and only owners can delete other owners. If you're the sole owner, who would be able to delete you?
There was a problem hiding this comment.
After much thinking, yeah i think about right. EnsureUserIsNotSoleOrganizationOwnerAsync should handle all cases for use, so we can remove PreventOrganizationSoleOwnerDeletion.
(me thinking out loud) These logics are pretty complex. I wonder if there is a way to make them discoverable than deep in a validation method.
There was a problem hiding this comment.
(me thinking out loud) These logics are pretty complex. I wonder if there is a way to make them discoverable than deep in a validation method.
I would say validation is the primary source of complexity in our commands (and in old service code that should be commands). It's definitely a strong argument for making this code as simple and as easy to follow as possible.
In terms of discoverability/location, I believe Jared had previously mooted separate Validator classes which encapsulate the validation logic for a given command. Not sure if that makes it more discoverable, but it would provide a stronger separation than private methods.
...onsole/OrganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommand.cs
Outdated
Show resolved
Hide resolved
|
Also - tests are failing. At a glance, maybe just because the order of validation has changed? |
...onsole/OrganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommand.cs
Outdated
Show resolved
Hide resolved
…eleteManagedOrganizationUserAccountCommand.cs Co-authored-by: Thomas Rittson <31796059+eliykat@users.noreply.github.com>
…eleteManagedOrganizationUserAccountCommand.cs Co-authored-by: Thomas Rittson <31796059+eliykat@users.noreply.github.com>
...onsole/OrganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommand.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Great work @JimmyVo16 !
EDIT: putting a needs-qa label on it because it needs QA before merge. However it would be ideal if QA can hit it before we release the larger feature, can you please mention this to them?
JimmyVo16
dismissed
BTreston’s stale review
I addressed Brandon's comment, and I think he's out today, so I don't want this to block QA.
|
Update: QA found an edge case that I missed. Changes:
Testing for the edge case Testing.for.self.delete.movRegression testing Happy.path.for.multiple.user.deletions.movhappy.path.for.single.user.deletion.mov |
| if (exception != null) | ||
| { | ||
| throw exception; | ||
| } |
There was a problem hiding this comment.
This interface is inconsistent - DeleteUserAsync throws if there's a validation error, whereas DeleteManyUsersAsync returns validation results up to the caller. I think the latter is a better pattern.
I suggest that DeleteUserAsync can still call DeleteUsersAsync (how it was before), but it can just return the single result out to the caller; then the caller can handle it. (Which also makes the controller code consistent.)
This method would therefore return a Task<(Guid OrganizationUserId, string? ErrorMessage)>. Or in this case, the OrganizationUserId is a bit redundant, so maybe just a string? ErrorMessage. If you wanted to take this a step further you could use the CommandResult class instead of nullable strings, but I understand if you don't want to do that at this late stage.
There was a problem hiding this comment.
Yeah, the interfaces are inconsistent, but they're the existing interfaces. I am trying to avoid breaking them.
With that said, I do think using CommandResult is the right call here and moving away from using exceptions to handle validation errors, but I notice a problem with just returning the error message(s) because, for the single resource endpoint, the HTTP code will be different depending on the error message, unlike the multiple resources endpoint where the HTTP code is the same regardless of the error message.
I'm going to move this conversation to the AC team Slack channel, so we can iterate quicker.
| catch (Exception exception) | ||
| { | ||
| userDeletionResults.Add((orgUserId, orgUser, user, ex.Message)); | ||
| userDeletionResults.Add((orgUserId, orgUser, user, exception)); |
There was a problem hiding this comment.
I understand why you made this change, but given my feedback above I think it's unnecessary. This change also means the try/catch pattern is leaking out to other code (because we need it to give us the Exception) which I'd prefer not to do.
JimmyVo16
changed the title
JimmyVo16
changed the title
| return await _userRepository.GetManyAsync(userIds); | ||
| } | ||
|
|
||
| private async Task<CommandResult> ValidateAsync(Guid organizationId, OrganizationUser orgUser, User user, Guid? deletingUserId, IDictionary<Guid, bool> managementStatus) |
There was a problem hiding this comment.
@jrmccannon Check out this ValidateAsync
|
There was a problem hiding this comment.
I like the use of CommandResult and how the validators are executed. I left some comments in your related PR about the CommandResultValidator but otherwise this is looking good to me once tests are passing.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-15621
📔 Objective
📸 Screenshots
I think test coverage is sufficient here, and it's low risk since it's behind a feature, but I did do a quick smoke test by starting up the service.
⏰ 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