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.
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>
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.
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
(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.
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>
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>
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
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>
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>
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>
Created attachment 114512 [details] [review] Bug 27205: Improve logic readability in conditional
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 })
(In reply to Jonathan Druart from comment #12) > Tomas, do you agree it reads better like that? +1
Pushed to master for 21.05, thanks to everybody involved!
Pushed to 20.11.x for 20.11.01
Pushed to 20.05.x for 20.05.07
Missing dependencies for 19.11.x, can't backport. Send help to bug 27209 if you need this one in 19.11.x