Skip to content
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.

Sign up for GitHub

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

bug: Calculate component error handling fix #3805

Merged
merged 7 commits into from
Oct 17, 2024
Merged

bug: Calculate component error handling fix #3805

Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9421fa0
add validation to validate option of formik
RODO94 Oct 15, 2024
c8f808a
fix: remove unused vars
RODO94 Oct 15, 2024
100dcc9
fix: error in state update of sampleResult
RODO94 Oct 15, 2024
78f801c
refactor: alter error messaging
RODO94 Oct 15, 2024
32b74a5
clear error message on Sample Result
RODO94 Oct 16, 2024
0d0d020
fix: undefined values won't show up in Try section
RODO94 Oct 17, 2024
5fe58b5
refactor: enhanced messaging on undefined return
RODO94 Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 43 additions & 31 deletions editor.planx.uk/src/@planx/components/Calculate/Editor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,8 @@ import { styled } from "@mui/material/styles";
import Switch from "@mui/material/Switch";
import Typography from "@mui/material/Typography";
import { ComponentType as TYPES } from "@opensystemslab/planx-core/types";
import {
EditorProps,
ICONS,
} from "@planx/components/ui";
import { useFormik } from "formik";
import { EditorProps, ICONS } from "@planx/components/ui";
import { FormikErrors, useFormik } from "formik";
import React from "react";
import InputGroup from "ui/editor/InputGroup";
import { ModalFooter } from "ui/editor/ModalFooter";
Expand Down Expand Up @@ -36,12 +33,49 @@ export default function Component(props: Props) {
props.handleSubmit({ type: TYPES.Calculate, data: newValues });
}
},
validate: () => {
// can parse formula
getVariables(formik.values.formula);
validate: (values) => {
const errors: FormikErrors<Calculate> = {};
try {
// can parse formula
getVariables(formik.values.formula);

// Validate formula
const result = evaluate(
values.formula,
values.samples,
values.defaults,
);

if (Number.isNaN(Number(result))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the description, I have used this to catch issues with the NumberInput component which existed but wasn't picked up because we were never blocking the update of the comp

errors.formula = "Enter a formula which outputs a number";
}
} catch (error: any) {
errors.formula = error.message;
}
return errors;
},

validateOnChange: false,
});

const sampleResult = React.useMemo(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to ensure this was an additive fix, I didn't want to take away too much which makes the component work because I couldn't fully grasp how it all functioned within the timeboxed time

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what I think is happening here is that the useMemo() and associated array are just covering the same functionality as validateOnChange: true.

The pattern we want to follow, and other components employ, is to keep validateOnChange: false and then handle validation onSubmit.

I think this means removing this custom validation function, and just using formik 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also feeds the "Try" field which gives you a preview of the formula output:

         <p>
            <strong>{formik.values.output || "<output>"}</strong> would be set
            to <strong>{sampleResult}</strong>.
          </p>

I could maybe move it to state and use that in the validation step as well, updating with a useEffect or something but, this felt like a change on top of the bug fix.

Happy to do a bit more on this if you think it is worth it?

Screenshot 2024-10-16 at 11 11 46

Copy link
Contributor

@DafyddLlyr DafyddLlyr Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, thanks. This is what I've missed 👍

So we do need a way to get a runtime result out which is sampleResult.

I think I'd aim to see if there's a way we can avoid duplicating the validation logic - maybe a function with a signature like this could work in both instances?

type Validate = (values: Calculate) => [result: number | undefined, errors: FormikErrors<Calculate>]

This would look something like this -

validate: (values) => {
  const [_, errors]: validate(values);
  return errors;
},

...

const sampleResult = React.useMemo(() => {
  const [result, _]: validate(formik.values);
  if (!result) return UNKNOWN
}, [formik.values, validate]);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DafyddLlyr got to the end of my timebox and didn't get it working as required. It's definitely possible though and I'll make a note for a potential follow-up PR to tidy this up, if that sounds good to you?

I was mostly caught resolving types and harmonising return messaging.

try {
const result = evaluate(
formik.values.formula,
formik.values.samples,
formik.values.defaults,
);
// Type guard as mathjs evaluates `m` to a "Unit" object for "meter"
if (!Number.isNaN(Number(result))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is !Number.isNaN(Number(result)) actually the same as original type guard check typeof result === "number" ? What's motivation for rewriting this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I could work out, the result would return a string if you were using a data field populated by a NumberInput so result === "number" would return false, although the calculation would work fine. This wasn't a problem when validating on change in the way it was, because it didn't stop you adding the calculate component, but it was a problem when I used Formik to validate on submit, which blocked it.

return result;
} else {
return `the ${typeof result}: '${result}'`;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this to ensure clear error messaging is given to Editors

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

I think it's worth looking at how this is handling undefined values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now got it showing nothing if undefined, which feels right. I kept getting weird behaviour on existing Calculate components when using formik.dirty so I've not used it for conditional rendering

}
} catch (e) {
return UNKNOWN;
}
}, [formik.values.formula, formik.values.defaults, formik.values.samples]);

/**
* When the formula is updated, remove any defaults which are no longer used
*/
Expand All @@ -65,24 +99,6 @@ export default function Component(props: Props) {
}
}, [formik.values.formula]);

const sampleResult = React.useMemo(() => {
try {
const result = evaluate(
formik.values.formula,
formik.values.samples,
formik.values.defaults,
);
// Type guard as mathjs evaluates `m` to a "Unit" object for "meter"
if (typeof result === "number") {
return result;
} else {
return UNKNOWN;
}
} catch (e) {
return UNKNOWN;
}
}, [formik.values.formula, formik.values.defaults, formik.values.samples]);

return (
<form onSubmit={formik.handleSubmit} id="modal">
<ModalSection>
Expand Down Expand Up @@ -140,11 +156,7 @@ export default function Component(props: Props) {
name="formula"
value={formik.values.formula}
onChange={formik.handleChange}
errorMessage={
sampleResult === UNKNOWN
? "Invalid formula or missing default values."
: ""
}
errorMessage={formik.errors.formula}
/>
</InputRow>
</ModalSectionContent>
Expand Down
Loading