[#496] Review error response codes in task operations.

Web services return the code contained in the first OperationResult. Codes have been reviewed to use 400 (bad request) and 403 (forbidden) instead of 500 when they make more sense, which is most known error cases.
Igalia · Mar 8, 2023 · 82d4d97 · 82d4d97
1 parent ddbeb19
commit 82d4d97
Show file tree

Hide file tree

Showing 7 changed files with 24 additions and 21 deletions.
diff --git a/model/dao/TaskDAO/PostgreSQLTaskDAO.php b/model/dao/TaskDAO/PostgreSQLTaskDAO.php
@@ -707,13 +707,14 @@ public function partialUpdate(DirtyTaskVO $taskVO) {
             $resultMessage = "Task update failed:\n";
             if(strpos($errorMessage, "end_after_init_task")) {
                 $resultMessage .= "Start time later than end time.";
+                $result->setResponseCode(400);
             }
             else {
                 $resultMessage .= $errorMessage;
+                $result->setResponseCode(500);
             }
             $result->setMessage($resultMessage);
             $result->setIsSuccessful(false);
-            $result->setResponseCode(500);
         }
         else if (pg_affected_rows($res) == 1) {
             $result->setIsSuccessful(true);
@@ -742,7 +743,7 @@ public function batchPartialUpdate($tasks) {
             $result = new OperationResult(false);
             $result->setErrorNumber(10);
             $result->setMessage("Task update failed:\nDetected overlapping times.");
-            $result->setResponseCode(500);
+            $result->setResponseCode(400);
             return array($result);
         }
 
@@ -910,21 +911,23 @@ private function createInternal(TaskVO $taskVO) {
             $resultMessage = "Error creating task:\n";
             if(strpos($errorMessage, "end_after_init_task")) {
                 $resultMessage .= "Start time later than end time.";
+                $result->setResponseCode(400);
             }
             else if(strpos($errorMessage, "Not null violation")) {
                 $resultMessage .= "Missing compulsory data";
                 if(!$taskVO->getInit() || !$taskVO->getEnd()) {
                     $resultMessage .= ": init and/or end time";
                 }
                 $resultMessage .= ".";
+                $result->setResponseCode(400);
             }
             else {
                 $resultMessage .= $errorMessage;
+                $result->setResponseCode(500);
             }
             $result->setErrorNumber($ex->getCode());
             $result->setMessage($resultMessage);
             $result->setIsSuccessful(false);
-            $result->setResponseCode(500);
         }
 
         return $result;
@@ -942,7 +945,7 @@ public function batchCreate($tasks) {
             $result = new OperationResult(false);
             $result->setErrorNumber(10);
             $result->setMessage("Task creation failed:\nDetected overlapping times.");
-            $result->setResponseCode(500);
+            $result->setResponseCode(400);
             return array($result);
         }
 

diff --git a/model/facade/action/CreateTasksAction.php b/model/facade/action/CreateTasksAction.php
@@ -82,7 +82,7 @@ protected function doExecute() {
             if (!$configDao->isWriteAllowedForDate($task->getDate())) {
                 $result = new OperationResult(false);
                 $result->setErrorNumber(20);
-                $result->setResponseCode(500);
+                $result->setResponseCode(403);
                 $result->setMessage("Error creating task:\nNot allowed to write to date.");
                 $discardedResults[] = $result;
                 $discardedTasks[] = $task;
@@ -93,7 +93,7 @@ protected function doExecute() {
             if (!$projectVO || !$projectVO->getActivation()) {
                 $result = new OperationResult(false);
                 $result->setErrorNumber(30);
-                $result->setResponseCode(500);
+                $result->setResponseCode(403);
                 $result->setMessage("Error creating task:\nNot allowed to write to project.");
                 $discardedTasks[] = $task;
                 $discardedResults[] = $result;

diff --git a/model/facade/action/DeleteReportAction.php b/model/facade/action/DeleteReportAction.php
@@ -77,7 +77,7 @@ protected function doExecute() {
         if(!$configDao->isWriteAllowedForDate($this->task->getDate())) {
             $result = new OperationResult(false);
             $result->setErrorNumber(20);
-            $result->setResponseCode(500);
+            $result->setResponseCode(403);
             $result->setMessage("Error deleting task:\nNot allowed to write to date.");
             return $result;
         }
@@ -87,7 +87,7 @@ protected function doExecute() {
         if (!$dao->checkTaskUserId($this->task->getId(), $this->task->getUserId())) {
             $result = new OperationResult(false);
             $result->setErrorNumber(50);
-            $result->setResponseCode(500);
+            $result->setResponseCode(403);
             $result->setMessage("Error deleting task:\nBelongs to a different user.");
             return $result;
         }
@@ -99,7 +99,7 @@ protected function doExecute() {
         if (!$projectVO || !$projectVO->getActivation()) {
             $result = new OperationResult(false);
             $result->setErrorNumber(30);
-            $result->setResponseCode(500);
+            $result->setResponseCode(403);
             $result->setMessage("Error updating task:\nNot allowed to write to project.");
             return $result;
         }

diff --git a/model/facade/action/PartialUpdateTasksAction.php b/model/facade/action/PartialUpdateTasksAction.php
@@ -88,7 +88,7 @@ protected function doExecute() {
                 if(!$configDao->isWriteAllowedForDate($task->getDate())) {
                     $result = new OperationResult(false);
                     $result->setErrorNumber(20);
-                    $result->setResponseCode(500);
+                    $result->setResponseCode(403);
                     $result->setMessage("Error updating task:\nNot allowed to write to date.");
                     $discardedResults[] = $result;
                     $discardedTasks[] = $task;
@@ -101,7 +101,7 @@ protected function doExecute() {
             if (!isset($oldTask)) {
                 $result = new OperationResult(false);
                 $result->setErrorNumber(40);
-                $result->setResponseCode(500);
+                $result->setResponseCode(400);
                 $result->setMessage("Error updating task:\nTask does not exist.");
                 $discardedResults[] = $result;
                 $discardedTasks[] = $task;
@@ -113,7 +113,7 @@ protected function doExecute() {
             if(!$configDao->isWriteAllowedForDate($oldTask->getDate())) {
                 $result = new OperationResult(false);
                 $result->setErrorNumber(20);
-                $result->setResponseCode(500);
+                $result->setResponseCode(403);
                 $result->setMessage("Error updating task:\nNot allowed to write to date.");
                 $discardedResults[] = $result;
                 $discardedTasks[] = $task;
@@ -126,7 +126,7 @@ protected function doExecute() {
                         $task->getId(), $task->getUserId())) {
                 $result = new OperationResult(false);
                 $result->setErrorNumber(50);
-                $result->setResponseCode(500);
+                $result->setResponseCode(403);
                 $result->setMessage("Error updating task:\nBelongs to a different user.");
                 $discardedResults[] = $result;
                 $discardedTasks[] = $task;
@@ -141,7 +141,7 @@ protected function doExecute() {
                 if (!$projectVO || !$projectVO->getActivation()) {
                     $result = new OperationResult(false);
                     $result->setErrorNumber(30);
-                    $result->setResponseCode(500);
+                    $result->setResponseCode(403);
                     $result->setMessage("Error updating task:\nNot allowed to write to project.");
                     $discardedResults[] = $result;
                     $discardedTasks[] = $task;
@@ -156,7 +156,7 @@ protected function doExecute() {
             if (!$projectVO || !$projectVO->getActivation()) {
                 $result = new OperationResult(false);
                 $result->setErrorNumber(30);
-                $result->setResponseCode(500);
+                $result->setResponseCode(403);
                 $result->setMessage("Error updating task:\nNot allowed to write to project.");
                 $discardedResults[] = $result;
                 $discardedTasks[] = $task;

diff --git a/web/services/createTasksService.php b/web/services/createTasksService.php
@@ -230,8 +230,8 @@
             return (!$item->getIsSuccessful());
         });
         if ($errors) {
-            //if multiple failures, let's just return a 500
-            http_response_code(500);
+            // if multiple failures, just return the code of the first one
+            http_response_code($errors[0]->getResponseCode());
             $string = "<return service='createTasks'><errors>";
             foreach((array) $errors as $result){
                 if (!$result->getIsSuccessful())

diff --git a/web/services/deleteTasksService.php b/web/services/deleteTasksService.php
@@ -131,8 +131,8 @@
             return (!$item->getIsSuccessful());
         });
         if ($errors) {
-            //if multiple failures, let's just return a 500
-            http_response_code(500);
+            // if multiple failures, just return the code of the first one
+            http_response_code($errors[0]->getResponseCode());
             $string = "<return service='deleteTasks'><errors>";
             foreach((array) $errors as $result){
                 if (!$result->getIsSuccessful())

diff --git a/web/services/updateTasksService.php b/web/services/updateTasksService.php
@@ -271,8 +271,8 @@
             return (!$item->getIsSuccessful());
         });
         if ($errors) {
-            //if multiple failures, let's just return a 500
-            http_response_code(500);
+            // if multiple failures, just return the code of the first one
+            http_response_code($errors[0]->getResponseCode());
             $string = "<return service='updateTasks'><errors>";
             foreach((array) $errors as $result){
                 if (!$result->getIsSuccessful())