Fixes #38829 - Hosts bulk action: Change host collections

[lfu]

What are the changes introduced in this pull request?

Considerations taken when implementing this change?

Requires theforeman/foreman#10721

What are the testing steps for this pull request?

Hosts can be added/removed from host collections.

Summary by Sourcery

Implement a new bulk change host collections feature for the hosts index, including a modal UI trigger, Redux API actions for adding/removing collections, and comprehensive unit tests.

New Features:

• Add a Bulk Change Host Collections modal to the hosts actions bar for bulk addition or removal of hosts from selected host collections
• Introduce a menu item trigger and global fills to integrate the bulk change host collections modal into the hosts bulk actions menu
• Implement Redux actions and API integration for bulkAddHostCollections and bulkRemoveHostCollections

Tests:

• Add unit tests for the Bulk Change Host Collections modal covering rendering, table data display, save button states, radio toggling, cancel behavior, and conditional data fetching when closed

[sourcery-ai]

Hey there - I've reviewed your changes - here's some feedback:

• BulkChangeHostCollectionsModal is very large and mixes data fetching, selection logic, and UI; consider extracting the table fetching/selection into a custom hook or smaller components to improve readability.
• There are a few inline styles (e.g. marginLeft on the Radio and width on the Modal) — it would be better to use PatternFly spacing/utilities or CSS classes for consistent styling.
• The useEffect that syncs HCIds disables the exhaustive-deps rule; try including all necessary dependencies (like selectedResults) or computing HCIds directly to avoid the eslint-disable comment.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- BulkChangeHostCollectionsModal is very large and mixes data fetching, selection logic, and UI; consider extracting the table fetching/selection into a custom hook or smaller components to improve readability.
- There are a few inline styles (e.g. marginLeft on the Radio and width on the Modal) — it would be better to use PatternFly spacing/utilities or CSS classes for consistent styling.
- The useEffect that syncs HCIds disables the exhaustive-deps rule; try including all necessary dependencies (like selectedResults) or computing HCIds directly to avoid the eslint-disable comment.

## Individual Comments

