-
Notifications
You must be signed in to change notification settings - Fork 918
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][Discover] Allow saved sort from search embeddable to load in Dashboard #5934
Conversation
…shboard Remove default sort to use saved search sort. Issue Resolve opensearch-project#5933 Signed-off-by: Anan Zhuang <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5934 +/- ##
=======================================
Coverage 67.00% 67.00%
=======================================
Files 3307 3307
Lines 63614 63614
Branches 10163 10163
=======================================
Hits 42627 42627
Misses 18518 18518
Partials 2469 2469
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Would be nice if we had a test for this.
…shboard (#5934) Remove default sort to use saved search sort. Issue Resolve #5933 Signed-off-by: Anan Zhuang <[email protected]> (cherry picked from commit 790c076) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md
…shboard (opensearch-project#5934) Remove default sort to use saved search sort. Issue Resolve opensearch-project#5933 Signed-off-by: Anan Zhuang <[email protected]>
I'm curious to know why it is nice not required to have test? just want to be sure we could maintain same quality bar |
discover-sort.mov |
@seraphjiang you are right, this is not a nice to have test, but a required one. @ananzh can you as a followup to this PR add a test for this scenario? My original hesitation was because I was worried that we would be adding testing here for testing sake. But that is an unnecessary worry. |
Thanks @ashwin-pc for clarification. |
@ashwin-pc @seraphjiang Yes, we do have a meta issue to create more tests for discover #5713. |
…shboard (opensearch-project#5934) Remove default sort to use saved search sort. Issue Resolve opensearch-project#5933 Signed-off-by: Anan Zhuang <[email protected]>
…shboard (#5934) (#5938) Remove default sort to use saved search sort. Issue Resolve #5933 Signed-off-by: Anan Zhuang <[email protected]> (cherry picked from commit 790c076) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Qingyang(Abby) Hu <[email protected]>
…shboard (opensearch-project#5934) Remove default sort to use saved search sort. Issue Resolve opensearch-project#5933 Signed-off-by: Anan Zhuang <[email protected]>
…shboard (opensearch-project#5934) Remove default sort to use saved search sort. Issue Resolve opensearch-project#5933 Signed-off-by: Anan Zhuang <[email protected]>
…shboard (opensearch-project#5934) Remove default sort to use saved search sort. Issue Resolve opensearch-project#5933 Signed-off-by: Anan Zhuang <[email protected]>
Description
Remove default sort to use saved search sort.
Issues Resolved
#5933
Screenshot
save.mov
Check List
yarn test:jest
yarn test:jest_integration