From 79d64a37a0d200fc3f6ecf03731ff6c954fc0749 Mon Sep 17 00:00:00 2001 From: Andrew Isherwood Date: Tue, 2 Oct 2018 14:55:58 +0100 Subject: [PATCH] Bug 20750: Allow logging of arbitrary actions 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 Signed-off-by: Josef Moravec 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 Signed-off-by: Nick Clemens --- Koha/Illrequest.pm | 124 +++++++-- Koha/Illrequest/Logger.pm | 237 ++++++++++++++++++ .../bug_20750-add_illlog_preference.perl | 7 + installer/data/mysql/sysprefs.sql | 1 + .../en/modules/admin/preferences/logs.pref | 6 + .../prog/en/modules/ill/ill-requests.tt | 36 ++- .../prog/en/modules/ill/log/status_change.tt | 11 + .../prog/en/modules/tools/viewlog.tt | 4 +- t/db_dependent/Illrequests.t | 4 +- 9 files changed, 405 insertions(+), 25 deletions(-) create mode 100644 Koha/Illrequest/Logger.pm create mode 100644 installer/data/mysql/atomicupdate/bug_20750-add_illlog_preference.perl create mode 100644 koha-tmpl/intranet-tmpl/prog/en/modules/ill/log/status_change.tt diff --git a/Koha/Illrequest.pm b/Koha/Illrequest.pm index 94e24debe5..13c0b1114d 100644 --- a/Koha/Illrequest.pm +++ b/Koha/Illrequest.pm @@ -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 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 index 0000000000..0625370237 --- /dev/null +++ b/Koha/Illrequest/Logger.pm @@ -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 + +=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 index 0000000000..75838f9be2 --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_20750-add_illlog_preference.perl @@ -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"; +} diff --git a/installer/data/mysql/sysprefs.sql b/installer/data/mysql/sysprefs.sql index 1d095d4a14..c6d2746378 100644 --- a/installer/data/mysql/sysprefs.sql +++ b/installer/data/mysql/sysprefs.sql @@ -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'), diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/logs.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/logs.pref index 2a94ff6ab6..b3ca3c2433 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/logs.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/logs.pref @@ -36,6 +36,12 @@ Logging: on: Log 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: diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt index 2041cf0172..18e7820207 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt @@ -319,6 +319,10 @@ Display supplier metadata + + + ILL request log +
@@ -429,6 +433,30 @@
+ +

[% request.illcomments.count | html %] comments

@@ -467,7 +495,7 @@
- + [% ELSIF query_type == 'illlist' %] @@ -835,6 +863,12 @@ } }; + // 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 index 0000000000..da0bf561f2 --- /dev/null +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/ill/log/status_change.tt @@ -0,0 +1,11 @@ +

+[% log.timestamp | $KohaDates with_hours => 1 %] : Status changed +[% IF log.info.status_before %] +[% before = log.info.status_before %] +[% display_before = log.aliases.$before ? log.aliases.$before.lib : request.capabilities.$before.name %] +from "[% display_before | html %]" +[% END %] +[% after = log.info.status_after %] +[% display_after = log.aliases.$after ? log.aliases.$after.lib : request.capabilities.$after.name %] +to "[% display_after | html %]" +

diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt index b3e11059b5..a75270e82b 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt @@ -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 %] @@ -103,7 +105,7 @@ [% ELSE %] [% 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 %] [% ELSE %] diff --git a/t/db_dependent/Illrequests.t b/t/db_dependent/Illrequests.t index 9046852b45..ffbc98dc4f 100644 --- a/t/db_dependent/Illrequests.t +++ b/t/db_dependent/Illrequests.t @@ -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}, -- 2.39.5