[#173,#496] Report detailed errors on user delete.

Modify the user delete pipeline (service, facade, DAO) to use OperationResult. HybridUserDAO is changed to reuse the same operation from Postgres DAO, since it did not have any code related to LDAP.
Igalia · Mar 8, 2023 · cc7ebe2 · cc7ebe2
1 parent eb92549
commit cc7ebe2
Show file tree

Hide file tree

Showing 6 changed files with 52 additions and 56 deletions.
diff --git a/model/dao/UserDAO/HybridUserDAO.php b/model/dao/UserDAO/HybridUserDAO.php
@@ -468,29 +468,10 @@ public function create(UserVO $userVO) {
      * This function deletes the data of a User by its {@link UserVO}.
      *
      * @param UserVO $userVO the {@link UserVO} with the data we want to delete from database.
-     * @return int the number of rows that have been affected (it should be 1).
-     * @throws {@link SQLQueryErrorException}
+     * @return OperationResult the result {@link OperationResult} with information about operation status
      */
     public function delete(UserVO $userVO) {
-        $affectedRows = 0;
-
-        // Check for a user ID.
-        if($userVO->getId() >= 0) {
-            $currUserVO = $this->getById($userVO->getId());
-        }
-
-        // Otherwise delete a user.
-        if(sizeof($currUserVO) > 0) {
-            $sql = "DELETE FROM usr WHERE id=".$userVO->getId();
-
-            $res = pg_query($this->connect, $sql);
-
-            if ($res == NULL) throw new SQLQueryErrorException(pg_last_error());
-
-            $affectedRows = pg_affected_rows($res);
-        }
-
-        return $affectedRows;
+        return parent::delete($userVO);
     }
 }
 

diff --git a/model/dao/UserDAO/PostgreSQLUserDAO.php b/model/dao/UserDAO/PostgreSQLUserDAO.php
@@ -503,29 +503,52 @@ public function create(UserVO $userVO) {
      * This function deletes the data of a User by its {@link UserVO}.
      *
      * @param UserVO $userVO the {@link UserVO} with the data we want to delete from database.
-     * @return int the number of rows that have been affected (it should be 1).
-     * @throws {@link SQLQueryErrorException}
+     * @return OperationResult the result {@link OperationResult} with information about operation status
      */
     public function delete(UserVO $userVO) {
-        $affectedRows = 0;
-
-        // Check for a user ID.
-        if($userVO->getId() >= 0) {
-            $currUserVO = $this->getById($userVO->getId());
-        }
-
-        // Otherwise delete a user.
-        if(isset($currUserVO)) {
-            $sql = "DELETE FROM usr WHERE id=".$userVO->getId();
+        $result = new OperationResult(false);
 
-            $res = pg_query($this->connect, $sql);
+        $sql = "DELETE FROM usr WHERE id=:id";
 
-            if ($res == NULL) throw new SQLQueryErrorException(pg_last_error());
+        try {
+            $statement = $this->pdo->prepare($sql);
+            $statement->bindValue(":id", $userVO->getId(), PDO::PARAM_INT);
+            $statement->execute();
 
-            $affectedRows = pg_affected_rows($res);
+            $result->setIsSuccessful(true);
+            $result->setMessage('User updated successfully.');
+            $result->setResponseCode(201);
+        }
+        catch (PDOException $ex) {
+            $errorMessage = $ex->getMessage();
+            $resultMessage = "Error updating user:\n";
+            if(strpos($errorMessage, "Foreign key violation")) {
+                $result->setResponseCode(403);
+                if(strpos($errorMessage, "task")) {
+                    $resultMessage .= "The user has assigned tasks.";
+                }
+                else if(strpos($errorMessage, "project_usr")) {
+                    $resultMessage .= "The user is assigned to projects.";
+                }
+                else if(strpos($errorMessage, "history")) {
+                    $resultMessage .= "There is user history data associated to the user.";
+                }
+                else {
+                    // catch-all for any foreign key violations that we may add in the future
+                    $resultMessage .= "There is data associated to the user.";
+                    error_log($errorMessage);
+                }
+            }
+            else {
+                $resultMessage .= $errorMessage;
+                $result->setResponseCode(500);
+            }
+            $result->setErrorNumber($ex->getCode());
+            $result->setMessage($resultMessage);
+            $result->setIsSuccessful(false);
         }
 
-        return $affectedRows;
+        return $result;
     }
 }
 

diff --git a/model/dao/UserDAO/UserDAO.php b/model/dao/UserDAO/UserDAO.php
@@ -286,8 +286,7 @@ public abstract function create(UserVO $userVO);
      * This function deletes the data of a User by its {@link UserVO}.
      *
      * @param UserVO $userVO the {@link UserVO} with the data we want to delete from database.
-     * @return int the number of rows that have been affected (it should be 1).
-     * @throws {@link OperationErrorException}, {@link InvalidOperationException}
+     * @return OperationResult the result {@link OperationResult} with information about operation status
      */
     public abstract function delete(UserVO $userVO);
 

diff --git a/model/facade/UsersFacade.php b/model/facade/UsersFacade.php
@@ -180,15 +180,11 @@ static function CreateUser(UserVO $user) {
      *  This function is used for deleting a User.
      *
      * @param UserVO $user the User value object we want to delete.
-     * @return int it just indicates if there was any error (<i>-1</i>) or not (<i>0</i>).
-     * @throws {@link SQLQueryErrorException}
+     * @return OperationResult the result {@link OperationResult} with information about operation status
      */
     static function DeleteUser(UserVO $user) {
-
-    $action = new DeleteUserAction($user);
-
-    return $action->execute();
-
+        $action = new DeleteUserAction($user);
+        return $action->execute();
     }
 
     /** Update User Function

diff --git a/model/facade/action/DeleteUserAction.php b/model/facade/action/DeleteUserAction.php
@@ -68,16 +68,11 @@ public function __construct(UserVO $user) {
      *
      * This is the function that contains the code that deletes the User from persistent storing.
      *
-     * @return int it just indicates if there was any error (<i>-1</i>) or not (<i>0</i>).
+     * @return OperationResult the result {@link OperationResult} with information about operation status
      */
     protected function doExecute() {
-
         $dao = DAOFactory::getUserDAO();
-        if ($dao->delete($this->user)!=1) {
-            return -1;
-        }
-
-        return 0;
+        return $dao->delete($this->user);
     }
 
 }

diff --git a/web/services/deleteUsersService.php b/web/services/deleteUsersService.php
@@ -128,9 +128,11 @@
         if (count($deleteUsers) >= 1)
             foreach((array)$deleteUsers as $user)
             {
-                if (UsersFacade::DeleteUser($user) == -1)
-                {
-                    $string = "<return service='deleteUsers'><error id='1'>There was some error while deleting the users</error></return>";
+                $result = UsersFacade::DeleteUser($user);
+                if (!$result->getIsSuccessful()) {
+                    http_response_code($result->getResponseCode());
+                    $string = "<return service='updateUsers'><error id='" . $result->getErrorNumber() . "'>" .
+                            $result->getMessage() . "</error></return>";
                     break;
                 }
             }