From cbcc641bcf7ca99d766153b79d8d68b77cd0ffff Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Fri, 28 Feb 2014 10:38:18 -0500 Subject: [PATCH] Bug 10276 [QA Followup 2] - Remove stringify, use placeholders for db queries Signed-off-by: Mark Tompsett --- C4/Acquisition.pm | 22 ++++++++++------ C4/Branch.pm | 25 ++++++------------ C4/Items.pm | 11 ++++++-- C4/Members.pm | 24 +++++++++++------ C4/Serials.pm | 65 ++++++++++++++++++++++++++++------------------ C4/Suggestions.pm | 30 +++++++++++---------- circ/pendingreserves.pl | 5 ++- circ/reserveratios.pl | 5 ++- t/db_dependent/Branch.t | 14 +--------- 9 files changed, 107 insertions(+), 94 deletions(-) diff --git a/C4/Acquisition.pm b/C4/Acquisition.pm index 374e05d..9d00245 100644 --- a/C4/Acquisition.pm +++ b/C4/Acquisition.pm @@ -1950,9 +1950,12 @@ sub GetParcel { my @query_params = ( $supplierid, $code, $datereceived ); if ( C4::Context->preference("IndependentBranches") ) { unless ( C4::Context->IsSuperLibrarian() ) { - my $branches = - GetIndependentGroupModificationRights( { stringify => 1 } ); - $strsth .= " AND ( borrowers.branchcode IN ( $branches ) OR borrowers.branchcode = '')"; + my @branches = GetIndependentGroupModificationRights(); + $strsth .= + " AND ( borrowers.branchcode IN ( " + . join( ',', ('?') x @branches ) + . " ) OR borrowers.branchcode = '')"; + push( @query_params, @branches ); } } $strsth .= " ORDER BY aqbasket.basketno"; @@ -2167,8 +2170,9 @@ sub GetLateOrders { } if (C4::Context->preference("IndependentBranches") && !C4::Context->IsSuperLibrarian() ) { - my $branches = GetIndependentGroupModificationRights( { stringify => 1 } ); - $from .= qq{ AND borrowers.branchcode IN ( $branches ) }; + my @branches = GetIndependentGroupModificationRights(); + $from .= "AND borrowers.branchcode IN ( " . join(',', ('?') x @branches) . " ) "; + push( @query_params, @branches ); } $from .= " AND orderstatus <> 'cancelled' "; my $query = "$select $from $having\nORDER BY latesince, basketno, borrowers.branchcode, supplier"; @@ -2395,9 +2399,11 @@ sub GetHistory { if ( C4::Context->preference("IndependentBranches") && !C4::Context->IsSuperLibrarian() ) { - my $branches = - GetIndependentGroupModificationRights( { stringify => 1 } ); - $query .= qq{ AND ( borrowers.branchcode = ? OR borrowers.branchcode IN ( $branches ) ) }; + my @branches = GetIndependentGroupModificationRights(); + $query .= + " AND ( borrowers.branchcode = ? OR borrowers.branchcode IN ( " + . join( ',', ('?') x @branches ) . " ) ) "; + push( @query_params, @branches ); } $query .= " ORDER BY id"; diff --git a/C4/Branch.pm b/C4/Branch.pm index 86ee7e8..5817fe6 100644 --- a/C4/Branch.pm +++ b/C4/Branch.pm @@ -116,8 +116,9 @@ sub GetBranches { my $query = "SELECT * FROM branches"; my @bind_parameters; if ($onlymine && C4::Context->userenv && C4::Context->userenv->{branch}){ - my $branches = GetIndependentGroupModificationRights({ stringify => 1 }); - $query .= qq{ WHERE branchcode IN ( $branches ) }; + my @branches = GetIndependentGroupModificationRights(); + $query .= " WHERE branchcode IN ( " . join(',', ('?') x @branches) . " ) "; + push( @bind_parameters, @branches ); } $query .= " ORDER BY branchname"; $sth = $dbh->prepare($query); @@ -432,12 +433,11 @@ sub GetBranchesInCategory { =head2 GetIndependentGroupModificationRights GetIndependentGroupModificationRights( - { - branch => $this_branch, - for => $other_branch, - stringify => 1, - } - ); + { + branch => $this_branch, + for => $other_branch, + } + ); Returns a list of branches this branch shares a common independent group with. @@ -452,12 +452,6 @@ sub GetBranchesInCategory { If called in a scalar context, it returns a count of matching branchcodes. Returns 1 if - - If stringify param is passed, the return value will - be a string of the comma delimited branchcodes. This - is useful for "branchcode IN $branchcodes" clauses - in SQL queries. - $this_branch and $other_branch are equal for efficiency. So you can write: @@ -472,7 +466,6 @@ sub GetIndependentGroupModificationRights { my $this_branch = $params->{branch}; my $other_branch = $params->{for}; - my $stringify = $params->{stringify}; $this_branch ||= C4::Context->userenv->{branch}; @@ -506,8 +499,6 @@ sub GetIndependentGroupModificationRights { my $dbh = C4::Context->dbh; my @branchcodes = @{ $dbh->selectcol_arrayref( $sql, {}, @params ) }; - return join( ',', map { qq{'$_'} } @branchcodes ) if $stringify; - if ( wantarray() ) { if ( @branchcodes ) { return @branchcodes; diff --git a/C4/Items.pm b/C4/Items.pm index 1a8c15d..d87e801 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -2990,9 +2990,14 @@ sub PrepareItemrecordDisplay { if ( $tagslib->{$tag}->{$subfield}->{'authorised_value'} eq "branches" ) { if ( ( C4::Context->preference("IndependentBranches") ) && !C4::Context->IsSuperLibrarian() ) { - my $branches = GetIndependentGroupModificationRights( { stringify => 1 } ); - my $sth = $dbh->prepare( "SELECT branchcode,branchname FROM branches WHERE branchcode IN ( $branches ) ORDER BY branchname" ); - $sth->execute(); + my @branches = + GetIndependentGroupModificationRights(); + my $sql = + "SELECT branchcode,branchname FROM branches WHERE branchcode IN (" + . join( ',', ('?') x @branches ) + . ") ORDER BY branchname"; + my $sth = $dbh->prepare($sql); + $sth->execute(@branches); push @authorised_values, "" unless ( $tagslib->{$tag}->{$subfield}->{mandatory} ); while ( my ( $branchcode, $branchname ) = $sth->fetchrow_array ) { diff --git a/C4/Members.pm b/C4/Members.pm index bf10ab7..540ca71 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -1296,19 +1296,25 @@ sub checkuniquemember { "SELECT borrowernumber,categorycode FROM borrowers WHERE surname=? and firstname=? and dateofbirth=?" : "SELECT borrowernumber,categorycode FROM borrowers WHERE surname=? and firstname=?"; + my @params; + if ($collectivity) { + push( @params, uc($surname) ); + } + elsif ($dateofbirth) { + push( @params, uc($surname), ucfirst($firstname), $dateofbirth ); + } + else { + push( @params, uc($surname), ucfirst($firstname) ); + } + if ( C4::Context->preference('IndependentBranches') ) { - my $branches = GetIndependentGroupModificationRights( { stringify => 1 } ); - $request .= " AND branchcode IN ( $branches )"; + my @branches = GetIndependentGroupModificationRights(); + $request .= " AND branchcode IN ( " . join(',', ('?') x @branches) ." )"; + push( @params, @branches ); } my $sth = $dbh->prepare($request); - if ($collectivity) { - $sth->execute( uc($surname) ); - } elsif($dateofbirth){ - $sth->execute( uc($surname), ucfirst($firstname), $dateofbirth ); - }else{ - $sth->execute( uc($surname), ucfirst($firstname)); - } + $sth->execute( @params ); my @data = $sth->fetchrow; ( $data[0] ) and return $data[0], $data[1]; return 0; diff --git a/C4/Serials.pm b/C4/Serials.pm index 84fa2d4..826b794 100644 --- a/C4/Serials.pm +++ b/C4/Serials.pm @@ -203,21 +203,25 @@ sub GetSerialInformation { subscription.*, subscription.subscriptionid as subsid |; + my @branches; if ( C4::Context->preference('IndependentBranches') && C4::Context->userenv && C4::Context->userenv->{'flags'} % 2 != 1 && C4::Context->userenv->{'branch'} ) { - my $branches = GetIndependentGroupModificationRights( { stringify => 1 } ); - $query .= qq| - , ( ( subscription.branchcode NOT IN ( $branches ) ) AND subscription.branchcode <> '' AND subscription.branchcode IS NOT NULL ) AS cannotedit - |; + @branches = GetIndependentGroupModificationRights(); + $query .= " + , ( + ( subscription.branchcode NOT IN ( " . join(',', ('?') x @branches) ." ) ) + AND subscription.branchcode <> '' AND subscription.branchcode IS NOT NULL + ) AS cannotedit + "; } $query .= qq| FROM serial LEFT JOIN subscription ON subscription.subscriptionid=serial.subscriptionid WHERE serialid = ? |; my $rq = $dbh->prepare($query); - $rq->execute($serialid); + $rq->execute($serialid, @branches); my $data = $rq->fetchrow_hashref; # create item information if we have serialsadditems for this subscription @@ -324,17 +328,20 @@ sub GetSubscription { subscription.biblionumber as bibnum |; + my @branches; if ( C4::Context->preference('IndependentBranches') && C4::Context->userenv && C4::Context->userenv->{'flags'} % 2 != 1 && C4::Context->userenv->{'branch'} ) { - my $branches = - GetIndependentGroupModificationRights( { stringify => 1 } ); + @branches = GetIndependentGroupModificationRights(); - $query .= qq| - , ( ( subscription.branchcode NOT IN ( $branches ) ) AND subscription.branchcode <> '' AND subscription.branchcode IS NOT NULL ) AS cannotedit - |; + $query .= " + , ( + ( subscription.branchcode NOT IN ( " . join(',', ('?') x @branches) ." ) ) + AND subscription.branchcode <> '' AND subscription.branchcode IS NOT NULL + ) AS cannotedit + "; } $query .= qq| @@ -347,7 +354,7 @@ sub GetSubscription { $debug and warn "query : $query\nsubsid :$subscriptionid"; my $sth = $dbh->prepare($query); - $sth->execute($subscriptionid); + $sth->execute($subscriptionid, @branches); my $subscription = $sth->fetchrow_hashref; $subscription->{cannotedit} = not can_edit_subscription( $subscription ); return $subscription; @@ -382,17 +389,20 @@ sub GetFullSubscription { subscription.subscriptionid AS subscriptionid |; + my @branches; if ( C4::Context->preference('IndependentBranches') && C4::Context->userenv && C4::Context->userenv->{'flags'} % 2 != 1 && C4::Context->userenv->{'branch'} ) { - my $branches = - GetIndependentGroupModificationRights( { stringify => 1 } ); + @branches = GetIndependentGroupModificationRights(); - $query .= qq| - , ( ( subscription.branchcode NOT IN ( $branches ) ) AND subscription.branchcode <> '' AND subscription.branchcode IS NOT NULL ) AS cannotedit - |; + $query .= " + , ( + ( subscription.branchcode NOT IN ( " . join(',', ('?') x @branches) . " ) ) + AND subscription.branchcode <> '' AND subscription.branchcode IS NOT NULL + ) AS cannotedit + "; } $query .= qq| @@ -408,7 +418,7 @@ sub GetFullSubscription { |; $debug and warn "GetFullSubscription query: $query"; my $sth = $dbh->prepare($query); - $sth->execute($subscriptionid); + $sth->execute( $subscriptionid, @branches ); my $subscriptions = $sth->fetchall_arrayref( {} ); for my $subscription ( @$subscriptions ) { $subscription->{cannotedit} = not can_edit_subscription( $subscription ); @@ -565,17 +575,20 @@ sub GetFullSubscriptionsFromBiblionumber { subscription.subscriptionid AS subscriptionid |; + my @branches; if ( C4::Context->preference('IndependentBranches') && C4::Context->userenv && C4::Context->userenv->{'flags'} != 1 && C4::Context->userenv->{'branch'} ) { - my $branches = - GetIndependentGroupModificationRights( { stringify => 1 } ); + @branches = GetIndependentGroupModificationRights(); - $query .= qq| - , ( ( subscription.branchcode NOT IN ( $branches ) ) AND subscription.branchcode <> '' AND subscription.branchcode IS NOT NULL ) AS cannotedit - |; + $query .= " + , ( + ( subscription.branchcode NOT IN (" . join(',', ('?') x @branches) . ") ) + AND subscription.branchcode <> '' AND subscription.branchcode IS NOT NULL + ) AS cannotedit + "; } $query .= qq| @@ -590,7 +603,7 @@ sub GetFullSubscriptionsFromBiblionumber { serial.subscriptionid |; my $sth = $dbh->prepare($query); - $sth->execute($biblionumber); + $sth->execute($biblionumber, @branches); my $subscriptions = $sth->fetchall_arrayref( {} ); for my $subscription ( @$subscriptions ) { $subscription->{cannotedit} = not can_edit_subscription( $subscription ); diff --git a/C4/Suggestions.pm b/C4/Suggestions.pm index f4b9816..f1ce474 100644 --- a/C4/Suggestions.pm +++ b/C4/Suggestions.pm @@ -140,9 +140,10 @@ sub SearchSuggestion { && !C4::Context->IsSuperLibrarian() && !$suggestion->{branchcode} ) { - my $branches = - GetIndependentGroupModificationRights( { stringify => 1 } ); - push( @query, qq{ AND (suggestions.branchcode IN ( $branches ) OR suggestions.branchcode='') } ); + my @branches = + GetIndependentGroupModificationRights(); + push( @query, " AND (suggestions.branchcode IN ( " . join(',', ('?') x @branches) ." ) OR suggestions.branchcode='') " ); + push( @sql_params, @branches ); } else { if ( defined $suggestion->{branchcode} && $suggestion->{branchcode} ) { @@ -347,12 +348,13 @@ sub GetSuggestionByStatus { && !C4::Context->IsSuperLibrarian() ) { - my $branches = - GetIndependentGroupModificationRights( { stringify => 1 } ); + my @branches = + GetIndependentGroupModificationRights(); - $query .= qq{ - AND (U1.branchcode IN ( $branches ) OR U1.branchcode ='') - }; + $query .= " + AND (U1.branchcode IN ( " . join(',', ('?') x @branches) . " ) OR U1.branchcode ='') + "; + push( @sql_params, @branches ); } if ($branchcode) { @@ -400,22 +402,22 @@ sub CountSuggestion { if ( C4::Context->preference("IndependentBranches") && !C4::Context->IsSuperLibrarian() ) { - my $branches = - GetIndependentGroupModificationRights( { stringify => 1 } ); + my @branches = + GetIndependentGroupModificationRights(); - my $query = qq{ + my $query = " SELECT count(*) FROM suggestions LEFT JOIN borrowers ON borrowers.borrowernumber=suggestions.suggestedby WHERE STATUS=? AND ( - borrowers.branchcode IN ( $branches ) + borrowers.branchcode IN (" . join(',', ('?') x @branches) . ") OR borrowers.branchcode=? ) - }; + "; $sth = $dbh->prepare($query); - $sth->execute( $status, $userenv->{branch} ); + $sth->execute( $status, @branches, $userenv->{branch} ); } else { my $query = q{ diff --git a/circ/pendingreserves.pl b/circ/pendingreserves.pl index 5129290..1c3a2ab 100755 --- a/circ/pendingreserves.pl +++ b/circ/pendingreserves.pl @@ -154,8 +154,9 @@ if ( $run_report ) { if (C4::Context->preference('IndependentBranches')){ - my $branches = GetIndependentGroupModificationRights( { stringify => 1 } ); - $strsth .= " AND items.holdingbranch IN ( $branches ) "; + my @branches = GetIndependentGroupModificationRights(); + $strsth .= " AND items.holdingbranch IN ( " . join(',', ('?') x @branches) . " ) "; + push( @query_params, @branches ); } $strsth .= " GROUP BY reserves.biblionumber ORDER BY biblio.title "; diff --git a/circ/reserveratios.pl b/circ/reserveratios.pl index 43e9b40..15ecc03 100755 --- a/circ/reserveratios.pl +++ b/circ/reserveratios.pl @@ -125,8 +125,9 @@ my $strsth = "; if ( C4::Context->preference('IndependentBranches') ) { - my $branches = GetIndependentGroupModificationRights( { stringify => 1 } ); - $strsth .= " AND items.holdingbranch IN ( $branches ) "; + my @branches = GetIndependentGroupModificationRights(); + $strsth .= " AND items.holdingbranch IN (" . join(',', ('?') x @branches) . ") "; + push( @query_params, @branches ); } $strsth .= " GROUP BY reserves.biblionumber ORDER BY reservecount DESC"; diff --git a/t/db_dependent/Branch.t b/t/db_dependent/Branch.t index 84b894a..b7b386f 100644 --- a/t/db_dependent/Branch.t +++ b/t/db_dependent/Branch.t @@ -21,7 +21,7 @@ use Modern::Perl; use C4::Context; use Data::Dumper; -use Test::More tests => 70; +use Test::More tests => 66; use C4::Branch; @@ -359,9 +359,6 @@ is( scalar(@$loop), GetBranchesCount(), 'There is the right number of branches' my @branches_bra = GetIndependentGroupModificationRights({ branch => 'BRA' }); is_deeply( \@branches_bra, [ 'BRA' ], 'Library with no group only has rights for its own branch' ); -my $string = GetIndependentGroupModificationRights({ branch => 'BRA', stringify => 1 }); -ok( $string eq q{'BRA'}, "String returns correctly" ); - ok( GetIndependentGroupModificationRights({ branch => 'BRA', for => 'BRA' }), 'Boolean test for BRA rights to BRA returns true' ); ok( !GetIndependentGroupModificationRights({ branch => 'BRA', for => 'BRB' }), 'Boolean test for BRA rights to BRB returns false' ); ok( !GetIndependentGroupModificationRights({ branch => 'BRA', for => 'BRC' }), 'Boolean test for BRA rights to BRC returns false' ); @@ -386,12 +383,6 @@ ModBranch({ @branches_bra = GetIndependentGroupModificationRights({ branch => 'BRA' }); is_deeply( \@branches_bra, [ 'BRA', 'BRB' ], 'Libraries in LIBCATCODE returned correctly' ); -$string = GetIndependentGroupModificationRights({ branch => 'BRA', stringify => 1 }); -ok( $string eq q{'BRA','BRB'}, "String returns correctly" ); - -$string = GetIndependentGroupModificationRights({ branch => 'BRC', stringify => 1 }); -ok( $string eq q{'BRC'}, "String returns correctly" ); - ok( GetIndependentGroupModificationRights({ branch => 'BRA', for => 'BRA' }), 'Boolean test for BRA rights to BRA returns true' ); ok( GetIndependentGroupModificationRights({ branch => 'BRA', for => 'BRB'}), 'Boolean test for BRA rights to BRB returns true' ); ok( !GetIndependentGroupModificationRights({ branch => 'BRA', for => 'BRC'}), 'Boolean test for BRA rights to BRC returns false' ); @@ -421,9 +412,6 @@ ok( GetIndependentGroupModificationRights({ branch => 'BRC', for => 'BRC' }), 'B ok( GetIndependentGroupModificationRights({ branch => 'BRC', for => 'BRA'}), 'Boolean test for BRC rights to BRA returns true' ); ok( GetIndependentGroupModificationRights({ branch => 'BRC', for => 'BRA'}), 'Boolean test for BRC rights to BRB returns true' ); -$string = GetIndependentGroupModificationRights({ branch => 'BRA', stringify => 1 }); -ok( $string eq q{'BRA','BRB','BRC'}, "String returns correctly" ); - # End transaction $dbh->rollback; -- 1.7.2.5