Enable linter #67

tuckerzp · 2021-06-29T17:20:45Z

This makes our code conform to the linter as much as possible. I have removed /*disable-linter*/ comments where they were not needed. This led to a lot of issues with the code to arise, so some of the logic had to be changed and cleaned up. .eslintignore and .eslintrc.json were both disabling parts of the linter that did not need to be disabled. Finally, package.json had to be changed to lint .test files.

Where and why linter is still disabled

eslint no-param-reassign: "error"
- This is done in: ComponentResolver, ProfileResolver, & SSPResolver.
- Due to how we are using state and passing props, it would require a very large change to the code base to fix these linting issues. Instead of disabling large portions of the code, the linter is disabled for only a small segment.
.eslintrc.json rules
- Rules enabled:
  - react/jsx-filename-extension
  - react/prop-types
  - react/destructuring-assignment
  - react/jsx-props-no-spreading
  - prettier/prettier
- Much like above, it would take a significant change of the code base to conform to these linter rules. prettier/prettier makes eslint play nice with prettier.

uuid's and keys

I also imported react-uuid to deal with places where generating keys is an issue. Specifically OSCALControl generating a list of OSCALControlParts is a situation where having a meaningful and unique key is difficult. A uuid acts as a next best solution for providing a key. This may be something to deal with better in a future issue or this PR. I was hoping to discuss this more with the PR.

tuckerzp · 2021-06-29T17:22:50Z

I also thought it would be worthwhile to discuss linting practices now that our code should be in line with linter requirements.

Linting in the future

Writing code and fixing linting issues later on is not a very efficient way to use the linter. Like testing, I think it should be done as part of an issue not it's own issue. After working on this issue I think that some linter issues might just have to be ignored/disabled. When doing so, we should:

Prefer eslint no-param-reassign: "error". Disabling multiple lines of code is not necessary and instead should be disabled by individual line.
Document why it is needed to disable the linter either in comment or in PR.

tuckerzp · 2021-06-29T17:25:09Z

src/OSCALControl.js

            <OSCALControlPart
              part={part}
              parameters={props.control.params}
              implReqStatements={props.implReqStatements}
              componentId={props.componentId}
              control={props.control}
              modifications={props.modifications}
-              // eslint-disable-next-line
-              key={`part-${index}`}
+              key={uuid(part)}


This map is what I am referring to with not having a great key to use for each OSCALControlPart created.

kylelaker · 2021-06-29T19:27:23Z

What happened to package-lock.json?

hreineck

Looks good.

hreineck · 2021-06-29T21:29:22Z

src/oscal-utils/OSCALSspResolver.js

