diff --git a/config/suppliers/letter-variant/client3-colour-admail.json b/config/suppliers/letter-variant/client3-colour-admail.json index 4d05b6272..2cd385c2b 100644 --- a/config/suppliers/letter-variant/client3-colour-admail.json +++ b/config/suppliers/letter-variant/client3-colour-admail.json @@ -26,7 +26,7 @@ "id": "client3-colour-admail", "name": "Client 3 Colour Admail", "packSpecificationIds": [ - "notify-admail-colour-whitemail" + "notify-admail-whitemail" ], "priority": 50, "status": "INT", diff --git a/config/suppliers/letter-variant/notify-standard-colour.json b/config/suppliers/letter-variant/notify-standard-colour.json index 772a4146c..056e56f67 100644 --- a/config/suppliers/letter-variant/notify-standard-colour.json +++ b/config/suppliers/letter-variant/notify-standard-colour.json @@ -29,6 +29,7 @@ ], "priority": 99, "status": "INT", + "supplierId": "supplier1", "type": "STANDARD", - "volumeGroupId": "volumeGroup-test3" + "volumeGroupId": "volumeGroup-test1" } diff --git a/infrastructure/terraform/components/api/module_lambda_upsert_letter.tf b/infrastructure/terraform/components/api/module_lambda_upsert_letter.tf index 7519d1556..eeaed3420 100644 --- a/infrastructure/terraform/components/api/module_lambda_upsert_letter.tf +++ b/infrastructure/terraform/components/api/module_lambda_upsert_letter.tf @@ -67,11 +67,26 @@ data "aws_iam_policy_document" "upsert_letter_lambda" { resources = [ aws_dynamodb_table.letters.arn, - aws_dynamodb_table.idempotency.arn, "${aws_dynamodb_table.letters.arn}/index/supplierStatus-index" ] } + statement { + sid = "AllowIdempotencyTableWrite" + effect = "Allow" + + actions = [ + "dynamodb:PutItem", + "dynamodb:GetItem", + "dynamodb:UpdateItem", + "dynamodb:DeleteItem" + ] + + resources = [ + aws_dynamodb_table.idempotency.arn, + ] + } + statement { sid = "AllowSQSRead" effect = "Allow" diff --git a/lambdas/supplier-allocator/src/handler/__tests__/allocate-handler.test.ts b/lambdas/supplier-allocator/src/handler/__tests__/allocate-handler.test.ts index cc02087cd..97d6e391a 100644 --- a/lambdas/supplier-allocator/src/handler/__tests__/allocate-handler.test.ts +++ b/lambdas/supplier-allocator/src/handler/__tests__/allocate-handler.test.ts @@ -744,6 +744,7 @@ describe("createSupplierAllocatorHandler", () => { expect(allocationConfig.selectSupplierByFactor).toHaveBeenCalledWith( expect.any(Array), expect.any(Array), + "letter1", mockedDeps, ); }); diff --git a/lambdas/supplier-allocator/src/handler/__tests__/allocation-config.test.ts b/lambdas/supplier-allocator/src/handler/__tests__/allocation-config.test.ts index 17ce944a5..ef3e576f2 100644 --- a/lambdas/supplier-allocator/src/handler/__tests__/allocation-config.test.ts +++ b/lambdas/supplier-allocator/src/handler/__tests__/allocation-config.test.ts @@ -196,6 +196,37 @@ describe("eligibleSuppliers", () => { "Supplier service error", ); }); + it("should filter allocations by letterVariantSupplierId if provided", async () => { + ( + supplierConfigService.getSupplierAllocationsForVolumeGroup as jest.Mock + ).mockResolvedValue(mockSupplierAllocations); + (supplierConfigService.getSupplierDetails as jest.Mock).mockResolvedValue( + mockSuppliers, + ); + + const letterVariantSupplierId = "supplier-1"; + await eligibleSuppliers(mockVolumeGroup, mockDeps, letterVariantSupplierId); + expect(supplierConfigService.getSupplierDetails).toHaveBeenCalledWith( + ["supplier-1"], + mockDeps, + ); + }); + it("should log a warning if no allocations found for specified letter variant supplier", async () => { + ( + supplierConfigService.getSupplierAllocationsForVolumeGroup as jest.Mock + ).mockResolvedValue([]); + (supplierConfigService.getSupplierDetails as jest.Mock).mockResolvedValue( + [], + ); + + const letterVariantSupplierId = "supplier-1"; + await eligibleSuppliers(mockVolumeGroup, mockDeps, letterVariantSupplierId); + expect(mockDeps.logger.warn).toHaveBeenCalledWith({ + description: "No allocations found for specified letter variant supplier", + volumeGroupId: mockVolumeGroup.id, + letterVariantSupplierId, + }); + }); }); describe("preferredSupplierPack", () => { @@ -779,6 +810,7 @@ describe("selectSupplierByFactor", () => { let mockDeps: jest.Mocked; let mockSuppliers: Supplier[]; let mockSupplierAllocations: SupplierAllocation[]; + const domainId = "test-domain-id"; beforeEach(() => { jest.clearAllMocks(); @@ -844,6 +876,7 @@ describe("selectSupplierByFactor", () => { const result = await selectSupplierByFactor( mockSuppliers, mockSupplierAllocations, + domainId, mockDeps, ); @@ -875,6 +908,7 @@ describe("selectSupplierByFactor", () => { const result = await selectSupplierByFactor( mockSuppliers, allocationsWithMissingSupplier, + domainId, mockDeps, ); @@ -906,6 +940,7 @@ describe("selectSupplierByFactor", () => { const result = await selectSupplierByFactor( mockSuppliers, allocationsWithZeroPercentage, + domainId, mockDeps, ); @@ -936,7 +971,12 @@ describe("selectSupplierByFactor", () => { ]; await expect( - selectSupplierByFactor(mockSuppliers, zeroAllocations, mockDeps), + selectSupplierByFactor( + mockSuppliers, + zeroAllocations, + domainId, + mockDeps, + ), ).rejects.toThrow( "No valid supplier allocations found for suppliers with valid pack", ); @@ -956,6 +996,7 @@ describe("selectSupplierByFactor", () => { const result = await selectSupplierByFactor( mockSuppliers, mockSupplierAllocations, + domainId, mockDeps, ); @@ -975,6 +1016,7 @@ describe("selectSupplierByFactor", () => { const result = await selectSupplierByFactor( singleSupplier, singleAllocation, + domainId, mockDeps, ); @@ -995,6 +1037,7 @@ describe("selectSupplierByFactor", () => { const result = await selectSupplierByFactor( mockSuppliers, mockSupplierAllocations, + domainId, mockDeps, ); @@ -1015,6 +1058,7 @@ describe("selectSupplierByFactor", () => { const result = await selectSupplierByFactor( mockSuppliers, mockSupplierAllocations, + domainId, mockDeps, ); @@ -1028,7 +1072,12 @@ describe("selectSupplierByFactor", () => { ).mockRejectedValue(error); await expect( - selectSupplierByFactor(mockSuppliers, mockSupplierAllocations, mockDeps), + selectSupplierByFactor( + mockSuppliers, + mockSupplierAllocations, + domainId, + mockDeps, + ), ).rejects.toThrow("Factor calculation error"); }); @@ -1053,6 +1102,7 @@ describe("selectSupplierByFactor", () => { const result = await selectSupplierByFactor( mockSuppliers, allocationsWithUnrelatedSupplier, + domainId, mockDeps, ); @@ -1093,6 +1143,7 @@ describe("selectSupplierByFactor", () => { const result = await selectSupplierByFactor( manySuppliers, manyAllocations, + domainId, mockDeps, ); @@ -1105,7 +1156,12 @@ describe("selectSupplierByFactor", () => { ).mockResolvedValue([]); await expect( - selectSupplierByFactor(mockSuppliers, mockSupplierAllocations, mockDeps), + selectSupplierByFactor( + mockSuppliers, + mockSupplierAllocations, + domainId, + mockDeps, + ), ).rejects.toThrow("No supplier factors could be calculated for allocation"); }); }); diff --git a/lambdas/supplier-allocator/src/handler/allocate-handler.ts b/lambdas/supplier-allocator/src/handler/allocate-handler.ts index 397a53abe..de532ae06 100644 --- a/lambdas/supplier-allocator/src/handler/allocate-handler.ts +++ b/lambdas/supplier-allocator/src/handler/allocate-handler.ts @@ -29,7 +29,6 @@ import { selectSupplierByFactor, suppliersWithValidPack, } from "./allocation-config"; - import { Deps } from "../config/deps"; import { PreparedEventSchema, PreparedEvents, SupplierDetails } from "./types"; @@ -94,7 +93,7 @@ async function getSupplierFromConfig( ); const { supplierAllocations, suppliers: allocatedSuppliers } = - await eligibleSuppliers(volumeGroup, deps); + await eligibleSuppliers(volumeGroup, deps, letterVariant.supplierId); const preferredPack: PackSpecification = await preferredSupplierPack( letterEvent, @@ -125,17 +124,20 @@ async function getSupplierFromConfig( ? await selectSupplierByFactor( suppliersForPackWithCapacity, supplierAllocations, + letterEvent.data.domainId, deps, ) : undefined) ?? (await selectSupplierByFactor( allSuppliersForPack, supplierAllocations, + letterEvent.data.domainId, deps, )); deps.logger.info({ description: "Fetched supplier details for supplier allocations", + domainId: letterEvent.data.domainId, variantId: letterEvent.data.letterVariantId, volumeGroupId: volumeGroup.id, supplierAllocationIds: supplierAllocations.map((a) => a.id), @@ -391,6 +393,9 @@ export default function createSupplierAllocatorHandler(deps: Deps): SQSHandler { ({ priority, supplier } = supplierAllocationResult); } catch (error) { + console.log( + `Error processing allocation of record ${record.messageId}: ${error}`, + ); deps.logger.error({ description: "Error processing allocation of record", err: error, diff --git a/lambdas/supplier-allocator/src/handler/allocation-config.ts b/lambdas/supplier-allocator/src/handler/allocation-config.ts index f029b619a..f9e7f92d1 100644 --- a/lambdas/supplier-allocator/src/handler/allocation-config.ts +++ b/lambdas/supplier-allocator/src/handler/allocation-config.ts @@ -23,6 +23,7 @@ import { PreparedEvents } from "./types"; export async function eligibleSuppliers( volumeGroup: VolumeGroup, deps: Deps, + letterVariantSupplierId?: string, ): Promise<{ supplierAllocations: SupplierAllocation[]; suppliers: Supplier[]; @@ -31,6 +32,32 @@ export async function eligibleSuppliers( volumeGroup.id, deps, ); + if (letterVariantSupplierId) { + deps.logger.info({ + description: "Filtering allocations for letter variant supplier", + volumeGroupId: volumeGroup.id, + letterVariantSupplierId, + }); + const filteredAllocations = supplierAllocations.find( + (alloc) => alloc.supplier === letterVariantSupplierId, + ); + if (!filteredAllocations) { + deps.logger.warn({ + description: + "No allocations found for specified letter variant supplier", + volumeGroupId: volumeGroup.id, + letterVariantSupplierId, + }); + } + return { + supplierAllocations: filteredAllocations ? [filteredAllocations] : [], + suppliers: await getSupplierDetails( + filteredAllocations ? [filteredAllocations.supplier] : [], + deps, + ), + }; + } + const allocationPercentageSum = supplierAllocations.reduce( (sum, alloc) => sum + alloc.allocationPercentage, 0, @@ -121,6 +148,7 @@ export async function filterSuppliersWithCapacity( export async function selectSupplierByFactor( suppliers: Supplier[], supplierAllocations: SupplierAllocation[], + domainId: string, deps: Deps, ): Promise { const supplierAllocationsForPack = supplierAllocations.filter((alloc) => { @@ -148,6 +176,7 @@ export async function selectSupplierByFactor( deps.logger.info({ description: "Calculated supplier factors for allocation", + domainId, supplierFactors, }); let selectedSupplierId = supplierFactors[0].supplierId; diff --git a/lambdas/supplier-allocator/src/services/supplier-config.ts b/lambdas/supplier-allocator/src/services/supplier-config.ts index 52ac1c434..7ed2509bd 100644 --- a/lambdas/supplier-allocator/src/services/supplier-config.ts +++ b/lambdas/supplier-allocator/src/services/supplier-config.ts @@ -301,6 +301,7 @@ export async function filterPacksForLetter( if (violatedConstraints.length > 0) { deps.logger.info({ description: `Pack specification filtered out based on pageCount constraints`, + dommainId: letterEvent.data.domainId, packSpecId, pageCount, violatedConstraints, diff --git a/tests/component-tests/allocation-tests/allocation-letter-variant.spec.ts b/tests/component-tests/allocation-tests/allocation-letter-variant.spec.ts new file mode 100644 index 000000000..d1489014b --- /dev/null +++ b/tests/component-tests/allocation-tests/allocation-letter-variant.spec.ts @@ -0,0 +1,35 @@ +import { randomUUID } from "node:crypto"; +import { expect, test } from "@playwright/test"; +import { + FetchedSupplierLog, + getAllocationDetailsForDomainId, + getVariantsForAllocation, +} from "tests/helpers/allocation-helper"; + +import { createPreparedV1Event } from "tests/helpers/event-fixtures"; +import { sendSnsEvent } from "tests/helpers/send-sns-event"; + +test.describe("Letter Variant Tests", () => { + test.setTimeout(180_000); // 3 minutes for long running polling + + test("Verify that supplierId on letter variant takes precedence over volume group", async () => { + const letterVariant = getVariantsForAllocation(3); + const domainId = randomUUID(); + const preparedEvent = createPreparedV1Event({ + domainId, + letterVariantId: letterVariant, + }); + + const response = await sendSnsEvent(preparedEvent); + + expect(response.MessageId).toBeTruthy(); + + const allocationDetails: FetchedSupplierLog = + await getAllocationDetailsForDomainId(domainId); + + expect(allocationDetails.allocatedSuppliers).toHaveLength(1); + + const allocatedSupplierId = allocationDetails.allocatedSuppliers?.[0]?.id; + expect(allocatedSupplierId).toBe("supplier1"); + }); +}); diff --git a/tests/component-tests/allocation-tests/letter-allocation-capacity.spec.ts b/tests/component-tests/allocation-tests/letter-allocation-capacity.spec.ts index 7a172a4a8..616ab1df9 100644 --- a/tests/component-tests/allocation-tests/letter-allocation-capacity.spec.ts +++ b/tests/component-tests/allocation-tests/letter-allocation-capacity.spec.ts @@ -66,6 +66,7 @@ test.describe("Allocator Lambda Tests", () => { const supplierAllocatorLog = await getAllocationLog( "Pack specification filtered out based on pageCount constraints", + { extraPatterns: [domainId] }, ); const filteredPackSpecId = supplierAllocatorLog.packSpecId; logger.info(`Pack spec filtered out ${filteredPackSpecId}`); diff --git a/tests/component-tests/allocation-tests/letter-allocation-weighting.spec.ts b/tests/component-tests/allocation-tests/letter-allocation-weighting.spec.ts index d51cbedbd..25340aada 100644 --- a/tests/component-tests/allocation-tests/letter-allocation-weighting.spec.ts +++ b/tests/component-tests/allocation-tests/letter-allocation-weighting.spec.ts @@ -97,6 +97,7 @@ test.describe("Allocator Weighting Tests", () => { "Calculated supplier factors for allocation", { startTimeMs: testStartedAt, + extraPatterns: [domainId], }, ); diff --git a/tests/component-tests/allocation-tests/supplier-allocation.spec.ts b/tests/component-tests/allocation-tests/supplier-allocation.spec.ts index d33a6cc7c..fc1f03c27 100644 --- a/tests/component-tests/allocation-tests/supplier-allocation.spec.ts +++ b/tests/component-tests/allocation-tests/supplier-allocation.spec.ts @@ -117,6 +117,8 @@ test.describe("Supplier Allocation Tests", () => { logger.info( `New total daily allocation for date ${allocationDate}: ${newTotalDailyAllocation}`, ); - expect(newTotalDailyAllocation).toBe(originalTotalDailyAllocation + 1); + expect(newTotalDailyAllocation).toBeGreaterThanOrEqual( + originalTotalDailyAllocation + 1, + ); }); }); diff --git a/tests/helpers/allocation-helper.ts b/tests/helpers/allocation-helper.ts index bed146780..e73ccb1fc 100644 --- a/tests/helpers/allocation-helper.ts +++ b/tests/helpers/allocation-helper.ts @@ -9,6 +9,7 @@ import { import { envName } from "tests/constants/api-constants"; import { pollAllocatorLogWithOptions, + pollSupplierAllocatorLogForAllocationDetails, pollSupplierAllocatorLogForExceededDailyCapacity, pollSupplierAllocatorLogForResolvedSpec, } from "./aws-cloudwatch-helper"; @@ -83,6 +84,22 @@ export type AllocationLogOptions = { extraPatterns?: string[]; }; +export type AllocatedSuppliersLogEntry = { + id: string; + name: string; + channelType: string; + dailyCapacity: number; + status: string; +}; + +export type FetchedSupplierLog = { + supplierAllocationIds?: string[]; + allocatedSuppliers?: AllocatedSuppliersLogEntry[]; + allSuppliersForPack?: string[]; + supplierForPackWithCapacity?: string[]; + selectedSupplierId?: string; +}; + type LetterVariantConfig = { id: string; packSpecificationIds: string[]; @@ -137,6 +154,15 @@ export async function getAllocationLogForDomainId( return supplierAllocatorLog; } +export async function getAllocationDetailsForDomainId( + domainId: string, +): Promise { + const message = await pollSupplierAllocatorLogForAllocationDetails(domainId); + const fetchedSupplierLog = JSON.parse(message) as FetchedSupplierLog; + + return fetchedSupplierLog; +} + export async function getAllocationLog< TLog extends { description: string } = PackSpecificationLog, >(description: string, options?: AllocationLogOptions): Promise { diff --git a/tests/helpers/aws-cloudwatch-helper.ts b/tests/helpers/aws-cloudwatch-helper.ts index af9c56553..c31d0933b 100644 --- a/tests/helpers/aws-cloudwatch-helper.ts +++ b/tests/helpers/aws-cloudwatch-helper.ts @@ -60,6 +60,16 @@ export async function pollSupplierAllocatorLogForResolvedSpec( `"${domainId}"`, ]); } + +export async function pollSupplierAllocatorLogForAllocationDetails( + domainId: string, +): Promise { + return pollLambdaLog("supplier-allocator", [ + '"Fetched supplier details for supplier allocations"', + `"${domainId}"`, + ]); +} + export async function pollSupplierAllocatorLogForError( msgToCheck: string, domainId?: string,