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 :)
Since we run this script from cron.monthly, it would be enough to check for say four weeks.
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>
Isn't that still risky for March 1st being run 2419199 seconds after February 1st?
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)
(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..
(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.
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>
Well, ended up with a 'long' commit message/test plan for this tiny change :) Please test !
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>
(In reply to Phil Ringnalda from comment #9) > Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Thanks!
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?
(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.
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.
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).
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>
SO and QA in one go. Trivial change.
Pushed for 25.05! Well done everyone, thank you!