Bugzilla – Attachment 28356 Details for
Bug 10276
Extend IndependentBranches to support groups of libraries
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 10276i [QA Followup 2] - Remove stringify, use placeholders for db queries
Bug-10276i-QA-Followup-2---Remove-stringify-use-pl.patch (text/plain), 19.98 KB, created by
Kyle M Hall (khall)
on 2014-05-20 13:04:23 UTC
(
hide
)
Description:
Bug 10276i [QA Followup 2] - Remove stringify, use placeholders for db queries
Filename:
MIME Type:
Creator:
Kyle M Hall (khall)
Created:
2014-05-20 13:04:23 UTC
Size:
19.98 KB
patch
obsolete
>From e91df3f889280edc46a3cd3c11830f26015de900 Mon Sep 17 00:00:00 2001 >From: Kyle M Hall <kyle@bywatersolutions.com> >Date: Fri, 28 Feb 2014 10:38:18 -0500 >Subject: [PATCH] Bug 10276i [QA Followup 2] - Remove stringify, use placeholders for db queries > >Signed-off-by: Mark Tompsett <mtompset@hotmail.com> >--- > 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 9d1ad6d..6196f22 100644 >--- a/C4/Acquisition.pm >+++ b/C4/Acquisition.pm >@@ -1965,9 +1965,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"; >@@ -2182,8 +2185,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"; >@@ -2407,9 +2411,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"; > my $sth = $dbh->prepare($query); >diff --git a/C4/Branch.pm b/C4/Branch.pm >index 2db1fd0..9f249f2 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); >@@ -427,12 +428,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. >@@ -447,12 +447,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: >@@ -467,7 +461,6 @@ sub GetIndependentGroupModificationRights { > > my $this_branch = $params->{branch}; > my $other_branch = $params->{for}; >- my $stringify = $params->{stringify}; > > $this_branch ||= C4::Context->userenv->{branch}; > >@@ -501,8 +494,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 2233983..e59ceb1 100644 >--- a/C4/Items.pm >+++ b/C4/Items.pm >@@ -2760,9 +2760,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 5024de7..739ece3 100644 >--- a/C4/Members.pm >+++ b/C4/Members.pm >@@ -1345,19 +1345,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 f98b095..e31f524 100644 >--- a/C4/Serials.pm >+++ b/C4/Serials.pm >@@ -238,21 +238,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 >@@ -355,17 +359,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| >@@ -378,7 +385,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; >@@ -413,17 +420,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| >@@ -439,7 +449,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 ); >@@ -596,17 +606,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| >@@ -621,7 +634,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 acaaa5f..bbb9475 100644 >--- a/C4/Suggestions.pm >+++ b/C4/Suggestions.pm >@@ -137,9 +137,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} ) { >@@ -343,12 +344,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) { >@@ -396,22 +398,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 5708155..1acd9cc 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 7d23db1..91c0de2 100755 >--- a/circ/reserveratios.pl >+++ b/circ/reserveratios.pl >@@ -121,8 +121,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 2cce07a..13dcce7 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; > >@@ -351,9 +351,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' ); >@@ -378,12 +375,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' ); >@@ -413,9 +404,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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 10276
:
18205
|
18479
|
18519
|
18716
|
18717
|
18850
|
20773
|
21145
|
22791
|
23037
|
23484
|
23489
|
23637
|
23641
|
23953
|
23992
|
24427
|
24428
|
25722
|
25723
|
25724
|
26015
|
26026
|
26027
|
26028
|
26029
|
28354
|
28355
|
28356
|
28357
|
28358
|
28359
|
28360
|
28361
|
28365
|
28366
|
28367
|
36027
|
36028
|
36029
|
36030
|
36031
|
36032
|
41064
|
41065
|
41066
|
41067
|
41068
|
41069
|
41070