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
This commit is contained in:
Michal Gold 2025-07-28 11:40:26 +03:00
parent 44c3674072
commit a5aa15cbcb
3 changed files with 239 additions and 65 deletions

View file

@ -50,6 +50,7 @@ import {
Thead, Thead,
Tr, Tr,
} from '@patternfly/react-table'; } from '@patternfly/react-table';
import { orderBy } from 'lodash';
import { useDispatch } from 'react-redux'; import { useDispatch } from 'react-redux';
import CustomHelperText from './components/CustomHelperText'; import CustomHelperText from './components/CustomHelperText';
@ -66,7 +67,6 @@ import {
} from '../../../../constants'; } from '../../../../constants';
import { useGetArchitecturesQuery } from '../../../../store/backendApi'; import { useGetArchitecturesQuery } from '../../../../store/backendApi';
import { import {
ApiPackageSourcesResponse,
ApiRepositoryResponseRead, ApiRepositoryResponseRead,
ApiSearchRpmResponse, ApiSearchRpmResponse,
useCreateRepositoryMutation, useCreateRepositoryMutation,
@ -700,7 +700,7 @@ const Packages = () => {
); );
} }
const unpackedData: IBPackageWithRepositoryInfo[] = let unpackedData: IBPackageWithRepositoryInfo[] =
combinedPackageData.flatMap((item) => { combinedPackageData.flatMap((item) => {
// Spread modules into separate rows by application stream // Spread modules into separate rows by application stream
if (item.sources) { if (item.sources) {
@ -724,13 +724,16 @@ const Packages = () => {
}); });
// group by name, but sort by application stream in descending order // group by name, but sort by application stream in descending order
unpackedData.sort((a, b) => { unpackedData = orderBy(
if (a.name === b.name) { unpackedData,
return (b.stream ?? '').localeCompare(a.stream ?? ''); [
} else { 'name',
return a.name.localeCompare(b.name); (pkg) => pkg.stream || '',
} (pkg) => pkg.repository || '',
}); (pkg) => pkg.module_name || '',
],
['asc', 'desc', 'asc', 'asc'],
);
if (toggleSelected === 'toggle-available') { if (toggleSelected === 'toggle-available') {
if (activeTabKey === Repos.INCLUDED) { if (activeTabKey === Repos.INCLUDED) {
@ -866,8 +869,6 @@ const Packages = () => {
dispatch(addPackage(pkg)); dispatch(addPackage(pkg));
if (pkg.type === 'module') { if (pkg.type === 'module') {
setActiveStream(pkg.stream || ''); setActiveStream(pkg.stream || '');
setActiveSortIndex(2);
setPage(1);
dispatch( dispatch(
addModule({ addModule({
name: pkg.module_name || '', 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 [expandedPkgs, setExpandedPkgs] = useState(initialExpandedPkgs);
const setPkgExpanded = ( const setPkgExpanded = (
@ -1001,12 +1013,13 @@ const Packages = () => {
isExpanding: boolean, isExpanding: boolean,
) => ) =>
setExpandedPkgs((prevExpanded) => { setExpandedPkgs((prevExpanded) => {
const otherExpandedPkgs = prevExpanded.filter((p) => p.name !== pkg.name); const pkgKey = getPackageUniqueKey(pkg);
return isExpanding ? [...otherExpandedPkgs, pkg] : otherExpandedPkgs; const otherExpandedPkgs = prevExpanded.filter((key) => key !== pkgKey);
return isExpanding ? [...otherExpandedPkgs, pkgKey] : otherExpandedPkgs;
}); });
const isPkgExpanded = (pkg: IBPackageWithRepositoryInfo) => const isPkgExpanded = (pkg: IBPackageWithRepositoryInfo) =>
expandedPkgs.includes(pkg); expandedPkgs.includes(getPackageUniqueKey(pkg));
const initialExpandedGroups: GroupWithRepositoryInfo['name'][] = []; const initialExpandedGroups: GroupWithRepositoryInfo['name'][] = [];
const [expandedGroups, setExpandedGroups] = useState(initialExpandedGroups); const [expandedGroups, setExpandedGroups] = useState(initialExpandedGroups);
@ -1030,51 +1043,37 @@ const Packages = () => {
'asc' | 'desc' 'asc' | 'desc'
>('asc'); >('asc');
const getSortableRowValues = ( const sortedPackages = useMemo(() => {
pkg: IBPackageWithRepositoryInfo, if (!transformedPackages || !Array.isArray(transformedPackages)) {
): (string | number | ApiPackageSourcesResponse[] | undefined)[] => { return [];
return [pkg.name, pkg.summary, pkg.stream, pkg.end_date, pkg.repository]; }
};
let sortedPackages = transformedPackages; return orderBy(
sortedPackages = transformedPackages.sort((a, b) => { transformedPackages,
const aValue = getSortableRowValues(a)[activeSortIndex]; [
const bValue = getSortableRowValues(b)[activeSortIndex]; // Active stream packages first (if activeStream is set)
if (typeof aValue === 'number') { (pkg) => (activeStream && pkg.stream === activeStream ? 0 : 1),
// Numeric sort // Then by name
if (activeSortDirection === 'asc') { 'name',
return (aValue as number) - (bValue as number); // Then by stream version (descending)
} (pkg) => {
return (bValue as number) - (aValue as number); if (!pkg.stream) return '';
} const parts = pkg.stream
// String sort .split('.')
// if active stream is set, sort it to the top .map((part) => parseInt(part, 10) || 0);
if (aValue === activeStream) { // Convert to string with zero-padding for proper sorting
return -1; return parts.map((p) => p.toString().padStart(10, '0')).join('.');
} },
if (bValue === activeStream) { // Then by end date (nulls last)
return 1; (pkg) => pkg.end_date || '9999-12-31',
} // Then by repository
if (activeSortDirection === 'asc') { (pkg) => pkg.repository || '',
// handle packages with undefined stream // Finally by module name
if (!aValue) { (pkg) => pkg.module_name || '',
return -1; ],
} ['asc', 'asc', 'desc', 'asc', 'asc', 'asc'],
if (!bValue) { );
return 1; }, [transformedPackages, activeStream]);
}
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);
}
});
const getSortParams = (columnIndex: number) => ({ const getSortParams = (columnIndex: number) => ({
sortBy: { sortBy: {
@ -1100,14 +1099,14 @@ const Packages = () => {
(module) => module.name === pkg.name, (module) => module.name === pkg.name,
); );
isSelected = 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') { if (pkg.type === 'module') {
// the package is selected if it's added to the packages state // the package is selected if its module stream matches one in enabled_modules
// and its module stream matches one in enabled_modules
isSelected = isSelected =
packages.some((p) => p.name === pkg.name) && packages.some((p) => p.name === pkg.name && p.stream === pkg.stream) &&
modules.some( modules.some(
(m) => m.name === pkg.module_name && m.stream === pkg.stream, (m) => m.name === pkg.module_name && m.stream === pkg.stream,
); );
@ -1208,7 +1207,7 @@ const Packages = () => {
.slice(computeStart(), computeEnd()) .slice(computeStart(), computeEnd())
.map((grp, rowIndex) => ( .map((grp, rowIndex) => (
<Tbody <Tbody
key={`${grp.name}-${rowIndex}`} key={`${grp.name}-${grp.repository || 'default'}`}
isExpanded={isGroupExpanded(grp.name)} isExpanded={isGroupExpanded(grp.name)}
> >
<Tr data-testid='package-row'> <Tr data-testid='package-row'>
@ -1308,7 +1307,7 @@ const Packages = () => {
.slice(computeStart(), computeEnd()) .slice(computeStart(), computeEnd())
.map((pkg, rowIndex) => ( .map((pkg, rowIndex) => (
<Tbody <Tbody
key={`${pkg.name}-${rowIndex}`} key={`${pkg.name}-${pkg.stream || 'default'}-${pkg.module_name || pkg.name}`}
isExpanded={isPkgExpanded(pkg)} isExpanded={isPkgExpanded(pkg)}
> >
<Tr data-testid='package-row'> <Tr data-testid='package-row'>

View file

@ -513,6 +513,123 @@ describe('Step Packages', () => {
expect(secondAppStreamRow).toBeDisabled(); expect(secondAppStreamRow).toBeDisabled();
expect(secondAppStreamRow).not.toBeChecked(); 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');
});
}); });
}); });

View file

@ -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') { if (search === 'mock') {
return [ return [
{ {