From 76d2ae7e8ad4c2ef541756e2590d39cf64c0536a Mon Sep 17 00:00:00 2001 From: David Gustafsson Date: Thu, 28 Mar 2024 19:19:45 +0100 Subject: [PATCH] Bug 26744: Add support for repeatable and deleted attributes --- Koha/Patron.pm | 52 +++++++- Koha/Patron/Attribute.pm | 55 -------- t/db_dependent/Koha/Patron/Attribute.t | 171 ++++++++++++++++--------- 3 files changed, 162 insertions(+), 116 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 5b0bd9ea7f5..59330fe07dd 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -20,11 +20,12 @@ package Koha::Patron; use Modern::Perl; -use List::MoreUtils qw( any uniq ); +use List::MoreUtils qw( any 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; @@ -2124,6 +2125,21 @@ Or setter FIXME sub extended_attributes { my ( $self, $attributes ) = @_; if ($attributes) { # setter + my %attribute_changes; + my $attribute_key = sub { + my ($attribute) = @_; + return join(':', $attribute->code, $self->borrowernumber); + }; + if (C4::Context->preference("BorrowersLog")) { + for my $attribute ($self->extended_attributes->as_list) { + my $repeatable = $attribute->type->repeatable ? 1 : 0; + $attribute_changes{$repeatable}->{$attribute_key->($attribute)}->{before} = []; + push ( + @{$attribute_changes{$repeatable}->{$attribute_key->($attribute)}->{before}}, + $attribute->attribute + ); + } + } my $schema = $self->_result->result_source->schema; $schema->txn_do( sub { @@ -2155,6 +2171,40 @@ sub extended_attributes { } } ); + if (C4::Context->preference("BorrowersLog")) { + for my $attribute ($self->extended_attributes->as_list) { + my $repeatable = $attribute->type->repeatable ? 1 : 0; + $attribute_changes{$repeatable}->{$attribute_key->($attribute)}->{after} = []; + push ( + @{$attribute_changes{$repeatable}->{$attribute_key->($attribute)}->{after}}, + $attribute->attribute + ); + } + 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 ($attribute_key, $change) = each %{$changes}) { + my ($code) = split(':', $attribute_key); + $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] : ''; + } + logaction( + "MEMBERS", + "MODIFY", + $self->borrowernumber, + "Patron attribute " . $code . ": " . to_json($change, { pretty => 1, canonical => 1 }) + ); + } + } + } + } } my $rs = $self->_result->borrower_attributes; diff --git a/Koha/Patron/Attribute.pm b/Koha/Patron/Attribute.pm index cce32893e7f..aaaefbcbb80 100644 --- a/Koha/Patron/Attribute.pm +++ b/Koha/Patron/Attribute.pm @@ -21,14 +21,9 @@ use Koha::Database; use Koha::Exceptions::Patron::Attribute; use Koha::Patron::Attribute::Types; use Koha::AuthorisedValues; -use Koha::Cache::Memory::Lite; -use C4::Log qw( logaction ); -use JSON qw( to_json ); use base qw(Koha::Object); -our %deleted_attributes; - =head1 NAME Koha::Patron::Attribute - Koha Patron Attribute Object class @@ -60,33 +55,6 @@ sub store { Koha::Exceptions::Patron::Attribute::UniqueIDConstraint->throw( attribute => $self ) unless $self->unique_ok(); - # Only log for non-repeatable attributes - if (!$self->type->repeatable && C4::Context->preference("BorrowersLog")) { - my $change; - if ( exists $deleted_attributes{$self->_deleted_attribute_key} ) { - my $previous_attribute = $deleted_attributes{$self->_deleted_attribute_key}; - if ( $previous_attribute->attribute ne $self->attribute ) { - $change = { - before => $previous_attribute->attribute, - after => $self->attribute - }; - } - delete $deleted_attributes{$self->_deleted_attribute_key}; - } else { - $change = { - before => "", - after => $self->attribute - }; - } - if (defined $change) { - logaction( - "MEMBERS", - "MODIFY", - $self->borrowernumber, - "Patron attribute " . $self->code . ": " . to_json($change, { pretty => 1, canonical => 1 }) - ); - } - } return $self->SUPER::store(); } @@ -221,20 +189,6 @@ sub unique_ok { return $ok; } -=head3 delete - -Overloaded I method to temporarily stash deleted attribute for action log. - -=cut - -sub delete { - my ($self) = @_; - if (!$self->type->repeatable && C4::Context->preference("BorrowersLog")) { - $deleted_attributes{$self->_deleted_attribute_key} = $self; - } - $self->SUPER::delete(); -} - =head2 Internal methods =head3 _type @@ -245,13 +199,4 @@ sub _type { return 'BorrowerAttribute'; } -=head3 _deleted_attribute_key - -=cut - -sub _deleted_attribute_key { - my ($self) = @_; - return join(':', $self->borrowernumber, $self->code); -} - 1; diff --git a/t/db_dependent/Koha/Patron/Attribute.t b/t/db_dependent/Koha/Patron/Attribute.t index d4307f3e496..705acee8101 100755 --- a/t/db_dependent/Koha/Patron/Attribute.t +++ b/t/db_dependent/Koha/Patron/Attribute.t @@ -409,10 +409,24 @@ subtest 'merge_and_replace_with' => sub { }; subtest 'action log tests' => sub { - plan tests => 4; + plan tests => 9; 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}) { + $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( { @@ -429,11 +443,8 @@ subtest 'action log tests' => sub { } ]; $patron->extended_attributes($attributes); - my $change = { - before => "", - after => 'Foo', - }; - my $info = "Patron attribute " . $attribute_type->code . ": " . to_json($change, { pretty => 1, canonical => 1 }); + + my $info = $get_info->('', 'Foo', $attribute_type->code); my $action_logs = Koha::ActionLogs->search( { module => "MEMBERS", @@ -447,64 +458,104 @@ subtest 'action log tests' => sub { 0, 'No action log entry has been created when adding patron attribute if BorrowersLog syspref disabled' ); - $patron->extended_attributes([]); t::lib::Mocks::mock_preference( 'BorrowersLog', 1 ); - $patron->extended_attributes($attributes); - $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 patron attribute' - ); + my $current_action_logs_count; + my $repeatable_text; + for my $repeatable (0, 1) { + $repeatable_text = $repeatable ? ' repeatable' : ''; - $patron->extended_attributes($attributes); - $action_logs = Koha::ActionLogs->search( - { - module => "MEMBERS", - action => "MODIFY", - object => $patron->borrowernumber, - info => $info - } - ); - is( - $action_logs->count, - 1, - 'No additional action log entry has been created when updating patron attribute with same value' - ); + $patron->extended_attributes([]); - $attributes = [ - { - attribute => 'Bar', - code => $attribute_type->code, - } - ]; - $patron->extended_attributes($attributes); - $change = { - before => 'Foo', - after => 'Bar' - }; - $info = "Patron attribute " . $attribute_type->code . ": " . to_json($change, { pretty => 1, canonical => 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 patron attribute with different value' - ); + $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" + ); + } $schema->storage->txn_rollback; }; -- 2.43.0