[Bug 14494] New: Terribly slow checkout caused by DateTime->new in far future
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Bug ID: 14494 Summary: Terribly slow checkout caused by DateTime->new in far future Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: gmcharlt@gmail.com Reporter: m.de.rooy@rijksmuseum.nl QA Contact: testopia@bugs.koha-community.org An expiry date of 9999-12-31 for organizations was a nice idea :) But checking out an item to that patron takes 1 minute or longer ! The cause? The following lines in CanBookBeIssued seem to be the main cause: my $expiry_dt = DateTime->new( year => $y, month => $m, day => $d, time_zone => C4::Context->tz, ); This call takes forever with 9999-12-31 when the timezone is not 'floating' or 'UTC', but e.g. Europe/Amsterdam. If the timezone is UTC, it takes 0.0002 seconds. DateTime gives the following advices: === Do not try to use named time zones (like "America/Chicago") with dates very far in the future (thousands of years). The current implementation of DateTime::TimeZone will use a huge amount of memory calculating all the DST changes from now until the future date. Use UTC or the floating time zone and you will be safe. •use UTC for all calculations If you do care about time zones (particularly DST) or leap seconds, try to use non-UTC time zones for presentation and user input only. Convert to UTC immediately and convert back to the local time zone for presentation The results DateTime.pm produces are predictable and correct, and mostly intuitive, but datetime math gets very ugly when time zones are involved ... === We are not doing that currently. We use in C4::Context: DateTime::TimeZone->new(name => 'local') Could we implement the calculation/presentation advice of DateTime in Koha somehow? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=13242 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@biblibre.co | |m Assignee|gmcharlt@gmail.com |jonathan.druart@biblibre.co | |m Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 --- Comment #1 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 40860 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40860&action=edit Bug 14494: Prevent slow checkout if the patron does not have an expiry date If a patron has a expiry date set to 9999-12-31 (for organizations for instance), the checkouts are very slow. It's caused by 2 different calls to DateTime in CanBookBeIssued: 1/ DateTime->new( year => 9999, month => 12, day => 31, time_zone => C4::Context->tz ); The time_zone should not be set (as it's done in Koha::DateUtils), set to UTC or floating tz. 2/ DateTime->compare($today, $expiry_dt) The comparaison of 2 DT with 1 related to 9999 is very slow, as you can imagine. For 1/ we need to call Koha::DateUtils::dt_from_string (actually, we should never call DateTime directly). For 2/ we just need to test if the date is != 9999, no need to compare it in this case. Test plan: Before this patch, confirm that the checkouts are slow if the patron has a dateexpiry set to 9999-12-31. update borrowers set dateexpiry="9999-12-31" where borrowernumber=42; After this patch, you should not see any regression when checking out items to an expired patron and to a valid patron. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |14197 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|14197 | Depends on| |14197 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 --- Comment #2 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hm, I feel a bit like this builds in a secret feature - if I use 9998 it will be slow, but if I use 9999 it will be faster? When we fixed the same thing for debarments it made more sense, as the 'ulimited' debarment uses 9999 automatically, without the user entering a date. For the patron expiry the date is defined by the library and could be anything. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 --- Comment #3 from Jonathan Druart <jonathan.druart@biblibre.com> --- (In reply to Katrin Fischer from comment #2)
Hm, I feel a bit like this builds in a secret feature - if I use 9998 it will be slow, but if I use 9999 it will be faster?
When we fixed the same thing for debarments it made more sense, as the 'ulimited' debarment uses 9999 automatically, without the user entering a date. For the patron expiry the date is defined by the library and could be anything.
As I understood the description, the feature has existed. Anyway we should call Koha::DateUtils here and not directly DateTime. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #4 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- I don't think it's a feature - it has to be typed in manually in the configuration. I think it would make sense to me if we introduced an option to have an 'unlimited' expiry date in the GUI - then the check on 9999 would make more sense to me. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 --- Comment #5 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 40873 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40873&action=edit Bug 14494: Terribly slow checkout caused by DateTime->new in far future An expiry date like 9999-12-31 in the local timezone will make DateTime spend a lot of time (maybe 60 seconds) on date calculation. See the DateTime documention on CPAN. A calculation in floating (or alternatively in UTC) would only take a few milliseconds. This patch makes two changes in this regard: [1] The compare between expiry date and today in CanBookBeIssued is now done in the floating timezone. [2] If ReturnBeforeExpiry is enabled, CalcDateDue compares the normal due date with the expiry date. The comparison is now done in the floating timezone. If the expiry date is before the due date, it is returned in the user context's timezone. NOTE: The calls to set_time_zone moving to or from floating do not adjust the local time. TEST PLAN: First without this patch: [1] Set expiry date to 9999-12-31 for a patron. [2] Enable ReturnBeforeExpiry. [3] Checkout a book to this patron. This will be (very) slow. Continue now with this patch applied: [4] Check in the same book. [5] Check it out again. Should be much faster. Bonus test: [6] Set borrower expiry date to today. Change relevant circulation rule to loan period of 21 hours. Test checking out with a manual due date /time just before today 23:59 and after that. In the second case the due date/time should become today 23:59 (note that 23:59 is not shown on the checkout form). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 --- Comment #6 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Typical example of patch collision. Can we get the best of both? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 --- Comment #7 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Katrin Fischer from comment #2)
Hm, I feel a bit like this builds in a secret feature - if I use 9998 it will be slow, but if I use 9999 it will be faster?
When we fixed the same thing for debarments it made more sense, as the 'ulimited' debarment uses 9999 automatically, without the user entering a date. For the patron expiry the date is defined by the library and could be anything.
We should not do hardcoded tests on 9999. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 --- Comment #8 from Jonathan Druart <jonathan.druart@biblibre.com> --- (In reply to Marcel de Rooy from comment #6)
Typical example of patch collision. Can we get the best of both?
As I already said, IMO we should not use DateTime outside of Koha::DateUtils. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #8)
(In reply to Marcel de Rooy from comment #6)
Typical example of patch collision. Can we get the best of both?
As I already said, IMO we should not use DateTime outside of Koha::DateUtils.
That is fine. My patch also touches CalcDateDue. I will rework it now. Can we prevent another duplication of effort? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 --- Comment #10 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- btw I will also remove the 9999 Note that I am having trouble now with the Circulation test. It fails with me on test 49 already. not ok 49 - No items due in less than one day (0 days in advance) # Failed test 'No items due in less than one day (0 days in advance)' # at t/db_dependent/Circulation.t line 527. # got: '2' # expected: '0' -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|jonathan.druart@biblibre.co |m.de.rooy@rijksmuseum.nl |m | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #40873|0 |1 is obsolete| | --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 40874 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40874&action=edit Bug 14494: Terribly slow checkout caused by DateTime->new in far future An expiry date like 9999-12-31 in the local timezone will make DateTime spend a lot of time (maybe 60 seconds) on date calculation. See the DateTime documention on CPAN. A calculation in floating (or alternatively in UTC) would only take a few milliseconds. This patch makes two changes in this regard: [1] The compare between expiry date and today in CanBookBeIssued has been adjusted in Jonathan's patch. I am moving the compare to the floating timezone (as was done in my original patch). This removes a hardcoded 9999. [2] If ReturnBeforeExpiry is enabled, CalcDateDue compares the normal due date with the expiry date. The comparison is now done in the floating timezone. If the expiry date is before the due date, it is returned in the user context's timezone. NOTE: The calls to set_time_zone moving to or from floating do not adjust the local time. TEST PLAN: First without this patch (and the one from Jonathan): [1] Set expiry date to 9999-12-31 for a patron. [2] Enable ReturnBeforeExpiry. [3] Checkout a book to this patron. This will be (very) slow. Continue now with this patch applied: [4] Check in the same book. [5] Check it out again. Should be much faster. Bonus test: [6] Set borrower expiry date to today. Change relevant circulation rule to loan period of 21 hours. Test checking out with a manual due date /time just before today 23:59 and after that. In the second case the due date/time should become today 23:59 (note that 23:59 is not shown on the checkout form). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- not ok 49 - No items due in less than one day (0 days in advance) # Failed test 'No items due in less than one day (0 days in advance)' # at t/db_dependent/Circulation.t line 527. # got: '2' # expected: '0' Resolved this one now. The staff expiry was too quick :) Still looking now at: ok 61 - Calling UpdateFine on non-existant fine with an amount of 0 does not result in an empty fine DBD::mysql::db begin_work failed: Already in a transaction at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1339. DBIx::Class::Schema::txn_begin(): DBD::mysql::db begin_work failed: Already in a transaction at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1339. at /home/koha/testclone/t/lib/TestBuilder.pm line 75 DBIx::Class::Schema::txn_rollback(): Storage transaction_depth 0 does not match false AutoCommit of DBI::db=HASH(0xb24823c), attempting ROLLBACK anyway at /home/koha/testclone/t/lib/TestBuilder.pm line 360 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #40860|0 |1 is obsolete| | --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 40879 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40879&action=edit Bug 14494: Prevent slow checkout if the patron does not have an expiry date If a patron has a expiry date set to 9999-12-31 (for organizations for instance), the checkouts are very slow. It's caused by 2 different calls to DateTime in CanBookBeIssued: 1/ DateTime->new( year => 9999, month => 12, day => 31, time_zone => C4::Context->tz ); The time_zone should not be set (as it's done in Koha::DateUtils), set to UTC or floating tz. 2/ DateTime->compare($today, $expiry_dt) The comparaison of 2 DT with 1 related to 9999 is very slow, as you can imagine. For 1/ we need to call Koha::DateUtils::dt_from_string (actually, we should never call DateTime directly). For 2/ we just need to test if the date is != 9999, no need to compare it in this case. Test plan: Before this patch, confirm that the checkouts are slow if the patron has a dateexpiry set to 9999-12-31. update borrowers set dateexpiry="9999-12-31" where borrowernumber=42; After this patch, you should not see any regression when checking out items to an expired patron and to a valid patron. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #40874|0 |1 is obsolete| | --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 40880 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40880&action=edit Bug 14494: Terribly slow checkout caused by DateTime->new in far future An expiry date like 9999-12-31 in the local timezone will make DateTime spend a lot of time (maybe 60 seconds) on date calculation. See the DateTime documention on CPAN. A calculation in floating (or alternatively in UTC) would only take a few milliseconds. This patch makes two changes in this regard: [1] The compare between expiry date and today in CanBookBeIssued has been adjusted in Jonathan's patch. I am moving the compare to the floating timezone (as was done in my original patch). This removes a hardcoded 9999. [2] If ReturnBeforeExpiry is enabled, CalcDateDue compares the normal due date with the expiry date. The comparison is now done in the floating timezone. If the expiry date is before the due date, it is returned in the user context's timezone. NOTE: The calls to set_time_zone moving to or from floating do not adjust the local time. TEST PLAN: First without this patch (and the one from Jonathan): [1] Set expiry date to 9999-12-31 for a patron. [2] Enable ReturnBeforeExpiry. [3] Checkout a book to this patron. This will be (very) slow. Continue now with this patch applied: [4] Check in the same book. [5] Check it out again. Should be much faster. Bonus test: [6] Set borrower expiry date to today. Change relevant circulation rule to loan period of 21 hours. Test checking out with a manual due date /time just before today 23:59 and after that. In the second case the due date/time should become today 23:59 (note that 23:59 is not shown on the checkout form). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 40881 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40881&action=edit Bug 14494: Unit test Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Moved the test on its own. Nicer than adding TestBuilder in the final part of the test. No warnings/errors from TestBuilder, no dependency.. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|14197 | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=14197 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 --- Comment #16 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Needs a signoff on the second path to get this into SO queue. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #40881|0 |1 is obsolete| | --- Comment #17 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 40882 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40882&action=edit Bug 14494: Unit test Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Moved the test on its own. Nicer than adding TestBuilder in the final part of the test. No warnings/errors from TestBuilder, no dependency.. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #40882|0 |1 is obsolete| | --- Comment #18 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 40883 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40883&action=edit Bug 14494: Unit tests for CanBookBeIssued related to dateexpiry Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Moved the test on its own. Nicer than adding TestBuilder in the final part of the test. No warnings/errors from TestBuilder, no dependency.. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 --- Comment #19 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 40908 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40908&action=edit Bug 14494: Add some unit tests too for CalcDateDue The second patch of this report made some changes to CalcDateDue. We are adding some unit tests here. See the commments on the third patch too. Test plan: Run t/db_dependent/Circulation_dateexpiry.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #40879|0 |1 is obsolete| | --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 40909 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40909&action=edit Bug 14494: Prevent slow checkout if the patron does not have an expiry date If a patron has a expiry date set to 9999-12-31 (for organizations for instance), the checkouts are very slow. It's caused by 2 different calls to DateTime in CanBookBeIssued: 1/ DateTime->new( year => 9999, month => 12, day => 31, time_zone => C4::Context->tz ); The time_zone should not be set (as it's done in Koha::DateUtils), set to UTC or floating tz. 2/ DateTime->compare($today, $expiry_dt) The comparaison of 2 DT with 1 related to 9999 is very slow, as you can imagine. For 1/ we need to call Koha::DateUtils::dt_from_string (actually, we should never call DateTime directly). For 2/ we just need to test if the date is != 9999, no need to compare it in this case. Test plan: Before this patch, confirm that the checkouts are slow if the patron has a dateexpiry set to 9999-12-31. update borrowers set dateexpiry="9999-12-31" where borrowernumber=42; After this patch, you should not see any regression when checking out items to an expired patron and to a valid patron. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #40880|0 |1 is obsolete| | --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 40910 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40910&action=edit Bug 14494: Terribly slow checkout caused by DateTime->new in far future An expiry date like 9999-12-31 in the local timezone will make DateTime spend a lot of time (maybe 60 seconds) on date calculation. See the DateTime documention on CPAN. A calculation in floating (or alternatively in UTC) would only take a few milliseconds. This patch makes two changes in this regard: [1] The compare between expiry date and today in CanBookBeIssued has been adjusted in Jonathan's patch. I am moving the compare to the floating timezone (as was done in my original patch). This removes a hardcoded 9999. [2] If ReturnBeforeExpiry is enabled, CalcDateDue compares the normal due date with the expiry date. The comparison is now done in the floating timezone. If the expiry date is before the due date, it is returned in the user context's timezone. NOTE: The calls to set_time_zone moving to or from floating do not adjust the local time. TEST PLAN: First without this patch (and the one from Jonathan): [1] Set expiry date to 9999-12-31 for a patron. [2] Enable ReturnBeforeExpiry. [3] Checkout a book to this patron. This will be (very) slow. Continue now with this patch applied: [4] Check in the same book. [5] Check it out again. Should be much faster. Bonus test: [6] Set borrower expiry date to today. Change relevant circulation rule to loan period of 21 hours. Test checking out with a manual due date /time just before today 23:59 and after that. In the second case the due date/time should become today 23:59 (note that 23:59 is not shown on the checkout form). Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #40883|0 |1 is obsolete| | --- Comment #22 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 40911 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40911&action=edit Bug 14494: Unit tests for CanBookBeIssued related to dateexpiry Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Moved the test on its own. Nicer than adding TestBuilder in the final part of the test. No warnings/errors from TestBuilder, no dependency.. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #40908|0 |1 is obsolete| | --- Comment #23 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 40912 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40912&action=edit Bug 14494: Add some unit tests too for CalcDateDue The second patch of this report made some changes to CalcDateDue. We are adding some unit tests here. See the commments on the third patch too. Test plan: Run t/db_dependent/Circulation_dateexpiry.t Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Amended patch: Set the number of tests for the second subtest (was commented) and perltidy the second block. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 --- Comment #24 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 40932 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40932&action=edit [PASSED QA] Bug 14494: Prevent slow checkout if the patron does not have an expiry date If a patron has a expiry date set to 9999-12-31 (for organizations for instance), the checkouts are very slow. It's caused by 2 different calls to DateTime in CanBookBeIssued: 1/ DateTime->new( year => 9999, month => 12, day => 31, time_zone => C4::Context->tz ); The time_zone should not be set (as it's done in Koha::DateUtils), set to UTC or floating tz. 2/ DateTime->compare($today, $expiry_dt) The comparaison of 2 DT with 1 related to 9999 is very slow, as you can imagine. For 1/ we need to call Koha::DateUtils::dt_from_string (actually, we should never call DateTime directly). For 2/ we just need to test if the date is != 9999, no need to compare it in this case. Test plan: Before this patch, confirm that the checkouts are slow if the patron has a dateexpiry set to 9999-12-31. update borrowers set dateexpiry="9999-12-31" where borrowernumber=42; After this patch, you should not see any regression when checking out items to an expired patron and to a valid patron. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 --- Comment #25 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 40933 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40933&action=edit [PASSED QA] Bug 14494: Terribly slow checkout caused by DateTime->new in far future An expiry date like 9999-12-31 in the local timezone will make DateTime spend a lot of time (maybe 60 seconds) on date calculation. See the DateTime documention on CPAN. A calculation in floating (or alternatively in UTC) would only take a few milliseconds. This patch makes two changes in this regard: [1] The compare between expiry date and today in CanBookBeIssued has been adjusted in Jonathan's patch. I am moving the compare to the floating timezone (as was done in my original patch). This removes a hardcoded 9999. [2] If ReturnBeforeExpiry is enabled, CalcDateDue compares the normal due date with the expiry date. The comparison is now done in the floating timezone. If the expiry date is before the due date, it is returned in the user context's timezone. NOTE: The calls to set_time_zone moving to or from floating do not adjust the local time. TEST PLAN: First without this patch (and the one from Jonathan): [1] Set expiry date to 9999-12-31 for a patron. [2] Enable ReturnBeforeExpiry. [3] Checkout a book to this patron. This will be (very) slow. Continue now with this patch applied: [4] Check in the same book. [5] Check it out again. Should be much faster. Bonus test: [6] Set borrower expiry date to today. Change relevant circulation rule to loan period of 21 hours. Test checking out with a manual due date /time just before today 23:59 and after that. In the second case the due date/time should become today 23:59 (note that 23:59 is not shown on the checkout form). Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 --- Comment #26 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 40934 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40934&action=edit [PASSED QA] Bug 14494: Unit tests for CanBookBeIssued related to dateexpiry Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Moved the test on its own. Nicer than adding TestBuilder in the final part of the test. No warnings/errors from TestBuilder, no dependency.. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com Attachment #40909|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 --- Comment #27 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 40935 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40935&action=edit [PASSED QA] Bug 14494: Add some unit tests too for CalcDateDue The second patch of this report made some changes to CalcDateDue. We are adding some unit tests here. See the commments on the third patch too. Test plan: Run t/db_dependent/Circulation_dateexpiry.t Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Amended patch: Set the number of tests for the second subtest (was commented) and perltidy the second block. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #40910|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #40911|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #40912|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 --- Comment #28 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- We should have a look later at DateTime::Infinite http://search.cpan.org/~drolsky/DateTime-1.20/lib/DateTime/Infinite.pm -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com Status|Passed QA |Pushed to Master --- Comment #29 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Patches pushed to master. Thanks Marcel and Jonathan! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=14522 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |chris@bigballofwax.co.nz --- Comment #30 from Chris Cormack <chris@bigballofwax.co.nz> --- Pushed to 3.20.x will be in 3.20.3 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Liz Rea <liz@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |liz@catalyst.net.nz --- Comment #31 from Liz Rea <liz@catalyst.net.nz> --- Pushed to 3.18.x, will be in 3.18.10. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtj@kohaaloha.com --- Comment #32 from Mason James <mtj@kohaaloha.com> --- (In reply to Liz Rea from comment #31)
Pushed to 3.18.x, will be in 3.18.10.
Pushed to 3.16.x, will be in 3.16.14 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |12669 Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12669 [Bug 12669] "Template process failed: undef error - Invalid local time for date in time zone" -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14494 Marjorie Barry-Vila <marjorie.barry-vila@ccsr.qc.ca> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |marjorie.barry-vila@ccsr.qc | |.ca -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org