Bug 5601 - SIP Due dates miscalculated via DateTime
Summary: SIP Due dates miscalculated via DateTime
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: PATCH-Sent (DO NOT USE) normal (vote)
Assignee: Colin Campbell
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks: 29410
  Show dependency treegraph
 
Reported: 2011-01-09 17:20 UTC by Joe Atzberger
Modified: 2021-11-18 09:20 UTC (History)
2 users (show)

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


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Joe Atzberger 2011-01-09 17:20:45 UTC
SIP checked out items report incorrect due date of 1 day earlier than actual due date.  Koha data internally and staff interface both show correct date, so the error is only in SIP's "view" of the date.

mysql> select itemnumber,barcode,onloan from items where barcode="33518003139989" or barcode="33518003273994";
+------------+----------------+------------+
| itemnumber | barcode        | onloan     |
+------------+----------------+------------+
|     115532 | 33518003139989 | 2011-01-23 | 
|     127096 | 33518003273994 | 2011-01-23 | 
+------------+----------------+------------+

Staff interface shows, for example: "Checked out to 23518000442008 : due 01/23/2011".

But when we pull it using SIP telnet login via an Item Information (17) request:

1720110103    115501AOLGPL|AB33518003139989|
1804020120110103    115559AB33518003139989|AJDon't sleep, there are snakes :|AQLGPL|BGLGPL|AH20110122    160000|

1720110103    115501AOLGPL|AB33518003273994|
1804020120110103    115655AB33518003273994|AJThe elegance of the hedgehog|AQLGPL|BGLGPL|AH20110122    160000|

So that shows the date due stamp (AH) is consistently 8 hours early.

The use of the DateTime module has introduced this logical error.  This is exactly why *not* to use one-off new Date dependencies and string hacking in leaf code, if there is any way to avoid it.

Specifically, the DateTime constructor does NOT default to local timezone, or in fact to ANY timezone, when invoked with the current arguments.  You can demonstrate this w/ the following test:

$ date
Mon Jan  3 14:47:30 PST 2011

$ perl -e 'use DateTime; 
 my $x=DateTime->now(); 
 my $y=DateTime->new(year=>2011,month=>1,day=>3); 
 print $x, " ", $x->time_zone_long_name(), " ", $x->epoch, "\n",
       $y, " ", $y->time_zone_long_name(), " ", $y->epoch, "\n";
'
2011-01-03T22:47:33 UTC 1294094853
2011-01-03T00:00:00 floating 1294012800

So the timezone of the objects that we are getting is "floating".  See:
    http://search.cpan.org/~drolsky/DateTime-0.66/lib/DateTime.pm#Floating_DateTimes

where we are advised:
   "If you are planning to use any objects with a real time zone, it is strongly recommended that you do not mix these with floating datetimes."

All of our objects have real timezones, namely the local $ENV{TZ} one.
Comment 1 Colin Campbell 2011-01-12 01:55:45 UTC
Actually there is no need to process duedate here as a datetime value at all as it is never manipulated as such its just the iso format string from the db. We should pass as is but reformat as required as in checkout. (If we were to need to do so we should provide a DateTime::TimeZone initialized to local in case TZ is not set.)
The date processing was a bit messy all told. ILS/Item.pm did all the manipulation as well then immediately overwrote the result with the string it had just deconstructed. 
Removed ILS/Item.pm and Sip.pm's dependencies on DateTime.
Comment 2 Colin Campbell 2011-01-12 02:22:58 UTC
Second patch sent which adds test for Sip::timestamp's output with iso-strings, time_t secs and no params (i.e. now)
Comment 3 Joe Atzberger 2011-01-14 15:27:16 UTC
(In reply to comment #2)
> Second patch sent which adds test for Sip::timestamp's output with iso-strings,
> time_t secs and no params (i.e. now)

Sorry to say, I already drafted a patch for this that just uses DateTime correctly.  

I support removing it as a dependency, but there is more work to take it off the list of required modules, pull it from INSTALL docs, etc.  

I don't think I agree with artificially seeding the due date time with 23:59.  That's not helpful, imho.
Comment 4 Joe Atzberger 2011-01-27 22:04:54 UTC
Also, we should increment $VERSION for as many touches as are going in.
Comment 6 Colin Campbell 2011-07-07 09:21:30 UTC
The submitted patch was signed off and pushed to master in Feb. Updating status to reflect that
Comment 7 Jared Camins-Esakov 2012-12-31 00:24:56 UTC
There have been no further reports of problems so I am marking this bug resolved.