[Data Explorer][Discover 2.0] restore single and surroundings doc view #4816

ananzh · 2023-08-24T22:08:48Z

Description

add initial route logic to single/surroundings doc view and re-organize files
restore surrounding doc view comp

restore single doc view comp

Issues Resolved

#4231
#4230

Check List

codecov · 2023-08-24T22:29:58Z

Codecov Report

Merging #4816 (a3a041d) into feature/data-explorer (521f306) will decrease coverage by 0.17%.
Report is 12 commits behind head on feature/data-explorer.
The diff coverage is 75.72%.

❗ Current head a3a041d differs from pull request most recent head f381e0d. Consider uploading reports for the commit f381e0d to get more accurate results

@@                    Coverage Diff                    @@
##           feature/data-explorer    #4816      +/-   ##
=========================================================
- Coverage                  66.50%   66.33%   -0.17%     
=========================================================
  Files                       3289     3396     +107     
  Lines                      62821    64851    +2030     
  Branches                    9788    10349     +561     
=========================================================
+ Hits                       41782    43022    +1240     
- Misses                     18658    19271     +613     
- Partials                    2381     2558     +177

Flag	Coverage Δ
Linux_1	`34.89% <82.00%> (?)`
Linux_2	`55.13% <77.22%> (?)`
Linux_3	`44.03% <81.63%> (?)`
Linux_4	`35.10% <82.00%> (?)`
Windows	`?`
Windows_1	`34.91% <82.00%> (?)`
Windows_2	`55.10% <77.22%> (?)`
Windows_3	`44.04% <81.63%> (?)`
Windows_4	`35.10% <82.00%> (?)`

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

Files Changed	Coverage Δ
packages/osd-babel-preset/node_preset.js	`100.00% <ø> (ø)`
...osd-opensearch-archiver/src/lib/docs/test_stubs.ts	`68.75% <ø> (ø)`
...ch-archiver/src/lib/indices/create_index_stream.ts	`69.76% <ø> (ø)`
...ch-archiver/src/lib/indices/delete_index_stream.ts	`50.00% <ø> (ø)`
...ver/src/lib/indices/opensearch_dashboards_index.ts	`0.00% <0.00%> (ø)`
packages/osd-pm/src/utils/package_json.ts	`66.66% <ø> (ø)`
packages/osd-ui-shared-deps/theme.ts	`41.66% <0.00%> (ø)`
src/core/public/chrome/chrome_service.mock.ts	`100.00% <ø> (ø)`
...c/core/public/chrome/ui/header/collapsible_nav.tsx	`95.45% <ø> (+0.90%)`	⬆️
src/core/public/chrome/ui/header/header.tsx	`75.00% <ø> (+1.66%)`	⬆️
... and 37 more

... and 323 files with indirect coverage changes

ashwin-pc

Most of the big changes look good. Just 3 callouts:

Lets keep the doc_view and doc_view_links services where they were initially. The are used more outside of the context and view single doc vies anyways and are not actually components but services.
Can you move doc_views_components to components/doc_views? Having all components be in the same place is how we've had it so far.
The most important change is loading the legacy doc view URL's and context URL's correctly. And also only loading the new experience when the legacy discover is turned off for these two pages. See my comment on that for more details.

ashwin-pc · 2023-08-26T00:49:18Z

src/plugins/discover/public/plugin.ts

