clean up

src/Api/AdminConsole/Public/Controllers/OrganizationController.cs

@@ -20,20 +20,20 @@ public class OrganizationController : Controller
     private readonly IOrganizationService _organizationService;
     private readonly ICurrentContext _currentContext;
     private readonly GlobalSettings _globalSettings;
-    private readonly IImportOrganizationUserCommand _importOrganizationUserCommand;
+    private readonly IImportOrganizationUsersAndGroupsCommand _importOrganizationUsersAndGroupsCommand;
     private readonly IFeatureService _featureService;
 
     public OrganizationController(
         IOrganizationService organizationService,
         ICurrentContext currentContext,
         GlobalSettings globalSettings,
-        IImportOrganizationUserCommand importOrganizationUserCommand,
+        IImportOrganizationUsersAndGroupsCommand importOrganizationUsersAndGroupsCommand,
         IFeatureService featureService)
     {
         _organizationService = organizationService;
         _currentContext = currentContext;
         _globalSettings = globalSettings;
-        _importOrganizationUserCommand = importOrganizationUserCommand;
+        _importOrganizationUsersAndGroupsCommand = importOrganizationUsersAndGroupsCommand;
         _featureService = featureService;
     }
 
@@ -57,7 +57,7 @@ public class OrganizationController : Controller
 
         if (_featureService.IsEnabled(FeatureFlagKeys.ScimInviteUserOptimization))
         {
-            await _importOrganizationUserCommand.ImportAsync(
+            await _importOrganizationUsersAndGroupsCommand.ImportAsync(
                 _currentContext.OrganizationId.Value,
                 model.Groups.Select(g => g.ToImportedGroup(_currentContext.OrganizationId.Value)),
                 model.Members.Where(u => !u.Deleted).Select(u => u.ToImportedOrganizationUser()),

src/Core/AdminConsole/OrganizationFeatures/Import/IImportOrganizationUserAndGroupsCommand.cs

@@ -1,9 +1,11 @@
-using Bit.Core.AdminConsole.Models.Business;
+#nullable enable
+
+using Bit.Core.AdminConsole.Models.Business;
 using Bit.Core.Models.Business;
 
 namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces;
 
-public interface IImportOrganizationUserCommand
+public interface IImportOrganizationUsersAndGroupsCommand
 {
     Task ImportAsync(Guid organizationId,
         IEnumerable<ImportedGroup> groups,

src/Core/AdminConsole/OrganizationFeatures/Import/ImportOrganizationUsersAndGroupsCommand.cs

@@ -1,4 +1,6 @@
-using Bit.Core.AdminConsole.Entities;
+#nullable enable
+
+using Bit.Core.AdminConsole.Entities;
 using Bit.Core.AdminConsole.Models.Business;
 using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces;
 using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.InviteUsers;
@@ -16,11 +18,9 @@ using Bit.Core.Models.Data.Organizations.OrganizationUsers;
 using Bit.Core.Repositories;
 using Bit.Core.Services;
 
-#nullable enable
-
 namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers;
 
-public class ImportOrganizationUserCommand : IImportOrganizationUserCommand
+public class ImportOrganizationUsersAndGroupsCommand : IImportOrganizationUsersAndGroupsCommand
 {
     private readonly IOrganizationRepository _organizationRepository;
     private readonly IOrganizationUserRepository _organizationUserRepository;
@@ -34,7 +34,7 @@ public class ImportOrganizationUserCommand : IImportOrganizationUserCommand
 
     private readonly EventSystemUser _EventSystemUser = EventSystemUser.PublicApi;
 
-    public ImportOrganizationUserCommand(IOrganizationRepository organizationRepository,
+    public ImportOrganizationUsersAndGroupsCommand(IOrganizationRepository organizationRepository,
             IOrganizationUserRepository organizationUserRepository,
             IPaymentService paymentService,
             IGroupRepository groupRepository,
@@ -95,7 +95,7 @@ public class ImportOrganizationUserCommand : IImportOrganizationUserCommand
             await OverwriteExisting(events, importUserData);
         }
 
-        await Update(importedUsers, importUserData);
+        await UpdateExistingUsers(importedUsers, importUserData);
 
         await AddNewUsers(organization, importedUsers, importUserData);
 
@@ -144,7 +144,7 @@ public class ImportOrganizationUserCommand : IImportOrganizationUserCommand
     /// </summary>
     /// <param name="importedUsers">List of imported organization users.</param>
     /// <param name="importUserData">Data containing existing and imported users, along with mapping dictionaries.</param>
-    private async Task Update(IEnumerable<ImportedOrganizationUser> importedUsers, OrganizationUserImportData importUserData)
+    private async Task UpdateExistingUsers(IEnumerable<ImportedOrganizationUser> importedUsers, OrganizationUserImportData importUserData)
     {
         if (!importedUsers.Any())
         {
@@ -160,27 +160,24 @@ public class ImportOrganizationUserCommand : IImportOrganizationUserCommand
         var importedUsersEmailsDict = importedUsers.ToDictionary(u => u.Email);
 
         // Determine which users to update.
-        var userDetailsToUpdate = existingUsersEmailsDict.Keys.Intersect(importedUsersEmailsDict.Keys).ToList();
+        var userEmailsToUpdate = existingUsersEmailsDict.Keys.Intersect(importedUsersEmailsDict.Keys).ToList();
-        var userIdsToUpdate = importUserData.ExistingUsers
+        var userIdsToUpdate = userEmailsToUpdate.Select(e => existingUsersEmailsDict[e].Id).ToList();
-            .Where(u => userDetailsToUpdate.Contains(u.Email))
-            .Select(u => u.Id)
-            .ToList();
 
         var organizationUsers = (await _organizationUserRepository.GetManyAsync(userIdsToUpdate)).ToDictionary(u => u.Id);
 
-        foreach (var userEmail in userDetailsToUpdate)
+        foreach (var userEmail in userEmailsToUpdate)
         {
             // verify userEmail has an associated OrganizationUser
             existingUsersEmailsDict.TryGetValue(userEmail, out var existingUser);
             organizationUsers.TryGetValue(existingUser!.Id, out var organizationUser);
-            importedUsersEmailsDict.TryGetValue(userEmail, out var user);
+            importedUsersEmailsDict.TryGetValue(userEmail, out var importedUser);
 
-            if (organizationUser is null || user is null)
+            if (organizationUser is null || importedUser is null)
             {
                 continue;
             }
 
-            organizationUser.ExternalId = user.ExternalId;
+            organizationUser.ExternalId = importedUser.ExternalId;
             updateUsers.Add(organizationUser);
             importUserData.ExistingExternalUsersIdDict.Add(organizationUser.ExternalId, organizationUser.Id);
         }
@@ -302,11 +299,8 @@ public class ImportOrganizationUserCommand : IImportOrganizationUserCommand
     /// <param name="organization">The organization into which groups are being imported.</param>
     /// <param name="importedGroups">A collection of groups to be imported.</param>
     /// <param name="importUserData">Data containing information about existing and imported users.</param>
-    private async Task ImportGroups(Organization organization,
+    private async Task ImportGroups(Organization organization, IEnumerable<ImportedGroup> importedGroups, OrganizationUserImportData importUserData)
-            IEnumerable<ImportedGroup> importedGroups,
-            OrganizationUserImportData importUserData)
     {
-
         if (!importedGroups.Any())
         {
             return;
@@ -314,7 +308,7 @@ public class ImportOrganizationUserCommand : IImportOrganizationUserCommand
 
         if (!organization.UseGroups)
         {
-            throw new BadRequestException("Organization cannot use importedGroups.");
+            throw new BadRequestException("Organization cannot use groups.");
         }
 
         var existingGroups = await _groupRepository.GetManyByOrganizationIdAsync(organization.Id);
@@ -332,9 +326,11 @@ public class ImportOrganizationUserCommand : IImportOrganizationUserCommand
     /// <param name="importUserData">Data containing information about existing and imported users.</param>
     private async Task SaveNewGroups(OrganizationGroupImportData importGroupData, OrganizationUserImportData importUserData)
     {
+        var existingExternalGroupsDict = importGroupData.ExistingExternalGroups.ToDictionary(g => g.ExternalId!);
         var newGroups = importGroupData.Groups
-            .Where(g => !importGroupData.ExistingExternalGroups.ToDictionary(g => g.ExternalId!).ContainsKey(g.Group.ExternalId!))
+            .Where(g => !existingExternalGroupsDict.ContainsKey(g.Group.ExternalId!))
-            .Select(g => g.Group).ToList()!;
+            .Select(g => g.Group)
+            .ToList()!;
 
         var savedGroups = new List<Group>();
         foreach (var group in newGroups)

src/Core/AdminConsole/OrganizationFeatures/Import/OrganizationGroupImportData.cs

@@ -1,19 +1,40 @@
-using Bit.Core.AdminConsole.Entities;
+#nullable enable
+
+using Bit.Core.AdminConsole.Entities;
 using Bit.Core.AdminConsole.Models.Business;
 
 namespace Bit.Core.Models.Data.Organizations;
 
+/// <summary>
+/// Represents the data required to import organization groups,
+/// including newly imported groups and existing groups within the organization.
+/// </summary>
 public class OrganizationGroupImportData
 {
+    /// <summary>
+    /// The collection of groups that are being imported.
+    /// </summary>
     public readonly IEnumerable<ImportedGroup> Groups;
+
+    /// <summary>
+    /// Collection of groups that already exist in the organization.
+    /// </summary>
     public readonly ICollection<Group> ExistingGroups;
+
+    /// <summary>
+    /// Existing groups with ExternalId set.
+    /// </summary>
     public readonly IEnumerable<Group> ExistingExternalGroups;
+
+    /// <summary>
+    /// Mapping of imported groups keyed by their ExternalId.
+    /// </summary>
     public readonly IDictionary<string, ImportedGroup> GroupsDict;
 
     public OrganizationGroupImportData(IEnumerable<ImportedGroup> groups, ICollection<Group> existingGroups)
     {
         Groups = groups;
-        GroupsDict = groups.ToDictionary(g => g.Group.ExternalId);
+        GroupsDict = groups.ToDictionary(g => g.Group.ExternalId!);
         ExistingGroups = existingGroups;
         ExistingExternalGroups = existingGroups.Where(u => !string.IsNullOrWhiteSpace(u.ExternalId)).ToList();
     }

src/Core/AdminConsole/OrganizationFeatures/Import/OrganizationUserImportData.cs

@@ -1,4 +1,6 @@
-using Bit.Core.Models.Business;
+#nullable enable
+
+using Bit.Core.Models.Business;
 namespace Bit.Core.Models.Data.Organizations.OrganizationUsers;
 
 public class OrganizationUserImportData
@@ -8,11 +10,11 @@ public class OrganizationUserImportData
     /// </summary>
     public readonly HashSet<string> ImportedExternalIds;
     /// <summary>
-    /// Exising organization users details
+    /// All existing OrganizationUsers for the organization
     /// </summary>
     public readonly ICollection<OrganizationUserUserDetails> ExistingUsers;
     /// <summary>
-    /// List of ExternalIds belonging to existing organization Users
+    /// Existing OrganizationUsers with ExternalIds set.
     /// </summary>
     public readonly IEnumerable<OrganizationUserUserDetails> ExistingExternalUsers;
     /// <summary>

test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/ImportOrganizationUserCommandTests.cs

@@ -1,5 +1,4 @@
 using Bit.Core.AdminConsole.Models.Business;
-using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers;
 using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.InviteUsers;
 using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.InviteUsers.Models;
 using Bit.Core.AdminConsole.Utilities.Commands;