From 08b269071f34b23c40f27d42049e0a8f350143d0 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sun, 29 Mar 2026 11:09:41 -0400 Subject: [PATCH 1/3] fix: allow users to have no libraries assigned Previously, three layers enforced that non-admin users must have at least one library: the backend service validation, the frontend data provider (which silently skipped the API call for empty lists), and the frontend form validation. This prevented both creating users without libraries and removing all libraries from existing users. Removed the "at least one library" constraint across all layers so that library assignment is fully optional for non-admin users. --- core/library.go | 11 ++----- core/library_test.go | 7 +++-- server/nativeapi/library_test.go | 23 +++++++++++--- tests/mock_library_repo.go | 35 +++++++++++++++++----- ui/src/dataProvider/wrapperDataProvider.js | 8 ++--- ui/src/user/userValidation.js | 16 +--------- ui/src/user/userValidation.test.js | 33 ++------------------ 7 files changed, 58 insertions(+), 75 deletions(-) diff --git a/core/library.go b/core/library.go index 0bf3be9fa..eb865cfc7 100644 --- a/core/library.go +++ b/core/library.go @@ -78,16 +78,9 @@ func (s *libraryService) SetUserLibraries(ctx context.Context, userID string, li return fmt.Errorf("%w: cannot manually assign libraries to admin users", model.ErrValidation) } - // Regular users must have at least one library - if len(libraryIDs) == 0 { - return fmt.Errorf("%w: at least one library must be assigned to non-admin users", model.ErrValidation) - } - // Validate all library IDs exist - if len(libraryIDs) > 0 { - if err := s.validateLibraryIDs(ctx, libraryIDs); err != nil { - return err - } + if err := s.validateLibraryIDs(ctx, libraryIDs); err != nil { + return err } // Set user libraries diff --git a/core/library_test.go b/core/library_test.go index 175d9c37d..0ff1ac077 100644 --- a/core/library_test.go +++ b/core/library_test.go @@ -535,11 +535,12 @@ var _ = Describe("Library Service", func() { Expect(err.Error()).To(ContainSubstring("cannot manually assign libraries to admin users")) }) - It("fails when no libraries provided for regular user", func() { + It("allows setting empty libraries for regular user", func() { err := service.SetUserLibraries(ctx, "user1", []int{}) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("at least one library must be assigned to non-admin users")) + Expect(err).NotTo(HaveOccurred()) + libraries := userRepo.UserLibraries["user1"] + Expect(libraries).To(BeEmpty()) }) It("fails when library doesn't exist", func() { diff --git a/server/nativeapi/library_test.go b/server/nativeapi/library_test.go index ed5564a41..41982548d 100644 --- a/server/nativeapi/library_test.go +++ b/server/nativeapi/library_test.go @@ -350,9 +350,20 @@ var _ = Describe("Library API", func() { Expect(w.Body.String()).To(ContainSubstring("library ID 999 does not exist")) }) - It("requires at least one library for regular users", func() { + It("allows removing all libraries from regular users", func() { + // First assign some libraries + setupRequest := map[string][]int{ + "libraryIds": {1, 2}, + } + setupBody, _ := json.Marshal(setupRequest) + setupReq := createAuthenticatedRequest("PUT", fmt.Sprintf("/user/%s/library", regularUser.ID), bytes.NewBuffer(setupBody), adminToken) + setupW := httptest.NewRecorder() + router.ServeHTTP(setupW, setupReq) + Expect(setupW.Code).To(Equal(http.StatusOK)) + + // Then remove all libraries request := map[string][]int{ - "libraryIds": {}, // Empty libraries + "libraryIds": {}, } body, _ := json.Marshal(request) req := createAuthenticatedRequest("PUT", fmt.Sprintf("/user/%s/library", regularUser.ID), bytes.NewBuffer(body), adminToken) @@ -360,8 +371,12 @@ var _ = Describe("Library API", func() { router.ServeHTTP(w, req) - Expect(w.Code).To(Equal(http.StatusBadRequest)) - Expect(w.Body.String()).To(ContainSubstring("at least one library must be assigned")) + Expect(w.Code).To(Equal(http.StatusOK)) + + var libraries []model.Library + err := json.Unmarshal(w.Body.Bytes(), &libraries) + Expect(err).ToNot(HaveOccurred()) + Expect(libraries).To(BeEmpty()) }) It("prevents manual assignment to admin users", func() { diff --git a/tests/mock_library_repo.go b/tests/mock_library_repo.go index 3f0e576e9..6e922ed88 100644 --- a/tests/mock_library_repo.go +++ b/tests/mock_library_repo.go @@ -14,9 +14,10 @@ import ( type MockLibraryRepo struct { model.LibraryRepository - Data map[int]model.Library - Err error - PutFn func(*model.Library) error // Allow custom Put behavior for testing + Data map[int]model.Library + UserLibraries map[string][]int // per-user library ID assignments + Err error + PutFn func(*model.Library) error // Allow custom Put behavior for testing } func (m *MockLibraryRepo) SetData(data model.Libraries) { @@ -266,12 +267,28 @@ func (m *MockLibraryRepo) GetUserLibraries(ctx context.Context, userID string) ( if userID == "non-existent" { return nil, model.ErrNotFound } - // Convert map to slice for return + // If per-user tracking is set, return only the assigned libraries + if m.UserLibraries != nil { + ids, ok := m.UserLibraries[userID] + if !ok { + return nil, nil + } + var libraries model.Libraries + for _, id := range ids { + if lib, exists := m.Data[id]; exists { + libraries = append(libraries, lib) + } + } + slices.SortFunc(libraries, func(a, b model.Library) int { + return a.ID - b.ID + }) + return libraries, nil + } + // Fallback: return all libraries var libraries model.Libraries for _, lib := range m.Data { libraries = append(libraries, lib) } - // Sort by ID for predictable order slices.SortFunc(libraries, func(a, b model.Library) int { return a.ID - b.ID }) @@ -288,15 +305,17 @@ func (m *MockLibraryRepo) SetUserLibraries(ctx context.Context, userID string, l if userID == "admin-1" { return fmt.Errorf("%w: cannot manually assign libraries to admin users", model.ErrValidation) } - if len(libraryIDs) == 0 { - return fmt.Errorf("%w: at least one library must be assigned to non-admin users", model.ErrValidation) - } // Validate all library IDs exist for _, id := range libraryIDs { if _, exists := m.Data[id]; !exists { return fmt.Errorf("%w: library ID %d does not exist", model.ErrValidation, id) } } + // Store per-user assignments + if m.UserLibraries == nil { + m.UserLibraries = make(map[string][]int) + } + m.UserLibraries[userID] = libraryIDs return nil } diff --git a/ui/src/dataProvider/wrapperDataProvider.js b/ui/src/dataProvider/wrapperDataProvider.js index 268d3668d..2a0df26e9 100644 --- a/ui/src/dataProvider/wrapperDataProvider.js +++ b/ui/src/dataProvider/wrapperDataProvider.js @@ -93,14 +93,10 @@ const callDeleteMany = (resource, params) => { // Helper function to handle user-library associations const handleUserLibraryAssociation = async (userId, libraryIds) => { - if (!libraryIds || libraryIds.length === 0) { - return // Admin users or users without library assignments - } - try { await httpClient(`${REST_URL}/user/${userId}/library`, { method: 'PUT', - body: JSON.stringify({ libraryIds }), + body: JSON.stringify({ libraryIds: libraryIds || [] }), }) } catch (error) { console.error('Error setting user libraries:', error) //eslint-disable-line no-console @@ -118,7 +114,7 @@ const createUser = async (params) => { const userId = userResponse.data.id // Then set library associations for non-admin users - if (!userData.isAdmin && libraryIds && libraryIds.length > 0) { + if (!userData.isAdmin) { await handleUserLibraryAssociation(userId, libraryIds) } diff --git a/ui/src/user/userValidation.js b/ui/src/user/userValidation.js index e90fd2acb..e4c43219d 100644 --- a/ui/src/user/userValidation.js +++ b/ui/src/user/userValidation.js @@ -1,19 +1,5 @@ // User form validation utilities -export const validateUserForm = (values, translate) => { +export const validateUserForm = (_values, _translate) => { const errors = {} - - // Only require library selection for non-admin users - if (!values.isAdmin) { - // Check both libraryIds (array of IDs) and libraries (array of objects) - const hasLibraryIds = values.libraryIds && values.libraryIds.length > 0 - const hasLibraries = values.libraries && values.libraries.length > 0 - - if (!hasLibraryIds && !hasLibraries) { - errors.libraryIds = translate( - 'resources.user.validation.librariesRequired', - ) - } - } - return errors } diff --git a/ui/src/user/userValidation.test.js b/ui/src/user/userValidation.test.js index 2ee473910..f1d1aadba 100644 --- a/ui/src/user/userValidation.test.js +++ b/ui/src/user/userValidation.test.js @@ -23,48 +23,21 @@ describe('User Validation Utilities', () => { expect(errors).toEqual({}) }) - it('should return error for non-admin users without libraries', () => { + it('should not return errors for non-admin users without libraries', () => { const values = { isAdmin: false, libraryIds: [], } const errors = validateUserForm(values, mockTranslate) - expect(errors.libraryIds).toBe( - 'resources.user.validation.librariesRequired', - ) + expect(errors).toEqual({}) }) - it('should return error for non-admin users with undefined libraryIds', () => { + it('should not return errors for non-admin users with undefined libraryIds', () => { const values = { isAdmin: false, } const errors = validateUserForm(values, mockTranslate) - expect(errors.libraryIds).toBe( - 'resources.user.validation.librariesRequired', - ) - }) - - it('should not return errors for non-admin users with libraries array', () => { - const values = { - isAdmin: false, - libraries: [ - { id: 1, name: 'Library 1' }, - { id: 2, name: 'Library 2' }, - ], - } - const errors = validateUserForm(values, mockTranslate) expect(errors).toEqual({}) }) - - it('should return error for non-admin users with empty libraries array', () => { - const values = { - isAdmin: false, - libraries: [], - } - const errors = validateUserForm(values, mockTranslate) - expect(errors.libraryIds).toBe( - 'resources.user.validation.librariesRequired', - ) - }) }) }) From ef3dddd164613c8ee9915b281c4f671c001ae730 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sun, 29 Mar 2026 11:48:05 -0400 Subject: [PATCH 2/3] refactor: remove dead user validation code and simplify mock Removed the now-empty userValidation.js and its test file since the validation function was reduced to a no-op. Cleaned up the callers in UserCreate and UserEdit that referenced it. Simplified the mock GetUserLibraries fallback to delegate to GetAll instead of duplicating its logic, and removed redundant comments. --- tests/mock_library_repo.go | 13 +-------- ui/src/user/UserCreate.jsx | 10 +------ ui/src/user/UserEdit.jsx | 7 ----- ui/src/user/userValidation.js | 5 ---- ui/src/user/userValidation.test.js | 43 ------------------------------ 5 files changed, 2 insertions(+), 76 deletions(-) delete mode 100644 ui/src/user/userValidation.js delete mode 100644 ui/src/user/userValidation.test.js diff --git a/tests/mock_library_repo.go b/tests/mock_library_repo.go index 6e922ed88..5feb8fb06 100644 --- a/tests/mock_library_repo.go +++ b/tests/mock_library_repo.go @@ -267,7 +267,6 @@ func (m *MockLibraryRepo) GetUserLibraries(ctx context.Context, userID string) ( if userID == "non-existent" { return nil, model.ErrNotFound } - // If per-user tracking is set, return only the assigned libraries if m.UserLibraries != nil { ids, ok := m.UserLibraries[userID] if !ok { @@ -284,15 +283,7 @@ func (m *MockLibraryRepo) GetUserLibraries(ctx context.Context, userID string) ( }) return libraries, nil } - // Fallback: return all libraries - var libraries model.Libraries - for _, lib := range m.Data { - libraries = append(libraries, lib) - } - slices.SortFunc(libraries, func(a, b model.Library) int { - return a.ID - b.ID - }) - return libraries, nil + return m.GetAll() } func (m *MockLibraryRepo) SetUserLibraries(ctx context.Context, userID string, libraryIDs []int) error { @@ -305,13 +296,11 @@ func (m *MockLibraryRepo) SetUserLibraries(ctx context.Context, userID string, l if userID == "admin-1" { return fmt.Errorf("%w: cannot manually assign libraries to admin users", model.ErrValidation) } - // Validate all library IDs exist for _, id := range libraryIDs { if _, exists := m.Data[id]; !exists { return fmt.Errorf("%w: library ID %d does not exist", model.ErrValidation, id) } } - // Store per-user assignments if m.UserLibraries == nil { m.UserLibraries = make(map[string][]int) } diff --git a/ui/src/user/UserCreate.jsx b/ui/src/user/UserCreate.jsx index ce69b6542..b6370ad9b 100644 --- a/ui/src/user/UserCreate.jsx +++ b/ui/src/user/UserCreate.jsx @@ -51,17 +51,9 @@ const UserCreate = (props) => { [mutate, notify, redirect], ) - // Custom validation function - const validateUserForm = (values) => { - const errors = {} - // Library selection is optional for non-admin users since they will be auto-assigned to default libraries - // No validation required for library selection - return errors - } - return ( } {...props}> - + { [mutate, notify, permissions, redirect, refresh], ) - // Custom validation function - const validateForm = (values) => { - return validateUserForm(values, translate) - } - return ( } undoable={false} {...props}> } save={save} - validate={validateForm} > {permissions === 'admin' && ( { - const errors = {} - return errors -} diff --git a/ui/src/user/userValidation.test.js b/ui/src/user/userValidation.test.js deleted file mode 100644 index f1d1aadba..000000000 --- a/ui/src/user/userValidation.test.js +++ /dev/null @@ -1,43 +0,0 @@ -import { describe, it, expect, vi } from 'vitest' -import { validateUserForm } from './userValidation' - -describe('User Validation Utilities', () => { - const mockTranslate = vi.fn((key) => key) - - describe('validateUserForm', () => { - it('should not return errors for admin users', () => { - const values = { - isAdmin: true, - libraryIds: [], - } - const errors = validateUserForm(values, mockTranslate) - expect(errors).toEqual({}) - }) - - it('should not return errors for non-admin users with libraries', () => { - const values = { - isAdmin: false, - libraryIds: [1, 2, 3], - } - const errors = validateUserForm(values, mockTranslate) - expect(errors).toEqual({}) - }) - - it('should not return errors for non-admin users without libraries', () => { - const values = { - isAdmin: false, - libraryIds: [], - } - const errors = validateUserForm(values, mockTranslate) - expect(errors).toEqual({}) - }) - - it('should not return errors for non-admin users with undefined libraryIds', () => { - const values = { - isAdmin: false, - } - const errors = validateUserForm(values, mockTranslate) - expect(errors).toEqual({}) - }) - }) -}) From 02ef4037df81a699f3b923ed68ae759154821d7e Mon Sep 17 00:00:00 2001 From: Deluan Date: Sun, 29 Mar 2026 12:11:04 -0400 Subject: [PATCH 3/3] fix: only send library association on create when libraryIds is defined Guard the createUser library association call with libraryIds !== undefined, matching the updateUser pattern. This prevents accidentally clearing backend default library assignments when the form field hasn't been initialized yet. --- ui/src/dataProvider/wrapperDataProvider.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/src/dataProvider/wrapperDataProvider.js b/ui/src/dataProvider/wrapperDataProvider.js index 2a0df26e9..23bb69587 100644 --- a/ui/src/dataProvider/wrapperDataProvider.js +++ b/ui/src/dataProvider/wrapperDataProvider.js @@ -114,7 +114,7 @@ const createUser = async (params) => { const userId = userResponse.data.id // Then set library associations for non-admin users - if (!userData.isAdmin) { + if (!userData.isAdmin && libraryIds !== undefined) { await handleUserLibraryAssociation(userId, libraryIds) }