Description
Marc Balmer
2012-03-22 15:57:21 UTC
My search of Koha code returns only instances where checks are being done for a '0000-00-00' value but no instances where a '0000-00-00' value is being set, with one exception: sample_quotes.sql How could we remove all the checks of 0000-00-00 in the code? I personally have no good idea. We could remove the not null constraint and update all date fields with NULL. Or keep the constraint and update with 1970-01-01. I like NULL better and it looks less dangerous and cleaner than 1970-01-01 Using a valid date like 1.1.1970 to denote an invalid date is so obviously a bad idea. I suggest to go the NULL route. I really don't know where I have found this stupid idea :) Actually, I find tons of 0000-00-00 now in the codebase... It could be useful to know if production DBs still contain such values (?) I've recently spotted a fair few errors during upgrades where we have these bad dates.. I'll try to draw up a list of where I've seen it.. versions being upgraded and tables affected etc. Created attachment 114528 [details] [review] Bug 7806: Fix remaining occurrences of 0000-00-00 We should remove all SQL queries that contain 0000-00-00 and finally assume we do not longer have such value in our DB (for date type) We already dealt with such values in previous update DB entries. The 2 added by this one haven't been replaced already. The code will now assume that either a valid date exist, or NULL/undef. Test plan: QA review is needed and test of the different places where code is modified. Not sure about the change from reports/issues_avg_stats.pl (In reply to Jonathan Druart from comment #7) > It could be useful to know if production DBs still contain such values (?) I have 175 in items.replacementpricedate, looks like a chunk from the original import to this system, another chunk from importing 7 more libraries, then a chunk starting a couple weeks ago when we updated to 20.05, which apparently inserts 0000-00-00 when you mispaste your replacementprice into replacementpricedate's input. Not sure where else to look for 0000-00-00, though. Created attachment 117266 [details] [review] Bug 7806: Fix remaining occurrences of 0000-00-00 We should remove all SQL queries that contain 0000-00-00 and finally assume we do not longer have such value in our DB (for date type) We already dealt with such values in previous update DB entries. The 2 added by this one haven't been replaced already. The code will now assume that either a valid date exist, or NULL/undef. Test plan: QA review is needed and test of the different places where code is modified. Not sure about the change from reports/issues_avg_stats.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 117267 [details] [review] Bug 7806: (follow-up) Update to use sanitize_zero_date Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> These patches appear to work as expected for me so signing off. However, I do wonder about the tooltips left behind in a number of templates.. <span title="0000-00-00"> Those could be misleading but I'm not entirely sure how they're intended to work.. (In reply to Martin Renvoize from comment #13) > <span title="0000-00-00"> In many places DataTable columns are sorted not by the value displayed but the value in the title attribute as seen above. Instances where "0000-00-00" is hard-coded into the template indicate a place where an empty date might be misinterpreted by the sorting algorithm. (In reply to Owen Leonard from comment #14) > (In reply to Martin Renvoize from comment #13) > > <span title="0000-00-00"> > > In many places DataTable columns are sorted not by the value displayed but > the value in the title attribute as seen above. Instances where "0000-00-00" > is hard-coded into the template indicate a place where an empty date might > be misinterpreted by the sorting algorithm. Thanks Owen, that's really helpful to know :) (In reply to Jonathan Druart from comment #9) > Test plan: > QA review is needed and test of the different places where code is > modified. Hahaha, good plan ;) (In reply to Martin Renvoize from comment #13) > These patches appear to work as expected for me so signing off. Since there is no test plan, it would be beneficial for us to know what you exactly tested and why you conclude that they appear to work? (In reply to Jonathan Druart from comment #9) > Not sure about the change from reports/issues_avg_stats.pl Since returndate comes from oldissues and you sanitize the field in the atomic update, we should be fine imo. Although this looks quite good to me, it is really hard to be convinced that all appearances of 0000-00-00 in the data now are caught correctly by the codebase. I found at least one issue (no blocker): select count(*) from items where dateaccessioned= '0000-00-00'; +----------+ | count(*) | +----------+ | 335 | +----------+ 1 row in set (0.12 sec) This comes from a production database. So I tested with an item. Set dateaccessioned to 0*. Open item editor. You see the 0* on the form. Tab thru the field so that the date gets FOCUS. This will update the 0* in the old situation, but not in the new one (obviously). Note that the CLICK event uses the force parameter and will set the date. Could you add this one to the atomicupdate too? And please provide me some evidence that we found the last case ;) For instance by checking date columns in a larger production database being used for a longer time.. PQA for now. Created attachment 117361 [details] [review] Bug 7806: Fix remaining occurrences of 0000-00-00 We should remove all SQL queries that contain 0000-00-00 and finally assume we do not longer have such value in our DB (for date type) We already dealt with such values in previous update DB entries. The 2 added by this one haven't been replaced already. The code will now assume that either a valid date exist, or NULL/undef. Test plan: QA review is needed and test of the different places where code is modified. Not sure about the change from reports/issues_avg_stats.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 117362 [details] [review] Bug 7806: (follow-up) Update to use sanitize_zero_date Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to Marcel de Rooy from comment #19) > Although this looks quite good to me, it is really hard to be convinced that > all appearances of 0000-00-00 in the data now are caught correctly by the > codebase. > > I found at least one issue (no blocker): > select count(*) from items where dateaccessioned= '0000-00-00'; > +----------+ > | count(*) | > +----------+ > | 335 | > +----------+ > 1 row in set (0.12 sec) > This comes from a production database. > So I tested with an item. Set dateaccessioned to 0*. Open item editor. You > see the 0* on the form. Tab thru the field so that the date gets FOCUS. This > will update the 0* in the old situation, but not in the new one (obviously). > Note that the CLICK event uses the force parameter and will set the date. > > Could you add this one to the atomicupdate too? > And please provide me some evidence that we found the last case ;) For > instance by checking date columns in a larger production database being used > for a longer time.. > > PQA for now. Thanks for QAing this Marcel, your right, I should have detailed my testing :(.. I basically looked at where the code changed and tried to trigger those actions via the UI to check things weren't broken. As for this comment, I'm pretty sure we'll have left behind some bad data, but it should get picked up by upgrades now that we throw errors in the updatedatabase script. We could I suppose write a one off atomicupdate to check and fix all possible datefields at this point in time.. I used the following to get a list of such fields.... we have 210! ''' SELECT col.table_schema AS database_name, col.table_name, col.ordinal_position AS column_id, col.column_name, col.data_type, col.datetime_precision FROM information_schema.columns col JOIN information_schema.tables tab ON tab.table_schema = col.table_schema AND tab.table_name = col.table_name AND tab.table_type = 'BASE TABLE' WHERE col.data_type IN ( 'date', 'time', 'datetime', 'year', 'timestamp' ) AND col.table_schema NOT IN ( 'information_schema', 'sys', 'performance_schema', 'mysql' ) AND col.table_schema = 'database_name' ORDER BY col.table_schema, col.table_name, col.ordinal_position; ''' 142 when I reduce the query to only those fields that are nullable: SELECT Concat(col.table_name, '.', col.column_name), col.data_type FROM information_schema.columns col JOIN information_schema.tables tab ON tab.table_schema = col.table_schema AND tab.table_name = col.table_name AND tab.table_type = 'BASE TABLE' WHERE col.data_type IN ( 'date', 'time', 'datetime', 'year', 'timestamp' ) AND col.is_nullable = 'YES' AND col.table_schema NOT IN ( 'information_schema', 'sys', 'performance_schema', 'mysql' ) AND col.table_schema = 'koha_demo1' ORDER BY col.table_schema, col.table_name, col.ordinal_position; Can't call method "subtract" on an undefined value at /usr/share/koha/Koha/Patron.pm line 770 When dateexpiry == 0000-00-00 But I did put it in explicitly ;) (In reply to Martin Renvoize from comment #22) > Thanks for QAing this Marcel, your right, I should have detailed my testing > :(.. I basically looked at where the code changed and tried to trigger those > actions via the UI to check things weren't broken. Great. This will build up more confidence for pushing it. I wrote bug 27821, otherwise the cast on old_issues.returndate didn't work. Pushed to master for 21.05, thanks to everybody involved! Pushed to 20.11.x for 20.11.04 This doesn't apply cleanly to 20.05, please rebase. I got the following test failure after clearing merge errors myself: root@kohadevbox:koha(rmain2005)$ prove t/db_dependent/Circulation/dateexpiry.t t/db_dependent/Circulation/dateexpiry.t .. 1/2 # No tests run! t/db_dependent/Circulation/dateexpiry.t .. 2/2 # Failed test 'No tests run for subtest "Tests for CalcDateDue related to dateexpiry"' # at t/db_dependent/Circulation/dateexpiry.t line 46. Can't call method "effective_itemtype" on unblessed reference at t/db_dependent/Circulation/dateexpiry.t line 109. # Looks like your test exited with 255 just after 2. t/db_dependent/Circulation/dateexpiry.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 1/2 subtests Test Summary Report ------------------- t/db_dependent/Circulation/dateexpiry.t (Wstat: 65280 Tests: 2 Failed: 1) Failed test: 2 Non-zero exit status: 255 Files=1, Tests=2, 4 wallclock secs ( 0.01 usr 0.01 sys + 2.24 cusr 0.42 csys = 2.68 CPU) Result: FAIL Andrew, resolve the conflict like that: 108 $patron->{dateexpiry} = undef; 109 my $d = C4::Circulation::CalcDateDue( $today, $item->{itype}, $branch->{branchcode}, $patron ); 110 is( ref $d eq "DateTime" && $d->mdy() =~ /^\d+/, 1, "CalcDateDue with expiry undef" ); Created attachment 117915 [details] [review] Bug 7806: [20.05] Fix remaining occurrences of 0000-00-00 We should remove all SQL queries that contain 0000-00-00 and finally assume we do not longer have such value in our DB (for date type) We already dealt with such values in previous update DB entries. The 2 added by this one haven't been replaced already. The code will now assume that either a valid date exist, or NULL/undef. Test plan: QA review is needed and test of the different places where code is modified. Not sure about the change from reports/issues_avg_stats.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 117916 [details] [review] Bug 7806: [20.05] (follow-up) Update to use sanitize_zero_date Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Thanks, Jonathan! Pushed to 20.05.x for 20.05.10 Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed. |