Bug 9302: (QA follow-up) Consistency follow-up
authorTomas Cohen Arazi <tomascohen@theke.io>
Wed, 4 Apr 2018 19:56:34 +0000 (16:56 -0300)
committerJonathan Druart <jonathan.druart@bugs.koha-community.org>
Fri, 20 Apr 2018 16:34:41 +0000 (13:34 -0300)
This patch moves the Koha::Patrons->merge method into
Koha::Patron->merge_with in the line of the discussed implementation for
bug 15336. I agree with that implementation so I provide this follow-up.

Tests are adjusted, the controller script is adapted too. The behaviour
remains.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Koha/Patron.pm
Koha/Patrons.pm
koha-tmpl/intranet-tmpl/prog/en/modules/members/merge-patrons.tt
members/merge-patrons.pl
t/db_dependent/Koha/Patrons.t
t/db_dependent/Patrons.t

index c8e319e..ddf3f37 100644 (file)
@@ -42,6 +42,33 @@ use Koha::Account;
 
 use base qw(Koha::Object);
 
+our $RESULTSET_PATRON_ID_MAPPING = {
+    Accountline          => 'borrowernumber',
+    ArticleRequest       => 'borrowernumber',
+    BorrowerAttribute    => 'borrowernumber',
+    BorrowerDebarment    => 'borrowernumber',
+    BorrowerFile         => 'borrowernumber',
+    BorrowerModification => 'borrowernumber',
+    ClubEnrollment       => 'borrowernumber',
+    Issue                => 'borrowernumber',
+    ItemsLastBorrower    => 'borrowernumber',
+    Linktracker          => 'borrowernumber',
+    Message              => 'borrowernumber',
+    MessageQueue         => 'borrowernumber',
+    OldIssue             => 'borrowernumber',
+    OldReserve           => 'borrowernumber',
+    Rating               => 'borrowernumber',
+    Reserve              => 'borrowernumber',
+    Review               => 'borrowernumber',
+    Statistic            => 'borrowernumber',
+    SearchHistory        => 'userid',
+    Suggestion           => 'suggestedby',
+    TagAll               => 'borrowernumber',
+    Virtualshelfcontent  => 'borrowernumber',
+    Virtualshelfshare    => 'borrowernumber',
+    Virtualshelve        => 'owner',
+};
+
 =head1 NAME
 
 Koha::Patron - Koha Patron Object class
@@ -201,6 +228,54 @@ sub siblings {
     );
 }
 
+=head3 merge_with
+
+    my $patron = Koha::Patrons->find($id);
+    $patron->merge_with( \@patron_ids );
+
+    This subroutine merges a list of patrons into the patron record. This is accomplished by finding
+    all related patron ids for the patrons to be merged in other tables and changing the ids to be that
+    of the keeper patron.
+
+=cut
+
+sub merge_with {
+    my ( $self, $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;
+
+    my $schema = Koha::Database->new()->schema();
+
+    my $results;
+
+    $self->_result->result_source->schema->txn_do( sub {
+        foreach my $patron_id (@patron_ids) {
+            my $patron = Koha::Patrons->find( $patron_id );
+
+            next unless $patron;
+
+            # Unbless for safety, the patron will end up being deleted
+            $results->{merged}->{$patron_id}->{patron} = $patron->unblessed;
+
+            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 });
+            }
+
+            $patron->move_to_deleted();
+            $patron->delete();
+        }
+    });
+
+    return $results;
+}
+
+
+
 =head3 wants_check_for_previous_checkout
 
     $wants_check = $patron->wants_check_for_previous_checkout;
index 0ef5f4f..0d2d9ec 100644 (file)
@@ -31,33 +31,6 @@ use Koha::Patron;
 
 use base qw(Koha::Objects);
 
-our $RESULTSET_PATRON_ID_MAPPING = {
-    Accountline          => 'borrowernumber',
-    ArticleRequest       => 'borrowernumber',
-    BorrowerAttribute    => 'borrowernumber',
-    BorrowerDebarment    => 'borrowernumber',
-    BorrowerFile         => 'borrowernumber',
-    BorrowerModification => 'borrowernumber',
-    ClubEnrollment       => 'borrowernumber',
-    Issue                => 'borrowernumber',
-    ItemsLastBorrower    => 'borrowernumber',
-    Linktracker          => 'borrowernumber',
-    Message              => 'borrowernumber',
-    MessageQueue         => 'borrowernumber',
-    OldIssue             => 'borrowernumber',
-    OldReserve           => 'borrowernumber',
-    Rating               => 'borrowernumber',
-    Reserve              => 'borrowernumber',
-    Review               => 'borrowernumber',
-    Statistic            => 'borrowernumber',
-    SearchHistory        => 'userid',
-    Suggestion           => 'suggestedby',
-    TagAll               => 'borrowernumber',
-    Virtualshelfcontent  => 'borrowernumber',
-    Virtualshelfshare    => 'borrowernumber',
-    Virtualshelve        => 'owner',
-};
-
 =head1 NAME
 
 Koha::Patron - Koha Patron Object class
