-
Notifications
You must be signed in to change notification settings - Fork 50
Display 'Analysis disabled' in CVE column #1141
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
Display 'Analysis disabled' in CVE column #1141
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates the CVE count cell rendering to use a typographic dash and text label instead of an icon for unknown/disabled states, adds handling for hosts with analysis opt-out, normalizes API response attributes to camelCase, and adjusts the Total CVEs column weight in the hosts index table. Flow diagram for CVECountCell rendering logicflowchart TD
A[Start CVECountCell with hostDetails] --> B[Fetch cve_count and opt_out via API]
B --> C{isIopEnabled?
AND uuid defined?}
C -- No --> D[Render typographic dash]
C -- Yes --> E[Normalize attributes with propsToCamelCase]
E --> F{optOut is true?}
F -- Yes --> G[Render text Analysis disabled]
F -- No --> H{cveCount is undefined?}
H -- Yes --> D[Render typographic dash]
H -- No --> I[Render Link with cveCount]
I --> J[End]
D --> J
G --> J
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey - I've left some high level feedback:
- Returning a raw '—' string instead of the previous
<UnknownIcon />changes the cell’s visual and accessibility behavior; consider verifying that the table styling and screen reader output remain consistent with other cells and, if needed, centralizing this placeholder as a shared component or constant. - You’re now calling
propsToCamelCaseon the API response inside the render; if this component is rendered frequently, consider memoizing or restructuring so the transformation isn’t re-run on every render for the same data.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Returning a raw '—' string instead of the previous `<UnknownIcon />` changes the cell’s visual and accessibility behavior; consider verifying that the table styling and screen reader output remain consistent with other cells and, if needed, centralizing this placeholder as a shared component or constant.
- You’re now calling `propsToCamelCase` on the API response inside the render; if this component is rendered frequently, consider memoizing or restructuring so the transformation isn’t re-run on every render for the same data.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
8802ef5 to
e028c06
Compare
nofaralfasi
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.
Tested the code locally and it works as expected - hosts with vulnerability analysis disabled now correctly show "Analysis disabled" instead of the CVE count, and the em-dash replacement for unknown states is consistent with the rest of the UI.
One suggestion: consider adding a test case for the new optOut feature to ensure it's covered.
webpack/InsightsVulnerabilityHostIndexExtensions/__tests__/CVECountCell.test.js
Outdated
Show resolved
Hide resolved
e028c06 to
42ac4aa
Compare
|
@nofaralfasi updated 👍 |
@jeremylenz What do you think about adding tests for this? |
|
oops, I saw that and then forgot 😄 working on it! |
Adds two test cases to CVECountCell.test.js: - Renders "Analysis disabled" when opt_out is true - Renders CVE count link when valid count is returned Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
9f60914 to
f4bcd7a
Compare
|
@nofaralfasi test added. 👍 |
nofaralfasi
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.
LGTM. The test refactoring is a nice improvement over the previous approach, and the opt-out feature now has proper coverage.
* Display 'Analysis disabled' in CVE column * fix tests * Add test coverage for 'Analysis disabled' CVE column behavior Adds two test cases to CVECountCell.test.js: - Renders "Analysis disabled" when opt_out is true - Renders CVE count link when valid count is returned Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
What are the changes introduced in this pull request?
Hey, we've got some upgrades to the Total CVEs column for IoP-enabled systems!
Considerations taken when implementing this change?
everything is well considered!!
This code is ✨ artisan ✨ and no AI was used in its creation
What are the testing steps for this pull request?
Hosts > All Hosts > Manage Columns > add the Total CVEs column
Alternatively, you could figure out the proper curl command to send IoP a
[PATCH /api/vulnerability/v1/systems/opt_out](https://console.redhat.com/docs/api/vulnerability/v1#operations-default-manager\.system_handler\.PatchBulkSystemsOptOut\.patch)You should now see "Analysis disabled" for your host.
Additionally, you should see '—' and NOT a QuestionIcon for unregistered hosts.
You should continue to see a number of vulnerabilities (with link) for hosts with vulnerabilities.