Check form redesign

[ckbedwell]

Very very aware of what a bad dev citizen I have been with this branch. I am just putting my hands up and will keep apologising over and over for the amount of files touched and the undisciplined commit history. I don't expect it to be reviewed in depth so that's why I wanted to make sure it had comprehensive test coverage.

Problem

We have an internal document outlining the main problems with the current check creation forms with some discussion from the SM team. The main problems with the current flow are:

• Differing amount of steps according to check type which in most cases come across as very daunting (e.g. 8 steps for Http)
• Disjointed steps where depending on what type of check you are the same settings are located in different places
• Related options are fragmented, in particular the options where users 'define uptime' are across multiple steps
• Difficult DX architecture to work with and scale for the future

Solution

This PR includes a comprehensive solution to address all of the points above:

• It reduces cognitive overhead when choosing a check type at the start of the process
• It prioritises what is important when creating checks (making requests) and laying the foundations for a tighter feedback loop for testing requests.
• Creates a consistent and predictable experience regardless of check type (5 steps which are always the same)
• Creates separation of concerns for the larger contributing components (e.g. <CheckForm />, <FormLayout />, etc.)
• Creates reusable components which follow a familiar design pattern (e.g. <Request />)
• Adds a comprehensive testing solution for check creation pages
• Introduced <CheckFormContextProvider /> -- in particular this is used for sharing isFormDisabled but it will also be the mechanism for sharing state when testing individual requests

Screenshots

New check creation page

New api endpoint check creation form

New multistep check creation form

New multistep check creation form on the define uptime step

New scripted check creation form

Test coverage

I've created and added a meta test framework for the check creation pages. The aim is that we now have individual and specific tests for every individual aspect of the check creation forms. They are full integration tests so we have high confidence that any regressions for any part of the form will be targeted specifically and throw an error and be caught.

Jest test runners don't work well and often timeout when a single test file has many individual tests, so the goal of this test framework is to subdivide the tests into sensible and predictable files where it is easy to pinpoint and locate the tests you are looking for.

All of the tests are located in two __tests__ folders under page/NewCheck and page/EditCheck. Each of these folders has subfolders corresponding to the top-level CheckTypeGroups which then contain folders for the CheckType. The file naming convention for each tests follows the following pattern:

{SectionNumber-{SectionName}_{?subSection}.{payload|ui}.test.tsx

The subsections are optional and necessary to reduce the amount of tests per file. (Http checks have a lot of options in the first section so were proving to be a little flaky and timing out in the CI pipelines).

Before

Test Suites: 40 passed, 40 total
Tests:       249 passed, 249 total
Snapshots:   0 total
Time:        40.533 s
Ran all test suites.
✨  Done in 41.27s.

After

Test Suites: 73 passed, 73 total
Tests:       3 skipped, 368 passed, 371 total
Snapshots:   0 total
Time:        77.738 s, estimated 100 s
Ran all test suites.
✨  Done in 78.28s.

Whilst I was there

I was debating having a little more discipline and leaving this cleanup and tech debt work to a future PR, but as I was working with all these files and having a bad DX experience with them it was difficult not to just correct them whilst I was there rather than checking out and making separate branches. Many of the changed files in this diff are:

• Changing styles.stack and styles.stackCol to <Stack /> and <Stack direction="column" />
• Alphabetising props so it was easier to find what I was looking for
• Adding aria-labels to inputs which were missing them
• Removing unnecessary props to components that didn't use/need them (e.g. disabled on the <Field /> components)

Further work

1. The request components should be fully decoupled from react-hook-form. Currently they are difficult to re-use but with a little work their props could be altered to make them 'dumber' and more reusable for use across the application (e.g. like in helping create requests for scripted checks).

2. More test coverage for existing checks. Currently the tests focus on being able to submit new values to the api in the correct shape, but we don't have any tests ensuring that existing data is getting transformed and displayed in the UI correctly. This work shouldn't be addressed until this ticket creating check object factory has been completed.

3. More test coverage for read-only checks. As each input has to be individually disabled, it could be easy in future work to miss adding a disabled state when isFormDisabled is true. I have added a single test for this ensuring the submit button does not work as that is the most element to disable.

4. Add redirects from the old URLs to the new ones where appropriate. This will be in a PR put up shortly.

5. Add nested error discovery when submitting the form. At the moment nested errors are difficult to locate when submitting the form. Link to PR: feat: nested error discovery in check form #868

6. I have left in several unused components (e.g. useTestRequests, <FormSupportingContent />) and commented out lines of code which will be reintroduced in the near future (scripted check card mentioning support for gRPC and extension support).

Extra features

Some of these existed already but I've tidied up the UI and experience to ensure it is as smooth as possible.

Scripted check limit reached

Loaded checks but couldn't find the one requested (e.g. bad URL)

Unable to load check(s) (e.g. 500 server error)

[ckbedwell]

And here 😅 (this also needs some tests to catch it as mentioned in the future work part of the PR).

[VikaCep]

Seems like the button to go to the next step in the form is showing the current step name instead of the next one. For example, in the screenshot we see 3. Define uptime but the step 3 is called Labels

[VikaCep]

Not to do here, but just making a note that we'll have to go through the docs and update the images that are outdated after these changes, like:

[VikaCep]

Amazing work! 🔥🔥🔥
Left a few comments, I still want to take another look but I really love how this is looking, components are much better organized and I really like the consistency between checks. Also, superb work on improving and adding tests!

[VikaCep]

Probably not related to this PR and not suggesting to change it here, but I wonder why we have two separate folders for form related files, CheckEditor and CheckForm. Don't they refer to the same thing?

[ckbedwell]

Yeah, good point! I was thinking as Thomas is out, we should maybe bump up this ticket and tackle it ASAP to cause the least disruption: #848. That way we can address this problem at the same time.

I'll put it in our 1:1 notes and we can discuss it on Wednesday 😃

[ckbedwell]

Not to do here, but just making a note that we'll have to go through the docs and update the images that are outdated after these changes, like:

@heitortsergent Tagging you here but will drop a message on Slack, too. We can work out what will need changing in the documentation. Thanks for pointing this out, @VikaCep!

[ckbedwell]

Seems like the button to go to the next step in the form is showing the current step name instead of the next one. For example, in the screenshot we see 3. Define uptime but the step 3 is called Labels

Good catch! This is because one of the last things I added was the alerts section above the form and it was being counted when working out what the next section was so the count was off by 1. I've fixed this added some tests to make sure it doesn't regress 😄

[VikaCep]

Should this be a Stack component instead?

[VikaCep]

These two props seem to already be set in RequestInput by default, so it looks like they could be removed from here.

[VikaCep]

👏 👏 👏 👏 👏