Non blocker

[amitgalitz]

Signed-off-by: Amit Galitzky [email protected]

Description

This PR integrates the new additional type parameter for validation API that includes model validation. Since model (non-blocker) validation also runs detector (blocker) validation, currently I only call the validate API with model parameter. In the future we can separate the two if we want to make the non-blocker validation an optional action. With this API integrated the user will be able to see any non-blocking issues.

Changes include:

• If response type is model then object passed to model or detector definition will include that information and the callout will be of color warning instead of danger

Screenshot:

Check List

• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[ohltyler]

issues -> Issues

[ohltyler]

Why would we want to remove the warning toast here? What does an aggregation issue type mean?

[amitgalitz]

The reason for this is due to the fact that if there is an aggregation issue then it might be an issue that arouse from a change in core and user won't have much they can do about it and for timeout they also can't really re-try since action happens automatically when entering ReviewAndCreate page so I thought that I could just make it basically invisible to users instead of additionally giving a warning toast that will confuse them on there next action.

[ohltyler]

In this same if block, I see it sets the variables indicating valid detector settings and model configs, when in fact that might not be true if there was a problem performing the queries, or if it timed out. It seems like a bad user experience to see it showing as valid in the callouts, then failing later when actually creating the detector, or trying to start it.

How about having a separate callout text indicating that validation failed to run? It could be generic so that it covers both aggregation and timeout failures. I think showing the toast with more details on the failure (aggregation or timeout) could be helpful to the user, and would allow for quick user feedback in case there is something wrong with the AD code that we would need to fix (e.g., aggregation failure where the queries being made in validation API call need to be updated after core changes something internal).

[ohltyler]

As discussed separately, these issue types only come up during non-blocker validation, after blocker validation has passed. So in that case it is mostly validated, just not fully validated / not able to provide any recommendations in case there is data sparsity due to any of the configs / features.

I think the 2 options are (1) hide these failures like you're doing now, or (2) find a way to tell the user that blocking validation passed, but non-blocking validation failed to run properly. For simplicity, I think its ok to go with option 1 for this release, and sync with UX team on a solution to option 2 later on.

Suggest to update the comments on line 117 to reflect something along the lines of this

[codecov-commenter]

Codecov Report

Merging #202 (c2fc4e9) into main (53a0f4f) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #202      +/-   ##
==========================================
- Coverage   45.93%   45.90%   -0.04%     
==========================================
  Files         150      150              
  Lines        4944     4956      +12     
  Branches      951      955       +4     
==========================================
+ Hits         2271     2275       +4     
- Misses       2455     2463       +8     
  Partials      218      218              

Impacted Files	Coverage Δ
...ges/ReviewAndCreate/containers/ReviewAndCreate.tsx	39.53% <0.00%> (-2.24%)	⬇️
...ectorDefinitionFields/DetectorDefinitionFields.tsx	68.42% <0.00%> (-2.17%)	⬇️
...in/public/pages/DefineDetector/utils/constants.tsx	100.00% <0.00%> (ø)
...elConfigurationFields/ModelConfigurationFields.tsx	44.11% <0.00%> (+0.17%)	⬆️
...tion-dashboards-plugin/public/redux/reducers/ad.ts	57.50% <0.00%> (+0.35%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53a0f4f...c2fc4e9. Read the comment docs.

[ohltyler]

LGTM! Just one leftover log to clean: #202 (comment)

[ohltyler]

Let's wait for the backend PR to merge before merging this one.