From b80c69f53fdb425a09178f527057f1344e1eb47b Mon Sep 17 00:00:00 2001 From: Jonathan Druart 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 | 87 +++++++++++++++++++++++++------------ tools/import_borrowers.pl | 2 +- 5 files changed, 98 insertions(+), 34 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 2039db0..c481462 100755 --- a/t/db_dependent/Members_Attributes.t +++ b/t/db_dependent/Members_Attributes.t @@ -22,8 +22,10 @@ use Modern::Perl; 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_ok('C4::Members::Attributes'); @@ -31,28 +33,34 @@ my $dbh = C4::Context->dbh; $dbh->{AutoCommit} = 0; $dbh->{RaiseError} = 1; +my $builder = t::lib::TestBuilder->new; + $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 $borrowernumber = AddMember( - firstname => 'my firstname', - surname => 'my surname', - categorycode => 'S', - branchcode => 'CPL', +my $library = $builder->build({ + source => 'Branch', +}); + +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' ); @@ -60,6 +68,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' ); @@ -67,6 +78,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' ); @@ -83,6 +98,10 @@ my $attributes = [ value => 'my attribute2', code => $attribute_type2->code(), password => 'my password2', + }, + { + value => 'my attribute limited', + code => $attribute_type_limited->code(), } ]; @@ -99,7 +118,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' ); @@ -108,14 +127,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' ); @@ -139,7 +172,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' ); @@ -177,17 +210,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'); @@ -195,6 +228,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' ); - -$dbh->rollback; +is( @$borrower_attributes, 1, 'DeleteBorrowerAttribute deletes a borrower attribute' ); diff --git a/tools/import_borrowers.pl b/tools/import_borrowers.pl index 1caf7c4..44ec950 100755 --- a/tools/import_borrowers.pl +++ b/tools/import_borrowers.pl @@ -311,7 +311,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