feat: display remove control elements #733

mpemy · 2023-03-20T20:23:00Z

This pull request is for the Display remove control elements in Profile.
For testing this implementation, you can use this FedRAMP profile.
https://gist.githubusercontent.com/mpemy/9ea8e9b285cc9198ded3fa46d4154850/raw/9969488e66064281e8394305d352ae73cb60d9ec/Correct_rev4_LI-SaaS-baseline_profile.json

Closes #89
Closes #91

The MarkupMultine method is used instead of the Markuline in the getTextSegment method because this is where we handle prose part. However, this method add an empty line at the begining which does not align well with the placeholder label. This empty line should be remove.

Your branch is up to date with 'fork/mpemy/display-remove-control-elements'. Changes to be committed: modified: src/components/OSCALControlProse.js Fixing the failing build due to linting issue. The line of code that was causing this issue have been addressed. There was method MarkupLines that was added and then removed because it was not needed I just forgot to delete it that is why lint was failing because I was importing a component that was not used and was inexistent. I have simply deleted that reference.

…emy/oscal-react-library into mpemy/display-remove-control-elements merging these two branches so that I can complete this commit and fix the failing build

… symbol is contains in json prose.

…e fix

First gather all removed items and display them below the added items. Also implemented the strike-line to highlight the fact that these items are removed

…t review

… also committing changes related to feat #91 support include-all and exclude-control

…ast commit. Have modified the code to make sure the props.modificationDisplay is not undefined before trying to access its children

tuckerzp · 2023-03-23T13:16:24Z

@mpemy can you provide a screenshot of this?

mpemy · 2023-03-23T13:19:56Z

Ok, this is a screenshot

mpemy · 2023-03-23T13:21:44Z

Another one

tuckerzp

Looking good so far! I had a few clarifying questions.

src/components/OSCALControlModification.js

tuckerzp · 2023-03-23T14:10:08Z

src/components/OSCALControlModification.js

@@ -42,13 +42,30 @@ function getAlterAddsOrRemovesDisplay(addsElements, addsLabel, controlPartId) {
        Name: {item.name}, Value: {item.value}
      </Typography>
    ));
