Bug 14642 - Add logging of hold modifications
Summary: Add logging of hold modifications
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
: 6890 15022 24472 (view as bug list)
Depends on: 16819 16829
Blocks: 24547
  Show dependency treegraph
 
Reported: 2015-08-04 12:57 UTC by Kyle M Hall
Modified: 2020-04-08 12:32 UTC (History)
13 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 14642: Add HoldsLog syspref (3.77 KB, patch)
2016-06-24 15:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14642: Add logging for Holds (6.12 KB, patch)
2016-06-24 15:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14642: Add logging for Holds (6.58 KB, patch)
2016-06-24 17:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14642: Log on ::delete (801 bytes, patch)
2016-06-28 14:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14642: Add logging for Holds (6.93 KB, patch)
2016-07-04 02:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14642: Add logging for Holds (7.25 KB, patch)
2016-07-07 19:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14642: Add logging for Holds (7.41 KB, patch)
2016-07-18 04:10 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 14642: (QA followup) Make HoldsLog default to 0 (3.30 KB, patch)
2016-07-26 19:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14642: (QA followup) Add tests for Koha::Hold->delete (2.59 KB, patch)
2016-08-09 02:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14642: Add HoldsLog syspref (3.84 KB, patch)
2016-08-09 09:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14642: Add logging for Holds (7.47 KB, patch)
2016-08-09 09:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14642: (QA followup) Make HoldsLog default to 0 (3.37 KB, patch)
2016-08-09 09:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14642: (QA followup) Add tests for Koha::Hold->delete (2.66 KB, patch)
2016-08-09 09:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14642: Make sure the log will be written only if the hold has been deleted (891 bytes, patch)
2016-08-09 09:16 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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. ***