Report detailed error messages on task update and delete #618

jaragunde · 2023-02-28T13:30:19Z

I'm increasing the error reporting for task update and delete operations, and also making some changes in the front-end to make error messages more noticeable; I'm afraid that users have become accustomed to ignore them.

jaragunde · 2023-03-06T10:56:23Z

I've just fixed some copy-paste in error messages but this is ready to review, I'm not working more on it.

anarute

I've added some minor comments, testing locally it works fine! Thank you

anarute · 2023-03-06T15:04:19Z

model/dao/TaskDAO/PostgreSQLTaskDAO.php

+            }
+            $result->setMessage($resultMessage);
+            $result->setIsSuccessful(false);
+            $result->setResponseCode(500);


I know we've been using 500 everywhere but I wonder if it shouldn't be 400 in this case, what do you think? Here and in the other places where the request fails because the user sent a bad input.

Now you mention, most errors would fall under the "bad request" or "forbidden" umbrellas so it makes sense to review them all...

I like the idea of trying to be more specific with the http result code when we can.

anarute · 2023-03-06T15:08:14Z

model/facade/action/PartialUpdateTasksAction.php

-        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


I think with your changes we can remove this comment? :)

I don't think so, we don't match the errors with the discarded tasks, so we don't report the client which tasks were successfully created and which weren't...

I'm not sure if we'll ever want to do that, it would increase the client complexity quite a lot.

anarute · 2023-03-06T15:09:56Z

model/dao/TaskDAO/PostgreSQLTaskDAO.php

-            if ($res == NULL) throw new SQLQueryErrorException(pg_last_error());
-                $affectedRows = pg_affected_rows($res);
+            $result->setIsSuccessful(true);
+            $result->setMessage('Project deleted successfully.');


I think you meant Task deleted successfully.

And the string of copy-paste errors never ends :)

anarute · 2023-03-06T15:11:59Z

model/facade/action/DeleteReportAction.php

+        if (!$projectVO || !$projectVO->getActivation()) {
+            $result = new OperationResult(false);
+            $result->setErrorNumber(30);
+            $result->setResponseCode(500);


Maybe 403 here?

web/include/phpreport.css

dmtrek14

Other than one small nit (which could be addressed later), I think this looks good. Thanks for getting the detailed error messages implemented for tasks - this is a huge improvement.

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.

The DAO operation is also ported to PDO, addressing #513.

Make "error" font red, add an icon next to it and increase message time on screen.

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.

jaragunde

I've submitted a new patch set amending the existing patches to address your comments. In a new patch I've reviewed all the error codes related to task create, update and delete and make web services pass them instead of using always 500.

jaragunde · 2023-03-06T17:02:05Z

model/dao/TaskDAO/PostgreSQLTaskDAO.php

+            }
+            $result->setMessage($resultMessage);
+            $result->setIsSuccessful(false);
+            $result->setResponseCode(500);


Now you mention, most errors would fall under the "bad request" or "forbidden" umbrellas so it makes sense to review them all...

jaragunde · 2023-03-06T17:03:30Z

model/facade/action/PartialUpdateTasksAction.php

-        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


I don't think so, we don't match the errors with the discarded tasks, so we don't report the client which tasks were successfully created and which weren't...

I'm not sure if we'll ever want to do that, it would increase the client complexity quite a lot.

jaragunde · 2023-03-06T17:06:02Z

model/dao/TaskDAO/PostgreSQLTaskDAO.php

-            if ($res == NULL) throw new SQLQueryErrorException(pg_last_error());
-                $affectedRows = pg_affected_rows($res);
+            $result->setIsSuccessful(true);
+            $result->setMessage('Project deleted successfully.');


And the string of copy-paste errors never ends :)

web/include/phpreport.css

jaragunde · 2023-03-07T10:46:43Z

model/facade/action/DeleteReportAction.php

+        if (!$projectVO || !$projectVO->getActivation()) {
+            $result = new OperationResult(false);
+            $result->setErrorNumber(30);
+            $result->setResponseCode(500);


jaragunde · 2023-03-08T11:57:04Z

Thanks for your reviews! I understand it can be merged already.

jaragunde requested review from anarute and dmtrek14 February 28, 2023 13:30

jaragunde force-pushed the task-error-messages-update-delete branch 3 times, most recently from b1eb1e7 to 148f3d7 Compare March 6, 2023 10:54

anarute approved these changes Mar 6, 2023

View reviewed changes

dmtrek14 reviewed Mar 6, 2023

View reviewed changes

web/include/phpreport.css Show resolved Hide resolved

dmtrek14 approved these changes Mar 6, 2023

View reviewed changes

jaragunde added 4 commits March 7, 2023 11:43

[#496] Return more detailed errors on task update.

eb7eee3

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.

[#496,#513] Return more detailed errors on task delete.

cadc4af

The DAO operation is also ported to PDO, addressing #513.

[#173] Make error messages more noticeable.

6b5ff76

Make "error" font red, add an icon next to it and increase message time on screen.

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

a89be33

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.

jaragunde force-pushed the task-error-messages-update-delete branch from 148f3d7 to a89be33 Compare March 7, 2023 10:46

jaragunde commented Mar 7, 2023

View reviewed changes

jaragunde merged commit 82d4d97 into main Mar 8, 2023

jaragunde deleted the task-error-messages-update-delete branch March 8, 2023 11:57

This was referenced Mar 8, 2023

Do something meaningful with SQL exceptions #496

Open

Improve feedback on error in the application #173

Open

Adopt PDO for database access #513

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report detailed error messages on task update and delete #618

Report detailed error messages on task update and delete #618

jaragunde commented Feb 28, 2023

jaragunde commented Mar 6, 2023

anarute left a comment

anarute Mar 6, 2023

jaragunde Mar 6, 2023

dmtrek14 Mar 6, 2023

anarute Mar 6, 2023

jaragunde Mar 6, 2023

anarute Mar 6, 2023

jaragunde Mar 6, 2023

anarute Mar 6, 2023

jaragunde Mar 7, 2023

dmtrek14 left a comment

jaragunde left a comment

jaragunde Mar 6, 2023

jaragunde Mar 6, 2023

jaragunde Mar 6, 2023

jaragunde Mar 7, 2023

jaragunde commented Mar 8, 2023

Report detailed error messages on task update and delete #618

Report detailed error messages on task update and delete #618

Conversation

jaragunde commented Feb 28, 2023

jaragunde commented Mar 6, 2023

anarute left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmtrek14 left a comment

Choose a reason for hiding this comment

jaragunde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaragunde commented Mar 8, 2023