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

[bug] address different issues with dataset selector #8665

Merged
merged 6 commits into from
Oct 19, 2024

Conversation

kavilla
Copy link
Member

@kavilla kavilla commented Oct 19, 2024

Description

Moved the dataset selector back to the search bar and access it by ref in the sidebar.

Avoid out of sync issue.

NOTE: In the empty state it doesn't show the dataset selector in the top left corner.

Issues Resolved

Screenshot

Testing the changes

Changelog

  • skip

Check List

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

Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

Copy link

codecov bot commented Oct 19, 2024

Codecov Report

Attention: Patch coverage is 30.23256% with 30 lines in your changes missing coverage. Please review.

Project coverage is 60.86%. Comparing base (c8b5318) to head (83156d7).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ublic/application/view_components/canvas/index.tsx 0.00% 9 Missing ⚠️
...a/public/ui/dataset_selector/advanced_selector.tsx 0.00% 5 Missing ⚠️
...components/no_index_patterns/no_index_patterns.tsx 20.00% 4 Missing ⚠️
...ta/public/ui/dataset_selector/dataset_selector.tsx 0.00% 3 Missing ⚠️
...rns/index_patterns/ensure_default_index_pattern.ts 33.33% 2 Missing ⚠️
...c/application/components/no_results/no_results.tsx 33.33% 2 Missing ⚠️
...lic/application/view_components/canvas/top_nav.tsx 33.33% 2 Missing ⚠️
...s/data/public/ui/dataset_selector/configurator.tsx 0.00% 1 Missing ⚠️
.../data_explorer/public/components/sidebar/index.tsx 80.00% 0 Missing and 1 partial ⚠️
...er/public/utils/state_management/metadata_slice.ts 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8665      +/-   ##
==========================================
+ Coverage   60.83%   60.86%   +0.02%     
==========================================
  Files        3793     3793              
  Lines       90412    90367      -45     
  Branches    14196    14182      -14     
==========================================
- Hits        55002    54998       -4     
+ Misses      31933    31891      -42     
- Partials     3477     3478       +1     
Flag Coverage Δ
Linux_1 29.09% <10.81%> (+0.01%) ⬆️
Linux_2 56.39% <0.00%> (-0.01%) ⬇️
Linux_3 37.69% <30.23%> (+0.02%) ⬆️
Linux_4 29.82% <0.00%> (+<0.01%) ⬆️
Windows_1 29.11% <10.81%> (+0.01%) ⬆️
Windows_2 56.34% <0.00%> (-0.01%) ⬇️
Windows_3 37.69% <30.23%> (+0.02%) ⬆️
Windows_4 29.82% <0.00%> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

Looks good in general. Cant wait for the rest to be wired back up

Copy link
Member

Choose a reason for hiding this comment

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

Oh nice!

iconSide="right"
iconGap="s"
>
<EuiText size="xs">
Copy link
Member

Choose a reason for hiding this comment

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

Hmm cant we get this from the language config? We can limit it to 4

