Core 1498 Fix preferences validation #416

sri2k1us · 2021-09-22T15:31:59Z

No description provided.

sri2k1us · 2021-09-22T15:32:30Z

src/components/preferences/Preferences.js

+        if (values?.webhook?.url) {
+            const type = values?.webhook?.type?.type;
+            if (type === null || type === undefined || type === '') {
+                errors["webhook.type.type"] =  t("webhookTypeError");
+            }
+        }


This validation will run but will not mark errors on the form 😭

Going off memory, I think errors is an object with the same format as the model you provided.
So errors["webhook.type.type"] would be accessing an object like {"webhook.type.type": "someValue"} instead of {webhook: {type: {type: "someValue"}}}. So I think if you update this to errors.webhook.type.type you'll get the result you want.

Ahh that makes sense although errors.webhook.type.type = err msg resulted in error. So I guess I have to construct the error object manually 🤔

Actually that reminds me! I don't think it's advertised anywhere, but Formik has a setIn function (similar to the getIn function that we're using in our helper form fields) that would be useful in this case.

errors = setIn(errors, "webhook.type.type", t("webhookTypeError"))

errors = { webhook: { type: { type: t("webhookTypeError") } } }; worked for me 🎉

sri2k1us · 2021-09-22T15:33:35Z

src/components/preferences/Webhooks.js

                        size="small"
-                        component={FormTextField}
+                        component={FormSelectField}


If I add field level validation, it will work but I have to touch the field first 😕 . Also I had to change it FormSelectField instead of FormTextField

Having to touch the field first sounds right to me. If the user opens a blank form, for example, we don't want every field to automatically be red just because they haven't filled it out yet. So validation should run after a field has been touched or when the user tries to submit.

sri2k1us · 2021-09-22T15:35:13Z

src/serviceFacades/users.js

@@ -94,7 +93,7 @@ function updateWebhooks(webhooks) {
    return callApi({
        endpoint: `/api/webhooks`,
        method: "PUT",
-        body: webhooks,
+        body: webhooks ? webhooks : { "webhooks": [] },


Allow users to delete webhooks.

sri2k1us · 2021-09-22T15:35:54Z

src/components/preferences/Webhooks.js

+        const hasURL = values?.webhook?.url ? true : false;
+        setEnableTestButton(hasURL && !isTesting);
+    }, [isTesting, values]);
+


Enable Test button only when there is url or another test is not in progress.

sri2k1us · 2021-09-22T18:47:46Z

src/components/preferences/Webhooks.js

+    React.useEffect(() => {
+        const hasURL = values?.webhook?.url ? true : false;
+        setEnableTestButton(hasURL && !isTesting);
+        setFieldTouched("webhook.type.type", hasURL, false);
+    }, [isTesting, setFieldTouched, values]);
+


Ok I got validation message to show up immediately after user tabs out of (onBlur) of url field. Is this reasonable ? or there is better way to do this ?

This seems ok to me, but I'll defer to @psarando since he probably has the most experience with Formik.

An alternative might be to add a validate prop to the url field which does this same logic. That way, the logic should only run when the url value changes instead of when any value in values changes.

I think it's acceptable to only show the validation error message if the user actually touches the Type field, or attempts to save the form without the Type set (and as far as I can tell the Type isn't required for testing the URL).

If we set the required prop on the Type field based on the URL field's value, then I don't think we need to manually set this field as "touched" here.

sri2k1us · 2021-09-22T20:47:34Z

Also I think we should hide KB shortcuts we can implement them.

psarando · 2021-09-22T22:25:15Z

src/components/preferences/Preferences.js

+            const type = values?.webhook?.type?.type;
+            if (type === null || type === undefined || type === "") {


I think this is a bit simpler:

Suggested change

const type = values?.webhook?.type?.type;

if (type === null || type === undefined || type === "") {

if (!values?.webhook?.type?.type) {

psarando · 2021-09-22T22:34:06Z

src/components/preferences/Webhooks.js

@@ -82,7 +90,6 @@ export default function Webhooks(props) {
                        name="webhook.type.type"
                        id={buildID(baseId, ids.WEBHOOK_TYPES_SELECT)}
                        label={t("type")}
-                        required


This could actually be set to true if there is a value entered for the URL field:
required={!!(values?.webhook?.url)}

Actually, doing this only partially solved the problem 🤔 . Now the formik built in Required validation marker shows up but our custom error message still doesn't show until the field is touched.

After field is touched:

This might be because every field in the form should have an initial value, even if it's an empty string: jaredpalmer/formik#445 (comment)

If you're seeing uncontrolled to controlled warnings in the console for this field, then that's probably it.

So the mapPropsToValues might need to return an empty webhook.

Looking at these form field names, maybe it needs to return something like this:

return { ...bootstrap.preferences, webhook: { url: "", type: { type: "" } }, }

Ahh yes. I keep forgetting to check initial values.

psarando · 2021-09-22T22:38:05Z

src/components/preferences/Webhooks.js

+    React.useEffect(() => {
+        const hasURL = values?.webhook?.url ? true : false;
+        setEnableTestButton(hasURL && !isTesting);
+        setFieldTouched("webhook.type.type", hasURL, false);
+    }, [isTesting, setFieldTouched, values]);
+


I think it's acceptable to only show the validation error message if the user actually touches the Type field, or attempts to save the form without the Type set (and as far as I can tell the Type isn't required for testing the URL).

If we set the required prop on the Type field based on the URL field's value, then I don't think we need to manually set this field as "touched" here.

sri2k1us · 2021-09-24T15:21:38Z

src/components/preferences/Webhooks.js

+                        validate={(value) => {
+                            if (values?.webhook?.url && !value) {
+                                return t("webhookTypeError");
+                            }
+                        }}
                    >


Finally got the validation to work by removing required and adding field level validation.

psarando

LGTM 👍

psarando · 2021-09-24T20:37:55Z

src/components/preferences/Preferences.js

    const mapPropsToValues = (bootstrap) => {
+        const emptyPref = {


psarando · 2021-09-24T20:38:27Z

src/components/preferences/Preferences.js

                                webhookTopics={webhookTopics}
                                webhookTypes={webhookTypes}
                                values={props.values}
+                                setFieldTouched={props.setFieldTouched}


Looks like this is no longer needed:

Suggested change

setFieldTouched={props.setFieldTouched}

ash3rz

LGTM

sri2k1us · 2021-09-27T14:12:33Z

Thanks for the reviews.

Sriram Sirinvasan added 2 commits September 21, 2021 13:55

Make webhooks and kb shortcuts optional.

433a5df

core-1498 Try fixing webhooks validation.

78718b4

sri2k1us commented Sep 22, 2021

View reviewed changes

Sriram Sirinvasan added 2 commits September 22, 2021 11:37

core-1498 Code formatting.

59397f2

core-1498 Fix webhook type validation.

a3f0cf6

sri2k1us commented Sep 22, 2021

View reviewed changes

psarando reviewed Sep 22, 2021

View reviewed changes

Sriram Sirinvasan added 4 commits September 23, 2021 11:09

core-1498 Set type field required based on the webhook url field value.

39956a0

core-1498 Fix webhook type validation.

1db25ef

core-1498 Hide Kb shortcut preferences until it is implemented.

1bcf688

core-1498 Add some comments.

956733d

sri2k1us commented Sep 24, 2021

View reviewed changes

sri2k1us marked this pull request as ready for review September 24, 2021 15:22

sri2k1us requested review from psarando and ash3rz September 24, 2021 15:22

psarando approved these changes Sep 24, 2021

View reviewed changes

ash3rz approved these changes Sep 24, 2021

View reviewed changes

core-1498 Remove unused prop.

e325bef

sri2k1us merged commit 9db9130 into cyverse-de:master Sep 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core 1498 Fix preferences validation #416

Core 1498 Fix preferences validation #416

sri2k1us commented Sep 22, 2021

sri2k1us Sep 22, 2021

ash3rz Sep 22, 2021

sri2k1us Sep 22, 2021

ash3rz Sep 22, 2021

sri2k1us Sep 22, 2021

sri2k1us Sep 22, 2021

ash3rz Sep 22, 2021

sri2k1us Sep 22, 2021

sri2k1us Sep 22, 2021

sri2k1us Sep 22, 2021

ash3rz Sep 22, 2021

psarando Sep 22, 2021

sri2k1us commented Sep 22, 2021

psarando Sep 22, 2021

psarando Sep 22, 2021

sri2k1us Sep 23, 2021

sri2k1us Sep 23, 2021

sri2k1us Sep 23, 2021

psarando Sep 23, 2021 •

edited

sri2k1us Sep 24, 2021

psarando Sep 22, 2021

sri2k1us Sep 24, 2021

psarando left a comment

psarando Sep 24, 2021

psarando Sep 24, 2021

ash3rz left a comment

sri2k1us commented Sep 27, 2021

		const type = values?.webhook?.type?.type;
		if (type === null \|\| type === undefined \|\| type === "") {

	const type = values?.webhook?.type?.type;
	if (type === null \|\| type === undefined \|\| type === "") {
	if (!values?.webhook?.type?.type) {

		const mapPropsToValues = (bootstrap) => {
		const emptyPref = {

Core 1498 Fix preferences validation #416

Core 1498 Fix preferences validation #416

Conversation

sri2k1us commented Sep 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sri2k1us commented Sep 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psarando Sep 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psarando left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ash3rz left a comment

Choose a reason for hiding this comment

sri2k1us commented Sep 27, 2021

psarando Sep 23, 2021 •

edited