-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[Discover] Persist query mode to local storage #250388
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
Changes from 7 commits
20b6058
d0129fd
eeff241
ebccd71
116f0f6
bc3a50f
2f1163d
57d52ec
2d3a1b9
cfb3c95
26d21b8
bdd9f10
80c5678
5a4c68b
9676c7d
2e24f59
a135bf0
3e64040
479240c
f0a07f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,10 @@ import { | |
| selectAllTabs, | ||
| } from '../../state_management/redux'; | ||
| import { useDiscoverServices } from '../../../../hooks/use_discover_services'; | ||
| import { ESQL_TRANSITION_MODAL_KEY } from '../../../../../common/constants'; | ||
| import { | ||
| DISCOVER_QUERY_MODE_KEY, | ||
| ESQL_TRANSITION_MODAL_KEY, | ||
| } from '../../../../../common/constants'; | ||
| import { useTopNavMenuItems } from '../top_nav/use_top_nav_menu_items'; | ||
| import { isEsqlSource } from '../../../../../common/data_sources'; | ||
|
|
||
|
|
@@ -98,6 +101,7 @@ export const useAppMenuData = ({ currentDataView }: UseAppMenuDataParams): UseAp | |
| } else { | ||
| dispatch(transitionFromESQLToDataView({ dataViewId: currentDataView?.id ?? '' })); | ||
| } | ||
| services.storage.set(DISCOVER_QUERY_MODE_KEY, 'classic'); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would suggest to clear the previously saved value then so when we continue with #250038 it would not interfere.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you mean to instead of save which of the modes was the last one we just store if the last one was ESQL? i guess the problem then is going to be that once ESQL is the default in #250201 if someone switches back to classic it won't use it as the next default
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, good point! Then we just need to keep in mind that if users had switched to
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thx for raising, yes, also didn't think about this. so it makes sense, to store it with |
||
| }, | ||
| }, | ||
| ]; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the "Elastic License | ||
| * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
| * Public License v 1"; you may not use this file except in compliance with, at | ||
| * your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
| import type { FtrProviderContext } from '../ftr_provider_context'; | ||
|
|
||
| export default function ({ getPageObjects, getService }: FtrProviderContext) { | ||
| const testSubjects = getService('testSubjects'); | ||
|
|
||
| const { discover, common } = getPageObjects(['discover', 'common']); | ||
|
|
||
| describe('Default query mode', () => { | ||
| afterEach(async () => { | ||
| await discover.resetQueryMode(); | ||
| }); | ||
|
|
||
| beforeEach(async () => { | ||
| await common.navigateToApp('discover'); | ||
| }); | ||
|
|
||
| describe('when the default query mode is ES|QL', () => { | ||
| it('should open Discover in ES|QL mode', async () => { | ||
| await discover.setQueryMode('esql'); | ||
| await common.navigateToApp('discover'); | ||
| await discover.expectSourceViewerToExist(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('when the default query mode is classic', () => { | ||
| it('should open Discover in classic mode', async () => { | ||
| await discover.setQueryMode('classic'); | ||
| await common.navigateToApp('discover'); | ||
| await testSubjects.existOrFail('discover-dataView-switch-link'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('when the default query mode is unset', () => { | ||
| it('should open Discover in classic mode', async () => { | ||
| await discover.resetQueryMode(); | ||
| await common.navigateToApp('discover'); | ||
| await testSubjects.existOrFail('discover-dataView-switch-link'); | ||
| }); | ||
| }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be nice to also add a test when the path is not empty.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think the unit tests are kind of covering this scenario, but i can follow up in the feature flag PR with more tests if it doesn't look that way |
||
| }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the "Elastic License | ||
| * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
| * Public License v 1"; you may not use this file except in compliance with, at | ||
| * your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
| import expect from '@kbn/expect'; | ||
| import type { FtrProviderContext } from '../ftr_provider_context'; | ||
|
|
||
| export default function ({ getPageObjects }: FtrProviderContext) { | ||
| const { discover, common } = getPageObjects(['discover', 'common']); | ||
|
|
||
| describe('Update query mode', () => { | ||
| afterEach(async () => { | ||
| await discover.resetQueryMode(); | ||
| }); | ||
|
|
||
| describe('when the user clicks ES|QL mode', () => { | ||
| it('should set the default mode to ES|QL', async () => { | ||
| await common.navigateToApp('discover'); | ||
| await discover.selectTextBaseLang(); | ||
| const queryMode = await discover.getQueryMode(); | ||
| expect(queryMode).to.contain('esql'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('when the user clicks classic', () => { | ||
| it('should set the default mode to classic', async () => { | ||
| await common.navigateToApp('discover'); | ||
| await discover.selectTextBaseLang(); | ||
| await discover.selectDataViewMode(); | ||
| const queryMode = await discover.getQueryMode(); | ||
| expect(queryMode).to.contain('classic'); | ||
| }); | ||
| }); | ||
| }); | ||
| } | ||
|
jughosta marked this conversation as resolved.
Outdated
|
||
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 the transition modal to Classic can be dismissed, the value should not change to
classicin this case.Then a single place in the code, where to move
services.storage.set(DISCOVER_QUERY_MODE_KEY, 'classic');to, might be insidetransitionFromESQLToDataViewaction. Or better as a redux listener to the action.For consistency we could apply a similar change to
transitionFromESQLToDataView.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.
that makes sense, i thought of that at first but i wasn't sure if we wanted to introduce a side-effect in the redux actions
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.
updated to redux listener in 5a4c68b - it seems to be working but it's my first time doing one redux listener so I hope it's the way it should be
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 meant listeners for
transitionFromESQLToDataViewandtransitionFromDataViewToESQLactions. We still want to capture only the manual changes to the mode.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 looking into it!
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 think i got it now a135bf0 🙈
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.
Just wanted to say this is totally fine to do in thunks and middleware, often preferable even. It keeps the logic consolidated and makes it easy to test.
Personally in your case I'd put the logic directly in the thunk to avoid the extra actions indirection, but middleware works too.
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'm happy with either, i've seen now that i can access the storage in the thunks - no strong opinion from my side