-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Discover] Fix legacy sort saved search stored in dashboard saved objects #137488
[Discover] Fix legacy sort saved search stored in dashboard saved objects #137488
Conversation
…-ref HEAD~1..HEAD --fix'
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice refactoring! 🙂
Left some questions.
src/plugins/discover/public/embeddable/saved_search_embeddable.tsx
Outdated
Show resolved
Hide resolved
@elasticmachine merge upstream |
@@ -426,12 +422,11 @@ export class SavedSearchEmbeddable | |||
{ columns: this.input.columns || this.savedSearch.columns }, | |||
this.services.core.uiSettings | |||
).columns; | |||
searchProps.sort = this.getSort( | |||
this.input.sort || this.savedSearch.sort, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this case!
I noticed one weird thing when a saved search configuration on Dashboard has legacy format for a column "sort":["extension","desc"]
. It replaces the current sorting setting when trying to add another sort criteria on Dashboard (on Discover pages it works as expected)
Is it something we can improve?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely! another good catch 🎣 ! thx!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is interesting, because it worked for me locally ... maybe you could export the dashboard saved object and send it to me so I could reproduce? many thx!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx for the saved object! So the problem here is that extension
is a field that's not sortable, extension.keyword
is sortable. when changing the sort, this field is removed from sorting (sorted out 😃). Since the sorting state of the UI can only be changed with manually tweaking the saved search, I think we're safe here. However there might be a potential follow up taking care of the behavior in Discover, which seems to work differently in this case.
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this old issue and for the refactoring! LGTM 👍
We merge it after #138388, right? |
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Async chunks
Public APIs missing exports
History
To update your PR or re-run it, just comment with: cc @kertal |
…ects (elastic#137488) * Refactor and centralize sort functionality
Summary
This PR fixes the handling of legacy sort URL stored in Dashboards. Those URLs are automatically migrated on the fly like it works in
main
Discover.It contains a refactoring migrating sort functions from
public/components/doc_table/utils
topublic/utils
, away from classic table, since it was already shared among different parts of our plugin.Those functions now are also used in a better way in our embeddable code, centralizing handling of the sort property of state and its usage in data fetching. This change fixes the bug.
Fixes #81559
Testing
"sort\":[[\"taxful_total_price\",\"asc\"],[\"order_date\",\"desc\"]],
Legacy sort wasn't an array of arrays, but a single array containing the property to sort by, and the direction, so it needs to be changed like this
"sort\":[\"taxful_total_price\",\"asc\"],
Checklist