-
Notifications
You must be signed in to change notification settings - Fork 917
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
Make sure customer always have a default datasource #6237
Conversation
9822ef0
to
330211d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6237 +/- ##
==========================================
+ Coverage 67.44% 67.45% +0.01%
==========================================
Files 3368 3368
Lines 65394 65423 +29
Branches 10553 10559 +6
==========================================
+ Hits 44102 44134 +32
+ Misses 18718 18716 -2
+ Partials 2574 2573 -1
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
One question about the flow: after you create the first data source, it goes back to the list page. But why would we require user to refresh page for the default icon to show up? |
330211d
to
19d4ba6
Compare
Hi Tianle. Thanks for pointing this out. I have fixed the issue. Now it will show the default. You can check it on the video |
We may need confirmation from product manager about this behavior. |
|
||
await handleSetDefaultDatasourceAfterDeletion(savedObjects.client, uiSettings); | ||
expect(uiSettings.set).toHaveBeenCalled(); |
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: according to the behavior which set the first data source in the list as default, maybe it's nice to assert that uiSettings.set
is called with the first data source id.
expect(uiSettings.set).toHaveBeenCalledWith('defaultDataSource', 'test');
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.
Would do
}); | ||
}); | ||
describe('handleSetDefaultDatasourceDuringCreation', () => { | ||
test('should set defaultDataSource if only one data source exists', async () => { |
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.
For unit test of this function, I think it's worth to add a test to cover the case that when number of data source > 1 that uiSettings.set
should not be called.
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.
Added!
19d4ba6
to
65474c2
Compare
Thanks for pointing this out. Customer can set a default datasource in the edit page anytime after they created the datasource. For first question, after customer enables data_source, then he or she can adding datasource and the first created datasource would be default datasouce. This is the default behavior. I don't see a scenario customer can create the first datasource without setting it be default. I might miss something. Please point to me if I do. For second one, I wil check with product manager for this.Thank you for pointing this out. |
@zhyuanqi @seraphjiang @xluo-aws @kgcreative |
Yes, user can view all saved objects they have access(including home workspace objects) in home workspace. But if a user create a saved object in this workspace, it's associated with this workspace. |
@@ -49,6 +54,29 @@ export async function getDataSourcesWithFields( | |||
return response?.savedObjects; | |||
} | |||
|
|||
export async function handleSetDefaultDatasourceAfterDeletion( | |||
savedObjects: SavedObjectsClientContract, |
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:
Based on the type "SavedObjectsClientContract", Looks like here we want to provide a savedObjectClient here.
What if we rename savedObjects
to savedObjectsClient
like we we did here: https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6237/files#diff-fde383d0fdaaed3bb243ed36abd631c9c311596b2057f737253fc5ab11c4d5edR45?
What do you think?
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.
Fixed it!
export async function handleSetDefaultDatasourceAfterDeletion( | ||
savedObjects: SavedObjectsClientContract, | ||
uiSettings: IUiSettingsClient | ||
) { | ||
uiSettings.remove('defaultDataSource'); | ||
const listOfDataSources: DataSourceTableItem[] = await getDataSources(savedObjects); | ||
if (Array.isArray(listOfDataSources) && listOfDataSources.length >= 1) { | ||
const datasourceId = listOfDataSources[0].id; | ||
await uiSettings.set('defaultDataSource', datasourceId); | ||
} | ||
} | ||
|
||
export async function handleSetDefaultDatasourceDuringCreation( | ||
savedObjects: SavedObjectsClientContract, | ||
uiSettings: IUiSettingsClient | ||
) { | ||
const listOfDataSources: DataSourceTableItem[] = await getDataSources(savedObjects); | ||
if (Array.isArray(listOfDataSources) && listOfDataSources.length === 1) { | ||
const datasourceId = listOfDataSources[0].id; | ||
await uiSettings.set('defaultDataSource', datasourceId); | ||
} | ||
} | ||
|
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 this change,
Maybe following scenario is out of scope:
Suppose we already have dataSource created before before we support default dataSource, this line will failed to set the first dataSource as default: https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6237/files#diff-fde383d0fdaaed3bb243ed36abd631c9c311596b2057f737253fc5ab11c4d5edR74
Thus I'm thinking if we can handle that scenario and also keep one function to handle set default dataSource by updating it to:
export async function handleSetDefaultDatasourceAfterDeletion( | |
savedObjects: SavedObjectsClientContract, | |
uiSettings: IUiSettingsClient | |
) { | |
uiSettings.remove('defaultDataSource'); | |
const listOfDataSources: DataSourceTableItem[] = await getDataSources(savedObjects); | |
if (Array.isArray(listOfDataSources) && listOfDataSources.length >= 1) { | |
const datasourceId = listOfDataSources[0].id; | |
await uiSettings.set('defaultDataSource', datasourceId); | |
} | |
} | |
export async function handleSetDefaultDatasourceDuringCreation( | |
savedObjects: SavedObjectsClientContract, | |
uiSettings: IUiSettingsClient | |
) { | |
const listOfDataSources: DataSourceTableItem[] = await getDataSources(savedObjects); | |
if (Array.isArray(listOfDataSources) && listOfDataSources.length === 1) { | |
const datasourceId = listOfDataSources[0].id; | |
await uiSettings.set('defaultDataSource', datasourceId); | |
} | |
} | |
export async function handleSetDefaultDatasource( | |
savedObjects: SavedObjectsClientContract, | |
uiSettings: IUiSettingsClient | |
) { | |
const currentDefaultDataSourceId = uiSettings.get('defaultDataSource'); | |
if (currentDefaultDataSourceId) { | |
const targetDataSource = await getDataSourceById(currentDefaultDataSourceId, savedObjectsClient); | |
if (targetDataSource.id === currentDefaultDataSourceId) { | |
return; | |
} | |
} | |
setFirstDataSourceAsDefault(savedObjectsClient, uiSettings) | |
} | |
async function setFirstDataSourceAsDefault( | |
savedObjectsClient: SavedObjectsClientContract, | |
uiSettings: IUiSettingsClient | |
) { | |
uiSettings.remove('defaultDataSource'); | |
const listOfDataSources: DataSourceTableItem[] = await getDataSources(savedObjects); | |
if (Array.isArray(listOfDataSources) && listOfDataSources.length >= 1) { | |
const datasourceId = listOfDataSources[0].id; | |
await uiSettings.set('defaultDataSource', datasourceId); | |
} | |
} |
Just for suggestions, what do you think?
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 this is a better approach. I will have a new CR on default with picker. Do you mind if i make the relative change in the new CR.
fe3b323
to
2a97910
Compare
} | ||
} | ||
} catch (e) { | ||
setIsLoading(false); |
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.
what happens if error happens and we set isloading as false?
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 will stop loading and show error as "Unable to set the Data Source to default. Please try it again."
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.
Should use setIsDeleting(false), let me rephrase this part
setIsLoading(false); | ||
handleDisplayToastMessage({ | ||
id: 'dataSourcesManagement.editDataSource.setDefaultDataSourceFailMsg', | ||
defaultMessage: 'Unable to set the Data Source to default. Please try it again.', |
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 does index pattern handle the error? How would user be able to retry if the data source is already deleted?
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.
Let me change the default message to "Unable to find a default datasource. Please set a new default datasource."
Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>
18d64a4
to
0cd6a68
Compare
Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>
0cd6a68
to
0c44ea9
Compare
Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>
* Make sure customer always have a default datasource Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> * Handle set as default Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> --------- Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> Co-authored-by: Miki <[email protected]> (cherry picked from commit 91a0530) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md
* Make sure customer always have a default datasource Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> * Handle set as default Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> --------- Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> Co-authored-by: Miki <[email protected]> (cherry picked from commit 91a0530) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
Make sure customer always have a default datasource
Screenshot
Screen.Recording.2024-03-21.at.4.03.32.PM.mov
Testing the changes
Check List
yarn test:jest
yarn test:jest_integration