Skip to content

Commit 76cdd94

Browse files
grahammcvea-defraBartonkeys
grahammcvea-defra
and
Bartonkeys
authored
Bug/ulitp 5394 Refactor common land mapping and address selection (#238)
* 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. * 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. * 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. * 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. * 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. * 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. * Whitespace * Refactor common land mapping and address selection SamCommonLandMapper.ToSilver is now async and supports country resolution for enriched address data. Address mapping uses resolved country identifiers and codes. SamHoldingMapper gains SelectAddressSource to prioritize common land addresses when mapping to gold. Site creation and update logic now use the prioritized address source. Unit tests updated and expanded for new async and address selection logic. --------- Co-authored-by: Graham McVea <graham.mcvea@bartonkeys.com>
1 parent c208544 commit 76cdd94

5 files changed

Lines changed: 229 additions & 35 deletions

File tree

‎src/KeeperData.Application/Orchestration/Imports/Sam/Holdings/Steps/SamHoldingImportSilverMappingStep.cs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,10 @@ protected override async Task ExecuteCoreAsync(SamHoldingImportContext context,
2525
countryIdentifierLookupService.FindAsync,
2626
cancellationToken);
2727

28-
var commonLandHoldings = SamCommonLandMapper.ToSilver(context.RawCommonLandsByCommonCph);
28+
var commonLandHoldings = await SamCommonLandMapper.ToSilver(
29+
context.RawCommonLandsByCommonCph,
30+
countryIdentifierLookupService.FindAsync,
31+
cancellationToken);
2932
if (commonLandHoldings.Count > 0)
3033
{
3134
context.SilverHoldings.AddRange(commonLandHoldings);

‎src/KeeperData.Application/Orchestration/Imports/Sam/Mappings/SamCommonLandMapper.cs‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,10 @@ public static class SamCommonLandMapper
1010
private const string CommonLandSiteTypeCode = "CL";
1111
private const string CommonLandBusinessUsage = "Common Land";
1212

13-
public static List<SamHoldingDocument> ToSilver(List<SamCommonLand> rawCommonLands)
13+
public static async Task<List<SamHoldingDocument>> ToSilver(
14+
List<SamCommonLand> rawCommonLands,
15+
Func<string?, string?, CancellationToken, Task<(string? countryId, string? countryCode, string? countryName)>> resolveCountry,
16+
CancellationToken cancellationToken)
1417
{
1518
if (rawCommonLands == null || rawCommonLands.Count == 0)
1619
return [];
@@ -33,6 +36,8 @@ public static List<SamHoldingDocument> ToSilver(List<SamCommonLand> rawCommonLan
3336
});
3437
}
3538

39+
var (countryId, countryCode, _) = await resolveCountry(representative.COUNTRY, null, cancellationToken);
40+
3641
var holding = new SamHoldingDocument
3742
{
3843
LastUpdatedBatchId = representative.BATCH_ID,
@@ -64,10 +69,11 @@ public static List<SamHoldingDocument> ToSilver(List<SamCommonLand> rawCommonLan
6469
{
6570
IdentifierId = Guid.NewGuid().ToString(),
6671
AddressLine = representative.ADDRESS_LINE_1,
67-
AddressLocality = representative.ADDRESS_LINE_2,
68-
AddressStreet = representative.ADDRESS_LINE_3,
72+
AddressStreet = representative.ADDRESS_LINE_2,
73+
AddressTown = representative.ADDRESS_LINE_3,
6974
AddressPostCode = representative.POSTCODE,
70-
CountryCode = representative.COUNTRY
75+
CountryCode = countryCode,
76+
CountryIdentifier = countryId
7177
}
7278
}
7379
};

‎src/KeeperData.Application/Orchestration/Imports/Sam/Mappings/SamHoldingMapper.cs‎

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,19 @@ internal static SamHoldingDocument SelectRepresentativeHolding(List<SamHoldingDo
165165
return silverHoldings.OrderByDescending(h => h.LastUpdatedDate).First();
166166
}
167167

168+
public static SamHoldingDocument SelectAddressSource(List<SamHoldingDocument> silverHoldings)
169+
{
170+
const string commonLandBusinessUsage = "Common Land";
171+
172+
// Common land address takes precedence — use the most recently updated common land if present
173+
var commonLand = silverHoldings
174+
.Where(x => x.SourceFacilitySubBusinessActivityCode == commonLandBusinessUsage)
175+
.OrderByDescending(h => h.LastUpdatedDate)
176+
.FirstOrDefault();
177+
178+
return commonLand ?? SelectRepresentativeHolding(silverHoldings);
179+
}
180+
168181
public static async Task<SiteDocument?> ToGold(
169182
string goldSiteId,
170183
SiteDocument? existingSite,
@@ -185,6 +198,9 @@ internal static SamHoldingDocument SelectRepresentativeHolding(List<SamHoldingDo
185198
// Prefer SAM Holding over Common Land when selecting representative
186199
var representative = SelectRepresentativeHolding(silverHoldings);
187200

201+
// Common land address takes precedence over site address for location data
202+
var addressSource = SelectAddressSource(silverHoldings);
203+
188204
var distinctSpecies = await GetDistinctReferenceDataAsync(
189205
silverHoldings.Select(h => h.SpeciesTypeCode),
190206
findSpecies,
@@ -220,6 +236,7 @@ internal static SamHoldingDocument SelectRepresentativeHolding(List<SamHoldingDo
220236
var site = existingSite is not null
221237
? await UpdateSiteAsync(
222238
representative,
239+
addressSource,
223240
existingSite,
224241
goldSiteGroupMarks,
225242
goldParties,
@@ -232,6 +249,7 @@ internal static SamHoldingDocument SelectRepresentativeHolding(List<SamHoldingDo
232249
: await CreateSiteAsync(
233250
goldSiteId,
234251
representative,
252+
addressSource,
235253
goldSiteGroupMarks,
236254
goldParties,
237255
getCountryById,
@@ -341,6 +359,7 @@ private static bool IsActivityAlreadyAdded(List<SiteActivity> activities, string
341359
private static async Task<Site> CreateSiteAsync(
342360
string goldSiteId,
343361
SamHoldingDocument representative,
362+
SamHoldingDocument addressSource,
344363
List<SiteGroupMarkRelationshipDocument> goldSiteGroupMarks,
345364
List<PartyDocument> goldParties,
346365
Func<string?, CancellationToken, Task<CountryDocument?>> getCountryById,
@@ -350,13 +369,13 @@ private static async Task<Site> CreateSiteAsync(
350369
SiteIdentifierType? siteIdentifierType,
351370
CancellationToken cancellationToken)
352371
{
353-
var (address, communication) = await ResolveLocationPartsAsync(representative, getCountryById, cancellationToken);
372+
var (address, communication) = await ResolveLocationPartsAsync(addressSource, getCountryById, cancellationToken);
354373
var isPermanentLandHolding = representative.CphRelationshipType.IsPermanentLandHolding();
355374

356375
var location = Location.Create(
357-
representative.Location?.OsMapReference,
358-
representative.Location?.Easting,
359-
representative.Location?.Northing,
376+
addressSource.Location?.OsMapReference,
377+
addressSource.Location?.Easting,
378+
addressSource.Location?.Northing,
360379
address,
361380
communication: [communication]);
362381

@@ -384,6 +403,7 @@ private static async Task<Site> CreateSiteAsync(
384403

385404
private static async Task<Site> UpdateSiteAsync(
386405
SamHoldingDocument representative,
406+
SamHoldingDocument addressSource,
387407
SiteDocument existing,
388408
List<SiteGroupMarkRelationshipDocument> goldSiteGroupMarks,
389409
List<PartyDocument> goldParties,
@@ -410,16 +430,16 @@ private static async Task<Site> UpdateSiteAsync(
410430
representative.CphTypeIdentifier,
411431
isPermanentLandHolding ? representative.SecondaryCph : null);
412432

413-
var (updatedAddress, updatedCommunication) = await ResolveLocationPartsAsync(representative, getCountryById, cancellationToken);
433+
var (updatedAddress, updatedCommunication) = await ResolveLocationPartsAsync(addressSource, getCountryById, cancellationToken);
414434

415435
// Always set the derived site type (may be null if no mapping found).
416436
site.SetSiteType(siteType, representative.LastUpdatedDate);
417437

418438
site.SetLocation(
419439
representative.LastUpdatedDate,
420-
representative.Location?.OsMapReference,
421-
representative.Location?.Easting,
422-
representative.Location?.Northing,
440+
addressSource.Location?.OsMapReference,
441+
addressSource.Location?.Easting,
442+
addressSource.Location?.Northing,
423443
updatedAddress,
424444
[updatedCommunication]);
425445

‎tests/KeeperData.Application.Tests.Unit/Orchestration/Imports/Sam/Mappings/SamCommonLandMapperTests.cs‎

Lines changed: 32 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -9,34 +9,40 @@ namespace KeeperData.Application.Tests.Unit.Orchestration.Imports.Sam.Mappings;
99

1010
public class SamCommonLandMapperTests
1111
{
12+
private static readonly Func<string?, string?, CancellationToken, Task<(string?, string?, string?)>> NoopResolveCountry =
13+
(_, _, _) => Task.FromResult<(string?, string?, string?)>((null, null, null));
14+
15+
private static Func<string?, string?, CancellationToken, Task<(string?, string?, string?)>> ResolveCountry(string? id, string? code) =>
16+
(_, _, _) => Task.FromResult<(string?, string?, string?)>((id, code, null));
17+
1218
[Fact]
13-
public void ToSilver_WithNullInput_ShouldReturnEmptyList()
19+
public async Task ToSilver_WithNullInput_ShouldReturnEmptyList()
1420
{
1521
// Arrange
1622
List<SamCommonLand>? rawCommonLands = null;
1723

1824
// Act
19-
var result = SamCommonLandMapper.ToSilver(rawCommonLands!);
25+
var result = await SamCommonLandMapper.ToSilver(rawCommonLands!, NoopResolveCountry, CancellationToken.None);
2026

2127
// Assert
2228
result.Should().BeEmpty();
2329
}
2430

2531
[Fact]
26-
public void ToSilver_WithEmptyList_ShouldReturnEmptyList()
32+
public async Task ToSilver_WithEmptyList_ShouldReturnEmptyList()
2733
{
2834
// Arrange
2935
var rawCommonLands = new List<SamCommonLand>();
3036

3137
// Act
32-
var result = SamCommonLandMapper.ToSilver(rawCommonLands);
38+
var result = await SamCommonLandMapper.ToSilver(rawCommonLands, NoopResolveCountry, CancellationToken.None);
3339

3440
// Assert
3541
result.Should().BeEmpty();
3642
}
3743

3844
[Fact]
39-
public void ToSilver_WithDefinitionRecordOnly_ShouldCreateSamHoldingDocument()
45+
public async Task ToSilver_WithDefinitionRecordOnly_ShouldCreateSamHoldingDocument()
4046
{
4147
// Arrange
4248
var now = DateTime.UtcNow;
@@ -68,7 +74,10 @@ public void ToSilver_WithDefinitionRecordOnly_ShouldCreateSamHoldingDocument()
6874
};
6975

7076
// Act
71-
var result = SamCommonLandMapper.ToSilver(rawCommonLands);
77+
var result = await SamCommonLandMapper.ToSilver(
78+
rawCommonLands,
79+
ResolveCountry("country-id-1", "England"),
80+
CancellationToken.None);
7281

7382
// Assert
7483
result.Should().HaveCount(1);
@@ -88,14 +97,15 @@ public void ToSilver_WithDefinitionRecordOnly_ShouldCreateSamHoldingDocument()
8897
holding.Location.Northing.Should().Be(569204);
8998
holding.Location.Address.Should().NotBeNull();
9099
holding.Location.Address!.AddressLine.Should().Be("Land off Road");
91-
holding.Location.Address.AddressLocality.Should().Be("Village");
92-
holding.Location.Address.AddressStreet.Should().Be("District");
100+
holding.Location.Address.AddressStreet.Should().Be("Village");
101+
holding.Location.Address.AddressTown.Should().Be("District");
93102
holding.Location.Address.AddressPostCode.Should().Be("AB12 3CD");
94103
holding.Location.Address.CountryCode.Should().Be("England");
104+
holding.Location.Address.CountryIdentifier.Should().Be("country-id-1");
95105
}
96106

97107
[Fact]
98-
public void ToSilver_WithDeletedRecord_ShouldMapDeletedStatus()
108+
public async Task ToSilver_WithDeletedRecord_ShouldMapDeletedStatus()
99109
{
100110
// Arrange
101111
var rawCommonLands = new List<SamCommonLand>
@@ -112,7 +122,7 @@ public void ToSilver_WithDeletedRecord_ShouldMapDeletedStatus()
112122
};
113123

114124
// Act
115-
var result = SamCommonLandMapper.ToSilver(rawCommonLands);
125+
var result = await SamCommonLandMapper.ToSilver(rawCommonLands, NoopResolveCountry, CancellationToken.None);
116126

117127
// Assert
118128
result.Should().HaveCount(1);
@@ -121,7 +131,7 @@ public void ToSilver_WithDeletedRecord_ShouldMapDeletedStatus()
121131
}
122132

123133
[Fact]
124-
public void ToSilver_WithPlaceholderPremisesName_ShouldSetToNull()
134+
public async Task ToSilver_WithPlaceholderPremisesName_ShouldSetToNull()
125135
{
126136
// Arrange
127137
var rawCommonLands = new List<SamCommonLand>
@@ -136,14 +146,14 @@ public void ToSilver_WithPlaceholderPremisesName_ShouldSetToNull()
136146
};
137147

138148
// Act
139-
var result = SamCommonLandMapper.ToSilver(rawCommonLands);
149+
var result = await SamCommonLandMapper.ToSilver(rawCommonLands, NoopResolveCountry, CancellationToken.None);
140150

141151
// Assert
142152
result[0].LocationName.Should().BeNull();
143153
}
144154

145155
[Fact]
146-
public void ToSilver_WithEmptyCommonCph_ShouldBeFiltered()
156+
public async Task ToSilver_WithEmptyCommonCph_ShouldBeFiltered()
147157
{
148158
// Arrange
149159
var rawCommonLands = new List<SamCommonLand>
@@ -163,14 +173,14 @@ public void ToSilver_WithEmptyCommonCph_ShouldBeFiltered()
163173
};
164174

165175
// Act
166-
var result = SamCommonLandMapper.ToSilver(rawCommonLands);
176+
var result = await SamCommonLandMapper.ToSilver(rawCommonLands, NoopResolveCountry, CancellationToken.None);
167177

168178
// Assert
169179
result.Should().BeEmpty();
170180
}
171181

172182
[Fact]
173-
public void ToSilver_WithInvalidEastingNorthing_ShouldSetToNull()
183+
public async Task ToSilver_WithInvalidEastingNorthing_ShouldSetToNull()
174184
{
175185
// Arrange
176186
var rawCommonLands = new List<SamCommonLand>
@@ -187,15 +197,15 @@ public void ToSilver_WithInvalidEastingNorthing_ShouldSetToNull()
187197
};
188198

189199
// Act
190-
var result = SamCommonLandMapper.ToSilver(rawCommonLands);
200+
var result = await SamCommonLandMapper.ToSilver(rawCommonLands, NoopResolveCountry, CancellationToken.None);
191201

192202
// Assert
193203
result[0].Location!.Easting.Should().BeNull();
194204
result[0].Location!.Northing.Should().BeNull();
195205
}
196206

197207
[Fact]
198-
public void ToSilver_WithFutureDate_ShouldNormaliseToNull()
208+
public async Task ToSilver_WithFutureDate_ShouldNormaliseToNull()
199209
{
200210
// Arrange
201211
var rawCommonLands = new List<SamCommonLand>
@@ -216,7 +226,7 @@ public void ToSilver_WithFutureDate_ShouldNormaliseToNull()
216226
};
217227

218228
// Act
219-
var result = SamCommonLandMapper.ToSilver(rawCommonLands);
229+
var result = await SamCommonLandMapper.ToSilver(rawCommonLands, NoopResolveCountry, CancellationToken.None);
220230

221231
// Assert
222232
result[1].AssociatedMainHoldings[0].StartDate.Should().BeNull();
@@ -376,7 +386,7 @@ public void ToAssociatedCommonLands_WithVariousDateFormats_ShouldNormaliseCorrec
376386
}
377387

378388
[Fact]
379-
public void ToSilver_ShouldMapBusinessUsageToSourceFacilitySubBusinessActivityCode()
389+
public async Task ToSilver_ShouldMapBusinessUsageToSourceFacilitySubBusinessActivityCode()
380390
{
381391
// Arrange
382392
var rawCommonLands = new List<SamCommonLand>
@@ -392,15 +402,15 @@ public void ToSilver_ShouldMapBusinessUsageToSourceFacilitySubBusinessActivityCo
392402
};
393403

394404
// Act
395-
var result = SamCommonLandMapper.ToSilver(rawCommonLands);
405+
var result = await SamCommonLandMapper.ToSilver(rawCommonLands, NoopResolveCountry, CancellationToken.None);
396406

397407
// Assert
398408
result.Should().HaveCount(1);
399409
result[0].SourceFacilitySubBusinessActivityCode.Should().Be("Common Land");
400410
}
401411

402412
[Fact]
403-
public void ToSilver_ShouldMapSiteTypeCodeToCL()
413+
public async Task ToSilver_ShouldMapSiteTypeCodeToCL()
404414
{
405415
// Arrange
406416
var rawCommonLands = new List<SamCommonLand>
@@ -416,7 +426,7 @@ public void ToSilver_ShouldMapSiteTypeCodeToCL()
416426
};
417427

418428
// Act
419-
var result = SamCommonLandMapper.ToSilver(rawCommonLands);
429+
var result = await SamCommonLandMapper.ToSilver(rawCommonLands, NoopResolveCountry, CancellationToken.None);
420430

421431
// Assert
422432
result.Should().HaveCount(1);

0 commit comments

Comments
 (0)