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

Add standalone form state for source data #445

Merged
merged 5 commits into from
Oct 30, 2024

Conversation

ohltyler
Copy link
Member

@ohltyler ohltyler commented Oct 29, 2024

Description

This PR decouples the form state for configuring source data in the "Edit source data" modal. Instead of using the global, parent form, we create a new sub-form to persist interim form state. The parent form is only updated, if a user explicitly clicks "Update", and if there are no validation errors. Following PRs will follow this same pattern for other form-based modals, including input/output transforms, and query editor modals.

Details:

  • moved the modal into standalone SourceDataModal component. Here, we create a new Formik component, and persist some custom hooks etc. within it, so we can listen on changes only to the sub-form values.
  • refactors state vars and state handling from SourceData -> SourceDataModal as much as possible
  • add new types for the sub-form
  • removes leftover legacy WorkspaceFormValues -> WorkflowFormValues for type safety

Demo video:

screen-capture.9.webm

Issues resolved

Makes progress on #446

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.

Signed-off-by: Tyler Ohlsen <[email protected]>
@ohltyler
Copy link
Member Author

IT failure unrelated to this PR. After rerunning on main, it is happening there. Maybe some flaky test. Any idea @saimedhi ?

@saimedhi
Copy link
Collaborator

IT failure unrelated to this PR. After rerunning on main, it is happening there. Maybe some flaky test. Any idea @saimedhi ?

@ohltyler, Integ tests are passing on other PRs. I will take a look why tests are failing here.

@saimedhi
Copy link
Collaborator

IT failure unrelated to this PR. After rerunning on main, it is happening there. Maybe some flaky test. Any idea @saimedhi ?

@ohltyler, Integ tests are passing on other PRs. I will take a look why tests are failing here.

@ohltyler, spotted the reason for integ tests to fail. This PR needs integ test update because below is the difference when providing source data in ingest step. This PR adds update button.

Before:
Screenshot 2024-10-30 at 9 41 54 AM

Now:
Screenshot 2024-10-30 at 9 40 58 AM

@ohltyler
Copy link
Member Author

@saimedhi not sure I follow. I reran on main and it is failing the same, so it's not dependent on this PR. It's only failing for the semantic search use case as well: https://github.com/opensearch-project/dashboards-flow-framework/actions/runs/11577301891/job/32286225112

@ohltyler
Copy link
Member Author

@saimedhi in the meantime, can I get review of this PR? The integ test issue can be tracked separately.

@saimedhi
Copy link
Collaborator

@saimedhi not sure I follow. I reran on main and it is failing the same, so it's not dependent on this PR. It's only failing for the semantic search use case as well: https://github.com/opensearch-project/dashboards-flow-framework/actions/runs/11577301891/job/32286225112

@ohltyler
Copy link
Member Author

As discussed, I will update the corresponding IT for the docs to be propagated. But, there is still the flaky issue on the same test spec that was happening in main. Let's not let either be a blocker for this PR

@ohltyler
Copy link
Member Author

Button update will be fixed by opensearch-project/opensearch-dashboards-functional-test#1612

@ohltyler ohltyler merged commit a197021 into opensearch-project:main Oct 30, 2024
6 of 7 checks passed
@ohltyler ohltyler deleted the interim-form-modals branch October 30, 2024 17:54
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 30, 2024
Signed-off-by: Tyler Ohlsen <[email protected]>
(cherry picked from commit a197021)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@saimedhi
Copy link
Collaborator

Looks good to me, tested code.

ohltyler pushed a commit that referenced this pull request Oct 30, 2024
(cherry picked from commit a197021)

Signed-off-by: Tyler Ohlsen <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@saimedhi
Copy link
Collaborator

approved this PR but I am not maintainer for the Functional test repo : opensearch-project/opensearch-dashboards-functional-test#1612

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants