[Saved Searches] Add support for saved searches by value #146849

davismcphee · 2022-12-02T02:27:15Z

Summary

This PR adds support for saved searches by value. Functionality-wise this means that similar to other Dashboard panels, saved search panels now allow unlinking from the library as well as cloning by value instead of by reference.

Testing guide:

Test saved searches using both persisted and temporary data views.
Ensure your saved searches include a query, filters, time range, columns, sorting, breakdown field, etc.
Test both the "Unlink from library" and "Save to library" functionality.
Test "Clone panel" functionality using both by reference and by value saved searches (both should clone to a by value saved search).
Test the "Edit search" button functionality in Dashboard edit mode:
- All saved search configurations should be included when navigating (search params + persisted & temporary data views).
- Test navigation within the same tab (which will use in-app navigation to pass state) and opening the link in a new tab (which will use query params to pass state).
- By reference saved searches should use the /app/discover#/view/{savedSearchId} route.
- By value saved searches using persisted data views should pass all saved search configurations through the app state (_a) query param.
- By value saved searches using temporary data views should use a locator redirect URL (/app/r?l=DISCOVER_APP_LOCATOR...) in order to support encoding their temporary data view in the URL state.
Test the "Open in Discover" button functionality in Dashboard view mode:
- By reference saved searches should open the actual saved search in Discover.
- By value saved searches should pass all saved search configurations to Discover.

The following features are not included in this PR and comprise the remaining work for implementing Time to Visualize for saved searches (to be done at a later date; issue here: #141629):

Save and return / state transfer service.
Save modal with the ability to save directly to a dashboard.

Resolves #148995.
Unblocks #158632.

Checklist

~~Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)~~
~~Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)~~
~~If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list~~
~~This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)~~
~~This was checked for cross-browser compatibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately

davismcphee · 2023-07-25T01:33:22Z

src/plugins/discover/common/embeddable/search_inject_extract.ts

