apply most of the code review feedback

datahub-project · Aug 21, 2023 · e2516be · e2516be
1 parent c577729
commit e2516be
Show file tree

Hide file tree

Showing 12 changed files with 78 additions and 50 deletions.
diff --git a/datahub-graphql-core/src/main/resources/search.graphql b/datahub-graphql-core/src/main/resources/search.graphql
@@ -667,7 +667,7 @@ type MatchedField {
   value: String!
 
   """
-  Entity if the value was an urn
+  Entity if the value is an urn
   """
   entity: Entity
 }

diff --git a/datahub-web-react/src/app/entity/chart/ChartEntity.tsx b/datahub-web-react/src/app/entity/chart/ChartEntity.tsx
@@ -19,7 +19,7 @@ import { EntityMenuItems } from '../shared/EntityDropdown/EntityDropdown';
 import { LineageTab } from '../shared/tabs/Lineage/LineageTab';
 import { ChartStatsSummarySubHeader } from './profile/stats/ChartStatsSummarySubHeader';
 import { InputFieldsTab } from '../shared/tabs/Entity/InputFieldsTab';
-import { chartMatchedFieldRenderer } from './chartMatchedFieldRenderer';
+import { matchedInputFieldRenderer } from '../../search/matchedInputFieldRenderer';
 import { EmbedTab } from '../shared/tabs/Embed/EmbedTab';
 import { capitalizeFirstLetterOnly } from '../../shared/textUtil';
 import DataProductSection from '../shared/containers/profile/sidebar/DataProduct/DataProductSection';
