User management improvements (#1101)

* Show more descriptive success messages for User actions

* Check username uniqueness when creating/updating User

* Adjust translations

* Add tests for `validateUsernameUnique()`

Co-authored-by: Deluan <deluan@navidrome.org>

persistence/user_repository.go

@@ -134,6 +134,9 @@ func (r *userRepository) Save(entity interface{}) (string, error) {
 		return "", rest.ErrPermissionDenied
 	}
 	u := entity.(*model.User)
+	if err := validateUsernameUnique(r, u); err != nil {
+		return "", err
+	}
 	err := r.Put(u)
 	if err != nil {
 		return "", err
@@ -157,6 +160,9 @@ func (r *userRepository) Update(entity interface{}, cols ...string) error {
 	if err := validatePasswordChange(u, usr); err != nil {
 		return err
 	}
+	if err := validateUsernameUnique(r, u); err != nil {
+		return err
+	}
 	err := r.Put(u)
 	if err == model.ErrNotFound {
 		return rest.ErrNotFound
@@ -186,6 +192,20 @@ func validatePasswordChange(newUser *model.User, logged *model.User) error {
 	return nil
 }
 
+func validateUsernameUnique(r model.UserRepository, u *model.User) error {
+	usr, err := r.FindByUsername(u.UserName)
+	if err == model.ErrNotFound {
+		return nil
+	}
+	if err != nil {
+		return err
+	}
+	if usr.ID != u.ID {
+		return &rest.ValidationError{Errors: map[string]string{"userName": "ra.validation.unique"}}
+	}
+	return nil
+}
+
 func (r *userRepository) Delete(id string) error {
 	usr := loggedUser(r.ctx)
 	if !usr.IsAdmin {

persistence/user_repository_test.go

@@ -2,11 +2,13 @@ package persistence
 
 import (
 	"context"
+	"errors"
 
 	"github.com/astaxie/beego/orm"
 	"github.com/deluan/rest"
 	"github.com/navidrome/navidrome/log"
 	"github.com/navidrome/navidrome/model"
+	"github.com/navidrome/navidrome/tests"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 )
@@ -144,4 +146,32 @@ var _ = Describe("UserRepository", func() {
 			})
 		})
 	})
+	Describe("validateUsernameUnique", func() {
+		var repo *tests.MockedUserRepo
+		var existingUser *model.User
+		BeforeEach(func() {
+			existingUser = &model.User{ID: "1", UserName: "johndoe"}
+			repo = tests.CreateMockUserRepo()
+			err := repo.Put(existingUser)
+			Expect(err).ToNot(HaveOccurred())
+		})
+		It("allows unique usernames", func() {
+			var newUser = &model.User{ID: "2", UserName: "unique_username"}
+			err := validateUsernameUnique(repo, newUser)
+			Expect(err).ToNot(HaveOccurred())
+		})
+		It("returns ValidationError if username already exists", func() {
+			var newUser = &model.User{ID: "2", UserName: "johndoe"}
+			err := validateUsernameUnique(repo, newUser)
+			Expect(err).To(BeAssignableToTypeOf(&rest.ValidationError{}))
+			Expect(err.(*rest.ValidationError).Errors).To(HaveKeyWithValue("userName", "ra.validation.unique"))
+		})
+		It("returns generic error if repository call fails", func() {
+			repo.Err = errors.New("fake error")
+
+			var newUser = &model.User{ID: "2", UserName: "newuser"}
+			err := validateUsernameUnique(repo, newUser)
+			Expect(err).To(MatchError("fake error"))
+		})
+	})
 })

tests/mock_persistence.go