+        if (path.startsWith('#/context') || path.startsWith('#/doc')) {
+          const { renderDocView } = await import('./application/doc_views_components');
+          const unmount = renderDocView(params.element);
+          return () => {
+            unmount();
+          };
+        } else if (!v2Enabled) {


This should only happen if the legacy app is turned off. You can move this to the else condition since this view is only for the new discover pages.

Also use the migrateUrlState function to migrate legacy context and single doc view URLs correctly to the new URL's

ashwin-pc · 2023-08-26T00:52:03Z

src/plugins/discover/public/opensearch_dashboards_services.ts

+import { DocViewsRegistry } from './application/doc_views_components/doc_views/doc_views_registry';
+import { DocViewsLinksRegistry } from './application/doc_views_components/doc_views_links/doc_views_links_registry';


Do we need to move this? cant we keep all the components inside the components folder? Thats a pattern we have followed so far.

ashwin-pc · 2023-08-26T00:53:37Z

src/plugins/discover/public/application/view_components/utils/use_search.ts

@@ -14,7 +14,7 @@ import { search } from '../../../../../data/public';
 import { validateTimeRange } from '../../helpers/validate_time_range';
 import { updateSearchSource } from './update_search_source';
 import { useIndexPattern } from './use_index_pattern';
-import { OpenSearchSearchHit } from '../../doc_views/doc_views_types';
+import { OpenSearchSearchHit } from '../../doc_view_components/doc_views/doc_views_types';


Same here. I want to keep the diff as small as possible, so i'd like to avoid moving things unless its necessary.

ashwin-pc · 2023-08-26T00:53:48Z

src/plugins/discover/public/application/view_components/canvas/discover_table.tsx

@@ -19,7 +19,7 @@ import {
 } from '../../utils/state_management';
 import { ResultStatus, SearchData } from '../utils/use_search';
 import { IndexPatternField, opensearchFilters } from '../../../../../data/public';
-import { DocViewFilterFn } from '../../doc_views/doc_views_types';
+import { DocViewFilterFn } from '../../doc_views_components/doc_views/doc_views_types';


Same here. I want to keep the diff as small as possible, so i'd like to avoid moving things unless its necessary.

ashwin-pc · 2023-08-26T00:54:16Z

src/plugins/discover/public/application/doc_views_components/surrounding_docs_view.tsx

can we move this to components?

ashwin-pc · 2023-08-26T00:59:06Z

src/plugins/discover/public/application/components/doc_viewer_links/doc_viewer_links.tsx

+  const handleUrlClick = (url) => {
+    // split the pathname into segments
+    const urlSegments = window.location.href.split('/');
+
+    // find the index of the "data-explorer" segment
+    const indexOfDataExplorerInUrl = urlSegments.indexOf('data-explorer');
+
+    // if "data-explorer" is found, remove it from the array
+    if (indexOfDataExplorerInUrl !== -1) {
+      urlSegments.splice(indexOfDataExplorerInUrl, 1);
+    }
+
+    // Create a new URL object from the current location
+    const newUrl = urlSegments.join('/');
+    const updatedUrlSegments = newUrl.split('#');
+    updatedUrlSegments[1] = url;
+    const updatedUrl = updatedUrlSegments.join('');
+    window.location.href = updatedUrl;
+  };


You dont need to do this. In src/plugins/discover/public/plugin.ts the different docViewLinks are registered. There you can update the generateCb function to create the URL you need.

ashwin-pc · 2023-08-26T01:00:41Z

src/plugins/discover/public/application/doc_views_components/context/NOTES.md

+  |  v
+  |  |* state
+  |  v
+  |  |* angular templates render state


You may want to do a quick update of this file. It still talks about the angular code.

will update the whole file

ashwin-pc · 2023-08-26T01:05:49Z

src/plugins/discover/public/application/doc_views_components/context/api/context.ts

+  if (typeof anchor !== 'object' || anchor === null || !size) {
+    return [];
+  }
+  const timeField = indexPattern.timeFieldName!;


Wont this be undefined if the indexPattern does not have a timeField.

In the angular implimentation, this is called as timeField in this function but sortField in src/plugins/discover_legacy/public/application/angular/context/query/actions.js. SortField makes sense since you likely wont always have a timeField.

I think it is okay for now. For index pattern without a timeField, we only have view single doc which won't use context fetch to re-fetch data.

But the legacy app has view surrounding docs for index patterns without timestamps

ashwin-pc · 2023-08-26T01:21:27Z

src/plugins/discover/public/application/doc_views_components/context/utils/use_query_actions.ts

+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ *
+ * Any modifications Copyright OpenSearch Contributors. See
+ * GitHub history for details.
+ */
+
+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */


Cant this be the new license? looks like you have modified quite a bit here

ashwin-pc · 2023-08-26T01:22:02Z

src/plugins/discover/public/application/doc_views_components/context/utils/use_query_actions.ts

+import { Filter } from '../../../../../../../../src/plugins/data/public';
+import { fetchAnchor } from '../api/anchor';
+import { OpenSearchHitRecord, fetchSurroundingDocs } from '../api/context';
+// import { MarkdownSimple, toMountPoint } from '../../../../../../opensearch_dashboards_react/public';


ashwin-pc

The change looks good to me. The two major changes are:

The imports for DocViewFilterFn are incorrect, looks like you added an extra ../ in most cases
I have a suggested fix for the breadcrumb issue, can you make the change?

ashwin-pc · 2023-08-29T22:03:49Z

src/plugins/discover/public/application/components/doc_views/surrounding_docs_view.tsx

+  const contextAppMemoized = useMemo(
+    () => (
+      <ContextApp
+        onAddFilter={onAddFilter as DocViewFilterFn}
+        rows={rows}
+        indexPattern={indexPattern}
+        setAppState={setContextAppState}
+        contextQueryState={contextQueryState}
+        appState={contextAppState}
+      />
+    ),
+    [onAddFilter, rows, indexPattern, setContextAppState, contextQueryState, contextAppState]
+  );


nit: do we need this memoization?

ashwin-pc · 2023-08-29T22:04:37Z

src/plugins/discover/public/migrate_state.ts

+    // so we can use the saved legacy path in new discover
+    case `doc`:
+    case `context`:
+      path = oldPath;


Have you tested this?

ashwin-pc · 2023-08-29T23:08:16Z

src/plugins/discover/public/application/components/doc_views/surrounding_docs_app.tsx

+  const {
+    services: { chrome, indexPatterns },
+  } = useOpenSearchDashboards<DiscoverServices>();


This should fix the broken breadcrumb issue. You will need to do the same for the single doc view

Suggested change

const {

services: { chrome, indexPatterns },

} = useOpenSearchDashboards<DiscoverServices>();

const {

services: {

chrome,

indexPatterns,

core: {

application: { getUrlForApp },

},

},

} = useOpenSearchDashboards<DiscoverServices>();

const baseUrl = getUrlForApp(PLUGIN_ID);

And then down below where you set the breadcrumbs you can do:

...getRootBreadcrumbs(baseUrl),

Then in src/plugins/discover/public/application/helpers/breadcrumbs.ts update rootBreadcrumb as follows:

export function getRootBreadcrumbs(baseURL: string) { return [ { text: i18n.translate('discover.rootBreadcrumb', { defaultMessage: 'Discover', }), href: baseURL, }, ]; }

ashwin-pc · 2023-08-29T23:14:31Z

src/plugins/discover/public/application/components/data_grid/data_grid_table.tsx

@@ -12,9 +12,9 @@ import { DocViewExpandButton } from './data_grid_table_docview_expand_button';
 import { DataGridFlyout } from './data_grid_table_flyout';
 import { DiscoverGridContextProvider } from './data_grid_table_context';
 import { toolbarVisibility } from './constants';
-import { DocViewFilterFn } from '../../doc_views/doc_views_types';
+import { DocViewFilterFn } from '../../../doc_views/doc_views_types';


My linter is showing that these paths are wrong, can you look at it? Seems to be the case wherever you have updated this path

ananzh · 2023-08-30T00:06:25Z

The change looks good to me. The two major changes are:

The imports for DocViewFilterFn are incorrect, looks like you added an extra ../ in most cases

I have a suggested fix for the breadcrumb issue, can you make the change?

@ashwin-pc thanks for the review. I am double check all the import path and take the breadcrumb change.

ashwin-pc

LGTM :)

