Bug 11244

Summary: notices ignoring the dateformat preference
Product: Koha Reporter: Nicole C. Engard <nengard>
Component: NoticesAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: chris, crohde, dcook, jdemuth, jonathan.druart, katrin.fischer, kyle, olli-antti.kivilahti, pasi.kallinen, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13226
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 13011    
Attachments: Bug 11244 - notices ignoring the dateformat preference
[SIGNED-OFF] Bug 11244 - notices ignoring the dateformat preference
[ALT] Bug 11244: notices ignoring the dateformat preference
Bug 11244: notices ignoring the dateformat preference
Bug 11244: notices ignoring the dateformat preference
Bug 11244: notices ignoring the dateformat preference
Bug 11244: (follow-up) Fix $dateonly flag
[SIGNED-OFF] Bug 11244: notices ignoring the dateformat preference
[SIGNED-OFF] Bug 11244: (follow-up) Fix $dateonly flag
Bug 11244: notices ignoring the dateformat preference
Bug 11244: (follow-up) Fix $dateonly flag
Bug 11244: Add unit tests for GetPreparedLetter

Description Nicole C. Engard 2013-11-13 15:24:27 UTC
Overdue notices are using the MySQL date format and not the dateformat in the system preferences.
Comment 1 Kyle M Hall 2013-11-13 16:09:00 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-11-13 16:11:57 UTC
This patch looks up each field in the information schema table, and
if it is a date, datetime, or timestamp, it converts is to the format
specified in the preference dateformat.

Though the query in this patch looks like a mysql'ism, I believe it should also work in postgres at the very least. I believe we will in the near future move to Template Toolkit based notices which will render this issue moot. With TT notices, we will be able to use the KohaDates plugin to format dates correctly at that point.
Comment 3 Owen Leonard 2013-11-15 15:54:49 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2013-11-28 14:14:00 UTC
Kyle,
The patch works but don't you think it is easier to parse the variable value and assume it is a date if it matches YYYY-MM-DD or YYYY-MM-DD HH:MM:SS?
Comment 5 Jonathan Druart 2013-11-28 14:16:30 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2013-12-13 12:22:35 UTC
Kyle, what is your opinion on my patch? Maybe it is stupid, what do you think about it?
Comment 7 Kyle M Hall 2013-12-20 13:23:35 UTC
(In reply to Jonathan Druart from comment #6)
> Kyle, what is your opinion on my patch? Maybe it is stupid, what do you
> think about it?

I think it's a far better solution than mine!
Comment 8 Kyle M Hall 2013-12-20 13:28:05 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2013-12-27 22:10:39 UTC
I like this idea, but I think we want to display the time in some cases - like for the due date in case of a hourly loan.
Comment 10 Jonathan Druart 2014-01-06 16:34:02 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2014-01-06 16:34:36 UTC
(In reply to Katrin Fischer from comment #9)
> I like this idea, but I think we want to display the time in some cases -
> like for the due date in case of a hourly loan.

Yes, you are right. The last patch takes the 2 formats into account.
Comment 12 David Cook 2014-01-20 02:03:57 UTC
*** Bug 9084 has been marked as a duplicate of this bug. ***
Comment 13 David Cook 2014-01-20 03:00:14 UTC Comment hidden (obsolete)
Comment 14 David Cook 2014-01-20 03:00:39 UTC Comment hidden (obsolete)
Comment 15 David Cook 2014-02-06 00:04:16 UTC
Jonathan, I think if you sign off on my follow-up, we can get this over to the QA people.

I would love to see this one get through soon. It's been a long-standing bug for us.

Happy to test someone else's patch if it means this one gets through as well.
Comment 16 Olli-Antti Kivilahti 2014-02-06 16:10:10 UTC
Hi there!

Applied the bug.
I am noticing a huge performance drop with cronjobs/overdue_notices.pl

getting 12 items to the message_queue takes 31s with this feature applied.
getting 12 items to the message_queue takes 1s without this feature applied.

oops...
Comment 17 David Cook 2014-02-07 00:59:05 UTC
(In reply to Olli-Antti Kivilahti from comment #16)
> Hi there!
> 
> Applied the bug.
> I am noticing a huge performance drop with cronjobs/overdue_notices.pl
> 
> getting 12 items to the message_queue takes 31s with this feature applied.
> getting 12 items to the message_queue takes 1s without this feature applied.
> 
> oops...

I've reproduced this problem as well, although I'm getting 35 items at around 31s.

When I use strace, I'm noticing that 99% of the time is being spent on "brk" syscalls.

I'm guessing the regex or the DateTime conversions are slowing things?
Comment 18 David Cook 2014-02-07 01:58:16 UTC
I thought about using DBIX::Class via Koha::Database to figure out the data type for the column, but I don't think that's going to be practical in this case, since the sourcename for the Koha::Schema::Result classes are sometimes quite different from the actual table name (e.g. borrowers vs Borrower, or MatcherMatchpoint vs matcher_matchpoints).

I suppose we also have to consider...is decreased performance worth having correct timestamps?

It might be worth testing this with a much larger load than 10-30 notices.
Comment 19 David Cook 2014-02-07 02:07:02 UTC
(In reply to David Cook from comment #18)
> I thought about using DBIx::Class via Koha::Database to figure out the data
> type for the column, but I don't think that's going to be practical in this
> case, since the sourcename for the Koha::Schema::Result classes are
> sometimes quite different from the actual table name (e.g. borrowers vs
> Borrower, or MatcherMatchpoint vs matcher_matchpoints).
> 
> I suppose we also have to consider...is decreased performance worth having
> correct timestamps?
> 
> It might be worth testing this with a much larger load than 10-30 notices.

Sorry to keep spamming everyone, but perhaps it would be a good idea to add a "source_name" alias to each Result class so that we can use the actual table name as a source name when trying to use DBIx::Class.

http://search.cpan.org/~ribasushi/DBIx-Class-0.08270/lib/DBIx/Class/ResultSource.pm#source_name
Comment 20 paxed 2014-06-09 08:35:05 UTC
This needs to be fixed. Patrons getting notices with dates in wrong format can cause problems, and will cause complaints...
Comment 21 Jonathan Druart 2014-06-09 14:32:49 UTC
(In reply to Olli-Antti Kivilahti from comment #16)
> Hi there!
> 
> Applied the bug.
> I am noticing a huge performance drop with cronjobs/overdue_notices.pl
> 
> getting 12 items to the message_queue takes 31s with this feature applied.
> getting 12 items to the message_queue takes 1s without this feature applied.
> 
> oops...

I don't reproduce. I generated 10 overdues, and I got:

koha@koha-qa:~/src (BZ11244↑⚡)% time perl misc/cronjobs/overdue_notices.pl
perl misc/cronjobs/overdue_notices.pl  1.46s user 0.05s system 76% cpu 1.976 total

koha@koha-qa:~/src (BZ11244↑⚡)% git checkout master
Switched to branch 'master'

koha@koha-qa:~/src (master⚡)% time perl misc/cronjobs/overdue_notices.pl
perl misc/cronjobs/overdue_notices.pl  1.32s user 0.05s system 73% cpu 1.853 total

I have 5 dates in the template:
[skip]
<<items.datelastborrowed>><<items.datelastseen>><<items.timestamp>>
[skip]
<<borrowers.dateofbirth>><<borrowers.dateexpiry>>
[skip]
Comment 22 David Cook 2014-06-16 04:28:12 UTC
(In reply to paxed from comment #20)
> This needs to be fixed. Patrons getting notices with dates in wrong format
> can cause problems, and will cause complaints...

I agree. This is the oldest bug in my local bug tracker at the moment, and I would love to see this get through. 

Honestly, even if it is slower than the past version, I rather have correct output and slower performance on a job which is run once a day anyway. 

Changing this back to "Signed Off" to see what QA team has to say about this one.
Comment 23 Kyle M Hall 2014-07-11 15:52:25 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2014-07-11 15:52:39 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2014-07-11 15:53:45 UTC
I would consider this such a major bug that we must fix it despite any performance decrease associated with it.

(In reply to David Cook from comment #22)
> (In reply to paxed from comment #20)
> > This needs to be fixed. Patrons getting notices with dates in wrong format
> > can cause problems, and will cause complaints...
> 
> I agree. This is the oldest bug in my local bug tracker at the moment, and I
> would love to see this get through. 
> 
> Honestly, even if it is slower than the past version, I rather have correct
> output and slower performance on a job which is run once a day anyway. 
> 
> Changing this back to "Signed Off" to see what QA team has to say about this
> one.
Comment 26 Jonathan Druart 2014-07-25 15:01:07 UTC
Created attachment 30126 [details] [review]
Bug 11244: notices ignoring the dateformat preference

Overdue notices are using the MySQL date format and not the dateformat
in the system preferences.

Test Plan:
1) Enable checkout notices for a patron, make sure the date due is in
   the notice.
2) Check out an item to that patron, note the date is in the mysql
   datetime format
3) Apply this patch
4) Check out another item to the patron, not the date is now in the
   preferred date format.

Signed-off-by: David Cook <dcook@prosentient.com.au>

I love this patch! It is the best solution to this problem that I've
seen. I think it is set up to perfectly handle dates in the notices.

Unfortunately, the $dateonly flag is backwards, so the time is stripped
from timestamps and 00:00:00 is added to dates without times.

I'm adding a follow-up to reverse the setting of this flag.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 27 Jonathan Druart 2014-07-25 15:01:12 UTC
Created attachment 30127 [details] [review]
Bug 11244: (follow-up) Fix $dateonly flag

At the moment, $dateonly is set to true when $1 is defined. However,
since the regex capture group only includes the time, this flag will
only be set when there is a value that includes a time.

In effect, this means that timestamps are reduced to dates only,
while dates have 00-00-0000 added to them.

This patch keeps the logic but reverses the values, so that $dateonly
will default to true unless $1 is defined.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 28 Jonathan Druart 2014-07-25 15:02:36 UTC
Kyle passed QA on the first patch, I pass QA on the second one. So the bug report is passed QA :)
Comment 29 Tomás Cohen Arazi 2014-07-30 12:46:43 UTC
Can anyone write a regression test for this?
Comment 30 Chris Cormack 2014-08-05 00:19:11 UTC
Also i'd love to see some benchmarks.
Comment 31 David Cook 2014-08-05 03:33:26 UTC
(In reply to Chris Cormack from comment #30)
> Also i'd love to see some benchmarks.

That would be interesting. So far, three different people got fairly different results. I wonder a bit about the differences in hardware, number of notices, number of substitutions per notice, etc.
Comment 32 Katrin Fischer 2014-08-05 05:17:38 UTC
I am not sure about the implementation or its effects on performance of the job, but I'd really like to see this fixed. Notices are sent out to the patrons and nicely formatted dates would be great.
Comment 33 David Cook 2014-08-06 00:16:44 UTC
(In reply to Katrin Fischer from comment #32)
> I am not sure about the implementation or its effects on performance of the
> job, but I'd really like to see this fixed. Notices are sent out to the
> patrons and nicely formatted dates would be great.

Yeah, it's the oldest issue in my local tracker, so I would love to see this fixed.

The effect on performance probably wouldn't matter much with a small load, but I wonder how it would perform under a higher load which might be more common in larger libraries?
Comment 34 Jonathan Druart 2014-08-18 09:37:25 UTC
(In reply to Chris Cormack from comment #30)
> Also i'd love to see some benchmarks.

see comment 21.
Comment 35 Jonathan Druart 2014-08-20 15:59:18 UTC
Created attachment 31030 [details] [review]
Bug 11244: Add unit tests for GetPreparedLetter
Comment 36 Tomás Cohen Arazi 2014-08-20 16:45:23 UTC
Patches pushed to master.

Thanks Jonathan and David!
Comment 37 Katrin Fischer 2014-11-21 06:54:35 UTC
*** Bug 6029 has been marked as a duplicate of this bug. ***