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(catalog): catalog viewer only shows controls if they are under a group #792

Merged
merged 13 commits into from
Apr 28, 2023

Conversation

tuckerzp
Copy link
Contributor

@tuckerzp tuckerzp commented Apr 21, 2023

Previously OSCALCatalog ignored the Controls prop it was passed.
This creates an OSCALControls component to display controls outside
a group.

closes #741
closes #742

Testing

No controls

https://raw.githubusercontent.com/EasyDynamics/oscal-demo-content/main/catalogs/NIST_SP-800-53_rev4_catalog.json

Controls + Groups

https://raw.githubusercontent.com/EasyDynamics/oscal-demo-content/tuckerzp/group-prose/catalogs/basic-test-catalog.json

Just Controls

https://raw.githubusercontent.com/EasyDynamics/oscal-demo-content/tuckerzp/group-prose/catalogs/basic-test-catalog-no-groups.json


Groups without controls is displayed the same

image


Top Controls is now shown as a group

image


Top level control groups with prose is displayed

image


Children groups with prose is displayed

image


No groups listed

image

@tuckerzp tuckerzp force-pushed the tuckerzp/show-controls-without-group branch 2 times, most recently from 2c897ec to c29c738 Compare April 25, 2023 17:17
@tuckerzp tuckerzp marked this pull request as ready for review April 25, 2023 18:13
@easy-dynamics-oscal-automation easy-dynamics-oscal-automation bot requested a review from a team April 25, 2023 18:14
@tuckerzp
Copy link
Contributor Author

Note: I still need to add interface documentation

@easy-dynamics-oscal-automation easy-dynamics-oscal-automation bot requested a review from a team April 25, 2023 19:34
@tuckerzp tuckerzp changed the title feat(catalog): Display Catalog Controls fix(catalog): Catalog viewer only shows controls if they are under a group Apr 25, 2023
@tuckerzp tuckerzp changed the title fix(catalog): Catalog viewer only shows controls if they are under a group fix(catalog): catalog viewer only shows controls if they are under a group Apr 25, 2023
Copy link
Contributor

@Bronstrom Bronstrom left a comment

Choose a reason for hiding this comment

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

Looking good so far and thanks for adjusting this to typescript.

@easy-dynamics-oscal-automation easy-dynamics-oscal-automation bot requested a review from a team April 26, 2023 14:11
@easy-dynamics-oscal-automation easy-dynamics-oscal-automation bot requested a review from a team April 26, 2023 15:40
@kylelaker
Copy link
Contributor

kylelaker commented Apr 26, 2023

Children groups with prose is displayed

image

Prose should be displayed above the other children (like we do with controls)

@kylelaker
Copy link
Contributor

Top level control groups with prose is displayed

image

Can you show this but with controls in the group?

@easy-dynamics-oscal-automation easy-dynamics-oscal-automation bot requested a review from a team April 26, 2023 16:31
Copy link
Contributor

@kylelaker kylelaker left a comment

Choose a reason for hiding this comment

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

If the catalog specifies only controls and not groups, how is it displayed?

Comment on lines +99 to +102
const subControl: Control | undefined = upperLayer?.controls?.find(
(control) => control.id === controlLayers[i]
);
const subGroup: ControlGroup | undefined = upperLayer?.groups?.find(
Copy link
Contributor

Choose a reason for hiding this comment

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

I am confused about why this needs a specific type -- what part of this causes tsc to lose the ability to infer types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'subControl' implicitly has type 'any' because it does not have a type annotation and is referenced directly or indirectly in its own initializer. [7022]

'subGroup' implicitly has type 'any' because it does not have a type annotation and is referenced directly or indirectly in its own initializer. [7022]

@tuckerzp tuckerzp force-pushed the tuckerzp/show-controls-without-group branch from 73b30fc to 552a819 Compare April 27, 2023 16:47
Copy link
Contributor

@kylelaker kylelaker left a comment

Choose a reason for hiding this comment

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

Why does this use a different way to display the list of controls than a Profile does? It seems like when we're listing just controls, those two should use the same code to display the controls?

@tuckerzp
Copy link
Contributor Author

tuckerzp commented Apr 27, 2023

Why does this use a different way to display the list of controls than a Profile does?

Profile has always displayed lists of controls differently than the catalog does. That may be something to look into at some point. I figured we would not want to remove the accordion like feature the controls in the catalog currently have. I can just have them displayed as we do in Profile.

@kylelaker
Copy link
Contributor

Profile has always displayed lists of controls differently than the catalog does. That may be something to look into at some point

I am not sure why we'd go from having three ways of displaying controls (the way CDef/SSP does it, the way profile does it, and the way catalog does it) to four ways (CDef/SSP, Profile, Catalog w/ Groups, Catalog w/out Groups).

If it's something to look into later, we should open the ticket for that fix now.

@tuckerzp
Copy link
Contributor Author

If it's something to look into later, we should open the ticket for that fix now.

#816 was created to track that. We may want to refactor it a bit / create a few tasks from it.

Copy link
Contributor

@kylelaker kylelaker left a comment

Choose a reason for hiding this comment

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

Thanks for the work on this @tuckerzp and for opening up #816.

@Bronstrom Bronstrom merged commit 43561c3 into develop Apr 28, 2023
4 checks passed
@Bronstrom Bronstrom deleted the tuckerzp/show-controls-without-group branch April 28, 2023 13:25
@kylelaker kylelaker added the bug Something isn't working label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prose is not displayed for groups in a catalog Catalog viewer only shows controls if they are under a group
3 participants