Bug 5182 - Attempting to export a patron card with no text causes an error to be...
[koha_fer] / C4 / Members.pm
index 3dcd46a..cddbf41 100644 (file)
@@ -13,12 +13,13 @@ package C4::Members;
 # WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
 # A PARTICULAR PURPOSE.  See the GNU General Public License for more details.
 #
-# You should have received a copy of the GNU General Public License along with
-# Koha; if not, write to the Free Software Foundation, Inc., 59 Temple Place,
-# Suite 330, Boston, MA  02111-1307 USA
+# You should have received a copy of the GNU General Public License along
+# with Koha; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
 
 
 use strict;
+#use warnings; FIXME - Bug 2505
 use C4::Context;
 use C4::Dates qw(format_date_in_iso);
 use Digest::MD5 qw(md5_base64);
@@ -28,7 +29,8 @@ use C4::Overdues;
 use C4::Reserves;
 use C4::Accounts;
 use C4::Biblio;
-use C4::SQLHelper qw(InsertInTable UpdateInTable);
+use C4::SQLHelper qw(InsertInTable UpdateInTable SearchInTable);
+use C4::Members::Attributes qw(SearchIdMatchingAttribute);
 
 our ($VERSION,@ISA,@EXPORT,@EXPORT_OK,$debug);
 
