Bug 8236 - Prevent renewing if overdue or restriction
Summary: Prevent renewing if overdue or restriction
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Koha Team University Lyon 3
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-11 15:12 UTC by Adrien SAURAT
Modified: 2017-09-06 19:40 UTC (History)
29 users (show)

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


Attachments
proposed patch #1 (2.33 KB, patch)
2012-06-11 15:27 UTC, Adrien SAURAT
Details | Diff | Splinter Review
Block renewal if any checked out book is late (2.54 KB, patch)
2012-09-04 13:52 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Block renewal for overdue documents (18.86 KB, patch)
2012-09-19 16:21 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Block renewal when overdues documents (17.83 KB, patch)
2012-10-01 10:25 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Block renewal for overdue documents (18.19 KB, patch)
2012-10-08 08:07 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
[SIGNED-OFF] Block renewal for overdue documents (18.02 KB, patch)
2012-10-12 14:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8236 - Prevent renewing if overdue or restriction - Followup - Fix up preference language. (1.65 KB, patch)
2012-10-12 14:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8236 Block renewing for overdue items (18.22 KB, patch)
2012-11-19 11:36 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 8236 Block renewing for overdue items (17.86 KB, patch)
2013-01-04 18:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8236 Block renewing for overdue items (25.08 KB, patch)
2013-05-03 13:04 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 8236 Block renewing for overdue items (26.35 KB, patch)
2013-05-10 15:05 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 8236 Block renewing for overdue items (26.42 KB, patch)
2013-05-10 15:24 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug-8236-Block-renewing-for-overdue-items.patch (26.50 KB, patch)
2013-05-30 09:53 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 8236 Block renewing for overdue items (25.44 KB, patch)
2013-06-07 10:19 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 8236 Block renewing for overdue items (25.18 KB, patch)
2013-08-29 07:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8236 - Prevent renewing if overdue or restriction (24.17 KB, patch)
2013-08-29 12:01 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 8236 Block renewing for overdue items (24.28 KB, patch)
2013-09-18 11:38 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 8236 Block renewing for overdue items (24.48 KB, patch)
2013-09-19 08:28 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 8236 Block renewing for overdue items (24.54 KB, patch)
2013-10-22 09:45 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 8236 Block renewing for overdue items (24.61 KB, patch)
2013-10-22 14:34 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 8236 Block renewing for overdue items (24.62 KB, patch)
2013-10-22 15:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8236 Renewing policy in overdue or restriction case (12.51 KB, patch)
2014-12-09 15:16 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 8236 Renewing policy in overdue or restriction case (12.63 KB, patch)
2014-12-11 14:30 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 8236 Renewing policy in overdue or restriction case (12.58 KB, patch)
2015-01-19 11:38 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 8236 Renewing policy in overdue or restriction case (12.75 KB, patch)
2015-03-06 10:46 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 8236 Renewing policy in overdue or restriction case (12.61 KB, patch)
2015-03-09 15:58 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8236 Renewing policy in overdue or restriction case (12.56 KB, patch)
2015-03-31 14:17 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 8236: Tests for Renewing policy in overdue or restriction case (6.14 KB, patch)
2015-04-01 13:06 UTC, Matthias Meusburger
Details | Diff | Splinter Review
[PASSED QA] Bug 8236 Renewing policy in overdue or restriction case (12.61 KB, patch)
2015-04-03 14:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 8236: Tests for Renewing policy in overdue or restriction case (6.21 KB, patch)
2015-04-03 14:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8236 [QA Followup] - Tweak language, fix whitespace error (4.00 KB, patch)
2015-04-03 14:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8236 Renewing policy in overdue or restriction case (12.56 KB, patch)
2015-08-26 14:30 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 8236: Tests for Renewing policy in overdue or restriction case (5.78 KB, patch)
2015-08-26 14:31 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 8236 [QA Followup] - Tweak language, fix whitespace error (4.05 KB, patch)
2015-08-26 14:31 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 8236 [QA Followup] - Tweak language, fix whitespace error (4.18 KB, patch)
2015-08-27 07:22 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 8236: (QA followup) fix test count error due to bad merge (824 bytes, patch)
2015-10-20 13:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Adrien SAURAT 2012-06-11 15:12:06 UTC
Goal: no renewal possible if an item is overdue or if the patron is restricted (for consistency with the initial checkout rules)
Comment 1 Adrien SAURAT 2012-06-11 15:27:48 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2012-07-20 20:05:39 UTC
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.
Comment 3 Nicole C. Engard 2012-08-20 13:16:54 UTC
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.
Comment 4 Koha Team University Lyon 3 2012-09-04 13:52:37 UTC Comment hidden (obsolete)
Comment 5 Owen Leonard 2012-09-10 19:24:12 UTC
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).
Comment 6 Chris Cormack 2012-09-10 21:31:39 UTC
(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/
Comment 7 Owen Leonard 2012-09-11 12:26:19 UTC
(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?
Comment 8 Nicole C. Engard 2012-09-11 16:30:34 UTC
I agree with Owen - sys prefs are the strength of Koha!
Comment 9 Chris Cormack 2012-09-11 18:31:27 UTC
(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.
Comment 10 Chris Cormack 2012-09-11 18:32:21 UTC
Hehe auto correct turns don't even into contravene.
Comment 11 Nicole C. Engard 2012-09-11 20:30:35 UTC
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 :)
Comment 12 Koha Team University Lyon 3 2012-09-19 16:21:48 UTC Comment hidden (obsolete)
Comment 13 Marc Véron 2012-09-23 16:49:47 UTC
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
Comment 14 Koha Team University Lyon 3 2012-09-24 15:47:24 UTC
(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.
Comment 15 Koha Team University Lyon 3 2012-10-01 10:25:31 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2012-10-05 13:51:58 UTC
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
Comment 17 Koha Team University Lyon 3 2012-10-08 08:07:48 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2012-10-12 14:57:45 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2012-10-12 14:58:42 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2012-11-13 14:21:04 UTC
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.
Comment 21 Koha Team University Lyon 3 2012-11-15 18:00:48 UTC
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 ?
Comment 22 Jonathan Druart 2012-11-16 08:54:19 UTC
(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.
Comment 23 Koha Team University Lyon 3 2012-11-19 11:36:43 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2012-11-21 13:48:26 UTC
Merge conflict in opac/opac-user.pl
Comment 25 Koha Team University Lyon 3 2012-11-22 17:30:26 UTC
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
Comment 26 Kyle M Hall 2012-12-03 17:20:54 UTC
Probably an error on my part then. Resetting status to "Needs Signoff".
Comment 27 Kyle M Hall 2013-01-04 18:12:20 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2013-02-07 13:57:04 UTC
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?
Comment 29 Koha Team University Lyon 3 2013-02-14 10:17:12 UTC
(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
Comment 30 David Cook 2013-04-16 09:24:08 UTC
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 :)
Comment 31 David Cook 2013-04-16 09:36:57 UTC
I think this patch is also missing the "opac-renew.pl" file...
Comment 32 David Cook 2013-04-22 03:40:08 UTC
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
Comment 33 Koha Team University Lyon 3 2013-04-22 08:55:21 UTC
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).
Comment 34 Koha Team University Lyon 3 2013-05-03 13:04:36 UTC Comment hidden (obsolete)
Comment 35 Koha Team AMU 2013-05-03 14:31:46 UTC
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
Comment 36 Koha Team University Lyon 3 2013-05-03 14:47:02 UTC
Yes, I think it's not possible, on the sandbox because of the versioning in updatedatabase.pl file.
Comment 37 Jonathan Druart 2013-05-03 14:48:29 UTC
(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.
Comment 38 Koha Team University Lyon 3 2013-05-03 15:18:34 UTC
?! I am surely missing something (much indeed) about git, but I did this patch today on top of master
Comment 39 David Cook 2013-05-05 23:54:43 UTC
(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.
Comment 40 Koha Team University Lyon 3 2013-05-10 15:05:33 UTC Comment hidden (obsolete)
Comment 41 Koha Team University Lyon 3 2013-05-10 15:24:03 UTC Comment hidden (obsolete)
Comment 42 Koha Team University Lyon 3 2013-05-30 09:53:51 UTC Comment hidden (obsolete)
Comment 43 Pierre Angot 2013-06-06 07:00:21 UTC
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
Comment 44 Jonathan Druart 2013-06-06 07:36:12 UTC
Works for me.
Comment 45 Pierre Angot 2013-06-06 09:56:51 UTC
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.
Comment 46 Koha Team University Lyon 3 2013-06-07 10:19:11 UTC Comment hidden (obsolete)
Comment 47 Fridolin Somers 2013-07-11 07:53:10 UTC
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.
Comment 48 Koha Team University Lyon 3 2013-08-21 15:43:34 UTC
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
Comment 49 David Cook 2013-08-22 00:03:21 UTC
(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.
Comment 50 Fridolin Somers 2013-08-22 11:13:39 UTC
Indeed, this patch does not provide the same behavior.
I set back to "Needs Signoff"
Comment 51 Campbell Reid-Tait 2013-08-29 03:57:59 UTC
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
Comment 52 Jonathan Druart 2013-08-29 07:45:46 UTC Comment hidden (obsolete)
Comment 53 Fridolin Somers 2013-08-29 08:51:24 UTC
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.
Comment 54 Jonathan Druart 2013-08-29 09:00:56 UTC
Fridolyn,

Yes you are right. I just rebased the patch against master, this deletion existed in the precedent patch.

Marked as Failed QA.
Comment 55 Fridolin Somers 2013-08-29 12:01:38 UTC Comment hidden (obsolete)
Comment 56 I'm just a bot 2013-09-18 10:00:50 UTC
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".
Comment 57 Fridolin Somers 2013-09-18 11:38:01 UTC Comment hidden (obsolete)
Comment 58 Pierre Angot 2013-09-19 08:14:32 UTC
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
Comment 59 Fridolin Somers 2013-09-19 08:28:49 UTC Comment hidden (obsolete)
Comment 60 I'm just a bot 2013-09-24 05:48:40 UTC
Patch applied cleanly, go forth and signoff
Comment 61 Aurélie Fichot 2013-10-22 08:09:28 UTC
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 .
Comment 62 Koha Team University Lyon 3 2013-10-22 09:45:22 UTC Comment hidden (obsolete)
Comment 63 Aurélie Fichot 2013-10-22 10:06:09 UTC
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
Comment 64 Biblibre Sandboxes 2013-10-22 14:34:17 UTC
Patch tested with a sandbox, by Aurélie Fichot <aurelie.fichot@iepg.fr>
Comment 65 Biblibre Sandboxes 2013-10-22 14:34:47 UTC Comment hidden (obsolete)
Comment 66 Jonathan Druart 2013-10-22 15:01:59 UTC Comment hidden (obsolete)
Comment 67 Katrin Fischer 2013-12-26 14:43:01 UTC
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&amp;item=[% ISSUE.itemnumber %]&amp;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.
Comment 68 Katrin Fischer 2013-12-26 14:45:27 UTC
Hm, it looks like the patch is from "Koha Team Lyon 3", but the bug is assigned to Fridolin - got a bit confused here :)
Comment 69 Koha Team University Lyon 3 2014-01-17 11:20:54 UTC
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
Comment 70 Koha Team University Lyon 3 2014-12-09 15:16:32 UTC Comment hidden (obsolete)
Comment 71 Biblibre Sandboxes 2014-12-11 14:29:37 UTC
Patch tested with a sandbox, by Rémy-Juliette <juliette.levast@iepg.fr>
Comment 72 Biblibre Sandboxes 2014-12-11 14:30:01 UTC Comment hidden (obsolete)
Comment 73 Levast 2014-12-11 14:37:27 UTC
This patch works perfectly. He answers all our needs and it is essential for our users. Hoping it quickly pushed master.
Comment 74 Kyle M Hall 2014-12-19 13:34:21 UTC
If I choose only this item for OverduesBlockRenewing, all my items are blocked from renewal.
Comment 75 Koha Team University Lyon 3 2015-01-16 10:56:59 UTC
(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 ?
Comment 76 Koha Team University Lyon 3 2015-01-19 11:38:56 UTC Comment hidden (obsolete)
Comment 77 Aurélie Fichot 2015-03-04 13:37:49 UTC
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 .
Comment 78 Koha Team University Lyon 3 2015-03-06 10:46:48 UTC Comment hidden (obsolete)
Comment 79 delaye 2015-03-06 11:35:22 UTC
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...
Comment 80 Koha Team University Lyon 3 2015-03-09 15:58:22 UTC Comment hidden (obsolete)
Comment 81 Matthias Meusburger 2015-03-31 14:11:04 UTC
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)
Comment 82 Matthias Meusburger 2015-03-31 14:17:53 UTC Comment hidden (obsolete)
Comment 83 Matthias Meusburger 2015-04-01 13:06:38 UTC Comment hidden (obsolete)
Comment 84 Kyle M Hall 2015-04-03 14:07:38 UTC Comment hidden (obsolete)
Comment 85 Kyle M Hall 2015-04-03 14:22:17 UTC Comment hidden (obsolete)
Comment 86 Kyle M Hall 2015-04-03 14:22:29 UTC Comment hidden (obsolete)
Comment 87 Tomás Cohen Arazi 2015-08-11 17:45:37 UTC
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.
Comment 88 Alex Arnaud 2015-08-26 14:30:51 UTC
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>
Comment 89 Alex Arnaud 2015-08-26 14:31:04 UTC
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>
Comment 90 Alex Arnaud 2015-08-26 14:31:14 UTC Comment hidden (obsolete)
Comment 91 Alex Arnaud 2015-08-26 14:32:51 UTC
@Tomás: I've rebased these patches and use atomic updates for the news sysprefs
Comment 92 Joonas Kylmälä 2015-08-27 07:22:32 UTC
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>
Comment 93 David Cook 2015-10-19 02:21:23 UTC
Any movement on this one? I'd be keen to see this one go in.
Comment 94 Tomás Cohen Arazi 2015-10-19 17:54:10 UTC
Patches pushed to master.

Thanks Lyon3 team!
Comment 95 Tomás Cohen Arazi 2015-10-20 13:53:58 UTC
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>