-
Notifications
You must be signed in to change notification settings - Fork 50
SAT-37395 - Don't make API call for cveColumn if non-iop #1108
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
Conversation
Reviewer's GuideThis PR integrates the useAdvisorEngineConfig hook to conditionally enable CVE API calls only for IoP-enabled hosts and adds corresponding tests to verify behavior in IoP, non-IoP, and pending scenarios. Sequence diagram for conditional CVE API call based on IoP statussequenceDiagram
participant "CVECountCell"
participant "useAdvisorEngineConfig()"
participant "useAPI()"
participant "API Server"
"CVECountCell"->>"useAdvisorEngineConfig()": Check IoP status
"useAdvisorEngineConfig()"-->>"CVECountCell": Return isIopEnabled
alt IoP enabled and uuid exists
"CVECountCell"->>"useAPI()": Request CVE count (GET systems?uuid=...)
"useAPI()"->>"API Server": Make API call
"API Server"-->>"useAPI()": Return CVE data
"useAPI()"-->>"CVECountCell": Provide CVE data
else Not IoP enabled or uuid missing
"CVECountCell"-->>"CVECountCell": Render UnknownIcon (no API call)
end
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 there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `webpack/InsightsVulnerabilityHostIndexExtensions/__tests__/CVECountCell.test.js:36-38` </location>
<code_context>
+ jest.clearAllMocks();
+ });
+
+ it('renders an empty cves count column when no subscription UUID', () => {
const hostDetailsMock = {
name: 'test-host.example.com',
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for when the subscription_facet_attributes object itself is missing.
Testing for missing subscription_facet_attributes will ensure the code handles incomplete hostDetails objects correctly.
```suggestion
it('renders an empty cves count column when no subscription UUID', () => {
const hostDetailsMock = {
name: 'test-host.example.com',
subscription_facet_attributes: {
render(<CVECountCell hostDetails={hostDetailsMock} />);
expect(screen.getByRole('img', { hidden: true })).toBeTruthy();
});
it('renders an empty cves count column when subscription_facet_attributes is missing', () => {
const hostDetailsMock = {
name: 'test-host.example.com'
// subscription_facet_attributes is intentionally missing
};
render(<CVECountCell hostDetails={hostDetailsMock} />);
expect(screen.getByRole('img', { hidden: true })).toBeTruthy();
});
```
</issue_to_address>
### Comment 2
<location> `webpack/InsightsVulnerabilityHostIndexExtensions/__tests__/CVECountCell.test.js:55-52` </location>
<code_context>
+ expect(ConfigHooks.useAdvisorEngineConfig).toHaveBeenCalled();
+ });
+
+ it('renders UnknownIcon when IoP is undefined (API call pending)', () => {
+ ConfigHooks.useAdvisorEngineConfig.mockReturnValue(undefined);
+
+ const hostDetailsMock = {
+ name: 'test-host.example.com',
+ subscription_facet_attributes: {
+ uuid: 'test-uuid-123',
+ },
+ };
+
+ render(<CVECountCell hostDetails={hostDetailsMock} />);
+ expect(screen.getByRole('img', { hidden: true })).toBeTruthy();
+ expect(ConfigHooks.useAdvisorEngineConfig).toHaveBeenCalled();
+ });
});
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for when IoP is true but the API call returns an error.
Adding a test for IoP being true with a valid uuid, but the API call failing, will verify the component's error handling in this scenario.
Suggested implementation:
```javascript
expect(ConfigHooks.useAdvisorEngineConfig).toHaveBeenCalled();
});
it('renders error handling UI when IoP is true and API call fails', () => {
// Simulate IoP true with valid uuid
const hostDetailsMock = {
name: 'test-host.example.com',
subscription_facet_attributes: {
uuid: 'test-uuid-123',
},
};
// Simulate API call failure
ConfigHooks.useAdvisorEngineConfig.mockImplementation(() => {
throw new Error('API call failed');
});
render(<CVECountCell hostDetails={hostDetailsMock} />);
// Adjust the selector below to match your error UI (icon, message, etc.)
expect(screen.getByTestId('cve-count-error-icon')).toBeInTheDocument();
expect(ConfigHooks.useAdvisorEngineConfig).toHaveBeenCalled();
});
```
- Ensure that your `CVECountCell` component renders an element with `data-testid="cve-count-error-icon"` (or similar) when an error occurs. If your error UI uses a different selector, update the test accordingly.
- If your error handling is not based on throwing errors but on returning an error object, adjust the mock and assertions to match your implementation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
webpack/InsightsVulnerabilityHostIndexExtensions/__tests__/CVECountCell.test.js
Show resolved
Hide resolved
f4d2397 to
d5c7710
Compare
webpack/InsightsVulnerabilityHostIndexExtensions/__tests__/CVECountCell.test.js
Outdated
Show resolved
Hide resolved
cc2475c to
b95d397
Compare
webpack/InsightsVulnerabilityHostIndexExtensions/__tests__/CVECountCell.test.js
Outdated
Show resolved
Hide resolved
d82cd05 to
65f691e
Compare
webpack/InsightsVulnerabilityHostIndexExtensions/CVECountCell.js
Outdated
Show resolved
Hide resolved
fe1e716 to
9e0027e
Compare
lfu
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 and verified for both IoP and non-IoP 👍
What are the changes introduced in this pull request?
Considerations taken when implementing this change?
Register foreman_rh_cloud app metadata #1107
Fixes #38799 - Allow ForemanContext app_metadata to be extended from plugins foreman#10713
What are the testing steps for this pull request?
Summary by Sourcery
Gate CVECountCell API calls on IoP enablement by using the useAdvisorEngineConfig hook and update tests to cover IoP-enabled, IoP-disabled, and API pending scenarios
Enhancements:
Tests: