Fix D&D resource display names #5498
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
fides
|
||||||||||||||||||||||||||||||
| Project |
fides
|
| Branch Review |
refs/pull/5498/merge
|
| Run status |
|
| Run duration | 00m 38s |
| Commit |
|
| Committer | jpople |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
4
|
Upgrade your plan to view test results. | |
| View all changes introduced in this branch ↗︎ | |
| !resource.parent_table_urn || | ||
| splitUrn.length === TOP_LEVEL_FIELD_URN_PARTS | ||
| !table_name || | ||
| !top_level_field_name || |
There was a problem hiding this comment.
If it would be helpful I am adding in top_level_field_urn as well in this PR: https://github.com/ethyca/fidesplus/pull/1727/files#diff-8bd50e59212bea55bbd599dd3564999eecbda0c718c2681318bbc990a29d28c9R354
not sure if it's actually better or the same as top_level_field_name but I figured better to give as much info as possible now and then we can remove one later if it's not helpful
There was a problem hiding this comment.
I imagine top_level_field_name to be maybe a little tricky in structures like
same_name
|------- same_name
|------- same_name
|------- same_name
etc etc
There was a problem hiding this comment.
Thanks for flagging-- yeah, that'll make this much cleaner if I can just elide that.
| // for a subfield, we want to show all subfield names separated by "." | ||
| return splitUrn.slice(TOP_LEVEL_FIELD_URN_PARTS).join(URN_SEPARATOR); | ||
| partsToRemove.forEach((part) => { | ||
| const index = splitUrn.indexOf(part!); |
There was a problem hiding this comment.
super nitpicky: I would feel more comfortable with this if it were wrapped in an if rather than using a forced Type !
| const index = splitUrn.indexOf(part!); | |
| if (!!part) { | |
| const index = splitUrn.indexOf(part); |
Take this example:
"my string has a null word in it".indexOf(null);
returns 16 because Javascript!
fides
|
||||||||||||||||||||||||||||||
| Project |
fides
|
| Branch Review |
main
|
| Run status |
|
| Run duration | 00m 38s |
| Commit |
|
| Committer | jpople |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
Failures
|
0
|
|
Flaky
|
0
|
|
Pending
|
0
|
|
Skipped
|
0
|
|
Passing
|
4
|
Upgrade your plan to view test results. | |
| View all changes introduced in this branch ↗︎ | |
Closes #HJ-162
Description Of Changes
Fixes D&D resource display names to be more generalizable across systems and handle some edge cases better by no longer relying (solely) on URN length.
Steps to Confirm
To test, have some data in your detection or discovery tables with:
.Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works