From c25f2e1c32f412bb89b6e35f6bbf00ba3a7d9503 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Wed, 24 Jan 2018 14:20:34 +0000 Subject: [PATCH] Bug 20086 - AddRenewal is not executed as a transaction and can results in partial success and doubled fines This patch starts a transaction and only commits if renewal and fines updates and charges are successful (partial in any cna be problematic) There is no feedback (as currently there is none either) but if part fails, all fails. I didn't include stats and notifications in the transaction, but we could. To test: 1 - Apply patch 2 - prove t/db_dependent/Circulation.t 3 - Attempt circs and renewals should be no difference 4 - If possible make part of transaction fail and ensure all fails --- C4/Circulation.pm | 77 +++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 49 insertions(+), 28 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 2b7722c..cbcb6bf 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2808,6 +2808,7 @@ sub AddRenewal { my $item_object = Koha::Items->find( $itemnumber ); # Should replace $item my $biblio = $item_object->biblio; + my $schema = Koha::Database->new->schema; my $dbh = C4::Context->dbh; # Find the issues record for this book @@ -2825,10 +2826,22 @@ sub AddRenewal { my $patron = Koha::Patrons->find( $borrowernumber ) or return; # FIXME Should do more than just return my $patron_unblessed = $patron->unblessed; + $schema->storage->txn_begin; + if ( C4::Context->preference('CalculateFinesOnReturn') && $issue->is_overdue ) { - _CalculateAndUpdateFine( { issue => $issue, item => $item, borrower => $patron_unblessed } ); + eval{ + _CalculateAndUpdateFine( { issue => $issue, item => $item, borrower => $patron_unblessed } ); + }; + if ( $@ ) { + $schema->txn_rollback; + return; + } + } + eval{ _FixOverduesOnReturn( $borrowernumber, $itemnumber ); }; + if ( $@ ) { + $schema->txn_rollback; + return; } - _FixOverduesOnReturn( $borrowernumber, $itemnumber ); # If the due date wasn't specified, calculate it by adding the # book's loan length to today's date or the current due date @@ -2844,34 +2857,42 @@ sub AddRenewal { # Update the issues record to have the new due date, and a new count # of how many times it has been renewed. - my $renews = $issue->renewals + 1; - my $sth = $dbh->prepare("UPDATE issues SET date_due = ?, renewals = ?, lastreneweddate = ? - WHERE borrowernumber=? - AND itemnumber=?" - ); - - $sth->execute( $datedue->strftime('%Y-%m-%d %H:%M'), $renews, $lastreneweddate, $borrowernumber, $itemnumber ); - - # Update the renewal count on the item, and tell zebra to reindex - $renews = $item->{renewals} + 1; - ModItem({ renewals => $renews, onloan => $datedue->strftime('%Y-%m-%d %H:%M')}, $item->{biblionumber}, $itemnumber); - - # Charge a new rental fee, if applicable? - my ( $charge, $type ) = GetIssuingCharges( $itemnumber, $borrowernumber ); - if ( $charge > 0 ) { - my $accountno = getnextacctno( $borrowernumber ); - my $manager_id = 0; - $manager_id = C4::Context->userenv->{'number'} if C4::Context->userenv; - $sth = $dbh->prepare( - "INSERT INTO accountlines - (date, borrowernumber, accountno, amount, manager_id, - description,accounttype, amountoutstanding, itemnumber) - VALUES (now(),?,?,?,?,?,?,?,?)" + my ( $charge, $type ); + eval { + my $renews = $issue->renewals + 1; + my $sth = $dbh->prepare("UPDATE issues SET date_due = ?, renewals = ?, lastreneweddate = ? + WHERE borrowernumber=? + AND itemnumber=?" ); - $sth->execute( $borrowernumber, $accountno, $charge, $manager_id, - "Renewal of Rental Item " . $biblio->title . " $item->{'barcode'}", - 'Rent', $charge, $itemnumber ); + + $sth->execute( $datedue->strftime('%Y-%m-%d %H:%M'), $renews, $lastreneweddate, $borrowernumber, $itemnumber ); + + # Update the renewal count on the item, and tell zebra to reindex + $renews = $item->{renewals} + 1; + ModItem({ renewals => $renews, onloan => $datedue->strftime('%Y-%m-%d %H:%M')}, $item->{biblionumber}, $itemnumber); + + # Charge a new rental fee, if applicable? + ( $charge, $type ) = GetIssuingCharges( $itemnumber, $borrowernumber ); + if ( $charge > 0 ) { + my $accountno = getnextacctno( $borrowernumber ); + my $manager_id = 0; + $manager_id = C4::Context->userenv->{'number'} if C4::Context->userenv; + $sth = $dbh->prepare( + "INSERT INTO accountlines + (date, borrowernumber, accountno, amount, manager_id, + description,accounttype, amountoutstanding, itemnumber) + VALUES (now(),?,?,?,?,?,?,?,?)" + ); + $sth->execute( $borrowernumber, $accountno, $charge, $manager_id, + "Renewal of Rental Item " . $biblio->title . " $item->{'barcode'}", + 'Rent', $charge, $itemnumber ); + } + }; + if ( $@ ) { + $schema->txn_rollback; + return; } + $schema->txn_commit; # Send a renewal slip according to checkout alert preferencei if ( C4::Context->preference('RenewalSendNotice') eq '1' ) { -- 2.1.4