CCM-14372: Add a callout message for letter if letter authoring is di…#829
CCM-14372: Add a callout message for letter if letter authoring is di…#829m-salaudeen wants to merge 20 commits intomainfrom
Conversation
.tool-versions
Outdated
| gitleaks 8.24.0 | ||
| jq 1.6 | ||
| nodejs 22.22.0 | ||
| nodejs 22.16.0 |
There was a problem hiding this comment.
Why is this being downgraded?
There was a problem hiding this comment.
Fixed, it was a local merge error
|
Looks like just pa11y tests are failing? These have been disabled on main, so you should get a green pipeline if you merge in main. |
| warningCalloutContent: { | ||
| headingLabel: 'To create a letter template', | ||
| firstParagraph: 'You cannot upload a letter template using this service.', | ||
| secondParagraph: 'Follow our guidance to ', |
There was a problem hiding this comment.
I don't think we should include trailing whitespace here
There was a problem hiding this comment.
Could do the link with markdown and use a MarkdownContent to avoid the split entirely
|
|
||
| await expect(chooseTemplateTypePage.templateTypeRadioButtons).toHaveCount( | ||
| 4 | ||
| 3 |
There was a problem hiding this comment.
This depends on the letter authoring flag, right? If so, we should have a test which asserts that it's 4 when authoring is true. The other removed test could also still be run with an authoring user
| ))} | ||
| </Radios> | ||
| </Fieldset> | ||
| {isLetterAuthoringEnabled ? null : ( |
There was a problem hiding this comment.
This is temporary so maybe it doesn't matter - but I think it would be cleaner to pass in children here instead of putting something really specific in a generic component
There was a problem hiding this comment.
Alternatively, fork the component completely, call it ChooseTemplateTypeRadios or similar, add a comment to say to switch it back to NHSNotifyRadioButtonForm once we get rid of the flag.
However it's done, the conditional content should be off by default. This will allow you to remove unnecessary changes to pages which aren't related to letters at all
There was a problem hiding this comment.
Files like PreviewDigitalTemplate.test.ts shouldn't need to be changed
|
|
||
| jest.mock('@providers/client-config-provider', () => ({ | ||
| useFeatureFlags: jest.fn().mockReturnValue({ | ||
| letterAuthoring: false, |
There was a problem hiding this comment.
We probably need to test both on and off here?
| screen.getByTestId('email-radio'), | ||
| screen.getByTestId('nhsapp-radio'), | ||
| screen.getByTestId('sms-radio'), | ||
| screen.getByTestId('letter-radio'), |
There was a problem hiding this comment.
I would explicitly check that letter-radio isn't there for the off case
4 participants
…sabled
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.