bug: Calculate component error handling fix

[RODO94]

Bug report here: https://trello.com/c/kmRRM7uQ/3041-remove-incorrect-calculate-component-errors-for-valid-formulas

Currently being reviewed by Content Team

To fix this, I changed how errors were handled in the component, using Formik for the error handling and moving it to validate onsubmit rather than on change.

Main change is here:

    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))) {
          errors.formula = "Enter a formula which outputs a number";
        }
      } catch (error: any) {
        errors.formula = "Invalid formula: " + error.message;
      }
      return errors;
    },

    validateOnChange: false,

I have changed the condition from typeof result === "number" to Number.isNaN(Number(result)) so we can properly deal with String values coming from the NumberInput component

---

Refactor Opportunity

There's a slight repetition of logic here between validate:{} and SampleResult which we could look to refactor in future. The refactor could either look to put SampleResult within a UseEffect() and use it within the useFormik({validate:{}}) or add a new function to deal with the validation separately which would output {result, errors} as noted by @DafyddLlyr below.

Although logic is similar, the return messaging is different between the two with the validate: logic focusing on error messaging whereas the SampleResult() focuses on use guidance and help.

[github-actions]

Removed vultr server and associated DNS entries

[RODO94]

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

[RODO94]

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

[DafyddLlyr]

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 👍

[RODO94]

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?

[DafyddLlyr]

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]);

[RODO94]

@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.

[RODO94]

Remove incorrect Calculate component errors for valid formulas

[jessicamcinchak]

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

[RODO94]

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.

[RODO94]

Added this to ensure clear error messaging is given to Editors

[DafyddLlyr]

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

[RODO94]

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

[DafyddLlyr]

Tricky one! I think it's worth spending a bit more time here aiming to leave both the bug fixed and the code better than we found it.

Happy to jump on a call post-dev call and talk it through if helpful 👍

[DafyddLlyr]

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