Bugzilla – Attachment 178771 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), 17.07 KB, created by
Lucas Gass (lukeg)
on 2025-02-26 18:38:14 UTC
(
hide
)
Description:
Bug 26744: Log changes to extended patron attributes
Filename:
MIME Type:
Creator:
Lucas Gass (lukeg)
Created:
2025-02-26 18:38:14 UTC
Size:
17.07 KB
patch
obsolete
>From a46d10b81e41171a57fa16c5f0b43cb8c3f6c07e 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 | 87 +++++++++- > Koha/Patron/Attribute.pm | 22 ++- > members/memberentry.pl | 44 +---- > t/db_dependent/Koha/Patron/Attribute.t | 226 ++++++++++++++++++++++++- > 4 files changed, 328 insertions(+), 51 deletions(-) > >diff --git a/Koha/Patron.pm b/Koha/Patron.pm >index ec7f6704983..ee93d9b3557 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,91 @@ 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; >+ 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; > >- # Insert the new ones >+ # 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..fa8df66b560 100755 >--- a/members/memberentry.pl >+++ b/members/memberentry.pl >@@ -297,8 +297,7 @@ foreach my $guarantor (@guarantors) { > if ( ( $op eq 'cud-save' || $op eq 'cud-insert' ) > && ( $guarantor->is_child || $guarantor->is_guarantee || ( $patron && $patron->is_guarantor ) ) ) > { >- push @errors, 'ERROR_child_is_guarantor' if ( $guarantor->is_child ); >- push @errors, 'ERROR_guarantor_is_guarantee' if ( !$guarantor->is_child ); >+ push @errors, 'ERROR_guarantor_is_guarantee'; > } > } > >@@ -626,46 +625,7 @@ if ( ( !$nok ) and $nodouble and ( $op eq 'cud-insert' or $op eq 'cud-save' ) ) > 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); >- } >+ $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..3817f0877f1 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,224 @@ 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.39.5
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