opensearch-project / dashboards-flow-framework

A UI designer for constructing AI applications with OpenSearch
Apache License 2.0
6 stars 5 forks source link

Enforce fine-grained form and flow validation #114

Closed ohltyler closed 4 months ago

ohltyler commented 4 months ago

Description

This PR improves the overall validation logic on the workflow editor:

Additional changes:

Note that the form validation logic is complete - however, the flow validation logic is still in TODO state; for now, it always succeeds / is valid.

Screenshots:

Form validation:

Screenshot 2024-03-26 at 11 45 02 AM

Flow validation:

Screenshot 2024-03-26 at 11 44 26 AM

Demo videos:

Form validation integrated with Save button being disabled/enabled depending on unsaved changes. Note that on submitting / clicking Save, all invalid fields are highlighted appropriately, across all components. And, once everything is valid, the callout does not appear when saving.

screen-capture (18).webm

Flow validation integrated with Save button being disabled/enabled depending on unsaved changes. Ignore the callout as that logic is still a placeholder.

screen-capture (19).webm

Issues Resolved

Resolves #5

Check List

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.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 21.39%. Comparing base (83835c2) to head (439b05f).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #114 +/- ## ========================================== + Coverage 20.20% 21.39% +1.19% ========================================== Files 54 54 Lines 797 818 +21 Branches 74 79 +5 ========================================== + Hits 161 175 +14 - Misses 630 639 +9 + Partials 6 4 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

amitgalitz commented 4 months ago

We will we use the backend validation here also in the future when saving?

amitgalitz commented 4 months ago

Also will we have the first save be a create basically then update?

ohltyler commented 4 months ago

We will we use the backend validation here also in the future when saving?

Definitely, the way I envision it is a 3-layer validation. First is basic form validation (no missing fields), second is flow validation (a DAG), and third would be backend validation. Whether we have it proactive (validation API) or reactive, we can support any way.

Also will we have the first save be a create basically then update?

Yes: https://github.com/opensearch-project/dashboards-flow-framework/blob/main/public/pages/workflow_detail/utils/utils.ts#L29:L33

amitgalitz commented 4 months ago

We will we use the backend validation here also in the future when saving?

Definitely, the way I envision it is a 3-layer validation. First is basic form validation (no missing fields), second is flow validation (a DAG), and third would be backend validation. Whether we have it proactive (validation API) or reactive, we can support any way.

Also will we have the first save be a create basically then update?

Yes: https://github.com/opensearch-project/dashboards-flow-framework/blob/main/public/pages/workflow_detail/utils/utils.ts#L29:L33

looks good to me, might want to revisit when we have those integrated how the save functionality will look like with the different validation, since as you commented save = create or update and launch is provision I assume. wonder if we should have differentiation for just quick create and launch too, or like we force to save and then launch, and obvious that the first save is equal to create

ohltyler commented 4 months ago

We will we use the backend validation here also in the future when saving?

Definitely, the way I envision it is a 3-layer validation. First is basic form validation (no missing fields), second is flow validation (a DAG), and third would be backend validation. Whether we have it proactive (validation API) or reactive, we can support any way.

Also will we have the first save be a create basically then update?

Yes: https://github.com/opensearch-project/dashboards-flow-framework/blob/main/public/pages/workflow_detail/utils/utils.ts#L29:L33

looks good to me, might want to revisit when we have those integrated how the save functionality will look like with the different validation, since as you commented save = create or update and launch is provision I assume. wonder if we should have differentiation for just quick create and launch too, or like we force to save and then launch, and obvious that the first save is equal to create

Right this will all need to be revisited when things are more finalized, for now we have placeholders and TODOs in code. This is just incremental.