@@ -39,6 +41,7 @@ BEGIN {
        @ISA = qw(Exporter);
        #Get data
        push @EXPORT, qw(
+               &Search
                &SearchMember 
                &GetMemberDetails
                &GetMember
@@ -53,6 +56,8 @@ BEGIN {
                &getzipnamecity 
                &getidcity
 
+                &GetFirstValidEmailAddress
+
                &GetAge 
                &GetCities 
                &GetRoadTypes 
@@ -130,13 +135,10 @@ This module contains routines for adding, modifying and deleting members/patrons
 
 =head1 FUNCTIONS
 
-=over 2
-
-=item SearchMember
+=head2 SearchMember
 
-  ($count, $borrowers) = &SearchMember($searchstring, $type,$category_type,$filter,$showallbranches);
-
-=back
+  ($count, $borrowers) = &SearchMember($searchstring, $type, 
+                     $category_type, $filter, $showallbranches);
 
 Looks up patrons (borrowers) by name.
 
@@ -164,7 +166,6 @@ C<$count> is the number of elements in C<$borrowers>.
 
 #'
 #used by member enquiries from the intranet
-#called by member.pl and circ/circulation.pl
 sub SearchMember {
     my ($searchstring, $orderby, $type,$category_type,$filter,$showallbranches ) = @_;
     my $dbh   = C4::Context->dbh;
@@ -184,7 +185,6 @@ sub SearchMember {
     if (@$data){
         return ( scalar(@$data), $data );
     }
-    $sth->finish;
 
     if ( $type eq "simple" )    # simple search for one letter only
     {
@@ -229,16 +229,6 @@ sub SearchMember {
         }
         $query = $query . ") OR cardnumber LIKE ? ";
         push( @bind, $searchstring );
-        if (C4::Context->preference('ExtendedPatronAttributes')) {
-            $query .= "OR borrowernumber IN (
-SELECT borrowernumber
-FROM borrower_attributes
-JOIN borrower_attribute_types USING (code)
-WHERE staff_searchable = 1
-AND attribute like ?
-)";
-            push (@bind, $searchstring);
-        }
         $query .= "order by $orderby";
 
         # FIXME - .= <<EOT;
@@ -251,10 +241,60 @@ AND attribute like ?
     my @results;
     $data = $sth->fetchall_arrayref({});
 
-    $sth->finish;
     return ( scalar(@$data), $data );
 }
 
+=head2 Search
+
+  $borrowers_result_array_ref = &Search($filter,$orderby, $limit, 
+                       $columns_out, $search_on_fields,$searchtype);
+
+Looks up patrons (borrowers) on filter.
+
+BUGFIX 499: C<$type> is now used to determine type of search.
+if $type is "simple", search is performed on the first letter of the
+surname only.
+
+$category_type is used to get a specified type of user. 
+(mainly adults when creating a child.)
+
+C<$filter> can be
+   - a space-separated list of search terms. Implicit AND is done on them
+   - a hash ref containing fieldnames associated with queried value
+   - an array ref combining the two previous elements Implicit OR is done between each array element
+
+
+C<$orderby> is an arrayref of hashref. Contains the name of the field and 0 or 1 depending if order is ascending or descending
+
+C<$limit> is there to allow limiting number of results returned
+
+C<&columns_out> is an array ref to the fieldnames you want to see in the result list
+
+C<&search_on_fields> is an array ref to the fieldnames you want to limit search on when you are using string search
+
+C<&searchtype> is a string telling the type of search you want todo : start_with, exact or contains are allowed
+
+=cut
+
+sub Search {
+    my ($filter,$orderby, $limit, $columns_out, $search_on_fields,$searchtype) = @_;
+       my @filters;
+       if (ref($filter) eq "ARRAY"){
+               push @filters,@$filter;
+       }
+       else {
+               push @filters,$filter;
+       }
+    if (C4::Context->preference('ExtendedPatronAttributes')) {
+               my $matching_records = C4::Members::Attributes::SearchIdMatchingAttribute($filter);
+               push @filters,@$matching_records;
+    }
+       $searchtype||="start_with";
+       my $data=SearchInTable("borrowers",\@filters,$orderby,$limit,$columns_out,$search_on_fields,$searchtype);
+
+    return ( $data );
+}
+
 =head2 GetMemberDetails
 
 ($borrower) = &GetMemberDetails($borrowernumber, $cardnumber);
@@ -317,7 +357,6 @@ sub GetMemberDetails {
             $accessflagshash->{$flag} = 1;
         }
     }
-    $sth->finish;
     $borrower->{'flags'}     = $flags;
     $borrower->{'authflags'} = $accessflagshash;
 
@@ -335,9 +374,9 @@ sub GetMemberDetails {
 
  $flags = &patronflags($patron);
 
- This function is not exported.
+This function is not exported.
 
- The following will be set where applicable:
+The following will be set where applicable:
  $flags->{CHARGES}->{amount}        Amount of debt
  $flags->{CHARGES}->{noissues}      Set if debt amount >$5.00 (or syspref noissuescharge)
  $flags->{CHARGES}->{message}       Message -- deprecated
@@ -369,17 +408,17 @@ sub GetMemberDetails {
  $flags->{WAITING}->{message}       Message -- deprecated
  $flags->{WAITING}->{itemlist}      ref-to-array: list of available items
 
-=over 4
+=over 
 
-C<$flags-E<gt>{ODUES}-E<gt>{itemlist}> is a reference-to-array listing the
+=item C<$flags-E<gt>{ODUES}-E<gt>{itemlist}> is a reference-to-array listing the
 overdue items. Its elements are references-to-hash, each describing an
 overdue item. The keys are selected fields from the issues, biblio,
 biblioitems, and items tables of the Koha database.
 
-C<$flags-E<gt>{ODUES}-E<gt>{itemlisttext}> is a string giving a text listing of
+=item C<$flags-E<gt>{ODUES}-E<gt>{itemlisttext}> is a string giving a text listing of
 the overdue items, one per line.  Deprecated.
 
-C<$flags-E<gt>{WAITING}-E<gt>{itemlist}> is a reference-to-array listing the
+=item C<$flags-E<gt>{WAITING}-E<gt>{itemlist}> is a reference-to-array listing the
 available items. Each element is a reference-to-hash whose keys are
 fields from the reserves table of the Koha database.
 
@@ -473,78 +512,101 @@ sub patronflags {
 
   $borrower = &GetMember(%information);
 
-Looks up information about a patron (borrower) by either card number
-,firstname, or borrower number, depending on $type value.
-If C<$type> == 'cardnumber', C<&GetBorrower>
-searches by cardnumber then by firstname if not found in cardnumber; 
-otherwise, it searches by borrowernumber.
+Retrieve the first patron record meeting on criteria listed in the
+C<%information> hash, which should contain one or more
+pairs of borrowers column names and values, e.g.,
+
+   $borrower = GetMember(borrowernumber => id);
 
 C<&GetBorrower> returns a reference-to-hash whose keys are the fields of
 the C<borrowers> table in the Koha database.
 
+FIXME: GetMember() is used throughout the code as a lookup
+on a unique key such as the borrowernumber, but this meaning is not
+enforced in the routine itself.
+
 =cut
 
 #'
 sub GetMember {
     my ( %information ) = @_;
+    if (exists $information{borrowernumber} && !defined $information{borrowernumber}) {
+        #passing mysql's kohaadmin?? Makes no sense as a query
+        return;
+    }
     my $dbh = C4::Context->dbh;
-    my $sth;
-    my $select = "
-SELECT borrowers.*, categories.category_type, categories.description
-FROM borrowers 
-LEFT JOIN categories on borrowers.categorycode=categories.categorycode 
-";
-    $select.=" WHERE ".join(" AND ",map {"$_ = ?"}keys %information);
-    $select=~s/AND $//;
+    my $select =
+    q{SELECT borrowers.*, categories.category_type, categories.description
+    FROM borrowers 
+    LEFT JOIN categories on borrowers.categorycode=categories.categorycode WHERE };
+    my $more_p = 0;
+    my @values = ();
+    for (keys %information ) {
+        if ($more_p) {
+            $select .= ' AND ';
+        }
+        else {
+            $more_p++;
+        }
+
+        if (defined $information{$_}) {
+            $select .= "$_ = ?";
+            push @values, $information{$_};
+        }
+        else {
+            $select .= "$_ IS NULL";
+        }
+    }
     $debug && warn $select, " ",values %information;
-    $sth = $dbh->prepare("$select");
+    my $sth = $dbh->prepare("$select");
     $sth->execute(map{$information{$_}} keys %information);
     my $data = $sth->fetchall_arrayref({});
-    return undef if (scalar(@$data)==0);        
-    if (scalar(@$data)==1) {return $$data[0];}
-    ($data) and return $data;
+    #FIXME interface to this routine now allows generation of a result set
+    #so whole array should be returned but bowhere in the current code expects this
+    if (@{$data} ) {
+        return $data->[0];
+    }
+
+    return;
 }
 
 
 =head2 IsMemberBlocked
 
-=over 4
+  my ($block_status, $count) = IsMemberBlocked( $borrowernumber );
 
-my $blocked = IsMemberBlocked( $borrowernumber );
+Returns whether a patron has overdue items that may result
+in a block or whether the patron has active fine days
+that would block circulation privileges.
 
-return the status, and the number of day or documents, depends his punishment
+C<$block_status> can have the following values:
 
-return :
--1 if the user have overdue returns
-1 if the user is punished X days
-0 if the user is authorised to loan
+1 if the patron has outstanding fine days, in which case C<$count> is the number of them
 
-=back
+-1 if the patron has overdue items, in which case C<$count> is the number of them
+
+0 if the patron has no overdue items or outstanding fine days, in which case C<$count> is 0
+
+Outstanding fine days are checked before current overdue items
+are.
+
+FIXME: this needs to be split into two functions; a potential block
+based on the number of current overdue items could be orthogonal
+to a block based on whether the patron has any fine days accrued.
 
 =cut
 
 sub IsMemberBlocked {
     my $borrowernumber = shift;
     my $dbh            = C4::Context->dbh;
-    # if he have late issues
-    my $sth = $dbh->prepare(
-        "SELECT COUNT(*) as latedocs
-         FROM issues
-         WHERE borrowernumber = ?
-         AND date_due < now()"
-    );
-    $sth->execute($borrowernumber);
-    my $latedocs = $sth->fetchrow_hashref->{'latedocs'};
-
-    return (-1, $latedocs) if $latedocs > 0;
 
+    # does patron have current fine days?
        my $strsth=qq{
             SELECT
             ADDDATE(returndate, finedays * DATEDIFF(returndate,date_due) ) AS blockingdate,
             DATEDIFF(ADDDATE(returndate, finedays * DATEDIFF(returndate,date_due)),NOW()) AS blockedcount
             FROM old_issues
        };
-    # or if he must wait to loan
     if(C4::Context->preference("item-level_itypes")){
         $strsth.=
                qq{ LEFT JOIN items ON (items.itemnumber=old_issues.itemnumber)
@@ -561,7 +623,7 @@ sub IsMemberBlocked {
             AND borrowernumber = ?
             ORDER BY blockingdate DESC, blockedcount DESC
             LIMIT 1};
-       $sth=$dbh->prepare($strsth);
+       my $sth=$dbh->prepare($strsth);
     $sth->execute($borrowernumber);
     my $row = $sth->fetchrow_hashref;
     my $blockeddate  = $row->{'blockeddate'};
@@ -569,7 +631,19 @@ sub IsMemberBlocked {
 
     return (1, $blockedcount) if $blockedcount > 0;
 
-    return 0
+    # if he have late issues
+    $sth = $dbh->prepare(
+        "SELECT COUNT(*) as latedocs
+         FROM issues
+         WHERE borrowernumber = ?
+         AND date_due < curdate()"
+    );
+    $sth->execute($borrowernumber);
+    my $latedocs = $sth->fetchrow_hashref->{'latedocs'};
+
+    return (-1, $latedocs) if $latedocs > 0;
+
+    return (0, 0);
 }
 
 =head2 GetMemberIssuesAndFines
@@ -596,21 +670,18 @@ sub GetMemberIssuesAndFines {
     my $sth = $dbh->prepare($query);
     $sth->execute($borrowernumber);
     my $issue_count = $sth->fetchrow_arrayref->[0];
-    $sth->finish;
 
     $sth = $dbh->prepare(
         "SELECT COUNT(*) FROM issues 
          WHERE borrowernumber = ? 
-         AND date_due < now()"
+         AND date_due < curdate()"
     );
     $sth->execute($borrowernumber);
     my $overdue_count = $sth->fetchrow_arrayref->[0];
-    $sth->finish;
 
     $sth = $dbh->prepare("SELECT SUM(amountoutstanding) FROM accountlines WHERE borrowernumber = ?");
     $sth->execute($borrowernumber);
     my $total_fines = $sth->fetchrow_arrayref->[0];
-    $sth->finish;
 
     return ($overdue_count, $issue_count, $total_fines);
 }
@@ -619,22 +690,18 @@ sub columns(;$) {
     return @{C4::Context->dbh->selectcol_arrayref("SHOW columns from borrowers")};
 }
 
-=head2
-
 =head2 ModMember
 
-=over 4
-
-my $success = ModMember(borrowernumber => $borrowernumber, [ field => value ]... );
+  my $success = ModMember(borrowernumber => $borrowernumber,
+                                            [ field => value ]... );
 
 Modify borrower's data.  All date fields should ALREADY be in ISO format.
 
 return :
 true on success, or false on failure
 
-=back
-
 =cut
+
 sub ModMember {
     my (%data) = @_;
     # test to know if you must update or not the borrower password
@@ -661,8 +728,6 @@ sub ModMember {
 }
 
 
-=head2
-
 =head2 AddMember
 
   $borrowernumber = &AddMember(%borrower);
@@ -676,7 +741,7 @@ Returns the borrowernumber
 sub AddMember {
     my (%data) = @_;
     my $dbh = C4::Context->dbh;
-    $data{'userid'} = '' unless $data{'password'};
+    $data{'password'} = '!' if (not $data{'password'} and $data{'userid'});
     $data{'password'} = md5_base64( $data{'password'} ) if $data{'password'};
        $data{'borrowernumber'}=InsertInTable("borrowers",\%data);      
     # mysql_insertid is probably bad.  not necessarily accurate and mysql-specific at best.
@@ -857,7 +922,6 @@ sub GetGuarantees {
 
     my @dat;
     my $data = $sth->fetchall_arrayref({}); 
-    $sth->finish;
     return ( scalar(@$data), $data );
 }
 
@@ -889,7 +953,6 @@ sub UpdateGuarantees {
         |;
         my $sth3 = $dbh->prepare($guaquery);
         $sth3->execute;
-        $sth3->finish;
     }
 }
 =head2 GetPendingIssues
@@ -952,7 +1015,7 @@ sub GetPendingIssues {
 
 =head2 GetAllIssues
 
-  ($count, $issues) = &GetAllIssues($borrowernumber, $sortkey, $limit);
+  $issues = &GetAllIssues($borrowernumber, $sortkey, $limit);
 
 Looks up what the patron with the given borrowernumber has borrowed,
 and sorts the results.
@@ -963,11 +1026,9 @@ C<biblioitems>, or C<items> table in the Koha database.
 
 C<$limit> is the maximum number of results to return.
 
-C<&GetAllIssues> returns a two-element array. C<$issues> is a
-reference-to-array, where each element is a reference-to-hash; the
-keys are the fields from the C<issues>, C<biblio>, C<biblioitems>, and
-C<items> tables of the Koha database. C<$count> is the number of
-elements in C<$issues>
+C<&GetAllIssues> an arrayref, C<$issues>, of hashrefs, the keys of which
+are the fields from the C<issues>, C<biblio>, C<biblioitems>, and
+C<items> tables of the Koha database.
 
 =cut
 
@@ -977,16 +1038,15 @@ sub GetAllIssues {
 
     #FIXME: sanity-check order and limit
     my $dbh   = C4::Context->dbh;
-    my $count = 0;
     my $query =
-  "SELECT *,issues.renewals AS renewals,items.renewals AS totalrenewals,items.timestamp AS itemstimestamp 
+  "SELECT *, issues.timestamp as issuestimestamp, issues.renewals AS renewals,items.renewals AS totalrenewals,items.timestamp AS itemstimestamp 
   FROM issues 
   LEFT JOIN items on items.itemnumber=issues.itemnumber
   LEFT JOIN biblio ON items.biblionumber=biblio.biblionumber
   LEFT JOIN biblioitems ON items.biblioitemnumber=biblioitems.biblioitemnumber
   WHERE borrowernumber=? 
   UNION ALL
-  SELECT *,old_issues.renewals AS renewals,items.renewals AS totalrenewals,items.timestamp AS itemstimestamp 
+  SELECT *, old_issues.timestamp as issuestimestamp, old_issues.renewals AS renewals,items.renewals AS totalrenewals,items.timestamp AS itemstimestamp 
   FROM old_issues 
   LEFT JOIN items on items.itemnumber=old_issues.itemnumber
   LEFT JOIN biblio ON items.biblionumber=biblio.biblionumber
@@ -997,48 +1057,15 @@ sub GetAllIssues {
         $query .= " limit $limit";
     }
 
-    #print $query;
     my $sth = $dbh->prepare($query);
     $sth->execute($borrowernumber, $borrowernumber);
     my @result;
     my $i = 0;
     while ( my $data = $sth->fetchrow_hashref ) {
-        $result[$i] = $data;
-        $i++;
-        $count++;
-    }
-
-    # get all issued items for borrowernumber from oldissues table
-    # large chunk of older issues data put into table oldissues
-    # to speed up db calls for issuing items
-    if ( C4::Context->preference("ReadingHistory") ) {
-        # FIXME oldissues (not to be confused with old_issues) is
-        # apparently specific to HLT.  Not sure if the ReadingHistory
-        # syspref is still required, as old_issues by design
-        # is no longer checked with each loan.
-        my $query2 = "SELECT * FROM oldissues
-                      LEFT JOIN items ON items.itemnumber=oldissues.itemnumber
-                      LEFT JOIN biblio ON items.biblionumber=biblio.biblionumber
-                      LEFT JOIN biblioitems ON items.biblioitemnumber=biblioitems.biblioitemnumber
-                      WHERE borrowernumber=? 
-                      ORDER BY $order";
-        if ( $limit != 0 ) {
-            $limit = $limit - $count;
-            $query2 .= " limit $limit";
-        }
-
-        my $sth2 = $dbh->prepare($query2);
-        $sth2->execute($borrowernumber);
-
-        while ( my $data2 = $sth2->fetchrow_hashref ) {
-            $result[$i] = $data2;
-            $i++;
-        }
-        $sth2->finish;
+        push @result, $data;
     }
-    $sth->finish;
 
-    return ( $i, \@result );
+    return \@result;
 }
 
 
@@ -1084,7 +1111,6 @@ sub GetMemberAccountRecords {
         $total += int(1000 * $data->{'amountoutstanding'}); # convert float to integer to avoid round-off errors
     }
     $total /= 1000;
-    $sth->finish;
     return ( $total, \@acctlines,$numlines);
 }
 
@@ -1127,7 +1153,6 @@ sub GetBorNotifyAcctRecord {
         $total += int(100 * $data->{'amountoutstanding'});
     }
     $total /= 100;
-    $sth->finish;
     return ( $total, \@acctlines, $numlines );
 }
 
@@ -1166,7 +1191,6 @@ sub checkuniquemember {
         $sth->execute( uc($surname), ucfirst($firstname));
     }
     my @data = $sth->fetchrow;
-    $sth->finish;
     ( $data[0] ) and return $data[0], $data[1];
     return 0;
 }
@@ -1188,7 +1212,6 @@ sub checkcardnumber {
     else {
         return 0;
     }
-    $sth->finish();
 }  
 
 
@@ -1226,6 +1249,33 @@ sub getidcity {
     return $data;
 }
 
+=head2 GetFirstValidEmailAddress
+
+  $email = GetFirstValidEmailAddress($borrowernumber);
+
+Return the first valid email address for a borrower, given the borrowernumber.  For now, the order 
+is defined as email, emailpro, B_email.  Returns the empty string if the borrower has no email 
+addresses.
+
+=cut
+
+sub GetFirstValidEmailAddress {
+    my $borrowernumber = shift;
+    my $dbh = C4::Context->dbh;
+    my $sth = $dbh->prepare( "SELECT email, emailpro, B_email FROM borrowers where borrowernumber = ? ");
+    $sth->execute( $borrowernumber );
+    my $data = $sth->fetchrow_hashref;
+
+    if ($data->{'email'}) {
+       return $data->{'email'};
+    } elsif ($data->{'emailpro'}) {
+       return $data->{'emailpro'};
+    } elsif ($data->{'B_email'}) {
+       return $data->{'B_email'};
+    } else {
+       return '';
+    }
+}
 
 =head2 GetExpiryDate 
 
@@ -1238,16 +1288,20 @@ Return date is also in ISO format.
 
 sub GetExpiryDate {
     my ( $categorycode, $dateenrolled ) = @_;
-    my $enrolmentperiod = 12;   # reasonable default
+    my $enrolments;
     if ($categorycode) {
         my $dbh = C4::Context->dbh;
-        my $sth = $dbh->prepare("select enrolmentperiod from categories where categorycode=?");
+        my $sth = $dbh->prepare("SELECT enrolmentperiod,enrolmentperioddate FROM categories WHERE categorycode=?");
         $sth->execute($categorycode);
-        $enrolmentperiod = $sth->fetchrow;
+        $enrolments = $sth->fetchrow_hashref;
     }
     # die "GetExpiryDate: for enrollmentperiod $enrolmentperiod (category '$categorycode') starting $dateenrolled.\n";
-    my @date = split /-/,$dateenrolled;
-    return sprintf("%04d-%02d-%02d", Add_Delta_YM(@date,0,$enrolmentperiod));
+    my @date = split (/-/,$dateenrolled);
+    if($enrolments->{enrolmentperiod}){
+        return sprintf("%04d-%02d-%02d", Add_Delta_YM(@date,0,$enrolments->{enrolmentperiod}));
+    }else{
+        return $enrolments->{enrolmentperioddate};
+    }
 }
 
 =head2 checkuserpassword (OUEST-PROVENCE)
@@ -1307,7 +1361,6 @@ sub GetborCatFromCatType {
         push @codes, $data->{'categorycode'};
         $labels{ $data->{'categorycode'} } = $data->{'description'};
     }
-    $sth->finish;
     return ( \@codes, \%labels );
 }
 
@@ -1317,8 +1370,9 @@ sub GetborCatFromCatType {
 
 Given the borrower's category code, the function returns the corresponding
 data hashref for a comprehensive information display.
-  
+
   $arrayref_hashref = &GetBorrowercategory;
+
 If no category code provided, the function returns all the categories.
 
 =cut
@@ -1336,14 +1390,13 @@ sub GetBorrowercategory {
         $sth->execute($catcode);
         my $data =
         $sth->fetchrow_hashref;
-        $sth->finish();
         return $data;
     } 
     return;  
 }    # sub getborrowercategory
 
 =head2 GetBorrowercategoryList
+
   $arrayref_hashref = &GetBorrowercategoryList;
 If no category code provided, the function returns all the categories.
 
@@ -1360,7 +1413,6 @@ sub GetBorrowercategoryList {
     $sth->execute;
     my $data =
     $sth->fetchall_arrayref({});
-    $sth->finish();
     return $data;
 }    # sub getborrowercategory
 
@@ -1387,7 +1439,6 @@ sub ethnicitycategories {
         push @codes, $data->{'code'};
         $labels{ $data->{'code'} } = $data->{'name'};
     }
-    $sth->finish;
     return ( \@codes, \%labels );
 }
 
@@ -1410,7 +1461,6 @@ sub fixEthnicity {
     my $sth       = $dbh->prepare("Select name from ethnicity where code = ?");
     $sth->execute($ethnicity);
     my $data = $sth->fetchrow_hashref;
-    $sth->finish;
     return $data->{'name'};
 }    # sub fixEthnicity
 
@@ -1442,6 +1492,7 @@ sub GetAge{
 }    # sub get_age
 
 =head2 get_institutions
+
   $insitutions = get_institutions();
 
 Just returns a list of all the borrowers of type I, borrownumber and name
@@ -1460,7 +1511,6 @@ sub get_institutions {
     while ( my $data = $sth->fetchrow_hashref() ) {
         $orgs{ $data->{'borrowernumber'} } = $data;
     }
-    $sth->finish();
     return ( \%orgs );
 
 }    # sub get_institutions
@@ -1483,53 +1533,34 @@ sub add_member_orgs {
     foreach my $otherborrowernumber (@$otherborrowers) {
         $sth->execute( $borrowernumber, $otherborrowernumber );
     }
-    $sth->finish();
 
 }    # sub add_member_orgs
 
-=head2 GetCities (OUEST-PROVENCE)
+=head2 GetCities
 
-  ($id_cityarrayref, $city_hashref) = &GetCities();
+  $cityarrayref = GetCities();
 
-Looks up the different city and zip in the database. Returns two
-elements: a reference-to-array, which lists the zip city
-codes, and a reference-to-hash, which maps the name of the city.
-WHERE =>OUEST PROVENCE OR EXTERIEUR
+  Returns an array_ref of the entries in the cities table
+  If there are entries in the table an empty row is returned
+  This is currently only used to populate a popup in memberentry
 
 =cut
 
 sub GetCities {
 
-    #my ($type_city) = @_;
     my $dbh   = C4::Context->dbh;
-    my $query = qq|SELECT cityid,city_zipcode,city_name 
-        FROM cities 
-        ORDER BY city_name|;
-    my $sth = $dbh->prepare($query);
-
-    #$sth->execute($type_city);
-    $sth->execute();
-    my %city;
-    my @id;
-    #    insert empty value to create a empty choice in cgi popup
-    push @id, " ";
-    $city{""} = "";
-    while ( my $data = $sth->fetchrow_hashref ) {
-        push @id, $data->{'city_zipcode'}."|".$data->{'city_name'};
-        $city{ $data->{'city_zipcode'}."|".$data->{'city_name'} } = $data->{'city_name'};
-    }
-
-#test to know if the table contain some records if no the function return nothing
-    my $id = @id;
-    $sth->finish;
-    if ( $id == 1 ) {
-        # all we have is the one blank row
-        return ();
-    }
-    else {
-        unshift( @id, "" );
-        return ( \@id, \%city );
-    }
+    my $city_arr = $dbh->selectall_arrayref(
+        q|SELECT cityid,city_zipcode,city_name FROM cities ORDER BY city_name|,
+        { Slice => {} });
+    if ( @{$city_arr} ) {
+        unshift @{$city_arr}, {
+            city_zipcode => q{},
+            city_name    => q{},
+            cityid       => q{},
+        };
+    }
+
+    return  $city_arr;
 }
 
 =head2 GetSortDetails (OUEST-PROVENCE)
@@ -1587,7 +1618,7 @@ sub MoveMemberToDeleted {
 
 =head2 DelMember
 
-DelMember($borrowernumber);
+    DelMember($borrowernumber);
 
 This function remove directly a borrower whitout writing it on deleteborrower.
 + Deletes reserves for the borrower
@@ -1605,7 +1636,6 @@ sub DelMember {
           WHERE borrowernumber=?|;
     my $sth = $dbh->prepare($query);
     $sth->execute($borrowernumber);
-    $sth->finish;
     $query = "
        DELETE
        FROM borrowers
@@ -1613,7 +1643,6 @@ sub DelMember {
    ";
     $sth = $dbh->prepare($query);
     $sth->execute($borrowernumber);
-    $sth->finish;
     logaction("MEMBERS", "DELETE", $borrowernumber, "") if C4::Context->preference("BorrowersLog");
     return $sth->rows;
 }
@@ -1683,7 +1712,6 @@ ORDER BY road_type|;
 
 #test to know if the table contain some records if no the function return nothing
     my $id = @id;
-    $sth->finish;
     if ( $id eq 0 ) {
         return ();
     }
@@ -1704,7 +1732,7 @@ Looks up the different title . Returns array  with all borrowers title
 =cut
 
 sub GetTitles {
-    my @borrowerTitle = split /,|\|/,C4::Context->preference('BorrowersTitles');
+    my @borrowerTitle = split (/,|\|/,C4::Context->preference('BorrowersTitles'));
     unshift( @borrowerTitle, "" );
     my $count=@borrowerTitle;
     if ($count == 1){
@@ -1800,16 +1828,15 @@ WHERE roadtypeid=?|;
 
 =head2 GetBorrowersWhoHaveNotBorrowedSince
 
-&GetBorrowersWhoHaveNotBorrowedSince($date)
+  &GetBorrowersWhoHaveNotBorrowedSince($date)
 
 this function get all borrowers who haven't borrowed since the date given on input arg.
-      
+
 =cut
 
 sub GetBorrowersWhoHaveNotBorrowedSince {
-### TODO : It could be dangerous to delete Borrowers who have just been entered and who have not yet borrowed any book. May be good to add a dateexpiry or dateenrolled filter.      
-       
-                my $filterdate = shift||POSIX::strftime("%Y-%m-%d",localtime());
+    my $filterdate = shift||POSIX::strftime("%Y-%m-%d",localtime());
+    my $filterexpiry = shift;
     my $filterbranch = shift || 
                         ((C4::Context->preference('IndependantBranches') 
                              && C4::Context->userenv 
@@ -1819,20 +1846,29 @@ sub GetBorrowersWhoHaveNotBorrowedSince {
                          : "");  
     my $dbh   = C4::Context->dbh;
     my $query = "
-        SELECT borrowers.borrowernumber,max(issues.timestamp) as latestissue
+        SELECT borrowers.borrowernumber,
+               max(old_issues.timestamp) as latestissue,
+               max(issues.timestamp) as currentissue
         FROM   borrowers
         JOIN   categories USING (categorycode)
-        LEFT JOIN issues ON borrowers.borrowernumber = issues.borrowernumber
+        LEFT JOIN old_issues USING (borrowernumber)
+        LEFT JOIN issues USING (borrowernumber) 
         WHERE  category_type <> 'S'
+        AND borrowernumber NOT IN (SELECT guarantorid FROM borrowers WHERE guarantorid IS NOT NULL AND guarantorid <> 0) 
    ";
     my @query_params;
     if ($filterbranch && $filterbranch ne ""){ 
         $query.=" AND borrowers.branchcode= ?";
         push @query_params,$filterbranch;
-    }    
+    }
+    if($filterexpiry){
+        $query .= " AND dateexpiry < ? ";
+        push @query_params,$filterdate;
+    }
     $query.=" GROUP BY borrowers.borrowernumber";
     if ($filterdate){ 
-        $query.=" HAVING latestissue <? OR latestissue IS NULL";
+        $query.=" HAVING (latestissue < ? OR latestissue IS NULL) 
+                  AND currentissue IS NULL";
         push @query_params,$filterdate;
     }
     warn $query if $debug;
@@ -1853,9 +1889,9 @@ sub GetBorrowersWhoHaveNotBorrowedSince {
 
 =head2 GetBorrowersWhoHaveNeverBorrowed
 
-$results = &GetBorrowersWhoHaveNeverBorrowed
+  $results = &GetBorrowersWhoHaveNeverBorrowed
 
-this function get all borrowers who have never borrowed.
+This function get all borrowers who have never borrowed.
 
 I<$result> is a ref to an array which all elements are a hasref.
 
@@ -1900,7 +1936,7 @@ sub GetBorrowersWhoHaveNeverBorrowed {
 
 =head2 GetBorrowersWithIssuesHistoryOlderThan
 
-$results = &GetBorrowersWithIssuesHistoryOlderThan($date)
+  $results = &GetBorrowersWithIssuesHistoryOlderThan($date)
 
 this function get all borrowers who has an issue history older than I<$date> given on input arg.
 
@@ -1945,7 +1981,7 @@ sub GetBorrowersWithIssuesHistoryOlderThan {
 
 =head2 GetBorrowersNamesAndLatestIssue
 
-$results = &GetBorrowersNamesAndLatestIssueList(@borrowernumbers)
+  $results = &GetBorrowersNamesAndLatestIssueList(@borrowernumbers)
 
 this function get borrowers Names and surnames and Issue information.
 
@@ -1971,9 +2007,7 @@ sub GetBorrowersNamesAndLatestIssue {
 
 =head2 DebarMember
 
-=over 4
-
-my $success = DebarMember( $borrowernumber );
+  my $success = DebarMember( $borrowernumber );
 
 marks a Member as debarred, and therefore unable to checkout any more
 items.
@@ -1981,8 +2015,6 @@ items.
 return :
 true on success, false on failure
 
-=back
-
 =cut
 
 sub DebarMember {
@@ -1998,9 +2030,7 @@ sub DebarMember {
 
 =head2 AddMessage
 
-=over 4
-
-AddMessage( $borrowernumber, $message_type, $message, $branchcode );
+  AddMessage( $borrowernumber, $message_type, $message, $branchcode );
 
 Adds a message to the messages table for the given borrower.
 
@@ -2008,8 +2038,6 @@ Returns:
   True on success
   False on failure
 
-=back
-
 =cut
 
 sub AddMessage {
@@ -2030,17 +2058,13 @@ sub AddMessage {
 
 =head2 GetMessages
 
-=over 4
-
-GetMessages( $borrowernumber, $type );
+  GetMessages( $borrowernumber, $type );
 
 $type is message type, B for borrower, or L for Librarian.
 Empty type returns all messages of any type.
 
 Returns all messages for the given borrowernumber
 
-=back
-
 =cut
 
 sub GetMessages {
@@ -2075,17 +2099,13 @@ sub GetMessages {
 
 =head2 GetMessages
 
-=over 4
-
-GetMessagesCount( $borrowernumber, $type );
+  GetMessagesCount( $borrowernumber, $type );
 
 $type is message type, B for borrower, or L for Librarian.
 Empty type returns all messages of any type.
 
 Returns the number of messages for the given borrowernumber
 
-=back
-
 =cut
 
 sub GetMessagesCount {
@@ -2112,11 +2132,7 @@ sub GetMessagesCount {
 
 =head2 DeleteMessage
 
-=over 4
-
-DeleteMessage( $message_id );
-
-=back
+  DeleteMessage( $message_id );
 
 =cut