Bugzilla – Attachment 177605 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.22 KB, created by
David Gustafsson
on 2025-02-06 23:14:38 UTC
(
hide
)
Description:
Bug 26744: Log changes to extended patron attributes
Filename:
MIME Type:
Creator:
David Gustafsson
Created:
2025-02-06 23:14:38 UTC
Size:
17.22 KB
patch
obsolete
>From 339d9b21270b472ed7f2f5b5f7b16e0a88d512b7 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 | 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 <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; >@@ -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
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