### Comment 1
<location> `webpack/components/extensions/Hosts/BulkActions/BulkChangeHostCollectionsModal/BulkChangeHostCollectionsModal.js:102-108` </location>
<code_context>
+  const pageStats = getPageStats({ total: subtotal, page, perPage });
+
+  // Update HCIds when selections change
+  useEffect(() => {
+    if (isOpen) {
+      const selectedIds = selectedResults.map(result => result.id);
+      setHCIds(selectedIds);
+    }
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [selectedCount, isOpen]);
+
</code_context>

<issue_to_address>
**suggestion (bug_risk):** Review dependency array for useEffect to ensure correct updates.

selectedResults is used inside useEffect but not included in its dependency array, which may cause HCIds to become outdated if selectedResults changes independently of selectedCount. Add selectedResults to the dependency array to prevent stale updates.

```suggestion
  useEffect(() => {
    if (isOpen) {
      const selectedIds = selectedResults.map(result => result.id);
      setHCIds(selectedIds);
    }
    // eslint-disable-next-line react-hooks/exhaustive-deps
  }, [selectedCount, isOpen, selectedResults]);
```
</issue_to_address>

### Comment 2
<location> `webpack/components/extensions/Hosts/BulkActions/BulkChangeHostCollectionsModal/index.js:37` </location>
<code_context>
+      key="change-host-collections-dropdown-item"
+      onClick={setModalOpen}
+      isDisabled={selectedCount === 0 || !orgId}
+      description={!orgId && <DisabledMenuItemDescription disabledReason={__('To manage host collections, a specific organization must be selected from the organization context.')} />}
+    >
+      {__('Host collections')}
</code_context>

<issue_to_address>
**suggestion:** Consider providing a tooltip for disabled menu items for better accessibility.

Adding a tooltip or aria-label would improve accessibility for users who rely on screen readers or cannot see the description.

Suggested implementation:

```javascript
  const disabledReason = __('To manage host collections, a specific organization must be selected from the organization context.');

  return (
    <Tooltip
      content={!orgId ? disabledReason : null}
      isDisabled={orgId}
      position="top"
    >
      <MenuItem
        itemId="change-host-collections-dropdown-item"
        key="change-host-collections-dropdown-item"
        onClick={setModalOpen}
        isDisabled={selectedCount === 0 || !orgId}
        description={!orgId && <DisabledMenuItemDescription disabledReason={disabledReason} />}
        aria-label={!orgId ? disabledReason : undefined}
      >
        {__('Host collections')}
      </MenuItem>
    </Tooltip>
  );

```

Make sure that the `Tooltip` component is imported from your UI library (e.g., `@patternfly/react-core`). If not already imported, add:
```js
import { Tooltip } from '@patternfly/react-core';
```
at the top of the file.
</issue_to_address>

### Comment 3
<location> `webpack/components/extensions/Hosts/BulkActions/BulkChangeHostCollectionsModal/__tests__/BulkChangeHostCollectionsModal.test.js:156-86` </location>
<code_context>
+  assertNockRequest(autocompleteScope, false);
+});
+
+test('Save button is enabled when host collection is selected', async () => {
+  const autocompleteScope = nockInstance
+    .get(autocompleteUrl)
+    .query(true)
+    .reply(200, [])
+    .persist();
+
+  const hostCollectionsScope = nockInstance
+    .get(hostCollectionsApiUrl)
+    .query(true)
+    .reply(200, mockHostCollections)
+    .persist();
+
+  const { getAllByRole, getByText } = renderWithRedux(
+    <BulkChangeHostCollectionsModal {...defaultProps} />,
+    renderOptions(),
+  );
+
+  await patientlyWaitFor(() => {
+    expect(getByText('Test Host Collection 1')).toBeInTheDocument();
+  });
</code_context>

<issue_to_address>
**suggestion (testing):** Consider adding tests for error and success toast notifications.

Tests should simulate API error and success responses and assert that the appropriate toast notifications are displayed to ensure user feedback is properly handled.

Suggested implementation:

```javascript
import { toast } from 'react-toastify';

jest.mock('react-toastify', () => ({
  toast: {
    success: jest.fn(),
    error: jest.fn(),
  },
}));

beforeEach(() => {
  jest.clearAllMocks();
});

```

```javascript
});

// Test for success toast notification
test('shows success toast notification on successful save', async () => {
  const autocompleteScope = nockInstance
    .get(autocompleteUrl)
    .query(true)
    .reply(200, [])
    .persist();

  const hostCollectionsScope = nockInstance
    .get(hostCollectionsApiUrl)
    .query(true)
    .reply(200, mockHostCollections)
    .persist();

  const saveScope = nockInstance
    .post(/bulk_host_collections/)
    .reply(200, { success: true });

  const { getAllByRole, getByText } = renderWithRedux(
    <BulkChangeHostCollectionsModal {...defaultProps} />,
    renderOptions(),
  );

  await patientlyWaitFor(() => {
    expect(getByText('Test Host Collection 1')).toBeInTheDocument();
  });

  // Simulate selecting a host collection
  const hostCollectionItem = getByText('Test Host Collection 1');
  hostCollectionItem.click();

  // Save button should be enabled now
  const saveButton = getAllByRole('button', { name: 'Save' })[0];
  expect(saveButton).toHaveAttribute('aria-disabled', 'false');

  // Click Save
  saveButton.click();

  await patientlyWaitFor(() => {
    expect(toast.success).toHaveBeenCalledWith(expect.stringContaining('success'), expect.anything());
  });

  assertNockRequest(saveScope, false);
  assertNockRequest(hostCollectionsScope, false);
  assertNockRequest(autocompleteScope, false);
});

// Test for error toast notification
test('shows error toast notification on failed save', async () => {
  const autocompleteScope = nockInstance
    .get(autocompleteUrl)
    .query(true)
    .reply(200, [])
    .persist();

  const hostCollectionsScope = nockInstance
    .get(hostCollectionsApiUrl)
    .query(true)
    .reply(200, mockHostCollections)
    .persist();

  const saveScope = nockInstance
    .post(/bulk_host_collections/)
    .reply(500, { message: 'Internal Server Error' });

  const { getAllByRole, getByText } = renderWithRedux(
    <BulkChangeHostCollectionsModal {...defaultProps} />,
    renderOptions(),
  );

  await patientlyWaitFor(() => {
    expect(getByText('Test Host Collection 1')).toBeInTheDocument();
  });

  // Simulate selecting a host collection
  const hostCollectionItem = getByText('Test Host Collection 1');
  hostCollectionItem.click();

  // Save button should be enabled now
  const saveButton = getAllByRole('button', { name: 'Save' })[0];
  expect(saveButton).toHaveAttribute('aria-disabled', 'false');

  // Click Save
  saveButton.click();

  await patientlyWaitFor(() => {
    expect(toast.error).toHaveBeenCalledWith(expect.stringContaining('error'), expect.anything());
  });

  assertNockRequest(saveScope, false);
  assertNockRequest(hostCollectionsScope, false);
  assertNockRequest(autocompleteScope, false);
});

```

- If your toast notification implementation uses a different API or library, adjust the mock and assertions accordingly.
- Ensure that the BulkChangeHostCollectionsModal component actually calls `toast.success` and `toast.error` on success and error, respectively.
- If the Save button is not enabled by clicking the host collection item, you may need to simulate the selection differently.
</issue_to_address>

### Comment 4
<location> `webpack/components/extensions/Hosts/BulkActions/BulkChangeHostCollectionsModal/__tests__/BulkChangeHostCollectionsModal.test.js:128-137` </location>
<code_context>
+  assertNockRequest(autocompleteScope, false);
+});
+
+test('Save button is disabled when no host collections selected', async () => {
+  const autocompleteScope = nockInstance
+    .get(autocompleteUrl)
+    .query(true)
+    .reply(200, [])
+    .persist();
+
+  const hostCollectionsScope = nockInstance
+    .get(hostCollectionsApiUrl)
+    .query(true)
+    .reply(200, mockHostCollections)
+    .persist();
+
+  const { getAllByRole } = renderWithRedux(
+    <BulkChangeHostCollectionsModal {...defaultProps} />,
+    renderOptions(),
+  );
+
+  await patientlyWaitFor(() => {
+    const saveButton = getAllByRole('button', { name: 'Save' })[0];
+    expect(saveButton).toBeInTheDocument();
+    expect(saveButton).toHaveAttribute('aria-disabled', 'true');
+  });
+
</code_context>

<issue_to_address>
**suggestion (testing):** Consider testing the loading state of the Save button.

Please add a test that simulates the loading state (e.g., while fetching host collections) to verify the Save button is disabled and displays a loading indicator if applicable.

Suggested implementation:

```javascript
test('Save button is disabled when no host collections selected', async () => {
  const autocompleteScope = nockInstance
    .get(autocompleteUrl)
    .query(true)
    .reply(200, [])
    .persist();

  const hostCollectionsScope = nockInstance
    .get(hostCollectionsApiUrl)
    .query(true)
    .reply(200, mockHostCollections)
    .persist();

  const { getAllByRole } = renderWithRedux(
    <BulkChangeHostCollectionsModal {...defaultProps} />,
    renderOptions(),
  );

  await patientlyWaitFor(() => {
    const saveButton = getAllByRole('button', { name: 'Save' })[0];
    expect(saveButton).toBeInTheDocument();
    expect(saveButton).toHaveAttribute('aria-disabled', 'true');
  });
});

// Test loading state of Save button
test('Save button is disabled and shows loading indicator while fetching host collections', async () => {
  // Simulate a delayed response for host collections
  const autocompleteScope = nockInstance
    .get(autocompleteUrl)
    .query(true)
    .reply(200, [])
    .persist();

  const hostCollectionsScope = nockInstance
    .get(hostCollectionsApiUrl)
    .query(true)
    .delay(1000) // simulate loading
    .reply(200, mockHostCollections)
    .persist();

  const { getAllByRole, getByText } = renderWithRedux(
    <BulkChangeHostCollectionsModal {...defaultProps} />,
    renderOptions(),
  );

  // Immediately after render, before host collections resolve
  const saveButton = getAllByRole('button', { name: 'Save' })[0];
  expect(saveButton).toBeInTheDocument();
  expect(saveButton).toHaveAttribute('aria-disabled', 'true');

  // Check for loading indicator (adjust selector as needed for your implementation)
  // Example: spinner, loading text, etc.
  // If your Save button shows a spinner or "Loading..." text, check for it:
  // expect(getByText(/loading/i)).toBeInTheDocument();
  // Or if you use a spinner with a test id:
  // expect(saveButton.querySelector('[data-testid="loading-spinner"]')).toBeInTheDocument();

  // Wait for host collections to load
  await patientlyWaitFor(() => {
    expect(saveButton).toBeInTheDocument();
    // After loading, Save button should still be disabled if no selection
    expect(saveButton).toHaveAttribute('aria-disabled', 'true');
  });

  assertNockRequest(hostCollectionsScope, false);
  assertNockRequest(autocompleteScope, false);
});

```

- If your Save button uses a specific loading indicator (spinner, "Loading..." text, etc.), adjust the selector in the test accordingly.
- If the loading indicator is not present, you may need to update the BulkChangeHostCollectionsModal component to add a testable loading indicator for the Save button.
</issue_to_address>

### Comment 5
<location> `webpack/components/extensions/Hosts/BulkActions/BulkChangeHostCollectionsModal/__tests__/BulkChangeHostCollectionsModal.test.js:194-203` </location>
<code_context>
+test('switches between Add and Remove radio buttons', async () => {
</code_context>

<issue_to_address>
**suggestion (testing):** Consider adding a test for the actual API call made when Save is clicked for both Add and Remove actions.

Currently, there are no tests confirming that the correct API endpoint is called for each action. Adding these tests will verify the backend is triggered as expected.
</issue_to_address>

### Comment 6
<location> `webpack/components/extensions/Hosts/BulkActions/BulkChangeHostCollectionsModal/__tests__/BulkChangeHostCollectionsModal.test.js:237-246` </location>
<code_context>
+test('closes modal and clears selections on Cancel', async () => {
</code_context>

<issue_to_address>
**suggestion (testing):** Consider verifying that selections and radio state are reset after closing the modal.

Add assertions to confirm that selected host collections and radio button state are reset after closing, ensuring the modal is ready for reuse.

Suggested implementation:

```javascript
test('closes modal and clears selections on Cancel', async () => {
  const autocompleteScope = nockInstance
    .get(autocompleteUrl)
    .query(true)
    .reply(200, [])
    .persist();

  const hostCollectionsScope = nockInstance
    .get(hostCollectionsApiUrl)
    .query(true)
    .reply(200, mockHostCollections)

  // ...existing setup and opening modal...

  // Simulate selecting host collections and changing radio button
  const addRadio = getByLabelText('Add to host collection(s)');
  const removeRadio = getByLabelText('Remove from host collection(s)');
  fireEvent.click(removeRadio);

  // Simulate selecting a host collection (assuming chips/tags are rendered with testid 'selected-host-collection')
  const hostCollectionOption = await findByText(mockHostCollections[0].name);
  fireEvent.click(hostCollectionOption);

  // Click Cancel to close modal
  const cancelButton = getByText('Cancel');
  fireEvent.click(cancelButton);

  // Assert modal is closed (assuming modal has role 'dialog')
  await waitFor(() => {
    expect(queryByRole('dialog')).not.toBeInTheDocument();
  });

  // Reopen modal to check reset state
  fireEvent.click(getByText('Bulk change host collections'));

  // Assert radio buttons are reset
  expect(getByLabelText('Add to host collection(s)')).toBeChecked();
  expect(getByLabelText('Remove from host collection(s)')).not.toBeChecked();

  // Assert selected host collections are cleared (no chips/tags)
  expect(queryByTestId('selected-host-collection')).toBeNull();

```

- You may need to adjust the testid or query selector for selected host collections to match your implementation (e.g., 'selected-host-collection', 'chip', etc.).
- Ensure that the modal can be reopened in the test (e.g., by clicking a button labeled 'Bulk change host collections').
- If your modal uses a different method for tracking selected host collections, update the assertion accordingly.
</issue_to_address>

### Comment 7
<location> `webpack/components/extensions/Hosts/BulkActions/BulkChangeHostCollectionsModal/BulkChangeHostCollectionsModal.js:49` </location>
<code_context>
  const replacementResponse = !isOpen ? { response: {} } : false;

</code_context>

<issue_to_address>
**suggestion (code-quality):** Invert ternary operator to remove negation ([`invert-ternary`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/invert-ternary))

```suggestion
  const replacementResponse = isOpen ? false : { response: {} };
```

<br/><details><summary>Explanation</summary>Negated conditions are more difficult to read than positive ones, so it is best
to avoid them where we can. By inverting the ternary condition and swapping the
expressions we can simplify the code.
</details>
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[lfu]

[LadislavVasina1]

I have started testing this PR with packit build and while removing multiple hosts from multiple host collections I noticed this alert message

Could we add new lines to the alert so its more readable please.

[lfu]

[LadislavVasina1]

Perfect, thanks @lfu

[jeremylenz]

Thanks @lfu !

Some initial observations

• If the limit is exceeded, can we make the collection not selectable to add?
• If there are no host collections in the system, we have a very nice flow on the host details page that guides you to the create host collection page. We should have a similar thing here, so I've opened https://projects.theforeman.org/issues/38843 for a followup.
• Can we add a Host Collections column in All Hosts Manage Columns? It's annoying to go to the details page to confirm that my changes worked. Can be here or a followup, doesn't matter to me

More comments below

[lfu]

Will add a Host Collections column in All Hosts Manage Columns in a separate PR.
Updated.

[lfu]

Tests passed locally with foreman change.

[lfu]

Added Host collections as a column: #11539

[jeremylenz]

Thanks @lfu, and also, uh, me!

ACK 👍