Bug 30960 - Koha lets you place item-level holds without a pick-up place
Summary: Koha lets you place item-level holds without a pick-up place
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Assignee: Peter Vashchuk
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-06-14 09:44 UTC by Peter Vashchuk
Modified: 2023-06-08 22:26 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.01


Attachments
Bug 30960: Koha lets to place item-level holds without a pick-up place (5.26 KB, patch)
2022-06-14 09:50 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 30960: Don't allow user to place the hold without a pick-up place (5.24 KB, patch)
2022-06-17 11:41 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 30960: Don't allow user to place the hold without a pick-up place (5.29 KB, patch)
2022-06-17 19:49 UTC, David Nind
Details | Diff | Splinter Review
Bug 30960: Don't allow user to place the hold without a pick-up place (5.34 KB, patch)
2022-06-18 21:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30960: Fix JS error message when no pick-up location is selected when placing a hold (5.36 KB, patch)
2022-06-18 21:27 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Vashchuk 2022-06-14 09:44:00 UTC

    
Comment 1 Peter Vashchuk 2022-06-14 09:50:12 UTC Comment hidden (obsolete)
Comment 2 Joonas Kylmälä 2022-06-15 19:01:36 UTC
Comment on attachment 136066 [details] [review]
Bug 30960: Koha lets to place item-level holds without a pick-up place

Review of attachment 136066 [details] [review]:
-----------------------------------------------------------------

::: reserve/placerequest.pl
@@ +83,2 @@
>  
> +                if ( $item_pickup_location || $can_item_be_reserved eq 'OK' || ( $can_item_be_reserved ne 'itemAlreadyOnHold' && $can_override ) ) {

$item_pickup_location shouldn't be here, this lets anyone to place holds, regardless of the circ rules/override allowability.
Comment 3 Joonas Kylmälä 2022-06-15 19:06:34 UTC
Also the commit title should be changed to describe the change, not the issue at hand, so => Disallow placing item-level holds without a pick-up place
Comment 4 Peter Vashchuk 2022-06-17 11:41:40 UTC
Created attachment 136257 [details] [review]
Bug 30960: Don't allow user to place the hold without a pick-up place

It's possible to place item-level hold without selecting a pick-up
location, which causes problems: if the item is then returned,
Koha tells about a hold, but gives error 500.
At customer's hold tab you see the number of holds, but cannot see the
actual holds there. However, if you go to the title in question,
then modify the hold so that it has a pick-up place,
then the hold will work normally again.

This patch fixes already existing but not working js error message and
ensures that hold cannot be made while pickup location is undefined.

To reproduce:
1. Go to admin page, to the libraries configurations, and disable
pickup location for one of them.
2. Pick any biblio that has items that have that same library as a
default pickup location.
3. When placing the item-level hold, notice that the pickup location
dropdown box is empty by default. Keep it empty, place the hold.
4. Go to the patron's page of the patron who you placed that hold for,
check that it doesn't show the new hold.
5. Apply patch.
6. Repeat steps 2 and 3, it shouldn't let you make the item-level hold
until you select a specific pickup location.
Comment 5 David Nind 2022-06-17 19:49:27 UTC
Created attachment 136304 [details] [review]
Bug 30960: Don't allow user to place the hold without a pick-up place

It's possible to place item-level hold without selecting a pick-up
location, which causes problems: if the item is then returned,
Koha tells about a hold, but gives error 500.
At customer's hold tab you see the number of holds, but cannot see the
actual holds there. However, if you go to the title in question,
then modify the hold so that it has a pick-up place,
then the hold will work normally again.

This patch fixes already existing but not working js error message and
ensures that hold cannot be made while pickup location is undefined.

To reproduce:
1. Go to admin page, to the libraries configurations, and disable
pickup location for one of them.
2. Pick any biblio that has items that have that same library as a
default pickup location.
3. When placing the item-level hold, notice that the pickup location
dropdown box is empty by default. Keep it empty, place the hold.
4. Go to the patron's page of the patron who you placed that hold for,
check that it doesn't show the new hold.
5. Apply patch.
6. Repeat steps 2 and 3, it shouldn't let you make the item-level hold
until you select a specific pickup location.

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 Katrin Fischer 2022-06-18 21:25:26 UTC
Created attachment 136325 [details] [review]
Bug 30960: Don't allow user to place the hold without a pick-up place

It's possible to place item-level hold without selecting a pick-up
location, which causes problems: if the item is then returned,
Koha tells about a hold, but gives error 500.
At customer's hold tab you see the number of holds, but cannot see the
actual holds there. However, if you go to the title in question,
then modify the hold so that it has a pick-up place,
then the hold will work normally again.

This patch fixes already existing but not working js error message and
ensures that hold cannot be made while pickup location is undefined.

To reproduce:
1. Go to admin page, to the libraries configurations, and disable
pickup location for one of them.
2. Pick any biblio that has items that have that same library as a
default pickup location.
3. When placing the item-level hold, notice that the pickup location
dropdown box is empty by default. Keep it empty, place the hold.
4. Go to the patron's page of the patron who you placed that hold for,
check that it doesn't show the new hold.
5. Apply patch.
6. Repeat steps 2 and 3, it shouldn't let you make the item-level hold
until you select a specific pickup location.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 7 Katrin Fischer 2022-06-18 21:27:24 UTC
Created attachment 136326 [details] [review]
Bug 30960: Fix JS error message when no pick-up location is selected when placing a hold

It's possible to place item-level hold without selecting a pick-up
location, which causes problems: if the item is then returned,
Koha tells about a hold, but gives error 500.
At patron's hold tab you see the number of holds, but cannot see the
actual holds there. However, if you go to the title in question,
then modify the hold so that it has a pick-up location,
then the hold will work normally again.

This patch fixes already existing but not working JS error message and
ensures that hold cannot be made while pickup location is undefined.

To reproduce:
1. Go to admin page, to the libraries configurations, and disable
pickup location for one of them.
2. Pick any biblio that has items that have that same library as a
default pickup location.
3. When placing the item-level hold, notice that the pickup location
dropdown box is empty by default. Keep it empty, place the hold.
4. Go to the patron's page of the patron who you placed that hold for,
check that it doesn't show the new hold.
5. Apply patch.
6. Repeat steps 2 and 3, it shouldn't let you make the item-level hold
until you select a specific pickup location.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 8 Martin Renvoize 2022-06-20 14:07:44 UTC
I'm not sure about this patch actually.. the controller change feels somehow wrong, silently skipping.

I'd expect to either see an error thrown back or for us to default to the items homebranch or holdingbranch (though holdingbranch may be 'unknown' as the item may be on loan)
Comment 9 Martin Renvoize 2022-06-20 14:11:52 UTC
OK, I see.. the controller already has president of just skipping bad case and hopes for the best clientside.

As you were..
Comment 10 Tomás Cohen Arazi 2022-06-20 14:51:01 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 11 Lucas Gass 2022-06-24 15:54:03 UTC
Backported to 22.05.x for 22.05.01
Comment 12 Arthur Suzuki 2022-06-29 09:14:14 UTC
I don't reproduce the bug in 21.11.x.
Not backporting.