Bug 12847 - Date comparisons in C4::Members::IssueSlip are broken
Summary: Date comparisons in C4::Members::IssueSlip are broken
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Katrin Fischer
URL:
Whiteboard:
Keywords:
: 8813 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-08-28 19:00 UTC by Barton Chittenden
Modified: 2015-12-03 22:02 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 12847 - Date comparisons in C4::Members::IssueSlip are broken (2.26 KB, patch)
2014-08-29 13:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12847 [Follwup] - Remove code that calculates "overdue" key as C4::Members::GetPendingIssues already creates it (1.16 KB, patch)
2014-08-29 13:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12847 [Followup] - Remove code that calculates "overdue" key as C4::Members::GetPendingIssues already creates it (1.51 KB, patch)
2014-08-29 14:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12847 [Follwup] - Remove code that calculates "overdue" key as C4::Members::GetPendingIssues already creates it (1.56 KB, patch)
2015-02-17 14:30 UTC, Barton Chittenden
Details | Diff | Splinter Review
[SIGNED OFF] Bug 12847 - Date comparisons in C4::Members::IssueSlip are broken (2.32 KB, patch)
2015-02-19 19:15 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED OFF] Bug 12847 [Follwup] - Remove code that calculates "overdue" key as C4::Members::GetPendingIssues already creates it (1.61 KB, patch)
2015-02-19 19:15 UTC, Nick Clemens
Details | Diff | Splinter Review
screenshot of the wrong display (75.75 KB, image/png)
2015-04-15 14:18 UTC, Katrin Fischer
Details
Bug 12847: Items issued today is considered as overdue (15.97 KB, patch)
2015-04-16 11:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED OFF] Bug 12847: Items issued today is considered as overdue (16.04 KB, patch)
2015-04-16 21:57 UTC, Nick Clemens
Details | Diff | Splinter Review
[PPASSED QA] Bug 12847: Items issued today is considered as overdue (16.10 KB, patch)
2015-04-17 10:16 UTC, Katrin Fischer
Details | Diff | Splinter Review
[3.18.x] Bug 12847: Items issued today is considered as overdue (15.78 KB, patch)
2015-04-22 05:56 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Barton Chittenden 2014-08-28 19:00:15 UTC
In C4/Members.pm:

2379 sub IssueSlip {
2380     my ($branch, $borrowernumber, $quickslip) = @_;
2381 
2382 #   return unless ( C4::Context->boolean_preference('printcirculationslips') );
2383 
2384     my $now       = POSIX::strftime("%Y-%m-%d", localtime);
2385 
2386     my $issueslist = GetPendingIssues($borrowernumber);
2387     foreach my $it (@$issueslist){
2388         if ((substr $it->{'issuedate'}, 0, 10) eq $now || (substr $it->{'lastreneweddate'}, 0, 10) eq $now) {
2389             $it->{'now'} = 1;
2390         }
2391         elsif ((substr $it->{'date_due'}, 0, 10) le $now) {
2392             $it->{'overdue'} = 1;
2393         }
2394         my $dt = dt_from_string( $it->{'date_due'} );
2395         $it->{'date_due'} = output_pref( $dt );;
2396     }

At line 2384, $now is being defined with the format "%Y-%m-%d", however this is being compared against 'issuedate', 'lastrenewdate' and 'date_due'.

'date_due' is formatted '09/04/2014 23:59'

The comparison only uses the first 10 characters of 'date_due', so we can ignore the timestamp at the end, but it's trying to compare 'YYYY-MM-DD' against 'MM/YY/DDDD', e.g. 2014-08-28 against 09/04/2014 -- This is a string comparison; so the initial '0' in '09/04/2014' sorts before the '2' in '2014-08-28'... since the date due is less than $now, the 'overdue' flag is set for this item.

I believe that all of the dates involved (issuedate, lastreneweddate, and date_due) are Koha::DateUtils objects; as such it should be possible to define $now the same way, and use Koha::DateUtils methods directly for date comparisons, rather than doing string comparisons.
Comment 1 Barton Chittenden 2014-08-28 19:16:50 UTC
All items checked out by a patron will show between <overdue></overdue> tags in ISSUESLIP.

To replicate:

go to 

cgi-bin/koha/tools/letter.pl?op=add_form&module=circulation&code=ISSUESLIP


Add the following section

<h4>Overdues</h4>
<overdue>
<p>
<<biblio.title>> <br />
Date due: <<issues.date_due>> at <<issues.branchcode>><br />
</p>
</overdue>

Check out an item. to a sample patron.

Print the issue slip for that patron.

Even though the item is not overdue, it will show in the 'Overdues' section.
Comment 2 Kyle M Hall 2014-08-29 13:42:36 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2014-08-29 13:48:37 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2014-08-29 14:10:02 UTC Comment hidden (obsolete)
Comment 5 Frédéric Demians 2014-10-25 15:30:30 UTC
Impossible to reproduce this bug. Following the test plan, at step 4, I don't have last issue printed in the 'overdue' section.
Comment 6 Chris Cormack 2014-10-27 05:26:17 UTC
(In reply to Frédéric Demians from comment #5)
> Impossible to reproduce this bug. Following the test plan, at step 4, I
> don't have last issue printed in the 'overdue' section.

I think it will depend on the date, also the syspref you have set.

eg if you have it dd/mm/yyyy

28/10/2014 > 2014-10-28 ..  so i think you need to make sure you have it set to mm/dd/yyyy (american format)
Comment 7 Frédéric Demians 2014-10-27 07:16:08 UTC
(In reply to Chris Cormack from comment #6)

> 28/10/2014 > 2014-10-28 ..  so i think you need to make sure you have it set
> to mm/dd/yyyy (american format)

I already tested switching dateformat do dd/mm/yyyy. It doesn't help. Sorry.
Comment 8 Barton Chittenden 2014-11-03 21:37:41 UTC
(In reply to Frédéric Demians from comment #7)
> (In reply to Chris Cormack from comment #6)
> 
> > 28/10/2014 > 2014-10-28 ..  so i think you need to make sure you have it set
> > to mm/dd/yyyy (american format)
> 
> I already tested switching dateformat do dd/mm/yyyy. It doesn't help. Sorry.

Frédéric: did you test dd/mm/yyyy or mm/dd/yyyy? The latter is the one we're having trouble with. Your reply above suggests that you tested the former, but I can't tell if you mis-typed your answer or mis-tested.

No big deal either way, I'm just looking for clarification.
Comment 9 Frédéric Demians 2014-11-04 18:01:18 UTC
(In reply to Barton Chittenden from comment #8)

> Frédéric: did you test dd/mm/yyyy or mm/dd/yyyy? The latter is the one we're
> having trouble with. Your reply above suggests that you tested the former,
> but I can't tell if you mis-typed your answer or mis-tested.
> 
> No big deal either way, I'm just looking for clarification.

Sorry. I've tried all values for dateformat syspref, and it always works on master. I can't understand how it wouldn't work, indeed. You said: 

  'date_due' is formatted '09/04/2014 23:59'

But this is not what I have. I have yyyy/mm/dd hh:mm, and so the code in master seems valid. There is a mystery...
Comment 10 Katrin Fischer 2014-11-23 07:31:29 UTC
Is this really 3.14, or should it be master?
Comment 11 Katrin Fischer 2015-01-18 17:48:48 UTC
I think the other problem blocking this is that people haven't been able to see the issue, could we get a more detailed test plan?
Maybe with an example date?
Comment 12 Barton Chittenden 2015-02-17 14:30:31 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2015-02-19 11:34:27 UTC
(In reply to Katrin Fischer from comment #11)
> I think the other problem blocking this is that people haven't been able to
> see the issue, could we get a more detailed test plan?
> Maybe with an example date?

Could we get an answer?
Please add the syspref value + the dates to reproduce the issue.
Confirm the issue is still on master.
Back to assigned.
Comment 14 Nick Clemens 2015-02-19 19:08:56 UTC
Taking a guess at problems I had in bug 13679 I was able to recreate the issue today. Materials needed to be issued prior to today, but due today.

I checked out an item as due today (February 19, 2015) , updated the issuedate to set it to 8 days prior (February 11, 2015), then printed a due slip:  item showed as overdue

Applied patch: Item no longer showed as overdue.
Comment 15 Nick Clemens 2015-02-19 19:15:02 UTC Comment hidden (obsolete)
Comment 16 Nick Clemens 2015-02-19 19:15:11 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2015-02-20 09:36:24 UTC
Kyle (or someone else),
Could you add some tests to highlight the problem?
Comment 18 Katrin Fischer 2015-02-21 23:57:11 UTC
*** Bug 8813 has been marked as a duplicate of this bug. ***
Comment 19 Jonathan Druart 2015-02-23 14:35:00 UTC
Failing QA for missing tests.
Comment 20 Kyle M Hall 2015-03-12 13:53:34 UTC
I can no longer reproduce this issue. It appears to be fixed in master.
Comment 21 Katrin Fischer 2015-04-15 14:04:09 UTC
I just got a screenshot proof from a library on 3.18.5.1 - it's still happening.

Item is due today, 23:59
Dateformat is set to metric.

ISSUSLIP

<checkedout>
<p>
<<biblio.title>> <br />
Signatur: <<items.itemcallnumber>><br/>
Barcode: <<items.barcode>><br />
Fällig am: <<issues.date_due>><br />
</p>
</checkedout>
<br />
<h4>Überfällig</h4>
<overdue>
<p>
<<biblio.title>> <br />
Signatur: <<items.itemcallnumber>><br/>
Barcode: <<items.barcode>><br />
Fällig am: <<issues.date_due>><br />
</p>
</overdue>
Comment 22 Katrin Fischer 2015-04-15 14:18:08 UTC
Created attachment 37908 [details]
screenshot of the wrong display

note date on top and overdues (Überfällig) due date
Comment 23 Jonathan Druart 2015-04-15 14:46:11 UTC
Kyle,
I am not able to reproduce the issue, and looking at the cait's data:
 issuedate: '2015-04-15 16:16:14'
 lastreneweddate: NULL

I really don't understand how this code (C4::Members::IssueSlip)

2443         if ((substr $it->{'issuedate'}, 0, 10) eq $now || (substr $it->{'lastreneweddate'}, 0, 10) eq $now) {     
2444             $it->{'now'} = 1;                                                                       
2445         }
2446         elsif ((substr $it->{'date_due'}, 0, 10) le $now) {                                         
2447             $it->{'overdue'} = 1;
2448         }

could set overdue :)

All what I know is that in 3.18.05.1, GetPendingIssues does some stuff with the date_due using DateTime::Format::DateParse, which has been removed from master.

Anyway, the overdue flag is set in GetPendingIssues, why don't you remove all the code around the date manipulations?
Comment 24 Katrin Fischer 2015-04-15 15:01:22 UTC
Even if we weren't able to figure it out yet, this:
2446         elsif ((substr $it->{'date_due'}, 0, 10) le $now) {                
2447             $it->{'overdue'} = 1;

... is wrong, if you think of hourly loans, I think? So we shouldn't keep that alive.

I can see that we need the flag now for the Quick slip, but why do we need to mangle with the overdue status here...?
Comment 25 Katrin Fischer 2015-04-15 15:37:16 UTC
I think I managed to create a test case:

1) Check out an item today, due date is today 23.59
2) Check out an item yesterday (change issue date in db), due date is today 23:59

... and the ISSUESLIP is reliably wrong it appears.
Comment 26 Jonathan Druart 2015-04-15 15:48:02 UTC
So, confirmed on master.

make sure you have:

item1 => date_due = today, issuedate = yesterday
item2 => date_due = today, issuedate = today
Comment 27 Katrin Fischer 2015-04-15 18:19:41 UTC
Kyle, would you be ok squashing the patches?
Comment 28 Jonathan Druart 2015-04-16 07:09:49 UTC
Kyle, some questions have been asked, please have a look.
Also tests are missing.
Comment 29 Jonathan Druart 2015-04-16 08:09:56 UTC
I will try to provide a patch today.
Comment 30 Jonathan Druart 2015-04-16 11:12:03 UTC Comment hidden (obsolete)
Comment 31 Nick Clemens 2015-04-16 21:57:25 UTC Comment hidden (obsolete)
Comment 32 Katrin Fischer 2015-04-17 10:16:36 UTC Comment hidden (obsolete)
Comment 33 Tomás Cohen Arazi 2015-04-20 13:26:54 UTC
Patch pushed to master.

Thanks Jonathan!
Comment 34 Chris Cormack 2015-04-21 23:24:03 UTC
Doesn't apply to 3.18.x please rebase if it is needed
Comment 35 Katrin Fischer 2015-04-22 05:34:59 UTC
It's needed - library found this in 3.18.5.1
Comment 36 Chris Cormack 2015-04-22 05:38:45 UTC
Right, well you've (collective you) have a few hours to get it in a state to apply on 3.18.x (until Bernardo sends the po files and I do the release that is)
Comment 37 Katrin Fischer 2015-04-22 05:39:38 UTC
Trying to see if I can fix it
Comment 38 Katrin Fischer 2015-04-22 05:56:15 UTC
Created attachment 38302 [details] [review]
[3.18.x] Bug 12847: Items issued today is considered as overdue

The date comparisons in C4::Members::IssueSlip does not work as
expected.
Is an item is issue yesterday and due today (23:59), it should not be
considered as an overdue yet.

Test plan:
Define a valid issue slip (code ISSUESLIP)
Check 2 items out and update the issuedate value for one of them as
yesterday (using the mariadb/mysql cli or similar)
Print the slip
Before this patch the item marked as issued yesterday is considered as
overdue.

Special cases:
- hourly loans
- Quick slip is impacted too

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Bug 13601 is not in 3.18.x, so I removed the change connected
to that and it still tested ok.
Comment 39 Chris Cormack 2015-04-22 08:16:28 UTC
Pushed to 3.18.x will be in 3.18.6