Fixes #38903 - rewrite class-based React components as functions with hooks

[nofaralfasi]

What are the changes introduced in this pull request?

This pull request converts class-based React components to functional components using hooks for the following pages:

• Red Hat Repositories page
• Subscriptions page

Considerations taken when implementing this change?

This modernization improves code maintainability, readability, and alignment with current React best practices.

What are the testing steps for this pull request?

• Ensure all automated tests pass.
• Verify that both the Red Hat Repositories and Subscriptions pages function as expected in the UI.

Summary by Sourcery

Convert class-based React components to functional components with hooks for the SubscriptionsPage and RedHatRepositoriesPage, update lifecycle handling, and adjust related tests and snapshots.

Enhancements:

• Rewrite SubscriptionsPage as a functional component using useState, useEffect, useCallback, and useRef to manage lifecycle and state.
• Rewrite RedHatRepositoriesPage as a functional component using useEffect for data loading.
• Refactor internal handlers, permission checks, and table interactions to use hook-based patterns.

Tests:

• Update SubscriptionsPage tests to use mount, async act, and additional mocks for hook-based components.
• Refresh snapshots for ContentDetails, SubscriptionDetails, and SubscriptionsPage tests.

[sourcery-ai]

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

• SubscriptionsPage has grown quite complex—consider extracting related useEffect logic (e.g. data loading, task handling, org changes) into custom hooks to improve readability and reduce potential stale-ref issues.
• Double-check the prevPropsRef usage in your hooks (for subscriptionsResults, isTaskPending, organization) to ensure you’re tracking the right previous values and avoiding mismatches or missing dependencies.
• There’s quite a bit of inline utility logic (getEnabledColumns, toolTip handlers, getDisabledReason)—you might move these into memoized callbacks or separate helper modules to keep the main component lean.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- SubscriptionsPage has grown quite complex—consider extracting related useEffect logic (e.g. data loading, task handling, org changes) into custom hooks to improve readability and reduce potential stale-ref issues.
- Double-check the prevPropsRef usage in your hooks (for subscriptionsResults, isTaskPending, organization) to ensure you’re tracking the right previous values and avoiding mismatches or missing dependencies.
- There’s quite a bit of inline utility logic (getEnabledColumns, toolTip handlers, getDisabledReason)—you might move these into memoized callbacks or separate helper modules to keep the main component lean.

## Individual Comments

### Comment 1
<location> `webpack/scenes/Subscriptions/SubscriptionsPage.js:144-145` </location>
<code_context>
+  useEffect(() => {
+    const prevProps = prevPropsRef.current;
+
     if (hasUpstreamConnection) {
-      const subscriptionsChanged = subscriptions.results !== prevProps.subscriptions.results;
-      if (subscriptionsChanged || !this.state.availableQuantitiesLoaded) {
+      const subscriptionsChanged = subscriptions.results !== prevProps.subscriptionsResults;
+      if (subscriptionsChanged || !availableQuantitiesLoaded) {
         const poolIds = filterRHSubscriptions(subscriptions.results).map(subs => subs.id);
</code_context>

<issue_to_address>
**suggestion (bug_risk):** Comparing subscriptions.results by reference may miss actual data changes.

If subscriptions.results is recreated on each update, this comparison will always evaluate as changed, leading to unnecessary calls to loadAvailableQuantities. Use deep equality or compare unique identifiers to prevent redundant effect executions.

Suggested implementation:

```javascript
    if (hasUpstreamConnection) {
      const currentSubscriptionIds = (subscriptions.results || []).map(subs => subs.id).sort();
      const prevSubscriptionIds = (prevProps.subscriptionsResults || []).map(subs => subs.id).sort();
      const subscriptionsChanged = JSON.stringify(currentSubscriptionIds) !== JSON.stringify(prevSubscriptionIds);
      if (subscriptionsChanged || !availableQuantitiesLoaded) {
        const poolIds = filterRHSubscriptions(subscriptions.results).map(subs => subs.id);
        if (poolIds.length > 0) {
          loadAvailableQuantities({ poolIds });
          setAvailableQuantitiesLoaded(true);
        }
      }
    }

```

```javascript
    prevPropsRef.current = { 
      ...prevPropsRef.current, 
      organization,
      subscriptionsResults: subscriptions.results // update stored previous results for next comparison
    };

```
</issue_to_address>

### Comment 2
<location> `webpack/scenes/Subscriptions/SubscriptionsPage.js:212-214` </location>
<code_context>
-    };
+    return enabledColumns;
+  };
+  const toolTipOnclose = (columns) => {
+    const enabledColumns = getEnabledColumns(columns);
+    loadTableColumns({ columns: enabledColumns });

-    const onDeleteSubscriptions = (selectedRows) => {
-      this.props.deleteSubscriptions(selectedRows);
-      this.handleSelectedRowsChange([]);
-      closeDeleteModal();
+    if (isEmpty(subscriptionTableSettings)) {
+      createColumns({ name: SUBSCRIPTION_TABLE_NAME, columns: enabledColumns });
+    } else {
</code_context>

<issue_to_address>
**suggestion:** toolTipOnclose mutates options before passing to updateColumns.

Create a new columns object instead of mutating options.columns to prevent unintended side effects if subscriptionTableSettings is used elsewhere.

```suggestion
      const updatedOptions = { ...subscriptionTableSettings, columns: enabledColumns };
      updateColumns(updatedOptions);
```
</issue_to_address>

### Comment 3
<location> `webpack/scenes/Subscriptions/__tests__/SubscriptionsPage.test.js:135-141` </location>
<code_context>
   });

   it('should poll tasks when org changes', async () => {
-    page.setProps({ organization: { id: 1 } });
+    await act(async () => {
+      page.setProps({ organization: { id: 1 } });
+    });
+    page.update();

     expect(pollTasks).toHaveBeenCalled();
   });

</code_context>

<issue_to_address>
**suggestion (testing):** Test for polling tasks on org change is good, but missing assertion for other side effects.

Please add assertions to confirm that loadSubscriptions, loadTables, and loadTableColumns are called when the organization changes.

Suggested implementation:

```javascript
   it('should poll tasks when org changes', async () => {
     await act(async () => {
       page.setProps({ organization: { id: 1 } });
     });
     page.update();

     expect(pollTasks).toHaveBeenCalled();
     expect(loadSubscriptions).toHaveBeenCalled();
     expect(loadTables).toHaveBeenCalled();
     expect(loadTableColumns).toHaveBeenCalled();
   });

```

If `loadSubscriptions`, `loadTables`, or `loadTableColumns` are not already mocked and tracked by Jest, you will need to add Jest mocks for them at the top of your test file, similar to how `pollTasks` is handled. For example:

```js
jest.mock('path/to/loadSubscriptions');
jest.mock('path/to/loadTables');
jest.mock('path/to/loadTableColumns');
```
Adjust the import paths as needed for your codebase.
</issue_to_address>

### Comment 4
<location> `webpack/scenes/Subscriptions/__tests__/SubscriptionsPage.test.js:144-152` </location>
<code_context>
     expect(pollTasks).toHaveBeenCalled();
   });

   it('should not poll tasks if org has not changed', async () => {
-    page.setProps({ simpleContentAccess: true });
+    pollTasks.mockClear(); // Clear calls from mount
+    
+    await act(async () => {
+      page.setProps({ simpleContentAccess: true });
+    });
+    page.update();

     expect(pollTasks).not.toHaveBeenCalled();
   });
</code_context>

<issue_to_address>
**suggestion (testing):** Test for not polling tasks is present, but could be more robust.

Also verify that functions like loadSubscriptions and loadTables are not called when the organization remains unchanged.

Suggested implementation:

```javascript
   it('should not poll tasks if org has not changed', async () => {
     pollTasks.mockClear(); // Clear calls from mount
     loadSubscriptions.mockClear();
     loadTables.mockClear();

     await act(async () => {
       page.setProps({ simpleContentAccess: true });
     });
     page.update();

     expect(pollTasks).not.toHaveBeenCalled();
     expect(loadSubscriptions).not.toHaveBeenCalled();
     expect(loadTables).not.toHaveBeenCalled();
   });

```

Make sure that `loadSubscriptions` and `loadTables` are properly mocked in your test setup (e.g., using `jest.mock()` or similar). If they are not already mocked, you will need to add those mocks at the top of your test file.
</issue_to_address>

### Comment 5
<location> `webpack/scenes/Subscriptions/__tests__/SubscriptionsPage.test.js:127-128` </location>
<code_context>
+    if (permissionDeniedPage) permissionDeniedPage.unmount();
+  });

   it('should render', async () => {
     expect(toJson(page)).toMatchSnapshot();
</code_context>

<issue_to_address>
**suggestion (testing):** Snapshot test is present, but could be improved with more granular assertions.

Add assertions for specific UI elements and states, such as permission denied, loading, and empty states, to improve test reliability.

Suggested implementation:

```javascript
  it('should render', async () => {
    expect(toJson(page)).toMatchSnapshot();
  });

  it('should show permission denied state', () => {
    permissionDeniedPage = mount(
      <SubscriptionsPage
        organization={null}
        permissionDenied={true}
        loading={false}
        tasks={[]}
        enableDeleteButton={noop}
      />
    );
    expect(permissionDeniedPage.find('.permission-denied').exists()).toBe(true);
    expect(permissionDeniedPage.find('.permission-denied').text()).toContain('Permission Denied');
  });

  it('should show loading state', () => {
    const loadingPage = mount(
      <SubscriptionsPage
        organization={{ id: 1 }}
        permissionDenied={false}
        loading={true}
        tasks={[]}
        enableDeleteButton={noop}
      />
    );
    expect(loadingPage.find('.loading-indicator').exists()).toBe(true);
    expect(loadingPage.find('.loading-indicator').text()).toContain('Loading');
    loadingPage.unmount();
  });

  it('should show empty state when no tasks', () => {
    const emptyPage = mount(
      <SubscriptionsPage
        organization={{ id: 1 }}
        permissionDenied={false}
        loading={false}
        tasks={[]}
        enableDeleteButton={noop}
      />
    );
    expect(emptyPage.find('.empty-state').exists()).toBe(true);
    expect(emptyPage.find('.empty-state').text()).toContain('No subscriptions found');
    emptyPage.unmount();
  });

```

- Ensure that your `SubscriptionsPage` component renders elements with the class names `.permission-denied`, `.loading-indicator`, and `.empty-state` for these tests to work.
- Adjust the props and expected text as needed to match your actual implementation.
</issue_to_address>

### Comment 6
<location> `webpack/scenes/Subscriptions/SubscriptionsPage.js:127-135` </location>
<code_context>
    if (organization) {
      if (!prevProps.organization || prevProps.organization.id !== organization.id) {
        loadData();
        if (isManifestImported) {
          pingUpstreamSubscriptions();
          setAvailableQuantitiesLoaded(false);
        }
      }
    }

</code_context>

<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/merge-nested-ifs))