The base branch was changed.

* add initial route logic to single/surroundings doc view and re-organize files * restore surrounding doc view comp * restore single doc view comp Signed-off-by: ananzh <ananzh@amazon.com>

opensearch-trigger-bot · 2023-08-30T19:36:03Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-4816-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 4aa6d37b5ccee963c840ff9d1457e4265e4b4d96
# Push it to GitHub
git push --set-upstream origin backport/backport-4816-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-4816-to-2.x.

opensearch-project#4816) * add initial route logic to single/surroundings doc view and re-organize files * restore surrounding doc view comp * restore single doc view comp Signed-off-by: ananzh <ananzh@amazon.com> (cherry picked from commit 4aa6d37)

ananzh requested review from kavilla, seanneumann, AMoo-Miki, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, kristenTian, zhongnansu, manasvinibs, ZilongX, Flyingliuhub, BSFishy and curq as code owners August 24, 2023 22:08

github-actions bot added the distinguished-contributor label Aug 24, 2023

ananzh mentioned this pull request Aug 24, 2023

Feature/data explorer surroundings #4740

Closed

8 tasks

ananzh mentioned this pull request Aug 25, 2023

[Data Explorer][Discover 2.0] Fix display for index pattern without a default time field #4821

Merged

8 tasks

ashwin-pc reviewed Aug 26, 2023

