Controlled by a syspref, this will reduce the amount of time something can be onloan, if it has a lot of holds on it
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
Created attachment 8313 [details] [review] Bug 7751 : Decrease loan period on checkout for items with high holds
The second patch depends on bug 5549, the first one doesn't/
Created attachment 8314 [details] [review] Bug 7751 : Make Reduce Duedate aware of New Date formats
Created attachment 8315 [details] [review] Bug 7751 : Fix a couple of comparisons in C4::Circulation Changed a string comparison called on an integer Deal with case where value is not set without runtime warnings This is dependent on bug 5549
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...)
(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.
(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.
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.
Created attachment 9801 [details] [review] Rebased Patch - Decrease loan period Patch rebased against master and from the original code by Mark Gavillet. Two further patches to follow. None of which are dependant up bug5549.
Created attachment 9803 [details] [review] Updates HIGHHOLDS data-structure. Additional patch to correct data-structure of HIGHHOLDS. Data should be returned as data, not as a flag.
Created attachment 9804 [details] [review] Bug 7751 : Selfcheckout should be aware of the Reduced Loan Periods Additional patch to make Selfcheckout "Decreased Loan Period" aware.
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 :)
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
(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
Created attachment 9810 [details] [review] Bug 7751 : Decrease Loans for Items with Highholds Combined patches that don't rely on hourly loans work and added preferences to circulation.pref. I have not appended the sql to sysprefs.sql under installer/data/mysql as I'm not sure of the process yet (any hints?). Initial testing: Apply only this patch. Three new preferences should appear under Home › Administration › System preferences (Circulation). decreaseLoanHighHolds (Enable/Don't Enable) to enable the function, decreaseLoanHighHoldsDuration (Integer) the number of days to reduce a loan by if enables and finally decreaseLoanHighHoldsValue (Integer) the threshold number of holds an item can have before the the decreased loan period kicks in.
Created attachment 9811 [details] [review] Bug 7751 : Make Reduce Duedate aware of New Date formats
Created attachment 9812 [details] [review] Bug 7751 : Fix a couple of comparisons in C4::Circulation
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...'
>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 ;.
Created attachment 9834 [details] [review] Bug 7751 : Decrease Loans for Items with Highholds OK.. Rolled all into one Patch for easy application and changed sql to conform with koha standards.
Created attachment 10040 [details] [review] Bug 7751 : Decrease Loans for Items with Highholds Re-based again to apply to master.
Created attachment 10199 [details] [review] Bug 7751 : Decrease Loans for Items with Highholds Rebased yet again for master..
Created attachment 10204 [details] [review] Bug 7751 : Decrease Loans for Items with Highholds
Created attachment 11477 [details] [review] Bug 7751 : Fix a couple of comparisons in C4::Circulation Changed a string comparison called on an integer Deal with case where value is not set without runtime warnings This is dependent on bug 5549 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 11672 [details] [review] Updated Patch Had to update this for an upgraded site. Have merged all the separate patches to one and updated it to be applicable to master
Created attachment 11676 [details] [review] Bug 7751: Decrease Loans for Items in Demand for Holds Rebase and merge based on Mark Gavillet's patch to reduce the loan period given to items on renewal when they are in high demand for holds Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
> > 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
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." ?
Yes thats it, except DecreaseLoanHighholdsDuration is the value to reduce the loan by.
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 ;-) )
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.
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.
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.
(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 !
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
(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)" ?
Created attachment 12345 [details] [review] Supplemental tidy-up patch Trivial tidy up of the typos - All credit to Paul's proofreading
Created attachment 12528 [details] [review] [FOLLOW UP] Bug 7751: Decrease loan length with many holds This patch merges the three preferences in to one sentence to make it clear that they are all linked together and to eliminate confusion that can come by having them as individual preferences.
Submitting a follow up to clean up the preferences in to one sentence.
Follow up needs sign off.
Comment on attachment 12345 [details] [review] Supplemental tidy-up patch Obsoleted by the merging of the three preferences in to one sentence.
Created attachment 12643 [details] [review] [SIGNED-OFF] Bug 7751: Decrease loan length with many holds This patch merges the three preferences in to one sentence to make it clear that they are all linked together and to eliminate confusion that can come by having them as individual preferences. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
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."
You are correct.
follow-up is string (syspref) only, passes QA
follow-up pushed (status changed back to ENH to have it in the release notes)
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
Created attachment 12786 [details] [review] 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'.
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
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
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.
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
51
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.
Patch doesn't apply to 3.8.x please format one for 3.8.x if it is needed there
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)