Bug 8251 - Patrons are systematically debarred at checkin
Summary: Patrons are systematically debarred at checkin
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: 3.8
Hardware: All All
: P1 - high critical (vote)
Assignee: Colin Campbell
QA Contact: Mason James
URL:
Keywords:
Depends on: 8486
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-15 11:53 UTC by Koha Team University Lyon 3
Modified: 2019-06-27 09:24 UTC (History)
13 users (show)

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


Attachments
Patrons systematically debarred at checkin (1.14 KB, patch)
2012-06-15 11:53 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Patrons systematically debarred at checkin (1.06 KB, patch)
2012-06-15 14:04 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 8251 - Patrons get incorrectly debarred (1.37 KB, patch)
2012-06-15 17:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8251 - Patrons get incorrectly debarred (1.39 KB, patch)
2012-06-18 10:35 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8251 - Follow up, use 'day' instead of 'day' for Datetime::truncate (7.28 KB, patch)
2012-06-21 18:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Patrons get incorrectly debarred (1.79 KB, patch)
2012-07-02 09:52 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8251 - Follow up, use 'day' instead of 'day' for Datetime::truncate (7.29 KB, patch)
2012-07-11 13:10 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Patrons get incorrectly debarred (1.64 KB, patch)
2012-07-11 13:10 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Patrons get incorrectly debarred (1.75 KB, patch)
2012-07-18 00:24 UTC, Mason James
Details | Diff | Splinter Review
Proposed Patch (6.62 KB, patch)
2012-09-05 12:09 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 8251 Do not try to debar patrons if returns are not overdue (6.70 KB, patch)
2012-09-05 13:49 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 Koha Team University Lyon 3 2012-06-15 11:53:09 UTC Comment hidden (obsolete)
Comment 1 Koha Team University Lyon 3 2012-06-15 14:04:11 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2012-06-15 17:59:20 UTC Comment hidden (obsolete)
Comment 3 Julian Maurice 2012-06-18 10:35:48 UTC
Created attachment 10348 [details] [review]
[SIGNED-OFF] Bug 8251 - Patrons get incorrectly debarred

I can confirm the problem, and that the patch fix it.
Signed off.
Comment 4 Jonathan Druart 2012-06-18 12:10:46 UTC
QA Comments:

This patch looks good and fixes the reported issue.

But a followup is required for several reasons:
1/ Il would be great to replace all occurrences of the truncate routine : 
    truncate( to => 'days' );
with
    truncate( to => 'day' );

(with specifications from doc http://search.cpan.org/~drolsky/DateTime-0.75/lib/DateTime.pm)

2/ I think that the same fix should be made for the routine hours_between. But I am not sure about this :-/
Comment 5 Ian Walls 2012-06-20 18:56:26 UTC
Correctly reimplements duration as signed instead of absolute value.  Marking Passed QA.
Comment 6 Tomás Cohen Arazi 2012-06-21 18:47:01 UTC Comment hidden (obsolete)
Comment 7 Julian Maurice 2012-06-22 07:40:37 UTC
Changing status to needs signoff
Comment 8 Chris Cormack 2012-06-23 19:17:52 UTC
Holding off on pushing this to 3.8.x until the followup is signed off, passed qa, and pushed to master also
Comment 9 Koha Team University Lyon 3 2012-07-02 09:52:33 UTC Comment hidden (obsolete)
Comment 10 Zeno Tajoli 2012-07-05 15:39:57 UTC
I want to test this patch.
But is not clear what I need to do.
I need to use all two patches that are valid ?

Do I need to implement as first the older (from Tomás Cohen Arazi) and after the patch of Koha Team Lyon 3 ?
Comment 11 Koha Team University Lyon 3 2012-07-06 09:01:41 UTC
I suppose that if you test a checkin from a loan in days  "Patrons get incorrectly debarred" patch should be enough (truncate function used in days_between routine is corrected into it)  but if you do it on an hours loan, you should have both patches applied.
Comment 12 Zeno Tajoli 2012-07-06 14:32:31 UTC
The fixes are for 3.8.x or for master ?
I insert them in 3.8.2 by hand because i have same little problems with patches.
I installed the two patches and for me they work.
Comment 13 Dobrica Pavlinusic 2012-07-11 13:10:42 UTC
Created attachment 10767 [details] [review]
[SIGNED-OFF] Bug 8251 - Follow up, use 'day' instead of 'day' for Datetime::truncate

As noted in comment #4 by Jonathan Druart, this should be fixed in every call
to DateTie::truncate function. This patch does exactly that.

julian_m tested that truncate( to => 'days' ) didn't actually do anything, so
it is understandable that this 'fix' might introduce new bugs as we might have
'fixed' at a higher libs level this issue.

'minutes' is used in truncate function too, so fixing.

Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org>
Comment 14 Dobrica Pavlinusic 2012-07-11 13:10:50 UTC Comment hidden (obsolete)
Comment 15 Dobrica Pavlinusic 2012-07-11 13:13:14 UTC
I signed off first patch because I would really love to see it in Koha ASAP :-)

