From bf5b594d8b3d162f673ea6b7bc0db570197290fd Mon Sep 17 00:00:00 2001 From: Andrew Isherwood 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 +++++++++++++++----------------------- 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