@@ -19,12 +19,15 @@ export default function OSCALSspResolveProfile(
  if (!profileUrl.startsWith("http")) {
    profileUrl = `${parentUrl}/../${profileUrl}`;
  }
+
+  // Fixing linting error here would take significant change to codebase given how we use props.
+  /* eslint no-param-reassign: "error" */


If I understand correctly, this is necessary because we are overwriting the props that are passed into this component? If so, that could be something to fix in the future; React specifies that components should have read-only props.

@hreineck yes that is why it is necessary. @rgauss or @zclarkEDC can correct me here, but I think this is due to an intentional decision on how we interact with state. So potentially something we will just have without so very big changes.

@hreineck and @tuckerzp, we could look at havingOSCALSspResolveProfile return the resolved controls rather than adding them to the ssp object.

Is that worth doing in this issue or another one? This is done in: ComponentResolver, ProfileResolver, & SSPResolver.

A separate issue please, thanks.

kylelaker

Lots of improvements here that I am very glad to see! Thank you for your work. I do have a few requests.

src/OSCALControl.js

src/OSCALControlPart.js

src/OSCALControlProse.js

src/OSCALMetadata.js

src/oscal-utils/OSCALControlResolver.js

kylelaker · 2021-06-29T23:58:10Z

src/oscal-utils/OSCALControlResolver.js

+  // If the control isn't at the root level, it may be a subcontrol of it's parent;
+  // so for example, "ac-2.3" may be a child of "ac-2"
+  const isSubControl = (testControlId) => testControlId.includes(".");
+  if (isSubControl(controlId)) {
+    const parentControlId = controlId.split(".")[0];
+    const parentControl = findControlById(resolvedControls, parentControlId);
+    const subControl = findControlById(parentControl?.controls, controlId);
+
+    if (subControl) {
+      return subControl;
    }
  }


Is the complexity of this check worth the potential improvement in the case where it works? I'd be curious to see how often we end up having to fall through to the flatMap().find() step below. Always? Sometimes? Never? We may not know that. But we should somehow track this as a potentially overly complex and unnecessary performance improvement.

From just testing default profile, I have found it did not have use the flatMap().find() at all. I also have not seen many examples, if any, where the control is not in the root level or a sub control of it's parent. I am leaning towards it being worth it but also am curious if there are examples that disprove what I am seeing.

The convention of 'subcontrols' having a '.' notation is 800-53-specific, but that is certainly the most used control framework in OSCAL at the moment.

As is, I don't believe this comprehensively address the specification since controls can have an unbounded depth, and we'd need to traverse that.

There probably aren't enough controls for performance to really be an issue with 800-53.

Please create a Jira issue to address an unbounded control depth and further investigate the solution in the future.

package.json

src/OSCALControl.js

kylelaker · 2021-07-01T21:47:34Z

While this looks good; I wonder whether it'd be easier to hold this until after #63 is merged. Feels easier to fix/move any lint errors than to ask to have the owner(s) of #63 resolve conflicts again

rgauss

Follow up tasks, but the changes look good. Thanks!

rgauss · 2021-07-01T20:05:41Z

src/OSCALCatalogGroup.js

@@ -11,7 +11,7 @@ import FolderIcon from "@material-ui/icons/Folder";
 import { makeStyles } from "@material-ui/core/styles";
 import OSCALControl from "./OSCALControl";

-const useStyles = makeStyles((theme) => ({
+const useStyles = makeStyles(() => ({


Did the linter complain about the theme not being used?

Yeah it does not like unused vars.

rgauss · 2021-07-01T20:09:50Z

src/OSCALControl.js

-        color: "#d4d4d4",
-      };
-    }
+  if (props.control?.props?.find((property) => property.name === "status")) {


Looks like this needs a property.value === "withdrawn" check as well. Please raise a separate bug.

This is being done in #127

src/OSCALMetadata.js

rgauss · 2021-07-01T21:49:28Z

src/oscal-utils/OSCALControlResolver.js

+  // If the control isn't at the root level, it may be a subcontrol of it's parent;
+  // so for example, "ac-2.3" may be a child of "ac-2"
+  const isSubControl = (testControlId) => testControlId.includes(".");
+  if (isSubControl(controlId)) {
+    const parentControlId = controlId.split(".")[0];
+    const parentControl = findControlById(resolvedControls, parentControlId);
+    const subControl = findControlById(parentControl?.controls, controlId);
+
+    if (subControl) {
+      return subControl;
    }
  }


The convention of 'subcontrols' having a '.' notation is 800-53-specific, but that is certainly the most used control framework in OSCAL at the moment.

As is, I don't believe this comprehensively address the specification since controls can have an unbounded depth, and we'd need to traverse that.

There probably aren't enough controls for performance to really be an issue with 800-53.

Please create a Jira issue to address an unbounded control depth and further investigate the solution in the future.

rgauss · 2021-07-01T21:50:25Z

src/oscal-utils/OSCALSspResolver.js

@@ -19,12 +19,15 @@ export default function OSCALSspResolveProfile(
  if (!profileUrl.startsWith("http")) {
    profileUrl = `${parentUrl}/../${profileUrl}`;
  }
+
+  // Fixing linting error here would take significant change to codebase given how we use props.
+  /* eslint no-param-reassign: "error" */


A separate issue please, thanks.

rgauss · 2021-07-01T21:52:12Z

While this looks good; I wonder whether it'd be easier to hold this until after #63 is merged. Feels easier to fix/move any lint errors than to ask to have the owner(s) of #63 resolve conflicts again

Complete agree, please get #63 in first.

kylelaker · 2021-07-01T22:06:18Z

.eslintignore

 ## uncomment this to disable linting 
 ##src/**/*.js
-src/serviceWorker.ts
-src/oscal-utils/OSCALSspResolver.js
-public/*


I'd almost like to just remove this file and find another way to disable the linter when necessary

That is a good point, but I believe #63 adds things to ignore again. I do not think we have completely resolved what is needed and what isn't, but it is safe to assume there will be something in the file after that is merged.

tuckerzp · 2021-07-07T17:51:22Z

Changes from EGRC-168 are now merged into EGRC-412. It was a little wonky so I had to make changes to OSCALLoader & package*. I think it should be ready for one last review and to be merged.

zclarkEDC

Excellent work resolving the merge conflicts.

There were a few places where the linter was disabled unnecessarily. Removing the /*disable*/ comment and doing minor formatting resolved any issues the linter had originally.

There were multiple linter violations due to illegal function useage and logic. Cleaning up these functions to be in accordance with style guide makes code cleaner and keeps original functionality.

Linter checking for these issues was disabled. We renabled checking for this issue and fixed problems that arose.

OSCALProfile was attempting to preform a React state update on an unmounted component. This indicates a memory leak and caused the tests to have warnings. We added a check to see if a component is mounted before updating state.

OSCALControl was using an index as a key for each OSCALControlPart created. This is advised against by react. The parts being mapped do not have a clear id or field to use as a unique key. Using uuid is the next best way of giving each child element of a list a unique key that persists until the child is recreated.

A few changes were needed to make the code more elegant and readable. Using part.id in OSCALControlPart when possible will also speed up the code.

Due to concern for preformace, index makes more sense to use than generating a uuid for each part.

OSCALMetadata's getPartyRolesText function sometimes would try to use filter() on null values.

EGRC-168 made some very major changes. In order to merge EGRC-412 into develop, they had to be merged. Changes to OSCALLoader & the package-lock files were also needed.

tuckerzp · 2021-07-07T18:09:14Z

After another fun round of merge conflicts, I now am fairly certain we can review and merge.

kylelaker

Looks good! Thank you so much for your patience with all these rebases and conflicts. You've done a great job with this.

tuckerzp requested review from kkennedy26, rgauss, zclarkEDC, Bronstrom, folksgl, hreineck and kylelaker June 29, 2021 17:23

tuckerzp commented Jun 29, 2021

View reviewed changes

tuckerzp self-assigned this Jun 29, 2021

tuckerzp force-pushed the EGRC-412 branch from 5acc238 to 6b71d50 Compare June 29, 2021 19:14

tuckerzp force-pushed the EGRC-412 branch 2 times, most recently from 45b1588 to ef93d7c Compare June 29, 2021 19:53

hreineck approved these changes Jun 29, 2021

View reviewed changes

kylelaker suggested changes Jun 30, 2021

View reviewed changes

tuckerzp force-pushed the EGRC-412 branch 2 times, most recently from cfe1499 to 877b546 Compare June 30, 2021 17:57

rgauss reviewed Jun 30, 2021

View reviewed changes

src/OSCALControl.js Outdated Show resolved Hide resolved

tuckerzp mentioned this pull request Jul 1, 2021

Build OSCAL React library #63

Merged

kylelaker approved these changes Jul 1, 2021

View reviewed changes

tuckerzp requested a review from rgauss July 1, 2021 19:29

kkennedy26 approved these changes Jul 1, 2021

View reviewed changes

rgauss approved these changes Jul 1, 2021

View reviewed changes

kylelaker reviewed Jul 1, 2021

View reviewed changes

tuckerzp force-pushed the EGRC-412 branch from 74f0d57 to 36e6ef8 Compare July 7, 2021 17:49

kylelaker approved these changes Jul 7, 2021

View reviewed changes

zclarkEDC approved these changes Jul 7, 2021

View reviewed changes

tuckerzp added 12 commits July 7, 2021 14:04

Enable linter where it shouldnt be disabled

a0e1f58

There were a few places where the linter was disabled unnecessarily. Removing the /*disable*/ comment and doing minor formatting resolved any issues the linter had originally.

Change logic of code to satisfy linter

8711bb5

There were multiple linter violations due to illegal function useage and logic. Cleaning up these functions to be in accordance with style guide makes code cleaner and keeps original functionality.

Disallow shadowing variables and unused vars

8170c05

Linter checking for these issues was disabled. We renabled checking for this issue and fixed problems that arose.

Remove SSPResolver from lint ignore

1e1d09a

Start linting all .test.js files

0320bf9

Cancel asynchronous tasks in OSCALProfile

dd149ea

OSCALProfile was attempting to preform a React state update on an unmounted component. This indicates a memory leak and caused the tests to have warnings. We added a check to see if a component is mounted before updating state.

Clean code up

27ca72e

A few changes were needed to make the code more elegant and readable. Using part.id in OSCALControlPart when possible will also speed up the code.

Use index instead of uuid

9082a1e

Due to concern for preformace, index makes more sense to use than generating a uuid for each part.

Fix cases where parties are null

34b14c6

OSCALMetadata's getPartyRolesText function sometimes would try to use filter() on null values.

Use part.id when available

80d9af1

Fix merging issues

d85de92

EGRC-168 made some very major changes. In order to merge EGRC-412 into develop, they had to be merged. Changes to OSCALLoader & the package-lock files were also needed.

tuckerzp force-pushed the EGRC-412 branch from 36e6ef8 to d85de92 Compare July 7, 2021 18:08

kylelaker approved these changes Jul 7, 2021

View reviewed changes

Fix OSCALMetadata

2028b93

kylelaker merged commit 1ab856e into develop Jul 7, 2021

kylelaker deleted the EGRC-412 branch July 7, 2021 18:19

This was referenced Jul 19, 2021

Support for Control Depth: Unbounded #126

Open

Control Display Does not Check Withdrawn Status #127

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable linter #67

Enable linter #67

tuckerzp commented Jun 29, 2021

tuckerzp commented Jun 29, 2021 •

edited

tuckerzp Jun 29, 2021

kylelaker commented Jun 29, 2021

hreineck left a comment

hreineck Jun 29, 2021

tuckerzp Jun 30, 2021

rgauss Jun 30, 2021

tuckerzp Jun 30, 2021

rgauss Jul 1, 2021

kylelaker left a comment

kylelaker Jun 29, 2021

tuckerzp Jun 30, 2021

rgauss Jul 1, 2021

kylelaker commented Jul 1, 2021

rgauss left a comment

rgauss Jul 1, 2021

tuckerzp Jul 2, 2021

rgauss Jul 1, 2021

tuckerzp Jul 20, 2021

rgauss Jul 1, 2021

rgauss Jul 1, 2021

rgauss commented Jul 1, 2021

kylelaker Jul 1, 2021

tuckerzp Jul 2, 2021

tuckerzp commented Jul 7, 2021 •

edited

zclarkEDC left a comment

tuckerzp commented Jul 7, 2021

kylelaker left a comment

Enable linter #67

Enable linter #67

Conversation

tuckerzp commented Jun 29, 2021

Where and why linter is still disabled

uuid's and keys

tuckerzp commented Jun 29, 2021 • edited

Linting in the future

Choose a reason for hiding this comment

kylelaker commented Jun 29, 2021

hreineck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylelaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylelaker commented Jul 1, 2021

rgauss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgauss commented Jul 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuckerzp commented Jul 7, 2021 • edited

zclarkEDC left a comment

Choose a reason for hiding this comment

tuckerzp commented Jul 7, 2021

kylelaker left a comment

Choose a reason for hiding this comment

tuckerzp commented Jun 29, 2021 •

edited

tuckerzp commented Jul 7, 2021 •

edited