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
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
Isn't it actually a hidden feature to search for negative balance (credit)? If not you can test this patch!
Created attachment 105660 [details] [review] Bug 25683: [alternate] Patron with no accountlines should have 0 outstanding (not NULL)
(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
Created attachment 105662 [details] [review] Bug 25683: [alternate] Patron with no accountlines should have 0 outstanding (not NULL)
Nick, both changes are valid I think. My patch is fixing what is describe in comment 0.
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.
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.
(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
I obsoleted my patch.
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
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
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>
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>
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)
Created attachment 107792 [details] [review] Bug 25683: (follow-up) Use COALESCE instead of IFNULL
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>
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>
Created attachment 107852 [details] [review] Bug 25683: (follow-up) Use COALESCE instead of IFNULL Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Works as expected, no regressions found, passes qa scripts. Passing QA
Pushed to master for 20.11, thanks to everybody involved!
backported to 20.05.x for 20.05.03
backported to 19.11.x for 19.11.09
Missing dependencies for 19.05.x, can't backport.