From 5bfeb19b040a162a241e0ca2b2ee0541f5d4e456 Mon Sep 17 00:00:00 2001 From: Alban Bailly Date: Tue, 22 Oct 2024 13:25:56 -0400 Subject: [PATCH] sorting and ordering --- .../ImageSelect/ImageSelect.test.tsx | 93 +++++++++++++++++-- .../components/ImageSelect/ImageSelect.tsx | 40 ++++++-- .../LinodeSettings/ImageAndPassword.tsx | 1 + 3 files changed, 119 insertions(+), 15 deletions(-) diff --git a/packages/manager/src/components/ImageSelect/ImageSelect.test.tsx b/packages/manager/src/components/ImageSelect/ImageSelect.test.tsx index 8963a67f818..1947fb7cc6d 100644 --- a/packages/manager/src/components/ImageSelect/ImageSelect.test.tsx +++ b/packages/manager/src/components/ImageSelect/ImageSelect.test.tsx @@ -12,7 +12,7 @@ import { ImageSelect } from './ImageSelect'; describe('ImageSelect', () => { it('should render a default "Images" label', () => { const { getByLabelText } = renderWithTheme( - + ); expect(getByLabelText('Images')).toBeVisible(); @@ -20,7 +20,7 @@ describe('ImageSelect', () => { it('should render default placeholder text', () => { const { getByPlaceholderText } = renderWithTheme( - + ); expect(getByPlaceholderText('Choose an image')).toBeVisible(); @@ -36,7 +36,7 @@ describe('ImageSelect', () => { ); const { getByPlaceholderText, getByText } = renderWithTheme( - + ); await userEvent.click(getByPlaceholderText('Choose an image')); @@ -57,7 +57,7 @@ describe('ImageSelect', () => { ); const { getByPlaceholderText, getByText } = renderWithTheme( - + ); await userEvent.click(getByPlaceholderText('Choose an image')); @@ -81,7 +81,7 @@ describe('ImageSelect', () => { ); const { findByDisplayValue } = renderWithTheme( - + ); await findByDisplayValue(image.label); @@ -97,7 +97,7 @@ describe('ImageSelect', () => { ); const { findByTestId } = renderWithTheme( - + ); await findByTestId('os-icon'); @@ -133,7 +133,7 @@ describe('ImageSelect', () => { ); const { getByPlaceholderText, getByText, queryByText } = renderWithTheme( - + ); await userEvent.click(getByPlaceholderText('Choose an image')); @@ -146,7 +146,12 @@ describe('ImageSelect', () => { it('should display an error', () => { const { getByText } = renderWithTheme( - + ); expect(getByText('An error')).toBeInTheDocument(); }); @@ -161,6 +166,7 @@ describe('ImageSelect', () => { multiple onChange={onSelect} value={[]} + variant="public" /> ); @@ -172,4 +178,75 @@ describe('ImageSelect', () => { expect.objectContaining({ id: 'any/all' }), ]); }); + + it('should sort images by vendor, then by creation date, then "My Images" first', async () => { + const publicImages = [ + imageFactory.build({ + created: '2023-01-01T00:00:00', + is_public: true, + label: 'Public Image 1', + vendor: 'CentOS', + }), + imageFactory.build({ + created: '2023-02-01T00:00:00', + is_public: true, + label: 'Public Image 2', + vendor: 'Debian', + }), + imageFactory.build({ + created: '2023-03-01T00:00:00', + is_public: true, + label: 'Public Image 3', + vendor: 'Ubuntu', + }), + ]; + const privateImages = [ + imageFactory.build({ + created: '2023-04-01T00:00:00', + is_public: false, + label: 'Private Image 1', + }), + imageFactory.build({ + created: '2023-05-01T00:00:00', + is_public: false, + label: 'Private Image 2', + }), + ]; + + // The API returns private images first, then public images + // Granted this won't change, I don't think we need to filter them client side + // Therefore this test assumes the API initial sort order + const images = [...privateImages, ...publicImages]; + + const { getAllByRole, getByLabelText, getByText } = renderWithTheme( + + ); + + await userEvent.click(getByLabelText('Images')); + expect(getByText('My Images')).toBeInTheDocument(); + expect(getByText('CentOS')).toBeInTheDocument(); + expect(getByText('Debian')).toBeInTheDocument(); + expect(getByText('Ubuntu')).toBeInTheDocument(); + + const options = getAllByRole('option'); + + expect(getByText('My Images')).toBeInTheDocument(); + expect(getByText('CentOS')).toBeInTheDocument(); + expect(getByText('Debian')).toBeInTheDocument(); + expect(getByText('Ubuntu')).toBeInTheDocument(); + + // Assert that private images ("My Images") come first + expect(options[0].textContent?.trim()).toContain('Private Image 1'); + expect(options[1].textContent?.trim()).toContain('Private Image 2'); + + // Assert the order of public images by vendor + expect(options[2].textContent?.trim()).toContain('Public Image 1'); + expect(options[3].textContent?.trim()).toContain('Public Image 2'); + expect(options[4].textContent?.trim()).toContain('Public Image 3'); + }); }); diff --git a/packages/manager/src/components/ImageSelect/ImageSelect.tsx b/packages/manager/src/components/ImageSelect/ImageSelect.tsx index 8fb43338725..9c7f355188a 100644 --- a/packages/manager/src/components/ImageSelect/ImageSelect.tsx +++ b/packages/manager/src/components/ImageSelect/ImageSelect.tsx @@ -30,7 +30,7 @@ interface BaseProps label?: string; placeholder?: string; selectIfOnlyOneOption?: boolean; - variant?: ImageSelectVariant; + variant: ImageSelectVariant; } interface SingleProps extends BaseProps { @@ -56,7 +56,7 @@ export const ImageSelect = (props: Props) => { onChange, placeholder, selectIfOnlyOneOption, - variant = 'all', + variant, ...rest } = props; @@ -87,6 +87,35 @@ export const ImageSelect = (props: Props) => { return _options; }, [anyAllOption, _options]); + // We need to sort options when grouping in order to avoid duplicate headers + // see https://mui.com/material-ui/react-autocomplete/#grouped + // We want: + // - Vendors to be sorted alphabetically + // - "My Images" to be first + // - Images to be sorted by creation date, newest first + const sortedOptions = useMemo(() => { + const myImages = options.filter((option) => !option.is_public); + const otherImages = options.filter((option) => option.is_public); + + const sortedVendors = Array.from( + new Set(otherImages.map((img) => img.vendor)) + ).sort((a, b) => (a ?? '').localeCompare(b ?? '')); + + return [ + ...myImages.sort( + (a, b) => new Date(b.created).getTime() - new Date(a.created).getTime() + ), + ...sortedVendors.flatMap((vendor) => + otherImages + .filter((img) => img.vendor === vendor) + .sort( + (a, b) => + new Date(b.created).getTime() - new Date(a.created).getTime() + ) + ), + ]; + }, [options]); + const selected = props.value; const value = useMemo(() => { if (multiple) { @@ -157,7 +186,7 @@ export const ImageSelect = (props: Props) => { clearOnBlur label={label || 'Images'} loading={isLoading} - options={options} + options={sortedOptions} placeholder={placeholder || 'Choose an image'} {...rest} disableClearable={ @@ -177,10 +206,7 @@ export const ImageSelect = (props: Props) => { ); }; -const StyledList = styled( - List, - {} -)({ +const StyledList = styled(List, { label: 'ImageSelectStyledList' })({ listStyle: 'none', padding: 0, }); diff --git a/packages/manager/src/features/Linodes/LinodesDetail/LinodeSettings/ImageAndPassword.tsx b/packages/manager/src/features/Linodes/LinodesDetail/LinodeSettings/ImageAndPassword.tsx index 799203bf50d..a1020509ce7 100644 --- a/packages/manager/src/features/Linodes/LinodesDetail/LinodeSettings/ImageAndPassword.tsx +++ b/packages/manager/src/features/Linodes/LinodesDetail/LinodeSettings/ImageAndPassword.tsx @@ -49,6 +49,7 @@ export const ImageAndPassword = (props: Props) => { errorText={imageFieldError} onChange={onImageChange} value={selectedImage} + variant="all" />