Skip to content

Commit 05606d5

Browse files
francisco-videira-nhsfrancisco-videira-nhs
authored
CCM-11188: Patch letter endpoint (#173)
* Change env var name * fix env var name * Refactor contracts and mappers * Fix invalid error message * Clean up updateLetterStatus * patchLetters to patchLetter * Fix sonar issues * Read headers in lower case
1 parent 32f9464 commit 05606d5

File tree

30 files changed

+356
-264
lines changed

30 files changed

+356
-264
lines changed

‎infrastructure/terraform/components/api/README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ No requirements.
3737
| <a name="module_get_letters"></a> [get\_letters](#module\_get\_letters) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.20/terraform-lambda.zip | n/a |
3838
| <a name="module_kms"></a> [kms](#module\_kms) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.20/terraform-kms.zip | n/a |
3939
| <a name="module_logging_bucket"></a> [logging\_bucket](#module\_logging\_bucket) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.20/terraform-s3bucket.zip | n/a |
40-
| <a name="module_patch_letters"></a> [patch\_letters](#module\_patch\_letters) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.20/terraform-lambda.zip | n/a |
40+
| <a name="module_patch_letter"></a> [patch\_letter](#module\_patch\_letter) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.20/terraform-lambda.zip | n/a |
4141
| <a name="module_s3bucket_test_letters"></a> [s3bucket\_test\_letters](#module\_s3bucket\_test\_letters) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.20/terraform-s3bucket.zip | n/a |
4242
| <a name="module_supplier_ssl"></a> [supplier\_ssl](#module\_supplier\_ssl) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.20/terraform-ssl.zip | n/a |
4343
## Outputs

‎infrastructure/terraform/components/api/iam_role_api_gateway_execution_role.tf‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ data "aws_iam_policy_document" "api_gateway_execution_policy" {
5050
resources = [
5151
module.authorizer_lambda.function_arn,
5252
module.get_letters.function_arn,
53-
module.patch_letters.function_arn
53+
module.patch_letter.function_arn
5454
]
5555
}
5656
}

‎infrastructure/terraform/components/api/locals.tf‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,15 +9,15 @@ locals {
99
AWS_REGION = var.region
1010
AUTHORIZER_LAMBDA_ARN = module.authorizer_lambda.function_arn
1111
GET_LETTERS_LAMBDA_ARN = module.get_letters.function_arn
12-
PATCH_LETTERS_LAMBDA_ARN = module.patch_letters.function_arn
12+
PATCH_LETTER_LAMBDA_ARN = module.patch_letter.function_arn
1313
})
1414

1515
destination_arn = "arn:aws:logs:${var.region}:${var.shared_infra_account_id}:destination:nhs-main-obs-firehose-logs"
1616

17-
common_db_access_lambda_env_vars = {
17+
common_lambda_env_vars = {
1818
LETTERS_TABLE_NAME = aws_dynamodb_table.letters.name,
1919
LETTER_TTL_HOURS = 24,
2020
SUPPLIER_ID_HEADER = "nhsd-supplier-id"
21-
SUPPLIER_ID_HEADER = "nhsd-correlation-id"
21+
APIM_CORRELATION_HEADER = "nhsd-correlation-id"
2222
}
2323
}

‎infrastructure/terraform/components/api/module_lambda_get_letters.tf‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ module "get_letters" {
3535
log_destination_arn = local.destination_arn
3636
log_subscription_role_arn = local.acct.log_subscription_role_arn
3737

38-
lambda_env_vars = merge(local.common_db_access_lambda_env_vars, {
38+
lambda_env_vars = merge(local.common_lambda_env_vars, {
3939
MAX_LIMIT = var.max_get_limit
4040
})
4141
}

infrastructure/terraform/components/api/module_lambda_patch_letters.tf renamed to infrastructure/terraform/components/api/module_lambda_patch_letter.tf

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1-
module "patch_letters" {
1+
module "patch_letter" {
22
source = "https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.20/terraform-lambda.zip"
33

4-
function_name = "patch_letters"
4+
function_name = "patch_letter"
55
description = "Update the status of a letter"
66

77
aws_account_id = var.aws_account_id
@@ -15,14 +15,14 @@ module "patch_letters" {
1515
kms_key_arn = module.kms.key_arn
1616

1717
iam_policy_document = {
18-
body = data.aws_iam_policy_document.patch_letters_lambda.json
18+
body = data.aws_iam_policy_document.patch_letter_lambda.json
1919
}
2020

2121
function_s3_bucket = local.acct.s3_buckets["lambda_function_artefacts"]["id"]
2222
function_code_base_path = local.aws_lambda_functions_dir_path
2323
function_code_dir = "api-handler/dist"
2424
function_include_common = true
25-
handler_function_name = "patchLetters"
25+
handler_function_name = "patchLetter"
2626
runtime = "nodejs22.x"
2727
memory = 128
2828
timeout = 5
@@ -35,10 +35,10 @@ module "patch_letters" {
3535
log_destination_arn = local.destination_arn
3636
log_subscription_role_arn = local.acct.log_subscription_role_arn
3737

38-
lambda_env_vars = merge(local.common_db_access_lambda_env_vars, {})
38+
lambda_env_vars = merge(local.common_lambda_env_vars, {})
3939
}
4040

41-
data "aws_iam_policy_document" "patch_letters_lambda" {
41+
data "aws_iam_policy_document" "patch_letter_lambda" {
4242
statement {
4343
sid = "KMSPermissions"
4444
effect = "Allow"

‎infrastructure/terraform/components/api/resources/spec.tmpl.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@
6767
],
6868
"patch": {
6969
"description": "Update the status of a letter by providing the new status in the request body.",
70-
"operationId": "patchLetters",
70+
"operationId": "patchLetter",
7171
"requestBody": {
7272
"required": true
7373
},
@@ -102,7 +102,7 @@
102102
},
103103
"timeoutInMillis": 29000,
104104
"type": "AWS_PROXY",
105-
"uri": "arn:aws:apigateway:${AWS_REGION}:lambda:path/2015-03-31/functions/${PATCH_LETTERS_LAMBDA_ARN}/invocations"
105+
"uri": "arn:aws:apigateway:${AWS_REGION}:lambda:path/2015-03-31/functions/${PATCH_LETTER_LAMBDA_ARN}/invocations"
106106
}
107107
}
108108
}

‎internal/datastore/src/__test__/letter-repository.test.ts‎

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { Letter } from '../types';
44
import { Logger } from 'pino';
55
import { createTestLogger, LogStream } from './logs';
66
import { PutCommand } from '@aws-sdk/lib-dynamodb';
7+
import { LetterDto } from '../../../../lambdas/api-handler/src/contracts/letters';
78

89
function createLetter(supplierId: string, letterId: string, status: Letter['status'] = 'PENDING'): Omit<Letter, 'ttl' | 'supplierStatus' | 'supplierStatusSk'> {
910
return {
@@ -106,7 +107,14 @@ describe('LetterRepository', () => {
106107
await letterRepository.putLetter(letter);
107108
await checkLetterStatus('supplier1', 'letter1', 'PENDING');
108109

109-
await letterRepository.updateLetterStatus('supplier1', 'letter1', 'REJECTED', 1, "Reason text");
110+
const letterDto: LetterDto = {
111+
id: 'letter1',
112+
supplierId: 'supplier1',
113+
status: 'REJECTED',
114+
reasonCode: 1,
115+
reasonText: 'Reason text'
116+
};
117+
await letterRepository.updateLetterStatus(letterDto);
110118

111119
const updatedLetter = await letterRepository.getLetterById('supplier1', 'letter1');
112120
expect(updatedLetter.status).toBe('REJECTED');
@@ -124,13 +132,25 @@ describe('LetterRepository', () => {
124132
// Month is zero-indexed in JavaScript Date
125133
// Day is one-indexed
126134
jest.setSystemTime(new Date(2020, 1, 2));
127-
await letterRepository.updateLetterStatus('supplier1', 'letter1', 'DELIVERED', undefined, undefined);
135+
const letterDto: LetterDto = {
136+
id: 'letter1',
137+
supplierId: 'supplier1',
138+
status: 'DELIVERED'
139+
};
140+
141+
await letterRepository.updateLetterStatus(letterDto);
128142
const updatedLetter = await letterRepository.getLetterById('supplier1', 'letter1');
143+
129144
expect(updatedLetter.updatedAt).toBe('2020-02-02T00:00:00.000Z');
130145
});
131146

132147
test('can\'t update a letter that does not exist', async () => {
133-
await expect(letterRepository.updateLetterStatus('supplier1', 'letter1', 'DELIVERED', undefined, undefined))
148+
const letterDto: LetterDto = {
149+
id: 'letter1',
150+
supplierId: 'supplier1',
151+
status: 'DELIVERED'
152+
};
153+
await expect(letterRepository.updateLetterStatus(letterDto))
134154
.rejects.toThrow('Letter with id letter1 not found for supplier supplier1');
135155
});
136156

@@ -139,7 +159,13 @@ describe('LetterRepository', () => {
139159
...db.config,
140160
lettersTableName: 'nonexistent-table'
141161
});
142-
await expect(misconfiguredRepository.updateLetterStatus('supplier1', 'letter1', 'DELIVERED', undefined, undefined))
162+
163+
const letterDto: LetterDto = {
164+
id: 'letter1',
165+
supplierId: 'supplier1',
166+
status: 'DELIVERED'
167+
};
168+
await expect(misconfiguredRepository.updateLetterStatus(letterDto))
143169
.rejects.toThrow('Cannot do operations on a non-existent table');
144170
});
145171

@@ -164,7 +190,12 @@ describe('LetterRepository', () => {
164190
const pendingLetters = await letterRepository.getLettersByStatus('supplier1', 'PENDING');
165191
expect(pendingLetters.letters).toHaveLength(2);
166192

167-
await letterRepository.updateLetterStatus('supplier1', 'letter1', 'DELIVERED', undefined, undefined);
193+
const letterDto: LetterDto = {
194+
id: 'letter1',
195+
supplierId: 'supplier1',
196+
status: 'DELIVERED'
197+
};
198+
await letterRepository.updateLetterStatus(letterDto);
168199
const remainingLetters = await letterRepository.getLettersByStatus('supplier1', 'PENDING');
169200
expect(remainingLetters.letters).toHaveLength(1);
170201
expect(remainingLetters.letters[0].id).toBe('letter2');

‎internal/datastore/src/letter-repository.ts‎

Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import {
1010
import { Letter, LetterBase, LetterSchema, LetterSchemaBase } from './types';
1111
import { Logger } from 'pino';
1212
import { z } from 'zod';
13+
import { LetterDto } from '../../../lambdas/api-handler/src/contracts/letters';
1314

1415
export type PagingOptions = Partial<{
1516
exclusiveStartKey: Record<string, any>,
@@ -133,37 +134,35 @@ export class LetterRepository {
133134
}
134135
}
135136

136-
async updateLetterStatus(supplierId: string, letterId: string, status: Letter['status'], reasonCode: number | undefined, reasonText: string | undefined): Promise<Letter> {
137-
this.log.debug(`Updating letter ${letterId} to status ${status}`);
137+
async updateLetterStatus(letterToUpdate: LetterDto): Promise<Letter> {
138+
this.log.debug(`Updating letter ${letterToUpdate.id} to status ${letterToUpdate.status}`);
138139
let result: UpdateCommandOutput;
139140
try {
140141
let updateExpression = 'set #status = :status, updatedAt = :updatedAt, supplierStatus = :supplierStatus, #ttl = :ttl';
141-
let expressionAttributeValues = {
142-
':status': status,
143-
':updatedAt': new Date().toISOString(),
144-
':supplierStatus': `${supplierId}#${status}`,
145-
':ttl': Math.floor(Date.now() / 1000 + 60 * 60 * this.config.ttlHours),
146-
...(!reasonCode && {':reasonCode': reasonCode}),
147-
...(!reasonText && {':reasonText': reasonText})
148-
};
149-
150-
if (reasonCode)
142+
let expressionAttributeValues : Record<string, any> = {
143+
':status': letterToUpdate.status,
144+
':updatedAt': new Date().toISOString(),
145+
':supplierStatus': `${letterToUpdate.supplierId}#${letterToUpdate.status}`,
146+
':ttl': Math.floor(Date.now() / 1000 + 60 * 60 * this.config.ttlHours)
147+
};
148+
149+
if (letterToUpdate.reasonCode)
151150
{
152151
updateExpression += ', reasonCode = :reasonCode';
153-
expressionAttributeValues[':reasonCode'] = reasonCode;
152+
expressionAttributeValues[':reasonCode'] = letterToUpdate.reasonCode;
154153
}
155154

156-
if (reasonText)
155+
if (letterToUpdate.reasonText)
157156
{
158157
updateExpression += ', reasonText = :reasonText';
159-
expressionAttributeValues[':reasonText'] = reasonText;
158+
expressionAttributeValues[':reasonText'] = letterToUpdate.reasonText;
160159
}
161160

162161
result = await this.ddbClient.send(new UpdateCommand({
163162
TableName: this.config.lettersTableName,
164163
Key: {
165-
supplierId: supplierId,
166-
id: letterId
164+
supplierId: letterToUpdate.supplierId,
165+
id: letterToUpdate.id
167166
},
168167
UpdateExpression: updateExpression,
169168
ConditionExpression: 'attribute_exists(id)', // Ensure letter exists
@@ -176,12 +175,12 @@ export class LetterRepository {
176175
}));
177176
} catch (error) {
178177
if (error instanceof Error && error.name === 'ConditionalCheckFailedException') {
179-
throw new Error(`Letter with id ${letterId} not found for supplier ${supplierId}`);
178+
throw new Error(`Letter with id ${letterToUpdate.id} not found for supplier ${letterToUpdate.supplierId}`);
180179
}
181180
throw error;
182181
}
183182

184-
this.log.debug(`Updated letter ${letterId} to status ${status}`);
183+
this.log.debug(`Updated letter ${letterToUpdate.id} to status ${letterToUpdate.status}`);
185184
return LetterSchema.parse(result.Attributes);
186185
}
187186

‎lambdas/api-handler/src/contracts/json-api.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
import { z } from 'zod';
2+
3+
// Single document wrapper
4+
export const makeDocumentSchema = <T extends z.ZodTypeAny>(resourceSchema: T) =>
5+
z.object({ data: resourceSchema }).strict();
6+
7+
// Collection document wrapper
8+
export const makeCollectionSchema = <T extends z.ZodTypeAny>(resourceSchema: T) =>
9+
z.object({ data: z.array(resourceSchema) }).strict();

‎lambdas/api-handler/src/contracts/letter-api.ts‎

Lines changed: 0 additions & 48 deletions
This file was deleted.

0 commit comments

Comments
 (0)