-
Notifications
You must be signed in to change notification settings - Fork 916
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
[data] updates query and lang if language is not supported by query data #8749
Changes from all commits
f30b1bd
0ed7a53
6cebc22
096d189
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 |
---|---|---|
@@ -0,0 +1,2 @@ | ||
fix: | ||
- Updates query and language if language is not supported by query data ([#8749](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8749)) |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -31,7 +31,7 @@ | |||||
import { BehaviorSubject } from 'rxjs'; | ||||||
import { skip } from 'rxjs/operators'; | ||||||
import { CoreStart, NotificationsSetup } from 'opensearch-dashboards/public'; | ||||||
import { debounce, isEqual } from 'lodash'; | ||||||
import { isEqual } from 'lodash'; | ||||||
import { i18n } from '@osd/i18n'; | ||||||
import { Dataset, DataStorage, Query, TimeRange, UI_SETTINGS } from '../../../common'; | ||||||
import { createHistory, QueryHistory } from './query_history'; | ||||||
|
@@ -154,10 +154,40 @@ | |||||
*/ | ||||||
public setQuery = (query: Partial<Query>) => { | ||||||
const curQuery = this.query$.getValue(); | ||||||
const newQuery = { ...curQuery, ...query }; | ||||||
let newQuery = { ...curQuery, ...query }; | ||||||
if (!isEqual(curQuery, newQuery)) { | ||||||
// Add to recent datasets if dataset has changed | ||||||
// Check if dataset changed and if new dataset has language restrictions | ||||||
if (newQuery.dataset && !isEqual(curQuery.dataset, newQuery.dataset)) { | ||||||
// Get supported languages for the dataset | ||||||
const supportedLanguages = this.datasetService | ||||||
.getType(newQuery.dataset.type) | ||||||
?.supportedLanguages(newQuery.dataset); | ||||||
|
||||||
// If we have supported languages and current language isn't supported | ||||||
if (supportedLanguages && !supportedLanguages.includes(newQuery.language)) { | ||||||
// Get initial query with first supported language and new dataset | ||||||
newQuery = this.getInitialQuery({ | ||||||
language: supportedLanguages[0], | ||||||
dataset: newQuery.dataset, | ||||||
}); | ||||||
|
||||||
// Show warning about language change | ||||||
showWarning(this.notifications, { | ||||||
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. Shouldn't this just be an info? 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. +1 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. it could be show warning is the helper function for this manager that passes an id that prevents duplicate notifications from popping up. so if the user constantly switches between recent data and somehow the current language is always invalid it won't keep adding a notification on the right side and keep pushing the duplicate ones upwards. it will just have a single notification available. 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. |
||||||
title: i18n.translate('data.languageChangeTitle', { | ||||||
defaultMessage: 'Language Changed', | ||||||
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.
Suggested change
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'm not sure if this will retrigger requiring approvals so will do this in fast follow! thanks |
||||||
}), | ||||||
text: i18n.translate('data.languageChangeBody', { | ||||||
defaultMessage: 'Query language changed to {supportedLanguage}.', | ||||||
values: { | ||||||
supportedLanguage: | ||||||
this.languageService.getLanguage(supportedLanguages[0])?.title || | ||||||
supportedLanguages[0], | ||||||
}, | ||||||
}), | ||||||
}); | ||||||
} | ||||||
|
||||||
// Add to recent datasets | ||||||
this.datasetService.addRecentDataset(newQuery.dataset); | ||||||
} | ||||||
this.query$.next(newQuery); | ||||||
|
@@ -204,7 +234,6 @@ | |||||
* If only language is provided, uses current dataset | ||||||
* If only dataset is provided, uses current or dataset's preferred language | ||||||
*/ | ||||||
// TODO: claude write jest test for this | ||||||
public getInitialQuery = (partialQuery?: Partial<Query>) => { | ||||||
if (!partialQuery) { | ||||||
return this.getInitialQueryByLanguage(this.query$.getValue().language); | ||||||
|
@@ -243,7 +272,6 @@ | |||||
* Gets initial query for a language, preserving current dataset | ||||||
* Called by getInitialQuery when only language changes | ||||||
*/ | ||||||
// TODO: claude write jest test for this | ||||||
public getInitialQueryByLanguage = (languageId: string) => { | ||||||
const curQuery = this.query$.getValue(); | ||||||
const language = this.languageService.getLanguage(languageId); | ||||||
|
@@ -264,7 +292,6 @@ | |||||
* Gets initial query for a dataset, using dataset's preferred language or current language | ||||||
* Called by getInitialQuery when only dataset changes | ||||||
*/ | ||||||
// TODO: claude write jest test for this | ||||||
public getInitialQueryByDataset = (newDataset: Dataset) => { | ||||||
const curQuery = this.query$.getValue(); | ||||||
// Use dataset's preferred language or fallback to current language | ||||||
|
@@ -320,7 +347,10 @@ | |||||
}; | ||||||
} | ||||||
|
||||||
const showWarning = (notifications: NotificationsSetup, { title, text }) => { | ||||||
const showWarning = ( | ||||||
notifications: NotificationsSetup, | ||||||
{ title, text }: { title: string; text: string } | ||||||
) => { | ||||||
notifications.toasts.addWarning({ title, text, id: 'unsupported_language_selected' }); | ||||||
}; | ||||||
|
||||||
|
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 it okay if
language
isundefined
?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.
it shouldn't but with new features i'd imagine there prolly is some way a plugin developer can attempt. will add an issue