Fixes #38958 - Add basic Leapp preupgrade report table to the new job invocation page #165

kmalyjur · 2025-12-15T12:12:48Z

TO-DO: Fix tests

#38951 needs to be merged for this PR

Before:

After:

- Add PropTypes and defaultProps to mock components (Table, Pagination) - Fix React Hooks rules violation by moving hooks before early return - Fix camelcase linting for API properties (template_name, preupgrade_report_entries) - Simplify else-if structure to avoid lonely-if error - Fix consistent-return by explicitly returning undefined in early exit - Fix boolean attribute syntax (isEmbedded) - Remove trailing whitespace for prettier compliance 🤖 Generated with [Claude Code](https://claude.com/claude-code) Assisted-By: Claude <[email protected]>

adamlazik1

Haven't looked at the whole code yet but test failures are related and need to be addressed.

adamlazik1 · 2026-01-14T11:36:49Z

webpack/components/PreupgradeReportsTable/PreupgradeReportsTable.scss

+ }
+


Trailing whitespace here.

Suggested change

}

}

adamruzicka · 2026-01-14T12:48:15Z

webpack/components/PreupgradeReportsTable/__tests__/PreupgradeReportsTable.test.js

+jest.mock('foremanReact/components/Pagination', () => ({
+  __esModule: true,
+  default: ({ onChange, itemCount, page, perPage }) => (
+    <div data-testid="pagination">
+      <button onClick={() => onChange({ page: page + 1 })}>Next Page</button>
+      <button onClick={() => onChange({ perPage: 10 })}>Set 10</button>
+      <span>
+        {itemCount} items, page {page}, per page {perPage}
+      </span>
+    </div>
+  ),
+}));


Duplicating mocks ain't cool, Mr. C

Fixed multiple test issues: - Changed redux-thunk import from named to default export - Added proper mocks for I18n, Pagination, and Table components - Updated Table mock to handle childrenOutsideTbody and customEmptyState props - Updated snapshots for PreupgradeReportsList tests All 61 tests now passing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Generated-By: Claude <[email protected]>

MariaAga · 2026-01-15T10:51:30Z

webpack/__mocks__/foremanReact/components/Pagination.js

No need to mock foremanReact

No need to mock anything from foremanReact?

Right 🥳 since: https://community.theforeman.org/t/testing-plugins-from-foreman-core/43293

It seems I still need to mock ForemanContext to avoid this.