@@ -234,58 +207,7 @@ sub anonymise_issue_history {
     return $nb_rows;
 }
 
-=head3 merge
-
-    Koha::Patrons->search->merge( { keeper => $borrowernumber, patrons => \@borrowernumbers } );
-
-    This subroutine merges a list of patrons into another patron record. This is accomplished by finding
-    all related patron ids for the patrons to be merged in other tables and changing the ids to be that
-    of the keeper patron.
-
-=cut
-
-sub merge {
-    my ( $self, $params ) = @_;
-
-    my $keeper          = $params->{keeper};
-    my @borrowernumbers = @{ $params->{patrons} };
-
-    my $patron_to_keep = Koha::Patrons->find( $keeper );
-    return unless $patron_to_keep;
-
-    # Ensure the keeper isn't in the list of patrons to merge
-    @borrowernumbers = grep { $_ ne $keeper } @borrowernumbers;
-
-    my $schema = Koha::Database->new()->schema();
-
-    my $results;
-
-    $self->_resultset->result_source->schema->txn_do( sub {
-        foreach my $borrowernumber (@borrowernumbers) {
-            my $patron = Koha::Patrons->find( $borrowernumber );
-
-            next unless $patron;
-
-            # Unbless for safety, the patron will end up being deleted
-            $results->{merged}->{$borrowernumber}->{patron} = $patron->unblessed;
-
-            while (my ($r, $field) = each(%$RESULTSET_PATRON_ID_MAPPING)) {
-                my $rs = $schema->resultset($r)->search({ $field => $borrowernumber} );
-                $results->{merged}->{ $borrowernumber }->{updated}->{$r} = $rs->count();
-                $rs->update( { $field => $keeper });
-            }
-
-            $patron->move_to_deleted();
-            $patron->delete();
-        }
-    });
-
-    $results->{keeper} = $patron_to_keep;
-
-    return $results;
-}
-
-=head3 type
+=head3 _type
 
 =cut
 
