Bug 20750 - Allow timestamped auditing of ILL request events
Summary: Allow timestamped auditing of ILL request events
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Andrew Isherwood
QA Contact: Josef Moravec
URL:
Whiteboard:
Keywords:
: 21461 (view as bug list)
Depends on: 20639 22363
Blocks: 21063
  Show dependency treegraph
 
Reported: 2018-05-10 11:25 UTC by Andrew Isherwood
Modified: 2019-03-19 10:43 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 20750: Allow logging of arbitrary actions (17.19 KB, patch)
2018-05-14 13:37 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Allow logging of arbitrary actions (23.44 KB, patch)
2018-05-15 11:19 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Squashed commit for ILL enhancements (20.33 KB, patch)
2018-05-15 13:12 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Allow logging of arbitrary actions (19.62 KB, patch)
2018-05-15 13:15 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Allow logging of arbitrary actions (19.93 KB, patch)
2018-08-13 15:30 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Allow logging of arbitrary actions (19.93 KB, patch)
2018-08-15 14:44 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Signed-off-by: Niamh Walker <Niamh.Walker-Headon@it-tallaght.ie> (20.00 KB, patch)
2018-08-15 14:46 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Allow logging of arbitrary actions (19.93 KB, patch)
2018-08-15 14:50 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 20750: Allow logging of arbitrary actions (20.00 KB, patch)
2018-08-16 14:55 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 20750: Allow logging of arbitrary actions (19.42 KB, patch)
2018-10-02 13:58 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Add unit tests (5.65 KB, patch)
2018-10-02 13:58 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Allow logging of arbitrary actions (19.49 KB, patch)
2018-10-02 14:04 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix sorting of log entries (1.40 KB, patch)
2018-10-18 15:10 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Add unit tests (5.65 KB, patch)
2018-11-08 14:19 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Allow logging of arbitrary actions (19.72 KB, patch)
2018-11-08 14:19 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix sorting of log entries (1.40 KB, patch)
2018-11-08 14:19 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Allow logging of arbitrary actions (20.01 KB, patch)
2018-11-08 15:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Add unit tests (5.65 KB, patch)
2018-11-08 15:39 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Allow logging of arbitrary actions (20.01 KB, patch)
2018-11-08 15:39 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix sorting of log entries (1.40 KB, patch)
2018-11-08 15:39 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Add unit tests (5.65 KB, patch)
2018-11-19 14:17 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Allow logging of arbitrary actions (19.94 KB, patch)
2018-11-19 14:17 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix sorting of log entries (1.40 KB, patch)
2018-11-19 14:17 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Allow logging of arbitrary actions (19.27 KB, patch)
2019-01-18 14:33 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix sorting of log entries (1.40 KB, patch)
2019-01-18 14:33 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Add unit tests (5.37 KB, patch)
2019-01-18 14:33 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix QA errors (1.15 KB, patch)
2019-01-18 15:34 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Remove Time::Piece dep (4.25 KB, patch)
2019-01-21 14:25 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix dependency merge error (1.31 KB, patch)
2019-01-28 16:38 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix logging of status changes (7.37 KB, patch)
2019-01-28 17:10 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Allow logging of arbitrary actions (19.72 KB, patch)
2019-02-06 14:30 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Allow logging of arbitrary actions (19.72 KB, patch)
2019-02-06 15:07 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Add unit tests (5.37 KB, patch)
2019-02-06 15:07 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix QA errors (1.15 KB, patch)
2019-02-06 15:07 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Remove Time::Piece dep (4.21 KB, patch)
2019-02-06 15:07 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix logging of status changes (9.08 KB, patch)
2019-02-06 15:07 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Allow logging of arbitrary actions (20.14 KB, patch)
2019-02-11 14:43 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix sorting of log entries (1.40 KB, patch)
2019-02-11 14:43 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Add unit tests (5.37 KB, patch)
2019-02-11 14:43 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix QA errors (1.15 KB, patch)
2019-02-11 14:43 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Remove Time::Piece dep (4.25 KB, patch)
2019-02-11 14:43 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix logging of status changes (8.11 KB, patch)
2019-02-11 14:43 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix merge problems (3.15 KB, patch)
2019-02-11 14:43 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix QA script errors (1.56 KB, patch)
2019-02-14 10:25 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Remove ill_logging_pref.* (1.36 KB, patch)
2019-02-14 10:28 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Modify display log wording (1.45 KB, patch)
2019-02-14 10:40 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Remove wayward space (1.10 KB, patch)
2019-02-14 10:53 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Remove status_alias test (1.46 KB, patch)
2019-02-14 13:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Set logging default off (2.47 KB, patch)
2019-02-18 10:56 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Use Koha::ActionLogs (2.36 KB, patch)
2019-02-19 11:38 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Allow logging of arbitrary actions (20.11 KB, patch)
2019-02-26 10:08 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix sorting of log entries (1.40 KB, patch)
2019-02-26 10:08 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Add unit tests (5.37 KB, patch)
2019-02-26 10:08 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix QA errors (1.15 KB, patch)
2019-02-26 10:08 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Remove Time::Piece dep (4.25 KB, patch)
2019-02-26 10:08 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix logging of status changes (8.11 KB, patch)
2019-02-26 10:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix merge problems (3.15 KB, patch)
2019-02-26 10:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix QA script errors (1.56 KB, patch)
2019-02-26 10:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Remove ill_logging_pref.* (1.36 KB, patch)
2019-02-26 10:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Modify display log wording (1.45 KB, patch)
2019-02-26 10:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Remove wayward space (1.10 KB, patch)
2019-02-26 10:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Remove status_alias test (1.46 KB, patch)
2019-02-26 10:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Set logging default off (2.43 KB, patch)
2019-02-26 10:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Use Koha::ActionLogs (2.36 KB, patch)
2019-02-26 10:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Don't use object variable (3.96 KB, patch)
2019-02-27 10:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Don't use object variable (5.45 KB, patch)
2019-02-27 11:19 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Pass hashrefs to functions (6.43 KB, patch)
2019-02-27 11:41 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Explicitly pass log var (1.25 KB, patch)
2019-02-28 10:37 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: (follow-up) Remove BLDSS_STATUS_CHECK (1.06 KB, patch)
2019-02-28 10:40 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20750: Allow logging of arbitrary actions (20.16 KB, patch)
2019-02-28 11:20 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix sorting of log entries (1.45 KB, patch)
2019-02-28 11:20 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: Add unit tests (5.43 KB, patch)
2019-02-28 11:20 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix QA errors (1.21 KB, patch)
2019-02-28 11:20 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: (follow-up) Remove Time::Piece dep (4.31 KB, patch)
2019-02-28 11:20 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix logging of status changes (8.17 KB, patch)
2019-02-28 11:20 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix merge problems (3.20 KB, patch)
2019-02-28 11:20 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: (follow-up) Fix QA script errors (1.62 KB, patch)
2019-02-28 11:21 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: (follow-up) Remove ill_logging_pref.* (1.42 KB, patch)
2019-02-28 11:21 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: (follow-up) Modify display log wording (1.50 KB, patch)
2019-02-28 11:21 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: (follow-up) Remove wayward space (1.15 KB, patch)
2019-02-28 11:21 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: (follow-up) Remove status_alias test (1.52 KB, patch)
2019-02-28 11:21 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: (follow-up) Set logging default off (2.49 KB, patch)
2019-02-28 11:21 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: (follow-up) Use Koha::ActionLogs (2.41 KB, patch)
2019-02-28 11:21 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: (follow-up) Don't use object variable (5.51 KB, patch)
2019-02-28 11:21 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: (follow-up) Pass hashrefs to functions (6.49 KB, patch)
2019-02-28 11:21 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: (follow-up) Explicitly pass log var (1.31 KB, patch)
2019-02-28 11:21 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: (follow-up) Remove BLDSS_STATUS_CHECK (1.12 KB, patch)
2019-02-28 11:21 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: (QA follow-up) Add ILL status change option to actions in log viewer (1.53 KB, patch)
2019-02-28 11:21 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: (QA follow-up) Add use KohaDates to status change template (890 bytes, patch)
2019-02-28 11:21 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: Allow logging of arbitrary actions (23.43 KB, patch)
2019-03-11 12:22 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: Add unit tests (5.44 KB, patch)
2019-03-11 12:22 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: (QA follow-up) Fix templates (2.03 KB, patch)
2019-03-11 12:23 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20750: Allow logging of arbitrary actions (23.45 KB, patch)
2019-03-12 13:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20750: Add unit tests (5.51 KB, patch)
2019-03-12 13:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20750: (QA follow-up) Fix templates (2.10 KB, patch)
2019-03-12 13:32 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Isherwood 2018-05-10 11:25:12 UTC
Currently there is no audit trail provided for ILL requests, this is required.