● PreupgradeReportsList › encountered a declaration exception TypeError: Cannot read properties of undefined (reading 'perPage') 20 | toggleSelectAll, 21 | }) => { > 22 | const { perPage, perPageOptions } = useForemanSettings(); | ^ 23 | const [pagination, setPagination] = useState({ 24 | page: 1, 25 | per_page: perPage, at PreupgradeReportsList (../foreman-leapp/webpack/components/PreupgradeReportsList/index.js:22:11) at ReactShallowRenderer.apply [as render] (node_modules/enzyme-adapter-react-16/node_modules/react-test-renderer/cjs/react-test-renderer-shallow.development.js:829:32) at renderElement (node_modules/enzyme-adapter-react-16/src/ReactSixteenAdapter.js:643:41) at fn (node_modules/enzyme-adapter-react-16/src/ReactSixteenAdapter.js:707:46) at withSetStateAllowed (node_modules/enzyme-adapter-utils/src/Utils.js:100:18) at Object.render (node_modules/enzyme-adapter-react-16/src/ReactSixteenAdapter.js:707:39) at new ShallowWrapper (node_modules/enzyme/src/ShallowWrapper.js:411:22) at shallow (node_modules/enzyme/src/shallow.js:10:10) at map (webpack/assets/javascripts/react_app/common/testHelpers.js:62:23) at Array.map (<anonymous>) at shallowRenderComponentWithFixtures (webpack/assets/javascripts/react_app/common/testHelpers.js:60:28) at testComponentSnapshotsWithFixtures (webpack/assets/javascripts/react_app/common/testHelpers.js:71:3) at Suite.<anonymous> (../foreman-leapp/webpack/components/PreupgradeReportsList/__tests__/PreupgradeReportsList.test.js:40:37) at Object.<anonymous> (../foreman-leapp/webpack/components/PreupgradeReportsList/__tests__/PreupgradeReportsList.test.js:39:1)

Or is there an alternative to this?

ok this specific mock is not correct in foreman core and should be fixed to include all items, so for this I would use a mock

theforeman/foreman#10823
Added a full mock in core

MariaAga · 2026-01-15T10:52:16Z

.../components/PreupgradeReportsList/__tests__/__snapshots__/PreupgradeReportsList.test.js.snap

Can the test be updated to not use snapshots?

MariaAga · 2026-01-15T12:39:28Z

webpack/components/PreupgradeReportsTable/index.js

+        customToolbarItems={[
+          <ToolbarItem key="pagination">{topPagination}</ToolbarItem>,
+        ]}
+        replacementResponse={combinedResponse}
+        searchable={false}
+        updateParamsByUrl={false}
+      >
+        <Table
+          ouiaId="leapp-report-table"
+          columns={columns}
+          isEmbedded
+          params={{
+            page: pagination.page,
+            per_page: pagination.per_page,
+            perPage: pagination.per_page,
+            order: '',
+          }}
+          results={pagedEntries}
+          itemCount={entries.length}
+          url=""
+          isPending={status === STATUS.PENDING}
+          errorMessage={
+            status === STATUS.ERROR && error?.message ? error.message : null
+          }
+          showCheckboxes={false}
+          refreshData={() => {}}
+          isDeleteable={false}
+          customEmptyState={
+            status === STATUS.RESOLVED && entries.length === 0
+              ? customEmptyState
+              : null
+          }
+          setParams={handleParamsChange}
+          bottomPagination={bottomPagination}


Why is the custom top and bottom Pagination needed? I tried to remove them and the table still works

I'll have to defer to @kmalyjur

MariaAga · 2026-01-15T12:49:40Z

webpack/components/PreupgradeReportsTable/index.js

+    setPagination(prev => ({
+      ...prev,
+      page: newParams.page || prev.page,
+      per_page: newParams.perPage || newParams.per_page || prev.per_page,


not sure why, but newParams.per_page is the correct one while newParams.perPage gives the old value

What I'm seeing is newParams.perPage always being undefined and newParams.per_page being correct. Thinking about it, I'm not sure where should newParams.perPage be coming from

All per_page, except newParams.per_page should be perPage.
so

webpack/components/PreupgradeReports/PreupgradeReportsHelpers.js export const entriesPage = (entries, pagination) => { const offset = (pagination.page - 1) * pagination.perPage; return entries.slice(offset, offset + pagination.perPage); };

webpack/components/PreupgradeReportsTable/index.js const [pagination, setPagination] = useState({ page: 1, perPage: 5 }); ... perPage: newParams.per_page || prev.perPage, ... params={{ page: pagination.page, perPage: pagination.perPage, order: '', }}

And bottomPagination={bottomPagination} is not needed

Replace snapshot-based testing with React Testing Library assertions that test actual component behavior: - Verify header columns render correctly - Test entry data (titles, hostnames) appears in the document - Check checkbox interactions call the correct handlers - Test selected state rendering - Verify pagination component is present Generated-By: Claude Code 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

MariaAga · 2026-01-15T15:03:19Z

webpack/components/PreupgradeReportsTable/PreupgradeReportsTableConstants.js

+export const columns = {
+  title: {
+    title: __('Title'),
+    wrapper: () => null,
+  },
+  host: {
+    title: __('Host'),
+    wrapper: () => null,
+  },
+  risk_factor: {
+    title: __('Risk Factor'),
+    wrapper: () => null,
+  },
+  has_remediation: {
+    title: __('Has Remediation?'),
+    wrapper: () => null,
+  },
+  inhibitor: {
+    title: __('Inhibitor?'),
+    wrapper: () => null,
+  },
+};
+


Instead of this, and the tbody, I think it would be better to do:

const columns = { title: { title: __('Title'), }, host: { title: __('Host'), wrapper: entry => entry.hostname || (reportData && reportData.hostname) || '-', }, risk_factor: { title: __('Risk Factor'), wrapper: ({ severity }) => renderSeverityLabel(severity), }, has_remediation: { title: __('Has Remediation?'), wrapper: entry => entry.detail && entry.detail.remediations ? __('Yes') : __('No'), }, inhibitor: { title: __('Inhibitor?'), wrapper: entry => entry.flags && entry.flags.some(flag => flag === 'inhibitor') ? ( <Tooltip content={__('This issue inhibits the upgrade.')}> <span>{__('Yes')}</span> </Tooltip> ) : ( __('No') ), }, };

Done, but I had to drop the (reportData && reportData.hostname) part from host as that is state within PreupgradeReportsTable and I wasn't sure how to access that, if that's even possible.

its possible to move conts columns into PreupgradeReportsTable

MariaAga · 2026-01-15T15:04:11Z

webpack/components/PreupgradeReportsTable/index.js

+          }
+          setParams={handleParamsChange}
+          bottomPagination={bottomPagination}
+          childrenOutsideTbody


This prop is for when the table has expendable rows

MariaAga · 2026-01-15T15:07:45Z

webpack/components/PreupgradeReportsTable/index.js

+      <TableIndexPage
+        apiUrl=""
+        controller="preupgrade_reports"
+        customToolbarItems={[
+          <ToolbarItem key="pagination">{topPagination}</ToolbarItem>,
+        ]}
+        replacementResponse={combinedResponse}
+        searchable={false}
+        updateParamsByUrl={false}
+      >


TableIndexPage is not needed

MariaAga · 2026-01-15T15:11:20Z

webpack/components/PreupgradeReportsTable/index.js

+  const customEmptyState = (
+    <Tr ouiaId="leapp-table-empty">
+      <Td colSpan={100}>
+        <EmptyState variant={EmptyStateVariant.sm}>
+          <EmptyStateHeader
+            titleText={<>{__('No issues found')}</>}
+            headingLevel="h5"
+          />
+          <EmptyStateBody>
+            {__('The preupgrade report shows no issues.')}
+          </EmptyStateBody>
+        </EmptyState>
+      </Td>
+    </Tr>
+  );


Can be just sent as a prop to the Table emptyMessage={__('The preupgrade report shows no issues.')}

- wrappers in columns instead of explicity tbody - no tableindexpage - no childrenOutsideTbody - no customEmptyState

- use perPage over per_page whereever possible - drop custom bottom pagination & friends

MariaAga · 2026-01-16T11:32:35Z

webpack/components/PreupgradeReportsTable/index.js

+import { Table } from 'foremanReact/components/PF4/TableIndexPage/Table/Table';
+import { APIActions } from 'foremanReact/redux/API';
+import { entriesPage } from '../PreupgradeReports/PreupgradeReportsHelpers';
+import { STATUS, renderSeverityLabel } from './PreupgradeReportsTableConstants';


STATUS should come from foreman core, and not re-defined here

MariaAga · 2026-01-16T11:35:53Z

webpack/components/PreupgradeReportsTable/PreupgradeReportsTable.scss

+ .leapp-report-section .table-title-section,
+ .leapp-report-section .table-toolbar-section {
+   padding-top: 0 !important;
+ }


This css is no longer needed as the css classes are not present anymore

MariaAga · 2026-01-16T11:36:11Z

webpack/components/PreupgradeReportsTable/PreupgradeReportsTableConstants.js

+export const STATUS = {
+  PENDING: 'PENDING',
+  RESOLVED: 'RESOLVED',
+  ERROR: 'ERROR',
+};


Should be imported from core and not redefined

MariaAga · 2026-01-16T11:36:21Z

webpack/components/PreupgradeReportsTable/PreupgradeReportsTableConstants.js

+export const STATUS = {
+  PENDING: 'PENDING',
+  RESOLVED: 'RESOLVED',
+  ERROR: 'ERROR',
+};


Should be imported from core and not redefined

- drop redefined STATUS constant - drop unused css - move rendering function out of constants

adamruzicka · 2026-01-16T13:58:01Z

webpack/components/PreupgradeReportsList/index.js

  const { perPage, perPageOptions } = useForemanSettings();
  const [pagination, setPagination] = useState({
    page: 1,
-    per_page: perPage,
+    perPage,
    perPageOptions,


perPageOptions seem to be non-standard (see theforeman/foreman#10823 (comment)), what if I dropped it?

it will probably be undefined and can be removed, also I dont see it being used as a value anywhere

Thank you for confirming, I just dropped it

Fixes #38958 - Add leapp preupgrade table to job invocation page

2baca5e

kmalyjur changed the title ~~Add basic Leapp preupgrade report table to the new job invocation page~~ Fixes #38958 - Add basic Leapp preupgrade report table to the new job invocation page Dec 15, 2025

kmalyjur added the breaks-robottelo Can be set via comment: /label breaks-robottelo label Dec 15, 2025

adamlazik1 reviewed Jan 14, 2026

View reviewed changes

adamruzicka closed this Jan 14, 2026

adamruzicka force-pushed the job-inv-leapp branch from 131319a to 80017aa Compare January 14, 2026 12:28

adamruzicka reopened this Jan 14, 2026

adamruzicka reviewed Jan 14, 2026

View reviewed changes

adamruzicka added 2 commits January 14, 2026 14:05

Drop trailing whitespace

ba5ec56

adamruzicka force-pushed the job-inv-leapp branch from e42f6c1 to ba5ec56 Compare January 14, 2026 13:05

MariaAga reviewed Jan 15, 2026

View reviewed changes

adamruzicka added 3 commits January 15, 2026 15:03

Drop all (but one) global foremanReact mocks

b419319

Drop some more mocks

4a97492

Drop even more mocks

e369a39

adamruzicka force-pushed the job-inv-leapp branch from 57bf4e2 to e369a39 Compare January 15, 2026 14:03

MariaAga reviewed Jan 15, 2026

View reviewed changes

adamruzicka added 2 commits January 15, 2026 17:28

Address review comments

7fbd625

- wrappers in columns instead of explicity tbody - no tableindexpage - no childrenOutsideTbody - no customEmptyState

Move columns back to the table itself

f0f99ec

adamruzicka force-pushed the job-inv-leapp branch from 1cf15c8 to c7c4653 Compare January 16, 2026 11:28

Pagination changes

2c260dd

- use perPage over per_page whereever possible - drop custom bottom pagination & friends

adamruzicka force-pushed the job-inv-leapp branch from c7c4653 to 2c260dd Compare January 16, 2026 11:29

MariaAga requested changes Jan 16, 2026

View reviewed changes

pr-processor bot added the Waiting on contributor label Jan 16, 2026

Address comments

6a684d4

- drop redefined STATUS constant - drop unused css - move rendering function out of constants

adamruzicka force-pushed the job-inv-leapp branch from 92b3a3b to 6a684d4 Compare January 16, 2026 12:48

adamruzicka reviewed Jan 16, 2026

View reviewed changes

Drop custom perPageOptions

b7fb96c

		}
		No newline at end of file

Fixes #38958 - Add basic Leapp preupgrade report table to the new job invocation page #165

Are you sure you want to change the base?

Fixes #38958 - Add basic Leapp preupgrade report table to the new job invocation page #165

Conversation

kmalyjur commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamlazik1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MariaAga Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MariaAga Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kmalyjur commented Dec 15, 2025 •

edited

Loading

MariaAga Jan 16, 2026 •

edited

Loading

MariaAga Jan 15, 2026 •

edited

Loading