[PM-19145] refactor organization service.import async #5800
Conversation
|
New Issues (1)Checkmarx found the following issues in this Pull Request
|
BTreston
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5800 +/- ##
==========================================
+ Coverage 47.52% 47.78% +0.26%
==========================================
Files 1666 1668 +2
Lines 74910 75103 +193
Branches 6736 6758 +22
==========================================
+ Hits 35599 35890 +291
+ Misses 37853 37746 -107
- Partials 1458 1467 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /// Contains the details for inviting the imported organization users. | ||
| /// </param> | ||
| /// <returns>Response from InviteOrganiationUsersAsync<see cref="InviteOrganizationUsersResponse"/></returns> | ||
| Task<CommandResult<InviteOrganizationUsersResponse>> InviteImportedOrganizationUsersAsync(InviteOrganizationUsersRequest request, Guid organizationId); |
There was a problem hiding this comment.
PUt org id into the request object
...Console/OrganizationFeatures/OrganizationUsers/InviteUsers/InviteOrganizationUsersCommand.cs
Show resolved
Hide resolved
| { | ||
| var plan = await _pricingClient.GetPlanOrThrow(organization.PlanType); | ||
| var inviteOrganization = new InviteOrganization(organization, plan); | ||
| var request = new InviteOrganizationUsersRequest(invites.ToArray(), inviteOrganization, organization.Id, DateTimeOffset.UtcNow); |
There was a problem hiding this comment.
Should this 3rd param (PerformedBy) be organization.Id or Guid.Empty?
| OrganizationUserImportData importUserData) | ||
| { | ||
|
|
||
| var hasStandaloneSecretsManager = await _paymentService.HasSecretsManagerStandalone(organization); |
There was a problem hiding this comment.
You don't need to do this. It is handled inside of InviteOrgUsers
| NewUsersSet = new HashSet<string>(newUsers?.Select(u => u.ExternalId) ?? new List<string>()), | ||
| ExistingUsers = existingUsers, | ||
| ExistingExternalUsers = GetExistingExternalUsers(existingUsers), | ||
| ExistingExternalUsersIdDict = GetExistingExternalUsers(existingUsers).ToDictionary(u => u.ExternalId, u => u.Id) |
There was a problem hiding this comment.
You could maybe move this to a constructor and have it pass the newUsers and existingUsers as parameters. That way you don't have to call GetExistingExternalUsers twice.
ExistingUsers = existingUsers,
ExistingExternalUsers = GetExistingExternalUsers(existingUsers),
ExistingExternalUsersIdDict = ExistingExternalUsers.ToDictionary(u => u.ExternalId, u => u.Id)
| events.AddRange(removeUsersSet.Select(u => ( | ||
| u, | ||
| EventType.OrganizationUser_Removed, | ||
| (DateTime?)DateTime.UtcNow |
There was a problem hiding this comment.
Instead of DateTime.UtcNow you can use the TimeProvider. It allows you to override it in tests.
There was a problem hiding this comment.
There are some spots where we have to continue using DateTime.UtcNow until other parts of the system are refactored (calling out the Groups object specifically). This is probably best left for a followup PR.
|
|
||
| foreach (var user in newAndExistingUsersIntersection) | ||
| { | ||
| var organizationUser = organizationUsers[existingUsersEmailsDict[user].Id]; |
There was a problem hiding this comment.
If the user doesn't exist, I think this will throw an exception. You want to use TryGetValue() instead
| ); | ||
| } | ||
|
|
||
| private async Task UpsertExistingUsers(Organization organization, |
There was a problem hiding this comment.
organization parameter is unused
| await OverwriteExisting(events, importUserData); | ||
| } | ||
|
|
||
| await UpsertExistingUsers(organization, newUsers, importUserData); |
There was a problem hiding this comment.
So the point of this step is to find all existing users that match the email provided and create new org users for them if they don't exist for this organization?
There was a problem hiding this comment.
Essentially we are just updating the existing org users (if any) to have the updated externalId that the corresponding user in the importData has.
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-19145
📔 Objective
Refactors import method as a command utilizing
InviteOrganizationUsersCommand📸 Screenshots
⏰ 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