At the most basic form this would be a timestamped record of changes of a request's status. It would be useful to be able to also record other arbitrary events.

This bug proposes the addition of a requests_actions log that allows either the core ILL code or backends to record arbitrary events relating to a request. This could then be displayed in the UI as an "audit trail" or reported upon.
Comment 1 Andrew Isherwood 2018-05-10 14:13:48 UTC
Discussed with joubu on IRC, suggestion was to use actions_logs rather than bake our own recording mechanism
Comment 2 Magnus Enger 2018-05-11 09:01:26 UTC
Sounds like a most excellent idea! Would this include some kind of diff between old and new metadata when a request is updated? That would be awesome!
Comment 3 Andrew Isherwood 2018-05-11 10:57:38 UTC
The initial implementation will only log status changes and this will indeed record "before" and "after" statuses. I'm implementing it in as extensible a way as possible to allow other events/actions to be logged with whatever data is required
Comment 4 Andrew Isherwood 2018-05-14 13:37:32 UTC Comment hidden (obsolete)
Comment 5 Magnus Enger 2018-05-14 18:46:04 UTC Comment hidden (obsolete)
Comment 6 Magnus Enger 2018-05-14 18:51:07 UTC Comment hidden (obsolete)
Comment 7 Magnus Enger 2018-05-14 18:52:45 UTC Comment hidden (obsolete)
Comment 8 Andrew Isherwood 2018-05-15 11:18:20 UTC Comment hidden (obsolete)
Comment 9 Andrew Isherwood 2018-05-15 11:19:42 UTC Comment hidden (obsolete)
Comment 10 Magnus Enger 2018-05-15 12:43:38 UTC Comment hidden (obsolete)
Comment 11 Andrew Isherwood 2018-05-15 13:12:18 UTC Comment hidden (obsolete)
Comment 12 Andrew Isherwood 2018-05-15 13:15:46 UTC
Created attachment 75330 [details] [review]
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.
Comment 13 Andrew Isherwood 2018-05-15 13:18:49 UTC
Well spotted Magnus, thanks! I've submitted a new patch which removes changes to Schema files.

Also, ignore comment #11 and attachment #75329 [details] [review] - over zealous fingers typing faster than my brain...
Comment 14 Magnus Enger 2018-05-22 07:28:12 UTC
From looking at the code, it seems that the test plan only covers part of the functionality, that of keeping a record of the actual status of a request. It looks like the new code can also track changes to the other parts of the metadata. Is this correct, and would it be possible to enhance the Dummy or FreeForm backends in such a way that this other functionality could also be tested?
Comment 15 Andrew Isherwood 2018-05-23 13:24:48 UTC
Hi Magnus - Thanks for looking at this. Currently, all this code does is log status changes. I can see how you might have thought that other stuff was being logged as, in addition to logging during ->status calls, we also have potential logging during a ->store call. This was to catch the case where a status was being set directly in a ->store call, e.g.

->store({ status => 'REQ' })

Which obviously wouldn't have been caught by the logger in the status method.

So, there is potential to log other "things" during the ->store call, but currently we don't. It is easily added by adding a new element to the Koha::ILLRequest::Logger $loggers class property:

	
$self->{loggers} = {
    status => sub {
        $self->log_status_change(@_);
    }
};

[...]

sub log_maybe {
    my ($self, $req, $attrs) = @_;
	
    if (defined $req && defined $attrs) {
        foreach my $key (keys %{ $attrs }) {
            if (defined($self->{loggers}->{$key})) {
                $self->{loggers}->{$key}($req, $attrs->{$key});
            }
        }
    }
}

[...]

sub log_status_change {
    my ( $self, $req, $new_status ) = @_;

    $self->set_data({
        actionname   => 'STATUS_CHANGE',
        objectnumber => $req->id,
        infos        => to_json({
            log_origin    => 'core',
            status_before => $req->{previous_status},
            status_after  => $new_status
        })
    });

    $self->log_something();
}

As you can see, currently it says "if we get a call to $request->store and the thing we're storing contains a 'status' property, log the value of it"

We also provide display templates for when it comes to displaying that stored "thing" back to the user.

I *have* added a call to the logger in the BLDSS backend whenever an API request is sent to the BLDSS status check endpoint. It's a very simple addition and can be seen here:

https://github.com/PTFS-Europe/BLDSS-backend/commit/40a4ccdce6e04293970258797d7a0c4fcb33ff32

