Bug 38104 - share_usage_with_koha_community.pl: Check between two runs is not needed
Summary: share_usage_with_koha_community.pl: Check between two runs is not needed
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-10-07 09:05 UTC by Marcel de Rooy
Modified: 2025-01-24 15:15 UTC (History)
4 users (show)

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


Attachments
Bug 38104: Adjust number of seconds in UsageStats::NeedUpdate (1.26 KB, patch)
2024-10-07 09:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 38104: Adjust number of seconds in UsageStats::NeedUpdate (2.22 KB, patch)
2025-01-16 15:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 38104: Adjust number of seconds in UsageStats::NeedUpdate (2.23 KB, patch)
2025-01-16 22:40 UTC, Phil Ringnalda
Details | Diff | Splinter Review
[ALTERNATIVE-PATCH] Bug 38104: Always update when hea cronjob is run (4.00 KB, patch)
2025-01-23 13:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38104: Always update when hea cronjob is run (4.09 KB, patch)
2025-01-24 07:20 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 Marcel de Rooy 2024-10-07 09:05:13 UTC
From production data:

HEA preference: 1725159125
select FROM_UNIXTIME( 1725159125 );
| 2024-09-01 04:52:05         |

select * from action_logs where timestamp>'2024-10-01' and module='CRONJOBS' and script like '%usage%';
|   7429456 | 2024-10-01 04:52:04 |    0 | CRONJOBS | Run    | 116996 | /usr/share/koha/misc/cronjobs/share_usage_with_koha_community.pl           | cron      | share_usage_with_koha_community.pl |
|   7429457 | 2024-10-01 04:52:04 |    0 | CRONJOBS | End    | 116996 | /usr/share/koha/misc/cronjobs/share_usage_with_koha_community.pl COMPLETED | cron      | share_usage_with_koha_community.pl |

So the HEA update was skipped since the script was 1 second too early :)
Comment 1 Marcel de Rooy 2024-10-07 09:06:12 UTC
Since we run this script from cron.monthly, it would be enough to check for say four weeks.
Comment 2 Marcel de Rooy 2024-10-07 09:30:04 UTC
Created attachment 172447 [details] [review]
Bug 38104: Adjust number of seconds in UsageStats::NeedUpdate

Test plan:
Check pref UsageStatsLastUpdateTime
Run perl -MC4::UsageStats -e'print C4::UsageStats::NeedUpdate()'
Verify that it prints 1 only if time interval exceeds 4 weeks.
Temporarily clear this pref.
Run again. Should always print 1 now.
Restore pref value.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Phil Ringnalda 2024-10-25 13:46:07 UTC
Isn't that still risky for March 1st being run 2419199 seconds after February 1st?
Comment 4 David Nind 2024-10-26 19:02:50 UTC
Could you be a bit more specific in the test plan how to test this, please?

- There is no UsageStatsLastUpdateTime system preference (in main).
- Do I apply the patch first?
- Do I need to configure Administration > Additional parameters > Share your usage statistics?

