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.
Created attachment 14186 [details] [review] Bug 9231 - Batch revert staged MARC records fails if one or more records in a batch have been deleted.
This patch should have a commit message which includes a description of the problem, a description of the solution, and a test plan.
Created attachment 17910 [details] [review] 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.
Comment on attachment 17910 [details] [review] Bug 9231 - Batch revert staged MARC records fails if one or more records in a batch have been deleted. Review of attachment 17910 [details] [review]: ----------------------------------------------------------------- ::: 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 }
Created attachment 19348 [details] [review] 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.
Created attachment 19349 [details] [review] 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.
(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?
Created attachment 19350 [details] [review] 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.
(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?
Created attachment 20219 [details] [review] 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.
Comment on attachment 20219 [details] [review] Bug 9231 - Batch revert staged MARC records fails if one or more records in a batch have been deleted. Review of attachment 20219 [details] [review]: ----------------------------------------------------------------- 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.
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