-
Notifications
You must be signed in to change notification settings - Fork 11
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
Edit SSP Control Implementations UI #277
Conversation
Created the UI for editing default control implementation labels. This includes creating the appropriate partial PATCH data.
If a user edits a control implementation and does so by modifying a placeholder value, they will be adding a new statement by-component. This requires a description, so this commit allows users to create their own description and then send the REST request.
Add the lodash dependency as the deep copying utility function will be used.
Redesign the control implementation editing UI to use a popover for editing instead of inline text fields or modals. This commit also appropriately sends in the REST request, updating the Control and other files as needed so a valid request is sent.
…-library into edit-control-implementations-ui
Add documentation of all params of restPatch function. Move populatePartialPatchData back to OSCALLoader as it is only called by restPatch. This makes it unecessary to add documentation for populatePartialPatchData.
Resolve import statements of populatePartialPatchData and move the function to OSCALSspResolver to avoid circular dependencies.
Rename functions and variables, as well as adjust the wording of documentation, to accurately reflect the purpose of the underlying function or variable.
I know we mentioned that we wanted the backend service to create a new uuid if necessary, but I left the uuid and the auto-generation of the new uuid in for now until we definitively decide the path taken for that. The new uuid generation is necessary when editing a parameter label, as in that situation, there is no by-component object, which requires a uuid, for that statement by component. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to have to break my review on this into a couple different passes -- here's my first try at it. I'll try to do another here soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few code documentation things. I will leave the rest to Kyle's next couple passes.
props.editedField, | ||
props.reference.current.value | ||
); | ||
if (props.onFieldSave.length === 6) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is this called with no arguments? If we do need it we should not pin to a specific number of arguments. See #351.
<Typography>Parameter Values: </Typography> | ||
</Grid> | ||
<Grid item xs={9}> | ||
<Autocomplete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we want an Autocomplete
here. The point in previous comments was that a specific line of prose within a part might reference more that one parameter, so we'd need to send objects containing the param-id and value (and potentially remarks in the future. See #351 updateSspControlImplementationImplementedRequirementStatementByComponent
.
src/components/OSCALControlProse.js
Outdated
import StyledTooltip from "./OSCALStyledTooltip"; | ||
import { getStatementByComponent } from "./oscal-utils/OSCALControlResolver"; | ||
import OSCALControlPartEditor, { | ||
onFieldSaveByComponentParameterValue, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems very strange to import these from OSCALControlPartEditor
only to send them back in to the rendering of OSCALControlPartEditor
below. A calling component should not need to know the inner workings of child components it renders.
src/components/OSCALControlProse.js
Outdated
return ( | ||
<OSCALReplacedProseWithParameterLabel | ||
label={props.label} | ||
prose={props.prose} | ||
parameters={props.parameters} | ||
modificationDisplay={props.modificationDisplay} | ||
className={classes.OSCALStatementNotImplemented} | ||
controlId={props.controlId} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we're greatly overloading this component. Rather than trying to pass in everything to enable that component to make an update, perhaps we should consider rethinking where the rendering of OSCALControlPartEditor
is declared and potentially rendering a different component depending on the edit state of the control part.
src/components/OSCALControlProse.js
Outdated
onCancel={() => { | ||
setAnchorEl(null); | ||
}} | ||
onFieldSave={(descriptionReference, implementationReference) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, we shouldn't import functions like onFieldSaveByComponentParameterValue
from a component then send them back in to rendering declaration of that component.
…ontrol-implementations-ui-rgauss-2
…ontrol-implementations-ui-rgauss-2
…lementations-ui-rgauss-2 Separation of Model/REST code from Rendering
…ontrol-implementations-ui
…lementations-ui Suggestion/edit control implementations UI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are maybe a few things we can still work on here (I mean, there are some TODO
s) but overall, I think anything I see is really nitpicky and I don't want to hold this up. We've done quite a bit of review across several PRs now and I am, overall, really happy with the state of this. Thanks for all the work on this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work!
Created #390 for the tracking future work on the TODO mentioned in standup.
Closes #337
Creates the user interface for editing existing control implementations. These edits can be to statement descriptions, placeholder labels for non-existent set-parameter values, and to existing set-parameter values. The UI then sends in an HTTP PUT request to a backend service and handles the response. This allows a user, given the backend can handle the request, to modify existing control implementations of an SSP using the OSCAL Viewer. Like all edits using the OSCAL Viewer, changes are reflected on the viewer if the request is successful and are not reflected on the viewer if the request is not successful. On an unsuccessful request, an alert is sent to the user notifying them that the changes could not be made.
For the HTTP PUT request, the url path is of the form
/system-security-pan/{ssp-uuid}/control-implementation/implemented-requirements/{implemented-requirement-uuid}
, where we are just passing in the body of one implemented requirement, not the entire SSP.