From cb62a145f5acf596713f1358cac3ed822bf04f97 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Wed, 19 Jun 2019 17:59:14 +0000 Subject: [PATCH] Bug 20816: [19.11.x] Add ability to define custom templated fields in SIP patron responses To test: 1 - You will need to enable SIP on your testing instance cp etc/SIPconfig.xml /etc/koha/sites/kohadev/ sudo koha-start-sip add a user listed in the SIPconfig to your system and give them permissions (superlibrarian works) on koha-testing-docker you should be able to start sip with user koha/koha without any adjustments 2 - If you copied the above file you should be set to get custom field DE with dateexpiry Otherwise edit the sip login for the user to have a custom section like: 3 - send a status test using the sip cli tester: perl misc/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su koha -sp koha -l kohalibrary --patron 23529001000463 -m patron_status_request 4 - send an information test using the sip cli tester: perl misc/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su koha -sp koha -l kohalibrary --patron 23529001000463 -m patron_information 5 - confirm you receive the DE field with a dateexpiry 6 - Add your own custom fields and confirm it works with several 7 - prove -v t/db_dependent/SIP/Patron.t 8 - prove -v t/db_dependent/SIP/ Signed-off-by: Andrew Fuerste-Henry Signed-off-by: Martin Renvoize Bug 20816: Make SIP tests pass under ES Signed-off-by: Andrew Fuerste-Henry Signed-off-by: Martin Renvoize Signed-off-by: Joy Nelson --- C4/SIP/ILS/Patron.pm | 47 +++++++++++++++++++++++++++----- C4/SIP/Sip.pm | 1 - C4/SIP/Sip/MsgType.pm | 17 ++++++++++-- etc/SIPconfig.xml | 1 + t/db_dependent/SIP/Message.t | 17 +++++++----- t/db_dependent/SIP/Patron.t | 46 ++++++++++++++++++++++++++++++- t/db_dependent/SIP/Transaction.t | 21 ++++++-------- 7 files changed, 118 insertions(+), 32 deletions(-) diff --git a/C4/SIP/ILS/Patron.pm b/C4/SIP/ILS/Patron.pm index 99546da569..b528501f39 100644 --- a/C4/SIP/ILS/Patron.pm +++ b/C4/SIP/ILS/Patron.pm @@ -15,7 +15,7 @@ use Carp; use Sys::Syslog qw(syslog); use Data::Dumper; -use C4::SIP::Sip qw(add_field); +use C4::SIP::Sip qw(add_field maybe_add); use C4::Debug; use C4::Context; @@ -216,7 +216,14 @@ sub AUTOLOAD { } } -sub name { +=head2 format + +This method uses a template to build a string from a Koha::Patron object +If errors are encountered in processing template we log them and return nothing + +=cut + +sub format { my ( $self, $template ) = @_; if ($template) { @@ -228,12 +235,15 @@ sub name { my $patron = Koha::Patrons->find( $self->{borrowernumber} ); my $output; - $tt->process( \$template, { patron => $patron }, \$output ); + eval { + $tt->process( \$template, { patron => $patron }, \$output ); + }; + if ( $@ ){ + syslog("LOG_DEBUG", "Error processing template: $template"); + return ""; + } return $output; } - else { - return $self->{name}; - } } sub check_password { @@ -528,7 +538,6 @@ sub build_patron_attributes_string { my ( $self, $server ) = @_; my $string = q{}; - if ( $server->{account}->{patron_attribute} ) { my @attributes_to_send = ref $server->{account}->{patron_attribute} eq "ARRAY" @@ -553,6 +562,30 @@ sub build_patron_attributes_string { return $string; } + +=head2 build_custom_field_string + +This method builds the part of the sip message for custom patron fields as defined in the sip config + +=cut + +sub build_custom_field_string { + my ( $self, $server ) = @_; + + my $string = q{}; + + if ( $server->{account}->{custom_patron_field} ) { + my @custom_fields = + ref $server->{account}->{custom_patron_field} eq "ARRAY" + ? @{ $server->{account}->{custom_patron_field} } + : $server->{account}->{custom_patron_field}; + foreach my $custom_field ( @custom_fields ) { + $string .= maybe_add( $custom_field->{field}, $self->format( $custom_field->{template} ) ) if defined $custom_field->{field}; + } + } + return $string; +} + 1; __END__ diff --git a/C4/SIP/Sip.pm b/C4/SIP/Sip.pm index a708fa148a..2dc4749214 100644 --- a/C4/SIP/Sip.pm +++ b/C4/SIP/Sip.pm @@ -108,7 +108,6 @@ sub maybe_add { $value =~ s/$regex->{find}/$regex->{replace}/g; } } - return (defined($value) && $value) ? add_field($fid, $value) : ''; } diff --git a/C4/SIP/Sip/MsgType.pm b/C4/SIP/Sip/MsgType.pm index 2a82a85e64..1ff9bb0093 100644 --- a/C4/SIP/Sip/MsgType.pm +++ b/C4/SIP/Sip/MsgType.pm @@ -435,7 +435,12 @@ sub build_patron_status { $resp .= patron_status_string($patron); $resp .= $lang . timestamp(); - $resp .= add_field( FID_PERSONAL_NAME, $patron->name( $server->{account}->{ae_field_template} ), $server ); + if ( defined $server->{account}->{ae_field_template} ) { + $resp .= add_field( FID_PERSONAL_NAME, $patron->format( $server->{account}->{ae_field_template}, $server ) ); + } else { + $resp .= add_field( FID_PERSONAL_NAME, $patron->name, $server ); + } + # while the patron ID we got from the SC is valid, let's # use the one returned from the ILS, just in case... @@ -458,6 +463,7 @@ sub build_patron_status { if ( $server->{account}->{send_patron_home_library_in_af} ); $resp .= maybe_add( FID_PRINT_LINE, $patron->print_line, $server ); + $resp .= $patron->build_custom_field_string( $server ); $resp .= $patron->build_patron_attributes_string( $server ); } else { @@ -971,7 +977,11 @@ sub handle_patron_info { # while the patron ID we got from the SC is valid, let's # use the one returned from the ILS, just in case... $resp .= add_field( FID_PATRON_ID, $patron->id, $server ); - $resp .= add_field( FID_PERSONAL_NAME, $patron->name( $server->{account}->{ae_field_template} ), $server ); + if ( defined $server->{account}->{ae_field_template} ) { + $resp .= add_field( FID_PERSONAL_NAME, $patron->format( $server->{account}->{ae_field_template} ), $server ); + } else { + $resp .= add_field( FID_PERSONAL_NAME, $patron->name, $server ); + } # TODO: add code for the fields # hold items limit @@ -1026,6 +1036,7 @@ sub handle_patron_info { } $resp .= maybe_add( FID_PRINT_LINE, $patron->print_line, $server ); + $resp .= $patron->build_custom_field_string( $server ); $resp .= $patron->build_patron_attributes_string( $server ); } else { @@ -1274,7 +1285,7 @@ sub handle_patron_enable { $resp .= $patron->language . timestamp(); $resp .= add_field( FID_PATRON_ID, $patron->id, $server ); - $resp .= add_field( FID_PERSONAL_NAME, $patron->name( $server->{account}->{ae_field_template} ), $server ); + $resp .= add_field( FID_PERSONAL_NAME, $patron->format( $server->{account}->{ae_field_template} ), $server ); if ( defined($patron_pwd) ) { $resp .= add_field( FID_VALID_PATRON_PWD, sipbool( $patron->check_password($patron_pwd) ), $server ); } diff --git a/etc/SIPconfig.xml b/etc/SIPconfig.xml index 1e5c2314a2..883cc15aed 100644 --- a/etc/SIPconfig.xml +++ b/etc/SIPconfig.xml @@ -65,6 +65,7 @@ 0 + diff --git a/t/db_dependent/SIP/Message.t b/t/db_dependent/SIP/Message.t index bc8e87a4b3..75ecb7f336 100755 --- a/t/db_dependent/SIP/Message.t +++ b/t/db_dependent/SIP/Message.t @@ -326,11 +326,14 @@ sub test_checkin_v2 { my $card1 = $patron1->{cardnumber}; my $sip_patron1 = C4::SIP::ILS::Patron->new( $card1 ); $findpatron = $sip_patron1; - my $item = $builder->build({ - source => 'Item', - value => { damaged => 0, withdrawn => 0, itemlost => 0, restricted => 0, homebranch => $branchcode, holdingbranch => $branchcode }, + my $item_object = $builder->build_sample_item({ + damaged => 0, + withdrawn => 0, + itemlost => 0, + restricted => 0, + homebranch => $branchcode, + holdingbranch => $branchcode, }); - my $item_object = Koha::Items->find( $item->{itemnumber} ); my $mockILS = $mocks->{ils}; my $server = { ils => $mockILS, account => {} }; @@ -364,7 +367,7 @@ sub test_checkin_v2 { $siprequest = CHECKIN . 'N' . 'YYYYMMDDZZZZHHMMSS' . siprequestdate( $today->clone->add( days => 1) ) . FID_INST_ID . $branchcode . '|'. - FID_ITEM_ID . $item->{barcode} . '|' . + FID_ITEM_ID . $item_object->barcode . '|' . FID_TERMINAL_PWD . 'ignored' . '|'; undef $response; $msg = C4::SIP::Sip::MsgType->new( $siprequest, 0 ); @@ -443,7 +446,7 @@ sub test_checkin_v2 { $server->{account}->{ct_always_send} = 0; # Checkin at wrong branch: issue item and switch branch, and checkin - my $issue = Koha::Checkout->new({ branchcode => $branchcode, borrowernumber => $patron1->{borrowernumber}, itemnumber => $item->{itemnumber} })->store; + my $issue = Koha::Checkout->new({ branchcode => $branchcode, borrowernumber => $patron1->{borrowernumber}, itemnumber => $item_object->itemnumber })->store; $branchcode = $builder->build({ source => 'Branch' })->{branchcode}; t::lib::Mocks::mock_preference( 'AllowReturnToBranch', 'homebranch' ); undef $response; @@ -452,7 +455,7 @@ sub test_checkin_v2 { is( substr($response,2,1), '0', 'OK flag is false when we check in at the wrong branch and we do not allow it' ); is( substr($response,5,1), 'Y', 'Alert flag is set' ); check_field( $respcode, $response, FID_SCREEN_MSG, 'Checkin failed', 'Check screen msg' ); - $branchcode = $item->{homebranch}; # switch back + $branchcode = $item_object->homebranch; # switch back t::lib::Mocks::mock_preference( 'AllowReturnToBranch', 'anywhere' ); # Data corrupted: add same issue_id to old_issues diff --git a/t/db_dependent/SIP/Patron.t b/t/db_dependent/SIP/Patron.t index 0f20ac3e5c..d517c7693b 100755 --- a/t/db_dependent/SIP/Patron.t +++ b/t/db_dependent/SIP/Patron.t @@ -4,7 +4,7 @@ # This needs to be extended! Your help is appreciated.. use Modern::Perl; -use Test::More tests => 5; +use Test::More tests => 6; use Koha::Database; use Koha::Patrons; @@ -116,6 +116,50 @@ subtest "Test build_patron_attribute_string" => sub { is( $attribute_string, "XYTest Attribute|YZAnother Test Attribute|", 'Attribute field generated correctly with multiple params' ); }; +subtest "Test build_custom_field_string" => sub { + + plan tests => 5; + + my $patron = $builder->build_object( { class => 'Koha::Patrons',value=>{surname => "Duck", firstname => "Darkwing"} } ); + + + my $ils_patron = C4::SIP::ILS::Patron->new( $patron->cardnumber ); + + my $server = {}; + $server->{account}->{custom_patron_field}->{field} = "DW"; + my $attribute_string = $ils_patron->build_custom_field_string( $server ); + is( $attribute_string, "", 'Custom field not generated if no value passed' ); + + $server = {}; + $server->{account}->{custom_patron_field}->{template} = "[% patron.surname %]"; + $attribute_string = $ils_patron->build_custom_field_string( $server ); + is( $attribute_string, "", 'Custom field not generated if no field passed' ); + + + $server = {}; + $server->{account}->{custom_patron_field}->{field} = "DW"; + $server->{account}->{custom_patron_field}->{template} = "[% patron.firstname %] [% patron.surname %], let's get dangerous!"; + $attribute_string = $ils_patron->build_custom_field_string( $server ); + is( $attribute_string, "DWDarkwing Duck, let's get dangerous!|", 'Custom field processed correctly' ); + + $server = {}; + $server->{account}->{custom_patron_field}->[0]->{field} = "DW"; + $server->{account}->{custom_patron_field}->[0]->{template} = "[% patron.firstname %] [% patron.surname %], let's get dangerous!"; + $server->{account}->{custom_patron_field}->[1]->{field} = "LM"; + $server->{account}->{custom_patron_field}->[1]->{template} = "Launchpad McQuack crashed on [% patron.dateexpiry %]"; + $attribute_string = $ils_patron->build_custom_field_string( $server ); + is( $attribute_string, "DWDarkwing Duck, let's get dangerous!|LMLaunchpad McQuack crashed on ".$patron->dateexpiry."|", 'Custom fields processed correctly when multiple exist' ); + + $server = {}; + $server->{account}->{custom_patron_field}->[0]->{field} = "DW"; + $server->{account}->{custom_patron_field}->[0]->{template} = "[% IF (patron.firstname) %] patron.surname, let's get dangerous!"; + $server->{account}->{custom_patron_field}->[1]->{field} = "LM"; + $server->{account}->{custom_patron_field}->[1]->{template} = "Launchpad McQuack crashed on [% patron.dateexpiry %]"; + $attribute_string = $ils_patron->build_custom_field_string( $server ); + is( $attribute_string, "LMLaunchpad McQuack crashed on ".$patron->dateexpiry."|", 'Custom fields processed correctly, bad template generate no text' ); + +}; + subtest "update_lastseen tests" => sub { plan tests => 2; diff --git a/t/db_dependent/SIP/Transaction.t b/t/db_dependent/SIP/Transaction.t index fa149e28ec..c6b2ce7abc 100755 --- a/t/db_dependent/SIP/Transaction.t +++ b/t/db_dependent/SIP/Transaction.t @@ -50,25 +50,20 @@ subtest fill_holds_at_checkout => sub { t::lib::Mocks::mock_userenv({ branchcode => $branch->{branchcode}, flags => 1 }); my $itype = $builder->build({ source => 'Itemtype', value =>{notforloan=>0} }); - my $biblio = $builder->build({ source => 'Biblio' }); - my $biblioitem = $builder->build({ source => 'Biblioitem', value=>{biblionumber=>$biblio->{biblionumber}} }); - my $item1 = $builder->build({ source => 'Item', value => { + my $item1 = $builder->build_sample_item({ barcode => 'barcode4test', homebranch => $branch->{branchcode}, holdingbranch => $branch->{branchcode}, - biblionumber => $biblio->{biblionumber}, itype => $itype->{itemtype}, notforloan => 0, - } - }); - my $item2 = $builder->build({ source => 'Item', value => { + })->unblessed; + my $item2 = $builder->build_sample_item({ homebranch => $branch->{branchcode}, holdingbranch => $branch->{branchcode}, - biblionumber => $biblio->{biblionumber}, + biblionumber => $item1->{biblionumber}, itype => $itype->{itemtype}, notforloan => 0, - } - }); + })->unblessed; Koha::IssuingRule->new({ categorycode => $borrower->{categorycode}, @@ -81,9 +76,9 @@ subtest fill_holds_at_checkout => sub { lengthunit => 'days', })->store; - my $reserve1 = AddReserve($branch->{branchcode},$borrower->{borrowernumber},$biblio->{biblionumber}); - my $reserve2 = AddReserve($branch->{branchcode},$borrower->{borrowernumber},$biblio->{biblionumber}); - my $bib = Koha::Biblios->find( $biblio->{biblionumber} ); + my $reserve1 = AddReserve($branch->{branchcode},$borrower->{borrowernumber},$item1->{biblionumber}); + my $reserve2 = AddReserve($branch->{branchcode},$borrower->{borrowernumber},$item1->{biblionumber}); + my $bib = Koha::Biblios->find( $item1->{biblionumber} ); is( $bib->holds->count(), 2, "Bib has 2 holds"); my $sip_patron = C4::SIP::ILS::Patron->new( $borrower->{cardnumber} ); -- 2.39.5