From 436bf36cf8bb0bcacbd0afe787e18840a2da4ccb Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Tue, 17 Oct 2023 18:33:16 +0000 Subject: [PATCH] Bug 22632: (follow-up) Tidy and fix tests --- Koha/Patron.pm | 90 ++++++++++++++++---------------- t/db_dependent/Koha/Patron/Log.t | 46 ++++++++++------ 2 files changed, 76 insertions(+), 60 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 7540334f60..b9ea0acb30 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -627,7 +627,7 @@ sub merge_with { my $anonymous_patron = C4::Context->preference("AnonymousPatron"); return if $anonymous_patron && $self->id eq $anonymous_patron; - my @patron_ids = @{ $patron_ids }; + my @patron_ids = @{$patron_ids}; # Ensure the keeper isn't in the list of patrons to merge @patron_ids = grep { $_ ne $self->id } @patron_ids; @@ -636,60 +636,62 @@ sub merge_with { my $results; - $self->_result->result_source->schema->txn_do( sub { - foreach my $patron_id (@patron_ids) { + $self->_result->result_source->schema->txn_do( + sub { + foreach my $patron_id (@patron_ids) { - next if $patron_id eq $anonymous_patron; + next if $patron_id eq $anonymous_patron; - my $patron = Koha::Patrons->find( $patron_id ); + my $patron = Koha::Patrons->find($patron_id); - next unless $patron; + next unless $patron; - # Unbless for safety, the patron will end up being deleted - $results->{merged}->{$patron_id}->{patron} = $patron->unblessed; + # Unbless for safety, the patron will end up being deleted + $results->{merged}->{$patron_id}->{patron} = $patron->unblessed; - my $attributes = $patron->extended_attributes; - my $new_attributes = [ - map { { code => $_->code, attribute => $_->attribute } } - $attributes->as_list - ]; - $attributes->delete; # We need to delete before trying to merge them to prevent exception on unique and repeatable - for my $attribute ( @$new_attributes ) { - try { - $self->add_extended_attribute($attribute); - } catch { - # Don't block the merge if there is a non-repeatable attribute that cannot be added to the current patron. - unless ( $_->isa('Koha::Exceptions::Patron::Attribute::NonRepeatable') ) { - $_->rethrow; + my $attributes = $patron->extended_attributes; + my $new_attributes = [ map { { code => $_->code, attribute => $_->attribute } } $attributes->as_list ]; + $attributes->delete + ; # We need to delete before trying to merge them to prevent exception on unique and repeatable + for my $attribute (@$new_attributes) { + try { + $self->add_extended_attribute($attribute); + } catch { + + # Don't block the merge if there is a non-repeatable attribute that cannot be added to the current patron. + unless ( $_->isa('Koha::Exceptions::Patron::Attribute::NonRepeatable') ) { + $_->rethrow; + } + }; + } + + while ( my ( $r, $field ) = each(%$RESULTSET_PATRON_ID_MAPPING) ) { + my $rs = $schema->resultset($r)->search( { $field => $patron_id } ); + $results->{merged}->{$patron_id}->{updated}->{$r} = $rs->count(); + $rs->update( { $field => $self->id } ); + if ( $r eq 'BorrowerDebarment' ) { + Koha::Patron::Debarments::UpdateBorrowerDebarmentFlags( $self->id ); } - }; - } + } - while (my ($r, $field) = each(%$RESULTSET_PATRON_ID_MAPPING)) { - my $rs = $schema->resultset($r)->search({ $field => $patron_id }); - $results->{merged}->{ $patron_id }->{updated}->{$r} = $rs->count(); - $rs->update({ $field => $self->id }); - if ( $r eq 'BorrowerDebarment' ) { - Koha::Patron::Debarments::UpdateBorrowerDebarmentFlags($self->id); + $patron->move_to_deleted(); + $patron->delete(); + + if ( C4::Context->preference("BorrowersLog") ) { + my $info = + $patron->firstname . " " + . $patron->surname . " (" + . $patron->cardnumber . ")" + . " has been merged into " + . $self->firstname . " " + . $self->surname . " (" + . $self->cardnumber . ")"; + logaction( "MEMBERS", "PATRON_MERGE", $self->id, $info ); } } - $patron->move_to_deleted(); - $patron->delete(); - - if ( C4::Context->preference("BorrowersLog") ) { - my $info = - $patron->firstname . " " - . $patron->surname . " (" - . $patron->cardnumber . ")" - . " has been merged into " - . $self->firstname . " " - . $self->surname . " (" - . $self->cardnumber . ")"; - logaction( "MEMBERS", "PATRON_MERGE", $self->id, $info ); - } } - }); + ); return $results; } diff --git a/t/db_dependent/Koha/Patron/Log.t b/t/db_dependent/Koha/Patron/Log.t index 0aafcd074a..df6d5b2cae 100755 --- a/t/db_dependent/Koha/Patron/Log.t +++ b/t/db_dependent/Koha/Patron/Log.t @@ -18,6 +18,8 @@ use Test::More tests => 1; use C4::Log; +use Koha::ActionLogs; + use t::lib::Mocks; use t::lib::TestBuilder; @@ -26,7 +28,7 @@ $schema->storage->txn_begin; subtest 'Test Koha::Patrons::merge' => sub { - plan tests => 4; + plan tests => 3; my $builder = t::lib::TestBuilder->new; my $keeper = $builder->build_object( { class => 'Koha::Patrons' } ); @@ -41,7 +43,14 @@ subtest 'Test Koha::Patrons::merge' => sub { my $results = $keeper->merge_with( [ $borrower1->{borrowernumber}, $borrower2->{borrowernumber} ] ); - my $log = GetLogs( "", "", "", ["MEMBERS"], ["PATRON_MERGE"], $keeper->id, "" ); + # Action logs records the merges under the 'object' of the kept borrowernumber + my $logs = Koha::ActionLogs->search( + { + module => "MEMBERS", + action => "PATRON_MERGE", + object => $keeper->id + } + ); my $info_borrower1 = $borrower1->{firstname} . " " @@ -51,11 +60,14 @@ subtest 'Test Koha::Patrons::merge' => sub { . $keeper->firstname . " " . $keeper->surname . " (" . $keeper->cardnumber . ")"; - my $info_log = $log->[0]{info}; + my $info_log = $logs->next->info; is( $info_log, $info_borrower1, - "GetLogs returns results in the log viewer for the merge of " . $borrower1->{borrowernumber} + "ActionLogs returns results for the merge of " + . $borrower1->{borrowernumber} + . " into " + . $keeper->borrowernumber ); my $info_borrower2 = @@ -66,11 +78,14 @@ subtest 'Test Koha::Patrons::merge' => sub { . $keeper->firstname . " " . $keeper->surname . " (" . $keeper->cardnumber . ")"; - $info_log = $log->[1]{info}; + $info_log = $logs->next->info; is( $info_log, $info_borrower2, - "GetLogs returns results in the log viewer for the merge of " . $borrower2->{borrowernumber} + "ActionLogs returns results in the log viewer for the merge of " + . $borrower2->{borrowernumber} + . " into " + . $keeper->borrowernumber ); #test with BorrowersLog off @@ -80,20 +95,19 @@ subtest 'Test Koha::Patrons::merge' => sub { $results = $keeper->merge_with( [ $borrower3->{borrowernumber}, $borrower4->{borrowernumber} ] ); - $log = GetLogs( "", "", "", ["MEMBERS"], ["PATRON_MERGE"], $keeper->id, "" ); - $info_log = $log->[0]{info}; - - is( - $info_log, undef, - "GetLogs didn't returns results in the log viewer for the merge of " . $borrower3->{borrowernumber} + $logs = Koha::ActionLogs->search( + { + module => "MEMBERS", + action => "PATRON_MERGE", + object => $keeper->id + } ); - $info_log = $log->[1]{info}; - is( - $info_log, undef, - "GetLogs didn't returns results in the log viewer for the merge of " . $borrower4->{borrowernumber} + $logs->count, 0, + "Koha::ActionLogs didn't returns results in the log viewer for the merge of " . $borrower3->{borrowernumber} ); + }; $schema->storage->txn_rollback; -- 2.30.2