+): EmbeddableStateWithType => {
+  if (hasAttributes(state)) {
+    // Filter out references that are not in the state
+    // https://github.com/elastic/kibana/pull/119079


@ThomThomson It seems like we have to use the same approach to filtering references as the Lens inject function due to the same issue outlined in #119079. When we have a by value saved search using a temporary data view, the saved search has no references which seems to result in the Dashboard inject code passing in all dashboard references instead:

kibana/src/plugins/dashboard/common/dashboard_container/persistable_state/dashboard_container_references.ts

Lines 41 to 45 in 0d8a2da

const filteredReferences = references

.filter((reference) => reference.name.indexOf(prefix) === 0)

.map((reference) => ({ ...reference, name: reference.name.replace(prefix, '') }));

const panelReferences = filteredReferences.length === 0 ? references : filteredReferences;

Let me know if this makes sense to you or if I'm missing something here.

Ah of course, that makes a lot of sense!

davismcphee · 2023-07-25T01:36:17Z

src/plugins/discover/public/embeddable/saved_search_embeddable.tsx

Since I had to make so many changes to this anyway, I also did some general cleanup to the embeddable to improve the organization a bit.

davismcphee · 2023-07-25T01:45:58Z

src/plugins/saved_search/public/services/saved_searches/saved_search_attribute_service.ts

Since the SavedSearchAttributeService requires a serializable model and SavedSearch isn't serializable due to SearchSource, I'm using a similar approach as Lens and Visualizations by introducing a serializable model called SavedSearchByValueAttributes that is essentially the raw SO attributes with the references tucked in.

davismcphee · 2023-07-25T01:48:18Z

src/plugins/saved_search/public/services/saved_searches/types.ts

+}
+
+export type SavedSearchByValueAttributes = Omit<SavedSearchAttributes, 'description'> & {
+  description?: string;


description must be optional to conform to the expected by value input interface.

elasticmachine · 2023-07-25T01:54:05Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

ThomThomson

Looked through the code and ran through the test cases locally on chrome.

Everything looks inline with the other implementations of Time to Visualize, and the local testing went well. Nice jest test additions also!

…-ref HEAD~1..HEAD --fix'

…fix'

…-fix'

…fix'

kertal

Had a quick incomplete look at it, and wanted to share 2 minor things I've found while testing

It seems when editing a unlinked saved search, the sorting is lost: https://github.com/elastic/kibana/assets/463851/5322bb26-4b47-45bd-9983-e0e0726a30dc
Secondly, when unlinking a saved search from it's library item, there's a toast telling the user that it was unlinked of the visualize library ... guess this wording should be more generic, even it's an honor to be mentioned by a such great library

Kapture.2023-07-28.at.20.15.42.mp4

davismcphee · 2023-07-31T00:15:45Z

@kertal Thanks for the initial feedback!

It seems when editing a unlinked saved search, the sorting is lost: https://github.com/elastic/kibana/assets/463851/5322bb26-4b47-45bd-9983-e0e0726a30dc

I think this may be how it should work. Currently the behaviour is the same for by reference saved searches too because the edit button takes the user to the last persisted state of the saved search, not the current dashboard state. For by value saved searches this might not matter currently since we don't have the "Save and return" functionality yet, but I think once we do it would make sense to use the last persisted state when editing to prevent implicitly modifying the saved search state when navigating to Discover. WDYT?

Secondly, when unlinking a saved search from it's library item, there's a toast telling the user that it was unlinked of the visualize library ... guess this wording should be more generic, even it's an honor to be mentioned by a such great library

This makes sense to me and I started making the change, but I noticed Dashboard seems to refer to the entire library as the "Visualize Library" in multiple places in its copy:

It seems like this may be intentional, but maybe @ThomThomson can help clarify this?

ThomThomson · 2023-07-31T14:31:46Z

I think this naming was just something that happened naturally more-or-less resulting from a confusion about Library vs Visualize Library. In a lot of places we specifically called actions add to Library to be more generic, but we didn't get everything.

IMO we should do a quick once-over to rename everything from visualize library to simply library. @amyjtechwriter FYI.

kertal · 2023-07-31T17:19:13Z

src/plugins/discover/public/embeddable/saved_search_embeddable.tsx

  private prevTimeRange?: TimeRange;
  private prevFilters?: Filter[];
  private prevQuery?: Query;
  private prevSort?: SortOrder[];
  private prevSearchSessionId?: string;
  private searchProps?: SearchProps;
-
+  private isInitialized?: boolean;
+  private isDestroyed?: boolean;


QQ, why not re-using destroyed, which is contains the same functionality and is inherited?

kibana/src/plugins/embeddable/public/lib/embeddables/embeddable.tsx

Line 63 in 3e3419d

protected destroyed: boolean = false;

And in case this is a good idea, to align naming isInitialized could be initialized here ... just for alignment, ... I personally prefer the is... naming

No good reason 🙂 I used isDestroyed initially because the Lens embeddable I based this work on uses it's own isDestroyed property, but it seems like destroyed works just as well. Updated and switched to initialized as well here: f9ee121.

kertal · 2023-07-31T17:22:16Z

It seems when editing a unlinked saved search, the sorting is lost: https://github.com/elastic/kibana/assets/463851/5322bb26-4b47-45bd-9983-e0e0726a30dc

I think this may be how it should work. Currently the behaviour is the same for by reference saved searches too because the edit button takes the user to the last persisted state of the saved search, not the current dashboard state. For by value saved searches this might not matter currently since we don't have the "Save and return" functionality yet, but I think once we do it would make sense to use the last persisted state when editing to prevent implicitly modifying the saved search state when navigating to Discover. WDYT?

Thx, makes sense 👍

…troyed, also update isInitialized to initialized

davismcphee

@ThomThomson I did a pass at replacing all instances of "Visualization Library" with "library" in Dashboard here: 52160a6. Would you mind doing another quick review of this PR for these changes? Or I can split that commit into a separate PR if you'd prefer not to have it lumped in with these changes.

davismcphee · 2023-08-01T00:19:57Z

src/plugins/discover/public/embeddable/saved_search_embeddable.tsx

  private prevTimeRange?: TimeRange;
  private prevFilters?: Filter[];
  private prevQuery?: Query;
  private prevSort?: SortOrder[];
  private prevSearchSessionId?: string;
  private searchProps?: SearchProps;
-
+  private isInitialized?: boolean;
+  private isDestroyed?: boolean;


No good reason 🙂 I used isDestroyed initially because the Lens embeddable I based this work on uses it's own isDestroyed property, but it seems like destroyed works just as well. Updated and switched to initialized as well here: f9ee121.

kertal

Code LGTM 👍 All looking pretty good from my side, just one last thing:

opening the link in a new tab (which will use query params to pass state).

Thx for this guide, else I wouldn't have thought to do this, it seems there's problem in my local dev mode, because the link I get is:

...http://localhost:5601/**xnf/xnf/**app/...

With Lens it seems to work

src/plugins/saved_search/public/services/saved_searches/types.ts

src/plugins/saved_search/public/services/saved_searches/create_get_saved_search_deps.ts

ThomThomson

Dashboard copy changes LGTM!

davismcphee

Thx for this guide, else I wouldn't have thought to do this, it seems there's problem in my local dev mode, because the link I get is:

...http://localhost:5601/**xnf/xnf/**app/...

With Lens it seems to work

@kertal Great catch! That'll teach me not to run with --no-base-path 🙂 Looks like I had things a bit backwards and was double prepending the base path. Fixed here: 13cf1fa.

src/plugins/saved_search/public/services/saved_searches/create_get_saved_search_deps.ts

src/plugins/saved_search/public/services/saved_searches/types.ts

kibana-ci · 2023-08-02T00:50:21Z

💚 Build Succeeded

Buildkite Build
Commit: 13cf1fa

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`discover`	605	608	+3
`savedSearch`	18	47	+29
total			+32

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`discover`	60	51	-9
`savedSearch`	61	71	+10
total			+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`dashboard`	358.6KB	358.5KB	-70.0B
`discover`	547.9KB	549.0KB	+1.0KB
total			+999.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`discover`	31.0KB	31.6KB	+601.0B
`savedSearch`	9.4KB	10.9KB	+1.5KB
total			+2.1KB

Unknown metric groups

API count

id	before	after	diff
`discover`	86	77	-9
`savedSearch`	62	72	+10
total			+1

References to deprecated APIs

id	before	after	diff
`discover`	44	45	+1

History

💚 Build #146597 succeeded 86dc808
💔 Build #146411 failed 44f3621
💔 Build #146249 failed 361d410
💔 Build #146240 failed 52160a6
💚 Build #145364 succeeded 2464c63
💚 Build #145253 succeeded 87ffe57

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @davismcphee

kertal

Thx for the last fix, works as expected now ... LGTM, thx for taking care of this and exciting about next steps, which are now possible

@kertal Great catch! That'll teach me not to run with --no-base-path 🙂 Looks like I had things a bit backwards and was double prepending the base path.

How do you work without having the little pleasure of a random base path like wtf, omg or ice being added to your URL? Or if you're lucky like me recently spalger? 😄

… deleted saved search can't be removed (#169896) ## Summary This PR fixes an issue where an undefined reference error makes it so that Dashboard panels targeting deleted saved searches cannot be removed. The bug was likely introduced in 8.10 with embeddable changes in #146849. Fixes #169829. ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

… deleted saved search can't be removed (elastic#169896) ## Summary This PR fixes an issue where an undefined reference error makes it so that Dashboard panels targeting deleted saved searches cannot be removed. The bug was likely introduced in 8.10 with embeddable changes in elastic#146849. Fixes elastic#169829. ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit 8c93e5c)

davismcphee added Feature:Discover Discover Application enhancement New value added to drive a business result Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Dec 2, 2022

davismcphee self-assigned this Dec 2, 2022

This was referenced Jan 16, 2023

[Discover] Add support for saved searches by value #148995

Closed

[Discover] Research support for 'by value' saved searches #143542

Closed

jughosta mentioned this pull request Mar 1, 2023

Editing a panel filter on a search panel adds global filter to the same panel upon navigating back to dashboard #152375

Closed

davismcphee force-pushed the enhancement-saved-searches-by-value branch from fbb86ec to dacae99 Compare June 16, 2023 04:32

davismcphee changed the title ~~[PoC] Saved searches by value~~ [Saved Searches] Add support for saved searches by value Jul 14, 2023

davismcphee added Feature:Embeddables Relating to the Embeddable system release_note:skip Skip the PR/issue when compiling release notes labels Jul 14, 2023

davismcphee force-pushed the enhancement-saved-searches-by-value branch 2 times, most recently from 1a57e78 to b3f27a3 Compare July 19, 2023 20:55

davismcphee force-pushed the enhancement-saved-searches-by-value branch from 6dd7bfc to 1614130 Compare July 24, 2023 01:15

davismcphee commented Jul 25, 2023

View reviewed changes

davismcphee added the v8.10.0 label Jul 25, 2023

davismcphee marked this pull request as ready for review July 25, 2023 01:54

davismcphee requested review from a team as code owners July 25, 2023 01:54

davismcphee requested a review from ThomThomson July 25, 2023 01:54

ThomThomson approved these changes Jul 26, 2023

View reviewed changes

davismcphee and others added 7 commits July 26, 2023 19:36

[Discover] Initial implementation of saved searches by value

dc0c67f

Finish initial implementation of saved searches by value

0340368

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

ac280fa

…-ref HEAD~1..HEAD --fix'

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

3da5211

…fix'

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

09f942c

…-fix'

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

33aaa6a

…fix'

Change savedSearch.embeddable to savedSearch.byValue

9a66a71

kertal reviewed Jul 28, 2023

View reviewed changes

kertal requested a review from a team July 31, 2023 05:46

kertal reviewed Jul 31, 2023

View reviewed changes

davismcphee added 3 commits July 31, 2023 20:34

Merge branch 'main' into enhancement-saved-searches-by-value

9c78dc2

Remove isDestroyed from saved search embeddable and replaced with des…

f9ee121

…troyed, also update isInitialized to initialized

Update Dashboard copy to change 'Visualization Library' to 'library'

52160a6

davismcphee requested a review from a team as a code owner August 1, 2023 00:17

davismcphee commented Aug 1, 2023

View reviewed changes

davismcphee added 2 commits July 31, 2023 22:43

Merge branch 'main' into enhancement-saved-searches-by-value

e087529

Update Dashboard Jest test snapshot

361d410

kertal reviewed Aug 1, 2023

View reviewed changes

src/plugins/saved_search/public/services/saved_searches/types.ts Show resolved Hide resolved

src/plugins/saved_search/public/services/saved_searches/create_get_saved_search_deps.ts Show resolved Hide resolved

Merge branch 'main' into enhancement-saved-searches-by-value

44f3621

ThomThomson approved these changes Aug 1, 2023

View reviewed changes

davismcphee added 2 commits August 1, 2023 16:26

Merge branch 'main' into enhancement-saved-searches-by-value

86dc808

Fix saved search embeddable edit link base path

13cf1fa

davismcphee commented Aug 1, 2023

View reviewed changes

src/plugins/saved_search/public/services/saved_searches/create_get_saved_search_deps.ts Show resolved Hide resolved

src/plugins/saved_search/public/services/saved_searches/types.ts Show resolved Hide resolved

kertal approved these changes Aug 2, 2023

View reviewed changes

davismcphee merged commit 716fb14 into elastic:main Aug 2, 2023

kibanamachine added the backport:skip This commit does not require backporting label Aug 2, 2023

davismcphee deleted the enhancement-saved-searches-by-value branch August 2, 2023 13:41

davismcphee mentioned this pull request Oct 25, 2023

[Saved Search] [Embeddable] Fix issue where Dashboard panel targeting deleted saved search can't be removed #169896

Merged

9 tasks

drewdaemon mentioned this pull request Jan 23, 2024

elastic/obs-infraobs-integrations: Address skipped validation SVR00004 (batch 2) elastic/integrations#8938

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Saved Searches] Add support for saved searches by value #146849

[Saved Searches] Add support for saved searches by value #146849

davismcphee commented Dec 2, 2022 •

edited

Loading

davismcphee Jul 25, 2023

ThomThomson Jul 25, 2023

davismcphee Jul 25, 2023

davismcphee Jul 25, 2023

davismcphee Jul 25, 2023

elasticmachine commented Jul 25, 2023

ThomThomson left a comment

kertal left a comment

davismcphee commented Jul 31, 2023

ThomThomson commented Jul 31, 2023 •

edited

Loading

kertal Jul 31, 2023

davismcphee Aug 1, 2023

kertal commented Jul 31, 2023

davismcphee left a comment

davismcphee Aug 1, 2023

kertal left a comment

ThomThomson left a comment

davismcphee left a comment

kibana-ci commented Aug 2, 2023

API count

References to deprecated APIs

kertal left a comment

	const filteredReferences = references
	.filter((reference) => reference.name.indexOf(prefix) === 0)
	.map((reference) => ({ ...reference, name: reference.name.replace(prefix, '') }));

	const panelReferences = filteredReferences.length === 0 ? references : filteredReferences;

[Saved Searches] Add support for saved searches by value #146849

[Saved Searches] Add support for saved searches by value #146849

Conversation

davismcphee commented Dec 2, 2022 • edited Loading

Summary

Checklist

For maintainers

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

elasticmachine commented Jul 25, 2023

ThomThomson left a comment

Choose a reason for hiding this comment

kertal left a comment

Choose a reason for hiding this comment

davismcphee commented Jul 31, 2023

ThomThomson commented Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kertal commented Jul 31, 2023

davismcphee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kertal left a comment

Choose a reason for hiding this comment

ThomThomson left a comment

Choose a reason for hiding this comment

davismcphee left a comment

Choose a reason for hiding this comment

kibana-ci commented Aug 2, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

References to deprecated APIs

History

kertal left a comment

Choose a reason for hiding this comment

davismcphee commented Dec 2, 2022 •

edited

Loading

ThomThomson commented Jul 31, 2023 •

edited

Loading