refactor C4::Log::logaction

This fix should resolve in whole or in part several bugs
characterized by the error message 'Can't use string ("0")
as a HASH ref while "strict refs" in use', including
bugs 1101, 1899, and 1910.

There are some possibilities for future work:

[1] Dealing with an operator override, e.g., where
    a circ operator needs to get a supervisor
    to enter a login and password and escalate
    the original operator's privileges for a
    transaction, e.g., to forgive a fine.  This
    is an enhancement, of course.
[2] Creating a dummy operator to represent
    batch job runs; or alternatively, give
    each batch job an option to log its work
    under a specified user ID.

Signed-off-by: Joshua Ferraro <jmf@liblime.com>

diff --git a/C4/Biblio.pm b/C4/Biblio.pm
index 35de382..1f6be62 100755 (executable)
--- a/C4/Biblio.pm
+++ b/C4/Biblio.pm
@@ -239,8 +239,7 @@ sub AddBiblio {
     # now add the record
     $biblionumber = ModBiblioMarc( $record, $biblionumber, $frameworkcode ) unless $defer_marc_save;
       
-    &logaction(C4::Context->userenv->{'number'},"CATALOGUING","ADD",$biblionumber,"biblio") 
-        if C4::Context->preference("CataloguingLog");
+    logaction("CATALOGUING", "ADD", $biblionumber, "biblio") if C4::Context->preference("CataloguingLog");
 
     return ( $biblionumber, $biblioitemnumber );
 }
@@ -256,7 +255,7 @@ sub ModBiblio {
     my ( $record, $biblionumber, $frameworkcode ) = @_;
     if (C4::Context->preference("CataloguingLog")) {
         my $newrecord = GetMarcBiblio($biblionumber);
-        &logaction(C4::Context->userenv->{'number'},"CATALOGUING","MODIFY",$biblionumber,"BEFORE=>".$newrecord->as_formatted);
+        logaction("CATALOGUING", "MODIFY", $biblionumber, "BEFORE=>".$newrecord->as_formatted);
     }
     
     my $dbh = C4::Context->dbh;
@@ -388,8 +387,8 @@ sub DelBiblio {
     # from being generated by _koha_delete_biblioitems
     $error = _koha_delete_biblio( $dbh, $biblionumber );
 
-    &logaction(C4::Context->userenv->{'number'},"CATALOGUING","DELETE",$biblionumber,"") 
-        if C4::Context->preference("CataloguingLog");
+    logaction("CATALOGUING", "DELETE", $biblionumber, "") if C4::Context->preference("CataloguingLog");
+
     return;
 }
 

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index 1cd8202..0add08e 100644 (file)
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -1026,7 +1026,7 @@ sub AddIssue {
         );
     }
     
-    &logaction(C4::Context->userenv->{'number'},"CIRCULATION","ISSUE",$borrower->{'borrowernumber'},$biblio->{'biblionumber'}) 
+    logaction("CIRCULATION", "ISSUE", $borrower->{'borrowernumber'}, $biblio->{'biblionumber'}) 
         if C4::Context->preference("IssueLog");
     return ($datedue);
   }
@@ -1275,7 +1275,7 @@ sub AddReturn {
             $borrower->{'borrowernumber'}
         );
         
-        &logaction(C4::Context->userenv->{'number'},"CIRCULATION","RETURN",$iteminformation->{borrowernumber},$iteminformation->{'biblionumber'}) 
+        logaction("CIRCULATION", "RETURN", $iteminformation->{borrowernumber}, $iteminformation->{'biblionumber'}) 
             if C4::Context->preference("ReturnLog");
         
         #adding message if holdingbranch is non equal a userenv branch to return the document to homebranch

diff --git a/C4/Items.pm b/C4/Items.pm
index d0c0fb3..a315e94 100644 (file)
--- a/C4/Items.pm
+++ b/C4/Items.pm
@@ -231,8 +231,7 @@ sub AddItem {
     my $new_item_marc = _marc_from_item_hash($item, $frameworkcode, $unlinked_item_subfields);
     _add_item_field_to_biblio($new_item_marc, $item->{'biblionumber'}, $frameworkcode );
    
-    logaction(C4::Context->userenv->{'number'},"CATALOGUING","ADD",$itemnumber,"item") 
-        if C4::Context->preference("CataloguingLog");
+    logaction("CATALOGUING", "ADD", $itemnumber, "item") if C4::Context->preference("CataloguingLog");
     
     return ($item->{biblionumber}, $item->{biblioitemnumber}, $itemnumber);
 }
