“Holds awaiting pickup – Holds over tab”, customer would like to sort by borrower in the “Holds over tab”. They would also like a “Cancel All” button on the same tab. It would make this task more efficient for the customer. Currently, we are having at least 15 to 20 holds expire each day and to have to click “Cancel Hold” for each one is long and tedious. New syspref TransferWhenCancelAllWaitingHolds will govern whether to skip holds that need to be transferred when bulk cancelling
Created attachment 3608 [details] [review] patch
"Cancel All" button working good, new sys pref too. All is ok
This patch no longer applies. It needs to be rebased onto Template:Toolkit. The database structure parts of the patch should be updated, as well, to include not only English but other languages, and to apply to the current code state. I'd recommend placing them in a separate patch from the functionality, for ease of testing and application
I can place them to separate patches, but then it will be a possible to have "Cancel All" button without TransferWhenCancelAllWaitingHolds syspref. Is that acceptable?
(In reply to comment #4) > I can place them to separate patches, but then it will be a possible to have > "Cancel All" button without TransferWhenCancelAllWaitingHolds syspref. Is that > acceptable? Once 6537 will have been applied, the problem with sysprefs in various language will be fixed, as there will be only one syspref file (with sometimes a specific file for specific values, but "localizers" will have to take care of it. Thus the problem Ian rises will be forgotten-able.
Srdjan, The functionality part of the patch can still be made to depend on the system preference TransferWhenCancelAllWaitingHolds; for testing, we'd just add it manually to our test DBs. That gives us a little more control over our test database's state (particular version number), and removes some of the potential merge conflicts that comes with the patch sitting in the queue for long enough for the database number to increment. Database update patches are usually pretty easy to QA, as well, so if the functionality works, it's simple to check the followup patch for the database entries, and QA that, as well.
Created attachment 7282 [details] [review] patch I kind of lost track here a bit. I hope all I needed to do was to rebase.
Patch applied with the caveat that I had to merge the sysprefs.sql and updatedatabase.pl files. However, I don't see the "cancel all" button on the "hold over" screen. Things I tried: flipped the syspref both ways, no luck applied/merged again, no luck.
In order for button to appear, holds must be older than ReservesMaxPickUpDelay days. Was that the case?
I believe so but I will double check - the holds were in the "hold over" tab, my impression was that things in that tab were things that were beyond reservesmaxpickupdelay. I'm happy to check this again though.
here are my reserves: *************************** 3. row *************************** borrowernumber: 44 reservedate: 2011-01-08 biblionumber: 39204 constrainttype: a branchcode: CPL notificationdate: NULL reminderdate: NULL cancellationdate: NULL reservenotes: priority: 0 found: W timestamp: 2012-02-08 22:11:17 itemnumber: 1036 waitingdate: 2012-01-08 expirationdate: NULL lowestPriority: 0 *************************** 4. row *************************** borrowernumber: 44 reservedate: 2011-01-08 biblionumber: 39201 constrainttype: a branchcode: CPL notificationdate: NULL reminderdate: NULL cancellationdate: NULL reservenotes: priority: 0 found: W timestamp: 2012-02-08 22:11:17 itemnumber: 1033 waitingdate: 2012-01-08 expirationdate: NULL lowestPriority: 0 *************************** 5. row *************************** borrowernumber: 44 reservedate: 2011-01-08 biblionumber: 39202 constrainttype: a branchcode: CPL notificationdate: NULL reminderdate: NULL cancellationdate: NULL reservenotes: priority: 0 found: W timestamp: 2012-02-08 22:11:17 itemnumber: 1034 waitingdate: 2012-01-08 expirationdate: NULL lowestPriority: 0 *************************** 6. row *************************** borrowernumber: 44 reservedate: 2011-01-08 biblionumber: 39203 constrainttype: a branchcode: CPL notificationdate: NULL reminderdate: NULL cancellationdate: NULL reservenotes: priority: 0 found: W timestamp: 2012-02-08 22:11:17 itemnumber: 1035 waitingdate: 2012-01-08 expirationdate: NULL lowestPriority: 0 reservesmaxpickupdelay set to 1 - all of these are well over 1 day waiting. I can see, when I flip the syspref, the message about "only holds that need not be transferred will be canceled" but I still see no actual "cancel all" button. Result is similar with the syspref set to "transfer" - the message seems right but there is no button.
Created attachment 7550 [details] [review] patch Apologies, totally my fault, missed closing " in the patch.
spectacular, I see the button now. Will test!
This patch works great with only one small but serious problem: Assume that there are a number of old waiting holds, both needing transfers and not need transfers. If TransferWhenCancelAllWaitingHolds is set to "Don't Transfer", and the "Cancel All' button is clicked, the page comes back with "No Holds Found". Only, when you visit the page again ( not just clicking refresh ) will you see that the holds with transfers were not canceled. I guarantee this will cause heart attacks for librarians if this is not fixed.
Created attachment 7778 [details] [review] bug 6039: Cancel All waiting holds button TransferWhenCancelAllWaitingHolds syspref Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 7779 [details] [review] bug 6039: Cancel All waiting holds button - Addendum: Add redirect to refresh list after canceling all holds.
Last patch needs signoff still
Created attachment 7781 [details] [review] bug 6039: Cancel All waiting holds button - Addendum: Add redirect to refresh list after canceling all holds. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
QA comment: clean patch, create a cancel sub to factorize cancellation. This sub could have been in a C4 package for future use, but won't reject QA for this reason passed QA
Two libraries on 3.8.1 have reported that the "Cancel All" button is not working as expected. After clicking the "Cancel All" button, all of their waiting holds were deleted, not just the expired ones. We had to restore the waiting holds from backup.
That is misfortunate. To my defence, I have to say that the request was for “Cancel All”, not “Cancel All Expired” button.
So let's clarify what the button is supposed to do. - Cancel all holds on the holds over tab? Or - Cancel all holds on the holds over AND holds waiting tabs? Right now it is canceling all holds over and all holds waiting. So it is canceling holds regardless of whether they are older than the ReservesMaxPickUpDelay.
Ok, now when I look at the screen I think it is rewasonable to expect only the holds over to be cancelled, because it is on that tab. How do we sort this one out? maybe create another bug cause this one has already been pushed?
Yes, I agree that it should only cancel holds over (which is the same as holds that are older than ReservesMaxPickUpDelay, yes?). The holds waiting do not all need to be canceled. I don't know if it's best to create another bug report or just submit a follow up patch. Not sure what the community likes to do in a case like this. Either way is fine with me!
(In reply to comment #24) > Yes, I agree that it should only cancel holds over (which is the same as > holds that are older than ReservesMaxPickUpDelay, yes?). I presume. In any case whatever is on that tab and not for transfer. > I don't know if it's best to create another bug report or just submit a > follow up patch. Not sure what the community likes to do in a case like > this. Either way is fine with me! I will wait for the second opinion, but I think whatever is pushed to a release should not be touched any more.
Best to file a new bug against this behaviour, since the feature has already been pushed.