Goal: no renewal possible if an item is overdue or if the patron is restricted (for consistency with the initial checkout rules)
Created attachment 10230 [details] [review] proposed patch #1 Rebased a patch made by the Lyon3 team.
Under what circumstances does this bug appear? I assume the "if an item is overdue" is contingent on the OverduesBlockCirc preference? I'm already blocked from renewing an item on circulation.pl by scanning the barcode. With our without the patch I am not prevented from renewing an overdue item via the table of checkouts on the page ("Renew or Return checked items"). Please provide more information and a clear test plan.
If this isn't dependent on the OverduesBlockCirc preference then there needs to be a new preference or this needs to be built in as an option to that preference. Not all libraries want to blog renewals if an item is overdue.
Created attachment 11958 [details] [review] Block renewal if any checked out book is late I corrected the error that caused the renewal was not blocked clicking the renew buttons. This patch doesn't really correct a bug but propose a new feature. It is independant from OverduesBlockCirc syspref. The logic is that a patron can't renew whatever of his checked out books if any of them is late or if he's restricted. The renewal remains nevertheless possible for the librarian checking the Override renewal limit case. Plan test : Set the OverduesBlockCirc syspref to "Ask for confirmation" Go to a patron account and checkout a book Force a second checkout specifying an outdated due date to make it late You'll see that every checked out books are marked as Not Renewable (Overdue) in the Renew column. As suggested by Nicole C. Engard, I suppose that this should be rather tight to a syspref. I will try to make it within a few days if I can find the time and understand the proper way to do it.
This appears to work as described in comment 4, but adding this to master without a system preference to turn it on and off would be an unwanted revision of circulation procedures for a lot of libraries (mine included).
(In reply to comment #5) > This appears to work as described in comment 4, but adding this to master > without a system preference to turn it on and off would be an unwanted > revision of circulation procedures for a lot of libraries (mine included). http://www.quickmeme.com/meme/3qgz25/
(In reply to comment #6) > http://www.quickmeme.com/meme/3qgz25/ Adding a system preference for every little detail gets tiresome, but what alternative do we have when a proposed change to software equals a proposed change to library circulation policy?
I agree with Owen - sys prefs are the strength of Koha!
(In reply to comment #7) > (In reply to comment #6) > > http://www.quickmeme.com/meme/3qgz25/ > > Adding a system preference for every little detail gets tiresome, but what > alternative do we have when a proposed change to software equals a proposed > change to library circulation policy? That's precisely what I was saying. I find aversion to sysprefs a comical thing. Personally I contravene find it tiresome, it's dead easy to do.
Hehe auto correct turns don't even into contravene.
I was so confused until I read that it was an autocorrect error! :) I'm with you - I have no idea why people want fewer of them - the librarians love the control and it's why one system can be used around the world :)
Created attachment 12362 [details] [review] Block renewal for overdue documents I reworked entirely the initial patch, added some parts that were obviously missing (Detail circulation tab was forgotten had different behaviour from Checkout one) and tight all with a new syspref (RenewIfOverdue). As for the updatedatabase.pl part and specially the version number I doubt it would be correct, cause I don't know the way to set it, I just increment the last version number present in our updatedatabase.pl file. The syspref offers a triple choice when any of a patron's document is late : => allow renewal anyhow (if the document has renewable type of course) which is today the default action. => block only the overdue items => block all patron's checked out documents (the big punishment...). In the circulation interface, the renewal remains possible through the Override renewal button. the Renew all one is hidden if blocking options are choosen. In the patron's interfaces (Patron's file and the Webservice apis), the same logic is working. Beside the object of this patch, I also noted 2 things that would diserve to have a look : 1) the comments of the IsMemberBlocked routine announces a control on the fines status of the borrower, but the control is done on his restriction status. 2) In case of restriction, the patron's file is frozen (so renewal is not possible but through the webservice interface (RenewLoan in Services.pm) it is. The present patch will correct that last point if validated (an other story) but I think it should be corrected anyhow.
Patch does not apply, sorry: error: patch failed: koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt:865 error: koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt: patch does not apply error: patch failed: koha-tmpl/opac-tmpl/prog/en/modules/opac-user.tt:247 error: koha-tmpl/opac-tmpl/prog/en/modules/opac-user.tt: patch does not apply Regarding updatedatabase.pl: You can set the version to "XXXX", i.e.: $DBversion = "XXXX"; Marc
(In reply to comment #13) > Patch does not apply, sorry: > > error: patch failed: > koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt:865 > error: koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt: patch > does not apply > error: patch failed: koha-tmpl/opac-tmpl/prog/en/modules/opac-user.tt:247 > error: koha-tmpl/opac-tmpl/prog/en/modules/opac-user.tt: patch does not apply > > Regarding updatedatabase.pl: You can set the version to "XXXX", i.e.: > > $DBversion = "XXXX"; > > Marc Oh yes, I realize I 've made my tests on our test base which is a 3.8 version. I guess it's the reason why it doesn't apply.
Created attachment 12619 [details] [review] Block renewal when overdues documents patch redone on master. I just change again the name of syspref to "OverduesBlockRenew" in order to make easier the comparision (and distinction) with the other syspref OverduesBlockCirc
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
Created attachment 12727 [details] [review] Block renewal for overdue documents it has been redone on top of master. it's applying successfully today.
Created attachment 12790 [details] [review] [SIGNED-OFF] Block renewal for overdue documents If any of patron's document is late, renewal can be allowed, blocked on every checked out items or only on overdue ones. The renewal remains nevertheless possible for the librarian checking the Override renewal limit Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 12791 [details] [review] Bug 8236 - Prevent renewing if overdue or restriction - Followup - Fix up preference language.
QA Comments: A lot of QA issues: * C4/ILSDI/Services.pm FAIL valid FAIL "my" variable $issue masks earlier declaration in same scope * members/moremember.pl FAIL valid FAIL "my" variable $renewerror masks earlier declaration in same statement "my" variable $renewokay masks earlier declaration in same statement * opac/opac-user.pl FAIL forbidden patterns FAIL The patch introduces a forbidden pattern: tabulation character (188) moremember.pl l.521,522: my ( $renewokay, $renewerror ) = my ( $renewokay, $renewerror ) = [...] Marked as Failed QA.
QA says "forbidden patterns The patch introduces a forbidden pattern: tabulation character (188)" in opac-user.pl There's actually a bunch of tabulations that were still there before my patch (lines 157-220) and that are still there in master. I saw them but thought that was discounselled but not forbidden. May I remove them in this patch or has it to be done in a separate patch ?
(In reply to comment #21) Hi Koha Team Lyon 3, > May I remove them in this patch or has it to be done in a separate patch ? It is better in a new patch. > There's actually a bunch of tabulations that were still there before my patch > (lines 157-220) and that are still there in master. All new patches have to respect some guidelines. One of them is to not introduce new tabulation characters. So if a patch add a new line (+ in the git diff) cannot contain a tab. This issue is not blocker but the 2 others are.
Created attachment 13510 [details] [review] Bug 8236 Block renewing for overdue items As for the tabulations pointed out by QA, I made a separate patch (bug 9098). It's assumed that 9098 had been previously applied before this one. Reminder of test plan : Plan test : Set the OverduesBlockCirc syspref to "Ask for confirmation" Set the OverduesBlockRenew syspref to "blockall" Go to a patron account and checkout a book Force a second checkout specifying an outdated due date to make it late You'll see that every checked out books are marked as Not Renewable (Overdue) in the Renew column. Redo same process setting "blockitem" option in the syspref You'll see that only the late item had been marked as Not Renewable (Overdue).
Merge conflict in opac/opac-user.pl
I don't understand : I tried today on a brand new master and except as for the updatedatabase.pl file that blocks as usual when somebody adds a new syspref or something else in sql, every file is applying cleanly. I did that on top of today's master : First apply the tabulations patch : git am 0001-Bug-9098-Replace-tabulations-by-spaces.. then : git apply --reject 0001-Bug-8236-Block-renewing-for... and got this : Applied patch C4/Circulation.pm cleanly. Applied patch C4/ILSDI/Services.pm cleanly. Applied patch circ/circulation.pl cleanly. Applied patch installer/data/mysql/sysprefs.sql cleanly. Applying patch installer/data/mysql/updatedatabase.pl with 1 rejects... Rejected hunk #1. Applied patch koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref cleanly. Applied patch koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt cleanly. Applied patch koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt cleanly. Applied patch koha-tmpl/opac-tmpl/prog/en/modules/opac-user.tt cleanly. Applied patch members/moremember.pl cleanly. Applied patch opac/opac-user.pl cleanly
Probably an error on my part then. Resetting status to "Needs Signoff".
Created attachment 14433 [details] [review] Bug 8236 Block renewing for overdue items If any of patron's document is late, renewal can be allowed, blocked on every checked out items or only on overdue ones. The renewal remains nevertheless possible for the librarian checking the Override renewal limit Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Hi Koha Team Lyon 3, I'm under the impression that some duplicated code could be factorized into the CanBookBeRenewed routine. Is there a reason for this code not being in it?
(En réponse au commentaire 28) > Hi Koha Team Lyon 3, > > I'm under the impression that some duplicated code could be factorized into > the CanBookBeRenewed routine. Is there a reason for this code not being in > it? As you suggest it, I first considered putting the following block in CanBookBeRenewed : # Check if any of patron's documents is late my ($memberblocked) = C4::Members::IsMemberBlocked($borrowernumber); # and if renewal is allowed in that case my $overduesblockrenew = C4::Context->preference('OverduesBlockRenew'); my $norenewal = 1 if $overduesblockrenew eq 'blockall' and $memberblocked == -1; But as this block is there to check if no renewal at all is allowed, one checking by borrower is enough and it would be redundant checking again for each item. So I choose to put it outside the @$issueslist loop (sub build_issue_data) and keep the result in $norenewal variable. So when $norenewal is true, I have not to call for CanBookBeRenewed, I can straightly affect the value to $renewokay and $renewerror : my ( $renewokay, $renewerror ) = $norenewal ? (0, 'overdue') : CanBookBeRenewed( $borrowernumber, $issue->{itemnumber}... At the end, I am not sure that it's a great economy but that's the reason why. May be I could make a function out of this block and put it in Circulation.pm : sub CanOverduesBlockRenewal { my $borrowernumber = shift; # Check if any of patron's documents is late my ($memberblocked) = C4::Members::IsMemberBlocked($borrowernumber); # the option selected for the syspref my $overduesblockrenew = C4::Context->preference('OverduesBlockRenew'); # and if renewal is blocked for all the patron's documents on loan my $norenewal = 1 if $overduesblockrenew eq 'blockall' and $memberblocked == -1; return ($norenewal,$overduesblockrenew); } Olivier Crouzet
First of all, I want to say that this patch is quite important and I'm keen to see it get through ASAP. However, I'm failing it for a few reasons... 1) The code isn't applied consistently/doesn't take the system preference into account across all the different scripts: The code in ILSDI/Services.pm looks ok: "my $norenewal = 1 if ( $overduesblockrenew eq 'blockall' and $memberblocked == -1 ) or ( $overduesblockrenew eq 'blockitem' and $overdue ) or $memberblocked == 1;# last to check if patron is restricted" - However, circulation.pl, moremember.pl, and opac-user.pl all use: "my $norenewal = 1 if $overduesblockrenew eq 'blockall' and $memberblocked == -1;" - They really should all be using the first code snippet, since that correctly takes the "OverduesBlockRenew" syspref into account. PLUS, it's important to check for $memberblocked == 1 to make sure that the patron/borrower isn't restricted! -- 2) That all said, I agree with JDruart. This code really should be either factorized into CanBookBeRenewed or into its own sub/function. I understand what you're saying Oliver about not wanting to call the code for every item, except...the system preference has the "blockitem" option, which will need to be checked for every item in the loop. -- 3) I haven't tried this patch yet, but I think the templates (at least circulation.tt) might have some issues too...there are separate checks for "previssue.renew_error_too_many" and "previssue.renew_error_overdue" which means that you could in theory have a line in the check out window that says "Not renewable Renewal not allowed (overdue)". These should probably be 1 If/ElseIf statement rather than 2 separate If statements. -- Ultimately, I think this needs an overhaul, but it's certainly a worthwhile patch that I hope gets in soon :)
I think this patch is also missing the "opac-renew.pl" file...
Given the complexity of this patch, I think it would be a good idea to split this into a "block renewing for overdue items" and "prevent renewing for restricted patrons". I've created a new bug with a patch for "preventing renewing for restricted patrons": http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10089
To tell the truth, there was a confusion in my mind at the beginning between the two aspects, restriction and delay, of the problem but the last patch treated only the delay question (the bug might have to be renamed). I added a checking on restriction only in ILSDI but to make it coherent with what happens in patron file (renewing is already impossible if a patron is restricted because the file is freezen). I found unusefull to add a check in moremember.pl and circulation.pl because as the restriction message appears clearly in the page, I think we can leave it to the power of the librarian to force the renewal or not. That said, I agree, it's better to separate into two different patches. As to your second remark, I insist that my solution is working for every options the loop is done in every case, the difference is that when blockall is choosen, CanBookBeRenewed is not called, which spares some extra sql queries. I will try to propose a new patch theese days with the necessary corrections (in renewscript.pl too).
Created attachment 17926 [details] [review] Bug 8236 Block renewing for overdue items At the end , I found more convenient to deal with the restriction case in same patch. I keep what seems to be the existing logic - a patron can't renew when he's restricted (his opac file is freezen) - making it more consistent. Any renewal for a restricted patron is now also blocked from webservice and clearly not possible from the pro interface unless by checking 'override_limit'. If someone would like to allowed renewal for restricted patrons, I think it would be not very complicated to add a syspref ( a line of code in HasWhateverRenewalToBeBlocked and some addings in opac-user.tt) As for the main object of the patch - blocking for late document - I made some ajustements you pointed out and one change in the logic : rather than hiding the 'Renew all' button, I move the checking in renewscript.pl. So, this button is always available in case of big list of checkouts items.
So sad. It seems we can't test it on a sandbox : Something went wrong ! Applying: Bug 8236 Block renewing for overdue items Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging C4/Circulation.pm Auto-merging circ/circulation.pl Auto-merging installer/data/mysql/sysprefs.sql CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt Auto-merging koha-tmpl/opac-tmpl/prog/en/modules/opac-user.tt Auto-merging opac/opac-renew.pl CONFLICT (content): Merge conflict in opac/opac-renew.pl Auto-merging opac/opac-user.pl Auto-merging reserve/renewscript.pl Failed to merge in the changes. Patch failed at 0001 Bug 8236 Block renewing for overdue items
Yes, I think it's not possible, on the sandbox because of the versioning in updatedatabase.pl file.
(In reply to comment #36) > Yes, I think it's not possible, on the sandbox because of the versioning in > updatedatabase.pl file. Yes it is possible, but you should provide a patch rebased against master.
?! I am surely missing something (much indeed) about git, but I did this patch today on top of master
(In reply to comment #34) > Created attachment 17926 [details] [review] [review] > Bug 8236 Block renewing for overdue items > > At the end , I found more convenient to deal with the restriction case in > same patch. > I keep what seems to be the existing logic - a patron can't renew when he's > restricted (his opac file is freezen) - > making it more consistent. Any renewal for a restricted patron is now also > blocked from webservice and clearly not possible from the pro interface > unless by checking 'override_limit'. > If someone would like to allowed renewal for restricted patrons, I think it > would be not very complicated to add a syspref ( a line of code in > HasWhateverRenewalToBeBlocked and some addings in opac-user.tt) > As for the main object of the patch - blocking for late document - I made > some ajustements you pointed out and one change in the logic : > rather than hiding the 'Renew all' button, I move the checking in > renewscript.pl. So, this button is always available in case of big list of > checkouts items. Olivier, I'm not sure that I completely understand what you are saying, but I think it's quite important to not allow any renewals for restricted patrons at all and that the button should not be visible for a restricted patron, as Owen mentioned in bug 10089. In Comment 33, you mention that we should leave renewals for restricted patrons up to the discretion of the librarian, but I don't think this is a good idea. First, we block all check outs for restricted patrons. Renewals really are just another type of check out, so logically it makes sense to block these too. Second, even though there is red text saying the patron is blocked, I have reported cases where library staff have accidentally ignored this and renewed patrons away (much to the chagrin of their co-workers/supervisors). I think it's necessary to enforce the prevention of renewals for restricted patrons. In any case, I think these are still conceptually two different issues that should probably be handled by different patches. That said, I haven't looked at your newest code. If you prevent all renewals for restricted patrons in all cases and hide the renew buttons, as Owen suggested in bug 10089, then I would have no problem using your patch instead of mine. In all honesty, another reason I separated the patches is because I wanted the "prevent renewals for restricted patrons" behaviour to be introduced as quickly as possible. The more atomic the patch, the easier it is to test/QA, and the faster it gets into the codebase. Rant aside, once you rebase this patch, I'd be more than happy to test it.
Created attachment 18071 [details] [review] Bug 8236 Block renewing for overdue items Hello David, About the override_limit question, my patch respects both following necessities : 1) To renew an item by mistake (clicking on the renew buttons) must be impossible : That is done by a second check in renewscript.pl (the script is called by the button). So whenever this button is clicked by error the renewal fails if the member/item is blocked.I also add a function to hide renew buttons when there are no renewal checkbox available 2) To renew an item knowingly must remains possible through the override_limit : Situations may always happen where a certain flexibility is needed and where passing over the rule is the more appropriate thing to do. Olivier Crouzet
Created attachment 18073 [details] [review] Bug 8236 Block renewing for overdue items
Created attachment 18507 [details] [review] Bug-8236-Block-renewing-for-overdue-items.patch patch rebased
Sandbox setup by tredok.pierre@gmail.com with database 2 and bug 8236 on Thu Jun 6 08:57:09 2013 Something went wrong ! Applying: Bug 8236 Block renewing for overdue items Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging C4/Circulation.pm CONFLICT (content): Merge conflict in C4/Circulation.pm Auto-merging circ/circulation.pl Auto-merging installer/data/mysql/sysprefs.sql CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt Auto-merging koha-tmpl/opac-tmpl/prog/en/modules/opac-user.tt Auto-merging members/moremember.pl Auto-merging opac/opac-user.pl Failed to merge in the changes. Patch failed at 0001 Bug 8236 Block renewing for overdue items When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". Bug 8236 - Prevent renewing if overdue or restriction Bug-8236-Block-renewing-for-overdue-items.patch Apply? [yn] Patch left in /tmp/Bug-8236-Block-renewing-for-overdue-itemspatch-qFvBtT.patch
Works for me.
We have tested : 1) Allow renewing all items : ok if overdue or restriction 2) Block renewing all itms : ok if overdue or restriction 3) Block only overdue items : a patron with 2 check out : only one is overdue. We have a message for this one (Renewal not allowed (overdue)). We can't renew the other one and we have no message. If we check "Override renewal limit" we can't renew the item which is not overdue.
Created attachment 18729 [details] [review] Bug 8236 Block renewing for overdue items oups, squeezed some lines that had to be kept; Problem corrected
This enhancement is clearly in conflict with Bug 6739 which provides the same behavior + manages the reserves. Bug 6739 is already signed-off so we should work on it only. I set in discussion.
Hello Fridolyn, I'm not sure that the patch of bug 6739 has the same goal : - with this patch we want to prevent renewing if patrons have overdues or restriction. - with bug 6739, it wants to prevent renewing if patrons who have a card no more valid. It's not the same behaviour. Can you confirm that I have understood well and switch back this patch "need signoff" ? Sonia
(In reply to Koha Team Lyon 3 from comment #48) > Hello Fridolyn, > I'm not sure that the patch of bug 6739 has the same goal : > - with this patch we want to prevent renewing if patrons have overdues or > restriction. > - with bug 6739, it wants to prevent renewing if patrons who have a card no > more valid. > > It's not the same behaviour. Can you confirm that I have understood well and > switch back this patch "need signoff" ? > Sonia I agree with Sonia's evaluation of the patches, Fridolyn. It seems like they are doing quite different things.
Indeed, this patch does not provide the same behavior. I set back to "Needs Signoff"
Patch does not apply. Bug 8236 Block renewing for overdue items (25.44 KB, patch) 2013-06-07 10:19 UTC, Koha Team Lyon 3 Applying: Bug 8236 Block renewing for overdue items Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging reserve/renewscript.pl Auto-merging opac/opac-user.pl Auto-merging members/moremember.pl Auto-merging koha-tmpl/opac-tmpl/prog/en/modules/opac-user.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Auto-merging installer/data/mysql/sysprefs.sql CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql Auto-merging circ/circulation.pl Auto-merging C4/ILSDI/Services.pm Auto-merging C4/Circulation.pm Failed to merge in the changes. Patch failed at 0001 Bug 8236 Block renewing for overdue items
Created attachment 20716 [details] [review] Bug 8236 Block renewing for overdue items If any of patron's document is late, renewal can be allowed, blocked on every checked out items or only on overdue ones. The renewal remains nevertheless possible for the librarian checking the Override renewal limit
This patch removes the variable $issue from C4/ILSDI/Services.pm : --- a/C4/ILSDI/Services.pm +++ b/C4/ILSDI/Services.pm @@ -561,11 +561,10 @@ sub RenewLoan { return { code => 'RecordNotFound' } unless $$item{itemnumber}; # Add renewal if possible - my @renewal = CanBookBeRenewed( $borrowernumber, $itemnumber ); + my ($blockallitems,$reasonwhy,$overduesblockrenew) = HasAnyRenewalToBeBlocked($borrowernumber,undef); + my @renewal = $blockallitems ? (0, $reasonwhy) : CanBookBeRenewed( $borrowernumber, $itemnumber, undef, $overduesblockrenew ); if ( $renewal[0] ) { AddRenewal( $borrowernumber, $itemnumber ); } - my $issue = GetItemIssue($itemnumber); - # Hashref building my $out; $out->{'renewals'} = $issue->{'renewals'}; Is this normal? This variable is still used in the script.
Fridolyn, Yes you are right. I just rebased the patch against master, this deletion existed in the precedent patch. Marked as Failed QA.
Created attachment 20719 [details] [review] Bug 8236 - Prevent renewing if overdue or restriction I revised the patch.
Applying: Bug 8236 Block renewing for overdue items Using index info to reconstruct a base tree... M C4/Circulation.pm M C4/ILSDI/Services.pm M installer/data/mysql/sysprefs.sql M installer/data/mysql/updatedatabase.pl M koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref M koha-tmpl/opac-tmpl/prog/en/modules/opac-user.tt M opac/opac-user.pl Falling back to patching base and 3-way merge... Auto-merging opac/opac-user.pl Auto-merging koha-tmpl/opac-tmpl/prog/en/modules/opac-user.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Auto-merging installer/data/mysql/sysprefs.sql Auto-merging C4/ILSDI/Services.pm Auto-merging C4/Circulation.pm Patch failed at 0001 Bug 8236 Block renewing for overdue items The copy of the patch that failed is found in: /home/christopher/git/koha/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
Created attachment 21163 [details] [review] Bug 8236 Block renewing for overdue items Rebased master
Hello, Test on sandbox : Something went wrong ! Applying: Bug 8236 Block renewing for overdue items\nUsing index info to reconstruct a base tree...\nFalling back to patching base and 3-way merge...\nAuto-merging C4/Circulation.pm Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt\nFailed to merge in the changes.\nPatch failed at 0001 Bug 8236 Block renewing for overdue items When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort".\nBug 8236 - Prevent renewing if overdue or restriction 21163 - Bug 8236 Block renewing for overdue items
Created attachment 21223 [details] [review] Bug 8236 Block renewing for overdue items Rebased master
Patch applied cleanly, go forth and signoff
The sandbox you've requested is not ready. Some problems occurred applying patches from bug 8236: <h1>Something went wrong !</h1>Applying: Bug 8236 Block renewing for overdue items Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging C4/Circulation.pm Auto-merging circ/circulation.pl Auto-merging installer/data/mysql/sysprefs.sql Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt Auto-merging members/moremember.pl Failed to merge in the changes. Patch failed at 0001 Bug 8236 Block renewing for overdue items When you have resolved this problem run git bz apply --continue. If you would prefer to skip this patch, instead run git bz apply --skip. To restore the original branch and stop patching run git bz apply --abort. Bug 8236 - Prevent renewing if overdue or restriction 21223 - Bug 8236 Block renewing for overdue items Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-8236-Block-renewing-for-overdue-items-RX74cM.patch .
Created attachment 22264 [details] [review] Bug 8236 Block renewing for overdue items patch rebased
I installed the patch and I receive a message saying to me : sandbox is not ready! But no text of error in the body of the message
Patch tested with a sandbox, by Aurélie Fichot <aurelie.fichot@iepg.fr>
Created attachment 22275 [details] [review] Bug 8236 Block renewing for overdue items If any of patron's document is late, renewal can be allowed, blocked on every checked out items or only on overdue ones. The renewal remains nevertheless possible for the librarian checking the Override renewal limit. Signed-off-by: Aur
Created attachment 22280 [details] [review] Bug 8236 Block renewing for overdue items Bug 8236 Block renewing for overdue items If any of patron's document is late, renewal can be allowed, blocked on every checked out items or only on overdue ones. The renewal remains nevertheless possible for the librarian checking the Override renewal limit. Signed-off-by: Aurélie Fichot <aurelie.fichot@iepg.fr> (I reupload the patch in order to fix the encoding issue caused by the sandbox)
Comment on attachment 22280 [details] [review] Bug 8236 Block renewing for overdue items Review of attachment 22280 [details] [review]: ----------------------------------------------------------------- Patch doesn't apply with a conflict in C4/Circulation.pm, that I cannot fix as part of the QA. I did a code review and found some issues with the code, that should also be taken care of. Without testing I am a bit worried that the change in restriction handling might have side effects on this patch. Fridolin, could you check that when working on this again? ::: C4/Circulation.pm @@ +75,4 @@ > push @EXPORT, qw( > &CanBookBeIssued > &CanBookBeRenewed > + &HasWhateverRenewalToBeBlocked I am not sure about the name for the new sub - I can't really tell what it's going to do. Maybe try to make it a bit more clear or check with a native speaker for a better wording? We will also need some unit tests for it. @@ +2481,5 @@ > > my $issuingrule = GetIssuingRule($borrower->{categorycode}, $item->{itype}, $branchcode); > > if ( ( $issuingrule->{renewalsallowed} > $itemissue->{renewals} ) || $override_limit ) { > + $renewokay = ( $overduesblockrenew eq 'blockitem' and $overdue and !$override_limit ) ? 0 : 1; These changes should be backed up by some unit tests as well. ::: installer/data/mysql/updatedatabase.pl @@ +7331,5 @@ > } > > +$DBversion = "3.13.00.XXX"; > +if ( CheckVersion($DBversion) ) { > + $dbh->do("INSERT INTO systempreferences (variable,value,explanation,options,type) VALUES ('OverduesBlockRenew','allow','If any of a patron checked out documents is late, should renewal be allowed, blocked only on overdue items or blocked on whatever checked out document','allow|blockitem|blockall','Choice')"); Maye the options would be more clear as: block, bockoverdue, allow ? ::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt @@ +72,4 @@ > $( '#override_limit' ).click( function () { > if ( this.checked ) { > $( '.renewals-allowed' ).show(); $( '.renewals-disabled' ).hide(); > + $("input[name='renew_checked']").val('Renew or Return checked items'); This looks like a translation problem. There are a few more below I marked with 'T'. @@ +107,5 @@ > + return; > + } > + }); > + if(!valid){ > + $("input[name='renew_checked']").val('Return checked items'); T ::: koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt @@ +86,4 @@ > $( '#override_limit' ).click( function () { > if ( this.checked ) { > $( '.renewals-allowed' ).show(); $( '.renewals-disabled' ).hide(); > + $("input[name='renew_checked']").val('Renew or Return checked items'); T @@ +154,5 @@ > + return; > + } > + }); > + if(!valid){ > + $("input[name='renew_checked']").val('Return checked items'); T ::: koha-tmpl/opac-tmpl/prog/en/modules/opac-user.tt @@ +191,5 @@ > [% IF ( OpacRenewalAllowed ) %] > <td class="renew">[% IF ( ISSUE.status ) %][% IF ( canrenew ) %]<input type="checkbox" name="item" value="[% ISSUE.itemnumber %]"/> <a href="/cgi-bin/koha/opac-renew.pl?from=opac_user&item=[% ISSUE.itemnumber %]&borrowernumber=[% ISSUE.borrowernumber %]">Renew</a>[% END %] <span class="renewals">([% ISSUE.renewsleft %] of [% ISSUE.renewsallowed %] renewals remaining)</span> > [% ELSE %] > + [% IF ( ISSUE.norenew_overdue ) %] > + Renewal not allowed <span class="renewals">(overdue on a document)</span> I think instead of using 'document' better use 'item' to be consistent with the language in Koha.
Hm, it looks like the patch is from "Koha Team Lyon 3", but the bug is assigned to Fridolin - got a bit confused here :)
the HasWhateverRenewalToBeBlocked function makes a first level checking to see if any renewal must be blocked. The reason why may be one of the followings : 1) the borrower is already restricted 2) the borrower have late item(s) AND the 'blockall' option of OverduesBlockRenew syspref is enabled. the function gives the result of the checking through the value of first returned variable $blockallitems and also return in the third variable $overduesblockrenew the value of option syspref OverduesBlockRenew that will be passed to CanBookBeReserved if a second level checking is necesary on every items (if $blockallitems != 1). May be I could rename this function MustRenewalBeBlocked ? As for the following lines : $("input[name='renew_checked']").val('Return checked items')" cCould you tell me where exactly is the traduction problem since I kept the preexisting english terms ? Olivier Crouzet
Created attachment 34234 [details] [review] Bug 8236 Renewing policy in overdue or restriction case The patch have been remade again in a slight different way : It now introduces 2 sysprefs : RestrictionBlockRenewing to allow/block renewal of items when patron is restricted. OverduesBlockRenewing to allow, block only the late ones or block all checked out items. Default is "allow" in both case. Test Plan : Apply patch. The database must then be updated (so it cannot be tested on sandbox unless you asked the maintainer for doing so). RestrictionBlockRenewing is set by default to allow renewing of late items : 1) choose a borrowernumber (you also need his login and password) and 2 items : item1 and item2. 2) check out item1 3) check out item2 with an already passed due date (using the Specify-due-date box) 4) click on restritions tab and add a manual restriction You can see that renewal is still possible for both items. 5) Now go to set the syspref to block status and go back to patron's account : You can see a "not renewable : patron restricted" warning instead of the previous checkboxes. 6) check the "Override renewal limit" checkbox tand verify that renewing is still possible by this way OverduesBlockRenewing is set by default to allow renewing of late items : 7)Now remove the restriction, set the OverdueBlockRenewing syspref to "Block renewal only for late items" and go back to patron's professionnal page 8) The renewal is possible for item1 and not allowed for item2 (a "Not allowed : overdue" warning is displayed). 9) Set syspref to "Block renewal for all items".. 10) Both items display the "not allowed" warning instead of a checkbox. 11) set the syspref to "allow renewal" 12) Both items can be renewed 13) check the "Override renewal limit" checkbox and verify that renewing is still possible by this way. 14) Connect to the patron's personal file and note that both items are renewable. 15) Redo stages 7 up to 10. Olivier Crouzet
Patch tested with a sandbox, by Rémy-Juliette <juliette.levast@iepg.fr>
Created attachment 34322 [details] [review] Bug 8236 Renewing policy in overdue or restriction case This patch introduces 2 sysprefs : RestrictionBlockRenewing to allow/block renewal of items when patron is restricted. OverduesBlockRenewing to allow, block only the late ones or block all checked out items Default is "allow" in both case. Signed-off-by: R
This patch works perfectly. He answers all our needs and it is essential for our users. Hoping it quickly pushed master.
If I choose only this item for OverduesBlockRenewing, all my items are blocked from renewal.
(In reply to Kyle M Hall from comment #74) > If I choose only this item for OverduesBlockRenewing, all my items are > blocked from renewal. I tested again and it's still OK. I can't see why you could have this result. Could you be more precise ? is it on the personnal file, on the pro interface ? some other clue ?
Created attachment 35372 [details] [review] Bug 8236 Renewing policy in overdue or restriction case patch rebased
The sandbox you've requested is not ready. Some problems occurred applying patches from bug 8236: <h1>Something went wrong !</h1>Applying: Bug 8236 Renewing policy in overdue or restriction case Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Auto-merging installer/data/mysql/sysprefs.sql Auto-merging C4/Circulation.pm Failed to merge in the changes. Patch failed at 0001 Bug 8236 Renewing policy in overdue or restriction case When you have resolved this problem run git bz apply --continue. If you would prefer to skip this patch, instead run git bz apply --skip. To restore the original branch and stop patching run git bz apply --abort. Bug 8236 - Prevent renewing if overdue or restriction 35372 - Bug 8236 Renewing policy in overdue or restriction case Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-8236-Renewing-policy-in-overdue-or-restriction-Gwo9FG.patch .
Created attachment 36674 [details] [review] Bug 8236 Renewing policy in overdue or restriction case patch rebased
after apply the patch i can't see the items in the account borrower ; i have the message "loading..." so i can't apply the test plan...
Created attachment 36743 [details] [review] Bug 8236 Renewing policy in overdue or restriction case grrrmmffff... silly error on calling HasOverdues function. patch redone
I've tested the last version of this patch following the more recent test plan provided. Everything works to me. I'm signing off the patch. Can someone have a look at the QA? (Should not be much, as most of the problems were already addressed)
Created attachment 37407 [details] [review] [SIGNED-OFF] Bug 8236 Renewing policy in overdue or restriction case This patch introduces 2 sysprefs : RestrictionBlockRenewing to allow/block renewal of items when patron is restricted. OverduesBlockRenewing to allow, block only the late ones or block all checked out items Default is "allow" in both case. Signed-off-by: Matthias Meusburger <matthias.meusburger@biblibre.com>
Created attachment 37418 [details] [review] Bug 8236: Tests for Renewing policy in overdue or restriction case - Add tests in t/db_dependent/Circulation.t
Created attachment 37457 [details] [review] [PASSED QA] Bug 8236 Renewing policy in overdue or restriction case This patch introduces 2 sysprefs : RestrictionBlockRenewing to allow/block renewal of items when patron is restricted. OverduesBlockRenewing to allow, block only the late ones or block all checked out items Default is "allow" in both case. Signed-off-by: Matthias Meusburger <matthias.meusburger@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 37458 [details] [review] [PASSED QA] Bug 8236: Tests for Renewing policy in overdue or restriction case - Add tests in t/db_dependent/Circulation.t Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 37459 [details] [review] Bug 8236 [QA Followup] - Tweak language, fix whitespace error
I'm sorry, but can u please rebase it? The updatedatabase.pl issue was trivial to solve, but such was not the case for the tests.
Created attachment 41992 [details] [review] Bug 8236 Renewing policy in overdue or restriction case This patch introduces 2 sysprefs : RestrictionBlockRenewing to allow/block renewal of items when patron is restricted. OverduesBlockRenewing to allow, block only the late ones or block all checked out items Default is "allow" in both case. Signed-off-by: Matthias Meusburger <matthias.meusburger@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 41993 [details] [review] Bug 8236: Tests for Renewing policy in overdue or restriction case - Add tests in t/db_dependent/Circulation.t Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 41994 [details] [review] Bug 8236 [QA Followup] - Tweak language, fix whitespace error
@Tomás: I've rebased these patches and use atomic updates for the news sysprefs
Created attachment 42007 [details] [review] Bug 8236 [QA Followup] - Tweak language, fix whitespace error Changes in the strings look good. Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com>
Any movement on this one? I'd be keen to see this one go in.
Patches pushed to master. Thanks Lyon3 team!
Created attachment 43628 [details] [review] Bug 8236: (QA followup) fix test count error due to bad merge On rebasing the patchset I introduced a wrong number of tests. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>