Bug 6328 - Fine in days does not work
Summary: Fine in days does not work
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: 3.8
Hardware: All All
: PATCH-Sent (DO NOT USE) major (vote)
Assignee: Paul Poulain
QA Contact: Ian Walls
URL:
Keywords:
: 5754 (view as bug list)
Depends on: 7167
Blocks: 6097
  Show dependency treegraph
 
Reported: 2011-05-11 14:54 UTC by Paul Poulain
Modified: 2013-12-05 19:53 UTC (History)
10 users (show)

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


Attachments
proposed patch (21.55 KB, patch)
2011-05-11 14:55 UTC, Paul Poulain
Details | Diff | Splinter Review
Update DB structure (892 bytes, patch)
2011-05-13 08:00 UTC, Frédéric Demians
Details | Diff | Splinter Review
Signed-off patch (21.79 KB, patch)
2011-05-17 18:16 UTC, Frédéric Demians
Details | Diff | Splinter Review
DB update and structure change (2.56 KB, patch)
2011-05-17 18:16 UTC, Frédéric Demians
Details | Diff | Splinter Review
patron flags (12.52 KB, image/png)
2011-05-17 18:20 UTC, Katrin Fischer
Details
Follow up, fixing layout in borrower details and debarment by overdue_notices.pl (2.28 KB, patch)
2011-05-17 19:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
Follow up, fixing redirect from 'Lift debarment' link (1.26 KB, patch)
2011-05-17 19:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
DB update and structure change signed-off (2.56 KB, patch)
2011-05-19 09:38 UTC, Paul Poulain
Details | Diff | Splinter Review
Follow up, fixing layout in borrower details and debarment by overdue_notices.pl signed-off (2.28 KB, patch)
2011-05-19 09:39 UTC, Paul Poulain
Details | Diff | Splinter Review
Follow up, fixing redirect from 'Lift debarment' link signed-off (1.26 KB, patch)
2011-05-19 09:39 UTC, Paul Poulain
Details | Diff | Splinter Review
fix for comment 23 (4th follow-up) (2.91 KB, patch)
2011-05-27 13:20 UTC, Paul Poulain
Details | Diff | Splinter Review
proposed patch squashed & rebased (25.91 KB, patch)
2011-09-02 10:16 UTC, Paul Poulain
Details | Diff | Splinter Review
little changes on db schema end db update (28.28 KB, patch)
2011-09-23 13:27 UTC, Zeno Tajoli
Details | Diff | Splinter Review
follow-up to deal with QA comments (5.42 KB, patch)
2011-10-11 16:28 UTC, Paul Poulain
Details | Diff | Splinter Review
follow-up for 6328 (3.59 KB, patch)
2011-10-21 16:22 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 6328 : Previous patches broke templates, this is fixing them (2.08 KB, patch)
2011-11-27 07:31 UTC, Chris Cormack
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6328 : Previous patches broke templates, this is fixing them (2.22 KB, patch)
2011-11-27 09:02 UTC, Katrin Fischer
Details | Diff | Splinter Review
Merge remote-tracking branch 'origin/new/bug_6328' (906 bytes, patch)
2012-01-19 15:52 UTC, Paul Poulain
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Poulain 2011-05-11 14:54:20 UTC
Some code coming from BibLibre has been lost in the process of inclusion in 3.4. The result is that fine in days does not work at all (you can setup rules, but it does nothing)

Step to reproduce:
Koha > Admin > circ rules > set 1 day fine every day of overdue for default rule
Issue a book return date last week
check-in the book => no debarment is set

The following patch will fix all of those problems by :
* updating borrowers.debarred to a date field (instead of tinyint). It contains the limit of the debarment
* changing API of DebarMember and UpdateBorrowerDebarred to pass a date
* display debarrdate where applicable. Note that a debarrdate of 31/12/9999 is considered as unlimited and not displayed
* added a debarrcomment, usefull to explain why a patron is debarred (this is independant from debarrdate changes and can be used when placing an unlimited debarment too)
Comment 1 Paul Poulain 2011-05-11 14:55:41 UTC Comment hidden (obsolete)
Comment 2 Paul Poulain 2011-05-11 14:57:04 UTC
*** Bug 5754 has been marked as a duplicate of this bug. ***
Comment 3 Katrin Fischer 2011-05-13 07:32:48 UTC
Note for QA: This patch is missing the update to kohastructure and will not work for new installs. Needs a follow-up.
Comment 4 Frédéric Demians 2011-05-13 07:57:08 UTC
> Note for QA: This patch is missing the update to kohastructure and will not
> work for new installs. Needs a follow-up.

