From e2b6e807c49029bea709179567392520b6081125 Mon Sep 17 00:00:00 2001 From: Cyrus <32754336+cyrusjc@users.noreply.github.com> Date: Wed, 9 Jul 2025 08:12:04 -0700 Subject: [PATCH] [MM-21466] delete users cmd return err (#31191) * refactored error managment for deleteUsersCmdF * updated tests related to deleteUsersCmdF * updated user_e2e_test to reflect changes made to the deleteUsersCmdF function * Empty-Commit to retrigger workflow * applied gofmt formating reqs to user_test.go * added suggested changes to the deleteUsersCmdF regarding error gathering * added requested changes regarding error aggragation on deleteUsersCmdF * style(mmctl): removing trailing whitespace * feat(mmctl): returning errors in deleteUserCmdF on err * fix(mmctl): returns when err parsing args * test(mmctl): updated tests to expect err instead of reading printer * style: updating returned errs * tests: updated test to reflect error change * tests(mmctl): updated e2e DeleteUserCmd test * Update server/cmd/mmctl/commands/user.go Co-authored-by: Ben Schumacher * refactor: changing error to return username instead of email * refactor: changing email to username for errors --------- Co-authored-by: Arnaud Wanet Co-authored-by: Antonis Stamatiou Co-authored-by: Ben Schumacher --- server/cmd/mmctl/commands/user.go | 14 +++++--- server/cmd/mmctl/commands/user_e2e_test.go | 31 +++++++++++------- server/cmd/mmctl/commands/user_test.go | 37 ++++++++++++---------- 3 files changed, 50 insertions(+), 32 deletions(-) diff --git a/server/cmd/mmctl/commands/user.go b/server/cmd/mmctl/commands/user.go index c076eb8da8..65c6809519 100644 --- a/server/cmd/mmctl/commands/user.go +++ b/server/cmd/mmctl/commands/user.go @@ -738,24 +738,30 @@ func deleteUsersCmdF(c client.Client, cmd *cobra.Command, args []string) error { users, err := getUsersFromArgs(c, args) if err != nil { - printer.PrintError(err.Error()) + return err } + + var errs *multierror.Error for i, user := range users { if user == nil { printer.PrintError("Unable to find user '" + args[i] + "'") continue } if res, err := c.PermanentDeleteUser(context.TODO(), user.Id); err != nil { - printer.PrintError("Unable to delete user '" + user.Username + "' error: " + err.Error()) + errs = multierror.Append(errs, + fmt.Errorf("unable to delete user %s error: %w", user.Username, err)) } else { // res.StatusCode is checked for 202 to identify issues with file deletion. if res.StatusCode == http.StatusAccepted { - printer.PrintError("There were issues with deleting profile image of the user. Please delete it manually. Id: " + user.Id) + errs = multierror.Append(errs, + fmt.Errorf("unable to delete the profile image of the user, please delete it manually, id:%s", user.Username)) + continue } printer.PrintT("Deleted user '{{.Username}}'", user) } } - return nil + + return errs.ErrorOrNil() } func deleteAllUsersCmdF(c client.Client, cmd *cobra.Command, args []string) error { diff --git a/server/cmd/mmctl/commands/user_e2e_test.go b/server/cmd/mmctl/commands/user_e2e_test.go index 4e307995d6..b0ff15a9af 100644 --- a/server/cmd/mmctl/commands/user_e2e_test.go +++ b/server/cmd/mmctl/commands/user_e2e_test.go @@ -789,6 +789,9 @@ func (s *MmctlE2ETestSuite) TestDeleteUsersCmd() { printer.Clean() emailArg := "nonexistentUser@example.com" + var expectedErr *multierror.Error + expectedErr = multierror.Append(expectedErr, fmt.Errorf("user %s not found", emailArg)) + previousVal := s.th.App.Config().ServiceSettings.EnableAPIUserDeletion s.th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableAPIUserDeletion = true }) defer func() { @@ -800,10 +803,8 @@ func (s *MmctlE2ETestSuite) TestDeleteUsersCmd() { cmd.Flags().BoolVar(&confirm, "confirm", confirm, "confirm") err := deleteUsersCmdF(c, cmd, []string{emailArg}) - s.Require().Nil(err) - s.Len(printer.GetLines(), 0) - s.Len(printer.GetErrorLines(), 1) - s.Equal(fmt.Sprintf("1 error occurred:\n\t* user %s not found\n\n", emailArg), printer.GetErrorLines()[0]) + s.Require().NotNil(err) + s.Require().EqualError(err, expectedErr.Error()) }) s.Run("Delete user without permission", func() { @@ -820,11 +821,14 @@ func (s *MmctlE2ETestSuite) TestDeleteUsersCmd() { cmd.Flags().BoolVar(&confirm, "confirm", confirm, "confirm") newUser := s.th.CreateUser() + + var expectedErr *multierror.Error + expectedErr = multierror.Append(expectedErr, fmt.Errorf("unable to delete user %s error: %w", newUser.Username, + fmt.Errorf("You do not have the appropriate permissions."))) + err := deleteUsersCmdF(s.th.Client, cmd, []string{newUser.Email}) - s.Require().Nil(err) - s.Len(printer.GetLines(), 0) - s.Len(printer.GetErrorLines(), 1) - s.Require().Equal(fmt.Sprintf("Unable to delete user '%s' error: You do not have the appropriate permissions.", newUser.Username), printer.GetErrorLines()[0]) + s.Require().NotNil(err) + s.Require().EqualError(err, expectedErr.Error()) // expect user not deleted user, err := s.th.App.GetUser(newUser.Id) @@ -846,11 +850,14 @@ func (s *MmctlE2ETestSuite) TestDeleteUsersCmd() { cmd.Flags().BoolVar(&confirm, "confirm", confirm, "confirm") newUser := s.th.CreateUser() + + var expectedErr *multierror.Error + expectedErr = multierror.Append(expectedErr, fmt.Errorf("unable to delete user %s error: %w", newUser.Username, + fmt.Errorf("Permanent user deletion feature is not enabled. Please contact your System Administrator."))) + err := deleteUsersCmdF(s.th.SystemAdminClient, cmd, []string{newUser.Email}) - s.Require().Nil(err) - s.Len(printer.GetLines(), 0) - s.Len(printer.GetErrorLines(), 1) - s.Require().Equal(fmt.Sprintf("Unable to delete user '%s' error: Permanent user deletion feature is not enabled. Please contact your System Administrator.", newUser.Username), printer.GetErrorLines()[0]) + s.Require().NotNil(err) + s.Require().EqualError(err, expectedErr.Error()) // expect user not deleted user, err := s.th.App.GetUser(newUser.Id) diff --git a/server/cmd/mmctl/commands/user_test.go b/server/cmd/mmctl/commands/user_test.go index 3e229ed9f2..8b177cfd80 100644 --- a/server/cmd/mmctl/commands/user_test.go +++ b/server/cmd/mmctl/commands/user_test.go @@ -405,6 +405,9 @@ func (s *MmctlUnitTestSuite) TestDeleteUsersCmd() { printer.Clean() arg := "userdoesnotexist@example.com" + var expectedErr *multierror.Error + expectedErr = multierror.Append(expectedErr, fmt.Errorf("user %s not found", arg)) + s.client. EXPECT(). GetUserByEmail(context.TODO(), arg, ""). @@ -426,9 +429,8 @@ func (s *MmctlUnitTestSuite) TestDeleteUsersCmd() { cmd := &cobra.Command{} cmd.Flags().Bool("confirm", true, "") err := deleteUsersCmdF(s.client, cmd, []string{arg}) - s.Require().Nil(err) - s.Require().Len(printer.GetLines(), 0) - s.Require().Equal(fmt.Sprintf("1 error occurred:\n\t* user %s not found\n\n", arg), printer.GetErrorLines()[0]) + s.Require().Error(err) + s.Require().EqualError(err, expectedErr.Error()) }) s.Run("Delete users should delete users", func() { @@ -470,6 +472,9 @@ func (s *MmctlUnitTestSuite) TestDeleteUsersCmd() { mockError := errors.New("an error occurred on deleting a user") + var expectedErr *multierror.Error + expectedErr = multierror.Append(expectedErr, fmt.Errorf("unable to delete user %s error: %w", mockUser1.Username, mockError)) + s.client. EXPECT(). GetUserByEmail(context.TODO(), email1, ""). @@ -486,10 +491,8 @@ func (s *MmctlUnitTestSuite) TestDeleteUsersCmd() { cmd.Flags().Bool("confirm", true, "") err := deleteUsersCmdF(s.client, cmd, []string{email1}) - s.Require().Nil(err) - s.Require().Len(printer.GetErrorLines(), 1) - s.Require().Equal("Unable to delete user 'User1' error: an error occurred on deleting a user", - printer.GetErrorLines()[0]) + s.Require().EqualError(err, expectedErr.Error()) + s.Require().Len(printer.GetErrorLines(), 0) }) s.Run("Delete two users, first fails with error other passes", func() { @@ -497,6 +500,9 @@ func (s *MmctlUnitTestSuite) TestDeleteUsersCmd() { mockError := errors.New("an error occurred on deleting a user") + var expectedErr *multierror.Error + expectedErr = multierror.Append(expectedErr, fmt.Errorf("unable to delete user %s error: %w", mockUser1.Username, mockError)) + s.client. EXPECT(). GetUserByEmail(context.TODO(), email1, ""). @@ -523,17 +529,18 @@ func (s *MmctlUnitTestSuite) TestDeleteUsersCmd() { cmd.Flags().Bool("confirm", true, "") err := deleteUsersCmdF(s.client, cmd, []string{email1, email2}) - s.Require().Nil(err) - s.Require().Len(printer.GetLines(), 1) - s.Require().Len(printer.GetErrorLines(), 1) + s.Require().NotNil(err) + s.Require().EqualError(err, expectedErr.Error()) s.Require().Equal(&mockUser2, printer.GetLines()[0]) - s.Require().Equal("Unable to delete user 'User1' error: an error occurred on deleting a user", - printer.GetErrorLines()[0]) }) s.Run("partial delete of user, i.e failing to delete profile image gives a warning on the console.", func() { printer.Clean() + var expectedErr *multierror.Error + expectedErr = multierror.Append(expectedErr, + fmt.Errorf("unable to delete the profile image of the user, please delete it manually, id:%s", mockUser1.Username)) + s.client. EXPECT(). GetUserByEmail(context.TODO(), email1, ""). @@ -549,10 +556,8 @@ func (s *MmctlUnitTestSuite) TestDeleteUsersCmd() { cmd.Flags().Bool("confirm", true, "") err := deleteUsersCmdF(s.client, cmd, []string{email1}) - s.Require().Nil(err) - s.Require().Len(printer.GetLines(), 1) - s.Require().Len(printer.GetErrorLines(), 1) - s.Require().Equal(fmt.Sprintf("There were issues with deleting profile image of the user. Please delete it manually. Id: %s", mockUser1.Id), printer.GetErrorLines()[0]) + s.Require().NotNil(err) + s.Require().EqualError(err, expectedErr.Error()) }) }