Feat: allow suppression of rules from the reports page #4623
Conversation
|
Do you think text-based is enough? i'm worried about an actual "toggle" button being distractingly visible at the top of each finding.
|
|
@sknep will you run |
|
What's your thoughts on adding another toggle location at the top of the page in the violated rules list? Like 👇 |
|
hmm... I don't really want folks to be able to suppress without fully understanding the finding. Also that spot is already kind of busy/full, and keeping these reports scannable is important to me. But what do you think? |
There was a problem hiding this comment.
I like the idea of having this alongside the rule (not in the top summary) and we can test out whether folks notice/use it in the future. I think maybe it could be more noticable but 🤷 . It looks like the suppression toggle logic has 2 issues:
- I think it's reversed because of some react hook "stuff" going on
- If a user removes a suppression that had a
matchproperty, that would be removed when trying to re-enable it (maybe that's okay, we should just note)
api/controllers/build-task.js
Outdated
|
|
||
| const type = appMatch(task.BuildTaskType); | ||
|
|
||
| const fullJSON = { | ||
| ...taskJSON, report, type, siteId: task.Build.Site.id, | ||
| ...taskJSON, report, type, siteId: task.Build.Site.id, siteBuildTaskId: task.SiteBuildTask.id, |
There was a problem hiding this comment.
siteBuildTaskId doesn't need to be added here now that it's been added to the serializer/json/include
| const [customerRules, setCustomerRules] = useState(sbtCustomRules); | ||
| const willBeSuppressed = customerRules.find(r => r.id === ruleId); | ||
|
|
||
| useEffect(() => { | ||
| setCustomerRules(sbtCustomRules); | ||
| }, [sbtCustomRules]); |
There was a problem hiding this comment.
I'm caveating this comment by saying we should probably investigate swapping in a consistent querying library like react-query because I've also lost the plot a little on hook re-rending logic. But I think this might be the block that is causing you issues: I don't know that you need a useEffect call because sbtCustomRules are used to initialize the local state already. And they shouldn't change since we aren't refetching the report while on this page.
There was a problem hiding this comment.
So i deleted the useEffect, but I am not sure that the set state for customer rules is working... it seems to issue at the same time as the update function, so I just passed in the new array as a param instead. Is that bad? I feel like it needs a promise somewhere, but I haven't seen a pattern like that in how we manage updates
sknep
force-pushed
the
feat-add-asr-rule-from-report
branch
from
ab8b159
to
786430f
Compare
sknep
force-pushed
the
feat-add-asr-rule-from-report
branch
from
786430f
to
3a29282
Compare
Changes proposed in this pull request:
Adds report suppression information to each finding
Allows the user to add or delete rules using the given rule id for each finding.
Cleans up use of "scanType" now that we have sbt, which has an id and a type
security considerations
none