Bug 35771

Summary: Unselecting titles when making multi-hold does not have any effect
Product: Koha Reporter: Anneli Österman <anneli.osterman>
Component: Hold requestsAssignee: Katariina Pohto <katariina.pohto>
Status: RESOLVED FIXED QA Contact: Paul Derscheid <paul.derscheid>
Severity: normal    
Priority: P5 - low CC: caroline.cyr-la-rose, david, dcook, fridolin.somers, gmcharlt, katariina.pohto, lucas, nick, paul.derscheid
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This fixes placing multiple holds for a patron from search results in the staff interface: - The place holds page has checkboxes for unselecting some of the listed items - unselecting an item did not work and holds were placed on all items where a hold could be placed. - Unselected items without a pickup location generated a 500 error.
Version(s) released in:
24.11.00,24.05.06
Circulation function:
Attachments: One title unselected
Hold is made for all the titles even when one was unselected
Bug 35771: Unselecting titles when making multi-hold does not have any effect
Bug 35771: Unselecting titles when making multi-hold
Bug 35771: Unselecting titles when making multi-hold
Bug 35771: Unselecting titles when making multi-hold

Description Anneli Österman 2024-01-11 08:29:19 UTC
Created attachment 160810 [details]
One title unselected

When you make multi-hold and unselect one or some of the titles shown in the multi-hold page and then save the holds assuming that hold is made only for the selected titles. However, holds are made for all the titles regardless of the selection.
Comment 1 Anneli Österman 2024-01-11 08:30:34 UTC
Created attachment 160811 [details]
Hold is made for all the titles even when one was unselected

Hold is made for all the titles even when one title was unselected.
Comment 2 Katariina Pohto 2024-05-14 14:57:42 UTC
The process can be repeated like this:
1. Select several biblios and choose Place hold
2. Choose a patron
3. Select a pickup location for all biblios and deselect one of the checkmarks
4. Place holds and note that even the deselected holds was placed.

I looked into the code and noticed that the biblionumbers passed on to placerequest.pl come from hidden input fields that have no connection to the checkmark fields. This also means that the form validation will not care about invalid information on the unchecked biblios, which results in errors at least when unchecked biblio doesn't have a pickup location set:
1. Select several biblios and click Place hold
2. Choose a patron
3. Deselect one of the biblio checkmarks and leave the pickup location empty for that biblio.
4. Place holds and end up on an error page.
Comment 3 Katariina Pohto 2024-05-15 14:06:01 UTC
Created attachment 166780 [details] [review]
Bug 35771: Unselecting titles when making multi-hold does not have any effect

This patch moves the holdable_bibs id and value to the checkmark input. It doesn't do the same for the club multihold (which probably should be added), and doesn't change the <input name="biblionumber"> field which lists the biblios that end up on the URL query.

It might also be beneficial to change the placerequest.pl's redirect to request.pl to use the holdable_bibs list instead of the biblionumber list, so the unselected biblio wouldn't appear on request.pl page after the holds are placed

Test plan:
1. Select several biblios and choose Place hold
2. Choose a patron
3. Select a pickup location for all biblios and unselect one of the checkmarks
4. Place holds and note that even the deselected holds was placed.
5. Repeat steps 1-2.
6. Leave pickup locations empty and try to place the holds.
7. Note alert: "Please make sure all selected titles have a pickup location set"
8. Uncheck one of the biblios and add pickup locations to the checked biblios.
9. Try to place the holds and note that there is no alert, and you get an error 500.
10. Apply patch.
11. Repeat steps 1,2,8 and place holds.
12. Note that there is no error 500, and while all the biblios are shown on page, only the checked biblios have a new hold placed on them.
Comment 4 Lucas Gass (lukeg) 2024-05-15 14:34:55 UTC
With this patch applied I cannot place multi-hold's at all. 

I think we should also fix the problem of multiple ID's ( #holdable_bibs ) on the same page.
Comment 5 Katariina Pohto 2024-05-16 06:55:43 UTC
(In reply to Lucas Gass from comment #4)
> With this patch applied I cannot place multi-hold's at all. 
> 
> I think we should also fix the problem of multiple ID's ( #holdable_bibs )
> on the same page.

The changes worked on my test version but I don't have a completely working community version, so couldn't test there, I just did the same changes to the community file. It wouldn't be a complete fix anyway.

And I did wonder about those multiple IDs myself. There's the three different input fields for each biblionumber one the page, and the checkbox one doesn't effect the others at all. Currently we've just hidden the checkboxes so people don't try to unselect them.
Comment 6 Katariina Pohto 2024-05-21 14:36:10 UTC
Created attachment 166977 [details] [review]
Bug 35771: Unselecting titles when making multi-hold

I had dropped out the name attribute, which might be the reason for the patch failing.

New patch with the same steps as previously.

Someone else should pick up on this and check the club multi-hold too, and get rid of the multiple IDs, I wont be able to work on this further.
Comment 7 David Nind 2024-05-23 05:11:46 UTC
Created attachment 167060 [details] [review]
Bug 35771: Unselecting titles when making multi-hold

When placing a hold on multiple biblios Place Holds page (request.pl) has checkboxes for unselecting some of the listed biblios.
Removing the checkmark does not actually unselect the biblio. Clicking the Place holds button will place a hold for all the biblios on page that can be reserved.
If unchecked biblio does not have a pickup location, it will get past form validation and cause an error.

Test plan:
1. Select several biblios and choose Place hold
2. Choose a patron
3. Select a pickup location for all biblios and unselect one of the checkmarks
4. Place holds and note that even the deselected holds was placed.
5. Repeat steps 1-2.
6. Leave pickup locations empty and try to place the holds.
7. Note alert: "Please make sure all selected titles have a pickup location set"
8. Uncheck one of the biblios and add pickup locations to the checked biblios.
9. Try to place the holds and note that there is no alert, and you get an error 500.
10. Apply patch.
11. Repeat steps 1,2,8 and place holds.
12. Note that there is no error 500, and while all the biblios are shown on page, only the checked biblios have a new hold placed on them.

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2024-05-23 05:20:07 UTC
I've signed off as everything now seems to work as per the test plan.

For items that are listed, but where the holds are not placed (where an item was unselected), the message shown is "There are no holds on this title.".

Ideally, this should say something "No hold placed for this title", or something similar.
Comment 9 Paul Derscheid 2024-09-10 15:47:39 UTC
Created attachment 171259 [details] [review]
Bug 35771: Unselecting titles when making multi-hold

When placing a hold on multiple biblios Place Holds page (request.pl) has checkboxes for unselecting some of the listed biblios.
Removing the checkmark does not actually unselect the biblio. Clicking the Place holds button will place a hold for all the biblios on page that can be reserved.
If unchecked biblio does not have a pickup location, it will get past form validation and cause an error.

Test plan:
1. Select several biblios and choose Place hold
2. Choose a patron
3. Select a pickup location for all biblios and unselect one of the checkmarks
4. Place holds and note that even the deselected holds was placed.
5. Repeat steps 1-2.
6. Leave pickup locations empty and try to place the holds.
7. Note alert: "Please make sure all selected titles have a pickup location set"
8. Uncheck one of the biblios and add pickup locations to the checked biblios.
9. Try to place the holds and note that there is no alert, and you get an error 500.
10. Apply patch.
11. Repeat steps 1,2,8 and place holds.
12. Note that there is no error 500, and while all the biblios are shown on page, only the checked biblios have a new hold placed on them.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 10 Paul Derscheid 2024-09-10 15:48:15 UTC
Looking good!
Comment 11 Katrin Fischer 2024-09-13 10:07:32 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 12 Lucas Gass (lukeg) 2024-11-15 21:27:25 UTC
Backported to 24.05.x for upcoming 24.05.06
Comment 13 Fridolin Somers 2024-12-02 09:23:34 UTC
Pushed to 23.11.x for 23.11.11
Comment 14 David Cook 2025-02-24 05:29:15 UTC
(In reply to Fridolin Somers from comment #13)
> Pushed to 23.11.x for 23.11.11

Hey Frido, are you sure you applied this correctly?

I can't get multi-hold to work in 23.11.11 for the staff interface and when I compare the patches here... it looks like you applied changes to lines 341 and 501, but the patches for main were for lines 1062 and 1124

When I visit http://localhost:8091/cgi-bin/koha/reserve/request.pl?biblionumber=114&biblionumber=29&multi_hold=1&borrowernumber=51 I see that there's no "holdable_bibs" name applied to the checkboxes. 

request.tt is a bit of a mess, so I'm trying to puzzle it all out... 

I can't really work out everything but  it looks like line 947 is the one that needs a fix...
Comment 15 Fridolin Somers 2025-02-24 10:35:02 UTC
OK thanks a lot for the alert.

Looks like a minor issue.
I prefer revert from 23.11.x for its stability.

Reverted in 23.11.12
Comment 16 David Cook 2025-02-24 22:28:58 UTC
(In reply to Fridolin Somers from comment #15)
> OK thanks a lot for the alert.
> 
> Looks like a minor issue.
> I prefer revert from 23.11.x for its stability.
> 
> Reverted in 23.11.12

I think the functionality will still be broken in 23.11.x then but fair enough to revert it. As you say, I think it's a minor issue. I've only had 1 library out of many even notice the problem, and I was able to patch it for them.
Comment 17 Caroline Cyr La Rose 2025-04-18 19:36:56 UTC
Bug fix, nothing to add/edit in the manual.