Description
Martin Renvoize (ashimema)
2022-07-12 16:05:40 UTC
Created attachment 154299 [details] [review] Bug 31143: Update patrons with dates of 0000-00-00 to NULL To test: 1) Change a patron's value of one or more of the following to 0000-00-00: dateofbirth dateenrolled dateexpiry date_renewed 2) Run the fix_patron_dates.pl (-c -v) 3) Notice the value has changed from 0000-00-00 to NULL Sponsored by: Toi Ohomai Institute of Technology, New Zealand Hi! Thank you for working on this patch. I ran into this error: Missing right curly or square bracket at misc/cronjobs/fix_patron_dates.pl line 53, at end of line syntax error at misc/cronjobs/fix_patron_dates.pl line 53, at EOF After adding the curly this works as described! I'm marking this as Failed QA for now, but once the patch is updated I'll be happy to sign off! Created attachment 154399 [details] [review] Bug 31143: Update patrons with dates of 0000-00-00 to NULL To test: 1) Change a patron's value of one or more of the following to 0000-00-00: dateofbirth dateenrolled dateexpiry date_renewed 2) Run the fix_patron_dates.pl (-c -v) 3) Notice the value has changed from 0000-00-00 to NULL Sponsored by: Toi Ohomai Institute of Technology, New Zealand (In reply to Laura Escamilla from comment #2) > Hi! Thank you for working on this patch. I ran into this error: > > Missing right curly or square bracket at misc/cronjobs/fix_patron_dates.pl > line 53, at end of line > syntax error at misc/cronjobs/fix_patron_dates.pl line 53, at EOF > > After adding the curly this works as described! I'm marking this as Failed > QA for now, but once the patch is updated I'll be happy to sign off! Thanks Laura for pointing that out. I have added the curly bracket to the end of the file and reuploaded the patch :) Created attachment 155964 [details] [review] Bug 31143: Update patrons with dates of 0000-00-00 to NULL To test: 1) Change a patron's value of one or more of the following to 0000-00-00: dateofbirth dateenrolled dateexpiry date_renewed 2) Run the fix_patron_dates.pl (-c -v) 3) Notice the value has changed from 0000-00-00 to NULL Sponsored by: Toi Ohomai Institute of Technology, New Zealand Signed-off-by: David Nind <david@davidnind.com> Testing notes (using KTD): 1. Before applying the patch, I updated a couple of patron records using this SQL (Mary (49) and Henry (19)): . koha-mysql kohadev . update borrowers set dateofbirth ="0000-00-00" where borrowernumber = "49"; . update borrowers set date_renewed ="0000-00-00" where borrowernumber = "19"; 2. Note that the values updated now have a value of 0000-00-00 (SQL: select * from borrowers where borrowernumber = 19;). 3. In the staff interface, go to patrons. Note that the autocomplete for the search in the header doesn't work, and that you get a 500 error if you use the search on the left-hand side: Something went wrong when loading the table. 500: Internal Server Error. Month out of range. 4. Apply the patch (and restart_all). 5. Run the new script: misc/cronjobs/fix_patron_dates.pl -c -v 6. Output is: 2 wrong dates found Some other notes: 1. If you run when there are no wrong dates, you get this (with the -c and -v options): Use of uninitialized value $count in concatenation (.) or string at misc/cronjobs/fix_patron_dates.pl line 53. wrong dates found 2. I'm not sure of the protocol, but for many scripts: - you can add the -h or --help and get quick help with a description, what the different options do, and some examples. - normally the help would have the options, like -v, etc (in the script currently it just has c or v) - sometimes with verbose options, it may list the things changed 3. These could probably be added as new bugs, if determined as warranted. This has been done already. Are you sure it's not coming from migration/import instead? I would add a check in misc/maintenance/search_for_data_inconsistencies.pl instead. IMO it does not make sense to have such specific cronjobs. (In reply to Jonathan Druart from comment #7) > This has been done already. Are you sure it's not coming from > migration/import instead? Just curious where this has been dealt with already. (In reply to Lucas Gass from comment #8) > (In reply to Jonathan Druart from comment #7) > > This has been done already. Are you sure it's not coming from > > migration/import instead? > > Just curious where this has been dealt with already. Search for "sanitize_zero_date" in updatedatabase.pl A lot of people seem to have had problems recently with 0000-00-00 dates... There were two different threads on the mailing list about the same thing in the last month. https://lists.katipo.co.nz/public/koha/2024-June/060716.html https://lists.katipo.co.nz/public/koha/2024-June/060745.html (In reply to Caroline Cyr La Rose from comment #10) > A lot of people seem to have had problems recently with 0000-00-00 dates... > There were two different threads on the mailing list about the same thing in > the last month. > > https://lists.katipo.co.nz/public/koha/2024-June/060716.html > > https://lists.katipo.co.nz/public/koha/2024-June/060745.html One bug we have identified is bug 36488 - date of birth during patron self registration. Looks like there are more fields than "dateofbirth dateenrolled dateexpiry date_renewed" to check :
+lastseen
+updated_on
+debarred
> I would add a check in misc/maintenance/search_for_data_inconsistencies.pl instead.
+1
IIUC Wainui's script also corrected the dates to NULL instead of 0000-00-00. Is this something we can do with search_for_data_inconsistencies? I seem to remember that it corrects item-level itemtypes when the record-level itemtype is present, so I'm guessing yes? Apologies I haven't been able to find time to address the comments on this bug report Removing myself as the assignee, if someone else wants to pick this up, or I can check back in a few days when I have some time to look at it Created attachment 168636 [details] [review] Bug 31143: Update patrons with dates of 0000-00-00 to NULL To test: 1) Change a patron's value of one or more of the following to 0000-00-00: dateofbirth dateenrolled dateexpiry date_renewed lastseen updated_on debarred 2) Run the misc/maintenance/search_for_data_inconsistencies.pl script 3) Notice the value has changed from 0000-00-00 to NULL Sponsored by: Toi Ohomai Institute of Technology, New Zealand This script is designed to report data inconsistencies but it is not expected to fix them. If we want we should at least require a flag (--fix or whatever). But in that case... What about the other inconsistencies? I would suggest to search for 0000-00-00 then display the query to fix them in the hint part. (In reply to Caroline Cyr La Rose from comment #13) > IIUC Wainui's script also corrected the dates to NULL instead of 0000-00-00. > Is this something we can do with search_for_data_inconsistencies? > > I seem to remember that it corrects item-level itemtypes when the > record-level itemtype is present, so I'm guessing yes? No, I don't think it is doing that. (In reply to Caroline Cyr La Rose from comment #13) > IIUC Wainui's script also corrected the dates to NULL instead of 0000-00-00. > Is this something we can do with search_for_data_inconsistencies? Yes, I don't think it is correct. > I seem to remember that it corrects item-level itemtypes when the > record-level itemtype is present, so I'm guessing yes? No, I don't think the script is modifying data for now. Could/should we add this as a command-line option? (In reply to Katrin Fischer from comment #19) > Could/should we add this as a command-line option? It would be nice if we could provide people with some help in fixing the issue as it has come up a lot and can only be fixed with SQL right now. A hint could work too, but might require more explanations. I had a go at testing this yesterday (see my comments on how I tested previously #c6). (I used KTD and the sample data, with some SQL changes to make some patron records incorrect. However, for me it didn't fix anything for me or output a list of patrons that have data that needs correction. I was going to have another go today, but I'll leave it for the moment. I sort of liked how the previously proposed script worked. 8) Hi everyone, > It would be nice if we could provide people with some help in fixing the issue as it has come up a lot and can only be fixed with SQL right now. Yeah I could really use some help please with getting this fix through as soon as possible, it is a huge problem with many of our clients since a recent minor point. > I would suggest to search for 0000-00-00 then display the query to fix them in the hint part. We would prefer the fix automatically fixes the dates, and not just give a prompt. However, I see that my latest patch shouldn't change data in misc/maintenance/search_for_data_inconsistencies.pl > I sort of liked how the previously proposed script worked. I agree, and would be happy to amend it with the extra fields, unless someone can get this working as soon as possible Thanks, Wainui and the Catalyst team Unassigning myself if someone wants to pick it up? Otherwise I can work on it if I have some guidance :) We can have both. The search_for_inconsistencies maintenance script can point to the other script. Also I think the script to fix the dates should not contain "patrons" as we could want to reuse it to fix dates from other database tables. we have a task opened to make a plugin out of this script. Mostly as a "learn the ropes" for interns, but it has the advantage of being easily hackable to make it what we want. Like fix things when there's no doubt as to what must be done. Created attachment 168770 [details] [review] Bug 31143: Update patrons with dates of 0000-00-00 to NULL To test: 1) Change a patron's value of one or more of the following to 0000-00-00: dateofbirth dateenrolled dateexpiry date_renewed lastseen updated_on debarred 2) Run the misc/maintenance/search_for_data_inconsistencies.pl script 3) Run the fix_patron_dates.pl (-c -v) 4) Notice the value has changed from 0000-00-00 to NULL Sponsored by: Toi Ohomai Institute of Technology, New Zealand and Catalyst IT Thanks Wainui! I had a go at testing. 1. For me, misc/maintenance/search_for_data_inconsistencies.pl does not show anything in the output when I run it - where there are incorrect dates. I'm not sure if this is because of the error it generates when running (see step 2 below) - this occurred both before and after the patch was applied. 2. If the new script is run just with -v, it just says "0 wrong dates found and fixed". I guess that is correct. But if you were running it before using the -c option, I think you would want to know that "X wrong dates were found" (or something similar). 3. On the plus side, the new script does fix 0000-00-00 dates! 8-) Updated testing notes (using KTD and the sample data): 1. Fix record 369 (Strk̆v̜elser illustreret af Jean Anderson dansk udgave ved Lis Engel [overst̆telse ved Jesper Langer]) so that the misc/maintenance/search_for_data_inconsistencies.pl script doesn't show MARC records in the sample data that needs fixing. 2. Note that before applying the patch, running the search for data inconsistencies script returns an error/warning message: misc/maintenance/search_for_data_inconsistencies.pl Can't call method "time_zone" on an undefined value at /usr/lib/x86_64-linux-gnu/perl5/5.36/DateTime.pm line 1483. 3. I updated a couple of patron records using this SQL (Mary (49) and Henry (19)): . koha-mysql kohadev . update borrowers set dateofbirth ="0000-00-00" where borrowernumber = "49"; . update borrowers set date_renewed ="0000-00-00" where borrowernumber = "19"; 4. Note that the values updated now have a value of 0000-00-00 (SQL example: select * from borrowers where borrowernumber = 19;). 5. In the staff interface, go to patrons. Note that the autocomplete for the search in the header doesn't work, and that you get a 500 error if you use the search on the left-hand side: Something went wrong when loading the table. 500: Internal Server Error. Month out of range. 6. Run the search for data inconsistencies script: misc/maintenance/search_for_data_inconsistencies.pl NOTE: This shows no information for me about any problems with patron dates with 0000-00-00 7. Output from running the new script: - with just -v: misc/cronjobs/fix_invalid_dates.pl -v 0 wrong dates found and fixed - with -c -v: misc/cronjobs/fix_invalid_dates.pl -c -v 2 wrong dates found and fixed 8. Repeat step 5 - things should now work as expected. Some other notes: 1. The description in misc/maintenance/search_for_data_inconsistencies.pl needs updating to add: * Patrons with 0000-00-00 dates in fields dateofbirth, dateenrolled, dateexpiry, date_renewed, lastseen, updated_on debarred 2. I'm not sure of the protocol, but for many scripts: - you can add the -h or --help and get quick help with a description, what the different options do, and some examples. - normally the help would have the options, like -v, etc (in the script currently it just has c or v) - sometimes with verbose options, it may list the things changed 3. These could probably be added as new bugs, if determined as warranted. Created attachment 168775 [details] [review] Bug 31143: Update patrons with dates of 0000-00-00 to NULL To test: 1) Change a patron's value of one or more of the following to 0000-00-00: dateofbirth dateenrolled dateexpiry date_renewed lastseen updated_on debarred 2) Run the misc/maintenance/search_for_data_inconsistencies.pl script 3) Run the fix_patron_dates.pl (-c -v) 4) Notice the value has changed from 0000-00-00 to NULL Sponsored by: Toi Ohomai Institute of Technology, New Zealand and Catalyst IT Created attachment 168776 [details] [review] Bug 31143: Update patrons with dates of 0000-00-00 to NULL To test: 1) Change a patron's value of one or more of the following to 0000-00-00: dateofbirth dateenrolled dateexpiry date_renewed lastseen updated_on debarred 2) Run the misc/maintenance/search_for_data_inconsistencies.pl script 3) Run the fix_patron_dates.pl (-c -v) 4) Notice the value has changed from 0000-00-00 to NULL Sponsored by: Toi Ohomai Institute of Technology, New Zealand and Catalyst IT I get this error message after I have updated some patrons to have 0000-00-00 dates - both before and after the patch. misc/maintenance/search_for_data_inconsistencies.pl Can't call method "time_zone" on an undefined value at /usr/lib/x86_64-linux-gnu/perl5/5.36/DateTime.pm line 1483. Without the patch and without any incorrect dates, the above message doesn't appear. After the patch and after the dates are fixed with the new script, I get (as expected): misc/maintenance/search_for_data_inconsistencies.pl -v == 0 invalid dates found == (In reply to David Nind from comment #30) > I get this error message after I have updated some patrons to have > 0000-00-00 dates - both before and after the patch. > > misc/maintenance/search_for_data_inconsistencies.pl > Can't call method "time_zone" on an undefined value at > /usr/lib/x86_64-linux-gnu/perl5/5.36/DateTime.pm line 1483. > > Without the patch and without any incorrect dates, the above message doesn't > appear. > > After the patch and after the dates are fixed with the new script, I get (as > expected): > > misc/maintenance/search_for_data_inconsistencies.pl -v > == 0 invalid dates found == Thanks for testing and for your notes! - I changed a date to 0000-00-00 - I ran misc/maintenance/search_for_data_inconsistencies.pl -> I don't get that error - I added patch - I changed a date to 0000-00-00 - I ran misc/maintenance/search_for_data_inconsistencies.pl -> I don't get that error -> I get, as expected: == 1 invalid dates found == You may change the date with script: misc/cronjobs/fix_invalid_dates.pl (-c -v) - I then ran ./misc/cronjobs/fix_invalid_dates.pl -v -> I got the following, as expected: 1 invalid dates found - I then ran ./misc/cronjobs/fix_invalid_dates.pl -c -v -> I got the following, as expected: 1 invalid dates found 1 invalid dates fixed - Running it again -> I got, as expected: 0 invalid dates found 0 invalid dates fixed - Then I checked the date in the db, and it has indeed been fixed (changed to NULL) So yeah, I can't seem to replicate your issues :/ (In reply to wainuiwitikapark from comment #31) > (In reply to David Nind from comment #30) > > I get this error message after I have updated some patrons to have > > 0000-00-00 dates - both before and after the patch. > > > > misc/maintenance/search_for_data_inconsistencies.pl > > Can't call method "time_zone" on an undefined value at > > /usr/lib/x86_64-linux-gnu/perl5/5.36/DateTime.pm line 1483. > > > > Without the patch and without any incorrect dates, the above message doesn't > > appear. > > > > After the patch and after the dates are fixed with the new script, I get (as > > expected): > > > > misc/maintenance/search_for_data_inconsistencies.pl -v > > == 0 invalid dates found == > > Thanks for testing and for your notes! > > - I changed a date to 0000-00-00 > - I ran misc/maintenance/search_for_data_inconsistencies.pl > -> I don't get that error > > - I added patch > - I changed a date to 0000-00-00 > - I ran misc/maintenance/search_for_data_inconsistencies.pl > -> I don't get that error > -> I get, as expected: > == 1 invalid dates found == > You may change the date with script: misc/cronjobs/fix_invalid_dates.pl (-c > -v) > - I then ran ./misc/cronjobs/fix_invalid_dates.pl -v > -> I got the following, as expected: > 1 invalid dates found > - I then ran ./misc/cronjobs/fix_invalid_dates.pl -c -v > -> I got the following, as expected: > 1 invalid dates found > 1 invalid dates fixed > - Running it again > -> I got, as expected: > 0 invalid dates found > 0 invalid dates fixed > - Then I checked the date in the db, and it has indeed been fixed (changed > to NULL) > > So yeah, I can't seem to replicate your issues :/ Oh sorry I have retested and can now replicate the issue (In reply to David Nind from comment #30) > I get this error message after I have updated some patrons to have > 0000-00-00 dates - both before and after the patch. > > misc/maintenance/search_for_data_inconsistencies.pl > Can't call method "time_zone" on an undefined value at > /usr/lib/x86_64-linux-gnu/perl5/5.36/DateTime.pm line 1483. > > Without the patch and without any incorrect dates, the above message doesn't > appear. > > After the patch and after the dates are fixed with the new script, I get (as > expected): > > misc/maintenance/search_for_data_inconsistencies.pl -v > == 0 invalid dates found == What date fields were you changing? Were you changing dateofbirth? I have done some more testing and I think it is only dateofbirth that gives the error (In reply to wainuiwitikapark from comment #33) > > What date fields were you changing? Were you changing dateofbirth? > > I have done some more testing and I think it is only dateofbirth that gives > the error Thanks for checking and trying to replicate the issue I was having! I tested with changed dates for dateofbirth and date_renewed. The error happens with changing the dates before the patch is applied, so it must be something else already in the script. (In reply to David Nind from comment #34) > (In reply to wainuiwitikapark from comment #33) > > > > What date fields were you changing? Were you changing dateofbirth? > > > > I have done some more testing and I think it is only dateofbirth that gives > > the error > > Thanks for checking and trying to replicate the issue I was having! > > I tested with changed dates for dateofbirth and date_renewed. > > The error happens with changing the dates before the patch is applied, so it > must be something else already in the script. No worries and thanks for clarifying that! (In reply to David Nind from comment #30) > I get this error message after I have updated some patrons to have > 0000-00-00 dates - both before and after the patch. > > misc/maintenance/search_for_data_inconsistencies.pl > Can't call method "time_zone" on an undefined value at > /usr/lib/x86_64-linux-gnu/perl5/5.36/DateTime.pm line 1483. > > Without the patch and without any incorrect dates, the above message doesn't > appear. > > After the patch and after the dates are fixed with the new script, I get (as > expected): > > misc/maintenance/search_for_data_inconsistencies.pl -v > == 0 invalid dates found == Hi David, Do you have any ideas on how I could potentially fix the time zone issue? Thanks :) (In reply to wainuiwitikapark from comment #36) > (In reply to David Nind from comment #30) > > I get this error message after I have updated some patrons to have > > 0000-00-00 dates - both before and after the patch. > > > > misc/maintenance/search_for_data_inconsistencies.pl > > Can't call method "time_zone" on an undefined value at > > /usr/lib/x86_64-linux-gnu/perl5/5.36/DateTime.pm line 1483. > > > > Without the patch and without any incorrect dates, the above message doesn't > > appear. > > > > After the patch and after the dates are fixed with the new script, I get (as > > expected): > > > > misc/maintenance/search_for_data_inconsistencies.pl -v > > == 0 invalid dates found == > > Hi David, > > Do you have any ideas on how I could potentially fix the time zone issue? > > Thanks :) Sorry, I'm not a programmer and don't know Perl at all 8-(. I've looked into this a bit and this would either need: 1) dt_from_string to return a DateTime object instead of undef. 2) A fix for all dt_from_string calls within Koha::Patron. Pick your poison :D I'd like to hear some opinions. dt_from_string receiving something starting w/ '0000-0' that is. Koha/DateUtils.pm:62 `return if $date_string and $date_string =~ m|^0000-0|;` Created attachment 170086 [details] [review] Bug 31143: Work around subtracting datetime if date_of_birth undef To test: 1) Run the following SQL: update borrowers set dateofbirth = '0000-00-00' where borrowernumber = 45; 2) Access the borrower edit page through staff UI: http://localhost:8081/cgi-bin/koha/members/memberentry.pl?op=edit_form&destination=circ&borrowernumber=45 3) Notice you get an error 4) Run the script: perl misc/maintenance/search_for_data_inconsistencies.pl 5) Notice you get an error 6) Apply patch restart plack and repeat -> no errors This is not perfect but its all I have time for, for now. Follow-ups are welcomed as always. > my $borrowers = Koha::Patrons->search(
Looks very strange to me this var name.
We usually name it $patrons when it is a Koha::Patrons object.
> Work around subtracting datetime if date_of_birth undef
This change will impact unit test 't/db_dependent/Koha/Patrons.t' no ?
(In reply to Fridolin Somers from comment #42) > > Work around subtracting datetime if date_of_birth undef > This change will impact unit test 't/db_dependent/Koha/Patrons.t' no ? prove t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Patrons.t .. ok All tests successful. Files=1, Tests=43, 42 wallclock secs ( 0.06 usr 0.01 sys + 19.83 cusr 20.13 csys = 40.03 CPU) Result: PASS It doesn't seem to impact t/db_dependent/Koha/Patrons.t. I would expect it to impact it only if that test suite is already considering this possibility (undef $date_of_birth) which I don't believe it is, but I may be wrong. If the above is true, a new regression test for this should probably be added in t/db_dependent/Koha/Patrons.t (In reply to Fridolin Somers from comment #41) > > my $borrowers = Koha::Patrons->search( > > Looks very strange to me this var name. > We usually name it $patrons when it is a Koha::Patrons object. I can change it if you think that's best? Kia ora Wainui I have just found that this issue doesn't just affect the borrowers table, I just had to fix this for a database where some of the items table also included the bad daterange, specifically the replacementpricedate column. It may be worth to include the columns within the items table in this script to better capture all of the possible places this bad daterange may end up. From the items table the following columns could be affected: dateaccessioned replacementpricedate datelastborrowed datelastseen damaged_on itemlost_on withdrawn_on deleted_on onloan Cheers, Danyon (In reply to Danyon Sewell from comment #45) > Kia ora Wainui > > I have just found that this issue doesn't just affect the borrowers table, I > just had to fix this for a database where some of the items table also > included the bad daterange, specifically the replacementpricedate column. > > It may be worth to include the columns within the items table in this script > to better capture all of the possible places this bad daterange may end up. > > From the items table the following columns could be affected: > > dateaccessioned > replacementpricedate > datelastborrowed > datelastseen > damaged_on > itemlost_on > withdrawn_on > deleted_on > onloan > > Cheers, > Danyon Thanks Danyon! I'll look into that! Created attachment 170697 [details] [review] Bug 31143: Update patrons with dates of 0000-00-00 to NULL To test: 1) Change a patron's value of one or more of the following to 0000-00-00: dateofbirth dateenrolled dateexpiry date_renewed lastseen updated_on debarred 2) Run the misc/maintenance/search_for_data_inconsistencies.pl script 3) Run the fix_patron_dates.pl (-c -v) 4) Notice the value has changed from 0000-00-00 to NULL Sponsored by: Toi Ohomai Institute of Technology, New Zealand and Catalyst IT Signed-off-by: David Nind <david@davidnind.com> Created attachment 170698 [details] [review] Bug 31143: Work around subtracting datetime if date_of_birth undef To test: 1) Run the following SQL: update borrowers set dateofbirth = '0000-00-00' where borrowernumber = 45; 2) Access the borrower edit page through staff UI: http://localhost:8081/cgi-bin/koha/members/memberentry.pl?op=edit_form&destination=circ&borrowernumber=45 3) Notice you get an error 4) Run the script: perl misc/maintenance/search_for_data_inconsistencies.pl 5) Notice you get an error 6) Apply patch restart plack and repeat -> no errors This is not perfect but its all I have time for, for now. Follow-ups are welcomed as always. Signed-off-by: David Nind <david@davidnind.com> Testing notes (using KTD): 1. Fix record 369 (Strk̆v̜elser illustreret af Jean Anderson dansk udgave ved Lis Engel [overst̆telse ved Jesper Langer]) so that the misc/maintenance/search_for_data_inconsistencies.pl script doesn't show MARC records in the sample data that needs fixing. 2. Update a couple of patron records using this SQL (Mary (49) and Henry (19)): . koha-mysql kohadev . update borrowers set dateofbirth ="0000-00-00" where borrowernumber = "49"; . update borrowers set date_renewed ="0000-00-00" where borrowernumber = "19"; 3. Note that the values updated now have a value of 0000-00-00 (SQL example: select * from borrowers where borrowernumber = 19;). 4. In the staff interface, go to patrons. Note that the autocomplete for the search in the header doesn't work (search for hen), and that you get a 500 error if you use the search on the left-hand side: Something went wrong when loading the table. 500: Internal Server Error. Month out of range. 5. Run the search for data inconsistencies script: misc/maintenance/search_for_data_inconsistencies.pl. You will get this error: Can't call method "time_zone" on an undefined value at /usr/lib/x86_64-linux-gnu/perl5/5.36/DateTime.pm line 1483. 6. Apply the second patch: Bug 31143: Work around subtracting datetime if date_of_birth undef 7. Run this command: misc/maintenance/search_for_data_inconsistencies.pl -v Result: == Patrons with invalid age for category == * Patron borrowernumber=49 has an invalid age of 0 for their category 'PT' (18 to 999) => You may change the patron's category automatically with misc/cronjobs/update_patrons_category.pl 8. Apply the first patch. 9. Run this command again: misc/maintenance/search_for_data_inconsistencies.pl -v Result: now identifies that some patrons have invalid dates == Patrons with invalid age for category == * Patron borrowernumber=49 has an invalid age of 0 for their category 'PT' (18 to 999) => You may change the patron's category automatically with misc/cronjobs/update_patrons_category.pl == 2 invalid dates found == => You may change the date with script: misc/cronjobs/fix_invalid_dates.pl (-c -v) 10. Run the new script with just the -v option: misc/cronjobs/fix_invalid_dates.pl -v 2 invalid dates found 11. Run the new script with the -c -v options: misc/cronjobs/fix_invalid_dates.pl -c -v 2 wrong dates found and fixed 12. Repeat step 4 - things should now work as expected. Created attachment 171812 [details] [review] Bug 31143: Update patrons with dates of 0000-00-00 to NULL To test: 1) Change a patron's value of one or more of the following to 0000-00-00: dateofbirth dateenrolled dateexpiry date_renewed lastseen updated_on debarred 2) Run the misc/maintenance/search_for_data_inconsistencies.pl script 3) Run the fix_patron_dates.pl (-c -v) 4) Notice the value has changed from 0000-00-00 to NULL Sponsored by: Toi Ohomai Institute of Technology, New Zealand and Catalyst IT Signed-off-by: David Nind <david@davidnind.com> Created attachment 171813 [details] [review] Bug 31143: Update patrons with dates of 0000-00-00 to NULL To test: 1) Change a patron's value of one or more of the following to 0000-00-00: dateofbirth dateenrolled dateexpiry date_renewed lastseen updated_on debarred 2) Run the misc/maintenance/search_for_data_inconsistencies.pl script 3) Run the fix_patron_dates.pl (-c -v) 4) Notice the value has changed from 0000-00-00 to NULL Sponsored by: Toi Ohomai Institute of Technology, New Zealand and Catalyst IT Created attachment 171814 [details] [review] Bug 31143: Update patrons with dates of 0000-00-00 to NULL To test: 1) Change a patron's value of one or more of the following to 0000-00-00: dateofbirth dateenrolled dateexpiry date_renewed lastseen updated_on debarred 2) Run the misc/maintenance/search_for_data_inconsistencies.pl script 3) Run the fix_invalid_patron_dates.pl (-c -v) 4) Notice the value has changed from 0000-00-00 to NULL Sponsored by: Toi Ohomai Institute of Technology, New Zealand and Catalyst IT Created attachment 171815 [details] [review] Bug 31143: Update patrons with dates of 0000-00-00 to NULL To test: 1) Change a patron's value of one or more of the following to 0000-00-00: dateofbirth dateenrolled dateexpiry date_renewed lastseen updated_on debarred 2) Run the misc/maintenance/search_for_data_inconsistencies.pl script 3) Run the fix_invalid_patron_dates.pl (-c -v) 4) Notice the value has changed from 0000-00-00 to NULL Sponsored by: Toi Ohomai Institute of Technology, New Zealand and Catalyst IT Created attachment 171816 [details] [review] Bug 31143: Update items with dates of 0000-00-00 to NULL To test: 1) Change a patron's value of one or more of the following to 0000-00-00: dateaccessioned replacementpricedate datelastborrowed datelastseen damaged_on itemlost_on withdrawn_on deleted_on onloan 2) Run the misc/maintenance/search_for_data_inconsistencies.pl script 3) Run the fix_invalid_item_dates.pl (-c -v) 4) Notice the value has changed from 0000-00-00 to NULL Created attachment 171817 [details] [review] Bug 31143: Update items with dates of 0000-00-00 to NULL To test: 1) Change an item's value of one or more of the following to 0000-00-00: dateaccessioned replacementpricedate datelastborrowed datelastseen damaged_on itemlost_on withdrawn_on deleted_on onloan 2) Run the misc/maintenance/search_for_data_inconsistencies.pl script 3) Run the fix_invalid_item_dates.pl (-c -v) 4) Notice the value has changed from 0000-00-00 to NULL Hi I added in behaviour to fix incorrect item dates too Hello Wainui, The title of this bug report states that the patches are supposed to "identify all cases in the database". But with those 2 patches we are only dealing with two tables: items and patrons. I am suggesting the following snippet of code that will allow you to catch invalid dates in the whole database: use Koha::Database; my $schema = Koha::Database->new->schema; # Loop over all the DBIx::Class classes for my $class ( sort values %{$schema->{class_mappings}} ) { # Retrieve the resultset so we can access the columns info my $rs = $schema->resultset($class); my $columns = $rs->result_source->columns_info; # Loop over the columns while ( my ( $column, $info ) = each %$columns ) { # Next if data type is not date/datetime/timestamp my $data_type = $info->{data_type}; next unless grep { $data_type =~ m{^$_$} } qw( timestamp datetime date ); # Count the invalid dates my $invalid_dates = $rs->search({ $column => '0000-00-00' })->count; next unless $invalid_dates; # "items.withdrawn_on contains 42 invalid dates" say sprintf "Column %s.%s contains %s invalid dates", $rs->result_source->name, $column, $invalid_dates; } } Let me know if you have any questions about it. I can help you to adjust it to your needs. (In reply to Jonathan Druart from comment #57) > Hello Wainui, > The title of this bug report states that the patches are supposed to > "identify all cases in the database". But with those 2 patches we are only > dealing with two tables: items and patrons. > > I am suggesting the following snippet of code that will allow you to catch > invalid dates in the whole database: > > use Koha::Database; > my $schema = Koha::Database->new->schema; > # Loop over all the DBIx::Class classes > for my $class ( sort values %{$schema->{class_mappings}} ) { > # Retrieve the resultset so we can access the columns info > my $rs = $schema->resultset($class); > my $columns = $rs->result_source->columns_info; > > # Loop over the columns > while ( my ( $column, $info ) = each %$columns ) { > # Next if data type is not date/datetime/timestamp > my $data_type = $info->{data_type}; > next unless grep { $data_type =~ m{^$_$} } qw( timestamp datetime > date ); > > # Count the invalid dates > my $invalid_dates = $rs->search({ $column => '0000-00-00' })->count; > > next unless $invalid_dates; > > # "items.withdrawn_on contains 42 invalid dates" > say sprintf "Column %s.%s contains %s invalid dates", > $rs->result_source->name, $column, $invalid_dates; > } > } > > Let me know if you have any questions about it. I can help you to adjust it > to your needs. Thanks Jonathan! I will have a go at implementing this Created attachment 171918 [details] [review] Bug 31143: Fix/identify all cases where '0000-00-00' may still remain in the database This patch identifies date fields in the database (timestamp, datetime, date) that are 0000-00-00 and sets them to NULL Test plan - Examples of setting up some 0000-00-00 date fields: e.g. Borrowers table update borrowers set dateexpiry="0000-00-00" where borrowernumber=49; e.g. Items table update items set datelastseen="0000-00-00" where itemnumber=12; To identify the cases: 1) Set up some 0000-00-00 date fields 2) Run the misc/maintenance/search_for_data_inconsistencies.pl script 3) Run fix_invalid_dates.pl -v To fix the cases: 1) Run fix_invalid_dates.pl -c -v 2) Notice the value has changed from 0000-00-00 to NULL e.g. Borrowers table select dateexpiry from borrowers where borrowernumber=49; e.g. Items table select datelastseen from items where itemnumber=12; Created attachment 171976 [details] [review] Bug 31143: Work around subtracting datetime if date_of_birth undef To test: 1) Run the following SQL: update borrowers set dateofbirth = '0000-00-00' where borrowernumber = 45; 2) Access the borrower edit page through staff UI: http://localhost:8081/cgi-bin/koha/members/memberentry.pl?op=edit_form&destination=circ&borrowernumber=45 3) Notice you get an error 4) Run the script: perl misc/maintenance/search_for_data_inconsistencies.pl 5) Notice you get an error 6) Apply patch restart plack and repeat -> no errors This is not perfect but its all I have time for, for now. Follow-ups are welcomed as always. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Shi Yao Wang <shi-yao.wang@inlibro.com> Created attachment 171977 [details] [review] Bug 31143: Fix/identify all cases where '0000-00-00' may still remain in the database This patch identifies date fields in the database (timestamp, datetime, date) that are 0000-00-00 and sets them to NULL Test plan - Examples of setting up some 0000-00-00 date fields: e.g. Borrowers table update borrowers set dateexpiry="0000-00-00" where borrowernumber=49; e.g. Items table update items set datelastseen="0000-00-00" where itemnumber=12; To identify the cases: 1) Set up some 0000-00-00 date fields 2) Run the misc/maintenance/search_for_data_inconsistencies.pl script 3) Run fix_invalid_dates.pl -v To fix the cases: 1) Run fix_invalid_dates.pl -c -v 2) Notice the value has changed from 0000-00-00 to NULL e.g. Borrowers table select dateexpiry from borrowers where borrowernumber=49; e.g. Items table select datelastseen from items where itemnumber=12; Signed-off-by: Shi Yao Wang <shi-yao.wang@inlibro.com> (In reply to Shi Yao Wang from comment #61) > Created attachment 171977 [details] [review] [review] > Bug 31143: Fix/identify all cases where '0000-00-00' may still remain in the > database > > This patch identifies date fields in the database (timestamp, datetime, > date) that are 0000-00-00 and sets them to NULL > > Test plan - > > Examples of setting up some 0000-00-00 date fields: > > e.g. Borrowers table > update borrowers set dateexpiry="0000-00-00" where borrowernumber=49; > > e.g. Items table > update items set datelastseen="0000-00-00" where itemnumber=12; > > To identify the cases: > 1) Set up some 0000-00-00 date fields > 2) Run the misc/maintenance/search_for_data_inconsistencies.pl script > 3) Run fix_invalid_dates.pl -v > > To fix the cases: > 1) Run fix_invalid_dates.pl -c -v > 2) Notice the value has changed from 0000-00-00 to NULL > > e.g. Borrowers table > select dateexpiry from borrowers where borrowernumber=49; > > e.g. Items table > select datelastseen from items where itemnumber=12; > > Signed-off-by: Shi Yao Wang <shi-yao.wang@inlibro.com> Thanks so much for signing off! Created attachment 172138 [details] [review] Bug 31143: Work around subtracting datetime if date_of_birth undef To test: 1) Run the following SQL: update borrowers set dateofbirth = '0000-00-00' where borrowernumber = 45; 2) Access the borrower edit page through staff UI: http://localhost:8081/cgi-bin/koha/members/memberentry.pl?op=edit_form&destination=circ&borrowernumber=45 3) Notice you get an error 4) Run the script: perl misc/maintenance/search_for_data_inconsistencies.pl 5) Notice you get an error 6) Apply patch restart plack and repeat -> no errors This is not perfect but its all I have time for, for now. Follow-ups are welcomed as always. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Shi Yao Wang <shi-yao.wang@inlibro.com> Created attachment 172139 [details] [review] Bug 31143: Fix/identify all cases where '0000-00-00' may still remain in the database This patch identifies date fields in the database (timestamp, datetime, date) that are 0000-00-00 and sets them to NULL Test plan - Examples of setting up some 0000-00-00 date fields: e.g. Borrowers table update borrowers set dateexpiry="0000-00-00" where borrowernumber=49; e.g. Items table update items set datelastseen="0000-00-00" where itemnumber=12; To identify the cases: 1) Set up some 0000-00-00 date fields 2) Run the misc/maintenance/search_for_data_inconsistencies.pl script 3) Run fix_invalid_dates.pl -v To fix the cases: 1) Run fix_invalid_dates.pl -c -v 2) Notice the value has changed from 0000-00-00 to NULL e.g. Borrowers table select dateexpiry from borrowers where borrowernumber=49; e.g. Items table select datelastseen from items where itemnumber=12; Signed-off-by: Shi Yao Wang <shi-yao.wang@inlibro.com> Created attachment 172140 [details] [review] Bug 31143: (QA follow-up) move script to maintenance Created attachment 172141 [details] [review] Bug 31143: (QA follow-up) Tidy I moved the script from cronjobs to maintenance as I feel it should be avoided to be creating these, not expected to need fixing on a schedule, this can be dropped if desired. I also tidied. It would be nice to have increased verbosity and list which object were affected, but reporting the column allows for reporting, so this works. Nice patches, passing QA Created attachment 172143 [details] [review] Bug 31143: Work around subtracting datetime if date_of_birth undef To test: 1) Run the following SQL: update borrowers set dateofbirth = '0000-00-00' where borrowernumber = 45; 2) Access the borrower edit page through staff UI: http://localhost:8081/cgi-bin/koha/members/memberentry.pl?op=edit_form&destination=circ&borrowernumber=45 3) Notice you get an error 4) Run the script: perl misc/maintenance/search_for_data_inconsistencies.pl 5) Notice you get an error 6) Apply patch restart plack and repeat -> no errors This is not perfect but its all I have time for, for now. Follow-ups are welcomed as always. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Shi Yao Wang <shi-yao.wang@inlibro.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 172144 [details] [review] Bug 31143: Fix/identify all cases where '0000-00-00' may still remain in the database This patch identifies date fields in the database (timestamp, datetime, date) that are 0000-00-00 and sets them to NULL Test plan - Examples of setting up some 0000-00-00 date fields: e.g. Borrowers table update borrowers set dateexpiry="0000-00-00" where borrowernumber=49; e.g. Items table update items set datelastseen="0000-00-00" where itemnumber=12; To identify the cases: 1) Set up some 0000-00-00 date fields 2) Run the misc/maintenance/search_for_data_inconsistencies.pl script 3) Run fix_invalid_dates.pl -v To fix the cases: 1) Run fix_invalid_dates.pl -c -v 2) Notice the value has changed from 0000-00-00 to NULL e.g. Borrowers table select dateexpiry from borrowers where borrowernumber=49; e.g. Items table select datelastseen from items where itemnumber=12; Signed-off-by: Shi Yao Wang <shi-yao.wang@inlibro.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 172145 [details] [review] Bug 31143: (QA follow-up) move script to maintenance Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 172146 [details] [review] Bug 31143: (QA follow-up) Tidy Signed-off-by: Nick Clemens <nick@bywatersolutions.com> I think given the amount of times we had this issue come up in chat and elsewhere, this will be much appreciated. Thanks for your perseverance Wainui! One small thing: I think there is no way to have the script tell you what it does. Could we add some kind of description about the purpose of the script to it? Pushed for 24.11! Well done everyone, thank you! After pushing this I realized that Koha doesn't like borrowers.dateexpiry to be NULL :( The patron auto-complete search dies: Uncaught TypeError: item.expiry_date is null The error is the same if you use the auto-complete in the top search or when you transfer a list to another patron (found this while testing bug 30955) What I did was: * UPDATE borrowers set dateexpiry = "0000-00-00"; * Ran the script without -c to check that it would not change data * Then forgot I had not fixed the data and kept testing things... * I tested bug 30955, realized the problem and ran it with -c * auto-complete was still broken with the fixed data * I manually ran: UPDATE borrowers set dateexpiry = NULL; Still no luck. We either need to fix the auto-complete patron search or have the script re-calculate the dateexpiry from the patron category. I am leaning towards making the patron search deal with NULL. (In reply to Katrin Fischer from comment #74) > We either need to fix the auto-complete patron search or have the script > re-calculate the dateexpiry from the patron category. I am leaning towards > making the patron search deal with NULL. borrowers.dateexpiry can be NULL so the code needs to be fixed. I have not tried it but this would be the idea: iff --git a/koha-tmpl/intranet-tmpl/prog/js/staff-global.js b/koha-tmpl/intranet-tmpl/prog/js/staff-global.js index 73a4a0251e1..28060fb35da 100644 --- a/koha-tmpl/intranet-tmpl/prog/js/staff-global.js +++ b/koha-tmpl/intranet-tmpl/prog/js/staff-global.js @@ -694,10 +694,13 @@ function patron_autocomplete(node, options) { } let new_date = new Date(); let today = Date.parse($date_to_rfc3339($date(new_date.toString()))); - let expiration = Date.parse( - $date_to_rfc3339($date(item.expiry_date.toString())) - ); - let expired = today > expiration; + let expired = false; + if (item.expiry_date){ + let expiration = Date.parse( + $date_to_rfc3339($date(item.expiry_date.toString())) + ); + expired = today > expiration; + } return $("<li></li>") .addClass(loggedInClass) .data("ui-autocomplete-item", item) (In reply to Katrin Fischer from comment #74) > We either need to fix the auto-complete patron search or have the script > re-calculate the dateexpiry from the patron category. I am leaning towards > making the patron search deal with NULL. We have an installation currently where auto complete is erroring because a couple recently created patrons have updated_on as '0000-00-00'. I'm still investigating the cause (as I can't reproduce) but just adding this information here as well. (In reply to Pedro Amorim from comment #76) > (In reply to Katrin Fischer from comment #74) > > We either need to fix the auto-complete patron search or have the script > > re-calculate the dateexpiry from the patron category. I am leaning towards > > making the patron search deal with NULL. > > We have an installation currently where auto complete is erroring because a > couple recently created patrons have updated_on as '0000-00-00'. I'm still > investigating the cause (as I can't reproduce) but just adding this > information here as well. I have filed a new bug for the auto-complete issue: Bug 38254 - Patron auto-complete search explodes when dateexpiry is NULL (In reply to Katrin Fischer from comment #72) > I think given the amount of times we had this issue come up in chat and > elsewhere, this will be much appreciated. Thanks for your perseverance > Wainui! Thanks Katrin!! > One small thing: > I think there is no way to have the script tell you what it does. Could we > add some kind of description about the purpose of the script to it? Are you still wanting me to do this? (In reply to Wainui Witika-Park from comment #78) > (In reply to Katrin Fischer from comment #72) > > I think given the amount of times we had this issue come up in chat and > > elsewhere, this will be much appreciated. Thanks for your perseverance > > Wainui! > > Thanks Katrin!! > > > One small thing: > > I think there is no way to have the script tell you what it does. Could we > > add some kind of description about the purpose of the script to it? > > Are you still wanting me to do this? Yes please! I am leaving "additional_work_needed" as a keyword on anything with questions or a follow-up request. We moved the dateexpiry issue to another spot, but I think giving people a good clue about the script who didn't start out at the inconsistencies one would still be helpful. (In reply to Pedro Amorim from comment #76) > We have an installation currently where auto complete is erroring because a > couple recently created patrons have updated_on as '0000-00-00'. I'm still > investigating the cause (as I can't reproduce) but just adding this > information here as well. Leaving info here that this relates to bug 36822 Created attachment 173900 [details] [review] Bug 31143: (Follow-up) Add description to fix_invalid_dates.pl Sorry Wainui, I had missed the follow-up somehow! Pushed to main now. Backported to 24.05.x for upcoming 24.05.06 (In reply to Lucas Gass (lukeg) from comment #83) > Backported to 24.05.x for upcoming 24.05.06 Please be aware of bug 38254 - currently we break auto-complete with the script, although it does the right thing. We need this fixed too. > Please be aware of bug 38254 - currently we break auto-complete with the
> script, although it does the right thing. We need this fixed too.
Thanks. Do you think I should revert this for now?
(In reply to Lucas Gass (lukeg) from comment #85) > > Please be aware of bug 38254 - currently we break auto-complete with the > > script, although it does the right thing. We need this fixed too. > > Thanks. Do you think I should revert this for now? it's broken patron search vs. broken auto-complete - both are not great. I would really prefer a patch for bug 38254 :) If anyone else is interested: We have been setting up the cronjob in cron.daily/koha-custom #!/bin/sh sudo koha-foreach --enabled /usr/share/koha/bin/maintenance/fix_invalid_dates.pl -c -v Pushed to 23.11.x for 23.11.11 I see nobody's talking about the error when the below line gets executed my $invalid_dates = $rs->search( { $column => '0000-00-00' } )->count; This is throwing the error as below: DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Incorrect TIMESTAMP value: '0000-00-00' at ./fix_invalid_dates.pl line 65 I have made a quick fix for this. Please include to the next minor update (after the line:- my $schema = Koha::Database->new->schema;) if the similar is not implemented. # Disable strict mode for this session $schema->storage->dbh_do( sub { my ($storage, $dbh) = @_; $dbh->do("SET SESSION sql_mode = ''"); } ); (In reply to Aditya from comment #89) > I see nobody's talking about the error when the below line gets executed > > my $invalid_dates = $rs->search( { $column => '0000-00-00' } )->count; > > This is throwing the error as below: > DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st > execute failed: Incorrect TIMESTAMP value: '0000-00-00' at > ./fix_invalid_dates.pl line 65 > > I have made a quick fix for this. Please include to the next minor update > (after the line:- my $schema = Koha::Database->new->schema;) if the similar > is not implemented. > > > # Disable strict mode for this session > $schema->storage->dbh_do( > sub { > my ($storage, $dbh) = @_; > $dbh->do("SET SESSION sql_mode = ''"); > } > ); Hi Aditya, since this is already in a release, we need a new bug number for the fix. Can you please file one and add a test plan? It might depend on DBMS version, so that might be why we didn't see that in testing. I remember we had a similar error. Please also add your DBMS and the version to the report! (In reply to Katrin Fischer from comment #90) > (In reply to Aditya from comment #89) > > I see nobody's talking about the error when the below line gets executed > > > > my $invalid_dates = $rs->search( { $column => '0000-00-00' } )->count; > > > > This is throwing the error as below: > > DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st > > execute failed: Incorrect TIMESTAMP value: '0000-00-00' at > > ./fix_invalid_dates.pl line 65 > > > > I have made a quick fix for this. Please include to the next minor update > > (after the line:- my $schema = Koha::Database->new->schema;) if the similar > > is not implemented. > > > > > > # Disable strict mode for this session > > $schema->storage->dbh_do( > > sub { > > my ($storage, $dbh) = @_; > > $dbh->do("SET SESSION sql_mode = ''"); > > } > > ); > > Hi Aditya, since this is already in a release, we need a new bug number for > the fix. Can you please file one and add a test plan? It might depend on > DBMS version, so that might be why we didn't see that in testing. I remember > we had a similar error. Please also add your DBMS and the version to the > report! Please use bug number 38788 I added this to the manual. Whoever is the last to check it for backport can resolve it. Thanks! Applied to 22.11 for 22.11.28 |