review changes

Author: nhsd-david-wass

internal/datastore/src/__test__/supplier-config-repository.test.ts

@@ -1,12 +1,10 @@
-import { Logger } from "pino";
 import { PutCommand } from "@aws-sdk/lib-dynamodb";
 import {
   DBContext,
   createTables,
   deleteTables,
   setupDynamoDBContainer,
 } from "./db";
-import { LogStream, createTestLogger } from "./logs";
 import { SupplierConfigRepository } from "../supplier-config-repository";
 
 function createLetterVariantItem(variantId: string) {
@@ -26,10 +24,10 @@ function createLetterVariantItem(variantId: string) {
 function createVolumeGroupItem(groupId: string, status = "PROD") {
   const startDate = new Date(Date.now() - 24 * 1000 * 60 * 60)
     .toISOString()
-    .split("T")[0]; // Started an hour ago to ensure it's active based on start date. Tests can override this if needed.
+    .split("T")[0]; // Started a day ago to ensure it's active based on start date. Tests can override this if needed.
   const endDate = new Date(Date.now() + 24 * 1000 * 60 * 60)
     .toISOString()
-    .split("T")[0]; // Ends in an hour to ensure it's active based on end date. Tests can override this if needed.
+    .split("T")[0]; // Ends in a day to ensure it's active based on end date. Tests can override this if needed.
   return {
     PK: "VOLUME_GROUP",
     SK: groupId,
@@ -75,8 +73,6 @@ jest.setTimeout(30_000);
 describe("SupplierConfigRepository", () => {
   let dbContext: DBContext;
   let repository: SupplierConfigRepository;
-  let logStream: LogStream;
-  let logger: Logger;
 
   // Database tests can take longer, especially with setup and teardown
   beforeAll(async () => {
@@ -85,10 +81,8 @@ describe("SupplierConfigRepository", () => {
 
   beforeEach(async () => {
     await createTables(dbContext);
-    ({ logStream, logger } = createTestLogger());
     repository = new SupplierConfigRepository(
       dbContext.docClient,
-      logger,
       dbContext.config,
     );
   });
@@ -120,8 +114,6 @@ describe("SupplierConfigRepository", () => {
     expect(result.status).toBe("PROD");
     expect(result.volumeGroupId).toBe(`group-${variantId}`);
     expect(result.packSpecificationIds).toEqual([`pack-spec-${variantId}`]);
-
-    expect(logStream.logs).toEqual([]);
   });
 
   test("getLetterVariant throws error for non-existent variant", async () => {
@@ -130,12 +122,6 @@ describe("SupplierConfigRepository", () => {
     await expect(repository.getLetterVariant(variantId)).rejects.toThrow(
       `No letter variant details found for id ${variantId}`,
     );
-
-    expect(
-      logStream.logs.some((log) =>
-        log.includes("No letter variant found for id"),
-      ),
-    ).toBe(true);
   });
 
   test("getVolumeGroup returns correct details for existing group", async () => {
@@ -155,8 +141,6 @@ describe("SupplierConfigRepository", () => {
     expect(result.status).toBe("PROD");
     expect(new Date(result.startDate).getTime()).toBeLessThan(Date.now());
     expect(new Date(result.endDate!).getTime()).toBeGreaterThan(Date.now());
-
-    expect(logStream.logs).toEqual([]);
   });
 
   test("getVolumeGroup throws error for non-existent group", async () => {
@@ -165,12 +149,6 @@ describe("SupplierConfigRepository", () => {
     await expect(repository.getVolumeGroup(groupId)).rejects.toThrow(
       `No volume group details found for id ${groupId}`,
     );
-
-    expect(
-      logStream.logs.some((log) =>
-        log.includes("No volume group found for id"),
-      ),
-    ).toBe(true);
   });
 
   test("getSupplierAllocationsForVolumeGroup returns correct allocations", async () => {
@@ -197,8 +175,6 @@ describe("SupplierConfigRepository", () => {
         allocationPercentage: 50,
       },
     ]);
-
-    expect(logStream.logs).toEqual([]);
   });
 
   test("getSupplierAllocationsForVolumeGroup returns multiple allocations", async () => {
@@ -245,7 +221,6 @@ describe("SupplierConfigRepository", () => {
         },
       ]),
     );
-    expect(logStream.logs).toEqual([]);
   });
 
   test("getSupplierAllocationsForVolumeGroup throws error for non-existent group", async () => {
@@ -256,12 +231,6 @@ describe("SupplierConfigRepository", () => {
     ).rejects.toThrow(
       `No active supplier allocations found for volume group id ${groupId}`,
     );
-
-    expect(
-      logStream.logs.some((log) =>
-        log.includes("No supplier allocations found for volume group id"),
-      ),
-    ).toBe(true);
   });
 
   test("getSuppliersDetails returns correct supplier details", async () => {
@@ -285,7 +254,6 @@ describe("SupplierConfigRepository", () => {
         status: "PROD",
       },
     ]);
-    expect(logStream.logs).toEqual([]);
   });
 
   test("getSuppliersDetails throws error for non-existent supplier", async () => {
@@ -294,9 +262,5 @@ describe("SupplierConfigRepository", () => {
     await expect(repository.getSuppliersDetails([supplierId])).rejects.toThrow(
       `Supplier with id ${supplierId} not found`,
     );
-
-    expect(
-      logStream.logs.some((log) => log.includes("No supplier found for id")),
-    ).toBe(true);
   });
 });

internal/datastore/src/supplier-config-repository.ts

@@ -3,7 +3,6 @@ import {
   GetCommand,
   QueryCommand,
 } from "@aws-sdk/lib-dynamodb";
-import { Logger } from "pino";
 import {
   $LetterVariant,
   $Supplier,
@@ -22,7 +21,6 @@ export type SupplierConfigRepositoryConfig = {
 export class SupplierConfigRepository {
   constructor(
     readonly ddbClient: DynamoDBDocumentClient,
-    readonly log: Logger,
     readonly config: SupplierConfigRepositoryConfig,
   ) {}
 
@@ -34,10 +32,6 @@ export class SupplierConfigRepository {
       }),
     );
     if (!result.Item) {
-      this.log.error({
-        description: "No letter variant found for id",
-        variantId,
-      });
       throw new Error(`No letter variant details found for id ${variantId}`);
     }
 
@@ -52,10 +46,6 @@ export class SupplierConfigRepository {
       }),
     );
     if (!result.Item) {
-      this.log.error({
-        description: "No volume group found for id",
-        groupId,
-      });
       throw new Error(`No volume group details found for id ${groupId}`);
     }
     return $VolumeGroup.parse(result.Item);
@@ -83,10 +73,6 @@ export class SupplierConfigRepository {
       }),
     );
     if (!result.Items || result.Items.length === 0) {
-      this.log.error({
-        description: "No supplier allocations found for volume group id",
-        groupId,
-      });
       throw new Error(
         `No active supplier allocations found for volume group id ${groupId}`,
       );
@@ -105,10 +91,6 @@ export class SupplierConfigRepository {
         }),
       );
       if (!result.Item) {
-        this.log.error({
-          description: "No supplier found for id",
-          supplierId,
-        });
         throw new Error(`Supplier with id ${supplierId} not found`);
       }
       suppliers.push($Supplier.parse(result.Item));

lambdas/supplier-allocator/src/config/__tests__/deps.test.ts

@@ -47,7 +47,7 @@ describe("createDependenciesContainer", () => {
     expect(createLogger).toHaveBeenCalledTimes(1);
     expect(SupplierConfigRepository).toHaveBeenCalledTimes(1);
     const supplierConfigRepoCtorArgs = SupplierConfigRepository.mock.calls[0];
-    expect(supplierConfigRepoCtorArgs[2]).toEqual({
+    expect(supplierConfigRepoCtorArgs[1]).toEqual({
       supplierConfigTableName: "SupplierConfigTable",
     });
     expect(deps.env).toEqual(env);

lambdas/supplier-allocator/src/config/deps.ts

@@ -27,7 +27,7 @@ function createSupplierConfigRepository(
     supplierConfigTableName: envVars.SUPPLIER_CONFIG_TABLE_NAME,
   };
 
-  return new SupplierConfigRepository(createDocumentClient(), log, config);
+  return new SupplierConfigRepository(createDocumentClient(), config);
 }
 
 export function createDependenciesContainer(): Deps {

lambdas/supplier-allocator/src/handler/allocate-handler.ts

@@ -71,8 +71,8 @@ async function getSupplierFromConfig(letterEvent: PreparedEvents, deps: Deps) {
   const supplierAllocations: SupplierAllocation[] =
     await getSupplierAllocationsForVolumeGroup(
       variantDetails.volumeGroupId,
-      variantDetails.supplierId ?? "",
       deps,
+      variantDetails.supplierId,
     );
 
   const supplierDetails: Supplier[] = await getSupplierDetails(

lambdas/supplier-allocator/src/services/__tests__/supplier-config.test.ts

@@ -5,11 +5,13 @@ import {
   getVolumeGroupDetails,
 } from "../supplier-config";
 import { Deps } from "../../config/deps";
+import { warn } from "node:console";
 
 function makeDeps(overrides: Partial<Deps> = {}): Deps {
   const logger = {
     info: jest.fn(),
     error: jest.fn(),
+    warn: jest.fn(),
   } as unknown as Deps["logger"];
 
   const supplierConfigRepo = {
@@ -146,7 +148,7 @@ describe("supplier-config service", () => {
         .fn()
         .mockResolvedValue(allocations);
 
-      const result = await getSupplierAllocationsForVolumeGroup("g1", "", deps);
+      const result = await getSupplierAllocationsForVolumeGroup("g1", deps);
 
       expect(result).toEqual(allocations);
     });
@@ -159,8 +161,8 @@ describe("supplier-config service", () => {
 
       const result = await getSupplierAllocationsForVolumeGroup(
         "g1",
-        "s2",
         deps,
+        "s2",
       );
 
       expect(result).toEqual([allocations[1]]);
@@ -173,7 +175,7 @@ describe("supplier-config service", () => {
         .mockResolvedValue(allocations);
 
       await expect(
-        getSupplierAllocationsForVolumeGroup("g1", "missing", deps),
+        getSupplierAllocationsForVolumeGroup("g1", deps, "missing"),
       ).rejects.toThrow(/No supplier allocations found/);
       expect(deps.logger.error).toHaveBeenCalled();
     });
@@ -240,4 +242,47 @@ describe("supplier-config service", () => {
       ]);
     });
   });
+  it("logs a warning when supplier allocations count differs from supplier details count", async () => {
+    const allocations = [
+      { supplier: "s1", variantId: "v1" },
+      { supplier: "s2", variantId: "v2" },
+      { supplier: "s3", variantId: "v3" },
+    ] as any[];
+    const suppliers = [
+      { id: "s1", name: "Supplier 1" },
+      { id: "s2", name: "Supplier 2" },
+    ] as any[];
+    const deps = makeDeps();
+    deps.supplierConfigRepo.getSuppliersDetails = jest
+      .fn()
+      .mockResolvedValue(suppliers);
+
+    await getSupplierDetails(allocations, deps);
+
+    expect(deps.logger.warn).toHaveBeenCalledWith({
+      description: "Mismatch between supplier allocations and supplier details",
+      allocationsCount: 3,
+      detailsCount: 2,
+      missingSuppliers: ["s3"],
+    });
+  });
+
+  it("does not log a warning when counts match", async () => {
+    const allocations = [
+      { supplier: "s1", variantId: "v1" },
+      { supplier: "s2", variantId: "v2" },
+    ] as any[];
+    const suppliers = [
+      { id: "s1", name: "Supplier 1" },
+      { id: "s2", name: "Supplier 2" },
+    ] as any[];
+    const deps = makeDeps();
+    deps.supplierConfigRepo.getSuppliersDetails = jest
+      .fn()
+      .mockResolvedValue(suppliers);
+
+    await getSupplierDetails(allocations, deps);
+
+    expect(deps.logger.warn).not.toHaveBeenCalled();
+  });
 });

lambdas/supplier-allocator/src/services/supplier-config.ts

@@ -41,8 +41,8 @@ export async function getVolumeGroupDetails(
 
 export async function getSupplierAllocationsForVolumeGroup(
   groupId: string,
-  supplierId: string,
   deps: Deps,
+  supplierId?: string,
 ): Promise<SupplierAllocation[]> {
   const allocations =
     await deps.supplierConfigRepo.getSupplierAllocationsForVolumeGroup(groupId);
@@ -86,5 +86,18 @@ export async function getSupplierDetails(
       `No supplier details found for supplier ids ${supplierIds.join(", ")}`,
     );
   }
+  // Log a warning if some supplier details are missing compared to allocations
+  if (supplierAllocations.length !== supplierDetails.length) {
+    const foundSupplierIds = new Set(supplierDetails.map((s) => s.id));
+    const missingSupplierIds = supplierIds.filter(
+      (id) => !foundSupplierIds.has(id),
+    );
+    deps.logger.warn({
+      description: "Mismatch between supplier allocations and supplier details",
+      allocationsCount: supplierAllocations.length,
+      detailsCount: supplierDetails.length,
+      missingSuppliers: missingSupplierIds,
+    });
+  }
   return supplierDetails;
 }