See bug 36013 comment 3
Created attachment 164192 [details] [review] Bug 36473: Handle Invalid Metadata Exceptions in totalissues.pl This patch wraps the call for record in an eval, and catches any invalid metadata exceptions, letting the warning show, but allowing the script to continue To test: 1 - In default KTD record 369 has problems, otherwise you need to break a record 2 - Run : misc/cronjobs/update_totalissues.pl --use-stats --commit=1000 -v 3 - It dies at record 369 (or the one you broke) 4 - Apply patch 5 - Run again 6 - It succeeds, but skips the bad record
Created attachment 164247 [details] [review] Bug 36473: Handle Invalid Metadata Exceptions in totalissues.pl This patch wraps the call for record in an eval, and catches any invalid metadata exceptions, letting the warning show, but allowing the script to continue To test: 1 - In default KTD record 369 has problems, otherwise you need to break a record 2 - Run : misc/cronjobs/update_totalissues.pl --use-stats --commit=1000 -v 3 - It dies at record 369 (or the one you broke) 4 - Apply patch 5 - Run again 6 - It succeeds, but skips the bad record Signed-off-by: David Nind <david@davidnind.com>
Created attachment 164269 [details] [review] Bug 36473: Handle Invalid Metadata Exceptions in totalissues.pl This patch wraps the call for record in an eval, and catches any invalid metadata exceptions, letting the warning show, but allowing the script to continue To test: 1 - In default KTD record 369 has problems, otherwise you need to break a record 2 - Run : misc/cronjobs/update_totalissues.pl --use-stats --commit=1000 -v 3 - It dies at record 369 (or the one you broke) 4 - Apply patch 5 - Run again 6 - It succeeds, but skips the bad record
Created attachment 164270 [details] [review] Bug 36473: Unit tests
Created attachment 164271 [details] [review] Bug 36473: Handle Invalid Metadata Exceptions in totalissues.pl This patch wraps the call for record in an eval, and catches any invalid metadata exceptions, letting the warning show, but allowing the script to continue To test: 1 - In default KTD record 369 has problems, otherwise you need to break a record 2 - Run : misc/cronjobs/update_totalissues.pl --use-stats --commit=1000 -v 3 - It dies at record 369 (or the one you broke) 4 - Apply patch 5 - Run again 6 - It succeeds, but skips the bad record
Created attachment 164289 [details] [review] Bug 36473: Unit tests
Created attachment 164290 [details] [review] Bug 36473: Handle Invalid Metadata Exceptions in totalissues.pl This patch wraps the call for record in an eval, and catches any invalid metadata exceptions, letting the warning show, but allowing the script to continue To test: 1 - In default KTD record 369 has problems, otherwise you need to break a record 2 - Run : misc/cronjobs/update_totalissues.pl --use-stats --commit=1000 -v 3 - It dies at record 369 (or the one you broke) 4 - Apply patch 5 - Run again 6 - It succeeds, but skips the bad record Signed-off-by: David Nind <david@davidnind.com>
if ( C4::Context->preference('UpdateTotalIssuesOnCirc') ) { UpdateTotalIssues( $item_object->biblionumber, 1, undef, { skip_holds_queue => 1 } ); } IIUC AddIssue will no longer crash on an invalid metadata record, right? Is that really what we want?
Created attachment 164458 [details] [review] Bug 36473: Unit tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Tidied.
Created attachment 164459 [details] [review] Bug 36473: Handle Invalid Metadata Exceptions in totalissues.pl This patch wraps the call for record in an eval, and catches any invalid metadata exceptions, letting the warning show, but allowing the script to continue To test: 1 - In default KTD record 369 has problems, otherwise you need to break a record 2 - Run : misc/cronjobs/update_totalissues.pl --use-stats --commit=1000 -v 3 - It dies at record 369 (or the one you broke) 4 - Apply patch 5 - Run again 6 - It succeeds, but skips the bad record Signed-off-by: David Nind <david@davidnind.com>
Changing status for feedback
(In reply to Marcel de Rooy from comment #8) > if ( C4::Context->preference('UpdateTotalIssuesOnCirc') ) { > UpdateTotalIssues( $item_object->biblionumber, 1, undef, { > skip_holds_queue => 1 } ); > } > > IIUC AddIssue will no longer crash on an invalid metadata record, right? Is > that really what we want? This was an unintended consequence, however, I do think this is what we want - there will be a warning in the logs - but why should we prevent checking out an item with a bad record? We should be handling that on creation or editing of a record.
Created attachment 164805 [details] [review] Bug 36473: Unit tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Tidied.
Created attachment 164806 [details] [review] Bug 36473: Handle Invalid Metadata Exceptions in totalissues.pl This patch wraps the call for record in an eval, and catches any invalid metadata exceptions, letting the warning show, but allowing the script to continue To test: 1 - In default KTD record 369 has problems, otherwise you need to break a record 2 - Run : misc/cronjobs/update_totalissues.pl --use-stats --commit=1000 -v 3 - It dies at record 369 (or the one you broke) 4 - Apply patch 5 - Run again 6 - It succeeds, but skips the bad record Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Pushed for 24.05! Well done everyone, thank you!
It looks like this might make the tests fail (needs further investigation): t_db_dependent_Biblio_t.UpdateTotalIssues on Invalid record (2144) Error Details not ok 21 - UpdateTotalIssues on Invalid record
I have a hunch that this broke tests: prove t/db_dependent/Biblio.t # Failed test 'Expected warning found' # at t/db_dependent/Biblio.t line 1039. # found warning: UpdateTotalIssues could not get bibliographic record at /kohadevbox/koha/C4/Biblio.pm line 3065. # expected to find warning: UpdateTotalIssues could not get biblio record # Looks like you failed 1 test of 2. # Failed test 'UpdateTotalIssues on Invalid record' # at t/db_dependent/Biblio.t line 1043. # Looks like you failed 1 test of 21. t/db_dependent/Biblio.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/21 subtests
I don't understand this fix. Why don't you eval/try-catch in the script instead of inside UpdateTotalIssues? Also the warn should display the error, otherwise you are potentially hidden useful warnings. Please fix ASAP, Jenkins is failing!
Created attachment 165039 [details] [review] Bug 36473: [ALTERNATIVE PATCH] Prevent UpdateTotalIssues failure to crash update_totalissues.pl
This is how I would have fixed it initially.
Created attachment 165077 [details] [review] Bug 36473: (follow-up) Correct biblio to bibliographic
Created attachment 165078 [details] [review] Bug 36473: (follow-up) Add biblionumber to warning
Created attachment 165079 [details] [review] Bug 36473: (follow-up) Additionally warn the stringified exception
All 3 follow-ups have been pushed to main: https://git.koha-community.org/Koha-community/Koha/commits/branch/main/search?q=36473
Pushed to 23.11.x for 23.11.06
Backported to 23.05.x for upcoming 23.05.12