Bug 14642

Summary: Add logging of hold modifications
Product: Koha Reporter: Kyle M Hall <kyle.m.hall>
Component: Hold requestsAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: amy, colin.campbell, gmcharlt, gwilliams, jonathan.druart, jrobb, katrin.fischer, kyle, nicole, sally.healey, tomascohen, veron, wizzyrea
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18382
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18445
Change sponsored?: Sponsored Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 16819, 16829    
Bug Blocks: 24547    
Attachments: Bug 14642: Add HoldsLog syspref
Bug 14642: Add logging for Holds
Bug 14642: Add logging for Holds
Bug 14642: Log on ::delete
Bug 14642: Add logging for Holds
Bug 14642: Add logging for Holds
[SIGNED-OFF] Bug 14642: Add logging for Holds
Bug 14642: (QA followup) Make HoldsLog default to 0
Bug 14642: (QA followup) Add tests for Koha::Hold->delete
Bug 14642: Add HoldsLog syspref
Bug 14642: Add logging for Holds
Bug 14642: (QA followup) Make HoldsLog default to 0
Bug 14642: (QA followup) Add tests for Koha::Hold->delete
Bug 14642: Make sure the log will be written only if the hold has been deleted

Description Kyle M Hall 2015-08-04 12:57:20 UTC
I think it's high time we added a HoldsLog system preference to log creation, updating, and filling of holds.

I would say the following is needed:
1) Hold creation ( staff/opac, bib, item, date, priority )
2) All hold modifications ( changing of priorities except for reflowing priorities, trapping an item to a hold, deleting a hold, etc )
3) Hold fulfillment
Comment 1 Katrin Fischer 2015-08-04 22:47:16 UTC
I'd add suspending/unsuspending to the list :)
Comment 2 HB-NEKLS 2015-08-25 19:50:54 UTC
I agree -- suspending/unsuspending. 

I think this is a really good start to help troubleshoot better when something with holds go awry, esp in high-hold and/or multi-branch environments. I'm sure there's more things to add, but this is a great start. 

Thanks Kyle!
Comment 3 Jason Robb 2015-10-23 21:25:04 UTC
I want this!

I don't see it explicitly mentioned, will borrowernumbers for both the hold placer/modifier and the hold placee(who it's for) be recorded?

I know logs usually track "Librarian"(user) and "Object"(borrower), so will this track the patron as both the "Librarian" and the "Object" if they originate the hold in the OPAC?

Also, this sounds very similar to bug 6890.
Comment 4 Nicole C. Engard 2016-06-06 11:33:14 UTC
*** Bug 15022 has been marked as a duplicate of this bug. ***
Comment 5 Tomás Cohen Arazi 2016-06-24 15:59:31 UTC Comment hidden (obsolete)
Comment 6 Tomás Cohen Arazi 2016-06-24 15:59:40 UTC Comment hidden (obsolete)
Comment 7 Tomás Cohen Arazi 2016-06-24 16:01:00 UTC
This is an initial submission to get some feedback from other people.
It is still missing priority changes, fullfilment, etc.

