Bug 36128 - Use of uninitialized value in string eq at /usr/share/koha/lib/C4/Overdues.pm
Summary: Use of uninitialized value in string eq at /usr/share/koha/lib/C4/Overdues.pm
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All Linux
: P5 - low minor
Assignee: Denys Konovalov
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-02-17 14:01 UTC by Denys Konovalov
Modified: 2024-09-17 03:52 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This 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.
Version(s) released in:
24.11.00,24.05.02,23.11.07
Circulation function:


Attachments
Patch which resolves the issue (1.16 KB, patch)
2024-06-23 12:18 UTC, Denys Konovalov
Details | Diff | Splinter Review
Bug 36128: Use of uninitialized value in string eq at /usr/share/koha/lib/C4/Overdues.pm (1.57 KB, patch)
2024-06-27 09:02 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 36128: (QA follow-up) Add regression test (1.67 KB, patch)
2024-06-27 09:02 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Denys Konovalov 2024-02-17 14:01:45 UTC
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.
Comment 1 Denys Konovalov 2024-06-23 12:18:28 UTC
Created attachment 168008 [details] [review]
Patch which resolves the issue
Comment 2 Phil Ringnalda 2024-06-24 17:39:39 UTC
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?
Comment 3 Denys Konovalov 2024-06-24 20:23:38 UTC
(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.
Comment 4 Phil Ringnalda 2024-06-24 20:37:25 UTC
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).
Comment 5 Katrin Fischer 2024-06-25 07:41:19 UTC
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.
Comment 6 David Nind 2024-06-25 11:41:48 UTC
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.
Comment 7 Martin Renvoize (ashimema) 2024-06-27 08:22:29 UTC
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.
Comment 8 Martin Renvoize (ashimema) 2024-06-27 09:02:48 UTC
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>
Comment 9 Martin Renvoize (ashimema) 2024-06-27 09:02:51 UTC
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>
Comment 10 Martin Renvoize (ashimema) 2024-06-27 09:05:34 UTC
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.
Comment 11 Denys Konovalov 2024-06-27 10:05:30 UTC
(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 :)
Comment 12 Katrin Fischer 2024-06-27 12:06:28 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 13 David Nind 2024-06-27 21:08:52 UTC
(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!
Comment 14 David Nind 2024-06-27 21:09:24 UTC
Congratulations Denys on getting your first patch into Koha!
Comment 15 Lucas Gass (lukeg) 2024-07-19 15:22:52 UTC
Backported to 24.05.x for upcoming 24.05.02
Comment 16 Fridolin Somers 2024-07-22 09:49:55 UTC
Pushed to 23.11.x for 23.11.07
Comment 17 Wainui Witika-Park 2024-09-17 03:52:08 UTC
Not backporting to 23.05.x unless requested