Fixed issue #18936: User count in group is not OK after deleting a user

application/controllers/UserManagementController.php

@@ -371,7 +371,7 @@ public function actionDeleteUser()
             }
         }
 
-        $message = '';
+        $messages = [];
         $transferTo = Yii::app()->request->getPost('transfer_surveys_to');
 
         if (empty($transferTo)) {
@@ -405,31 +405,18 @@ public function actionDeleteUser()
             $iSurveysTransferred = Survey::model()->updateAll(array('owner_id' => $transferTo), 'owner_id=' . $userId);
             if ($iSurveysTransferred) {
                 $sTransferredTo = User::model()->findByPk($transferTo)->users_name;
-                $message = sprintf(gT("All of the user's surveys were transferred to %s."), $sTransferredTo) . " ";
+                $messages[] = sprintf(gT("All of the user's surveys were transferred to %s."), $sTransferredTo);
             }
         }
 
-        $siteAdminName = User::model()->findByPk(1)->users_name;
+        $userManager = new UserManager();
+        $result = $userManager->deleteUser($userId);
+        $messages = array_merge($messages, $result->getRawMessages());
 
-        // Transfer any User Groups owned by this user to site's admin
-        $userGroupsTranferred = UserGroup::model()->updateAll(['owner_id' => 1], 'owner_id = :owner_id', [':owner_id' => $userId]);
-        if ($userGroupsTranferred) {
-            $message .= sprintf(gT("All of the user's user groups were transferred to %s."), $siteAdminName) . " ";
-        }
-
-        // Transfer any Participants owned by this user to site's admin
-        $participantsTranferred = Participant::model()->updateAll(['owner_uid' => 1], 'owner_uid = :owner_uid', [':owner_uid' => $userId]);
-        if ($participantsTranferred) {
-            $message .= sprintf(gT("All participants owned by this user were transferred to %s."), $siteAdminName) . " ";
-        }
-
-        //todo REFACTORING user permissions should be deleted also ... (in table permissions)
-        $oUser->delete();
-        $message .= gT("User successfully deleted.");
         return App()->getController()->renderPartial('/admin/super/_renderJson', [
             'data' => [
-                'success' => true,
-                'message' => $message,
+                'success' => $result->isSuccess(),
+                'message' => implode(" ", $messages),
             ]
         ]);
     }
@@ -1289,11 +1276,9 @@ public function deleteUser(int $uid): bool
             return false;
         }
 
-        // Transfer any Participants owned by this user to site's admin
-        Participant::model()->updateAll(['owner_uid' => 1], 'owner_uid = :owner_uid', [':owner_uid' => $userId]);
-
-        //todo REFACTORING user permissions should be deleted also ... (in table permissions)
-        return $oUser->delete();
+        $userManager = new UserManager();
+        $result = $userManager->deleteUser($userId);
+        return $result->isSuccess();
     }
 
     /**

application/datavalueobjects/OperationResult.php

@@ -0,0 +1,120 @@
+<?php
+
+namespace LimeSurvey\Datavalueobjects;
+
+/**
+ * Class OperationResult
+ *
+ * This class represents the result of an operation (ie. the result of a
+ * service's method), with a status and an optional list of messages.
+ *
+ * @package LimeSurvey\Datavalueobjects
+ */
+class OperationResult
+{
+    /** @var bool the basic result of the operation */
+    private $success;
+
+    /** @var TypedMessage[] an array of messages providing extra details */
+    private $messages;
+
+    /**
+     * @param bool $success
+     * @param TypedMessage[]|TypedMessage $messages
+     */
+    public function __construct($success = false, $messages = []) {
+        $this->success = $success;
+        if (!is_array($messages)) {
+            $messages = [$messages];
+        }
+        $this->messages = $messages;
+    }
+
+    /**
+     * @return bool
+     */
+    public function isSuccess(): bool
+    {
+        return $this->success;
+    }
+
+    /**
+     * @param bool $success
+     */
+    public function setSuccess(bool $success): void
+    {
+        $this->success = $success;
+    }
+
+    /**
+     * Returns the messages of the given type, or all messages if
+     * no type is specified.
+     * @param string|null $type
+     * @return TypedMessage[]
+     */
+    public function getMessages($type = null)
+    {
+        if ($type === null) {
+            return $this->messages;
+        }
+        $messages = [];
+        foreach ($this->messages as $message) {
+            if ($message->getType() === $type) {
+                $messages[] = $message;
+            }
+        }
+        return $messages;
+    }
+
+    /**
+     * @param TypedMessage[] $messages
+     */
+    public function setMessages($messages)
+    {
+        $this->messages = $messages;
+    }
+
+    /**
+     * @param TypedMessage $message
+     */
+    public function appendMessage(TypedMessage $message)
+    {
+        $this->messages[] = $message;
+    }
+
+    /**
+     * @param string $message
+     * @param string $type
+     */
+    public function addMessage($message, $type)
+    {
+        $this->appendMessage(new TypedMessage($message, $type));
+    }
+
+    /**
+     * Sets messages from an array of strings
+     * @param string[] $messages
+     */
+    public function setRawMessages($messages)
+    {
+        $this->messages = [];
+        foreach ($messages as $message) {
+            $this->messages[] = new TypedMessage($message);
+        }
+    }
+
+    /**
+     * Returns the raw messages of the given type, or all messages if
+     * no type is specified.
+     * @param string|null $type
+     * @return string[]
+     */
+    public function getRawMessages($type = null)
+    {
+        $messages = [];
+        foreach ($this->getMessages($type) as $message) {
+            $messages[] = $message->getMessage();
+        }
+        return $messages;
+    }
+}