Unfortunately, I can't really test properly debarred code, but I did re-base it on top of current master, so Zeno if you could sign it off, this would be great :-)
Comment 16 Paul Poulain 2012-07-16 11:45:10 UTC
(In reply to comment #15)
> I signed off first patch because I would really love to see it in Koha ASAP
> :-)

QA comment for the 1st patch = I've checked that the API is singular, and not plural (day, minute,...) The 1st patch (http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10767) fixes only that, passed QA
> 

> Unfortunately, I can't really test properly debarred code, but I did re-base
> it on top of current master, so Zeno if you could sign it off, this would be
> great :-)

Zeno, i'll wait for your signoff before pushing the http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10768, but from QA pov, I don't see any reason not to pass QA
Comment 17 Paul Poulain 2012-07-16 11:48:39 UTC
Comment on attachment 10767 [details] [review]
[SIGNED-OFF] Bug 8251 - Follow up, use 'day' instead of 'day' for Datetime::truncate

This patch has been pushed.

Note that there could be some side-effect I haven't identified: before this patch, there was some calculations that were wrong. If another calculation was wrong somewhere else, it could result in a correct result. With this patch, as the 1st calculation is now correct, an hypothetical 2nd (wrong) will become visible. I hope there are none...
Comment 18 Zeno Tajoli 2012-07-16 13:05:54 UTC
Hi to all,
I tested this patch [last version, http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10768] with sandboxes.
I confirm that now patrons are NOT systematically debarred at checkin.

