Bug 7751 - Decrease loan period on items with a high number of holds
Summary: Decrease loan period on items with a high number of holds
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Ian Walls
URL:
Keywords:
Depends on: 5549
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-19 20:58 UTC by Chris Cormack
Modified: 2013-05-23 06:24 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 7751 : Decrease loan period on checkout for items with high holds (5.43 KB, patch)
2012-03-19 21:02 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7751 : Make Reduce Duedate aware of New Date formats (3.46 KB, patch)
2012-03-19 21:05 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7751 : Fix a couple of comparisons in C4::Circulation (1.72 KB, patch)
2012-03-19 21:23 UTC, Chris Cormack
Details | Diff | Splinter Review
Rebased Patch - Decrease loan period (5.43 KB, patch)
2012-05-28 14:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Updates HIGHHOLDS data-structure. (2.53 KB, patch)
2012-05-28 14:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 7751 : Selfcheckout should be aware of the Reduced Loan Periods (2.83 KB, patch)
2012-05-28 14:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 7751 : Decrease Loans for Items with Highholds (12.53 KB, patch)
2012-05-29 07:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 7751 : Make Reduce Duedate aware of New Date formats (3.48 KB, patch)
2012-05-29 08:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 7751 : Fix a couple of comparisons in C4::Circulation (1.33 KB, patch)
2012-05-29 08:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 7751 : Decrease Loans for Items with Highholds (17.83 KB, patch)
2012-05-29 14:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 7751 : Decrease Loans for Items with Highholds (18.31 KB, patch)
2012-06-09 15:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 7751 : Decrease Loans for Items with Highholds (18.70 KB, patch)
2012-06-11 13:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 7751 : Decrease Loans for Items with Highholds (18.71 KB, patch)
2012-06-11 13:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 7751 : Fix a couple of comparisons in C4::Circulation (1.38 KB, patch)
2012-08-09 09:38 UTC, Chris Cormack
Details | Diff | Splinter Review
Updated Patch (10.05 KB, patch)
2012-08-17 16:32 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 7751: Decrease Loans for Items in Demand for Holds (10.09 KB, patch)
2012-08-18 03:07 UTC, Chris Cormack
Details | Diff | Splinter Review
Supplementary Patch (db update to add sysprefs) (3.23 KB, patch)
2012-09-19 10:43 UTC, Colin Campbell
Details | Diff | Splinter Review
Supplemental tidy-up patch (1.13 KB, patch)
2012-09-19 13:54 UTC, Colin Campbell
Details | Diff | Splinter Review
[FOLLOW UP] Bug 7751: Decrease loan length with many holds (1.83 KB, patch)
2012-09-26 13:58 UTC, Nicole C. Engard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7751: Decrease loan length with many holds (1.93 KB, patch)
2012-10-02 14:49 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 7751: Follow up: fix preference text (1.20 KB, patch)
2012-10-12 14:15 UTC, Nicole C. Engard
Details | Diff | Splinter Review
warning message that appears (34.35 KB, image/png)
2012-10-12 14:17 UTC, Nicole C. Engard
Details
[SIGNED-OFF] Bug 7751: Follow up: fix preference text (1.35 KB, patch)
2012-10-14 03:57 UTC, Marc Véron
Details | Diff | Splinter Review
Due date is not cleared, affecting NEXT check out! (293.59 KB, image/jpeg)
2012-10-14 04:33 UTC, Marc Véron
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2012-03-19 20:58:02 UTC
Controlled by a syspref, this will reduce the amount of time something can be onloan, if it has a lot of holds on it
Comment 1 Chris Cormack 2012-03-19 21:02:09 UTC
I am going to attach 2 patches I removed from the bug 5549 work (hourly loans)

I have not tested these, so please test them thoroughly. I have rebased them off master
Comment 2 Chris Cormack 2012-03-19 21:02:16 UTC Comment hidden (obsolete)
Comment 3 Chris Cormack 2012-03-19 21:04:53 UTC
The second patch depends on bug 5549, the first one doesn't/
Comment 4 Chris Cormack 2012-03-19 21:05:04 UTC Comment hidden (obsolete)
Comment 5 Chris Cormack 2012-03-19 21:23:14 UTC Comment hidden (obsolete)
Comment 6 Paul Poulain 2012-03-26 15:02:20 UTC
This bug is missing a detailled test plan: what does the syspref do? How is the reduction calculated?

