-
Notifications
You must be signed in to change notification settings - Fork 898
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 Initialization if No Saved Query #8930
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Sean Li <lnse@amazon.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8930 +/- ##
=======================================
Coverage 60.87% 60.88%
=======================================
Files 3802 3802
Lines 91072 91072
Branches 14375 14374 -1
=======================================
+ Hits 55444 55447 +3
+ Misses 32086 32085 -1
+ Partials 3542 3540 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Sean Li <lnse@amazon.com>
@@ -392,8 +392,7 @@ export const useSearch = (services: DiscoverViewServices) => { | |||
const savedSearchInstance = await getSavedSearchById(savedSearchId); | |||
|
|||
const query = | |||
savedSearchInstance.searchSource.getField('query') || | |||
data.query.queryString.getDefaultQuery(); |
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.
Since .getDefaultQuery()
isn't being used here, is it still used in other locations? I would've assumed I would see the removal of this function as well after reading the description
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.
i dont know if we want removal of the function in general. but i understand what you mean it should probably be a private function within the query string manager.
@@ -432,7 +431,7 @@ export const useSearch = (services: DiscoverViewServices) => { | |||
} | |||
|
|||
filterManager.setAppFilters(actualFilters); | |||
data.query.queryString.setQuery(savedQuery ? data.query.queryString.getQuery() : query); | |||
data.query.queryString.setQuery(query); |
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.
does this have any impact to saved queries? like logically to me when reading this this makes sense why we would remove it but save query expected functionality is a little weird to me. so if a saved query is applied and we update the query. but dont fire it's the correct bheavior here right
Description
Upon Discover mount (when
getSavedSearchById
is called), we were either setting the query to be the saved search, if it exists, or the default query. In this PR, I updated the logic such that we use the current query instead, which should be the single entry point that also handles the default query state.Issues Resolved
Screenshot
Testing the changes
Changelog
Check List
yarn test:jest
yarn test:jest_integration