@@ -, +, @@ - it would be nice to get feedback on what was successfully renewed as well - In general I think I would prefer to see 'ok' and 'not_ok' returned as - There is no listing of errors if I use the 'pay' button on an - I don't really like that the functions are internal functions and then - I think the pref description should highlight that if 'RenewalPeriodBase' - Ideally SIP would get feedback in a screen message - I am also not sure about the code path if a patron paid fines on the --- C4/SIP/ILS.pm | 8 +- C4/SIP/ILS/Transaction/FeePayment.pm | 20 ++- C4/SIP/Sip/MsgType.pm | 42 +++++- Koha/Account.pm | 70 +++------- Koha/Account/Line.pm | 142 ++++++++++++++------- Koha/REST/V1/Patrons/Account.pm | 2 +- ..._23051_add_RenewAccruingItemInOpac_syspref.perl | 8 ++ ...3051_add_RenewAccruingItemWhenPaid_syspref.perl | 2 +- installer/data/mysql/sysprefs.sql | 3 +- .../prog/en/includes/renew_results.inc | 12 ++ .../prog/en/includes/renew_strings.inc | 32 +++++ .../en/modules/admin/preferences/circulation.pref | 9 +- .../prog/en/modules/members/boraccount.tt | 1 + .../intranet-tmpl/prog/en/modules/members/pay.tt | 1 + members/boraccount.pl | 20 +++ members/pay.pl | 19 +++ members/paycollect.pl | 36 +++++- t/db_dependent/Accounts.t | 16 +-- t/db_dependent/Koha/Account.t | 6 +- t/db_dependent/Koha/Account/Lines.t | 81 +++++++++++- 20 files changed, 397 insertions(+), 133 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/bug_23051_add_RenewAccruingItemInOpac_syspref.perl create mode 100644 koha-tmpl/intranet-tmpl/prog/en/includes/renew_results.inc create mode 100644 koha-tmpl/intranet-tmpl/prog/en/includes/renew_strings.inc --- a/C4/SIP/ILS.pm +++ a/C4/SIP/ILS.pm @@ -271,10 +271,14 @@ sub pay_fee { $trans->screen_msg('Invalid patron barcode.'); return $trans; } - my $ok = $trans->pay( $patron->{borrowernumber}, $fee_amt, $pay_type, $fee_id, $is_writeoff, $disallow_overpayment ); + my $trans_result = $trans->pay( $patron->{borrowernumber}, $fee_amt, $pay_type, $fee_id, $is_writeoff, $disallow_overpayment ); + my $ok = $trans_result->{ok}; $trans->ok($ok); - return $trans; + return { + status => $trans, + pay_response => $trans_result->{pay_response} + }; } sub add_hold { --- a/C4/SIP/ILS/Transaction/FeePayment.pm +++ a/C4/SIP/ILS/Transaction/FeePayment.pm @@ -57,13 +57,13 @@ sub pay { my $account = Koha::Account->new( { patron_id => $borrowernumber } ); if ($disallow_overpayment) { - return 0 if $account->balance < $amt; + return { ok => 0 } if $account->balance < $amt; } if ($fee_id) { my $fee = Koha::Account::Lines->find($fee_id); if ( $fee ) { - $account->pay( + my $pay_response = $account->pay( { amount => $amt, type => $type, @@ -72,14 +72,19 @@ sub pay { interface => C4::Context->interface } ); - return 1; + return { + ok => 1, + pay_response => $pay_response + }; } else { - return 0; + return { + ok => 0 + }; } } else { - $account->pay( + my $pay_response = $account->pay( { amount => $amt, type => $type, @@ -87,7 +92,10 @@ sub pay { interface => C4::Context->interface } ); - return 1; + return { + ok => 1, + pay_response => $pay_response + }; } } --- a/C4/SIP/Sip/MsgType.pm +++ a/C4/SIP/Sip/MsgType.pm @@ -20,6 +20,7 @@ use CGI qw ( -utf8 ); use C4::Auth qw(&check_api_auth); use Koha::Patron::Attributes; +use Koha::Items; use UNIVERSAL::can; @@ -1103,7 +1104,46 @@ sub handle_fee_paid { $ils->check_inst_id( $inst_id, "handle_fee_paid" ); - $status = $ils->pay_fee( $patron_id, $patron_pwd, $fee_amt, $fee_type, $pay_type, $fee_id, $trans_id, $currency, $is_writeoff, $disallow_overpayment ); + my $pay_result = $ils->pay_fee( $patron_id, $patron_pwd, $fee_amt, $fee_type, $pay_type, $fee_id, $trans_id, $currency, $is_writeoff, $disallow_overpayment ); + $status = $pay_result->{status}; + my $pay_response = $pay_result->{pay_response}; + + my $failmap = { + "no_item" => "No matching item could be found", + "no_checkout" => "Item is not checked out", + "too_soon" => "Cannot yet be renewed", + "too_many" => "Renewed the maximum number of times", + "auto_too_soon" => "Scheduled for automatic renewal and cannot yet be renewed", + "auto_too_late" => "Scheduled for automatic renewal and cannot yet be any more", + "auto_account_expired" => "Scheduled for automatic renewal and cannot be renewed because the patron's account has expired", + "auto_renew" => "Scheduled for automatic renewal", + "auto_too_much_oweing" => "Scheduled for automatic renewal", + "on_reserve" => "On hold for another patron", + "patron_restricted" => "Patron is currently restricted", + "item_denied_renewal" => "Item is not allowed renewal", + "onsite_checkout" => "Item is an onsite checkout" + }; + my @success = (); + my @fail = (); + foreach my $result( @{$pay_response->{renew_result}} ) { + my $item = Koha::Items->find({ itemnumber => $result->{itemnumber} }); + if ($result->{success}) { + push @success, '"' . $item->biblio->title . '"'; + } else { + push @fail, '"' . $item->biblio->title . '" : ' . $failmap->{$result->{error}}; + } + } + + my $msg = ""; + if (scalar @success > 0) { + $msg.="The following items were renewed: " . join(", ", @success) . ". "; + } + if (scalar @fail > 0) { + $msg.="The following items were not renewed: " . join(", ", @fail) . "."; + } + if (length $msg > 0) { + $status->screen_msg($status->screen_msg . " $msg"); + } $resp .= ( $status->ok ? 'Y' : 'N' ) . timestamp; $resp .= add_field( FID_INST_ID, $inst_id, $server ); --- a/Koha/Account.pm +++ a/Koha/Account.pm @@ -96,10 +96,10 @@ sub pay { && !defined($cash_register) ); my @fines_paid; # List of account lines paid on with this payment - # Item numbers that have had a fine paid where the line has a accounttype - # of OVERDUE and a status of UNRETURNED. We might want to try and renew - # these items. - my $overdue_unreturned = {}; + + # The outcome of any attempted item renewals as a result of fines being + # paid off + my $renew_outcomes = []; my $balance_remaining = $amount; # Set it now so we can adjust the amount if necessary $balance_remaining ||= 0; @@ -118,16 +118,14 @@ sub pay { $fine->amountoutstanding($new_amountoutstanding)->store(); $balance_remaining = $balance_remaining - $amount_to_pay; - # If we need to make a note of the item associated with this line, - # in order that we can potentially renew it, do so. - if ( - $new_amountoutstanding == 0 && - $fine->accounttype && - $fine->accounttype eq 'OVERDUE' && - $fine->status && - $fine->status eq 'UNRETURNED' - ) { - $overdue_unreturned->{$fine->itemnumber} = $fine; + # Attempt to renew the item associated with this debit if + # appropriate + if ($fine->renewable) { + # We're ignoring the definition of $interface above, by all + # accounts we can't rely on C4::Context::interface, so here + # we're only using what we've been explicitly passed + my $outcome = $fine->renew_item($params->{interface}); + push @{$renew_outcomes}, $outcome if $outcome; } # Same logic exists in Koha::Account::Line::apply @@ -192,14 +190,10 @@ sub pay { # If we need to make a note of the item associated with this line, # in order that we can potentially renew it, do so. - if ( - $old_amountoutstanding - $amount_to_pay == 0 && - $fine->accounttype && - $fine->accounttype eq 'OVERDUE' && - $fine->status && - $fine->status eq 'UNRETURNED' - ) { - $overdue_unreturned->{$fine->itemnumber} = $fine; + my $amt = $old_amountoutstanding - $amount_to_pay; + if ($fine->renewable) { + my $outcome = $fine->renew_item; + push @{$renew_outcomes}, $outcome; } if ( $fine->amountoutstanding == 0 @@ -282,36 +276,6 @@ sub pay { } ); - # If we have overdue unreturned items that have had payments made - # against them, check whether the balance on those items is now zero - # and, if the syspref is set, renew them - # Same logic exists in Koha::Account::Line::apply - if ( - C4::Context->preference('RenewAccruingItemWhenPaid') && - keys %{$overdue_unreturned} - ) { - foreach my $itemnumber (keys %{$overdue_unreturned}) { - # Only do something if this item has no fines left on it - my $fine = C4::Overdues::GetFine( $itemnumber, $self->{patron_id} ); - next if $fine && $fine > 0; - - my ( $renew_ok, $error ) = - C4::Circulation::CanBookBeRenewed( - $self->{patron_id}, $itemnumber - ); - if ( $renew_ok ) { - C4::Circulation::AddRenewal( - $self->{patron_id}, - $itemnumber, - $library_id, - undef, - undef, - 1 - ); - } - } - } - if ( C4::Context->preference("FinesLog") ) { logaction( "FINES", 'CREATE', @@ -359,7 +323,7 @@ sub pay { } } - return $payment->id; + return { payment_id => $payment->id, renew_result => $renew_outcomes }; } =head3 add_credit --- a/Koha/Account/Line.pm +++ a/Koha/Account/Line.pm @@ -218,11 +218,6 @@ sub apply { my $schema = Koha::Database->new->schema; - # Item numbers that have had a fine paid where the line has a accounttype - # of OVERDUE and a status of UNRETURNED. We might want to try and renew - # these items. - my $overdue_unreturned = {}; - $schema->txn_do( sub { for my $debit ( @{$debits} ) { @@ -255,17 +250,10 @@ sub apply { $self->amountoutstanding( $available_credit * -1 )->store; $debit->amountoutstanding( $owed - $amount_to_cancel )->store; - # If we need to make a note of the item associated with this line, - # in order that we can potentially renew it, do so. - # Same logic existing in Koha::Account::pay - if ( - $debit->amountoutstanding == 0 && - $debit->accounttype && - $debit->accounttype eq 'OVERDUE' && - $debit->status && - $debit->status eq 'UNRETURNED' - ) { - $overdue_unreturned->{$debit->itemnumber} = $debit; + # Attempt to renew the item associated with this debit if + # appropriate + if ($debit->renewable) { + $debit->renew_item($params->{interface}); } # Same logic exists in Koha::Account::pay @@ -280,36 +268,6 @@ sub apply { } }); - # If we have overdue unreturned items that have had payments made - # against them, check whether the balance on those items is now zero - # and, if the syspref is set, renew them - # Same logic existing in Koha::Account::pay - if ( - C4::Context->preference('RenewAccruingItemWhenPaid') && - keys %{$overdue_unreturned} - ) { - foreach my $itemnumber (keys %{$overdue_unreturned}) { - # Only do something if this item has no fines left on it - my $fine = C4::Overdues::GetFine( $itemnumber, $self->borrowernumber ); - next if $fine && $fine > 0; - - my ( $renew_ok, $error ) = - C4::Circulation::CanBookBeRenewed( - $self->borrowernumber, $itemnumber - ); - if ( $renew_ok ) { - C4::Circulation::AddRenewal( - $self->borrowernumber, - $itemnumber, - $overdue_unreturned->{$itemnumber}->{branchcode}, - undef, - undef, - 1 - ); - } - } - } - return $available_credit; } @@ -481,6 +439,98 @@ sub to_api_mapping { manager_id => 'user_id', note => 'internal_note', }; + +=head3 renewable + + my $bool = $line->renewable; + +=cut + +sub renewable { + my ($self) = @_; + + return ( + $self->amountoutstanding == 0 && + $self->accounttype && + $self->accounttype eq 'OVERDUE' && + $self->status && + $self->status eq 'UNRETURNED' + ) ? 1 : 0; +} + +=head3 renew_item + + my $renew_result = $line->renew_item; + +Conditionally attempt to renew an item and return the outcome. This is +as a consequence of the fine on an item being fully paid off + +=cut + +sub renew_item { + my ($self, $params) = @_; + + my $outcome = {}; + + # We want to reject the call to renew if any of these apply: + # - The RenewAccruingItemWhenPaid syspref is off + # - The line item doesn't have an item attached to it + # - The line item doesn't have a patron attached to it + # + # - The RenewAccruingItemInOpac syspref is off + # AND + # - There is an interface param passed and it's value is 'opac' + + if ( + !C4::Context->preference('RenewAccruingItemWhenPaid') || + !$self->item || + !$self->patron || + ( + !C4::Context->preference('RenewAccruingItemInOpac') && + $params->{interface} && + $params->{interface} eq 'opac' + ) + ) { + return; + } + + my $itemnumber = $self->item->itemnumber; + my $borrowernumber = $self->patron->borrowernumber; + # Only do something if this item has no fines left on it + my $fine = C4::Overdues::GetFine($itemnumber, $borrowernumber); + if ($fine && $fine > 0) { + return { + itemnumber => $itemnumber, + error => 'has_fine', + success => 0 + }; + } + my ( $can_renew, $error ) = C4::Circulation::CanBookBeRenewed( + $borrowernumber, + $itemnumber + ); + if ( $can_renew ) { + my $due_date = C4::Circulation::AddRenewal( + $borrowernumber, + $itemnumber, + $self->{branchcode}, + undef, + undef, + 1 + ); + return { + itemnumber => $itemnumber, + due_date => $due_date, + success => 1 + }; + } else { + return { + itemnumber => $itemnumber, + error => $error, + success => 0 + }; + } + } =head2 Internal methods --- a/Koha/REST/V1/Patrons/Account.pm +++ a/Koha/REST/V1/Patrons/Account.pm @@ -130,7 +130,7 @@ sub add_credit { my $outstanding_credit = $credit->amountoutstanding; if ($debits) { # pay them! - $outstanding_credit = $credit->apply({ debits => [ $debits->as_list ], offset_type => 'payment' }); + $outstanding_credit = $credit->apply({ debits => [ $debits->as_list ], offset_type => 'payment' })->{outstanding_amount}; } if ($outstanding_credit) { --- a/installer/data/mysql/atomicupdate/bug_23051_add_RenewAccruingItemInOpac_syspref.perl +++ a/installer/data/mysql/atomicupdate/bug_23051_add_RenewAccruingItemInOpac_syspref.perl @@ -0,0 +1,8 @@ +$DBversion = 'XXX'; # will be replaced by the RM +if( CheckVersion( $DBversion ) ) { + + $dbh->do( q| INSERT IGNORE INTO systempreferences (variable, value, explanation, options, type) VALUES ('RenewAccruingItemInOpac', '0', 'If enabled, when the fines on an item accruing is paid off in the OPAC via a payment plugin, attempt to renew that item. If the syspref "RenewalPeriodBase" is set to "due date", renewed items may still be overdue', '', 'YesNo'); | ); + + SetVersion( $DBversion ); + print "Upgrade to $DBversion done (Bug 23051 - Add RenewAccruingItemInOpac syspref)\n"; +} --- a/installer/data/mysql/atomicupdate/bug_23051_add_RenewAccruingItemWhenPaid_syspref.perl +++ a/installer/data/mysql/atomicupdate/bug_23051_add_RenewAccruingItemWhenPaid_syspref.perl @@ -1,7 +1,7 @@ $DBversion = 'XXX'; # will be replaced by the RM if( CheckVersion( $DBversion ) ) { - $dbh->do( q| INSERT IGNORE INTO systempreferences (variable, value, explanation, options, type) VALUES ('RenewAccruingItemWhenPaid', '0', 'If enabled, when the fines on an item accruing is paid off, attempt to renew that item', '', 'YesNo'); | ); + $dbh->do( q| INSERT IGNORE INTO systempreferences (variable, value, explanation, options, type) VALUES ('RenewAccruingItemWhenPaid', '0', 'If enabled, when the fines on an item accruing is paid off, attempt to renew that item. If the syspref "RenewalPeriodBase" is set to "due date", renewed items may still be overdue', '', 'YesNo'); | ); SetVersion( $DBversion ); print "Upgrade to $DBversion done (Bug 23051 - Add RenewAccruingItemWhenPaid syspref)\n"; --- a/installer/data/mysql/sysprefs.sql +++ a/installer/data/mysql/sysprefs.sql @@ -513,7 +513,8 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('RandomizeHoldsQueueWeight','0',NULL,'if ON, the holds queue in circulation will be randomized, either based on all location codes, or by the location codes specified in StaticHoldsQueueWeight','YesNo'), ('RecordLocalUseOnReturn','0',NULL,'If ON, statistically record returns of unissued items as local use, instead of return','YesNo'), ('RefundLostOnReturnControl','CheckinLibrary','CheckinLibrary|ItemHomeBranch|ItemHoldingBranch','If a lost item is returned, choose which branch to pick rules for refunding.','Choice'), -('RenewAccruingItemWhenPaid','0','','If enabled, when the fines on an item accruing is paid off, attempt to renew that item','YesNo'), +('RenewAccruingItemWhenPaid','0','','If enabled, when the fines on an item accruing is paid off, attempt to renew that item. If the syspref "RenewalPeriodBase" is set to "due date", renewed items may still be overdue','YesNo'), +('RenewAccruingItemInOpac','0','','If enabled, when the fines on an item accruing is paid off in the OPAC via a payment plugin, attempt to renew that item. If the syspref "RenewalPeriodBase" is set to "due date", renewed items may still be overdue','YesNo'), ('RenewalLog','0','','If ON, log information about renewals','YesNo'), ('RenewalPeriodBase','date_due','date_due|now','Set whether the renewal date should be counted from the date_due or from the moment the Patron asks for renewal ','Choice'), ('RenewalSendNotice','0','',NULL,'YesNo'), --- a/koha-tmpl/intranet-tmpl/prog/en/includes/renew_results.inc +++ a/koha-tmpl/intranet-tmpl/prog/en/includes/renew_results.inc @@ -0,0 +1,12 @@ +[% IF renew_results && renew_results.size > 0 %] +
[% INCLUDE 'biblio-title.inc' biblio=result.item.biblio %] ( [% result.item.barcode | html %] ): Renewed - due [% result.info | html %]
+ [% ELSE %] +[% INCLUDE 'biblio-title.inc' biblio=result.item.biblio %] ( [% result.item.barcode | html %] ): Not renewed - [% INCLUDE 'renew_strings.inc' error=result.info %]
+ [% END %] + [% END %] +