From 58cf32dfa1e6a4132dd99650ef19c57359eaaf89 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Fri, 28 Jul 2017 12:52:42 -0400 Subject: [PATCH] Bug 16117: Log borrower cardnumbers and item barcodes which are not valid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We should be able to log invalid patrons and items for trouble-shooting purposes. It is especially helpful when determining if there are issues with barcode scanners. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Attempt to check out to an invalid patron cardnumber 4) Attempt to checkout a non-existent barcode 5) Attempt to check in a non-existant barcode 6) Note nothing new is added to the statistics table 7) Enable the new system preferences LogInvalidItems and LogInvalidPatrons 8) Repeat steps 4-6 9) Note each action creates a new line in the statistics table Signed-off-by: Marc VĂ©ron --- C4/Circulation.pm | 3 + Koha/Statistic.pm | 2 +- Koha/Statistics.pm | 73 ++++++++++++++++++- circ/circulation.pl | 2 + .../data/mysql/atomicupdate/bug_16117.pl | 19 +++++ installer/data/mysql/mandatory/sysprefs.sql | 2 + .../admin/preferences/circulation.pref | 12 +++ opac/sco/sco-main.pl | 6 +- t/db_dependent/Koha/Statistics.t | 59 ++++++++++++++- 9 files changed, 174 insertions(+), 4 deletions(-) create mode 100755 installer/data/mysql/atomicupdate/bug_16117.pl diff --git a/C4/Circulation.pm b/C4/Circulation.pm index e11a94e26b6..23a1b9e1578 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -63,6 +63,7 @@ use Koha::SearchEngine::Indexer; use Koha::Exceptions::Checkout; use Koha::Plugins; use Koha::Recalls; +use Koha::Statistics; use Carp qw( carp ); use List::MoreUtils qw( any ); use Scalar::Util qw( looks_like_number blessed ); @@ -778,6 +779,7 @@ sub CanBookBeIssued { # MANDATORY CHECKS - unless item exists, nothing else matters unless ( $item_object ) { + Koha::Statistics->log_invalid_item( { item => $barcode } ); $issuingimpossible{UNKNOWN_BARCODE} = 1; } return ( \%issuingimpossible, \%needsconfirmation ) if %issuingimpossible; @@ -2122,6 +2124,7 @@ sub AddReturn { # get information on item my $item = Koha::Items->find({ barcode => $barcode }); unless ($item) { + Koha::Statistics->log_invalid_item( { item => $barcode } ); return ( 0, { BadBarcode => $barcode } ); # no barcode means no item or borrower. bail out. } diff --git a/Koha/Statistic.pm b/Koha/Statistic.pm index c3deb71b2a1..2161a91ad77 100644 --- a/Koha/Statistic.pm +++ b/Koha/Statistic.pm @@ -28,7 +28,7 @@ use Koha::PseudonymizedTransaction; use base qw(Koha::Object); our @allowed_accounts_types = qw( writeoff payment ); -our @allowed_circulation_types = qw( renew issue localuse return onsite_checkout recall item_found item_lost ); +our @allowed_circulation_types = qw( renew issue localuse return onsite_checkout recall item_found item_lost invalid_item invalid_patron); our @mandatory_accounts_keys = qw( type branch borrowernumber value ); # note that amount is mapped to value our @mandatory_circulation_keys = qw( type branch borrowernumber itemnumber ccode itemtype ); diff --git a/Koha/Statistics.pm b/Koha/Statistics.pm index 4b3a4ebf391..219da6867ea 100644 --- a/Koha/Statistics.pm +++ b/Koha/Statistics.pm @@ -21,6 +21,7 @@ use Modern::Perl; use Koha::Database; use Koha::Statistic; +use Koha::DateUtils qw(dt_from_string); use base qw(Koha::Objects); @@ -32,9 +33,79 @@ Koha::Statistics - Koha Statistic Object set class =head2 Class Methods +=head3 log_invalid_patron + +Koha::Statistics->log_invalid_patron( { patron => $cardnumber } ); + +=cut + +sub log_invalid_patron { + return unless C4::Context->preference('LogInvalidPatrons'); + + my ( $class, $params ) = @_; + + my $patron = $params->{patron}; + + return $class->_log_invalid_value( + { + type => 'patron', + value => $patron + } + ); +} + +=head3 log_invalid_item + +Koha::Statistics->log_invalid_item( { item => $barcode } ); + =cut -=head3 type +sub log_invalid_item { + return unless C4::Context->preference('LogInvalidItems'); + + my ( $class, $params ) = @_; + + my $item = $params->{item}; + + return $class->_log_invalid_value( + { + type => 'item', + value => $item + } + ); +} + +=head3 invalid_value + +Koha::Statistics->invalid_value( { type => 'patron', value => $patron } ); + +=cut + +sub _log_invalid_value { + my ( $class, $params ) = @_; + + my $type = $params->{type}; + my $value = $params->{value}; + + my $branch = C4::Context->userenv ? C4::Context->userenv->{branch} : undef; + my $dt = dt_from_string(); + my $borrowernumber = C4::Context->userenv->{'number'}; + + return Koha::Statistic->new( + { + type => "invalid_$type", + other => $value, + itemnumber => "", + ccode => "", + itemtype => "", + datetime => $dt, + branch => $branch, + borrowernumber => $borrowernumber + } + )->store(); +} + +=head3 _type =cut diff --git a/circ/circulation.pl b/circ/circulation.pl index 63ad77fdffd..af716c20050 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -55,6 +55,7 @@ use Koha::SearchEngine; use Koha::SearchEngine::Search; use Koha::Patron::Modifications; use Koha::Token; +use Koha::Statistics; use List::MoreUtils qw( uniq ); @@ -240,6 +241,7 @@ if ($findborrower) { if ( $patron ) { $borrowernumber = $patron->borrowernumber; } else { + Koha::Statistics->log_invalid_patron( { patron => $findborrower } ); print $query->redirect( "/cgi-bin/koha/members/member.pl?quicksearch=1&circsearch=1&searchmember=" . uri_escape_utf8($findborrower) ); exit; } diff --git a/installer/data/mysql/atomicupdate/bug_16117.pl b/installer/data/mysql/atomicupdate/bug_16117.pl new file mode 100755 index 00000000000..21510aeaa80 --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_16117.pl @@ -0,0 +1,19 @@ +use Modern::Perl; + +return { + bug_number => "16117", + description => "Log invalid borrower cardnumbers and item barcodes", + up => sub { + my ($args) = @_; + my ($dbh, $out) = @$args{qw(dbh out)}; + + $dbh->do(q{ + INSERT IGNORE INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `type` ) VALUES + ('LogInvalidItems','0','','Log scanned invalid item identifiers as statistics','YesNo'), + ('LogInvalidPatrons','0','','Log scanned invalid patron identifiers as statistics','YesNo'); + }); + # sysprefs + say $out "Added new system preference 'LogInvalidItems'"; + say $out "Added new system preference 'LogInvalidPatrons'"; + }, +}; diff --git a/installer/data/mysql/mandatory/sysprefs.sql b/installer/data/mysql/mandatory/sysprefs.sql index 50ab4e15212..d3edc8d3dbb 100644 --- a/installer/data/mysql/mandatory/sysprefs.sql +++ b/installer/data/mysql/mandatory/sysprefs.sql @@ -366,6 +366,8 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('LocalHoldsPriorityItemControl', 'holdingbranch', 'holdingbranch|homebranch', 'decides if the feature operates using the item''s home or holding library.', 'Choice'), ('LocalHoldsPriorityPatronControl', 'PickupLibrary', 'HomeLibrary|PickupLibrary', 'decides if the feature operates using the library set as the patron''s home library, or the library set as the pickup library for the given hold.', 'Choice'), ('LockExpiredDelay','','','Delay for locking expired patrons (empty means no locking)','Integer'), +('LogInvalidItems','0','','Log scanned invalid item identifiers as statistics','YesNo'), +('LogInvalidPatrons','0','','Log scanned invalid patron identifiers as statistics','YesNo'), ('makePreviousSerialAvailable','0','','make previous serial automatically available when collecting a new serial. Please note that the item-level_itypes syspref must be set to specific item.','YesNo'), ('Mana','2',NULL,'request to Mana Webservice. Mana centralize common information between other Koha to facilitate the creation of new subscriptions, vendors, report queries etc... You can search, share, import and comment the content of Mana.','Choice'), ('MARCAuthorityControlField008','|| aca||aabn | a|a d',NULL,'Define the contents of MARC21 authority control field 008 position 06-39','Textarea'), diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref index 5cb207d3f0e..15579b9dc63 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref @@ -19,6 +19,18 @@ Circulation: 0: "Don't enable" - the automatic redirection to another patron when a patron barcode is scanned instead of a book. - This should not be enabled if you have overlapping patron and book barcodes. + - + - pref: LogInvalidPatrons + choices: + yes: Log + no: Do not log + - scanned invalid patron identifiers as statistics. + - + - pref: LogInvalidItems + choices: + yes: Log + no: Do not log + - scanned invalid item identifiers as statistics. - - pref: PatronAutoComplete choices: diff --git a/opac/sco/sco-main.pl b/opac/sco/sco-main.pl index bffa06f1c71..5ac71d00367 100755 --- a/opac/sco/sco-main.pl +++ b/opac/sco/sco-main.pl @@ -131,6 +131,7 @@ my $patron; if ( $patronid ) { Koha::Plugins->call( 'patron_barcode_transform', \$patronid ); $patron = Koha::Patrons->find( { cardnumber => $patronid } ); + Koha::Statistics->log_invalid_patron( { patron => $patronid } ) unless $patron; } undef $jwt unless $patron; @@ -142,8 +143,9 @@ my $return_only = 0; if ( $patron && $op eq "returnbook" && $allowselfcheckreturns ) { my $success = 1; - my $item = Koha::Items->find( { barcode => $barcode } ); + Koha::Statistics->log_invalid_item( { item => $barcode } ) unless $item; + if ( $success && C4::Context->preference("CircConfirmItemParts") ) { if ( defined($item) && $item->materials ) @@ -167,6 +169,7 @@ if ( $patron && $op eq "returnbook" && $allowselfcheckreturns ) { elsif ( $patron && ( $op eq 'checkout' ) ) { my $item = Koha::Items->find( { barcode => $barcode } ); + Koha::Statistics->log_invalid_item( { item => $barcode } ) unless $item; my $impossible = {}; my $needconfirm = {}; ( $impossible, $needconfirm ) = CanBookBeIssued( @@ -277,6 +280,7 @@ elsif ( $patron && ( $op eq 'checkout' ) ) { if ( $patron && ( $op eq 'renew' ) ) { my $item = Koha::Items->find({ barcode => $barcode }); + Koha::Statistics->log_invalid_item( { item => $barcode } ) unless $item; if ( $patron->checkouts->find( { itemnumber => $item->itemnumber } ) ) { my ($status,$renewerror) = CanBookBeRenewed( $patron, $item->checkout ); diff --git a/t/db_dependent/Koha/Statistics.t b/t/db_dependent/Koha/Statistics.t index 30b6e571cf4..6cfc2c61748 100755 --- a/t/db_dependent/Koha/Statistics.t +++ b/t/db_dependent/Koha/Statistics.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 3; +use Test::More tests => 4; use Test::Exception; use C4::Context; @@ -27,6 +27,8 @@ use Koha::Database; use Koha::Statistics; use t::lib::TestBuilder; +use t::lib::Mocks; +use Test::MockModule; our $schema = Koha::Database->new->schema; our $builder = t::lib::TestBuilder->new; @@ -159,3 +161,58 @@ sub insert_and_fetch { # FIXME discard_changes would be nicer, but we dont have a PK (yet) } + +subtest 'Log borrower cardnumbers and item barcodes which are not valid' => sub { + plan tests => 10; + $schema->storage->txn_begin; + + #my $builder = t::lib::TestBuilder->new; + my $library = $builder->build( { source => 'Branch' } ); + my $branchcode = $library->{branchcode}; + my $context = Test::MockModule->new('C4::Context'); + $context->mock( + 'userenv', + sub { + return { + flags => 1, + id => 'my_userid', + branch => $branchcode, + number => '-1', + }; + } + ); + + # Test Koha::Statistic->log_invalid_patron + my $dbh = $schema->storage->dbh; + $dbh->do(q{DELETE FROM statistics}); + t::lib::Mocks::mock_preference( "LogInvalidPatrons", 0 ); + Koha::Statistics->log_invalid_patron( { patron => 'InvalidCardnumber' } ); + is( + Koha::Statistics->search()->count(), 0, + 'No stat line added if system preference LogInvalidPatrons is disabled' + ); + + t::lib::Mocks::mock_preference( "LogInvalidPatrons", 1 ); + Koha::Statistics->log_invalid_patron( { patron => 'InvalidCardnumber' } ); + my $stat = Koha::Statistics->search()->next(); + is( $stat->type, 'invalid_patron', 'Type set to invalid_patron' ); + is( $stat->borrowernumber, '-1', 'Associated library id set correctly' ); + is( $stat->other, 'InvalidCardnumber', 'Invalid cardnumber is set correctly' ); + is( $stat->branch, $branchcode, 'Branchcode is set correctly' ); + + # Test Koha::Statistic->log_invalid_item + $dbh->do(q{DELETE FROM statistics}); + t::lib::Mocks::mock_preference( "LogInvalidItems", 0 ); + Koha::Statistics->log_invalid_item( { item => 'InvalidBarcode' } ); + is( Koha::Statistics->search()->count(), 0, 'No stat line added if system preference LogInvalidItems is disabled' ); + + t::lib::Mocks::mock_preference( "LogInvalidItems", 1 ); + Koha::Statistics->log_invalid_item( { item => 'InvalidBarcode' } ); + $stat = Koha::Statistics->search()->next(); + is( $stat->type, 'invalid_item', 'Type set to invalid_item' ); + is( $stat->borrowernumber, '-1', 'Associated library id set correctly' ); + is( $stat->other, 'InvalidBarcode', 'Invalid barcode is set correctly' ); + is( $stat->branch, $branchcode, 'Branchcode is set correctly' ); + + $schema->storage->txn_rollback; +}; -- 2.30.2