Bug 20443: Remove extended_attributes_code_value_arrayref AND C4::Members::Attributes
authorJonathan Druart <jonathan.druart@bugs.koha-community.org>
Tue, 17 Jul 2018 14:03:49 +0000 (11:03 -0300)
committerMartin Renvoize <martin.renvoize@ptfs-europe.com>
Mon, 23 Mar 2020 13:49:22 +0000 (13:49 +0000)
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

C4/Auth_with_ldap.pm
C4/Members/Attributes.pm [deleted file]
Koha/Patron/Attributes.pm
Koha/Patrons/Import.pm
members/memberentry.pl
t/db_dependent/Koha/Patrons/Import.t
t/db_dependent/Members/Attributes.t [deleted file]
t/db_dependent/Utils/Datatables_Members.t
tools/modborrowers.pl

index 020ade4..a85369b 100644 (file)
@@ -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 (file)
index 127ad4e..0000000
+++ /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 <http://www.gnu.org/licenses>.
-
-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 <http://koha-community.org/>
-
-Galen Charlton <galen.charlton@liblime.com>
-
-=cut
-
-1;
index 9af07c0..84685d9 100644 (file)
@@ -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;
index bc6f8c6..413aae3 100644 (file)
@@ -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
index e786681..8ddfa49 100755 (executable)
@@ -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;
index 0ca456c..e7af90a 100644 (file)
@@ -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 (file)
index 33aa063..0000000
+++ /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 <http://www.gnu.org/licenses>.
-
-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' );
index f9459b5..7a77ea4 100644 (file)
@@ -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;
index 5c3b5f7..7ba01f6 100755 (executable)
@@ -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 );