From 339d9b21270b472ed7f2f5b5f7b16e0a88d512b7 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 | 126 ++++++++++---- members/memberentry.pl | 50 +----- t/db_dependent/Koha/Patron/Attribute.t | 227 ++++++++++++++++++++++++- 3 files changed, 326 insertions(+), 77 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 20ac75196d8..905d71fb562 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; @@ -2241,42 +2242,103 @@ Or setter FIXME sub extended_attributes { my ( $self, $attributes ) = @_; if ($attributes) { # setter - 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 - my $new_types = {}; - for my $attribute (@$attributes) { - $self->add_extended_attribute($attribute); - $new_types->{$attribute->{code}} = 1; - } - # Check globally mandatory types - my $interface = C4::Context->interface; - my $params = { - mandatory => 1, - category_code => [ undef, $self->categorycode ], - 'borrower_attribute_types_branches.b_branchcode' => undef, - }; + my $new_types = {}; + for my $attribute (@{$attributes}) { + $new_types->{$attribute->{code}} = 1; + } - if ( $interface eq 'opac' ) { - $params->{opac_editable} = 1; - } + # Check globally mandatory types + my $interface = C4::Context->interface; + my $params = { + mandatory => 1, + category_code => [ undef, $self->categorycode ], + 'borrower_attribute_types_branches.b_branchcode' => undef, + }; + + if ( $interface eq 'opac' ) { + $params->{opac_editable} = 1; + } + + my @required_attribute_types = Koha::Patron::Attribute::Types->search( + $params, + { join => 'borrower_attribute_types_branches' } + )->get_column('code'); + for my $type (@required_attribute_types) { + Koha::Exceptions::Patron::MissingMandatoryExtendedAttribute->throw( + type => $type, + ) if !$new_types->{$type}; + } + + my %attribute_changes; - my @required_attribute_types = Koha::Patron::Attribute::Types->search( - $params, - { join => 'borrower_attribute_types_branches' } - )->get_column('code'); - for my $type (@required_attribute_types) { - Koha::Exceptions::Patron::MissingMandatoryExtendedAttribute->throw( - type => $type, - ) if !$new_types->{$type}; + # 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}; + + # 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 $schema = $self->_result->result_source->schema; + my $is_different = sub { + my ($a, $b) = map { [ sort @{$_} ] } @_; + return @{$a} != @{$b} || notall { $_->[0] eq $_->[1] } zip6 @{$a}, @{$b}; + }; + 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] : ''; + } + $schema->txn_do( + sub { + # Remove existing + $self->extended_attributes->filter_by_branch_limitations->search({ + 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 $rs = $self->_result->borrower_attributes; 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