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> Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
This commit is contained in:
Stefan Berndtsson
Katrin Fischer
parent
046c93d682
commit
af6f6331c4
|
|
@ -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} //= [];
|
||||
|
||||
# Insert the new ones
|
||||
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;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
|
|
|||
|
|
@ -299,8 +299,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';
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -628,46 +627,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 (
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
};
|
||||
|
|
|
|||
Loading…
Reference in New Issue