From f822fd7db8e98f7aaa0639eaaf90335ccbc3e1e0 Mon Sep 17 00:00:00 2001
From: Kyle M Hall <kyle@bywatersolutions.com>
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) Enable CataloguingLog
2) Note the checkins and checkouts cause log entries
3) Apply this patch
4) Note that checkins and checkouts no longer cause log entries,
   but editing the item will continue to cause log entries.
---
 C4/Circulation.pm      | 18 ++++++++++--------
 C4/Items.pm            | 17 +++++++++++------
 t/db_dependent/Items.t | 43 ++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 63 insertions(+), 15 deletions(-)

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index 9773f69..b796933 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -1416,7 +1416,8 @@ sub AddIssue {
                     datelastborrowed => DateTime->now( time_zone => C4::Context->tz() )->ymd(),
                 },
                 $item->{'biblionumber'},
-                $item->{'itemnumber'}
+                $item->{'itemnumber'},
+                0
             );
             ModDateLastSeen( $item->{'itemnumber'} );
 
@@ -1854,7 +1855,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)
@@ -1878,7 +1879,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;
                 }
             }
@@ -1950,7 +1951,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.
@@ -2186,7 +2187,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 );
@@ -2425,7 +2426,7 @@ sub _FixAccountForLostAndReturned {
         }
     );
 
-    ModItem( { paidfor => '' }, undef, $itemnumber );
+    ModItem( { paidfor => '' }, undef, $itemnumber, 0 );
 
     return $credit_id;
 }
@@ -2842,7 +2843,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 );
@@ -3718,7 +3719,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 b64aa60..16ba246 100644
--- a/C4/Items.pm
+++ b/C4/Items.pm
@@ -518,7 +518,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.
@@ -539,12 +539,16 @@ the derived value of a column such as C<items.cn_sort>,
 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) {
@@ -553,8 +557,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);
@@ -603,7 +607,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
@@ -645,9 +650,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 f257f78..32e8661 100755
--- a/t/db_dependent/Items.t
+++ b/t/db_dependent/Items.t
@@ -26,7 +26,7 @@ use Koha::Library;
 use t::lib::Mocks;
 use t::lib::TestBuilder;
 
-use Test::More tests => 12;
+use Test::More tests => 13;
 
 use Test::Warn;
 
@@ -797,6 +797,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.10.2