So I sign-off the patch
Comment 19 Koha Team University Lyon 3 2012-07-16 14:39:38 UTC
It solves the problem of patrons getting systematically debarred but create a new one  : false calculation of closed days (as I had already explained in comment 9 and for the same reason that has been also noted in bug 8110). 
the subtraction of closed days, line 189 ($duration->subtract( days => 1 ) is not correct when duration is greater than one month because subtract method only subtracts days from days amount and not month one.

When initial duration is calculated with a subtraction between 2 datetime objects (my $duration = $dateend_temp - $datestart_temp), here's the hash you could get after the closed days had been 'subtracted' in "while ( DateTime->compare" loop :
example : the patron is 65 days late, there are 14 days closed
    $VAR1 = bless( {
                   'seconds' => 0,
                   'minutes' => 0,
                   'end_of_month' => 'wrap',
                   'nanoseconds' => 0,
                   'days' => -10,
                   'months' => 2
                 }, 'DateTime::Duration' ); 
So the patron won't be restriced at all !!

But if initial duration is calculated with delta_days fonction, with the same example you'll obtain the following  :
$VAR1 = bless( {
                   'seconds' => 0,
                   'minutes' => 0,
                   'end_of_month' => 'wrap',
                   'nanoseconds' => 0,
                   'days' => 51,
                   'months' => 0
                 }, 'DateTime::Duration' ); 

So, this way, it's ok, the patron will be restricted for 51 days.
Others solution would be to calculate duration with integer and not duration object as suggested in bug 8110 discussion or to find a way to subtract closed day in correct way
Comment 20 Mason James 2012-07-16 23:22:51 UTC
(In reply to comment #18)
> Hi to all,
> I tested this patch [last version,
> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10768] with
> sandboxes.
> I confirm that now patrons are NOT systematically debarred at checkin.
> 
> So I sign-off the patch

Zeno, 

you need to add a sign-off message to that patch, before it is properly signed-off

like this...
 Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

can you do that please?
Comment 21 Zeno Tajoli 2012-07-17 10:18:02 UTC
Hi to all,

today with this commit,
http://git.koha-community.org/gitweb/?p=koha.git;a=commit;h=9d4e241adadc9a3c62d94a2d487a738e9490925a
the problem [Patrons are systematically debarred at checkin] is not more present in master.

I test it with sanboxes
But I don't know what to do.
Is it usesul to test the last Dobrica's patch with the present master ?
I see comment #19 but is very difficult to test so complex situation.

For comment #20: Yesterday I test the path with sanboxes so I don't sign the patch.
Comment 22 Mason James 2012-07-17 23:01:23 UTC
 
> For comment #20: Yesterday I test the path with sanboxes so I don't sign the
> patch.


Zeno, 

i understand it is possible to sign-off patches using the sandbox...
Comment 23 Mason James 2012-07-18 00:24:08 UTC Comment hidden (obsolete)
Comment 24 Mason James 2012-07-18 00:33:42 UTC
(In reply to comment #23)
> Created attachment 10914 [details] [review]
> Patrons get incorrectly debarred
> Signed-off-by: Tajoli Zeno <tajoli@cilea.it>

Zeno, i've added your sign-off (this time... :)

fyi: i used git-bz to sign your patch, it took me 60 seconds ;)
 http://wiki.koha-community.org/wiki/Git_bz_configuration
Comment 25 Mason James 2012-07-18 00:37:49 UTC
(In reply to comment #18)
> Hi to all,
> I tested this patch [last version,
> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10768] with
> sandboxes.
> I confirm that now patrons are NOT systematically debarred at checkin.
> 
> So I sign-off the patch

passing QA on this patch

$ koha-qa.pl
- 58cc95a Patrons get incorrectly debarred
        Koha/Calendar.pm
- perlcritic-progressive tests... OK
- perl -c syntax tests... OK
- xt/tt_valid.t tests... OK
- xt/author/vaild-template.t tests... OK
Comment 26 Chris Cormack 2012-07-18 07:46:03 UTC
Still waiting on the final patch before pushing to 3.8.x
Comment 27 Koha Team University Lyon 3 2012-07-18 08:21:11 UTC
This patch should not be pushed because it causes false calculation of late delay when patron is more than one month late.

Here's how you can test it :

First you have to install patch of Bug 8348 (http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10619) to correct 2 errors on day closed calculation (if not, day_month_closed_days and single_holidays won't be subtracted in 'while( DateTime...' loop, so the test would be falsed). 
Another patch fixes these errors and could be simpler and prefered but I have not tested it (bug 8418 : http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10840).

Then you must have Follow-up of this page (http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10418) installed too.

In our install the finesCalendar syspref is set to 'not including days the library is closed' and the useDaysMode syspref to 'the calendar to push the due date to the next open day'.

Go to checkout tab of a patron.
paste a barcode in barcode textbox

In "specify due date" textbox set a date in order that the patron should be more than one month late (say '2012-05-07').
checkout and confirm

Go to Calendar Tool and set between date due and today a number of closed days slightly greater than the nth day (7)  of date due (say 9).

Checkin the barcode.

You 'll see that patron is not restricted.

Uninstall the previous "Patrons are systematically debarred at checkin" patch, Go to obsolete patches of this page and install this one (Patrons get incorrectly debarred, 2012-07-02) :
http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10597

Redo the same test.
Comment 28 Chris Cormack 2012-07-18 08:28:12 UTC
So far, none of the 3 patches are in 3.8.x

But the first 2 are in master.

So if none should be in master you will need to tell Paul that so he can remove them.

I am not pushing any to 3.8.x until this is working.
Comment 29 Paul Poulain 2012-07-20 16:09:59 UTC
(In reply to comment #27)
> This patch should not be pushed because it causes false calculation of late
> delay when patron is more than one month late.

mmm... could you look at http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7420#c29

I think there are interesting informations here. I spent 3 hours trying to understand what was happening :(
Comment 30 Paul Poulain 2012-07-25 15:37:31 UTC
(In reply to comment #28)
> So far, none of the 3 patches are in 3.8.x
> 
> But the first 2 are in master.
> 
> So if none should be in master you will need to tell Paul that so he can
> remove them.
> 
> I am not pushing any to 3.8.x until this is working.

I hope we've solved the problem:
 * There was a badly wrong calculation in days_between, proven on bug 8486, and also fixed here by Colin.
 * The last patch attached here (http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10914) should not be pushed, as it tries to address (uncorrectly) the problem fixed on bug 8486
 * The 2 other patches can be applied (c6f1f64cb71e9d8ec560a201a4a0bc16c1510f3f & 9d4e241adadc9a3c62d94a2d487a738e9490925a = obvious API mistake, see comment 16 and comment 17 )


Chris_c = team Lyon 3 is AFK, until something like Aug 20th. I think we should not wait until they come back to push those 2 patches. So please check I'm not wrong, but I don't think so

Dobrica, Mason & Tomas, you're welcomed to jump in the discussion too !
Comment 31 Paul Poulain 2012-07-25 15:38:08 UTC
Comment on attachment 10914 [details] [review]
Patrons get incorrectly debarred

True problem but wrong patch, the patch on bug 8486 is the proper one
Comment 32 Paul Poulain 2012-07-25 15:39:16 UTC
Comment on attachment 10348 [details] [review]
[SIGNED-OFF] Bug 8251 - Patrons get incorrectly debarred

This patch has already been pushed, unobsoleting it just to have a clean bug status with the 2 patches applied visible
Comment 33 Paul Poulain 2012-07-25 15:39:37 UTC
Comment on attachment 10767 [details] [review]
[SIGNED-OFF] Bug 8251 - Follow up, use 'day' instead of 'day' for Datetime::truncate

This patch has already been pushed, unobsoleting it just to have a clean bug status with the 2 patches applied visible
Comment 34 Chris Cormack 2012-07-26 08:16:07 UTC
(In reply to comment #30)
> (In reply to comment #28)
> > So far, none of the 3 patches are in 3.8.x
> > 
> > But the first 2 are in master.
> > 
> > So if none should be in master you will need to tell Paul that so he can
> > remove them.
> > 
> > I am not pushing any to 3.8.x until this is working.
> 
> I hope we've solved the problem:
>  * There was a badly wrong calculation in days_between, proven on bug 8486,
> and also fixed here by Colin.
>  * The last patch attached here
> (http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10914) should
> not be pushed, as it tries to address (uncorrectly) the problem fixed on bug
> 8486

The patch on bug 8486 does not apply cleanly on 3.8.x

Does it need to to be pushed after these two?  

Not pushing to 3.8.x until there is a clear plan and they all apply cleanly.
Comment 35 Chris Cormack 2012-08-06 20:56:38 UTC
Second patch (follow up) pushed to 3.8.x, will be in 3.8.4
Comment 36 Magnus Enger 2012-09-03 09:46:18 UTC
Sorry, but I'm still seeing this behaviour on both 3.8.4 (running off the official packages) and current master (3.09.00.032, dev-install): 

1. Check out then return = OK
2. Add a circ rule that includes "Suspension in days" = 7
3. Check out then return = Patron is debarred until 18/03/2013 (March 18th next year, which seems like a very long and somewhat arbitrary period of time)
Comment 37 Colin Campbell 2012-09-05 10:30:31 UTC
There is a basic flaw in the logic in that patron's finedays are being calculated on all returns not just overdues. The delta days is a positive element that is the documented interface. Its purpose is not to inform if an issue is overdue that is already indicated in the return from GetItemIssue
Comment 38 Chris Cormack 2012-09-05 10:36:05 UTC
(In reply to comment #37)
> There is a basic flaw in the logic in that patron's finedays are being
> calculated on all returns not just overdues. The delta days is a positive
> element that is the documented interface. Its purpose is not to inform if an
> issue is overdue that is already indicated in the return from GetItemIssue

Ah yes that makes sense. Yeah, that is a flaw, we should only be working out the delta days on things that are overdue, instead of on all items. 

Now you have explained it, it makes perfect sense.
Comment 39 Colin Campbell 2012-09-05 12:09:23 UTC Comment hidden (obsolete)
Comment 40 Colin Campbell 2012-09-05 12:10:44 UTC
switch status back to requires signoff for latest patch
Comment 41 Tomás Cohen Arazi 2012-09-05 13:49:02 UTC
Created attachment 11979 [details] [review]
Bug 8251 Do not try to debar patrons if returns are not overdue

If a period of suspension is configured in the issuing rules
a calculation to debar the patron was called on all returns
It should be limited to overdue returns

Renamed _FixFineDaysOnReturn subroutine to _debar_user_on_return
which is more descriptive of its purpose
Removed some unnecessary or duplicated processing
Changed visibility of $today so it didnt need
calculating twice
Removed declaration of a datedue variable that is
never used

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 42 Mason James 2012-09-07 05:06:43 UTC
(In reply to comment #40)
> switch status back to requires signoff for latest patch
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

passing QA...
patch looks good and applies clean 3.8.x branch - thanx for the patch Colin

$ koha-qa.pl 

testing 1 commit(s) (applied to commit 4446e9b)
 * 912bfa8 Bug 8251 Do not try to debar patrons if returns are not overdue
      C4/Circulation.pm

- perlcritic-progressive tests...                 OK
- perl -c syntax tests...                         OK
- xt/tt_valid.t tests...                          OK
- xt/author/valid-template.t tests...             OK
- t/00-valid-xml.t tests...                       OK
Comment 43 Paul Poulain 2012-09-12 14:45:15 UTC
"Do not try to debar patrons if returns are not overdue" follow-up pushed
Comment 44 Chris Cormack 2012-09-12 21:19:39 UTC
Pushed to 3.8.x will be in 3.8.5
Comment 45 Magnus Enger 2012-09-27 06:57:50 UTC
Just checked that the problem is no longer present in 3.8.5. Yay!