Bug 5644 - dropbox mode doesn't set the correct date
Summary: dropbox mode doesn't set the correct date
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low critical (vote)
Assignee: Kyle M Hall
QA Contact: Paul Poulain
URL:
Keywords:
: 8087 (view as bug list)
Depends on:
Blocks: 8338
  Show dependency treegraph
 
Reported: 2011-01-20 16:23 UTC by Nicole C. Engard
Modified: 2019-06-27 09:24 UTC (History)
7 users (show)

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


Attachments
Bug 5644 : Fixing dropbox mode calculation (747 bytes, patch)
2012-06-29 21:06 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 5644 : Fixing dropbox mode calculation (805 bytes, patch)
2012-07-03 11:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5644 : Fixing the test (698 bytes, patch)
2012-07-13 20:26 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2011-01-20 16:23:55 UTC
It was my understanding that checking the dropbox mode option on the check in screen made the due date effectively the last day the library was open and as such wiped out fines for the extra day.  It appears that is not the case.  Fines for the item being late remain on the patron's account.

The only way around this is to 'forgive fines' when using dropbox mode, but that forgives all fines and really we only want to forgive one day of fines.
Comment 1 Nicole C. Engard 2011-04-28 11:54:22 UTC
Has anyone else noticed this issue? I'm hoping it's an easy fix.

Nicole
Comment 2 Nicole C. Engard 2011-06-09 16:33:23 UTC
Just a little nudge again :)
Comment 3 Jesse Weaver 2011-07-07 01:23:41 UTC
I can take a look at this, but my understanding is that a fines rewrite is in progress, right? Might be better to fix once we have a better foundation, unless this is extremely crucial and that's going to take a long time.
Comment 4 Chris Cormack 2011-07-07 01:34:10 UTC
Speaking of that, there is stuff ready for initial testing

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=6427
Comment 5 Melia Meggs 2012-06-29 18:33:41 UTC
This bug is kind of a big deal.  Libraries need to be able to use dropbox mode to prevent the fine calculation from happening in the morning when they are checking in books that were put in the dropbox the night before.  Right now in 3.8, they are not able to backdate the checkin date, so fines are being incorrectly charged on all books that come in from the dropbox.  That's a lot of extra fines to remove manually every single day!  Can we fix just the checkin date part of this without waiting on the entire Accountlines Rewrite?
Comment 6 Chris Cormack 2012-06-29 20:06:47 UTC
Hmm there are 2 different things here Melia.

Backdating the  checkin date, and wiping fines.

Backdating the checkin date always used to work, and was working with this bug was logged. What wasn't working was that if the fines had already run things were not wiped.

If you are saying now that you can't even backdate the checkin date that is a different problem and something broken recently.

Is that the case? IF so, fixing the backdating will still not remove the fines, but if the books are checked in before the fines script runs they will not get fines added to them.

So .. is setting the backdate broken now too? Or can you still do that and is the problem that you still don't wipe fines?
Comment 7 Melia Meggs 2012-06-29 20:31:40 UTC
Ah ok. Thanks for the explanation.

I'm saying that backdating the checkin date is not working. (Maybe this needs a new bug report?)  

The library runs the fines script at noon.  Before they upgraded to 3.8, they used to checkin the books from the dropbox in the morning and backdate the checkin date, so no fines were added for that day when the fines script ran at noon.  Now they cannot backdate the checkin date, so fines get charged on those books.
Comment 8 Chris Cormack 2012-06-29 20:36:32 UTC
Right that is a separate issue from the one nicole reported, which was an ehancement request to wipe fines already charged.

Two choices, rename the description of this to be, dropbox mode doesnt set the correct date and leave it critical, and make a new one for the wiping fines, an ehancement, that is blocked by this. Or vice versa.

I'm looking into the dropbox not setting the date issue now.
Comment 9 Melia Meggs 2012-06-29 20:47:57 UTC
Thank you, Chris!
Comment 10 Chris Cormack 2012-06-29 20:59:48 UTC
The problem is in Koha::Calendar


if ( $add_duration->is_negative() ){
  $day_dur->inverse();
}

This does not do what we expect, namely it does not invert the value.

Working on a fix now
Comment 11 Chris Cormack 2012-06-29 21:06:55 UTC Comment hidden (obsolete)
Comment 12 Chris Cormack 2012-06-29 21:11:53 UTC
+----------------+------------+---------------------+------------+---------------+---------------------+-----------------+--------+----------+---------------------+---------------------+
| borrowernumber | itemnumber | date_due            | branchcode | issuingbranch | returndate          | lastreneweddate | return | renewals | timestamp           | issuedate           |
+----------------+------------+---------------------+------------+---------------+---------------------+-----------------+--------+----------+---------------------+---------------------+
|          15861 |      23035 | 2012-07-21 23:59:00 | ME         | NULL          | 2012-06-29 09:10:00 | NULL            | NULL   |     NULL | 2012-06-30 09:10:43 | 2012-06-30 09:09:00 |
+----------------+------------+---------------------+------------+---------------+---------------------+-----------------+--------+----------+---------------------+---------------------+


Looks to be working now.
Comment 13 Kyle M Hall 2012-07-02 11:42:39 UTC
Just looking at the diff, I'm wondering if this patch only fixes it if the library has only been closed for one day, and not for multiple days at a time. Nicole's description of her understanding of the feature is that "checking the dropbox mode option on the check in screen made the due date effectively the last day the library was open". What do you think?
Comment 14 Katrin Fischer 2012-07-02 12:00:53 UTC
Hi Kyle, can't say anything about the code, but you described it right I think. That's how the feature has been documented and how I would expect it to work. So if people give something back over the weekend and I check it in Monday, the date should probably be Friday?
Comment 15 Nicole C. Engard 2012-07-02 12:04:22 UTC
Yes, that's what should happen.  Dropbox means that the patrons used the drop box for book/materials outside of the library.  Many libraries don't penalize patrons who drop materials off while the library is closed so when the library opens again they take everything out of the box, and want to check the items in as if they were returned on a day the library was open.  The intended side effect of this is that the patrons are not charged fines for the days the library was closed (if the library uses this feature).  For libraries who don't have this policy they just don't use the drop box mode.

Nicole
Comment 16 Albert Oller 2012-07-02 12:53:57 UTC
*** Bug 8087 has been marked as a duplicate of this bug. ***
Comment 17 Chris Cormack 2012-07-02 18:12:52 UTC
(In reply to comment #13)
> Just looking at the diff, I'm wondering if this patch only fixes it if the
> library has only been closed for one day, and not for multiple days at a
> time. Nicole's description of her understanding of the feature is that
> "checking the dropbox mode option on the check in screen made the due date
> effectively the last day the library was open". What do you think?

This is why just looking at a diff is an entirely ineffective way to test a patch.

This bug now is to do with the dropbox not setting the correct date. Anything to do with fines is on another bug. So please talk about that issue there.
Bug 8338

But if you look at the whole of the code, or better yet run it.
You will see all the patch changes is setting the day_duration to be negative. It then, (depending on the mode the calendar is in, or if its hours does different things)

EG

  my $days = abs $add_duration->in_units('days');
            while ($days) {
                $base_date->add_duration($day_dur);
                if ( $self->is_holiday($base_date) ) {
                    next;
                } else {
                    --$days;
                }
            }

If the library is closed, it will keep subtracting (in this case because $day_dur) is negative.

This is how this had always worked, up until 3.8.0 and so that regression is now fixed.

Please, looking at diffs is not really a valid test of anything.
Comment 18 Kyle M Hall 2012-07-03 11:03:17 UTC Comment hidden (obsolete)
Comment 19 Paul Poulain 2012-07-10 13:49:38 UTC
mmm... I need a test plan, before pushing this patch:I tried -without the patch- to check-in a book, I checked "dropbox mode"
I made a check-out of a book, return date july 31th
Then I went to check-in, checked "Dropbox mode" button, made the checkin, and, in the database, I see:
 borrowernumber: 7852
     itemnumber: 23170
       date_due: 2012-07-31 23:59:00
     branchcode: MAURES
  issuingbranch: NULL
     returndate: 2012-07-09 15:37:00  <<<<< YESTERDAY, dropbox mode OK
lastreneweddate: NULL
         return: NULL
       renewals: NULL
      timestamp: 2012-07-10 15:37:45
      issuedate: 2012-07-10 14:46:00
things seems OK.

Any hint to understand what is fixed and I'll push it !

In the meantime = tiny patch, passed QA
Comment 20 Katrin Fischer 2012-07-10 13:53:41 UTC
Hi Paul,

I think the way to test this involves the holiday calendar. It should always use the last opening day as return date - so the question is probably if it does work correctly with single, weekly and repeated holidays between between the last opening day and the checkin day.
Comment 21 Chris Cormack 2012-07-11 06:13:41 UTC
Yep, it depends on what calendar mode you have set, whether it works or not.

The bug only manifests if you have days mode set to calendar (not date due).
Comment 22 Paul Poulain 2012-07-13 09:27:01 UTC
(In reply to comment #21)
> Yep, it depends on what calendar mode you have set, whether it works or not.
> 
> The bug only manifests if you have days mode set to calendar (not date due).

Thanks,

Testing =
* set useDaysMode to "calendar"
* reach return.pl page, click "dropbox mode, the display is "Dropbox Mode. (Effective checkin date is 14/07/2012 11:20 ).", which is wrong.
* applying patch 
* reach return again, the display is "Dropbox Mode. (Effective checkin date is 11/07/2012 11:25 )." 12th is marked as closed => it's OK !

patch pushed !
Comment 23 Chris Cormack 2012-07-13 20:24:25 UTC
   Failed test 'Negative call to addDate'
#   at t/Kalendar.t line 62.
#     '2011-06-15'
#         cmp
#     '2011-06-15'
# Looks like you failed 1 test of 9.

Weird thing happening here, that looks ok to me
Comment 24 Chris Cormack 2012-07-13 20:26:45 UTC
Created attachment 10835 [details] [review]
Bug 5644 : Fixing the test
Comment 25 Chris Cormack 2012-07-13 20:27:35 UTC
Fix for the test suite
Comment 26 Paul Poulain 2012-07-16 10:43:17 UTC
Patch fixes the test, signed off & passed QA & push in one go
Comment 27 Chris Cormack 2012-07-17 05:40:54 UTC
Pushed,will be in 3.8.3