```suggestion
    if (organization && (!prevProps.organization || prevProps.organization.id !== organization.id)) {
          loadData();
          if (isManifestImported) {
            pingUpstreamSubscriptions();
            setAvailableQuantitiesLoaded(false);
          }
    }

```

<br/><details><summary>Explanation</summary>Reading deeply nested conditional code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</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.}

[jeremylenz]

2nd commit needs a Redmine issue ("Refs #38903"), or just squash it :)

[nofaralfasi]

2nd commit needs a Redmine issue ("Refs #38903"), or just squash it :)

I wasn't sure if we want to keep the second commit (if the tests I added are needed).

[jeremylenz]

I wasn't sure if we want to keep the second commit (if the tests I added are needed).

I would say keep them. Replacing snapshots with RTL is always great :)

[jeremylenz]

It seems the tests here are not React Testing Library tests; rather, they use Enzyme mount which is a pattern I don't think we use elsewhere. Can you please look at existing Katello RTL tests and model the tests here after those?

[nofaralfasi]

It seems the tests here are not React Testing Library tests; rather, they use Enzyme mount which is a pattern I don't think we use elsewhere. Can you please look at existing Katello RTL tests and model the tests here after those?

I used Enzyme mount because I saw it in existing tests like SubscriptionsTable.test.js and withOrganization.test.js, so I assumed it was still acceptable. I agree that RTL is better, though.
I had attempted an RTL conversion before but encountered test failures. I've now converted it following the RTL patterns from other tests in the codebase.

[jeremylenz]

Works well and is a huge improvement over what we had before! Thanks @nofaralfasi

ACK 👍