From bddfed75913a2302c19c00b00a6fe86538af6f9f Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Sun, 7 Jan 2018 14:57:23 -0300 Subject: [PATCH] Bug 19933: Remove patronflags - tricky ones Here we are, patronflags is used in a couple of places where (almost) all flags were really useful: C4::SIP::ILS::Patron->new and circulation.pl This patch only deal with the circulation code as I am not convident enough with SIP code. The change does not seems trivial because of the complexity of the existing code, but the logic is the same. We send a variable to the template depending on the situation of the patron. I guess only code eyes ball could catch issue in this patch quickly. Maybe we need to find a good place in a Koha module to move this code and provide code coverage (especially when C4::SIP::ILS::Patron will reuse it). Signed-off-by: Kyle M Hall Signed-off-by: Josef Moravec Signed-off-by: Jonathan Druart --- C4/Members.pm | 1 + C4/SIP/ILS/Patron.pm | 18 ++- circ/circulation.pl | 137 ++++++++---------- .../prog/en/modules/circ/circulation.tt | 6 +- .../circ/circulation_batch_checkouts.tt | 4 +- 5 files changed, 77 insertions(+), 89 deletions(-) diff --git a/C4/Members.pm b/C4/Members.pm index 53e131ac3e..b14f907bd2 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -173,6 +173,7 @@ The "message" field that comes from the DB is OK. # TODO: use {anonymous => hashes} instead of a dozen %flaginfo # FIXME rename this function. +# DEPRECATED Do not use this subroutine! sub patronflags { my %flags; my ( $patroninformation) = @_; diff --git a/C4/SIP/ILS/Patron.pm b/C4/SIP/ILS/Patron.pm index 92c032aa5b..04904fd6ae 100644 --- a/C4/SIP/ILS/Patron.pm +++ b/C4/SIP/ILS/Patron.pm @@ -38,18 +38,18 @@ sub new { my ($class, $patron_id) = @_; my $type = ref($class) || $class; my $self; - $kp = Koha::Patrons->find( { cardnumber => $patron_id } ) + my $patron = Koha::Patrons->find( { cardnumber => $patron_id } ) || Koha::Patrons->find( { userid => $patron_id } ); - $debug and warn "new Patron: " . Dumper($kp->unblessed) if $kp; - unless ($kp) { + $debug and warn "new Patron: " . Dumper($patron->unblessed) if $patron; + unless ($patron) { syslog("LOG_DEBUG", "new ILS::Patron(%s): no such patron", $patron_id); return; } - $kp = $kp->unblessed; + $kp = $patron->unblessed; my $pw = $kp->{password}; my $flags = C4::Members::patronflags( $kp ); - my $debarred = defined($flags->{DBARRED}); - $debug and warn sprintf("Debarred = %s : ", ($debarred||'undef')) . Dumper(%$flags); + my $debarred = $patron->is_debarred; + $debug and warn sprintf("Debarred = %s : ", ($debarred||'undef')); # Do we need more debug info here? my ($day, $month, $year) = (localtime)[3,4,5]; my $today = sprintf '%04d-%02d-%02d', $year+1900, $month+1, $day; my $expired = ($today gt $kp->{dateexpiry}) ? 1 : 0; @@ -65,7 +65,7 @@ sub new { $dob and $dob =~ s/-//g; # YYYYMMDD my $dexpiry = $kp->{dateexpiry}; $dexpiry and $dexpiry =~ s/-//g; # YYYYMMDD - my $fines_amount = $flags->{CHARGES}->{amount}; + my $fines_amount = $patron->account->balance; $fines_amount = ($fines_amount and $fines_amount > 0) ? $fines_amount : 0; my $fee_limit = _fee_limit(); my $fine_blocked = $fines_amount > $fee_limit; @@ -112,6 +112,10 @@ sub new { ); } $debug and warn "patron fines: $ilspatron{fines} ... amountoutstanding: $kp->{amountoutstanding} ... CHARGES->amount: $flags->{CHARGES}->{amount}"; + + if ( $patron->is_debarred and $patron->debarredcomment ) { + $ilspatron{screen_msg} .= " -- " . $patron->debarredcomment; + } for (qw(EXPIRED CHARGES CREDITS GNA LOST DBARRED NOTES)) { ($flags->{$_}) or next; if ($_ ne 'NOTES' and $flags->{$_}->{message}) { diff --git a/circ/circulation.pl b/circ/circulation.pl index 125d75d4f1..9ddf3d5a45 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -28,6 +28,7 @@ use Modern::Perl; use CGI qw ( -utf8 ); use DateTime; use DateTime::Duration; +use Scalar::Util qw( looks_like_number ); use C4::Output; use C4::Print; use C4::Auth qw/:DEFAULT get_session haspermission/; @@ -272,6 +273,7 @@ if ($findborrower) { } # get the borrower information..... +my $balance = 0; $patron ||= Koha::Patrons->find( $borrowernumber ) if $borrowernumber; if ($patron) { @@ -280,7 +282,7 @@ if ($patron) { my $overdues = $patron->get_overdues; my $issues = $patron->checkouts; - my $balance = $patron->account->balance; + $balance = $patron->account->balance; # if the expiry date is before today ie they have expired @@ -468,91 +470,72 @@ if ($patron) { ); } -#title -my $flags = $patron ? C4::Members::patronflags( $patron->unblessed ) : {}; -foreach my $flag ( sort keys %$flags ) { - $flags->{$flag}->{'message'} =~ s#\n#
#g; - if ( $flags->{$flag}->{'noissues'} ) { +if ( $patron ) { + my $noissues; + if ( $patron->gonenoaddress ) { + $template->param( gna => 1 ); + $noissues = 1; + } + if ( $patron->lost ) { + $template->param( lost=> 1 ); + $noissues = 1; + } + if ( $patron->is_debarred ) { + $template->param( dbarred=> 1 ); + $noissues = 1; + } + my $account = $patron->account; + if( ( my $owing = $account->non_issues_charges ) > 0 ) { + my $noissuescharge = C4::Context->preference("noissuescharge") || 5; # FIXME If noissuescharge == 0 then 5, why?? + $noissues = ( not C4::Context->preference("AllowFineOverride") and ( $owing > $noissuescharge ) ); $template->param( - noissues => ($force_allow_issue) ? 0 : 'true', - forceallow => $force_allow_issue, + charges => 1, + chargesamount => $owing, + ) + } elsif ( $balance < 0 ) { + $template->param( + credits => 1, + creditsamount => -$balance, ); - if ( $flag eq 'GNA' ) { - $template->param( gna => 'true' ); - } - elsif ( $flag eq 'LOST' ) { - $template->param( lost => 'true' ); - } - elsif ( $flag eq 'DBARRED' ) { - $template->param( dbarred => 'true' ); - } - elsif ( $flag eq 'CHARGES' ) { - $template->param( - charges => 'true', - chargesmsg => $flags->{'CHARGES'}->{'message'}, - chargesamount => $flags->{'CHARGES'}->{'amount'}, - charges_is_blocker => 1 - ); - } - elsif ( $flag eq 'CHARGES_GUARANTEES' ) { - $template->param( - charges_guarantees => 'true', - chargesmsg_guarantees => $flags->{'CHARGES_GUARANTEES'}->{'message'}, - chargesamount_guarantees => $flags->{'CHARGES_GUARANTEES'}->{'amount'}, - charges_guarantees_is_blocker => 1 - ); - } - elsif ( $flag eq 'CREDITS' ) { - $template->param( - credits => 'true', - creditsmsg => $flags->{'CREDITS'}->{'message'}, - creditsamount => sprintf("%.02f", -($flags->{'CREDITS'}->{'amount'})), # from patron's pov - ); - } } - else { - if ( $flag eq 'CHARGES' ) { - $template->param( - charges => 'true', - chargesmsg => $flags->{'CHARGES'}->{'message'}, - chargesamount => $flags->{'CHARGES'}->{'amount'}, - ); - } - elsif ( $flag eq 'CHARGES_GUARANTEES' ) { - $template->param( - charges_guarantees => 'true', - chargesmsg_guarantees => $flags->{'CHARGES_GUARANTEES'}->{'message'}, - chargesamount_guarantees => $flags->{'CHARGES_GUARANTEES'}->{'amount'}, - ); - } - elsif ( $flag eq 'CREDITS' ) { - $template->param( - credits => 'true', - creditsmsg => $flags->{'CREDITS'}->{'message'}, - creditsamount => sprintf("%.02f", -($flags->{'CREDITS'}->{'amount'})), # from patron's pov - ); - } - elsif ( $flag eq 'ODUES' ) { - $template->param( - odues => 'true', - oduesmsg => $flags->{'ODUES'}->{'message'} - ); - my $items = $flags->{$flag}->{'itemlist'}; - if ( ! $query->param('module') || $query->param('module') ne 'returns' ) { - $template->param( nonreturns => 'true' ); - } + my $no_issues_charge_guarantees = C4::Context->preference("NoIssuesChargeGuarantees"); + $no_issues_charge_guarantees = undef unless looks_like_number( $no_issues_charge_guarantees ); + if ( defined $no_issues_charge_guarantees ) { + my $guarantees_non_issues_charges = 0; + my $guarantees = $patron->guarantees; + while ( my $g = $guarantees->next ) { + $guarantees_non_issues_charges += $g->account->non_issues_charges; } - elsif ( $flag eq 'NOTES' ) { + if ( $guarantees_non_issues_charges > $no_issues_charge_guarantees ) { $template->param( - notes => 'true', - notesmsg => $flags->{'NOTES'}->{'message'} + charges_guarantees => 1, + chargesamount_guarantees => $guarantees_non_issues_charges, ); + $noissues = 1 unless C4::Context->preference("allowfineoverride"); } } -} -my $total = $patron ? $patron->account->balance : 0; + if ( $patron->has_overdues ) { + $template->param( odues => 1 ); + } + + if ( $patron->borrowernotes ) { + my $borrowernotes = $patron->borrowernotes; + $borrowernotes =~ s#\n#
#g; + $template->param( + notes =>1, + notesmsg => $borrowernotes, + ) + } + + if ( $noissues ) { + $template->param( + noissues => ($force_allow_issue) ? 0 : 'true', + forceallow => $force_allow_issue, + ); + } +} if ( $patron && $patron->is_child) { my $patron_categories = Koha::Patron::Categories->search_limited({ category_type => 'A' }, {order_by => ['categorycode']}); @@ -634,7 +617,7 @@ $template->param( duedatespec => $duedatespec, restoreduedatespec => $restoreduedatespec, message => $message, - totaldue => sprintf('%.2f', $total), + totaldue => sprintf('%.2f', $balance), # FIXME not used in template? inprocess => $inprocess, $view => 1, batch_allowed => $batch_allowed, diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt index 2f1d1185e3..198c47dd12 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt @@ -707,7 +707,7 @@ No patron matched [% message | html %] [% END %] - [% IF ( odues ) %]
  • [% IF ( nonreturns ) %]Overdues: Patron has ITEMS OVERDUE. See highlighted items below[% END %]
    • + [% IF ( odues ) %]
  • Overdues: Patron has ITEMS OVERDUE. See highlighted items below
    • [% END %] [% IF ( charges ) %] @@ -717,7 +717,7 @@ No patron matched [% message | html %] [% IF ( charges_guarantees ) %]
  • Fees & Charges: Patron's guarantees collectively owe [% chargesamount_guarantees | $Price %]. - [% IF ( charges_guarantees_is_blocker ) %] + [% IF noissues %] Checkouts are BLOCKED because fine balance is OVER THE LIMIT. [% END %]
    • @@ -726,7 +726,7 @@ No patron matched [% message | html %] [% IF ( credits ) %]
  • - Credits: Patron has a credit[% IF ( creditsamount ) %] of [% creditsamount %][% END %] + Credits: Patron has a credit[% IF ( creditsamount ) %] of [% creditsamount | $Price %][% END %]
    • [% END %] diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation_batch_checkouts.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation_batch_checkouts.tt index b40a9b3c2c..87860b92c4 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation_batch_checkouts.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation_batch_checkouts.tt @@ -43,10 +43,10 @@ [% ELSIF patron and noissues and not checkout_infos %]
    Cannot check out! - [% IF charges_is_blocker %] + [% IF charges %] Checkouts are BLOCKED because fine balance is OVER THE LIMIT. [% END %] - [% IF charges_guarantees_is_blocker %] + [% IF charges_guarantees %]
  • Fees & Charges: Patron's guarantees collectively owe [% chargesamount_guarantees | $Price %].
    • -- 2.39.5