From 1b08d0ce799d49e565f3b7db752a7e0f7bde1628 Mon Sep 17 00:00:00 2001 From: Francisco Videira Date: Tue, 24 Mar 2026 11:37:16 +0000 Subject: [PATCH 1/8] Refactor handling of letter not found --- .github/PULL_REQUEST_TEMPLATE.md | 6 ++++++ .../src/__test__/letter-queue-repository.test.ts | 2 +- .../datastore/src/__test__/letter-repository.test.ts | 2 +- .../letter-already-exists-error.ts} | 0 .../datastore/src/errors/letter-not-found-error.ts | 12 ++++++++++++ internal/datastore/src/index.ts | 2 +- internal/datastore/src/letter-queue-repository.ts | 2 +- internal/datastore/src/letter-repository.ts | 5 ++--- .../src/services/__tests__/letter-operations.test.ts | 11 ++++------- .../api-handler/src/services/letter-operations.ts | 12 +++--------- 10 files changed, 31 insertions(+), 23 deletions(-) rename internal/datastore/src/{errors.ts => errors/letter-already-exists-error.ts} (100%) create mode 100644 internal/datastore/src/errors/letter-not-found-error.ts diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 812a8ca0e..5e2fe2011 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -27,6 +27,12 @@ - [ ] This PR is a result of pair or mob programming - [ ] If I have used the 'skip-trivy-package' label I have done so responsibly and in the knowledge that this is being fixed as part of a separate ticket/PR. +## DT3-Specific Checklist + + + +- [ ] If I have added a new resource (SQS, Lambda, Gateway, DDB table, etc), I have created the appropriate alarms + --- ## Sensitive Information Declaration diff --git a/internal/datastore/src/__test__/letter-queue-repository.test.ts b/internal/datastore/src/__test__/letter-queue-repository.test.ts index fdba8e81e..8904384cd 100644 --- a/internal/datastore/src/__test__/letter-queue-repository.test.ts +++ b/internal/datastore/src/__test__/letter-queue-repository.test.ts @@ -7,7 +7,7 @@ import { } from "./db"; import LetterQueueRepository from "../letter-queue-repository"; import { InsertPendingLetter } from "../types"; -import { LetterAlreadyExistsError } from "../errors"; +import { LetterAlreadyExistsError } from "../errors/letter-already-exists-error"; import { createTestLogger } from "./logs"; function createLetter(letterId = "letter1"): InsertPendingLetter { diff --git a/internal/datastore/src/__test__/letter-repository.test.ts b/internal/datastore/src/__test__/letter-repository.test.ts index 193c1c077..f68378dac 100644 --- a/internal/datastore/src/__test__/letter-repository.test.ts +++ b/internal/datastore/src/__test__/letter-repository.test.ts @@ -140,7 +140,7 @@ describe("LetterRepository", () => { await expect( letterRepository.getLetterById("supplier1", "letter1"), ).rejects.toThrow( - "Letter with id letter1 not found for supplier supplier1", + "Letter not found: supplierId=supplier1, letterId=letter1", ); }); diff --git a/internal/datastore/src/errors.ts b/internal/datastore/src/errors/letter-already-exists-error.ts similarity index 100% rename from internal/datastore/src/errors.ts rename to internal/datastore/src/errors/letter-already-exists-error.ts diff --git a/internal/datastore/src/errors/letter-not-found-error.ts b/internal/datastore/src/errors/letter-not-found-error.ts new file mode 100644 index 000000000..32af3092d --- /dev/null +++ b/internal/datastore/src/errors/letter-not-found-error.ts @@ -0,0 +1,12 @@ +/** + * Error thrown when a letter is not found in the database. + */ +export default class LetterNotFoundError extends Error { + constructor( + public readonly supplierId: string, + public readonly letterId: string, + ) { + super(`Letter not found: supplierId=${supplierId}, letterId=${letterId}`); + this.name = "LetterNotFoundError"; + } +} diff --git a/internal/datastore/src/index.ts b/internal/datastore/src/index.ts index 7ee912c23..854e8d131 100644 --- a/internal/datastore/src/index.ts +++ b/internal/datastore/src/index.ts @@ -1,5 +1,5 @@ export * from "./types"; -export * from "./errors"; +export * from "./errors/letter-already-exists-error"; export * from "./mi-repository"; export * from "./letter-repository"; export * from "./supplier-repository"; diff --git a/internal/datastore/src/letter-queue-repository.ts b/internal/datastore/src/letter-queue-repository.ts index 5e1da7dd4..facb377ab 100644 --- a/internal/datastore/src/letter-queue-repository.ts +++ b/internal/datastore/src/letter-queue-repository.ts @@ -5,7 +5,7 @@ import { PendingLetter, PendingLetterSchema, } from "./types"; -import { LetterAlreadyExistsError } from "./errors"; +import { LetterAlreadyExistsError } from "./errors/letter-already-exists-error"; type LetterQueueRepositoryConfig = { letterQueueTableName: string; diff --git a/internal/datastore/src/letter-repository.ts b/internal/datastore/src/letter-repository.ts index def7c1b36..35fc35803 100644 --- a/internal/datastore/src/letter-repository.ts +++ b/internal/datastore/src/letter-repository.ts @@ -18,6 +18,7 @@ import { LetterSchemaBase, UpdateLetter, } from "./types"; +import LetterNotFoundError from "./errors/letter-not-found-error"; export type PagingOptions = Partial<{ exclusiveStartKey: Record; @@ -117,9 +118,7 @@ export class LetterRepository { ); if (!result.Item) { - throw new Error( - `Letter with id ${letterId} not found for supplier ${supplierId}`, - ); + throw new LetterNotFoundError(supplierId, letterId); } return LetterSchema.parse(result.Item); } diff --git a/lambdas/api-handler/src/services/__tests__/letter-operations.test.ts b/lambdas/api-handler/src/services/__tests__/letter-operations.test.ts index 08e103ab0..bd1c42ae0 100644 --- a/lambdas/api-handler/src/services/__tests__/letter-operations.test.ts +++ b/lambdas/api-handler/src/services/__tests__/letter-operations.test.ts @@ -3,6 +3,7 @@ import pino from "pino"; import { getSignedUrl } from "@aws-sdk/s3-request-presigner"; import { S3Client } from "@aws-sdk/client-s3"; import { SQSClient } from "@aws-sdk/client-sqs"; +import LetterNotFoundError from "@internal/datastore/src/errors/letter-not-found-error"; import { enqueueLetterUpdateRequests, getLetterById, @@ -104,13 +105,11 @@ describe("getLetterById", () => { const mockRepo = { getLetterById: jest .fn() - .mockRejectedValue( - new Error("Letter with id l1 not found for supplier s1"), - ), + .mockRejectedValue(new LetterNotFoundError("supplier1", "letter1")), }; await expect( - getLetterById("supplierid", "letter1", mockRepo as any), + getLetterById("supplier1", "letter1", mockRepo as any), ).rejects.toThrow("No resource found with that ID"); }); @@ -171,9 +170,7 @@ describe("getLetterDataUrl function", () => { deps.letterRepo = { getLetterById: jest .fn() - .mockRejectedValue( - new Error("Letter with id l1 not found for supplier s1"), - ), + .mockRejectedValue(new LetterNotFoundError("supplier1", "letter42")), } as unknown as LetterRepository; await expect( diff --git a/lambdas/api-handler/src/services/letter-operations.ts b/lambdas/api-handler/src/services/letter-operations.ts index dc2de921d..e892dd27a 100644 --- a/lambdas/api-handler/src/services/letter-operations.ts +++ b/lambdas/api-handler/src/services/letter-operations.ts @@ -2,18 +2,12 @@ import { LetterBase, LetterRepository } from "@internal/datastore"; import { getSignedUrl } from "@aws-sdk/s3-request-presigner"; import { GetObjectCommand, S3Client } from "@aws-sdk/client-s3"; import { SendMessageBatchCommand } from "@aws-sdk/client-sqs"; +import LetterNotFoundError from "@internal/datastore/src/errors/letter-not-found-error"; import NotFoundError from "../errors/not-found-error"; import { UpdateLetterCommand } from "../contracts/letters"; import { ApiErrorDetail } from "../contracts/errors"; import { Deps } from "../config/deps"; -function isNotFoundError(error: any) { - return ( - error instanceof Error && - /^Letter with id \w+ not found for supplier \w+$/.test(error.message) - ); -} - async function getDownloadUrl( s3Uri: string, s3Client: S3Client, @@ -50,7 +44,7 @@ export const getLetterById = async ( try { letter = await letterRepo.getLetterById(supplierId, letterId); } catch (error) { - if (isNotFoundError(error)) { + if (error instanceof LetterNotFoundError) { throw new NotFoundError(ApiErrorDetail.NotFoundLetterId); } throw error; @@ -74,7 +68,7 @@ export const getLetterDataUrl = async ( deps.env.DOWNLOAD_URL_TTL_SECONDS, ); } catch (error) { - if (isNotFoundError(error)) { + if (error instanceof LetterNotFoundError) { throw new NotFoundError(ApiErrorDetail.NotFoundLetterId); } throw error; From 24b752b5c8d13a0952507ad7193789e13bbf206f Mon Sep 17 00:00:00 2001 From: Francisco Videira Date: Tue, 24 Mar 2026 15:33:28 +0000 Subject: [PATCH 2/8] fix tests --- .../apiGateway-tests/get-letter-pdf.spec.ts | 11 ++++----- .../get-letter-status.spec.ts | 23 +++---------------- 2 files changed, 7 insertions(+), 27 deletions(-) diff --git a/tests/component-tests/apiGateway-tests/get-letter-pdf.spec.ts b/tests/component-tests/apiGateway-tests/get-letter-pdf.spec.ts index 0e747589c..f9dafbdbb 100644 --- a/tests/component-tests/apiGateway-tests/get-letter-pdf.spec.ts +++ b/tests/component-tests/apiGateway-tests/get-letter-pdf.spec.ts @@ -7,10 +7,7 @@ import { SUPPLIER_LETTERS, } from "../../constants/api-constants"; import { createValidRequestHeaders } from "../../constants/request-headers"; -import { - error404ResponseBody, - error500ResponseBody, -} from "../../helpers/common-types"; +import { error404ResponseBody } from "../../helpers/common-types"; let baseUrl: string; @@ -84,7 +81,7 @@ test.describe("API Gateway Tests to Verify Get Letter PDF Endpoint", () => { }); // CCM-14318: Remove this test - test(`Get /letters/{id}/data returns 500 if letter is not found for supplierId ${SUPPLIERID}`, async ({ + test(`Get /letters/{id}/data returns 404 if letter is not found for supplierId ${SUPPLIERID}`, async ({ request, }) => { const id = "non-existing-id-12345"; @@ -98,7 +95,7 @@ test.describe("API Gateway Tests to Verify Get Letter PDF Endpoint", () => { ); const responseBody = await response.json(); - expect(response.status()).toBe(500); - expect(responseBody).toMatchObject(error500ResponseBody()); + expect(response.status()).toBe(404); + expect(responseBody).toMatchObject(error404ResponseBody()); }); }); diff --git a/tests/component-tests/apiGateway-tests/get-letter-status.spec.ts b/tests/component-tests/apiGateway-tests/get-letter-status.spec.ts index aa5d475ce..8622327aa 100644 --- a/tests/component-tests/apiGateway-tests/get-letter-status.spec.ts +++ b/tests/component-tests/apiGateway-tests/get-letter-status.spec.ts @@ -3,10 +3,7 @@ import getRestApiGatewayBaseUrl from "../../helpers/aws-gateway-helper"; import { getLettersBySupplier } from "../../helpers/generate-fetch-test-data"; import { SUPPLIERID, SUPPLIER_LETTERS } from "../../constants/api-constants"; import { createValidRequestHeaders } from "../../constants/request-headers"; -import { - error404ResponseBody, - error500ResponseBody, -} from "../../helpers/common-types"; +import { error404ResponseBody } from "../../helpers/common-types"; let baseUrl: string; @@ -49,10 +46,10 @@ test.describe("API Gateway Tests to Verify Get Letter Status Endpoint", () => { }); }); - test(`Get /letters/{id} returns 404 if no resource is found for id`, async ({ + test(`Get /letters/{id} returns 404 if letter is not found for supplierId ${SUPPLIERID}`, async ({ request, }) => { - const id = "11"; + const id = "non-existing-id-12345"; const headers = createValidRequestHeaders(); const response = await request.get(`${baseUrl}/${SUPPLIER_LETTERS}/${id}`, { headers, @@ -62,18 +59,4 @@ test.describe("API Gateway Tests to Verify Get Letter Status Endpoint", () => { expect(response.status()).toBe(404); expect(responseBody).toMatchObject(error404ResponseBody()); }); - - test(`Get /letters/{id} returns 500 if letter is not found for supplierId ${SUPPLIERID}`, async ({ - request, - }) => { - const id = "non-existing-id-12345"; - const headers = createValidRequestHeaders(); - const response = await request.get(`${baseUrl}/${SUPPLIER_LETTERS}/${id}`, { - headers, - }); - - const responseBody = await response.json(); - expect(response.status()).toBe(500); - expect(responseBody).toMatchObject(error500ResponseBody()); - }); }); From 8a9709711206e5b2affaf66251133a5026aa2d21 Mon Sep 17 00:00:00 2001 From: Francisco Videira Date: Tue, 24 Mar 2026 17:11:11 +0000 Subject: [PATCH 3/8] fix another test --- tests/component-tests/events-tests/event-subscription.spec.ts | 2 +- tests/e2e-tests/api/data/test_get_letter_data.py | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/component-tests/events-tests/event-subscription.spec.ts b/tests/component-tests/events-tests/event-subscription.spec.ts index 48f28fae8..067dbae8b 100644 --- a/tests/component-tests/events-tests/event-subscription.spec.ts +++ b/tests/component-tests/events-tests/event-subscription.spec.ts @@ -95,8 +95,8 @@ test.describe("Event Subscription SNS Tests", () => { headers, }, ); + expect(getLetterResponse.status()).toBe(404); - expect(getLetterResponse.status()).toBe(500); await pollUpsertLetterLogForError( "Message did not match an expected schema", domainId, diff --git a/tests/e2e-tests/api/data/test_get_letter_data.py b/tests/e2e-tests/api/data/test_get_letter_data.py index 658d078cb..4a3a19bd9 100644 --- a/tests/e2e-tests/api/data/test_get_letter_data.py +++ b/tests/e2e-tests/api/data/test_get_letter_data.py @@ -57,3 +57,5 @@ def test_500_letter_does_not_exist(url, bearer_token): ErrorHandler.handle_retry(get_message_response) assert get_message_response.status_code == 500 + +#TODO CCM-14318 probably From 2a202ed028ade73654f26677dce59f64589ed90a Mon Sep 17 00:00:00 2001 From: Francisco Videira Date: Wed, 25 Mar 2026 21:29:08 +0000 Subject: [PATCH 4/8] remove e2e test 500 not found --- tests/e2e-tests/api/data/test_get_letter_data.py | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/tests/e2e-tests/api/data/test_get_letter_data.py b/tests/e2e-tests/api/data/test_get_letter_data.py index 4a3a19bd9..56168f946 100644 --- a/tests/e2e-tests/api/data/test_get_letter_data.py +++ b/tests/e2e-tests/api/data/test_get_letter_data.py @@ -45,17 +45,3 @@ def test_404_letter_does_not_exist(url, bearer_token): ErrorHandler.handle_retry(get_message_response) assert get_message_response.status_code == 404 assert get_message_response.json().get("errors")[0].get("detail") == "No resource found with that ID" - -@pytest.mark.test -@pytest.mark.devtest -@pytest.mark.inttest -@pytest.mark.prodtest -def test_500_letter_does_not_exist(url, bearer_token): - letter_id = "00000000-0000-0000-0000-000000000000" - headers = Generators.generate_valid_headers(bearer_token.value) - get_message_response = requests.get(f"{url}/{LETTERS_ENDPOINT}/{letter_id}/data", headers=headers) - - ErrorHandler.handle_retry(get_message_response) - assert get_message_response.status_code == 500 - -#TODO CCM-14318 probably From 8cb683ec57215b2ac8461755ef4abedd0265bd63 Mon Sep 17 00:00:00 2001 From: Francisco Videira Date: Wed, 1 Apr 2026 14:25:32 +0000 Subject: [PATCH 5/8] fix import --- internal/datastore/src/__test__/letter-repository.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/datastore/src/__test__/letter-repository.test.ts b/internal/datastore/src/__test__/letter-repository.test.ts index c166d12e5..57a649476 100644 --- a/internal/datastore/src/__test__/letter-repository.test.ts +++ b/internal/datastore/src/__test__/letter-repository.test.ts @@ -9,7 +9,7 @@ import { import { LetterRepository } from "../letter-repository"; import { InsertLetter, Letter, UpdateLetter } from "../types"; import { LogStream, createTestLogger } from "./logs"; -import { LetterAlreadyExistsError } from "../letter-already-exists-error"; +import LetterAlreadyExistsError from "../errors/letter-already-exists-error"; function createLetter( supplierId: string, From 354c39d3a74b20ce50f8e199d29a212a96894455 Mon Sep 17 00:00:00 2001 From: Francisco Videira Date: Thu, 2 Apr 2026 09:23:21 +0000 Subject: [PATCH 6/8] try --- .github/PULL_REQUEST_TEMPLATE.md | 1 + scripts/config/markdownlint.yaml | 11 +++++++++++ 2 files changed, 12 insertions(+) create mode 100644 scripts/config/markdownlint.yaml diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 9fc08b3b3..01d3a928d 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -33,6 +33,7 @@ - [ ] If I have added a new resource (SQS, Lambda, Gateway, DDB table, etc), I have created the appropriate alarms + --- ## Sensitive Information Declaration diff --git a/scripts/config/markdownlint.yaml b/scripts/config/markdownlint.yaml new file mode 100644 index 000000000..554ab554b --- /dev/null +++ b/scripts/config/markdownlint.yaml @@ -0,0 +1,11 @@ +# SEE: https://github.com/DavidAnson/markdownlint/blob/main/schema/.markdownlint.yaml + +# https://github.com/DavidAnson/markdownlint/blob/main/doc/md013.md +MD013: false + +# https://github.com/DavidAnson/markdownlint/blob/main/doc/md024.md +MD024: + siblings_only: true + +# https://github.com/DavidAnson/markdownlint/blob/main/doc/md033.md +MD033: false From 5137a535996d8950da9d660a0e372f9418a62b45 Mon Sep 17 00:00:00 2001 From: Francisco Videira Date: Thu, 2 Apr 2026 09:30:16 +0000 Subject: [PATCH 7/8] fix --- .github/PULL_REQUEST_TEMPLATE.md | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 01d3a928d..9fc08b3b3 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -33,7 +33,6 @@ - [ ] If I have added a new resource (SQS, Lambda, Gateway, DDB table, etc), I have created the appropriate alarms - --- ## Sensitive Information Declaration From b9ed951df581e17a97dfbd1346385d143a1f54c3 Mon Sep 17 00:00:00 2001 From: Francisco Videira Date: Thu, 2 Apr 2026 09:49:47 +0000 Subject: [PATCH 8/8] remove regex from vale accepted --- scripts/config/vale/styles/config/vocabularies/words/accept.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/scripts/config/vale/styles/config/vocabularies/words/accept.txt b/scripts/config/vale/styles/config/vocabularies/words/accept.txt index ead5f5a48..3ea2ec995 100644 --- a/scripts/config/vale/styles/config/vocabularies/words/accept.txt +++ b/scripts/config/vale/styles/config/vocabularies/words/accept.txt @@ -1,6 +1,5 @@ actioned API|api -[A-Z]+s Bitwarden bot Cognito