Please contact me if you have any advice or ideas of what to log.
Comment 8 Tomás Cohen Arazi 2016-06-24 17:22:04 UTC Comment hidden (obsolete)
Comment 9 Colin Campbell 2016-06-28 09:16:09 UTC
One thing that would be useful is to log if its an opac hold or staff generated hold. In the UK public libraries standard reporting includes a requirement to report number of holds placed via opac
Comment 10 Tomás Cohen Arazi 2016-06-28 11:02:50 UTC
(In reply to Colin Campbell from comment #9)
> One thing that would be useful is to log if its an opac hold or staff
> generated hold. In the UK public libraries standard reporting includes a
> requirement to report number of holds placed via opac

Ack
Comment 11 Tomás Cohen Arazi 2016-06-28 14:27:37 UTC Comment hidden (obsolete)
Comment 12 Tomás Cohen Arazi 2016-07-04 01:56:36 UTC
(In reply to Colin Campbell from comment #9)
> One thing that would be useful is to log if its an opac hold or staff
> generated hold. In the UK public libraries standard reporting includes a
> requirement to report number of holds placed via opac

Colin, please take a look at bug 16829. And sign it :-D One thing I'd add (on another bugs, will try to get funding for it) is that SCO should be a separate interface from OPAC, and also we should make cli scripts explicitly set 'commandline' if they still don't.
Comment 13 Tomás Cohen Arazi 2016-07-04 02:08:45 UTC Comment hidden (obsolete)
Comment 14 Tomás Cohen Arazi 2016-07-07 19:09:02 UTC Comment hidden (obsolete)
Comment 15 Marc Véron 2016-07-08 08:03:04 UTC
Logging holds is enabled by default. Is that intended?
Comment 16 Liz Rea 2016-07-18 04:10:54 UTC Comment hidden (obsolete)
Comment 17 Tomás Cohen Arazi 2016-07-26 19:01:45 UTC
Comment on attachment 52824 [details] [review]
Bug 14642: Add HoldsLog syspref

>From 133621157e8fe90cd4728970c23a4869d3212f43 Mon Sep 17 00:00:00 2001
>From: Tomas Cohen Arazi <tomascohen@theke.io>
>Date: Fri, 24 Jun 2016 11:34:01 -0300
>Subject: [PATCH] Bug 14642: Add HoldsLog syspref
>
>This patch introduces a new syspref 'HoldsLog' which is to be used for controlling
>holds actions logs.
>
>Sponsored-by: NEKLS
>---
> .../data/mysql/atomicupdate/bug_14642_holds_logging.sql     | 13 +++++++++++++
> installer/data/mysql/sysprefs.sql                           |  1 +
> .../prog/en/modules/admin/preferences/logs.pref             |  6 ++++++
> 3 files changed, 20 insertions(+)
> create mode 100644 installer/data/mysql/atomicupdate/bug_14642_holds_logging.sql
>
>diff --git a/installer/data/mysql/atomicupdate/bug_14642_holds_logging.sql b/installer/data/mysql/atomicupdate/bug_14642_holds_logging.sql
>new file mode 100644
>index 0000000..aca2ee3
>--- /dev/null
>+++ b/installer/data/mysql/atomicupdate/bug_14642_holds_logging.sql
>@@ -0,0 +1,13 @@
>+INSERT IGNORE INTO systempreferences ( variable, value, options, explanation, type ) VALUES
>+('HoldsLog','1',NULL,'If ON, log create/cancel/suspend/resume actions on holds.','YesNo');
>+
>+-- $DBversion = "16.06.00.XXX";
>+-- if ( CheckVersion($DBversion) ) {
>+--     $dbh->do(q{
>+--         INSERT IGNORE INTO systempreferences ( variable, value, options, explanation, type ) VALUES
>+--         ('HoldsLog','1',NULL,'If ON, log create/cancel/suspend/resume actions on holds.','YesNo');
>+--     });
>+
>+--     print "Upgrade to $DBversion done (Bug 14642: Add logging of hold modifications)\n";
>+--     SetVersion($DBversion);
>+-- }
>diff --git a/installer/data/mysql/sysprefs.sql b/installer/data/mysql/sysprefs.sql
>index cd0f8bf..099db56 100644
>--- a/installer/data/mysql/sysprefs.sql
>+++ b/installer/data/mysql/sysprefs.sql
>@@ -167,6 +167,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `
> ('HighlightOwnItemsOnOPAC','0','','If on, and a patron is logged into the OPAC, items from his or her home library will be emphasized and shown first in search results and item details.','YesNo'),
> ('HighlightOwnItemsOnOPACWhich','PatronBranch','PatronBranch|OpacURLBranch','Decides which branch\'s items to emphasize. If PatronBranch, emphasize the logged in user\'s library\'s items. If OpacURLBranch, highlight the items of the Apache var BRANCHCODE defined in Koha\'s Apache configuration file.','Choice'),
> ('HoldFeeMode','not_always','always|not_always','Set the hold fee mode','Choice'),
>+('HoldsLog','1',NULL,'If ON, log create/cancel/suspend/resume actions on holds.','YesNo'),
> ('HoldsQueueSkipClosed', '0', NULL, 'If enabled, any libraries that are closed when the holds queue is built will be ignored for the purpose of filling holds.', 'YesNo'),
> ('HoldsToPullStartDate','2',NULL,'Set the default start date for the Holds to pull list to this many days ago','Integer'),
> ('HomeOrHoldingBranch','holdingbranch','holdingbranch|homebranch','Used by Circulation to determine which branch of an item to check with independent branches on, and by search to determine which branch to choose for availability ','Choice'),
>diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/logs.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/logs.pref
>index 8d6c4c8..4aa5790 100644
>--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/logs.pref
>+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/logs.pref
>@@ -31,6 +31,12 @@ Logging:
>                   off: "Don't log"
>             - when fines are charged, paid, or forgiven.
>         -
>+            - pref: HoldsLog
>+              choices:
>+                  on: Log
>+                  off: "Don't log"
>+            - any actions on holds (create, cancel, suspend, resume, etc).
>+        -
>             - pref: IssueLog
>               choices:
>                   on: Log
>-- 
>2.9.0
Comment 18 Tomás Cohen Arazi 2016-07-26 19:02:24 UTC
I'm not sure why the patch that introduces the syspref got obsoleted! Reverting it
Comment 19 Tomás Cohen Arazi 2016-07-26 19:06:27 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2016-08-01 14:24:17 UTC
Tomas, regarding the Koha::Hold->delete method, I think you must provide tests.
Have a look the first patch of bug 16907, I have overwritten Koha::Patron->delete which log as well and provided relevant tests.
Comment 21 Tomás Cohen Arazi 2016-08-01 14:25:05 UTC
(In reply to Jonathan Druart from comment #20)
> Tomas, regarding the Koha::Hold->delete method, I think you must provide
> tests.
> Have a look the first patch of bug 16907, I have overwritten
> Koha::Patron->delete which log as well and provided relevant tests.

Will do!
Comment 22 Tomás Cohen Arazi 2016-08-09 02:47:33 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2016-08-09 09:15:42 UTC
Created attachment 54179 [details] [review]
Bug 14642: Add HoldsLog syspref

This patch introduces a new syspref 'HoldsLog' which is to be used for controlling
holds actions logs.

Sponsored-by: NEKLS

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 24 Jonathan Druart 2016-08-09 09:15:45 UTC
Created attachment 54180 [details] [review]
Bug 14642: Add logging for Holds

This patch adds logging for several holds actions. Specifically for:

- CREATE
- CANCEL
- DELETE
- RESUME
- SUSPEND
- MODIFY

To test:
- Enable the HoldsLog syspref
- Add a hold on a record/item
=> SUCCESS: The log view shows the CREATE action
- Click on the <Suspend> button
=> SUCCESS: The log view shows the SUSPEND action
- Click on the <Unsuspend> button
=> SUCCESS: The log view shows the RESUME action
- Click on the red cross, to delete the hold
=> SUCCESS: The log view shows the CANCEL action

Note: The DELETE action is logged when DelMember is called, with bug 16819 patches applied.

Sponsored-by: NEKLS
Signed-off-by: Liz Rea <liz@catalyst.net.nz>
I also wonder about this going in defaulted on, but since the other logs are as well it seems ok to me.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 25 Jonathan Druart 2016-08-09 09:15:50 UTC
Created attachment 54181 [details] [review]
Bug 14642: (QA followup) Make HoldsLog default to 0

After a couple comments regarding the default value I picked for the
new syspref, I've decided to set it 0, as libraries interested on the
fetarue will just enable it.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 26 Jonathan Druart 2016-08-09 09:15:56 UTC
Created attachment 54182 [details] [review]
Bug 14642: (QA followup) Add tests for Koha::Hold->delete

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 27 Jonathan Druart 2016-08-09 09:16:00 UTC
Created attachment 54183 [details] [review]
Bug 14642: Make sure the log will be written only if the hold has been deleted

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Kyle M Hall 2016-08-17 18:43:57 UTC
Pushed to master for 16.11, thanks Tomas, Jonathan!
Comment 29 Tomás Cohen Arazi 2016-09-21 10:51:20 UTC
*** Bug 6890 has been marked as a duplicate of this bug. ***
Comment 30 Kyle M Hall 2020-01-28 14:10:23 UTC
*** Bug 24472 has been marked as a duplicate of this bug. ***