Bug 20750: Allow logging of arbitrary actions
authorAndrew Isherwood <andrew.isherwood@ptfs-europe.com>
Tue, 2 Oct 2018 13:55:58 +0000 (14:55 +0100)
committerNick Clemens <nick@bywatersolutions.com>
Fri, 15 Mar 2019 19:07:08 +0000 (19:07 +0000)
This patch allows logging of arbitrary actions on request objects. A
chronological summary of these actions can then be displayed when
viewing a request.

This patch also adds logging of request status changes. Additional work
has been done on the BLDSS backend to log requests to the BLDSS request
status check API.

To test:
- Apply patch
- Ensure the Illlog logging preference is turned on
- Create an ILL request and perform actions on it that change it's
status.
- Navigate to the "Manage ILL request" screen
- Click the "Display request log" button
- Observe that a modal opens displaying a summary of the status changes.

Signed-off-by: Niamh Walker <Niamh.Walker-Headon@it-tallaght.ie>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Since this bug now is dependent on Bug 20581, it should be aware of the
custom statuses provided in 20581. This patch enables logging of request
statuses being changed to custom ones. As such the test plan is
modified:

To test:
- Apply patch
- Ensure the Illlog logging preference is turned on
- Ensure you have some custom request statuses defined in the
Authorised Values ILLSTATUS category
- Create an ILL request and perform actions on it that change it's
status.
- Edit the request and change status to a custom one
- Navigate to the "Manage ILL request" screen
- Click the "Display request log" button
- Observe that a modal opens displaying a summary of the status changes.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Koha/Illrequest.pm
Koha/Illrequest/Logger.pm [new file with mode: 0644]
installer/data/mysql/atomicupdate/bug_20750-add_illlog_preference.perl [new file with mode: 0644]
installer/data/mysql/sysprefs.sql
koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/logs.pref
koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt
koha-tmpl/intranet-tmpl/prog/en/modules/ill/log/status_change.tt [new file with mode: 0644]
koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt
t/db_dependent/Illrequests.t

index 94e24de..13c0b11 100644 (file)
@@ -32,6 +32,7 @@ use Koha::Exceptions::Ill;
 use Koha::Illcomments;
 use Koha::Illrequestattributes;
 use Koha::AuthorisedValue;
+use Koha::Illrequest::Logger;
 use Koha::Patron;
 use Koha::AuthorisedValues;
 
@@ -157,6 +158,16 @@ sub illcomments {
     );
 }
 
+=head3 logs
+
+=cut
+
+sub logs {
+    my ( $self ) = @_;
+    my $logger = Koha::Illrequest::Logger->new;
+    return $logger->get_request_logs($self);
+}
+
 =head3 patron
 
 =cut
@@ -169,15 +180,26 @@ sub patron {
 }
 
 =head3 status_alias
+
+    $Illrequest->status_alias(143);
+
 Overloaded getter/setter for status_alias,
 that only returns authorised values from the
