FIX: separate urn with non-nested-pattern for now #5383

gilluminate · 2024-10-15T16:17:02Z

Closes HJ-36

Description Of Changes

This is a bit of a bandaid fix to handle nested fields on the frontend. We are specifying something other than . used in nested field names for the URN so that we can distinguish those. More robust fixes are happening on both the BE and FE in separate tickets.

Code Changes

Specify a non-period separator for FE logic
Apply some best practices to the Edit drawer and Breadcrumbs to avoid race conditions during E2E testing and for optimization in general.

Steps to Confirm

In Fides Plus Admin UI, visit the Manage datasets page (eg. http://localhost:3000/dataset)
If the dataset example_dataset_issue_hj36 exists

Click it
Click example_table
Ensure that you can still add/remove Categories from the "Can save data category changes" fields.
Click through to the nested field example_nested_field
The field example_failure_nested_field.1 with description "Fail to save data category changes on nested field with '.' in name" should now be able to add/remove categories.
Click "edit" on the same example_failure_nested_field.1 row and make sure you can still "Save"

If the dataset example_dataset_issue_hj36 does not exist

Click "+ Add Dataset" button
Click "Upload a Dataset YML"
Paste the following into the editor:

- fides_key: example_dataset_issue_hj36
  organization_fides_key: default_organization
  tags: null
  name: example_dataset_issue_hj36
  description: Minimal dataset reproducing issue (HJ-36)
  meta: null
  data_categories: null
  fides_meta: null
  collections:
    - name: example_table
      description: null
      data_categories: null
      fields:
        - name: example_nested_field
          description: null
          data_categories: null
          fides_meta: null
          fields:
            - name: example_success_nested_field
              description: Can save data category changes on nested field
              data_categories:
                - user.device
              fides_meta: null
              fields: []
            - name: example_failure_nested_field.1
              description: >-
                Fail to save data category changes on nested field with '.' in
                name
              data_categories:
                - user.location
                - system
              fides_meta: null
              fields: []
        - name: example_success_field
          description: Can save data category changes
          data_categories:
            - user.device
          fides_meta: null
          fields: null
        - name: example_success_field.1
          description: Can save data category changes
          data_categories:
            - user.device
          fides_meta: null
          fields: null
      fides_meta: null

Click "Create dataset" button
Proceed to follow steps 1-6 above for the example_dataset_issue_hj36 dataset

Pre-Merge Checklist

All CI Pipelines Succeeded
Issue Requirements are Met
Relevant Follow-Up Issues Created
Update CHANGELOG.md

vercel · 2024-10-15T16:17:07Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
fides-plus-nightly	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 15, 2024 8:16pm

cypress · 2024-10-15T16:32:32Z

fides Run #10452

Run Properties: Passed #10452 • 68d419a874 ℹ️: Merge 52021b633d69bb02a929c94c4e44b741fb24bac8 into 7038f614920aa503e0494dd64290...

Project	`fides`
Run status	`Passed #10452`
Run duration	`00m 36s`
Commit	`68d419a874 ℹ️: Merge 52021b633d69bb02a929c94c4e44b741fb24bac8 into 7038f614920aa503e0494dd64290...`
Committer	`Jason Gill`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`0`
Skipped	`0`
Passing	`4`
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

lucanovera

I reproduced the issue and confirmed that it's fix in this branch. The code looks good. left a minor question below. approved!

lucanovera · 2024-10-15T19:19:12Z

clients/admin-ui/src/features/dataset/EditCollectionDrawer.tsx

-  const collectionIndex = dataset?.collections.indexOf(collection!);
+  const collectionIndex = useMemo(
+    () => dataset?.collections.indexOf(collection),
+    // eslint-disable-next-line react-hooks/exhaustive-deps


i'm curious, did it cause an issue trying to add the deps?

This was causing a test to fail and I believe it's because the supplied collection was no longer matching anything in the dataset after a save. Since we're now requiring both dataset and collection to be available from the start, we want to snag this index and then stop worrying about it once either gets updated. Including the dependencies recreates the problem.

I think if I were to rewrite this, I'd pass in the index rather than the collection but this seemed like the less risky fix.

lucanovera · 2024-10-15T19:23:29Z

clients/admin-ui/src/features/dataset/constants.ts

@@ -29,3 +29,5 @@ export const FIELD = {
      "Arrays of Data Category resources, identified by fides_key, that apply to this field.",
  },
 };
+
+export const URN_SEPARATOR = "/";


Nice quick solution changing the separator to something different.
Although this makes me wonder, could we expect to have "/" as a part of the name too? 😂

Anyways, we shouldn't have accepted dots as part of the name either, and kirk is working on a fix for that.

Yes, it's party why I made this a constant we can update. We can come back and tweak if it's still a problem for other data.

lucanovera · 2024-10-15T19:24:09Z

clients/admin-ui/src/pages/dataset/[datasetId]/[collectionName]/[subfieldUrn].tsx

@@ -260,46 +261,48 @@ const FieldsDetailPage: NextPage = () => {
    DatasetField | undefined
  >();

+  const breadcrumbs = useMemo(() => {


Good optimization!

lucanovera · 2024-10-15T19:29:54Z

clients/admin-ui/src/pages/dataset/[datasetId]/index.tsx

-        isOpen={isEditingCollection}
-        onClose={() => setIsEditingCollection(false)}
-      />
+      {dataset && selectedCollectionForEditing && (


Nice, avoiding needing to use the ! or handling undefined values further down.

IMO we should avoid ! always. It's usually an indication for a potential bug, as was the case here.

lucanovera · 2024-10-15T19:40:51Z

@gilluminate While testing I found another bug that would be very easy to fix, can we include it here?

The bug is that in the table view if you try to add a category by adding the plus icon, it will navigate down to the nested field level.

In src/pages/dataset/[datasetId]/[collectionName]/index.tsx the data categories column is missing this option:
meta: { disableRowClick: true, },

It won't cause an issue here src/pages/dataset/[datasetId]/[collectionName]/[subfieldUrn].tsx but we could add it for consistency too.

…vigate into it

cypress · 2024-10-15T21:13:28Z

fides Run #10454

Run Properties: Passed #10454 • e55d71f66e: FIX: separate urn with non-nested-pattern for now (#5383)

Project	`fides`
Run status	`Passed #10454`
Run duration	`00m 38s`
Commit	`e55d71f66e: FIX: separate urn with non-nested-pattern for now (#5383)`
Committer	`Jason Gill`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`0`
Skipped	`0`
Passing	`4`
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Co-authored-by: Lucano Vera <lucanovera@ethyca.com>

vercel bot deployed to Preview October 15, 2024 16:20 View deployment

gilluminate force-pushed the HJ-36-dataset-with-nested-fields branch from d23329b to a199567 Compare October 15, 2024 17:32

vercel bot deployed to Preview October 15, 2024 17:40 View deployment

vercel bot deployed to Preview October 15, 2024 18:10 View deployment

gilluminate force-pushed the HJ-36-dataset-with-nested-fields branch from 889c5e7 to daa7330 Compare October 15, 2024 18:12

gilluminate requested a review from lucanovera October 15, 2024 18:13

gilluminate marked this pull request as ready for review October 15, 2024 18:14

gilluminate force-pushed the HJ-36-dataset-with-nested-fields branch from b437e1b to 4b37cfc Compare October 15, 2024 18:41

lucanovera approved these changes Oct 15, 2024

View reviewed changes

gilluminate added 4 commits October 15, 2024 14:10

optimize/fix breadcrumb issues

568da0e

fix issues with edit drawer

b23238e

FIX: separate urn with non-nested-pattern for now

7979a20

Update CHANGELOG.md

0cfcbcb

gilluminate force-pushed the HJ-36-dataset-with-nested-fields branch from 4b37cfc to 0cfcbcb Compare October 15, 2024 20:10

Fix bug where clicking on data category on nested field would also na…

52021b6

…vigate into it

vercel bot deployed to Preview October 15, 2024 20:16 View deployment

gilluminate merged commit e55d71f into main Oct 15, 2024
13 checks passed

gilluminate deleted the HJ-36-dataset-with-nested-fields branch October 15, 2024 20:58

Kelsey-Ethyca mentioned this pull request Oct 16, 2024

update changelog for 2.47.1 #5391

Merged

14 tasks

Kelsey-Ethyca pushed a commit that referenced this pull request Oct 16, 2024

FIX: separate urn with non-nested-pattern for now (#5383)

3c760e5

Co-authored-by: Lucano Vera <lucanovera@ethyca.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: separate urn with non-nested-pattern for now #5383

FIX: separate urn with non-nested-pattern for now #5383

gilluminate commented Oct 15, 2024 •

edited

Loading

vercel bot commented Oct 15, 2024 •

edited

Loading

cypress bot commented Oct 15, 2024 •

edited

Loading

lucanovera left a comment

lucanovera Oct 15, 2024

gilluminate Oct 15, 2024 •

edited

Loading

gilluminate Oct 15, 2024

lucanovera Oct 15, 2024

gilluminate Oct 15, 2024

lucanovera Oct 15, 2024

lucanovera Oct 15, 2024

gilluminate Oct 15, 2024

lucanovera commented Oct 15, 2024

cypress bot commented Oct 15, 2024

FIX: separate urn with non-nested-pattern for now #5383

FIX: separate urn with non-nested-pattern for now #5383

Conversation

gilluminate commented Oct 15, 2024 • edited Loading

Description Of Changes

Code Changes

Steps to Confirm

Pre-Merge Checklist

vercel bot commented Oct 15, 2024 • edited Loading

cypress bot commented Oct 15, 2024 • edited Loading

lucanovera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilluminate Oct 15, 2024 • edited Loading

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

lucanovera commented Oct 15, 2024

cypress bot commented Oct 15, 2024

gilluminate commented Oct 15, 2024 •

edited

Loading

vercel bot commented Oct 15, 2024 •

edited

Loading

cypress bot commented Oct 15, 2024 •

edited

Loading

gilluminate Oct 15, 2024 •

edited

Loading