From 05394e14ce4641fb7b48cb912766efb50be48f16 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Wed, 4 Apr 2018 16:56:34 -0300 Subject: [PATCH] Bug 9302: (QA follow-up) Consistency follow-up 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 --- Koha/Patron.pm | 75 +++++++++++++++++ Koha/Patrons.pm | 80 +------------------ .../prog/en/modules/members/merge-patrons.tt | 2 +- members/merge-patrons.pl | 27 ++++--- t/db_dependent/Koha/Patrons.t | 48 ++++++++++- t/db_dependent/Patrons.t | 48 +---------- 6 files changed, 140 insertions(+), 140 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 2322131713..e81eaeee9b 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -41,6 +41,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 @@ -200,6 +227,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; diff --git a/Koha/Patrons.pm b/Koha/Patrons.pm index 0ef5f4f0b9..0d2d9ec6b4 100644 --- a/Koha/Patrons.pm +++ b/Koha/Patrons.pm @@ -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 diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/merge-patrons.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/merge-patrons.tt index 6ae668f517..354ffbf555 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/merge-patrons.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/merge-patrons.tt @@ -108,7 +108,7 @@ $(document).ready(function() {
Merge failed! The following error was reported: [% error %].
[% ELSE %]

- Patron records merged into [% results.keeper.firstname %] [% results.keeper.surname %] ([% results.keeper.cardnumber | html %]) + Patron records merged into [% keeper.firstname %] [% keeper.surname %] ([% keeper.cardnumber | html %])

[% FOREACH pair IN results.merged.pairs %] diff --git a/members/merge-patrons.pl b/members/merge-patrons.pl index a35fe83f29..2008fd4917 100755 --- a/members/merge-patrons.pl +++ b/members/merge-patrons.pl @@ -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 => $_ ); } } diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index f0b1992054..8a020a6ad6 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 27; +use Test::More tests => 28; use Test::Warn; use Time::Fake; use DateTime; @@ -1217,9 +1217,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) = @_; diff --git a/t/db_dependent/Patrons.t b/t/db_dependent/Patrons.t index 53ee4b4e01..f0d116a7bc 100755 --- a/t/db_dependent/Patrons.t +++ b/t/db_dependent/Patrons.t @@ -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(); -- 2.17.0