Bug 18816 - Make CataloguingLog work in production by preventing circulation from spamming the log
Summary: Make CataloguingLog work in production by preventing circulation from spammin...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
: 17195 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-06-16 12:59 UTC by Kyle M Hall
Modified: 2018-10-17 16:54 UTC (History)
10 users (show)

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


Attachments
Bug 18816 - Make CataloguingLog work in production by preventing circulation from spamming the log (5.30 KB, patch)
2017-06-16 13:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18816 - Make CataloguingLog work in production by preventing circulation from spamming the log (7.40 KB, patch)
2017-06-16 14:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18816 - Make CataloguingLog work in production by preventing circulation from spamming the log (7.15 KB, patch)
2017-08-09 13:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18816 - Make CataloguingLog work in production by preventing circulation from spamming the log (7.46 KB, patch)
2017-08-09 13:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18816 - Make CataloguingLog work in production by preventing circulation from spamming the log (8.15 KB, patch)
2017-10-26 12:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18816 - Make CataloguingLog work in production by preventing circulation from spamming the log (8.68 KB, patch)
2017-10-27 00:10 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18816: Make CataloguingLog work in production by preventing circulation from spamming the log (8.88 KB, patch)
2018-03-22 10:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18816 [QA Followup]: Convert param to hashref, fix typo (5.69 KB, patch)
2018-03-22 10:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18816: Add rarely used params to hashref, remove param (2.76 KB, patch)
2018-03-23 11:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18816 [QA Followup]: Add rarely used params to hashref, remove param (2.77 KB, patch)
2018-03-23 11:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18816 [QA Followup]: Add rarely used params to hashref, remove param (2.80 KB, patch)
2018-03-23 11:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18816: Make CataloguingLog work in production by preventing circulation from spamming the log (8.95 KB, patch)
2018-04-06 06:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18816 [QA Followup]: Convert param to hashref, fix typo (5.75 KB, patch)
2018-04-06 06:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18816 [QA Followup]: Add rarely used params to hashref, remove param (2.87 KB, patch)
2018-04-06 06:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18816: (QA follow-up) Framework parameter is not needed (2.73 KB, patch)
2018-04-06 06:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18816: (QA follow-up) Fix Circulation.t (1.32 KB, patch)
2018-04-06 06:11 UTC, Marcel de Rooy
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 2017-06-16 12:59:22 UTC
The system preference CataloguingLog is not recommended for use in production. This is do to the fact that every checkin and checkout generates one or more log entires. This seems to be not only bad behavior, but unnecessary and outside the needs of CataloguingLog as we have CirculationLog.
Comment 1 Kyle M Hall 2017-06-16 13:01:27 UTC
Created attachment 64383 [details] [review]
Bug 18816 - Make CataloguingLog work in production by preventing circulation from spamming the log

The system preference CataloguingLog is not recommended for use in
production. This is do to the fact that every checkin and checkout
generates one or more log entires. This seems to be not only bad
behavior, but unnecessary and outside the needs of CataloguingLog as we
have CirculationLog.

Test Plan:
1) Enable CataloguingLog
2) Note the checkins and checkouts cause log entries
3) Apply this patch
4) Note that checkins and checkouts no longer cause log entries,
   but editing the item will continue to cause log entries.
Comment 2 Kyle M Hall 2017-06-16 14:24:37 UTC
Created attachment 64393 [details] [review]
Bug 18816 - Make CataloguingLog work in production by preventing circulation from spamming the log

The system preference CataloguingLog is not recommended for use in
production. This is do to the fact that every checkin and checkout
generates one or more log entires. This seems to be not only bad
behavior, but unnecessary and outside the needs of CataloguingLog as we
have CirculationLog.

Test Plan:
1) Enable CataloguingLog
2) Note the checkins and checkouts cause log entries
3) Apply this patch
4) Note that checkins and checkouts no longer cause log entries,
   but editing the item will continue to cause log entries.
Comment 3 M. Tompsett 2017-06-29 18:20:48 UTC Comment hidden (obsolete)
Comment 4 M. Tompsett 2017-06-29 18:21:45 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2017-08-09 13:40:04 UTC
Created attachment 65711 [details] [review]
Bug 18816 - Make CataloguingLog work in production by preventing circulation from spamming the log

The system preference CataloguingLog is not recommended for use in
production. This is do to the fact that every checkin and checkout
generates one or more log entires. This seems to be not only bad
behavior, but unnecessary and outside the needs of CataloguingLog as we
have CirculationLog.

