Bug 7408 - Expire holds that have been waiting too long
Summary: Expire holds that have been waiting too long
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low blocker (vote)
Assignee: Ian Walls
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-05 20:41 UTC by Ian Walls
Modified: 2019-06-27 09:24 UTC (History)
11 users (show)

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


Attachments
Proposed Patch (4.25 KB, patch)
2012-01-27 22:07 UTC, Ian Walls
Details | Diff | Splinter Review
Bug 7408 - Expire holds that have been waiting too long (5.87 KB, patch)
2012-02-23 17:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7408 - Expire holds that have been waiting too long (7.48 KB, patch)
2012-02-23 18:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7408 - Expire holds that have been waiting too long (7.30 KB, patch)
2012-02-23 18:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7408 - Expire holds that have been waiting too long (2.71 KB, patch)
2012-02-23 19:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7408 - Expire holds that have been waiting too long (7.69 KB, patch)
2012-02-24 14:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED OFF] Bug 7408 - Expire holds that have been waiting too long (7.87 KB, patch)
2012-03-05 20:53 UTC, Ian Walls
Details | Diff | Splinter Review
Bug 7408 - Don't cancel found reserves. (1.02 KB, patch)
2012-03-08 19:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Fix syspref def preventing its translation (1.14 KB, patch)
2012-03-16 18:55 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 7408 Fix ExpireReservesMaxPickUpDelayCharge syspref definition (1.50 KB, patch)
2012-03-16 19:53 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 7408 : Fixing a typo that was breaking the installer (2.51 KB, patch)
2012-03-20 01:21 UTC, Chris Cormack
Details | Diff | Splinter Review
signed-off patch (2.58 KB, patch)
2012-03-20 03:22 UTC, Kristina Hoeppner
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ian Walls 2012-01-05 20:41:46 UTC
For holds that have been waiting for pickup longer than the ReservesMaxPickupDelay number of days, there should be an automated way to mark these holds as "expired".

It would also be nice (well, for the library, perhaps) to have the option to charge a fee for the patron not picking up the material.
Comment 1 Nicole C. Engard 2012-01-05 22:48:14 UTC
Doesn't this script do most of that (except the charging part): misc/cronjobs/holds/cancel_expired_holds.pl
Comment 2 Ian Walls 2012-01-05 23:42:53 UTC
Nicole,


No, cancel_expired_holds.pl moves holds from being expired to being cancelled, and thus moved to the old_reserves table.  Not every waiting hold has an expiration date; it's not filled in by the holds fill process.  The patron or librarian has to set it manually.  What's needed is an automated way of adding these expiration dates based on the ReservesMaxPickupDelay value, so that cancel_expired_holds.pl can capture and cancel them.
Comment 3 Ian Walls 2012-01-27 22:07:16 UTC Comment hidden (obsolete)
Comment 4 Owen Leonard 2012-02-07 19:22:30 UTC
I've tested this successfully but have one question about how it works: After running the script, the holds get an expiration date of today. Running cronjobs/holds/cancel_expired_holds.pl immediately afterwards will cancel expired holds, but only those with an expiration date *less* than today. So none of the holds expired by this new script will be cancelled. If I then run the new script again with the --charge option the user will get double fines.

I know this isn't they way the script would typically be run, but it's worrisome to me.

On another note, testing this makes me wonder: Why don't we simply set an expiration date at the time the hold is marked waiting, based on the ReservesMaxPickupDelay setting?
Comment 5 Kyle M Hall 2012-02-23 17:12:27 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2012-02-23 17:13:09 UTC
(In reply to comment #4)

That does seem like a simpler solution, and the fewer cronjobs, the better. Patch submitted.

> I've tested this successfully but have one question about how it works: After
> running the script, the holds get an expiration date of today. Running
> cronjobs/holds/cancel_expired_holds.pl immediately afterwards will cancel
> expired holds, but only those with an expiration date *less* than today. So
> none of the holds expired by this new script will be cancelled. If I then run
> the new script again with the --charge option the user will get double fines.
> 
> I know this isn't they way the script would typically be run, but it's
> worrisome to me.
> 
> On another note, testing this makes me wonder: Why don't we simply set an
> expiration date at the time the hold is marked waiting, based on the
> ReservesMaxPickupDelay setting?
Comment 7 Ian Walls 2012-02-23 17:27:59 UTC
Kyle,


Your patch does not include a provision for charging the patron for not picking up the hold, like the cronjob-based patch did, so it doesn't seem like a sufficient replacement (but, of course, I'm biased here).  Reimplementing this to include not only a change to a C4 subroutine, but also a new syspref and consequential DB rev, seems to me to be a less worthwhile tradeoff.