index 6ae668f..354ffbf 100644 (file)
@@ -108,7 +108,7 @@ $(document).ready(function() {
                     <div class="dialog alert">Merge failed! The following error was reported: [% error %].</div>
                 [% ELSE %]
                     <p>
-                        Patron records merged into <a href="moremember.pl?borrowernumber=[% results.keeper.id %]">[% results.keeper.firstname %] [% results.keeper.surname %] ([% results.keeper.cardnumber | html %])</a>
+                        Patron records merged into <a href="moremember.pl?borrowernumber=[% keeper.id %]">[% keeper.firstname %] [% keeper.surname %] ([% keeper.cardnumber | html %])</a>
                     </p>
 
                     [% FOREACH pair IN results.merged.pairs %]
index a35fe83..2008fd4 100755 (executable)
@@ -19,6 +19,7 @@
 use Modern::Perl;
 
 use CGI qw ( -utf8 );
+use Try::Tiny;
 
 use C4::Auth;
 use C4::Output;
@@ -33,26 +34,30 @@ my ( $template, $loggedinuser, $cookie, $flags ) = get_template_and_user(
         query           => $cgi,
         type            => "intranet",
         authnotrequired => 0,
-        flagsrequired   => { borrowers => 1 },
-        debug           => 1,
+        flagsrequired   => { borrowers => 1 }
     }
 );
 
 my $action = $cgi->param('action') || 'show';
-my @ids = $cgi->multi_param('id');
+my @ids    = $cgi->multi_param('id');
 
 if ( $action eq 'show' ) {
-    my $patrons =
-      Koha::Patrons->search( { borrowernumber => { -in => \@ids } } );
+    my $patrons = Koha::Patrons->search({ borrowernumber => { -in => \@ids } });
     $template->param( patrons => $patrons );
 } elsif ( $action eq 'merge' ) {
-    my $keeper = $cgi->param('keeper');
+    my $keeper_id = $cgi->param('keeper');
     my $results;
-    eval { $results = Koha::Patrons->merge( { keeper => $keeper, patrons => \@ids } ); };
-    if ($@) {
-        $template->param( results => $results );
-    } else {
-        $template->param( error => $@ );
+
+    try {
+        my $keeper = Koha::Patrons->find( $keeper_id );
+        $results = $keeper->merge_with( \@ids );
+        $template->param(
+            keeper  => $keeper,
+            results => $results
+        );
+    }
+    catch {
+        $template->param( error => $_ );
     }
 }
 
index 0b6e122..8eb80e6 100644 (file)
@@ -19,7 +19,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 29;
+use Test::More tests => 30;
 use Test::Warn;
 use Time::Fake;
 use DateTime;
@@ -1334,9 +1334,53 @@ subtest 'Log cardnumber change' => sub {
     is( scalar @logs, 2, 'With BorrowerLogs, Change in cardnumber should be logged, as well as general alert of patron mod.' );
 };
 
-
 $schema->storage->txn_rollback;
 
+subtest 'Test Koha::Patrons::merge' => sub {
+    plan tests => 98;
+
+    my $schema = Koha::Database->new()->schema();
+
+    my $resultsets = $Koha::Patron::RESULTSET_PATRON_ID_MAPPING;
+
+    $schema->storage->txn_begin;
+
+    my $keeper  = $builder->build_object({ class => 'Koha::Patrons' });
+    my $loser_1 = $builder->build({ source => 'Borrower' })->{borrowernumber};
+    my $loser_2 = $builder->build({ source => 'Borrower' })->{borrowernumber};
+
+    while (my ($r, $field) = each(%$resultsets)) {
+        $builder->build({ source => $r, value => { $field => $keeper->id } });
+        $builder->build({ source => $r, value => { $field => $loser_1 } });
+        $builder->build({ source => $r, value => { $field => $loser_2 } });
+
+        my $keeper_rs =
+          $schema->resultset($r)->search( { $field => $keeper->id } );
+        is( $keeper_rs->count(), 1, "Found 1 $r rows for keeper" );
+
+        my $loser_1_rs =
+          $schema->resultset($r)->search( { $field => $loser_1 } );
+        is( $loser_1_rs->count(), 1, "Found 1 $r rows for loser_1" );
+
+        my $loser_2_rs =
+          $schema->resultset($r)->search( { $field => $loser_2 } );
+        is( $loser_2_rs->count(), 1, "Found 1 $r rows for loser_2" );
+    }
+
+    my $results = $keeper->merge_with([ $loser_1, $loser_2 ]);
+
+    while (my ($r, $field) = each(%$resultsets)) {
+        my $keeper_rs =
+          $schema->resultset($r)->search( {$field => $keeper->id } );
+        is( $keeper_rs->count(), 3, "Found 2 $r rows for keeper" );
+    }
+
+    is( Koha::Patrons->find($loser_1), undef, 'Loser 1 has been deleted' );
+    is( Koha::Patrons->find($loser_2), undef, 'Loser 2 has been deleted' );
+
+    $schema->storage->txn_rollback;
+};
+
 # TODO Move to t::lib::Mocks and reuse it!
 sub set_logged_in_user {
     my ($patron) = @_;
index 53ee4b4..f0d116a 100755 (executable)
@@ -17,7 +17,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 18;
+use Test::More tests => 17;
 use Test::Warn;
 
 use C4::Context;
@@ -104,51 +104,5 @@ foreach my $b ( $patrons->as_list() ) {
     is( $b->categorycode(), $categorycode, "Iteration returns a patron object" );
 }
 
-subtest 'Test Koha::Patrons::merge' => sub {
-    plan tests => 98;
-
-    my $schema = Koha::Database->new()->schema();
-
-    my $resultsets = $Koha::Patrons::RESULTSET_PATRON_ID_MAPPING;
-
-    my $keeper  = $builder->build( { source => 'Borrower' } )->{borrowernumber};
-    my $loser_1 = $builder->build( { source => 'Borrower' } )->{borrowernumber};
-    my $loser_2 = $builder->build( { source => 'Borrower' } )->{borrowernumber};
-
-    while (my ($r, $field) = each(%$resultsets)) {
-        $builder->build( { source => $r, value => { $field => $keeper } } );
-        $builder->build( { source => $r, value => { $field => $loser_1 } } );
-        $builder->build( { source => $r, value => { $field => $loser_2 } } );
-
-        my $keeper_rs =
-          $schema->resultset($r)->search( { $field => $keeper } );
-        is( $keeper_rs->count(), 1, "Found 1 $r rows for keeper" );
-
-        my $loser_1_rs =
-          $schema->resultset($r)->search( { $field => $loser_1 } );
-        is( $loser_1_rs->count(), 1, "Found 1 $r rows for loser_1" );
-
-        my $loser_2_rs =
-          $schema->resultset($r)->search( { $field => $loser_2 } );
-        is( $loser_2_rs->count(), 1, "Found 1 $r rows for loser_2" );
-    }
-
-    my $results = Koha::Patrons->merge(
-        {
-            keeper  => $keeper,
-            patrons => [ $loser_1, $loser_2 ],
-        }
-    );
-
-    while (my ($r, $field) = each(%$resultsets)) {
-        my $keeper_rs =
-          $schema->resultset($r)->search( {$field => $keeper } );
-        is( $keeper_rs->count(), 3, "Found 2 $r rows for keeper" );
-    }
-
-    is( Koha::Patrons->find($loser_1), undef, 'Loser 1 has been deleted' );
-    is( Koha::Patrons->find($loser_2), undef, 'Loser 2 has been deleted' );
-};
-
 $schema->storage->txn_rollback();