From 15202ccc48524ac5d099198bf62c5378735c568f Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Mon, 13 Apr 2015 16:21:50 +0200 Subject: [PATCH] Bug 12633: Remove SQLHelper in C4::Members This is the only places where SQLHelper is still called. The C4::Members::Search is not used anymore, but ModMember and AddMember. This patch replaced the calls to SQLHelper to use DBIX::Class. TODO: Move them to Koha::Borrower. Test plan: 1/ Make sure the patron search still works (no changes expected since the code was not in used). 2/ Add a patron with all fields filled 3/ Add another patron with some fields filled 4/ Update them with other values 5/ Delete them You should not get any errors. --- C4/Members.pm | 165 +++++++---------------------------------------- t/db_dependent/Members.t | 66 ++----------------- 2 files changed, 28 insertions(+), 203 deletions(-) diff --git a/C4/Members.pm b/C4/Members.pm index 4ca83b9..ad60bbf 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -32,7 +32,6 @@ use C4::Reserves; use C4::Accounts; use C4::Biblio; use C4::Letters; -use C4::SQLHelper qw(InsertInTable UpdateInTable SearchInTable); use C4::Members::Attributes qw(SearchIdMatchingAttribute UpdateBorrowerAttribute); use C4::NewsChannels; #get slip news use DateTime; @@ -157,139 +156,6 @@ This module contains routines for adding, modifying and deleting members/patrons =head1 FUNCTIONS -=head2 Search - - $borrowers_result_array_ref = &Search($filter,$orderby, $limit, - $columns_out, $search_on_fields,$searchtype); - -Looks up patrons (borrowers) on filter. A wrapper for SearchInTable('borrowers'). - -For C<$filter>, C<$orderby>, C<$limit>, C<&columns_out>, C<&search_on_fields> and C<&searchtype> -refer to C4::SQLHelper:SearchInTable(). - -Special C<$filter> key '' is effectively expanded to search on surname firstname othernamescw -and cardnumber unless C<&search_on_fields> is defined - -Examples: - - $borrowers = Search('abcd', 'cardnumber'); - - $borrowers = Search({''=>'abcd', category_type=>'I'}, 'surname'); - -=cut - -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 - 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} ); - } - - my ($search_on_fields, $searchtype); - if ( length($filter) == 1 ) { - $search_on_fields = [ qw(surname) ]; - $searchtype = 'start_with'; - } else { - $search_on_fields = [ qw(surname firstname othernames cardnumber) ]; - $searchtype = 'contain'; - } - - return (undef, $search_on_fields, $searchtype); -} - -sub Search { - my ( $filter, $orderby, $limit, $columns_out, $search_on_fields, $searchtype, $not_attributes ) = @_; - - 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 ( !$found_borrower && C4::Context->preference('ExtendedPatronAttributes') && $search_string && !$not_attributes ) { - my $matching_records = C4::Members::Attributes::SearchIdMatchingAttribute($search_string); - if(scalar(@$matching_records)>0) { - 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("IndependentBranches") ) { # && !$showallbranches){ - if ( my $userenv = C4::Context->userenv ) { - my $branch = $userenv->{'branch'}; - if ( !C4::Context->IsSuperLibrarian() && $branch ){ - 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"; - - return SearchInTable( "borrowers", $filter, $orderby, $limit, $columns_out, $search_on_fields, $searchtype ); -} - =head2 GetMemberDetails ($borrower) = &GetMemberDetails($borrowernumber, $cardnumber); @@ -797,8 +663,18 @@ sub ModMember { } } my $old_categorycode = GetBorrowerCategorycode( $data{borrowernumber} ); - my $execute_success=UpdateInTable("borrowers",\%data); - if ($execute_success) { # only proceed if the update was a success + + # get only the columns of a borrower + my $schema = Koha::Database->new()->schema; + my @columns = $schema->source('Borrower')->columns; + my $new_borrower = { map { join(' ', @columns) =~ /$_/ ? ( $_ => $data{$_} ) : () } keys(%data) }; + delete $new_borrower->{flags}; + + my $rs = $schema->resultset('Borrower')->search({ + borrowernumber => $new_borrower->{borrowernumber}, + }); + my $execute_success = $rs->update($new_borrower); + if ($execute_success ne '0E0') { # only proceed if the update was a success # ok if its an adult (type) it may have borrowers that depend on it as a guarantor # so when we update information for an adult we should check for guarantees and update the relevant part # of their records, ie addresses and phone numbers @@ -857,6 +733,7 @@ Returns as undef upon any db error without further processing sub AddMember { my (%data) = @_; my $dbh = C4::Context->dbh; + my $schema = Koha::Database->new()->schema; # generate a proper login if none provided $data{'userid'} = Generate_Userid( $data{'borrowernumber'}, $data{'firstname'}, $data{'surname'} ) @@ -872,9 +749,7 @@ sub AddMember { $data{'dateenrolled'} = C4::Dates->new()->output("iso"); } - my $patron_category = - Koha::Database->new()->schema()->resultset('Category') - ->find( $data{'categorycode'} ); + my $patron_category = $schema->resultset('Category')->find( $data{'categorycode'} ); $data{'privacy'} = $patron_category->default_privacy() eq 'default' ? 1 : $patron_category->default_privacy() eq 'never' ? 2 @@ -885,7 +760,15 @@ sub AddMember { # create a disabled account if no password provided $data{'password'} = ($data{'password'})? hash_password($data{'password'}) : '!'; - $data{'borrowernumber'}=InsertInTable("borrowers",\%data); + $data{'dateofbirth'} = undef if( not $data{'dateofbirth'} ); + + # get only the columns of Borrower + my @columns = $schema->source('Borrower')->columns; + my $new_member = { map { join(' ',@columns) =~ /$_/ ? ( $_ => $data{$_} ) : () } keys(%data) } ; + delete $new_member->{borrowernumber}; + + my $rs = $schema->resultset('Borrower'); + $data{borrowernumber} = $rs->create($new_member)->id; # If NorwegianPatronDBEnable is enabled, we set syncstatus to something that a # cronjob will use for syncing with NL @@ -904,7 +787,7 @@ sub AddMember { AddEnrolmentFeeIfNeeded( $data{categorycode}, $data{borrowernumber} ); - return $data{'borrowernumber'}; + return $data{borrowernumber}; } =head2 Check_Userid diff --git a/t/db_dependent/Members.t b/t/db_dependent/Members.t index cf19819..d9136fb 100755 --- a/t/db_dependent/Members.t +++ b/t/db_dependent/Members.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 72; +use Test::More tests => 61; use Test::MockModule; use Data::Dumper; use C4::Context; @@ -47,8 +47,6 @@ my $PHONE = "555-12123"; # XXX should be randomised and checked against the database my $IMPOSSIBLE_CARDNUMBER = "XYZZZ999"; -my $INDEPENDENT_BRANCHES_PREF = 'IndependentBranches'; - # XXX make a non-commit transaction and rollback rather than insert/delete #my ($usernum, $userid, $usercnum, $userfirstname, $usersurname, $userbranch, $branchname, $userflags, $emailaddress, $branchprinter)= @_; @@ -128,61 +126,6 @@ ok ( $changedmember->{firstname} eq $CHANGED_FIRSTNAME && , "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)); - C4::Context->set_preference( 'CardnumberLength', '' ); C4::Context->clear_syspref_cache(); @@ -263,9 +206,8 @@ is( @$messages, 0, 'DeleteMessage deletes a message correctly' ); # 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 $borrower = GetMember( cardnumber => $CARDNUMBER ); +is( $borrower, undef, 'DelMember should remove the patron' ); # Check_Userid tests %data = ( @@ -295,7 +237,7 @@ is( Check_Userid( 'tomasito.none', '' ), 0, 'userid not unique (blank borrowernumber)' ); is( Check_Userid( 'tomasito.none', $new_borrowernumber ), 0, 'userid not unique (second borrowernumber passed)' ); -my $borrower = GetMember( borrowernumber => $new_borrowernumber ); +$borrower = GetMember( borrowernumber => $new_borrowernumber ); ok( $borrower->{userid} ne 'tomasito', "Borrower with duplicate userid has new userid generated" ); $data{ cardnumber } = "234567890"; -- 2.1.0