[MM-16229] Implement proper client-side error handling for create/attach Jira issue dialogs

[cpoile]

Summary

• Display a client-side error when the user clicks submit and there are empty fields marked required.
• Removed the built in validation for input fields (bootstrap?) because @jasonblais found that these errors did not display in the desktop app. Replaced with this method.
• For now, the validation is simply whether or not it is required, and if so make sure it's not empty. Can be improved as needed.

Unrelated fixes:

• Found a few problems with required props, fixed those.
• Should be using find instead of filter in react-select value fields.

Looks like:

Links

MM-16229

[jasonblais]

Looks good to me. Thanks Chris! Confirming that the error text color is theme-dependent? From reading the diff file seems like it is

[cpoile]

@jasonblais It's the error-text class, so I'm assuming yes. Here's with the default theme:

[crspeller]

because @jasonblais found that these errors did not display in the desktop app

That doesn't make sense. it's Electron which is just Chrome. Any explanation for this?

This overall seems over complicating what should be simple. Can't we just validate on submit?

[cpoile]

@crspeller

1. Weird, I know, but I didn't see the errors either on desktop. Try it out on community -- any input field will have the validation working (the react-select fields don't validate). So, even if we got them to show, we still would have to validate the react-select fields manually.
2. What do you mean by validating on submit? As in, through the form fields? The problem was the validation didn't show (point 1), and the react-select components.

[crspeller]

@cpoile

1. Are you saying there are existing fields that have this issue? Can you point out one for me?
2. I mean when the form is submitted run the validation and display the error like we do pretty much everywhere in MM. Maybe I am going overboard in the name of simplicity.

[cpoile]

@crspeller No, they're good questions -- I just hope I'm understanding you.

1. Yes, every one that is a field that is required. In desktop, for me and Jason, these don't show the floating tooltip when you press create (create an issue, select a project like Customer Reports, don't fill in summary, press create -- no tooltip, but it should show one).
2. Here I may not be understanding you -- this is what the PR does -- on create, it runs validation and the components that aren't valid display an error. We wanted the error to be below the field that isn't valid (as it should be).

[levb]

@crspeller I asked basically the same even if semantically different question talking to @cpoile offline, "why don't we do it on the server?" I suppose the answer is, we value displaying these errors in line, enough to have this code. I really welcome this conversation - I'm server-side (and simplicity) biased.

I'd say we have a separate conversation about it - input validation philosophy for interactive plugins? And for now merge this PR if it's the only outstanding question?

[crspeller]

1. I actually thought you where meaning the other places we use react-select. Since that issue would be a bug of react-select no? Wondering if just solving this would be enough since we only validate empty.

2. What I really meant was what Lev was taking about which is centralizing the validation and not worrying about putting it inline. It's what MM does all over the interface.

[cpoile]

@crspeller

1. Not a bug, it's on their roadmap (not implemented yet because it's complicated) How to make react-select required in a form? JedWatson/react-select#3140
2. @jasonblais what do you think, just have a single error message saying something like One of the form values was marked as required but was not filled out? Personal opinion: I think having the error appear under the required field is a better UX.

[jasonblais]

This approach is more consistent with Mattermost modals as far as I know

@crspeller @cpoile

[crspeller]

@jasonblais Thanks for pointing out those modals.
@cpoile Is it possible to do a simpler implementation like those modals do?

[cpoile]

@crspeller

What new_channel_modal does:

1. in handleSubmit: it validates the displayName element, and sets state (displayNameError) if it's not valid, then returns immediately (cancelling the submit)
2. that state change triggers the render, which displays the error

What we're doing here:

1. the same thing
2. The added difference is that we don't know how many JiraFields we have to validate. So we're putting them in a map, and going through that when we validate. If any of the components in the map are not valid, then we don't submit.
3. If we knew what fields we had ahead of time, it would be the same as what the new_channel_modal does. And for the project and issue fields (which aren't dynamic) it basically is.

We have to add the components to the map, and remove them, because people can switch projects and new jira_field components are created and destroyed. So we have to keep track of that.

I'm totally open to any other solutions. :) (Please!) But I don't think this is overly complicated. I tried a couple solutions before settling on this, but I can't think of any other way to do it when we don't know what fields we have to validate ahead of time.

[crspeller]

Alright we will have to accept this complexity then.