From c54ec504515a1e2a29152214d07125692d2d529f Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Tue, 17 Jul 2018 09:50:10 -0300 Subject: [PATCH] Bug 20443: Remove extended_attributes_merge TODO We need tests here! Signed-off-by: Nick Clemens --- C4/Members/Attributes.pm | 46 +----------------- Koha/Patron/Attributes.pm | 28 +++++++++++ Koha/Patrons/Import.pm | 3 +- t/Members_Attributes.t | 98 --------------------------------------- 4 files changed, 32 insertions(+), 143 deletions(-) delete mode 100755 t/Members_Attributes.t diff --git a/C4/Members/Attributes.pm b/C4/Members/Attributes.pm index 76cd6a9b32..127ad4e735 100644 --- a/C4/Members/Attributes.pm +++ b/C4/Members/Attributes.pm @@ -31,7 +31,7 @@ our ($csv, $AttributeTypes); BEGIN { @ISA = qw(Exporter); @EXPORT_OK = qw( - extended_attributes_code_value_arrayref extended_attributes_merge + extended_attributes_code_value_arrayref ); %EXPORT_TAGS = ( all => \@EXPORT_OK ); } @@ -61,6 +61,7 @@ Caches Text::CSV parser object for efficiency. 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(); @@ -73,49 +74,6 @@ sub extended_attributes_code_value_arrayref { # nested map because of split } -=head2 extended_attributes_merge - - my $old_attributes = extended_attributes_code_value_arrayref("homeroom:224,grade:04,deanslist:2007,deanslist:2008,somedata:xxx"); - my $new_attributes = extended_attributes_code_value_arrayref("homeroom:115,grade:05,deanslist:2009,extradata:foobar"); - my $merged = extended_attributes_merge($patron_attributes, $new_attributes, 1); - - # assuming deanslist is a repeatable code, value same as: - # $merged = extended_attributes_code_value_arrayref("homeroom:115,grade:05,deanslist:2007,deanslist:2008,deanslist:2009,extradata:foobar,somedata:xxx"); - -Takes three arguments. The first two are references to array of hashrefs, each like: - [ { code => 'CODE', value => 'value' }, { code => 'CODE2', value => 'othervalue' } ... ] - -The third option specifies whether repeatable codes are clobbered or collected. True for non-clobber. - -Returns one reference to (merged) array of hashref. - -Caches results of attribute types for efficiency. # FIXME That is very bad and can cause side-effects undef plack - -=cut - -sub extended_attributes_merge { - my $old = shift or return; - my $new = shift or return $old; - my $keep = @_ ? shift : 0; - $AttributeTypes or $AttributeTypes = Koha::Patron::Attribute::Types->search->unblessed; - my @merged = @$old; - foreach my $att (@$new) { - unless ($att->{code}) { - warn "Cannot merge element: no 'code' defined"; - next; - } - unless ($AttributeTypes->{$att->{code}}) { - warn "Cannot merge element: unrecognized code = '$att->{code}'"; - next; - } - unless ($AttributeTypes->{$att->{code}}->{repeatable} and $keep) { - @merged = grep {$att->{code} ne $_->{code}} @merged; # filter out any existing attributes of the same code - } - push @merged, $att; - } - return [( sort {&_sort_by_code($a,$b)} @merged )]; -} - sub _sort_by_code { my ($x, $y) = @_; defined ($x->{code}) or return -1; diff --git a/Koha/Patron/Attributes.pm b/Koha/Patron/Attributes.pm index 47ba594f74..9af07c0228 100644 --- a/Koha/Patron/Attributes.pm +++ b/Koha/Patron/Attributes.pm @@ -87,6 +87,34 @@ sub filter_by_branch_limitations { return $self->search( $or, $join ); } +sub merge_with { + my ( $self, $new_attributes ) = @_; + + my @merged = $self->as_list; + while ( my ( $attr ) = $new_attributes->next ) { + unless ( $attr->code ) { + warn "Cannot merge element: no 'code' defined"; + next; + } + + # FIXME Do we need that here or let ->store do the job? + unless ( $attr->type ) { + warn "Cannot merge element: unrecognized code = '$attr->code'"; + next; + } + unless ( $attr->type->repeatable ) { + # filter out any existing attributes of the same code + @merged = grep {$attr->code ne $_->code} @merged; + } + + push @merged, $attr; + } + + # WARNING - we would like to return a set, but $new_attributes is not in storage yet + # Maybe there is something obvious I (JD) am missing + return [ sort { $a->{code} cmp $b->{code} || $a->{attribute} cmp $b->{attribute} } @merged ]; +} + =head3 _type =cut diff --git a/Koha/Patrons/Import.pm b/Koha/Patrons/Import.pm index 9d76aeeec8..38c6eb69bd 100644 --- a/Koha/Patrons/Import.pm +++ b/Koha/Patrons/Import.pm @@ -290,7 +290,8 @@ sub import_patrons { if ($extended) { if ($ext_preserve) { my $old_attributes = $patron->extended_attributes->as_list; - $patron_attributes = extended_attributes_merge( $old_attributes, $patron_attributes ); + + $patron_attributes = $patron->extended_attributes->merge_with( $patron_attributes ); } eval { # We do not want to filter by branch, maybe we should? diff --git a/t/Members_Attributes.t b/t/Members_Attributes.t deleted file mode 100755 index 205d6117f1..0000000000 --- a/t/Members_Attributes.t +++ /dev/null @@ -1,98 +0,0 @@ -#!/usr/bin/perl -# -# - -use strict; -use warnings; - -use Test::More tests => 11; - -BEGIN { - use_ok('C4::Members::Attributes', qw(:all)); -} - -INIT { - $C4::Members::Attributes::AttributeTypes = { - 'grade' => { - 'opac_display' => '1', - 'staff_searchable' => '1', - 'description' => 'Grade level', - 'authorised_value_category' => '', - 'repeatable' => '0', - 'code' => 'grade', - 'unique_id' => '0' - }, - 'deanslist' => { - 'opac_display' => '0', - 'staff_searchable' => '1', - 'description' => 'Deans List (annual)', - 'authorised_value_category' => '', - 'repeatable' => '1', - 'code' => 'deanslist', - 'unique_id' => '0' - }, - 'somedata' => { - 'opac_display' => '0', - 'staff_searchable' => '0', - 'description' => 'Some Ext. Attribute', - 'authorised_value_category' => '', - 'repeatable' => '0', - 'code' => 'somedata', - 'unique_id' => '0' - }, - 'extradata' => { - 'opac_display' => '0', - 'staff_searchable' => '0', - 'description' => 'Another Ext. Attribute', - 'authorised_value_category' => '', - 'repeatable' => '0', - 'code' => 'extradata', - 'unique_id' => '0' - }, - 'school_id' => { - 'opac_display' => '1', - 'staff_searchable' => '1', - 'description' => 'School ID Number', - 'authorised_value_category' => '', - 'repeatable' => '0', - 'code' => 'school_id', - 'unique_id' => '1' - }, - 'homeroom' => { - 'opac_display' => '1', - 'staff_searchable' => '1', - 'description' => 'Homeroom', - 'authorised_value_category' => '', - 'repeatable' => '0', - 'code' => 'homeroom', - 'unique_id' => '0' - } - }; # This is important to prevent extended_attributes_merge from touching DB. -} - - -my @merge_tests = ( - { - line1 => "homeroom:501", - line2 => "grade:01", - merge => "homeroom:501,grade:01", - }, - { - line1 => "homeroom:224,grade:04,deanslist:2008,deanslist:2007,somedata:xxx", - line2 => "homeroom:115,grade:05,deanslist:2009,extradata:foobar", - merge => "homeroom:115,grade:05,deanslist:2008,deanslist:2007,deanslist:2009,extradata:foobar,somedata:xxx", - }, -); - -can_ok('C4::Members::Attributes', qw(extended_attributes_merge extended_attributes_code_value_arrayref)); - -ok(ref($C4::Members::Attributes::AttributeTypes) eq 'HASH', '$C4::Members::Attributes::AttributeTypes is a hashref'); - -foreach my $test (@merge_tests) { - my ($old, $new, $merged); - ok($old = extended_attributes_code_value_arrayref($test->{line1}), "extended_attributes_code_value_arrayref('$test->{line1}')"); - ok($new = extended_attributes_code_value_arrayref($test->{line2}), "extended_attributes_code_value_arrayref('$test->{line2}')"); - ok($merged = extended_attributes_merge($old, $new), "extended_attributes_merge(\$old, \$new)"); - ok($merged = extended_attributes_merge($old, $new, 1), "extended_attributes_merge(\$old, \$new, 1)"); -} - -- 2.20.1