Test Plan:
1) Enable CataloguingLog
2) Note the checkins and checkouts cause log entries
3) Apply this patch
4) Note that checkins and checkouts no longer cause log entries,
   but editing the item will continue to cause log entries.
Comment 6 Kyle M Hall 2017-08-09 13:41:59 UTC
Created attachment 65712 [details] [review]
Bug 18816 - Make CataloguingLog work in production by preventing circulation from spamming the log

The system preference CataloguingLog is not recommended for use in
production. This is do to the fact that every checkin and checkout
generates one or more log entires. This seems to be not only bad
behavior, but unnecessary and outside the needs of CataloguingLog as we
have CirculationLog.

Test Plan:
1) Enable CataloguingLog
2) Note the checkins and checkouts cause log entries
3) Apply this patch
4) Note that checkins and checkouts no longer cause log entries,
   but editing the item will continue to cause log entries.
Comment 7 Lisette Scheer 2017-08-11 22:00:18 UTC
We applied the patch cleanly but the desired change in behavior did not happen. 
Tested with check in and check out.
We were looking at the logs for the particular item.
Comment 8 Kyle M Hall 2017-10-26 12:47:10 UTC
Created attachment 68600 [details] [review]
Bug 18816 - Make CataloguingLog work in production by preventing circulation from spamming the log

The system preference CataloguingLog is not recommended for use in
production. This is do to the fact that every checkin and checkout
generates one or more log entires. This seems to be not only bad
behavior, but unnecessary and outside the needs of CataloguingLog as we
have CirculationLog.

Test Plan:
1) Enable CataloguingLog
2) Note the checkins and checkouts cause log entries
3) Apply this patch
4) Note that checkins and checkouts no longer cause log entries,
   but editing the item will continue to cause log entries.
Comment 9 Kyle M Hall 2017-10-26 12:47:32 UTC
(In reply to Lisette from comment #7)
> We applied the patch cleanly but the desired change in behavior did not
> happen. 
> Tested with check in and check out.
> We were looking at the logs for the particular item.

Fixed!
Comment 10 M. Tompsett 2017-10-27 00:10:24 UTC
Created attachment 68647 [details] [review]
Bug 18816 - Make CataloguingLog work in production by preventing circulation from spamming the log

The system preference CataloguingLog is not recommended for use in
production. This is do to the fact that every checkin and checkout
generates one or more log entires. This seems to be not only bad
behavior, but unnecessary and outside the needs of CataloguingLog as we
have CirculationLog.

Test Plan:
1) Log into staff client
2) Home -> Koha administration -> Global system preferences -> Logs
3) Set only CataloguingLog to 'Log', everything else to "Don't log"
4) Click 'Save all Logging preferences'
5) In MySQL, use your instance DB, and then type 'delete from action_logs;'
6) Have a person checkout and checkin anything.
7) In MySQL, 'select * from action_logs;'
   -- there will be data. This is the floodiness that will be removed.
8) Apply this patch
9) Repeat steps 5-7
   -- there should be no data.
10) Edit any biblio or item.
11) In MySQL, 'select * from action_logs;'
    -- there should be data reflecting the changes made.
12) run koha qa test tools

NOTE: Improved clarity of test plan -- Mark Tompsett

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 11 Marcel de Rooy 2017-11-27 12:04:13 UTC
This imo is an enhancement.
Comment 12 Julian Maurice 2018-03-12 11:24:34 UTC
It might be useful to turn the last parameter of ModItem into a hashref of parameters.

  ModItem(\%item, $biblionumber, $itemnumber, {log_action => 0});

is more readable than

  ModItem(\%item, $biblionumber, $itemnumber, 0);

What do you think ?
Comment 13 Marcel de Rooy 2018-03-22 07:53:23 UTC
(In reply to Julian Maurice from comment #12)
> It might be useful to turn the last parameter of ModItem into a hashref of
> parameters.
> 
>   ModItem(\%item, $biblionumber, $itemnumber, {log_action => 0});
> 
> is more readable than
> 
>   ModItem(\%item, $biblionumber, $itemnumber, 0);
> 
> What do you think ?

I would agree.
Comment 14 Marcel de Rooy 2018-03-22 07:53:30 UTC
+subtest 'Test logging for AddItem' => sub {
You mean ModItem.
Comment 15 Kyle M Hall 2018-03-22 10:44:05 UTC
Created attachment 73154 [details] [review]
Bug 18816: Make CataloguingLog work in production by preventing circulation from spamming the log

The system preference CataloguingLog is not recommended for use in
production. This is do to the fact that every checkin and checkout
generates one or more log entires. This seems to be not only bad
behavior, but unnecessary and outside the needs of CataloguingLog as we
have CirculationLog.

Test Plan:
1) Log into staff client
2) Home -> Koha administration -> Global system preferences -> Logs
3) Set only CataloguingLog to 'Log', everything else to "Don't log"
4) Click 'Save all Logging preferences'
5) In MySQL, use your instance DB, and then type 'delete from action_logs;'
6) Have a person checkout and checkin anything.
7) In MySQL, 'select * from action_logs;'
   -- there will be data. This is the floodiness that will be removed.
