Created attachment 10289 [details] [review] Patrons systematically debarred at checkin When suspension in days are used, the delta days between due-date and checkindate is tested in _FixFineDaysOnReturn line 1843 : if ( ( $deltadays - $grace )->is_positive ) and if positive the patron is debarred. The problem is that it's always positive even if the borrower is not late because the delta_days function of the Datetime module used in 'days_between' function always return a positive delta (from what I understood) Test plan : 1) configure a rule in issuingrules that includes a suspension in days 2) do a checkout for a borrower and document type corresponding to the rule. 3) do a checkin
Created attachment 10290 [details] [review] Patrons systematically debarred at checkin Oups ! I left a dirty bare i on first version.
Created attachment 10308 [details] [review] Bug 8251 - Patrons get incorrectly debarred Patrons get incorrectly debarred because of the use of Datetime->delta_days wich happens to always return a positive number. Also changed the use of Datetime->truncate function so it follows the docs, using 'day' instead of 'days' (which is used in DateTime::Duration). This patch has been tested against master.
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.
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 :-/
Correctly reimplements duration as signed instead of absolute value. Marking Passed QA.
Created attachment 10418 [details] [review] 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.
Changing status to needs signoff
Holding off on pushing this to 3.8.x until the followup is signed off, passed qa, and pushed to master also
Created attachment 10597 [details] [review] Patrons get incorrectly debarred From a duration calculated with "$duration= $datestart_temp - $dateend_temp" subtract method (used to exclude holidays) can't apply correctly because it only do subtraction to the day value and not the global amount of month and days. Depending on the amount of closed holidays, this may end with a negative value for the days : a patron may not be restricted even with much late. example : A patron is 1 month and 3 days late. There are 6 days closed for holidays. Duration hash will look like this : $VAR1 = bless( { 'seconds' => 0, 'minutes' => 0, 'end_of_month' => 'wrap', 'nanoseconds' => 0, 'days' => -3, 'months' => 1 }, 'DateTime::Duration' ); May be there is an adequate Datetime::Duration method to calculate correctly but I could'nt find it in Datetime jungle. So, I came back to my first idea : keeping the calculation of duration with delta_days method (with this one the duration value is set only in day units and so subtract method applies correctly), then checking for late and if not, make the delta number negative.
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 ?
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.
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.
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>
Created attachment 10768 [details] [review] Patrons get incorrectly debarred Patrons get incorrectly debarred because of the use of Datetime->delta_days wich happens to always return a positive number. Added a check to inverse the delta when patron is not late. Also changed the use of Datetime->truncate function so it follows the docs, using 'day' instead of 'days' (which is used in DateTime::Duration).
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 :-)
(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 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...
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
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
(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?
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.
> 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...
Created attachment 10914 [details] [review] Patrons get incorrectly debarred Patrons get incorrectly debarred because of the use of Datetime->delta_days wich happens to always return a positive number. Added a check to inverse the delta when patron is not late. Also changed the use of Datetime->truncate function so it follows the docs, using 'day' instead of 'days' (which is used in DateTime::Duration). http://bugs.koha-community.org/show_bug.cgi?id=8251 Signed-off-by: Tajoli Zeno <tajoli@cilea.it>
(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
(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
Still waiting on the final patch before pushing to 3.8.x
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.
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.
(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 :(
(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 on attachment 10914 [details] [review] Patrons get incorrectly debarred True problem but wrong patch, the patch on bug 8486 is the proper one
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 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
(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.
Second patch (follow up) pushed to 3.8.x, will be in 3.8.4
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)
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
(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.
Created attachment 11976 [details] [review] Proposed Patch This patch attempts to clarify the logic and the routine is now called only for an overdue return.
switch status back to requires signoff for latest patch
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>
(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
"Do not try to debar patrons if returns are not overdue" follow-up pushed
Pushed to 3.8.x will be in 3.8.5
Just checked that the problem is no longer present in 3.8.5. Yay!