GroundApps / ShoppingList_Backend

Simple Centralized Shoppinglist - php backend
GNU General Public License v3.0
27 stars 23 forks source link

Error handling on multiple* functions. #51

Closed jklmnn closed 8 years ago

jklmnn commented 8 years ago

I went throught the database connectors and noticed that the error handling on multipleSave/multipleDelete is misleading. You iterate over all items and overwrite the return value each time. So the return status depends only on the last action.

J-8 commented 8 years ago

I think you proposed that earlier, but it got forgotten. It could give back something like {'itemTitle':item, 'success':true/false}

jklmnn commented 8 years ago

I would give back a return object in an array, like: [{'itemTitle'='<item>', 'status'={'type'=<error code>, 'content'='message'}},...].

J-8 commented 8 years ago

I am not sure if that is necessary for the app. I would leave the list open with the checked items to add and display a symbol for the save status. But I think it is a great idea to include the error msg, but it would proably only be useful for debugging.

jklmnn commented 8 years ago

I just want to keep the error messages/codes consistent. Also, we could just write a wrapper with this kind of return that executes the save method for each item which would improve the code size.

J-8 commented 8 years ago

My proposal is an array like this: {'itemTitle'='', 'error'={'type'=, 'content'='message'}, 'status'='true/false'} That way error messages are consistent and the app can just display which item could not be saved in a user friendly manner and when you click the error symbol, the error message could be displayed.

jklmnn commented 8 years ago

Why don't you just take the type from error? That contains exactly the same information as status?

jklmnn commented 8 years ago

The type is also returned on success.

J-8 commented 8 years ago

Oh man, I shouldn't be allowed to comment when I'm sick. Yeah, of course, you are right. Once again ;)

jklmnn commented 8 years ago

Ok, so I will change that to [{'itemTitle'='', 'error'={'type'=, 'content'='message'}}, {...},...].

J-8 commented 8 years ago

Perfect!

jklmnn commented 8 years ago

Should be fixed with 7044f871ad1d2efc2deb391d783ca7e91b99aeac.