8) Apply this patch
9) Repeat steps 5-7
   -- there should be no data.
10) Edit any biblio or item.
11) In MySQL, 'select * from action_logs;'
    -- there should be data reflecting the changes made.
12) run koha qa test tools

NOTE: Improved clarity of test plan -- Mark Tompsett

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 16 Kyle M Hall 2018-03-22 10:44:12 UTC
Created attachment 73155 [details] [review]
Bug 18816 [QA Followup]: Convert param to hashref, fix typo
Comment 17 Marcel de Rooy 2018-03-23 09:48:59 UTC
We have a clash with parameters here:
Look at "The fourth, optional parameter, C<$unlinked_item_subfields> "
Comment 18 Kyle M Hall 2018-03-23 11:05:20 UTC
Created attachment 73169 [details] [review]
Bug 18816: Add rarely used params to hashref, remove  param
Comment 19 Kyle M Hall 2018-03-23 11:05:54 UTC
Created attachment 73170 [details] [review]
Bug 18816 [QA Followup]: Add rarely used params to hashref, remove  param
Comment 20 Kyle M Hall 2018-03-23 11:07:19 UTC
Created attachment 73171 [details] [review]
Bug 18816 [QA Followup]: Add rarely used params to hashref, remove  param
Comment 21 Marcel de Rooy 2018-04-06 05:57:52 UTC
QA: Looking here
Comment 22 Marcel de Rooy 2018-04-06 06:10:44 UTC
Created attachment 73729 [details] [review]
Bug 18816: Make CataloguingLog work in production by preventing circulation from spamming the log

The system preference CataloguingLog is not recommended for use in
production. This is do to the fact that every checkin and checkout
generates one or more log entires. This seems to be not only bad
behavior, but unnecessary and outside the needs of CataloguingLog as we
have CirculationLog.

Test Plan:
1) Log into staff client
2) Home -> Koha administration -> Global system preferences -> Logs
3) Set only CataloguingLog to 'Log', everything else to "Don't log"
4) Click 'Save all Logging preferences'
5) In MySQL, use your instance DB, and then type 'delete from action_logs;'
6) Have a person checkout and checkin anything.
7) In MySQL, 'select * from action_logs;'
   -- there will be data. This is the floodiness that will be removed.
8) Apply this patch
9) Repeat steps 5-7
   -- there should be no data.
10) Edit any biblio or item.
11) In MySQL, 'select * from action_logs;'
    -- there should be data reflecting the changes made.
12) run koha qa test tools

NOTE: Improved clarity of test plan -- Mark Tompsett

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2018-04-06 06:10:49 UTC
Created attachment 73730 [details] [review]
Bug 18816 [QA Followup]: Convert param to hashref, fix typo

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Marcel de Rooy 2018-04-06 06:10:54 UTC
Created attachment 73731 [details] [review]
Bug 18816 [QA Followup]: Add rarely used params to hashref, remove param

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 25 Marcel de Rooy 2018-04-06 06:10:58 UTC
Created attachment 73732 [details] [review]
Bug 18816: (QA follow-up) Framework parameter is not needed

$frameworkcode is not used in ModItem (no need to pass it in
ModItemFromMarc).
$dbh is not used in ModItem.

Test plan:
Run t/db_dependent/Items.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Marcel de Rooy 2018-04-06 06:11:03 UTC
Created attachment 73733 [details] [review]
Bug 18816: (QA follow-up) Fix Circulation.t

Resolve:
ok 33 - Can renew, item is marked not for loan, hold does not block
Can't use string ("1") as a HASH ref while "strict refs" in use at /usr/share/koha/devclone/C4/Items.pm line 557.

Trivial fix. Remove fourth parameter.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 27 Jonathan Druart 2018-04-06 17:53:23 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 28 Nick Clemens 2018-05-14 17:26:11 UTC
Enhancement, not backported for 17.11.x
Comment 29 Kyle M Hall 2018-10-17 16:54:47 UTC
*** Bug 17195 has been marked as a duplicate of this bug. ***