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 Update SSP and Component Definition to Display Profile Modifications #146

Merged
merged 10 commits into from
Aug 10, 2021

Conversation

tuckerzp
Copy link
Contributor

This PR is a fix to #124 and should hopefully closes #75

This reworks ProfileResolver to save modifications per imported profiles. SSP and Component Definition should now properly display modifications only when given an imported profile.

Tests have also been changed. Changes in how we get modifications for each imported profile makes these tests no longer needed. Both the SSP and Component Definition rely on OSCALControlImplementationImplReq to display modifications. My reason for thinking these tests are unneeded is really due to how we pass modifications down to Control making it redundant to check on multiple levels that modifications are passed down. I would like if we can confirm that I am right in doing this as I still consider myself fairly new to the tests.

When resolving a profile, modifications will now be set. This allows for
SSP and Component Definition to display control modifications from
imported profiles.
Changes in how we get modifications for each imported profile makes
these tests no longer needed. Both the component definition and ssp rely
on OSCALControlImplementationImplReq to display modifications.
@tuckerzp
Copy link
Contributor Author

I am assigning @hreineck to hopefully address comments in my absence.

Copy link
Contributor

@rgauss rgauss left a comment

Choose a reason for hiding this comment

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

The component definition viewer does not seem to show the parameter constraints for the FedRAMP High profile for the default component definition URL.

@hreineck
Copy link
Contributor

hreineck commented Aug 3, 2021

The component definition viewer does not seem to be the parameter constraints for the FedRAMP High profile for the default component definition URL.

@rgauss could you clarify the intended behavior? seems this does not need to display any parameter constraints. It's currently calling OSCALReplacedProseWithByComponentParameterValue, but with no statement arguments; the default Component url doesn't have any statements props to pass in.

@rgauss
Copy link
Contributor

rgauss commented Aug 3, 2021

The component definition viewer does not seem to be the parameter constraints for the FedRAMP High profile for the default component definition URL.

@rgauss could you clarify the intended behavior? seems this does not need to display any parameter constraints. It's currently calling OSCALReplacedProseWithByComponentParameterValue, but with no statement arguments; the default Component url doesn't have any statements props to pass in.

In #75 acceptance criteria we see:

... example component definition ... should display that profile's AC-2.3 constraints

I'm not sure what you mean by "the default Component url doesn't have any statements props to pass in".

Both the SSP and the Component Definition viewers should display those parameter constraints so that a user can immediately see whether the parameter value they've entered is within those constraints rather than having to navigate to the Profile.

Fixed a typo, and modified the conditional around the call to
the Control Prose functions in `OSCALControlProse` in order to
have the parameter constraints display on mouse hover when applicable
in the Component Viewer.
Change the way the `isLoaded` state variable is set to
fix an issue where modifications would not render in the
Component Viewer.
`isLoaded` was being set to true upon the modifications for the
first Control Implementation being resolved, which was causing
the state to be "loaded" when not all the modifications were loaded.
Added a `pendingProcesses` counter to track when all of the
implementations have had their modifications resolved before
setting the isLoaded state.
Made changes to address test and linter failures:

Added a conditional around `processesToRun` to deal with cases
where `componenents` is undefined or not an array.

Addressed linter issues; got rid of `--` operators and `==` operator.
src/components/oscal-utils/OSCALComponentResolver.js Outdated Show resolved Hide resolved
src/components/oscal-utils/OSCALComponentResolver.js Outdated Show resolved Hide resolved
src/components/oscal-utils/OSCALComponentResolver.js Outdated Show resolved Hide resolved
src/components/oscal-utils/OSCALComponentResolver.js Outdated Show resolved Hide resolved
Changed the check for pending processes to a cleaner map/reduce
function.

Removed the unnecessary check for when `components` is not an
array; to make this work, the test data was changed to the correct
format in which `components` is an array instead of an object.
Minor change to a conditional in control part;
combines these redundnant expressions.
@hreineck hreineck requested review from kylelaker and rgauss and removed request for Bronstrom and hreineck August 5, 2021 15:37
Minor change to the conditional on the `pendingProcesses` variable
to make things a bit cleaner.
@hreineck hreineck requested a review from kylelaker August 5, 2021 18:20
Copy link
Contributor

@mikeisen1 mikeisen1 left a comment

Choose a reason for hiding this comment

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

Everything works great and all requested changes have been resolved.

@rgauss
Copy link
Contributor

rgauss commented Aug 9, 2021

@hreineck, looks like this still has conflicts to be resolved. Other than that looks good.

Copy link
Contributor

@kkennedy26 kkennedy26 left a comment

Choose a reason for hiding this comment

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

good work on the pendingProcesses workaround

@rgauss
Copy link
Contributor

rgauss commented Aug 10, 2021

@kylelaker, this look OK now?

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.

Phenomenal work here! Thanks for the iterations and good discussion. I really appreciate you being able to take over on this PR and get it delivered.

@kylelaker kylelaker merged commit eb3bf66 into develop Aug 10, 2021
@kylelaker kylelaker deleted the EGRC-295 branch August 10, 2021 18:46
@tuckerzp
Copy link
Contributor Author

@hreineck Thanks again for taking this for me! Great job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update SSP and Component Definition to Display Profile Modifications
6 participants