From e50503050663082f3a7f04368f6111a7f3dd3098 Mon Sep 17 00:00:00 2001 From: Stefan Berndtsson Date: Fri, 22 Sep 2023 13:14:03 +0200 Subject: [PATCH] Bug 26744: Log changes to extended patron attributes Log non-repeatable attributes and only log changes and creations, not deletions. When changed, attributes are first all deleted, then the relevant ones are recreated. This patch catches all deletions, and checks against them in the creation phase, logging only the attributes that have changed (with before/after content). This makes it impossible to log actual deletions. To test: 1) Ensure tests in t/db_dependent/Koha/Patron/Attribute.t passes Sponsored-by: Gothenburg University Library Signed-off-by: Lucas Gass Signed-off-by: Clemens Tubach Signed-off-by: Andrew Fuerste Henry Signed-off-by: Lucas Gass --- Koha/Patron.pm | 87 +++++++++- Koha/Patron/Attribute.pm | 22 ++- members/memberentry.pl | 44 +---- t/db_dependent/Koha/Patron/Attribute.t | 226 ++++++++++++++++++++++++- 4 files changed, 328 insertions(+), 51 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index ec7f6704983..ee93d9b3557 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} //= []; + + 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; - # Insert the new ones + # 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 93b25ece028..d9055483d21 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 c2df4523366..fa8df66b560 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -297,8 +297,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'; } } @@ -626,46 +625,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 e233a39050a..3817f0877f1 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; +}; -- 2.39.5