Attachment 84978 Details for Bug 20750 – Bug 20750: (follow-up) Fix logging of status changes

[patch] Bug 20750: (follow-up) Fix logging of status changes

Bug-20750-follow-up-Fix-logging-of-status-changes.patch (text/plain), 8.11 KB, created by Andrew Isherwood on 2019-02-11 14:43:53 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Andrew Isherwood

Created: 2019-02-11 14:43:53 UTC

Size: 8.11 KB

patch

obsolete

>From 20462a7e2db4c5d1adbd378f45ed52781d3ad2be Mon Sep 17 00:00:00 2001
>From: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
>Date: Mon, 28 Jan 2019 17:06:50 +0000
>Subject: [PATCH] Bug 20750: (follow-up) Fix logging of status changes
>
>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.
>---
> Koha/Illrequest.pm                                 | 38 +++++++++++++++++-----
> Koha/Illrequest/Logger.pm                          |  8 +++++
> .../prog/en/modules/ill/log/status_change.tt       | 16 +++++++--
> t/db_dependent/Illrequest/Logger.t                 | 31 +-----------------
> t/db_dependent/Illrequests.t                       |  4 ++-
> 5 files changed, 55 insertions(+), 42 deletions(-)
>
>diff --git a/Koha/Illrequest.pm b/Koha/Illrequest.pm
>index bea164fd3e..3ace1268ad 100644
>--- a/Koha/Illrequest.pm
>+++ b/Koha/Illrequest.pm
>@@ -180,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
>@@ -196,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(
>+                $self,
>+                $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
>@@ -232,11 +249,14 @@ sub status {
>     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;
>+        $self->{previous_status} = $current_status_alias ?
>+            $current_status_alias :
>+            $current_status;
>         my $ret = $self->SUPER::status($new_status)->store;
>         if ($ret) {
>             # This is hackery to enable us to undefine
>diff --git a/Koha/Illrequest/Logger.pm b/Koha/Illrequest/Logger.pm
>index c68d206bcc..1b016f84d8 100644
>--- a/Koha/Illrequest/Logger.pm
>+++ b/Koha/Illrequest/Logger.pm
>@@ -21,6 +21,7 @@ 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 GetLogs );
>@@ -238,7 +239,14 @@ sub get_request_logs {
>         undef,
>         undef
>     );
>+    # Populate a lookup table for status aliases
>+    my $aliases = GetAuthorisedValues('ILLSTATUS');
>+    my $alias_hash;
>+    foreach my $alias(@{$aliases}) {
>+        $alias_hash->{$alias->{id}} = $alias;
>+    }
>     foreach my $log(@{$logs}) {
>+        $log->{aliases} = $alias_hash;
>         $log->{info} = from_json($log->{info});
>         $log->{template} = $self->get_log_template(
>         $request,
>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
>index 78e994ee87..7fade01edb 100644
>--- 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
>@@ -1,7 +1,19 @@
> <p>
> [% log.timestamp | $KohaDates with_hours => 1 %] : <b>Status changed </b>
> [% IF log.info.status_before %]
>-from &quot;[% request.capabilities(log.info.status_before).name | html %]&quot;
>+[% before = log.info.status_before %]
>+[%
>+    display_before = log.info.status_before.match('^\d+$') ?
>+        log.aliases.$before.lib :
>+        request.capabilities.$before.name
>+%]
>+from &quot;[% display_before %]&quot;
> [% END %]
>-to &quot;[% request.capabilities(log.info.status_after).name | html %]&quot;
>+[% after = log.info.status_after %]
>+[%
>+    display_after = log.info.status_after.match('^\d+$') ?
>+        log.aliases.$after.lib :
>+        request.capabilities.$after.name;
>+%]
>+to &quot;[% display_after | html %]&quot;
> </p>
>diff --git a/t/db_dependent/Illrequest/Logger.t b/t/db_dependent/Illrequest/Logger.t
>index 1e9ffcf5ad..b9c9ac8071 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 => 10;
>+    plan tests => 9;
> 
>     $schema->storage->txn_begin;
> 
>@@ -120,35 +120,6 @@ subtest 'Basics' => sub {
>         'get_log_template() fetches correct core template'
>     );
> 
>-    # get_request_logs
>-    #
>-    my $res_obj = [
>-        {
>-            'template' => 'mock',
>-            'info' => { 'log_origin' => 'core' },
>-            'timestamp' => '2018-10-02 11:12:32',
>-            'action' => 'STATUS_CHANGE'
>-        },
>-        {
>-            'template' => 'mock',
>-            'action' => 'STATUS_CHANGE',
>-            'timestamp' => '2018-10-02 11:12:22',
>-            'info' => { 'log_origin' => 'core' }
>-        },
>-        {
>-            'template' => 'mock',
>-            'action' => 'STATUS_CHANGE',
>-            'info' => { 'log_origin' => 'core' },
>-            'timestamp' => '2018-10-02 11:12:12'
>-        }
>-    ];
>-    my $me_mock = Test::MockModule->new('Koha::Illrequest::Logger');
>-    $me_mock->mock('get_log_template', sub { return 'mock'; });
>-    my $mock_req = Test::MockObject->new();
>-    $mock_req->mock('id', sub { 1 });
>-    is_deeply($logger->get_request_logs($mock_req), $res_obj,
>-		'get_request_logs() returns logs property structured and sorted');
>-
>     $schema->storage->txn_rollback;
> };
> 
>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.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