Show profile and catalog inheritance #186

mikeisen1 · 2021-11-15T17:04:12Z

Create a react component, OSCALProfileCatalogInheritance, which displays the tree of inherited profiles and catalogs for any Component Definition, Profile, or SSP. This allows us to view that inheritance tree, providing users greater insight into which profiles and catalogs were used in the creation of a given OSCAL document.

Additionally created a storybook page for the new component so any developer can view how it looks and functions and potentially reuse it in their code.

closes #161

Display the tree of inherited profiles and catalogs for any profile json document in the Profile viewer.

Display the tree of inherited profiles and catalogs for any component definition json document in the Component Viewer. Add rule to linting json file that forces the linter to allow the reassignment of function parameters.

Display the tree on inherited profiles and catalogs for any SSP json document in the SSP Viewer. Remove the original document title from being displayed in the inherited profiles and catalogs.

Identify whether an inherited profile or catalog is a profile or catalog, making it easy to identify the document type.

Change the collapse icon to ExpandLess to use an icon more representative of the action taken. Use IconButton in order to give labels to the icons, which is important for testing purposes.

Create tests for the newly added react component to ensure it functions as expected.

Remove subtracting one from the id array so icon ids are the same as the node id.

kylelaker · 2021-11-15T20:02:03Z

.eslintrc.json

@@ -25,7 +25,8 @@
      "react/prop-types": [0],
      "react/destructuring-assignment": [0],
      "react/jsx-props-no-spreading": "off",
-      "prettier/prettier": "error"
+      "prettier/prettier": "error",
+      "no-param-reassign": 0


I don't love the addition of this. Why don't we just use a different name for the variable inside the function rather than reusing a parameter name? Shadowing variables is always a little iffy.

Remove the no-param-reassign linting rule as we do not want to shadow variables.

mikeisen1 · 2021-11-15T20:48:01Z

Here are some of the other options for the expand/collapse icons. Just wanted to put them here in case anyone thought one of these could be better than the ones currently used.

Expand icon: Add, ControlPoint, MoreHoriz, MoreVert, UnfoldMore, ExpandMore

Collapse icon: Remove, Unfold Less, ExpandLess, Close, Cancel, HighlightOff, Remove

You can easily view these visually here by searching for the above icons in the search box.

rgauss · 2021-11-15T20:45:03Z

src/components/OSCALProfileCatalogInheritance.js

+    return null;
+  }
+
+  const generateLabel = (title, type) =>


Can this be pulled out of renderTree?

rgauss · 2021-11-15T20:48:42Z

src/components/OSCALProfileCatalogInheritance.js

