Bug 24266 - Noisy error in reconcile_balances.pl
Summary: Noisy error in reconcile_balances.pl
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Aleisha Amohia
QA Contact: Marcel de Rooy
URL:
Keywords:
: 25148 (view as bug list)
Depends on: 21915
Blocks:
  Show dependency treegraph
 
Reported: 2019-12-18 14:57 UTC by Liz Rea
Modified: 2020-05-13 21:16 UTC (History)
10 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.06, 19.05.11


Attachments
Bug 24266: Don't search for patrons if no borrowernumbers (3.28 KB, patch)
2019-12-19 12:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24266: (alternate patch) Only use defined borrowernumbers in reconcile_balances.pl cron (1.79 KB, patch)
2020-03-16 22:18 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 24266: (alternate patch) Only use defined borrowernumbers in reconcile_balances.pl cron (1.70 KB, patch)
2020-03-17 22:32 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 24266: (alternate patch) Only use defined borrowernumbers in reconcile_balances.pl cron (1.77 KB, patch)
2020-04-08 11:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24266: (alternate patch) Only use defined borrowernumbers in reconcile_balances.pl cron (1.87 KB, patch)
2020-04-10 08:21 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 Liz Rea 2019-12-18 14:57:45 UTC
Cron <instance-koha@kohaMain> $KOHA_CRON_PATH/reconcile_balances.pl --verbose 

Gives this message when run:

SQL::Abstract::puke(): [SQL::Abstract::__ANON__] Fatal: SQL::Abstract before v1.75 used to generate incorrect SQL when the -IN operator was given an undef-containing list: !!!AUDIT YOUR CODE AND DATA!!! (the upcoming Data::Query-based version of SQL::Abstract will emit the logically correct SQL instead of raising this exception) at /usr/share/koha/lib/Koha/Objects.pm line 209

It would be nice if this wouldn't happen, so that cron doesn't send mails about this error.
Comment 1 Jonathan Druart 2019-12-19 08:05:52 UTC
Hi Liz,

I do not recreate. How do you?
I tried the 2 situations:
- patron has one accountline with amountoutstanding < 0
- nothing in the accountlines table

My understanding of the error is that

   92 my $patrons = Koha::Patrons->search({ borrowernumber => { -in => \@patron_ids } });

is called with @patron_ids = ( undef );
Comment 2 Tomás Cohen Arazi 2019-12-19 10:56:43 UTC
I cannot reproduce either, but I agree the code needs to be fixed.
Comment 3 Jonathan Druart 2019-12-19 11:18:48 UTC
(In reply to Tomás Cohen Arazi from comment #2)
> I cannot reproduce either, but I agree the code needs to be fixed.

It's not. It should never be (undef)
Comment 4 Tomás Cohen Arazi 2019-12-19 12:55:05 UTC
Created attachment 96484 [details] [review]
Bug 24266: Don't search for patrons if no borrowernumbers

This patch makes the reconcile_balances.pl script skip searching for
patrons from a given borrowernumbers list, if the list is empty.

The code was logically incorrect, and it doesn't break because of a bug
in SQL::Abstract that in recent versions started raising a noisy
warning.

I couldn't reproduce it in koha-testing-docker but it's been found in
the wild. I submit this obvious fix so people having the issue can test
it properly.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi 2019-12-19 12:55:38 UTC
Liz, please test it on your production site and let me know if it fixes the issue.
Comment 6 Jonathan Druart 2019-12-19 15:46:13 UTC
Tomas, the warning is about ->search({ -in => [undef] }), not ->search({ -in => [] }). So I do not think your patch will change anything.
Comment 7 Aleisha Amohia 2020-03-16 22:18:35 UTC
Created attachment 100832 [details] [review]
Bug 24266: (alternate patch) Only use defined borrowernumbers in reconcile_balances.pl cron

I believe the error is triggered when borrowernumbers are left empty in
the accountlines table. Not sure why this would happen, but it appears
to be what causes the problem.

Do not apply the first patch if testing this patch.

To test:
1) sudo koha-mysql INSTANCENAME
2) if you don't have any accountlines where the borrowernumber is null,
    update one of your accountlines where the amountoutstanding is more
than zero, and make the borrowernumber NULL
3) ensure the AccountAutoReconcile syspref is disabled
4) Go to [borrowernumber]'s accounting tab
5) Create a manual credit or debit. Confirm this shows in the 'Make a
payment' tab as an amount that COULD be applied, but isn't automatically
applied
6) in your terminal, run the reconcile_balances.pl script
7) Confirm the error does not show in the logs and the balance for
[borrowernumber] is correctly reconciled.

Sponsored-by: Horowhenua District Council
Comment 8 Tomás Cohen Arazi 2020-03-16 23:19:30 UTC
I totally forgot about this one. Sorry.

