Bugzilla – Attachment 48269 Details for
Bug 15163
Patron attributes with branch limiits are not saved when invisible
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 15163: Do not erase patron attributes if limited to another library
Bug-15163-Do-not-erase-patron-attributes-if-limite.patch (text/plain), 15.55 KB, created by
Jonathan Druart
on 2016-02-22 10:45:14 UTC
(
hide
)
Description:
Bug 15163: Do not erase patron attributes if limited to another library
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2016-02-22 10:45:14 UTC
Size:
15.55 KB
patch
obsolete
>From 2bfc4c4027aa87a7a44a36845430a7b85b3fa201 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Mon, 22 Feb 2016 10:08:55 +0000 >Subject: [PATCH] Bug 15163: Do not erase patron attributes if limited to > another library > >The patron attributes displayed on editing a patron are not displayed if >limited to another library. > >C4::Members::Attributes::SetBorrowerAttributes will now only delete attributes >the librarian is editing. >SetBorrowerAttributes takes a new $no_branch_limit parameter. If set, >the branch limitations have not effect and all attributes are deleted >(same behavior as before this patch). > >Test plan: >1/ Create 2 patron attributes, without branch limitations. >2/ Edit a patron and set a value for these attributes >3/ Limit a patron attributes to a library (one you are not logged in >with). >4/ Edit again the patron. >=> You should not see the limited attributes >5/ Edit the patron attributes and remove the branch limitation >=> Without this patch, it has been removed from the database and is not >displayed anymore. >=> With this patch, you should see it. >--- > C4/Auth_with_ldap.pm | 2 +- > C4/Members/Attributes.pm | 39 ++++++++++++++++-- > members/nl-search.pl | 2 +- > t/db_dependent/Members_Attributes.t | 79 +++++++++++++++++++++++++------------ > tools/import_borrowers.pl | 2 +- > 5 files changed, 92 insertions(+), 32 deletions(-) > >diff --git a/C4/Auth_with_ldap.pm b/C4/Auth_with_ldap.pm >index 58484a2..775f779 100644 >--- a/C4/Auth_with_ldap.pm >+++ b/C4/Auth_with_ldap.pm >@@ -223,7 +223,7 @@ sub checkpw_ldap { > warn "ERROR_extended_unique_id_failed $attr->{code} $attr->{value}"; > } > } >- C4::Members::Attributes::SetBorrowerAttributes($borrowernumber, \@unique_attr); >+ C4::Members::Attributes::SetBorrowerAttributes($borrowernumber, \@unique_attr, 'no_branch_limit'); > } > return(1, $cardnumber, $userid); > } >diff --git a/C4/Members/Attributes.pm b/C4/Members/Attributes.pm >index b25a813..2f560c7 100644 >--- a/C4/Members/Attributes.pm >+++ b/C4/Members/Attributes.pm >@@ -71,7 +71,6 @@ marked for OPAC display are returned. > sub GetBorrowerAttributes { > my $borrowernumber = shift; > my $opac_only = @_ ? shift : 0; >- my $branch_limit = @_ ? shift : 0; > > my $dbh = C4::Context->dbh(); > my $query = "SELECT code, description, attribute, lib, password, display_checkout, category_code, class >@@ -218,10 +217,11 @@ replacing any that existed previously. > sub SetBorrowerAttributes { > my $borrowernumber = shift; > my $attr_list = shift; >+ my $no_branch_limit = shift // 0; > > my $dbh = C4::Context->dbh; >- my $delsth = $dbh->prepare("DELETE FROM borrower_attributes WHERE borrowernumber = ?"); >- $delsth->execute($borrowernumber); >+ >+ DeleteBorrowerAttributes( $borrowernumber, $no_branch_limit ); > > my $sth = $dbh->prepare("INSERT INTO borrower_attributes (borrowernumber, code, attribute, password) > VALUES (?, ?, ?, ?)"); >@@ -236,6 +236,39 @@ sub SetBorrowerAttributes { > return 1; # borrower attributes successfully set > } > >+=head2 DeleteBorrowerAttributes >+ >+ DeleteBorrowerAttributes($borrowernumber); >+ >+Delete borrower attributes for the patron identified by C<$borrowernumber>. >+ >+=cut >+ >+sub DeleteBorrowerAttributes { >+ my $borrowernumber = shift; >+ my $no_branch_limit = @_ ? shift : 0; >+ my $branch_limit = $no_branch_limit >+ ? 0 >+ : C4::Context->userenv ? C4::Context->userenv->{"branch"} : 0; >+ >+ my $dbh = C4::Context->dbh; >+ my $query = q{ >+ DELETE borrower_attributes FROM borrower_attributes >+ }; >+ >+ $query .= $branch_limit >+ ? q{ >+ LEFT JOIN borrower_attribute_types_branches ON bat_code = code >+ WHERE b_branchcode = ? OR b_branchcode IS NULL >+ AND borrowernumber = ? >+ } >+ : q{ >+ WHERE borrowernumber = ? >+ }; >+ >+ $dbh->do( $query, undef, $branch_limit ? $branch_limit : (), $borrowernumber ); >+} >+ > =head2 DeleteBorrowerAttribute > > DeleteBorrowerAttribute($borrowernumber, $attribute); >diff --git a/members/nl-search.pl b/members/nl-search.pl >index 08f3a04..dc6e585 100755 >--- a/members/nl-search.pl >+++ b/members/nl-search.pl >@@ -140,7 +140,7 @@ if ( $op && $op eq 'search' ) { > # Add extended patron attributes > SetBorrowerAttributes($borrowernumber, [ > { code => 'fnr', value => $cgi->param('fnr_hash') }, >- ] ); >+ ], 'no_branch_limit' ); > # Override the default sync data created by AddMember > my $borrowersync = Koha::Database->new->schema->resultset('BorrowerSync')->find({ > 'synctype' => 'norwegianpatrondb', >diff --git a/t/db_dependent/Members_Attributes.t b/t/db_dependent/Members_Attributes.t >index 71e8394..47a1417 100755 >--- a/t/db_dependent/Members_Attributes.t >+++ b/t/db_dependent/Members_Attributes.t >@@ -23,8 +23,9 @@ use C4::Context; > use C4::Members; > use C4::Members::AttributeTypes; > use Koha::Database; >+use t::lib::TestBuilder; > >-use Test::More tests => 60; >+use Test::More tests => 57; > > use t::lib::TestBuilder; > >@@ -40,27 +41,28 @@ $dbh->do(q|DELETE FROM issues|); > $dbh->do(q|DELETE FROM borrowers|); > $dbh->do(q|DELETE FROM borrower_attributes|); > $dbh->do(q|DELETE FROM borrower_attribute_types|); >- > my $library = $builder->build({ > source => 'Branch', > }); >-my $borrowernumber = AddMember( >- firstname => 'my firstname', >- surname => 'my surname', >- categorycode => 'S', >- branchcode => $library->{branchcode}, >-); > >+my $patron = $builder->build( >+ { source => 'Borrower', >+ value => { >+ firstname => 'my firstname', >+ surname => 'my surname', >+ categorycode => 'S', >+ branchcode => $library->{branchcode}, >+ } >+ } >+); >+C4::Context->_new_userenv('DUMMY SESSION'); >+C4::Context->set_userenv(123, 'userid', 'usercnum', 'First name', 'Surname', $library->{branchcode}, 'My Library', 0); >+my $borrowernumber = $patron->{borrowernumber}; > > my $attribute_type1 = C4::Members::AttributeTypes->new('my code1', 'my description1'); > $attribute_type1->unique_id(1); >-my $attribute_type2 = C4::Members::AttributeTypes->new('my code2', 'my description2'); >-$attribute_type2->opac_display(1); >-$attribute_type2->staff_searchable(1); >- > my $attribute_types = C4::Members::Attributes::GetAttributes(); > is( @$attribute_types, 0, 'GetAttributes returns the correct number of attribute types' ); >- > $attribute_type1->store(); > $attribute_types = C4::Members::Attributes::GetAttributes(); > is( @$attribute_types, 1, 'GetAttributes returns the correct number of attribute types' ); >@@ -68,6 +70,9 @@ is( $attribute_types->[0], $attribute_type1->code(), 'GetAttributes returns the > $attribute_types = C4::Members::Attributes::GetAttributes(1); > is( @$attribute_types, 0, 'GetAttributes returns the correct number of attribute types with the filter opac_only' ); > >+my $attribute_type2 = C4::Members::AttributeTypes->new('my code2', 'my description2'); >+$attribute_type2->opac_display(1); >+$attribute_type2->staff_searchable(1); > $attribute_type2->store(); > $attribute_types = C4::Members::Attributes::GetAttributes(); > is( @$attribute_types, 2, 'GetAttributes returns the correct number of attribute types' ); >@@ -75,6 +80,10 @@ is( $attribute_types->[1], $attribute_type2->code(), 'GetAttributes returns the > $attribute_types = C4::Members::Attributes::GetAttributes(1); > is( @$attribute_types, 1, 'GetAttributes returns the correct number of attribute types with the filter opac_only' ); > >+my $new_library = $builder->build( { source => 'Branch' } ); >+my $attribute_type_limited = C4::Members::AttributeTypes->new('my code3', 'my description3'); >+$attribute_type_limited->branches([ $new_library->{branchcode} ]); >+$attribute_type_limited->store; > > my $borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes(); > is( @$borrower_attributes, 0, 'GetBorrowerAttributes without the borrower number returns an empty array' ); >@@ -91,6 +100,10 @@ my $attributes = [ > value => 'my attribute2', > code => $attribute_type2->code(), > password => 'my password2', >+ }, >+ { >+ value => 'my attribute limited', >+ code => $attribute_type_limited->code(), > } > ]; > >@@ -107,7 +120,7 @@ is( $set_borrower_attributes, 1, 'SetBorrowerAttributes returns the success code > $borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes(); > is( @$borrower_attributes, 0, 'GetBorrowerAttributes without the borrower number returns an empty array' ); > $borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber); >-is( @$borrower_attributes, 2, 'GetBorrowerAttributes returns the correct number of borrower attributes' ); >+is( @$borrower_attributes, 3, 'GetBorrowerAttributes returns the correct number of borrower attributes' ); > is( $borrower_attributes->[0]->{code}, $attributes->[0]->{code}, 'SetBorrowerAttributes stores the correct code correctly' ); > is( $borrower_attributes->[0]->{description}, $attribute_type1->description(), 'SetBorrowerAttributes stores the field description correctly' ); > is( $borrower_attributes->[0]->{value}, $attributes->[0]->{value}, 'SetBorrowerAttributes stores the field value correctly' ); >@@ -116,14 +129,28 @@ is( $borrower_attributes->[1]->{code}, $attributes->[1]->{code}, 'SetBorrowerAtt > is( $borrower_attributes->[1]->{description}, $attribute_type2->description(), 'SetBorrowerAttributes stores the field description correctly' ); > is( $borrower_attributes->[1]->{value}, $attributes->[1]->{value}, 'SetBorrowerAttributes stores the field value correctly' ); > is( $borrower_attributes->[1]->{password}, $attributes->[1]->{password}, 'SetBorrowerAttributes stores the field password correctly' ); >+$borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber); >+is( @$borrower_attributes, 3, 'GetBorrowerAttributes returns the correct number of borrower attributes' ); > >-$borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber, 1); >-is( @$borrower_attributes, 1, 'GetBorrowerAttributes returns the correct number of borrower attributes with the filter opac_only' ); >-is( $borrower_attributes->[0]->{code}, $attributes->[1]->{code}, 'GetBorrowerAttributes returns the correct code' ); >-is( $borrower_attributes->[0]->{description}, $attribute_type2->description(), 'GetBorrowerAttributes returns the correct description' ); >-is( $borrower_attributes->[0]->{value}, $attributes->[1]->{value}, 'GetBorrowerAttributes returns the correct value' ); >-is( $borrower_attributes->[0]->{password}, $attributes->[1]->{password}, 'GetBorrowerAttributes returns the correct password' ); >+$attributes = [ >+ { >+ value => 'my attribute1', >+ code => $attribute_type1->code(), >+ password => 'my password1', >+ }, >+ { >+ value => 'my attribute2', >+ code => $attribute_type2->code(), >+ password => 'my password2', >+ } >+]; >+C4::Members::Attributes::SetBorrowerAttributes($borrowernumber, $attributes); >+$borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber); >+is( @$borrower_attributes, 3, 'SetBorrowerAttributes should not have removed the attributes limited to another branch' ); > >+# TODO This is not implemented yet >+#$borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber, undef, 'branch_limited'); >+#is( @$borrower_attributes, 2, 'GetBorrowerAttributes returns the correct number of borrower attributes filtered on library' ); > > my $attribute_value = C4::Members::Attributes::GetBorrowerAttributeValue(); > is( $attribute_value, undef, 'GetBorrowerAttributeValue without arguments returns undef' ); >@@ -147,7 +174,7 @@ my $attribute = { > }; > C4::Members::Attributes::UpdateBorrowerAttribute($borrowernumber, $attribute); > $borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber); >-is( @$borrower_attributes, 2, 'UpdateBorrowerAttribute does not change the number of borrower attributes' ); >+is( @$borrower_attributes, 3, 'UpdateBorrowerAttribute does not change the number of borrower attributes' ); > is( $borrower_attributes->[0]->{code}, $attribute->{code}, 'UpdateBorrowerAttribute updates the field code correctly' ); > is( $borrower_attributes->[0]->{description}, $attribute_type1->description(), 'UpdateBorrowerAttribute updates the field description correctly' ); > is( $borrower_attributes->[0]->{value}, $attribute->{attribute}, 'UpdateBorrowerAttribute updates the field value correctly' ); >@@ -185,17 +212,17 @@ for my $attr( split(' ', $attributes->[1]->{value}) ) { > > C4::Members::Attributes::DeleteBorrowerAttribute(); > $borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber); >-is( @$borrower_attributes, 2, 'DeleteBorrowerAttribute without arguments deletes nothing' ); >+is( @$borrower_attributes, 3, 'DeleteBorrowerAttribute without arguments deletes nothing' ); > C4::Members::Attributes::DeleteBorrowerAttribute($borrowernumber); > $borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber); >-is( @$borrower_attributes, 2, 'DeleteBorrowerAttribute without the attribute deletes nothing' ); >+is( @$borrower_attributes, 3, 'DeleteBorrowerAttribute without the attribute deletes nothing' ); > C4::Members::Attributes::DeleteBorrowerAttribute(undef, $attribute); > $borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber); >-is( @$borrower_attributes, 2, 'DeleteBorrowerAttribute with a undef borrower number deletes nothing' ); >+is( @$borrower_attributes, 3, 'DeleteBorrowerAttribute with a undef borrower number deletes nothing' ); > > C4::Members::Attributes::DeleteBorrowerAttribute($borrowernumber, $attribute); > $borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber); >-is( @$borrower_attributes, 1, 'DeleteBorrowerAttribute deletes a borrower attribute' ); >+is( @$borrower_attributes, 2, 'DeleteBorrowerAttribute deletes a borrower attribute' ); > is( $borrower_attributes->[0]->{code}, $attributes->[1]->{code}, 'DeleteBorrowerAttribute deletes the correct entry'); > is( $borrower_attributes->[0]->{description}, $attribute_type2->description(), 'DeleteBorrowerAttribute deletes the correct entry'); > is( $borrower_attributes->[0]->{value}, $attributes->[1]->{value}, 'DeleteBorrowerAttribute deletes the correct entry'); >@@ -203,4 +230,4 @@ is( $borrower_attributes->[0]->{password}, $attributes->[1]->{password}, 'Delete > > C4::Members::Attributes::DeleteBorrowerAttribute($borrowernumber, $attributes->[1]); > $borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber); >-is( @$borrower_attributes, 0, 'DeleteBorrowerAttribute deletes a borrower attribute' ); >+is( @$borrower_attributes, 1, 'DeleteBorrowerAttribute deletes a borrower attribute' ); >diff --git a/tools/import_borrowers.pl b/tools/import_borrowers.pl >index 42e7a8e..6a32611 100755 >--- a/tools/import_borrowers.pl >+++ b/tools/import_borrowers.pl >@@ -308,7 +308,7 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) { > my $old_attributes = GetBorrowerAttributes($borrowernumber); > $patron_attributes = extended_attributes_merge($old_attributes, $patron_attributes); #TODO: expose repeatable options in template > } >- push @errors, {unknown_error => 1} unless SetBorrowerAttributes($borrower{'borrowernumber'}, $patron_attributes); >+ push @errors, {unknown_error => 1} unless SetBorrowerAttributes($borrower{'borrowernumber'}, $patron_attributes, 'no_branch_limit' ); > } > $overwritten++; > $template->param('lastoverwritten'=>$borrower{'surname'}.' / '.$borrowernumber); >-- >2.7.0
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 15163
:
45258
|
45260
|
46416
|
48269
|
48322
|
49329
|
49448