@@ -327,8 +326,7 @@ sub AddItemBatchFromMarc {
         push @itemnumbers, $itemnumber; # FIXME not checking error
         $item->{'itemnumber'} = $itemnumber;
 
-        &logaction(C4::Context->userenv->{'number'},"CATALOGUING","ADD",$itemnumber,"item")
-        if C4::Context->preference("CataloguingLog"); 
+        logaction("CATALOGUING", "ADD", $itemnumber, "item") if C4::Context->preference("CataloguingLog"); 
 
         my $new_item_marc = _marc_from_item_hash($item, $frameworkcode, $unlinked_item_subfields);
         $item_field->replace_with($new_item_marc->field($itemtag));
@@ -444,10 +442,8 @@ sub ModItem {
         or die "FAILED _marc_from_item_hash($whole_item, $frameworkcode)";
     
     _replace_item_field_in_biblio($new_item_marc, $biblionumber, $itemnumber, $frameworkcode);
-       (C4::Context->userenv eq '0') and die "userenv is '0', not hashref";         # logaction line would crash anyway
        ($new_item_marc       eq '0') and die "$new_item_marc is '0', not hashref";  # logaction line would crash anyway
-    logaction(C4::Context->userenv->{'number'},"CATALOGUING","MODIFY",$itemnumber,$new_item_marc->as_formatted)
-        if C4::Context->preference("CataloguingLog");
+    logaction("CATALOGUING", "MODIFY", $itemnumber, $new_item_marc->as_formatted) if C4::Context->preference("CataloguingLog");
 }
 
 =head2 ModItemTransfer
@@ -537,8 +533,7 @@ sub DelItem {
         }
     }
     &ModBiblioMarc( $record, $biblionumber, $frameworkcode );
-    &logaction(C4::Context->userenv->{'number'},"CATALOGUING","DELETE",$itemnumber,"item") 
-        if C4::Context->preference("CataloguingLog");
+    logaction("CATALOGUING", "DELETE", $itemnumber, "item") if C4::Context->preference("CataloguingLog");
 }
 
 =head2 CheckItemPreSave