View reviewed changes

ashwin-pc added discover for discover reinvent de-angular de-angularize work data explorer Issues related to the Data Explorer project labels Aug 28, 2023

ananzh force-pushed the feature/data-explorer-single-surroundings branch from 8563a6e to 160ddad Compare August 29, 2023 19:45

ashwin-pc reviewed Aug 29, 2023

View reviewed changes

ashwin-pc added the v2.10.0 label Aug 29, 2023

ananzh force-pushed the feature/data-explorer-single-surroundings branch from a3a041d to f381e0d Compare August 30, 2023 00:46

ashwin-pc previously approved these changes Aug 30, 2023

View reviewed changes

ananzh changed the base branch from feature/data-explorer to main August 30, 2023 16:43

[Data Explorer][Discover 2.0] restore single and surroundings doc view

6234ce5

* add initial route logic to single/surroundings doc view and re-organize files * restore surrounding doc view comp * restore single doc view comp Signed-off-by: ananzh <ananzh@amazon.com>

ashwin-pc added the backport 2.x label Aug 30, 2023

ananzh force-pushed the feature/data-explorer-single-surroundings branch from f381e0d to 6234ce5 Compare August 30, 2023 16:45

ashwin-pc added the Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry label Aug 30, 2023

ashwin-pc approved these changes Aug 30, 2023

View reviewed changes

abbyhu2000 approved these changes Aug 30, 2023

View reviewed changes

ananzh merged commit 4aa6d37 into opensearch-project:main Aug 30, 2023
54 of 57 checks passed

opensearch-trigger-bot bot added the failed backport label Aug 30, 2023

ashwin-pc mentioned this pull request Aug 31, 2023

[Manual Backport 2.x] Combined backport of Discover 2.0 commits #4872

Merged

ashwin-pc removed the failed backport label Aug 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Data Explorer][Discover 2.0] restore single and surroundings doc view #4816

[Data Explorer][Discover 2.0] restore single and surroundings doc view #4816

ananzh commented Aug 24, 2023

codecov bot commented Aug 24, 2023 •

edited

Loading

ashwin-pc left a comment •

edited

Loading

ashwin-pc Aug 26, 2023

ashwin-pc Aug 26, 2023

ashwin-pc Aug 26, 2023

ashwin-pc Aug 26, 2023

ashwin-pc Aug 26, 2023

ashwin-pc Aug 26, 2023

ashwin-pc Aug 26, 2023

ananzh Aug 28, 2023

ashwin-pc Aug 26, 2023

ashwin-pc Aug 26, 2023

ananzh Aug 28, 2023

ashwin-pc Aug 29, 2023

ashwin-pc Aug 26, 2023

ashwin-pc Aug 26, 2023

ashwin-pc left a comment •

edited

Loading

ashwin-pc Aug 29, 2023 •

edited

Loading

ashwin-pc Aug 29, 2023

ananzh Aug 30, 2023

ashwin-pc Aug 29, 2023

ashwin-pc Aug 29, 2023

ananzh commented Aug 30, 2023

ashwin-pc left a comment

opensearch-trigger-bot bot commented Aug 30, 2023

		import { DocViewsRegistry } from './application/doc_views_components/doc_views/doc_views_registry';
		import { DocViewsLinksRegistry } from './application/doc_views_components/doc_views_links/doc_views_links_registry';

[Data Explorer][Discover 2.0] restore single and surroundings doc view #4816

[Data Explorer][Discover 2.0] restore single and surroundings doc view #4816

Conversation

ananzh commented Aug 24, 2023

Description

Issues Resolved

Check List

codecov bot commented Aug 24, 2023 • edited Loading

Codecov Report

ashwin-pc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashwin-pc left a comment • edited Loading

Choose a reason for hiding this comment

ashwin-pc Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ananzh commented Aug 30, 2023

ashwin-pc left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Aug 30, 2023

codecov bot commented Aug 24, 2023 •

edited

Loading

ashwin-pc left a comment •

edited

Loading

ashwin-pc left a comment •

edited

Loading

ashwin-pc Aug 29, 2023 •

edited

Loading