[Bug 12995] New: script update_totalissues.pl stops on corrupted record
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 Bug ID: 12995 Summary: script update_totalissues.pl stops on corrupted record Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: minor Priority: P5 - low Component: Command-line Utilities Assignee: gmcharlt@gmail.com Reporter: fridolyn.somers@biblibre.com QA Contact: testopia@bugs.koha-community.org When running update_totalissues.pl cronjob, it will stop on a corrupted record. It should alert and process next record. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|gmcharlt@gmail.com |fridolyn.somers@biblibre.co | |m -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Patch complexity|--- |Trivial patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 --- Comment #1 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- Created attachment 31875 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31875&action=edit Bug 12995 - script update_totalissues.pl stops on corrupted record When running update_totalissues.pl cronjob, it will stop on a corrupted record. This patch adds eval around record modification method call, alerts if error and process next record. Test plan : - Create a dabase with a few biblios and some issues - Modify first biblio record (use direct sql update) : set a wrong value in biblioitems.marcxml - Lauch script : misc/cronjobs/update_totalissues.pl --use-stats --commit=1000 -v => Without patch : the script stops at first record => With patch : the script prints error for first record and processes all records -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 Cindy Murdock Ames <cmurdock@ccfls.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |cmurdock@ccfls.org --- Comment #2 from Cindy Murdock Ames <cmurdock@ccfls.org> --- Created attachment 32454 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32454&action=edit Signed off patch Successfully applied patch & passed test. Also used it on my actual database in a dev test environment, and it worked. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 Cindy Murdock Ames <cmurdock@ccfls.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=12995 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #31875|0 |1 is obsolete| | CC| |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=12995 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #3 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- QA Comment: Fridolin, although our coding guidelines do not say anything about the use of eval, I do not think that this is the most elegant solution. It also will degrade performance btw. The actual problem is somewhere else and I would prefer to solve it there. In this case the corrupted record will be discovered by GetMarcBiblio (it also contains an eval already btw!). Note that GetMarcBiblio returns undef now, but this return value is NOT checked by UpdateTotalIssues. So here we come closer.. UpdateTotalIssues should test that value and not just say $record->field.. (There will be [much] more instances where this happens.] So, we could do at least two things now: [1] Return from UpdateTotalIssues: return if !$record; the cronjob continues. (Note that there is a warn in GetMarcBiblio.) [2] Also return a value to the caller (note that only the cronjob and Circulation.pm call this routine). Print the number of skipped records in the cronjob. Setting status to Failed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 --- Comment #4 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- (In reply to M. de Rooy from comment #3) Thanks for your advice, I agree 100%. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32454|0 |1 is obsolete| | --- Comment #5 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- Created attachment 32534 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32534&action=edit Bug 12995 - script update_totalissues.pl stops on corrupted record This patch adds an unit test on C4::Biblio::UpdateTotalIssues method call on non existing record -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32534|0 |1 is obsolete| | --- Comment #6 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- Created attachment 32535 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32535&action=edit Bug 12995 - script update_totalissues.pl stops on corrupted record - UT This patch adds an unit test on C4::Biblio::UpdateTotalIssues method call on non existing record -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 --- Comment #7 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- Created attachment 32536 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32536&action=edit Bug 12995 - script update_totalissues.pl stops on corrupted record When running update_totalissues.pl cronjob, it will stop on a corrupted record. This patch changes UpdateTotalIssues so that it return 1 if processing record has succeded. Also, if mapping with biblioitems.totalissues does not exist, the method has nothing to do so it stops and returns 1. When processing a corrupted record, script now alerts about the error on this biblionumber (if script is verbose) and process next record. A total number of records with error will be printed at the end of the script. Test plan : - Create a dabase with a few biblios and some issues - Modify first biblio record (use direct sql update) : set empty value in biblioitems.marcxml - Launch script : misc/cronjobs/update_totalissues.pl --use-stats --commit=1000 -v => Without patch : the script stops at first record => With patch : the script prints error for first record and processes all records -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 --- Comment #8 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- Here we are, new patches. I've added a simple unit test on C4::Biblio::UpdateTotalIssues. One may create more complex unit tests to cover all cases. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 Owen Leonard <oleonard@myacpl.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=12995 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32536|0 |1 is obsolete| | --- Comment #9 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 32629 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32629&action=edit [SIGNED-OFF] Bug 12995 - script update_totalissues.pl stops on corrupted record When running update_totalissues.pl cronjob, it will stop on a corrupted record. This patch changes UpdateTotalIssues so that it return 1 if processing record has succeded. Also, if mapping with biblioitems.totalissues does not exist, the method has nothing to do so it stops and returns 1. When processing a corrupted record, script now alerts about the error on this biblionumber (if script is verbose) and process next record. A total number of records with error will be printed at the end of the script. Test plan : - Create a dabase with a few biblios and some issues - Modify first biblio record (use direct sql update) : set empty value in biblioitems.marcxml - Launch script : misc/cronjobs/update_totalissues.pl --use-stats --commit=1000 -v => Without patch : the script stops at first record => With patch : the script prints error for first record and processes all records Signed-off-by: Owen Leonard <oleonard@myacpl.org> I was able to confirm the problem before the patch and successfully follow the test plan. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32535|0 |1 is obsolete| | --- Comment #10 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 32630 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32630&action=edit [SIGNED-OFF] Bug 12995 - script update_totalissues.pl stops on corrupted record - UT This patch adds an unit test on C4::Biblio::UpdateTotalIssues method call on non existing record Signed-off-by: Owen Leonard <oleonard@myacpl.org> I can't comment on the correctness of the test other than to say it ran successfully. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 Katrin Fischer <katrin.fischer@bsz-bw.de> 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=12995 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32629|0 |1 is obsolete| | Attachment #32630|0 |1 is obsolete| | --- Comment #11 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 32654 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32654&action=edit [PASSED QA] Bug 12995 - script update_totalissues.pl stops on corrupted record - UT This patch adds an unit test on C4::Biblio::UpdateTotalIssues method call on non existing record Signed-off-by: Owen Leonard <oleonard@myacpl.org> I can't comment on the correctness of the test other than to say it ran successfully. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Improves CLI script, works as described. Note: A small improvement would be to output the problematic biblionumber. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 --- Comment #12 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- I have the impression that the improved patch became obsoleted and do not know if it was qa-ed ? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32654|0 |1 is obsolete| | --- Comment #13 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 32695 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32695&action=edit [PASSED QA] Bug 12995 - script update_totalissues.pl stops on corrupted record When running update_totalissues.pl cronjob, it will stop on a corrupted record. This patch changes UpdateTotalIssues so that it return 1 if processing record has succeded. Also, if mapping with biblioitems.totalissues does not exist, the method has nothing to do so it stops and returns 1. When processing a corrupted record, script now alerts about the error on this biblionumber (if script is verbose) and process next record. A total number of records with error will be printed at the end of the script. Test plan : - Create a dabase with a few biblios and some issues - Modify first biblio record (use direct sql update) : set empty value in biblioitems.marcxml - Launch script : misc/cronjobs/update_totalissues.pl --use-stats --commit=1000 -v => Without patch : the script stops at first record => With patch : the script prints error for first record and processes all records Signed-off-by: Owen Leonard <oleonard@myacpl.org> I was able to confirm the problem before the patch and successfully follow the test plan. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 --- Comment #14 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 32696 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32696&action=edit [PASSED QA] Bug 12995 - script update_totalissues.pl stops on corrupted record - UT This patch adds an unit test on C4::Biblio::UpdateTotalIssues method call on non existing record Signed-off-by: Owen Leonard <oleonard@myacpl.org> I can't comment on the correctness of the test other than to say it ran successfully. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Improves CLI script, works as described. Note: A small improvement would be to output the problematic biblionumber. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #15 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Sorry, my fault - missed to attach the first patch. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |tomascohen@gmail.com --- Comment #16 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Patches pushed to master. Thanks Fridolin! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |robin@catalyst.net.nz --- Comment #17 from Robin Sheat <robin@catalyst.net.nz> --- (In reply to Katrin Fischer from comment #14)
Created attachment 32696 [details] [review] [PASSED QA] Bug 12995 - script update_totalissues.pl stops on corrupted record - UT
This patch adds an unit test on C4::Biblio::UpdateTotalIssues method call on non existing record
Signed-off-by: Owen Leonard <oleonard@myacpl.org> I can't comment on the correctness of the test other than to say it ran successfully.
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Improves CLI script, works as described.
Note: A small improvement would be to output the problematic biblionumber.
This patch causes the package builds to fail, as it requires a database. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtompset@hotmail.com See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=13230 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 --- Comment #18 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- (In reply to Robin Sheat from comment #17)
This patch causes the package builds to fail, as it requires a database. Oh, carp. How can we fix the UT ? mock the db ?
-- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 --- Comment #19 from M. Tompsett <mtompset@hotmail.com> --- (In reply to Fridolin SOMERS from comment #18)
(In reply to Robin Sheat from comment #17)
This patch causes the package builds to fail, as it requires a database. Oh, carp. How can we fix the UT ? mock the db ?
See bug 13230. There's a patch for this. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |13230 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12995 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=13976 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org