In the checks we have: if ( $item_id and $biblio_id ) { .... $biblio = Koha::Biblios->find($biblio_id); ... } $item does not get assigned, then later: my $can_place_hold = $item ? C4::Reserves::CanItemBeReserved( $patron, $item ) : C4::Reserves::CanBookBeReserved( $patron_id, $biblio_id ); So while the item may not be holdable, we check if the bib is, then place the hold using the item_id
Created attachment 157231 [details] [review] Bug 35053: Regression tests
Created attachment 157232 [details] [review] Bug 35053: Make sure request is checked as an item-level request if item_id passed This patch makes the validation code have the `$item` variable defined when checking holdability, in the case both `item_id` and `biblio_id` params are passed. Otherwise, if the requested item is not holdable, but a biblio-level hold *could* be placed, the item-level hold is placed. This is highlighted by the regression tests. To test: 1. Apply the regression tests patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/holds.t => FAIL: Tests don't pass. A request that should be rejected is allowed because biblio-level hold is allowed. 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! Item-level rules are checked and thus the request rejected (code 403) 5. Sign off :-D
Created attachment 157389 [details] [review] Bug 35053: Regression tests Signed-off-by: David Nind <david@davidnind.com>
Created attachment 157390 [details] [review] Bug 35053: Make sure request is checked as an item-level request if item_id passed This patch makes the validation code have the `$item` variable defined when checking holdability, in the case both `item_id` and `biblio_id` params are passed. Otherwise, if the requested item is not holdable, but a biblio-level hold *could* be placed, the item-level hold is placed. This is highlighted by the regression tests. To test: 1. Apply the regression tests patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/holds.t => FAIL: Tests don't pass. A request that should be rejected is allowed because biblio-level hold is allowed. 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! Item-level rules are checked and thus the request rejected (code 403) 5. Sign off :-D Signed-off-by: David Nind <david@davidnind.com>
Created attachment 157424 [details] [review] Bug 35053: Regression tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 157425 [details] [review] Bug 35053: Make sure request is checked as an item-level request if item_id passed This patch makes the validation code have the `$item` variable defined when checking holdability, in the case both `item_id` and `biblio_id` params are passed. Otherwise, if the requested item is not holdable, but a biblio-level hold *could* be placed, the item-level hold is placed. This is highlighted by the regression tests. To test: 1. Apply the regression tests patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/holds.t => FAIL: Tests don't pass. A request that should be rejected is allowed because biblio-level hold is allowed. 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! Item-level rules are checked and thus the request rejected (code 403) 5. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Pushed to master for 23.11. Nice work everyone, thanks!
Pushed to 23.05.x for 23.05.05
Nice work everyone! Pushed to oldstable for 22.11.x
*** Bug 33477 has been marked as a duplicate of this bug. ***