Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix D&D resource display names #5498

Merged
merged 7 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ The types of changes are:

### Fixed
- Fixed deletion of ConnectionConfigs that have related MonitorConfigs [#5478](https://github.com/ethyca/fides/pull/5478)
- Fixed incorrect display names on some D&D resources [#5498](https://github.com/ethyca/fides/pull/5498)

### Changed
- Allow hiding systems via a `hidden` parameter and add two flags on the `/system` api endpoint; `show_hidden` and `dnd_relevant`, to display only systems with integrations [#5484](https://github.com/ethyca/fides/pull/5484)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,22 +1,47 @@
import { DiscoveryMonitorItem } from "~/features/data-discovery-and-detection/types/DiscoveryMonitorItem";

const TOP_LEVEL_FIELD_URN_PARTS = 5;
const MAX_NON_NESTED_URN_LENGTH = 5;
const URN_SEPARATOR = ".";
/**
* Helper method for deriving a resource's display name from its URN
*/
const getResourceName = ({
name,
urn,
monitor_config_id,
database_name,
schema_name,
table_name,
top_level_field_name,
}: DiscoveryMonitorItem) => {
const splitUrn = urn.split(URN_SEPARATOR);

const getResourceName = (resource: DiscoveryMonitorItem) => {
const URN_SEPARATOR = ".";
const splitUrn = resource.urn.split(URN_SEPARATOR);
if (
!resource.parent_table_urn ||
splitUrn.length === TOP_LEVEL_FIELD_URN_PARTS
!table_name ||
!top_level_field_name ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@thingscouldbeworse thingscouldbeworse Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for flagging-- yeah, that'll make this much cleaner if I can just elide that.

splitUrn.length < MAX_NON_NESTED_URN_LENGTH
) {
// use name as-is if it's not a subfield
return resource.name;
return name;
}
// TODO HJ-162: better handle case where field name contains "."
// URN format is "monitor.project?.dataset.table.field.[any number of subfields]"
// we want to show all subfield names separated by "."
const partsToRemove = [
monitor_config_id,
database_name,
schema_name,
table_name,
top_level_field_name,
];

// URN format is "monitor.project.dataset.field.[any number of subfields]"
// 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!);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nitpicky: I would feel more comfortable with this if it were wrapped in an if rather than using a forced Type !

Suggested change
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!

if (index > -1) {
splitUrn.splice(index, 1);
}
});

return splitUrn.join(URN_SEPARATOR);
};

export default getResourceName;
Loading