diff --git a/Koha/Patron.pm b/Koha/Patron.pm index ec7f670498..ee93d9b355 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -20,11 +20,12 @@ package Koha::Patron; use Modern::Perl; -use List::MoreUtils qw( any none uniq ); +use List::MoreUtils qw( any none uniq notall zip6); use JSON qw( to_json ); use Unicode::Normalize qw( NFKD ); use Try::Tiny; use DateTime (); +use C4::Log qw( logaction ); use C4::Auth qw( checkpw_hash ); use C4::Context; @@ -2255,17 +2256,91 @@ Or setter FIXME sub extended_attributes { my ( $self, $attributes ) = @_; + if ($attributes) { # setter + my %attribute_changes; + + # Stash changes before + for my $attribute ( $self->extended_attributes->as_list ) { + my $repeatable = $attribute->type->repeatable ? 1 : 0; + $attribute_changes{$repeatable}->{ $attribute->code }->{before} //= []; + push( + @{ $attribute_changes{$repeatable}->{ $attribute->code }->{before} }, + $attribute->attribute + ); + } + my @new_attributes = map { + Koha::Patron::Attribute->new( + { + %{$_}, + ( borrowernumber => $self->borrowernumber ), + } + ) + } @{$attributes}; + + # Make sure all attribute types are valid + for my $attribute (@new_attributes) { + $attribute->validate_type(); + } + + # Sort new attributes by code + @new_attributes = sort { $a->attribute cmp $b->attribute } @new_attributes; + + # Stash changes after + for my $attribute ( values @new_attributes ) { + my $repeatable = $attribute->type->repeatable ? 1 : 0; + $attribute_changes{$repeatable}->{ $attribute->code }->{after} //= []; + push( + @{ $attribute_changes{$repeatable}->{ $attribute->code }->{after} }, + $attribute->attribute + ); + } + + my $is_different = sub { + my ( $a, $b ) = map { [ sort @{$_} ] } @_; + return @{$a} != @{$b} || notall { $_->[0] eq $_->[1] } zip6 @{$a}, @{$b}; + }; + my $schema = $self->_result->result_source->schema; $schema->txn_do( sub { - # Remove the existing one - $self->extended_attributes->filter_by_branch_limitations->delete; + while ( my ( $repeatable, $changes ) = each %attribute_changes ) { + while ( my ( $code, $change ) = each %{$changes} ) { + $change->{before} //= []; + $change->{after} //= []; - # Insert the new ones + if ( $is_different->( $change->{before}, $change->{after} ) ) { + unless ($repeatable) { + $change->{before} = @{ $change->{before} } ? $change->{before}->[0] : ''; + $change->{after} = @{ $change->{after} } ? $change->{after}->[0] : ''; + } + + # Remove existing + $self->extended_attributes->filter_by_branch_limitations->search( + { + 'me.code' => $code, + } + )->delete; + + # Add possible new attribute values + for my $attribute (@new_attributes) { + $attribute->store() if ( $attribute->code eq $code ); + } + if ( C4::Context->preference("BorrowersLog") ) { + logaction( + "MEMBERS", + "MODIFY", + $self->borrowernumber, + "Patron attribute " + . $code . ": " + . to_json( $change, { pretty => 1, canonical => 1 } ) + ); + } + } + } + } my $new_types = {}; - for my $attribute (@$attributes) { - $self->add_extended_attribute($attribute); + for my $attribute ( @{$attributes} ) { $new_types->{ $attribute->{code} } = 1; } diff --git a/Koha/Patron/Attribute.pm b/Koha/Patron/Attribute.pm index 93b25ece02..d9055483d2 100644 --- a/Koha/Patron/Attribute.pm +++ b/Koha/Patron/Attribute.pm @@ -35,6 +35,25 @@ Koha::Patron::Attribute - Koha Patron Attribute Object class =cut +=head3 validate_type + + my $attribute = Koha::Patron::Attribute->new({ code => 'a_code', ... }); + try { $attribute->validate_type } + catch { handle_exception }; + +Validate type of C object, used in extended_attributes +to validate type when attribute has not yet been stored. + +=cut + +sub validate_type { + + my $self = shift; + + Koha::Exceptions::Patron::Attribute::InvalidType->throw( type => $self->code ) + unless $self->type; +} + =head3 store my $attribute = Koha::Patron::Attribute->new({ code => 'a_code', ... }); @@ -47,8 +66,7 @@ sub store { my $self = shift; - Koha::Exceptions::Patron::Attribute::InvalidType->throw( type => $self->code ) - unless $self->type; + $self->validate_type(); Koha::Exceptions::Patron::Attribute::NonRepeatable->throw( attribute => $self ) unless $self->repeatable_ok(); diff --git a/members/memberentry.pl b/members/memberentry.pl index 0b8107ff19..0794a9cff3 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -299,8 +299,7 @@ foreach my $guarantor (@guarantors) { if ( ( $op eq 'cud-save' || $op eq 'cud-insert' ) && ( $guarantor->is_child || $guarantor->is_guarantee || ( $patron && $patron->is_guarantor ) ) ) { - push @errors, 'ERROR_child_is_guarantor' if ( $guarantor->is_child ); - push @errors, 'ERROR_guarantor_is_guarantee' if ( !$guarantor->is_child ); + push @errors, 'ERROR_guarantor_is_guarantee'; } } @@ -628,46 +627,7 @@ if ( ( !$nok ) and $nodouble and ( $op eq 'cud-insert' or $op eq 'cud-save' ) ) if ( C4::Context->preference('ExtendedPatronAttributes') and $input->param('setting_extended_patron_attributes') ) { - my $existing_attributes = $patron->extended_attributes->filter_by_branch_limitations->unblessed; - - my $needs_update = 1; - - # If there are an unqueunal number of new and old patron attributes they definitely need updated - if ( scalar @{$existing_attributes} == scalar @{$extended_patron_attributes} ) { - my $seen = 0; - for ( my $i = 0 ; $i <= scalar @{$extended_patron_attributes} ; $i++ ) { - my $new_attr = $extended_patron_attributes->[$i]; - next unless $new_attr; - for ( my $j = 0 ; $j <= scalar @{$existing_attributes} ; $j++ ) { - my $existing_attr = $existing_attributes->[$j]; - next unless $existing_attr; - - if ( $new_attr->{attribute} eq $existing_attr->{attribute} - && $new_attr->{borrowernumber} eq $existing_attr->{borrowernumber} - && $new_attr->{code} eq $existing_attr->{code} ) - { - $seen++; - - # Remove the match from the "old" attribute - splice( @{$existing_attributes}, $j, 1 ); - - # Move on to look at the next "new" attribute - last; - } - } - } - - # If we found a match for each existing attribute and the number of see attributes matches the number seen - # we don't need to update the attributes - if ( scalar @{$existing_attributes} == 0 && $seen == @{$extended_patron_attributes} ) { - $needs_update = 0; - } - } - - if ($needs_update) { - $patron->extended_attributes->filter_by_branch_limitations->delete; - $patron->extended_attributes($extended_patron_attributes); - } + $patron->extended_attributes($extended_patron_attributes); } if ( diff --git a/t/db_dependent/Koha/Patron/Attribute.t b/t/db_dependent/Koha/Patron/Attribute.t index e233a39050..3817f0877f 100755 --- a/t/db_dependent/Koha/Patron/Attribute.t +++ b/t/db_dependent/Koha/Patron/Attribute.t @@ -18,15 +18,18 @@ # along with Koha; if not, see . use Modern::Perl; +use JSON qw( to_json ); -use Test::More tests => 3; +use Test::More tests => 4; use t::lib::TestBuilder; +use t::lib::Mocks; use Test::Exception; use Koha::Database; use Koha::Patron::Attribute; use Koha::Patron::Attributes; +use Koha::ActionLogs; my $schema = Koha::Database->new->schema; my $builder = t::lib::TestBuilder->new; @@ -519,3 +522,224 @@ subtest 'merge_and_replace_with' => sub { $schema->storage->txn_rollback; }; + +subtest 'action log tests' => sub { + plan tests => 11; + my $schema = Koha::Database->new->schema; + $schema->storage->txn_begin; + + my $get_info = sub { + my ( $before, $after, $code, $repeatable ) = @_; + my $change = { + before => $before, + after => $after + }; + if ($repeatable) { + while ( my ( $k, $v ) = each %{$change} ) { + if ( ref $v eq 'ARRAY' ) { + $change->{$k} = [ sort @{$v} ]; + } else { + $change->{$k} = $v ? [$v] : []; + } + } + } + return "Patron attribute " . $code . ": " . to_json( $change, { pretty => 1, canonical => 1 } ); + }; + + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + my $attribute_type = $builder->build_object( + { + class => 'Koha::Patron::Attribute::Types', + value => { repeatable => 0 } + } + ); + + t::lib::Mocks::mock_preference( 'BorrowersLog', 0 ); + my $attributes = [ + { + attribute => 'Foo', + code => $attribute_type->code, + } + ]; + $patron->extended_attributes($attributes); + + my $info = $get_info->( '', 'Foo', $attribute_type->code ); + my $action_logs = Koha::ActionLogs->search( + { + module => "MEMBERS", + action => "MODIFY", + object => $patron->borrowernumber, + info => $info + } + ); + is( + $action_logs->count, + 0, + 'No action log entry has been created when adding patron attribute if BorrowersLog syspref disabled' + ); + + t::lib::Mocks::mock_preference( 'BorrowersLog', 1 ); + my $current_action_logs_count; + my $repeatable_text; + for my $repeatable ( 0, 1 ) { + $repeatable_text = $repeatable ? ' repeatable' : ''; + + $patron->extended_attributes( [] ); + + $attribute_type = $builder->build_object( + { + class => 'Koha::Patron::Attribute::Types', + value => { repeatable => $repeatable } + } + ); + $attributes = [ + { + attribute => 'Foo', + code => $attribute_type->code, + } + ]; + + $patron->extended_attributes($attributes); + $info = $get_info->( '', 'Foo', $attribute_type->code, $repeatable ); + $action_logs = Koha::ActionLogs->search( + { + module => "MEMBERS", + action => "MODIFY", + object => $patron->borrowernumber, + info => $info + } + ); + is( + $action_logs->count, + 1, + "An action log entry has been created when adding$repeatable_text patron attribute" + ); + + $current_action_logs_count = Koha::ActionLogs->search( + { + module => "MEMBERS", + action => "MODIFY", + object => $patron->borrowernumber + } + )->count; + + $patron->extended_attributes($attributes); + $action_logs = Koha::ActionLogs->search( + { + module => "MEMBERS", + action => "MODIFY", + object => $patron->borrowernumber + } + ); + is( + $action_logs->count, + $current_action_logs_count, + "No additional action log entry has been created when updating$repeatable_text patron attribute with same value" + ); + + $attributes = [ + { + attribute => 'Bar', + code => $attribute_type->code, + } + ]; + $patron->extended_attributes($attributes); + $info = $get_info->( 'Foo', 'Bar', $attribute_type->code, $repeatable ); + $action_logs = Koha::ActionLogs->search( + { + module => "MEMBERS", + action => "MODIFY", + object => $patron->borrowernumber, + info => $info + } + ); + is( + $action_logs->count, + 1, + "New action log entry has been created when updating$repeatable_text patron attribute with different value" + ); + + $patron->extended_attributes( [] ); + $info = $get_info->( 'Bar', '', $attribute_type->code, $repeatable ); + $action_logs = Koha::ActionLogs->search( + { + module => "MEMBERS", + action => "MODIFY", + object => $patron->borrowernumber, + info => $info + } + ); + is( + $action_logs->count, + 1, + "New action log entry has been created when deleting$repeatable_text patron attribute value" + ); + } + + $attributes = [ + { + attribute => 'Foo', + code => $attribute_type->code, + }, + { + attribute => 'Bar', + code => $attribute_type->code, + } + ]; + $patron->extended_attributes($attributes); + + $info = $get_info->( [], [ 'Foo', 'Bar' ], $attribute_type->code, 1 ); + use Data::Dumper; + print Dumper($info); + $action_logs = Koha::ActionLogs->search( + { + module => "MEMBERS", + action => "MODIFY", + object => $patron->borrowernumber, + info => $info + } + ); + is( + $action_logs->count, + 1, + "New action log entry has been created when updating repeatable patron attribute with multiple values" + ); + + $attributes = [ + { + attribute => 'Foo', + code => $attribute_type->code, + }, + { + attribute => 'Bar', + code => $attribute_type->code, + }, + { + attribute => 'Baz', + code => $attribute_type->code, + } + ]; + $patron->extended_attributes($attributes); + + $info = $get_info->( + [ 'Foo', 'Bar' ], + [ 'Foo', 'Bar', 'Baz' ], + $attribute_type->code, + 1 + ); + $action_logs = Koha::ActionLogs->search( + { + module => "MEMBERS", + action => "MODIFY", + object => $patron->borrowernumber, + info => $info + } + ); + is( + $action_logs->count, + 1, + "New action log entry has been created when updating repeatable patron attribute with existing multiple values with multiple values" + ); + + $schema->storage->txn_rollback; +};