diff --git a/C4/Letters.pm b/C4/Letters.pm
index dffe2c2..31033a8 100644 (file)
--- a/C4/Letters.pm
+++ b/C4/Letters.pm
@@ -354,7 +354,6 @@ sub SendAlerts {
         }
         if ( C4::Context->preference("LetterLog") ) {
             logaction(
-                $userenv->{number},
                 "ACQUISITION",
                 "Send Acquisition claim letter",
                 "",
@@ -419,8 +418,7 @@ sub SendAlerts {
                 Message => "" . $innerletter->{content},
             );
             sendmail(%mail);
-            &logaction(
-                C4::Context->userenv->{'number'},
+            logaction(
                 "ACQUISITION",
                 "CLAIM ISSUE",
                 undef,

diff --git a/C4/Log.pm b/C4/Log.pm
index c7088c7..b4732ac 100644 (file)
--- a/C4/Log.pm
+++ b/C4/Log.pm
@@ -52,16 +52,25 @@ The functions in this module perform various functions in order to log all the o
 
 =item logaction
 
-  &logaction($usernumber, $modulename, $actionname, $objectnumber, $infos);
+  &logaction($modulename, $actionname, $objectnumber, $infos);
 
-Adds a record into action_logs table to report the different changes upon the database
+Adds a record into action_logs table to report the different changes upon the database.
+Each log entry includes the number of the user currently logged in.  For batch
+jobs, which operate without authenticating a user and setting up a session, the user
+number is set to 0, which is the same as the superlibrarian's number.
 
 =cut
 
 #'
 sub logaction {
-  my ($usernumber,$modulename, $actionname, $objectnumber, $infos)=@_;
-    $usernumber='' unless $usernumber;
+    my ($modulename, $actionname, $objectnumber, $infos)=@_;
+
+    # Get ID of logged in user.  if called from a batch job,
+    # no user session exists and C4::Context->userenv() returns
+    # the scalar '0'.
+    my $userenv = C4::Context->userenv();
+    my $usernumber = (ref($userenv) eq 'HASH') ? $userenv->{'number'} : 0;
+
     my $dbh = C4::Context->dbh;
     my $sth=$dbh->prepare("Insert into action_logs (timestamp,user,module,action,object,info) values (now(),?,?,?,?,?)");
     $sth->execute($usernumber,$modulename,$actionname,$objectnumber,$infos);

diff --git a/C4/Members.pm b/C4/Members.pm
index 8d38619..f62baf7 100644 (file)
--- a/C4/Members.pm
+++ b/C4/Members.pm
@@ -633,7 +633,7 @@ sub ModMember {
         # is adult check guarantees;
         UpdateGuarantees(%data);
     }
-    &logaction(C4::Context->userenv->{'number'},"MEMBERS","MODIFY",$data{'borrowernumber'},"$query (executed w/ arg: $data{'borrowernumber'})") 
+    logaction("MEMBERS", "MODIFY", $data{'borrowernumber'}, "$query (executed w/ arg: $data{'borrowernumber'})") 
         if C4::Context->preference("BorrowersLog");
 }
 
@@ -729,8 +729,7 @@ sub AddMember {
     $data{'borrowernumber'} = $dbh->{'mysql_insertid'};     # unneeded w/ autoincrement ?  
     # mysql_insertid is probably bad.  not necessarily accurate and mysql-specific at best.
     
-    &logaction(C4::Context->userenv->{'number'},"MEMBERS","CREATE",$data{'borrowernumber'},"") 
-        if C4::Context->preference("BorrowersLog");
+    logaction("MEMBERS", "CREATE", $data{'borrowernumber'}, "") if C4::Context->preference("BorrowersLog");
     
     # check for enrollment fee & add it if needed
     $sth = $dbh->prepare("SELECT enrolmentfee FROM categories WHERE categorycode=?");
@@ -783,8 +782,7 @@ sub changepassword {
         return 1;
     }
     
-    &logaction(C4::Context->userenv->{'number'},"MEMBERS","CHANGE PASS",$member,"") 
-        if C4::Context->preference("BorrowersLog");
+    logaction("MEMBERS", "CHANGE PASS", $member, "") if C4::Context->preference("BorrowersLog");
 }
 
 
@@ -1604,8 +1602,7 @@ sub DelMember {
     $sth = $dbh->prepare($query);
     $sth->execute($borrowernumber);
     $sth->finish;
-    &logaction(C4::Context->userenv->{'number'},"MEMBERS","DELETE",$borrowernumber,"") 
-        if C4::Context->preference("BorrowersLog");
+    logaction("MEMBERS", "DELETE", $borrowernumber, "") if C4::Context->preference("BorrowersLog");
     return $sth->rows;
 }
 

diff --git a/C4/Overdues.pm b/C4/Overdues.pm
index 9db5c6d..0457090 100644 (file)
--- a/C4/Overdues.pm
+++ b/C4/Overdues.pm
@@ -507,7 +507,6 @@ sub UpdateFine {
     }
     # logging action
     &logaction(
-        C4::Context->userenv->{'number'},
         "FINES",
         $type,
         $borrowernumber,

diff --git a/C4/Serials.pm b/C4/Serials.pm
index dce2c61..d86f90b 100644 (file)
--- a/C4/Serials.pm
+++ b/C4/Serials.pm
@@ -1268,8 +1268,7 @@ sub ModSubscription {
     my $rows=$sth->rows;
     $sth->finish;
     
-    &logaction(C4::Context->userenv->{'number'},"SERIAL","MODIFY",$subscriptionid,"") 
-        if C4::Context->preference("SubscriptionLog");
+    logaction("SERIAL", "MODIFY", $subscriptionid, "") if C4::Context->preference("SubscriptionLog");
     return $rows;
 }
 
@@ -1382,8 +1381,7 @@ sub NewSubscription {
         format_date_in_iso($startdate)
     );
     
-    &logaction(C4::Context->userenv->{'number'},"SERIAL","ADD",$subscriptionid,"") 
-        if C4::Context->preference("SubscriptionLog");
+    logaction("SERIAL", "ADD", $subscriptionid, "") if C4::Context->preference("SubscriptionLog");
     
 #set serial flag on biblio if not already set.
     my ($null, ($bib)) = GetBiblio($biblionumber);
@@ -1444,8 +1442,7 @@ sub ReNewSubscription {
     $sth->execute( format_date_in_iso($startdate),
         $numberlength, $weeklength, $monthlength, $subscriptionid );
         
-    &logaction(C4::Context->userenv->{'number'},"SERIAL","RENEW",$subscriptionid,"") 
-        if C4::Context->preference("SubscriptionLog");
+    logaction("SERIAL", "RENEW", $subscriptionid, "") if C4::Context->preference("SubscriptionLog");
 }
 
 =head2 NewIssue
@@ -1797,8 +1794,7 @@ sub DelSubscription {
         "DELETE FROM subscriptionhistory WHERE subscriptionid=$subscriptionid");
     $dbh->do("DELETE FROM serial WHERE subscriptionid=$subscriptionid");
     
-    &logaction(C4::Context->userenv->{'number'},"SERIAL","DELETE",$subscriptionid,"") 
-        if C4::Context->preference("SubscriptionLog");
+    logaction("SERIAL", "DELETE", $subscriptionid, "") if C4::Context->preference("SubscriptionLog");
 }
 
 =head2 DelIssue