From 18b8991cf1832a0ee79a526e6438b51fdb4f3208 Mon Sep 17 00:00:00 2001 From: Srdjan Jankovic Date: Thu, 22 Sep 2011 09:43:12 -0500 Subject: [PATCH] bug_6253: Unified member Search() Removed SearchMembers() and replaced with more generic Search() Amended Search() to try cardnumber first Replaced SearchMembers() calls with Search() Replaced SELECT with Search() where appropriate C4::SQLHelper: - added support for '' key for search filter. - when passing an array to filter, join with OR (rather than AND) - added support for key => [val1, val2] in filter - did not document - there was no input documentation to start with, and SQLHelper should be replaced with something better anyway Signed-off-by: Liz Rea (again - testing merge issue) The functionality of the patch seems to be maintained with Biblibre's changes. I tested the following: Extended attribute searching: works 3 part name searching: works 2 part name searching: works 1 part name searching: works From: mainpage.pl members-home.pl Patron search limited by branch: Works Patron search limited by patron category: works Ordering by cardnumber instead of surname: works The "Check Out" field in the masthead. Circ Autocomplete is not reliably functional at this time, but the problem appears to predate this patch. Signed-off-by: Ian Walls Signed-off-by: Chris Cormack --- C4/Members.pm | 256 +++++++++++-------------- C4/Members/Attributes.pm | 13 +- C4/SQLHelper.pm | 77 ++++++-- admin/aqbudget_owner_search.pl | 2 +- circ/circulation.pl | 2 +- circ/ysearch.pl | 29 +-- members/guarantor_search.pl | 11 +- members/member.pl | 19 +- patroncards/members-search.pl | 14 +- reserve/request.pl | 20 +- t/db_dependent/Koha.t | 14 +- t/db_dependent/Members.t | 181 ++++++++++++----- t/db_dependent/lib/KohaTest/Members.pm | 2 +- 13 files changed, 357 insertions(+), 283 deletions(-) diff --git a/C4/Members.pm b/C4/Members.pm index 1730d6cb62..56718f08f4 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -44,7 +44,6 @@ BEGIN { #Get data push @EXPORT, qw( &Search - &SearchMember &GetMemberDetails &GetMemberRelatives &GetMember @@ -141,178 +140,139 @@ This module contains routines for adding, modifying and deleting members/patrons =head1 FUNCTIONS -=head2 SearchMember - - ($count, $borrowers) = &SearchMember($searchstring, $type, - $category_type, $filter, $showallbranches); +=head2 Search -Looks up patrons (borrowers) by name. + $borrowers_result_array_ref = &Search($filter,$orderby, $limit, + $columns_out, $search_on_fields,$searchtype); -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. +Looks up patrons (borrowers) on filter. A wrapper for SearchInTable('borrowers'). -$category_type is used to get a specified type of user. -(mainly adults when creating a child.) +For C<$filter>, C<$orderby>, C<$limit>, C<&columns_out>, C<&search_on_fields> and C<&searchtype> +refer to C4::SQLHelper:SearchInTable(). -C<$searchstring> is a space-separated list of search terms. Each term -must match the beginning a borrower's surname, first name, or other -name. +Special C<$filter> key '' is effectively expanded to search on surname firstname othernamescw +and cardnumber unless C<&search_on_fields> is defined -C<$filter> is assumed to be a list of elements to filter results on +Examples: -C<$showallbranches> is used in IndependantBranches Context to display all branches results. + $borrowers = Search('abcd', 'cardnumber'); -C<&SearchMember> returns a two-element list. C<$borrowers> is a -reference-to-array; each element is a reference-to-hash, whose keys -are the fields of the C table in the Koha database. -C<$count> is the number of elements in C<$borrowers>. + $borrowers = Search({''=>'abcd', category_type=>'I'}, 'surname'); =cut -#' -#used by member enquiries from the intranet -sub SearchMember { - my ($searchstring, $orderby, $type,$category_type,$filter,$showallbranches ) = @_; - my $dbh = C4::Context->dbh; - my $query = ""; - my $count; - my @data; - my @bind = (); - +sub _express_member_find { + my ($filter) = @_; + # this is used by circulation everytime a new borrowers cardnumber is scanned # so we can check an exact match first, if that works return, otherwise do the rest - $query = "SELECT * FROM borrowers - LEFT JOIN categories ON borrowers.categorycode=categories.categorycode - "; - my $sth = $dbh->prepare("$query WHERE cardnumber = ?"); - $sth->execute($searchstring); - my $data = $sth->fetchall_arrayref({}); - if (@$data){ - return ( scalar(@$data), $data ); - } - - if ( $type eq "simple" ) # simple search for one letter only - { - $query .= ($category_type ? " AND category_type = ".$dbh->quote($category_type) : ""); - $query .= " WHERE (surname LIKE ? OR cardnumber like ?) "; - if (C4::Context->preference("IndependantBranches") && !$showallbranches){ - if (C4::Context->userenv && C4::Context->userenv->{flags} % 2 !=1 && C4::Context->userenv->{'branch'}){ - $query.=" AND borrowers.branchcode =".$dbh->quote(C4::Context->userenv->{'branch'}) unless (C4::Context->userenv->{'branch'} eq "insecure"); - } - } - $query.=" ORDER BY $orderby"; - @bind = ("$searchstring%","$searchstring"); + my $dbh = C4::Context->dbh; + my $query = "SELECT borrowernumber FROM borrowers WHERE cardnumber = ?"; + if ( my $borrowernumber = $dbh->selectrow_array($query, undef, $filter) ) { + return( {"borrowernumber"=>$borrowernumber} ); } - else # advanced search looking in surname, firstname and othernames - { - @data = split( ' ', $searchstring ); - $count = @data; - $query .= " WHERE "; - if (C4::Context->preference("IndependantBranches") && !$showallbranches){ - if (C4::Context->userenv && C4::Context->userenv->{flags} % 2 !=1 && C4::Context->userenv->{'branch'}){ - $query.=" borrowers.branchcode =".$dbh->quote(C4::Context->userenv->{'branch'})." AND " unless (C4::Context->userenv->{'branch'} eq "insecure"); - } - } - $query.="((surname LIKE ? OR (surname LIKE ? AND surname REGEXP ?) - OR firstname LIKE ? OR (firstname LIKE ? AND firstname REGEXP ?) - OR othernames LIKE ? OR (othernames LIKE ? AND othernames REGEXP ?)) - " . - ($category_type?" AND category_type = ".$dbh->quote($category_type):""); - my $regex = '[[:punct:][:space:]]'.$data[0]; - @bind = ( - "$data[0]%", "%$data[0]%", $regex, - "$data[0]%", "%$data[0]%", $regex, - "$data[0]%", "%$data[0]%", $regex - ); - for ( my $i = 1 ; $i < $count ; $i++ ) { - $query = $query . " AND (" . " surname LIKE ? OR (surname LIKE ? AND surname REGEXP ?) - OR firstname LIKE ? OR (firstname LIKE ? AND firstname REGEXP ?) - OR othernames LIKE ? OR (othernames LIKE ? AND othernames REGEXP ?))"; - $regex = '[[:punct:][:space:]]'.$data[$i]; - push( @bind, - "$data[$i]%", "%$data[$i]%", $regex, - "$data[$i]%", "%$data[$i]%", $regex, - "$data[$i]%", "%$data[$i]%", $regex - ); - - - # FIXME - .= <prepare($query); - - $debug and print STDERR "Q $orderby : $query\n"; - $sth->execute(@bind); - my @results; - $data = $sth->fetchall_arrayref({}); - - return ( scalar(@$data), $data ); + return (undef, $search_on_fields, $searchtype); } -=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; - my %filtersmatching_record; - my @finalfilter; - if ( ref($filter) eq "ARRAY" ) { - push @filters, @$filter; - } else { - push @filters, $filter; + + my $search_string; + my $found_borrower; + + if ( my $fr = ref $filter ) { + if ( $fr eq "HASH" ) { + if ( my $search_string = $filter->{''} ) { + my ($member_filter, $member_search_on_fields, $member_searchtype) = _express_member_find($search_string); + if ($member_filter) { + $filter = $member_filter; + $found_borrower = 1; + } else { + $search_on_fields ||= $member_search_on_fields; + $searchtype ||= $member_searchtype; + } + } + } + else { + $search_string = $filter; + } + } + else { + $search_string = $filter; + my ($member_filter, $member_search_on_fields, $member_searchtype) = _express_member_find($search_string); + if ($member_filter) { + $filter = $member_filter; + $found_borrower = 1; + } else { + $search_on_fields ||= $member_search_on_fields; + $searchtype ||= $member_searchtype; + } } - if ( C4::Context->preference('ExtendedPatronAttributes') ) { - my $matching_records = C4::Members::Attributes::SearchIdMatchingAttribute($filter); + + if ( !$found_borrower && C4::Context->preference('ExtendedPatronAttributes') && $search_string ) { + my $matching_records = C4::Members::Attributes::SearchIdMatchingAttribute($search_string); if(scalar(@$matching_records)>0) { - foreach my $matching_record (@$matching_records) { - $filtersmatching_record{$$matching_record[0]}=1; - } - foreach my $k (keys(%filtersmatching_record)) { - push @filters, {"borrowernumber"=>$k}; - } + if ( my $fr = ref $filter ) { + if ( $fr eq "HASH" ) { + my %f = %$filter; + $filter = [ $filter ]; + delete $f{''}; + push @$filter, { %f, "borrowernumber"=>$$matching_records }; + } + else { + push @$filter, {"borrowernumber"=>$matching_records}; + } + } + else { + $filter = [ $filter ]; + push @$filter, {"borrowernumber"=>$matching_records}; + } } } + + # $showallbranches was not used at the time SearchMember() was mainstreamed into Search(). + # Mentioning for the reference + + if ( C4::Context->preference("IndependantBranches") ) { # && !$showallbranches){ + if ( my $userenv = C4::Context->userenv ) { + my $branch = $userenv->{'branch'}; + if ( ($userenv->{flags} % 2 !=1) && + $branch && $branch ne "insecure" ){ + + if (my $fr = ref $filter) { + if ( $fr eq "HASH" ) { + $filter->{branchcode} = $branch; + } + else { + foreach (@$filter) { + $_ = { '' => $_ } unless ref $_; + $_->{branchcode} = $branch; + } + } + } + else { + $filter = { '' => $filter, branchcode => $branch }; + } + } + } + } + + if ($found_borrower) { + $searchtype = "exact"; + } $searchtype ||= "start_with"; - push @finalfilter, \@filters; - my $data = SearchInTable( "borrowers", \@finalfilter, $orderby, $limit, $columns_out, $search_on_fields, $searchtype ); - return ($data); + + return SearchInTable( "borrowers", $filter, $orderby, $limit, $columns_out, $search_on_fields, $searchtype ); } =head2 GetMemberDetails diff --git a/C4/Members/Attributes.pm b/C4/Members/Attributes.pm index 29a31e6e7e..4ae5600591 100644 --- a/C4/Members/Attributes.pm +++ b/C4/Members/Attributes.pm @@ -119,23 +119,24 @@ sub GetBorrowerAttributeValue { =head2 SearchIdMatchingAttribute - my $matching_records = C4::Members::Attributes::SearchIdMatchingAttribute($filter); + my $matching_borrowernumbers = C4::Members::Attributes::SearchIdMatchingAttribute($filter); =cut sub SearchIdMatchingAttribute{ my $filter = shift; - my $finalfilter=$filter->[0]; + $filter = [$filter] unless ref $filter; + my $dbh = C4::Context->dbh(); my $query = qq{ -SELECT borrowernumber +SELECT DISTINCT borrowernumber FROM borrower_attributes JOIN borrower_attribute_types USING (code) WHERE staff_searchable = 1 -AND attribute like ?}; +AND (} . join (" OR ", map "attribute like ?", @$filter) .qq{)}; my $sth = $dbh->prepare_cached($query); - $sth->execute("%$finalfilter%"); - return $sth->fetchall_arrayref; + $sth->execute(map "%$_%", @$filter); + return [map $_->[0], @{ $sth->fetchall_arrayref }]; } =head2 CheckUniqueness diff --git a/C4/SQLHelper.pm b/C4/SQLHelper.pm index a15beb86b6..720dc998cc 100644 --- a/C4/SQLHelper.pm +++ b/C4/SQLHelper.pm @@ -85,6 +85,21 @@ $filtercolums is an array ref on field names : is used to limit expansion of res $searchtype is string Can be "start_with" or "exact" +This query builder is very limited, it should be replaced with DBIx::Class +or similar very soon +Meanwhile adding support for special key '' in case of a data_hashref to +support filters of type + + ( f1 = a OR f2 = a ) AND fx = b AND fy = c + +Call for the query above is: + + SearchInTable($tablename, {'' => a, fx => b, fy => c}, $orderby, $limit, + $columns_out, [f1, f2], 'exact'); + +NOTE: Current implementation may remove parts of the iinput hashrefs. If that is a problem +a copy needs to be created in _filter_fields() below + =cut sub SearchInTable{ @@ -101,7 +116,7 @@ sub SearchInTable{ if ($keys){ my @criteria=grep{defined($_) && $_ !~/^\W$/ }@$keys; if (@criteria) { - $sql.= do { local $"=') AND ('; + $sql.= do { local $"=') OR ('; qq{ WHERE (@criteria) } }; } @@ -109,8 +124,12 @@ sub SearchInTable{ if ($orderby){ #Order by desc by default my @orders; - foreach my $order (@$orderby){ - push @orders,map{ "$_".($order->{$_}? " DESC " : "") } keys %$order; + foreach my $order ( ref($orderby) ? @$orderby : $orderby ){ + if (ref $order) { + push @orders,map{ "$_".($order->{$_}? " DESC " : "") } keys %$order; + } else { + push @orders,$order; + } } $sql.= do { local $"=', '; qq{ ORDER BY @orders} @@ -287,13 +306,21 @@ sub _filter_fields{ my @keys; my @values; if (ref($filter_input) eq "HASH"){ - my ($keys, $values) = _filter_hash($tablename,$filter_input, $searchtype); + my ($keys, $values); + if (my $special = delete $filter_input->{''}) { # XXX destroyes '' key + ($keys, $values) = _filter_fields($tablename,$special, $searchtype,$filtercolumns); + } + my ($hkeys, $hvalues) = _filter_hash($tablename,$filter_input, $searchtype); + if ($hkeys){ + push @$keys, @$hkeys; + push @$values, @$hvalues; + } if ($keys){ - my $stringkey="(".join (") AND (",@$keys).")"; - return [$stringkey],$values; + my $stringkey="(".join (") AND (",@$keys).")"; + return [$stringkey],$values; } else { - return (); + return (); } } elsif (ref($filter_input) eq "ARRAY"){ foreach my $element_data (@$filter_input){ @@ -334,7 +361,9 @@ sub _filter_hash{ my $elements=join "|",@columns_filtered; foreach my $field (grep {/\b($elements)\b/} keys %$filter_input){ ## supposed to be a hash of simple values, hashes of arrays could be implemented - $filter_input->{$field}=format_date_in_iso($filter_input->{$field}) if ($columns->{$field}{Type}=~/date/ && $filter_input->{$field} !~C4::Dates->regexp("iso")); + $filter_input->{$field}=format_date_in_iso($filter_input->{$field}) + if $columns->{$field}{Type}=~/date/ && + $filter_input->{$field} && $filter_input->{$field} !~C4::Dates->regexp("iso"); my ($tmpkeys, $localvalues)=_Process_Operands($filter_input->{$field},"$tablename.$field",$searchtype,$columns); if (@$tmpkeys){ push @values, @$localvalues; @@ -352,7 +381,11 @@ sub _filter_hash{ sub _filter_string{ my ($tablename,$filter_input, $searchtype,$filtercolumns)=@_; return () unless($filter_input); - my @operands=split / /,$filter_input; + my @operands=split /\s+/,$filter_input; + + # An act of desperation + $searchtype = 'contain' if @operands > 1 && $searchtype =~ /start_with/o; + my @columns_filtered= _filter_columns($tablename,$searchtype,$filtercolumns); my $columns= _get_columns($tablename); my (@values,@keys); @@ -381,8 +414,12 @@ sub _Process_Operands{ my @values; my @tmpkeys; my @localkeys; - push @tmpkeys, " $field = ? "; - push @values, $operand; + + $operand = [$operand] unless ref $operand; + foreach (@$operand) { + push @tmpkeys, " $field = ? "; + push @values, $_; + } #By default, exact search if (!$searchtype ||$searchtype eq "exact"){ return \@tmpkeys,\@values; @@ -394,16 +431,22 @@ sub _Process_Operands{ if ($columns->{$col_field}->{Type}=~/varchar|text/i){ my @localvaluesextended; if ($searchtype eq "contain"){ - push @tmpkeys,(" $field LIKE ? "); - push @localvaluesextended,("\%$operand\%") ; + foreach (@$operand) { + push @tmpkeys,(" $field LIKE ? "); + push @localvaluesextended,("\%$_\%") ; + } } if ($searchtype eq "field_start_with"){ - push @tmpkeys,("$field LIKE ?"); - push @localvaluesextended, ("$operand\%") ; + foreach (@$operand) { + push @tmpkeys,("$field LIKE ?"); + push @localvaluesextended, ("$_\%") ; + } } if ($searchtype eq "start_with"){ - push @tmpkeys,("$field LIKE ?","$field LIKE ?"); - push @localvaluesextended, ("$operand\%", " $operand\%") ; + foreach (@$operand) { + push @tmpkeys,("$field LIKE ?","$field LIKE ?"); + push @localvaluesextended, ("$_\%", " $_\%") ; + } } push @values,@localvaluesextended; } diff --git a/admin/aqbudget_owner_search.pl b/admin/aqbudget_owner_search.pl index 69e465fbde..5c73e9eb48 100755 --- a/admin/aqbudget_owner_search.pl +++ b/admin/aqbudget_owner_search.pl @@ -64,7 +64,7 @@ my @resultsdata; my $toggle = 0; if ( $member ) { - my $results= SearchMember($member,"surname",undef,undef,undef); + my $results= Search($member,"surname"); foreach my $res (@$results) { diff --git a/circ/circulation.pl b/circ/circulation.pl index 98fee97a7b..bbcccd5340 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -187,7 +187,7 @@ if ( $print eq 'yes' && $borrowernumber ne '' ) { my $borrowerslist; my $message; if ($findborrower) { - my ($count, $borrowers) = SearchMember($findborrower, 'cardnumber', 'web'); + my $borrowers = Search($findborrower, 'cardnumber'); my @borrowers = @$borrowers; if (C4::Context->preference("AddPatronLists")) { $template->param( diff --git a/circ/ysearch.pl b/circ/ysearch.pl index ce890f5b19..beab6a1f4e 100755 --- a/circ/ysearch.pl +++ b/circ/ysearch.pl @@ -28,6 +28,7 @@ use strict; #use warnings; FIXME - Bug 2505 use CGI; use C4::Context; +use C4::Members; use C4::Auth qw/check_cookie_auth/; my $input = new CGI; @@ -41,23 +42,11 @@ if ($auth_status ne "ok") { exit 0; } -my $dbh = C4::Context->dbh; -my $sql = qq(SELECT surname, firstname, cardnumber, address, city, zipcode, country - FROM borrowers - WHERE surname LIKE ? - OR firstname LIKE ? - OR cardnumber LIKE ? - ORDER BY surname, firstname - LIMIT 10); -my $sth = $dbh->prepare( $sql ); -$sth->execute("$query%", "$query%", "$query%"); - -while ( my $rec = $sth->fetchrow_hashref ) { - print $rec->{surname} . ", " . $rec->{firstname} . "\t" . - $rec->{cardnumber} . "\t" . - $rec->{address} . "\t" . - $rec->{city} . "\t" . - $rec->{zip} . "\t" . - $rec->{country} . - "\n"; -} +print map $_->{surname} . ", " . $_->{firstname} . "\t" . + $_->{cardnumber} . "\t" . + $_->{address} . "\t" . + $_->{city} . "\t" . + $_->{zipcode} . "\t" . + $_->{country} . + "\n", + @{ Search($query, [qw(surname firstname cardnumber)], [10], [qw(surname firstname cardnumber address city zipcode country)]) }; diff --git a/members/guarantor_search.pl b/members/guarantor_search.pl index 9eb60de2cf..59ea5b19de 100755 --- a/members/guarantor_search.pl +++ b/members/guarantor_search.pl @@ -66,14 +66,9 @@ my @resultsdata; my $background = 0; if ($member ne ''){ - if(length($member) == 1) - { - ($count,$results)=SearchMember($member,$orderby,"simple",$search_category); - } - else - { - ($count,$results)=SearchMember($member,$orderby,"advanced",$search_category); - } + $results = Search({''=>$member, category_type=>$search_category},$orderby); + $count = $results ? @$results : 0; + for (my $i=0; $i < $count; $i++){ #find out stats my ($od,$issue,$fines)=GetMemberIssuesAndFines($results->[$i]{'borrowerid'}); diff --git a/members/member.pl b/members/member.pl index 724c99655b..e2a6ef54dd 100755 --- a/members/member.pl +++ b/members/member.pl @@ -75,10 +75,11 @@ foreach my $category (@categories){ }; $categories_dislay{$$category{categorycode}} = $hash; } +my $AddPatronLists = C4::Context->preference("AddPatronLists") || ''; $template->param( - "AddPatronLists_".C4::Context->preference("AddPatronLists")=> "1", + "AddPatronLists_$AddPatronLists" => "1", ); -if (C4::Context->preference("AddPatronLists")=~/code/){ +if ($AddPatronLists=~/code/){ $categories[0]->{'first'}=1; } @@ -96,17 +97,15 @@ else { $member =~ s/,//g; #remove any commas from search string $member =~ s/\*/%/g; -my ($count,$results); - -my @searchpatron; -push @searchpatron, $member if ($member); -push @searchpatron, $patron if ( keys %$patron ); my $from = ( $startfrom - 1 ) * $resultsperpage; my $to = $from + $resultsperpage; -#($results)=Search(\@searchpatron,{surname=>1,firstname=>1},[$from,$to],undef,["firstname","surname","email","othernames"] ) if (@searchpatron); -my $search_scope = ( $quicksearch ? "field_start_with" : "start_with" ); -($results) = Search( \@searchpatron, \@orderby, undef, undef, [ "firstname", "surname", "othernames", "cardnumber", "userid" ], $search_scope ) if (@searchpatron); +my ($count,$results); +if ($member || keys %$patron) { + #($results)=Search($member || $patron,{surname=>1,firstname=>1},[$from,$to],undef,["firstname","surname","email","othernames"] ); + my $search_scope = ( $quicksearch ? "field_start_with" : "start_with" ); + ($results) = Search( $member || $patron, \@orderby, undef, undef, [ "firstname", "surname", "othernames", "cardnumber", "userid" ], $search_scope ); +} if ($results) { for my $field ('categorycode','branchcode'){ diff --git a/patroncards/members-search.pl b/patroncards/members-search.pl index c2abe07753..dfa0462340 100755 --- a/patroncards/members-search.pl +++ b/patroncards/members-search.pl @@ -49,17 +49,9 @@ $member =~ s/,//g; #remove any commas from search string $member =~ s/\*/%/g; if ($member || $category) { - my ($count,$results) = 0,0; - - if(length($member) == 1) - { - ($count,$results) = SearchMember($member,$orderby,"simple"); - } - else - { - ($count,$results) = SearchMember($member,$orderby,"advanced",$category); - } - + my $results = $category ? Search({''=>$member, category_type=>$category}, $orderby) + : Search($member, $orderby); + my $count = $results ? @$results : 0; my @resultsdata = (); my $to = ($count>($startfrom * $resultsperpage)?$startfrom * $resultsperpage:$count); diff --git a/reserve/request.pl b/reserve/request.pl index 5d59e2fbd8..cfeea73765 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -112,20 +112,18 @@ if ( $action eq 'move' ) { } if ($findborrower) { - my ( $count, $borrowers ) = - SearchMember($findborrower, 'cardnumber', 'web' ); + my $borrowers = Search($findborrower, 'cardnumber'); - my @borrowers = @$borrowers; - - if ( !@borrowers ) { + if ($borrowers && @$borrowers) { + if ( @$borrowers == 1 ) { + $borrowernumber_hold = $borrowers->[0]->{'borrowernumber'}; + } + else { + $borrowerslist = $borrowers; + } + } else { $messageborrower = "'$findborrower'"; } - elsif ( @borrowers == 1 ) { - $borrowernumber_hold = $borrowers[0]->{'borrowernumber'}; - } - else { - $borrowerslist = \@borrowers; - } } # If we have the borrowernumber because we've performed an action, then we diff --git a/t/db_dependent/Koha.t b/t/db_dependent/Koha.t index 016525a92e..378a0c1076 100644 --- a/t/db_dependent/Koha.t +++ b/t/db_dependent/Koha.t @@ -7,10 +7,11 @@ use strict; use warnings; use C4::Context; -use Test::More tests => 4; +use Test::More tests => 8; BEGIN { use_ok('C4::Koha'); + use_ok('C4::Members'); } my $data = { @@ -32,10 +33,19 @@ ok($insert_success, "Insert data in database"); # Tests SKIP: { - skip "INSERT failed", 2 unless $insert_success; + skip "INSERT failed", 5 unless $insert_success; is ( GetAuthorisedValueByCode($data->{category}, $data->{authorised_value}), $data->{lib}, "GetAuthorisedValueByCode" ); is ( GetKohaImageurlFromAuthorisedValues($data->{category}, $data->{lib}), $data->{imageurl}, "GetKohaImageurlFromAuthorisedValues" ); + + my $sortdet=C4::Members::GetSortDetails("lost", "3"); + is ($sortdet, "Lost and Paid For", "lost and paid works"); + + my $sortdet2=C4::Members::GetSortDetails("loc", "child"); + is ($sortdet2, "Children's Area", "Child area works"); + + my $sortdet3=C4::Members::GetSortDetails("withdrawn", "1"); + is ($sortdet3, "Withdrawn", "Withdrawn works"); } # Clean up diff --git a/t/db_dependent/Members.t b/t/db_dependent/Members.t index e28fbae742..b09d887117 100755 --- a/t/db_dependent/Members.t +++ b/t/db_dependent/Members.t @@ -6,62 +6,146 @@ use strict; use warnings; -use Test::More tests => 15; +use Test::More tests => 20; +use Data::Dumper; BEGIN { use_ok('C4::Members'); } -# Make a borrower for testing -my $data = { cardnumber => 'TESTCARD01', - firstname => 'Marie', - surname => 'Mcknight', - categorycode => 'S', - branchcode => 's' - }; +my $CARDNUMBER = 'TESTCARD01'; +my $FIRSTNAME = 'Marie'; +my $SURNAME = 'Mcknight'; +my $CATEGORYCODE = 'S'; +my $BRANCHCODE = 's'; -my $addmem=AddMember(%$data); +my $CHANGED_FIRSTNAME = "Marry Ann"; +my $EMAIL = "Marie\@email.com"; +my $ETHNICITY = "German"; +my $PHONE = "555-12123"; +# XXX should be randomised and checked against the database +my $IMPOSSIBLE_CARDNUMBER = "XYZZZ999"; -my $member=GetMemberDetails("","TESTCARD01"); -is ($member->{firstname}, "Marie", "Got member"); +my $INDEPENDENT_BRANCHES_PREF = 'IndependantBranches'; -$member->{firstname}="Claire"; -ModMember(%$member); -my $changedmember=GetMemberDetails("","TESTCARD01"); -is ($changedmember->{firstname}, "Claire", "Member Changed"); +# XXX make a non-commit transaction and rollback rather than insert/delete -$member->{firstname}="Marie"; -ModMember(%$member); -$changedmember=GetMemberDetails("","TESTCARD01"); -is ($changedmember->{firstname}, "Marie", "Member Returned"); +#my ($usernum, $userid, $usercnum, $userfirstname, $usersurname, $userbranch, $branchname, $userflags, $emailaddress, $branchprinter)= @_; +my @USERENV = ( + 1, + 'test', + 'MASTERTEST', + 'Test', + 'Test', + 't', + 'Test', + 0, +); +my $BRANCH_IDX = 5; -$member->{email}="Marie\@email.com"; -ModMember(%$member); -$changedmember=GetMemberDetails("","TESTCARD01"); -is ($changedmember->{email}, "Marie\@email.com", "Email Set works"); +C4::Context->_new_userenv ('DUMMY_SESSION_ID'); +C4::Context->set_userenv ( @USERENV ); -$member->{ethnicity}="German"; -ModMember(%$member); -$changedmember=GetMemberDetails("","TESTCARD01"); -is ($changedmember->{ethnicity}, "German", "Ethnicity Works"); - -my @searchstring=("Mcknight"); -my ($results) = Search(\@searchstring,undef,undef,undef,["surname"]); -is ($results->[0]->{surname}, "Mcknight", "Surname Search works"); +my $userenv = C4::Context->userenv + or BAIL_OUT("No userenv"); -$member->{phone}="555-12123"; +# Make a borrower for testing +my %data = ( + cardnumber => $CARDNUMBER, + firstname => $FIRSTNAME, + surname => $SURNAME, + categorycode => $CATEGORYCODE, + branchcode => $BRANCHCODE, +); + +my $addmem=AddMember(%data); +ok($addmem, "AddMember()"); + +my $member=GetMemberDetails("",$CARDNUMBER) + or BAIL_OUT("Cannot read member with card $CARDNUMBER"); + +ok ( $member->{firstname} eq $FIRSTNAME && + $member->{surname} eq $SURNAME && + $member->{categorycode} eq $CATEGORYCODE && + $member->{branchcode} eq $BRANCHCODE + , "Got member") + or diag("Mismatching member details: ".Dumper(\%data, $member)); + +$member->{firstname} = $CHANGED_FIRSTNAME; +$member->{email} = $EMAIL; +$member->{ethnicity} = $ETHNICITY; +$member->{phone} = $PHONE; ModMember(%$member); - -@searchstring=("555-12123"); -($results) = Search(\@searchstring,undef,undef,undef,["phone"]); -is ($results->[0]->{phone}, "555-12123", "phone Search works"); - -my $checkcardnum=C4::Members::checkcardnumber("TESTCARD01", ""); +my $changedmember=GetMemberDetails("",$CARDNUMBER); +ok ( $changedmember->{firstname} eq $CHANGED_FIRSTNAME && + $changedmember->{email} eq $EMAIL && + $changedmember->{ethnicity} eq $ETHNICITY && + $changedmember->{phone} eq $PHONE + , "Member Changed") + or diag("Mismatching member details: ".Dumper($member, $changedmember)); + +C4::Context->set_preference( $INDEPENDENT_BRANCHES_PREF, '0' ); +C4::Context->clear_syspref_cache(); + +my $results = Search($CARDNUMBER); +ok (@$results == 1, "Search cardnumber returned only one result") + or diag("Multiple members with Card $CARDNUMBER: ".Dumper($results)); +ok (_find_member($results), "Search cardnumber") + or diag("Card $CARDNUMBER not found in the resultset: ".Dumper($results)); + +my @searchstring=($SURNAME); +$results = Search(\@searchstring); +ok (_find_member($results), "Search (arrayref)") + or diag("Card $CARDNUMBER not found in the resultset: ".Dumper($results)); + +$results = Search(\@searchstring,undef,undef,undef,["surname"]); +ok (_find_member($results), "Surname Search (arrayref)") + or diag("Card $CARDNUMBER not found in the resultset: ".Dumper($results)); + +$results = Search("$CHANGED_FIRSTNAME $SURNAME", "surname"); +ok (_find_member($results), "Full name Search (string)") + or diag("Card $CARDNUMBER not found in the resultset: ".Dumper($results)); + +@searchstring=($PHONE); +$results = Search(\@searchstring,undef,undef,undef,["phone"]); +ok (_find_member($results), "Phone Search (arrayref)") + or diag("Card $CARDNUMBER not found in the resultset: ".Dumper($results)); + +$results = Search($PHONE,undef,undef,undef,["phone"]); +ok (_find_member($results), "Phone Search (string)") + or diag("Card $CARDNUMBER not found in the resultset: ".Dumper($results)); + +C4::Context->set_preference( $INDEPENDENT_BRANCHES_PREF, '1' ); +C4::Context->clear_syspref_cache(); + +$results = Search("$CHANGED_FIRSTNAME $SURNAME", "surname"); +ok (!_find_member($results), "Full name Search (string) for independent branches, different branch") + or diag("Card $CARDNUMBER found in the resultset for independent branches: ".Dumper(C4::Context->preference($INDEPENDENT_BRANCHES_PREF), $results)); + +@searchstring=($SURNAME); +$results = Search(\@searchstring); +ok (!_find_member($results), "Search (arrayref) for independent branches, different branch") + or diag("Card $CARDNUMBER found in the resultset for independent branches: ".Dumper(C4::Context->preference($INDEPENDENT_BRANCHES_PREF), $results)); + +$USERENV[$BRANCH_IDX] = $BRANCHCODE; +C4::Context->set_userenv ( @USERENV ); + +$results = Search("$CHANGED_FIRSTNAME $SURNAME", "surname"); +ok (_find_member($results), "Full name Search (string) for independent branches, same branch") + or diag("Card $CARDNUMBER not found in the resultset for independent branches: ".Dumper(C4::Context->preference($INDEPENDENT_BRANCHES_PREF), $results)); + +@searchstring=($SURNAME); +$results = Search(\@searchstring); +ok (_find_member($results), "Search (arrayref) for independent branches, same branch") + or diag("Card $CARDNUMBER not found in the resultset for independent branches: ".Dumper(C4::Context->preference($INDEPENDENT_BRANCHES_PREF), $results)); + + +my $checkcardnum=C4::Members::checkcardnumber($CARDNUMBER, ""); is ($checkcardnum, "1", "Card No. in use"); -$checkcardnum=C4::Members::checkcardnumber("67", ""); +$checkcardnum=C4::Members::checkcardnumber($IMPOSSIBLE_CARDNUMBER, ""); is ($checkcardnum, "0", "Card No. not used"); my $age=GetAge("1992-08-14", "2011-01-19"); @@ -70,14 +154,17 @@ is ($age, "18", "Age correct"); $age=GetAge("2011-01-19", "1992-01-19"); is ($age, "-19", "Birthday In the Future"); -my $sortdet=C4::Members::GetSortDetails("lost", "3"); -is ($sortdet, "Lost and Paid For", "lost and paid works"); +# clean up +DelMember($member->{borrowernumber}); +$results = Search($CARDNUMBER,undef,undef,undef,["cardnumber"]); +ok (!_find_member($results), "Delete member") + or diag("Card $CARDNUMBER found for the deleted member in the resultset: ".Dumper($results)); -my $sortdet2=C4::Members::GetSortDetails("loc", "child"); -is ($sortdet2, "Children's Area", "Child area works"); -my $sortdet3=C4::Members::GetSortDetails("withdrawn", "1"); -is ($sortdet3, "Withdrawn", "Withdrawn works"); +exit; -# clean up -DelMember($member->{borrowernumber}); +sub _find_member { + my ($resultset) = @_; + my $found = $resultset && grep( { $_->{cardnumber} && $_->{cardnumber} eq $CARDNUMBER } @$resultset ); + return $found; +} diff --git a/t/db_dependent/lib/KohaTest/Members.pm b/t/db_dependent/lib/KohaTest/Members.pm index ff18869bc9..5646be1a36 100644 --- a/t/db_dependent/lib/KohaTest/Members.pm +++ b/t/db_dependent/lib/KohaTest/Members.pm @@ -12,7 +12,7 @@ sub testing_class { 'C4::Members' }; sub methods : Test( 1 ) { my $self = shift; - my @methods = qw( SearchMember + my @methods = qw( Search GetMemberDetails patronflags GetMember -- 2.39.5