As stated in our coding guidelines for the API [1], when deleting a resource, the API should return a 204 instead of the 200 some do now. [1] https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API#SWAGGER3.2.4_DELETE
References for people reviewing this: - RESTful DELETE: https://restfulapi.net/http-methods/#delete - OpenAPI handling on 204: https://swagger.io/docs/specification/2-0/describing-responses/ In the latter search for 'Empty Response Body'.
Created attachment 102332 [details] [review] Bug 25048: Regression tests This patch adds regression tests for the response bodies and statuses on DELETE actions against existing API routes. This is just enforcing the existing (voted) Coding guidelines for the API (tm). To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/*.t => FAIL: Several routes have problems
Created attachment 102333 [details] [review] Bug 25048: Make successful resource deletion return 204 This patch adapts the spec and the controllers so existing routes return 204 and an empty response body when a successful deletion happens. Right now we have a coding guideline but haven't adapted the existing routes. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/*.t => FAIL: Some tests fail 3. Apply this patch 4. Repeat 2. => SUCCESS: Tests pass! 5. Sign off :-D
+ ->status_is(204, 'SWAGGER3.2.4') + ->content_is('', 'SWAGGER3.3.4'); Can you explain that?
(In reply to Jonathan Druart from comment #4) > + ->status_is(204, 'SWAGGER3.2.4') > + ->content_is('', 'SWAGGER3.3.4'); > > Can you explain that? That refers to the API coding guidelines pointed by the bug. This is something already present in some tests.
Created attachment 102403 [details] [review] Bug 25048: Regression tests This patch adds regression tests for the response bodies and statuses on DELETE actions against existing API routes. This is just enforcing the existing (voted) Coding guidelines for the API (tm). To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/*.t => FAIL: Several routes have problems Signed-off-by: David Nind <david@davidnind.com>
Created attachment 102404 [details] [review] Bug 25048: Make successful resource deletion return 204 This patch adapts the spec and the controllers so existing routes return 204 and an empty response body when a successful deletion happens. Right now we have a coding guideline but haven't adapted the existing routes. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/*.t => FAIL: Some tests fail 3. Apply this patch 4. Repeat 2. => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: David Nind <david@davidnind.com>
Dependency is not signed off yet - moving to BLOCKED. Please unlock when bug 25032 is ready.
(In reply to Katrin Fischer from comment #8) > Dependency is not signed off yet - moving to BLOCKED. Please unlock when bug > 25032 is ready. Dependency unlocked.
Created attachment 103943 [details] [review] Bug 25048: Regression tests This patch adds regression tests for the response bodies and statuses on DELETE actions against existing API routes. This is just enforcing the existing (voted) Coding guidelines for the API (tm). To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/*.t => FAIL: Several routes have problems Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 103944 [details] [review] Bug 25048: Make successful resource deletion return 204 This patch adapts the spec and the controllers so existing routes return 204 and an empty response body when a successful deletion happens. Right now we have a coding guideline but haven't adapted the existing routes. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/*.t => FAIL: Some tests fail 3. Apply this patch 4. Repeat 2. => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Nice work everyone! Pushed to master for 20.05
missing dependencies - not backported