Bugzilla – Attachment 94421 Details for
Bug 20086
AddRenewal is not executed as a transaction and can results in partial success and doubled fines
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 20086: Execute AddRenewal in a transaction to avoid partial success and doubled fines
Bug-20086-Execute-AddRenewal-in-a-transaction-to-a.patch (text/plain), 10.72 KB, created by
Jonathan Druart
on 2019-10-21 07:08:00 UTC
(
hide
)
Description:
Bug 20086: Execute AddRenewal in a transaction to avoid partial success and doubled fines
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2019-10-21 07:08:00 UTC
Size:
10.72 KB
patch
obsolete
>From 1403f1e05f1bbeb2d5f393f10230a052afe2b483 Mon Sep 17 00:00:00 2001 >From: Nick <nick@bywatersolutions.com> >Date: Thu, 17 Oct 2019 14:31:27 +0000 >Subject: [PATCH] Bug 20086: Execute AddRenewal in a transaction to avoid > 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. (Edit JD: not true, they are included) > >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 > >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> > >Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Amended: commit title and one indendation (return statement) >--- > C4/Circulation.pm | 189 ++++++++++++++++++++++++++++-------------------------- > 1 file changed, 97 insertions(+), 92 deletions(-) > >diff --git a/C4/Circulation.pm b/C4/Circulation.pm >index 014c018628..03b623b331 100644 >--- a/C4/Circulation.pm >+++ b/C4/Circulation.pm >@@ -2864,113 +2864,118 @@ sub AddRenewal { > > my $circ_library = Koha::Libraries->find( _GetCircControlBranch($item_unblessed, $patron_unblessed) ); > >- if ( C4::Context->preference('CalculateFinesOnReturn') && $issue->is_overdue ) { >- _CalculateAndUpdateFine( { issue => $issue, item => $item_unblessed, borrower => $patron_unblessed } ); >- } >- _FixOverduesOnReturn( $borrowernumber, $itemnumber, undef, 'RENEWED' ); >- >- # 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 >- # based on the value of the RenewalPeriodBase syspref. >- my $itemtype = $item_object->effective_itemtype; >- unless ($datedue) { >- >- $datedue = (C4::Context->preference('RenewalPeriodBase') eq 'date_due') ? >- dt_from_string( $issue->date_due, 'sql' ) : >- DateTime->now( time_zone => C4::Context->tz()); >- $datedue = CalcDateDue($datedue, $itemtype, $circ_library->branchcode, $patron_unblessed, 'is a renewal'); >- } >+ my $schema = Koha::Database->new->schema; >+ $schema->txn_do(sub{ > >- my $fees = Koha::Charges::Fees->new( >- { >- patron => $patron, >- library => $circ_library, >- item => $item_object, >- from_date => dt_from_string( $issue->date_due, 'sql' ), >- to_date => dt_from_string($datedue), >+ if ( C4::Context->preference('CalculateFinesOnReturn') && $issue->is_overdue ) { >+ _CalculateAndUpdateFine( { issue => $issue, item => $item_unblessed, borrower => $patron_unblessed } ); >+ } >+ _FixOverduesOnReturn( $borrowernumber, $itemnumber, undef, 'RENEWED' ); >+ >+ # 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 >+ # based on the value of the RenewalPeriodBase syspref. >+ my $itemtype = $item_object->effective_itemtype; >+ unless ($datedue) { >+ >+ $datedue = (C4::Context->preference('RenewalPeriodBase') eq 'date_due') ? >+ dt_from_string( $issue->date_due, 'sql' ) : >+ DateTime->now( time_zone => C4::Context->tz()); >+ $datedue = CalcDateDue($datedue, $itemtype, $circ_library->branchcode, $patron_unblessed, 'is a renewal'); > } >- ); > >- # 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=?" >- ); >+ my $fees = Koha::Charges::Fees->new( >+ { >+ patron => $patron, >+ library => $circ_library, >+ item => $item_object, >+ from_date => dt_from_string( $issue->date_due, 'sql' ), >+ to_date => dt_from_string($datedue), >+ } >+ ); > >- $sth->execute( $datedue->strftime('%Y-%m-%d %H:%M'), $renews, $lastreneweddate, $borrowernumber, $itemnumber ); >+ # 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=?" >+ ); > >- # Update the renewal count on the item, and tell zebra to reindex >- $renews = $item_object->renewals + 1; >- ModItem( { renewals => $renews, onloan => $datedue->strftime('%Y-%m-%d %H:%M')}, $item_object->biblionumber, $itemnumber, { log_action => 0 } ); >+ $sth->execute( $datedue->strftime('%Y-%m-%d %H:%M'), $renews, $lastreneweddate, $borrowernumber, $itemnumber ); > >- # Charge a new rental fee, if applicable >- my ( $charge, $type ) = GetIssuingCharges( $itemnumber, $borrowernumber ); >- if ( $charge > 0 ) { >- AddIssuingCharge($issue, $charge, 'rent_renew'); >- } >+ # Update the renewal count on the item, and tell zebra to reindex >+ $renews = $item_object->renewals + 1; >+ ModItem( { renewals => $renews, onloan => $datedue->strftime('%Y-%m-%d %H:%M')}, $item_object->biblionumber, $itemnumber, { log_action => 0 } ); > >- # Charge a new accumulate rental fee, if applicable >- my $itemtype_object = Koha::ItemTypes->find( $itemtype ); >- if ( $itemtype_object ) { >- my $accumulate_charge = $fees->accumulate_rentalcharge(); >- if ( $accumulate_charge > 0 ) { >- AddIssuingCharge( $issue, $accumulate_charge, 'rent_daily_renew' ) >+ # Charge a new rental fee, if applicable >+ my ( $charge, $type ) = GetIssuingCharges( $itemnumber, $borrowernumber ); >+ if ( $charge > 0 ) { >+ AddIssuingCharge($issue, $charge, 'rent_renew'); > } >- $charge += $accumulate_charge; >- } > >- # Send a renewal slip according to checkout alert preferencei >- if ( C4::Context->preference('RenewalSendNotice') eq '1' ) { >- my $circulation_alert = 'C4::ItemCirculationAlertPreference'; >- my %conditions = ( >- branchcode => $branch, >- categorycode => $patron->categorycode, >- item_type => $itemtype, >- notification => 'CHECKOUT', >- ); >- if ( $circulation_alert->is_enabled_for( \%conditions ) ) { >- SendCirculationAlert( >- { >- type => 'RENEWAL', >- item => $item_unblessed, >- borrower => $patron->unblessed, >- branch => $branch, >- } >- ); >+ # Charge a new accumulate rental fee, if applicable >+ my $itemtype_object = Koha::ItemTypes->find( $itemtype ); >+ if ( $itemtype_object ) { >+ my $accumulate_charge = $fees->accumulate_rentalcharge(); >+ if ( $accumulate_charge > 0 ) { >+ AddIssuingCharge( $issue, $accumulate_charge, 'rent_daily_renew' ) >+ } >+ $charge += $accumulate_charge; > } >- } > >- # Remove any OVERDUES related debarment if the borrower has no overdues >- if ( $patron >- && $patron->is_debarred >- && ! $patron->has_overdues >- && @{ GetDebarments({ borrowernumber => $borrowernumber, type => 'OVERDUES' }) } >- ) { >- DelUniqueDebarment({ borrowernumber => $borrowernumber, type => 'OVERDUES' }); >- } >+ # Send a renewal slip according to checkout alert preferencei >+ if ( C4::Context->preference('RenewalSendNotice') eq '1' ) { >+ my $circulation_alert = 'C4::ItemCirculationAlertPreference'; >+ my %conditions = ( >+ branchcode => $branch, >+ categorycode => $patron->categorycode, >+ item_type => $itemtype, >+ notification => 'CHECKOUT', >+ ); >+ if ( $circulation_alert->is_enabled_for( \%conditions ) ) { >+ SendCirculationAlert( >+ { >+ type => 'RENEWAL', >+ item => $item_unblessed, >+ borrower => $patron->unblessed, >+ branch => $branch, >+ } >+ ); >+ } >+ } > >- unless ( C4::Context->interface eq 'opac' ) { #if from opac we are obeying OpacRenewalBranch as calculated in opac-renew.pl >- $branch = C4::Context->userenv ? C4::Context->userenv->{branch} : $branch; >- } >+ # Remove any OVERDUES related debarment if the borrower has no overdues >+ if ( $patron >+ && $patron->is_debarred >+ && ! $patron->has_overdues >+ && @{ GetDebarments({ borrowernumber => $borrowernumber, type => 'OVERDUES' }) } >+ ) { >+ DelUniqueDebarment({ borrowernumber => $borrowernumber, type => 'OVERDUES' }); >+ } > >- # Add the renewal to stats >- UpdateStats( >- { >- branch => $branch, >- type => 'renew', >- amount => $charge, >- itemnumber => $itemnumber, >- itemtype => $itemtype, >- location => $item_object->location, >- borrowernumber => $borrowernumber, >- ccode => $item_object->ccode, >+ unless ( C4::Context->interface eq 'opac' ) { #if from opac we are obeying OpacRenewalBranch as calculated in opac-renew.pl >+ $branch = C4::Context->userenv ? C4::Context->userenv->{branch} : $branch; > } >- ); > >- #Log the renewal >- logaction("CIRCULATION", "RENEWAL", $borrowernumber, $itemnumber) if C4::Context->preference("RenewalLog"); >+ # Add the renewal to stats >+ UpdateStats( >+ { >+ branch => $branch, >+ type => 'renew', >+ amount => $charge, >+ itemnumber => $itemnumber, >+ itemtype => $itemtype, >+ location => $item_object->location, >+ borrowernumber => $borrowernumber, >+ ccode => $item_object->ccode, >+ } >+ ); >+ >+ #Log the renewal >+ logaction("CIRCULATION", "RENEWAL", $borrowernumber, $itemnumber) if C4::Context->preference("RenewalLog"); >+ }); >+ > return $datedue; > } > >-- >2.11.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 20086
:
70879
|
94374
|
94377
|
94378
| 94421 |
94422