From 87012a19fca43e1b97b33a08e3d2e85c4e3870e9 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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
---
 Koha/Illrequest/Logger.pm          | 65 +++++++++++++++-----------------------
 t/db_dependent/Illrequest/Logger.t | 13 ++------
 2 files changed, 27 insertions(+), 51 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);
diff --git a/t/db_dependent/Illrequest/Logger.t b/t/db_dependent/Illrequest/Logger.t
index b9c9ac8071..7beb8d62ab 100644
--- a/t/db_dependent/Illrequest/Logger.t
+++ b/t/db_dependent/Illrequest/Logger.t
@@ -57,7 +57,7 @@ use_ok('Koha::Illrequest::Logger');
 
 subtest 'Basics' => sub {
 
-    plan tests => 9;
+    plan tests => 7;
 
     $schema->storage->txn_begin;
 
@@ -68,8 +68,6 @@ subtest 'Basics' => sub {
     ok( defined($logger), 'new() returned something' );
     ok( $logger->isa('Koha::Illrequest::Logger'),
         'new() returns the correct object' );
-    is_deeply($logger->{data}, {modulename => 'ILL'},
-        'new() setting modulename');
 
     # This is an incomplete data hashref, we use it to
     # test validation of the data before logging
@@ -79,12 +77,6 @@ subtest 'Basics' => sub {
         infos        => 'infos'
     };
 
-    # set_data()
-    #
-    $logger->set_data($log_obj);
-    is_deeply($logger->{data}->{actionname}, 'actionname',
-        'set_data() setter works');
-
     # log_something()
     #
     # Do we only log when the pref is set (currently unset)
@@ -100,8 +92,7 @@ subtest 'Basics' => sub {
 
     # Fix the data hashref, then test that logging occurs
     $log_obj->{objectnumber} = 'objectnumber';
-    $logger->set_data($log_obj);
-    is($logger->log_something(), 1,
+    is($logger->log_something($log_obj), 1,
         'logaction() being called when pref is set and data is complete');
 
     # log_maybe()
-- 
2.11.0