+      <TreeItem
+        nodeId={idReassign[0].toString()}
+        label={generateLabel(node.title, node.type)}
+        expandIcon={


Can we just the the defaults on TreeView?

I chose not to because if we use the TreeView default icon properties, then all icons will have the same value for the aria-label property. That won't go well for testing purposes so if we want to be able to test the expansion and collapse of the tree, then we can't use the TreeView defaults. If we don't care about that, then the defaults are fine.

Ideally we don't want to be required to rely on aria labels, instead taking an approach of finding the expected text then the closest button.

As mentioned elsewhere on this PR, we also want the tree to be expanded by default, so I'm not sure will need to test clicking on these at all.

rgauss · 2021-11-15T20:50:02Z

src/components/OSCALProfileCatalogInheritance.test.js

+
+test("OSCALProfileCatalogInheritance displays nested inherited objects", async () => {
+  profileCatalogInheritanceRenderer();
+  const inheritanceButton = await screen.findByRole(


We want all nodes in the tree expanded by default so this may not be needed?

It seems like it would make sense to at least test the expansion and collapse of the tree to ensure everything works and is displayed as expected in both cases.

But this test can be rewritten once everything is expanded by default.

OK, that's fine.

rgauss · 2021-11-15T21:23:22Z

src/components/oscal-utils/OSCALProfileResolver.js

@@ -27,17 +27,19 @@ import getUriFromBackMatterByHref from "./OSCALBackMatterUtils";
 * @see {@link https://pages.nist.gov/OSCAL/documentation/schema/profile-layer/profile/xml-schema/#global_import}
 * @see {@link https://pages.nist.gov/OSCAL/documentation/schema/profile-layer/profile/xml-schema/#global_back-matter_h2}
 */
-export default function OSCALResolveProfileOrCatalogUrlControls(
+export default async function OSCALResolveProfileOrCatalogUrlControls(


Is there a reason we're introducing asyncs and awaits for this?

There was some issue when I was first doing the task that async/await seemed to resolve. It isn't needed I just forgot to remove them from the code.

kylelaker · 2021-11-15T21:58:41Z

src/components/OSCALProfileCatalogInheritance.js

-    id[0] += 1;
+    idReassign[0] += 1;


We're not really short on characters here. How about inheritedNodeId or just nodeId to give it a more descriptive name?

Another thought here would be to add the UUID of the OSCAL document to the inheritance object, but we also talk about the possibility of using default icons at the TreeView level, so this ID may not be needed at all.

Add second React component, one for the TreeView that displays the inherited profiles and catalogs, so the defaultExpanded prop of TreeView works as expected, allowing us to see the expanded tree by default and collapse the expanded tree as desired. TreeView has an expanded prop that doesn't require a second React component but that doesn't allow for the collapse of TreeView nodes. Clean up component, testing, and other files to remove code that was not necessary and rename functions and variables to improve readability.

Remove async/await, as they are not needed and do not impact the result.

Test that nested profiles/catalogs will be displayed.

hreineck

Looks good. Both the Viewer and Storybook are working locally for me.
Just a few small changes/questions.

hreineck · 2021-11-18T22:09:24Z

src/components/OSCALComponentDefinition.js

+    setInheritedProfilesAndCatalogs(
+      OSCALComponentResolveSources(
+        props.componentDefinition,
+        props.parentUrl,
+        () => {
+          setIsLoaded(true);
+          props.onResolutionComplete();
+        },
+        (errorReturned) => {
+          setError(errorReturned);
+          setIsLoaded(true);
+          props.onResolutionComplete();
+        }
+      )


This is getting a bit convoluted. Instead of having a method that takes in the return of the existing OSCALComponentResolveSources method, could we have an overall "resolve" wrapper method like the other OSCAL types do?

hreineck · 2021-11-18T22:19:41Z

src/components/OSCALProfileCatalogInheritanceTree.js

+import ListSubheader from "@material-ui/core/ListSubheader";
+
+export default function OSCALProfileCatalogInheritanceTree(props) {
+  const tree =


Is the const necessary? Why not just return this object?

hreineck · 2021-11-18T22:29:10Z

src/components/OSCALProfileCatalogInheritanceTree.js

+                  <ExpandLessIcon />
+                </IconButton>
+              }
+              defaultExpanded={props.validIds}


Just putting this out here - is the ability to actually collapse the tree necessary? If it makes things simpler to have it permanently expanded that could work as well.

I'm not sure that it's absolutely necessary but if we can allow users to collapse the tree we should provide it as it's something, albeit small, that could improve user experience.

Remove intialization and use of a variable that was not needed. Remove return from a function and instead pass in a function that sets a state variable so a component will rerender.

hreineck

Great work!

rgauss · 2021-11-22T16:12:09Z

src/components/OSCALProfileCatalogInheritance.js

+
+export default function OSCALProfileCatalogInheritance(props) {
+  const classes = useStyles();
+  const [validIds, setValidIds] = useState([false, []]);


Why do we need all of these instances of IDs separately rather than just using the UUID already in the inherited objects?

Do we want to ensure each nodeId in the tree is unique? We could have the same profile or catalog in different parts of the tree which could cause repeated nodeIds.

If we're setting node IDs and keys we do want them to be unique.

Are we sure we need useState here?

I'd also recommend changing these to something like expandedIds everywhere.

We need useState so when we set the expanded ids, we re-render the component. If we do not re-render, the tree will not be expanded by default.

rgauss · 2021-11-22T22:05:23Z

src/components/OSCALProfileCatalogInheritance.js

+
+export default function OSCALProfileCatalogInheritance(props) {
+  const classes = useStyles();
+  const [validIds, setValidIds] = useState([false, []]);


If we're setting node IDs and keys we do want them to be unique.

Are we sure we need useState here?

I'd also recommend changing these to something like expandedIds everywhere.

src/components/OSCALProfileCatalogInheritance.js

Remove the second react component that was created, as it was not needed and we want to avoid creating additional, unnecessary components.

Rename variable to make code more readable.

src/components/OSCALComponentDefinition.js

Relocate call of setInheritedProfilesAndCatalogs to within onSuccess and remove that parameter from util resolver functions.

src/components/oscal-utils/OSCALProfileResolver.js

rgauss

Two minor rendering changes:

I think the tree needs more of a left margin. The expand arrow currently bumps up against the card edge.
The header is sticky rather than scrolling with the page

Create a left margin for the tree so the expand and collapse icons do not brush against the edge. Make the header scroll with the page instead of being sticky.

rgauss

Looks great, thanks for working through all the changes.

mikeisen1 added 12 commits November 10, 2021 15:48

Show profiles inherited profiles and catalogs

026c533

Display the tree of inherited profiles and catalogs for any profile json document in the Profile viewer.

Show components inherited profiles and catalogs

f992de6

Display the tree of inherited profiles and catalogs for any component definition json document in the Component Viewer. Add rule to linting json file that forces the linter to allow the reassignment of function parameters.

Remove unused imports

de71311

Shows ssps inherited profiles and catalogs

8801277

Display the tree on inherited profiles and catalogs for any SSP json document in the SSP Viewer. Remove the original document title from being displayed in the inherited profiles and catalogs.

Differentiate between profiles and catalogs

19e246e

Identify whether an inherited profile or catalog is a profile or catalog, making it easy to identify the document type.

Update icons and icon labels

bbb16ba

Change the collapse icon to ExpandLess to use an icon more representative of the action taken. Use IconButton in order to give labels to the icons, which is important for testing purposes.

Test the new react component

b2202ba

Create tests for the newly added react component to ensure it functions as expected.

Remove unnecessary variables

bbb4c19

Add component storybook page

b6687e8

Resolve linting issues so linter won't fail

8aadadd

Resolve linter issues

e597dfd

Replace ChevronRightIcon with ExpandMoreIcon

6e7f1a0

mikeisen1 added the enhancement New feature or request label Nov 15, 2021

mikeisen1 requested review from rgauss and hreineck November 15, 2021 17:04

mikeisen1 self-assigned this Nov 15, 2021

mikeisen1 added 11 commits November 15, 2021 12:15

Resolve linter errors

7e01d60

Adjust timeout so test will pass

7b21a2f

Adjust timeout so test will pass

edddffd

Adjust timeout at beginning of test so it will pass

795d6bc

Adjust timeout so test will pass

ac07603

Adjust timeout so test will pass

472c78e

Set timers for whole test page

7318acf

Make nodeId a valid string and not a number

a3d297c

Update how icon IDs are determined

f6c7d8c

Remove subtracting one from the id array so icon ids are the same as the node id.

Resolve linter errors

913a07d

Resolve linter errors

ebb0fe1

kylelaker reviewed Nov 15, 2021

View reviewed changes

Remove added linting rule

18c3dab

Remove the no-param-reassign linting rule as we do not want to shadow variables.

mikeisen1 requested a review from kylelaker November 15, 2021 20:37

rgauss suggested changes Nov 15, 2021

View reviewed changes

kylelaker reviewed Nov 15, 2021

View reviewed changes

mikeisen1 added 3 commits November 18, 2021 14:59

Remove unecessary keywords

5252ec7

Remove async/await, as they are not needed and do not impact the result.

Add additional inheritance test

bdf23e1

Test that nested profiles/catalogs will be displayed.

hreineck requested changes Nov 18, 2021

View reviewed changes

mikeisen1 added 2 commits November 19, 2021 09:59

Clean up code

dbb9f05

Remove intialization and use of a variable that was not needed. Remove return from a function and instead pass in a function that sets a state variable so a component will rerender.

Resolve linter errors

2833a47

mikeisen1 requested review from rgauss, kylelaker and hreineck November 19, 2021 19:45

hreineck approved these changes Nov 19, 2021

View reviewed changes

rgauss suggested changes Nov 22, 2021

View reviewed changes

mikeisen1 added 3 commits November 29, 2021 10:15

Remove second React component

1208332

Remove the second react component that was created, as it was not needed and we want to avoid creating additional, unnecessary components.

Clean up state variable

b9f4035

Resolve Linter Errors

9d3e316

mikeisen1 requested a review from rgauss November 29, 2021 15:48

Make code more readable

e8d4c72

Rename variable to make code more readable.

rgauss suggested changes Nov 30, 2021

View reviewed changes

src/components/OSCALComponentDefinition.js Outdated Show resolved Hide resolved

Relocate function call and remove parameter

2aafc49

Relocate call of setInheritedProfilesAndCatalogs to within onSuccess and remove that parameter from util resolver functions.

rgauss reviewed Nov 30, 2021

View reviewed changes

src/components/oscal-utils/OSCALProfileResolver.js Show resolved Hide resolved

rgauss suggested changes Nov 30, 2021

View reviewed changes

mikeisen1 added 2 commits December 1, 2021 09:45

Improve inheritance tree aesthetics

d935edd

Create a left margin for the tree so the expand and collapse icons do not brush against the edge. Make the header scroll with the page instead of being sticky.

Resolve linter errors.

7c64fd3

rgauss approved these changes Dec 1, 2021

View reviewed changes

rgauss merged commit bc46b8f into develop Dec 1, 2021

rgauss deleted the show-profile-catalog-inheritance branch December 1, 2021 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show profile and catalog inheritance #186

Show profile and catalog inheritance #186

mikeisen1 commented Nov 15, 2021

kylelaker Nov 15, 2021

mikeisen1 commented Nov 15, 2021 •

edited

rgauss Nov 15, 2021

rgauss Nov 15, 2021

mikeisen1 Nov 15, 2021 •

edited

rgauss Nov 16, 2021

rgauss Nov 15, 2021

mikeisen1 Nov 15, 2021

mikeisen1 Nov 15, 2021

rgauss Nov 16, 2021

rgauss Nov 15, 2021

mikeisen1 Nov 15, 2021

kylelaker Nov 15, 2021

rgauss Nov 16, 2021

hreineck left a comment

hreineck Nov 18, 2021

hreineck Nov 18, 2021

hreineck Nov 18, 2021

mikeisen1 Nov 19, 2021

hreineck left a comment

rgauss Nov 22, 2021

mikeisen1 Nov 22, 2021 •

edited

rgauss Nov 22, 2021

mikeisen1 Nov 23, 2021

rgauss Nov 22, 2021

rgauss left a comment

rgauss left a comment

Show profile and catalog inheritance #186

Show profile and catalog inheritance #186

Conversation

mikeisen1 commented Nov 15, 2021

Choose a reason for hiding this comment

mikeisen1 commented Nov 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeisen1 Nov 15, 2021 • edited

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

hreineck 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

hreineck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeisen1 Nov 22, 2021 • edited

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

rgauss left a comment

Choose a reason for hiding this comment

rgauss left a comment

Choose a reason for hiding this comment

mikeisen1 commented Nov 15, 2021 •

edited

mikeisen1 Nov 15, 2021 •

edited

mikeisen1 Nov 22, 2021 •

edited