From 74837e922809d083de7d54f987b18db45e29bc48 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Mon, 10 Dec 2012 15:45:28 -0500 Subject: [PATCH] Bug 7067 QA Followup Adjusts calling conventions to use hashrefs and eliminate redundant procedural/OO mixed code. --- Koha/Borrower/Modifications.pm | 121 ++++++++------------------ opac/opac-memberentry.pl | 4 +- opac/opac-registration-verify.pl | 2 +- t/db_dependent/Koha_borrower_modifications.t | 94 ++++++++++++++++++++ 4 files changed, 134 insertions(+), 87 deletions(-) create mode 100755 t/db_dependent/Koha_borrower_modifications.t diff --git a/Koha/Borrower/Modifications.pm b/Koha/Borrower/Modifications.pm index 443febb..1d32d16 100644 --- a/Koha/Borrower/Modifications.pm +++ b/Koha/Borrower/Modifications.pm @@ -28,34 +28,15 @@ use C4::Context; use C4::Debug; use C4::SQLHelper qw(InsertInTable UpdateInTable); -use vars qw($VERSION @ISA @EXPORT @EXPORT_OK %EXPORT_TAGS); - -BEGIN { - - # set the version for version checking - $VERSION = 0.01; - require Exporter; - @ISA = qw(Exporter); - @EXPORT = qw( - - ); - - my $debug = C4::Context->preference("DebugLevel"); -} - sub new { my ( $class, %args ) = @_; - my $self = bless( {}, $class ); - - $self->{'verification_token'} = $args{'verification_token'}; - $self->{'borrowernumber'} = $args{'borrowernumber'}; - return $self; + return bless( \%args, $class ); } =head2 AddModifications -Koha::Borrower::Modifications->AddModifications( %data ); +Koha::Borrower::Modifications->AddModifications( $data ); Adds or updates modifications for a borrower. @@ -65,13 +46,13 @@ to be part of the passed in hash. =cut sub AddModifications { - my ( $self, %data ) = @_; + my ( $self, $data ) = @_; if ( $self->{'borrowernumber'} ) { - delete $data{'borrowernumber'}; + delete $data->{'borrowernumber'}; - if ( keys %data ) { - $data{'borrowernumber'} = $self->{'borrowernumber'}; + if ( keys %$data ) { + $data->{'borrowernumber'} = $self->{'borrowernumber'}; my $dbh = C4::Context->dbh; my $query = " @@ -85,20 +66,20 @@ sub AddModifications { my $result = $sth->fetchrow_hashref(); if ( $result->{'count'} ) { - $data{'verification_token'} = q{}; - return UpdateInTable( "borrower_modifications", \%data ); + $data->{'verification_token'} = q{}; + return UpdateInTable( "borrower_modifications", $data ); } else { - return InsertInTable( "borrower_modifications", \%data ); + return InsertInTable( "borrower_modifications", $data ); } } } elsif ( $self->{'verification_token'} ) { - delete $data{'borrowernumber'}; - $data{'verification_token'} = $self->{'verification_token'}; + delete $data->{'borrowernumber'}; + $data->{'verification_token'} = $self->{'verification_token'}; - return InsertInTable( "borrower_modifications", \%data ); + return InsertInTable( "borrower_modifications", $data ); } else { return; @@ -116,12 +97,10 @@ Returns true if the passed in token is valid. sub Verify { my ( $self, $verification_token ) = @_; - if ( ref($self) ) { - $verification_token = - ($verification_token) - ? $verification_token - : $self->{'verification_token'}; - } + $verification_token = + ($verification_token) + ? $verification_token + : $self->{'verification_token'}; my $dbh = C4::Context->dbh; my $query = " @@ -219,17 +198,15 @@ them from the modifications table. sub ApproveModifications { my ( $self, $borrowernumber ) = @_; - if ( ref($self) ) { - $borrowernumber = - ($borrowernumber) ? $borrowernumber : $self->{'borrowernumber'}; - } + $borrowernumber = + ($borrowernumber) ? $borrowernumber : $self->{'borrowernumber'}; return unless $borrowernumber; - my $data = $self->GetModifications( borrowernumber => $borrowernumber ); + my $data = $self->GetModifications({ borrowernumber => $borrowernumber }); if ( UpdateInTable( "borrowers", $data ) ) { - $self->DelModifications( borrowernumber => $borrowernumber ); + $self->DelModifications({ borrowernumber => $borrowernumber }); } } @@ -245,50 +222,37 @@ without commiting the changes to the borrower record. sub DenyModifications { my ( $self, $borrowernumber ) = @_; - if ( ref($self) ) { - $borrowernumber = - ($borrowernumber) ? $borrowernumber : $self->{'borrowernumber'}; - } + $borrowernumber = + ($borrowernumber) ? $borrowernumber : $self->{'borrowernumber'}; return unless $borrowernumber; - return $self->DelModifications( borrowernumber => $borrowernumber ); + return $self->DelModifications({ borrowernumber => $borrowernumber }); } =head2 DelModifications -Koha::Borrower::Modifications->DelModifications( +Koha::Borrower::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 ( $self, $params ) = @_; my ( $field, $value ); - if ( $params{'borrowernumber'} ) { + if ( $params->{'borrowernumber'} ) { $field = 'borrowernumber'; - $value = $params{'borrowernumber'}; + $value = $params->{'borrowernumber'}; } - elsif ( $params{'verification_token'} ) { + elsif ( $params->{'verification_token'} ) { $field = 'verification_token'; - $value = $params{'verification_token'}; - } - - if ( ref($self) && !$value ) { - if ( $self->{'borrowernumber'} ) { - $field = 'borrowernumber'; - $value = $self->{'borrowernumber'}; - } - elsif ( $self->{'verification_token'} ) { - $field = 'verification_token'; - $value = $self->{'verification_token'}; - } + $value = $params->{'verification_token'}; } return unless $value; @@ -309,38 +273,27 @@ sub DelModifications { =head2 GetModifications -$hashref = Koha::Borrower::Modifications->GetModifications( +$hashref = Koha::Borrower::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 ( $self, $params ) = @_; my ( $field, $value ); - if ( $params{'borrowernumber'} ) { + if ( defined( $params->{'borrowernumber'} ) ) { $field = 'borrowernumber'; - $value = $params{'borrowernumber'}; + $value = $params->{'borrowernumber'}; } - elsif ( $params{'verification_token'} ) { + elsif ( defined( $params->{'verification_token'} ) ) { $field = 'verification_token'; - $value = $params{'verification_token'}; - } - - if ( ref($self) && !$value ) { - if ( $self->{'borrowernumber'} ) { - $field = 'borrowernumber'; - $value = $self->{'borrowernumber'}; - } - elsif ( $self->{'verification_token'} ) { - $field = 'verification_token'; - $value = $self->{'verification_token'}; - } + $value = $params->{'verification_token'}; } return unless $value; diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl index 039d499..7f8230d 100755 --- a/opac/opac-memberentry.pl +++ b/opac/opac-memberentry.pl @@ -107,7 +107,7 @@ if ( $action eq 'create' ) { Koha::Borrower::Modifications->new( verification_token => $verification_token ) - ->AddModifications(%borrower); + ->AddModifications(\%borrower); #Send verification email my $letter = C4::Letters::GetPreparedLetter( @@ -188,7 +188,7 @@ elsif ( $action eq 'update' ) { borrowernumber => $borrowernumber ); $m->DelModifications; - $m->AddModifications(%borrower_changes); + $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 682dacb..daf1e12 100755 --- a/opac/opac-registration-verify.pl +++ b/opac/opac-registration-verify.pl @@ -55,7 +55,7 @@ if ( $m->Verify() ) { ( $borrowernumber, $password ) = AddMember_Opac(%$borrower); if ($borrowernumber) { - $m->DelModifications(); + Koha::Borrower::Modifications->DelModifications({ verification_token => $token }); $template->param( password_cleartext => $password ); $template->param( diff --git a/t/db_dependent/Koha_borrower_modifications.t b/t/db_dependent/Koha_borrower_modifications.t new file mode 100755 index 0000000..cc30313 --- /dev/null +++ b/t/db_dependent/Koha_borrower_modifications.t @@ -0,0 +1,94 @@ +#!/usr/bin/perl + +use Modern::Perl; +use Test::More tests => 14; + +use C4::Context; +use C4::Members; + +use Koha::Borrower::Modifications; + +C4::Context->dbh->do("TRUNCATE TABLE borrower_modifications"); + +## Create new pending modification +Koha::Borrower::Modifications->new( verification_token => '1234567890' )->AddModifications({ surname => 'Hall', firstname => 'Kyle' }); + +## Get the new pending modification +my $borrower = Koha::Borrower::Modifications->GetModifications({ + verification_token => '1234567890' +}); + +## Verify we get the same data +ok( $borrower->{'surname'} = 'Hall' ); + +## Check the Verify method +ok( Koha::Borrower::Modifications->Verify( '1234567890' ) ); + +## Delete the pending modification +$borrower = Koha::Borrower::Modifications->DelModifications({ + verification_token => '1234567890' +}); + +## Verify it's no longer in the database +$borrower = Koha::Borrower::Modifications->GetModifications({ + verification_token => '1234567890' +}); +ok( !defined( $borrower->{'surname'} ) ); + +## Check the Verify method +ok( !Koha::Borrower::Modifications->Verify( '1234567890' ) ); + +## Create new pending modification, but for an existing borrower +Koha::Borrower::Modifications->new( borrowernumber => '2' )->AddModifications({ surname => 'Hall', firstname => 'Kyle' }); + +## Test the counter +ok( Koha::Borrower::Modifications->GetPendingModificationsCount() == 1 ); + +## Create new pending modification for another existing borrower +Koha::Borrower::Modifications->new( borrowernumber => '3' )->AddModifications({ surname => 'Smith', firstname => 'Sandy' }); + +## Test the counter +ok( Koha::Borrower::Modifications->GetPendingModificationsCount() == 2 ); + +## Check GetPendingModifications +my $pending = Koha::Borrower::Modifications->GetPendingModifications(); +ok( $pending->[0]->{'firstname'} eq 'Kyle' ); +ok( $pending->[1]->{'firstname'} eq 'Sandy' ); + +## This should delete the row from the table +Koha::Borrower::Modifications->DenyModifications( '3' ); + +## Test the counter +ok( Koha::Borrower::Modifications->GetPendingModificationsCount() == 1 ); + +## Save a copy of the borrowers original data +my $old_borrower = GetMember(borrowernumber => '2' ); + +## Apply the modifications +Koha::Borrower::Modifications->ApproveModifications( '2' ); + +## Test the counter +ok( Koha::Borrower::Modifications->GetPendingModificationsCount() == 0 ); + +## Get a copy of the borrowers current data +my $new_borrower = GetMember(borrowernumber => '2' ); + +## Check to see that the approved modifications were saved +ok( $new_borrower->{'surname'} eq 'Hall' ); + +## Now let's put it back the way it was +Koha::Borrower::Modifications->new( borrowernumber => '2' )->AddModifications({ surname => $old_borrower->{'surname'}, firstname => $old_borrower->{'firstname'} }); + +## Test the counter +ok( Koha::Borrower::Modifications->GetPendingModificationsCount() == 1 ); + +## Apply the modifications +Koha::Borrower::Modifications->ApproveModifications( '2' ); + +## Test the counter +ok( Koha::Borrower::Modifications->GetPendingModificationsCount() == 0 ); + +$new_borrower = GetMember(borrowernumber => '2' ); + +## Test to verify the borrower has been updated with the original values +ok( $new_borrower->{'surname'} eq $old_borrower->{'surname'} ); -- 1.7.9.5