@@ -66,7 +66,7 @@ func (db *MockDataStore) Property(context.Context) model.PropertyRepository {
 
 func (db *MockDataStore) User(context.Context) model.UserRepository {
 	if db.MockedUser == nil {
-		db.MockedUser = &mockedUserRepo{}
+		db.MockedUser = CreateMockUserRepo()
 	}
 	return db.MockedUser
 }

tests/mock_user_repo.go

@@ -7,35 +7,48 @@ import (
 	"github.com/navidrome/navidrome/model"
 )
 
-type mockedUserRepo struct {
+func CreateMockUserRepo() *MockedUserRepo {
+	return &MockedUserRepo{
+		Data: map[string]*model.User{},
+	}
+}
+
+type MockedUserRepo struct {
 	model.UserRepository
-	data map[string]*model.User
+	Err  error
+	Data map[string]*model.User
 }
 
-func (u *mockedUserRepo) CountAll(qo ...model.QueryOptions) (int64, error) {
+func (u *MockedUserRepo) CountAll(qo ...model.QueryOptions) (int64, error) {
-	return int64(len(u.data)), nil
+	if u.Err != nil {
+		return 0, u.Err
+	}
+	return int64(len(u.Data)), nil
 }
 
-func (u *mockedUserRepo) Put(usr *model.User) error {
+func (u *MockedUserRepo) Put(usr *model.User) error {
-	if u.data == nil {
+	if u.Err != nil {
-		u.data = make(map[string]*model.User)
+		return u.Err
 	}
 	if usr.ID == "" {
 		usr.ID = base64.StdEncoding.EncodeToString([]byte(usr.UserName))
 	}
 	usr.Password = usr.NewPassword
-	u.data[strings.ToLower(usr.UserName)] = usr
+	u.Data[strings.ToLower(usr.UserName)] = usr
 	return nil
 }
 
-func (u *mockedUserRepo) FindByUsername(username string) (*model.User, error) {
+func (u *MockedUserRepo) FindByUsername(username string) (*model.User, error) {
-	usr, ok := u.data[strings.ToLower(username)]
+	if u.Err != nil {
+		return nil, u.Err
+	}
+	usr, ok := u.Data[strings.ToLower(username)]
 	if !ok {
 		return nil, model.ErrNotFound
 	}
 	return usr, nil
 }
 
-func (u *mockedUserRepo) UpdateLastLoginAt(id string) error {
+func (u *MockedUserRepo) UpdateLastLoginAt(id string) error {
-	return nil
+	return u.Err
 }

ui/src/i18n/en.json

@@ -94,6 +94,11 @@
       },
       "helperTexts": {
         "name": "Changes to your name will only be reflected on next login"
+      },
+      "notifications": {
+        "created": "User created",
+        "updated": "User updated",
+        "deleted": "User deleted"
       }
     },
     "player": {
@@ -167,7 +172,8 @@
       "number": "Must be a number",
       "email": "Must be a valid email",
       "oneOf": "Must be one of: %{options}",
-      "regex": "Must match a specific format (regexp): %{pattern}"
+      "regex": "Must match a specific format (regexp): %{pattern}",
+      "unique": "Must be unique"
     },
     "action": {
       "add_filter": "Add filter",

ui/src/user/DeleteUserButton.js

@@ -3,7 +3,13 @@ import DeleteIcon from '@material-ui/icons/Delete'
 import { makeStyles } from '@material-ui/core/styles'
 import { fade } from '@material-ui/core/styles/colorManipulator'
 import clsx from 'clsx'
-import { useDeleteWithConfirmController, Button, Confirm } from 'react-admin'
+import {
+  useDeleteWithConfirmController,
+  Button,
+  Confirm,
+  useNotify,
+  useRedirect,
+} from 'react-admin'
 
 const useStyles = makeStyles(
   (theme) => ({
@@ -22,22 +28,23 @@ const useStyles = makeStyles(
 )
 
 const DeleteUserButton = (props) => {
-  const {
+  const { resource, record, basePath, className, onClick, ...rest } = props
-    resource,
+
-    record,
+  const notify = useNotify()
-    basePath,
+  const redirect = useRedirect()
-    redirect = 'list',
+
-    className,
+  const onSuccess = () => {
-    onClick,
+    notify('resources.user.notifications.deleted')
-    ...rest
+    redirect('/user')
-  } = props
+  }
+
   const { open, loading, handleDialogOpen, handleDialogClose, handleDelete } =
     useDeleteWithConfirmController({
       resource,
       record,
-      redirect,
       basePath,
       onClick,
+      onSuccess,
     })
 
   const classes = useStyles(props)

ui/src/user/UserCreate.js

@@ -1,4 +1,4 @@
-import React from 'react'
+import React, { useCallback } from 'react'
 import {
   BooleanInput,
   Create,
@@ -8,18 +8,49 @@ import {
   email,
   SimpleForm,
   useTranslate,
+  useMutation,
+  useNotify,
+  useRedirect,
 } from 'react-admin'
 import { Title } from '../common'
 
 const UserCreate = (props) => {
   const translate = useTranslate()
+  const [mutate] = useMutation()
+  const notify = useNotify()
+  const redirect = useRedirect()
   const resourceName = translate('resources.user.name', { smart_count: 1 })
   const title = translate('ra.page.create', {
     name: `${resourceName}`,
   })
+
+  const save = useCallback(
+    async (values) => {
+      try {
+        await mutate(
+          {
+            type: 'create',
+            resource: 'user',
+            payload: { data: values },
+          },
+          { returnPromise: true }
+        )
+        notify('resources.user.notifications.created', 'info', {
+          smart_count: 1,
+        })
+        redirect('/user')
+      } catch (error) {
+        if (error.body.errors) {
+          return error.body.errors
+        }
+      }
+    },
+    [mutate, notify, redirect]
+  )
+
   return (
     <Create title={<Title subTitle={title} />} {...props}>
-      <SimpleForm redirect="list" variant={'outlined'}>
+      <SimpleForm save={save} variant={'outlined'}>
         <TextInput source="userName" validate={[required()]} />
         <TextInput source="name" validate={[required()]} />
         <TextInput source="email" validate={[email()]} />

ui/src/user/UserEdit.js

@@ -87,7 +87,9 @@ const UserEdit = (props) => {
           },
           { returnPromise: true }
         )
-        notify('ra.notification.updated', 'info', { smart_count: 1 })
+        notify('resources.user.notifications.updated', 'info', {
+          smart_count: 1,
+        })
         permissions === 'admin' ? redirect('/user') : refresh()
       } catch (error) {
         if (error.body.errors) {