From 875b358217af865b48b2000684526f77aa5299c0 Mon Sep 17 00:00:00 2001 From: Andrew Isherwood 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 375e7586a1..acaf1fcdcf 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 @@

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

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