From 7e1be698fb57830a1a31081eb3d885e94b621e47 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 | 44 ++++++++ Koha/Statistics.pm | 123 +++++++++++++++++++++ circ/circulation.pl | 2 + installer/data/mysql/atomicupdate/bug_16117.perl | 11 ++ installer/data/mysql/sysprefs.sql | 2 + .../en/modules/admin/preferences/circulation.pref | 12 ++ t/db_dependent/Stats.t | 51 ++++++++- 8 files changed, 247 insertions(+), 1 deletion(-) create mode 100644 Koha/Statistic.pm create mode 100644 Koha/Statistics.pm create mode 100644 installer/data/mysql/atomicupdate/bug_16117.perl diff --git a/C4/Circulation.pm b/C4/Circulation.pm index d583e5372c..3e1d5a5bd2 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -56,6 +56,7 @@ use Koha::RefundLostItemFeeRule; use Koha::RefundLostItemFeeRules; use Koha::Account::Lines; use Koha::Account::Offsets; +use Koha::Statistics; use Carp; use List::MoreUtils qw( uniq ); use Scalar::Util qw( looks_like_number ); @@ -676,6 +677,7 @@ sub CanBookBeIssued { # MANDATORY CHECKS - unless item exists, nothing else matters unless ( $item->{barcode} ) { + Koha::Statistics->invalid_item( { item => $barcode } ); $issuingimpossible{UNKNOWN_BARCODE} = 1; } return ( \%issuingimpossible, \%needsconfirmation ) if %issuingimpossible; @@ -1801,6 +1803,7 @@ sub AddReturn { # get information on item my $item = GetItem( undef, $barcode ); unless ($item) { + Koha::Statistics->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 new file mode 100644 index 0000000000..db70b9fbb4 --- /dev/null +++ b/Koha/Statistic.pm @@ -0,0 +1,44 @@ +package Koha::Statistic; + +# 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, write to the Free Software Foundation, Inc., +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +use Modern::Perl; + +use Carp; + +use Koha::Database; + +use base qw(Koha::Object); + +=head1 NAME + +Koha::Statistic - Koha Statistic Object class + +=head1 API + +=head2 Class Methods + +=cut + +=head3 type + +=cut + +sub _type { + return 'Statistic'; +} + +1; diff --git a/Koha/Statistics.pm b/Koha/Statistics.pm new file mode 100644 index 0000000000..b2abfce964 --- /dev/null +++ b/Koha/Statistics.pm @@ -0,0 +1,123 @@ +package Koha::Statistics; + +# 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, write to the Free Software Foundation, Inc., +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +use Modern::Perl; + +use Carp; + +use C4::Context; +use Koha::Database; +use Koha::DateUtils; + +use Koha::Statistic; + +use base qw(Koha::Objects); + +=head1 NAME + +Koha::Statistics - Koha Statistic Object set class + +=head1 API + +=head2 Class Methods + +=head3 invalid_patron + +Koha::Statistics->invalid_patron( { patron => $cardnumber } ); + +=cut + +sub invalid_patron { + return unless C4::Context->preference('LogInvalidPatrons'); + + my ( $class, $params ) = @_; + + my $patron = $params->{patron}; + croak('Invalid patron passed in!') unless $patron; + + return $class->_invalid_value( + { + type => 'patron', + value => $patron + } + ); +} + +=head3 invalid_item + +Koha::Statistics->invalid_item( { item => $barcode } ); + +=cut + +sub invalid_item { + return unless C4::Context->preference('LogInvalidItems'); + + my ( $class, $params ) = @_; + + my $item = $params->{item}; + croak('Invalid item passed in!') unless $item; + + return $class->_invalid_value( + { + type => 'item', + value => $item + } + ); +} + +=head3 invalid_value + +Koha::Statistics->invalid_value( { type => 'patron', value => $patron } ); + +=cut + +sub _invalid_value { + my ( $class, $params ) = @_; + + my $type = $params->{type}; + my $value = $params->{value}; + croak('Invalid type passed in!') unless $type eq 'patron' || $type eq 'item'; + croak('No value passed in!') unless $value; + + my $branch = C4::Context->userenv ? C4::Context->userenv->{branch} : undef; + my $dt = dt_from_string(); + my $associatedborrower = C4::Context->userenv->{'number'}; + + return Koha::Statistic->new( + { + type => "invalid_$type", + other => $value, + datetime => $dt, + branch => $branch, + associatedborrower => $associatedborrower + } + )->store(); +} + +=head3 type + +=cut + +sub _type { + return 'Statistic'; +} + +sub object_class { + return 'Koha::Statistic'; +} + +1; diff --git a/circ/circulation.pl b/circ/circulation.pl index 93ed09c169..63384ffbfa 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -55,6 +55,7 @@ use Koha::Patron::Messages; use Koha::SearchEngine; use Koha::SearchEngine::Search; use Koha::Patron::Modifications; +use Koha::Statistics; use Date::Calc qw( Today @@ -265,6 +266,7 @@ if ($findborrower) { } elsif ( @$borrowers ) { $template->param( borrowers => $borrowers ); } else { + Koha::Statistics->invalid_patron( { patron => $findborrower } ); $query->param( 'findborrower', '' ); $message = "'$findborrower'"; } diff --git a/installer/data/mysql/atomicupdate/bug_16117.perl b/installer/data/mysql/atomicupdate/bug_16117.perl new file mode 100644 index 0000000000..afb2a0785a --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_16117.perl @@ -0,0 +1,11 @@ +$DBversion = 'XXX'; # will be replaced by the RM +if( CheckVersion( $DBversion ) ) { + $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'); + }); + + SetVersion( $DBversion ); + print "Upgrade to $DBversion done (Bug 16117 - Feature request: Log borrower cardnumbers and item barcodes which are not valid)\n"; +} diff --git a/installer/data/mysql/sysprefs.sql b/installer/data/mysql/sysprefs.sql index 76bb2cc03a..54da831c65 100644 --- a/installer/data/mysql/sysprefs.sql +++ b/installer/data/mysql/sysprefs.sql @@ -255,6 +255,8 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('LocalHoldsPriority', '0', NULL, 'Enables the LocalHoldsPriority feature', 'YesNo'), ('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'), +('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'), ('ManInvInNoissuesCharge','1',NULL,'MANUAL_INV charges block checkouts (added to noissuescharge).','YesNo'), ('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 178a048e91..8478e43181 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 @@ -14,6 +14,18 @@ Circulation: no: "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: CircAutocompl choices: diff --git a/t/db_dependent/Stats.t b/t/db_dependent/Stats.t index 7675a1628a..cc8e34a2a3 100644 --- a/t/db_dependent/Stats.t +++ b/t/db_dependent/Stats.t @@ -3,10 +3,15 @@ use Modern::Perl; use C4::Stats; -use Test::More tests => 20; +use Test::More tests => 32; +use Test::MockModule; +use t::lib::TestBuilder; +use t::lib::Mocks; BEGIN { use_ok('C4::Stats'); + use_ok('Koha::Statistic'); + use_ok('Koha::Statistics'); } can_ok( 'C4::Stats', @@ -171,5 +176,49 @@ is( $line->{location}, undef, is (TotalPaid (),undef,"TotalPaid returns undef if no params are given"); # More tests to write! +my $builder = t::lib::TestBuilder->new; +my $library = $builder->build( { source => 'Branch' } ); +my $branchcode = $library->{branchcode}; +my $context = new Test::MockModule('C4::Context'); +$context->mock( + 'userenv', + sub { + return { + flags => 1, + id => 'my_userid', + branch => $branchcode, + number => '-1', + }; + } +); +# Test Koha::Statistic->invalid_patron +$dbh->do(q{DELETE FROM statistics}); +t::lib::Mocks::mock_preference( "LogInvalidPatrons", 0 ); +Koha::Statistics->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->invalid_patron( { patron => 'InvalidCardnumber' } ); +my $stat = Koha::Statistics->search()->next(); +is( $stat->type, 'invalid_patron', 'Type set to invalid_patron' ); +is( $stat->associatedborrower, '-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->invalid_item +$dbh->do(q{DELETE FROM statistics}); +t::lib::Mocks::mock_preference( "LogInvalidItems", 0 ); +Koha::Statistics->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->invalid_item( { item => 'InvalidBarcode' } ); +$stat = Koha::Statistics->search()->next(); +is( $stat->type, 'invalid_item', 'Type set to invalid_item' ); +is( $stat->associatedborrower, '-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.14.3 (Apple Git-98)