From 5485d3730c34378e23f67e8b60ac7304f1978669 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Fri, 28 Jul 2017 12:52:42 -0400 Subject: [PATCH] Bug 16117 - Feature request: Log borrower cardnumbers and item barcodes which are not valid 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 --- 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 | 67 +++++++++-- 8 files changed, 255 insertions(+), 9 deletions(-) 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 fba723f..deab914 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -53,6 +53,7 @@ use Koha::Libraries; use Koha::Holds; use Koha::RefundLostItemFeeRule; use Koha::RefundLostItemFeeRules; +use Koha::Statistics; use Carp; use List::MoreUtils qw( uniq ); use Scalar::Util qw( looks_like_number ); @@ -681,6 +682,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; @@ -1816,6 +1818,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 0000000..db70b9f --- /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 0000000..b2abfce --- /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 fad8623..244d962 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -56,6 +56,7 @@ use Koha::Patron::Messages; use Koha::SearchEngine; use Koha::SearchEngine::Search; use Koha::Patron::Modifications; +use Koha::Statistics; use Date::Calc qw( Today @@ -254,6 +255,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 0000000..afb2a07 --- /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 0bd8cac..09c0a46 100644 --- a/installer/data/mysql/sysprefs.sql +++ b/installer/data/mysql/sysprefs.sql @@ -246,6 +246,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 90b2844..b132082 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 @@ -8,6 +8,18 @@ Circulation: no: Deactivate - the navigation sidebar on all Circulation pages. - + - 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: yes: Try diff --git a/t/db_dependent/Stats.t b/t/db_dependent/Stats.t index 5ad9608..7facce1 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 => 17; +use Test::More tests => 29; +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', @@ -113,14 +118,14 @@ UpdateStats ($params); my $sth = $dbh->prepare("SELECT * FROM statistics"); $sth->execute(); my $line = ${ $sth->fetchall_arrayref( {} ) }[0]; -is ($params-> {branch}, $line->{branch}, "UpdateStats save branch param in branch field of statistics table"); -is ($params-> {type}, $line->{type}, "UpdateStats save type param in type field of statistics table"); -is ($params-> {borrowernumber}, $line->{borrowernumber}, "UpdateStats save borrowernumber param in borrowernumber field of statistics table"); -cmp_ok($params-> {amount},'==', $line->{value}, "UpdateStats save amount param in value field of statistics table"); -is ($params-> {other}, $line->{other}, "UpdateStats save other param in other field of statistics table"); -is ($params-> {itemtype}, $line->{itemtype}, "UpdateStats save itemtype param in itemtype field of statistics table"); -is ($params-> {accountno}, $line->{proccode}, "UpdateStats save accountno param in proccode field of statistics table"); -is ($params-> {ccode}, $line->{ccode}, "UpdateStats save ccode param in ccode field of statistics table"); +is ($params->{branch}, $line->{branch}, "UpdateStats save branch param in branch field of statistics table"); +is ($params->{type}, $line->{type}, "UpdateStats save type param in type field of statistics table"); +is ($params->{borrowernumber}, $line->{borrowernumber}, "UpdateStats save borrowernumber param in borrowernumber field of statistics table"); +cmp_ok($params->{amount},'==', $line->{value}, "UpdateStats save amount param in value field of statistics table"); +is ($params->{other}, $line->{other}, "UpdateStats save other param in other field of statistics table"); +is ($params->{itemtype}, $line->{itemtype}, "UpdateStats save itemtype param in itemtype field of statistics table"); +is ($params->{accountno}, $line->{proccode}, "UpdateStats save accountno param in proccode field of statistics table"); +is ($params->{ccode}, $line->{ccode}, "UpdateStats save ccode param in ccode field of statistics table"); # # Test TotalPaid @@ -129,5 +134,49 @@ is ($params-> {ccode}, $line->{ccode}, "UpdateStats save ccode 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.10.2