Bug 25048 - Successful resource deletion should return 204
Summary: Successful resource deletion should return 204
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL: https://wiki.koha-community.org/wiki/...
Keywords:
Depends on: 25032
Blocks:
  Show dependency treegraph
 
Reported: 2020-04-02 18:25 UTC by Tomás Cohen Arazi
Modified: 2020-11-30 21:49 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 25048: Regression tests (6.65 KB, patch)
2020-04-02 21:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25048: Make successful resource deletion return 204 (5.84 KB, patch)
2020-04-02 21:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25048: Regression tests (6.70 KB, patch)
2020-04-04 21:50 UTC, David Nind
Details | Diff | Splinter Review
Bug 25048: Make successful resource deletion return 204 (5.88 KB, patch)
2020-04-04 21:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 25048: Regression tests (6.77 KB, patch)
2020-04-29 13:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25048: Make successful resource deletion return 204 (5.95 KB, patch)
2020-04-29 14:00 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2020-04-02 18:25:16 UTC
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
Comment 1 Tomás Cohen Arazi 2020-04-02 18:30:44 UTC
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'.
Comment 2 Tomás Cohen Arazi 2020-04-02 21:51:00 UTC
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
Comment 3 Tomás Cohen Arazi 2020-04-02 21:51:05 UTC
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
Comment 4 Jonathan Druart 2020-04-03 08:52:08 UTC
+      ->status_is(204, 'SWAGGER3.2.4')
+      ->content_is('', 'SWAGGER3.3.4');

Can you explain that?
Comment 5 Tomás Cohen Arazi 2020-04-03 11:41:30 UTC
(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.
Comment 6 David Nind 2020-04-04 21:50:58 UTC
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>
Comment 7 David Nind 2020-04-04 21:51:01 UTC
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>
Comment 8 Katrin Fischer 2020-04-12 19:21:14 UTC
Dependency is not signed off yet - moving to BLOCKED. Please unlock when bug 25032 is ready.
Comment 9 Tomás Cohen Arazi 2020-04-24 12:44:40 UTC
(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.
Comment 10 Kyle M Hall 2020-04-29 13:59:58 UTC
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>
Comment 11 Kyle M Hall 2020-04-29 14:00:19 UTC
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>
Comment 12 Martin Renvoize 2020-04-29 15:25:17 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 13 Joy Nelson 2020-05-08 22:20:37 UTC
missing dependencies - not backported