Bug 36013 - update_totalissues.pl should not log
Summary: update_totalissues.pl should not log
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Fridolin Somers
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-02-07 10:08 UTC by Fridolin Somers
Modified: 2024-04-01 15:13 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This updates logging when the misc/cronjobs/update_totalissues.pl script is run and CataloguingLog is enabled). This previously added one entry to the log for each record updated - this information is not useful and can take up a lot of database space.
Version(s) released in:


Attachments
Bug 36013: update_totalissues.pl should not log (1.31 KB, patch)
2024-02-07 10:15 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 36013: update_totalissues.pl should not log (1.49 KB, patch)
2024-03-08 12:04 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 36013: update_totalissues.pl should not log (1.54 KB, patch)
2024-03-09 20:52 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2024-02-07 10:08:52 UTC
Even when CataloguingLog is on, misc/cronjobs/update_totalissues.pl should not log the changes.
It is useless and takes database space.

Do like bulkmarcimport.pl :
https://git.koha-community.org/Koha-community/Koha/src/branch/23.11.x/misc/migration_tools/bulkmarcimport.pl#L165
Comment 1 Fridolin Somers 2024-02-07 10:15:20 UTC Comment hidden (obsolete)
Comment 2 David Nind 2024-02-10 21:46:34 UTC
I had a go at testing.

However, I'm not sure what I should see in the log viewer before the patch is applied.

With CataloguingLog enabled, I don't see any entries in the log viewer after running the cron job.

If I enable the CronjobLog log system preference, I see a single entry for the cron job running (Info column: misc/cronjobs/update_totalissues.pl --use-stats --commit=1000 -v).
Comment 3 Magnus Enger 2024-03-08 12:03:18 UTC
I had a problem where a bad record would stop the script from completing: 

$ misc/cronjobs/update_totalissues.pl --use-stats --commit=1000 -v
Processing bib 1 (0 issues)
Processing bib 2 (0 issues)
Processing bib 3 (0 issues)
...
Processing bib 368 (0 issues)
Processing bib 369 (0 issues)
:8: parser error : PCDATA invalid Char value 31
  <controlfield tag="001">00aD000015937</controlfield>
                            ^
:9: parser error : PCDATA invalid Char value 31
  <controlfield tag="004">00satmrnu0</controlfield>
                            ^
:9: parser error : PCDATA invalid Char value 31
  <controlfield tag="004">00satmrnu0</controlfield>
                               ^
:9: parser error : PCDATA invalid Char value 31
  <controlfield tag="004">00satmrnu0</controlfield>
                                  ^
:9: parser error : PCDATA invalid Char value 31
  <controlfield tag="004">00satmrnu0</controlfield>
                                     ^
:10: parser error : PCDATA invalid Char value 31
  <controlfield tag="008">00ar19881981bdkldan</controlfield>

After deleting that record the script runs as it should:

...
Processing bib 437 (0 issues)
Processing bib 438 (0 issues)
Update total issues count script report
=======================================================
Run started at:                         03/08/24 11:48:53
Run ended at:                           03/08/24 11:48:57
Total run time:                         4751 ms
Number of bibs modified:                435
Number of bibs with error:              0

After this the cataloguing log shows one entry per record, so that will quickley amount to a lot of unnecessary data!
Comment 4 Magnus Enger 2024-03-08 12:04:55 UTC
Created attachment 162957 [details] [review]
Bug 36013: update_totalissues.pl should not log

Even when CataloguingLog is on, misc/cronjobs/update_totalissues.pl should not log the changes.
It is useless and takes database space.

Do like bulkmarcimport.pl :
https://git.koha-community.org/Koha-community/Koha/src/branch/23.11.x/misc/migration_tools/bulkmarcimport.pl#L165

Test plan :
1) Enable system preference CataloguingLog
2) Perform some issues and returns
3) Run : misc/cronjobs/update_totalissues.pl --use-stats --commit=1000 -v
4) Go to Tools > Log viewer
5) Check there are no new cataloguing logs

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Works as advertised. Before the patch, running the script adds one
entry to action_logs per record, with the patch nothing is logged.
Comment 5 David Nind 2024-03-09 20:52:44 UTC
Created attachment 163012 [details] [review]
Bug 36013: update_totalissues.pl should not log

Even when CataloguingLog is on, misc/cronjobs/update_totalissues.pl should not log the changes.
It is useless and takes database space.

Do like bulkmarcimport.pl :
https://git.koha-community.org/Koha-community/Koha/src/branch/23.11.x/misc/migration_tools/bulkmarcimport.pl#L165

Test plan :
1) Enable system preference CataloguingLog
2) Perform some issues and returns
3) Run : misc/cronjobs/update_totalissues.pl --use-stats --commit=1000 -v
4) Go to Tools > Log viewer
5) Check there are no new cataloguing logs

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Works as advertised. Before the patch, running the script adds one
entry to action_logs per record, with the patch nothing is logged.
Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2024-03-09 21:02:55 UTC
THanks Magnus!

Have added my sign off in case that helps with getting through QA.
Comment 7 Nick Clemens 2024-04-01 15:13:05 UTC
I don't love just overriding the syspref here - or at least not without making it an option.

Please see bug 36474 - I reduce the logs by not updating a biblio when there are no changes. Beyond that I think it should be a switch in the command --nolog or something.

I am willing to be convinced otherwise, but I think 36474 should be addressed first