-correct category
+correct category and records the fact that the status has changed
 
 =cut
 
 sub status_alias {
-    my ($self, $newval) = @_;
-    if ($newval) {
+    my ($self, $new_status_alias) = @_;
+
+    my $current_status_alias = $self->SUPER::status_alias;
+
+    if ($new_status_alias) {
+        # Keep a record of the previous status before we change it,
+        # we might need it
+        $self->{previous_status} = $current_status_alias ?
+            $current_status_alias :
+            scalar $self->status;
         # This is hackery to enable us to undefine
         # status_alias, since we need to have an overloaded
         # status_alias method to get us around the problem described
@@ -185,13 +207,19 @@ sub status_alias {
         # https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c156
         # We need a way of accepting implied undef, so we can nullify
         # the status_alias column, when called from $self->status
-        my $val = $newval eq "-1" ? undef : $newval;
-        my $newval = $self->SUPER::status_alias($val);
-        if ($newval) {
-            return $newval;
+        my $val = $new_status_alias eq "-1" ? undef : $new_status_alias;
+        my $ret = $self->SUPER::status_alias($val);
+        my $val_to_log = $val ? $new_status_alias : scalar $self->status;
+        if ($ret) {
+            my $logger = Koha::Illrequest::Logger->new;
+            $logger->log_status_change({
+                request => $self,
+                value   => $val_to_log
+            });
         } else {
-            return;
+            delete $self->{previous_status};
         }
+        return $ret;
     }
     # We can't know which result is the right one if there are multiple
     # ILLSTATUS authorised values with the same authorised_value column value
@@ -210,25 +238,47 @@ sub status_alias {
 
 =head3 status
 
+    $Illrequest->status('CANREQ');
+
 Overloaded getter/setter for request status,
-also nullifies status_alias
+also nullifies status_alias and records the fact that the status has changed
 
 =cut
 
 sub status {
-    my ( $self, $newval) = @_;
-    if ($newval) {
-        # This is hackery to enable us to undefine
-        # status_alias, since we need to have an overloaded
-        # status_alias method to get us around the problem described
-        # here:
-        # https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c156
-        # We need a way of passing implied undef to nullify status_alias
-        # so we pass -1, which is special cased in the overloaded setter
-        $self->status_alias("-1");
-        return $self->SUPER::status($newval);
+    my ( $self, $new_status) = @_;
+
+    my $current_status = $self->SUPER::status;
+    my $current_status_alias = $self->SUPER::status_alias;
+
+    if ($new_status) {
+        # Keep a record of the previous status before we change it,
+        # we might need it
+        $self->{previous_status} = $current_status_alias ?
+            $current_status_alias :
+            $current_status;
+        my $ret = $self->SUPER::status($new_status)->store;
+        if ($current_status_alias) {
+            # This is hackery to enable us to undefine
+            # status_alias, since we need to have an overloaded
+            # status_alias method to get us around the problem described
+            # here:
+            # https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c156
+            # We need a way of passing implied undef to nullify status_alias
+            # so we pass -1, which is special cased in the overloaded setter
+            $self->status_alias("-1");
+        } else {
+            my $logger = Koha::Illrequest::Logger->new;
+            $logger->log_status_change({
+                request => $self,
+                value   => $new_status
+            });
+        }
+        delete $self->{previous_status};
+        return $ret;
+    } else {
+        return $current_status;
     }
-    return $self->SUPER::status;
 }
 
 =head3 load_backend
@@ -252,7 +302,10 @@ sub load_backend {
     my $location = join "/", @raw, $backend_name, "Base.pm";    # File to load
     my $backend_class = join "::", @raw, $backend_name, "Base"; # Package name
     require $location;
-    $self->{_my_backend} = $backend_class->new({ config => $self->_config });
+    $self->{_my_backend} = $backend_class->new({
+        config => $self->_config,
+        logger => Koha::Illrequest::Logger->new
+    });
     return $self;
 }
 
@@ -1107,6 +1160,33 @@ sub _censor {
     return $params;
 }
 
+=head3 store
+
+    $Illrequest->store;
+
+Overloaded I<store> method that, in addition to performing the 'store',
+possibly records the fact that something happened
+
+=cut
+
+sub store {
+    my ( $self, $attrs ) = @_;
+
+    my $ret = $self->SUPER::store;
+
+    $attrs->{log_origin} = 'core';
+
+    if ($ret && defined $attrs) {
+        my $logger = Koha::Illrequest::Logger->new;
+        $logger->log_maybe({
+            request => $self,
+            attrs   => $attrs
+        });
+    }
+
+    return $ret;
+}
+
 =head3 TO_JSON
 
     $json = $illrequest->TO_JSON
diff --git a/Koha/Illrequest/Logger.pm b/Koha/Illrequest/Logger.pm
new file mode 100644 (file)
index 0000000..0625370
--- /dev/null
@@ -0,0 +1,237 @@
+package Koha::Illrequest::Logger;
+
+# Copyright 2018 PTFS Europe Ltd
+#
+# This file is part of Koha.
+#
+# Koha is free software; you can redistribute it and/or modify it under the
+# terms of the GNU General Public License as published by the Free Software
+# Foundation; either version 3 of the License, or (at your option) any later
+# version.
+#
+# Koha is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE.  See the GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with Koha; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+use Modern::Perl;
+use JSON qw( to_json from_json );
+use Time::Local;
+
+use C4::Koha;
+use C4::Context;
+use C4::Templates;
+use C4::Log qw( logaction );
+use Koha::ActionLogs;
+
+=head1 NAME
+
+Koha::Illrequest::Logger - Koha ILL Action / Event logger
+
+=head1 SYNOPSIS
+
+Object-oriented class that provides event logging functionality for
+ILL requests
+
+=head1 DESCRIPTION
+
+This class provides the ability to log arbitrary actions or events
+relating to Illrequest to the action log.
+
+=head1 API
+
+=head2 Class Methods
+
+=head3 new
+
+    my $config = Koha::Illrequest::Logger->new();
+
+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
+
+sub new {
+    my ( $class ) = @_;
+    my $self  = {};
+
+    $self->{loggers} = {
+        status => sub {
+            $self->log_status_change(@_);
+        }
+    };
+
+    my ( $htdocs, $theme, $lang, $base ) =
+        C4::Templates::_get_template_file('ill/log/', 'intranet');
+
+    $self->{templates} = {
+        STATUS_CHANGE => $base . 'status_change.tt'
+    };
+
+    bless $self, $class;
+
+    return $self;
+}
+
+=head3 log_maybe
+
+    Koha::IllRequest::Logger->log_maybe($params);
+
+Receive params hashref, containing a request object and an attrs
+hashref (which may or may not be defined) If the attrs hashref contains
+a key matching our "loggers" hashref then we want to log it
+
+=cut
+
+sub log_maybe {
+    my ($self, $params) = @_;
+
+    if (defined $params->{request} && defined $params->{attrs}) {
+        foreach my $key (keys %{ $params->{attrs} }) {
+            if (defined($self->{loggers}->{$key})) {
+                $self->{loggers}->{$key}(
+                    $params->{request},
+                    $params->{attrs}->{$key}
+                );
+            }
+        }
+    }
+}
+
+=head3 log_status_change
+
+    Koha::IllRequest::Logger->log_status_change($params);
+
+Receive a hashref containing a request object and a status to log,
+and log it
+
+=cut
+
+sub log_status_change {
+    my ( $self, $params ) = @_;
+
+    if (defined $params->{request} && defined $params->{value}) {
+        $self->log_something({
+            modulename   => 'ILL',
+            actionname   => 'STATUS_CHANGE',
+            objectnumber => $params->{request}->id,
+            infos        => to_json({
+                log_origin    => 'core',
+                status_before => $params->{request}->{previous_status},
+                status_after  => $params->{value}
+            })
+        });
+    }
+}
+
+=head3 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 passed, log an action
+
+=cut
+
+sub log_something {
+    my ( $self, $to_log ) = @_;
+
+    if (
+        defined $to_log->{modulename} &&
+        defined $to_log->{actionname} &&
+        defined $to_log->{objectnumber} &&
+        defined $to_log->{infos} &&
+        C4::Context->preference("IllLog")
+    ) {
+        logaction(
+            $to_log->{modulename},
+            $to_log->{actionname},
+            $to_log->{objectnumber},
+            $to_log->{infos}
+        );
+    }
+}
+
+=head3 get_log_template
+
+    $template_path = get_log_template($params);
+
+Given a log's origin and action, get the appropriate display template
+
+=cut
+
+sub get_log_template {
+    my ($self, $params) = @_;
+
+    my $origin = $params->{origin};
+    my $action = $params->{action};
+
+    if ($origin eq 'core') {
+        # It's a core log, so we can just get the template path from
+        # the hashref above
+        return $self->{templates}->{$action};
+    } else {
+        # It's probably a backend log, so we need to get the path to the
+        # template from the backend
+        my $backend =$params->{request}->{_my_backend};
+        return $backend->get_log_template_path($action);
+    }
+}
+
+=head3 get_request_logs
+
+    $requestlogs = Koha::IllRequest::Logger->get_request_logs($request_id);
+
+Get all logged actions for a given request
+
+=cut
+
+sub get_request_logs {
+    my ( $self, $request ) = @_;
+
+    my $logs = Koha::ActionLogs->search(
+        {
+            module => 'ILL',
+            object => $request->id
+        },
+        { order_by => { -desc => "timestamp" } }
+    )->unblessed;
+
+    # Populate a lookup table for status aliases
+    my $aliases = GetAuthorisedValues('ILLSTATUS');
+    my $alias_hash;
+    foreach my $alias(@{$aliases}) {
+        $alias_hash->{$alias->{authorised_value}} = $alias;
+    }
+    foreach my $log(@{$logs}) {
+        $log->{aliases} = $alias_hash;
+        $log->{info} = from_json($log->{info});
+        $log->{template} = $self->get_log_template({
+            request => $request,
+            origin => $log->{info}->{log_origin},
+            action => $log->{action}
+        });
+    }
+
+    return $logs;
+}
+
+=head1 AUTHOR
+
+Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
+
+=cut
+
+1;
diff --git a/installer/data/mysql/atomicupdate/bug_20750-add_illlog_preference.perl b/installer/data/mysql/atomicupdate/bug_20750-add_illlog_preference.perl
new file mode 100644 (file)
index 0000000..75838f9
--- /dev/null
@@ -0,0 +1,7 @@
+$DBversion = 'XXX';  # will be replaced by the RM
+if( CheckVersion( $DBversion ) ) {
+    $dbh->do( "INSERT IGNORE INTO systempreferences (variable, value, explanation, type) VALUES ('IllLog', 0, 'If ON, log information about ILL requests', 'YesNo')" );
+
+    SetVersion( $DBversion );
+    print "Upgrade to $DBversion done (Bug 20750 - Allow timestamped auditing of ILL request events)\n";
+}
index 1d095d4..c6d2746 100644 (file)
@@ -216,6 +216,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `
 ('IDreamBooksResults','0','','Display IDreamBooks.com rating in search results','YesNo'),
 ('IDreamBooksReviews','0','','Display book review snippets from IDreamBooks.com','YesNo'),
 ('IdRef','0','','Disable/enable the IdRef webservice from the OPAC detail page.','YesNo'),
+('IllLog', 0, '', 'If ON, log information about ILL requests', 'YesNo'),
 ('ILLModule','0','If ON, enables the interlibrary loans module.','','YesNo'),
 ('ILLModuleCopyrightClearance','','70|10','Enter text to enable the copyright clearance stage of request creation. Text will be displayed','Textarea'),
 ('ILLOpacbackends',NULL,NULL,'ILL backends to enabled for OPAC initiated requests','multiple'),
index 2a94ff6..b3ca3c2 100644 (file)
@@ -37,6 +37,12 @@ Logging:
                   off: "Don't log"
             - any actions on holds (create, cancel, suspend, resume, etc).
         -
+            - pref: IllLog
+              choices:
+                  on: Log
+                  off: "Don't log"
+            - when changes to ILL requests take place
+        -
             - pref: IssueLog
               choices:
                   on: Log
index 2041cf0..18e7820 100644 (file)
                             <span class="fa fa-eye"></span>
                             Display supplier metadata
                         </a>
+                        <a title="ILL request log" id="ill-request-display-log" class="btn btn-sm btn-default pull-right" href="#">
+                            <span class="fa fa-calendar"></span>
+                            ILL request log
+                        </a>
                     </div>
                     <div class="ill-view-panel panel panel-default">
                         <div class="panel-heading">
                         </div>
                     </div>
 
+                    <div id="requestLog" class="modal fade" tabindex="-1" role="dialog" aria-labelledby="dataPreviewLabel" aria-hidden="true">
+                        <div class="modal-dialog">
+                            <div class="modal-content">
+                                <div class="modal-header">
+                                    <button type="button" class="closebtn" data-dismiss="modal" aria-hidden="true">×</button>
+                                    <h3 id="requestLogLabel"> Request log</h3>
+                                </div>
+                                <div class="modal-body">
+                                [% IF request.logs.size > 0 %]
+                                    [% FOREACH log IN request.logs %]
+                                        [% tpl = log.template %]
+                                        [% INCLUDE $tpl log=log %]
+                                    [% END %]
+                                [% ELSE %]
+                                    There are no recorded logs for this request
+                                [% END %]
+                                </div>
+                                <div class="modal-footer">
+                                    <button class="btn btn-default" data-dismiss="modal" aria-hidden="true">Close</button>
+                                </div>
+                            </div>
+                        </div>
+                    </div>
+
                     <div class="ill-view-panel panel panel-default">
                         <div class="panel-heading">
                             <h3>[% request.illcomments.count | html %] comments</h3>
                                         </form>
                                     </div>
                                 </div>
-                            </div>
+                        </div>
                     </div>
 
                 [% ELSIF query_type == 'illlist' %]
                 }
             };
 
+            // Display the modal containing request supplier metadata
+            $('#ill-request-display-log').on('click', function(e) {
+                e.preventDefault();
+                $('#requestLog').modal({show:true});
+            });
+
             // Toggle request attributes in Illview
             $('#toggle_requestattributes').on('click', function(e) {
                 e.preventDefault();
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/ill/log/status_change.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/ill/log/status_change.tt
new file mode 100644 (file)
index 0000000..da0bf56
--- /dev/null
@@ -0,0 +1,11 @@
+<p>
+[% log.timestamp | $KohaDates with_hours => 1 %] : <b>Status changed</b>
+[% IF log.info.status_before %]
+[% before = log.info.status_before %]
+[% display_before = log.aliases.$before ? log.aliases.$before.lib : request.capabilities.$before.name %]
+from &quot;[% display_before | html %]&quot;
+[% END %]
+[% after = log.info.status_after %]
+[% display_after = log.aliases.$after ? log.aliases.$after.lib : request.capabilities.$after.name %]
+to &quot;[% display_after | html %]&quot;
+</p>
index b3e1105..a75270e 100644 (file)
@@ -29,6 +29,7 @@
 [%        CASE 'ACQUISITIONS' %]Acquisitions
 [%        CASE 'SERIAL'       %]Serials
 [%        CASE 'HOLDS'        %]Holds
+[%        CASE 'ILL'          %]Interlibrary loans
 [%        CASE 'CIRCULATION'  %]Circulation
 [%        CASE 'LETTER'       %]Letter
 [%        CASE 'FINES'        %]Fines
@@ -54,6 +55,7 @@
 [%        CASE 'CHANGE PASS' %]Change password
 [%        CASE 'ADDCIRCMESSAGE' %]Add circulation message
 [%        CASE 'DELCIRCMESSAGE' %]Delete circulation message
+[%        CASE 'STATUS_CHANGE'  %]Change ILL request status
 [%        CASE 'Run'    %]Run
 [%        CASE %][% action | html %]
 [%    END %]
                                     [% ELSE %]
                                         <option value="">All</option>
                                     [% END %]
-                                    [% FOREACH modx IN [ 'CATALOGUING' 'AUTHORITIES' 'MEMBERS' 'ACQUISITIONS' 'SERIAL' 'HOLDS' 'CIRCULATION' 'LETTER' 'FINES' 'SYSTEMPREFERENCE' 'CRONJOBS', 'REPORTS' ] %]
+                                    [% FOREACH modx IN [ 'CATALOGUING' 'AUTHORITIES' 'MEMBERS' 'ACQUISITIONS' 'SERIAL' 'HOLDS' 'ILL' 'CIRCULATION' 'LETTER' 'FINES' 'SYSTEMPREFERENCE' 'CRONJOBS', 'REPORTS' ] %]
                                         [% IF modules.grep(modx).size %]
                                             <option value="[% modx | html %]" selected="selected">[% PROCESS translate_log_module module=modx %]</option>
                                         [% ELSE %]
index 9046852..ffbc98d 100644 (file)
@@ -39,7 +39,7 @@ use_ok('Koha::Illrequests');
 
 subtest 'Basic object tests' => sub {
 
-    plan tests => 24;
+    plan tests => 25;
 
     $schema->storage->txn_begin;
 
@@ -62,6 +62,8 @@ subtest 'Basic object tests' => sub {
        "Branchcode getter works.");
     is($illrq_obj->status, $illrq->{status},
        "Status getter works.");
+    is($illrq_obj->status_alias, $illrq->{status_alias},
+       "Status_alias getter works.");
     is($illrq_obj->placed, $illrq->{placed},
        "Placed getter works.");
     is($illrq_obj->replied, $illrq->{replied},