Bug 19933: Remove patronflags - tricky ones
authorJonathan Druart <jonathan.druart@bugs.koha-community.org>
Sun, 7 Jan 2018 17:57:23 +0000 (14:57 -0300)
committerJonathan Druart <jonathan.druart@bugs.koha-community.org>
Fri, 23 Mar 2018 14:45:38 +0000 (11:45 -0300)
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 <kyle@bywatersolutions.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

C4/Members.pm
C4/SIP/ILS/Patron.pm
circ/circulation.pl
koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation_batch_checkouts.tt

index 53e131a..b14f907 100644 (file)
@@ -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) = @_;
index 92c032a..04904fd 100644 (file)
@@ -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}) {
index 125d75d..9ddf3d5 100755 (executable)
@@ -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#<br />#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#<br />#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,
index 2f1d118..198c47d 100644 (file)
@@ -707,7 +707,7 @@ No patron matched <span class="ex">[% message | html %]</span>
                </li>
             [% END %]
 
-                [% IF ( odues ) %]<li>[% IF ( nonreturns ) %]<span class="circ-hlt">Overdues: Patron has ITEMS OVERDUE.</span> <a href="#checkouts">See highlighted items below</a>[% END %]</li>
+                [% IF ( odues ) %]<li><span class="circ-hlt">Overdues: Patron has ITEMS OVERDUE.</span> <a href="#checkouts">See highlighted items below</a></li>
             [% END %]
 
             [% IF ( charges ) %]
@@ -717,7 +717,7 @@ No patron matched <span class="ex">[% message | html %]</span>
             [% IF ( charges_guarantees ) %]
                 <li>
                     <span class="circ-hlt">Fees &amp; Charges:</span> Patron's guarantees collectively owe [% chargesamount_guarantees | $Price %].
-                        [% IF ( charges_guarantees_is_blocker ) %]
+                        [% IF noissues %]
                             <span class="circ-hlt">Checkouts are BLOCKED because fine balance is OVER THE LIMIT.</span>
                         [% END %]
                 </li>
@@ -726,7 +726,7 @@ No patron matched <span class="ex">[% message | html %]</span>
 
             [% IF ( credits ) %]
                 <li>
-                    <span class="circ-hlt">Credits:</span> Patron has a credit[% IF ( creditsamount ) %] of [% creditsamount %][% END %]
+                    <span class="circ-hlt">Credits:</span> Patron has a credit[% IF ( creditsamount ) %] of [% creditsamount | $Price %][% END %]
                 </li>
             [% END %]
 
index b40a9b3..87860b9 100644 (file)
 [% ELSIF patron and noissues and not checkout_infos %]
   <div class="dialog alert">
     Cannot check out!
-    [% IF charges_is_blocker %]
+    [% IF charges %]
       <span class="circ-hlt">Checkouts are BLOCKED because fine balance is OVER THE LIMIT.</span>
     [% END %]
-    [% IF charges_guarantees_is_blocker %]
+    [% IF charges_guarantees %]
         <li>
             <span class="circ-hlt">Fees &amp; Charges:</span> Patron's guarantees collectively owe [% chargesamount_guarantees | $Price %].
         </li>