+  let removedTypographies = null;
+  if (addsLabel === "Removes ") {


If addsLablel === "Removes " is it really an adds label? Same for addsElements.

The function documentation also describes the addsLabel as a boolean variable which makes this more confusing.

This label should be string, I changed it to the more descriptive addsRemovesLabel.

Yeah this string comes from

oscal-react-library/src/components/OSCALControlModification.js

Lines 145 to 163 in 57b305b

if (alter.adds) {

[addsDisplay, len] = getModifications(

controlPartId,

props.controlId,

alter.adds,

"Adds "

);

modLength += len;

}

// Get all remove modifications

if (alter.removes) {

[removesDisplay, len] = getModifications(

controlPartId,

props.controlId,

alter.removes,

"Removes "

);

modLength += len;

}

and it's an odd architectural decision...

src/components/OSCALControlProse.js

tuckerzp

I think we need to have some discussion on the overall design here. I'd need to look into the issue and actual json structure a bit more before I could be too prescriptive on best solution, but I am thinking we may need to change how we are displaying removed elements.

I am requesting changes until we can talk more on this.

src/components/OSCALControlModification.js

tuckerzp · 2023-03-23T16:42:17Z

src/components/OSCALControlModification.js

 * @param {String} controlPartId Control part ID to match
 * @returns html object
 */
-function getAlterAddsOrRemovesDisplay(addsElements, addsLabel, controlPartId) {
+function getAlterAddsOrRemovesDisplay(
+  addsElements,


Is this only the adds elements?

I think you could change a lot of the addsorremoves to be alters. So altersElements, altersLabel, etc.

@mpemy I wanted to bump this. I still think that addsElements is not a very descriptive variable name here.

src/components/OSCALControlModification.js

kylelaker · 2023-03-23T18:10:54Z

src/components/OSCALControlModification.js

@@ -42,13 +42,30 @@ function getAlterAddsOrRemovesDisplay(addsElements, addsLabel, controlPartId) {
        Name: {item.name}, Value: {item.value}
      </Typography>
    ));
+  let removedTypographies = null;
+  if (addsLabel === "Removes ") {


Yeah this string comes from

oscal-react-library/src/components/OSCALControlModification.js

Lines 145 to 163 in 57b305b

if (alter.adds) {

[addsDisplay, len] = getModifications(

controlPartId,

props.controlId,

alter.adds,

"Adds "

);

modLength += len;

}

// Get all remove modifications

if (alter.removes) {

[removesDisplay, len] = getModifications(

controlPartId,

props.controlId,

alter.removes,

"Removes "

);

modLength += len;

}

and it's an odd architectural decision...

mpemy · 2023-03-24T16:37:35Z

I pushed changes for my two issues #89 and #91 in this branch. I am sorry it may be confusing when reviewing, moving forward each issue will have its own branch.
As for issues #91 you will need a profile that has include-all and exclude-control.
The following fedRAMP profile has include-all and exclude-control
https://gist.githubusercontent.com/mpemy/b9bbae968fb0b700bc669e3c8288b223/raw/5e3fc1e6abfa284399870f0ac794e37dc41e3987/FedRAMP_rev4_LI-SaaS-baseline_profile.json
This other profile only has include-control and exclude-control.
https://gist.githubusercontent.com/mpemy/9ea8e9b285cc9198ded3fa46d4154850/raw/9969488e66064281e8394305d352ae73cb60d9ec/Correct_rev4_LI-SaaS-baseline_profile.json

mpemy · 2023-03-24T16:40:39Z

For this profile AC-1 is excluded

tuckerzp

Looking over this, it seems like we only are dealing with things being removed by-id. Looking at the remove outline, it seems we can also remove by name, class, item-name, and ns.

The acceptance criteria for #89 does not explicitly say we need to all of them, but it also doesnt say to just do by-id. Would this be a big change in this pr?

Overall though this looks a lot better, I am starting to wonder if maybe we push refactoring some of this code to a separate issue. It seems like a lot of this needs to be reworked when we rework profile resolution.

src/components/OSCALControlProse.js

src/components/OSCALControl.js

kylelaker

Okay so I think I am good with the overall approach here! There are just some specific code cleanups to resolve. I don't love how many times we return basically the same

<Typography>
  {props.label} {prose}
  {props.modificationDisplay}
</Typography>

But I suppose we can always refactor that later...

src/components/OSCALProfile.js

src/components/OSCALControlProse.js

kylelaker · 2023-03-28T17:42:56Z

It will be necessary to pull in the changes from the develop branch and resolve the merge conflicts. I am happy to help with that as I think they conflict with the work I just did in #746

Co-authored-by: Kyle Laker <klaker@easydynamics.com>

… into mpemy/display-remove-control-elements

…iew.

src/components/OSCALControlProse.js

Co-authored-by: Kyle Laker <klaker@easydynamics.com>

kylelaker

Thanks for all the work on this and for the iterations from review! I think at this point we're good to go ahead and merge. I still want to make sure that in the future we handle profile resolution more holistically; however, we have a separate ticket to implement that.

tuckerzp

@mpemy I had a few comments on things that were most likely lost in the sea of comments.

Since a lot of this will need refactoring, I am ok with the fact that we did not add unit tests for this. Would you be able to create an issue to test your added functionality at some point?

src/components/OSCALProfile.js

tuckerzp · 2023-03-30T16:53:36Z

src/components/OSCALControlModification.js

 * @param {String} controlPartId Control part ID to match
 * @returns html object
 */
-function getAlterAddsOrRemovesDisplay(addsElements, addsLabel, controlPartId) {
+function getAlterAddsOrRemovesDisplay(
+  addsElements,


@mpemy I wanted to bump this. I still think that addsElements is not a very descriptive variable name here.

Dismissing this so I don't hold up review after my changes are addressed.

tuckerzp · 2023-03-31T15:18:43Z

@mpemy can you send a link to that issue on this pr? I will move forward with merging once that is done. I just don't want to forget to add tests for this later.

mpemy · 2023-03-31T16:30:37Z

@tuckerzp these are the unit test issues:
https://github.com/EasyDynamics/easygrc/issues/181
https://github.com/EasyDynamics/easygrc/issues/180
Thanks.

mpemy and others added 9 commits March 6, 2023 02:55

Just address a typo that was mistakenly inserted in the last commit.

e37df27

Merge branch 'develop' into mpemy/display-remove-control-elements

2e64483

Merge branch 'mpemy/display-remove-control-elements' of github.com:mp…

efd7b97

…emy/oscal-react-library into mpemy/display-remove-control-elements merging these two branches so that I can complete this commit and fix the failing build

Address the last main issue with the new line not apprearing when the…

3971908

… symbol is contains in json prose.

Fix liniting issue

8f17664

Implementing the suggested recommendation for a cleaner version of th…

f7ab12b

…e fix

Implementation of the issue Display removed control parts from profile

57b305b

First gather all removed items and display them below the added items. Also implemented the strike-line to highlight the fact that these items are removed

easy-dynamics-oscal-automation bot requested a review from a team March 20, 2023 20:23

mpemy added 3 commits March 22, 2023 04:01

Implementing the suggestions and recommendations from the pull reques…

6348f6d

…t review

I have addressed issues raised during the code review, morevover I am…

e8d99e1

… also committing changes related to feat #91 support include-all and exclude-control

addressing the issue with some failing tests that occured after the l…

a4cabc0

…ast commit. Have modified the code to make sure the props.modificationDisplay is not undefined before trying to access its children

tuckerzp linked an issue Mar 23, 2023 that may be closed by this pull request

Display of Removed Control Elements in Profiles #89

Closed

tuckerzp reviewed Mar 23, 2023

View reviewed changes

easy-dynamics-oscal-automation bot requested a review from a team March 23, 2023 14:16

Bronstrom reviewed Mar 23, 2023

View reviewed changes

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

easy-dynamics-oscal-automation bot requested a review from a team March 23, 2023 16:40

tuckerzp previously requested changes Mar 23, 2023

View reviewed changes

kylelaker reviewed Mar 23, 2023

View reviewed changes

mpemy requested review from kylelaker, Bronstrom and tuckerzp March 24, 2023 12:01

tuckerzp reviewed Mar 28, 2023

View reviewed changes

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

src/components/OSCALControl.js Show resolved Hide resolved

kylelaker suggested changes Mar 28, 2023

View reviewed changes

mpemy and others added 6 commits March 29, 2023 10:14

Update src/components/OSCALProfile.js

57d3a01

Co-authored-by: Kyle Laker <klaker@easydynamics.com>

Update src/components/OSCALControlProse.js

e982675

Co-authored-by: Kyle Laker <klaker@easydynamics.com>

Update src/components/OSCALControlProse.js

6234494

Co-authored-by: Kyle Laker <klaker@easydynamics.com>

Update src/components/OSCALControlProse.js

de4a6eb

Co-authored-by: Kyle Laker <klaker@easydynamics.com>

Merge branch 'develop' of github.com:EasyDynamics/oscal-react-library…

928a94e

… into mpemy/display-remove-control-elements

Implementation of all recommendations and suggestions of the code rev…

204278d

…iew.

mpemy requested review from tuckerzp and kylelaker March 30, 2023 02:44

kylelaker reviewed Mar 30, 2023

View reviewed changes

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

mpemy and others added 2 commits March 30, 2023 11:23

Update src/components/OSCALControlProse.js

d106724

Co-authored-by: Kyle Laker <klaker@easydynamics.com>

Fix the failing linting

1798a63

kylelaker approved these changes Mar 30, 2023

View reviewed changes

tuckerzp reviewed Mar 30, 2023

View reviewed changes

easy-dynamics-oscal-automation bot requested a review from a team March 30, 2023 16:56

Fix the remaining issues raised in the pull review.

08f592a

mpemy requested a review from tuckerzp March 31, 2023 14:52

tuckerzp approved these changes Mar 31, 2023

View reviewed changes

tuckerzp merged commit 82c9f61 into develop Mar 31, 2023

tuckerzp deleted the mpemy/display-remove-control-elements branch March 31, 2023 18:12

kylelaker added the enhancement New feature or request label Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: display remove control elements #733

feat: display remove control elements #733

mpemy commented Mar 20, 2023 •

edited by kylelaker

tuckerzp commented Mar 23, 2023

mpemy commented Mar 23, 2023

mpemy commented Mar 23, 2023

tuckerzp left a comment

tuckerzp Mar 23, 2023

mpemy Mar 23, 2023

kylelaker Mar 23, 2023

tuckerzp left a comment •

edited

tuckerzp Mar 23, 2023

tuckerzp Mar 30, 2023

kylelaker Mar 23, 2023

mpemy commented Mar 24, 2023

mpemy commented Mar 24, 2023

tuckerzp left a comment

kylelaker left a comment

kylelaker commented Mar 28, 2023

kylelaker left a comment

tuckerzp left a comment

tuckerzp Mar 30, 2023

tuckerzp commented Mar 31, 2023 •

edited

mpemy commented Mar 31, 2023

	if (alter.adds) {
	[addsDisplay, len] = getModifications(
	controlPartId,
	props.controlId,
	alter.adds,
	"Adds "
	);
	modLength += len;
	}
	// Get all remove modifications
	if (alter.removes) {
	[removesDisplay, len] = getModifications(
	controlPartId,
	props.controlId,
	alter.removes,
	"Removes "
	);
	modLength += len;
	}

feat: display remove control elements #733

feat: display remove control elements #733

Conversation

mpemy commented Mar 20, 2023 • edited by kylelaker

tuckerzp commented Mar 23, 2023

mpemy commented Mar 23, 2023

mpemy commented Mar 23, 2023

tuckerzp left a comment

Choose a reason for hiding this comment

tuckerzp Mar 23, 2023

Choose a reason for hiding this comment

mpemy Mar 23, 2023

Choose a reason for hiding this comment

kylelaker Mar 23, 2023

Choose a reason for hiding this comment

tuckerzp left a comment • edited

Choose a reason for hiding this comment

tuckerzp Mar 23, 2023

Choose a reason for hiding this comment

tuckerzp Mar 30, 2023

Choose a reason for hiding this comment

kylelaker Mar 23, 2023

Choose a reason for hiding this comment

mpemy commented Mar 24, 2023

mpemy commented Mar 24, 2023

tuckerzp left a comment

Choose a reason for hiding this comment

kylelaker left a comment

Choose a reason for hiding this comment

kylelaker commented Mar 28, 2023

kylelaker left a comment

Choose a reason for hiding this comment

tuckerzp left a comment

Choose a reason for hiding this comment

tuckerzp Mar 30, 2023

Choose a reason for hiding this comment

tuckerzp commented Mar 31, 2023 • edited

mpemy commented Mar 31, 2023

mpemy commented Mar 20, 2023 •

edited by kylelaker

tuckerzp left a comment •

edited

tuckerzp commented Mar 31, 2023 •

edited