Handle mandatory components in recipe JSON smoothly #973
Conversation
Signed-off-by: Christian Wolf <[email protected]>
Signed-off-by: Christian Wolf <[email protected]>
Signed-off-by: Christian Wolf <[email protected]>
Signed-off-by: Christian Wolf <[email protected]>
Signed-off-by: Christian Wolf <[email protected]>
|
This PR just focuses on the backend part. The mentioned frontend issues as suggested in #923 with red border and subtext etc must be implemented in the frontend. This is considered an additional feature and should be handled in a dedicated feature request. |
Codecov Report
@@ Coverage Diff @@
## master #973 +/- ##
==========================================
+ Coverage 28.95% 29.59% +0.63%
==========================================
Files 30 31 +1
Lines 1547 1561 +14
==========================================
+ Hits 448 462 +14
Misses 1099 1099
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Signed-off-by: Christian Wolf <[email protected]>
… best practices Signed-off-by: Christian Wolf <[email protected]>
…ame-exception Signed-off-by: Christian Wolf <[email protected]>
lib/Controller/RecipeController.php
Outdated
| 'file' => $ex->getFile(), | ||
| 'line' => $ex->getLine(), | ||
| ]; | ||
| return new JSONResponse($json, Http::STATUS_NOT_ACCEPTABLE); |
There was a problem hiding this comment.
Is this really the correct status code to return here? Isn’t this more like a 400?
There was a problem hiding this comment.
Yes, you are right. 400 seems more suited. I do not like the 400 as it is items used as a catch all code. But we do not use it currently as far as I know.
I will update soon.
There was a problem hiding this comment.
I fixed this.
There was a problem hiding this comment.
I understood your intention, but looking through the 4xx error codes on https://developer.mozilla.org/en-US/docs/Web/HTTP/Status only 422 seemed suited but is restricted to WebDAV. we could misuse it though to have a more specific code...
There was a problem hiding this comment.
Would you prefer the 400 or the 422? I'd maybe even go for the 422 as we are building the API in any case. So I would maybe even vote for 422. I will wait with merging for your thoughts.
We can of course say that for that specific endpoint (/api/recipe/<id>/image) the 400 is sufficient and we provide an error message as well...
There was a problem hiding this comment.
I’d support either way, take your pick ;)
Signed-off-by: Christian Wolf <[email protected]>
Signed-off-by: Christian Wolf <[email protected]>
Closes #923
This introduces a new possible status in the case of storing a recipe (both new and existing). If the name was empty, the recipe could not be stored and thus a 406 is returned by the server.
It can easily be extended for other mandatory entries in the recipe description.