-
Notifications
You must be signed in to change notification settings - Fork 1
CCM-12038: preview production message plan #792
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
CCM-12038: preview production message plan #792
Conversation
| }, | ||
| ctas: { | ||
| primary: { | ||
| href: '/message-plans/get-ready-to-move/{{routingConfigId}}', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how come?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how was this working before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| export function renderTemplateMarkdown(template: TemplateDto): string { | |
| export function renderDigitalTemplateMarkdown(template: DigitalTemplate): string { |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had wondered this.
...s/test-team/template-mgmt-routing-component-tests/choose-templates.routing-component.spec.ts
Show resolved
Hide resolved
| {children} | ||
| </SummaryList.Value> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
...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
Show resolved
Hide resolved
| @@ -0,0 +1,92 @@ | |||
| 'use client'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm surprised you've had to create this vs using from the react lib?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean?
| templateIds.NHSAPP, | ||
| user, | ||
| 'Test NHS App template' | ||
| ` 'Test NHS App template - ${templateIds.NHSAPP}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra spaces here
| `details.${targetClassName}` | ||
| ); | ||
| for (const element of details) { | ||
| element.open = isOpen; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you briefly explain why we mutate the dom here, for my understanding?
| throw new Error('Routing configuration not found'); | ||
| } | ||
|
|
||
| const channelsWithoutTemplates = getChannelsMissingTemplates(routingConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the PROOF_APPROVED PR should go in today - at that point you might want to add validation that all letters are PROOF_APPROVED
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this case shouldn't be reachable on the frontend anyway, since we prevent selection of letter templates in other statuses. So it doesn't need to be handled on the frontend?
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.