Hello, I'm getting the following error message every 24h via email due to a failed cron job execution. As it doesn't disappear for a few versions now, and I don't have any experience with Koha's source, I'd like to report it. /etc/cron.daily/koha-common: Use of uninitialized value in string eq at /usr/share/koha/lib/C4/Overdues.pm line 686.
Created attachment 168008 [details] [review] Patch which resolves the issue
You're going to get tripped up by https://wiki.koha-community.org/wiki/Commit_messages (and then probably later by the project usually not being fond of adding checks for things being undefined when they shouldn't be) And when you get to the part about writing a test plan, it looks to me like the test plan is going to look like: 1. Go to Tools - Overdue notice/status triggers and verify that for every single patron type for both Default and every individual library, you have no value set for Delay, so that you will never send anyone an overdue notice 2. Run the cron job which creates and sends overdue notices which seems like rather odd behavior. If you don't want to send notices, wouldn't it be better to comment out the line in /etc/cron.daily/koha-common that runs the overdue notices job? Or if you really want to be able to send them later without anyone having to remember to uncomment the job, maybe just add an unused patron type and set a notice delay for that one patron type?
(In reply to Phil Ringnalda from comment #2) > You're going to get tripped up by > https://wiki.koha-community.org/wiki/Commit_messages (and then probably > later by the project usually not being fond of adding checks for things > being undefined when they shouldn't be) > > And when you get to the part about writing a test plan, it looks to me like > the test plan is going to look like: > > 1. Go to Tools - Overdue notice/status triggers and verify that for every > single patron type for both Default and every individual library, you have > no value set for Delay, so that you will never send anyone an overdue notice > 2. Run the cron job which creates and sends overdue notices > > which seems like rather odd behavior. If you don't want to send notices, > wouldn't it be better to comment out the line in /etc/cron.daily/koha-common > that runs the overdue notices job? Or if you really want to be able to send > them later without anyone having to remember to uncomment the job, maybe > just add an unused patron type and set a notice delay for that one patron > type? I understand your doubts, however after fixing this, the cron job returns much more helpful error messages about overdue rules not being configured correctly: /etc/cron.daily/koha-common: No branches with active overduerules at /usr/share/koha/bin/cronjobs/overdue_notices.pl line 365. bibliothek: 11 status returned by "/usr/share/koha/bin/cronjobs/overdue_notices.pl -t" Thus I think that this fix will at least help when you to find the cause of the crob error messages instead of giving you a file and a LOC.
That would convince me it's an improvement (though I'm not the one who would be doing the QA step and making the decision).
Hi Denys, thanks for the patch! On servers with multiple instances of Koha not every library might have configured overdue notice triggers if they don't intend to use the feature. I think this fix is definitely worthwhile.
Hi Denys, welcome and thanks for the patch! As mentioned, the commit message needs to follow the guidelines at https://wiki.koha-community.org/wiki/Commit_messages A commit message normally looks something like this: Bug 36128: [A short title about what the bug does] [A description of what problem the bug addresses, or what feature it adds]. Test plan: 1. Steps to replicate the issue. 2. Apply the patch. 3. Steps that show how the issue is fixed. It can be a bit daunting when you first get started with Koha, and trying to understand the process. This page on the Wiki gives a good overview of the steps a bug goes through before it gets into Koha https://wiki.koha-community.org/wiki/Development_workflow Many Koha developers use the development environment for Koha - once set up, it gives you a fully functional Koha environment https://gitlab.com/koha-community/koha-testing-docker The Development channel at https://chat.koha-community.org/ is probably the best place to ask for help.
Apologies Danys, I'd meant to get to this earlier. I'll give it a proper test this morning, but actually, it looks pretty solid to me, and reading the comments you've already added here I think the logic all follows. The commit message looks reasonable.. I think people just aren't used to the attachment title not matching the first part of the commit line. (Are you aware of git-bz?.. it helps with the uploading/download process for patches... I know the workflow is a little old fashioned in today's world of github/lab pull/merge requests.. I'm working to improve that - https://wiki.koha-community.org/wiki/Git_bz_configuration). What's your development environment look like.. KTD is well worth looking into if you've not already as it comes with all the tools and environment pre-installed, thus making submissions somewhat easier. Anyway.. I'll give it a test now.
Created attachment 168188 [details] [review] Bug 36128: Use of uninitialized value in string eq at /usr/share/koha/lib/C4/Overdues.pm Fixes the following error message when running the overdues check cronjob on a Koha system without defined overdue rules: /etc/cron.daily/koha-common: Use of uninitialized value in string eq at /usr/share/koha/lib/C4/Overdues.pm line 686. by checking if the variable is defined before comparing it. Test plan: 1. Go to Tools - Overdue notice/status triggers and verify that for every single patron type for both Default and every individual library, you have no value set for Delay, so that you will never send anyone an overdue notice 2. Run the cron job which creates and sends overdue notices 3. Confirm the above mentioned error no longer appears Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 168189 [details] [review] Bug 36128: (QA follow-up) Add regression test This patch adds a simple regression test to ensure we don't re-introduce the errant warning. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Thanks for your submission Danys, it works great, doesn't cause any regressions that I can detect. I'm going to go straight for a QA here as it's a trivial code change and I'm confident in the work. I've added a small regression test as a QA follow-up.. normally we'd have a Signoff from one party followed by a QA check by another, but in this case, I'm going to push it through a little quicker. Congratulations and thanks for your first submission.
(In reply to Martin Renvoize from comment #10) > Thanks for your submission Danys, it works great, doesn't cause any > regressions that I can detect. > > I'm going to go straight for a QA here as it's a trivial code change and I'm > confident in the work. > > I've added a small regression test as a QA follow-up.. normally we'd have a > Signoff from one party followed by a QA check by another, but in this case, > I'm going to push it through a little quicker. > > Congratulations and thanks for your first submission. Hello, thank you for your work and for accepting my patch. I am on vacation currently, so it wasn't possible for me to adapt the patch to the commit message guidelines fast enough - thank you for doing it yourself already. I will take your advices into account for possible future contributions :)
Pushed for 24.11! Well done everyone, thank you!
(In reply to Martin Renvoize from comment #7) > > The commit message looks reasonable.. I think people just aren't used to the > attachment title not matching the first part of the commit line. (Are you > aware of git-bz?.. it helps with the uploading/download process for > patches... I know the workflow is a little old fashioned in today's world of > github/lab pull/merge requests.. I'm working to improve that - > https://wiki.koha-community.org/wiki/Git_bz_configuration). Thanks Martin for pointing this out - I always forget to look at the details for the attachment where the attachment title doesn't follow the commit message guidelines. Apologies Denys if the comments about not following the commit message guidelines came across as a bit harsh!
Congratulations Denys on getting your first patch into Koha!
Backported to 24.05.x for upcoming 24.05.02
Pushed to 23.11.x for 23.11.07
Not backporting to 23.05.x unless requested