I hope this helps :)
Comment 16 Magnus Enger 2018-05-24 09:45:44 UTC
(In reply to Andrew Isherwood from comment #15)
> Hi Magnus - Thanks for looking at this. Currently, all this code does is log
> status changes. 

Ah, gotcha! I'll sign off, then. 

If you do find the time to add logging of any changes to the metadata I'll be happy to test and sign off on it!
Comment 17 Magnus Enger 2018-05-24 09:53:42 UTC
Hm, the previous version of the patch was working as expected, but with the most recent version, when I go to "Manage request" i get an "Internal Server Error". The Plack error log says: 

Template process failed: undef error - The method logs is not covered by tests! at /home/vagrant/kohaclone/C4/Templates.pm line 122

The code around line 122 looks like this:

120     my $data;
121     binmode( STDOUT, ":utf8" );
122     $template->process( $self->filename, $vars, \$data )
123       || die "Template process failed: ", $template->error();
124     return $data;

Not quite sure if there is a problem with the patch or if i screwed up my testing environment?
Comment 18 Andrew Isherwood 2018-05-30 11:04:00 UTC
Hi Magnus. I can't replicate this, seems to work fine for me. It sounds like your Illrequest object doesn't know about the "logs" method. Try setting up the testing env again maybe?
Comment 19 Andrew Isherwood 2018-08-13 15:30:08 UTC
Created attachment 77757 [details] [review]
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.
Comment 20 PTFS Europe Sandboxes 2018-08-15 14:44:04 UTC
Created attachment 77816 [details] [review]
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.
Comment 21 Andrew Isherwood 2018-08-15 14:46:42 UTC
Created attachment 77817 [details] [review]
Signed-off-by: Niamh Walker <Niamh.Walker-Headon@it-tallaght.ie>
Comment 22 PTFS Europe Sandboxes 2018-08-15 14:50:09 UTC
Created attachment 77818 [details] [review]
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 <Niamh.Walker-Headon@it-tallaght.ie>
Comment 23 PTFS Europe Sandboxes 2018-08-16 14:55:32 UTC
Created attachment 77931 [details] [review]
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 <Niamh.Walker-Headon@it-tallaght.ie>
Comment 24 Martin Renvoize 2018-08-16 14:56:34 UTC
Just had to sort out some randomness with our sandboxes.. that final patch actually includes the signoff line this time..
Comment 25 Andrew Isherwood 2018-10-02 13:58:26 UTC
Created attachment 79822 [details] [review]
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.
Comment 26 Andrew Isherwood 2018-10-02 13:58:34 UTC
Created attachment 79823 [details] [review]
Bug 20750: Add unit tests
Comment 27 Andrew Isherwood 2018-10-02 14:04:00 UTC
Created attachment 79824 [details] [review]
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 <Niamh.Walker-Headon@it-tallaght.ie>
Comment 28 Magnus Enger 2018-10-02 15:36:36 UTC
*** Bug 21461 has been marked as a duplicate of this bug. ***
Comment 29 Andrew Isherwood 2018-10-18 15:10:49 UTC
Created attachment 80860 [details] [review]
Bug 20750: (follow-up) Fix sorting of log entries

The log entries are received from C4::Log::GetLogs with their timestamps
formatted as '2018-18-10 16:09:29', this doesn't sort well. In fact, it
causes a warning in the Koha logs:

Argument "2018-10-04 15:48:15" isn't numeric in numeric comparison (<=>)
at /home/koha/kohaclone/Koha/Illrequest/Logger.pm line 231

This patch converts the timestamps to be sorted into epoch dates prior
to them being compared
Comment 30 Andrew Isherwood 2018-11-08 14:19:16 UTC
Created attachment 82082 [details] [review]
Bug 20750: Add unit tests
Comment 31 Andrew Isherwood 2018-11-08 14:19:20 UTC
Created attachment 82083 [details] [review]
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 <Niamh.Walker-Headon@it-tallaght.ie>
Comment 32 Andrew Isherwood 2018-11-08 14:19:26 UTC
Created attachment 82085 [details] [review]
Bug 20750: (follow-up) Fix sorting of log entries

The log entries are received from C4::Log::GetLogs with their timestamps
formatted as '2018-18-10 16:09:29', this doesn't sort well. In fact, it
causes a warning in the Koha logs:

Argument "2018-10-04 15:48:15" isn't numeric in numeric comparison (<=>)
at /home/koha/kohaclone/Koha/Illrequest/Logger.pm line 231

This patch converts the timestamps to be sorted into epoch dates prior
to them being compared
Comment 33 Andrew Isherwood 2018-11-08 14:20:09 UTC
Rebased on top of master then the current dependency tree
Comment 34 Andrew Isherwood 2018-11-08 15:36:52 UTC
Created attachment 82096 [details] [review]
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 <Niamh.Walker-Headon@it-tallaght.ie>
Comment 35 Andrew Isherwood 2018-11-08 15:39:00 UTC
Created attachment 82097 [details] [review]
Bug 20750: Add unit tests
Comment 36 Andrew Isherwood 2018-11-08 15:39:04 UTC
Created attachment 82098 [details] [review]
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 <Niamh.Walker-Headon@it-tallaght.ie>
Comment 37 Andrew Isherwood 2018-11-08 15:39:08 UTC
Created attachment 82099 [details] [review]
Bug 20750: (follow-up) Fix sorting of log entries

The log entries are received from C4::Log::GetLogs with their timestamps
formatted as '2018-18-10 16:09:29', this doesn't sort well. In fact, it
causes a warning in the Koha logs:

Argument "2018-10-04 15:48:15" isn't numeric in numeric comparison (<=>)
at /home/koha/kohaclone/Koha/Illrequest/Logger.pm line 231

This patch converts the timestamps to be sorted into epoch dates prior
to them being compared
Comment 38 Andrew Isherwood 2018-11-19 14:17:25 UTC
Created attachment 82500 [details] [review]
Bug 20750: Add unit tests
Comment 39 Andrew Isherwood 2018-11-19 14:17:29 UTC
Created attachment 82501 [details] [review]
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 <Niamh.Walker-Headon@it-tallaght.ie>
Comment 40 Andrew Isherwood 2018-11-19 14:17:33 UTC
Created attachment 82502 [details] [review]
Bug 20750: (follow-up) Fix sorting of log entries

The log entries are received from C4::Log::GetLogs with their timestamps
formatted as '2018-18-10 16:09:29', this doesn't sort well. In fact, it
causes a warning in the Koha logs:

Argument "2018-10-04 15:48:15" isn't numeric in numeric comparison (<=>)
at /home/koha/kohaclone/Koha/Illrequest/Logger.pm line 231

This patch converts the timestamps to be sorted into epoch dates prior
to them being compared
Comment 41 Andrew Isherwood 2019-01-18 14:33:18 UTC
Created attachment 84193 [details] [review]
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 <Niamh.Walker-Headon@it-tallaght.ie>
Comment 42 Andrew Isherwood 2019-01-18 14:33:21 UTC
Created attachment 84194 [details] [review]
Bug 20750: (follow-up) Fix sorting of log entries

The log entries are received from C4::Log::GetLogs with their timestamps
formatted as '2018-18-10 16:09:29', this doesn't sort well. In fact, it
causes a warning in the Koha logs:

Argument "2018-10-04 15:48:15" isn't numeric in numeric comparison (<=>)
at /home/koha/kohaclone/Koha/Illrequest/Logger.pm line 231

This patch converts the timestamps to be sorted into epoch dates prior
to them being compared
Comment 43 Andrew Isherwood 2019-01-18 14:33:25 UTC
Created attachment 84195 [details] [review]
Bug 20750: Add unit tests
Comment 44 Andrew Isherwood 2019-01-18 14:34:05 UTC
Rebased following changes to C4/Log.pm & t/Log.t
Comment 45 Andrew Isherwood 2019-01-18 15:34:46 UTC
Created attachment 84203 [details] [review]
Bug 20750: (follow-up) Fix QA errors

This patch fixes the "missing_filter" QA error
Comment 46 Andrew Isherwood 2019-01-21 14:25:12 UTC
Created attachment 84266 [details] [review]
Bug 20750: (follow-up) Remove Time::Piece dep

The sorting of log entries was being achieved using Time::Piece::epoch
to derive the epoch date from the log timestamps. Time::Piece is not
used anywhere else, therefore it was desirable to find an alternative
method that uses a library that is already in use in Koha.

It would have been possible to use Koha::DateUtils::dt_from_string to do
this, however, this function instantiates a DateTime object to do the
conversion. DateTime objects are notoriously slow at instantiating, so
for this use case where we're potentially converting a lot, it didn't
seem ideal. Besides, dt_from_string is built to convert from a variety
of formats, but since our timestamps are in a defined format, this seems
overkill.

In the end, we're using Time::Local::timelocal to obtain the epoch time.
Time::Local is in use elsewhere and doesn't depend on DateTime.

Also updated the unit test of get_request_logs, it was using the wrong
date format anyway!
Comment 47 Andrew Isherwood 2019-01-28 16:38:31 UTC
Created attachment 84497 [details] [review]
Bug 20750: (follow-up) Fix dependency merge error

During the dependency merge the embed of status_alias in the API call
had got lost, resulting in the user defined statuses not displaying.
This patch fixes that.
Comment 48 Andrew Isherwood 2019-01-28 17:10:31 UTC
Created attachment 84498 [details] [review]
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.
Comment 49 Andrew Isherwood 2019-02-06 14:30:53 UTC
Created attachment 84821 [details] [review]
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 <Niamh.Walker-Headon@it-tallaght.ie>
Comment 50 Andrew Isherwood 2019-02-06 15:07:43 UTC
Created attachment 84823 [details] [review]
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 <Niamh.Walker-Headon@it-tallaght.ie>
Comment 51 Andrew Isherwood 2019-02-06 15:07:47 UTC
Created attachment 84824 [details] [review]
Bug 20750: Add unit tests
Comment 52 Andrew Isherwood 2019-02-06 15:07:50 UTC
Created attachment 84825 [details] [review]
Bug 20750: (follow-up) Fix QA errors

This patch fixes the "missing_filter" QA error
Comment 53 Andrew Isherwood 2019-02-06 15:07:54 UTC
Created attachment 84826 [details] [review]
Bug 20750: (follow-up) Remove Time::Piece dep

The sorting of log entries was being achieved using Time::Piece::epoch
to derive the epoch date from the log timestamps. Time::Piece is not
used anywhere else, therefore it was desirable to find an alternative
method that uses a library that is already in use in Koha.

It would have been possible to use Koha::DateUtils::dt_from_string to do
this, however, this function instantiates a DateTime object to do the
conversion. DateTime objects are notoriously slow at instantiating, so
for this use case where we're potentially converting a lot, it didn't
seem ideal. Besides, dt_from_string is built to convert from a variety
of formats, but since our timestamps are in a defined format, this seems
overkill.

In the end, we're using Time::Local::timelocal to obtain the epoch time.
Time::Local is in use elsewhere and doesn't depend on DateTime.

Also updated the unit test of get_request_logs, it was using the wrong
date format anyway!
Comment 54 Andrew Isherwood 2019-02-06 15:07:57 UTC
Created attachment 84827 [details] [review]
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.
Comment 55 Katrin Fischer 2019-02-09 22:24:47 UTC
Patch currently doesn't apply, can you please rebase?
Comment 56 Andrew Isherwood 2019-02-11 11:47:52 UTC
This needs some serious rebasing due to the changes in Bug 20581. I'll get this sorted ASAP
Comment 57 Andrew Isherwood 2019-02-11 14:43:35 UTC
Created attachment 84973 [details] [review]
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 <Niamh.Walker-Headon@it-tallaght.ie>
Comment 58 Andrew Isherwood 2019-02-11 14:43:39 UTC
Created attachment 84974 [details] [review]
Bug 20750: (follow-up) Fix sorting of log entries

The log entries are received from C4::Log::GetLogs with their timestamps
formatted as '2018-18-10 16:09:29', this doesn't sort well. In fact, it
causes a warning in the Koha logs:

Argument "2018-10-04 15:48:15" isn't numeric in numeric comparison (<=>)
at /home/koha/kohaclone/Koha/Illrequest/Logger.pm line 231

This patch converts the timestamps to be sorted into epoch dates prior
to them being compared
Comment 59 Andrew Isherwood 2019-02-11 14:43:42 UTC
Created attachment 84975 [details] [review]
Bug 20750: Add unit tests
Comment 60 Andrew Isherwood 2019-02-11 14:43:46 UTC
Created attachment 84976 [details] [review]
Bug 20750: (follow-up) Fix QA errors

This patch fixes the "missing_filter" QA error
Comment 61 Andrew Isherwood 2019-02-11 14:43:49 UTC
Created attachment 84977 [details] [review]
Bug 20750: (follow-up) Remove Time::Piece dep

The sorting of log entries was being achieved using Time::Piece::epoch
to derive the epoch date from the log timestamps. Time::Piece is not
used anywhere else, therefore it was desirable to find an alternative
method that uses a library that is already in use in Koha.

It would have been possible to use Koha::DateUtils::dt_from_string to do
this, however, this function instantiates a DateTime object to do the
conversion. DateTime objects are notoriously slow at instantiating, so
for this use case where we're potentially converting a lot, it didn't
seem ideal. Besides, dt_from_string is built to convert from a variety
of formats, but since our timestamps are in a defined format, this seems
overkill.

In the end, we're using Time::Local::timelocal to obtain the epoch time.
Time::Local is in use elsewhere and doesn't depend on DateTime.

Also updated the unit test of get_request_logs, it was using the wrong
date format anyway!
Comment 62 Andrew Isherwood 2019-02-11 14:43:53 UTC
Created attachment 84978 [details] [review]
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.
Comment 63 Andrew Isherwood 2019-02-11 14:43:57 UTC
Created attachment 84979 [details] [review]
Bug 20750: (follow-up) Fix merge problems

Since we now merging on top of Bug 20581, we need some modifications to
make sure status_alias changes are being logged and displayed correctly
Comment 64 Katrin Fischer 2019-02-14 07:18:54 UTC
Hi Andrew, getting started with this one.

1) QA script: 
 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/ill/log/status_change.tt
   FAIL	  filters
		missing_filter at line 10 (from &quot;[% display_before %]&quot;)
   FAIL	  forbidden patterns
		forbidden pattern: Do not use line breaks inside template tags (bug 18675) (line 5)
		forbidden pattern: Do not use line breaks inside template tags (bug 18675) (line 13)
   OK	  git manipulation
   OK	  js_in_body
   OK	  spelling
   OK	  tt_valid
   FAIL	  valid_template
		: filter not found

