[Discover] Allow save query to load correctly in Discover #5951
Conversation
ananzh
requested review from
kavilla,
AMoo-Miki,
ashwin-pc,
joshuarrrr,
abbyhu2000,
zengyan-amazon,
kristenTian,
zhongnansu,
manasvinibs,
ZilongX,
Flyingliuhub,
BSFishy,
curq,
bandinib-amzn and
SuZhou-Joe
as code owners
ananzh
added
discover
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5951 +/- ##
==========================================
+ Coverage 67.45% 67.48% +0.02%
==========================================
Files 3448 3448
Lines 67916 67928 +12
Branches 11042 11045 +3
==========================================
+ Hits 45812 45840 +28
+ Misses 19435 19416 -19
- Partials 2669 2672 +3
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.
The change looks foo to me. Did not pull it down to validate it yet but i agree with most of the logic except on how to intiialize the undefined columns and sort. Also can you add a test to make sure we dont regress here. Not sure if functional tests her are preferred, but we can definitely atleast have unit/integ tests that ensure that the saved query is loaded when the page loads and that changing it changes the redux state.
| if (newSavedQueryId) { | ||
| dispatch(setSavedQuery(newSavedQueryId)); | ||
| } else { | ||
| // remove savedQueryId from state | ||
| const newState = rootState; | ||
| delete newState.visualization.savedQuery; | ||
| dispatch(setState(newState.visualization)); | ||
| } |
There was a problem hiding this comment.
Delete the saved query prop in the redux action if SavedQueryId is undefined
| if (newSavedQueryId) { | |
| dispatch(setSavedQuery(newSavedQueryId)); | |
| } else { | |
| // remove savedQueryId from state | |
| const newState = rootState; | |
| delete newState.visualization.savedQuery; | |
| dispatch(setState(newState.visualization)); | |
| } | |
| dispatch(setSavedQuery(newSavedQueryId)); |
There was a problem hiding this comment.
good point. Will clean it.
| const { columns } = useSelector((state) => { | ||
| const stateColumns = state.discover.columns; | ||
| // check if state columns is not undefined, otherwise use buildColumns | ||
| return { | ||
| columns: stateColumns !== undefined ? stateColumns : buildColumns([]), | ||
| }; | ||
| }); | ||
| const { sort } = useSelector((state) => { | ||
| const stateSort = state.discover.sort; | ||
| // check if state sort is not undefined, otherwise assign an empty array | ||
| return { | ||
| sort: stateSort !== undefined ? stateSort : [], | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Why do we have this here? Cant we initialize the redux store with these values directly?
There was a problem hiding this comment.
load saved query is like load a saved object in discover. since discover is registered as a view in ui
ui: {
defaults: async () => {
this.initializeServices?.();
const services = getServices();
return await getPreloadedState(services);
},
slice: discoverSlice,
},
it will not call getPreloadedState again. this is fine for saved object since we save sort and columns. but for saved query object, we only save an id. therefore for a tmp fix, we need to make the initiazation in panel and canvas to make it robust.
| const { columns } = useSelector(state => { | ||
| const stateColumns = state.discover.columns; | ||
|
|
||
| // check if stateColumns is not undefined, otherwise use buildColumns | ||
| return { | ||
| columns: stateColumns !== undefined ? stateColumns : buildColumns([]) | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Same goes here. Initializing them in the redux store means that you can always expect these to be not undefined and avoid this check alltogether.
There was a problem hiding this comment.
same explain here. we could create an issue and re-visit the routing logic.
|
@ashwin-pc this is the tracking issue for complete ftr tests for discover #5713 I believe |
* add save query logic in Discover * add save query logic in VisBuilder * remove double render Issue Resolve opensearch-project#5942 Signed-off-by: Anan Zhuang <[email protected]>
src/plugins/discover/public/application/utils/state_management/discover_slice.test.tsx
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/application/utils/state_management/discover_slice.tsx
Outdated
Show resolved
Hide resolved
| columns && | ||
| timeFieldname && | ||
| !prevColumns.current.includes(timeFieldname) && | ||
| columns.includes(timeFieldname) |
There was a problem hiding this comment.
tiny bit of hardening:
| columns && | |
| timeFieldname && | |
| !prevColumns.current.includes(timeFieldname) && | |
| columns.includes(timeFieldname) | |
| timeFieldname && | |
| !prevColumns.current.includes(timeFieldname) && | |
| columns?.includes?.(timeFieldname) |
| } as VisualizationState; | ||
| }); | ||
|
|
||
| it('should handle setIndexPattern', () => { |
There was a problem hiding this comment.
nit: not the best test titles
Signed-off-by: Anan Zhuang <[email protected]>
* [Discover] Allow save query to load correctly in Discover * add save query logic in Discover * add save query logic in VisBuilder * remove double render Issue Resolve #5942 --------- Signed-off-by: Anan Zhuang <[email protected]> (cherry picked from commit 170ac61) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
|
Changeset dismissed after commit a fix. Add a |
) * [Discover] Allow save query to load correctly in Discover * add save query logic in Discover * add save query logic in VisBuilder * remove double render --------- (cherry picked from commit 170ac61) Signed-off-by: Anan Zhuang <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
Issues Resolved
#5942
Changelog
Screenshot
discover-save-query.mov
visbuilder-save-query.mov
Check List
yarn test:jestyarn test:jest_integration