-
-
Notifications
You must be signed in to change notification settings - Fork 699
New IgnoreSuppressedFindings feature #5489
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
base: master
Are you sure you want to change the base?
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesFootnotes
|
|
@nscuro Please consider this PR in release 4.13.6 . |
…d alerts + tests. Signed-off-by: ElenaStroebele <elena.stroebele@rohde-schwarz.com>
stohrendorf
left a comment
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.
My review wasn't that deep, especially since I'm not familiar with the areas these changes interact with. You mentioned using AI - that's fine IMHO to get a basic coverage, but please make sure you understand the changes, especially in areas you're not familiar with.
src/main/java/org/dependencytrack/notification/vo/NewPolicyViolationsSummary.java
Show resolved
Hide resolved
src/main/java/org/dependencytrack/notification/vo/NewVulnerabilitiesSummary.java
Show resolved
Hide resolved
src/main/java/org/dependencytrack/notification/vo/NewVulnerabilitiesSummary.java
Show resolved
Hide resolved
src/main/java/org/dependencytrack/tasks/ScheduledNotificationDispatchTask.java
Outdated
Show resolved
Hide resolved
src/test/java/org/dependencytrack/notification/publisher/AbstractPublisherTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/dependencytrack/notification/publisher/AbstractPublisherTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/dependencytrack/notification/publisher/AbstractPublisherTest.java
Outdated
Show resolved
Hide resolved
I appreciate your review and am happy to include most of the suggestions you made. Nevertheless, I want to clarify that I am using AI to code faster and more efficiently, but not to let the AI do my job. Since you reviewed my implementation, I think you can agree that the implementation is logical, correct and in line with the existing codebase. |
Signed-off-by: ElenaStroebele <elena.stroebele@rohde-schwarz.com>
To be clear, I am using AI myself to code at times, so I'm not against it per se. All I wanted to say is: be careful and check its outputs closely. But this should not turn into a discussion about AI (although I believe you used AI to augment/refine your response, which is totally fine). After all, these changes look fine now. But I'm not a maintainer, I'm not familiar with these areas, so a maintainer should take a look at these changes now, it seems to be ready to stand the true test ;) |
Description
Added a new toggle in the scheduled alert creation menu called "Ignore suppressed findings". Included the implementation and the related test(s). The logic in NotificationRule and ScheduledNotificationDispatchTask follows the pattern of the existing "Skip publish if unchanged" toggle, adapted for the different behavior. The option is propagated via the JSON for NewVulnerabilitySummary/NewPolicyViolationSummary and subsequently handled within the e-mail template, etc. The UI toggle is provided in a separate PR in the frontend repository.
Also introduced a slightly different e-mail template when suppressed findings are ignored.
Added a new test to verify the correct template is used, another one that checks whether the DispatchTask works correctly and updated the docs to reflect the new functionality.
Addressed Issue
Addresses issue #5488.
Additional Details
Used GitHub Copilot and ChatGPT to understand the codebase and debug, and to suggest approaches that fit the existing patterns.
Checklist