You're correct. I send a DB structure update patch. Thanks.
Comment 5 Frédéric Demians 2011-05-13 08:00:49 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2011-05-13 08:15:11 UTC
Hi Frederic, thx for the quick update and patch! 

Looking at your patch I noticed something else: The deletedborrowers table is not updated (updatedatabas and kohastructure) - I think this needs to be done too to avoid problems when deleting borrowers and for consistency.
Comment 7 Frédéric Demians 2011-05-13 08:23:52 UTC
> Looking at your patch I noticed something else: The deletedborrowers table is
> not updated (updatedatabas and kohastructure) - I think this needs to be done
> too to avoid problems when deleting borrowers and for consistency.

Yes. I can't find where borrowers.debarred is updated from tinyint to date...
Comment 8 Katrin Fischer 2011-05-13 08:30:10 UTC
It's here, but I missed it at first too:

+$DBversion = "3.05.00.002";
+if ( C4::Context->preference("Version") < TransformToNum($DBversion) ) {
+    my $borrowers = $dbh->selectcol_arrayref( "SELECT borrowernumber from borrowers where debarred <>0;", { Columns => [1] } );
+    $dbh->do("ALTER TABLE borrowers MODIFY debarred DATE DEFAULT NULL;");
+    $dbh->do( "UPDATE borrowers set debarred='9999-12-31' where borrowernumber IN (" . join( ",", @$borrowers ) . ");" ) if ($borrowers and scalar(@$borrowers)>0);
+    $dbh->do("ALTER TABLE borrowers ADD COLUMN debarredcomment VARCHAR(255) DEFAULT NULL AFTER debarred;");
+    print "Upgrade done (Change borrowers.debarred into Date )\n";
+
+    SetVersion($DBversion);
+}
+
Comment 9 Frédéric Demians 2011-05-13 08:55:37 UTC
Send a followup adding also new debarredcomment field.
Comment 10 Frédéric Demians 2011-05-17 18:16:27 UTC Comment hidden (obsolete)
Comment 11 Frédéric Demians 2011-05-17 18:16:58 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2011-05-17 18:20:28 UTC
Created attachment 4154 [details]
patron flags

I have tested the follow-up and I am running into some problems:

1) the form on the patron record needs prettying (see screenshot). The field is way too long and should probably be a text box.

2) I tried debarring the patron by notice trigger (overdue_notices.pl). Trying with the same configuration before applying the patches the debarment is set. After updating the debarment will not be set. Before running overdue_notices.pl I checked that debarred and debarredcomment where empty.
Comment 13 Katrin Fischer 2011-05-17 19:09:12 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2011-05-17 19:09:57 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2011-05-17 19:11:27 UTC
Note: If you add a date and a comment, but forget to set the checkbox to 'Yes' the patron will not be debarred.
Comment 16 Paul Poulain 2011-05-19 09:38:14 UTC Comment hidden (obsolete)
Comment 17 Paul Poulain 2011-05-19 09:39:12 UTC Comment hidden (obsolete)
Comment 18 Paul Poulain 2011-05-19 09:39:42 UTC Comment hidden (obsolete)
Comment 19 Paul Poulain 2011-05-19 09:40:07 UTC
3 follow-up signed-off
Comment 20 Fernando L. Canizo 2011-05-24 13:00:09 UTC
Should we change related version here in the bug report? The first patch doesn't apply cleanly to a v3.04.00 based install, it fails to patch kohaversion.pl.

Once fixed that, the remaining 3 patches apply ok, however when running the web installer, updatedatabase.pl explodes with this error:

Update errors :

[Tue May 24 09:55:06 2011] updatedatabase.pl: Argument "3.0500XXX" isn't numeric in numeric lt (<) at /home/koha/installer/data/mysql/updatedatabase.pl line 4328.
Comment 21 Katrin Fischer 2011-05-25 07:45:01 UTC
When testing database updates you will have to edit kohaversion.pl to have the right next number and edit updatedatabase.pl from XXX to the right number.

It's common practice to have an XXX in the updatedatabase so the RM can fill in the correct number. kohaversion.pl should not be included in a patch, but you have to edit it either way to make the update to your database happen.

