[Bug 9231] New: Batch revert staged MARC records fails if one or more records in a batch have been deleted.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9231 Bug ID: 9231 Summary: Batch revert staged MARC records fails if one or more records in a batch have been deleted. Classification: Unclassified Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Cataloging Assignee: gmcharlt@gmail.com Reporter: kyle@bywatersolutions.com 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=9231 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Priority|P5 - low |P3 Severity|enhancement |normal --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- It appears that if a record in an imported batch is deleted, any attempt to revert the batch will result in the progress bar just sitting at '0%' forever. The background job fails when it hits the already deleted record, with the error occurring in C4::Items::DelItem for the line $copy2deleted->execute( $record->as_usmarc(), $itemnumber ); which fails because $record is undefined. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9231 --- Comment #2 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 14186 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=14186&action=edit Bug 9231 - Batch revert staged MARC records fails if one or more records in a batch have been deleted. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9231 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Assignee|gmcharlt@gmail.com |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=9231 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- 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=9231 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #3 from Owen Leonard <oleonard@myacpl.org> --- This patch should have a commit message which includes a description of the problem, a description of the solution, and a test plan. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9231 Kyle M Hall <kyle@bywatersolutions.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=9231 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #14186|0 |1 is obsolete| | --- Comment #4 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 17910 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=17910&action=edit Bug 9231 - Batch revert staged MARC records fails if one or more records in a batch have been deleted. If a record in an imported batch is dleted, any attempt to revert the batch will result in the progress bar never reaching 100%. The background job dies when it hits the already deleted record, with the error occurring in C4::Items::DelItem for the line: $copy2deleted->execute( $record->as_usmarc(), $itemnumber ); which fails because $record is undefined. To keep the failure from causing the script to die, we just need to eval it. Then, even if it fails, the batch revert will keep going. Test Plan: 1) Stage a batch of marc records 2) Import those records 3) Delete one of those records 4) Attempt to revert the batch, it will hang. 5) Apply this patch 6) Repeat steps 1-4, the revert should succeed. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9231 --- Comment #5 from Chris Cormack <chris@bigballofwax.co.nz> --- Comment on attachment 17910 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=17910 Bug 9231 - Batch revert staged MARC records fails if one or more records in a batch have been deleted. Review of attachment 17910: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=9231&attachment=17910) ----------------------------------------------------------------- ::: C4/Items.pm @@ +619,4 @@
# backup the record my $copy2deleted = $dbh->prepare("UPDATE deleteditems SET marc=? WHERE itemnumber=?"); + eval { $copy2deleted->execute( $record->as_usmarc(), $itemnumber ); }; warn $@ if $@;
If the error is caused by $record being undefined, wouldn't it be better to do if ($record){ my $copy2deleted = $dbh->prepare("UPDATE deleteditems SET marc=? WHERE itemnumber=?"); $copy2deleted->execute( $record->as_usmarc(), $itemnumber ); } To save creating a handle and hitting the db when we don't need to, could even do an else { log deleted one here to tell the user } -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9231 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |In Discussion CC| |chris@bigballofwax.co.nz -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9231 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9231 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #17910|0 |1 is obsolete| | --- Comment #6 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 19348 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19348&action=edit Bug 9231 - Batch revert staged MARC records fails if one or more records in a batch have been deleted. If a record in an imported batch is dleted, any attempt to revert the batch will result in the progress bar never reaching 100%. The background job dies when it hits the already deleted record, with the error occurring in C4::Items::DelItem for the line: $copy2deleted->execute( $record->as_usmarc(), $itemnumber ); which fails because $record is undefined. To keep the failure from causing the script to die, we just need to eval it. Then, even if it fails, the batch revert will keep going. Test Plan: 1) Stage a batch of marc records 2) Import those records 3) Delete one of those records 4) Attempt to revert the batch, it will hang. 5) Apply this patch 6) Repeat steps 1-4, the revert should succeed. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9231 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19348|0 |1 is obsolete| | --- Comment #7 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 19349 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19349&action=edit Bug 9231 - Batch revert staged MARC records fails if one or more records in a batch have been deleted. If a record in an imported batch is dleted, any attempt to revert the batch will result in the progress bar never reaching 100%. The background job dies when it hits the already deleted record, with the error occurring in C4::Items::DelItem for the line: $copy2deleted->execute( $record->as_usmarc(), $itemnumber ); which fails because $record is undefined. To keep the failure from causing the script to die, we just need to eval it. Then, even if it fails, the batch revert will keep going. Test Plan: 1) Stage a batch of marc records 2) Import those records 3) Delete one of those records 4) Attempt to revert the batch, it will hang. 5) Apply this patch 6) Repeat steps 1-4, the revert should succeed. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9231 --- Comment #8 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Chris Cormack from comment #5) Good idea! I've updated the patch to do so. It still seems odd to delete the item, and *then* look up the record, but I get the feeling this order is required, and it seems inefficient to call GetMarcBiblio both before and after calling _koha_delete_item, or to call something like GetBiblio. Any opinions? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9231 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19349|0 |1 is obsolete| | --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 19350 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19350&action=edit Bug 9231 - Batch revert staged MARC records fails if one or more records in a batch have been deleted. If a record in an imported batch is dleted, any attempt to revert the batch will result in the progress bar never reaching 100%. The background job dies when it hits the already deleted record, with the error occurring in C4::Items::DelItem for the line: $copy2deleted->execute( $record->as_usmarc(), $itemnumber ); which fails because $record is undefined. To keep the failure from causing the script to die, we just need to eval it. Then, even if it fails, the batch revert will keep going. Test Plan: 1) Stage a batch of marc records 2) Import those records 3) Delete one of those records 4) Attempt to revert the batch, it will hang. 5) Apply this patch 6) Repeat steps 1-4, the revert should succeed. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9231 Magnus Enger <magnus@enger.priv.no> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |magnus@enger.priv.no --- Comment #10 from Magnus Enger <magnus@enger.priv.no> --- (In reply to Kyle M Hall from comment #9)
To keep the failure from causing the script to die, we just need to eval it. Then, even if it fails, the batch revert will keep going.
It looks like earlier versions of this patch used eval, but the latest one does not. Maybe update the commit message, if it isn't too much trouble? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9231 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19350|0 |1 is obsolete| | --- Comment #11 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 20219 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20219&action=edit Bug 9231 - Batch revert staged MARC records fails if one or more records in a batch have been deleted. If a record in an imported batch is deleted, any attempt to revert the batch will result in the progress bar never reaching 100%. The background job dies when it hits the already deleted record, with the error occurring in C4::Items::DelItem for the line: $copy2deleted->execute( $record->as_usmarc(), $itemnumber ); which fails because $record is undefined. To keep the failure from causing the script to die, we just need to check the record to see if it loaded before working on it. Test Plan: 1) Stage a batch of marc records 2) Import those records 3) Delete one of those records 4) Attempt to revert the batch, it will hang. 5) Apply this patch 6) Repeat steps 1-4, the revert should succeed. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9231 --- Comment #12 from Galen Charlton <gmcharlt@gmail.com> --- Comment on attachment 20219 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20219 Bug 9231 - Batch revert staged MARC records fails if one or more records in a batch have been deleted. Review of attachment 20219: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=9231&attachment=20219) ----------------------------------------------------------------- Marking failed QA because of the infinite loop condition that this patch introduces. Also, I will not push any patch for this bug that does not include a regression test in the DB-dependent test suite. ::: C4/Items.pm @@ +611,5 @@
+ + # FIXME: Why pass $dbh in? We can get it from C4::Context + # FIXME: Why pass $biblionumber in? It would be safer to get it via the itemnumber + + return unless ( DelItemCheck( $dbh, $biblionumber, $itemnumber ) eq '1' );
DelItemCheck calls DelItem, so adding a call to DelItemCheck here creates an infinite loop -- which I've verified actually happens. Since DelItemCheck is meant as the entry point (although that is problematic since DelItemCheck currently checks userenv), and DelItem is the internal routine, I think a better implementation would be for DelItemCheck to check the results of its call to GetItem and return if the item doesn't exist. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9231 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA CC| |gmcharlt@gmail.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9231 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |RESOLVED Resolution|--- |FIXED --- Comment #13 from Kyle M Hall <kyle@bywatersolutions.com> --- I believe this has been fixed incidentally with the removal of the code block: # backup the record my $copy2deleted = $dbh->prepare("UPDATE deleteditems SET marc=? WHERE itemnumber=?"); eval { $copy2deleted->execute( $record->as_usmarc(), $itemnumber ); }; warn $@ if $@; # This last update statement makes that the timestamp column in deleteditems is updated too. If you remove these lines, please add a line to update the timestamp separately. See Bugzilla report 7146 and Biblio.pm (DelBiblio). from DelItem -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org