From c675048695156be5928c86bc9758e1090ef6816d 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/Statistics.pm | 70 ++++++++++++++++++- 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/Stats.t | 54 +++++++++++++- 8 files changed, 164 insertions(+), 4 deletions(-) create mode 100755 installer/data/mysql/atomicupdate/bug_16117.pl diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 0f1b6914aa..13212ba8fa 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 ); @@ -782,6 +783,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; @@ -2095,6 +2097,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/Statistics.pm b/Koha/Statistics.pm index 4b3a4ebf39..3fd45e1725 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,76 @@ 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, + datetime => $dt, + branch => $branch, + borrowernumber => $borrowernumber + } + )->store(); +} + +=head3 _type =cut diff --git a/circ/circulation.pl b/circ/circulation.pl index 9b925e29ad..1ab7a29809 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -54,6 +54,7 @@ use Koha::SearchEngine; use Koha::SearchEngine::Search; use Koha::Patron::Modifications; use Koha::Token; +use Koha::Statistics; use List::MoreUtils qw( uniq ); @@ -221,6 +222,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 0000000000..21510aeaa8 --- /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 38a756f817..4e77d9fc9b 100644 --- a/installer/data/mysql/mandatory/sysprefs.sql +++ b/installer/data/mysql/mandatory/sysprefs.sql @@ -357,6 +357,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 ffe8481869..e43802beee 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 e36db3d68b..2bbd8dbd32 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/Stats.t b/t/db_dependent/Stats.t index 117dfbea28..9f4f1b3aa8 100755 --- a/t/db_dependent/Stats.t +++ b/t/db_dependent/Stats.t @@ -4,10 +4,15 @@ use Modern::Perl; use C4::Stats qw( UpdateStats ); use Koha::Database; -use Test::More tests => 19; +use Test::More tests => 31; +use Test::MockModule; +use t::lib::TestBuilder; +use t::lib::Mocks; BEGIN { use_ok('C4::Stats', qw( UpdateStats )); + use_ok('Koha::Statistic'); + use_ok('Koha::Statistics'); } can_ok( 'C4::Stats', @@ -162,4 +167,49 @@ $line = ${ $sth->fetchall_arrayref( {} ) }[0]; is( $line->{location}, undef, "UpdateStats sets location to NULL if undef is passed in." ); -# More tests to write! +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 +$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' ); + + +#End transaction +$dbh->rollback; -- 2.30.2