diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 5a5026ee5..9fc08b3b3 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 +## 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 629d8aefd..685cb5dc1 100644 --- a/internal/datastore/src/__test__/letter-queue-repository.test.ts +++ b/internal/datastore/src/__test__/letter-queue-repository.test.ts @@ -8,9 +8,9 @@ import { } from "./db"; import LetterQueueRepository from "../letter-queue-repository"; import { InsertPendingLetter } from "../types"; -import { LetterAlreadyExistsError } from "../letter-already-exists-error"; +import LetterAlreadyExistsError from "../errors/letter-already-exists-error"; import { createTestLogger } from "./logs"; -import { LetterDoesNotExistError } from "../letter-does-not-exist-error"; +import LetterNotFoundError from "../errors/letter-not-found-error"; function createLetter( overrides: Partial = {}, @@ -132,7 +132,7 @@ describe("LetterQueueRepository", () => { it("throws an error when the letter does not exist", async () => { await expect( letterQueueRepository.deleteLetter("supplier1", "letter1"), - ).rejects.toThrow(LetterDoesNotExistError); + ).rejects.toThrow(LetterNotFoundError); }); it("rethrows errors from DynamoDB when deleting a letter", async () => { diff --git a/internal/datastore/src/__test__/letter-repository.test.ts b/internal/datastore/src/__test__/letter-repository.test.ts index dddd08879..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, @@ -142,7 +142,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/letter-already-exists-error.ts b/internal/datastore/src/errors/letter-already-exists-error.ts similarity index 75% rename from internal/datastore/src/letter-already-exists-error.ts rename to internal/datastore/src/errors/letter-already-exists-error.ts index 72b3d21b9..55b55d911 100644 --- a/internal/datastore/src/letter-already-exists-error.ts +++ b/internal/datastore/src/errors/letter-already-exists-error.ts @@ -1,8 +1,7 @@ /** * Error thrown when attempting to create a letter that already exists in the database. */ -// eslint-disable-next-line import-x/prefer-default-export -export class LetterAlreadyExistsError extends Error { +export default class LetterAlreadyExistsError extends Error { constructor( public readonly supplierId: string, public readonly letterId: string, 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 cd9464155..9b656d9ee 100644 --- a/internal/datastore/src/index.ts +++ b/internal/datastore/src/index.ts @@ -1,9 +1,9 @@ export * from "./types"; -export * from "./letter-already-exists-error"; -export * from "./letter-does-not-exist-error"; export * from "./mi-repository"; export * from "./letter-repository"; export * from "./supplier-repository"; export * from "./supplier-config-repository"; export { default as LetterQueueRepository } from "./letter-queue-repository"; export { default as DBHealthcheck } from "./healthcheck"; +export { default as LetterAlreadyExistsError } from "./errors/letter-already-exists-error"; +export { default as LetterNotFoundError } from "./errors/letter-not-found-error"; diff --git a/internal/datastore/src/letter-does-not-exist-error.ts b/internal/datastore/src/letter-does-not-exist-error.ts deleted file mode 100644 index ab5410b91..000000000 --- a/internal/datastore/src/letter-does-not-exist-error.ts +++ /dev/null @@ -1,15 +0,0 @@ -/** - * Error thrown when attempting to delete a letter that does not exist in the database. - */ -// eslint-disable-next-line import-x/prefer-default-export -export class LetterDoesNotExistError extends Error { - constructor( - public readonly supplierId: string, - public readonly letterId: string, - ) { - super( - `Letter does not exist: supplierId=${supplierId}, letterId=${letterId}`, - ); - this.name = "LetterDoesNotExistError"; - } -} diff --git a/internal/datastore/src/letter-queue-repository.ts b/internal/datastore/src/letter-queue-repository.ts index e30eadcfc..fa00358b7 100644 --- a/internal/datastore/src/letter-queue-repository.ts +++ b/internal/datastore/src/letter-queue-repository.ts @@ -9,8 +9,8 @@ import { PendingLetter, PendingLetterSchema, } from "./types"; -import { LetterAlreadyExistsError } from "./letter-already-exists-error"; -import { LetterDoesNotExistError } from "./letter-does-not-exist-error"; +import LetterAlreadyExistsError from "./errors/letter-already-exists-error"; +import LetterNotFoundError from "./errors/letter-not-found-error"; type LetterQueueRepositoryConfig = { letterQueueTableName: string; @@ -81,7 +81,7 @@ export default class LetterQueueRepository { error instanceof Error && error.name === "ConditionalCheckFailedException" ) { - throw new LetterDoesNotExistError(supplierId, letterId); + throw new LetterNotFoundError(supplierId, letterId); } throw error; } diff --git a/internal/datastore/src/letter-repository.ts b/internal/datastore/src/letter-repository.ts index 72b9be486..f22641f84 100644 --- a/internal/datastore/src/letter-repository.ts +++ b/internal/datastore/src/letter-repository.ts @@ -18,7 +18,8 @@ import { LetterSchemaBase, UpdateLetter, } from "./types"; -import { LetterAlreadyExistsError } from "./letter-already-exists-error"; +import LetterNotFoundError from "./errors/letter-not-found-error"; +import LetterAlreadyExistsError from "./errors/letter-already-exists-error"; export type PagingOptions = Partial<{ exclusiveStartKey: Record; @@ -116,9 +117,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 be69387e1..e4d6c487b 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, @@ -105,13 +106,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"); }); @@ -172,9 +171,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; diff --git a/lambdas/update-letter-queue/src/__tests__/update-letter-queue.test.ts b/lambdas/update-letter-queue/src/__tests__/update-letter-queue.test.ts index 954fbaa4f..d8bbbc193 100644 --- a/lambdas/update-letter-queue/src/__tests__/update-letter-queue.test.ts +++ b/lambdas/update-letter-queue/src/__tests__/update-letter-queue.test.ts @@ -1,7 +1,7 @@ import { Letter, LetterAlreadyExistsError, - LetterDoesNotExistError, + LetterNotFoundError, LetterQueueRepository, } from "@internal/datastore"; import { mockDeep } from "jest-mock-extended"; @@ -192,7 +192,7 @@ describe("update-letter-queue Lambda", () => { const newLetter1 = generateLetter("ACCEPTED", { id: "1" }); const newLetter2 = generateLetter("ACCEPTED", { id: "2" }); (mockedDeps.letterQueueRepository.deleteLetter as jest.Mock) - .mockRejectedValueOnce(new LetterDoesNotExistError("supplier1", "1")) + .mockRejectedValueOnce(new LetterNotFoundError("supplier1", "1")) .mockResolvedValueOnce({}); const testData = generateKinesisEvent([ @@ -338,7 +338,7 @@ describe("update-letter-queue Lambda", () => { const newLetter1 = generateLetter("ACCEPTED", { id: "1" }); const newLetter2 = generateLetter("ACCEPTED", { id: "2" }); (mockedDeps.letterQueueRepository.deleteLetter as jest.Mock) - .mockRejectedValueOnce(new LetterDoesNotExistError("supplier1", "1")) + .mockRejectedValueOnce(new LetterNotFoundError("supplier1", "1")) .mockResolvedValueOnce({}); const testData = generateKinesisEvent([ diff --git a/lambdas/update-letter-queue/src/update-letter-queue.ts b/lambdas/update-letter-queue/src/update-letter-queue.ts index 6a0b7aa9e..04ed963ff 100644 --- a/lambdas/update-letter-queue/src/update-letter-queue.ts +++ b/lambdas/update-letter-queue/src/update-letter-queue.ts @@ -11,7 +11,7 @@ import { InsertPendingLetter, Letter, LetterAlreadyExistsError, - LetterDoesNotExistError, + LetterNotFoundError, LetterSchema, } from "@internal/datastore"; import { Deps } from "./deps"; @@ -105,7 +105,7 @@ async function deletePendingLetterFromQueue( await deps.letterQueueRepository.deleteLetter(letter.supplierId, letter.id); return 1; } catch (error) { - if (error instanceof LetterDoesNotExistError) { + if (error instanceof LetterNotFoundError) { deps.logger.warn({ description: "Letter does not exist", supplierId: letter.supplierId, 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 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 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()); - }); }); diff --git a/tests/component-tests/events-tests/event-subscription.spec.ts b/tests/component-tests/events-tests/event-subscription.spec.ts index 3c972be91..4d6044f60 100644 --- a/tests/component-tests/events-tests/event-subscription.spec.ts +++ b/tests/component-tests/events-tests/event-subscription.spec.ts @@ -96,8 +96,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..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,15 +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