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

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

Merged

Conversation

davismcphee
Copy link
Contributor

@davismcphee davismcphee commented Dec 2, 2022

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

For maintainers

@davismcphee davismcphee added Feature:Discover Discover Application enhancement New value added to drive a business result Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) labels Dec 2, 2022
@davismcphee davismcphee self-assigned this Dec 2, 2022
@davismcphee davismcphee force-pushed the enhancement-saved-searches-by-value branch from fbb86ec to dacae99 Compare June 16, 2023 04:32
@davismcphee davismcphee changed the title [PoC] Saved searches by value [Saved Searches] Add support for saved searches by value Jul 14, 2023
@davismcphee 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 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 davismcphee force-pushed the enhancement-saved-searches-by-value branch from 6dd7bfc to 1614130 Compare July 24, 2023 01:15
): EmbeddableStateWithType => {
if (hasAttributes(state)) {
// Filter out references that are not in the state
// https://github.com/elastic/kibana/pull/119079
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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:

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah of course, that makes a lot of sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

export type SavedSearchByValueAttributes = Omit<SavedSearchAttributes, 'description'> & {
description?: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@davismcphee davismcphee marked this pull request as ready for review July 25, 2023 01:54
@davismcphee davismcphee requested review from a team as code owners July 25, 2023 01:54
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

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!

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

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

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

@davismcphee
Copy link
Contributor Author

@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:
visualize_library

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

@kertal kertal requested a review from a team July 31, 2023 05:46
@ThomThomson
Copy link
Contributor

ThomThomson commented Jul 31, 2023

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.

private prevTimeRange?: TimeRange;
private prevFilters?: Filter[];
private prevQuery?: Query;
private prevSort?: SortOrder[];
private prevSearchSessionId?: string;
private searchProps?: SearchProps;

private isInitialized?: boolean;
private isDestroyed?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Member

kertal commented Jul 31, 2023

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 👍

@davismcphee davismcphee requested a review from a team as a code owner August 1, 2023 00:17
Copy link
Contributor Author

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

@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.

private prevTimeRange?: TimeRange;
private prevFilters?: Filter[];
private prevQuery?: Query;
private prevSort?: SortOrder[];
private prevSearchSessionId?: string;
private searchProps?: SearchProps;

private isInitialized?: boolean;
private isDestroyed?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Dashboard copy changes LGTM!

Copy link
Contributor Author

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

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.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

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

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

cc @davismcphee

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

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? 😄

@davismcphee davismcphee merged commit 716fb14 into elastic:main Aug 2, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 2, 2023
@davismcphee davismcphee deleted the enhancement-saved-searches-by-value branch August 2, 2023 13:41
davismcphee added a commit that referenced this pull request Oct 26, 2023
… 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)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 26, 2023
… 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)
bryce-b pushed a commit to bryce-b/kibana that referenced this pull request Oct 30, 2023
… 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)
awahab07 pushed a commit to awahab07/kibana that referenced this pull request Oct 31, 2023
… 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting enhancement New value added to drive a business result Feature:Discover Discover Application Feature:Embeddables Relating to the Embeddable system release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Discover] Add support for saved searches by value
7 participants