Bug 16960 - Patron::Modifications should be fixed
authorKyle M Hall <kyle@bywatersolutions.com>
Fri, 22 Jul 2016 16:50:50 +0000 (16:50 +0000)
committerKyle M Hall <kyle@bywatersolutions.com>
Mon, 22 Aug 2016 11:46:05 +0000 (11:46 +0000)
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

Signed-off-by: Bob Birchall <bob@calyx.net.au>

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>

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

Koha/Patron/Modification.pm
Koha/Patron/Modifications.pm
mainpage.pl
members/members-home.pl
members/members-update-do.pl
members/members-update.pl
opac/opac-memberentry.pl
opac/opac-registration-verify.pl
t/db_dependent/Koha_borrower_modifications.t

index a06aec9..8afbd5e 100644 (file)
@@ -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
index 7e17289..088f7a0 100644 (file)
@@ -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';
 }
index d9e01b7..f4efc65 100755 (executable)
@@ -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(
index 247cc00..5b7c614 100755 (executable)
@@ -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,
index f55b7ab..db4686a 100755 (executable)
@@ -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' ) { }
     }
 }
 
index 1441346..c97bcc5 100755 (executable)
@@ -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) {
index c864c19..1bf5acf 100755 (executable)
@@ -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;
@@ -127,11 +128,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(
@@ -224,12 +225,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 ),
             );
index 5ceff26..1098af0 100755 (executable)
@@ -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 );
index f98dfeb..e09ae4b 100755 (executable)
 #!/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();