Bugzilla – Attachment 178728 Details for
Bug 26744
Log changes to extended patron attributes
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 26744: Log changes to extended patron attributes
Bug-26744-Log-changes-to-extended-patron-attribute.patch (text/plain), 16.52 KB, created by
David Gustafsson
on 2025-02-26 15:27:54 UTC
(
hide
)
Description:
Bug 26744: Log changes to extended patron attributes
Filename:
MIME Type:
Creator:
David Gustafsson
Created:
2025-02-26 15:27:54 UTC
Size:
16.52 KB
patch
obsolete
>From 90027b0422d096f7c7e4340a4f51028e1b04b7b2 Mon Sep 17 00:00:00 2001 >From: Stefan Berndtsson <stefan.berndtsson@gmail.com> >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 <lucas@bywatersolutions.com> >Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu> >--- > 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<Koha::Patron::Attribute> 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 <http://www.gnu.org/licenses>. > > 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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 26744
:
163957
|
163958
|
163959
|
164034
|
164035
|
164098
|
165212
|
165213
|
165301
|
165302
|
165303
|
165304
|
165305
|
177605
|
177652
|
178728
|
178771
|
178787
|
178788
|
178789
|
178801
|
178802
|
178803
|
178817
|
178839
|
178840
|
178841
|
178842
|
179284
|
179285
|
179286