Attachment 157281 Details for Bug 22632 – Bug 22632: (follow-up) Tidy and fix tests

[patch] Bug 22632: (follow-up) Tidy and fix tests

Bug-22632-follow-up-Tidy-and-fix-tests.patch (text/plain), 8.96 KB, created by Nick Clemens (kidclamp) on 2023-10-17 19:24:40 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Nick Clemens (kidclamp)

Created: 2023-10-17 19:24:40 UTC

Size: 8.96 KB

patch

obsolete

>From 436bf36cf8bb0bcacbd0afe787e18840a2da4ccb Mon Sep 17 00:00:00 2001
>From: Nick Clemens <nick@bywatersolutions.com>
>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

Actions: View | Diff | Splinter Review

Attachments on bug 22632: 87778 | 87920 | 87921 | 87922 | 87923 | 87926 | 87995 | 87996 | 87997 | 87998 | 87999 | 88043 | 88044 | 88045 | 88046 | 89728 | 89729 | 89730 | 89731 | 157260 | 157261 | 157262 | 157263 | 157264 | 157265 | 157266 | 157267 | 157268 | 157269 | 157270 | 157271 | 157278 | 157281 | 157286 | 159553 | 159554 | 159555 | 159556 | 159557 | 159558