Bug 27205 - Hold routes are not dealing with invalid pickup locations
Summary: Hold routes are not dealing with invalid pickup locations
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 27209
Blocks: 18729
  Show dependency treegraph
 
Reported: 2020-12-11 12:14 UTC by Jonathan Druart
Modified: 2020-12-28 18:45 UTC (History)
7 users (show)

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


Attachments
Bug 27205: Check valid pickup location on PUT /holds/:hold_id (8.54 KB, patch)
2020-12-11 20:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27205: Check valid pickup location on PUT /holds/:hold_id (8.30 KB, patch)
2020-12-14 10:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27205: Check valid pickup location on PUT /holds/:hold_id (8.35 KB, patch)
2020-12-14 12:34 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 27205: Check valid pickup location on POST /holds (10.02 KB, patch)
2020-12-14 13:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27205: Check valid pickup location on POST /holds (10.07 KB, patch)
2020-12-14 13:47 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 27205: Check valid pickup location on PUT /holds/:hold_id (8.42 KB, patch)
2020-12-16 15:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27205: Check valid pickup location on POST /holds (10.14 KB, patch)
2020-12-16 15:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27205: Improve logic readability in conditional (1.39 KB, patch)
2020-12-18 10:56 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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