From 4e842ca4788bfec0bd7c9a42a96b6e7e31019b19 Mon Sep 17 00:00:00 2001 From: Chris Collins Date: Tue, 3 Dec 2024 10:44:04 -0500 Subject: [PATCH 1/3] fix(ui) Add backwards compatibility to the UI for old policy filters --- .../permissions/policy/PolicyDetailsModal.tsx | 8 +- .../policy/PolicyPrivilegeForm.tsx | 29 +++-- .../policy/_tests_/policyUtils.test.tsx | 115 +++++++++++++++++- .../src/app/permissions/policy/constants.ts | 4 + .../src/app/permissions/policy/policyUtils.ts | 24 +++- 5 files changed, 164 insertions(+), 16 deletions(-) create mode 100644 datahub-web-react/src/app/permissions/policy/constants.ts diff --git a/datahub-web-react/src/app/permissions/policy/PolicyDetailsModal.tsx b/datahub-web-react/src/app/permissions/policy/PolicyDetailsModal.tsx index 37349585fa4c92..1988fea3496992 100644 --- a/datahub-web-react/src/app/permissions/policy/PolicyDetailsModal.tsx +++ b/datahub-web-react/src/app/permissions/policy/PolicyDetailsModal.tsx @@ -12,6 +12,7 @@ import { mapResourceTypeToDisplayName, } from './policyUtils'; import AvatarsGroup from '../AvatarsGroup'; +import { RESOURCE_TYPE, RESOURCE_URN, TYPE, URN } from './constants'; type PrivilegeOptionType = { type?: string; @@ -72,10 +73,11 @@ export default function PolicyDetailsModal({ policy, open, onClose, privileges } const isMetadataPolicy = policy?.type === PolicyType.Metadata; const resources = convertLegacyResourceFilter(policy?.resources); - const resourceTypes = getFieldValues(resources?.filter, 'TYPE') || []; + const resourceTypes = getFieldValues(resources?.filter, TYPE, RESOURCE_TYPE) || []; const dataPlatformInstances = getFieldValues(resources?.filter, 'DATA_PLATFORM_INSTANCE') || []; - const resourceEntities = getFieldValues(resources?.filter, 'URN') || []; - const resourceFilterCondition = getFieldCondition(resources?.filter, 'URN') || PolicyMatchCondition.Equals; + const resourceEntities = getFieldValues(resources?.filter, URN, RESOURCE_URN) || []; + const resourceFilterCondition = + getFieldCondition(resources?.filter, URN, RESOURCE_URN) || PolicyMatchCondition.Equals; const domains = getFieldValues(resources?.filter, 'DOMAIN') || []; const { diff --git a/datahub-web-react/src/app/permissions/policy/PolicyPrivilegeForm.tsx b/datahub-web-react/src/app/permissions/policy/PolicyPrivilegeForm.tsx index 7a0de67f414192..414346c2776db8 100644 --- a/datahub-web-react/src/app/permissions/policy/PolicyPrivilegeForm.tsx +++ b/datahub-web-react/src/app/permissions/policy/PolicyPrivilegeForm.tsx @@ -28,6 +28,7 @@ import ClickOutside from '../../shared/ClickOutside'; import { TagTermLabel } from '../../shared/tags/TagTermLabel'; import { ENTER_KEY_CODE } from '../../shared/constants'; import { useGetRecommendations } from '../../shared/recommendation'; +import { RESOURCE_TYPE, RESOURCE_URN, TYPE, URN } from './constants'; type Props = { policyType: PolicyType; @@ -102,8 +103,9 @@ export default function PolicyPrivilegeForm({ } = useAppConfig(); const resources: ResourceFilter = convertLegacyResourceFilter(maybeResources) || EMPTY_POLICY.resources; - const resourceTypes = getFieldValues(resources.filter, 'TYPE') || []; - const resourceEntities = getFieldValues(resources.filter, 'URN') || []; + // RESOURCE_TYPE and RESOURCE_URN are deprecated, but need to get them for backwards compatibility + const resourceTypes = getFieldValues(resources.filter, TYPE, RESOURCE_TYPE) || []; + const resourceEntities = getFieldValues(resources.filter, URN, RESOURCE_URN) || []; const getDisplayName = (entity) => { if (!entity) { @@ -178,9 +180,14 @@ export default function PolicyPrivilegeForm({ const filter = resources.filter || { criteria: [], }; + // remove the deprecated RESOURCE_TYPE field and replace with TYPE field + const filterWithoutDeprecatedField = setFieldValues(filter, RESOURCE_TYPE, []); setResources({ ...resources, - filter: setFieldValues(filter, 'TYPE', [...resourceTypes, createCriterionValue(selectedResourceType)]), + filter: setFieldValues(filterWithoutDeprecatedField, TYPE, [ + ...resourceTypes, + createCriterionValue(selectedResourceType), + ]), }); }; @@ -188,11 +195,13 @@ export default function PolicyPrivilegeForm({ const filter = resources.filter || { criteria: [], }; + // remove the deprecated RESOURCE_TYPE field and replace with TYPE field + const filterWithoutDeprecatedField = setFieldValues(filter, RESOURCE_TYPE, []); setResources({ ...resources, filter: setFieldValues( - filter, - 'TYPE', + filterWithoutDeprecatedField, + TYPE, resourceTypes?.filter((criterionValue) => criterionValue.value !== deselectedResourceType), ), }); @@ -203,9 +212,11 @@ export default function PolicyPrivilegeForm({ const filter = resources.filter || { criteria: [], }; + // remove the deprecated RESOURCE_URN field and replace with URN field + const filterWithoutDeprecatedField = setFieldValues(filter, RESOURCE_URN, []); setResources({ ...resources, - filter: setFieldValues(filter, 'URN', [ + filter: setFieldValues(filterWithoutDeprecatedField, URN, [ ...resourceEntities, createCriterionValueWithEntity( resource, @@ -220,11 +231,13 @@ export default function PolicyPrivilegeForm({ const filter = resources.filter || { criteria: [], }; + // remove the deprecated RESOURCE_URN field and replace with URN field + const filterWithoutDeprecatedField = setFieldValues(filter, RESOURCE_URN, []); setResources({ ...resources, filter: setFieldValues( - filter, - 'URN', + filterWithoutDeprecatedField, + URN, resourceEntities?.filter((criterionValue) => criterionValue.value !== resource), ), }); diff --git a/datahub-web-react/src/app/permissions/policy/_tests_/policyUtils.test.tsx b/datahub-web-react/src/app/permissions/policy/_tests_/policyUtils.test.tsx index 1c9884e5fcf09c..89b6df49980062 100644 --- a/datahub-web-react/src/app/permissions/policy/_tests_/policyUtils.test.tsx +++ b/datahub-web-react/src/app/permissions/policy/_tests_/policyUtils.test.tsx @@ -1,4 +1,11 @@ -import { addOrUpdatePoliciesInList, updateListPoliciesCache, removeFromListPoliciesCache } from '../policyUtils'; +import { PolicyMatchCondition } from '@src/types.generated'; +import { + addOrUpdatePoliciesInList, + updateListPoliciesCache, + removeFromListPoliciesCache, + getFieldValues, + getFieldCondition, +} from '../policyUtils'; // Mock the Apollo Client readQuery and writeQuery methods const mockReadQuery = vi.fn(); @@ -103,3 +110,109 @@ describe('removeFromListPoliciesCache', () => { }); }); }); + +describe('getFieldValues', () => { + it('should get field values for a given field', () => { + const filter = { + criteria: [ + { + condition: PolicyMatchCondition.Equals, + field: 'TYPE', + values: [{ value: 'dataset' }, { value: 'dataJob' }], + }, + ], + }; + + expect(getFieldValues(filter, 'TYPE')).toMatchObject([{ value: 'dataset' }, { value: 'dataJob' }]); + }); + + it('should get field values for a alternate field (for deprecated fields)', () => { + const filter = { + criteria: [ + { + condition: PolicyMatchCondition.Equals, + field: 'RESOURCE_TYPE', + values: [{ value: 'dataset' }, { value: 'dataJob' }], + }, + ], + }; + + expect(getFieldValues(filter, 'TYPE', 'RESOURCE_TYPE')).toMatchObject([ + { value: 'dataset' }, + { value: 'dataJob' }, + ]); + }); + + it('should get field values for main field with alternative field given and has values', () => { + const filter = { + criteria: [ + { + condition: PolicyMatchCondition.Equals, + field: 'RESOURCE_TYPE', + values: [{ value: 'container' }, { value: 'dataFlow' }], + }, + { + condition: PolicyMatchCondition.Equals, + field: 'TYPE', + values: [{ value: 'dataset' }, { value: 'dataJob' }], + }, + ], + }; + + // should only return values from main field + expect(getFieldValues(filter, 'TYPE', 'RESOURCE_TYPE')).toMatchObject([ + { value: 'dataset' }, + { value: 'dataJob' }, + ]); + }); +}); + +describe('getFieldCondition', () => { + it('should get field values for a given field', () => { + const filter = { + criteria: [ + { + condition: PolicyMatchCondition.Equals, + field: 'TYPE', + values: [{ value: 'dataset' }], + }, + ], + }; + + expect(getFieldCondition(filter, 'TYPE')).toBe(PolicyMatchCondition.Equals); + }); + + it('should get field values for a alternate field (for deprecated fields)', () => { + const filter = { + criteria: [ + { + condition: PolicyMatchCondition.Equals, + field: 'RESOURCE_TYPE', + values: [{ value: 'dataset' }], + }, + ], + }; + + expect(getFieldCondition(filter, 'TYPE', 'RESOURCE_TYPE')).toBe(PolicyMatchCondition.Equals); + }); + + it('should get field values for main field with alternative field given and has values', () => { + const filter = { + criteria: [ + { + condition: PolicyMatchCondition.StartsWith, + field: 'RESOURCE_TYPE', + values: [{ value: 'container' }, { value: 'dataFlow' }], + }, + { + condition: PolicyMatchCondition.Equals, + field: 'TYPE', + values: [{ value: 'dataset' }], + }, + ], + }; + + // should only return values from main field + expect(getFieldCondition(filter, 'TYPE', 'RESOURCE_TYPE')).toBe(PolicyMatchCondition.Equals); + }); +}); diff --git a/datahub-web-react/src/app/permissions/policy/constants.ts b/datahub-web-react/src/app/permissions/policy/constants.ts new file mode 100644 index 00000000000000..cdd20bf9b50d6a --- /dev/null +++ b/datahub-web-react/src/app/permissions/policy/constants.ts @@ -0,0 +1,4 @@ +export const TYPE = 'TYPE'; +export const RESOURCE_TYPE = 'RESOURCE_TYPE'; +export const URN = 'URN'; +export const RESOURCE_URN = 'RESOURCE_URN'; diff --git a/datahub-web-react/src/app/permissions/policy/policyUtils.ts b/datahub-web-react/src/app/permissions/policy/policyUtils.ts index b71a38f80fc256..d6221a0a9293af 100644 --- a/datahub-web-react/src/app/permissions/policy/policyUtils.ts +++ b/datahub-web-react/src/app/permissions/policy/policyUtils.ts @@ -114,12 +114,28 @@ export const convertLegacyResourceFilter = (resourceFilter: Maybe | undefined, resourceFieldType: string) => { - return filter?.criteria?.find((criterion) => criterion.field === resourceFieldType)?.values || []; +export const getFieldValues = ( + filter: Maybe | undefined, + resourceFieldType: string, + alternateResourceFieldType?: string, +) => { + return ( + filter?.criteria?.find((criterion) => criterion.field === resourceFieldType)?.values || + filter?.criteria?.find((criterion) => criterion.field === alternateResourceFieldType)?.values || + [] + ); }; -export const getFieldCondition = (filter: Maybe | undefined, resourceFieldType: string) => { - return filter?.criteria?.find((criterion) => criterion.field === resourceFieldType)?.condition || null; +export const getFieldCondition = ( + filter: Maybe | undefined, + resourceFieldType: string, + alternateResourceFieldType?: string, +) => { + return ( + filter?.criteria?.find((criterion) => criterion.field === resourceFieldType)?.condition || + filter?.criteria?.find((criterion) => criterion.field === alternateResourceFieldType)?.condition || + null + ); }; export const getFieldValuesOfTags = (filter: Maybe | undefined, resourceFieldType: string) => { From 23828fdf704834516566115a58a0ec5e5c6c41bb Mon Sep 17 00:00:00 2001 From: Chris Collins Date: Tue, 3 Dec 2024 11:04:45 -0500 Subject: [PATCH 2/3] add one more test --- .../policy/_tests_/policyUtils.test.tsx | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/datahub-web-react/src/app/permissions/policy/_tests_/policyUtils.test.tsx b/datahub-web-react/src/app/permissions/policy/_tests_/policyUtils.test.tsx index 89b6df49980062..72ad655ca74c57 100644 --- a/datahub-web-react/src/app/permissions/policy/_tests_/policyUtils.test.tsx +++ b/datahub-web-react/src/app/permissions/policy/_tests_/policyUtils.test.tsx @@ -5,6 +5,7 @@ import { removeFromListPoliciesCache, getFieldValues, getFieldCondition, + setFieldValues, } from '../policyUtils'; // Mock the Apollo Client readQuery and writeQuery methods @@ -216,3 +217,47 @@ describe('getFieldCondition', () => { expect(getFieldCondition(filter, 'TYPE', 'RESOURCE_TYPE')).toBe(PolicyMatchCondition.Equals); }); }); +describe('setFieldValues', () => { + it('should remove a field if you pass in an empty array', () => { + const filter = { + criteria: [ + { + condition: PolicyMatchCondition.Equals, + field: 'RESOURCE_TYPE', + values: [{ value: 'dataset' }], + }, + { + condition: PolicyMatchCondition.Equals, + field: 'TYPE', + values: [{ value: 'dataJob' }], + }, + ], + }; + + expect(setFieldValues(filter, 'RESOURCE_TYPE', [])).toMatchObject({ + criteria: [ + { + condition: PolicyMatchCondition.Equals, + field: 'TYPE', + values: [{ value: 'dataJob' }], + }, + ], + }); + }); + + it('should set values for a field properly', () => { + const filter = { + criteria: [], + }; + + expect(setFieldValues(filter, 'TYPE', [{ value: 'dataFlow' }])).toMatchObject({ + criteria: [ + { + condition: PolicyMatchCondition.Equals, + field: 'TYPE', + values: [{ value: 'dataFlow' }], + }, + ], + }); + }); +}); From 6297c269aa084693279ae32d57f468f6db67c59f Mon Sep 17 00:00:00 2001 From: Chris Collins Date: Tue, 10 Dec 2024 15:09:36 -0500 Subject: [PATCH 3/3] fix lint --- .../src/app/permissions/policy/_tests_/policyUtils.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datahub-web-react/src/app/permissions/policy/_tests_/policyUtils.test.tsx b/datahub-web-react/src/app/permissions/policy/_tests_/policyUtils.test.tsx index 72ad655ca74c57..eae735b3477f0f 100644 --- a/datahub-web-react/src/app/permissions/policy/_tests_/policyUtils.test.tsx +++ b/datahub-web-react/src/app/permissions/policy/_tests_/policyUtils.test.tsx @@ -1,4 +1,4 @@ -import { PolicyMatchCondition } from '@src/types.generated'; +import { PolicyMatchCondition } from '../../../../types.generated'; import { addOrUpdatePoliciesInList, updateListPoliciesCache,