Bug 29238 - Cataloging cron jobs are not logged
Summary: Cataloging cron jobs are not logged
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Eric Phetteplace
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-10-13 21:50 UTC by Eric Phetteplace
Modified: 2024-11-19 15:08 UTC (History)
5 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 29238 - Cataloging cron jobs are not logged (2.80 KB, patch)
2021-10-14 00:03 UTC, Eric Phetteplace
Details | Diff | Splinter Review
Screenshot of the logs (113.02 KB, image/png)
2021-10-14 00:03 UTC, Eric Phetteplace
Details
Bug 29238 - Cataloging cron jobs are not logged (2.53 KB, patch)
2024-09-30 21:45 UTC, Eric Phetteplace
Details | Diff | Splinter Review
Bug 29238 - Cataloging cron jobs are not logged (2.73 KB, patch)
2024-11-07 21:27 UTC, Eric Phetteplace
Details | Diff | Splinter Review
Bug 29238 - Cataloging cron jobs are not logged (2.80 KB, patch)
2024-11-07 21:47 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29238: Cataloging cron jobs are not logged (2.89 KB, patch)
2024-11-15 08:06 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 Eric Phetteplace 2021-10-13 21:50:25 UTC
After a few cron jobs related to authority records (misc/migration_tools/remove_unused_authorities.pl, misc/link_bibs_to_authorities.pl) were set up to run on our instance, I went to check the Log Viewer tool but couldn't tell that they were configured. I was told that only newer cron jobs actually appear in the logs, and on inspecting those two I saw that they lacked the call to `cronlogaction()` which our circulation-related cronjobs have. It'd be helpful to know if/when these jobs ran.

I realize this is a bit debatable because those two scripts don't reside in misc/cronjobs and right now the only scripts that call `cronlogaction()` are in that directory. Is it appropriate to add that logging to these scripts? We do run them on a schedule, it's important to our authority records workflow. This is the complete list of ones I'd like to add logging to:

- misc/link_bibs_to_authorities.pl
- misc/cronjobs/merge_authorities.pl
- misc/migration_tools/remove_unused_authorities.pl

A side note: there are several scripts in misc/cronjobs, like merge_authorities.pl, that are not logged and probably should be, but that is a larger bug and I'd like to focus on these first since they're relevant to us.
Comment 1 Eric Phetteplace 2021-10-14 00:03:01 UTC
Created attachment 126242 [details] [review]
Bug 29238 - Cataloging cron jobs are not logged

Here's an example patch. Is misc/migration_tools/remove_unused_authorities.pl maybe broken on master? Perl complained that DelAuthority() on line 102 was undefined, so I changed the "use" statement. But otherwise, the patch just adds a cronlogaction import and call when the script is run.

Generally, `cronlogaction()` is called during dry runs like `remove_unused_authorities.pl --test` but not if the script is printing its usage information (e.g. `remove_unused_authorities.pl --help`). It'd be much more complicated to only log when data actually changes.

Testing plan:

1. set CronjobLog to "Log"
2. Run the three cron jobs
   `perl misc/link_bibs_to_authorities.pl; perl misc/cronjobs/merge_authorities.pl -b; perl misc/migration_tools/remove_unused_authorities.pl --confirm`
3. Visit the log viewer /cgi-bin/koha/tools/viewlog.pl, select the "Cron jobs" module, & note it's empty
4. apply the patch
5. Repeat steps 2 & 3 but note the cron logs aren't empty
Comment 2 Eric Phetteplace 2021-10-14 00:03:45 UTC
Created attachment 126243 [details]
Screenshot of the logs
Comment 3 Sarah Cornell 2022-07-07 15:00:43 UTC
We'd appreciate this tiny enhancement.  I thought our linker cron wasn't running today because it wasn't in the log.  Can we bump this up?  Thanks!
Comment 4 Andrew Fuerste-Henry 2024-09-13 14:49:10 UTC
Eric, should this be at needs signoff?
Comment 5 Eric Phetteplace 2024-09-13 15:03:03 UTC
Yes, sorry, I'm not super familiar with Bugzilla.
Comment 6 Sukhmandeep 2024-09-26 20:23:29 UTC
The patch doesn't apply anymore. There seems to be a couple of conflicts.

Error while trying to run git bz apply 29238:

Using index info to reconstruct a base tree...
M	misc/link_bibs_to_authorities.pl
M	misc/migration_tools/remove_unused_authorities.pl
Falling back to patching base and 3-way merge...
Auto-merging misc/migration_tools/remove_unused_authorities.pl
CONFLICT (content): Merge conflict in misc/migration_tools/remove_unused_authorities.pl
Auto-merging misc/link_bibs_to_authorities.pl
CONFLICT (content): Merge conflict in misc/link_bibs_to_authorities.pl
error: Failed to merge in the changes.
Patch failed at 0001 Bug 29238 - Cataloging cron jobs are not logged
Comment 7 Eric Phetteplace 2024-09-30 21:45:19 UTC
Created attachment 172240 [details] [review]
Bug 29238 - Cataloging cron jobs are not logged