application/datavalueobjects/TypedMessage.php

@@ -0,0 +1,61 @@
+<?php
+
+namespace LimeSurvey\Datavalueobjects;
+
+/**
+ * Class TypedMessage
+ *
+ * This class represents a message with a type (ie. success, error, etc.).
+ *
+ * @package LimeSurvey\Datavalueobjects
+ */
+class TypedMessage
+{
+    /** @var string the type of the message */
+    private $type;
+
+    /** @var string the message */
+    private $message;
+
+    /**
+     * @param string $type
+     * @param string $message
+     */
+    public function __construct($message, $type = '')
+    {
+        $this->message = $message;
+        $this->type = $type;
+    }
+
+    /**
+     * @return string
+     */
+    public function getType(): string
+    {
+        return $this->type;
+    }
+
+    /**
+     * @param string $type
+     */
+    public function setType(string $type): void
+    {
+        $this->type = $type;
+    }
+
+    /**
+     * @return string
+     */
+    public function getMessage(): string
+    {
+        return $this->message;
+    }
+
+    /**
+     * @param string $message
+     */
+    public function setMessage(string $message): void
+    {
+        $this->message = $message;
+    }
+}

application/models/services/UserManager.php

@@ -5,6 +5,8 @@
 use LSWebUser;
 use Permission;
 use User;
+use LimeSurvey\Datavalueobjects\OperationResult;
+use LimeSurvey\Datavalueobjects\TypedMessage;
 
 /**
  * Service class for managing users and their permissions
@@ -18,11 +20,11 @@ class UserManager
     private $targetUser;
 
     /**
-     * @param LSWebUser $managingUser
+     * @param LSWebUser|null $managingUser
      * @param User|null $targetUser
      */
     public function __construct(
-        LSWebUser $managingUser,
+        LSWebUser $managingUser = null,
         User $targetUser = null
     ) {
         $this->managingUser = $managingUser;
@@ -83,4 +85,57 @@ public function canEdit()
                 && $this->targetUser->parent_id == $this->managingUser->id
             );
     }
+
+    /**
+     * Deletes the user with the given id.
+     * @param int $userId
+     * @return OperationResult
+     */
+    public function deleteUser($userId)
+    {
+        $messages = [];
+
+        $siteAdminName = \User::model()->findByPk(1)->users_name;
+
+        $transaction = \Yii::app()->db->beginTransaction();
+        try {
+            // Transfer any User Groups owned by this user to site's admin
+            $userGroupsTranferred = \UserGroup::model()->updateAll(['owner_id' => 1], 'owner_id = :owner_id', [':owner_id' => $userId]);
+            if ($userGroupsTranferred) {
+                $messages[] = new TypedMessage(sprintf(gT("All of the user's user groups were transferred to %s."), $siteAdminName), 'success');
+            }
+
+            // Transfer any Participants owned by this user to site's admin
+            $participantsTranferred = \Participant::model()->updateAll(['owner_uid' => 1], 'owner_uid = :owner_uid', [':owner_uid' => $userId]);
+            if ($participantsTranferred) {
+                $messages[] = new TypedMessage(sprintf(gT("All participants owned by this user were transferred to %s."), $siteAdminName), 'success');
+            }
+
+            // Remove from groups
+            $userAndGroupRelations = \UserInGroup::model()->findAllByAttributes(['uid' => $userId]);
+            if (count($userAndGroupRelations)) {
+                foreach ($userAndGroupRelations as $userAndGroupRelation) {
+                    $userAndGroupRelation->delete();
+                }
+            }
+
+            // TODO: User permissions should be deleted also...
+
+            // Delete the user
+            $user = \User::model()->findByPk($userId);
+            $success = $user->delete();
+            if (!$success) {
+                $messages = [new TypedMessage(gT("User could not be deleted."), 'error')];
+                $transaction->rollback();
+            } else {
+                $messages[] = new TypedMessage(gT("User successfully deleted."), 'success');
+                $transaction->commit();
+            }
+        } catch (\Exception $e) {
+            $transaction->rollback();
+            $messages = [new TypedMessage(gT("An error occurred while deleting the user."), 'error')];
+            $success = false;
+        }
+        return new OperationResult($success, $messages);
+    }
 }