From 90027b0422d096f7c7e4340a4f51028e1b04b7b2 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 ec7f6704983..7f8783c303f 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,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 c2df4523366..e5f27b71d56 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -622,50 +622,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 e233a39050a..b6ab6fed6d7 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,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.34.1