Bug 17767: Make Koha::Patron::Modification handle extended attributes

This patch makes Koha::Patron::Modification aware of the new extended_attributes
column, which is expected to contain valid JSON data.

The ->store method is modified so it validates the field value (i.e. the content
is decoded using the JSON library) and raises a convenient exception in case of
failure.

This behaviour change is covered by the provided unit tests.

To test:
- Apply the patchset
- Run:
  $ prove t/db_dependent/Koha/Patron/Modifications.t
=> SUCCESS: Tests make sense, and they pass
- Sign off :-D

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

diff --git a/Koha/Exceptions/Patron/Modification.pm b/Koha/Exceptions/Patron/Modification.pm
index 4cf5cfa..d9ba650 100644 (file)
--- a/Koha/Exceptions/Patron/Modification.pm
+++ b/Koha/Exceptions/Patron/Modification.pm
@@ -10,6 +10,10 @@ use Exception::Class (
     'Koha::Exceptions::Patron::Modification::DuplicateVerificationToken' => {
         isa => 'Koha::Exceptions::Patron::Modification',
         description => "The verification token given already exists"
+    },
+    'Koha::Exceptions::Patron::Modification::InvalidData' => {
+        isa => 'Koha::Exceptions::Patron::Modification',
+        description => "Some passed data is invalid"
     }
 );
 

diff --git a/Koha/Patron/Modification.pm b/Koha/Patron/Modification.pm
index d1e033b..696f7e8 100644 (file)
--- a/Koha/Patron/Modification.pm
+++ b/Koha/Patron/Modification.pm
@@ -22,9 +22,13 @@ use Modern::Perl;
 use Carp;
 
 use Koha::Database;
-
-use Koha::Patron::Modifications;
 use Koha::Exceptions::Patron::Modification;
+use Koha::Patron::Modifications;
+# TODO: Remove once Koha::Patron::Attribute(s) is implemented
+use C4::Members::Attributes qw( SetBorrowerAttributes );
+
+use JSON;
+use Try::Tiny;
 
 use base qw(Koha::Object);
 
