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

[VisBuilder] Adds UIState to vis, adds index patterns to embeddable, bug fixes #3751

Merged

Conversation

ashwin-pc
Copy link
Member

@ashwin-pc ashwin-pc commented Mar 31, 2023

Note: This Change was built on top of #3732. Some the changes from that PR are in here. This is the diff for the change thats going in this PR: https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3751/files/1472ee1ae26f19b4d8c49e37f65625af2f0076df..6fcace5b6038ae0a5dc41024369410df0c27f02a

Description

This PR has a combination of changes and fixes as listed below:

  1. Adds UI state to the saved object and metadata state to persist data
  2. the multiple errors shown on page load even when there are no aggregations selected
  3. Fixes the issue of being able to select the index pattern in the dashboard views filters when more than one index pattern exists (or if the index pattern differs from the default index pattern)
  4. Fixes typescript errors
  5. Simplifies the serializing and deserializing of saved object data with error handling

Demo:

Screen.Recording.2023-03-30.at.7.04.55.PM.mov

Issues Resolved

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Signed-off-by: Ashwin P Chandran <[email protected]>
Signed-off-by: Ashwin P Chandran <[email protected]>
Signed-off-by: Ashwin P Chandran <[email protected]>
Signed-off-by: Ashwin P Chandran <[email protected]>
Signed-off-by: Ashwin P Chandran <[email protected]>
Signed-off-by: Ashwin P Chandran <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Mar 31, 2023

Codecov Report

Merging #3751 (71ee8fc) into main (0762566) will increase coverage by 0.03%.
The diff coverage is 27.27%.

❗ Current head 71ee8fc differs from pull request most recent head 68c5a00. Consider uploading reports for the commit 68c5a00 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #3751      +/-   ##
==========================================
+ Coverage   66.38%   66.42%   +0.03%     
==========================================
  Files        3208     3209       +1     
  Lines       61750    61783      +33     
  Branches     9538     9542       +4     
==========================================
+ Hits        40993    41037      +44     
+ Misses      18461    18456       -5     
+ Partials     2296     2290       -6     
Flag Coverage Δ
Linux 66.36% <27.27%> (-0.03%) ⬇️
Windows 66.36% <27.27%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...lder/common/vis_builder_saved_object_attributes.ts 100.00% <ø> (ø)
...uilder/public/application/components/right_nav.tsx 12.50% <ø> (ø)
...uilder/public/application/components/workspace.tsx 5.26% <0.00%> (-1.19%) ⬇️
...plication/utils/state_management/metadata_slice.ts 61.53% <ø> (ø)
...cation/utils/state_management/redux_persistence.ts 80.00% <ø> (ø)
...public/application/utils/state_management/store.ts 18.75% <ø> (ø)
...application/utils/use/use_saved_vis_builder_vis.ts 2.77% <0.00%> (-0.26%) ⬇️
...ilder/public/embeddable/vis_builder_embeddable.tsx 0.91% <0.00%> (-0.11%) ⬇️
...blic/embeddable/vis_builder_embeddable_factory.tsx 8.69% <0.00%> (-2.42%) ⬇️
..._builder/public/saved_visualizations/_saved_vis.ts 0.00% <ø> (ø)
... and 4 more

... and 12 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -22,6 +22,7 @@ export interface MetadataState {
state: EditorState;
};
originatingApp?: string;
uiState: any;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we decide to put uiState in the metadata slice? Isn't it better to put uiState in the style slice?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does uiState have to be any type?

@joshuarrrr
Copy link
Member

@ashwin-pc When you rebase this, can you take a look at the remaining nits here? #3732 (review)

@joshuarrrr
Copy link
Member

@ashwin-pc CHANGELOG is missing.

Signed-off-by: Ashwin P Chandran <[email protected]>
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashwin-pc no particular concerns. But it needs a CHANGELOG entry (or skip label), and the snapshots need updating.

Comment on lines +48 to +49
uiState: '{}',
version: 3,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need a migration? To initially set the uiState for any previous VisBuilder objects?

"styleState",
"visualizationState"
],
"additionalProperties": false
}
Copy link
Member

@joshuarrrr joshuarrrr Apr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - missing EOF newline

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how did I type mission instead of missing 🤣

Signed-off-by: Ashwin P Chandran <[email protected]>
joshuarrrr
joshuarrrr previously approved these changes Apr 17, 2023
Signed-off-by: Ashwin P Chandran <[email protected]>
abbyhu2000
abbyhu2000 previously approved these changes Apr 17, 2023
joshuarrrr
joshuarrrr previously approved these changes Apr 17, 2023
@joshuarrrr
Copy link
Member

I'm waiting for automation co complete before resolving the CHANGELOG conflict.

@joshuarrrr
Copy link
Member

All tests pass. I'll resolve the changelog conflict, and then merge without awaiting another test run.

@joshuarrrr joshuarrrr dismissed stale reviews from abbyhu2000 and themself via 6c9bddc April 18, 2023 00:38
@joshuarrrr joshuarrrr merged commit ee32d20 into opensearch-project:main Apr 18, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 18, 2023
…bug fixes (#3751)

* adds uiActions to visBuilder
* prevents multiple errors on load
* fixes visbuilder type errors
* fixes save
* adds ui state to vis builder
* fixes tests
* Adds support in embeddables for multiple indices
* Moves ui state to separate slice
* Adds changelog

Signed-off-by: Ashwin P Chandran <[email protected]>

---------

Signed-off-by: Ashwin P Chandran <[email protected]>
Co-authored-by: Josh Romero <[email protected]>
(cherry picked from commit ee32d20)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
joshuarrrr added a commit that referenced this pull request Apr 18, 2023
…to embeddable, bug fixes (#3874)

* [VisBuilder] Adds UIState to vis, adds index patterns to embeddable, bug fixes (#3751)

* adds uiActions to visBuilder
* prevents multiple errors on load
* fixes visbuilder type errors
* fixes save
* adds ui state to vis builder
* fixes tests
* Adds support in embeddables for multiple indices
* Moves ui state to separate slice
* Adds changelog

Signed-off-by: Ashwin P Chandran <[email protected]>

---------

Signed-off-by: Ashwin P Chandran <[email protected]>
Co-authored-by: Josh Romero <[email protected]>
(cherry picked from commit ee32d20)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

* add changelog

Signed-off-by: Josh Romero <[email protected]>

---------

Signed-off-by: Josh Romero <[email protected]>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Josh Romero <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants