Description
Nicole C. Engard
2013-11-13 15:24:27 UTC
Created attachment 22912 [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. 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. Created attachment 22963 [details] [review] [SIGNED-OFF] 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: Owen Leonard <oleonard@myacpl.org> This results in correctly-formatted dates, but I think we need to think about adding time to the output since we have hourly checkouts. 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? Created attachment 23213 [details] [review] [ALT] 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. Kyle, what is your opinion on my patch? Maybe it is stupid, what do you think about it? (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! Created attachment 23711 [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. http://bugs.koha-community.org/show_bug.cgi?id=10774 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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. Created attachment 23965 [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. (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. *** Bug 9084 has been marked as a duplicate of this bug. *** Created attachment 24557 [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. Created attachment 24558 [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. 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. 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... (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? 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. (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 This needs to be fixed. Patrons getting notices with dates in wrong format can cause problems, and will cause complaints... (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] (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. Created attachment 29650 [details] [review] [SIGNED-OFF] 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> Created attachment 29651 [details] [review] [SIGNED-OFF] 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> 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. 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> 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> Kyle passed QA on the first patch, I pass QA on the second one. So the bug report is passed QA :) Can anyone write a regression test for this? Also i'd love to see some benchmarks. (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. 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. (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? (In reply to Chris Cormack from comment #30) > Also i'd love to see some benchmarks. see comment 21. Created attachment 31030 [details] [review] Bug 11244: Add unit tests for GetPreparedLetter Patches pushed to master. Thanks Jonathan and David! *** Bug 6029 has been marked as a duplicate of this bug. *** |