Sync query and filter when refreshing discover page #8168
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abbyhu2000
requested review from
ananzh,
kavilla,
AMoo-Miki,
ashwin-pc,
joshuarrrr,
zengyan-amazon,
zhongnansu,
manasvinibs,
ZilongX,
Flyingliuhub,
curq,
bandinib-amzn,
SuZhou-Joe,
ruanyl,
BionIT,
xinruiba,
zhyuanqi,
mengweieric,
LDrago27,
virajsanghvi,
sejli,
joshuali925 and
huyaboo
as code owners
abbyhu2000
added
discover
opensearch-changeset-bot bot
added a commit
to abbyhu2000/OpenSearch-Dashboards
that referenced
this pull request
Sep 12, 2024
kavilla
previously approved these changes
Sep 12, 2024
kavilla
reviewed
Sep 12, 2024
| @@ -324,6 +324,11 @@ export const useSearch = (services: DiscoverViewServices) => { | |||
| const savedSearchInstance = await getSavedSearchById(savedSearchId); | |||
| setSavedSearch(savedSearchInstance); | |||
|
|
|||
| // if saved search does not exist, do not atempt to sync filters and query from savedObject | |||
| if (!savedSearch) { | |||
There was a problem hiding this comment.
i think the linter will complain that this isn't included in the dependency if it wasn't disabled? however i noticed that 325 sets this value.
i can see in the future someone listening to the linter and including the savedsearch in the dependencies.
shouldn't we just do
if (!savedSearchInstance) {There was a problem hiding this comment.
changed to use savedSearchId instead since it is already a dependency
There was a problem hiding this comment.
savedSearchInstance does not work here because it is always defined
There was a problem hiding this comment.
savedSearchId here wont work as well, because when we create a new saved search, the id is null, but we do not want to return here.
kavilla
previously approved these changes
Sep 12, 2024
sejli
previously approved these changes
Sep 12, 2024
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
abbyhu2000
force-pushed
the
query_filter_fail_to_sync
branch
from
ec40894 to
88fb745
Compare
kavilla
approved these changes
Sep 13, 2024
sejli
approved these changes
Sep 13, 2024
7 tasks
|
Manual 2.17 backport : #8179 |
|
link check failure is not related: |
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Sep 13, 2024
* revert back to use savedSearch Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com> * change to use savedSearchId Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com> * Changeset file for PR #8168 created/updated * revert to use savesearch Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com> --------- Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit 18875f7) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
abbyhu2000
pushed a commit
that referenced
this pull request
Sep 13, 2024
kavilla
pushed a commit
to opensearch-project/opensearch-dashboards-functional-test
that referenced
this pull request
Sep 26, 2024
Follow up PR of opensearch-project/OpenSearch-Dashboards#8168 to add test to make sure queries and filters are persisted across refresh, and switching between new and legacy tables. Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>
opensearch-trigger-bot bot
pushed a commit
to opensearch-project/opensearch-dashboards-functional-test
that referenced
this pull request
Sep 26, 2024
Follow up PR of opensearch-project/OpenSearch-Dashboards#8168 to add test to make sure queries and filters are persisted across refresh, and switching between new and legacy tables. Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com> (cherry picked from commit fb85e41)
ashwin-pc
pushed a commit
to opensearch-project/opensearch-dashboards-functional-test
that referenced
this pull request
Oct 1, 2024
Follow up PR of opensearch-project/OpenSearch-Dashboards#8168 to add test to make sure queries and filters are persisted across refresh, and switching between new and legacy tables. Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com> (cherry picked from commit fb85e41) Co-authored-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>
|
This fix appears to have caused a very ugly bug where filters added on a dashboard or in the visualization editor then appear on the Discover page when you navigate there. What's more, the issues this pull request claims to fix appear to work fine without the change anyway. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR resolves previously identified three similar bugs when using legacy discover:
Query and filter failed to persist when:
Issues Resolved
Screenshot
Screen.Recording.2024-09-12.at.1.28.21.PM.mov
Testing the changes
Changelog
Check List
yarn test:jestyarn test:jest_integration