From 6dea40c8686511ced2295e60463fc55c1d9d8e2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rui=20Tom=C3=A9?= <108268980+r-tome@users.noreply.github.com> Date: Tue, 29 Jul 2025 15:04:00 +0100 Subject: [PATCH] [PM-23987] Fix saving to default collections by updating collection lookup (#6122) * Refactor ICollectionRepository.GetManyByOrganizationIdAsync logic to include default user collections * Add stored procedure Collection_ReadSharedCollectionsByOrganizationId to retrieve collections by organization ID, excluding default user collections. * Add GetManySharedCollectionsByOrganizationIdAsync method to ICollectionRepository and its implementations to retrieve collections excluding default user collections. * Add unit test for GetManySharedCollectionsByOrganizationIdAsync method in CollectionRepositoryTests to verify retrieval of collections excluding default user collections. * Refactor controllers to use GetManySharedCollectionsByOrganizationIdAsync for retrieving shared collections * Update unit tests to use GetManySharedCollectionsByOrganizationIdAsync for verifying shared collections retrieval * Revert CiphersController.CanEditItemsInCollections to use GetManyByOrganizationIdAsync for retrieving organization collections * Update stored procedures to retrieve only DefaultUserCollection by modifying the WHERE clause in Collection_ReadSharedCollectionsByOrganizationId.sql and its corresponding migration script. * Update EF CollectionRepository.GetManySharedCollectionsByOrganizationIdAsync to filter collections by SharedCollection * Update OrganizationUserRepository.GetManyDetailsByOrganizationAsync_vNext to only include Shared collections * Update comments in stored procedure and migration script to clarify filtering for SharedCollections only --- src/Api/Controllers/CollectionsController.cs | 2 +- .../Controllers/CollectionsController.cs | 2 +- .../Repositories/ICollectionRepository.cs | 8 ++- .../Repositories/CollectionRepository.cs | 13 ++++ .../OrganizationUserRepository.cs | 16 +++-- .../Repositories/CollectionRepository.cs | 15 ++++- .../Collection_ReadByOrganizationId.sql | 3 +- ..._ReadSharedCollectionsByOrganizationId.sql | 14 ++++ ...serUserDetails_ReadByOrganizationId_V2.sql | 4 +- .../Controllers/CollectionsControllerTests.cs | 4 +- .../CollectionRepositoryTests.cs | 67 ++++++++++++++++++- .../OrganizationUserRepositoryTests.cs | 19 ++++++ .../2025-07-24_00_ReadSharedCollections.sql | 65 ++++++++++++++++++ 13 files changed, 213 insertions(+), 19 deletions(-) create mode 100644 src/Sql/dbo/Stored Procedures/Collection_ReadSharedCollectionsByOrganizationId.sql create mode 100644 util/Migrator/DbScripts/2025-07-24_00_ReadSharedCollections.sql diff --git a/src/Api/Controllers/CollectionsController.cs b/src/Api/Controllers/CollectionsController.cs index 30d007a57e..6708a66326 100644 --- a/src/Api/Controllers/CollectionsController.cs +++ b/src/Api/Controllers/CollectionsController.cs @@ -109,7 +109,7 @@ public class CollectionsController : Controller var readAllAuthorized = (await _authorizationService.AuthorizeAsync(User, CollectionOperations.ReadAll(orgId))).Succeeded; if (readAllAuthorized) { - orgCollections = await _collectionRepository.GetManyByOrganizationIdAsync(orgId); + orgCollections = await _collectionRepository.GetManySharedCollectionsByOrganizationIdAsync(orgId); } else { diff --git a/src/Api/Public/Controllers/CollectionsController.cs b/src/Api/Public/Controllers/CollectionsController.cs index 836fe3a4f9..8615113906 100644 --- a/src/Api/Public/Controllers/CollectionsController.cs +++ b/src/Api/Public/Controllers/CollectionsController.cs @@ -65,7 +65,7 @@ public class CollectionsController : Controller [ProducesResponseType(typeof(ListResponseModel), (int)HttpStatusCode.OK)] public async Task List() { - var collections = await _collectionRepository.GetManyByOrganizationIdAsync( + var collections = await _collectionRepository.GetManySharedCollectionsByOrganizationIdAsync( _currentContext.OrganizationId.Value); // TODO: Get all CollectionGroup associations for the organization and marry them up here for the response. var collectionResponses = collections.Select(c => new CollectionResponseModel(c, null)); diff --git a/src/Core/Repositories/ICollectionRepository.cs b/src/Core/Repositories/ICollectionRepository.cs index da4f6aa580..9e2f253c9f 100644 --- a/src/Core/Repositories/ICollectionRepository.cs +++ b/src/Core/Repositories/ICollectionRepository.cs @@ -16,10 +16,16 @@ public interface ICollectionRepository : IRepository /// /// Return all collections that belong to the organization. Does not include any permission details or group/user - /// access relationships. Excludes default collections (My Items collections). + /// access relationships. /// Task> GetManyByOrganizationIdAsync(Guid organizationId); + /// + /// + /// Excludes default collections (My Items collections) - used by Admin Console Collections tab. + /// + Task> GetManySharedCollectionsByOrganizationIdAsync(Guid organizationId); + /// /// Return all shared collections that belong to the organization. Includes group/user access relationships for each collection. /// diff --git a/src/Infrastructure.Dapper/Repositories/CollectionRepository.cs b/src/Infrastructure.Dapper/Repositories/CollectionRepository.cs index 2e2c90d399..6b71b57e3d 100644 --- a/src/Infrastructure.Dapper/Repositories/CollectionRepository.cs +++ b/src/Infrastructure.Dapper/Repositories/CollectionRepository.cs @@ -79,6 +79,19 @@ public class CollectionRepository : Repository, ICollectionRep } } + public async Task> GetManySharedCollectionsByOrganizationIdAsync(Guid organizationId) + { + using (var connection = new SqlConnection(ConnectionString)) + { + var results = await connection.QueryAsync( + $"[{Schema}].[{Table}_ReadSharedCollectionsByOrganizationId]", + new { OrganizationId = organizationId }, + commandType: CommandType.StoredProcedure); + + return results.ToList(); + } + } + public async Task>> GetManyByOrganizationIdWithAccessAsync(Guid organizationId) { using (var connection = new SqlConnection(ConnectionString)) diff --git a/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs b/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs index a6bbf8e6e0..9c5f2a39cd 100644 --- a/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs +++ b/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs @@ -441,13 +441,15 @@ public class OrganizationUserRepository : Repository(), Collections = includeCollections - ? ou.CollectionUsers.Select(cu => new CollectionAccessSelection - { - Id = cu.CollectionId, - ReadOnly = cu.ReadOnly, - HidePasswords = cu.HidePasswords, - Manage = cu.Manage - }).ToList() + ? ou.CollectionUsers + .Where(cu => cu.Collection.Type == CollectionType.SharedCollection) + .Select(cu => new CollectionAccessSelection + { + Id = cu.CollectionId, + ReadOnly = cu.ReadOnly, + HidePasswords = cu.HidePasswords, + Manage = cu.Manage + }).ToList() : new List() }; diff --git a/src/Infrastructure.EntityFramework/Repositories/CollectionRepository.cs b/src/Infrastructure.EntityFramework/Repositories/CollectionRepository.cs index 0840a3f751..3169f86420 100644 --- a/src/Infrastructure.EntityFramework/Repositories/CollectionRepository.cs +++ b/src/Infrastructure.EntityFramework/Repositories/CollectionRepository.cs @@ -212,13 +212,26 @@ public class CollectionRepository : Repository> GetManyByOrganizationIdAsync(Guid organizationId) + { + using (var scope = ServiceScopeFactory.CreateScope()) + { + var dbContext = GetDatabaseContext(scope); + var query = from c in dbContext.Collections + where c.OrganizationId == organizationId + select c; + var collections = await query.ToArrayAsync(); + return collections; + } + } + + public async Task> GetManySharedCollectionsByOrganizationIdAsync(Guid organizationId) { using (var scope = ServiceScopeFactory.CreateScope()) { var dbContext = GetDatabaseContext(scope); var query = from c in dbContext.Collections where c.OrganizationId == organizationId && - c.Type != CollectionType.DefaultUserCollection + c.Type == CollectionType.SharedCollection select c; var collections = await query.ToArrayAsync(); return collections; diff --git a/src/Sql/dbo/Stored Procedures/Collection_ReadByOrganizationId.sql b/src/Sql/dbo/Stored Procedures/Collection_ReadByOrganizationId.sql index 6a7fefeb6b..0d317ebded 100644 --- a/src/Sql/dbo/Stored Procedures/Collection_ReadByOrganizationId.sql +++ b/src/Sql/dbo/Stored Procedures/Collection_ReadByOrganizationId.sql @@ -9,6 +9,5 @@ BEGIN FROM [dbo].[CollectionView] WHERE - [OrganizationId] = @OrganizationId AND - [Type] != 1 -- Exclude DefaultUserCollection + [OrganizationId] = @OrganizationId END \ No newline at end of file diff --git a/src/Sql/dbo/Stored Procedures/Collection_ReadSharedCollectionsByOrganizationId.sql b/src/Sql/dbo/Stored Procedures/Collection_ReadSharedCollectionsByOrganizationId.sql new file mode 100644 index 0000000000..9f54a7e10e --- /dev/null +++ b/src/Sql/dbo/Stored Procedures/Collection_ReadSharedCollectionsByOrganizationId.sql @@ -0,0 +1,14 @@ +CREATE PROCEDURE [dbo].[Collection_ReadSharedCollectionsByOrganizationId] + @OrganizationId UNIQUEIDENTIFIER +AS +BEGIN + SET NOCOUNT ON + + SELECT + * + FROM + [dbo].[CollectionView] + WHERE + [OrganizationId] = @OrganizationId AND + [Type] = 0 -- SharedCollections only +END \ No newline at end of file diff --git a/src/Sql/dbo/Stored Procedures/OrganizationUserUserDetails_ReadByOrganizationId_V2.sql b/src/Sql/dbo/Stored Procedures/OrganizationUserUserDetails_ReadByOrganizationId_V2.sql index 6bf32089c2..d581b3aa2c 100644 --- a/src/Sql/dbo/Stored Procedures/OrganizationUserUserDetails_ReadByOrganizationId_V2.sql +++ b/src/Sql/dbo/Stored Procedures/OrganizationUserUserDetails_ReadByOrganizationId_V2.sql @@ -26,6 +26,8 @@ BEGIN SELECT cu.* FROM [dbo].[CollectionUser] cu INNER JOIN [dbo].[OrganizationUser] ou ON cu.OrganizationUserId = ou.Id - WHERE ou.OrganizationId = @OrganizationId + INNER JOIN [dbo].[Collection] c ON cu.CollectionId = c.Id + WHERE ou.OrganizationId = @OrganizationId + AND c.Type = 0 -- SharedCollections only END END diff --git a/test/Api.Test/Controllers/CollectionsControllerTests.cs b/test/Api.Test/Controllers/CollectionsControllerTests.cs index bdcf6bc74e..99e329b500 100644 --- a/test/Api.Test/Controllers/CollectionsControllerTests.cs +++ b/test/Api.Test/Controllers/CollectionsControllerTests.cs @@ -173,12 +173,12 @@ public class CollectionsControllerTests .Returns(AuthorizationResult.Success()); sutProvider.GetDependency() - .GetManyByOrganizationIdAsync(organization.Id) + .GetManySharedCollectionsByOrganizationIdAsync(organization.Id) .Returns(collections); var response = await sutProvider.Sut.Get(organization.Id); - await sutProvider.GetDependency().Received(1).GetManyByOrganizationIdAsync(organization.Id); + await sutProvider.GetDependency().Received(1).GetManySharedCollectionsByOrganizationIdAsync(organization.Id); Assert.Equal(collections.Count, response.Data.Count()); } diff --git a/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CollectionRepositoryTests.cs b/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CollectionRepositoryTests.cs index 90596a23b1..79a96eeaeb 100644 --- a/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CollectionRepositoryTests.cs +++ b/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CollectionRepositoryTests.cs @@ -525,7 +525,7 @@ public class CollectionRepositoryTests var collection3 = new Collection { Name = "Collection 3", OrganizationId = organization.Id, }; await collectionRepository.CreateAsync(collection3, null, null); - // Create a default user collection (should not be returned by this method) + // Create a default user collection var defaultCollection = new Collection { Name = "My Items", @@ -536,12 +536,73 @@ public class CollectionRepositoryTests var collections = await collectionRepository.GetManyByOrganizationIdAsync(organization.Id); + Assert.NotNull(collections); + Assert.Equal(4, collections.Count); + Assert.All(collections, c => Assert.Equal(organization.Id, c.OrganizationId)); + + Assert.Contains(collections, c => c.Name == "Collection 1"); + Assert.Contains(collections, c => c.Name == "Collection 2"); + Assert.Contains(collections, c => c.Name == "Collection 3"); + Assert.Contains(collections, c => c.Name == "My Items"); + } + + /// + /// Test to ensure organization properly retrieves shared collections + /// + [DatabaseTheory, DatabaseData] + public async Task GetManySharedCollectionsByOrganizationIdAsync_Success( + IUserRepository userRepository, + IOrganizationRepository organizationRepository, + ICollectionRepository collectionRepository, + IOrganizationUserRepository organizationUserRepository) + { + var user = await userRepository.CreateAsync(new User + { + Name = "Test User", + Email = $"test+{Guid.NewGuid()}@email.com", + ApiKey = "TEST", + SecurityStamp = "stamp", + }); + + var organization = await organizationRepository.CreateAsync(new Organization + { + Name = "Test Org", + PlanType = PlanType.EnterpriseAnnually, + Plan = "Test Plan", + BillingEmail = "billing@email.com" + }); + + var orgUser = await organizationUserRepository.CreateAsync(new OrganizationUser + { + OrganizationId = organization.Id, + UserId = user.Id, + Status = OrganizationUserStatusType.Confirmed, + }); + + var collection1 = new Collection { Name = "Collection 1", OrganizationId = organization.Id, }; + await collectionRepository.CreateAsync(collection1, null, null); + + var collection2 = new Collection { Name = "Collection 2", OrganizationId = organization.Id, }; + await collectionRepository.CreateAsync(collection2, null, null); + + var collection3 = new Collection { Name = "Collection 3", OrganizationId = organization.Id, }; + await collectionRepository.CreateAsync(collection3, null, null); + + // Create a default user collection (should not be returned by this method) + var defaultCollection = new Collection + { + Name = "My Items", + OrganizationId = organization.Id, + Type = CollectionType.DefaultUserCollection + }; + await collectionRepository.CreateAsync(defaultCollection, null, null); + + var collections = await collectionRepository.GetManySharedCollectionsByOrganizationIdAsync(organization.Id); + Assert.NotNull(collections); Assert.Equal(3, collections.Count); // Should only return the 3 shared collections, excluding the default user collection Assert.All(collections, c => Assert.Equal(organization.Id, c.OrganizationId)); - Assert.All(collections, c => Assert.NotEqual(CollectionType.DefaultUserCollection, c.Type)); - // Verify specific collections are returned Assert.Contains(collections, c => c.Name == "Collection 1"); Assert.Contains(collections, c => c.Name == "Collection 2"); Assert.Contains(collections, c => c.Name == "Collection 3"); diff --git a/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationUserRepository/OrganizationUserRepositoryTests.cs b/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationUserRepository/OrganizationUserRepositoryTests.cs index 8eec878794..febfca89e4 100644 --- a/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationUserRepository/OrganizationUserRepositoryTests.cs +++ b/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationUserRepository/OrganizationUserRepositoryTests.cs @@ -911,6 +911,17 @@ public class OrganizationUserRepositoryTests RevisionDate = requestTime }); + var defaultUserCollection = await collectionRepository.CreateAsync(new Collection + { + Id = CoreHelpers.GenerateComb(), + OrganizationId = organization.Id, + Name = "My Items", + Type = CollectionType.DefaultUserCollection, + DefaultUserCollectionEmail = user1.Email, + CreationDate = requestTime, + RevisionDate = requestTime + }); + // Create organization user with both groups and collections using CreateManyAsync var createOrgUserWithCollections = new List { @@ -940,6 +951,13 @@ public class OrganizationUserRepositoryTests ReadOnly = false, HidePasswords = true, Manage = true + }, + new CollectionAccessSelection + { + Id = defaultUserCollection.Id, + ReadOnly = false, + HidePasswords = false, + Manage = true } ], Groups = [group1.Id, group2.Id] @@ -969,6 +987,7 @@ public class OrganizationUserRepositoryTests Assert.Equal(2, user1Result.Collections.Count()); Assert.Contains(user1Result.Collections, c => c.Id == collection1.Id); Assert.Contains(user1Result.Collections, c => c.Id == collection2.Id); + Assert.DoesNotContain(user1Result.Collections, c => c.Id == defaultUserCollection.Id); } [DatabaseTheory, DatabaseData] diff --git a/util/Migrator/DbScripts/2025-07-24_00_ReadSharedCollections.sql b/util/Migrator/DbScripts/2025-07-24_00_ReadSharedCollections.sql new file mode 100644 index 0000000000..3324e94964 --- /dev/null +++ b/util/Migrator/DbScripts/2025-07-24_00_ReadSharedCollections.sql @@ -0,0 +1,65 @@ +CREATE OR ALTER PROCEDURE [dbo].[Collection_ReadByOrganizationId] + @OrganizationId UNIQUEIDENTIFIER +AS +BEGIN + SET NOCOUNT ON + + SELECT + * + FROM + [dbo].[CollectionView] + WHERE + [OrganizationId] = @OrganizationId +END +GO + +CREATE OR ALTER PROCEDURE [dbo].[Collection_ReadSharedCollectionsByOrganizationId] + @OrganizationId UNIQUEIDENTIFIER +AS +BEGIN + SET NOCOUNT ON + + SELECT + * + FROM + [dbo].[CollectionView] + WHERE + [OrganizationId] = @OrganizationId AND + [Type] = 0 -- SharedCollections only +END +GO + +CREATE OR ALTER PROCEDURE [dbo].[OrganizationUserUserDetails_ReadByOrganizationId_V2] + @OrganizationId UNIQUEIDENTIFIER, + @IncludeGroups BIT = 0, + @IncludeCollections BIT = 0 +AS +BEGIN + SET NOCOUNT ON + + -- Result Set 1: User Details (always returned) + SELECT * + FROM [dbo].[OrganizationUserUserDetailsView] + WHERE OrganizationId = @OrganizationId + + -- Result Set 2: Group associations (if requested) + IF @IncludeGroups = 1 + BEGIN + SELECT gu.* + FROM [dbo].[GroupUser] gu + INNER JOIN [dbo].[OrganizationUser] ou ON gu.OrganizationUserId = ou.Id + WHERE ou.OrganizationId = @OrganizationId + END + + -- Result Set 3: Collection associations (if requested) + IF @IncludeCollections = 1 + BEGIN + SELECT cu.* + FROM [dbo].[CollectionUser] cu + INNER JOIN [dbo].[OrganizationUser] ou ON cu.OrganizationUserId = ou.Id + INNER JOIN [dbo].[Collection] c ON cu.CollectionId = c.Id + WHERE ou.OrganizationId = @OrganizationId + AND c.Type = 0 -- SharedCollections only + END +END +GO