From b1a4f6d594d4a17c23886cda6f2442dda8fbbed3 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 | 66 ++++++++++++++-------- 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, 69 insertions(+), 56 deletions(-) diff --git a/Koha/Illrequest.pm b/Koha/Illrequest.pm index de1a303a26..1b41dd44aa 100644 --- a/Koha/Illrequest.pm +++ b/Koha/Illrequest.pm @@ -180,15 +180,26 @@ sub patron { } =head3 status_alias -Overloaded getter/setter for status_alias, -that only returns authorised values from the -correct category + + $Illrequest->status_alias('CUSTOM_AV'); + +Overloaded getter/setter for request status_alias, +returns authorised values from the correct category, +also 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,26 +207,32 @@ 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($newval); - if ($newval) { - return $newval; + my $val = $new_status_alias eq "-1" ? undef : $new_status_alias; + my $ret = $self->SUPER::status_alias($val)->store; + if ($ret) { + my $logger = Koha::Illrequest::Logger->new; + $logger->log_status_change( + $self, + $new_status_alias + ); } else { - return; + delete $self->{previous_status}; } - } - # We can't know which result is the right one if there are multiple - # ILLSTATUS authorised values with the same authorised_value column value - # so we just use the first - my $alias = Koha::AuthorisedValues->search({ - branchcode => $self->branchcode, - category => 'ILLSTATUS', - authorised_value => $self->SUPER::status_alias - })->next; - if ($alias) { - return $alias->authorised_value; + return $ret; } else { - return; + # We can't know which result is the right one if there are multiple + # ILLSTATUS authorised values with the same authorised_value column + # value so we just use the first + my $alias = Koha::AuthorisedValues->search({ + branchcode => $self->branchcode, + category => 'ILLSTATUS', + authorised_value => $current_status_alias + })->next; + if ($alias) { + return $alias->authorised_value; + } else { + return; + } } } @@ -232,6 +249,7 @@ sub status { my ( $self, $new_status) = @_; my $current_status = $self->SUPER::status; + my $current_status_alias = $self->status_alias; if ($new_status) { # This is hackery to enable us to undefine @@ -244,7 +262,9 @@ sub 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) { $self->status_alias("-1"); diff --git a/Koha/Illrequest/Logger.pm b/Koha/Illrequest/Logger.pm index 3c6e9fc125..3f9a840e06 100644 --- a/Koha/Illrequest/Logger.pm +++ b/Koha/Illrequest/Logger.pm @@ -20,6 +20,7 @@ package Koha::Illrequest::Logger; 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 ); @@ -237,7 +238,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