CCM-12038: preview production message plan #792
Conversation
harrim91
changed the title
| }, | ||
| ctas: { | ||
| primary: { | ||
| href: '/message-plans/get-ready-to-move/{{routingConfigId}}', |
There was a problem hiding this comment.
I put most of the choose templates stuff into page.tsx which is server rendered. The CTA now uses a server action to do the validation / redirect, rather than having a link with an event listener / e.preventDefault etc.
There was a problem hiding this comment.
it should still have the href tho for accessibility purposes surely? so its clear from the markup that the button will take them to a different page, even if the href isnt going to be responsible for the navigation
There was a problem hiding this comment.
I don't know, I thought that this was better for accessibility. With the link/href, you'd expect clicking that to navigate you somewhere. But it actually has some client-side pseudo form validation, and if the validation failed, then it didn't navigate and you get a form error. It felt like a form masquerading as a link.
This way, it's a form with a submit button. Its not uncommon for submitting a form to navigate you somewhere different - probably most of our forms do it - and it's not clear where you're going to end up because none of the form submit buttons have an href attribute. So I figured this is in line with how we do forms elsewhere. The difference I guess is that the form doesn't do anything in the sense of performing a backend action - but it does do form validation, it can return form error state. So it felt right to have it as a form.
| <FormContext.Provider value={[state, action, isPending]}> | ||
| <NhsNotifyErrorSummary errorState={state.errorState} /> | ||
| <NhsNotifyErrorSummary | ||
| hint={errorSummaryHint} |
There was a problem hiding this comment.
how was this working before?
There was a problem hiding this comment.
There wasn't an instance of the FormProvider being used to render an error summary with a hint.
| renderSMSMarkdown, | ||
| } from './markdownit'; | ||
|
|
||
| export function renderTemplateMarkdown(template: TemplateDto): string { |
There was a problem hiding this comment.
| export function renderTemplateMarkdown(template: TemplateDto): string { | |
| export function renderDigitalTemplateMarkdown(template: DigitalTemplate): string { |
?
There was a problem hiding this comment.
I had wondered this.
| @@ -0,0 +1,11 @@ | |||
| 'use client'; | |||
There was a problem hiding this comment.
I meant to add a comment to explain this - will add one now.
It's basically like a proxy which allows you to import components from nhsuk-react-components into server side code.
| await expect(chooseTemplatesPage.moveToProductionButton).toHaveAttribute( | ||
| 'href', | ||
| `/templates/message-plans/get-ready-to-move/${routingConfigIds.valid}` | ||
| ); |
There was a problem hiding this comment.
Mentioned above - it's not a link anymore, it's a button which triggers a server action.
| {children} | ||
| </SummaryList.Value> | ||
| ); | ||
| } |
There was a problem hiding this comment.
i'm not sure what's being gained from this level of splitting everything out. this seems to be a 57-line file where all it does is append a class name to an existing nhsuk-react-components component, which would just be a few characters if it wasn't split out?
There was a problem hiding this comment.
This was an existing component (the nhsuk-react-components component with customised styling) which is now being reused in a few places. It's just the styling though, I guess I could just import the scss module into the places where this new component is being used, and just render the proper nhsuk component and apply the styles to it in each place. But having an scss module without a component feels odd - it feels to me to worth having it encapsulated into a proper component.
There was a problem hiding this comment.
could just put our style overrides in app.scss?
frontend/src/components/molecules/MessagePlansList/MessagePlansList.tsx
Outdated
Show resolved
Hide resolved
...st-team/template-mgmt-routing-component-tests/preview-message-plan.routing-component.spec.ts
Outdated
Show resolved
Hide resolved
...st-team/template-mgmt-routing-component-tests/preview-message-plan.routing-component.spec.ts
Show resolved
Hide resolved
...st-team/template-mgmt-routing-component-tests/preview-message-plan.routing-component.spec.ts
Outdated
Show resolved
Hide resolved
|
|
||
| await expect(page).toHaveURL( | ||
| `${baseURL}/templates/message-plans/choose-templates/${dbEntry.id}` | ||
| ); |
There was a problem hiding this comment.
what do you think about checking the status once we get here? i.e. to confirm its not got "production" status?
There was a problem hiding this comment.
unsure what the value is - why would it have changed? the db storage helper doesn't have any functionality for getting data, so presumably we're not doing anything like that anywhere else either. that sounds a bit lazy - not against adding it, just not sure if it's worth it
There was a problem hiding this comment.
well I more meant to clearly differentiate between the production ones going to the preview page and it being the choose templates when its not production
so not testing because it would have changed, but because we expect that and expect it not to be production
but if you dont think there's value, thats fair
There was a problem hiding this comment.
I get what you mean, it's just because the config is created explicitly by the test with a status of DRAFT. Checking the template status is not really testing the behaviour of the page, it's just ensuring that there's no random process outside of the test that has altered that status in the background in the time it took to create the item and load the page. Or that the act of redirecting from one page to the other doesn't change the status.
I guess I could check that the status tag on the choose templates page that it's redirected to says "Draft"
...st-team/template-mgmt-routing-component-tests/preview-message-plan.routing-component.spec.ts
Outdated
Show resolved
Hide resolved
...st-team/template-mgmt-routing-component-tests/preview-message-plan.routing-component.spec.ts
Outdated
Show resolved
Hide resolved
...st-team/template-mgmt-routing-component-tests/preview-message-plan.routing-component.spec.ts
Show resolved
Hide resolved
|
|
||
| await expect(links[index]).toHaveAttribute( | ||
| 'href', | ||
| `/templates/preview-submitted-letter-template/${templates[language].id}` |
There was a problem hiding this comment.
maybe we could click the last one? once we're at the end of the test, just so we've interacted with at least one of the links?
| }); | ||
| }); | ||
|
|
||
| test('letter only with no conditional templates', async ({ page }) => { |
There was a problem hiding this comment.
not a comment about this but a question - if its app only, is the preview already expanded and the toggle button also hidden? or is that just for letter because its links only?
There was a problem hiding this comment.
the second one - it's just hidden if the cascade is letter only because there are no template previews, just links
| constructor(page: Page) { | ||
| super(page); | ||
|
|
||
| this.messagePlanId = page.getByTestId('plan-id'); |
There was a problem hiding this comment.
nitpick, feel free to ignore but isnt this referred to/displayed on the page as routing config ID ?
There was a problem hiding this comment.
It's called routing plan id actually - new terminology which is a mash up of both 😆 I asked Alex and he said that it's right though. I'm really confused about the two names and where to use what.
frontend/src/content/content.ts
Outdated
| remove: 'Remove{{templateCount|| all}}', | ||
| templateWord: '{{templateCount|template|templates}}', | ||
| }, | ||
| optional: '(optional)', |
There was a problem hiding this comment.
did you say you weren't using this anymore?
| @@ -0,0 +1,92 @@ | |||
| 'use client'; | |||
There was a problem hiding this comment.
I think I'm surprised you've had to create this vs using from the react lib?
3 participants
Description
Adds new page to show preview of message plan in production. Page looks visually similar to "choose templates" page, but with different logic - e.g. the "card" elements have different content, conditional templates which aren't set are not shown (choose templates always shows slots for all conditional templates even if they are empty) - so also refactored a lot of the components there to be composable, and have lifted the choose templates specific logic out of the components and into the page.
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.