Bug 27205

Summary: Hold routes are not dealing with invalid pickup locations
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Hold requestsAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: andrewfh, caroline.cyr-la-rose, fridolin.somers, gmcharlt, jonathan.druart, martin.renvoize, tomascohen, victor
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.01,20.05.07
Bug Depends on: 27209    
Bug Blocks: 18729, 27865    
Attachments: Bug 27205: Check valid pickup location on PUT /holds/:hold_id
Bug 27205: Check valid pickup location on PUT /holds/:hold_id
Bug 27205: Check valid pickup location on PUT /holds/:hold_id
Bug 27205: Check valid pickup location on POST /holds
Bug 27205: Check valid pickup location on POST /holds
Bug 27205: Check valid pickup location on PUT /holds/:hold_id
Bug 27205: Check valid pickup location on POST /holds
Bug 27205: Improve logic readability in conditional

Description Jonathan Druart 2020-12-11 12:14:10 UTC
It's possible to PUT or POST /holds with invalid pickup locations.

As the staff cannot overwrite invalid pickup locations (to confirmed), we should raise an exception from AddReserve and ModReserve if called with invalid pickup locations.
Comment 1 Tomás Cohen Arazi 2020-12-11 20:57:17 UTC
Created attachment 114358 [details] [review]
Bug 27205: Check valid pickup location on PUT /holds/:hold_id

This patch adds a test for valid pickup locations when updating a hold
through the API.

Tests are adjusted to reflect this change.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2020-12-11 20:58:34 UTC
I submit this patch for review. If the approach is accepted, I will adjust the POST route too. The tests are obviously the difficult bit.
Comment 3 Victor Grousset/tuxayo 2020-12-12 14:57:04 UTC
Even after restarting the services, the tests don't pass:

kohadev-koha@kohadevbox:/kohadevbox/koha$ prove t/db_dependent/api/v1/holds.t
t/db_dependent/api/v1/holds.t .. 10/11 
    #   Failed test '400 Bad Request'
    #   at t/db_dependent/api/v1/holds.t line 817.
    #          got: '500'
    #     expected: '400'

    #   Failed test '400 Bad Request'
    #   at t/db_dependent/api/v1/holds.t line 850.
    #          got: '500'
    #     expected: '400'
    # Looks like you failed 2 tests of 12.
t/db_dependent/api/v1/holds.t .. 11/11 
#   Failed test 'edit() tests'
#   at t/db_dependent/api/v1/holds.t line 868.
# Looks like you failed 1 test of 11.
t/db_dependent/api/v1/holds.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/11 subtests 

Test Summary Report
-------------------
t/db_dependent/api/v1/holds.t (Wstat: 256 Tests: 11 Failed: 1)
  Failed test:  11
  Non-zero exit status: 1
Comment 4 Tomás Cohen Arazi 2020-12-14 10:56:10 UTC
(In reply to Victor Grousset/tuxayo from comment #3)
> Even after restarting the services, the tests don't pass:

Sorry for that, it was a missing dependency on the bug. Anyway, I updated the dependency, and this bug, so better start over.
Comment 5 Tomás Cohen Arazi 2020-12-14 10:56:44 UTC
Created attachment 114375 [details] [review]
Bug 27205: Check valid pickup location on PUT /holds/:hold_id

This patch adds a test for valid pickup locations when updating a hold
through the API.

Tests are adjusted to reflect this change.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Victor Grousset/tuxayo 2020-12-14 12:34:19 UTC
Created attachment 114377 [details] [review]
Bug 27205: Check valid pickup location on PUT /holds/:hold_id

This patch adds a test for valid pickup locations when updating a hold
through the API.

Tests are adjusted to reflect this change.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 7 Tomás Cohen Arazi 2020-12-14 13:22:38 UTC
Created attachment 114382 [details] [review]
Bug 27205: Check valid pickup location on POST /holds

This patch adds a check for valid pickup location to the POST /holds
route. A 400 code is returned if the supplied pickup library is not
valid.

To test:
1. Apply this patch
2. Run;
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 8 Victor Grousset/tuxayo 2020-12-14 13:47:08 UTC
Created attachment 114384 [details] [review]
Bug 27205: Check valid pickup location on POST /holds

This patch adds a check for valid pickup location to the POST /holds
route. A 400 code is returned if the supplied pickup library is not
valid.

To test:
1. Apply this patch
2. Run;
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 9 Kyle M Hall 2020-12-16 15:14:49 UTC
Created attachment 114446 [details] [review]
Bug 27205: Check valid pickup location on PUT /holds/:hold_id

This patch adds a test for valid pickup locations when updating a hold
through the API.

Tests are adjusted to reflect this change.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Kyle M Hall 2020-12-16 15:15:00 UTC
Created attachment 114447 [details] [review]
Bug 27205: Check valid pickup location on POST /holds

This patch adds a check for valid pickup location to the POST /holds
route. A 400 code is returned if the supplied pickup library is not
valid.

To test:
1. Apply this patch
2. Run;
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Jonathan Druart 2020-12-18 10:56:54 UTC
Created attachment 114512 [details] [review]
Bug 27205: Improve logic readability in conditional
Comment 12 Jonathan Druart 2020-12-18 10:57:46 UTC
Tomas, do you agree it reads better like that?

-        unless (
-            !defined $pickup_library_id
-            or $hold->is_pickup_location_valid(
-                { library_id => $pickup_library_id }
-            )
+        if (
+            defined $pickup_library_id
+            and not $hold->is_pickup_location_valid({ library_id => $pickup_library_id })
Comment 13 Tomás Cohen Arazi 2020-12-18 11:01:12 UTC
(In reply to Jonathan Druart from comment #12)
> Tomas, do you agree it reads better like that?

+1
Comment 14 Jonathan Druart 2020-12-21 09:20:31 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 15 Fridolin Somers 2020-12-28 13:43:25 UTC
Pushed to 20.11.x for 20.11.01
Comment 16 Andrew Fuerste-Henry 2020-12-28 15:06:12 UTC
Pushed to 20.05.x for 20.05.07
Comment 17 Victor Grousset/tuxayo 2020-12-28 18:45:33 UTC
Missing dependencies for 19.11.x, can't backport.

Send help to bug 27209 if you need this one in 19.11.x