2) I am wondering a bit about the date operations in the Logger module, get_epoch  especially.

3) This doesn't need to be part of the installer outside the sysprefs.sql file. 
Files ill_logging_pref.txt and ill_logging_pref.sql can be removed. sysprefs.sql will always be run during installation, installing all prefs. 

4) I wonder if we should set the logger to off by default for updated installations and allow people to turn it on themselves.

5) For translations it's always nice to be specific. 'request' is a very general term that appears in multiple contexts. I am suggesting 'Display request log' > "ILL request log" (it being a button kind of transports the display part :) )

6) In the log viewer, could we make the visibility of the ILL parts depend on the system preference? I think this would make it a little less confusing for people without the ILL module activated.

7) Just using this for a teaching moment :) <b>Status changed </b> will be turned into Status changed with a space at the end on translations, which will do nothing in this case, but can be a bit tedious. In other cases forgetting the space will be bad for display, so translators have to always look for those.

8) Tests are failing for me:
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Illrequests.t
t/db_dependent/Illrequests.t .. 1/11 
    #   Failed test 'Status_alias getter works.'
    #   at t/db_dependent/Illrequests.t line 66.
    #          got: undef
    #     expected: 'gCXbRHL2vgImhl1lv0AsoWfYj15fmIgBiFT9i_fmMnIzh1laXf'
no query in themelanguage at /home/vagrant/kohaclone/C4/Templates.pm line 266.
no query in themelanguage at /home/vagrant/kohaclone/C4/Templates.pm line 266.
no query in themelanguage at /home/vagrant/kohaclone/C4/Templates.pm line 266.
    # Looks like you failed 1 test of 25.
t/db_dependent/Illrequests.t .. 3/11 
#   Failed test 'Basic object tests'
#   at t/db_dependent/Illrequests.t line 117.
t/db_dependent/Illrequests.t .. 5/11 no query in themelanguage at /home/vagrant/kohaclone/C4/Templates.pm line 266.
no query in themelanguage at /home/vagrant/kohaclone/C4/Templates.pm line 266.
no query in themelanguage at /home/vagrant/kohaclone/C4/Templates.pm line 266.
no query in themelanguage at /home/vagrant/kohaclone/C4/Templates.pm line 266.
no query in themelanguage at /home/vagrant/kohaclone/C4/Templates.pm line 266.
t/db_dependent/Illrequests.t .. 7/11 no query in themelanguage at /home/vagrant/kohaclone/C4/Templates.pm line 266.
no query in themelanguage at /home/vagrant/kohaclone/C4/Templates.pm line 266.
no query in themelanguage at /home/vagrant/kohaclone/C4/Templates.pm line 266.
no query in themelanguage at /home/vagrant/kohaclone/C4/Templates.pm line 266.
no query in themelanguage at /home/vagrant/kohaclone/C4/Templates.pm line 266.
no query in themelanguage at /home/vagrant/kohaclone/C4/Templates.pm line 266.
t/db_dependent/Illrequests.t .. 10/11 no query in themelanguage at /home/vagrant/kohaclone/C4/Templates.pm line 266.
no query in themelanguage at /home/vagrant/kohaclone/C4/Templates.pm line 266.
# Looks like you failed 1 test of 11.
t/db_dependent/Illrequests.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/11 subtests 

Test Summary Report
-------------------
t/db_dependent/Illrequests.t (Wstat: 256 Tests: 11 Failed: 1)
  Failed test:  3
  Non-zero exit status: 1
Comment 65 Andrew Isherwood 2019-02-14 10:25:07 UTC
Created attachment 85097 [details] [review]
Bug 20750: (follow-up) Fix QA script errors

As per item 1) here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c64
Comment 66 Andrew Isherwood 2019-02-14 10:28:01 UTC
Created attachment 85098 [details] [review]
Bug 20750: (follow-up) Remove ill_logging_pref.*

As per item 3) here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c64
Comment 67 Andrew Isherwood 2019-02-14 10:40:37 UTC
Created attachment 85099 [details] [review]
Bug 20750: (follow-up) Modify display log wording

As per item 5) here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c64
Comment 68 Andrew Isherwood 2019-02-14 10:53:21 UTC
Created attachment 85101 [details] [review]
Bug 20750: (follow-up) Remove wayward space

As per item 7) here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c64
Comment 69 Andrew Isherwood 2019-02-14 13:36:25 UTC
(In reply to Katrin Fischer from comment #64)

> 1) QA script:

Fixed

> 2) I am wondering a bit about the date operations in the Logger module, get_epoch  especially.

What's concerning you about this? All it does is get the epoch value for dates returned so we can sort them for display. It doesn't modify them in any way

> 3) This doesn't need to be part of the installer outside the sysprefs.sql file. 
> Files ill_logging_pref.txt and ill_logging_pref.sql can be removed.

Done

> 4) I wonder if we should set the logger to off by default for updated installations and allow people to turn it on themselves.

I don't know really. I guess we need to be consistent with what other logging modules do. Do they default to off or on?

> 5) For translations it's always nice to be specific.

Done

> 6) In the log viewer, could we make the visibility of the ILL parts depend on the system preference?

I agree. This is a great idea. However, looking at the log viewer code and template, there's currently no way of setting the visibility of a logging module based on a syspref. Presumably, the need has never arisen before. I'm not sure this bug is the place to add that, since it's a more generic mechanism in the log viewer, rather than just special casing ILL. As I say, it's a great idea, I'm just not sure it should be done here. Happy to do it if you disagree however.

> 7) Just using this for a teaching moment

Mmmm, good point. That will have been a typo, the space should never have been inside the <b> tag. Fixed

> 8) Tests are failing for me:

Fixed

Thanks very much for the feedback, much appreciated!
Comment 70 Andrew Isherwood 2019-02-14 13:36:53 UTC
Created attachment 85113 [details] [review]
Bug 20750: (follow-up) Remove status_alias test

As per item 8) here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c64

This test is now redundant.

We can't test the return of $req->status_alias against the value of
$req->{status_alias} any more since we've overloaded the status_alias
method and it won't always return the value of the object's property.

We test the functionality of the status_alias method elsewhere so we're
still covered.
Comment 71 Katrin Fischer 2019-02-17 09:56:59 UTC
I'd really like to get a second set of QA eyes on the code for this one.
Comment 72 Josef Moravec 2019-02-18 10:08:54 UTC
(In reply to Andrew Isherwood from comment #69)
> (In reply to Katrin Fischer from comment #64)
> 
> > 1) QA script:
> 
> Fixed
> 
> > 2) I am wondering a bit about the date operations in the Logger module, get_epoch  especially.
> 
> What's concerning you about this? All it does is get the epoch value for
> dates returned so we can sort them for display. It doesn't modify them in
> any way

I don't think the time formatting should be done here in the logger... it is helper method working with date and time... this way we could end up with one time formatter per class...

> > 3) This doesn't need to be part of the installer outside the sysprefs.sql file. 
> > Files ill_logging_pref.txt and ill_logging_pref.sql can be removed.
> 
> Done
> 
> > 4) I wonder if we should set the logger to off by default for updated installations and allow people to turn it on themselves.
> 
> I don't know really. I guess we need to be consistent with what other
> logging modules do. Do they default to off or on?

Default logs:
AuthoritiesLog IS ON
BorrowersLog IS ON
CataloguingLog IS ON
FinesLog is ON
IssueLog is ON
LetterLog is ON
ReturnLog is ON
SubscriptionLog IS ON

CronjobLog is OFF
HoldsLog is OFF
RenewalLog is OFF
ReportsLog is OFF

so 8 are ON and 4 are OFF, but I prefer to this log off by default too

> 
> > 5) For translations it's always nice to be specific.
> 
> Done
> 
> > 6) In the log viewer, could we make the visibility of the ILL parts depend on the system preference?
> 
> I agree. This is a great idea. However, looking at the log viewer code and
> template, there's currently no way of setting the visibility of a logging
> module based on a syspref. Presumably, the need has never arisen before. I'm
> not sure this bug is the place to add that, since it's a more generic
> mechanism in the log viewer, rather than just special casing ILL. As I say,
> it's a great idea, I'm just not sure it should be done here. Happy to do it
> if you disagree however.
> 
> > 7) Just using this for a teaching moment
> 
> Mmmm, good point. That will have been a typo, the space should never have
> been inside the <b> tag. Fixed
> 
> > 8) Tests are failing for me:
> 
> Fixed
> 
> Thanks very much for the feedback, much appreciated!
Comment 73 Katrin Fischer 2019-02-18 10:43:50 UTC
(In reply to Josef Moravec from comment #72)
> (In reply to Andrew Isherwood from comment #69)
> > (In reply to Katrin Fischer from comment #64)
> > 
> > > 1) QA script:
> > 
> > Fixed
> > 
> > > 2) I am wondering a bit about the date operations in the Logger module, get_epoch  especially.
> > 
> > What's concerning you about this? All it does is get the epoch value for
> > dates returned so we can sort them for display. It doesn't modify them in
> > any way
> 
> I don't think the time formatting should be done here in the logger... it is
> helper method working with date and time... this way we could end up with
> one time formatter per class...
> 
> > > 3) This doesn't need to be part of the installer outside the sysprefs.sql file. 
> > > Files ill_logging_pref.txt and ill_logging_pref.sql can be removed.
> > 
> > Done
> > 
> > > 4) I wonder if we should set the logger to off by default for updated installations and allow people to turn it on themselves.
> > 
> > I don't know really. I guess we need to be consistent with what other
> > logging modules do. Do they default to off or on?
> 
> Default logs:
> AuthoritiesLog IS ON
> BorrowersLog IS ON
> CataloguingLog IS ON
> FinesLog is ON
> IssueLog is ON
> LetterLog is ON
> ReturnLog is ON
> SubscriptionLog IS ON
> 
> CronjobLog is OFF
> HoldsLog is OFF
> RenewalLog is OFF
> ReportsLog is OFF
> 
> so 8 are ON and 4 are OFF, but I prefer to this log off by default too
> 
> > 
> > > 5) For translations it's always nice to be specific.
> > 
> > Done
> > 
> > > 6) In the log viewer, could we make the visibility of the ILL parts depend on the system preference?
> > 
> > I agree. This is a great idea. However, looking at the log viewer code and
> > template, there's currently no way of setting the visibility of a logging
> > module based on a syspref. Presumably, the need has never arisen before. I'm
> > not sure this bug is the place to add that, since it's a more generic
> > mechanism in the log viewer, rather than just special casing ILL. As I say,
> > it's a great idea, I'm just not sure it should be done here. Happy to do it
> > if you disagree however.
> > 
> > > 7) Just using this for a teaching moment
> > 
> > Mmmm, good point. That will have been a typo, the space should never have
> > been inside the <b> tag. Fixed
> > 
> > > 8) Tests are failing for me:
> > 
> > Fixed
> > 
> > Thanks very much for the feedback, much appreciated!

Thx Joseph!

I agree with default OFF, especially since the ILL module is optional. And I see a pattern there, all the default off were added after 3.2. So there is a pattern for adding new logs.

