From e37bfe3e4b395ef71a54c65226829248847cf1ce Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Tue, 17 Jul 2018 11:03:49 -0300 Subject: [PATCH] Bug 20443: Remove extended_attributes_code_value_arrayref AND C4::Members::Attributes Signed-off-by: Nick Clemens Signed-off-by: Martin Renvoize --- C4/Auth_with_ldap.pm | 1 - C4/Members/Attributes.pm | 92 --------- Koha/Patron/Attributes.pm | 20 +- Koha/Patrons/Import.pm | 44 +++-- members/memberentry.pl | 1 - t/db_dependent/Koha/Patrons/Import.t | 13 +- t/db_dependent/Members/Attributes.t | 228 ---------------------- t/db_dependent/Utils/Datatables_Members.t | 2 - tools/modborrowers.pl | 1 - 9 files changed, 46 insertions(+), 356 deletions(-) delete mode 100644 C4/Members/Attributes.pm delete mode 100644 t/db_dependent/Members/Attributes.t diff --git a/C4/Auth_with_ldap.pm b/C4/Auth_with_ldap.pm index 020ade48b6..a85369b494 100644 --- a/C4/Auth_with_ldap.pm +++ b/C4/Auth_with_ldap.pm @@ -22,7 +22,6 @@ use Carp; use C4::Debug; use C4::Context; -use C4::Members::Attributes; use C4::Members::Messaging; use C4::Auth qw(checkpw_internal); use Koha::Patrons; diff --git a/C4/Members/Attributes.pm b/C4/Members/Attributes.pm deleted file mode 100644 index 127ad4e735..0000000000 --- a/C4/Members/Attributes.pm +++ /dev/null @@ -1,92 +0,0 @@ -package C4::Members::Attributes; - -# Copyright (C) 2008 LibLime -# -# This file is part of Koha. -# -# Koha is free software; you can redistribute it and/or modify it -# under the terms of the GNU General Public License as published by -# the Free Software Foundation; either version 3 of the License, or -# (at your option) any later version. -# -# Koha is distributed in the hope that it will be useful, but -# WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with Koha; if not, see . - -use strict; -use warnings; - -use Text::CSV; # Don't be tempted to use Text::CSV::Unicode -- even in binary mode it fails. -use C4::Context; - -use Koha::Patron::Attribute::Types; - -use vars qw(@ISA @EXPORT_OK @EXPORT %EXPORT_TAGS); -our ($csv, $AttributeTypes); - -BEGIN { - @ISA = qw(Exporter); - @EXPORT_OK = qw( - extended_attributes_code_value_arrayref - ); - %EXPORT_TAGS = ( all => \@EXPORT_OK ); -} - -=head1 NAME - -C4::Members::Attributes - manage extend patron attributes - -=head1 SYNOPSIS - - use C4::Members::Attributes; - -=head1 FUNCTIONS - -=head2 extended_attributes_code_value_arrayref - - my $patron_attributes = "homeroom:1150605,grade:01,extradata:foobar"; - my $aref = extended_attributes_code_value_arrayref($patron_attributes); - -Takes a comma-delimited CSV-style string argument and returns the kind of data structure that Koha::Patron->extended_attributes wants, -namely a reference to array of hashrefs like: - [ { code => 'CODE', attribute => 'value' }, { code => 'CODE2', attribute => 'othervalue' } ... ] - -Caches Text::CSV parser object for efficiency. - -=cut - -sub extended_attributes_code_value_arrayref { - my $string = shift or return; - use Data::Printer colored => 1; warn p $string; - $csv or $csv = Text::CSV->new({binary => 1}); # binary needed for non-ASCII Unicode - my $ok = $csv->parse($string); # parse field again to get subfields! - my @list = $csv->fields(); - # TODO: error handling (check $ok) - return [ - sort {&_sort_by_code($a,$b)} - map { map { my @arr = split /:/, $_, 2; { code => $arr[0], attribute => $arr[1] } } $_ } - @list - ]; - # nested map because of split -} - -sub _sort_by_code { - my ($x, $y) = @_; - defined ($x->{code}) or return -1; - defined ($y->{code}) or return 1; - return $x->{code} cmp $y->{code} || $x->{value} cmp $y->{value}; -} - -=head1 AUTHOR - -Koha Development Team - -Galen Charlton - -=cut - -1; diff --git a/Koha/Patron/Attributes.pm b/Koha/Patron/Attributes.pm index 9af07c0228..84685d9dc5 100644 --- a/Koha/Patron/Attributes.pm +++ b/Koha/Patron/Attributes.pm @@ -87,24 +87,32 @@ sub filter_by_branch_limitations { return $self->search( $or, $join ); } +=head3 + +$new_attributes is an arrayref of hashrefs + +=cut + sub merge_with { my ( $self, $new_attributes ) = @_; my @merged = $self->as_list; - while ( my ( $attr ) = $new_attributes->next ) { - unless ( $attr->code ) { + my $attribute_types = { map { $_->code => $_->unblessed } Koha::Patron::Attribute::Types->search }; + for my $attr ( @$new_attributes ) { + unless ( $attr->{code} ) { warn "Cannot merge element: no 'code' defined"; next; } + my $attribute_type = $attribute_types->{$attr->{code}}; # FIXME Do we need that here or let ->store do the job? - unless ( $attr->type ) { - warn "Cannot merge element: unrecognized code = '$attr->code'"; + unless ( $attribute_type ) { + warn "Cannot merge element: unrecognized code = '$attr->{code}'"; next; } - unless ( $attr->type->repeatable ) { + unless ( $attribute_type->{repeatable} ) { # filter out any existing attributes of the same code - @merged = grep {$attr->code ne $_->code} @merged; + @merged = grep {$attr->{code} ne $_->code} @merged; } push @merged, $attr; diff --git a/Koha/Patrons/Import.pm b/Koha/Patrons/Import.pm index bc6f8c6cf4..413aae3a12 100644 --- a/Koha/Patrons/Import.pm +++ b/Koha/Patrons/Import.pm @@ -24,7 +24,6 @@ use Text::CSV; use Encode qw( decode_utf8 ); use C4::Members; -use C4::Members::Attributes qw(:all); use Koha::Libraries; use Koha::Patrons; @@ -156,8 +155,8 @@ sub import_patrons { next LINE; } - # Set patron attributes if extended. - my $patron_attributes = $self->set_patron_attributes($extended, $borrower{patron_attributes}, \@feedback); + # Generate patron attributes if extended. + my $patron_attributes = $self->generate_patron_attributes($extended, $borrower{patron_attributes}, \@feedback); if( $extended ) { delete $borrower{patron_attributes}; } # Not really a field in borrowers. # Default date enrolled and date expiry if not already set. @@ -298,8 +297,6 @@ sub import_patrons { } if ($extended) { if ($ext_preserve) { - my $old_attributes = $patron->extended_attributes->as_list; - $patron_attributes = $patron->extended_attributes->merge_with( $patron_attributes ); } eval { @@ -465,29 +462,38 @@ sub set_column_keys { return @columnkeys; } -=head2 set_patron_attributes +=head2 generate_patron_attributes - my $patron_attributes = set_patron_attributes($extended, $borrower{patron_attributes}, $feedback); + my $patron_attributes = generate_patron_attributes($extended, $borrower{patron_attributes}, $feedback); -Returns a reference to array of hashrefs data structure as expected by Koha::Patron->extended_attributes +Returns a Koha::Patron::Attributes as expected by Koha::Patron->extended_attributes =cut -sub set_patron_attributes { - my ($self, $extended, $patron_attributes, $feedback) = @_; +sub generate_patron_attributes { + my ($self, $extended, $string, $feedback) = @_; unless( $extended ) { return; } - unless( defined($patron_attributes) ) { return; } + unless( defined $string ) { return; } # Fixup double quotes in case we are passed smart quotes - $patron_attributes =~ s/\xe2\x80\x9c/"/g; - $patron_attributes =~ s/\xe2\x80\x9d/"/g; - - push (@$feedback, { feedback => 1, name => 'attribute string', value => $patron_attributes }); - - my $result = extended_attributes_code_value_arrayref($patron_attributes); - - return $result; + $string =~ s/\xe2\x80\x9c/"/g; + $string =~ s/\xe2\x80\x9d/"/g; + + push (@$feedback, { feedback => 1, name => 'attribute string', value => $string }); + return [] unless $string; # Unit tests want the feedback, is it really needed? + + my $csv = Text::CSV->new({binary => 1}); # binary needed for non-ASCII Unicode + my $ok = $csv->parse($string); # parse field again to get subfields! + my @list = $csv->fields(); + my @patron_attributes = + sort { $a->{code} cmp $b->{code} || $a->{value} cmp $b->{value} } + map { + my @arr = split /:/, $_, 2; + { code => $arr[0], attribute => $arr[1] } + } @list; + return \@patron_attributes; + # TODO: error handling (check $ok) } =head2 check_branch_code diff --git a/members/memberentry.pl b/members/memberentry.pl index e78668196f..8ddfa49900 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -30,7 +30,6 @@ use C4::Auth; use C4::Context; use C4::Output; use C4::Members; -use C4::Members::Attributes; use C4::Koha; use C4::Log; use C4::Letters; diff --git a/t/db_dependent/Koha/Patrons/Import.t b/t/db_dependent/Koha/Patrons/Import.t index 0ca456cc38..e7af90a3a7 100644 --- a/t/db_dependent/Koha/Patrons/Import.t +++ b/t/db_dependent/Koha/Patrons/Import.t @@ -51,7 +51,7 @@ subtest 'test_methods' => sub { 'set_attribute_types', 'prepare_columns', 'set_column_keys', - 'set_patron_attributes', + 'generate_patron_attributes', 'check_branch_code', 'format_dates', ); @@ -217,6 +217,7 @@ my $params_4 = { file => $handle_4, matchpoint => $attribute->{code}, }; # When ... my $result_4 = $patrons_import->import_patrons($params_4); +use Data::Printer colored => 1; warn p $result_4; # Then ... is($result_4->{already_in_db}, 0, 'Got the expected 0 already_in_db from import_patrons with extended user'); @@ -528,19 +529,19 @@ subtest 'test_set_column_keys' => sub { is(scalar @columnkeys_1, @columns - 1 + $extended, 'Got the expected array size from set column keys with extended'); }; -subtest 'test_set_patron_attributes' => sub { +subtest 'test_generate_patron_attributes' => sub { plan tests => 13; # Given ... nothing at all # When ... Then ... - my $result_0 = $patrons_import->set_patron_attributes(undef, undef, undef); + my $result_0 = $patrons_import->generate_patron_attributes(undef, undef, undef); is($result_0, undef, 'Got the expected undef from set patron attributes with nothing'); # Given ... not extended. my $extended_1 = 0; # When ... Then ... - my $result_1 = $patrons_import->set_patron_attributes($extended_1, undef, undef); + my $result_1 = $patrons_import->generate_patron_attributes($extended_1, undef, undef); is($result_1, undef, 'Got the expected undef from set patron attributes with not extended'); # Given ... NO patrons attributes @@ -549,7 +550,7 @@ subtest 'test_set_patron_attributes' => sub { my @feedback_2; # When ... - my $result_2 = $patrons_import->set_patron_attributes($extended_2, $patron_attributes_2, \@feedback_2); + my $result_2 = $patrons_import->generate_patron_attributes($extended_2, $patron_attributes_2, \@feedback_2); # Then ... is($result_2, undef, 'Got the expected undef from set patron attributes with no patrons attributes'); @@ -560,7 +561,7 @@ subtest 'test_set_patron_attributes' => sub { my @feedback_3; # When ... - my $result_3 = $patrons_import->set_patron_attributes($extended_2, $patron_attributes_3, \@feedback_3); + my $result_3 = $patrons_import->generate_patron_attributes($extended_2, $patron_attributes_3, \@feedback_3); # Then ... ok($result_3, 'Got some data back from set patron attributes'); diff --git a/t/db_dependent/Members/Attributes.t b/t/db_dependent/Members/Attributes.t deleted file mode 100644 index 33aa063d95..0000000000 --- a/t/db_dependent/Members/Attributes.t +++ /dev/null @@ -1,228 +0,0 @@ -#!/usr/bin/perl - -# This file is part of Koha. -# -# Copyright 2014 Biblibre SARL -# -# Koha is free software; you can redistribute it and/or modify it -# under the terms of the GNU General Public License as published by -# the Free Software Foundation; either version 3 of the License, or -# (at your option) any later version. -# -# Koha is distributed in the hope that it will be useful, but -# WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with Koha; if not, see . - -use Modern::Perl; - -use C4::Context; -use C4::Members; -use Koha::Database; -use t::lib::TestBuilder; -use t::lib::Mocks; - -use Test::Exception; -use Test::More tests => 33; - -use_ok('C4::Members::Attributes'); - -my $schema = Koha::Database->schema; -$schema->storage->txn_begin; -my $builder = t::lib::TestBuilder->new; -my $dbh = C4::Context->dbh; -$dbh->{RaiseError} = 1; - -$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 $patron = $builder->build( - { source => 'Borrower', - value => { - firstname => 'my firstname', - surname => 'my surname', - branchcode => $library->{branchcode}, - } - } -); -t::lib::Mocks::mock_userenv({ branchcode => $library->{branchcode} }); -my $borrowernumber = $patron->{borrowernumber}; - -my $attribute_type1 = Koha::Patron::Attribute::Type->new( - { - code => 'my code1', - description => 'my description1', - unique_id => 1 - } -)->store; -my $attribute_type2 = Koha::Patron::Attribute::Type->new( - { - code => 'my code2', - description => 'my description2', - opac_display => 1, - staff_searchable => 1 - } -)->store; - -my $new_library = $builder->build( { source => 'Branch' } ); -my $attribute_type_limited = Koha::Patron::Attribute::Type->new( - { code => 'my code3', description => 'my description3' } )->store; -$attribute_type_limited->library_limits( [ $new_library->{branchcode} ] ); - -$patron = Koha::Patrons->find($borrowernumber); -my $borrower_attributes = $patron->extended_attributes; -is( $borrower_attributes->count, 0, 'extended_attributes returns the correct number of borrower attributes' ); - -my $attributes = [ - { - attribute => 'my attribute1', - code => $attribute_type1->code(), - }, - { - attribute => 'my attribute2', - code => $attribute_type2->code(), - }, - { - attribute => 'my attribute limited', - code => $attribute_type_limited->code(), - } -]; - -$patron->extended_attributes->filter_by_branch_limitations->delete; -my $set_borrower_attributes = $patron->extended_attributes($attributes); -is( ref($set_borrower_attributes), 'Koha::Patron::Attributes', ''); -is( $set_borrower_attributes->count, 3, ''); -$borrower_attributes = $patron->extended_attributes; -is( $borrower_attributes->count, 3, 'extended_attributes returns the correct number of borrower attributes' ); -my $attr_0 = $borrower_attributes->next; -is( $attr_0->code, $attributes->[0]->{code}, 'setter for extended_attributes sestores the correct code correctly' ); -is( $attr_0->type->description, $attribute_type1->description(), 'setter for extended_attributes stores the field description correctly' ); -is( $attr_0->attribute, $attributes->[0]->{attribute}, 'setter for extended_attributes stores the field value correctly' ); -my $attr_1 = $borrower_attributes->next; -is( $attr_1->code, $attributes->[1]->{code}, 'setter for extended_attributes stores the field code correctly' ); -is( $attr_1->type->description, $attribute_type2->description(), 'setter for extended_attributes stores the field description correctly' ); -is( $attr_1->attribute, $attributes->[1]->{attribute}, 'setter for extended_attributes stores the field value correctly' ); - -$attributes = [ - { - attribute => 'my attribute1', - code => $attribute_type1->code(), - }, - { - attribute => 'my attribute2', - code => $attribute_type2->code(), - } -]; -$patron->extended_attributes->filter_by_branch_limitations->delete; -$patron->extended_attributes($attributes); -$borrower_attributes = $patron->extended_attributes; -is( $borrower_attributes->count, 3, 'setter for extended_attributes 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' ); - -$patron = Koha::Patrons->find($borrowernumber); -my $extended_attributes = $patron->extended_attributes; -my $attribute_value = $extended_attributes->search({ code => 'my invalid code' }); -is( $attribute_value->count, 0, 'non existent attribute should return empty result set'); -$attribute_value = $patron->get_extended_attribute('my invalid code'); -is( $attribute_value, undef, 'non existent attribute should undef'); - -$attribute_value = $patron->get_extended_attribute($attributes->[0]->{code}); -is( $attribute_value->attribute, $attributes->[0]->{attribute}, 'get_extended_attribute returns the correct attribute value' ); -$attribute_value = $patron->get_extended_attribute($attributes->[1]->{code}); -is( $attribute_value->attribute, $attributes->[1]->{attribute}, 'get_extended_attribute returns the correct attribute value' ); - - -my $attribute = { - attribute => 'my attribute3', - code => $attribute_type1->code(), -}; -$patron->extended_attributes->search({'me.code' => $attribute->{code}})->filter_by_branch_limitations->delete; -$patron->add_extended_attribute($attribute); -$borrower_attributes = $patron->extended_attributes; -is( $borrower_attributes->count, 3, 'delete then add a new attribute does not change the number of borrower attributes' ); -$attr_0 = $borrower_attributes->next; -is( $attr_0->code, $attribute->{code}, 'delete then add a new attribute updates the field code correctly' ); -is( $attr_0->type->description, $attribute_type1->description(), 'delete then add a new attribute updates the field description correctly' ); -is( $attr_0->attribute, $attribute->{attribute}, 'delete then add a new attribute updates the field value correctly' ); - - -lives_ok { # Editing, new value, same patron - Koha::Patron::Attribute->new( - { - code => $attribute->{code}, - attribute => 'new value', - borrowernumber => $patron->borrowernumber - } - )->check_unique_id; -} 'no exception raised'; -lives_ok { # Editing, same value, same patron - Koha::Patron::Attribute->new( - { - code => $attributes->[1]->{code}, - attribute => $attributes->[1]->{attribute}, - borrowernumber => $patron->borrowernumber - } - )->check_unique_id; -} 'no exception raised'; -throws_ok { # Creating a new one, but already exists! - Koha::Patron::Attribute->new( - { code => $attribute->{code}, attribute => $attribute->{attribute} } ) - ->check_unique_id; -} 'Koha::Exceptions::Patron::Attribute::UniqueIDConstraint'; - - -$patron->get_extended_attribute($attribute->{code})->delete; -$borrower_attributes = $patron->extended_attributes; -is( $borrower_attributes->count, 2, 'delete attribute by code' ); -$attr_0 = $borrower_attributes->next; -is( $attr_0->code, $attributes->[1]->{code}, 'delete attribute by code'); -is( $attr_0->type->description, $attribute_type2->description(), 'delete attribute by code'); -is( $attr_0->attribute, $attributes->[1]->{attribute}, 'delete attribute by code'); - -$patron->get_extended_attribute($attributes->[1]->{code})->delete; -$borrower_attributes = $patron->extended_attributes; -is( $borrower_attributes->count, 1, 'delete attribute by code' ); - -# Regression tests for bug 16504 -t::lib::Mocks::mock_userenv({ branchcode => $new_library->{branchcode} }); -my $another_patron = $builder->build_object( - { class => 'Koha::Patrons', - value => { - firstname => 'my another firstname', - surname => 'my another surname', - branchcode => $new_library->{branchcode}, - } - } -); -$attributes = [ - { - attribute => 'my attribute1', - code => $attribute_type1->code(), - }, - { - attribute => 'my attribute2', - code => $attribute_type2->code(), - }, - { - attribute => 'my attribute limited', - code => $attribute_type_limited->code(), - } -]; - -$another_patron->extended_attributes->filter_by_branch_limitations->delete; -$another_patron->extended_attributes($attributes); -$borrower_attributes = $another_patron->extended_attributes; -is( $borrower_attributes->count, 3, 'setter for extended_attributes should have added the 3 attributes for another patron'); -$borrower_attributes = $patron->extended_attributes; -is( $borrower_attributes->count, 1, 'setter for extended_attributes should not have removed the attributes of other patrons' ); diff --git a/t/db_dependent/Utils/Datatables_Members.t b/t/db_dependent/Utils/Datatables_Members.t index f9459b54d5..7a77ea4ddd 100644 --- a/t/db_dependent/Utils/Datatables_Members.t +++ b/t/db_dependent/Utils/Datatables_Members.t @@ -22,8 +22,6 @@ use Test::More tests => 51; use C4::Context; use C4::Members; -use C4::Members::Attributes; - use Koha::Library; use Koha::Patrons; use Koha::Patron::Categories; diff --git a/tools/modborrowers.pl b/tools/modborrowers.pl index 5c3b5f7a5b..7ba01f638f 100755 --- a/tools/modborrowers.pl +++ b/tools/modborrowers.pl @@ -30,7 +30,6 @@ use CGI qw ( -utf8 ); use C4::Auth; use C4::Koha; use C4::Members; -use C4::Members::Attributes; use C4::Output; use List::MoreUtils qw /any uniq/; use Koha::DateUtils qw( dt_from_string ); -- 2.39.5