From e8779682c55ec58361d9728641f96e65429fd485 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 --- Koha/Patron.pm | 80 ++++++++- Koha/Patron/Attribute.pm | 22 ++- members/memberentry.pl | 50 +----- t/db_dependent/Koha/Patron/Attribute.t | 227 ++++++++++++++++++++++++- 4 files changed, 325 insertions(+), 54 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 20ac75196d8..d998e77f226 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; @@ -2240,17 +2241,82 @@ 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; - - # Insert the new ones + 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; + # 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 f1fd9bfe5bd..cfa9f282d66 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -593,50 +593,12 @@ if ((!$nok) and $nodouble and ($op eq 'cud-insert' or $op eq 'cud-save')){ } } - if ($success) { - 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); - } + if ( $success ) { + if ( + C4::Context->preference('ExtendedPatronAttributes') and + $input->param('setting_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 e9e9f9c7fb7..8421d680574 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; @@ -510,3 +513,225 @@ 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.48.1