Skip to content
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

Suggestion/edit control implementations UI #387

Conversation

rgauss
Copy link
Contributor

@rgauss rgauss commented May 5, 2022

This suggestion PR begins to simplify what information needs to be passed between components and moves the main editing capabilities to OSCALControlProse so that parameter inputs can be rendered inline in the prose.

Screen Shot 2022-05-05 at 9 07 14 AM

@rgauss
Copy link
Contributor Author

rgauss commented May 5, 2022

This is incomplete at the moment as it doesn't handle statementByComponents that don't yet exist and the onSuccess, onError page behavior isn't tied in yet.

I'll continue working on it.

@rgauss
Copy link
Contributor Author

rgauss commented May 6, 2022

This is definitely worth starting to look at. I'll give a pass of more comments for explanation and move it out of draft state soon.

Copy link
Contributor

@kylelaker kylelaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't really looked into JSX/React bits here but there is a little bit of cleanup that we might be able to do to make some of the null handling a little cleaner. Mostly leveraging ?? and ??=.

src/components/oscal-utils/OSCALRestUtils.js Outdated Show resolved Hide resolved
src/components/oscal-utils/OSCALRestUtils.js Outdated Show resolved Hide resolved
src/components/oscal-utils/OSCALRestUtils.js Outdated Show resolved Hide resolved
src/components/oscal-utils/OSCALRestUtils.js Outdated Show resolved Hide resolved
src/components/OSCALControlProse.js Outdated Show resolved Hide resolved
src/components/OSCALControlProse.js Outdated Show resolved Hide resolved
src/components/OSCALLoader.js Show resolved Hide resolved
@@ -85,14 +85,14 @@ export default function OSCALControl(props) {
control={props.control}
controlId={props.control.id}
parameters={props.control.params}
implReqStatements={props.implReqStatements}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this and similar changes, implementedRequirement contains implReqStatements so we don't need to pass around both.

isEditable={props.isEditable}
onFieldSave={props.onFieldSave}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're not using the simple field a more generic onRestSuccess was used instead.

@rgauss rgauss marked this pull request as ready for review May 9, 2022 17:16
@rgauss rgauss requested a review from kylelaker May 9, 2022 17:34
@rgauss
Copy link
Contributor Author

rgauss commented May 9, 2022

This is ready for a look.

Copy link
Contributor

@kylelaker kylelaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should have been a request on an earlier PR but looking into this a little more, the result request is structured like:

PUT /system-security-plans/cff8385f-108e-40a5-8f7a-82f3dc0eaba8/control-implementation/implemented-requirements/aaadb3ff-6ae8-4332-92db-211468c52af2

{
  "control-id": "...",
  "statements": [...],
  "uuid": "..."
}

But according to the specification (localhost:8080, source), the request needs to have a wrapping object with an "implemented-requirement" key:

{
  "implemented-requirement": {
    "control-id": "...",
    "statements": [],
    "uuid": "..."
  }
}

@kylelaker
Copy link
Contributor

Also to be totally clear, other than the minor issue with the request object structure, this is absolutely lit!! I am so looking forward to getting the service implementation done because this is awesome!

@rgauss
Copy link
Contributor Author

rgauss commented May 10, 2022

Maybe this should have been a request on an earlier PR but looking into this a little more, the result request is structured like:

PUT /system-security-plans/cff8385f-108e-40a5-8f7a-82f3dc0eaba8/control-implementation/implemented-requirements/aaadb3ff-6ae8-4332-92db-211468c52af2

{
  "control-id": "...",
  "statements": [...],
  "uuid": "..."
}

But according to the specification (localhost:8080, source), the request needs to have a wrapping object with an "implemented-requirement" key:

{
  "implemented-requirement": {
    "control-id": "...",
    "statements": [],
    "uuid": "..."
  }
}

Good catch, fixed.

@rgauss rgauss requested a review from kylelaker May 10, 2022 01:34
Copy link
Contributor

@kylelaker kylelaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved! Thanks for the work on this. Because this is going into another feature branch, this can be merged (well... it could have before too); I'll leave it up to you whether you want to merge it or wait for another review.

@rgauss
Copy link
Contributor Author

rgauss commented May 10, 2022

@mikeisen1, want to make sure you have a chance to ask any questions here. Let me know if you'd like to go through it together.

@mikeisen1
Copy link
Contributor

mikeisen1 commented May 10, 2022

@mikeisen1, want to make sure you have a chance to ask any questions here. Let me know if you'd like to go through it together.

Let's discuss it during standup tomorrow? I have taken a look through, but a quick discussion would be nice before merging this.

@mikeisen1 mikeisen1 merged commit 9750495 into edit-control-implementations-ui May 11, 2022
@mikeisen1 mikeisen1 deleted the suggestion/edit-control-implementations-ui branch May 11, 2022 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants