Bug 25683 - update_patron_categories.pl should recognize no fine history = 0 outstanding fines
Summary: update_patron_categories.pl should recognize no fine history = 0 outstanding ...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on: 17168
Blocks:
  Show dependency treegraph
 
Reported: 2020-06-05 19:58 UTC by Andrew Fuerste-Henry
Modified: 2020-09-20 14:16 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.03, 19.11.09


Attachments
Bug 25683: (bug 17168 follow-up) Don't consider negative balance (3.24 KB, patch)
2020-06-08 08:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25683: [alternate] Patron with no accountlines should have 0 outstanding (not NULL) (3.11 KB, patch)
2020-06-09 10:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25683: [alternate] Patron with no accountlines should have 0 outstanding (not NULL) (3.04 KB, patch)
2020-06-09 12:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25683: Fix grouping of results (2.96 KB, patch)
2020-06-10 12:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25683: [alternate] Patron with no accountlines should have 0 outstanding (not NULL) (3.08 KB, patch)
2020-06-10 17:26 UTC, David Nind
Details | Diff | Splinter Review
Bug 25683: Fix grouping of results (3.03 KB, patch)
2020-06-10 17:26 UTC, David Nind
Details | Diff | Splinter Review
Bug 25683: (follow-up) Use COALESCE instead of IFNULL (1.21 KB, patch)
2020-08-04 18:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25683: Patron with no accountlines should have 0 outstanding (not NULL) (3.47 KB, patch)
2020-08-05 16:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25683: Fix grouping of results (3.09 KB, patch)
2020-08-05 16:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25683: (follow-up) Use COALESCE instead of IFNULL (1.28 KB, patch)
2020-08-05 16:53 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2020-06-05 19:58:53 UTC
Bug 17168 introduced update_patron_categories.pl, which can change patron categories based on fine totals. But it doesn't understand that an absence of fines is the same as $0 owed.

To replicate:
- Have a patron with nothing in accountlines
- run update_patron_categories to find patrons with fines under $5 (-fu=5)
- Your patron is not found
- Give your patron a manual charge of $1
- rerun the cron, your patron is found
- pay off your patron's fine, putting their balance at $0
- rerun the cron, your patron is found
Comment 1 Jonathan Druart 2020-06-08 08:13:49 UTC
Created attachment 105620 [details] [review]
Bug 25683: (bug 17168 follow-up) Don't consider negative balance

Test plan:
- Have a patron with nothing in accountlines
- run update_patron_categories to find patrons with fines under $5 (-fu=5)
- Your patron is not found
- Give your patron a manual charge of $1
- rerun the cron, your patron is found
- pay off your patron's fine, putting their balance at $0
- rerun the cron, your patron is not found
Comment 2 Jonathan Druart 2020-06-08 08:14:34 UTC
Isn't it actually a hidden feature to search for negative balance (credit)?

If not you can test this patch!
Comment 3 Nick Clemens 2020-06-09 10:52:47 UTC
Created attachment 105660 [details] [review]
Bug 25683: [alternate] Patron with no accountlines should have 0 outstanding (not NULL)
Comment 4 Nick Clemens 2020-06-09 10:53:28 UTC
(In reply to Jonathan Druart from comment #2)
> Isn't it actually a hidden feature to search for negative balance (credit)?
> 
> If not you can test this patch!

I think the issue is that sum(amountoutstanding) is 'NULL' for patrons with no account history, it should be treated as 0
Comment 5 Nick Clemens 2020-06-09 12:01:22 UTC
Created attachment 105662 [details] [review]
Bug 25683: [alternate] Patron with no accountlines should have 0 outstanding (not NULL)
Comment 6 Jonathan Druart 2020-06-09 12:10:11 UTC
Nick, both changes are valid I think.
My patch is fixing what is describe in comment 0.
Comment 7 Andrew Fuerste-Henry 2020-06-09 12:15:46 UTC
I don't seem to have explained the issue well up front. Nick's patch addresses what I was looking at. Jonathan, I don't entirely follow what your patch is doing or why.
Comment 8 Jonathan Druart 2020-06-09 12:23:59 UTC
There is a difference between no accountlines and sum(accountlines)=0.

If I am dealing with the situation you described in comment 0: A patron had fines, but paied all of them: their balance is 0.
Comment 9 Nick Clemens 2020-06-09 12:50:31 UTC
(In reply to Jonathan Druart from comment #8)
> There is a difference between no accountlines and sum(accountlines)=0.
> 
> If I am dealing with the situation you described in comment 0: A patron had
> fines, but paied all of them: their balance is 0.

So your patch makes it be that a patron with 0 fines will not be returned if only a fine max is supplied? That is, it adds an assumption that you only want patrons with any fines if you supply a fine under amount?  I do not think this is desired behaviour. I think the final step in comment 0 was a correct return and step 3 was the error
Comment 10 Jonathan Druart 2020-06-09 13:14:01 UTC
I obsoleted my patch.
Comment 11 David Nind 2020-06-10 10:08:05 UTC
I'm a bit confused with this one, sorry!

Using J patron category and Bobbie Middleton in koha-testing-docker - no transactions to start with.

Before applying the patch, results are:

  perl update_patrons_category.pl -f J -t J -fu=5 -v
  No actions will be taken (test mode)
  Will update patrons from J to J with conditions below (if any)
  Conditions:
      Total fines less than 5
  0 found, 0 updated

After adding a charge of $1.00:

  perl update_patrons_category.pl -f J -t J -fu=5 -v
  No actions will be taken (test mode)
  Will update patrons from J to J with conditions below (if any)
  Conditions:
      Total fines less than 5
  WOULD HAVE Updated Bobbie Middleton from J to J
  1 found, 0 updated

After paying off charge (balance is $0, two transactions with charge and payment) - two lines appear for patron, even though the their balance is $0:

  perl update_patrons_category.pl -f J -t J -fu=5 -v
  No actions will be taken (test mode)
  Will update patrons from J to J with conditions below (if any)
  Conditions:
      Total fines less than 5
  WOULD HAVE Updated Bobbie Middleton from J to J
  WOULD HAVE Updated Bobbie Middleton from J to J
  1 found, 0 updated

After applying the patch:

  perl update_patrons_category.pl -f J -t J -fu=5 -v
  No actions will be taken (test mode)
  Will update patrons from J to J with conditions below (if any)
  Conditions:
      Total fines less than 5
  WOULD HAVE Updated Bobbie Middleton from J to J
  WOULD HAVE Updated Bobbie Middleton from J to J
  WOULD HAVE Updated Gordon Peterson from J to J
  WOULD HAVE Updated Mae Kinney from J to J
  3 found, 0 updated

After applying the patch and adding another charge - Bobbie Middleton appears three times:

  perl update_patrons_category.pl -f J -t J -fu=5 -v
  No actions will be taken (test mode)
  Will update patrons from J to J with conditions below (if any)
  Conditions:
      Total fines less than 5
  WOULD HAVE Updated Bobbie Middleton from J to J
  WOULD HAVE Updated Bobbie Middleton from J to J
  WOULD HAVE Updated Bobbie Middleton from J to J
  WOULD HAVE Updated Gordon Peterson from J to J
  WOULD HAVE Updated Mae Kinney from J to J
  3 found, 0 updated

Every transaction added increases the number of times they appear.

If the above is what is expected then I'm happy to sign off!

==> Outcome after applying the patch is that all patrons for the category with charges under 5 should appear, even if they have a 0 balance
Comment 12 Nick Clemens 2020-06-10 12:31:09 UTC
Created attachment 105690 [details] [review]
Bug 25683: Fix grouping of results

DBIX doesn't seem to work as I expected - if trying to group by we can only
get the columns we grouped by, otherwise the queries are much more complicated
and come out wrong

This patch removes the 'amountoutstanding' from the query. Note that we do return patron
object, however, to access all the columns we must discard_changes (or refetch)

Also fixes a typo 'verbosse'

To test:
1 - charge a $1 fine to a patron
2 - pay off the fine
3 - perl misc/cronjobs/update_patrons_category.pl -f J -t J -fu=5 -v
4 - Note the patron is returned twice
5 - export DBIC_TRACE=1
6 - repeat 3 - view the SQL query and see how odd it is
7 - Apply this patch
8 - repeat 3 - simpler query
9 - patron returned only once
Comment 13 David Nind 2020-06-10 17:26:06 UTC
Created attachment 105704 [details] [review]
Bug 25683: [alternate] Patron with no accountlines should have 0 outstanding (not NULL)

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2020-06-10 17:26:10 UTC
Created attachment 105705 [details] [review]
Bug 25683: Fix grouping of results

DBIX doesn't seem to work as I expected - if trying to group by we can only
get the columns we grouped by, otherwise the queries are much more complicated
and come out wrong

This patch removes the 'amountoutstanding' from the query. Note that we do return patron
object, however, to access all the columns we must discard_changes (or refetch)

Also fixes a typo 'verbosse'

To test:
1 - charge a $1 fine to a patron
2 - pay off the fine
3 - perl misc/cronjobs/update_patrons_category.pl -f J -t J -fu=5 -v
4 - Note the patron is returned twice
5 - export DBIC_TRACE=1
6 - repeat 3 - view the SQL query and see how odd it is
7 - Apply this patch
8 - repeat 3 - simpler query
9 - patron returned only once

Signed-off-by: David Nind <david@davidnind.com>
Comment 15 Katrin Fischer 2020-06-14 12:57:18 UTC
Can you please check the QA fail?

 FAIL	Koha/Patrons.pm
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: IFNULL (must be replaced by COALESCE) (line 402)
		forbidden pattern: IFNULL (must be replaced by COALESCE) (line 403)
Comment 16 Nick Clemens 2020-08-04 18:29:09 UTC
Created attachment 107792 [details] [review]
Bug 25683: (follow-up) Use COALESCE instead of IFNULL
Comment 17 Martin Renvoize 2020-08-05 16:53:29 UTC
Created attachment 107850 [details] [review]
Bug 25683: Patron with no accountlines should have 0 outstanding (not NULL)

Test plan:
- Have a patron with nothing in accountlines
- run update_patron_categories to find patrons with fines under $5 (-fu=5)
  - Your patron is not found
  - Give your patron a manual charge of $1
  - rerun the cron, your patron is found
  - pay off your patron's fine, putting their balance at $0
  - rerun the cron, your patron is found

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2020-08-05 16:53:33 UTC
Created attachment 107851 [details] [review]
Bug 25683: Fix grouping of results

DBIX doesn't seem to work as I expected - if trying to group by we can only
get the columns we grouped by, otherwise the queries are much more complicated
and come out wrong

This patch removes the 'amountoutstanding' from the query. Note that we do return patron
object, however, to access all the columns we must discard_changes (or refetch)

Also fixes a typo 'verbosse'

To test:
1 - charge a $1 fine to a patron
2 - pay off the fine
3 - perl misc/cronjobs/update_patrons_category.pl -f J -t J -fu=5 -v
4 - Note the patron is returned twice
5 - export DBIC_TRACE=1
6 - repeat 3 - view the SQL query and see how odd it is
7 - Apply this patch
8 - repeat 3 - simpler query
9 - patron returned only once

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2020-08-05 16:53:36 UTC
Created attachment 107852 [details] [review]
Bug 25683: (follow-up) Use COALESCE instead of IFNULL

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2020-08-05 16:54:11 UTC
Works as expected, no regressions found, passes qa scripts.

Passing QA
Comment 21 Jonathan Druart 2020-08-06 09:38:03 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 22 Lucas Gass 2020-08-14 21:18:31 UTC
backported to 20.05.x for 20.05.03
Comment 23 Aleisha Amohia 2020-08-16 22:39:13 UTC
backported to 19.11.x for 19.11.09
Comment 24 Victor Grousset/tuxayo 2020-09-20 14:16:43 UTC
Missing dependencies for 19.05.x, can't backport.