Borrowernumbers get emptied when the patron gets deleted.
Comment 9 Aleisha Amohia 2020-03-16 23:21:15 UTC
Ah that totally makes sense.

Perhaps instead of going into the database in the test plan, create a test borrower, add some accountline, then delete the borrower. The script should know to skip past them.
Comment 10 Jonathan Druart 2020-03-17 08:13:14 UTC
Aleisha, double check your patch, you have "1=" instead of "!="
Comment 11 Aleisha Amohia 2020-03-17 22:32:14 UTC
Created attachment 100926 [details] [review]
Bug 24266: (alternate patch) Only use defined borrowernumbers in reconcile_balances.pl cron

I believe the error is triggered when borrowernumbers are left empty in
the accountlines table. Not sure why this would happen, but it appears
to be what causes the problem.

Do not apply the first patch if testing this patch.

To test:
1) sudo koha-mysql INSTANCENAME
2) Create a test borrower, add any payment etc to create an accountline,
    then delete this borrower
3) ensure the AccountAutoReconcile syspref is disabled
4) Go to another borrower's accounting tab
5) Create a manual credit or debit. Confirm this shows in the 'Make a
payment' tab as an amount that COULD be applied, but isn't automatically
applied
6) in your terminal, run the reconcile_balances.pl script
7) Confirm the error does not show in the logs and the balance for
the borrower is correctly reconciled.

Sponsored-by: Horowhenua District Council
Comment 12 Fridolin SOMERS 2020-03-26 14:04:28 UTC
alternate patch ? Its more a follow-up no ?
Comment 13 Aleisha Amohia 2020-03-26 20:17:11 UTC
(In reply to Fridolin SOMERS from comment #12)
> alternate patch ? Its more a follow-up no ?

Alternate patch, because in Comment 6 it was mentioned that the first patch doesn't really fix the problem
Comment 14 Fridolin SOMERS 2020-03-27 11:17:41 UTC
(In reply to Aleisha Amohia from comment #13)
> (In reply to Fridolin SOMERS from comment #12)
> > alternate patch ? Its more a follow-up no ?
> 
> Alternate patch, because in Comment 6 it was mentioned that the first patch
> doesn't really fix the problem

Ah in this case set it obsolete or set status In Discussion :)
Comment 15 Jonathan Druart 2020-03-27 11:43:07 UTC
Let's try Aleisha patch and see if it fixes the original issue.
Comment 16 Martin Renvoize 2020-04-08 11:05:42 UTC
Created attachment 102553 [details] [review]
Bug 24266: (alternate patch) Only use defined borrowernumbers in reconcile_balances.pl cron

I believe the error is triggered when borrowernumbers are left empty in
the accountlines table. Not sure why this would happen, but it appears
to be what causes the problem.

Do not apply the first patch if testing this patch.

To test:
1) sudo koha-mysql INSTANCENAME
2) Create a test borrower, add any payment etc to create an accountline,
    then delete this borrower
3) ensure the AccountAutoReconcile syspref is disabled
4) Go to another borrower's accounting tab
5) Create a manual credit or debit. Confirm this shows in the 'Make a
payment' tab as an amount that COULD be applied, but isn't automatically
applied
6) in your terminal, run the reconcile_balances.pl script
7) Confirm the error does not show in the logs and the balance for
the borrower is correctly reconciled.

Sponsored-by: Horowhenua District Council
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2020-04-08 11:06:41 UTC
This appears to resolve the issue for me.. Signing off
Comment 18 Marcel de Rooy 2020-04-10 08:21:45 UTC
Created attachment 102680 [details] [review]
Bug 24266: (alternate patch) Only use defined borrowernumbers in reconcile_balances.pl cron

I believe the error is triggered when borrowernumbers are left empty in
the accountlines table. Not sure why this would happen, but it appears
to be what causes the problem.

Do not apply the first patch if testing this patch.

To test:
1) sudo koha-mysql INSTANCENAME
2) Create a test borrower, add any payment etc to create an accountline,
    then delete this borrower
3) ensure the AccountAutoReconcile syspref is disabled
4) Go to another borrower's accounting tab
5) Create a manual credit or debit. Confirm this shows in the 'Make a
payment' tab as an amount that COULD be applied, but isn't automatically
applied
6) in your terminal, run the reconcile_balances.pl script
7) Confirm the error does not show in the logs and the balance for
the borrower is correctly reconciled.

Sponsored-by: Horowhenua District Council
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Martin Renvoize 2020-04-14 07:36:52 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 20 Jonathan Druart 2020-04-20 09:37:57 UTC
*** Bug 25148 has been marked as a duplicate of this bug. ***
Comment 21 Joy Nelson 2020-05-04 21:39:15 UTC
backported to 19.11.x for 19.11.06
Comment 22 Lucas Gass 2020-05-13 21:16:37 UTC
backported to 19.05.x. for 19.05.11