From c76c28a881430a34d8b9696f74dbe18dcbac20bb Mon Sep 17 00:00:00 2001
From: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Date: Wed, 27 Feb 2019 10:57:20 +0000
Subject: [PATCH] Bug 20750: (follow-up) Don't use object variable

We were unecessarily setting a property on the Logger object containing
the data to be logged, prior to calling log_something. This was
overcomplicated things and could be problematic if an unexpected call to
log_something was made. Now we just explicitly pass the data to
log_something.

As per comment #99 here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c99
---
 Koha/Illrequest/Logger.pm | 65 ++++++++++++++++++-----------------------------
 1 file changed, 25 insertions(+), 40 deletions(-)

diff --git a/Koha/Illrequest/Logger.pm b/Koha/Illrequest/Logger.pm
index da4f65947b..566e8f6fca 100644
--- a/Koha/Illrequest/Logger.pm
+++ b/Koha/Illrequest/Logger.pm
@@ -49,8 +49,8 @@ relating to Illrequest to the action log.
 
     my $config = Koha::Illrequest::Logger->new();
 
-Create a new Koha::Illrequest::Logger object, with skeleton logging data
-We also set up data of what can be logged, how to do it and how to display
+Create a new Koha::Illrequest::Logger object.
+We also set up what can be logged, how to do it and how to display
 log entries we get back out
 
 =cut
@@ -59,10 +59,6 @@ sub new {
     my ( $class ) = @_;
     my $self  = {};
 
-    $self->{data} = {
-        modulename => 'ILL'
-    };
-
     $self->{loggers} = {
         status => sub {
             $self->log_status_change(@_);
@@ -105,7 +101,7 @@ sub log_maybe {
 
 =head3 log_status_change
 
-    Koha::IllRequest::Logger->log_status_change();
+    Koha::IllRequest::Logger->log_status_change($req, $new_status);
 
 Log a request's status change
 
@@ -114,7 +110,8 @@ Log a request's status change
 sub log_status_change {
     my ( $self, $req, $new_status ) = @_;
 
-    $self->set_data({
+    $self->log_something({
+        modulename   => 'ILL',
         actionname   => 'STATUS_CHANGE',
         objectnumber => $req->id,
         infos        => to_json({
@@ -123,56 +120,44 @@ sub log_status_change {
             status_after  => $new_status
         })
     });
-
-    $self->log_something();
 }
 
 =head3 log_something
 
-    Koha::IllRequest::Logger->log_something();
+    Koha::IllRequest::Logger->log_something({
+        modulename   => 'ILL',
+        actionname   => 'STATUS_CHANGE',
+        objectnumber => $req->id,
+        infos        => to_json({
+            log_origin    => 'core',
+            status_before => $req->{previous_status},
+            status_after  => $new_status
+        })
+    });
 
-If we have the required data set, log an action
+If we have the required data passed, log an action
 
 =cut
 
 sub log_something {
-    my ( $self ) = @_;
+    my ( $self, $to_log ) = @_;
 
     if (
-        defined $self->{data}->{modulename} &&
-        defined $self->{data}->{actionname} &&
-        defined $self->{data}->{objectnumber} &&
-        defined $self->{data}->{infos} &&
+        defined $to_log->{modulename} &&
+        defined $to_log->{actionname} &&
+        defined $to_log->{objectnumber} &&
+        defined $to_log->{infos} &&
         C4::Context->preference("IllLog")
     ) {
         logaction(
-            $self->{data}->{modulename},
-            $self->{data}->{actionname},
-            $self->{data}->{objectnumber},
-            $self->{data}->{infos}
+            $to_log->{modulename},
+            $to_log->{actionname},
+            $to_log->{objectnumber},
+            $to_log->{infos}
         );
     }
 }
 
-=head3 set_data
-
-    Koha::IllRequest::Logger->set_data({
-        key  => 'value',
-        key2 => 'value2'
-    });
-
-Set arbitrary data propert(ies) on the logger object
-
-=cut
-
-sub set_data {
-    my ( $self, $data ) = @_;
-
-    foreach my $key (keys %{ $data }) {
-        $self->{data}->{$key} = $data->{$key};
-    }
-}
-
 =head3 get_log_template
 
     $template_path = get_log_template($origin, $action);
-- 
2.11.0