Fix broken Component Control Implementation display #56
Conversation
Bronstrom
requested review from
rgauss,
kylelaker,
folksgl,
zclarkEDC and
kkennedy26
There was a problem hiding this comment.
Thanks! I've got some initial comments but need to step away for a bit but I want to provide this feedback so you can integrate as you're able. I'll do a more complete review later. I definitely appreciate the additions of new JSDoc but we want to make sure that if we're adding it that we're making it valuable, especially if it's on functions we're not otherwise changing.
Agreed. Note that we have a story dedicated to improving docs in the backlog. |
src/OSCALControlPart.js
Outdated
| function getParameterValue(parameterId) { | ||
| // trim | ||
| // Trims the parameterId to a string containing purley the id, | ||
| // removing the extra formating |
There was a problem hiding this comment.
The spirit of what this line is doing is covered in the comment line directly above it. This line should either be removed or be more descriptive about exactly what formatting is being removed. i.e. why 18? why len - 3?
There was a problem hiding this comment.
This is what's known as a "magic number"
There was a problem hiding this comment.
I'm also not convinced this is the right way to handle it. Seems like you're taking something like {{ insert: param, control-1_prm_1 }} and trying to cut it down to just control-1_prm_1. And that's not going to be a great way to do it. Poor formatting (extra/missing spaces) or other issues could totally break this in unexpected ways. Is there any way that we can parse that templating instead? That would certainly be my preference.
There was a problem hiding this comment.
To be fair, this line is not being added by this PR, and the specification for the parameter placeholders was previously much simpler and was then changed by NIST to include insert: param, but it is (now) a well defined, required format within prose.
We can throw a regex replacement in there which would be more readable code, and while we could try to accommodate potential formatting issues with it, that would technically no longer be following the OSCAL spec so should probably not be done.
There was a problem hiding this comment.
Good point. To be clear, my thought to consider more of a parsing direction was far more forward looking than this PR; however, if this comment is being rewritten anyway, then I think providing the full explanation of:
- based on structure of the parameter placeholders
- requires proper spacing/formatting/structure
{{ insert: param,is 18 chars and}}is 3.
But I think @folksgl was pointing out that something like:
const CHARS_BEFORE_PARAM_NAME = 18;
const CHARS_AFTER_PARAM_NAME = 3;
parameterId = parameterId.substring(CHARS_BEFORE_PARAM_NAME, parameterId.length - CHARS_AFTER_PARAM_NAME);And while I can't really find anything directly in the spec on this, the RC2 release notes imply that it should be possible for the type to be values other than param. But again, this something for a later discussion.
There was a problem hiding this comment.
But @Bronstrom, for clarity, focus on my comments about the comment. Not the code suggestions.
There was a problem hiding this comment.
I just changed the comment based on suggestions from @kylelaker. Let me know if it should be clarified further.
There was a problem hiding this comment.
We can throw a regex replacement in there which would be more readable code
FYI, the regex that's already in there for replacedProse would probably work for the above suggestion.
Bronstrom
changed the title
src/OSCALControlPart.js
Outdated
| * @param {object} implReqStatements Implementation Request Statements | ||
| * @param {string} statementId Id of a statement | ||
| * @param {string} componentId Id of a component | ||
| * @returns Returns the by-component object when a statement is found | ||
| */ | ||
| /* eslint-disable */ |
There was a problem hiding this comment.
I am just curious, is disabling eslint still needed here after changes? @Bronstrom
There was a problem hiding this comment.
Good suggestion. Made some modifications to make this work - main issue being consistent-return (which can be a simple fix just by adding a null/undefined type to a return without a value). To remove more of these eslint-enable/eslint-disable lines in OSCALControlPart, we can make a similar change to other functions. Nice to get rid of a couple of these.
The grouping scheme for several collections have been changed in OSCAL to be "BY-ARRAY", rather than "BY-KEY". SSP Viewer wasn't displaying the control implementation correctly for each component (missing highlighted statements and hrefs), as a result of key values returning undefined in OSCALControlPart. The following constants use their associated id/uuid values in place of their former removed key values: byComponent, statement, and parameterSetting.
Several Component Implementation tests were failing due to mock test data being inaccurate to new NIST json files. The test data has been amended. Additional documentation has been provided as well to OSCALControlPart and Control Implementation files to lessen the amount of code tracing.
Some code conditional statements were modified for better readibility and conciseness. Additionally, missing JSDoc parameter details and explanations were added.
Due to the recent changes from NIST, the System Security Plan (SSP) Viewer wasn't properly displaying the component control implementations correctly. When selecting the tabs on the left side of the Control Implementation, the panel would remain the same, rather than showing highlighted statements. The main issue was found in
OSCALControlPartwhen selecting statements, andby-components,statements, andset-parameterscollections were matched with a key that returned undefined. In the past these collections of objects were using the grouping scheme"BY_KEY", which have been recently changed to"ARRAY". InOSCALControlParts, the mapping to keys has been removed and objects from these collections are referenced with their respective uuid and id attributes. Highlighted statements now appear, as well as hoverable hyperlinks in the Control Implementation. TheOSCALControlImplemenationImplReqtests data have also been updated to reflect NIST's json files.