From ce10d64df4b392cd7183872d0521c29442993035 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Fri, 22 Jul 2016 16:50:50 +0000 Subject: [PATCH] Bug 16960 - Patron::Modifications should be fixed The changes from opac-memberentry do not reach the table, since the Patron::Modifications object does not work well. Test Plan: 1) Apply this patch 2) Create some patron modification requests 3) Ensure you can approve and deny modifications 4) Ensure patron self registration works --- Koha/Patron/Modification.pm | 36 ++++- Koha/Patron/Modifications.pm | 218 ++------------------------- mainpage.pl | 3 +- members/members-home.pl | 2 +- members/members-update-do.pl | 10 +- members/members-update.pl | 2 +- opac/opac-memberentry.pl | 17 +-- opac/opac-registration-verify.pl | 8 +- t/db_dependent/Koha_borrower_modifications.t | 137 ++++++----------- 9 files changed, 111 insertions(+), 322 deletions(-) diff --git a/Koha/Patron/Modification.pm b/Koha/Patron/Modification.pm index a06aec9..8afbd5e 100644 --- a/Koha/Patron/Modification.pm +++ b/Koha/Patron/Modification.pm @@ -27,14 +27,44 @@ use base qw(Koha::Object); =head1 NAME -Koha::Item - Koha Item object class - -=head1 API +Koha::Patron::Modification - Class represents a requrest to modify or create a patron =head2 Class Methods =cut +=head2 approve + +$m->approve(); + +Commits the pending modifications to the borrower record and removes +them from the modifications table. + +=cut + +sub approve { + my ($self) = @_; + + my $data = $self->unblessed(); + + delete $data->{timestamp}; + delete $data->{verification_token}; + + foreach my $key ( keys %$data ) { + delete $data->{$key} unless ( defined( $data->{$key} ) ); + } + + my $patron = Koha::Patrons->find( $self->borrowernumber ); + + return unless $patron; + + $patron->set($data); + + if ( $patron->store() ) { + return $self->delete(); + } +} + =head3 type =cut diff --git a/Koha/Patron/Modifications.pm b/Koha/Patron/Modifications.pm index 7e17289..088f7a0 100644 --- a/Koha/Patron/Modifications.pm +++ b/Koha/Patron/Modifications.pm @@ -25,80 +25,20 @@ Koha::Patron::Modifications use Modern::Perl; use C4::Context; -use C4::Debug; -use base qw(Koha::Objects); - -=head2 AddModifications - -Koha::Patron::Modifications->AddModifications( $data ); - -Adds or updates modifications for a patron - -Requires either the key borrowernumber, or verification_token -to be part of the passed in hash. - -=cut - -sub AddModifications { - my ( $self, $data ) = @_; - - delete $data->{borrowernumber}; - if( $self->{borrowernumber} ) { - return if( not keys %$data ); - $data->{borrowernumber} = $self->{borrowernumber}; - $data->{verification_token} = ''; - } - elsif( $self->{verification_token} ) { - $data->{verification_token} = $self->{verification_token}; - $data->{borrowernumber} = 0; - } - else { - return; - } +use Koha::Patron::Modification; - my $rs = Koha::Database->new()->schema->resultset('BorrowerModification'); - return $rs->update_or_create($data, { key => 'primary' } ); -} - -=head2 Verify - -$verified = Koha::Patron::Modifications->Verify( $verification_token ); - -Returns true if the passed in token is valid. - -=cut - -sub Verify { - my ( $self, $verification_token ) = @_; - - $verification_token = - ($verification_token) - ? $verification_token - : $self->{'verification_token'}; - - my $dbh = C4::Context->dbh; - my $query = " - SELECT COUNT(*) AS count - FROM borrower_modifications - WHERE verification_token = ? - "; - my $sth = $dbh->prepare($query); - $sth->execute($verification_token); - my $result = $sth->fetchrow_hashref(); - - return $result->{'count'}; -} +use base qw(Koha::Objects); -=head2 GetPendingModificationsCount +=head2 pending_count -$count = Koha::Patron::Modifications->GetPendingModificationsCount(); +$count = Koha::Patron::Modifications->pending_count(); -Returns the number of pending modifications for existing patron. +Returns the number of pending modifications for existing patrons. =cut -sub GetPendingModificationsCount { +sub pending_count { my ( $self, $branchcode ) = @_; my $dbh = C4::Context->dbh; @@ -119,18 +59,18 @@ sub GetPendingModificationsCount { $sth->execute(@params); my $result = $sth->fetchrow_hashref(); - return $result->{'count'}; + return $result->{count}; } -=head2 GetPendingModifications +=head2 pending -$arrayref = Koha::Patron::Modifications->GetPendingModifications(); +$arrayref = Koha::Patron::Modifications->pending(); Returns an arrayref of hashrefs for all pending modifications for existing patrons. =cut -sub GetPendingModifications { +sub pending { my ( $self, $branchcode ) = @_; my $dbh = C4::Context->dbh; @@ -162,144 +102,6 @@ sub GetPendingModifications { return \@m; } -=head2 ApproveModifications - -Koha::Patron::Modifications->ApproveModifications( $borrowernumber ); - -Commits the pending modifications to the borrower record and removes -them from the modifications table. - -=cut - -sub ApproveModifications { - my ( $self, $borrowernumber ) = @_; - - $borrowernumber = - ($borrowernumber) ? $borrowernumber : $self->{'borrowernumber'}; - - return unless $borrowernumber; - - my $data = $self->GetModifications( { borrowernumber => $borrowernumber } ); - delete $data->{timestamp}; - delete $data->{verification_token}; - - my $rs = Koha::Database->new()->schema->resultset('Borrower')->search({ - borrowernumber => $data->{borrowernumber}, - }); - if( $rs->update($data) ) { - $self->DelModifications( { borrowernumber => $borrowernumber } ); - } -} - -=head2 DenyModifications - -Koha::Patron::Modifications->DenyModifications( $borrowernumber ); - -Removes the modifications from the table for the given patron, -without committing the changes to the patron record. - -=cut - -sub DenyModifications { - my ( $self, $borrowernumber ) = @_; - - $borrowernumber = - ($borrowernumber) ? $borrowernumber : $self->{'borrowernumber'}; - - return unless $borrowernumber; - - return $self->DelModifications( { borrowernumber => $borrowernumber } ); -} - -=head2 DelModifications - -Koha::Patron::Modifications->DelModifications({ - [ borrowernumber => $borrowernumber ], - [ verification_token => $verification_token ] -}); - -Deletes the modifications for the given borrowernumber or verification token. - -=cut - -sub DelModifications { - my ( $self, $params ) = @_; - - my ( $field, $value ); - - if ( $params->{'borrowernumber'} ) { - $field = 'borrowernumber'; - $value = $params->{'borrowernumber'}; - } - elsif ( $params->{'verification_token'} ) { - $field = 'verification_token'; - $value = $params->{'verification_token'}; - } - - return unless $value; - - my $dbh = C4::Context->dbh; - - $field = $dbh->quote_identifier($field); - - my $query = " - DELETE - FROM borrower_modifications - WHERE $field = ? - "; - - my $sth = $dbh->prepare($query); - return $sth->execute($value); -} - -=head2 GetModifications - -$hashref = Koha::Patron::Modifications->GetModifications({ - [ borrowernumber => $borrowernumber ], - [ verification_token => $verification_token ] -}); - -Gets the modifications for the given borrowernumber or verification token. - -=cut - -sub GetModifications { - my ( $self, $params ) = @_; - - my ( $field, $value ); - - if ( defined( $params->{'borrowernumber'} ) ) { - $field = 'borrowernumber'; - $value = $params->{'borrowernumber'}; - } - elsif ( defined( $params->{'verification_token'} ) ) { - $field = 'verification_token'; - $value = $params->{'verification_token'}; - } - - return unless $value; - - my $dbh = C4::Context->dbh; - - $field = $dbh->quote_identifier($field); - - my $query = " - SELECT * - FROM borrower_modifications - WHERE $field = ? - "; - - my $sth = $dbh->prepare($query); - $sth->execute($value); - my $data = $sth->fetchrow_hashref(); - - foreach my $key ( keys %$data ) { - delete $data->{$key} unless ( defined( $data->{$key} ) ); - } - - return $data; -} - sub _type { return 'BorrowerModification'; } diff --git a/mainpage.pl b/mainpage.pl index d9e01b7..f4efc65 100755 --- a/mainpage.pl +++ b/mainpage.pl @@ -65,8 +65,7 @@ my $branch = my $pendingcomments = numberofreviews(0); my $pendingtags = get_count_by_tag_status(0); my $pendingsuggestions = CountSuggestion("ASKED"); -my $pending_borrower_modifications = - Koha::Patron::Modifications->GetPendingModificationsCount( $branch ); +my $pending_borrower_modifications = Koha::Patron::Modifications->pending_count( $branch ); my $pending_discharge_requests = Koha::Patron::Discharge::count({ pending => 1 }); $template->param( diff --git a/members/members-home.pl b/members/members-home.pl index 247cc00..5b7c614 100755 --- a/members/members-home.pl +++ b/members/members-home.pl @@ -89,7 +89,7 @@ else { my $pending_borrower_modifications = - Koha::Patron::Modifications->GetPendingModificationsCount( $branch ); + Koha::Patron::Modifications->pending_count( $branch ); $template->param( no_add => $no_add, diff --git a/members/members-update-do.pl b/members/members-update-do.pl index f55b7ab..db4686a 100755 --- a/members/members-update-do.pl +++ b/members/members-update-do.pl @@ -50,14 +50,14 @@ foreach my $param (@params) { my $action = $query->param($param); if ( $action eq 'approve' ) { - Koha::Patron::Modifications->ApproveModifications( $borrowernumber ); + my $m = Koha::Patron::Modifications->find( { borrowernumber => $borrowernumber } ); + $m->approve() if $m; } elsif ( $action eq 'deny' ) { - Koha::Patron::Modifications->DenyModifications( $borrowernumber ); - } - elsif ( $action eq 'ignore' ) { - + my $m = Koha::Patron::Modifications->find( { borrowernumber => $borrowernumber } ); + $m->delete() if $m; } + # elsif ( $action eq 'ignore' ) { } } } diff --git a/members/members-update.pl b/members/members-update.pl index 1441346..c97bcc5 100755 --- a/members/members-update.pl +++ b/members/members-update.pl @@ -49,7 +49,7 @@ my $branch = : undef; my $pending_modifications = - Koha::Patron::Modifications->GetPendingModifications($branch); + Koha::Patron::Modifications->pending($branch); my $borrowers; foreach my $pm (@$pending_modifications) { diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl index b73ee1b..8de231a 100755 --- a/opac/opac-memberentry.pl +++ b/opac/opac-memberentry.pl @@ -27,6 +27,7 @@ use C4::Output; use C4::Members; use C4::Form::MessagingPreferences; use Koha::Patrons; +use Koha::Patron::Modification; use Koha::Patron::Modifications; use C4::Branch qw(GetBranchesLoop); use C4::Scrubber; @@ -126,11 +127,11 @@ if ( $action eq 'create' ) { $template->param( 'email' => $borrower{'email'} ); my $verification_token = md5_hex( \%borrower ); - $borrower{'password'} = random_string(".........."); - Koha::Patron::Modifications->new( - verification_token => $verification_token ) - ->AddModifications(\%borrower); + $borrower{password} = random_string(".........."); + $borrower{verification_token} = $verification_token; + + Koha::Patron::Modification->new( \%borrower )->store(); #Send verification email my $letter = C4::Letters::GetPreparedLetter( @@ -213,12 +214,10 @@ elsif ( $action eq 'update' ) { } ); - my $m = - Koha::Patron::Modifications->new( - borrowernumber => $borrowernumber ); + $borrower_changes{borrowernumber} = $borrowernumber; + + my $m = Koha::Patron::Modification->new( \%borrower_changes )->store(); - $m->DelModifications; - $m->AddModifications(\%borrower_changes); $template->param( borrower => GetMember( borrowernumber => $borrowernumber ), ); diff --git a/opac/opac-registration-verify.pl b/opac/opac-registration-verify.pl index 5ceff26..1098af0 100755 --- a/opac/opac-registration-verify.pl +++ b/opac/opac-registration-verify.pl @@ -34,10 +34,10 @@ unless ( C4::Context->preference('PatronSelfRegistration') ) { } my $token = $cgi->param('token'); -my $m = Koha::Patron::Modifications->new( verification_token => $token ); +my $m = Koha::Patron::Modifications->find( { verification_token => $token } ); my ( $template, $borrowernumber, $cookie ); -if ( $m->Verify() ) { +if ( $m ) { ( $template, $borrowernumber, $cookie ) = get_template_and_user( { template_name => "opac-registration-confirmation.tt", @@ -50,13 +50,13 @@ if ( $m->Verify() ) { $template->param( OpacPasswordChange => C4::Context->preference('OpacPasswordChange') ); - my $borrower = Koha::Patron::Modifications->GetModifications({ verification_token => $token }); + my $borrower = $m->unblessed(); my $password; ( $borrowernumber, $password ) = AddMember_Opac(%$borrower); if ($borrowernumber) { - Koha::Patron::Modifications->DelModifications({ verification_token => $token }); + $m->delete(); C4::Form::MessagingPreferences::handle_form_action($cgi, { borrowernumber => $borrowernumber }, $template, 1, C4::Context->preference('PatronSelfRegistrationDefaultCategory') ) if C4::Context->preference('EnhancedMessagingPreferences'); $template->param( password_cleartext => $password ); diff --git a/t/db_dependent/Koha_borrower_modifications.t b/t/db_dependent/Koha_borrower_modifications.t index f98dfeb..e09ae4b 100755 --- a/t/db_dependent/Koha_borrower_modifications.t +++ b/t/db_dependent/Koha_borrower_modifications.t @@ -1,133 +1,92 @@ #!/usr/bin/perl use Modern::Perl; -use Test::More tests => 14; +use Test::More tests => 8; use C4::Context; use t::lib::TestBuilder; use C4::Members; -use Koha::Patron::Modifications; +BEGIN { + use_ok('Koha::Patron::Modification'); + use_ok('Koha::Patron::Modifications'); +} -my $dbh = C4::Context->dbh; -$dbh->{RaiseError} = 1; -$dbh->{AutoCommit} = 0; +my $schema = Koha::Database->new->schema; +$schema->storage->txn_begin; +my $dbh = C4::Context->dbh; $dbh->do("DELETE FROM borrower_modifications"); ## Create new pending modification -Koha::Patron::Modifications->new( verification_token => '1234567890' ) - ->AddModifications( { surname => 'Hall', firstname => 'Kyle' } ); +Koha::Patron::Modification->new( + { + verification_token => '1234567890', + surname => 'Hall', + firstname => 'Kyle' + } +)->store(); ## Get the new pending modification -my $borrower = Koha::Patron::Modifications->GetModifications( - { verification_token => '1234567890' } ); +my $borrower = + Koha::Patron::Modifications->find( { verification_token => '1234567890' } ); ## Verify we get the same data -ok( $borrower->{'surname'} = 'Hall', - 'Test AddModifications() and GetModifications()' ); - -## Check the Verify method -ok( - Koha::Patron::Modifications->Verify('1234567890'), - 'Test that Verify() succeeds with a valid token' -); - -## Delete the pending modification -$borrower = Koha::Patron::Modifications->DelModifications( - { verification_token => '1234567890' } ); - -## Verify it's no longer in the database -$borrower = Koha::Patron::Modifications->GetModifications( - { verification_token => '1234567890' } ); -ok( !defined( $borrower->{'surname'} ), 'Test DelModifications()' ); - -## Check the Verify method -ok( - !Koha::Patron::Modifications->Verify('1234567890'), - 'Test that Verify() method fails for a bad token' -); +is( $borrower->surname, 'Hall', 'Found modification has matching surname' ); ## Create new pending modification for a patron my $builder = t::lib::TestBuilder->new; -my $borr1 = $builder->build({ source => 'Borrower' })->{borrowernumber}; -Koha::Patron::Modifications->new( borrowernumber => $borr1 ) - ->AddModifications( { surname => 'Hall', firstname => 'Kyle' } ); +my $borr1 = $builder->build( { source => 'Borrower' } )->{borrowernumber}; + +my $m1 = Koha::Patron::Modification->new( + { + borrowernumber => $borr1, + surname => 'Hall', + firstname => 'Kyle' + } +)->store(); ## Test the counter -ok( Koha::Patron::Modifications->GetPendingModificationsCount() == 1, - 'Test GetPendingModificationsCount()' ); +is( Koha::Patron::Modifications->pending_count, + 1, 'Test pending_count()' ); ## Create new pending modification for another patron -my $borr2 = $builder->build({ source => 'Borrower' })->{borrowernumber}; -Koha::Patron::Modifications->new( borrowernumber => $borr2 ) - ->AddModifications( { surname => 'Smith', firstname => 'Sandy' } ); +my $borr2 = $builder->build( { source => 'Borrower' } )->{borrowernumber}; +my $m2 = Koha::Patron::Modification->new( + { + borrowernumber => $borr2, + surname => 'Smith', + firstname => 'Sandy' + } +)->store(); ## Test the counter -ok( - Koha::Patron::Modifications->GetPendingModificationsCount() == 2, -'Add a new pending modification and test GetPendingModificationsCount() again' +is( + Koha::Patron::Modifications->pending_count(), 2, +'Add a new pending modification and test pending_count() again' ); ## Check GetPendingModifications -my $pendings = Koha::Patron::Modifications->GetPendingModifications(); -my @firstnames_mod = sort ( $pendings->[0]->{firstname}, $pendings->[1]->{firstname} ); -ok( $firstnames_mod[0] eq 'Kyle', 'Test GetPendingModifications()' ); -ok( $firstnames_mod[1] eq 'Sandy', 'Test GetPendingModifications() again' ); +my $pendings = Koha::Patron::Modifications->pending; +my @firstnames_mod = + sort ( $pendings->[0]->{firstname}, $pendings->[1]->{firstname} ); +ok( $firstnames_mod[0] eq 'Kyle', 'Test pending()' ); +ok( $firstnames_mod[1] eq 'Sandy', 'Test pending() again' ); ## This should delete the row from the table -Koha::Patron::Modifications->DenyModifications( $borr2 ); - -## Test the counter -ok( Koha::Patron::Modifications->GetPendingModificationsCount() == 1, - 'Test DenyModifications()' ); +$m2->delete(); ## Save a copy of the borrowers original data my $old_borrower = GetMember( borrowernumber => $borr1 ); ## Apply the modifications -Koha::Patron::Modifications->ApproveModifications( $borr1 ); - -## Test the counter -ok( - Koha::Patron::Modifications->GetPendingModificationsCount() == 0, - 'Test ApproveModifications() removes pending modification from db' -); +$m1->approve(); ## Get a copy of the borrowers current data my $new_borrower = GetMember( borrowernumber => $borr1 ); ## Check to see that the approved modifications were saved ok( $new_borrower->{'surname'} eq 'Hall', - 'Test ApproveModifications() applys modification to borrower' ); - -## Now let's put it back the way it was -Koha::Patron::Modifications->new( borrowernumber => $borr1 )->AddModifications( - { - surname => $old_borrower->{'surname'}, - firstname => $old_borrower->{'firstname'} - } -); - -## Test the counter -ok( Koha::Patron::Modifications->GetPendingModificationsCount() == 1, - 'Test GetPendingModificationsCount()' ); - -## Apply the modifications -Koha::Patron::Modifications->ApproveModifications( $borr1 ); - -## Test the counter -ok( - Koha::Patron::Modifications->GetPendingModificationsCount() == 0, - 'Test ApproveModifications() removes pending modification from db, again' -); - -$new_borrower = GetMember( borrowernumber => $borr1 ); - -## Test to verify the borrower has been updated with the original values -ok( - $new_borrower->{'surname'} eq $old_borrower->{'surname'}, - 'Test ApproveModifications() applys modification to borrower, again' -); + 'Test approve() applys modification to borrower' ); $dbh->rollback(); -- 2.1.4