-
Notifications
You must be signed in to change notification settings - Fork 924
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
[Workspace] feat: only allow essential use case when creating workspace if all data sources are serverless #7612
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7612 +/- ##
=======================================
Coverage 63.69% 63.70%
=======================================
Files 3633 3633
Lines 80135 80153 +18
Branches 12699 12702 +3
=======================================
+ Hits 51043 51061 +18
Misses 25984 25984
Partials 3108 3108
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
export const getIsOnlyAllowEssentialUseCase = async (client: SavedObjectsStart['client']) => { | ||
const allDataSources = await getDataSourcesList(client, ['*']); | ||
if (allDataSources.length > 0) { | ||
const serverlessDataSource = allDataSources.filter( |
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 use allDataSources.every()
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, updated.
@@ -54,21 +60,60 @@ const WorkspaceUseCaseCard = ({ | |||
); | |||
}; | |||
|
|||
type AvailableUseCase = Pick< | |||
WorkspaceUseCaseObject, | |||
'id' | 'title' | 'description' | 'systematic' |
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.
Maybe this is irrelevant, but what does systematic
stand for? cc @wanglam
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 filtering out system nav group, we won't show system nav groups in the use case selector.
{displayedUseCases.map(({ id, title, description, disabled }) => ( | ||
<EuiFlexItem key={id}> | ||
<WorkspaceUseCaseCard | ||
id={id} | ||
title={title} | ||
description={description} | ||
checked={value === id} | ||
onChange={onChange} | ||
disabled={disabled} | ||
/> | ||
</EuiFlexItem> | ||
))} |
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't find the place set disabled
, do we still need it? I see we are doing filter instead of setting other use cases disabled.
// When creating and isOnlyAllowEssential is true, only display essential use case
if (isOnlyAllowEssential && operationType === WorkspaceOperationType.Create) {
allAvailableUseCases = allAvailableUseCases.filter(
(item) => item.id === DEFAULT_NAV_GROUPS.analytics.id
);
}
setDisplayedUseCases(allAvailableUseCases);
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.
Oh, I missed to delete it.Thanks for catching it. Will update it soon.
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.
Signed-off-by: tygao <[email protected]>
Signed-off-by: tygao <[email protected]>
Signed-off-by: tygao <[email protected]>
useEffect(() => { | ||
if (operationType === WorkspaceOperationType.Create) { | ||
getIsOnlyAllowEssentialUseCase(savedObjects.client).then((result: boolean) => { | ||
setIsOnlyAllowEssential(result); | ||
}); | ||
} | ||
}, [savedObjects, operationType]); |
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 looks we can simply use useMemo
instead of useEffect
+ useState
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.
Here useMemo
would be hard to combine with promise.
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.
So I used useEffect
+ useState
to make it clear.
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.
right, that's a promise
useEffect(() => { | ||
let allAvailableUseCases = availableUseCases | ||
.filter((item) => !item.systematic) | ||
.concat(DEFAULT_NAV_GROUPS.all); | ||
// When creating and isOnlyAllowEssential is true, only display essential use case | ||
if (isOnlyAllowEssential && operationType === WorkspaceOperationType.Create) { | ||
allAvailableUseCases = allAvailableUseCases.filter( | ||
(item) => item.id === DEFAULT_NAV_GROUPS.analytics.id | ||
); | ||
} | ||
setDisplayedUseCases(allAvailableUseCases); | ||
}, [availableUseCases, isOnlyAllowEssential, operationType]); |
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.
The same here, we can simply use useMemo
, isn't 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.
Updated.
Signed-off-by: tygao <[email protected]>
Signed-off-by: tygao <[email protected]>
@SuZhou-Joe @ruanyl Could you help me approve it again? Just fix a error that use case changes from analytics to essential after pulling main. |
…ce if all data sources are serverless (#7612) * feat: only allow essential use case when all data sources are serverless Signed-off-by: tygao <[email protected]> * Changeset file for PR #7612 created/updated * update and only keep workspace create case Signed-off-by: tygao <[email protected]> * Changeset file for PR #7612 created/updated * remove useless disabled logic Signed-off-by: tygao <[email protected]> * use useMemo Signed-off-by: tygao <[email protected]> * update filter and tests to update with essiential use case change Signed-off-by: tygao <[email protected]> --------- Signed-off-by: tygao <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit f59c2b9) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…ce if all data sources are serverless (#7612) (#7684) * feat: only allow essential use case when all data sources are serverless * Changeset file for PR #7612 created/updated * update and only keep workspace create case * Changeset file for PR #7612 created/updated * remove useless disabled logic * use useMemo * update filter and tests to update with essiential use case change --------- (cherry picked from commit f59c2b9) Signed-off-by: tygao <[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> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Description
Only allow essential use case when creating workspace if all data sources are serverless
Screenshot
Create workspace
Testing the changes
Set all data sources as serverless, then go to workspace create page or detail page.
Changelog
Check List
yarn test:jest
yarn test:jest_integration