Updated patch based off master. The files were just touched in between, this patch doesn't do anything different. Weird that git merge wouldn't have been able to work through it.
Comment 8 Andrew Fuerste-Henry 2024-11-04 17:09:09 UTC
I get this error when attempting to apply this patch on my testing docker: 
Traceback (most recent call last):
  File "/usr/bin/git-bz", line 2834, in <module>
    applied = do_apply(bug_ref)
              ^^^^^^^^^^^^^^^^^
  File "/usr/bin/git-bz", line 1918, in do_apply
    users = bug.server.get_xmlrpc_proxy().User.get({'names': [patch.attacher]})['users']
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/xmlrpc/client.py", line 1122, in __call__
    return self.__send(self.__name, args)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/xmlrpc/client.py", line 1464, in __request
    response = self.__transport.request(
               ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/xmlrpc/client.py", line 1166, in request
    return self.single_request(host, handler, request_body, verbose)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/xmlrpc/client.py", line 1178, in single_request
    http_conn = self.send_request(host, handler, request_body, verbose)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/bin/git-bz", line 1239, in send_request
    connection.putheader("Cookie", cookie)
    ^^^^^^^^^^^^^^^^^^^^
AttributeError: 'str' object has no attribute 'putheader'
Comment 9 Eric Phetteplace 2024-11-07 21:27:33 UTC
Created attachment 174260 [details] [review]
Bug 29238 - Cataloging cron jobs are not logged

Testing plan:

1. set CronjobLog to "Log"
2. Run the three cron jobs
   `perl misc/link_bibs_to_authorities.pl; perl misc/cronjobs/merge_authorities.pl -b; perl misc/migration_tools/remove_unused_authorities.pl --confirm`
3. Visit the log viewer /cgi-bin/koha/tools/viewlog.pl, select the "Cron jobs" module, & note it's empty
4. apply the patch
5. Repeat steps 2 & 3 but note the cron logs aren't empty
Comment 10 Eric Phetteplace 2024-11-07 21:29:35 UTC
Sorry, I made the patch manually, but git bz doesn't seem to be able to apply manual patches. I've generated it with `git bz attach` so hopefully it applies now.
Comment 11 Andrew Fuerste-Henry 2024-11-07 21:47:10 UTC
Created attachment 174261 [details] [review]
Bug 29238 - Cataloging cron jobs are not logged

Testing plan:

1. set CronjobLog to "Log"
2. Run the three cron jobs
   `perl misc/link_bibs_to_authorities.pl; perl misc/cronjobs/merge_authorities.pl -b; perl misc/migration_tools/remove_unused_authorities.pl --confirm`
3. Visit the log viewer /cgi-bin/koha/tools/viewlog.pl, select the "Cron jobs" module, & note it's empty
4. apply the patch
5. Repeat steps 2 & 3 but note the cron logs aren't empty

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 12 Andrew Fuerste-Henry 2024-11-07 21:47:31 UTC
(In reply to Eric Phetteplace from comment #10)
> Sorry, I made the patch manually, but git bz doesn't seem to be able to
> apply manual patches. I've generated it with `git bz attach` so hopefully it
> applies now.

Thanks for writing it!
Comment 13 Katrin Fischer 2024-11-08 07:59:20 UTC
(In reply to Eric Phetteplace from comment #10)
> Sorry, I made the patch manually, but git bz doesn't seem to be able to
> apply manual patches. I've generated it with `git bz attach` so hopefully it
> applies now.

I think it's not the manual bit, but somehow you got a very different header lines in the patch with your manual process than when using git bz (Author vs. From for example). git bz is better, but when you need to create a patch manually, you can use git format-patch, which should give you the same output.
Comment 14 Eric Phetteplace 2024-11-08 17:40:57 UTC
(In reply to Katrin Fischer from comment #13)
> (In reply to Eric Phetteplace from comment #10)
> > Sorry, I made the patch manually, but git bz doesn't seem to be able to
> > apply manual patches. I've generated it with `git bz attach` so hopefully it
> > applies now.
> 
> I think it's not the manual bit, but somehow you got a very different header
> lines in the patch with your manual process than when using git bz (Author
> vs. From for example). git bz is better, but when you need to create a patch
> manually, you can use git format-patch, which should give you the same
> output.

I used `git format-patch` for the original patch. Though when I go to `git format-patch -n HEAD^` on the bug's branch now, I do get a patch identical to the git bz one, with similar header lines. I'm not sure what I did wrong with my original, perhaps something in my git configuration, I made it a while ago. Anyways, I've learned to stick with git bz.
Comment 15 Marcel de Rooy 2024-11-15 08:03:31 UTC
 WARN   misc/cronjobs/merge_authorities.pl
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 17, now: 18)

 OK     misc/link_bibs_to_authorities.pl

 WARN   misc/migration_tools/remove_unused_authorities.pl
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 24, now: 25)


Processing additional checks

        * Commit title does not start with 'Bug XXXXX: ' - 909111fdd9
Comment 16 Marcel de Rooy 2024-11-15 08:06:10 UTC
Created attachment 174545 [details] [review]
Bug 29238: Cataloging cron jobs are not logged

Testing plan:

1. set CronjobLog to "Log"
2. Run the three cron jobs
   `perl misc/link_bibs_to_authorities.pl; perl misc/cronjobs/merge_authorities.pl -b; perl misc/migration_tools/remove_unused_authorities.pl --confirm`
3. Visit the log viewer /cgi-bin/koha/tools/viewlog.pl, select the "Cron jobs" module, & note it's empty
4. apply the patch
5. Repeat steps 2 & 3 but note the cron logs aren't empty

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2024-11-15 08:07:04 UTC
(In reply to Marcel de Rooy from comment #15)
> Processing additional checks
> 
>         * Commit title does not start with 'Bug XXXXX: ' - 909111fdd9

Fixed commit title with ':'
Ignoring the whitespace stuff
Comment 18 Katrin Fischer 2024-11-18 16:39:31 UTC
Hm, looking at other uses of cronlogaction, aren't we missing parameters here?

misc/cronjobs/automatic_item_modification_by_age.pl:use C4::Log qw( cronlogaction );
misc/cronjobs/automatic_item_modification_by_age.pl:cronlogaction({ info => $command_line_options });
misc/cronjobs/automatic_item_modification_by_age.pl:cronlogaction({ action => 'End', info => "COMPLETED" });

Can you please check?