Shouldn't the commit message have a description, as per the commit message guidelines? (https://wiki.koha-community.org/wiki/Commit_messages#Bug.2Ffeature_description)
Comment 5 Marcel de Rooy 2024-10-28 14:23:21 UTC
(In reply to Phil Ringnalda from comment #3)
> Isn't that still risky for March 1st being run 2419199 seconds after
> February 1st?

Yeah good point. Currently an issue too..
Comment 6 Marcel de Rooy 2024-10-28 14:25:54 UTC
(In reply to David Nind from comment #4)
> Could you be a bit more specific in the test plan how to test this, please?
> 
> - There is no UsageStatsLastUpdateTime system preference (in main).
Local prefs.
> - Do I apply the patch first?
> - Do I need to configure Administration > Additional parameters > Share your
> usage statistics?
Yeah. Needs to be enabled. Will add a step to insert some last update time in the test plan.

> Shouldn't the commit message have a description, as per the commit message
> guidelines?
> (https://wiki.koha-community.org/wiki/Commit_messages#Bug.
> 2Ffeature_description)

This is about 2 lines of code. So imo covered by the title..
Not uncommon to see.
Comment 7 Marcel de Rooy 2025-01-16 15:15:00 UTC
Created attachment 176674 [details] [review]
Bug 38104: Adjust number of seconds in UsageStats::NeedUpdate

The current interval between two HEA updates is hardcoded to
2592000 seconds, i.e. 30 days. This should be a bit lower
to compensate for shorter months (Feb) or a small difference
in execution time under cron.

This patch adjusts the 30 days to 27 days. This allows us to not
miss February too. Note that if you set it to 28 days, a few minutes
difference could already cause a HEA skip too.

Test plan:
Enable pref UsageStats.
Check pref UsageStatsLastUpdateTime (under Local use).
Keep the value X somewhere. (Don't worry if it was empty.)
You can convert this value X with:
  perl -MPOSIX -e 'print POSIX::strftime("%Y-%m-%d %H:%M:%S\n", localtime(X) )'
The current time is shown in seconds with:
  perl -e'print time()'

Enter a value like 1234 in UsageStatsLastUpdateTime. This should
trigger an update normally.
Run perl -MC4::UsageStats -e'print C4::UsageStats::NeedUpdate()'
This should print 1.
Now put the current time in seconds in the pref (see above).
Run perl -MC4::UsageStats -e'print C4::UsageStats::NeedUpdate()'
This should print 0.
[BONUS] Test with a value now - 26 days, now - 28 days.
Temporarily clear the pref.
Run again. Should print 1 again.
Restore pref value.
Disable UsageStats if it was not enabled.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2025-01-16 15:16:01 UTC
Well, ended up with a 'long' commit message/test plan for this tiny change :)
Please test !
Comment 9 Phil Ringnalda 2025-01-16 22:40:36 UTC
Created attachment 176702 [details] [review]
Bug 38104: Adjust number of seconds in UsageStats::NeedUpdate

The current interval between two HEA updates is hardcoded to
2592000 seconds, i.e. 30 days. This should be a bit lower
to compensate for shorter months (Feb) or a small difference
in execution time under cron.

This patch adjusts the 30 days to 27 days. This allows us to not
miss February too. Note that if you set it to 28 days, a few minutes
difference could already cause a HEA skip too.

Test plan:
Enable pref UsageStats.
Check pref UsageStatsLastUpdateTime (under Local use).
Keep the value X somewhere. (Don't worry if it was empty.)
You can convert this value X with:
  perl -MPOSIX -e 'print POSIX::strftime("%Y-%m-%d %H:%M:%S\n", localtime(X) )'
The current time is shown in seconds with:
  perl -e'print time()'

Enter a value like 1234 in UsageStatsLastUpdateTime. This should
trigger an update normally.
Run perl -MC4::UsageStats -e'print C4::UsageStats::NeedUpdate()'
This should print 1.
Now put the current time in seconds in the pref (see above).
Run perl -MC4::UsageStats -e'print C4::UsageStats::NeedUpdate()'
This should print 0.
[BONUS] Test with a value now - 26 days, now - 28 days.
Temporarily clear the pref.
Run again. Should print 1 again.
Restore pref value.
Disable UsageStats if it was not enabled.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 10 Marcel de Rooy 2025-01-17 07:02:02 UTC
(In reply to Phil Ringnalda from comment #9)
> Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>

Thanks!
Comment 11 Jonathan Druart 2025-01-23 11:40:18 UTC
I don't understand why we have both actually.

Either we run the cronjob every day and the script check if it needs to update, or we run the cronjob every month and the script just updates.

Should not we simply remove from this NeedUpdate check?
Comment 12 Marcel de Rooy 2025-01-23 12:31:40 UTC
(In reply to Jonathan Druart from comment #11)
> I don't understand why we have both actually.
> 
> Either we run the cronjob every day and the script check if it needs to
> update, or we run the cronjob every month and the script just updates.
> 
> Should not we simply remove from this NeedUpdate check?

Yeah fine with me. I am dropping this report for now.
Comment 13 Jonathan Druart 2025-01-23 13:35:33 UTC
Created attachment 176957 [details] [review]
[ALTERNATIVE-PATCH] Bug 38104: Always update when hea cronjob is run

If the cronjob is configured to run every month we don't need to add
another test on the delay in the script itself.
Comment 14 David Nind 2025-01-23 18:56:11 UTC
The alternative patch makes sense to me (given my limited understanding of any technical issues) - update when the cronjob was last run.

Happy to test and sign off (the t/db_dependent/UsageStats.t 
t/db_dependent/UsageStats.t tests pass before and after).
Comment 15 Marcel de Rooy 2025-01-24 07:20:16 UTC
Created attachment 177096 [details] [review]
Bug 38104: Always update when hea cronjob is run

If the cronjob is configured to run every month we don't need to add
another test on the delay in the script itself.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2025-01-24 07:21:17 UTC
SO and QA in one go. Trivial change.
Comment 17 Katrin Fischer 2025-01-24 15:15:11 UTC
Pushed for 25.05!

Well done everyone, thank you!