Skip to content

Commit

Permalink
[#496] Return more detailed errors on task update.
Browse files Browse the repository at this point in the history 
We build an OperationResult for every situation where a task update is
denied, and we also capture SQL exceptions and wrap them in
OperationResult objects.
  • Loading branch information
@jaragunde
jaragunde committed Mar 6, 2023
1 parent f3f8c5f commit c5b74f1
Show file tree
Hide file tree
Showing 5 changed files with 95 additions and 43 deletions.
44 changes: 30 additions & 14 deletions model/dao/TaskDAO/PostgreSQLTaskDAO.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -634,11 +634,10 @@ public function getVacationsDates(UserVO $userVO, DateTime $initDate = NULL, Dat
* @param DirtyTaskVO $taskVO the {@link TaskVO} with the data we want to
* update on database and the information about which fields must be
* updated.
* @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 partialUpdate(DirtyTaskVO $taskVO) {
$affectedRows = 0;
$result = new OperationResult(false);

if($taskVO->getId() != "") {
$currTaskVO = $this->getById($taskVO->getId());
Expand Down Expand Up @@ -703,11 +702,27 @@ public function partialUpdate(DirtyTaskVO $taskVO) {
$sql = $sql . ", updated_at=now() WHERE id=".$taskVO->getId();

$res = pg_query($this->connect, $sql);
if ($res == NULL) throw new SQLQueryErrorException(pg_last_error());
$affectedRows = pg_affected_rows($res);
if ($res == NULL) {
$errorMessage = pg_last_error();
$resultMessage = "Task update failed:\n";
if(strpos($errorMessage, "end_after_init_task")) {
$resultMessage .= "Start time later than end time.";
}
else {
$resultMessage .= $errorMessage;
}
$result->setMessage($resultMessage);
$result->setIsSuccessful(false);
$result->setResponseCode(500);
}
else if (pg_affected_rows($res) == 1) {
$result->setIsSuccessful(true);
$result->setMessage('Task updated successfully.');
$result->setResponseCode(201);
}
}

return $affectedRows;
return $result;
}

/** Update a batch of DirtyTaskVO objects.
Expand All @@ -720,22 +735,23 @@ public function partialUpdate(DirtyTaskVO $taskVO) {
* @param array $tasks the Task value objects we want to update. Must be
* DirtyTaskVO objects which contain also the information about
* which fields must be updated.
* @return int the number of rows that have been affected. It should match
* the length of $tasks, otherwise there was an error.
* @throws {@link SQLQueryErrorException}, {@link SQLUniqueViolationException}
* @return array OperationResult the array of {@link OperationResult} with information about operation status
*/
public function batchPartialUpdate($tasks) {
if (!$this->checkOverlappingWithDBTasks($tasks)) {
return 0;
$result = new OperationResult(false);
$result->setErrorNumber(10);
$result->setMessage("Task update failed:\nDetected overlapping times.");
$result->setResponseCode(500);
return array($result);
}

$affectedRows = 0;

$results = array();
foreach ($tasks as $task) {
$affectedRows += $this->partialUpdate($task);
$results[] = $this->partialUpdate($task);
}

return $affectedRows;
return $results;
}

/**
Expand Down
6 changes: 2 additions & 4 deletions model/dao/TaskDAO/TaskDAO.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,8 +224,7 @@ public abstract function getVacationsDates(UserVO $userVO, DateTime $initDate =
* @param DirtyTaskVO $taskVO the {@link TaskVO} with the data we want to
* update on database and the information about which fields must be
* updated.
* @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 abstract function partialUpdate(DirtyTaskVO $taskVO);

Expand All @@ -234,8 +233,7 @@ public abstract function partialUpdate(DirtyTaskVO $taskVO);
* Equivalent to {@see partialUpdate} for arrays of tasks.
*
* @param array $tasks array of {@link DirtyTaskVO} objects to be updated.
* @return int the number of rows that have been affected (it should be
* equal to the size of $tasks).
* @return array OperationResult the array of {@link OperationResult} with information about operation
*/
public abstract function batchPartialUpdate($tasks);

Expand Down
8 changes: 3 additions & 5 deletions model/facade/TasksFacade.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,11 +129,10 @@ static function DeleteReports($tasks) {
* @param DirtyTaskVO $task the Task value object we want to update. Must be
* a DirtyTaskVO object which contains also the information about
* which fields must be updated.
* @return int it just indicates if there was any error (<i>-1</i>) or not (<i>0</i>).
* @throws {@link SQLQueryErrorException}, {@link SQLUniqueViolationException}
* @return OperationResult the result {@link OperationResult} with information about operation status
*/
static function PartialUpdateReport(DirtyTaskVO $task) {
return TasksFacade::PartialUpdateReports(array($task));
return TasksFacade::PartialUpdateReports(array($task))[0];
}

/** Partial Update Tasks Function
Expand All @@ -144,8 +143,7 @@ static function PartialUpdateReport(DirtyTaskVO $task) {
* @param array $tasks the Task value objects we want to update. Must be
* a DirtyTaskVO object which contains also the information about
* which fields must be updated.
* @return int it just indicates if there was any error (<i>-1</i>) or not (<i>0</i>).
* @throws {@link SQLQueryErrorException}, {@link SQLUniqueViolationException}
* @return array OperationResult the array of {@link OperationResult} with information about operation status
*/
static function PartialUpdateReports($tasks) {
$action = new PartialUpdateTasksAction($tasks);
Expand Down
62 changes: 47 additions & 15 deletions model/facade/action/PartialUpdateTasksAction.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,20 +72,25 @@ public function __construct($tasks) {
*
* Runs the action itself.
*
* @return int it just indicates if there was any error (<i>-1</i>)
* or not (<i>0</i>).
* @return array OperationResult the array of {@link OperationResult} with information about operation status
*/
protected function doExecute() {
$configDao = DAOFactory::getConfigDAO();
$taskDao = DAOFactory::getTaskDAO();
$projectDAO = DAOFactory::getProjectDAO();
$discardedTasks = array();
$discardedResults = array();

//first check permission on task write
foreach ($this->tasks as $i => $task) {
// Do not allow assigning a task to a locked date
if ($task->isDateDirty()) {
if(!$configDao->isWriteAllowedForDate($task->getDate())) {
$result = new OperationResult(false);
$result->setErrorNumber(20);
$result->setResponseCode(500);
$result->setMessage("Error updating task:\nNot allowed to write to date.");
$discardedResults[] = $result;
$discardedTasks[] = $task;
unset($this->tasks[$i]);
continue;
Expand All @@ -94,16 +99,36 @@ protected function doExecute() {

$oldTask = $taskDao->getById($task->getId());
if (!isset($oldTask)) {
$result = new OperationResult(false);
$result->setErrorNumber(40);
$result->setResponseCode(500);
$result->setMessage("Error updating task:\nTask does not exist.");
$discardedResults[] = $result;
$discardedTasks[] = $task;
unset($this->tasks[$i]);
continue;
}

// Do not allow updating tasks saved in locked dates or belonging
// to a different user
if(!$configDao->isWriteAllowedForDate($oldTask->getDate()) ||
(!$taskDao->checkTaskUserId(
$task->getId(), $task->getUserId()))) {
// Do not allow updating tasks saved in locked dates
if(!$configDao->isWriteAllowedForDate($oldTask->getDate())) {
$result = new OperationResult(false);
$result->setErrorNumber(20);
$result->setResponseCode(500);
$result->setMessage("Error updating task:\nNot allowed to write to date.");
$discardedResults[] = $result;
$discardedTasks[] = $task;
unset($this->tasks[$i]);
continue;
}

// Do not allow updating tasks belonging to a different user
if(!$taskDao->checkTaskUserId(
$task->getId(), $task->getUserId())) {
$result = new OperationResult(false);
$result->setErrorNumber(50);
$result->setResponseCode(500);
$result->setMessage("Error updating task:\nBelongs to a different user.");
$discardedResults[] = $result;
$discardedTasks[] = $task;
unset($this->tasks[$i]);
continue;
Expand All @@ -114,6 +139,11 @@ protected function doExecute() {
$projectId = $task->getProjectId();
$projectVO = $projectDAO->getById($projectId);
if (!$projectVO || !$projectVO->getActivation()) {
$result = new OperationResult(false);
$result->setErrorNumber(30);
$result->setResponseCode(500);
$result->setMessage("Error updating task:\nNot allowed to write to project.");
$discardedResults[] = $result;
$discardedTasks[] = $task;
unset($this->tasks[$i]);
continue;
Expand All @@ -124,8 +154,14 @@ protected function doExecute() {
$projectId = $oldTask->getProjectId();
$projectVO = $projectDAO->getById($projectId);
if (!$projectVO || !$projectVO->getActivation()) {
$result = new OperationResult(false);
$result->setErrorNumber(30);
$result->setResponseCode(500);
$result->setMessage("Error updating task:\nNot allowed to write to project.");
$discardedResults[] = $result;
$discardedTasks[] = $task;
unset($this->tasks[$i]);
continue;
}

if ($task->isInitDirty() & !$task->isEndDirty()) {
Expand Down Expand Up @@ -153,17 +189,13 @@ protected function doExecute() {
}
}

if ($taskDao->batchPartialUpdate($this->tasks) < count($this->tasks)) {
return -1;
}
$results = $taskDao->batchPartialUpdate($this->tasks);

//TODO: do something meaningful with the list of discarded tasks
if (empty($discardedTasks)) {
return 0;
}
else {
return -1;
if (!empty($discardedTasks)) {
return array_merge($discardedResults, $results);
}
return $results;
}

}
18 changes: 13 additions & 5 deletions web/services/updateTasksService.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -266,12 +266,20 @@
} while ($parser->read());


if (count($updateTasks) >= 1)
if (TasksFacade::PartialUpdateReports($updateTasks) == -1) {
http_response_code(500);
$string = "<return service='updateTasks'><success>false</success><error id='1'>There was some error while updating the tasks</error></return>";
$operationResults = TasksFacade::PartialUpdateReports($updateTasks);
$errors = array_filter($operationResults, function ($item) {
return (!$item->getIsSuccessful());
});
if ($errors) {
//if multiple failures, let's just return a 500
http_response_code(500);
$string = "<return service='updateTasks'><errors>";
foreach((array) $errors as $result){
if (!$result->getIsSuccessful())
$string .= "<error id='" . $result->getErrorNumber() . "'>" . $result->getMessage() . "</error>";
}

$string .= "</errors></return>";
}

if (!isset($string))
$string = "<return service='updateTasks'><success>true</success><ok>Operation Success!</ok></return>";
Expand Down

0 comments on commit c5b74f1

Please sign in to comment.