Bug 7067 QA Followup

Adjusts calling conventions to use hashrefs and eliminate redundant
procedural/OO mixed code.

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>

diff --git a/Koha/Borrower/Modifications.pm b/Koha/Borrower/Modifications.pm
index 443febb..1d32d16 100644 (file)
--- 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 (executable)
--- 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 (executable)
--- 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 (executable)
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'} );