From 372d665d1795d893422a0ba6ee1e651a603502db Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Tue, 8 Nov 2016 13:52:56 +0000 Subject: [PATCH] [SIGNED-OFF] Bug 17588: Koha::Patrons - Move GetMemberIssuesAndFines The GetMemberIssuesAndFines subroutine used to retrieve the issues, overdues and fines for a given patron. Most of the time, only 1 or 2 of these values were used. This patch removes this subroutine and uses the new get_issues, get_overdues and get_balance method from Koha::Patron and Koha::Account::Lines. Test plan: 1/ Add overdues, issues and fines to different patrons 2/ On the checkout, checkin and patron search result and the patron detail pages, these 3 informations, if displayed before this patch, must be correctly displayed. 3/ Use the batch patron deletion tool and make sure that patrons with a balance > 0 are not deleted Signed-off-by: Josef Moravec --- C4/Members.pm | 42 ----------------------------------- C4/Utils/DataTables/Members.pm | 11 +++++---- C4/Utils/DataTables/VirtualShelves.pm | 1 - circ/circulation.pl | 19 ++++++++-------- circ/returns.pl | 9 +++++--- members/moremember.pl | 15 ++++++++----- t/db_dependent/Reserves.t | 9 ++++---- tools/cleanborrowers.pl | 3 ++- 8 files changed, 40 insertions(+), 69 deletions(-) diff --git a/C4/Members.pm b/C4/Members.pm index dfe4cbd..62ec040 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -62,7 +62,6 @@ BEGIN { push @EXPORT, qw( &GetMember - &GetMemberIssuesAndFines &GetPendingIssues &GetAllIssues @@ -343,47 +342,6 @@ sub GetMember { return; } -=head2 GetMemberIssuesAndFines - - ($overdue_count, $issue_count, $total_fines) = &GetMemberIssuesAndFines($borrowernumber); - -Returns aggregate data about items borrowed by the patron with the -given borrowernumber. - -C<&GetMemberIssuesAndFines> returns a three-element array. C<$overdue_count> is the -number of overdue items the patron currently has borrowed. C<$issue_count> is the -number of books the patron currently has borrowed. C<$total_fines> is -the total fine currently due by the borrower. - -=cut - -#' -sub GetMemberIssuesAndFines { - my ( $borrowernumber ) = @_; - my $dbh = C4::Context->dbh; - my $query = "SELECT COUNT(*) FROM issues WHERE borrowernumber = ?"; - - $debug and warn $query."\n"; - my $sth = $dbh->prepare($query); - $sth->execute($borrowernumber); - my $issue_count = $sth->fetchrow_arrayref->[0]; - - $sth = $dbh->prepare( - "SELECT COUNT(*) FROM issues - WHERE borrowernumber = ? - AND date_due < now()" - ); - $sth->execute($borrowernumber); - my $overdue_count = $sth->fetchrow_arrayref->[0]; - - $sth = $dbh->prepare("SELECT SUM(amountoutstanding) FROM accountlines WHERE borrowernumber = ?"); - $sth->execute($borrowernumber); - my $total_fines = $sth->fetchrow_arrayref->[0]; - - return ($overdue_count, $issue_count, $total_fines); -} - - =head2 ModMember my $success = ModMember(borrowernumber => $borrowernumber, diff --git a/C4/Utils/DataTables/Members.pm b/C4/Utils/DataTables/Members.pm index 2446703..e4f12cc 100644 --- a/C4/Utils/DataTables/Members.pm +++ b/C4/Utils/DataTables/Members.pm @@ -2,7 +2,6 @@ package C4::Utils::DataTables::Members; use Modern::Perl; use C4::Context; -use C4::Members qw/GetMemberIssuesAndFines/; use C4::Utils::DataTables; use Koha::DateUtils; @@ -169,14 +168,18 @@ sub search { # Get some information on patrons foreach my $patron (@$patrons) { - ($patron->{overdues}, $patron->{issues}, $patron->{fines}) = - GetMemberIssuesAndFines($patron->{borrowernumber}); + my $patron_object = Koha::Patrons->find( $patron->{borrowernumber} ); + $patron->{overdues} = $patron_object->get_overdues->count; + $patron->{issues} = $patron_object->get_issues->count; + my $balance = $patron_object->get_account_lines->get_balance; + # FIXME Should be formatted from the template + $patron->{fines} = sprintf("%.2f", $balance); + if($patron->{dateexpiry} and $patron->{dateexpiry} ne '0000-00-00') { $patron->{dateexpiry} = output_pref( { dt => dt_from_string( $patron->{dateexpiry}, 'iso'), dateonly => 1} ); } else { $patron->{dateexpiry} = ''; } - $patron->{fines} = sprintf("%.2f", $patron->{fines} || 0); } return { diff --git a/C4/Utils/DataTables/VirtualShelves.pm b/C4/Utils/DataTables/VirtualShelves.pm index 2b37706..28f579f 100644 --- a/C4/Utils/DataTables/VirtualShelves.pm +++ b/C4/Utils/DataTables/VirtualShelves.pm @@ -2,7 +2,6 @@ package C4::Utils::DataTables::VirtualShelves; use Modern::Perl; use C4::Context; -use C4::Members qw/GetMemberIssuesAndFines/; use C4::Utils::DataTables; use Koha::Virtualshelves; diff --git a/circ/circulation.pl b/circ/circulation.pl index b065267..82cc89e 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -266,7 +266,10 @@ my $patron; if ($borrowernumber) { $patron = Koha::Patrons->find( $borrowernumber ); $borrower = GetMember( borrowernumber => $borrowernumber ); - my ( $od, $issue, $fines ) = GetMemberIssuesAndFines( $borrowernumber ); + my $overdues = $patron->get_overdues; + my $issues = $patron->get_issues; + my $balance = $patron->get_account_lines->get_balance; + # if the expiry date is before today ie they have expired if ( $patron->is_expired ) { @@ -287,9 +290,9 @@ if ($borrowernumber) { } } $template->param( - overduecount => $od, - issuecount => $issue, - finetotal => $fines + overduecount => $overdues->count, + issuecount => $issues->count, + finetotal => $balance, ); if ( $patron and $patron->is_debarred ) { @@ -407,9 +410,6 @@ if (@$barcodes) { } } - # FIXME If the issue is confirmed, we launch another time GetMemberIssuesAndFines, now display the issue count after issue - my ( $od, $issue, $fines ) = GetMemberIssuesAndFines($borrowernumber); - if ($question->{RESERVE_WAITING} or $question->{RESERVED}){ $template->param( reserveborrowernumber => $question->{'resborrowernumber'} @@ -421,8 +421,9 @@ if (@$barcodes) { ); - - $template_params->{issuecount} = $issue; + # FIXME If the issue is confirmed, we launch another time get_issues->count, now display the issue count after issue + $patron = Koha::Patrons->find( $borrowernumber ); + $template_params->{issuecount} = $patron->get_issues->count; if ( $iteminfo ) { $iteminfo->{subtitle} = GetRecordValue('subtitle', GetMarcBiblio($iteminfo->{biblionumber}), GetFrameworkCode($iteminfo->{biblionumber})); diff --git a/circ/returns.pl b/circ/returns.pl index 5927759..25715e4 100755 --- a/circ/returns.pl +++ b/circ/returns.pl @@ -323,9 +323,12 @@ if ($barcode) { push( @inputloop, \%input ); if ( C4::Context->preference("FineNotifyAtCheckin") ) { - my ( $od, $issue, $fines ) = GetMemberIssuesAndFines( $borrower->{'borrowernumber'} ); - if ($fines && $fines > 0) { - $template->param( fines => sprintf("%.2f",$fines) ); + my $patron = Koha::Patrons->find( $borrower->{borrowernumber} ); + my $account_lines = $patron->get_account_lines; + my $balance = $patron->get_account_lines->get_balance; + + if ($balance > 0) { + $template->param( fines => sprintf("%.2f", $balance) ); $template->param( fineborrowernumber => $borrower->{'borrowernumber'} ); } } diff --git a/members/moremember.pl b/members/moremember.pl index 55b0901..c6ce1cd 100755 --- a/members/moremember.pl +++ b/members/moremember.pl @@ -119,8 +119,14 @@ my $borrowernumber = $input->param('borrowernumber'); my $error = $input->param('error'); $template->param( error => $error ) if ( $error ); -my ( $od, $issue, $fines ) = GetMemberIssuesAndFines($borrowernumber); -$template->param( issuecount => $issue, fines => $fines ); +my $patron = Koha::Patrons->find($borrowernumber); +my $issues = $patron->get_issues; +my $balance = $patron->get_account_lines->get_balance; +$template->param( + issuecount => $issues->count, + fines => $balance, +); + my $data = GetMember( 'borrowernumber' => $borrowernumber ); @@ -148,7 +154,7 @@ for (qw(gonenoaddress lost borrowernotes)) { $data->{$_} and $template->param(flagged => 1) and last; } -if ( Koha::Patrons->find( $borrowernumber )->is_debarred ) { +if ( $patron->is_debarred ) { $template->param( 'userdebarred' => 1, 'flagged' => 1 ); my $debar = $data->{'debarred'}; if ( $debar ne "9999-12-31" ) { @@ -165,7 +171,6 @@ if ( $category_type eq 'C') { $template->param( 'catcode' => $patron_categories->next ) if $patron_categories->count == 1; } -my $patron = Koha::Patrons->find($data->{borrowernumber}); my @relatives; if ( my $guarantor = $patron->guarantor ) { $template->param( guarantor => $guarantor ); @@ -321,7 +326,7 @@ if (C4::Context->preference('EnhancedMessagingPreferences')) { $template->param(TalkingTechItivaPhone => C4::Context->preference("TalkingTechItivaPhoneNotification")); } -# in template => instutitional (A for Adult, C for children) +# in template => instutitional (A for Adult, C for children) $template->param( $data->{'categorycode'} => 1 ); $template->param( patron => $patron, diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index 37d90d9..8a63bde 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -571,7 +571,8 @@ ok( !C4::Reserves::OnShelfHoldsAllowed($item, $borrower), "OnShelfHoldsAllowed() # Tests for bug 14464 $dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum)); -my ( undef, undef, $bz14464_fines ) = GetMemberIssuesAndFines( $borrowernumber ); +my $patron = Koha::Patrons->find( $borrowernumber ); +my $bz14464_fines = $patron->get_account_lines->get_balance; is( !$bz14464_fines || $bz14464_fines==0, 1, 'Bug 14464 - No fines at beginning' ); # First, test cancelling a reserve when there's no charge configured. @@ -598,7 +599,7 @@ CancelReserve({ reserve_id => $bz14464_reserve, charge_cancel_fee => 1 }); my $old_reserve = Koha::Database->new()->schema()->resultset('OldReserve')->find( $bz14464_reserve ); is($old_reserve->get_column('found'), 'W', 'Bug 14968 - Keep found column from reserve'); -( undef, undef, $bz14464_fines ) = GetMemberIssuesAndFines( $borrowernumber ); +$bz14464_fines = $patron->get_account_lines->get_balance; is( !$bz14464_fines || $bz14464_fines==0, 1, 'Bug 14464 - No fines after cancelling reserve with no charge configured' ); # Then, test cancelling a reserve when there's no charge desired. @@ -622,7 +623,7 @@ ok( $bz14464_reserve, 'Bug 14464 - 2nd reserve correctly created' ); CancelReserve({ reserve_id => $bz14464_reserve }); -( undef, undef, $bz14464_fines ) = GetMemberIssuesAndFines( $borrowernumber ); +$bz14464_fines = $patron->get_account_lines->get_balance; is( !$bz14464_fines || $bz14464_fines==0, 1, 'Bug 14464 - No fines after cancelling reserve with no charge desired' ); # Finally, test cancelling a reserve when there's a charge desired and configured. @@ -644,7 +645,7 @@ ok( $bz14464_reserve, 'Bug 14464 - 1st reserve correctly created' ); CancelReserve({ reserve_id => $bz14464_reserve, charge_cancel_fee => 1 }); -( undef, undef, $bz14464_fines ) = GetMemberIssuesAndFines( $borrowernumber ); +$bz14464_fines = $patron->get_account_lines->get_balance; is( int( $bz14464_fines ), 42, 'Bug 14464 - Fine applied after cancelling reserve with charge desired and configured' ); # tests for MoveReserve in relation to ConfirmFutureHolds (BZ 14526) diff --git a/tools/cleanborrowers.pl b/tools/cleanborrowers.pl index a1c2135..d2713b5 100755 --- a/tools/cleanborrowers.pl +++ b/tools/cleanborrowers.pl @@ -199,7 +199,8 @@ sub _skip_borrowers_with_nonzero_balance { my $borrowers = shift; my $balance; @$borrowers = map { - (undef, undef, $balance) = GetMemberIssuesAndFines( $_->{borrowernumber} ); + my $patron = Koha::Patrons->find( $_->{borrowernumber} ); + my $balance = $patron->get_account_lines->get_balance; (defined $balance && $balance != 0) ? (): ($_); } @$borrowers; } -- 2.1.4