I open this for discussion, in the hopes others will be able to provide a more balanced opinion than my own.
Comment 8 Kyle M Hall 2012-02-23 18:26:09 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2012-02-23 18:27:11 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2012-02-23 18:37:00 UTC
(In reply to comment #7)

I have updated my patch to address that issue. The benefit of this version is there are no worries about cronjob conflicts.

larryb also tested the cronjob version and felt that is was not working quite right. I don't know the specifics of what that meant though.

> Kyle,
> 
> 
> Your patch does not include a provision for charging the patron for not picking
> up the hold, like the cronjob-based patch did, so it doesn't seem like a
> sufficient replacement (but, of course, I'm biased here).  Reimplementing this
> to include not only a change to a C4 subroutine, but also a new syspref and
> consequential DB rev, seems to me to be a less worthwhile tradeoff.
> 
> I open this for discussion, in the hopes others will be able to provide a more
> balanced opinion than my own.
Comment 11 Kyle M Hall 2012-02-23 18:41:14 UTC
(In reply to comment #7)

> Reimplementing this to include not only a change to a C4 subroutine, but also a > new syspref and consequential DB rev, seems to me to be a less worthwhile 
> tradeoff.

My opinion is that the possibility of the two cronjobs stepping on each other warrants the syspref and db revision. But, of course, I'm biased as well.
Comment 12 Chris Cormack 2012-02-23 18:55:27 UTC
I'm not biased (well about this patch anyway) but this trend of reimplementing people's work without prior discussion has got to stop.
Comment 13 Kyle M Hall 2012-02-23 19:08:47 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2012-02-23 19:11:51 UTC
Ian, you may find this more to your liking. By implementing the functionality of your script directly in cancel_expired_holds.pl, we get rid of the possibility of cronjob conflicts, and remove the necessity of new system prefs and db revs. So we gain the advantages of both and the disadvantages of none.
Comment 15 Kyle M Hall 2012-02-24 13:30:51 UTC
I've decided to obsolete my patch that modifies the cancel expired reserves script and go back my original approach. Here's the case I will make. I feel that using a system preference is actually a benefit. 

By having a system preference, it exposes the functionality better. You can learn a lot about what Koha can do simply be looking through all the available system preferences. If the functionality is hidden away in a cronjob, it becomes much harder to new users to know that functionality even exists. 

In addition, this version has the potential to become much more extensible. We could eventually add the waiting holds fee to the circulation rules table such that each library can choose it's own fee, just like standard fines and rental charges. It almost seems to go against the conventions of Koha to have all fines and fees controlled at such a granular level, with a single exception. If you accept this reasoning, I would be more than happy to make it so.
Comment 16 Ian Walls 2012-02-24 14:03:08 UTC
I can agree that having a unified, integrated and systemic way of keeping track of when holds expire is a good one.  This should be part of the Holds Rewrite project, as we'll be rethinking a great many aspects of the way we do Holds in Koha.

I can also agree that it's better to allow a more granular approach to hold-not-picked-up fines than just the blanket charge.  This part of the feature was added for one particular ByWater customer, who had a very simple use case.  The idea was that if people were to start using the feature and demand more flexibility to it, it could later be expanded, but for just one library system, it wasn't worth developing more than necessary.  The cronjob solved both the specific need of that library system, and the larger issue of holds expiry, without having to muck around with the already-bloated Circ Matrix.

As for your (current) patch, there is a typo in the subroutine name (_GetExpireResevesSQL is missing an 'r').  The subroutine is an interesting approach to the problem; it's not done like that elsewhere in Koha, so I'm not sure how to feel about it.

The patch also locks in the expiry date when the hold is marked as ready.  If the Max Pickup Delay were to change while a hold is waiting, then the new expiry date would not be calculated, leading to incorrect cancellation and possibly incorrect fees.  My cronjob-based patch uses the ReservesMaxPickupDelay that is current to when the expiry and possible fees are applied.
Comment 17 Ian Walls 2012-02-24 14:04:02 UTC
Comment on attachment 7357 [details] [review]
Proposed Patch

Unobsoleting, since the patch is a competing implementation, not an obsoleted version.
Comment 18 Kyle M Hall 2012-02-24 14:49:55 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2012-02-24 14:58:32 UTC
(In reply to comment #16)
> I can agree that having a unified, integrated and systemic way of keeping track
> of when holds expire is a good one.  This should be part of the Holds Rewrite
> project, as we'll be rethinking a great many aspects of the way we do Holds in
> Koha.

Agreed.

> I can also agree that it's better to allow a more granular approach to
> hold-not-picked-up fines than just the blanket charge.  This part of the
> feature was added for one particular ByWater customer, who had a very simple
> use case.  The idea was that if people were to start using the feature and
> demand more flexibility to it, it could later be expanded, but for just one
> library system, it wasn't worth developing more than necessary.  The cronjob
> solved both the specific need of that library system, and the larger issue of
> holds expiry, without having to muck around with the already-bloated Circ
> Matrix.

I can understand that. I guess my instinct when it comes to solutions is to not focus on the specific problem, but to create an extensible system that happens to solve that problem. When the CCFLS asked me to create a system to track summer reading club membership, I created the Clubs & Services module instead. I'm hoping to get it into Koha proper soon.

> As for your (current) patch, there is a typo in the subroutine name
> (_GetExpireResevesSQL is missing an 'r').  The subroutine is an interesting
> approach to the problem; it's not done like that elsewhere in Koha, so I'm not
> sure how to feel about it.

Good catch. I am really not a big fan of what I did there. The only reason I used a separate subroutine like that was in an attempt to follow the DRY principle. Since the sql was used in two subroutines, I didn't want a future developer to introduce a bug by changing it in one sub but not in another.

> The patch also locks in the expiry date when the hold is marked as ready.  If
> the Max Pickup Delay were to change while a hold is waiting, then the new
> expiry date would not be calculated, leading to incorrect cancellation and
> possibly incorrect fees.  My cronjob-based patch uses the
> ReservesMaxPickupDelay that is current to when the expiry and possible fees are
> applied.

That is a really good point. The new version of the patch I just submitted resolves this issue. It doesn't muck about with expiration dates at all. Instead, when CancelExpiredReserves is called, it then looks for holds that have been waiting for too long and cancels them as well.
Comment 20 Brendan Gallagher 2012-02-24 18:31:05 UTC
This is a little discouraging to see some of the comments in this thread.  

This doesn't make much sense to me "trend of reimplementing
people's work without prior discussion has got to stop"  If someone sees "in their opinion"  a different way to work out code for a particular BUG - then why discourage them from working on it and submitting the code?

Since there is competing code - why not just look at the two options and test them both?  In this situation I do agree - less chance of cronjobs stepping on each others toes - is a win.  Also for Koha - I don't think this is what one library wants but there are a lot of other libraries that want this sort of functionality to be included (I'm sure that I can find you plenty of libraries that are interested in the same functionality).  Holds Rewrite has been going on for years - maybe it would be best to have code that is ready now - be implemented.

Thanks,
Brendan
Comment 21 Chris Cormack 2012-02-24 19:17:57 UTC
(In reply to comment #20)
> This is a little discouraging to see some of the comments in this thread.  
> 
> This doesn't make much sense to me "trend of reimplementing
> people's work without prior discussion has got to stop"  If someone sees "in
> their opinion"  a different way to work out code for a particular BUG - then
> why discourage them from working on it and submitting the code?
> 
> Since there is competing code - why not just look at the two options and test
> them both?  In this situation I do agree - less chance of cronjobs stepping on
> each others toes - is a win.  Also for Koha - I don't think this is what one
> library wants but there are a lot of other libraries that want this sort of
> functionality to be included (I'm sure that I can find you plenty of libraries
> that are interested in the same functionality).  Holds Rewrite has been going
> on for years - maybe it would be best to have code that is ready now - be
> implemented.
> 
Sure, by all means,

But at least talk to the persons who patch you are marking obsolete with your one before then. This is not the first bug this has happened on. 
Competing patches are fine, and now as Ian has unobsoleted his one, this is what we have. Before we had someone replacing another persons patch with their own.

The trend is not just this bug, its other ones too. Discuss first, talk to the person who spent hours writing the patch, don't just mark his/hers obsolete and replace with your own.

Chris
Comment 22 Larry Baerveldt 2012-03-05 19:39:02 UTC
So, there are two patches for this issue. Our site is still waiting on this. Can someone decide on which approach to use and finalize a patch? Ian, have you had a chance to look at Kyle's latest update to this?

Thanks.
Larry
Comment 23 Ian Walls 2012-03-05 20:52:48 UTC
I like Kyle's implementation.  It puts the controls into system preferences, which exposes more of the functionality to the librarian user.  It's also immune to the cronjob timing issue that my implementation had.

One small nit-pick.  In circulation.pref, the class for ExpireReservesMaxPickUpDelayCharge should be 'currency', not 'integer', like maxoutstanding.  I'll fix this with my signoff.
Comment 24 Ian Walls 2012-03-05 20:53:20 UTC Comment hidden (obsolete)
Comment 25 Ian Walls 2012-03-05 20:56:55 UTC
I'm going to mark this as Passed QA, as it's no longer my implementation, and the changes are pretty easy to read and confirm.  Paul, if you'd like a second QA on this, I'd certainly understand.
Comment 26 Paul Poulain 2012-03-08 16:58:58 UTC
(In reply to comment #25)
> I'm going to mark this as Passed QA, as it's no longer my implementation,
> and the changes are pretty easy to read and confirm.  Paul, if you'd like a
> second QA on this, I'd certainly understand.

Fortunatly I made a 2nd QA ;-)

There was a trailing ` that made the updatedatabase fail !

Plus, I don't understand well how it works: what makes the expired reserve be automatically cancelled ?

I have :
* a hold that expired, and appears in /cgi-bin/koha/circ/waitingreserves.pl, on tab "1 hold over"
* I switched the syspref ExpireReservesMaxPickUpDelay ON and OFF, no fines and thought that reaching the page would make the expiry automatically be made. But that's not that. Then I thought the button "cancel all" will appear/disappear, but that's not that also.

So: what do I misunderstand ?

(keeping passed QA status for now, I made the follow-up to remove the `)
Comment 27 Paul Poulain 2012-03-08 17:01:12 UTC
grepped CancelExpiredReserves, I see it's only used in 

misc/cronjobs/holds/cancel_expired_holds.pl

but this script was already here. what did it before the patch that the patch changes (except for the fine thing)
Comment 28 Kyle M Hall 2012-03-08 19:44:39 UTC
Before this patch is applied, CancelExpiredReserves only cancels reserves that haven't been completed by the expiration date. This really should not cancel reserves that are already waiting or in transit.

After the patch, not only does it cancel reserves that have passed their expiration date, but will also optionally cancel waiting reserves that haven't been picked up in the time frame set by ReservesMaxPickUpDelay. This is controlled by the value of ExpireReservesMaxPickUpDelay. Also, if ExpireReservesMaxPickUpDelayCharge is set, a fee will be charged to the patron for not picking up a waiting reserve in the timeframe given.

(In reply to comment #27)
> grepped CancelExpiredReserves, I see it's only used in 
> 
> misc/cronjobs/holds/cancel_expired_holds.pl
> 
> but this script was already here. what did it before the patch that the
> patch changes (except for the fine thing)
Comment 29 Kyle M Hall 2012-03-08 19:49:04 UTC Comment hidden (obsolete)
Comment 30 Kyle M Hall 2012-03-08 19:51:10 UTC
Please let me know if I should file a new bug for this second patch. It is trivially small, though.
Comment 31 Paul Poulain 2012-03-09 15:28:58 UTC
OK, got it now, it took me 2 hours !!!

EXPIRY DATE = the date the patron optionnally set when he place the reserve
it's different from the waiting date + the ReservesMaxPickUpDelay !!!

patch pushed !
Comment 32 Frédéric Demians 2012-03-16 18:52:01 UTC
The new syspref ExpireReservesMaxPickUpDelayCharge added in
circulation.pref YAML file breaks the translator: sysprefs can't be
translated at all. I send a patch to fix that.
Comment 33 Frédéric Demians 2012-03-16 18:55:38 UTC Comment hidden (obsolete)
Comment 34 Liz Rea 2012-03-16 19:53:50 UTC Comment hidden (obsolete)
Comment 35 Ian Walls 2012-03-16 20:01:15 UTC
Simple fix for two serious issues.  Marking as Passed QA.  (sorry about that, everyone!)
Comment 36 Paul Poulain 2012-03-19 16:02:05 UTC
follow-up pushed
Comment 37 Chris Cormack 2012-03-20 01:21:40 UTC Comment hidden (obsolete)
Comment 38 Chris Cormack 2012-03-20 01:22:46 UTC
There was a typo in an earlier patch breaking syspref.sql .. which meant a clean install fails.

Upping severity to blocker.
Comment 39 Kristina Hoeppner 2012-03-20 03:22:48 UTC
Created attachment 8325 [details] [review]
signed-off patch

There was just a ` too many in one of the lines.
Comment 40 Juan Romay Sieira 2012-03-20 09:10:35 UTC
I think instead of remove this type, all of this line must be removed, because it's duplicated. The syspref "ExpireReservesMaxPickUpDelayCharge" appears twice at sysprefs.sql file.
Comment 41 Paul Poulain 2012-03-20 14:05:19 UTC
follow-up pushed (with a conflict solved, as other sysprefs had been added in the meantime).
Tested that syspref.sql is loading, but please double check...