-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: filter out ADMIN application and add feature dependency logic #49
Changes from 2 commits
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,37 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
import { isFeatureDependOnSelectedFeatures, getFinalFeatureIdsByDependency } from './feature'; | ||
|
||
describe('feature utils', () => { | ||
describe('isFeatureDependOnSelectedFeatures', () => { | ||
it('should return true', () => { | ||
expect(isFeatureDependOnSelectedFeatures('a', ['b'], { b: ['a'] })).toBe(true); | ||
expect(isFeatureDependOnSelectedFeatures('a', ['b'], { b: ['a', 'c'] })).toBe(true); | ||
}); | ||
it('should return false', () => { | ||
expect(isFeatureDependOnSelectedFeatures('a', ['b'], { b: ['c'] })).toBe(false); | ||
expect(isFeatureDependOnSelectedFeatures('a', ['b'], {})).toBe(false); | ||
}); | ||
}); | ||
|
||
describe('getFinalFeatureIdsByDependency', () => { | ||
it('should return consistent feature ids', () => { | ||
expect(getFinalFeatureIdsByDependency(['a'], { a: ['b'] }, ['c', 'd'])).toStrictEqual([ | ||
'c', | ||
'd', | ||
'a', | ||
'b', | ||
]); | ||
expect(getFinalFeatureIdsByDependency(['a'], { a: ['b', 'e'] }, ['c', 'd'])).toStrictEqual([ | ||
'c', | ||
'd', | ||
'a', | ||
'b', | ||
'e', | ||
]); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
export const isFeatureDependOnSelectedFeatures = ( | ||
featureId: string, | ||
selectedFeatureIds: string[], | ||
featureDependencies: { [key: string]: string[] } | ||
) => | ||
selectedFeatureIds.some((selectedFeatureId) => | ||
(featureDependencies[selectedFeatureId] || []).some((dependencies) => | ||
dependencies.includes(featureId) | ||
) | ||
); | ||
|
||
export const getFinalFeatureIdsByDependency = ( | ||
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. What's |
||
featureIds: string[], | ||
featureDependencies: { [key: string]: string[] }, | ||
oldFeatureIds: string[] = [] | ||
) => | ||
Array.from( | ||
new Set([ | ||
...oldFeatureIds, | ||
...featureIds.reduce( | ||
(pValue, featureId) => [...pValue, ...(featureDependencies[featureId] || [])], | ||
featureIds | ||
), | ||
]) | ||
); |
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. This file is a little bit large(about 300 lines), maybe we can extract some filter functions, 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. Sure, we can separate these util function to a single file. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,12 +33,17 @@ import { | |
} from '@elastic/eui'; | ||
|
||
import { WorkspaceTemplate } from '../../../../../core/types'; | ||
import { AppNavLinkStatus, ApplicationStart } from '../../../../../core/public'; | ||
import { App, AppNavLinkStatus, ApplicationStart } from '../../../../../core/public'; | ||
import { useApplications, useWorkspaceTemplate } from '../../hooks'; | ||
import { WORKSPACE_OP_TYPE_CREATE, WORKSPACE_OP_TYPE_UPDATE } from '../../../common/constants'; | ||
import { | ||
isFeatureDependOnSelectedFeatures, | ||
getFinalFeatureIdsByDependency, | ||
} from '../utils/feature'; | ||
|
||
import { WorkspaceIconSelector } from './workspace_icon_selector'; | ||
|
||
interface WorkspaceFeature { | ||
interface WorkspaceFeature extends Pick<App, 'dependencies'> { | ||
id: string; | ||
name: string; | ||
templates: WorkspaceTemplate[]; | ||
|
@@ -74,6 +79,7 @@ interface WorkspaceFormProps { | |
defaultValues?: WorkspaceFormData; | ||
opType?: string; | ||
} | ||
|
||
export const WorkspaceForm = ({ | ||
application, | ||
onSubmit, | ||
|
@@ -115,13 +121,16 @@ export const WorkspaceForm = ({ | |
const apps = category2Applications[currentKey]; | ||
const features = apps | ||
.filter( | ||
({ navLinkStatus, chromeless }) => | ||
navLinkStatus !== AppNavLinkStatus.hidden && !chromeless | ||
({ navLinkStatus, chromeless, featureGroup }) => | ||
navLinkStatus !== AppNavLinkStatus.hidden && | ||
!chromeless && | ||
!featureGroup?.includes('ADMIN') | ||
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. Should it check 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. For now, the application doesn't include 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.
Makes sense to me, then we can check if |
||
) | ||
.map(({ id, title, workspaceTemplate }) => ({ | ||
.map(({ id, title, workspaceTemplate, dependencies }) => ({ | ||
id, | ||
name: title, | ||
templates: workspaceTemplate || [], | ||
dependencies, | ||
})); | ||
if (features.length === 1 || currentKey === 'undefined') { | ||
return [...previousValue, ...features]; | ||
|
@@ -141,6 +150,38 @@ export const WorkspaceForm = ({ | |
[defaultVISTheme] | ||
); | ||
|
||
const allFeatures = useMemo( | ||
() => | ||
featureOrGroups.reduce<WorkspaceFeature[]>( | ||
(previousData, currentData) => [ | ||
...previousData, | ||
...(isWorkspaceFeatureGroup(currentData) ? currentData.features : [currentData]), | ||
], | ||
[] | ||
), | ||
[featureOrGroups] | ||
); | ||
|
||
const featureDependencies = useMemo( | ||
() => | ||
allFeatures.reduce<{ [key: string]: string[] }>( | ||
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. Can you leave a comment here? It's hard to understand the code. Or maybe extract it to a function? |
||
(pValue, { id, dependencies }) => | ||
dependencies | ||
? { | ||
...pValue, | ||
[id]: [ | ||
...(pValue[id] || []), | ||
...Object.keys(dependencies).filter( | ||
(key) => dependencies[key].type === 'required' | ||
), | ||
], | ||
} | ||
: pValue, | ||
{} | ||
), | ||
[allFeatures] | ||
); | ||
|
||
if (!formIdRef.current) { | ||
formIdRef.current = workspaceHtmlIdGenerator(); | ||
} | ||
|
@@ -150,27 +191,33 @@ export const WorkspaceForm = ({ | |
const templateId = e.target.value; | ||
setSelectedTemplateId(templateId); | ||
setSelectedFeatureIds( | ||
featureOrGroups.reduce<string[]>( | ||
(previousData, currentData) => [ | ||
...previousData, | ||
...(isWorkspaceFeatureGroup(currentData) ? currentData.features : [currentData]) | ||
.filter(({ templates }) => !!templates.find((template) => template.id === templateId)) | ||
.map((feature) => feature.id), | ||
], | ||
[] | ||
getFinalFeatureIdsByDependency( | ||
allFeatures | ||
.filter(({ templates }) => !!templates.find((template) => template.id === templateId)) | ||
.map((feature) => feature.id), | ||
featureDependencies | ||
) | ||
); | ||
}, | ||
[featureOrGroups] | ||
[allFeatures, featureDependencies] | ||
); | ||
|
||
const handleFeatureChange = useCallback<EuiCheckboxGroupProps['onChange']>((featureId) => { | ||
setSelectedFeatureIds((previousData) => | ||
previousData.includes(featureId) | ||
? previousData.filter((id) => id !== featureId) | ||
: [...previousData, featureId] | ||
); | ||
}, []); | ||
const handleFeatureChange = useCallback<EuiCheckboxGroupProps['onChange']>( | ||
(featureId) => { | ||
setSelectedFeatureIds((previousData) => { | ||
if (!previousData.includes(featureId)) { | ||
return getFinalFeatureIdsByDependency([featureId], featureDependencies, previousData); | ||
} | ||
|
||
if (isFeatureDependOnSelectedFeatures(featureId, previousData, featureDependencies)) { | ||
return previousData; | ||
} | ||
|
||
return previousData.filter((selectedId) => selectedId !== featureId); | ||
}); | ||
}, | ||
[featureDependencies] | ||
); | ||
|
||
const handleFeatureCheckboxChange = useCallback<EuiCheckboxProps['onChange']>( | ||
(e) => { | ||
|
@@ -187,14 +234,37 @@ export const WorkspaceForm = ({ | |
setSelectedFeatureIds((previousData) => { | ||
const notExistsIds = groupFeatureIds.filter((id) => !previousData.includes(id)); | ||
if (notExistsIds.length > 0) { | ||
return [...previousData, ...notExistsIds]; | ||
return getFinalFeatureIdsByDependency( | ||
notExistsIds, | ||
featureDependencies, | ||
previousData | ||
); | ||
} | ||
return previousData.filter((id) => !groupFeatureIds.includes(id)); | ||
let groupRemainFeatureIds = groupFeatureIds; | ||
const outGroupFeatureIds = previousData.filter( | ||
(featureId) => !groupFeatureIds.includes(featureId) | ||
); | ||
|
||
while (true) { | ||
const lastRemainFeatures = groupRemainFeatureIds.length; | ||
groupRemainFeatureIds = groupRemainFeatureIds.filter((featureId) => | ||
isFeatureDependOnSelectedFeatures( | ||
featureId, | ||
[...outGroupFeatureIds, ...groupRemainFeatureIds], | ||
featureDependencies | ||
) | ||
); | ||
if (lastRemainFeatures === groupRemainFeatureIds.length) { | ||
break; | ||
} | ||
} | ||
|
||
return [...outGroupFeatureIds, ...groupRemainFeatureIds]; | ||
}); | ||
} | ||
} | ||
}, | ||
[featureOrGroups] | ||
[featureOrGroups, featureDependencies] | ||
); | ||
|
||
const handleFormSubmit = useCallback<FormEventHandler>( | ||
|
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 seems this function is checking if
featureId
is depend by selected features? So we want to know if selectedselectedFeatureIds
, shouldfeatureId
been selected. Am I right?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.
You're right. I think we can rename
isFeatureDependOnSelectedFeatures
toisFeatureDependBySelectedFeatures