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.

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

[FEATURE] Create/Edit rule validation on submit and visual-yaml editor switching #215

Closed
djindjic opened this issue Dec 13, 2022 · 10 comments
Labels
enhancement New feature or request

Comments

@djindjic
Copy link
Contributor

Is your feature request related to a problem?
Specific client side validations should be shown inlined on form submit (not only on blur) for both visual and yaml editor.
Additionally, show client side validation messages while switching between editors.

Screen.Recording.2022-12-13.at.8.02.28.AM.mov
@djindjic
Copy link
Contributor Author

djindjic commented Dec 22, 2022

@kamingleung @getsaurabh02 @amsiglan

We should have same form validation experience as we have on other plugins. Here is an example of Create rollup job form on Snapshot Management plugin.

Screen.Recording.2022-12-22.at.8.58.08.PM.mov

Please notice that even this form is not fully consistent since required message on job name is different depending if user action is clicking submit button vs blur out of input field.

Acceptance Criteria should be:

  • form has no red flags on initial state
  • touching the field (blur out) triggering validation on that field and shows eventual inline error message
  • clicking submit button (create or edit in case of rule forms) triggers all validations on all fields and shows all eventual error messages
  • generally speaking, server side validation error messages or some unhandled errors should be shown in form of toast notification

I still need to think on switching the editor type behavior in context of validation messages

@kamingleung
Copy link

kamingleung commented Dec 22, 2022

@djindjic Do we know which fields are required to create a rule?
I am assuming we need the following with requirements:

  • Rule name
  • Log type
  • Detection
  • Rule level
  • Author
  • Rule status

Here are some guidance on validation pattern:
If any of the required fields are empty, we can show validation error onBlur until the issue is fixed, [fieldName] is required.
Like this:
image

If users attempt to click on Create when there are validation errors, we should show a toast like this at the bottom right of the screen:
image

@kamingleung
Copy link

kamingleung commented Dec 22, 2022

For dropdowns in this create flow, such as Log type, Rule level, and Rule status, let's follow the following pattern:
We should leave them empty by default, with a placeholder text:
image

When users click on the dropdown, they will select from the selections.
image

If the user didn't select anything when clicking on Create, it will show the toast above and inline validation error:
image

@kamingleung
Copy link

I assume the following fields are optional?

  • Description
  • Tags
  • References
  • False positive cases

Let's add "- optional" in italics to help users identify which fields are required. Like this:
image
Example from Create rollup job in Index Management.

@djindjic
Copy link
Contributor Author

djindjic commented Dec 23, 2022

If users attempt to click on Create when there are validation errors, we should show a toast like this at the bottom right of the screen: image

@kamingleung
At the moment I have the only one issue with showing toaster if form is already in invalid state (form submission is blocked). Let me know if I can find this example on playground? I guess this toaster is produces by some backend error or validation outside of the form itself.

@djindjic
Copy link
Contributor Author

I assume the following fields are optional?

  • Description
  • Tags
  • References
  • False positive cases

Let's add "- optional" in italics to help users identify which fields are required. Like this: image Example from Create rollup job in Index Management.

@kamingleung
This is possible with custom flex group above the form field. Check how it is done in index management:
image
But I would try to go with some of form layout features defined in docs:
https://eui.elastic.co/v34.6.0/#/forms/form-layouts
Unfortunately, can't find some special flags for required/optional, but maybe help text below the field could be also used?

@djindjic
Copy link
Contributor Author

@kamingleung @amsiglan @getsaurabh02
I've covered all necessary behaviors except this two:

  1. - optional sufix is not gray-italic for now because of no clear directions on https://eui.elastic.co/v34.6.0/#/forms/form-layouts and I wanted to avoid custom layouting of that purpose (still open for discussion about custom layouting)
  2. Action buttons (Cancel, Create/Save changes) are now in the <ContentPanel> together with the form. This is still possible to be outside as it was before, but I've decided to put it on hold since its not "one line fix" and we have few more prio tasks at the moment.

Please check current behavior on the video and compare it to old behavior on playground

Screen.Recording.2022-12-27.at.9.26.26.AM.mov

@kamingleung
Copy link

kamingleung commented Dec 30, 2022

@djindjic Thanks for your updates.

  1. The - optional isn't a reusable style/component in EUI today, however we can align the style of - optional found in the Create detector flow.
  2. Action buttons - Let's place them underneath the container to align with the placement with other create flows in Dashboards

Follow-up questions from your video:

  1. Are these confirmed as naming restrictions on Rule name and author?
    image
    Let's rewrite this sentence into:

For rule name:

Rule name must contain 5-50 characters. Valid characters are a-z, A-Z, 0-9, hyphens, spaces, and underscores.

For author:

Author must contain 5-50 characters. Valid characters are a-z, A-Z, 0-9, hyphens, spaces, and underscores.

  1. We should show restriction text underneath the form field at all times like this (example from Anomaly Detection):
    image

  2. When there are invalid naming requirements, we can show error above the restriction text, "Invalid rule name." or "Invalid author." like this:
    EUI reference: https://eui.elastic.co/v34.6.0/#/forms/form-validation
    image

@djindjic
Copy link
Contributor Author

djindjic commented Dec 30, 2022

@kamingleung
I think I've addressed all your points plus:

  1. extended author restriction text with "commas" (reflecting existing validation in the code)
  2. added similar restriction text underneath description field based on existing validation in the code (including "commas" and "dots")

Since PR for this issue is already merged I've done it in currently active one which is related (#279)

Please check video:

Screen.Recording.2022-12-30.at.9.26.27.PM.mov

@kamingleung
Copy link

@djindjic This looks good to me. Thanks for your updates!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants