From b711ea638afc21ed93c6b7350e0d58d11f7e962b Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Fri, 16 Jun 2017 08:27:29 -0400 Subject: [PATCH] Bug 18816: Make CataloguingLog work in production by preventing circulation from spamming the log The system preference CataloguingLog is not recommended for use in production. This is do to the fact that every checkin and checkout generates one or more log entires. This seems to be not only bad behavior, but unnecessary and outside the needs of CataloguingLog as we have CirculationLog. Test Plan: 1) Log into staff client 2) Home -> Koha administration -> Global system preferences -> Logs 3) Set only CataloguingLog to 'Log', everything else to "Don't log" 4) Click 'Save all Logging preferences' 5) In MySQL, use your instance DB, and then type 'delete from action_logs;' 6) Have a person checkout and checkin anything. 7) In MySQL, 'select * from action_logs;' -- there will be data. This is the floodiness that will be removed. 8) Apply this patch 9) Repeat steps 5-7 -- there should be no data. 10) Edit any biblio or item. 11) In MySQL, 'select * from action_logs;' -- there should be data reflecting the changes made. 12) run koha qa test tools NOTE: Improved clarity of test plan -- Mark Tompsett Signed-off-by: Mark Tompsett Signed-off-by: Marcel de Rooy Signed-off-by: Jonathan Druart --- C4/Circulation.pm | 18 +++++++++-------- C4/Items.pm | 17 ++++++++++------ t/db_dependent/Items.t | 46 +++++++++++++++++++++++++++++++++++++++--- 3 files changed, 64 insertions(+), 17 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 3e711cdb01..15c05fc899 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -1397,7 +1397,8 @@ sub AddIssue { datelastborrowed => DateTime->now( time_zone => C4::Context->tz() )->ymd(), }, $item->{'biblionumber'}, - $item->{'itemnumber'} + $item->{'itemnumber'}, + 0 ); ModDateLastSeen( $item->{'itemnumber'} ); @@ -1830,7 +1831,7 @@ sub AddReturn { $item->{location} = $item->{permanent_location}; } - ModItem( $item, $item->{'biblionumber'}, $item->{'itemnumber'} ); + ModItem( $item, $item->{'biblionumber'}, $item->{'itemnumber'}, 0 ); } # full item data, but no borrowernumber or checkout info (no issue) @@ -1854,7 +1855,7 @@ sub AddReturn { foreach my $key ( keys %$rules ) { if ( $item->{notforloan} eq $key ) { $messages->{'NotForLoanStatusUpdated'} = { from => $item->{notforloan}, to => $rules->{$key} }; - ModItem( { notforloan => $rules->{$key} }, undef, $itemnumber ); + ModItem( { notforloan => $rules->{$key} }, undef, $itemnumber, 0 ); last; } } @@ -1920,7 +1921,7 @@ sub AddReturn { } - ModItem({ onloan => undef }, $item->{biblionumber}, $item->{'itemnumber'}); + ModItem( { onloan => undef }, $item->{biblionumber}, $item->{itemnumber}, 0 ); } # the holdingbranch is updated if the document is returned to another location. @@ -2156,7 +2157,7 @@ sub MarkIssueReturned { # And finally delete the issue $issue->delete; - ModItem( { 'onloan' => undef }, undef, $itemnumber ); + ModItem( { 'onloan' => undef }, undef, $itemnumber, 0 ); if ( C4::Context->preference('StoreLastBorrower') ) { my $item = Koha::Items->find( $itemnumber ); @@ -2395,7 +2396,7 @@ sub _FixAccountForLostAndReturned { } ); - ModItem( { paidfor => '' }, undef, $itemnumber ); + ModItem( { paidfor => '' }, undef, $itemnumber, 0 ); return $credit_id; } @@ -2820,7 +2821,7 @@ sub AddRenewal { # Update the renewal count on the item, and tell zebra to reindex $renews = $item->{renewals} + 1; - ModItem({ renewals => $renews, onloan => $datedue->strftime('%Y-%m-%d %H:%M')}, $item->{biblionumber}, $itemnumber); + ModItem( { renewals => $renews, onloan => $datedue->strftime('%Y-%m-%d %H:%M')}, $item->{biblionumber}, $itemnumber, 0 ); # Charge a new rental fee, if applicable? my ( $charge, $type ) = GetIssuingCharges( $itemnumber, $borrowernumber ); @@ -3699,7 +3700,8 @@ sub ProcessOfflineReturn { ModItem( { renewals => 0, onloan => undef }, $issue->{'biblionumber'}, - $itemnumber + $itemnumber, + 0 ); return "Success."; } else { diff --git a/C4/Items.pm b/C4/Items.pm index a3ecd87814..f2c7dc238d 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -517,7 +517,7 @@ sub ModItemFromMarc { =head2 ModItem - ModItem({ column => $newvalue }, $biblionumber, $itemnumber); + ModItem({ column => $newvalue }, $biblionumber, $itemnumber, $log_action ); Change one or more columns in an item record and update the MARC representation of the item. @@ -538,12 +538,16 @@ the derived value of a column such as C, this routine will perform the necessary calculation and set the value. +If log_action is set to false, the action will not be logged. +If log_action is true or undefined, the action will be logged. + =cut sub ModItem { my $item = shift; my $biblionumber = shift; my $itemnumber = shift; + my $log_action = shift // 1; # if $biblionumber is undefined, get it from the current item unless (defined $biblionumber) { @@ -552,8 +556,8 @@ sub ModItem { my $dbh = @_ ? shift : C4::Context->dbh; my $frameworkcode = @_ ? shift : C4::Biblio::GetFrameworkCode( $biblionumber ); - - my $unlinked_item_subfields; + + my $unlinked_item_subfields; if (@_) { $unlinked_item_subfields = shift; $item->{'more_subfields_xml'} = _get_unlinked_subfields_xml($unlinked_item_subfields); @@ -602,7 +606,8 @@ sub ModItem { # item status is possible ModZebra( $biblionumber, "specialUpdate", "biblioserver" ); - logaction("CATALOGUING", "MODIFY", $itemnumber, "item ".Dumper($item)) if C4::Context->preference("CataloguingLog"); + logaction( "CATALOGUING", "MODIFY", $itemnumber, "item " . Dumper($item) ) + if $log_action && C4::Context->preference("CataloguingLog"); } =head2 ModItemTransfer @@ -646,9 +651,9 @@ C<$itemnum> is the item number sub ModDateLastSeen { my ($itemnumber) = @_; - + my $today = output_pref({ dt => dt_from_string, dateformat => 'iso', dateonly => 1 }); - ModItem({ itemlost => 0, datelastseen => $today }, undef, $itemnumber); + ModItem( { itemlost => 0, datelastseen => $today }, undef, $itemnumber, 0 ); } =head2 DelItem diff --git a/t/db_dependent/Items.t b/t/db_dependent/Items.t index f6cc8916a4..da74ca3eef 100755 --- a/t/db_dependent/Items.t +++ b/t/db_dependent/Items.t @@ -25,13 +25,12 @@ use Koha::Database; use Koha::DateUtils qw( dt_from_string ); use Koha::Library; use Koha::DateUtils; +use Koha::MarcSubfieldStructures; +use Koha::Caches; use t::lib::Mocks; use t::lib::TestBuilder; -use Koha::MarcSubfieldStructures; -use Koha::Caches; - use Test::More tests => 13; use Test::Warn; @@ -797,6 +796,47 @@ subtest 'get_hostitemnumbers_of' => sub { is( @itemnumbers, 0, ); }; +subtest 'Test logging for AddItem' => sub { + + plan tests => 3; + + t::lib::Mocks::mock_preference('CataloguingLog', 1); + + $schema->storage->txn_begin; + + my $builder = t::lib::TestBuilder->new; + my $library = $builder->build({ + source => 'Branch', + }); + my $itemtype = $builder->build({ + source => 'Itemtype', + }); + + # Create a biblio instance for testing + t::lib::Mocks::mock_preference('marcflavour', 'MARC21'); + my ($bibnum, $bibitemnum) = get_biblio(); + + # Add an item. + my ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem({ homebranch => $library->{branchcode}, holdingbranch => $library->{branchcode}, location => $location, itype => $itemtype->{itemtype} } , $bibnum); + + # False means no logging + $schema->resultset('ActionLog')->search()->delete(); + ModItem({ location => $location }, $bibnum, $itemnumber, 0); + is( $schema->resultset('ActionLog')->count(), 0, 'False value does not trigger logging' ); + + # True means logging + $schema->resultset('ActionLog')->search()->delete(); + ModItem({ location => $location }, $bibnum, $itemnumber, 1, 'True value does trigger logging'); + is( $schema->resultset('ActionLog')->count(), 1 ); + + # Undefined defaults to true + $schema->resultset('ActionLog')->search()->delete(); + ModItem({ location => $location }, $bibnum, $itemnumber); + is( $schema->resultset('ActionLog')->count(), 1, 'Undefined value defaults to true, triggers logging' ); + + $schema->storage->txn_rollback; +}; + # Helper method to set up a Biblio. sub get_biblio { my ( $frameworkcode ) = @_; -- 2.39.5