-
Notifications
You must be signed in to change notification settings - Fork 920
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] query editor and language selector state fixes #8712
[discover] query editor and language selector state fixes #8712
Conversation
❌ Empty Changelog SectionThe Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8712 +/- ##
==========================================
- Coverage 60.75% 60.75% -0.01%
==========================================
Files 3798 3798
Lines 90650 90677 +27
Branches 14254 14269 +15
==========================================
+ Hits 55076 55089 +13
- Misses 32079 32091 +12
- Partials 3495 3497 +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: Kawika Avilla <kavilla414@gmail.com>
156b1d5
to
126f51b
Compare
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
❌ Empty Changelog SectionThe Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section. |
1 similar comment
❌ Empty Changelog SectionThe Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section. |
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
test it with new cypress tests. It can pass the two tests I created. |
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.
How have we tested this? Is there an endpoint I can try this out on?
@@ -66,7 +66,7 @@ export class LanguageService { | |||
return Array.from(this.languages.values()); | |||
} | |||
|
|||
public getDefaultLanguage(): LanguageConfig { | |||
public getDefaultLanguage(): LanguageConfig | undefined { |
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.
Was this response type just incorrect before or did it change?
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.
+1
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.
response type was incorrect. in current state there should never not be a default language. but since it's stored as a map, the linter is saying DQL
technically can't exist.
probaby need to bucket in having a more thorough default language usage and what that default language is. or did we want to lean in on perhaps there is no language state.
* If only language is provided, uses current dataset | ||
* If only dataset is provided, uses current or dataset's preferred language | ||
*/ | ||
public getInitialQuery = (partialQuery?: Partial<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.
do we have a plan to address test coverage of this method?
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.
+1
*/ | ||
public getInitialQuery = (partialQuery?: Partial<Query>) => { | ||
if (!partialQuery) { | ||
return this.getInitialQueryByLanguage(this.query$.getValue().language); |
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.
Is this.query$.getValue().language
always defined?
language: languageId, | ||
}; | ||
const queryString = language?.getQueryString(newQuery) || ''; | ||
this.languageService.setUserQueryString(queryString); |
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.
q: this updates the query in the query box?
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 pull it down and verified using new tests. Changes look good to me. Approve for unblocking. I am also curious about @virajsanghvi 's questions. It would be better if you can add more tests.
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.
Approving to unblock
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 the added comments in the flow. IT helped the review. No real blockers for the PR, had some questions before approving though
* If only language is provided, uses current dataset | ||
* If only dataset is provided, uses current or dataset's preferred language | ||
*/ | ||
public getInitialQuery = (partialQuery?: Partial<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.
+1
return datasetType?.supportedLanguages(dataset); | ||
}, [props.query.dataset, queryString]); | ||
const [isPopoverOpen, setPopover] = useState(false); | ||
const [currentLanguage, setCurrentLanguage] = useState<string>( |
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.
This works for now, but i dont like keeping track of another state here. This component should simply use the current state of the query service. Two issues can arise from this:
- If the query state does not update, but the
setCurrentLanguage
does. They ar no out of sync again - If someone in fututre accidentally updates the query state based on the
currentLanguage
, we have a race condition again
Not a blocker, but we need to be figure out a nicer pattern for components that want to simply reflect the current state of the query and update it without causing update loops
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.
yeah i think we discussed offline, but ui actions would help. originally i wanted to just rely on the query manager solely but then when modifying the code it was historically split because they wanted different states and actions for when the user selected the language vs like saved queries/saved searches modifying the language.
for example they will count an opt in/opt out of DQL based on the user selecting the language. but they would not increment an opt out count of DQL if the user selected a saved query that had a language of Lucene.
but we should revisit this and rely on modern services in OSD instead of handling the logic too specifically in a single component and passing around that prop.
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.
we need to update the tests here. These snapshots are not useful
@@ -57,7 +54,7 @@ const ConnectedDatasetSelector = ({ | |||
<DatasetSelector | |||
{...datasetSelectorProps} | |||
selectedDataset={selectedDataset} | |||
setSelectedDataset={handleDatasetChange} | |||
onSelect={onSelect} |
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.
Nit:
onSelect={onSelect} | |
onChance={handleChange} |
This is a more common pattern
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.
can make it a fast follow?
setSelectedDataset(query.dataset); | ||
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.
This is not a good pattern. We are effectively trying to keep two separate states in sync. That means we have 2 sources of truth, which is risky
@@ -69,224 +65,171 @@ interface Props extends QueryEditorProps { | |||
opensearchDashboards: OpenSearchDashboardsReactContextValue<IDataPluginServices>; | |||
} | |||
|
|||
interface State { | |||
isSuggestionsVisible: boolean; |
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.
Dont we need this anymore?
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 see it being used. autocomplete appear to be working
services.http.post('/api/opensearch-dashboards/dql_opt_in_stats', { | ||
body: JSON.stringify({ opt_in: languageId === 'kuery' }), | ||
}); |
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.
We should remove this as a whole
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 agree
!languageEditor.TopBar.Expanded && 'emptyExpanded' | ||
)} | ||
> | ||
<div |
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.
Why did we move the location of the banner?
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.
whoops seems to have duplicated
language: query.language, | ||
dataset: query.dataset, | ||
}, | ||
dateRange |
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.
We were missing this? How did it work before?
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
* Query editor functional component and cleanup language selector Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * pass language and dataset when the configurator sets it Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * clean up some dup and update language selector tests Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * fix tests Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * Changeset file for PR #8712 created/updated * update recent dataset on query update Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * add tests and remove extra div wrapper Signed-off-by: Kawika Avilla <kavilla414@gmail.com> --------- Signed-off-by: Kawika Avilla <kavilla414@gmail.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit 56eeab2) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…-project#8712) * Query editor functional component and cleanup language selector Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * pass language and dataset when the configurator sets it Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * clean up some dup and update language selector tests Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * fix tests Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * Changeset file for PR opensearch-project#8712 created/updated * update recent dataset on query update Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * add tests and remove extra div wrapper Signed-off-by: Kawika Avilla <kavilla414@gmail.com> --------- Signed-off-by: Kawika Avilla <kavilla414@gmail.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
) * Query editor functional component and cleanup language selector * pass language and dataset when the configurator sets it * clean up some dup and update language selector tests * fix tests * Changeset file for PR #8712 created/updated * update recent dataset on query update * add tests and remove extra div wrapper --------- (cherry picked from commit 56eeab2) Signed-off-by: Kawika Avilla <kavilla414@gmail.com> 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> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Description
Issues Resolved
n/a
Screenshot
Testing the changes
Changelog
Check List
yarn test:jest
yarn test:jest_integration