Thx!
Comment 74 Andrew Isherwood 2019-02-18 10:49:16 UTC
(In reply to Josef Moravec from comment #72)

> I don't think the time formatting should be done here in the logger... it is
> helper method working with date and time... this way we could end up with
> one time formatter per class...

Thanks for the feedback!

OK. The obvious solution here in that case would be for me to add an additional parameter to C4::Log::GetLogs that allows you to specify if the logs should be returned sorted by date. Do you agree this would address your concerns?
Comment 75 Andrew Isherwood 2019-02-18 10:56:40 UTC
Created attachment 85238 [details] [review]
Bug 20750: (follow-up) Set logging default off

As per comment #72 here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c72
Comment 76 Josef Moravec 2019-02-18 11:37:10 UTC
(In reply to Andrew Isherwood from comment #74)
> (In reply to Josef Moravec from comment #72)
> 
> > I don't think the time formatting should be done here in the logger... it is
> > helper method working with date and time... this way we could end up with
> > one time formatter per class...
> 
> Thanks for the feedback!
> 
> OK. The obvious solution here in that case would be for me to add an
> additional parameter to C4::Log::GetLogs that allows you to specify if the
> logs should be returned sorted by date. Do you agree this would address your
> concerns?

Maybe it would be better to create Koha::ActionLog[s] classes based on Koha::Object, it would be much better then adding new parameter to GetLogs
Comment 77 Andrew Isherwood 2019-02-18 12:01:28 UTC
Hi Josef

Thanks for that. Sorry, I'm a bit lost. Are you suggesting I rewrite C4::Log as a Koha object? If so, that seems a bit out of scope just to get this bug through QA. Apologies if I'm misunderstanding!
Comment 78 Josef Moravec 2019-02-18 16:26:14 UTC
(In reply to Andrew Isherwood from comment #77)
> Hi Josef
> 
> Thanks for that. Sorry, I'm a bit lost. Are you suggesting I rewrite C4::Log
> as a Koha object? If so, that seems a bit out of scope just to get this bug
> through QA. Apologies if I'm misunderstanding!

You do not need to rewrite whole C4::Log to Koha object, you just need to add Koha::ActionLog[s] objects and use them. Patch from Bug 22363 is enough for you ;)
Comment 79 Andrew Isherwood 2019-02-19 11:38:38 UTC
Created attachment 85266 [details] [review]
Bug 20750: (follow-up) Use Koha::ActionLogs

We now use Koha::ActionLogs in favour of C4::Log::GetLogs. This enables
us to deprecate Koha::Illrequest::Logger::get_epoch

This adds a dependency on Bug 22363.
Comment 80 Katrin Fischer 2019-02-24 11:41:32 UTC
Hi Andrew, could you maybe sign-off on the dependency bug 22363?
Comment 81 Andrew Isherwood 2019-02-25 08:52:09 UTC
Done :)
Comment 82 Josef Moravec 2019-02-25 18:05:48 UTC
Sorry, i got

fatal sha1 error, could you rebase please?
Comment 83 Andrew Isherwood 2019-02-26 10:08:39 UTC
Created attachment 85697 [details] [review]
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 <Niamh.Walker-Headon@it-tallaght.ie>
Comment 84 Andrew Isherwood 2019-02-26 10:08:43 UTC
Created attachment 85698 [details] [review]
Bug 20750: (follow-up) Fix sorting of log entries

The log entries are received from C4::Log::GetLogs with their timestamps
formatted as '2018-18-10 16:09:29', this doesn't sort well. In fact, it
causes a warning in the Koha logs:

Argument "2018-10-04 15:48:15" isn't numeric in numeric comparison (<=>)
at /home/koha/kohaclone/Koha/Illrequest/Logger.pm line 231

This patch converts the timestamps to be sorted into epoch dates prior
to them being compared
Comment 85 Andrew Isherwood 2019-02-26 10:08:47 UTC
Created attachment 85699 [details] [review]
Bug 20750: Add unit tests
Comment 86 Andrew Isherwood 2019-02-26 10:08:52 UTC
Created attachment 85700 [details] [review]
Bug 20750: (follow-up) Fix QA errors

This patch fixes the "missing_filter" QA error
Comment 87 Andrew Isherwood 2019-02-26 10:08:56 UTC
Created attachment 85701 [details] [review]
Bug 20750: (follow-up) Remove Time::Piece dep

The sorting of log entries was being achieved using Time::Piece::epoch
to derive the epoch date from the log timestamps. Time::Piece is not
used anywhere else, therefore it was desirable to find an alternative
method that uses a library that is already in use in Koha.

It would have been possible to use Koha::DateUtils::dt_from_string to do
this, however, this function instantiates a DateTime object to do the
conversion. DateTime objects are notoriously slow at instantiating, so
for this use case where we're potentially converting a lot, it didn't
seem ideal. Besides, dt_from_string is built to convert from a variety
of formats, but since our timestamps are in a defined format, this seems
overkill.

In the end, we're using Time::Local::timelocal to obtain the epoch time.
Time::Local is in use elsewhere and doesn't depend on DateTime.

Also updated the unit test of get_request_logs, it was using the wrong
date format anyway!
Comment 88 Andrew Isherwood 2019-02-26 10:09:01 UTC
Created attachment 85702 [details] [review]
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.
Comment 89 Andrew Isherwood 2019-02-26 10:09:06 UTC
Created attachment 85703 [details] [review]
Bug 20750: (follow-up) Fix merge problems

Since we now merging on top of Bug 20581, we need some modifications to
make sure status_alias changes are being logged and displayed correctly
Comment 90 Andrew Isherwood 2019-02-26 10:09:12 UTC
Created attachment 85704 [details] [review]
Bug 20750: (follow-up) Fix QA script errors

As per item 1) here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c64
Comment 91 Andrew Isherwood 2019-02-26 10:09:16 UTC
Created attachment 85705 [details] [review]
Bug 20750: (follow-up) Remove ill_logging_pref.*

As per item 3) here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c64
Comment 92 Andrew Isherwood 2019-02-26 10:09:20 UTC
Created attachment 85706 [details] [review]
Bug 20750: (follow-up) Modify display log wording

As per item 5) here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c64
Comment 93 Andrew Isherwood 2019-02-26 10:09:25 UTC
Created attachment 85707 [details] [review]
Bug 20750: (follow-up) Remove wayward space

As per item 7) here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c64
Comment 94 Andrew Isherwood 2019-02-26 10:09:29 UTC
Created attachment 85708 [details] [review]
Bug 20750: (follow-up) Remove status_alias test

As per item 8) here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c64

This test is now redundant.

We can't test the return of $req->status_alias against the value of
$req->{status_alias} any more since we've overloaded the status_alias
method and it won't always return the value of the object's property.

We test the functionality of the status_alias method elsewhere so we're
still covered.
Comment 95 Andrew Isherwood 2019-02-26 10:09:34 UTC
Created attachment 85709 [details] [review]
Bug 20750: (follow-up) Set logging default off

As per comment #72 here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c72
Comment 96 Andrew Isherwood 2019-02-26 10:09:39 UTC
Created attachment 85710 [details] [review]
Bug 20750: (follow-up) Use Koha::ActionLogs

We now use Koha::ActionLogs in favour of C4::Log::GetLogs. This enables
us to deprecate Koha::Illrequest::Logger::get_epoch

This adds a dependency on Bug 22363.
Comment 97 Andrew Isherwood 2019-02-26 10:11:30 UTC
Hi Josef - I've just rebased and reattached. Make sure you apply the 22363 dependency first, hopefully this bug will then apply.
Comment 98 Josef Moravec 2019-02-26 13:09:17 UTC
(In reply to Andrew Isherwood from comment #97)
> Hi Josef - I've just rebased and reattached. Make sure you apply the 22363
> dependency first, hopefully this bug will then apply.

Thanks Andrew, I just applied it cleanly, so I can now work on it ;)
Comment 99 Josef Moravec 2019-02-27 09:50:46 UTC
Comment on attachment 85697 [details] [review]
Bug 20750: Allow logging of arbitrary actions

Review of attachment 85697 [details] [review]:
-----------------------------------------------------------------

