From 0b2d728b21994cf2dc0437440cef52918e9b74ab Mon Sep 17 00:00:00 2001 From: Deluan Date: Sun, 29 Jun 2025 17:01:57 -0400 Subject: [PATCH] feat: enhance library selection with master checkbox functionality Added a master checkbox to the SelectLibraryInput component, allowing users to select or deselect all libraries at once. This improves user experience by simplifying the selection process when multiple libraries are available. Additionally, updated translations in the en.json file to include a new message for selecting all libraries, ensuring consistency in user interface messaging. Signed-off-by: Deluan --- ui/src/common/SelectLibraryInput.jsx | 42 +++++- ui/src/common/SelectLibraryInput.test.jsx | 156 ++++++++++++++++++++-- ui/src/i18n/en.json | 5 +- ui/src/user/UserCreate.jsx | 2 +- ui/src/user/UserEdit.jsx | 2 +- 5 files changed, 193 insertions(+), 14 deletions(-) diff --git a/ui/src/common/SelectLibraryInput.jsx b/ui/src/common/SelectLibraryInput.jsx index 4d34fd822..4825693ea 100644 --- a/ui/src/common/SelectLibraryInput.jsx +++ b/ui/src/common/SelectLibraryInput.jsx @@ -8,8 +8,9 @@ import { ListItemIcon, ListItemText, Typography, + Box, } from '@material-ui/core' -import { useGetList } from 'react-admin' +import { useGetList, useTranslate } from 'react-admin' import PropTypes from 'prop-types' import { makeStyles } from '@material-ui/core' @@ -18,6 +19,17 @@ const useStyles = makeStyles((theme) => ({ width: '960px', // MD breakpoint width maxWidth: '100%', }, + headerContainer: { + display: 'flex', + alignItems: 'center', + marginBottom: theme.spacing(1), + paddingLeft: theme.spacing(1), + }, + masterCheckbox: { + padding: '7px', // Match the default Material-UI checkbox padding + marginLeft: '-9px', // Negative margin to align with FormControlLabel checkboxes + marginRight: theme.spacing(1), + }, libraryList: { height: '120px', // 3 rows of libraries overflow: 'auto', @@ -72,6 +84,7 @@ const LibraryListItem = ({ library, isSelected, onToggle }) => { export const SelectLibraryInput = ({ onChange, value = [] }) => { const classes = useStyles() + const translate = useTranslate() const [selectedLibraryIds, setSelectedLibraryIds] = useState([]) const { ids, data } = useGetList( @@ -109,8 +122,35 @@ export const SelectLibraryInput = ({ onChange, value = [] }) => { onChange(newSelection) } + const handleMasterCheckboxChange = () => { + const isAllSelected = selectedLibraryIds.length === options.length + const newSelection = isAllSelected ? [] : options.map((lib) => lib.id) + + setSelectedLibraryIds(newSelection) + onChange(newSelection) + } + + const selectedCount = selectedLibraryIds.length + const totalCount = options.length + const isAllSelected = selectedCount === totalCount && totalCount > 0 + const isIndeterminate = selectedCount > 0 && selectedCount < totalCount + return (
+ {options.length > 1 && ( + + + + {translate('resources.user.message.selectAllLibraries')} + + + )} {options.length === 0 ? ( diff --git a/ui/src/common/SelectLibraryInput.test.jsx b/ui/src/common/SelectLibraryInput.test.jsx index 706ece428..35e640591 100644 --- a/ui/src/common/SelectLibraryInput.test.jsx +++ b/ui/src/common/SelectLibraryInput.test.jsx @@ -4,9 +4,50 @@ import { SelectLibraryInput } from './SelectLibraryInput' import { useGetList } from 'react-admin' import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +// Mock Material-UI components +vi.mock('@material-ui/core', () => ({ + List: ({ children }) =>
{children}
, + ListItem: ({ children, button, onClick, dense, className }) => ( + + ), + ListItemIcon: ({ children }) => {children}, + ListItemText: ({ primary }) => {primary}, + Typography: ({ children, variant }) => {children}, + Box: ({ children, className }) =>
{children}
, + Checkbox: ({ + checked, + indeterminate, + onChange, + size, + className, + ...props + }) => ( + { + if (el) el.indeterminate = indeterminate + }} + onChange={onChange} + className={className} + {...props} + /> + ), + makeStyles: () => () => ({}), +})) + +// Mock Material-UI icons +vi.mock('@material-ui/icons', () => ({ + CheckBox: () => ☑, + CheckBoxOutlineBlank: () => ☐, +})) + // Mock the react-admin hook vi.mock('react-admin', () => ({ useGetList: vi.fn(), + useTranslate: vi.fn(() => (key) => key), // Simple translation mock })) describe('', () => { @@ -60,8 +101,9 @@ describe('', () => { }) render() - // First test - click to select - fireEvent.click(screen.getAllByRole('button')[0]) // Click first list item + // Find the library buttons by their text content + const library1Button = screen.getByText('Library 1').closest('button') + fireEvent.click(library1Button) expect(mockOnChange).toHaveBeenCalledWith(['1']) // Clean up to avoid DOM duplication @@ -75,8 +117,11 @@ describe('', () => { }) render() - // Second test - click to deselect - fireEvent.click(screen.getAllByRole('button')[0]) // Click first list item again + // Find the library button again and click to deselect + const library1ButtonDeselect = screen + .getByText('Library 1') + .closest('button') + fireEvent.click(library1ButtonDeselect) expect(mockOnChange).toHaveBeenCalledWith([]) }) @@ -96,8 +141,9 @@ describe('', () => { // Check that checkbox for Library 1 is checked const checkboxes = screen.getAllByRole('checkbox') - expect(checkboxes[0].checked).toBe(true) - expect(checkboxes[1].checked).toBe(false) + // With master checkbox, individual checkboxes start at index 1 + expect(checkboxes[1].checked).toBe(true) // Library 1 + expect(checkboxes[2].checked).toBe(false) // Library 2 }) it('should handle value as array of objects', () => { @@ -116,7 +162,101 @@ describe('', () => { // Check that checkbox for Library 2 is checked const checkboxes = screen.getAllByRole('checkbox') - expect(checkboxes[0].checked).toBe(false) - expect(checkboxes[1].checked).toBe(true) + // With master checkbox, index shifts by 1 + expect(checkboxes[1].checked).toBe(false) // Library 1 + expect(checkboxes[2].checked).toBe(true) // Library 2 + }) + + it('should render master checkbox when there are multiple libraries', () => { + // Mock libraries + const mockLibraries = { + 1: { id: '1', name: 'Library 1' }, + 2: { id: '2', name: 'Library 2' }, + } + useGetList.mockReturnValue({ + ids: ['1', '2'], + data: mockLibraries, + }) + + render() + + // Should render master checkbox plus individual checkboxes + const checkboxes = screen.getAllByRole('checkbox') + expect(checkboxes).toHaveLength(3) // 1 master + 2 individual + expect( + screen.getByText('resources.user.message.selectAllLibraries'), + ).not.toBeNull() + }) + + it('should not render master checkbox when there is only one library', () => { + // Mock single library + const mockLibraries = { + 1: { id: '1', name: 'Library 1' }, + } + useGetList.mockReturnValue({ + ids: ['1'], + data: mockLibraries, + }) + + render() + + // Should render only individual checkbox + const checkboxes = screen.getAllByRole('checkbox') + expect(checkboxes).toHaveLength(1) // Only 1 individual checkbox + }) + + it('should handle master checkbox selection and deselection', () => { + // Mock libraries + const mockLibraries = { + 1: { id: '1', name: 'Library 1' }, + 2: { id: '2', name: 'Library 2' }, + } + useGetList.mockReturnValue({ + ids: ['1', '2'], + data: mockLibraries, + }) + + render() + + const checkboxes = screen.getAllByRole('checkbox') + const masterCheckbox = checkboxes[0] // Master is first + + // Click master checkbox to select all + fireEvent.click(masterCheckbox) + expect(mockOnChange).toHaveBeenCalledWith(['1', '2']) + + // Clean up and test deselect all + cleanup() + mockOnChange.mockClear() + + render() + const checkboxes2 = screen.getAllByRole('checkbox') + const masterCheckbox2 = checkboxes2[0] + + // Click master checkbox to deselect all + fireEvent.click(masterCheckbox2) + expect(mockOnChange).toHaveBeenCalledWith([]) + }) + + it('should show master checkbox as indeterminate when some libraries are selected', () => { + // Mock libraries + const mockLibraries = { + 1: { id: '1', name: 'Library 1' }, + 2: { id: '2', name: 'Library 2' }, + } + useGetList.mockReturnValue({ + ids: ['1', '2'], + data: mockLibraries, + }) + + render() + + const checkboxes = screen.getAllByRole('checkbox') + const masterCheckbox = checkboxes[0] // Master is first + + // Master checkbox should not be checked when only some libraries are selected + expect(masterCheckbox.checked).toBe(false) + // Note: Testing indeterminate property directly through JSDOM can be unreliable + // The important behavior is that it's not checked when only some are selected }) }) diff --git a/ui/src/i18n/en.json b/ui/src/i18n/en.json index 9525b878d..72cc3adb9 100644 --- a/ui/src/i18n/en.json +++ b/ui/src/i18n/en.json @@ -166,9 +166,8 @@ }, "message": { "listenBrainzToken": "Enter your ListenBrainz user token.", - "clickHereForToken": "Click here to get your token" - }, - "messages": { + "clickHereForToken": "Click here to get your token", + "selectAllLibraries": "Select all libraries", "adminAutoLibraries": "Admin users automatically have access to all libraries" } }, diff --git a/ui/src/user/UserCreate.jsx b/ui/src/user/UserCreate.jsx index 182f66ab5..ee1fa5f79 100644 --- a/ui/src/user/UserCreate.jsx +++ b/ui/src/user/UserCreate.jsx @@ -95,7 +95,7 @@ const UserCreate = (props) => { color="textSecondary" style={{ marginTop: 16, marginBottom: 16 }} > - {translate('resources.user.messages.adminAutoLibraries')} + {translate('resources.user.message.adminAutoLibraries')} )} diff --git a/ui/src/user/UserEdit.jsx b/ui/src/user/UserEdit.jsx index 9401f8f7c..3406aa4c4 100644 --- a/ui/src/user/UserEdit.jsx +++ b/ui/src/user/UserEdit.jsx @@ -171,7 +171,7 @@ const UserEdit = (props) => { color="textSecondary" style={{ marginTop: 16, marginBottom: 16 }} > - {translate('resources.user.messages.adminAutoLibraries')} + {translate('resources.user.message.adminAutoLibraries')} )}