Pass Control Id into Control Part to Render Modifications #155
Conversation
Add a missing controlId prop to a ControlPart, as this was causing top-level implementation statement modifications to not be displayed.
mikeisen1
requested review from
rgauss,
kylelaker,
hreineck,
zclarkEDC,
kkennedy26 and
snorouzzadeh
There was a problem hiding this comment.
Thanks for your work on this! The fix should work, however there's a couple issues with the new test:
The changes to the test and test data would get rid of tests and docs for the existing top-level modification feature; see my comment on our use of that term.
I think instead of swapping out our existing top-level modification tests for some different types of modifications, the tests could be expanded to test for all the different situations in which modifications can be rendered:
- Top Level, ie. on the level of the control itself
- Modifications that apply to the "first level" of control parts, ie. modifications that apply to control parts called from the control itself
- Modifications that apply to control parts that were called from other control parts.
Since all of these are called originating from the Control component, this test could probably be made a part of OSCALControl.test.js
| @@ -42,7 +42,7 @@ export const exampleModificationAltersTopLevel = [ | |||
| "control-id": "control-1", | |||
| adds: [ | |||
| { | |||
| "by-id": "control-1", | |||
| "by-id": "control-1_smt", | |||
There was a problem hiding this comment.
This object shouldn't be changed. We've been using the term "top-level" to refer to modifications that apply to the control as a whole, not any of the individual control parts.
Changing this data also changes the Storybook; I believe the intention with the modifications in the Storybook page for the Control component was to show off the top-level modifications and how to render them; this change gets rid of that.
There was a problem hiding this comment.
That makes sense, will make those changes. I will create another test in OSCALControl.test and the appropriate test data in ModificationsData
Revert changes to old tests and test data, as the changes were impacting the Storybook. Instead, new tests and test data were added so the added functionality is still tested.
There was a problem hiding this comment.
Looks good. Only one minor change.
Also, in reviewing this I came up with some notes about the modification tests.
Seems we're testing modifications in three different places now, and they're a bit redundant:
The Profile test and Control Implementation Impl. Req. test both have a test for top-level modifications. And now the new test that you've added properly tests the modifications that were fixed in this PR.
Since the only place modifications can actually be called or rendered is from the Control component, I think it'd make more sense for all the modification tests to live in the Control test file (like the new one you have added), and have the parent components simply test if they can properly render the controls.
Though cleaning up the tests is probably out of the scope of this issue. Up to you if you want to tack that onto this PR or get this merged first.
src/components/OSCALControl.test.js
Outdated
|
|
||
| test("OSCALCatalog displays controls", () => { | ||
| const testControl = { id: "ac-1" }; | ||
| render(<OSCALControl control={testControl} />); | ||
| const result = screen.getByText("ac-1"); | ||
| expect(result).toBeVisible(); | ||
| }); | ||
|
|
||
| test("OSCALControl displays _smt modifications", async () => { |
There was a problem hiding this comment.
| test("OSCALControl displays _smt modifications", async () => { | |
| test("OSCALControl displays statement modifications", async () => { |
Just one nitpick - this can be a tad more descriptive
I somewhat alluded to this in the comment made when I created this PR. I'm going to reduce the redundancy as part of this PR, as it doesn't need to be it's own ticket. However, I wonder if we should keep one of the other control modifications tests, as the modification control ids are slightly different. As we saw in this bug, these are different situations in which the modifications are rendered. |
Migrate control modification tests to a single file to make it easier to discover them and see where potential issues could arise. Also, remove one control modification test as it was redundant.
Remove unused import to resolve linter errors that were causing the PR checks to fail.
Modified the OSCALControl file to add a missing
controlIdprop to ControlPart. This was causing some control modifications to not be displayed and we want all control modifications to be visible to the viewer.Additionally, test data and the test were modified to test these changes. Previously, the test for displaying control modifications in the
OSCALControlImplementationImplReq.testtest file was not testing anything different than the test for displaying control modifications in theOSCALProfile.testtest file. There was that redundancy despiteOSCALControlImplementationImplReq.testusingprofileModifyTestData, created for the purpose of testing whether top-level control modifications would be displayed. That test data, and subsequently the test file, were modified to test if a modifications for a control implementation ending in_smtwere properly displayed. The distinction of_smtis important, as it is a top-level control modification and its appearance in a control's modifications, coupled with the missingcontrolIdprop, caused the control modification to not be displayed.