Attachment 37751 Details for Bug 12633 – Bug 12633: Remove SQLHelper in C4::Members

[patch] Bug 12633: Remove SQLHelper in C4::Members

Bug-12633-Remove-SQLHelper-in-C4Members.patch (text/plain), 13.86 KB, created by Jonathan Druart on 2015-04-13 14:26:35 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jonathan Druart

Created: 2015-04-13 14:26:35 UTC

Size: 13.86 KB

patch

obsolete

>From 15202ccc48524ac5d099198bf62c5378735c568f Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@biblibre.com>
>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

Actions: View | Diff | Splinter Review

Attachments on bug 12633: 30115 | 30119 | 30547 | 30937 | 37751 | 38272 | 38284 | 38285 | 38697 | 38698 | 57270