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.
Discussed with joubu on IRC, suggestion was to use actions_logs rather than bake our own recording mechanism
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!
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
Created attachment 75304 [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 - 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.
Looks like the file extension on the installer/data/mysql/atomicupdate/bug_20750-add_illlog_preference.sql file is wrong. It includes Perl code, so it should be .perl, not .sql: https://wiki.koha-community.org/wiki/Database_updates#How_to_write_an_atomicupdate_file
The test plan does not mention the need to turn IllLog on.
ILL is not in the list of available modules in the "Browse system logs" tool. It is of course possible to view the log related to a single ILL request, but it might also be interesting to use "Browse system logs" to see all ILL actions in a given period of time.
Thanks very much for that feedback, very useful indeed. I'm about to push an updated patch that addresses all these points.
Created attachment 75326 [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.
The latest patch touches Koha/Schema/Result/SocialData.pm. Is that supposed to happen?
Created attachment 75329 [details] [review] Squashed commit for ILL enhancements This squashed commit contains a number of ILL enhancements. It has been rebased on top of 17.11.x. Contains the following enhancements: - Bug 20041 - ILL module missing from more menu in staff when activated (Already present in 17.11.x) - Bug 20563 - ILL request list gives no indication of source and/or target - Bug 20651 - Improve display of "Toggle full supplier metadata" - Bug 20515 - "ILL Request" menu options displayed when user has no ILL permissions - Bug 20646 - "ILL Request" menu options not displayed when user has superlibrarian permission Bug 20548 - Remove copyright clearance workflow from staff created ILL requests - Bug 20284 - ILL: Adding a 'new request' from OPAC fails with template error if text exists in ILLModuleCopyrightClearance - Bug 20556 - Marking ILL request as complete results in "Internal server error" - Bug 20536 - ILL: authnotrequired not explicitly unset - Bug 20640 - Allow migrating a request between backends - Bug 20581 - Allow manual selection of custom ILL request statuses - Bug 20600 - Provide the ability for users to filter ILL requests - Bug 18837 - Add an unmediated Interlibrary Loans workflow - Bug 20639 - Allow setting a default/single backend for OPAC driven requests - Bug 18589 - Show ILLs as part of patron profile - Bug 20750 - Add audit trail of request actions. Currently change of request status
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.
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...
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?
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 :)
(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!
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?
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?
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.
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.
Created attachment 77817 [details] [review] Signed-off-by: Niamh Walker <Niamh.Walker-Headon@it-tallaght.ie>
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>
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>
Just had to sort out some randomness with our sandboxes.. that final patch actually includes the signoff line this time..
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.
Created attachment 79823 [details] [review] Bug 20750: Add unit tests
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>
*** Bug 21461 has been marked as a duplicate of this bug. ***
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
Created attachment 82082 [details] [review] Bug 20750: Add unit tests
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>
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
Rebased on top of master then the current dependency tree
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>
Created attachment 82097 [details] [review] Bug 20750: Add unit tests
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>
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
Created attachment 82500 [details] [review] Bug 20750: Add unit tests
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>
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
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>
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
Created attachment 84195 [details] [review] Bug 20750: Add unit tests
Rebased following changes to C4/Log.pm & t/Log.t
Created attachment 84203 [details] [review] Bug 20750: (follow-up) Fix QA errors This patch fixes the "missing_filter" QA error
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!
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.
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.
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>
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>
Created attachment 84824 [details] [review] Bug 20750: Add unit tests
Created attachment 84825 [details] [review] Bug 20750: (follow-up) Fix QA errors This patch fixes the "missing_filter" QA error
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!
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.
Patch currently doesn't apply, can you please rebase?
This needs some serious rebasing due to the changes in Bug 20581. I'll get this sorted ASAP
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>
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
Created attachment 84975 [details] [review] Bug 20750: Add unit tests
Created attachment 84976 [details] [review] Bug 20750: (follow-up) Fix QA errors This patch fixes the "missing_filter" QA error
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!
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.
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
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 "[% display_before %]") 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
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
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
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
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
(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!
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.
I'd really like to get a second set of QA eyes on the code for this one.
(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!
(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!
(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?
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
(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
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!
(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 ;)
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.
Hi Andrew, could you maybe sign-off on the dependency bug 22363?
Done :)
Sorry, i got fatal sha1 error, could you rebase please?
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>
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
Created attachment 85699 [details] [review] Bug 20750: Add unit tests
Created attachment 85700 [details] [review] Bug 20750: (follow-up) Fix QA errors This patch fixes the "missing_filter" QA error
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!
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.
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
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
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
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
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
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.
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
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.
Hi Josef - I've just rebased and reattached. Make sure you apply the 22363 dependency first, hopefully this bug will then apply.
(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 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?
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
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.
(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! :)
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
(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?
(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?
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
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
(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?
(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.
> > > [% 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.
(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 ;)
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
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
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>
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>
Created attachment 85831 [details] [review] Bug 20750: Add unit tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Hope we cought everything ;)
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.
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>
Created attachment 86436 [details] [review] Bug 20750: (QA follow-up) Fix templates Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Setting back to signed off, would be nice to have another QA team member look. Katrin? ;)
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>
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>
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>
Awesome work all! Pushed to master for 19.05
Enhancement, will not be backported to 18.11.x series.
The output of the tests contains warnings: kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Illrequests.t t/db_dependent/Illrequests.t .. 1/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. t/db_dependent/Illrequests.t .. 4/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. t/db_dependent/Illrequests.t .. ok All tests successful. Files=1, Tests=11, 6 wallclock secs ( 0.04 usr 0.00 sys + 4.94 cusr 1.16 csys = 6.14 CPU) Result: PASS