Hope that helps to explain. 

The patches are already signed off and are waiting to for QA manager.
Comment 22 Fernando L. Canizo 2011-05-26 11:39:43 UTC
Didn't know about that practice, now I understand.
Comment 23 Fernando L. Canizo 2011-05-26 14:55:38 UTC
After Katrin comments I finally could appropriately test this patch. Sorry to keep bothering you after all the sign-offs, but I'm not sure if this is the desired behaviour.

I found a couple of things that I think should not work that way:

(I'm branching from origin/3.4.x, as rangi pointed out via irc)

A. You can checkout new items to a patron who is already late if you haven't checked in the item that will cause debarrement. You only get a yellow message, but it's still possible to lend items to patron.

Steps to reproduce:
1. define a rule in circulation rules which apply fine in days
2. loan a book to some patron who will get the rule just defined applied to him/her
3. manually change the date_due in DB to one date (or wait enough days :P)

 update issues set date_due= <a_date_before_today> where borrowernumber = <fill_in> and itemnumber = <fill_in>;

(I tested this on a clean DB with an issues table with only one loan, so I didn't need the where clauses)

4. Now try to check out new items to patron and you'll be able to do it, when you shouln't. You only get a message on the right, in the "Attention" box. You can renew, check out and check in other items many times, as long as you don't check in the item that will produce debarrement.

I believe it's not logic to keep lending items to a patron who will be debarred upon check in. 

---

B. koha is watching for debarrement in more than one place: borrowers.debarred and in old_issues. Making an on-the-fly calculation upon check out for the debarred time left based on old_issues.date_due and old_issues.returndate, and not looking into borrowers.debarred, which I think is the purposo of these patches.

Steps to reproduce:
1. same as A
2. same as A
3. same as A
4. check in the item that will produce debarrement
5. patron gets debarred and a date is set in borrowers.debarred
6. lift debarrement and check DB:

select debarred from borrowers where borrowernumber = <subject_patron_number>;

In my test:

select debarred from borrowers where borrowernumber = 5;
+----------+
| debarred |
+----------+
| NULL     |
+----------+

So my patron is really NOT debarred now.

7. try to loan an item and you'll get this message:

Patron has had overdue items and is blocked for 3 day(s).

And you will be forbidden to loan an item to this patron, which you shouldn't since you lifted the debarrement.

That's because Circulation is checking old_issues, and the only way to administer this is to update database by hand, changing history!

---

C. Same as B but if your patron is from another library, he/she will not get debarred. You can test it by reproducing B. and checking borrowers.debarred in step 5.

---

I'm not sure if A is a feature. For B we need to modify circulation to stop checking old_issues and consult borrowers instead. And for C, we need to broaden the debarring rule to apply to any patron which we can loan to.
Comment 24 Paul Poulain 2011-05-27 09:46:46 UTC
(In reply to comment #23)
> After Katrin comments I finally could appropriately test this patch. Sorry to
> keep bothering you after all the sign-offs, but I'm not sure if this is the
> desired behaviour.
it's fine you keep bothering = the more eyes, the more bugs can be found !

> A. You can checkout new items to a patron who is already late if you haven't
> checked in the item that will cause debarrement. You only get a yellow message,
> but it's still possible to lend items to patron.
I think it's a correct behaviour. If you want to debarr the user immediatly when he is one day late, then you can/must use /cgi-bin/koha/tools/overduerules.pl, and run overdue_notices.pl every night (after midnight)


> B. koha is watching for debarrement in more than one place: borrowers.debarred
> and in old_issues. Making an on-the-fly calculation upon check out for the
> debarred time left based on old_issues.date_due and old_issues.returndate, and
> not looking into borrowers.debarred, which I think is the purposo of these
> patches.
> 
> Steps to reproduce:
> 1. same as A
> 2. same as A
> 3. same as A
> 4. check in the item that will produce debarrement
> 5. patron gets debarred and a date is set in borrowers.debarred
> 6. lift debarrement and check DB:
> 
> select debarred from borrowers where borrowernumber = <subject_patron_number>;
> 
> In my test:
> 
> select debarred from borrowers where borrowernumber = 5;
> +----------+
> | debarred |
> +----------+
> | NULL     |
> +----------+
> 
> So my patron is really NOT debarred now.
> 
> 7. try to loan an item and you'll get this message:
> 
> Patron has had overdue items and is blocked for 3 day(s).
> 
Well spotted ! It seems the behaviour is different on biblibre/master, investigating, there are some code diffs as well. Maybe a patch that has not been reported.
Comment 25 Paul Poulain 2011-05-27 13:20:36 UTC Comment hidden (obsolete)
Comment 26 Fernando L. Canizo 2011-05-30 16:09:33 UTC
I tested patch on comment 25 and it fixes B and C from my comment 23, A was a feature according to Paul so everything's fine now. I sent sign-off to mailing list. Status was already signed-off so no need to update.
Comment 27 Paul Poulain 2011-09-02 09:15:46 UTC
*** Bug 6775 has been marked as a duplicate of this bug. ***
Comment 28 Paul Poulain 2011-09-02 09:44:51 UTC
GBSD#4 rebasing this one
Comment 29 Paul Poulain 2011-09-02 10:16:35 UTC Comment hidden (obsolete)
Comment 30 Zeno Tajoli 2011-09-23 13:27:01 UTC Comment hidden (obsolete)
Comment 31 Zeno Tajoli 2011-09-23 13:31:15 UTC
For me patch signed-off
Comment 32 Ian Walls 2011-10-07 20:04:52 UTC
Doesn't update all instances of $borrower->{'debarred'}; there are usages on the following files that are not updated to reflect the change data type for 'debarred':

opac/opac-reserve.pl
opac/opac-user.pl
members/setdebar.pl
misc/cronjobs/notifyMailsOp.pl
C4/ILSDI/Utility.pm

C4::Members::patronflags() is not touched, so the DBARRED flag in GetMemberDetails is never set; thus any place in the code to use this is not handled:  This includes:

C4/SIP/ILS/Patron.pm
C4/Circulation::CanBookBeIssued
circ/circulation.pl

There is some documentation on C4/Auth_with_ldap.pm that needs updating as well.

This change will likely break opac-reserve.pl, ILSDI and SIP2 connections, as well as return erroneous values for CanBookBeIssued.  Marking as Failed QA.
Comment 33 Paul Poulain 2011-10-11 13:55:57 UTC
Comment on attachment 5283 [details] [review]
proposed patch squashed & rebased

obsoleted by zeno updated patch
Comment 34 Paul Poulain 2011-10-11 16:28:50 UTC Comment hidden (obsolete)
Comment 35 Ian Walls 2011-10-13 17:12:19 UTC
members/setdebar.pl is only used in the link on moremember.tt, which unblocks the patron.  The query executed merely sets 'debarred' to $status, for whatever status is input in the form.   Previously, when debarred was an integer, this was 0.  Now that debarred is a date, and there is a debarred comment, the values submitted to the form should be updated to reflect this, or the script itself should be changed to more thoroughly handle $status = 0.

The opac/opac-user.pl call is correct, you're right.

If notifyMailsOp.pl is deprecated, no worries there.

A few other issues I'm noticing:

This patch also changes the output of IsMemberBlocked, from returning the number of days they patron is blocked to the date they're blocked until.  This means that issuingimpossible{USERBLOCKEDREMAINING} in C4::Circulation::CanBookBeIssued is populated with a date instead of an integer, breaking the display circ/circulation.tt (<li>Patron has had overdue items and is blocked for [% USERBLOCKEDREMAINING %] day(s).</li>).  A simple change to circ/circulation.tt to handle this date correctly would suffice.  This would introduce string changes.

The new documentation in C4/Auth_with_ldap.pm is off by a couple spaces... the indentation is off by about a full tab's worth for debarredcomment.

KohaTest/Members/DebarMember.pm does not pass a date in it's tests, likely making the tests fail (or return false success).
Comment 36 Paul Poulain 2011-10-21 16:22:24 UTC Comment hidden (obsolete)
Comment 37 Paul Poulain 2011-10-21 16:24:42 UTC
ian, back to "please QA".
(In reply to comment #35)
> members/setdebar.pl is only used in the link on moremember.tt, which unblocks
> the patron.  The query executed merely sets 'debarred' to $status, for whatever
> status is input in the form.   Previously, when debarred was an integer, this
> was 0.  Now that debarred is a date, and there is a debarred comment, the
> values submitted to the form should be updated to reflect this, or the script
> itself should be changed to more thoroughly handle $status = 0.

OK, fixed in the "follow-up for 6238 uploaded just now

> This patch also changes the output of IsMemberBlocked, from returning the
> number of days they patron is blocked to the date they're blocked until.  This
> means that issuingimpossible{USERBLOCKEDREMAINING} in
> C4::Circulation::CanBookBeIssued is populated with a date instead of an
> integer, breaking the display circ/circulation.tt (<li>Patron has had overdue
> items and is blocked for [% USERBLOCKEDREMAINING %] day(s).</li>).  A simple
> change to circ/circulation.tt to handle this date correctly would suffice. 
> This would introduce string changes.

Haven't you missed the "follow-up to deal with QA comments" patch ?, because it removes this string that was useless here. At least, I can't find it anymore on my setup.

> The new documentation in C4/Auth_with_ldap.pm is off by a couple spaces... the
> indentation is off by about a full tab's worth for debarredcomment.

OK, fixed

> KohaTest/Members/DebarMember.pm does not pass a date in it's tests, likely
> making the tests fail (or return false success).

OK fixed
Comment 38 Paul Poulain 2011-11-14 16:18:32 UTC
Ian, this bug is really important, sorry to bug you once again, bug I'd like to see it in 3.6.1 !
Comment 39 Ian Walls 2011-11-19 15:07:38 UTC
Paul,


Okay, all QA comments dealt with but one:

> This patch also changes the output of IsMemberBlocked, from returning the
> number of days they patron is blocked to the date they're blocked until.  This
> means that issuingimpossible{USERBLOCKEDREMAINING} in
> C4::Circulation::CanBookBeIssued is populated with a date instead of an
> integer, breaking the display circ/circulation.tt (<li>Patron has had overdue
> items and is blocked for [% USERBLOCKEDREMAINING %] day(s).</li>).  A simple
> change to circ/circulation.tt to handle this date correctly would suffice. 
> This would introduce string changes.

The faulty string in question is on circ/circulation.tt, around line 390.  Change it to "<li>Patron has had overdue items, and is blocked until [% USERBLOCKEDREMAINING %]</li>" would be sufficient.  Since this is just a minor template only language issue (the key data is being presented, just not in a correct sentence), I don't think it's worth holding up the QA on this any more.

Marking as Passed QA.
Comment 40 Paul Poulain 2011-11-25 13:29:45 UTC
patch pushed, please test
Comment 41 Chris Cormack 2011-11-27 07:25:31 UTC
This is breaking the translations, 


 perl xt/tt_valid.t
1..1
not ok 1 - TT syntax: not using TT directive within HTML tag
#   Failed test 'TT syntax: not using TT directive within HTML tag'
#   at xt/tt_valid.t line 55.
# Files list: 
# intranet-tmpl/prog/en/modules/members/memberentrygen.tt: 1148, 1161
# Looks like you failed 1 test of 1.


Will submit a patch to fix this
Comment 42 Chris Cormack 2011-11-27 07:31:41 UTC Comment hidden (obsolete)
Comment 43 Katrin Fischer 2011-11-27 09:02:55 UTC
Created attachment 6429 [details] [review]
[SIGNED-OFF] Bug 6328 : Previous patches broke templates, this is fixing them

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Fixes xt/tt_valid.t
Checked the member entry page, didn't find any problems.
Comment 44 Paul Poulain 2011-11-28 09:12:55 UTC
follow-up pushed, thanks chris & katrin
Comment 45 Larry Baerveldt 2011-11-29 16:22:46 UTC
FYI, see new bug 7272. Changing the debarred field to a date field seems to have changed the default value of '0' to '0000-00-00', which other functions interpret as the borrower being frozen. Users were not able to login to their accounts after this fix was applied, and SIP2 users were unable to authenticate.

- Larry
Comment 46 Paul Poulain 2012-01-19 15:52:18 UTC
Created attachment 7248 [details] [review]
Merge remote-tracking branch 'origin/new/bug_6328'

The Delta_Days sub is called only when a patron that is already debarred must be debarred for a longer period.
This (rare) case probably hasn't be tested during QA, because the sub is not loaded.
Loading it fixes the problem

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Comment 47 Paul Poulain 2012-01-19 15:53:32 UTC
Lyon 3 has detected a problem and provided a patch. I've signed it & pushed to master. See http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=7248 for more details
Comment 48 Jared Camins-Esakov 2012-05-23 12:55:27 UTC
This was not intended for 3.6. Seems to be working now, so I'm closing the bug.