CCM-8354: NHS App test message details#945
Conversation
…emplate-management into feature/CCM-8354-nhs-app-digital-proofing
nicki-nhs
changed the title
… to template types
nicki-nhs
changed the title
| @@ -0,0 +1,128 @@ | |||
| import SendTestNhsAppMessagePage, { | |||
There was a problem hiding this comment.
last time we discussed as a team i thought we'd decided that having two separate approaches to where to put tests in the file structure was confusing and not what we wanted to do, did something change?
There was a problem hiding this comment.
the ticket to move them to be adjacent to the code is lined up in the backlog, and will be coming soon, so it made sense to me to do it like that for new files so we've got less to move.
but happy to move to the tests dir for now if you feel strongly - but it'll all get moved soon anyway :)
There was a problem hiding this comment.
if we're definitely doing the change to all the tests (i remain against it) then fine this makes sense
| expect(submitButton).toHaveAttribute('type', 'submit'); | ||
| }); | ||
|
|
||
| it('uses default serverAction passthrough when no serverAction is provided', async () => { |
There was a problem hiding this comment.
why would this be a case we want to support?
There was a problem hiding this comment.
test coverage complaint :D
There was a problem hiding this comment.
i mean why does serverAction need to be optional at all on SendTestMessage.tsx
| ).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should render children when provided', () => { |
There was a problem hiding this comment.
we don't appear to ever be using children in for TestPersonlisationForm in the app code
There was a problem hiding this comment.
interesting, I cant remember why I had this.... :D will check, maybe it was for a future ticket...
There was a problem hiding this comment.
ah I've remembered I think
I was originally planning on putting the "Personalisation" heading + description in as children (not in the version we want for letters)
but then ended up doing this instead in SendTestMessage
<section data-testid='personalisation-form-section'>
<h2 className='nhsuk-heading-m nhsuk-u-margin-top-6'>
{content.personalisationSection.heading}
</h2>
<p className='nhsuk-body'>
{content.personalisationSection.description[templateType]}
</p>
<TestPersonalisationForm
templateType={templateType}
recipientField={recipientField}
customFields={customFields}
/>
</section>
but we could
move
<section data-testid='personalisation-form-section'> to the root of TestPersonalisationForm
and then
do this in SendTestMessage
<TestPersonalisationForm
templateType={templateType}
recipientField={recipientField}
customFields={customFields}
>
<h2 className='nhsuk-heading-m nhsuk-u-margin-top-6'>
{content.personalisationSection.heading}
</h2>
<p className='nhsuk-body'>
{content.personalisationSection.description[templateType]}
</p>
</TestPersonalisationForm>
what do you think?
There was a problem hiding this comment.
don't mind either way, but i think children should either be used or removed as an option
…emplate-management into feature/CCM-8354-nhs-app-digital-proofing
| classifyPersonalisation([ | ||
| 'firstName', | ||
| 'clinicName', | ||
| 'date', |
There was a problem hiding this comment.
nitpick but 'date' is actually banned in digital channel personsalisation, so it might be confusing if it appears in test data in a couple of places
| digitalProofingDisabledUser: TestUser | ||
| ) { | ||
| return { | ||
| validNhsAppTemplate: { |
There was a problem hiding this comment.
add one to cover SUBMITTED status?
| return this; | ||
| } | ||
|
|
||
| setCustomPersonalisation(custom: string[]) { |
There was a problem hiding this comment.
For letters we also store systemPersonalisation (there is an existing update builder method called setPersonalisation, an empty array is set if there is no custom/system). We're not reading systemPersonalisation but I think it would be better to keep digital templates aligned if possible
There was a problem hiding this comment.
I had that at first but @chris-elliott-nhsd thought we should just add a separate method to just use custom since that's what all we need for digital - you guys can argue amongst yourselves about it cos I dont feel strongly either way :D
| ); | ||
|
|
||
| export type EmailProperties = { | ||
| customPersonalisation?: Array<string>; |
There was a problem hiding this comment.
These aren't type specific, they belong on BaseCreatedTemplate
There was a problem hiding this comment.
Actually I'm forgetting about PDF letters, which don't have it - but still might be better to just have it optional on all templates
|
|
||
| export const $EmailProperties = schemaFor<EmailProperties>()( | ||
| z.object({ | ||
| customPersonalisation: z.array(z.string()).optional(), |
There was a problem hiding this comment.
I think these could go in $BaseTemplateDto
| .setUpdatedByUserAt(this.internalUserKey(user)) | ||
| .incrementLockNumber(); | ||
|
|
||
| if (customPersonalisation?.length > 0) { |
There was a problem hiding this comment.
we set empty array for letters if there is no custom personalisation, so I think this should match
| mode = 'block', | ||
| overrides, | ||
| }: MarkdownContentProps) { | ||
| const isPlainObject = (value: unknown): value is Record<string, unknown> => |
There was a problem hiding this comment.
This doesn't need to be defined inside a component, it's more efficient in module scope
| return [...new Set(parameters)]; | ||
| } | ||
|
|
||
| export function classifyPersonalisation(parameters: string[]): { |
There was a problem hiding this comment.
The Personalisation type in letter-preview-renderer could move to utils/src/types
| }; | ||
| } | ||
|
|
||
| const { templateId, testNhsNumber } = result.data; |
There was a problem hiding this comment.
Could we make this testPatientNhsNumber to match the event schema?
|
|
||
| const personalisation = { | ||
| ...customPersonalisation, | ||
| nhsNumber: testNhsNumber, |
There was a problem hiding this comment.
NHS number shouldn't go in personalisation, we get it from PDS in core - since in rare cases it can be changed
Requests where personalisation contains nhsNumber will be rejected: https://github.com/NHSDigital/comms-mgr/blob/120f59bef4bb6d4e1c9df81fbae84b35f0badd27/packages/libs/utils/src/constants.ts#L20
3 participants
Description
send-test-nhs-app-messagepageSendTestMessagecomponent which will be used for the other digital channelsTestPersonalisationFormandCustomFieldsFormcomponentsMarkdownContentto merge overrides on linkscustomPersonalisationto schema for digital templatesContext
Allows users to enter the details to request digital proofs of their NHS App templates
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.