Attachment 85757 Details for Bug 20750 – Bug 20750: (follow-up) Don't use object variable

[patch] Bug 20750: (follow-up) Don't use object variable

Bug-20750-follow-up-Dont-use-object-variable.patch (text/plain), 5.45 KB, created by Andrew Isherwood on 2019-02-27 11:19:04 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Andrew Isherwood

Created: 2019-02-27 11:19:04 UTC

Size: 5.45 KB

patch

obsolete

>From bf5b594d8b3d162f673ea6b7bc0db570197290fd 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 +++++++++++++++-----------------------
> 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

Actions: View | Diff | Splinter Review

Attachments on bug 20750: 75304 | 75326 | 75329 | 75330 | 77757 | 77816 | 77817 | 77818 | 77931 | 79822 | 79823 | 79824 | 80860 | 82082 | 82083 | 82085 | 82096 | 82097 | 82098 | 82099 | 82500 | 82501 | 82502 | 84193 | 84194 | 84195 | 84203 | 84266 | 84497 | 84498 | 84821 | 84823 | 84824 | 84825 | 84826 | 84827 | 84973 | 84974 | 84975 | 84976 | 84977 | 84978 | 84979 | 85097 | 85098 | 85099 | 85101 | 85113 | 85238 | 85266 | 85697 | 85698 | 85699 | 85700 | 85701 | 85702 | 85703 | 85704 | 85705 | 85706 | 85707 | 85708 | 85709 | 85710 | 85756 | 85757 | 85765 | 85826 | 85827 | 85829 | 85830 | 85831 | 85832 | 85833 | 85834 | 85835 | 85836 | 85837 | 85838 | 85839 | 85840 | 85841 | 85842 | 85843 | 85844 | 85845 | 85846 | 85847 | 85848 | 86434 | 86435 | 86436 | 86534 | 86535 | 86536