Bug 27593 - Inconsistent return status on club holds routes
Summary: Inconsistent return status on club holds routes
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on: 27330
Blocks:
  Show dependency treegraph
 
Reported: 2021-02-02 13:54 UTC by Tomás Cohen Arazi
Modified: 2021-03-19 00:08 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.04,20.05.10


Attachments
Bug 27593: Regression tests (4.14 KB, patch)
2021-02-02 14:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27593: Consistent behavior for club holds route (1.54 KB, patch)
2021-02-02 14:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27593: Regression tests (4.19 KB, patch)
2021-02-18 17:41 UTC, David Nind
Details | Diff | Splinter Review
Bug 27593: Consistent behavior for club holds route (1.58 KB, patch)
2021-02-18 17:41 UTC, David Nind
Details | Diff | Splinter Review
Bug 27593: Regression tests (4.24 KB, patch)
2021-02-22 15:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27593: Consistent behavior for club holds route (1.65 KB, patch)
2021-02-22 15:53 UTC, Martin Renvoize
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 2021-02-02 13:54:23 UTC
When the requested item doesn't exist it returns a 404. But it returns a 400 if a biblio was requested and is not present.
Comment 1 Tomás Cohen Arazi 2021-02-02 14:27:00 UTC
Created attachment 116216 [details] [review]
Bug 27593: Regression tests

This patch introduces tests for the behaviour change (400 => 404) and
also adds tests for untested (error) behaviours. Notably, the 'Biblio
not found' problematic return value couldn't be triggered, because the
OpenAPI plugin returns a 500 because the response was malformed
(expected { error => 'message' } and it was returning a string).

For the above reason, running the regression tests will show a 500...
instead of the expected 400.

I covered the rest of the codepaths that trigger errors to have full
coverage.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/clubs_holds.t
=> FAIL: Tests fail loudly

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2021-02-02 14:27:04 UTC
Created attachment 116217 [details] [review]
Bug 27593: Consistent behavior for club holds route

This patch:
- Fixes a malformed response when biblio_id is invalid
- Fixes the return status (400 => 404) when biblio_id is invalid
- Adapts the error strings to be consistent with the Cities.pm file

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/clubs_holds.t
=> FAIL: Tests fail
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 David Nind 2021-02-18 17:41:15 UTC
Created attachment 117007 [details] [review]
Bug 27593: Regression tests

This patch introduces tests for the behaviour change (400 => 404) and
also adds tests for untested (error) behaviours. Notably, the 'Biblio
not found' problematic return value couldn't be triggered, because the
OpenAPI plugin returns a 500 because the response was malformed
(expected { error => 'message' } and it was returning a string).

For the above reason, running the regression tests will show a 500...
instead of the expected 400.

I covered the rest of the codepaths that trigger errors to have full
coverage.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/clubs_holds.t
=> FAIL: Tests fail loudly

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 David Nind 2021-02-18 17:41:18 UTC
Created attachment 117008 [details] [review]
Bug 27593: Consistent behavior for club holds route

This patch:
- Fixes a malformed response when biblio_id is invalid
- Fixes the return status (400 => 404) when biblio_id is invalid
- Adapts the error strings to be consistent with the Cities.pm file

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/clubs_holds.t
=> FAIL: Tests fail
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 Martin Renvoize 2021-02-22 15:53:08 UTC
Created attachment 117155 [details] [review]
Bug 27593: Regression tests

This patch introduces tests for the behaviour change (400 => 404) and
also adds tests for untested (error) behaviours. Notably, the 'Biblio
not found' problematic return value couldn't be triggered, because the
OpenAPI plugin returns a 500 because the response was malformed
(expected { error => 'message' } and it was returning a string).

For the above reason, running the regression tests will show a 500...
instead of the expected 400.

I covered the rest of the codepaths that trigger errors to have full
coverage.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/clubs_holds.t
=> FAIL: Tests fail loudly

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2021-02-22 15:53:11 UTC
Created attachment 117156 [details] [review]
Bug 27593: Consistent behavior for club holds route

This patch:
- Fixes a malformed response when biblio_id is invalid
- Fixes the return status (400 => 404) when biblio_id is invalid
- Adapts the error strings to be consistent with the Cities.pm file

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/clubs_holds.t
=> FAIL: Tests fail
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2021-02-22 15:53:49 UTC
Works as expected, a sensible improvement.

QA Script passes, covered by tests.

Passing QA
Comment 8 Jonathan Druart 2021-02-23 12:21:56 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 9 Fridolin Somers 2021-02-25 09:53:17 UTC
Pushed to 20.11.x for 20.11.04
Comment 10 Andrew Fuerste-Henry 2021-03-05 17:31:01 UTC
Pushed to 20.05.x for 20.05.10
Comment 11 Victor Grousset/tuxayo 2021-03-19 00:08:02 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.