@@ -44,16 +48,32 @@ sub store {
     my ($self) = @_;
 
     if ( $self->verification_token ) {
-        if ( Koha::Patron::Modifications->search( { verification_token => $self->verification_token } )->count() ) {
-            Koha::Exceptions::Patron::Modification::DuplicateVerificationToken->throw(
-                "Duplicate verification token " . $self->verification_token
-            );
+        if (Koha::Patron::Modifications->search(
+                { verification_token => $self->verification_token }
+            )->count()
+            )
+        {
+            Koha::Exceptions::Patron::Modification::DuplicateVerificationToken
+                ->throw(
+                "Duplicate verification token " . $self->verification_token );
+        }
+    }
+
+    if ( $self->extended_attributes ) {
+        try {
+            my $json_parser = JSON->new;
+            $json_parser->decode( $self->extended_attributes );
         }
+        catch {
+            Koha::Exceptions::Patron::Modification::InvalidData->throw(
+                'The passed extended_attributes is not valid JSON');
+        };
     }
 
     return $self->SUPER::store();
 }
 
+
 =head2 approve
 
 $m->approve();
@@ -70,6 +90,7 @@ sub approve {
 
     delete $data->{timestamp};
     delete $data->{verification_token};
+    delete $data->{extended_attributes};
 
     foreach my $key ( keys %$data ) {
         delete $data->{$key} unless ( defined( $data->{$key} ) );
@@ -81,11 +102,47 @@ sub approve {
 
     $patron->set($data);
 
-    if ( $patron->store() ) {
-        return $self->delete();
+    # Take care of extended attributes
+    if ( $self->extended_attributes ) {
+        our $extended_attributes
+            = try { decode_json( $self->extended_attributes ) }
+        catch {
+            Koha::Exceptions::Patron::Modification::InvalidData->throw(
+                'The passed extended_attributes is not valid JSON');
+        };
     }
+
+    $self->_result->result_source->schema->txn_do(
+        sub {
+            try {
+                $patron->store();
+
+                # Take care of extended attributes
+                if ( $self->extended_attributes ) {
+                    my $extended_attributes
+                        = decode_json( $self->extended_attributes );
+                    SetBorrowerAttributes( $patron->borrowernumber,
+                        $extended_attributes );
+                }
+            }
+            catch {
+                if ( $_->isa('DBIx::Class::Exception') ) {
+                    Koha::Exceptions::Patron::Modification->throw(
+                        $_->{msg} );
+                }
+                else {
+                    Koha::Exceptions::Patron::Modification->throw($@);
+                }
+            };
+        }
+    );
+
+    return $self->delete();
 }
 
+
+
+
 =head3 type
 
 =cut

diff --git a/Koha/Patron/Modifications.pm b/Koha/Patron/Modifications.pm
index 088f7a0..3ecef11 100644 (file)
--- a/Koha/Patron/Modifications.pm
+++ b/Koha/Patron/Modifications.pm
@@ -28,6 +28,8 @@ use C4::Context;
 
 use Koha::Patron::Modification;
 
+use JSON;
+
 use base qw(Koha::Objects);
 
 =head2 pending_count
@@ -93,6 +95,9 @@ sub pending {
     my @m;
     while ( my $row = $sth->fetchrow_hashref() ) {
         foreach my $key ( keys %$row ) {
+            if ( defined $row->{$key} && $key eq 'extended_attributes' ) {
+                $row->{$key} = from_json($row->{$key});
+            }
             delete $row->{$key} unless defined $row->{$key};
         }
 

diff --git a/t/db_dependent/Koha_borrower_modifications.t b/t/db_dependent/Koha_borrower_modifications.t
index 0a03821..febe2f9 100755 (executable)
--- a/t/db_dependent/Koha_borrower_modifications.t
+++ b/t/db_dependent/Koha_borrower_modifications.t
@@ -1,11 +1,30 @@
 #!/usr/bin/perl
 
+# This file is part of Koha.
+#
+# Koha is free software; you can redistribute it and/or modify it
+# under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# Koha is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Koha; if not, see <http://www.gnu.org/licenses>.
+
 use Modern::Perl;
-use Test::More tests => 10;
-use Try::Tiny;
+
+use Test::More tests => 11;
+use Test::Exception;
 
 use t::lib::TestBuilder;
 
+use String::Random qw( random_string );
+use Try::Tiny;
+
 use C4::Context;
 use C4::Members;
 
@@ -14,7 +33,61 @@ BEGIN {
     use_ok('Koha::Patron::Modifications');
 }
 
-my $schema = Koha::Database->new->schema;
+my $schema  = Koha::Database->new->schema;
+my $builder = t::lib::TestBuilder->new;
+
+
+subtest 'store( extended_attributes ) tests' => sub {
+
+    plan tests => 4;
+
+    $schema->storage->txn_begin;
+
+    Koha::Patron::Modifications->search->delete;
+
+    my $patron
+        = $builder->build( { source => 'Borrower' } )->{borrowernumber};
+    my $verification_token = random_string("..........");
+    my $valid_json_text    = '[{"code":"CODE","value":"VALUE"}]';
+    my $invalid_json_text  = '[{';
+
+    Koha::Patron::Modification->new(
+        {   verification_token  => $verification_token,
+            borrowernumber      => $patron,
+            surname             => 'Hall',
+            extended_attributes => $valid_json_text
+        }
+    )->store();
+
+    my $patron_modification
+        = Koha::Patron::Modifications->search( { borrowernumber => $patron } )
+        ->next;
+
+    is( $patron_modification->surname,
+        'Hall', 'Patron modification correctly stored with valid JSON data' );
+    is( $patron_modification->extended_attributes,
+        $valid_json_text,
+        'Patron modification correctly stored with valid JSON data' );
+
+    $verification_token = random_string("..........");
+    throws_ok {
+        Koha::Patron::Modification->new(
+            {   verification_token  => $verification_token,
+                borrowernumber      => $patron,
+                surname             => 'Hall',
+                extended_attributes => $invalid_json_text
+            }
+        )->store();
+    }
+    'Koha::Exceptions::Patron::Modification::InvalidData',
+        'Trying to store invalid JSON in extended_attributes field raises exception';
+
+    is( $@, 'The passed extended_attributes is not valid JSON' );
+
+    $schema->storage->txn_rollback;
+};
+
+
 $schema->storage->txn_begin;
 
 my $dbh = C4::Context->dbh;
@@ -52,7 +125,6 @@ my $borrower =
 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};
 
 my $m1 = Koha::Patron::Modification->new(
@@ -106,4 +178,6 @@ my $new_borrower = GetMember( borrowernumber => $borr1 );
 ok( $new_borrower->{'surname'} eq 'Hall',
     'Test approve() applies modification to borrower' );
 
-$dbh->rollback();
+$schema->storage->txn_rollback;
+
+1;