(not trying to test, just checking if this patch is included in the feature freeze i'll declare today. still not sure...)
Comment 7 Chris Cormack 2012-03-26 17:11:19 UTC
(In reply to comment #6)
> This bug is missing a detailled test plan: what does the syspref do? How is
> the reduction calculated?
> 
> (not trying to test, just checking if this patch is included in the feature
> freeze i'll declare today. still not sure...)

Hopefully Colin knows. I just removed it from hourly loans and put it on its own bug.
Comment 8 Martin Renvoize 2012-05-25 12:54:47 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > This bug is missing a detailled test plan: what does the syspref do? How is
> > the reduction calculated?
> > 
> > (not trying to test, just checking if this patch is included in the feature
> > freeze i'll declare today. still not sure...)
> 
> Hopefully Colin knows. I just removed it from hourly loans and put it on its
> own bug.
Comment 9 Martin Renvoize 2012-05-25 13:00:21 UTC
Ack, that last comment went through before I'd actually written it.

Just wanted to clarify. Mark Gavillet actually wrote most of the code for the above patches and is therefore probably the best to ask about testing.

I can say though, it's been working in production on at a couple of our sites for some time now. I'm about to update them to the latest and greatest so will test them against master and signoff once I'm happy it all still works.
Comment 10 Martin Renvoize 2012-05-28 14:06:14 UTC Comment hidden (obsolete)
Comment 11 Martin Renvoize 2012-05-28 14:08:52 UTC Comment hidden (obsolete)
Comment 12 Martin Renvoize 2012-05-28 14:10:23 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2012-05-28 14:29:54 UTC
Hi Mark and Martin, 

Do all 5 patches have to be applied in that sequence in order to test the patch? If not, could you mark the unneeded patches as obsolete? (Details > Edit details > obsolete flag). This makes it easier to test, especially for people using the sandboxes.

I notice the system preferences are not added in a standard way. Could you perhaps provide a follow up or squashed patch?

The SQL for adding new system preferences goes into installer/data/mysql/kohastructure.sql and they also have to be added to one of the .pref files in koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences.

Thx for your work on that feature :)
Comment 14 Katrin Fischer 2012-05-28 14:38:48 UTC
Ah, as an additional hint - there is a new module in Koha/DateUtils.pm that might be worth taking a look at, as this patch deals a lot with dates. There was also a recent mail to the mailing list about the topic: 
http://lists.koha-community.org/pipermail/koha-devel/2012-May/037381.html
Comment 15 Martin Renvoize 2012-05-28 15:39:41 UTC
(In reply to comment #14)

Thanks for your comments Katrin, I'm now looking at rolling this into two clearer patches (one that does everything upto the but not including the reliance on Bug 5549, and one for everything thereafter.. I'll also try and work out how DateUtils fits in too)..

Cheers
Comment 16 Martin Renvoize 2012-05-29 07:36:48 UTC Comment hidden (obsolete)
Comment 17 Martin Renvoize 2012-05-29 08:04:11 UTC Comment hidden (obsolete)
Comment 18 Martin Renvoize 2012-05-29 08:05:27 UTC Comment hidden (obsolete)
Comment 19 Martin Renvoize 2012-05-29 08:08:17 UTC
Just tested the additional two patches that apply the New Date Formats, they seem to apply and work as expected.

So.. to summarise testing. Apply all three patches, 'Decrease Loans for...' first followed by 'Make Reduce Duedate...' and finally 'Fix a couple of comparisons...'
Comment 20 Katrin Fischer 2012-05-29 08:34:19 UTC
>I have not appended the sql to sysprefs.sql under installer/data/mysql as I'm >not sure of the process yet (any hints?).

Appending the new system preferences at the end of the file is fine. That's all that needs to be done in there. Be careful that the line before ends with ;.
Comment 21 Martin Renvoize 2012-05-29 14:50:03 UTC Comment hidden (obsolete)
Comment 22 Martin Renvoize 2012-06-09 15:17:13 UTC Comment hidden (obsolete)
Comment 23 Martin Renvoize 2012-06-11 13:09:24 UTC Comment hidden (obsolete)
Comment 24 Martin Renvoize 2012-06-11 13:47:27 UTC Comment hidden (obsolete)
Comment 25 Chris Cormack 2012-08-09 09:38:24 UTC Comment hidden (obsolete)
Comment 26 Colin Campbell 2012-08-17 16:32:08 UTC Comment hidden (obsolete)
Comment 27 Chris Cormack 2012-08-18 03:07:55 UTC Comment hidden (obsolete)
Comment 28 Mason James 2012-09-14 11:13:00 UTC
> 
> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

passing QA...

$ koha-qa.pl 

testing 1 commit(s) (applied to commit 60790d0)
 * 77d74bb Bug 7751: Decrease Loans for Items in Demand for Holds
      C4/Circulation.pm
      C4/SIP/ILS/Transaction/Checkout.pm
      installer/data/mysql/sysprefs.sql
      koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
      koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt

* C4/Circulation.pm                                                        OK
* C4/SIP/ILS/Transaction/Checkout.pm                                       OK
* koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt              OK
Comment 29 Paul Poulain 2012-09-14 15:10:39 UTC
I must be dumb but I still not sure to see well how this feature work.
Am I right if I say 
"It reduces the check-out duration of an item by [decreaseLoanHighHolds] days for items that have more than [decreaseLoanHighHoldsValue] holds pending." ?
Comment 30 Colin Campbell 2012-09-17 14:49:21 UTC
Yes thats it, except DecreaseLoanHighholdsDuration is the value to reduce the loan by.
Comment 31 Paul Poulain 2012-09-18 19:12:41 UTC
Patch pushed to master

There was a conflict in C4/Circulation.pm, with bug 6151 (AllowToReturnBranch syspref that added a sub that was at the same place) and 7621 (Age Restriction)
I fixed the conflict, tested circulation was working well.

I was surprised to see no updatedatabase.pl, when I realised it was not needed : if they are not defined, there is no value, the feature is not enabled. Entering values in preference.pl create them (I've checked)
Colin = was it intended or a mistake with a nice effect ?


Isn't there a typo in the syspref description:
 "A loan should be reduced by decreaseLoanHighHoldsDuration when holds have been places (if decreaseLoanHighHolds is enables)"

shouldn't it be "have been placed" and "is enabled" ? (as Colin is english native, I feel less authoritative on this as he is ;-) )

(not enought to reject the patch though, so it's pushed ;-) )
Comment 32 Katrin Fischer 2012-09-18 19:43:33 UTC
Hm, can we maybe get a follow-up with the database update and system preferences? I like it better because it's more consistent, the scripts will pick up the new sys prefs for the release notes, people will see them in the update database and the syspref sql file will be a complete reference too.
Comment 33 Chris Cormack 2012-09-18 19:53:00 UTC
I concur. Having them for release notes and in the message for the update means users know it's there. Saving our time is always secondary to saving the users time.
Comment 34 Chris Cormack 2012-09-19 03:13:13 UTC
Jenkins will also keep complaining (as would the unit tests if run locally) that the sysprefs don't match. So can we please get a follow up.
Comment 35 Paul Poulain 2012-09-19 10:14:13 UTC
(In reply to comment #34)
> Jenkins will also keep complaining (as would the unit tests if run locally)
> that the sysprefs don't match. So can we please get a follow up.

Follow-up pushed

Note that I thought jenkins was doing a fresh install (from kohastructure.sql & sysprefs.sql).
Those sysprefs are in the sysprefs.sql file, so I was not understanding what was happening. Now I understand !
Comment 36 Colin Campbell 2012-09-19 10:43:20 UTC Comment hidden (obsolete)
Comment 37 Paul Poulain 2012-09-19 11:51:58 UTC
(In reply to comment #36)
> Created attachment 12337 [details] [review]
> Supplementary Patch (db update to add sysprefs)
> 
> Add the missing db update. Also fixed the text which as Paul spotted was
> written in nonsense rather than English

Sorry Colin, but I beated you (see comment 35) and already have pushed the updatedatabase one.

I haven't fixed the string one.
Note that, in your string fix:
holds have been placed (if decreaseLoanHighHolds is enables)

shouldn't we write "(if decreaseLoanHighHolds is enableD)" ?
Comment 38 Colin Campbell 2012-09-19 13:54:13 UTC Comment hidden (obsolete)
Comment 39 Nicole C. Engard 2012-09-26 13:58:05 UTC Comment hidden (obsolete)
Comment 40 Nicole C. Engard 2012-09-26 13:58:41 UTC
Submitting a follow up to clean up the preferences in to one sentence.
Comment 41 Nicole C. Engard 2012-09-26 13:59:00 UTC
Follow up needs sign off.
Comment 42 Nicole C. Engard 2012-09-26 14:01:05 UTC
Comment on attachment 12345 [details] [review]
Supplemental tidy-up patch

Obsoleted by the merging of the three preferences in to one sentence.
Comment 43 Owen Leonard 2012-10-02 14:49:11 UTC Comment hidden (obsolete)
Comment 44 Jared Camins-Esakov 2012-10-06 04:33:14 UTC
I believe the patch that has been signed off is fixing a bug in the feature which was already pushed to 3.10. Updating report to match that understanding. If I am mistaken, please set the severity back to "enhancement."
Comment 45 Nicole C. Engard 2012-10-09 10:38:31 UTC
You are correct.
Comment 46 Paul Poulain 2012-10-12 10:23:14 UTC
follow-up is string (syspref) only, passes QA
Comment 47 Paul Poulain 2012-10-12 10:24:33 UTC
follow-up pushed

(status changed back to ENH to have it in the release notes)
Comment 48 Nicole C. Engard 2012-10-12 14:07:02 UTC
Documentation question:

I don't see a detailed test plan here, so I need to know for sure what this does for the manual.

So, my questions:

Patron A has an item checked out with a due date of Nov 1, someone comes along and places a hold on the item they have checked out and the preference is set to decrease the loan length, does Patron A have a new due date? or do they still have a due date of Nov 1?

Patron B comes around and checked out a book today, normally they would get 2 weeks but the item has too many holds on it, does that change their due date at the time they check out by the number of days in the preference?

Basically - when does this preference kick in? Does it only apply to new checkouts, or checkouts in progress already?

Based on a chat in IRC I think it's the former, so that's how I'm documenting it, but I want to confirm.

Nicole
Comment 49 Nicole C. Engard 2012-10-12 14:15:48 UTC Comment hidden (obsolete)
Comment 50 Nicole C. Engard 2012-10-12 14:16:39 UTC
Obsoleted the patches that were pushed.  This patch is needed to make the preference clear. Changing from ENH back to BUG to get this fix in to 3.10.

Nicole
Comment 51 Nicole C. Engard 2012-10-12 14:17:41 UTC
Created attachment 12787 [details]
warning message that appears

This is why I changed the preference description - the warning says that it reduces the loan TO a certain number of days and my tests confirmed that is what's happening.

Nicole
Comment 52 Marc Véron 2012-10-14 03:57:38 UTC
Created attachment 12802 [details] [review]
[SIGNED-OFF] Bug 7751: Follow up: fix preference text

The preference used to read 'loan period by' but it's
actually reducing the checkout period 'to' the number
of days entered in the preference.  This patch changes
'by' to 'to'.

Signed-off-by: Marc Veron <veron@veron.ch>

Tiny patch, text change only, text apears as expected.
Comment 53 Marc Véron 2012-10-14 04:33:46 UTC
Created attachment 12803 [details]
Due date is not cleared, affecting NEXT check out!

After testing / singning off the patch to fix preference text I noticed an issue that should be addressed in a follow up:

After checking out or trying to check out such item, the due date is not cleared. As a conseqence the NEXT item one wants to check out will get a shorter loan period.

See screenshot attached.

Marc
Comment 54 Paul Poulain 2012-10-16 16:12:28 UTC
51
Comment 55 Paul Poulain 2012-10-16 16:16:09 UTC
follow-up for fixing preference test passed QA & pushed in one go.

switching back to "ASSIGNED", because the problem described in comment 53 must be adressed.
Lowering the severity of the problem: if the user look at what is written on the screen, it's easy to fix the usability problem. But Colin, please, provide a follow-up to fix this annoying problem.
Comment 56 Chris Cormack 2012-10-22 21:38:09 UTC
Patch doesn't apply to 3.8.x please format one for 3.8.x if it is needed there
Comment 57 Paul Poulain 2012-10-23 07:24:45 UTC
This bug is an ENH for 3.10 (thus, the follow-up don't apply to 3.8 and don't need to/can't be)