::: Koha/Illrequest/Logger.pm
@@ +110,5 @@
> +
> +sub log_status_change {
> +    my ( $self, $req, $new_status ) = @_;
> +
> +    $self->set_data({

I do not like to use 'data' variable to send data to logger. What happens when log_something is called accidentaly?

::: koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt
@@ +443,5 @@
> +                                <div class="modal-body">
> +                                [% IF request.logs.size > 0 %]
> +                                    [% FOREACH log IN request.logs %]
> +                                        [% tpl = log.template %]
> +                                        [% INCLUDE $tpl %]

I would call it with param log, to be sure what is rendered

::: koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt
@@ +54,5 @@
>  [%        CASE 'CHANGE PASS' %]Change password
>  [%        CASE 'ADDCIRCMESSAGE' %]Add circulation message
>  [%        CASE 'DELCIRCMESSAGE' %]Delete circulation message
> +[%        CASE 'STATUS_CHANGE'  %]Change ILL request status
> +[%        CASE 'BLDSS_STATUS_CHECK'  %]Check ILL request status with BLDSS

Why do you add BLDSS_STATUS_CHECK action? Koha can't be awared of all existing backends... it should instead be get from installed backends, maybe easier to do it after switch backend to regular Koha plugins?
Comment 100 Josef Moravec 2019-02-27 10:02:25 UTC
Also, when you need more than one param in method call, you should use hashref:
Koha::Illrequest::Logger
- log_maybe
- log_status_change
- get_log_tenmplate
Comment 101 Josef Moravec 2019-02-27 10:10:30 UTC
Andrew, have you some plans with this logger to the future? It is interesting idea. But the implementation is just for one module and action. It would be nice to have it a bit more generalized.
Comment 102 Andrew Isherwood 2019-02-27 10:28:54 UTC
(In reply to Josef Moravec from comment #99)
> Comment on attachment 85697 [details] [review] [review]
> Bug 20750: Allow logging of arbitrary actions
>
> ::: Koha/Illrequest/Logger.pm
> @@ +110,5 @@
> > +
> > +sub log_status_change {
> > +    my ( $self, $req, $new_status ) = @_;
> > +
> > +    $self->set_data({
> 
> I do not like to use 'data' variable to send data to logger. What happens
> when log_something is called accidentaly?

Interesting. This does seem overly convoluted, I'm sure there must have been a reason why I implemented it this way, rather than just send the data to be logged to log_something. I'll do some digging and see if I can remember. If I can't find any good reason, I'll just pass the data to log_something, rather than set it as an instance variable. Thanks! :)
Comment 103 Andrew Isherwood 2019-02-27 10:59:45 UTC
Created attachment 85756 [details] [review]
Bug 20750: (follow-up) Don't use object variable

We were unecessarily setting a property on the Logger object containing
the data to be logged, prior to calling log_something. This was
overcomplicated things and could be problematic if an unexpected call to
log_something was made. Now we just explicitly pass the data to
log_something.

As per comment #99 here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c99
Comment 104 Andrew Isherwood 2019-02-27 11:02:54 UTC
(In reply to Josef Moravec from comment #99)
> Comment on attachment 85697 [details] [review] [review]
> Bug 20750: Allow logging of arbitrary actions
> 
> Review of attachment 85697 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt
> @@ +443,5 @@
> > +                                <div class="modal-body">
> > +                                [% IF request.logs.size > 0 %]
> > +                                    [% FOREACH log IN request.logs %]
> > +                                        [% tpl = log.template %]
> > +                                        [% INCLUDE $tpl %]
> 
> I would call it with param log, to be sure what is rendered

Sorry Josef, I'm not clear what you mean, could you please elaborate?
Comment 105 Andrew Isherwood 2019-02-27 11:13:17 UTC
(In reply to Josef Moravec from comment #99)
> Comment on attachment 85697 [details] [review] [review]
> Bug 20750: Allow logging of arbitrary actions
> 
> Review of attachment 85697 [details] [review] [review]:
> -----------------------------------------------------------
> 
> ::: koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt
> @@ +54,5 @@
> >  [%        CASE 'CHANGE PASS' %]Change password
> >  [%        CASE 'ADDCIRCMESSAGE' %]Add circulation message
> >  [%        CASE 'DELCIRCMESSAGE' %]Delete circulation message
> > +[%        CASE 'STATUS_CHANGE'  %]Change ILL request status
> > +[%        CASE 'BLDSS_STATUS_CHECK'  %]Check ILL request status with BLDSS
> 
> Why do you add BLDSS_STATUS_CHECK action? Koha can't be awared of all
> existing backends... it should instead be get from installed backends, maybe
> easier to do it after switch backend to regular Koha plugins?

You're absolutely right, we shouldn't be hard coding to particular backends. Ideally we'd have some method, probably in Illrequests.pm, which something like viewlog.pl could call to poll all the available backends and see if they have anything it needs to include. That would require some thought to make sure we do it right. And I think you're right, it's probably something that would be made easier once backends are plugins.

I'm happy to remove this specific binding to the BLDSS backend here, if you agree?
Comment 106 Andrew Isherwood 2019-02-27 11:19:04 UTC
Created attachment 85757 [details] [review]
Bug 20750: (follow-up) Don't use object variable

We were unecessarily setting a property on the Logger object containing
the data to be logged, prior to calling log_something. This was
overcomplicated things and could be problematic if an unexpected call to
log_something was made. Now we just explicitly pass the data to
log_something.

As per comment #99 here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c99
Comment 107 Andrew Isherwood 2019-02-27 11:41:08 UTC
Created attachment 85765 [details] [review]
Bug 20750: (follow-up) Pass hashrefs to functions

As per comment #100 here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c100
Comment 108 Andrew Isherwood 2019-02-27 11:50:29 UTC
(In reply to Josef Moravec from comment #101)
> Andrew, have you some plans with this logger to the future? It is
> interesting idea. But the implementation is just for one module and action.
> It would be nice to have it a bit more generalized.

You're right, it is pretty specific to ILL but then it doesn't really do anything that the log viewer does already, it just does it a bit prettier. The functionality it provides was to address a request from customers relating to ILL.

It does currently only log a single action, but the instantiator can be expanded as more actions are required to be logged.

I dunno, it could be made more generic, I'm just not sure it's needed, can you see a use case where this would be required elsewhere? If it were to be made more generic, that would probably be better as separate development?
Comment 109 Josef Moravec 2019-02-27 20:24:07 UTC
(In reply to Andrew Isherwood from comment #104)
> (In reply to Josef Moravec from comment #99)
> > Comment on attachment 85697 [details] [review] [review] [review]
> > Bug 20750: Allow logging of arbitrary actions
> > 
> > Review of attachment 85697 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt
> > @@ +443,5 @@
> > > +                                <div class="modal-body">
> > > +                                [% IF request.logs.size > 0 %]
> > > +                                    [% FOREACH log IN request.logs %]
> > > +                                        [% tpl = log.template %]
> > > +                                        [% INCLUDE $tpl %]
> > 
> > I would call it with param log, to be sure what is rendered
> 
> Sorry Josef, I'm not clear what you mean, could you please elaborate?

I think somethink like this:
[% INCLUDE $tpl log=log %]

then in the template you always know the "log" variable is what you need, and when you need other name in parents, that's no problem then.
Comment 110 Josef Moravec 2019-02-27 20:26:13 UTC
> > >  [%        CASE 'CHANGE PASS' %]Change password
> > >  [%        CASE 'ADDCIRCMESSAGE' %]Add circulation message
> > >  [%        CASE 'DELCIRCMESSAGE' %]Delete circulation message
> > > +[%        CASE 'STATUS_CHANGE'  %]Change ILL request status
> > > +[%        CASE 'BLDSS_STATUS_CHECK'  %]Check ILL request status with BLDSS
> > 
> > Why do you add BLDSS_STATUS_CHECK action? Koha can't be awared of all
> > existing backends... it should instead be get from installed backends, maybe
> > easier to do it after switch backend to regular Koha plugins?
> 
> You're absolutely right, we shouldn't be hard coding to particular backends.
> Ideally we'd have some method, probably in Illrequests.pm, which something
> like viewlog.pl could call to poll all the available backends and see if
> they have anything it needs to include. That would require some thought to
> make sure we do it right. And I think you're right, it's probably something
> that would be made easier once backends are plugins.
> 
> I'm happy to remove this specific binding to the BLDSS backend here, if you
> agree?

Yes remove it, we could deal with this on another report.
Comment 111 Josef Moravec 2019-02-27 20:28:07 UTC
(In reply to Andrew Isherwood from comment #108)
> (In reply to Josef Moravec from comment #101)
> > Andrew, have you some plans with this logger to the future? It is
> > interesting idea. But the implementation is just for one module and action.
> > It would be nice to have it a bit more generalized.
> 
> You're right, it is pretty specific to ILL but then it doesn't really do
> anything that the log viewer does already, it just does it a bit prettier.
> The functionality it provides was to address a request from customers
> relating to ILL.
> 
> It does currently only log a single action, but the instantiator can be
> expanded as more actions are required to be logged.
> 
> I dunno, it could be made more generic, I'm just not sure it's needed, can
> you see a use case where this would be required elsewhere? If it were to be
> made more generic, that would probably be better as separate development?

It would be definitely a separate work. I would like to see this pushed as soon as possible ;)
Comment 112 Andrew Isherwood 2019-02-28 10:37:27 UTC
Created attachment 85826 [details] [review]
Bug 20750: (follow-up) Explicitly pass log var

As per comment #109 here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c109
Comment 113 Andrew Isherwood 2019-02-28 10:40:05 UTC
Created attachment 85827 [details] [review]
Bug 20750: (follow-up) Remove BLDSS_STATUS_CHECK

As per comment #110 here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c110
Comment 114 Josef Moravec 2019-02-28 11:20:27 UTC
Created attachment 85829 [details] [review]
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 <Niamh.Walker-Headon@it-tallaght.ie>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 115 Josef Moravec 2019-02-28 11:20:32 UTC
Created attachment 85830 [details] [review]
Bug 20750: (follow-up) Fix sorting of log entries

The log entries are received from C4::Log::GetLogs with their timestamps
formatted as '2018-18-10 16:09:29', this doesn't sort well. In fact, it
causes a warning in the Koha logs:

Argument "2018-10-04 15:48:15" isn't numeric in numeric comparison (<=>)
at /home/koha/kohaclone/Koha/Illrequest/Logger.pm line 231

This patch converts the timestamps to be sorted into epoch dates prior
to them being compared

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 116 Josef Moravec 2019-02-28 11:20:37 UTC
Created attachment 85831 [details] [review]
Bug 20750: Add unit tests

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 117 Josef Moravec 2019-02-28 11:20:42 UTC
Created attachment 85832 [details] [review]
Bug 20750: (follow-up) Fix QA errors

This patch fixes the "missing_filter" QA error

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 118 Josef Moravec 2019-02-28 11:20:47 UTC
Created attachment 85833 [details] [review]
Bug 20750: (follow-up) Remove Time::Piece dep

The sorting of log entries was being achieved using Time::Piece::epoch
to derive the epoch date from the log timestamps. Time::Piece is not
used anywhere else, therefore it was desirable to find an alternative
method that uses a library that is already in use in Koha.

It would have been possible to use Koha::DateUtils::dt_from_string to do
this, however, this function instantiates a DateTime object to do the
conversion. DateTime objects are notoriously slow at instantiating, so
for this use case where we're potentially converting a lot, it didn't
seem ideal. Besides, dt_from_string is built to convert from a variety
of formats, but since our timestamps are in a defined format, this seems
overkill.

In the end, we're using Time::Local::timelocal to obtain the epoch time.
Time::Local is in use elsewhere and doesn't depend on DateTime.

Also updated the unit test of get_request_logs, it was using the wrong
date format anyway!

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 119 Josef Moravec 2019-02-28 11:20:51 UTC
Created attachment 85834 [details] [review]
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.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 120 Josef Moravec 2019-02-28 11:20:56 UTC
Created attachment 85835 [details] [review]
Bug 20750: (follow-up) Fix merge problems

Since we now merging on top of Bug 20581, we need some modifications to
make sure status_alias changes are being logged and displayed correctly

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 121 Josef Moravec 2019-02-28 11:21:00 UTC
Created attachment 85836 [details] [review]
Bug 20750: (follow-up) Fix QA script errors

As per item 1) here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c64

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 122 Josef Moravec 2019-02-28 11:21:06 UTC
Created attachment 85837 [details] [review]
Bug 20750: (follow-up) Remove ill_logging_pref.*

As per item 3) here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c64

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 123 Josef Moravec 2019-02-28 11:21:11 UTC
Created attachment 85838 [details] [review]
Bug 20750: (follow-up) Modify display log wording

As per item 5) here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c64

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 124 Josef Moravec 2019-02-28 11:21:15 UTC
Created attachment 85839 [details] [review]
Bug 20750: (follow-up) Remove wayward space

As per item 7) here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c64

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 125 Josef Moravec 2019-02-28 11:21:20 UTC
Created attachment 85840 [details] [review]
Bug 20750: (follow-up) Remove status_alias test

As per item 8) here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c64

This test is now redundant.

We can't test the return of $req->status_alias against the value of
$req->{status_alias} any more since we've overloaded the status_alias
method and it won't always return the value of the object's property.

We test the functionality of the status_alias method elsewhere so we're
still covered.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 126 Josef Moravec 2019-02-28 11:21:24 UTC
Created attachment 85841 [details] [review]
Bug 20750: (follow-up) Set logging default off

As per comment #72 here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c72

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 127 Josef Moravec 2019-02-28 11:21:29 UTC
Created attachment 85842 [details] [review]
Bug 20750: (follow-up) Use Koha::ActionLogs

We now use Koha::ActionLogs in favour of C4::Log::GetLogs. This enables
us to deprecate Koha::Illrequest::Logger::get_epoch

This adds a dependency on Bug 22363.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 128 Josef Moravec 2019-02-28 11:21:33 UTC
Created attachment 85843 [details] [review]
Bug 20750: (follow-up) Don't use object variable

We were unecessarily setting a property on the Logger object containing
the data to be logged, prior to calling log_something. This was
overcomplicated things and could be problematic if an unexpected call to
log_something was made. Now we just explicitly pass the data to
log_something.

As per comment #99 here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c99

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 129 Josef Moravec 2019-02-28 11:21:38 UTC
Created attachment 85844 [details] [review]
Bug 20750: (follow-up) Pass hashrefs to functions

As per comment #100 here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c100

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 130 Josef Moravec 2019-02-28 11:21:43 UTC
Created attachment 85845 [details] [review]
Bug 20750: (follow-up) Explicitly pass log var

As per comment #109 here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c109

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 131 Josef Moravec 2019-02-28 11:21:48 UTC
Created attachment 85846 [details] [review]
Bug 20750: (follow-up) Remove BLDSS_STATUS_CHECK

As per comment #110 here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20750#c110

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 132 Josef Moravec 2019-02-28 11:21:53 UTC
Created attachment 85847 [details] [review]
Bug 20750: (QA follow-up) Add ILL status change option to actions in log viewer

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 133 Josef Moravec 2019-02-28 11:21:57 UTC
Created attachment 85848 [details] [review]
Bug 20750: (QA follow-up) Add use KohaDates to status change template

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 134 Josef Moravec 2019-02-28 11:28:38 UTC
Hope we cought everything ;)
Comment 135 Josef Moravec 2019-03-11 12:22:51 UTC
Created attachment 86434 [details] [review]
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 <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.
Comment 136 Josef Moravec 2019-03-11 12:22:56 UTC
Created attachment 86435 [details] [review]
Bug 20750: Add unit tests

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

Bug 20750: (follow-up) Remove status_alias test

This test is now redundant.

We can't test the return of $req->status_alias against the value of
$req->{status_alias} any more since we've overloaded the status_alias
method and it won't always return the value of the object's property.

We test the functionality of the status_alias method elsewhere so we're
still covered.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 137 Josef Moravec 2019-03-11 12:23:00 UTC
Created attachment 86436 [details] [review]
Bug 20750: (QA follow-up) Fix templates

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 138 Josef Moravec 2019-03-11 12:31:02 UTC
Setting back to signed off, would be nice to have another QA team member look.

Katrin? ;)
Comment 139 Kyle M Hall 2019-03-12 13:30:48 UTC
Created attachment 86534 [details] [review]
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 <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>
Comment 140 Kyle M Hall 2019-03-12 13:31:56 UTC
Created attachment 86535 [details] [review]
Bug 20750: Add unit tests

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

Bug 20750: (follow-up) Remove status_alias test

This test is now redundant.

We can't test the return of $req->status_alias against the value of
$req->{status_alias} any more since we've overloaded the status_alias
method and it won't always return the value of the object's property.

We test the functionality of the status_alias method elsewhere so we're
still covered.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 141 Kyle M Hall 2019-03-12 13:32:00 UTC
Created attachment 86536 [details] [review]
Bug 20750: (QA follow-up) Fix templates

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 142 Nick Clemens 2019-03-15 19:57:57 UTC
Awesome work all!

Pushed to master for 19.05
Comment 143 Martin Renvoize 2019-03-19 10:43:54 UTC
Enhancement, will not be backported to 18.11.x series.