Conditionally hide UI components and enable inventory sync in IOP mode #1088
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR enables inventory synchronization in IOP mode by removing the previous skip logic and updates the UI to conditionally hide cloud-specific components (settings, descriptions, and connector button) when IOP mode is active, accompanied by a comprehensive overhaul of related tests to use renderWithStore, mock IOP configurations, and remove outdated snapshots. Sequence diagram for conditional rendering of ToolbarButtons in IOP modesequenceDiagram
participant PageHeader
participant ToolbarButtons
participant useIopConfig
participant CloudConnectorButton
participant SyncButton
PageHeader->>ToolbarButtons: Render ToolbarButtons
ToolbarButtons->>useIopConfig: Check IOP mode
alt IOP mode enabled
ToolbarButtons-->>CloudConnectorButton: Do not render
ToolbarButtons->>SyncButton: Render
else IOP mode disabled
ToolbarButtons->>CloudConnectorButton: Render
ToolbarButtons->>SyncButton: Render
end
Class diagram for InventoryHostsSync logic changeclassDiagram
class InventoryHostsSync {
+plan(organizations)
}
InventoryHostsSync --|> QueryInventoryJob
class ForemanRhCloud {
+with_iop_smart_proxy?()
}
%% Previously: plan() would skip if ForemanRhCloud.with_iop_smart_proxy? was true
%% Now: plan() always runs, regardless of IOP mode
Class diagram for conditional UI components in PageHeaderclassDiagram
class PageHeader {
+useIopConfig()
+render()
}
class InventorySettings
class PageDescription
class ToolbarButtons
PageHeader o-- ToolbarButtons
PageHeader o-- InventorySettings
PageHeader o-- PageDescription
%% InventorySettings and PageDescription only rendered if useIopConfig() is false
%% ToolbarButtons always rendered
Class diagram for ToolbarButtons conditional renderingclassDiagram
class ToolbarButtons {
+useIopConfig()
+render()
}
class CloudConnectorButton
class SyncButton
ToolbarButtons o-- CloudConnectorButton
ToolbarButtons o-- SyncButton
%% CloudConnectorButton only rendered if useIopConfig() is false
%% SyncButton always rendered
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
jeremylenz
reviewed
Sep 11, 2025
nofaralfasi
force-pushed
the
SAT-36556
branch
from
3dffe83 to
5a96f57
Compare
ShimShtein
reviewed
Sep 15, 2025
Member
ShimShtein
left a comment
ShimShtein
left a comment
There was a problem hiding this comment.
A couple of small suggestions, but overall looks good.
webpack/ForemanInventoryUpload/Components/PageHeader/__tests__/PageHeader.test.js
Outdated
Show resolved
Hide resolved
webpack/ForemanInventoryUpload/Components/PageHeader/__tests__/PageHeader.test.js
Outdated
Show resolved
Hide resolved
nofaralfasi
force-pushed
the
SAT-36556
branch
4 times, most recently
from
2cc7ef5 to
296fbbd
Compare
jeremylenz
approved these changes
Sep 17, 2025
Member
|
I have just merged theforeman/foreman#10654, using those new helpers we can stop mocking the import { rtlHelpers } from '../../../common/rtlTestHelpers';
const { renderWithStore } = rtlHelpers;
test('hides inventory settings and description when in IoP mode', () => {
renderWithStore(
<PageHeader />,
{
ADVISOR_ENGINE_CONFIG: {
response: { use_iop_mode: true },
}
);
// Core components should still be present
expect(screen.getByTestId('settings-warning')).toBeTruthy();
expect(screen.getByTestId('page-title')).toBeTruthy();
expect(screen.getByTestId('inventory-filter')).toBeTruthy();
expect(screen.getByTestId('toolbar-buttons')).toBeTruthy();
// These components should be hidden in IoP mode
expect(screen.queryByTestId('inventory-settings')).toBeNull();
expect(screen.queryByTestId('page-description')).toBeNull();
});I won't block the PR on it, but I think it will create more technical debt. |
nofaralfasi
force-pushed
the
SAT-36556
branch
2 times, most recently
from
4769707 to
af3e24f
Compare
ShimShtein
reviewed
Sep 30, 2025
...ForemanInventoryUpload/Components/PageHeader/__tests__/__snapshots__/PageHeader.test.js.snap
Outdated
Show resolved
Hide resolved
webpack/ForemanInventoryUpload/Components/PageHeader/__tests__/PageHeader.test.js
Show resolved
Hide resolved
nofaralfasi
force-pushed
the
SAT-36556
branch
from
af3e24f to
7cd6256
Compare
ShimShtein
reviewed
Oct 19, 2025
webpack/ForemanInventoryUpload/Components/PageHeader/__tests__/PageHeader.test.js
Outdated
Show resolved
Hide resolved
...ntoryUpload/Components/PageHeader/components/ToolbarButtons/__tests__/ToolbarButtons.test.js
Show resolved
Hide resolved
Collaborator
Author
@ShimShtein I tried the approach you suggested, but unfortunately, it didn’t work as expected. I also tried nesting it under the API key, but with that setup, the tests are still failing, so it doesn’t seem to help either. |
Member
|
You need to recreate the structure of the existing API redux tree: |
Collaborator
|
Maybe we should look at adding something like Katello's renderWithRedux: https://github.com/jeremylenz/katello/blob/b886d85ae80876855f7da333a77441354626c916/webpack/test-utils/react-testing-lib-wrapper.js#L23 |
Collaborator
Author
@ShimShtein I tried it with this code, but I’m still seeing errors for components that should be hidden: |
Member
|
@nofaralfasi I ran the tests with on my env and they pass. What error do you see? |
Collaborator
Author
|
jeremylenz
reviewed
Nov 3, 2025
webpack/ForemanInventoryUpload/Components/PageHeader/__tests__/PageHeader.test.js
Outdated
Show resolved
Hide resolved
nofaralfasi
force-pushed
the
SAT-36556
branch
2 times, most recently
from
f31853b to
32ef189
Compare
nofaralfasi
changed the title
nofaralfasi
force-pushed
the
SAT-36556
branch
from
32ef189 to
d3cafcd
Compare
nofaralfasi
force-pushed
the
SAT-36556
branch
from
d3cafcd to
fd51ec2
Compare
jeremylenz
reviewed
Nov 17, 2025
...ack/ForemanInventoryUpload/Components/PageHeader/components/ToolbarButtons/ToolbarButtons.js
Outdated
Show resolved
Hide resolved
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The IOP mode mocking setup is duplicated across several test files—consider extracting the
useIopConfigmock andrenderWithStoresetup into a shared test utility to reduce boilerplate. - Since multiple components (PageHeader, ToolbarButtons) use the same
useIopConfigcheck, you might extract that conditional into a small HOC or helper to keep UI logic DRY. - In the CVECountCell tests you only cover the UnknownIcon path—adding a positive case where the API returns a real count would validate that the component renders the expected number.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The IOP mode mocking setup is duplicated across several test files—consider extracting the `useIopConfig` mock and `renderWithStore` setup into a shared test utility to reduce boilerplate.
- Since multiple components (PageHeader, ToolbarButtons) use the same `useIopConfig` check, you might extract that conditional into a small HOC or helper to keep UI logic DRY.
- In the CVECountCell tests you only cover the UnknownIcon path—adding a positive case where the API returns a real count would validate that the component renders the expected number.
## Individual Comments
### Comment 1
<location> `lib/inventory_sync/async/inventory_hosts_sync.rb:11-12` </location>
<code_context>
set_callback :step, :around, :create_missing_hosts
def plan(organizations)
- # Do not run for local advisor, since we use sub-man id to identify hosts.
- return if ForemanRhCloud.with_iop_smart_proxy?
# by default the tasks will be executed concurrently
super(organizations)
</code_context>
<issue_to_address>
**issue (bug_risk):** Removing the early return for IOP mode may affect host sync logic.
Please verify that running the job in local advisor mode does not result in duplicate host records or other unintended effects, particularly regarding sub-man id usage.
</issue_to_address>
### Comment 2
<location> `webpack/ForemanInventoryUpload/Components/PageHeader/__tests__/PageHeader.test.js:70-79` </location>
<code_context>
+ test('hides inventory settings and description when in IoP mode', () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for unexpected or invalid API responses.
Tests should also cover scenarios where ADVISOR_ENGINE_CONFIG or INVENTORY_SETTINGS API responses are missing, malformed, or contain unexpected values to verify error handling and UI stability.
</issue_to_address>
### Comment 3
<location> `webpack/ForemanInventoryUpload/Components/PageHeader/components/ToolbarButtons/__tests__/ToolbarButtons.test.js:61-67` </location>
<code_context>
+ expect(screen.getByTestId('sync-button')).toBeTruthy();
+ });
+
+ test('renders nothing when subscription connection is not enabled', () => {
+ useIopConfig.mockReturnValue(false);
+ selectSubscriptionConnectionEnabled.mockReturnValue(false);
+
+ const { container } = renderWithStore(<ToolbarButtons />);
+
+ expect(container.firstChild).toBeNull();
+ });
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for error and loading states of subscription connection.
Please add tests for cases where the selector returns undefined, null, or the API status is 'ERROR' or 'PENDING' to verify correct component behavior in these scenarios.
Suggested implementation:
```javascript
test('renders only sync button when in IOP mode', () => {
useIopConfig.mockReturnValue(true);
selectSubscriptionConnectionEnabled.mockReturnValue(true);
renderWithStore(<ToolbarButtons />);
expect(screen.queryByTestId('cloud-connector-button')).toBeNull();
expect(screen.getByTestId('sync-button')).toBeTruthy();
});
test('renders nothing when subscription connection is not enabled', () => {
useIopConfig.mockReturnValue(false);
selectSubscriptionConnectionEnabled.mockReturnValue(false);
const { container } = renderWithStore(<ToolbarButtons />);
expect(container.firstChild).toBeNull();
});
test('renders nothing when subscription connection enabled selector returns undefined', () => {
useIopConfig.mockReturnValue(false);
selectSubscriptionConnectionEnabled.mockReturnValue(undefined);
const { container } = renderWithStore(<ToolbarButtons />);
expect(container.firstChild).toBeNull();
});
test('renders nothing when subscription connection enabled selector returns null', () => {
useIopConfig.mockReturnValue(false);
selectSubscriptionConnectionEnabled.mockReturnValue(null);
const { container } = renderWithStore(<ToolbarButtons />);
expect(container.firstChild).toBeNull();
});
test('renders loading indicator when API status is PENDING', () => {
useIopConfig.mockReturnValue(false);
selectSubscriptionConnectionEnabled.mockReturnValue(true);
// Mock API status selector
jest.spyOn(require('../selectors'), 'selectSubscriptionConnectionStatus').mockReturnValue('PENDING');
renderWithStore(<ToolbarButtons />);
expect(screen.getByTestId('subscription-loading')).toBeTruthy();
});
test('renders error message when API status is ERROR', () => {
useIopConfig.mockReturnValue(false);
selectSubscriptionConnectionEnabled.mockReturnValue(true);
// Mock API status selector
jest.spyOn(require('../selectors'), 'selectSubscriptionConnectionStatus').mockReturnValue('ERROR');
renderWithStore(<ToolbarButtons />);
expect(screen.getByTestId('subscription-error')).toBeTruthy();
});
```
- Ensure that your `ToolbarButtons` component renders elements with `data-testid="subscription-loading"` and `data-testid="subscription-error"` for the loading and error states, respectively.
- If the selectors are imported differently, adjust the `jest.spyOn` path accordingly.
- If your component handles loading/error states differently, update the test assertions to match the actual rendered output.
</issue_to_address>
### Comment 4
<location> `webpack/InsightsVulnerabilityHostIndexExtensions/__tests__/CVECountCell.test.js:80-57` </location>
<code_context>
+ it('renders UnknownIcon when IoP is enabled but CVE API call fails', () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for successful CVE API response in IoP mode.
Please add a test to verify correct rendering when the CVE API call succeeds and returns a valid count in IoP mode.
</issue_to_address>
### Comment 5
<location> `webpack/InsightsCloudSync/Components/InsightsTable/__tests__/InsightsTable.test.js:15-16` </location>
<code_context>
+ jest.clearAllMocks();
+ });
+
+ it('renders without crashing', () => {
+ renderWithStore(<InsightsTable {...tableProps} />);
+ });
});
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding assertions for table content and edge cases.
Add assertions to check correct data rendering, empty data handling, and proper behavior during loading and error states.
Suggested implementation:
```javascript
it('renders without crashing', () => {
renderWithStore(<InsightsTable {...tableProps} />);
});
it('renders table rows with correct data', () => {
const { getByText } = renderWithStore(<InsightsTable {...tableProps} />);
tableProps.data.forEach(row => {
expect(getByText(row.name)).toBeInTheDocument();
// Add more assertions for other columns as needed
});
});
it('renders empty state when data is empty', () => {
const emptyProps = { ...tableProps, data: [] };
const { getByText } = renderWithStore(<InsightsTable {...emptyProps} />);
expect(getByText(/no data/i)).toBeInTheDocument();
});
it('shows loading indicator when loading', () => {
const loadingProps = { ...tableProps, loading: true };
const { getByTestId } = renderWithStore(<InsightsTable {...loadingProps} />);
expect(getByTestId('loading-indicator')).toBeInTheDocument();
});
it('shows error message when error occurs', () => {
const errorProps = { ...tableProps, error: 'Something went wrong' };
const { getByText } = renderWithStore(<InsightsTable {...errorProps} />);
expect(getByText(/something went wrong/i)).toBeInTheDocument();
});
```
- Ensure that your `InsightsTable` component renders a loading indicator with `data-testid="loading-indicator"` and an empty state message containing "no data" (case-insensitive) when appropriate.
- Adjust the column assertions in the "renders table rows with correct data" test to match your actual data structure and column names.
- If your error message or empty state uses different text, update the test accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
nofaralfasi
force-pushed
the
SAT-36556
branch
from
fd51ec2 to
4230cf3
Compare
nofaralfasi
changed the title
- Enable inventory synchronization in local advisor engine environments - Hide cloud-specific UI elements when use_iop_mode is true - Conditionally display settings based on iop mode - Remove APIHooks.js and update tests accordingly This allows inventory sync to run in IOP mode while hiding cloud-specific UI components that are not relevant in local advisor engine environments.
nofaralfasi
force-pushed
the
SAT-36556
branch
from
4230cf3 to
ebb2f26
Compare
jeremylenz
approved these changes
Nov 24, 2025
Collaborator
jeremylenz
left a comment
jeremylenz
left a comment
There was a problem hiding this comment.
LGTM, thanks @nofaralfasi!
Will wait a bit to merge in case any others have additional comments.
Collaborator
|
Merging. thanks @nofaralfasi ! |
Odilhao
pushed a commit
to Odilhao/foreman_rh_cloud
that referenced
this pull request
Jan 14, 2026
theforeman#1088) - Enable inventory synchronization in local advisor engine environments - Hide cloud-specific UI elements when use_iop_mode is true - Conditionally display settings based on iop mode - Remove APIHooks.js and update tests accordingly This allows inventory sync to run in IOP mode while hiding cloud-specific UI components that are not relevant in local advisor engine environments. (cherry picked from commit 269654a)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What are the changes introduced in this pull request?
use_iop_modeis true.These changes allow inventory synchronization to run in IOP mode while hiding cloud-specific UI components that are not relevant in local Advisor Engine environments.
Considerations taken when implementing this change?
What are the testing steps for this pull request?
In the top section of the page:
should be hidden.
ToDo: Consider whether the "Connectivity test" option in the main kebab menu should also be hidden in IOP mode.
Summary by Sourcery
Enable inventory synchronization in IOP mode and conditionally hide cloud-specific UI components using a new useIopConfig hook, and update component tests to use React Testing Library
New Features:
Enhancements:
Tests:
Chores: