Conversation
infrastructure/terraform/modules/backend-api/dynamodb_table_proof_requests.tf
Outdated
Show resolved
Hide resolved
infrastructure/terraform/modules/backend-api/module_sqs_proof_requests_table_events_pipe_dlq.tf
Outdated
Show resolved
Hide resolved
| }, | ||
| "additionalProperties": { | ||
| "type": "string" | ||
| } |
There was a problem hiding this comment.
taking these out aren't we?
There was a problem hiding this comment.
After digging a bit more to understand this because I've not explicitly added these fields.
They are generated because we are defining personalisation as a generic record as z.record(z.string(), z.string(). The same can be seen for /schemas/TemplateDrafted/v1.json.supplierReferences.
While confusing the JSON schema is correct. I'll mention this on the refcom.
There was a problem hiding this comment.
oh so you mean they get added as part of the auto generating?
| } | ||
| }, | ||
| "description": "Contact details to send the proof to" | ||
| } |
| }, | ||
| "additionalProperties": { | ||
| "type": "string" | ||
| } |
| }, | ||
| "email": { | ||
| "type": "string" | ||
| } |
| }, | ||
| "additionalProperties": { | ||
| "type": "string" | ||
| } |
| "properties": { | ||
| "sms": { | ||
| "type": "string" | ||
| }, |
| templateType: z.literal('SMS').meta({ | ||
| description: 'Template type for the template being proofed', | ||
| }), | ||
| contactDetails: $ProofContactDetails.extend({ |
There was a problem hiding this comment.
but doesnt this keep email as optional?
| templateType: z.literal('NHS_APP').meta({ | ||
| description: 'Template type for the template being proofed', | ||
| }), | ||
| contactDetails: $ProofContactDetails.optional(), |
| }), | ||
| }); | ||
|
|
||
| const $ProofContactDetails = z |
There was a problem hiding this comment.
not sure if this is valuable like this is it? there's no scenario where we'd ever expect both, so should this be one or the other? or do we even need it?
| templateType: z.literal('EMAIL').meta({ | ||
| description: 'Template type for the template being proofed', | ||
| }), | ||
| contactDetails: $ProofContactDetails.extend({ |
| await proofRequestsStorageHelper.seed([ | ||
| { | ||
| id: proofRequestId, | ||
| owner: `CLIENT#${user.clientId}`, |
There was a problem hiding this comment.
should this still be here?
|
|
||
| export type DigitalProofRequest = { | ||
| id: string; | ||
| owner: string; |
There was a problem hiding this comment.
This is a type for the DB record. Client ownership on this seems useful?
It doesn't seem unseful on the event because notify core int environment only has 1 client.
| /** | ||
| * Breaks a list into chunks of upto 25 items | ||
| */ | ||
| private static chunk<T>(list: T[], size = 25): T[][] { |
There was a problem hiding this comment.
in case you haven't seen, Michael's PR has split this out into its own file, maybe could align the approach across both PRs so we don't end up with duplicates?
| } | ||
| ); | ||
|
|
||
| it('fails when SMS proof request does not include contactDetails.sms', () => { |
There was a problem hiding this comment.
what about the inverse?
fails when SMS proof contains email? email proof contains sms?
apap contains either? dont we expect those to fail too?
4 participants
Description
Context
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.