storage,
appName,
data: {
query: { queryString },
Copy link
Member

Choose a reason for hiding this comment

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

were we not using this?

@@ -323,6 +330,8 @@ export default function QueryEditorTopRow(props: QueryEditorTopRowProps) {
direction="column"
justifyContent="flexEnd"
>
{props?.datasetSelectorRef?.current &&
createPortal(datasetSelector, props.datasetSelectorRef.current)}
Copy link
Member

Choose a reason for hiding this comment

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

not sure i understand why we need a protal, but that can be a fast follow to clean up

Copy link
Member Author

Choose a reason for hiding this comment

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

attempting to keep all the render logic relatively tight together since we got it pretty nailed down a couple weeeks ago. otherwise we will have to pass stateful props which felt like too new dev vs keeping it in the data plugin and portalling it where we want it.

}}
/>
) : (
{isEnhancementEnabled && <div ref={datasetSelectorRef} />}
Copy link
Member

Choose a reason for hiding this comment

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

Why couldnt we just add the DatasetSelector here?

Comment on lines -82 to -84
if (payload?.query) {
setQuery(payload?.query);
}
Copy link
Member

Choose a reason for hiding this comment

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

What did this do?

Comment on lines -142 to -177
const handleDatasetChange = (dataset: Dataset) => {
dispatch(setSelectedDataset(dataset));

// Update query and other necessary state
const queryString = data.query.queryString;
const initialQuery = queryString.getInitialQueryByDataset(dataset);
queryString.setQuery(initialQuery);
queryString.getDatasetService().addRecentDataset(dataset);
};

const handleOpenDataSelector = () => {
const overlay = overlays?.openModal(
toMountPoint(
<AdvancedSelector
services={services}
onSelect={(dataset?: Dataset) => {
overlay?.close();
if (dataset) {
handleDatasetChange(dataset);
}
}}
onCancel={() => overlay?.close()}
selectedDataset={undefined}
setSelectedDataset={setSelectedDataset}
setIndexPattern={setIndexPattern}
dispatch={dispatch}
/>
),
{
maxWidth: false,
className: 'datasetSelector__advancedModal',
}
);
};

const hasNoDataset = !currentIndexPattern && !loadedIndexPattern && isEnhancementsEnabled;
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

this was added for the improved empty state. it ended up setting the selected data set index pattern which gets listened to by the search source and then dispatched. so selected a dataset was causes some state issues.

Moved the dataset selector back to the search bar and access it by ref in the sidebar.

Avoid out of sync issue.

Signed-off-by: Kawika Avilla <[email protected]>
Signed-off-by: Kawika Avilla <[email protected]>
@kavilla kavilla changed the title [WIP][bug] address some issues with dataset selector [bug] address different issues with dataset selector Oct 19, 2024
Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

Signed-off-by: Kawika Avilla <[email protected]>
@kavilla kavilla force-pushed the kavilla/datasetupdates branch from 936a39a to 4515d42 Compare October 19, 2024 14:54
Signed-off-by: Kawika Avilla <[email protected]>
@github-actions github-actions bot added Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry and removed failed changeset labels Oct 19, 2024
@AMoo-Miki AMoo-Miki merged commit e23f332 into opensearch-project:main Oct 19, 2024
72 of 73 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 19, 2024
* [bug] address some issues with dataset selector

Moved the dataset selector back to the search bar and access it by ref in the sidebar.

Avoid out of sync issue.

Signed-off-by: Kawika Avilla <[email protected]>

* update the logic for ensuring index pattern

Signed-off-by: Kawika Avilla <[email protected]>

* dont add ability to open button

Signed-off-by: Kawika Avilla <[email protected]>

* empty state but missing data set selector button

Signed-off-by: Kawika Avilla <[email protected]>

* fix tests

Signed-off-by: Kawika Avilla <[email protected]>

* add back styling

Signed-off-by: Kawika Avilla <[email protected]>

---------

Signed-off-by: Kawika Avilla <[email protected]>
(cherry picked from commit e23f332)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@AMoo-Miki AMoo-Miki mentioned this pull request Oct 19, 2024
7 tasks
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Oct 19, 2024
Also:
* Fix some React errors

Signed-off-by: Miki <[email protected]>

---------

Fix random big number during loading in query editor result (opensearch-project#8650)

* Fix random big number during loading in query editor result

Signed-off-by: abbyhu2000 <[email protected]>

* Changeset file for PR opensearch-project#8650 created/updated

* Fix initial loading number

Signed-off-by: abbyhu2000 <[email protected]>
(cherry picked from commit a7414f0)

---------

[bug] address different issues with dataset selector (opensearch-project#8665)

* [bug] address some issues with dataset selector

Moved the dataset selector back to the search bar and access it by ref in the sidebar.

Avoid out of sync issue.
* update the logic for ensuring index pattern
* dont add ability to open button
* empty state but missing data set selector button
* fix tests
* add back styling

Signed-off-by: Kawika Avilla <[email protected]>
(cherry picked from commit e23f332)

---------

[bug] Discover UI stuck on searching after deleting index pattern (opensearch-project#8659)

* [bug] Discover UI stuck on searching after deleting index pattern

When using Discover with query enhancement enabled, deleting an index pattern from Index Management does not properly update the "Recently selected data" list in Discover. This causes the UI to become stuck in a "Searching" state when attempting to use
Discover after deleting an index pattern.

Handle the error case where the use index patterns hook caught error when
enhancements was enabled.

issue resolved:
opensearch-project#8612

(cherry picked from commit 4808094)

---------

[Discover]Sample Queries and Saved Queries in No Results Page (opensearch-project#8616)

* Sample Queries and Saved Queries in No Results Page

Signed-off-by: Sean Li <[email protected]>
Signed-off-by: Miki <[email protected]>

* Changeset file for PR opensearch-project#8616 created/updated

* Update styling

Signed-off-by: Miki <[email protected]>

(cherry picked from commit 9da1b77)

---------

Improve Empty State Handling: Add No Index Patterns Panel with Data Selection in Discover View (opensearch-project#8613)

* Improve Empty State Handling: Add No Index Patterns Panel with Data Selection in Discover View

This PR primarily addresses the scenario when no index patterns (general) is available in the Discover view.
Instead of redirecting users to the index management page, it introduces a new "No Index Patterns" panel.
This panel provides users with the option to open a data selector and add index patterns
directly from the Discover view, improving the user experience for new or empty deployments.

To achieve, we move the selectedDataset state from ConnectedDatasetSelector to the app container's
state management. This allows the AdvancedSelector, opened from the AppContainer, to update
the dataset state effectively. Key changes include:

* Implementing NoIndexPatternsPanel and AdvancedSelector components.
* Refactoring dataset state management in AppContainer and Sidebar.
* Modifying DiscoverCanvas to conditionally render NoIndexPatternsPanel.
* Updating ConnectedDatasetSelector to use shared state and dataset change handling.

* Update design of no data selected
* use i18n
* fix comments
* Update design of no data selected
* fix lint error

Signed-off-by: Anan Zhuang <[email protected]>

(cherry picked from commit 6659139)

---------

Update Discover appearance (opensearch-project#8651)

* Update Discover appearance

Signed-off-by: Miki <[email protected]>

(cherry picked from commit 17103ba)

---------

Move DatasetSelector from data plugin queryString comp to DataExplorer (opensearch-project#8598)

* Move DatasetSelector to DataExplorer
* Style Disover after moving DatasetSelector to DataExplorer
* fix the test by adding the getUpdates$ method to the mock queryString object

Signed-off-by: Anan Zhuang <[email protected]>

(cherry picked from commit 923cce8)
@AMoo-Miki
Copy link
Collaborator

Manually backported to 2.x with #8670

ashwin-pc pushed a commit to ashwin-pc/OpenSearch-Dashboards that referenced this pull request Oct 19, 2024
…ect#8665)

* [bug] address some issues with dataset selector

Moved the dataset selector back to the search bar and access it by ref in the sidebar.

Avoid out of sync issue.

Signed-off-by: Kawika Avilla <[email protected]>

* update the logic for ensuring index pattern

Signed-off-by: Kawika Avilla <[email protected]>

* dont add ability to open button

Signed-off-by: Kawika Avilla <[email protected]>

* empty state but missing data set selector button

Signed-off-by: Kawika Avilla <[email protected]>

* fix tests

Signed-off-by: Kawika Avilla <[email protected]>

* add back styling

Signed-off-by: Kawika Avilla <[email protected]>

---------

Signed-off-by: Kawika Avilla <[email protected]>
ruanyl pushed a commit that referenced this pull request Oct 22, 2024
…, #8650, #8668  (#8670)

* Fix No data selected appearance (#8668)

Also:
* Fix some React errors

Signed-off-by: Miki <[email protected]>

---------

Fix random big number during loading in query editor result (#8650)

* Fix random big number during loading in query editor result

Signed-off-by: abbyhu2000 <[email protected]>

* Changeset file for PR #8650 created/updated

* Fix initial loading number

Signed-off-by: abbyhu2000 <[email protected]>
(cherry picked from commit a7414f0)

---------

[bug] address different issues with dataset selector (#8665)

* [bug] address some issues with dataset selector

Moved the dataset selector back to the search bar and access it by ref in the sidebar.

Avoid out of sync issue.
* update the logic for ensuring index pattern
* dont add ability to open button
* empty state but missing data set selector button
* fix tests
* add back styling

Signed-off-by: Kawika Avilla <[email protected]>
(cherry picked from commit e23f332)

---------

[bug] Discover UI stuck on searching after deleting index pattern (#8659)

* [bug] Discover UI stuck on searching after deleting index pattern

When using Discover with query enhancement enabled, deleting an index pattern from Index Management does not properly update the "Recently selected data" list in Discover. This causes the UI to become stuck in a "Searching" state when attempting to use
Discover after deleting an index pattern.

Handle the error case where the use index patterns hook caught error when
enhancements was enabled.

issue resolved:
#8612

(cherry picked from commit 4808094)

---------

[Discover]Sample Queries and Saved Queries in No Results Page (#8616)

* Sample Queries and Saved Queries in No Results Page

Signed-off-by: Sean Li <[email protected]>
Signed-off-by: Miki <[email protected]>

* Changeset file for PR #8616 created/updated

* Update styling

Signed-off-by: Miki <[email protected]>

(cherry picked from commit 9da1b77)

---------

Improve Empty State Handling: Add No Index Patterns Panel with Data Selection in Discover View (#8613)

* Improve Empty State Handling: Add No Index Patterns Panel with Data Selection in Discover View

This PR primarily addresses the scenario when no index patterns (general) is available in the Discover view.
Instead of redirecting users to the index management page, it introduces a new "No Index Patterns" panel.
This panel provides users with the option to open a data selector and add index patterns
directly from the Discover view, improving the user experience for new or empty deployments.

To achieve, we move the selectedDataset state from ConnectedDatasetSelector to the app container's
state management. This allows the AdvancedSelector, opened from the AppContainer, to update
the dataset state effectively. Key changes include:

* Implementing NoIndexPatternsPanel and AdvancedSelector components.
* Refactoring dataset state management in AppContainer and Sidebar.
* Modifying DiscoverCanvas to conditionally render NoIndexPatternsPanel.
* Updating ConnectedDatasetSelector to use shared state and dataset change handling.

* Update design of no data selected
* use i18n
* fix comments
* Update design of no data selected
* fix lint error

Signed-off-by: Anan Zhuang <[email protected]>

(cherry picked from commit 6659139)

---------

Update Discover appearance (#8651)

* Update Discover appearance

Signed-off-by: Miki <[email protected]>

(cherry picked from commit 17103ba)

---------

Move DatasetSelector from data plugin queryString comp to DataExplorer (#8598)

* Move DatasetSelector to DataExplorer
* Style Disover after moving DatasetSelector to DataExplorer
* fix the test by adding the getUpdates$ method to the mock queryString object

Signed-off-by: Anan Zhuang <[email protected]>

(cherry picked from commit 923cce8)

* Resolve merge errors from manual backports

Signed-off-by: Miki <[email protected]>

---------

Signed-off-by: Miki <[email protected]>
Co-authored-by: Anan Zhuang <[email protected]>
Qxisylolo pushed a commit to Qxisylolo/OpenSearch-Dashboards that referenced this pull request Oct 30, 2024
…ect#8665)

* [bug] address some issues with dataset selector

Moved the dataset selector back to the search bar and access it by ref in the sidebar.

Avoid out of sync issue.

Signed-off-by: Kawika Avilla <[email protected]>

* update the logic for ensuring index pattern

Signed-off-by: Kawika Avilla <[email protected]>

* dont add ability to open button

Signed-off-by: Kawika Avilla <[email protected]>

* empty state but missing data set selector button

Signed-off-by: Kawika Avilla <[email protected]>

* fix tests

Signed-off-by: Kawika Avilla <[email protected]>

* add back styling

Signed-off-by: Kawika Avilla <[email protected]>

---------

Signed-off-by: Kawika Avilla <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discover-next distinguished-contributor Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry v2.18.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants