Bug 24410

Summary: Multi holds broken
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Hold requestsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: critical    
Priority: P5 - low CC: 1joynelson, agustinmoyano, gmcharlt, julian.maurice, lucas, martin.renvoize, nick, sally.healey, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24587
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.04
Bug Depends on: 19618    
Bug Blocks: 15261, 24599    
Attachments: Bug 24410: Fix multi holds
Bug 24410: Fix multi holds
Bug 24410: Fix multi holds
Bug 24410: Fix multi holds

Description Jonathan Druart 2020-01-13 13:29:30 UTC
Bug 19618 replaces the call to placerequest.pl with a REST API /holds
That is wrong as it does not support multi holds.
Comment 1 Jonathan Druart 2020-01-13 13:31:40 UTC
Created attachment 97310 [details] [review]
Bug 24410: Fix multi holds

Bug 19618 replaces the call to placerequest.pl with a REST API /holds
That is wrong as it does not support multi holds.

Test plan:
Confirm that prior to this patch you are not able to place a hold on
several records at once, and that this patch fixes it
Comment 2 Tomás Cohen Arazi 2020-01-13 14:06:30 UTC
I tested the multi_hold flow and it calls request.pl or modrequest.pl (when you update the multi hold).

I belive we are safe without this patch.
Comment 3 Jonathan Druart 2020-01-13 16:14:58 UTC
This seems to work ok. Assigning to Tomas for the next steps (ie. more work is needed if placerequest can be removed).
Comment 4 Jonathan Druart 2020-02-04 11:26:38 UTC
*** Bug 24571 has been marked as a duplicate of this bug. ***
Comment 5 Jonathan Druart 2020-02-04 11:27:43 UTC
From Nick on bug 24571

"""
In the staff client do a search that returns 20+  results that can be held

Select all

Place hold

Confirm

The api submits ~8 requests, then the page moves to the hold results, but not alll records are held


While this does work in Chrome, there is no feedback as to what is happening - the submit button is not disabled. Testing with 200 holds there is a lot of waiting with no feedback
"""
Comment 6 Agustín Moyano 2020-02-04 23:28:40 UTC
(In reply to Jonathan Druart from comment #5)
> From Nick on bug 24571
> 
> """
> In the staff client do a search that returns 20+  results that can be held
> 
> Select all
> 
> Place hold
> 
> Confirm
> 
> The api submits ~8 requests, then the page moves to the hold results, but
> not alll records are held
> 
> 
> While this does work in Chrome, there is no feedback as to what is happening
> - the submit button is not disabled. Testing with 200 holds there is a lot
> of waiting with no feedback
> """

I'm thinking.. is there any RFC for bulk processing endpoints?.. 

maybe we can create an endpoint that does multiholds.

What do You think Jonathan?
Comment 7 Jonathan Druart 2020-02-05 08:20:58 UTC
*** Bug 24587 has been marked as a duplicate of this bug. ***
Comment 8 Jonathan Druart 2020-02-05 08:22:56 UTC
Agustin, as this affect stable and that the bug is now confirmed I think we should move this patch forward ASAP then discuss later how we can use the endpoint for multi holds.
Comment 9 Julian Maurice 2020-02-05 13:23:18 UTC
Created attachment 98479 [details] [review]
Bug 24410: Fix multi holds

Bug 19618 replaces the call to placerequest.pl with a REST API /holds
That is wrong as it does not support multi holds.

Test plan:
Confirm that prior to this patch you are not able to place a hold on
several records at once, and that this patch fixes it

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 10 Agustín Moyano 2020-02-05 18:04:32 UTC
Created attachment 98508 [details] [review]
Bug 24410: Fix multi holds

Bug 19618 replaces the call to placerequest.pl with a REST API /holds
That is wrong as it does not support multi holds.

Test plan:
Confirm that prior to this patch you are not able to place a hold on
several records at once, and that this patch fixes it

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 11 Martin Renvoize 2020-02-06 08:52:41 UTC
Created attachment 98517 [details] [review]
Bug 24410: Fix multi holds

Bug 19618 replaces the call to placerequest.pl with a REST API /holds
That is wrong as it does not support multi holds.

Test plan:
Confirm that prior to this patch you are not able to place a hold on
several records at once, and that this patch fixes it

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2020-02-06 08:53:41 UTC
This is a simple partial revert to restore functionality.. Passing QA.

Lets handle the final removal of those controllers and switch to REST apis as a separate issue.
Comment 13 Martin Renvoize 2020-02-06 12:37:55 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 14 Joy Nelson 2020-03-05 00:20:35 UTC
Pushed to 19.11.x for 19.11.04
Comment 15 Lucas Gass 2020-03-09 17:51:24 UTC
missing dependencies for 19.05.x, no backport