From a5aa15cbcb711c10d472cf348eca4ed41e2554c4 Mon Sep 17 00:00:00 2001 From: Michal Gold Date: Mon, 28 Jul 2025 11:40:26 +0300 Subject: [PATCH] Wizard: Resolve row reordering issue on selection and expansion - Fix issue when clicking the expandable arrow or selecting a package checkbox in the Packages step it caused unexpected row reordering. - Updated sorting logic to ensure that selecting a package with a specific stream groups all related module streams together at the top. - Ensured that rows expand in place and selection does not affect row position. - Add unit test as well --- .../steps/Packages/Packages.tsx | 129 +++++++++--------- .../steps/Packages/Packages.test.tsx | 117 ++++++++++++++++ src/test/fixtures/packages.ts | 58 ++++++++ 3 files changed, 239 insertions(+), 65 deletions(-) diff --git a/src/Components/CreateImageWizard/steps/Packages/Packages.tsx b/src/Components/CreateImageWizard/steps/Packages/Packages.tsx index f067d16c..8106fd4b 100644 --- a/src/Components/CreateImageWizard/steps/Packages/Packages.tsx +++ b/src/Components/CreateImageWizard/steps/Packages/Packages.tsx @@ -50,6 +50,7 @@ import { Thead, Tr, } from '@patternfly/react-table'; +import { orderBy } from 'lodash'; import { useDispatch } from 'react-redux'; import CustomHelperText from './components/CustomHelperText'; @@ -66,7 +67,6 @@ import { } from '../../../../constants'; import { useGetArchitecturesQuery } from '../../../../store/backendApi'; import { - ApiPackageSourcesResponse, ApiRepositoryResponseRead, ApiSearchRpmResponse, useCreateRepositoryMutation, @@ -700,7 +700,7 @@ const Packages = () => { ); } - const unpackedData: IBPackageWithRepositoryInfo[] = + let unpackedData: IBPackageWithRepositoryInfo[] = combinedPackageData.flatMap((item) => { // Spread modules into separate rows by application stream if (item.sources) { @@ -724,13 +724,16 @@ const Packages = () => { }); // group by name, but sort by application stream in descending order - unpackedData.sort((a, b) => { - if (a.name === b.name) { - return (b.stream ?? '').localeCompare(a.stream ?? ''); - } else { - return a.name.localeCompare(b.name); - } - }); + unpackedData = orderBy( + unpackedData, + [ + 'name', + (pkg) => pkg.stream || '', + (pkg) => pkg.repository || '', + (pkg) => pkg.module_name || '', + ], + ['asc', 'desc', 'asc', 'asc'], + ); if (toggleSelected === 'toggle-available') { if (activeTabKey === Repos.INCLUDED) { @@ -866,8 +869,6 @@ const Packages = () => { dispatch(addPackage(pkg)); if (pkg.type === 'module') { setActiveStream(pkg.stream || ''); - setActiveSortIndex(2); - setPage(1); dispatch( addModule({ name: pkg.module_name || '', @@ -993,7 +994,18 @@ const Packages = () => { } }; - const initialExpandedPkgs: IBPackageWithRepositoryInfo[] = []; + const getPackageUniqueKey = (pkg: IBPackageWithRepositoryInfo): string => { + try { + if (!pkg || !pkg.name) { + return `invalid_${Date.now()}`; + } + return `${pkg.name}_${pkg.stream || 'none'}_${pkg.module_name || 'none'}_${pkg.repository || 'unknown'}`; + } catch { + return `error_${Date.now()}`; + } + }; + + const initialExpandedPkgs: string[] = []; const [expandedPkgs, setExpandedPkgs] = useState(initialExpandedPkgs); const setPkgExpanded = ( @@ -1001,12 +1013,13 @@ const Packages = () => { isExpanding: boolean, ) => setExpandedPkgs((prevExpanded) => { - const otherExpandedPkgs = prevExpanded.filter((p) => p.name !== pkg.name); - return isExpanding ? [...otherExpandedPkgs, pkg] : otherExpandedPkgs; + const pkgKey = getPackageUniqueKey(pkg); + const otherExpandedPkgs = prevExpanded.filter((key) => key !== pkgKey); + return isExpanding ? [...otherExpandedPkgs, pkgKey] : otherExpandedPkgs; }); const isPkgExpanded = (pkg: IBPackageWithRepositoryInfo) => - expandedPkgs.includes(pkg); + expandedPkgs.includes(getPackageUniqueKey(pkg)); const initialExpandedGroups: GroupWithRepositoryInfo['name'][] = []; const [expandedGroups, setExpandedGroups] = useState(initialExpandedGroups); @@ -1030,51 +1043,37 @@ const Packages = () => { 'asc' | 'desc' >('asc'); - const getSortableRowValues = ( - pkg: IBPackageWithRepositoryInfo, - ): (string | number | ApiPackageSourcesResponse[] | undefined)[] => { - return [pkg.name, pkg.summary, pkg.stream, pkg.end_date, pkg.repository]; - }; + const sortedPackages = useMemo(() => { + if (!transformedPackages || !Array.isArray(transformedPackages)) { + return []; + } - let sortedPackages = transformedPackages; - sortedPackages = transformedPackages.sort((a, b) => { - const aValue = getSortableRowValues(a)[activeSortIndex]; - const bValue = getSortableRowValues(b)[activeSortIndex]; - if (typeof aValue === 'number') { - // Numeric sort - if (activeSortDirection === 'asc') { - return (aValue as number) - (bValue as number); - } - return (bValue as number) - (aValue as number); - } - // String sort - // if active stream is set, sort it to the top - if (aValue === activeStream) { - return -1; - } - if (bValue === activeStream) { - return 1; - } - if (activeSortDirection === 'asc') { - // handle packages with undefined stream - if (!aValue) { - return -1; - } - if (!bValue) { - return 1; - } - return (aValue as string).localeCompare(bValue as string); - } else { - // handle packages with undefined stream - if (!aValue) { - return 1; - } - if (!bValue) { - return -1; - } - return (bValue as string).localeCompare(aValue as string); - } - }); + return orderBy( + transformedPackages, + [ + // Active stream packages first (if activeStream is set) + (pkg) => (activeStream && pkg.stream === activeStream ? 0 : 1), + // Then by name + 'name', + // Then by stream version (descending) + (pkg) => { + if (!pkg.stream) return ''; + const parts = pkg.stream + .split('.') + .map((part) => parseInt(part, 10) || 0); + // Convert to string with zero-padding for proper sorting + return parts.map((p) => p.toString().padStart(10, '0')).join('.'); + }, + // Then by end date (nulls last) + (pkg) => pkg.end_date || '9999-12-31', + // Then by repository + (pkg) => pkg.repository || '', + // Finally by module name + (pkg) => pkg.module_name || '', + ], + ['asc', 'asc', 'desc', 'asc', 'asc', 'asc'], + ); + }, [transformedPackages, activeStream]); const getSortParams = (columnIndex: number) => ({ sortBy: { @@ -1100,14 +1099,14 @@ const Packages = () => { (module) => module.name === pkg.name, ); isSelected = - packages.some((p) => p.name === pkg.name) && !isModuleWithSameName; + packages.some((p) => p.name === pkg.name && p.stream === pkg.stream) && + !isModuleWithSameName; } if (pkg.type === 'module') { - // the package is selected if it's added to the packages state - // and its module stream matches one in enabled_modules + // the package is selected if its module stream matches one in enabled_modules isSelected = - packages.some((p) => p.name === pkg.name) && + packages.some((p) => p.name === pkg.name && p.stream === pkg.stream) && modules.some( (m) => m.name === pkg.module_name && m.stream === pkg.stream, ); @@ -1208,7 +1207,7 @@ const Packages = () => { .slice(computeStart(), computeEnd()) .map((grp, rowIndex) => ( @@ -1308,7 +1307,7 @@ const Packages = () => { .slice(computeStart(), computeEnd()) .map((pkg, rowIndex) => ( diff --git a/src/test/Components/CreateImageWizard/steps/Packages/Packages.test.tsx b/src/test/Components/CreateImageWizard/steps/Packages/Packages.test.tsx index 85a2a12e..42fecc29 100644 --- a/src/test/Components/CreateImageWizard/steps/Packages/Packages.test.tsx +++ b/src/test/Components/CreateImageWizard/steps/Packages/Packages.test.tsx @@ -513,6 +513,123 @@ describe('Step Packages', () => { expect(secondAppStreamRow).toBeDisabled(); expect(secondAppStreamRow).not.toBeChecked(); }); + + test('module selection sorts selected stream to top while maintaining alphabetical order', async () => { + const user = userEvent.setup(); + + await renderCreateMode(); + await goToPackagesStep(); + await typeIntoSearchBox('sortingTest'); + + await screen.findAllByText('alphaModule'); + await screen.findAllByText('betaModule'); + await screen.findAllByText('gammaModule'); + + let rows = await screen.findAllByRole('row'); + rows.shift(); + expect(rows).toHaveLength(6); + + expect(rows[0]).toHaveTextContent('alphaModule'); + expect(rows[0]).toHaveTextContent('3.0'); + expect(rows[1]).toHaveTextContent('alphaModule'); + expect(rows[1]).toHaveTextContent('2.0'); + expect(rows[2]).toHaveTextContent('betaModule'); + expect(rows[2]).toHaveTextContent('4.0'); + expect(rows[3]).toHaveTextContent('betaModule'); + expect(rows[3]).toHaveTextContent('2.0'); + + // Select betaModule with stream 2.0 (row index 3) + const betaModule20Checkbox = await screen.findByRole('checkbox', { + name: /select row 3/i, + }); + + await waitFor(() => user.click(betaModule20Checkbox)); + expect(betaModule20Checkbox).toBeChecked(); + + // After selection, the active stream (2.0) should be prioritized + // All modules with stream 2.0 should move to the top, maintaining alphabetical order + rows = await screen.findAllByRole('row'); + rows.shift(); + expect(rows[0]).toHaveTextContent('alphaModule'); + expect(rows[0]).toHaveTextContent('2.0'); + expect(rows[1]).toHaveTextContent('betaModule'); + expect(rows[1]).toHaveTextContent('2.0'); + expect(rows[2]).toHaveTextContent('gammaModule'); + expect(rows[2]).toHaveTextContent('2.0'); + expect(rows[3]).toHaveTextContent('alphaModule'); + expect(rows[3]).toHaveTextContent('3.0'); + expect(rows[4]).toHaveTextContent('betaModule'); + expect(rows[4]).toHaveTextContent('4.0'); + expect(rows[5]).toHaveTextContent('gammaModule'); + expect(rows[5]).toHaveTextContent('1.5'); + + // Verify that only the selected module is checked + const updatedBetaModule20Checkbox = await screen.findByRole('checkbox', { + name: /select row 1/i, // betaModule 2.0 is now at position 1 + }); + expect(updatedBetaModule20Checkbox).toBeChecked(); + + // Verify that only one checkbox is checked + const allCheckboxes = await screen.findAllByRole('checkbox', { + name: /select row [0-9]/i, + }); + const checkedCheckboxes = allCheckboxes.filter( + (cb) => (cb as HTMLInputElement).checked, + ); + expect(checkedCheckboxes).toHaveLength(1); + expect(checkedCheckboxes[0]).toBe(updatedBetaModule20Checkbox); + }); + + test('unselecting a module does not cause jumping but may reset sort to default', async () => { + const user = userEvent.setup(); + + await renderCreateMode(); + await goToPackagesStep(); + await selectCustomRepo(); + await typeIntoSearchBox('sortingTest'); + await screen.findAllByText('betaModule'); + const betaModule20Checkbox = await screen.findByRole('checkbox', { + name: /select row 3/i, + }); + await waitFor(() => user.click(betaModule20Checkbox)); + expect(betaModule20Checkbox).toBeChecked(); + let rows = await screen.findAllByRole('row'); + rows.shift(); + expect(rows[0]).toHaveTextContent('alphaModule'); + expect(rows[0]).toHaveTextContent('2.0'); + expect(rows[1]).toHaveTextContent('betaModule'); + expect(rows[1]).toHaveTextContent('2.0'); + + const updatedBetaModule20Checkbox = await screen.findByRole('checkbox', { + name: /select row 1/i, + }); + await waitFor(() => user.click(updatedBetaModule20Checkbox)); + expect(updatedBetaModule20Checkbox).not.toBeChecked(); + + // After unselection, the sort may reset to default or stay the same + // The important thing is that we don't get jumping/reordering during the interaction + rows = await screen.findAllByRole('row'); + rows.shift(); // Remove header row + const allCheckboxes = await screen.findAllByRole('checkbox', { + name: /select row [0-9]/i, + }); + const checkedCheckboxes = allCheckboxes.filter( + (cb) => (cb as HTMLInputElement).checked, + ); + expect(checkedCheckboxes).toHaveLength(0); + + // The key test: the table should have a consistent, predictable order + // Either the original alphabetical order OR the stream-sorted order + // What we don't want is jumping around during the selection/unselection process + expect(rows).toHaveLength(6); // Still have all 6 modules + const moduleNames = rows.map((row) => { + const match = row.textContent?.match(/(\w+Module)/); + return match ? match[1] : ''; + }); + expect(moduleNames).toContain('alphaModule'); + expect(moduleNames).toContain('betaModule'); + expect(moduleNames).toContain('gammaModule'); + }); }); }); diff --git a/src/test/fixtures/packages.ts b/src/test/fixtures/packages.ts index 0f494415..08aefb20 100644 --- a/src/test/fixtures/packages.ts +++ b/src/test/fixtures/packages.ts @@ -75,6 +75,64 @@ export const mockSourcesPackagesResults = ( }, ]; } + if (search === 'sortingTest') { + return [ + { + package_name: 'alphaModule', + summary: 'Alpha module for sorting tests', + package_sources: [ + { + name: 'alphaModule', + type: 'module', + stream: '2.0', + end_date: '2025-12-01', + }, + { + name: 'alphaModule', + type: 'module', + stream: '3.0', + end_date: '2027-12-01', + }, + ], + }, + { + package_name: 'betaModule', + summary: 'Beta module for sorting tests', + package_sources: [ + { + name: 'betaModule', + type: 'module', + stream: '2.0', + end_date: '2025-06-01', + }, + { + name: 'betaModule', + type: 'module', + stream: '4.0', + end_date: '2028-06-01', + }, + ], + }, + { + package_name: 'gammaModule', + summary: 'Gamma module for sorting tests', + package_sources: [ + { + name: 'gammaModule', + type: 'module', + stream: '2.0', + end_date: '2025-08-01', + }, + { + name: 'gammaModule', + type: 'module', + stream: '1.5', + end_date: '2026-08-01', + }, + ], + }, + ]; + } if (search === 'mock') { return [ {