Bug 6039 - Cancel all waiting holds button
Summary: Cancel all waiting holds button
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: 3.8
Hardware: All All
: P4 enhancement (vote)
Assignee: Srdjan Jankovic
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-04 01:13 UTC by Srdjan Jankovic
Modified: 2013-12-05 19:57 UTC (History)
8 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
patch (13.62 KB, patch)
2011-04-04 01:47 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (12.34 KB, patch)
2012-01-23 00:51 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (13.01 KB, patch)
2012-02-10 01:07 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug 6039: Cancel All waiting holds button (12.99 KB, patch)
2012-02-20 17:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
bug 6039: Cancel All waiting holds button - Addendum: Add redirect to refresh list after canceling all holds. (918 bytes, patch)
2012-02-20 17:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
bug 6039: Cancel All waiting holds button - Addendum: Add redirect to refresh list after canceling all holds. (973 bytes, patch)
2012-02-21 02:01 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Srdjan Jankovic 2011-04-04 01:13:40 UTC
“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
Comment 1 Srdjan Jankovic 2011-04-04 01:47:53 UTC Comment hidden (obsolete)
Comment 2 Frederic Durand 2011-04-07 13:18:02 UTC
"Cancel All" button working good, new sys pref too. All is ok
Comment 3 Ian Walls 2011-08-05 17:22:55 UTC
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
Comment 4 Srdjan Jankovic 2011-08-08 00:05:06 UTC
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?
Comment 5 Paul Poulain 2011-08-09 16:07:48 UTC
(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.
Comment 6 Ian Walls 2011-08-12 14:14:14 UTC
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.
Comment 7 Srdjan Jankovic 2012-01-23 00:51:21 UTC Comment hidden (obsolete)
Comment 8 Liz Rea 2012-02-07 16:07:24 UTC
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.
Comment 9 Srdjan Jankovic 2012-02-08 23:59:30 UTC
In order for button to appear, holds must be older than ReservesMaxPickUpDelay days. Was that the case?
Comment 10 Liz Rea 2012-02-09 03:18:32 UTC
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.
Comment 11 Liz Rea 2012-02-09 04:20:28 UTC
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.
Comment 12 Srdjan Jankovic 2012-02-10 01:07:11 UTC Comment hidden (obsolete)
Comment 13 Liz Rea 2012-02-10 15:22:05 UTC
spectacular, I see the button now. Will test!
Comment 14 Kyle M Hall 2012-02-20 17:16:38 UTC
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.
Comment 15 Kyle M Hall 2012-02-20 17:19:04 UTC
Created attachment 7778 [details] [review]
bug 6039: Cancel All waiting holds button

TransferWhenCancelAllWaitingHolds syspref

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall 2012-02-20 17:20:37 UTC Comment hidden (obsolete)
Comment 17 Chris Cormack 2012-02-20 17:51:05 UTC
Last patch needs signoff still
Comment 18 Chris Cormack 2012-02-21 02:01:59 UTC
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>
Comment 19 Paul Poulain 2012-02-27 15:16:47 UTC
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
Comment 20 Melia Meggs 2012-06-05 20:24:28 UTC
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.
Comment 21 Srdjan Jankovic 2012-06-05 23:26:15 UTC
That is misfortunate. To my defence, I have to say that the request was for “Cancel All”, not “Cancel All Expired” button.
Comment 22 Melia Meggs 2012-06-05 23:45:01 UTC
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.
Comment 23 Srdjan Jankovic 2012-06-05 23:56:58 UTC
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?
Comment 24 Melia Meggs 2012-06-06 00:05:23 UTC
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!
Comment 25 Srdjan Jankovic 2012-06-06 00:24:07 UTC
(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.
Comment 26 Ian Walls 2012-06-07 19:21:08 UTC
Best to file a new bug against this behaviour, since the feature has already been pushed.