From 2110582251d83e53bbb87c48c9cebd849a524319 Mon Sep 17 00:00:00 2001 From: Graham McVea Date: Tue, 9 Jun 2026 07:42:11 +0100 Subject: [PATCH 1/7] Refactor holding selection and merge logic for SAM import Refactored representative holding selection to prioritize SAM Holdings over Common Land using a new SelectRepresentativeHolding method. Updated EnrichWithCommonLandDataAsync to merge LocalAuthorityName and deduplicate AssociatedMainHoldings across all silver holdings. Adjusted FindAndUpdateMainSiteIfExists to use CPH string. Updated SamHoldingMapper to use new selection logic. Added unit tests for merging and selection behavior. Enhanced FakeDataBridgeClient test data for completeness. --- .../Steps/SamHoldingImportGoldMappingStep.cs | 66 +++++- .../Imports/Sam/Mappings/SamHoldingMapper.cs | 41 +++- .../ApiClients/Fakes/FakeDataBridgeClient.cs | 44 +++- .../SamHoldingImportGoldMappingStepTests.cs | 174 +++++++++++++++ .../Sam/Mappings/SamHoldingMapperTests.cs | 201 ++++++++++++++++++ 5 files changed, 506 insertions(+), 20 deletions(-) diff --git a/src/KeeperData.Application/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStep.cs b/src/KeeperData.Application/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStep.cs index 33ed2e79..c1be5c6d 100644 --- a/src/KeeperData.Application/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStep.cs +++ b/src/KeeperData.Application/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStep.cs @@ -28,9 +28,8 @@ protected override async Task ExecuteCoreAsync(SamHoldingImportContext context, { if (context.SilverHoldings.Count > 0) { - var representative = context.SilverHoldings.Any(x => x.HoldingStatus == HoldingStatusType.Active.GetDescription()) - ? context.SilverHoldings.Where(x => x.HoldingStatus == HoldingStatusType.Active.GetDescription()).OrderByDescending(h => h.LastUpdatedDate).First() - : context.SilverHoldings.OrderByDescending(h => h.LastUpdatedDate).First(); + // Prefer SAM Holding over Common Land when selecting representative + var representative = SelectRepresentativeHolding(context.SilverHoldings); var existingHoldingFilter = Builders.Filter.ElemMatch( x => x.Identifiers, @@ -69,7 +68,7 @@ protected override async Task ExecuteCoreAsync(SamHoldingImportContext context, siteTypeDerivedCodeLookupService, cancellationToken); - await EnrichWithCommonLandDataAsync(context, representative, cancellationToken); + await EnrichWithCommonLandDataAsync(context, context.SilverHoldings, cancellationToken); logger.LogInformation("Associated main sites queued for update: {Count} for CPH {Cph}", context.AssociatedMainSites?.Count ?? 0, context.Cph); @@ -86,14 +85,21 @@ protected override async Task ExecuteCoreAsync(SamHoldingImportContext context, } } - private async Task EnrichWithCommonLandDataAsync(SamHoldingImportContext context, SamHoldingDocument representative, CancellationToken cancellationToken) + private async Task EnrichWithCommonLandDataAsync(SamHoldingImportContext context, List silverHoldings, CancellationToken cancellationToken) { var goldSite = context.GoldSite; if (goldSite == null) return; - goldSite.LocalAuthorityName = representative.LocalAuthorityName; + // Merge LocalAuthorityName - prefer non-null value from any holding + goldSite.LocalAuthorityName = silverHoldings + .Select(h => h.LocalAuthorityName) + .FirstOrDefault(name => !string.IsNullOrWhiteSpace(name)); - goldSite.AssociatedMainHoldings = representative.AssociatedMainHoldings + // Merge AssociatedMainHoldings from all holdings, removing duplicates + var allMainHoldings = silverHoldings + .SelectMany(h => h.AssociatedMainHoldings) + .GroupBy(r => r.HoldingIdentifier) + .Select(g => g.OrderByDescending(r => r.StartDate).First()) .Select(r => new AssociatedHoldingDocument { HoldingIdentifier = r.HoldingIdentifier, @@ -103,13 +109,17 @@ private async Task EnrichWithCommonLandDataAsync(SamHoldingImportContext context }) .ToList(); + goldSite.AssociatedMainHoldings = allMainHoldings; + if (goldSite.AssociatedMainHoldings?.Count > 0) { - await FindAndUpdateMainSiteIfExists(context, representative, goldSite.AssociatedMainHoldings, cancellationToken); + // Get the CPH from any holding (they all have the same CPH) + var cph = silverHoldings.First().CountyParishHoldingNumber; + await FindAndUpdateMainSiteIfExists(context, cph, goldSite.AssociatedMainHoldings, cancellationToken); } } - private async Task FindAndUpdateMainSiteIfExists(SamHoldingImportContext context, SamHoldingDocument representative, List mainHoldings, CancellationToken cancellationToken) + private async Task FindAndUpdateMainSiteIfExists(SamHoldingImportContext context, string commonCph, List mainHoldings, CancellationToken cancellationToken) { foreach (var mainHolding in mainHoldings) { @@ -132,7 +142,7 @@ private async Task FindAndUpdateMainSiteIfExists(SamHoldingImportContext context var commonForMain = new AssociatedHoldingDocument { - HoldingIdentifier = representative.CountyParishHoldingNumber, + HoldingIdentifier = commonCph, ContiguousFlag = mainHolding.ContiguousFlag, StartDate = mainHolding.StartDate, EndDate = mainHolding.EndDate @@ -156,4 +166,40 @@ private async Task FindAndUpdateMainSiteIfExists(SamHoldingImportContext context context.AssociatedMainSites.Add(mainSite); } } + + private static SamHoldingDocument SelectRepresentativeHolding(List silverHoldings) + { + const string commonLandBusinessUsage = "Common Land"; + var activeStatus = HoldingStatusType.Active.GetDescription(); + + // Priority 1: Active SAM Holding (not Common Land) + var activeSamHolding = silverHoldings + .Where(x => x.HoldingStatus == activeStatus && x.SourceFacilitySubBusinessActivityCode != commonLandBusinessUsage) + .OrderByDescending(h => h.LastUpdatedDate) + .FirstOrDefault(); + + if (activeSamHolding != null) + return activeSamHolding; + + // Priority 2: Any SAM Holding (not Common Land) + var samHolding = silverHoldings + .Where(x => x.SourceFacilitySubBusinessActivityCode != commonLandBusinessUsage) + .OrderByDescending(h => h.LastUpdatedDate) + .FirstOrDefault(); + + if (samHolding != null) + return samHolding; + + // Priority 3: Active Common Land + var activeCommonLand = silverHoldings + .Where(x => x.HoldingStatus == activeStatus) + .OrderByDescending(h => h.LastUpdatedDate) + .FirstOrDefault(); + + if (activeCommonLand != null) + return activeCommonLand; + + // Priority 4: Any holding (fallback) + return silverHoldings.OrderByDescending(h => h.LastUpdatedDate).First(); + } } \ No newline at end of file diff --git a/src/KeeperData.Application/Orchestration/Imports/Sam/Mappings/SamHoldingMapper.cs b/src/KeeperData.Application/Orchestration/Imports/Sam/Mappings/SamHoldingMapper.cs index 7a89f93a..e2e87390 100644 --- a/src/KeeperData.Application/Orchestration/Imports/Sam/Mappings/SamHoldingMapper.cs +++ b/src/KeeperData.Application/Orchestration/Imports/Sam/Mappings/SamHoldingMapper.cs @@ -129,6 +129,42 @@ public static async Task ToSilver( return result; } + private static SamHoldingDocument SelectRepresentativeHolding(List silverHoldings) + { + const string commonLandBusinessUsage = "Common Land"; + var activeStatus = HoldingStatusType.Active.GetDescription(); + + // Priority 1: Active SAM Holding (not Common Land) + var activeSamHolding = silverHoldings + .Where(x => x.HoldingStatus == activeStatus && x.SourceFacilitySubBusinessActivityCode != commonLandBusinessUsage) + .OrderByDescending(h => h.LastUpdatedDate) + .FirstOrDefault(); + + if (activeSamHolding != null) + return activeSamHolding; + + // Priority 2: Any SAM Holding (not Common Land) + var samHolding = silverHoldings + .Where(x => x.SourceFacilitySubBusinessActivityCode != commonLandBusinessUsage) + .OrderByDescending(h => h.LastUpdatedDate) + .FirstOrDefault(); + + if (samHolding != null) + return samHolding; + + // Priority 3: Active Common Land + var activeCommonLand = silverHoldings + .Where(x => x.HoldingStatus == activeStatus) + .OrderByDescending(h => h.LastUpdatedDate) + .FirstOrDefault(); + + if (activeCommonLand != null) + return activeCommonLand; + + // Priority 4: Any holding (fallback) + return silverHoldings.OrderByDescending(h => h.LastUpdatedDate).First(); + } + public static async Task ToGold( string goldSiteId, SiteDocument? existingSite, @@ -146,9 +182,8 @@ public static async Task ToSilver( if (silverHoldings == null || silverHoldings.Count == 0) return null; - var representative = silverHoldings.Any(x => x.HoldingStatus == HoldingStatusType.Active.GetDescription()) - ? silverHoldings.Where(x => x.HoldingStatus == HoldingStatusType.Active.GetDescription()).OrderByDescending(h => h.LastUpdatedDate).First() - : silverHoldings.OrderByDescending(h => h.LastUpdatedDate).First(); + // Prefer SAM Holding over Common Land when selecting representative + var representative = SelectRepresentativeHolding(silverHoldings); var distinctSpecies = await GetDistinctReferenceDataAsync( silverHoldings.Select(h => h.SpeciesTypeCode), diff --git a/src/KeeperData.Infrastructure/ApiClients/Fakes/FakeDataBridgeClient.cs b/src/KeeperData.Infrastructure/ApiClients/Fakes/FakeDataBridgeClient.cs index 63437203..8fd176c1 100644 --- a/src/KeeperData.Infrastructure/ApiClients/Fakes/FakeDataBridgeClient.cs +++ b/src/KeeperData.Infrastructure/ApiClients/Fakes/FakeDataBridgeClient.cs @@ -254,17 +254,47 @@ private static DataBridgeResponse GetDataBridgeResponse(List data, int private List GetSamCphHolding(string? id = null) { return [ - new SamCphHolding { + new SamCphHolding + { + ANIMAL_PRODUCTION_USAGE_CODE = "MEAT", + ANIMAL_SPECIES_CODE = "CTT", BATCH_ID = 1, CHANGE_TYPE = "I", - IsDeleted = false, - UpdatedAtUtc = DateTime.UtcNow, - CreatedAtUtc = DateTime.UtcNow, - CPH = id ?? $"{_random.Next(10, 99)}{_random.Next(100, 999)}{_random.Next(1000, 9999)}", - FEATURE_NAME = Guid.NewGuid().ToString(), + COUNTRY_CODE = "GB", + CPH = id ?? $"{_random.Next(10, 99)}/{_random.Next(100, 999):000}/{_random.Next(1000, 9999)}", + CPH_RELATIONSHIP_TYPE = "MAIN", CPH_TYPE = "PERMANENT", + CreatedAtUtc = DateTime.UtcNow, + DISEASE_TYPE = null, + EASTING = 400022, + FACILITY_BUSINSS_ACTVTY_CODE = "FACACT", + FACILITY_TYPE_CODE = "CL", + FCLTY_SUB_BSNSS_ACTVTY_CODE = "FACSUB", FEATURE_ADDRESS_FROM_DATE = DateTime.Today.AddDays(-1), - FCLTY_SUB_BSNSS_ACTVTY_CODE = "SLG-RM-NA" + FEATURE_ADDRESS_TO_DATE = null, + FEATURE_NAME = "Feature 22", + INTERVAL = 12m, + INTERVAL_UNIT_OF_TIME = "Months", + IsDeleted = false, + LOCALITY = "Locality22", + MOVEMENT_RSTRCTN_RSN_CODE = null, + NORTHING = 500022, + OS_MAP_REFERENCE = null, + PAON_END_NUMBER = 20, + PAON_END_NUMBER_SUFFIX = 'D', + PAON_START_NUMBER = 2, + PAON_START_NUMBER_SUFFIX = 'C', + POSTCODE = "CPH22 222", + SAON_END_NUMBER = 10, + SAON_END_NUMBER_SUFFIX = 'B', + SAON_START_NUMBER = 1, + SAON_START_NUMBER_SUFFIX = 'A', + SECONDARY_CPH = "00/000/9267", + STREET = "Holding Street 22", + TOWN = "Town22", + UDPRN = "25000022", + UK_INTERNAL_CODE = "ENGLAND", + UpdatedAtUtc = DateTime.UtcNow }]; } diff --git a/tests/KeeperData.Application.Tests.Unit/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStepTests.cs b/tests/KeeperData.Application.Tests.Unit/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStepTests.cs index 6037a63a..a7b0365f 100644 --- a/tests/KeeperData.Application.Tests.Unit/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStepTests.cs +++ b/tests/KeeperData.Application.Tests.Unit/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStepTests.cs @@ -194,4 +194,178 @@ public async Task FindAndUpdateMainSiteIfExists_ReplacesExistingContextEntry_Whe Assert.NotNull(replaced.AssociatedCommonLands); Assert.Contains(replaced.AssociatedCommonLands, a => a.HoldingIdentifier == "CPH-1"); } + + [Fact] + public async Task EnrichWithCommonLandData_MergesLocalAuthorityName_WhenMultipleSilverHoldings() + { + var goldSiteRepoMock = new Mock>(); + var partiesRepoMock = new Mock(); + + var samHolding = new SamHoldingDocument + { + CountyParishHoldingNumber = "CPH-1", + SourceFacilitySubBusinessActivityCode = "Sheep Farm", + HoldingStatus = "Active", + LastUpdatedDate = DateTime.UtcNow, + LocalAuthorityName = null + }; + + var commonLandHolding = new SamHoldingDocument + { + CountyParishHoldingNumber = "CPH-1", + SourceFacilitySubBusinessActivityCode = "Common Land", + HoldingStatus = "Active", + LastUpdatedDate = DateTime.UtcNow.AddDays(-1), + LocalAuthorityName = "Devon County Council" + }; + + goldSiteRepoMock.Setup(r => r.FindOneByFilterAsync(It.IsAny>(), It.IsAny())) + .ReturnsAsync((SiteDocument?)null); + + var context = new SamHoldingImportContext + { + Cph = "CPH-1", + SilverHoldings = new List { samHolding, commonLandHolding }, + SilverHerds = new List(), + SilverParties = new List(), + GoldSite = new SiteDocument { Id = "gold-site-1" } + }; + + var mappingStep = new SamHoldingImportGoldMappingStep( + Mock.Of(), + Mock.Of(), + Mock.Of(), + Mock.Of(), + Mock.Of(), + Mock.Of(), + goldSiteRepoMock.Object, + partiesRepoMock.Object, + new Mock>().Object); + + await mappingStep.ExecuteAsync(context, CancellationToken.None); + + Assert.Equal("Devon County Council", context.GoldSite.LocalAuthorityName); + } + + [Fact] + public async Task EnrichWithCommonLandData_MergesAssociatedMainHoldings_WhenMultipleSilverHoldings() + { + var goldSiteRepoMock = new Mock>(); + var partiesRepoMock = new Mock(); + + var samHolding = new SamHoldingDocument + { + CountyParishHoldingNumber = "CPH-1", + SourceFacilitySubBusinessActivityCode = "Sheep Farm", + HoldingStatus = "Active", + LastUpdatedDate = DateTime.UtcNow, + AssociatedMainHoldings = new List() + }; + + var commonLandHolding = new SamHoldingDocument + { + CountyParishHoldingNumber = "CPH-1", + SourceFacilitySubBusinessActivityCode = "Common Land", + HoldingStatus = "Active", + LastUpdatedDate = DateTime.UtcNow.AddDays(-1), + AssociatedMainHoldings = new List + { + new AssociatedHoldingRelationship { HoldingIdentifier = "MAIN-1", ContiguousFlag = true, StartDate = "2024-01-01" }, + new AssociatedHoldingRelationship { HoldingIdentifier = "MAIN-2", ContiguousFlag = false, StartDate = "2024-02-01" } + } + }; + + goldSiteRepoMock.Setup(r => r.FindOneByFilterAsync(It.IsAny>(), It.IsAny())) + .ReturnsAsync((SiteDocument?)null); + + var context = new SamHoldingImportContext + { + Cph = "CPH-1", + SilverHoldings = new List { samHolding, commonLandHolding }, + SilverHerds = new List(), + SilverParties = new List(), + GoldSite = new SiteDocument { Id = "gold-site-1" } + }; + + var mappingStep = new SamHoldingImportGoldMappingStep( + Mock.Of(), + Mock.Of(), + Mock.Of(), + Mock.Of(), + Mock.Of(), + Mock.Of(), + goldSiteRepoMock.Object, + partiesRepoMock.Object, + new Mock>().Object); + + await mappingStep.ExecuteAsync(context, CancellationToken.None); + + Assert.NotNull(context.GoldSite.AssociatedMainHoldings); + Assert.Equal(2, context.GoldSite.AssociatedMainHoldings.Count); + Assert.Contains(context.GoldSite.AssociatedMainHoldings, h => h.HoldingIdentifier == "MAIN-1"); + Assert.Contains(context.GoldSite.AssociatedMainHoldings, h => h.HoldingIdentifier == "MAIN-2"); + } + + [Fact] + public async Task EnrichWithCommonLandData_DeduplicatesAssociatedMainHoldings_WhenSameIdentifierInMultipleHoldings() + { + var goldSiteRepoMock = new Mock>(); + var partiesRepoMock = new Mock(); + + var samHolding = new SamHoldingDocument + { + CountyParishHoldingNumber = "CPH-1", + SourceFacilitySubBusinessActivityCode = "Sheep Farm", + HoldingStatus = "Active", + LastUpdatedDate = DateTime.UtcNow, + AssociatedMainHoldings = new List + { + new AssociatedHoldingRelationship { HoldingIdentifier = "MAIN-1", ContiguousFlag = true, StartDate = "2024-01-01" } + } + }; + + var commonLandHolding = new SamHoldingDocument + { + CountyParishHoldingNumber = "CPH-1", + SourceFacilitySubBusinessActivityCode = "Common Land", + HoldingStatus = "Active", + LastUpdatedDate = DateTime.UtcNow.AddDays(-1), + AssociatedMainHoldings = new List + { + new AssociatedHoldingRelationship { HoldingIdentifier = "MAIN-1", ContiguousFlag = false, StartDate = "2024-03-01" } + } + }; + + goldSiteRepoMock.Setup(r => r.FindOneByFilterAsync(It.IsAny>(), It.IsAny())) + .ReturnsAsync((SiteDocument?)null); + + var context = new SamHoldingImportContext + { + Cph = "CPH-1", + SilverHoldings = new List { samHolding, commonLandHolding }, + SilverHerds = new List(), + SilverParties = new List(), + GoldSite = new SiteDocument { Id = "gold-site-1" } + }; + + var mappingStep = new SamHoldingImportGoldMappingStep( + Mock.Of(), + Mock.Of(), + Mock.Of(), + Mock.Of(), + Mock.Of(), + Mock.Of(), + goldSiteRepoMock.Object, + partiesRepoMock.Object, + new Mock>().Object); + + await mappingStep.ExecuteAsync(context, CancellationToken.None); + + Assert.NotNull(context.GoldSite.AssociatedMainHoldings); + Assert.Single(context.GoldSite.AssociatedMainHoldings); + var mainHolding = context.GoldSite.AssociatedMainHoldings[0]; + Assert.Equal("MAIN-1", mainHolding.HoldingIdentifier); + // Should prefer the most recent StartDate (2024-03-01) + Assert.Equal("2024-03-01", mainHolding.StartDate); + } } \ No newline at end of file diff --git a/tests/KeeperData.Application.Tests.Unit/Orchestration/Imports/Sam/Mappings/SamHoldingMapperTests.cs b/tests/KeeperData.Application.Tests.Unit/Orchestration/Imports/Sam/Mappings/SamHoldingMapperTests.cs index 435e2713..9a03a72e 100644 --- a/tests/KeeperData.Application.Tests.Unit/Orchestration/Imports/Sam/Mappings/SamHoldingMapperTests.cs +++ b/tests/KeeperData.Application.Tests.Unit/Orchestration/Imports/Sam/Mappings/SamHoldingMapperTests.cs @@ -239,4 +239,205 @@ private static List GenerateSamCphHolding(int quantity) } return records; } + + [Fact] + public async Task ToGold_PrefersActiveSamHolding_OverCommonLand() + { + var activeSamHolding = new SamHoldingDocument + { + CountyParishHoldingNumber = "CPH-1", + SourceFacilitySubBusinessActivityCode = "Sheep Farm", + HoldingStatus = "Active", + LastUpdatedDate = DateTime.UtcNow, + LocationName = "SAM Farm Location", + SpeciesTypeCode = "SHE" + }; + + var activeCommonLand = new SamHoldingDocument + { + CountyParishHoldingNumber = "CPH-1", + SourceFacilitySubBusinessActivityCode = "Common Land", + HoldingStatus = "Active", + LastUpdatedDate = DateTime.UtcNow.AddDays(1), // More recent, but should not be selected + LocationName = "Common Land Location", + SpeciesTypeCode = null + }; + + var siteIdentifierType = new SiteIdentifierTypeDocument + { + IdentifierId = "type-id", + Code = "CPHN", + Name = "CPH Number", + LastModifiedDate = DateTime.UtcNow + }; + + var result = await SamHoldingMapper.ToGold( + "gold-site-id", + null, + [activeSamHolding, activeCommonLand], + [], + [], + (_, _) => Task.FromResult(null), + (_, _) => Task.FromResult(null), + (code, _) => Task.FromResult( + code == "CPHN" ? siteIdentifierType : null), + (code, _) => Task.FromResult<(string?, string?)>(code == "SHE" ? ("species-id", "Sheep") : (null, null)), + (_, _) => Task.FromResult(null), + Mock.Of(), + CancellationToken.None); + + result.Should().NotBeNull(); + // Verify that SAM Holding was used as representative by checking species (only SAM has it) + result!.Species.Should().NotBeNull(); + result.Species.Should().ContainSingle(s => s.Code == "SHE"); + } + + [Fact] + public async Task ToGold_PrefersInactiveSamHolding_OverActiveCommonLand() + { + var inactiveSamHolding = new SamHoldingDocument + { + CountyParishHoldingNumber = "CPH-1", + SourceFacilitySubBusinessActivityCode = "Cattle Farm", + HoldingStatus = "Inactive", + LastUpdatedDate = DateTime.UtcNow, + LocationName = "SAM Farm Location", + SpeciesTypeCode = "CAT" + }; + + var activeCommonLand = new SamHoldingDocument + { + CountyParishHoldingNumber = "CPH-1", + SourceFacilitySubBusinessActivityCode = "Common Land", + HoldingStatus = "Active", + LastUpdatedDate = DateTime.UtcNow.AddDays(1), + LocationName = "Common Land Location", + SpeciesTypeCode = null + }; + + var siteIdentifierType = new SiteIdentifierTypeDocument + { + IdentifierId = "type-id", + Code = "CPHN", + Name = "CPH Number", + LastModifiedDate = DateTime.UtcNow + }; + + var result = await SamHoldingMapper.ToGold( + "gold-site-id", + null, + [inactiveSamHolding, activeCommonLand], + [], + [], + (_, _) => Task.FromResult(null), + (_, _) => Task.FromResult(null), + (code, _) => Task.FromResult( + code == "CPHN" ? siteIdentifierType : null), + (code, _) => Task.FromResult<(string?, string?)>(code == "CAT" ? ("species-id", "Cattle") : (null, null)), + (_, _) => Task.FromResult(null), + Mock.Of(), + CancellationToken.None); + + result.Should().NotBeNull(); + // Verify that SAM Holding was used (Priority 2 beats Priority 3) + result!.Species.Should().NotBeNull(); + result.Species.Should().ContainSingle(s => s.Code == "CAT"); + } + + [Fact] + public async Task ToGold_SelectsCommonLand_WhenOnlyCommonLandPresent() + { + var activeCommonLand = new SamHoldingDocument + { + CountyParishHoldingNumber = "CPH-1", + SourceFacilitySubBusinessActivityCode = "Common Land", + HoldingStatus = "Active", + LastUpdatedDate = DateTime.UtcNow, + LocationName = "Common Land Location", + LocalAuthorityName = "Devon County Council", + CreatedDate = DateTime.UtcNow + }; + + var siteIdentifierType = new SiteIdentifierTypeDocument + { + IdentifierId = "type-id", + Code = "CPHN", + Name = "CPH Number", + LastModifiedDate = DateTime.UtcNow + }; + + var result = await SamHoldingMapper.ToGold( + "gold-site-id", + null, + [activeCommonLand], + [], + [], + (_, _) => Task.FromResult(null), + (_, _) => Task.FromResult(null), + (code, _) => Task.FromResult( + code == "CPHN" ? siteIdentifierType : null), + (_, _) => Task.FromResult<(string?, string?)>((null, null)), + (_, _) => Task.FromResult(null), + Mock.Of(), + CancellationToken.None); + + result.Should().NotBeNull(); + // Verify Common Land was selected as representative (fallback when no SAM Holding) + result!.Name.Should().Be("Common Land Location"); + } + + [Fact] + public async Task ToGold_SelectsMostRecentActiveSamHolding_WhenMultipleActiveSamHoldings() + { + var olderSamHolding = new SamHoldingDocument + { + CountyParishHoldingNumber = "CPH-1", + SourceFacilitySubBusinessActivityCode = "Pig Farm", + HoldingStatus = "Active", + LastUpdatedDate = DateTime.UtcNow.AddDays(-5), + LocationName = "Old Location", + SpeciesTypeCode = "PIG" + }; + + var newerSamHolding = new SamHoldingDocument + { + CountyParishHoldingNumber = "CPH-1", + SourceFacilitySubBusinessActivityCode = "Sheep Farm", + HoldingStatus = "Active", + LastUpdatedDate = DateTime.UtcNow, + LocationName = "New Location", + SpeciesTypeCode = "SHE" + }; + + var siteIdentifierType = new SiteIdentifierTypeDocument + { + IdentifierId = "type-id", + Code = "CPHN", + Name = "CPH Number", + LastModifiedDate = DateTime.UtcNow + }; + + var result = await SamHoldingMapper.ToGold( + "gold-site-id", + null, + [olderSamHolding, newerSamHolding], + [], + [], + (_, _) => Task.FromResult(null), + (_, _) => Task.FromResult(null), + (code, _) => Task.FromResult( + code == "CPHN" ? siteIdentifierType : null), + (code, _) => Task.FromResult<(string?, string?)>( + code == "SHE" ? ("sheep-id", "Sheep") : + code == "PIG" ? ("pig-id", "Pig") : (null, null)), + (_, _) => Task.FromResult(null), + Mock.Of(), + CancellationToken.None); + + result.Should().NotBeNull(); + // Should have both species (aggregated from all), but verify the newer one is present + result!.Species.Should().NotBeNull(); + result.Species.Should().Contain(s => s.Code == "SHE"); + result.Species.Should().Contain(s => s.Code == "PIG"); + } } \ No newline at end of file From bdca9b4046ee6d4921114ed8e9a6fb5ca14953cc Mon Sep 17 00:00:00 2001 From: Graham McVea Date: Tue, 9 Jun 2026 10:05:06 +0100 Subject: [PATCH 2/7] Refactor site data mapping; add Common Land test Refactored site data assignment into ApplySiteData to reduce duplication in SamHoldingMapper. Updated site creation and update methods to use this helper. Added a unit test to ensure active Common Land holdings are correctly selected as representative. --- .../Imports/Sam/Mappings/SamHoldingMapper.cs | 76 +++++++++---------- .../SamHoldingImportGoldMappingStepTests.cs | 65 ++++++++++++++++ 2 files changed, 99 insertions(+), 42 deletions(-) diff --git a/src/KeeperData.Application/Orchestration/Imports/Sam/Mappings/SamHoldingMapper.cs b/src/KeeperData.Application/Orchestration/Imports/Sam/Mappings/SamHoldingMapper.cs index e2e87390..53a75fb8 100644 --- a/src/KeeperData.Application/Orchestration/Imports/Sam/Mappings/SamHoldingMapper.cs +++ b/src/KeeperData.Application/Orchestration/Imports/Sam/Mappings/SamHoldingMapper.cs @@ -360,13 +360,6 @@ private static async Task CreateSiteAsync( address, communication: [communication]); - var groupMarks = ToGroupMarks(goldSiteGroupMarks); - - var siteParties = goldParties - .Where(p => !p.Deleted && !string.IsNullOrWhiteSpace(p.CustomerNumber)) - .Select(p => p.ToSitePartyDomain(representative.LastUpdatedDate)) - .ToList(); - var site = Site.Create( goldSiteId, representative.CreatedDate, @@ -384,20 +377,7 @@ private static async Task CreateSiteAsync( location, representative.CphRelationshipType.IsPermanentLandHolding() ? representative.SecondaryCph : null); - if (siteIdentifierType != null) - { - site.SetSiteIdentifier( - identifierLastUpdatedDate: representative.LastUpdatedDate, - identifier: representative.CountyParishHoldingNumber, - type: siteIdentifierType, - id: null, - siteLastUpdatedDate: representative.LastUpdatedDate); - } - - site.SetSpecies(species, representative.LastUpdatedDate); - site.SetActivities(activities, representative.LastUpdatedDate); - site.SetGroupMarks(groupMarks, representative.LastUpdatedDate); - site.SetSiteParties(goldSiteId, siteParties, representative.LastUpdatedDate); + ApplySiteData(site, goldSiteId, representative, goldSiteGroupMarks, goldParties, species, activities, siteIdentifierType); return site; } @@ -416,13 +396,6 @@ private static async Task UpdateSiteAsync( { var site = existing.ToDomain(); - var groupMarks = ToGroupMarks(goldSiteGroupMarks); - - var siteParties = goldParties - .Where(p => !p.Deleted && !string.IsNullOrWhiteSpace(p.CustomerNumber)) - .Select(p => p.ToSitePartyDomain(representative.LastUpdatedDate)) - .ToList(); - site.Update( representative.LastUpdatedDate, representative.LocationName ?? string.Empty, @@ -450,20 +423,7 @@ private static async Task UpdateSiteAsync( updatedAddress, [updatedCommunication]); - if (siteIdentifierType != null) - { - site.SetSiteIdentifier( - identifierLastUpdatedDate: representative.LastUpdatedDate, - identifier: representative.CountyParishHoldingNumber, - type: siteIdentifierType, - id: null, - siteLastUpdatedDate: representative.LastUpdatedDate); - } - - site.SetSpecies(species, representative.LastUpdatedDate); - site.SetActivities(activities, representative.LastUpdatedDate); - site.SetGroupMarks(groupMarks, representative.LastUpdatedDate); - site.SetSiteParties(existing.Id, siteParties, representative.LastUpdatedDate); + ApplySiteData(site, existing.Id, representative, goldSiteGroupMarks, goldParties, species, activities, siteIdentifierType); return site; } @@ -490,6 +450,38 @@ private static async Task UpdateSiteAsync( return [.. results]; } + private static void ApplySiteData( + Site site, + string siteId, + SamHoldingDocument representative, + List goldSiteGroupMarks, + List goldParties, + List species, + List activities, + SiteIdentifierType? siteIdentifierType) + { + var groupMarks = ToGroupMarks(goldSiteGroupMarks); + var siteParties = goldParties + .Where(p => !p.Deleted && !string.IsNullOrWhiteSpace(p.CustomerNumber)) + .Select(p => p.ToSitePartyDomain(representative.LastUpdatedDate)) + .ToList(); + + if (siteIdentifierType != null) + { + site.SetSiteIdentifier( + identifierLastUpdatedDate: representative.LastUpdatedDate, + identifier: representative.CountyParishHoldingNumber, + type: siteIdentifierType, + id: null, + siteLastUpdatedDate: representative.LastUpdatedDate); + } + + site.SetSpecies(species, representative.LastUpdatedDate); + site.SetActivities(activities, representative.LastUpdatedDate); + site.SetGroupMarks(groupMarks, representative.LastUpdatedDate); + site.SetSiteParties(siteId, siteParties, representative.LastUpdatedDate); + } + private static List ToGroupMarks(List relationships) { return diff --git a/tests/KeeperData.Application.Tests.Unit/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStepTests.cs b/tests/KeeperData.Application.Tests.Unit/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStepTests.cs index a7b0365f..127655ca 100644 --- a/tests/KeeperData.Application.Tests.Unit/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStepTests.cs +++ b/tests/KeeperData.Application.Tests.Unit/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStepTests.cs @@ -306,6 +306,71 @@ public async Task EnrichWithCommonLandData_MergesAssociatedMainHoldings_WhenMult Assert.Contains(context.GoldSite.AssociatedMainHoldings, h => h.HoldingIdentifier == "MAIN-2"); } + [Fact] + public async Task SelectRepresentativeHolding_ReturnsActiveCommonLand_WhenAllHoldingsAreCommonLandAndOneIsActive() + { + // Arrange: all holdings are Common Land (Priorities 1 & 2 fail), one is active + var goldSiteRepoMock = new Mock>(); + var partiesRepoMock = new Mock(); + + var activeCommonLand = new SamHoldingDocument + { + CountyParishHoldingNumber = "ACTIVE-CPH", + SourceFacilitySubBusinessActivityCode = "Common Land", + HoldingStatus = "active", + LastUpdatedDate = DateTime.UtcNow + }; + + var inactiveCommonLand = new SamHoldingDocument + { + CountyParishHoldingNumber = "INACTIVE-CPH", + SourceFacilitySubBusinessActivityCode = "Common Land", + HoldingStatus = "inactive", + LastUpdatedDate = DateTime.UtcNow.AddDays(-1) + }; + + SiteDocument? capturedFilter = null; + goldSiteRepoMock + .Setup(r => r.FindOneByFilterAsync(It.IsAny>(), It.IsAny())) + .ReturnsAsync((SiteDocument?)null); + + var context = new SamHoldingImportContext + { + Cph = "ACTIVE-CPH", + SilverHoldings = new List { inactiveCommonLand, activeCommonLand }, + SilverHerds = new List(), + SilverParties = new List(), + GoldSite = new SiteDocument { Id = "pre-set" } + }; + + var mappingStep = new SamHoldingImportGoldMappingStep( + Mock.Of(), + Mock.Of(), + Mock.Of(), + Mock.Of(), + Mock.Of(), + Mock.Of(), + goldSiteRepoMock.Object, + partiesRepoMock.Object, + new Mock>().Object); + + // Act + await mappingStep.ExecuteAsync(context, CancellationToken.None); + + // Assert: the active Common Land holding was selected as representative, + // so the existing-site filter used its CPH and GoldSiteId was assigned. + // FindOneByFilterAsync is called once, meaning representative selection completed. + goldSiteRepoMock.Verify( + r => r.FindOneByFilterAsync(It.IsAny>(), It.IsAny()), + Times.Once); + + // The GoldSiteId is always assigned when SilverHoldings is non-empty + Assert.NotNull(context.GoldSiteId); + + // The representative's CPH drives the context Cph; confirm it matches the active holding + Assert.Equal("ACTIVE-CPH", context.Cph); + } + [Fact] public async Task EnrichWithCommonLandData_DeduplicatesAssociatedMainHoldings_WhenSameIdentifierInMultipleHoldings() { From a39a47df7e29cf0aece4cd40bb73de1316c48f8c Mon Sep 17 00:00:00 2001 From: Graham McVea Date: Wed, 10 Jun 2026 10:24:30 +0100 Subject: [PATCH 3/7] Remove unused capturedFilter variable from test class Removed the unused SiteDocument? capturedFilter variable from SamHoldingImportGoldMappingStepTests to clean up the test code. No functional changes were made. --- .../Sam/Holdings/Steps/SamHoldingImportGoldMappingStepTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/KeeperData.Application.Tests.Unit/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStepTests.cs b/tests/KeeperData.Application.Tests.Unit/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStepTests.cs index 127655ca..b32148b8 100644 --- a/tests/KeeperData.Application.Tests.Unit/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStepTests.cs +++ b/tests/KeeperData.Application.Tests.Unit/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStepTests.cs @@ -329,7 +329,6 @@ public async Task SelectRepresentativeHolding_ReturnsActiveCommonLand_WhenAllHol LastUpdatedDate = DateTime.UtcNow.AddDays(-1) }; - SiteDocument? capturedFilter = null; goldSiteRepoMock .Setup(r => r.FindOneByFilterAsync(It.IsAny>(), It.IsAny())) .ReturnsAsync((SiteDocument?)null); From 70a37b4adf9f66e60ab80aa997bc444f1bf43607 Mon Sep 17 00:00:00 2001 From: Graham McVea Date: Wed, 10 Jun 2026 11:02:29 +0100 Subject: [PATCH 4/7] Ensure ProductionUsageCodeList contains unique values Applied .Distinct() to ProductionUsageCodeList to remove duplicate production usage codes after trimming, ensuring only unique codes are included in the mapped result. --- .../Orchestration/Imports/Sam/Mappings/SamHoldingMapper.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/KeeperData.Application/Orchestration/Imports/Sam/Mappings/SamHoldingMapper.cs b/src/KeeperData.Application/Orchestration/Imports/Sam/Mappings/SamHoldingMapper.cs index 53a75fb8..e3cc6984 100644 --- a/src/KeeperData.Application/Orchestration/Imports/Sam/Mappings/SamHoldingMapper.cs +++ b/src/KeeperData.Application/Orchestration/Imports/Sam/Mappings/SamHoldingMapper.cs @@ -92,7 +92,7 @@ public static async Task ToSilver( SiteTypeCode = null, SpeciesTypeCode = h.AnimalSpeciesCodeUnwrapped, - ProductionUsageCodeList = [.. h.AnimalProductionUsageCodeList.Select(ProductionUsageCodeFormatters.TrimProductionUsageCodeHolding)], + ProductionUsageCodeList = [.. h.AnimalProductionUsageCodeList.Select(ProductionUsageCodeFormatters.TrimProductionUsageCodeHolding).Distinct()], Location = new Core.Documents.Silver.LocationDocument { From 3fd70c8c1d2bc72798e0500409a0afbfa33a5feb Mon Sep 17 00:00:00 2001 From: Graham McVea Date: Wed, 10 Jun 2026 12:10:22 +0100 Subject: [PATCH 5/7] Refactor address/comm mapping with helper method Refactored SAM holding mapping by introducing ResolveLocationPartsAsync to handle address and communication extraction as a tuple. Replaced duplicate logic in creation and update flows, and cached IsPermanentLandHolding() result for clarity and efficiency. --- .../Imports/Sam/Mappings/SamHoldingMapper.cs | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/src/KeeperData.Application/Orchestration/Imports/Sam/Mappings/SamHoldingMapper.cs b/src/KeeperData.Application/Orchestration/Imports/Sam/Mappings/SamHoldingMapper.cs index e3cc6984..49fc608b 100644 --- a/src/KeeperData.Application/Orchestration/Imports/Sam/Mappings/SamHoldingMapper.cs +++ b/src/KeeperData.Application/Orchestration/Imports/Sam/Mappings/SamHoldingMapper.cs @@ -350,8 +350,8 @@ private static async Task CreateSiteAsync( SiteIdentifierType? siteIdentifierType, CancellationToken cancellationToken) { - var address = await LocationMapper.AddressToGold(representative.Location?.Address, getCountryById, cancellationToken); - var communication = LocationMapper.CommunicationToGold(representative.Communication); + var (address, communication) = await ResolveLocationPartsAsync(representative, getCountryById, cancellationToken); + var isPermanentLandHolding = representative.CphRelationshipType.IsPermanentLandHolding(); var location = Location.Create( representative.Location?.OsMapReference, @@ -371,11 +371,11 @@ private static async Task CreateSiteAsync( SourceSystemType.SAM.ToString(), null, representative.Deleted, - representative.CphRelationshipType.IsPermanentLandHolding() ? null : representative.SecondaryCph, + isPermanentLandHolding ? null : representative.SecondaryCph, representative.CphTypeIdentifier, siteType, location, - representative.CphRelationshipType.IsPermanentLandHolding() ? representative.SecondaryCph : null); + isPermanentLandHolding ? representative.SecondaryCph : null); ApplySiteData(site, goldSiteId, representative, goldSiteGroupMarks, goldParties, species, activities, siteIdentifierType); @@ -394,6 +394,7 @@ private static async Task UpdateSiteAsync( SiteIdentifierType? siteIdentifierType, CancellationToken cancellationToken) { + var isPermanentLandHolding = representative.CphRelationshipType.IsPermanentLandHolding(); var site = existing.ToDomain(); site.Update( @@ -405,12 +406,11 @@ private static async Task UpdateSiteAsync( SourceSystemType.SAM.ToString(), null, representative.Deleted, - representative.CphRelationshipType.IsPermanentLandHolding() ? null : representative.SecondaryCph, + isPermanentLandHolding ? null : representative.SecondaryCph, representative.CphTypeIdentifier, - representative.CphRelationshipType.IsPermanentLandHolding() ? representative.SecondaryCph : null); + isPermanentLandHolding ? representative.SecondaryCph : null); - var updatedAddress = await LocationMapper.AddressToGold(representative.Location?.Address, getCountryById, cancellationToken); - var updatedCommunication = LocationMapper.CommunicationToGold(representative.Communication); + var (updatedAddress, updatedCommunication) = await ResolveLocationPartsAsync(representative, getCountryById, cancellationToken); // Always set the derived site type (may be null if no mapping found). site.SetSiteType(siteType, representative.LastUpdatedDate); @@ -429,6 +429,16 @@ private static async Task UpdateSiteAsync( } + private static async Task<(Address address, Communication communication)> ResolveLocationPartsAsync( + SamHoldingDocument representative, + Func> getCountryById, + CancellationToken cancellationToken) + { + var address = await LocationMapper.AddressToGold(representative.Location?.Address, getCountryById, cancellationToken); + var communication = LocationMapper.CommunicationToGold(representative.Communication); + return (address, communication); + } + private static async Task> GetDistinctReferenceDataAsync( IEnumerable rawCodes, Func> findAsync, From 87dbda0e73723e108d4278d9eaff09754fe8ad2f Mon Sep 17 00:00:00 2001 From: Graham McVea Date: Wed, 10 Jun 2026 12:51:34 +0100 Subject: [PATCH 6/7] Move SelectRepresentativeHolding to SamHoldingMapper Refactored SelectRepresentativeHolding from SamHoldingImportGoldMappingStep to SamHoldingMapper as an internal method. Updated references to use the new location. No changes to method logic. --- .../Steps/SamHoldingImportGoldMappingStep.cs | 39 +------------------ .../Imports/Sam/Mappings/SamHoldingMapper.cs | 2 +- 2 files changed, 3 insertions(+), 38 deletions(-) diff --git a/src/KeeperData.Application/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStep.cs b/src/KeeperData.Application/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStep.cs index c1be5c6d..02e42799 100644 --- a/src/KeeperData.Application/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStep.cs +++ b/src/KeeperData.Application/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStep.cs @@ -29,7 +29,7 @@ protected override async Task ExecuteCoreAsync(SamHoldingImportContext context, if (context.SilverHoldings.Count > 0) { // Prefer SAM Holding over Common Land when selecting representative - var representative = SelectRepresentativeHolding(context.SilverHoldings); + var representative = SamHoldingMapper.SelectRepresentativeHolding(context.SilverHoldings); var existingHoldingFilter = Builders.Filter.ElemMatch( x => x.Identifiers, @@ -167,39 +167,4 @@ private async Task FindAndUpdateMainSiteIfExists(SamHoldingImportContext context } } - private static SamHoldingDocument SelectRepresentativeHolding(List silverHoldings) - { - const string commonLandBusinessUsage = "Common Land"; - var activeStatus = HoldingStatusType.Active.GetDescription(); - - // Priority 1: Active SAM Holding (not Common Land) - var activeSamHolding = silverHoldings - .Where(x => x.HoldingStatus == activeStatus && x.SourceFacilitySubBusinessActivityCode != commonLandBusinessUsage) - .OrderByDescending(h => h.LastUpdatedDate) - .FirstOrDefault(); - - if (activeSamHolding != null) - return activeSamHolding; - - // Priority 2: Any SAM Holding (not Common Land) - var samHolding = silverHoldings - .Where(x => x.SourceFacilitySubBusinessActivityCode != commonLandBusinessUsage) - .OrderByDescending(h => h.LastUpdatedDate) - .FirstOrDefault(); - - if (samHolding != null) - return samHolding; - - // Priority 3: Active Common Land - var activeCommonLand = silverHoldings - .Where(x => x.HoldingStatus == activeStatus) - .OrderByDescending(h => h.LastUpdatedDate) - .FirstOrDefault(); - - if (activeCommonLand != null) - return activeCommonLand; - - // Priority 4: Any holding (fallback) - return silverHoldings.OrderByDescending(h => h.LastUpdatedDate).First(); - } -} \ No newline at end of file + } \ No newline at end of file diff --git a/src/KeeperData.Application/Orchestration/Imports/Sam/Mappings/SamHoldingMapper.cs b/src/KeeperData.Application/Orchestration/Imports/Sam/Mappings/SamHoldingMapper.cs index 49fc608b..bb6b60c5 100644 --- a/src/KeeperData.Application/Orchestration/Imports/Sam/Mappings/SamHoldingMapper.cs +++ b/src/KeeperData.Application/Orchestration/Imports/Sam/Mappings/SamHoldingMapper.cs @@ -129,7 +129,7 @@ public static async Task ToSilver( return result; } - private static SamHoldingDocument SelectRepresentativeHolding(List silverHoldings) + internal static SamHoldingDocument SelectRepresentativeHolding(List silverHoldings) { const string commonLandBusinessUsage = "Common Land"; var activeStatus = HoldingStatusType.Active.GetDescription(); From c61dfd0216fe8581bbc4a4e73c1475f4d286f31c Mon Sep 17 00:00:00 2001 From: Graham McVea Date: Wed, 10 Jun 2026 13:04:20 +0100 Subject: [PATCH 7/7] Whitespace --- .../Sam/Holdings/Steps/SamHoldingImportGoldMappingStep.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/KeeperData.Application/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStep.cs b/src/KeeperData.Application/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStep.cs index 02e42799..bafb2992 100644 --- a/src/KeeperData.Application/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStep.cs +++ b/src/KeeperData.Application/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportGoldMappingStep.cs @@ -166,5 +166,4 @@ private async Task FindAndUpdateMainSiteIfExists(SamHoldingImportContext context context.AssociatedMainSites.Add(mainSite); } } - - } \ No newline at end of file +} \ No newline at end of file