CCM-20032: Letter Variant SupplierId Not Honoured

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",

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"
 }

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"

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,
     );
   });

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<Deps>;
   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");
   });
 });

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,

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<string> {
   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;

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,

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");
+  });
+});

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}`);

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],
         },
       );
 

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,
+    );
   });
 });