@@ -206,7 +206,7 @@ export class ChartEntity implements Entity<Chart> {
                 externalUrl={data.properties?.externalUrl}
                 snippet={
                     <MatchedFieldList
-                        customFieldRenderer={(matchedField) => chartMatchedFieldRenderer(matchedField, data)}
+                        customFieldRenderer={(matchedField) => matchedInputFieldRenderer(matchedField, data)}
                     />
                 }
                 degree={(result as any).degree}

diff --git a/datahub-web-react/src/app/entity/dashboard/DashboardEntity.tsx b/datahub-web-react/src/app/entity/dashboard/DashboardEntity.tsx
@@ -24,7 +24,7 @@ import { EntityMenuItems } from '../shared/EntityDropdown/EntityDropdown';
 import { LineageTab } from '../shared/tabs/Lineage/LineageTab';
 import { capitalizeFirstLetterOnly } from '../../shared/textUtil';
 import { DashboardStatsSummarySubHeader } from './profile/DashboardStatsSummarySubHeader';
-import { chartMatchedFieldRenderer } from '../chart/chartMatchedFieldRenderer';
+import { matchedInputFieldRenderer } from '../../search/matchedInputFieldRenderer';
 import { EmbedTab } from '../shared/tabs/Embed/EmbedTab';
 import EmbeddedProfile from '../shared/embed/EmbeddedProfile';
 import DataProductSection from '../shared/containers/profile/sidebar/DataProduct/DataProductSection';
@@ -229,7 +229,8 @@ export class DashboardEntity implements Entity<Dashboard> {
                 createdMs={data.properties?.created?.time}
                 snippet={
                     <MatchedFieldList
-                        customFieldRenderer={(matchedField) => chartMatchedFieldRenderer(matchedField, data)}
+                        customFieldRenderer={(matchedField) => matchedInputFieldRenderer(matchedField, data)}
+                        matchSuffix="on a contained chart"
                     />
                 }
                 subtype={data.subTypes?.typeNames?.[0]}

diff --git a/datahub-web-react/src/app/entity/dataset/DatasetEntity.tsx b/datahub-web-react/src/app/entity/dataset/DatasetEntity.tsx
@@ -30,7 +30,7 @@ import { EmbedTab } from '../shared/tabs/Embed/EmbedTab';
 import EmbeddedProfile from '../shared/embed/EmbeddedProfile';
 import DataProductSection from '../shared/containers/profile/sidebar/DataProduct/DataProductSection';
 import { getDataProduct } from '../shared/utils';
-import { datasetMatchedFieldRenderer } from './datasetMatchedFieldRenderer';
+import { matchedFieldPathsRenderer } from '../../search/matchedFieldPathsRenderer';
 
 const SUBTYPES = {
     VIEW: 'view',
@@ -291,7 +291,7 @@ export class DatasetEntity implements Entity<Dataset> {
                 subtype={data.subTypes?.typeNames?.[0]}
                 container={data.container}
                 parentContainers={data.parentContainers}
-                snippet={<MatchedFieldList customFieldRenderer={datasetMatchedFieldRenderer} />}
+                snippet={<MatchedFieldList customFieldRenderer={matchedFieldPathsRenderer} />}
                 insights={result.insights}
                 externalUrl={data.properties?.externalUrl}
                 statsSummary={data.statsSummary}

diff --git a/datahub-web-react/src/app/entity/dataset/datasetMatchedFieldRenderer.tsx b/datahub-web-react/src/app/entity/dataset/datasetMatchedFieldRenderer.tsx
diff --git a/datahub-web-react/src/app/search/MatchedFieldList.tsx b/datahub-web-react/src/app/search/MatchedFieldList.tsx
@@ -8,6 +8,7 @@ import { ANTD_GRAY_V2 } from '../entity/shared/constants';
 import { useSearchQuery } from './context/SearchContext';
 import { MatchesGroupedByFieldName } from './context/constants';
 import { useEntityRegistry } from '../useEntityRegistry';
+import { getDescriptionSlice, isDescriptionField, isHighlightableEntityField } from './context/utils';
 
 const MatchesContainer = styled.div`
     display: flex;
@@ -23,14 +24,14 @@ const MatchText = styled(Typography.Text)`
     padding-right: 4px;
 `;
 
-const SURROUNDING_DESCRIPTION_CHARS = 10;
 const MATCH_GROUP_LIMIT = 3;
 const TOOLTIP_MATCH_GROUP_LIMIT = 10;
 
 type CustomFieldRenderer = (field: MatchedField) => JSX.Element | null;
 
 type Props = {
     customFieldRenderer?: CustomFieldRenderer;
+    matchSuffix?: string;
 };
 
 const RenderedField = ({
@@ -44,34 +45,24 @@ const RenderedField = ({
     const query = useSearchQuery()?.trim().toLowerCase();
     const customRenderedField = customFieldRenderer?.(field);
     if (customRenderedField) return <b>{customRenderedField}</b>;
-    if (field.value.includes('urn:li:tag') && !field.entity) return <></>;
-    if (field.entity) return <>{entityRegistry.getDisplayName(field.entity.type, field.entity)}</>;
-    if (field.name.toLowerCase().includes('description') && query) {
-        const queryIndex = field.value.indexOf(query);
-        const start = Math.max(0, queryIndex - SURROUNDING_DESCRIPTION_CHARS);
-        const end = Math.min(field.value.length, queryIndex + query.length + SURROUNDING_DESCRIPTION_CHARS);
-        const startEllipsis = start > 0 ? '...' : undefined;
-        const endEllipsis = end < field.value.length ? '...' : undefined;
-        return (
-            <b>
-                {startEllipsis}
-                {field.value.slice(start, end)}
-                {endEllipsis}
-            </b>
-        );
+    if (isHighlightableEntityField(field)) {
+        return field.entity ? <>{entityRegistry.getDisplayName(field.entity.type, field.entity)}</> : <></>;
     }
+    if (isDescriptionField(field) && query) return <b>{getDescriptionSlice(field.value, query)}</b>;
     return <b>{field.value}</b>;
 };
 
 const MatchedFieldsList = ({
     groupedMatch,
     limit,
     tooltip,
+    matchSuffix = '',
     customFieldRenderer,
 }: {
     groupedMatch: MatchesGroupedByFieldName;
     limit: number;
     tooltip?: JSX.Element;
+    matchSuffix?: string;
     customFieldRenderer?: CustomFieldRenderer;
 }) => {
     const label = useMatchedFieldLabel(groupedMatch.fieldName);
@@ -103,12 +94,13 @@ const MatchedFieldsList = ({
                     </Tooltip>
                 ) : (
                     <>{andMore}</>
-                ))}
+                ))}{' '}
+            {matchSuffix}
         </>
     );
 };
 
-export const MatchedFieldList = ({ customFieldRenderer }: Props) => {
+export const MatchedFieldList = ({ customFieldRenderer, matchSuffix = '' }: Props) => {
     const groupedMatches = useMatchedFieldsForList('fieldLabels');
 
     return (
@@ -122,6 +114,7 @@ export const MatchedFieldList = ({ customFieldRenderer }: Props) => {
                                     groupedMatch={groupedMatch}
                                     limit={MATCH_GROUP_LIMIT}
                                     customFieldRenderer={customFieldRenderer}
+                                    matchSuffix={matchSuffix}
                                     tooltip={
                                         <MatchedFieldsList
                                             groupedMatch={groupedMatch}

diff --git a/datahub-web-react/src/app/search/context/SearchResultContext.tsx b/datahub-web-react/src/app/search/context/SearchResultContext.tsx
@@ -6,7 +6,7 @@ import {
     getMatchedFieldsByNames,
     shouldShowInMatchedFieldList,
     getMatchedFieldLabel,
-    getMatchesPrioritizingPrimary,
+    getMatchesPrioritized,
 } from './utils';
 import { MatchedFieldName } from './constants';
 
@@ -51,7 +51,7 @@ export const useMatchedFieldsForList = (primaryField: MatchedFieldName) => {
     const entityType = useEntityType();
     const matchedFields = useMatchedFields();
     const showableFields = matchedFields.filter((field) => shouldShowInMatchedFieldList(entityType, field));
-    return entityType ? getMatchesPrioritizingPrimary(entityType, showableFields, primaryField) : [];
+    return entityType ? getMatchesPrioritized(entityType, showableFields, primaryField) : [];
 };
 
 export const useMatchedFieldsByGroup = (fieldName: MatchedFieldName) => {

diff --git a/datahub-web-react/src/app/search/context/constants.ts b/datahub-web-react/src/app/search/context/constants.ts
@@ -148,3 +148,5 @@ export type MatchesGroupedByFieldName = {
     fieldName: string;
     matchedFields: Array<MatchedField>;
 };
+
+export const HIGHLIGHTABLE_ENTITY_TYPES = [EntityType.Tag, EntityType.GlossaryTerm];
diff --git a/datahub-web-react/src/app/search/context/utils.test.ts b/datahub-web-react/src/app/search/context/utils.test.ts
@@ -1,5 +1,5 @@
 import { EntityType } from '../../../types.generated';
-import { getMatchesPrioritizingPrimary } from './utils';
+import { getMatchesPrioritized } from './utils';
 
 const mapping = new Map();
 mapping.set('fieldPaths', 'column');
@@ -48,7 +48,7 @@ describe('utils', () => {
     describe('getMatchPrioritizingPrimary', () => {
         it('prioritizes exact match', () => {
             global.window.location.search = 'query=rainbow';
-            const groupedMatches = getMatchesPrioritizingPrimary(EntityType.Dataset, MOCK_MATCHED_FIELDS, 'fieldPaths');
+            const groupedMatches = getMatchesPrioritized(EntityType.Dataset, MOCK_MATCHED_FIELDS, 'fieldPaths');
             expect(groupedMatches).toEqual([
                 {
                     fieldName: 'fieldPaths',
@@ -66,7 +66,7 @@ describe('utils', () => {
         });
         it('will accept first contains match', () => {
             global.window.location.search = 'query=bow';
-            const groupedMatches = getMatchesPrioritizingPrimary(EntityType.Dataset, MOCK_MATCHED_FIELDS, 'fieldPaths');
+            const groupedMatches = getMatchesPrioritized(EntityType.Dataset, MOCK_MATCHED_FIELDS, 'fieldPaths');
             expect(groupedMatches).toEqual([
                 {
                     fieldName: 'fieldPaths',
@@ -84,7 +84,7 @@ describe('utils', () => {
         });
         it('will group by field name', () => {
             global.window.location.search = '';
-            const groupedMatches = getMatchesPrioritizingPrimary(
+            const groupedMatches = getMatchesPrioritized(
                 EntityType.Dataset,
                 MOCK_MATCHED_DESCRIPTION_FIELDS,
                 'fieldPaths',

diff --git a/datahub-web-react/src/app/search/context/utils.ts b/datahub-web-react/src/app/search/context/utils.ts
@@ -1,6 +1,12 @@
 import * as QueryString from 'query-string';
 import { EntityType, MatchedField } from '../../../types.generated';
-import { MATCHED_FIELD_CONFIG, MatchedFieldConfig, MatchedFieldName, MatchesGroupedByFieldName } from './constants';
+import {
+    HIGHLIGHTABLE_ENTITY_TYPES,
+    MATCHED_FIELD_CONFIG,
+    MatchedFieldConfig,
+    MatchedFieldName,
+    MatchesGroupedByFieldName,
+} from './constants';
 
 const getFieldConfigsByEntityType = (entityType: EntityType | undefined): Array<MatchedFieldConfig> => {
     return entityType && entityType in MATCHED_FIELD_CONFIG
@@ -51,16 +57,20 @@ function normalize(value: string) {
 function fromQueryGetBestMatch(
     selectedMatchedFields: MatchedField[],
     rawQuery: string,
-    primaryField: string,
+    prioritizedField: string,
 ): Array<MatchedField> {
     const query = normalize(rawQuery);
-    const primaryMatches: Array<MatchedField> = selectedMatchedFields.filter((field) => field.name === primaryField);
-    const nonPrimaryMatches: Array<MatchedField> = selectedMatchedFields.filter((field) => field.name !== primaryField);
+    const priorityMatches: Array<MatchedField> = selectedMatchedFields.filter(
+        (field) => field.name === prioritizedField,
+    );
+    const nonPriorityMatches: Array<MatchedField> = selectedMatchedFields.filter(
+        (field) => field.name !== prioritizedField,
+    );
     const exactMatches: Array<MatchedField> = [];
     const containedMatches: Array<MatchedField> = [];
     const rest: Array<MatchedField> = [];
 
-    [...primaryMatches, ...nonPrimaryMatches].forEach((field) => {
+    [...priorityMatches, ...nonPriorityMatches].forEach((field) => {
         const normalizedValue = normalize(field.value);
         if (normalizedValue === query) exactMatches.push(field);
         else if (normalizedValue.includes(query)) containedMatches.push(field);
@@ -92,14 +102,35 @@ const getMatchesGroupedByFieldName = (
     }));
 };
 
-export const getMatchesPrioritizingPrimary = (
+export const getMatchesPrioritized = (
     entityType: EntityType,
     matchedFields: MatchedField[],
-    primaryField: string,
+    prioritizedField: string,
 ): Array<MatchesGroupedByFieldName> => {
     const { location } = window;
     const params = QueryString.parse(location.search, { arrayFormat: 'comma' });
     const query: string = decodeURIComponent(params.query ? (params.query as string) : '');
-    const matches = fromQueryGetBestMatch(matchedFields, query, primaryField);
+    const matches = fromQueryGetBestMatch(matchedFields, query, prioritizedField);
     return getMatchesGroupedByFieldName(entityType, matches);
 };
+
+export const isHighlightableEntityField = (field: MatchedField) =>
+    !!field.entity && HIGHLIGHTABLE_ENTITY_TYPES.includes(field.entity.type);
+
+export const isDescriptionField = (field: MatchedField) => field.name.toLowerCase().includes('description');
+
+const SURROUNDING_DESCRIPTION_CHARS = 10;
+const MAX_DESCRIPTION_CHARS = 50;
+
+export const getDescriptionSlice = (text: string, target: string) => {
+    const queryIndex = text.indexOf(target);
+    const start = Math.max(0, queryIndex - SURROUNDING_DESCRIPTION_CHARS);
+    const end = Math.min(
+        start + MAX_DESCRIPTION_CHARS,
+        text.length,
+        queryIndex + target.length + SURROUNDING_DESCRIPTION_CHARS,
+    );
+    const startEllipsis = start > 0 ? '...' : '';
+    const endEllipsis = end < text.length ? '...' : '';
+    return `${startEllipsis}${text.slice(start, end)}${endEllipsis}`;
+};
diff --git a/datahub-web-react/src/app/search/matchedFieldPathsRenderer.tsx b/datahub-web-react/src/app/search/matchedFieldPathsRenderer.tsx
@@ -0,0 +1,8 @@
+import React from 'react';
+
+import { MatchedField } from '../../types.generated';
+import { downgradeV2FieldPath } from '../entity/dataset/profile/schema/utils/utils';
+
+export const matchedFieldPathsRenderer = (matchedField: MatchedField) => {
+    return matchedField?.name === 'fieldPaths' ? <b>{downgradeV2FieldPath(matchedField.value)}</b> : null;
+};
diff --git a/...ntity/chart/chartMatchedFieldRenderer.tsx → .../app/search/matchedInputFieldRenderer.tsx b/...ntity/chart/chartMatchedFieldRenderer.tsx → .../app/search/matchedInputFieldRenderer.tsx
@@ -1,7 +1,7 @@
 import React from 'react';
 
-import { Chart, Dashboard, EntityType, GlossaryTerm, MatchedField } from '../../../types.generated';
-import { useEntityRegistry } from '../../useEntityRegistry';
+import { Chart, Dashboard, EntityType, GlossaryTerm, MatchedField } from '../../types.generated';
+import { useEntityRegistry } from '../useEntityRegistry';
 
 const LABEL_INDEX_NAME = 'fieldLabels';
 const TYPE_PROPERTY_KEY_NAME = 'type';
@@ -11,7 +11,8 @@ const TermName = ({ term }: { term: GlossaryTerm }) => {
     return <>{entityRegistry.getDisplayName(EntityType.GlossaryTerm, term)}</>;
 };
 
-export const chartMatchedFieldRenderer = (matchedField: MatchedField, entity: Chart | Dashboard) => {
+export const matchedInputFieldRenderer = (matchedField: MatchedField, entity: Chart | Dashboard) => {
+    if (1) return <>hi</>;
     if (matchedField?.name === LABEL_INDEX_NAME) {
         const matchedSchemaField = entity.inputFields?.fields?.find(
             (field) => field?.schemaField?.label === matchedField.value,