Highlighted the description text on search #6400

Ankit-Keshari-Vituity · 2022-11-10T05:00:03Z

Checklist

The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
Links to related issues (if applicable)
Tests for the changes have been added/updated (if applicable)
Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

github-actions · 2022-11-10T05:31:43Z

Unit Test Results (build & test)

622 tests +9 618 ✔️ +9 15m 49s ⏱️ + 3m 51s
157 suites +6     4 💤 ±0
157 files +6     0 ❌ ±0

Results for commit 8f38f4c. ± Comparison against base commit ae2ea52.

♻️ This comment has been updated with latest results.

jjoyce0510 · 2022-11-14T23:22:29Z

datahub-web-react/src/app/entity/dataset/profile/schema/components/SchemaDescriptionField.tsx

@@ -161,6 +169,9 @@ export default function DescriptionField({ description, onUpdate, isEdited = fal
                        }
                        suffix={EditButton}
                    >
+                        {/* <Highlight matchStyle={highlightMatchStyle} search={highlightText}>


Why is this here? Can we remove

jjoyce0510 · 2022-11-14T23:23:03Z

datahub-web-react/src/app/entity/shared/tabs/Dataset/Schema/SchemaTable.tsx

@@ -69,7 +69,7 @@ export default function SchemaTable({
    const [selectedFkFieldPath, setSelectedFkFieldPath] =
        useState<null | { fieldPath: string; constraint?: ForeignKeyConstraint | null }>(null);

-    const descriptionRender = useDescriptionRenderer(editableSchemaMetadata);
+    const descriptionRender = useDescriptionRenderer(editableSchemaMetadata, filterText);


Where do we actually search the fields for this text?

chriscollins3456

nice! i have a suggestion on how to clean this up a bit, though, which should also improve filtering performance (minimize the number of times we need to loop through the list of rows)

could you also add tests to filterSchemaRows.test.ts?

chriscollins3456 · 2022-11-16T14:27:27Z

datahub-web-react/src/app/entity/shared/tabs/Dataset/Schema/utils/filterSchemaRows.ts

@@ -47,14 +71,19 @@ export function filterSchemaRows(
        entityRegistry,
        formattedFilterText,
    );
+
+    const filteredEditableDescription = getDescriptionFieldPathsByMetadata(editableSchemaMetadata, formattedFilterText);


since we need to do slightly more work for checking things inside of editableSchemaMetadata and we're already doing that in getFilteredFieldPathsByMetadata, how do you feel about moving the edited description filter check inside of that function? that way we just have one checking for things in editableSchemaMetadata.

We could then just have one filter function for matchesDescription like you already have that is reused between that function and down below similar to how matchesTagsOrTerms is being used twice

Worked on that and committed the changes. Please check.

chriscollins3456

nice! this is looking real close, just a few changes requested

chriscollins3456 · 2022-11-16T16:35:49Z

datahub-web-react/src/app/entity/shared/tabs/Dataset/Schema/utils/filterSchemaRows.ts

+function matchesDescription(fieldDescription: any, filterText: string) {
+    return fieldDescription?.toLocaleLowerCase().includes(filterText);
+}
+
 function matchesEditableTagsOrTerms(field: SchemaField, filteredFieldPathsByEditableMetadata: any) {


now that filteredFieldPathsByEditableMetadata is also checking by description, you can rename this function matchesEditableTagsOrTermsOrDescription

Fixed and committed!!

chriscollins3456 · 2022-11-16T16:37:10Z

datahub-web-react/src/app/entity/shared/tabs/Dataset/Schema/utils/filterSchemaRows.ts

            matchesEditableTagsOrTerms(row, filteredFieldPathsByEditableMetadata) ||
-            matchesTagsOrTerms(row, formattedFilterText, entityRegistry) // non-editable tags and terms
+            matchesEditableDescription(row, filteredFieldPathsByEditableMetadata) ||
+            matchesTagsOrTermsOrDescription(row, formattedFilterText, entityRegistry) || // non-editable tags and terms
+            matchesDescription(row.description, formattedFilterText) // non-editable description


since we can rename matchesEditableTagsOrTerms to matchesEditableTagsOrTermsOrDescription with the new functionality change, we don't need matchedEditableDescription here. also, since you're calling matchesTagsOrTermsOrDescription for non-editable info, you don't need matchesDescription either.

chriscollins3456 · 2022-11-16T16:37:40Z

datahub-web-react/src/app/entity/shared/tabs/Dataset/Schema/utils/filterSchemaRows.ts

            matchesEditableTagsOrTerms(row, filteredFieldPathsByEditableMetadata) ||
-            matchesTagsOrTerms(row, formattedFilterText, entityRegistry)
+            matchesEditableDescription(row, filteredFieldPathsByEditableMetadata) ||
+            matchesTagsOrTermsOrDescription(row, formattedFilterText, entityRegistry) || // non-editable tags and terms
+            matchesDescription(row.description, formattedFilterText) // non-editable description


same thing here, we can just check matchesFieldName, matchesEditableTagsOrTermsOrDescription, and matchesTagsOrTermsOrDescription

chriscollins3456 · 2022-11-16T16:38:35Z

datahub-web-react/src/app/entity/shared/tabs/Dataset/Schema/__tests__/filterSchemaRows.test.ts

@@ -35,6 +39,52 @@ describe('filterSchemaRows', () => {
        expect(expandedRowsFromFilter).toMatchObject(new Set());
    });

+    it('should properly filter schema rows based on description', () => {
+        const filterText = 'test';


"test" here will actually also match on the fieldPath. can you make this more specific to test that we're only matching on description? so you could just change this to const filterText = 'testing description';

chriscollins3456 · 2022-11-16T16:39:02Z

datahub-web-react/src/app/entity/shared/tabs/Dataset/Schema/__tests__/filterSchemaRows.test.ts

+
+    it('should properly filter schema rows based on description regardless of capitalization', () => {
+        const editableSchemaMetadata = { editableSchemaFieldInfo: [] };
+        const filterText = 'TeSt';


same here, can you change to 'TeSting DesCriptioN'? or to that effect

chriscollins3456 · 2022-11-16T16:40:35Z

datahub-web-react/src/app/entity/shared/tabs/Dataset/Schema/__tests__/filterSchemaRows.test.ts

+                { fieldPath: 'customer', description: 'customer description', globalTags: null, glossaryTerms: null },
+            ],
+        };
+        const filterText = sampleTag.properties.name;


can you make this description in editableSchemaFieldInfo something different to make sure we're testing that it's matching specifically on that? so you can change it to description: 'editable customer description' and then make the filter text editable customer description

then can you add one more test that's just like this but checking that we're filtering regardless of capitalization?

chriscollins3456 · 2022-11-16T17:09:18Z

datahub-web-react/src/app/entity/shared/tabs/Dataset/Schema/utils/filterSchemaRows.ts

+            matchesEditableTagsOrTermsOrDescription(row, filteredFieldPathsByEditableMetadata) ||
+            matchesEditableTagsOrTermsOrDescription(row, filteredFieldPathsByEditableMetadata) ||
+            matchesTagsOrTermsOrDescription(row, formattedFilterText, entityRegistry) || // non-editable tags and terms
+            matchesDescription(row.description, formattedFilterText) // non-editable description


so you don't need matchesEditableTagsOrTermsOrDescription twice, you can remove one of them.

Also you can remove matchesDescription since we're already checking for that in matchesTagsOrTermsOrDescription

chriscollins3456 · 2022-11-16T17:09:26Z

datahub-web-react/src/app/entity/shared/tabs/Dataset/Schema/utils/filterSchemaRows.ts

+            matchesEditableTagsOrTermsOrDescription(row, filteredFieldPathsByEditableMetadata) ||
+            matchesEditableTagsOrTermsOrDescription(row, filteredFieldPathsByEditableMetadata) ||
+            matchesTagsOrTermsOrDescription(row, formattedFilterText, entityRegistry) || // non-editable tags and terms
+            matchesDescription(row.description, formattedFilterText) // non-editable description


same comment here

chriscollins3456

nice! i have one minor comment on one of your test cases, but otherwise this is good to go

chriscollins3456 · 2022-11-17T14:40:33Z

datahub-web-react/src/app/entity/shared/tabs/Dataset/Schema/__tests__/filterSchemaRows.test.ts

+                },
+            ],
+        };
+        const filterText = 'EdiTable CuStoMer DesCriptioN';


isn't the point of this test to check that capitalization doesn't matter? I'd just make this all lowercase to actually test that (as opposed to the same piece of text that's in the description)

…ject#6400)

Highlighted the description text on search

c2b1c40

github-actions bot added the product PR or Issue related to the DataHub UI/UX label Nov 10, 2022

jjoyce0510 reviewed Nov 14, 2022

View reviewed changes

Worked on the description column for filtering

1fad1df

chriscollins3456 reviewed Nov 16, 2022

View reviewed changes

Ankit-Keshari-Vituity added 3 commits November 16, 2022 20:34

Added test cases and optimize code

59946e5

optimize the code

6ade12d

Optimize the code

7c496d0

chriscollins3456 reviewed Nov 16, 2022

View reviewed changes

removed the unused function

8514d8e

chriscollins3456 reviewed Nov 16, 2022

View reviewed changes

Ankit-Keshari-Vituity added 2 commits November 16, 2022 22:45

Worked on the PR commetns

1bdfd78

Worked on the test case

18ee6db

chriscollins3456 approved these changes Nov 17, 2022

View reviewed changes

Ankit-Keshari-Vituity added 2 commits November 17, 2022 20:36

Worked on the PR comments

55755e0

capitalise the test case

8f38f4c

chriscollins3456 merged commit 7ff765e into datahub-project:master Nov 17, 2022

cccs-Dustin pushed a commit to CybercentreCanada/datahub that referenced this pull request Feb 1, 2023

feat(ui) Filter by description text on schema tab search (datahub-pro…

66ec30d

…ject#6400)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Highlighted the description text on search #6400

Highlighted the description text on search #6400

Ankit-Keshari-Vituity commented Nov 10, 2022

github-actions bot commented Nov 10, 2022 •

edited

Loading

jjoyce0510 Nov 14, 2022

jjoyce0510 Nov 14, 2022

chriscollins3456 left a comment

chriscollins3456 Nov 16, 2022

Ankit-Keshari-Vituity Nov 16, 2022

chriscollins3456 left a comment

chriscollins3456 Nov 16, 2022

Ankit-Keshari-Vituity Nov 16, 2022

chriscollins3456 Nov 16, 2022

chriscollins3456 Nov 16, 2022

chriscollins3456 Nov 16, 2022

chriscollins3456 Nov 16, 2022

chriscollins3456 Nov 16, 2022

chriscollins3456 Nov 16, 2022

chriscollins3456 Nov 16, 2022

chriscollins3456 Nov 16, 2022

chriscollins3456 left a comment

chriscollins3456 Nov 17, 2022

Highlighted the description text on search #6400

Highlighted the description text on search #6400

Conversation

Ankit-Keshari-Vituity commented Nov 10, 2022

Checklist

github-actions bot commented Nov 10, 2022 • edited Loading

Unit Test Results (build & test)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriscollins3456 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriscollins3456 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriscollins3456 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 10, 2022 •

edited

Loading