From 332895b781993651b7df32a679ab180d83460117 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Tue, 13 Dec 2016 17:45:41 -0300 Subject: [PATCH] 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 Signed-off-by: Jonathan Druart Signed-off-by: Kyle M Hall --- Koha/Exceptions/Patron/Modification.pm | 4 + Koha/Patron/Modification.pm | 73 +++++++++++++++-- Koha/Patron/Modifications.pm | 5 ++ t/db_dependent/Koha_borrower_modifications.t | 84 ++++++++++++++++++-- 4 files changed, 153 insertions(+), 13 deletions(-) diff --git a/Koha/Exceptions/Patron/Modification.pm b/Koha/Exceptions/Patron/Modification.pm index 4cf5cfafb1..d9ba650e64 100644 --- 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 d1e033b3b6..696f7e8da5 100644 --- 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 088f7a0c84..3ecef11117 100644 --- 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 0a03821d7d..febe2f9d46 100755 --- 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 . + 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; -- 2.39.5