Separation of Model/REST code from Rendering #351
Conversation
… github.com:EasyDynamics/oscal-react-library into suggestion/edit-control-implementations-ui-rgauss-2
| @@ -94,7 +94,6 @@ export default function OSCALControl(props) { | |||
| isEditable={props.isEditable} | |||
| onFieldSave={props.onFieldSave} | |||
| partialRestData={props.partialRestData} | |||
| restPath={props.restPath} | |||
There was a problem hiding this comment.
OSCAL object renderers shouldn't need to be passed a restPath. The component knows what oscalObjectType it's rendering and can pass that to REST functions if needed.
|
The installation failures here were, I think, fixed in #338. Let me know if that seems to not be the case. |
| @@ -18,14 +18,12 @@ export default function OSCALEditableFieldActions(props) { | |||
| : `save-${props.editedFieldPath}` | |||
| } | |||
| onClick={() => { | |||
| if (props.onFieldSave.length === 6) { | |||
| if (props.onFieldSave.length > 0) { | |||
There was a problem hiding this comment.
I haven't looked in to when we'd call onFieldSave without arguments, but we shouldn't pin to the number of arguments if we can avoid it as that may change.
| @@ -13,41 +13,6 @@ import OSCALProfile from "./OSCALProfile"; | |||
| import OSCALLoaderForm from "./OSCALLoaderForm"; | |||
| import OSCALJsonEditor from "./OSCALJsonEditor"; | |||
|
|
|||
| const oscalObjectTypes = { | |||
There was a problem hiding this comment.
Moved this to OSCALRestUtils
| * @param restPath main url path for access the OSCAL files in REST mode | ||
| */ | ||
| const handleRestRequest = ( | ||
| const handleFieldSave = ( |
There was a problem hiding this comment.
Something named handleRestRequest should really only do that, but here we had a single function manipulating the data, building the request, and performing the request, and defining behaviors for success and failure.
handleFieldSave is still responsible for all of that, but delegates isolated bits of that functionality to other functions.
| partialRestData, | ||
| restMethods.PATCH, | ||
| requestUrl, | ||
| () => { |
There was a problem hiding this comment.
We just pass the pre, success, and failure behavior functions into performRestRequest.
| ) => ( | ||
| <OSCALCatalog | ||
| catalog={oscalData[oscalObjectType.jsonRootName]} | ||
| parentUrl={oscalUrl} | ||
| onResolutionComplete={onResolutionComplete} | ||
| onFieldSave={( | ||
| appendToLastFieldInPath, | ||
| data, |
There was a problem hiding this comment.
We want to keep naming as explicit as possible for others maintaining the code.
src/components/OSCALLoader.js
Outdated
| @@ -4,7 +4,7 @@ import Split from "react-split"; | |||
| import { makeStyles } from "@material-ui/core/styles"; | |||
| import { Box, Fab } from "@material-ui/core"; | |||
| import CodeIcon from "@material-ui/icons/Code"; | |||
| import { populatePartialRestData } from "./oscal-utils/OSCALUtils"; | |||
| import { populatePartialRestData, buildRestRequestUrl, performRestRequest, restMethods, oscalObjectTypes } from "./oscal-utils/OSCALRestUtils"; | |||
There was a problem hiding this comment.
Maybe:
import * as restUtils from "./oscal-utils/OSCALRestUtils";Or maybe a better local name than restUtils. It'd require some refactoring to use the namespace. But it also means that we could rename the actual functions to be populatePartialData, buildRequestUrl, performRequest, and methods since the whole "REST" thing is implied by the file/module name.
| let url; | ||
| if (!restUrlPath || restUrlPath === "") { | ||
| url = `${process.env.REACT_APP_REST_BASE_URL}/${oscalObjectType.restPath}/${partialRestData[oscalObjectType.jsonRootName].uuid}`; | ||
| } else if (restUrlPath.startsWith("http", 0)) { | ||
| url = restUrlPath; | ||
| } else { | ||
| url = `${process.env.REACT_APP_REST_BASE_URL}/${restUrlPath}`; | ||
| } | ||
| return url; |
There was a problem hiding this comment.
Maybe we can actually build a URL? That way we can actually specify paths vs host etc? This may just be a conversation for a later ticket.
There was a problem hiding this comment.
Yeah, this iteration was just moving the code over into REST utils. Feel free to repeat this comment on the main PR, or create a separate issue.
| PUT: "PUT", | ||
| }; | ||
|
|
||
| export const oscalObjectTypes = { |
There was a problem hiding this comment.
It's debatable whether the name, defaultUrl, and defaultUuid really belong here. We may want to pull them out and into another enum-like object in a different file.
There was a problem hiding this comment.
I'd say it makes sense to pull them into a separate file, but only when we break OSCALRestUtils down into more files to separate out different aspects of the REST requests or OSCAL types and generic OSCAL utils (which could also contain the deepClone function). We are not at that point right now, but should note that they are not similar enough to other functions/enums in this file to keep them together in the same file long-term.
| * @param onSuccess function called on a successful REST request with the result of the request as an argument | ||
| * @param onError function called on error with the error as an argument | ||
| */ | ||
| export function updateSspControlImplementationImplementedRequirementStatementByComponent( |
There was a problem hiding this comment.
This isn't used yet, but shows how we could continue the pattern of isolating the model/data manipulation and REST calls from the rendering, where this would be called from OSCALControlPartEditor.
| PUT: "PUT", | ||
| }; | ||
|
|
||
| export const oscalObjectTypes = { |
There was a problem hiding this comment.
I'd say it makes sense to pull them into a separate file, but only when we break OSCALRestUtils down into more files to separate out different aspects of the REST requests or OSCAL types and generic OSCAL utils (which could also contain the deepClone function). We are not at that point right now, but should note that they are not similar enough to other functions/enums in this file to keep them together in the same file long-term.
| * @param componentId the id of the by-component to be updated | ||
| * @param descriptionReference reference to the text field input containing the new description for the control implementation |
There was a problem hiding this comment.
Could we consider revising this function, as well as renaming the parameter, so that descriptionReference is not a reference to a text field input but rather just a string value that is a description for the control implementation? It would allow for greater flexibility for the function and does not force a developer to pass in a reference to an input field.
|
Lines 41 and 116 in |
@mikeisen1, addressed this. |
| onFieldSave={( | ||
| appendToLastFieldInPath, | ||
| data, | ||
| editedField, | ||
| partialRestData, | ||
| editedFieldJsonPath, | ||
| newValue, | ||
| restMethod, | ||
| restUrlPath | ||
| ) => { |
There was a problem hiding this comment.
The number of arguments here do not match up with the props.onFieldSave that is called in OSCALEditableFieldActions, which has support for 0 or 4 arguments.
There was a problem hiding this comment.
@mikeisen1, right, we're passing in a function that can take 5 arguments, but OSCALEditableFieldActions is only calling that function with 4 arguments, so restUrlPath will be null and handleFieldSave calls restUtils.buildRequestUrl which will construct the url from the partialRestData and oscalObjectType.
That works for simple field edits (as implied by the OSCALEditableFieldActions component name), and more complex operations like control editing would probably do better calling more specific REST util methods like updateSspControlImplementationImplementedRequirementStatementByComponent rather than trying to handle those scenarios with an overloaded onFieldSave, but again, I was trying to minimize the changes in this suggestion so did not yet wire that in to OSCALControlPartEditor
mikeisen1
dismissed
their stale review
There are some merge conflicts still waiting to be resolved
…ontrol-implementations-ui-rgauss-2
…ontrol-implementations-ui-rgauss-2
|
@mikeisen1, I merged |
|
As mentioned in standup, merging this with 1 approval since it's just a suggestion PR. |
@mikeisen1, as mentioned in standup, this is the start of pulling OSCAL model and REST handling code out of UI rendering and into utils, which can start to form sort of a pure Javascript SDK for the REST API.
It also starts to better define the responsibilities of functions and components, limiting what is passed around between them.
Note that this does not yet address the actual control editing as there are some other issues to discuss/resolve there, but it should show a path to how we can restructure the model and REST interaction there.