From 08b269071f34b23c40f27d42049e0a8f350143d0 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sun, 29 Mar 2026 11:09:41 -0400 Subject: [PATCH] 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', - ) - }) }) })