Bug 9231 - Batch revert staged MARC records fails if one or more records in a batch have been deleted.
Summary: Batch revert staged MARC records fails if one or more records in a batch have...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: unspecified
Hardware: All All
: P3 normal (vote)
Assignee: Kyle M Hall
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-06 16:20 UTC by Kyle M Hall
Modified: 2015-06-04 23:23 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9231 - Batch revert staged MARC records fails if one or more records in a batch have been deleted. (1.02 KB, patch)
2012-12-18 12:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9231 - Batch revert staged MARC records fails if one or more records in a batch have been deleted. (1.71 KB, patch)
2013-05-02 18:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9231 - Batch revert staged MARC records fails if one or more records in a batch have been deleted. (3.03 KB, patch)
2013-07-02 15:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9231 - Batch revert staged MARC records fails if one or more records in a batch have been deleted. (3.05 KB, patch)
2013-07-02 15:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9231 - Batch revert staged MARC records fails if one or more records in a batch have been deleted. (3.04 KB, patch)
2013-07-02 15:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9231 - Batch revert staged MARC records fails if one or more records in a batch have been deleted. (3.04 KB, patch)
2013-08-09 12:34 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2012-12-06 16:20:18 UTC

    
Comment 1 Kyle M Hall 2012-12-06 16:22:55 UTC
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.
Comment 2 Kyle M Hall 2012-12-18 12:47:35 UTC Comment hidden (obsolete)
Comment 3 Owen Leonard 2012-12-27 16:13:47 UTC
This patch should have a commit message which includes a description of the problem, a description of the solution, and a test plan.
Comment 4 Kyle M Hall 2013-05-02 18:34:42 UTC Comment hidden (obsolete)
Comment 5 Chris Cormack 2013-05-03 08:43:53 UTC
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
}
Comment 6 Kyle M Hall 2013-07-02 15:18:56 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2013-07-02 15:21:20 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2013-07-02 15:25:29 UTC
(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?
Comment 9 Kyle M Hall 2013-07-02 15:29:13 UTC Comment hidden (obsolete)
Comment 10 Magnus Enger 2013-08-09 10:50:20 UTC
(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?
Comment 11 Kyle M Hall 2013-08-09 12:34:43 UTC
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 12 Galen Charlton 2013-08-09 21:36:45 UTC
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.
Comment 13 Kyle M Hall 2014-06-03 10:34:36 UTC
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