Bug 15156 - Get all Borrowers with pending/unpaid fines/accountlines
Summary: Get all Borrowers with pending/unpaid fines/accountlines
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Emmi Takkinen
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 15157
  Show dependency treegraph
 
Reported: 2015-11-09 11:00 UTC by Olli-Antti Kivilahti
Modified: 2022-12-12 21:24 UTC (History)
15 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement adds an API call that returns patrons filtered by how much money they owe, between passed limits (for example less than $2.50 or more than $0.50). Optionally, can limit to debts of a particular debit_type and owed to a particular library.
Version(s) released in:
22.05.00


Attachments
Bug 15156 - Get all Borrowers with pending/unpaid fines/accountlines (5.50 KB, patch)
2015-11-09 11:00 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 15156 - Get all Borrowers with pending/unpaid fines/accountlines (5.74 KB, patch)
2015-11-09 11:18 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 15156 - Get all Borrowers with pending/unpaid fines/accountlines (5.87 KB, patch)
2016-01-04 14:51 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 15156: Get all Borrowers with pending/unpaid fines/accountlines (4.20 KB, patch)
2020-03-03 09:57 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 15156: Get all Borrowers with pending/unpaid fines/accountlines (4.01 KB, patch)
2020-06-09 07:56 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 15156: Get all Borrowers with pending/unpaid fines/accountlines (4.07 KB, patch)
2020-09-03 09:01 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 15156: Get all patrons with owings (3.97 KB, patch)
2020-09-29 05:36 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 15156: Get all patrons with owings (4.06 KB, patch)
2021-06-24 05:49 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 15156: Get all patrons with owings (4.11 KB, patch)
2021-06-24 14:38 UTC, David Nind
Details | Diff | Splinter Review
Bug 15156: [ALT] Add filter_by_amount_owed that's chainable (8.41 KB, patch)
2021-07-02 08:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15156: [ALT] Add filter_by_amount_owed that's chainable (9.02 KB, patch)
2021-07-02 10:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15156: Add filter_by_amount_owed that's chainable (9.06 KB, patch)
2021-09-03 15:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15156: Add filter_by_amount_owed that's chainable (9.11 KB, patch)
2021-09-03 22:49 UTC, David Nind
Details | Diff | Splinter Review
Bug 15156: Add filter_by_amount_owed that's chainable (9.17 KB, patch)
2021-09-15 07:19 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 15156: Add filter_by_amount_owed that's chainable (9.22 KB, patch)
2021-09-17 11:27 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Olli-Antti Kivilahti 2015-11-09 11:00:08 UTC
A new API call.

Useful when doing group operations for Borrowers with fines.

Includes some test-coverage for C4::Accountlines since it was missing any.
Comment 1 Olli-Antti Kivilahti 2015-11-09 11:00:41 UTC Comment hidden (obsolete)
Comment 2 Olli-Antti Kivilahti 2015-11-09 11:18:46 UTC Comment hidden (obsolete)
Comment 3 Frédéric Demians 2015-11-10 12:57:37 UTC
I can't apply this patch on master:

fatal: sha1 information is lacking or useless (C4/Accounts.pm).
error: could not build fake ancestor
Comment 4 Marc Véron 2015-12-16 20:18:19 UTC
(In reply to Frédéric Demians from comment #3)
> I can't apply this patch on master:
> 
> fatal: sha1 information is lacking or useless (C4/Accounts.pm).
> error: could not build fake ancestor

Same problem here. Setting to Patch doesn't apply.
Comment 5 Mark Tompsett 2016-01-04 14:51:42 UTC
Created attachment 46232 [details] [review]
Bug 15156 - Get all Borrowers with pending/unpaid  fines/accountlines

A new API call.
Useful when doing group operations for Borrowers with fines.
Includes some test-coverage for C4::Accountlines since it was missing any.

NOTE: Renamed test file to t/db_dependent/Accounts/...
Comment 6 Emmi Takkinen 2020-02-24 09:05:47 UTC
Maybe this needs to be reworked entirely? Bug 12603 introduced TestBuilder module so bug 13906 might not be valid anymore. Also I think GetAllBorrowersWithUnpaidFines should be implemented in Patrons.pm instead of C4::Accounts.pm.
Comment 7 Katrin Fischer 2020-02-24 21:04:39 UTC
(In reply to Emmi Takkinen from comment #6)
> Maybe this needs to be reworked entirely? Bug 12603 introduced TestBuilder
> module so bug 13906 might not be valid anymore. Also I think
> GetAllBorrowersWithUnpaidFines should be implemented in Patrons.pm instead
> of C4::Accounts.pm.

I think you might be on the right track here - I think removing the dependency on bug 13906 would be good. Also adding Tomas and Martin.
Comment 8 Emmi Takkinen 2020-03-03 09:57:46 UTC
Created attachment 99981 [details] [review]
Bug 15156: Get all Borrowers with pending/unpaid fines/accountlines

This patch adds method to fetch all borrowers with unpaid
fines.

To test:
prove t/db_dependent/Koha/Patrons.t

Sponsored-by: Koha-Suomi Oy
Comment 9 Martin Renvoize 2020-06-08 15:39:15 UTC
Comment on attachment 99981 [details] [review]
Bug 15156: Get all Borrowers with pending/unpaid fines/accountlines

Review of attachment 99981 [details] [review]:
-----------------------------------------------------------------

::: Koha/Account/Lines.pm
@@ +23,4 @@
>  use Koha::Account::Line;
>  
>  use base qw(Koha::Objects);
> +use List::MoreUtils qw( uniq );

Unless I'm missing it.. you don't actually use 'uniq' here?
Comment 10 Emmi Takkinen 2020-06-09 07:56:42 UTC
Created attachment 105656 [details] [review]
Bug 15156: Get all Borrowers with pending/unpaid fines/accountlines

Rebased patch and removed unnecessary line of code from Lines.pm.
Comment 11 Emmi Takkinen 2020-08-27 05:53:52 UTC
Removed dependency on bug 13906
Comment 12 Fridolin Somers 2020-09-03 09:01:43 UTC
Created attachment 109593 [details] [review]
Bug 15156: Get all Borrowers with pending/unpaid fines/accountlines

This patch adds method to fetch all borrowers with unpaid
fines.

To test:
prove t/db_dependent/Koha/Patrons.t

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 13 Fridolin Somers 2020-09-03 09:04:37 UTC
Can really be usefull, signing.

Just a note :
Since account is not only for fines (there can be copy fees, subscription payement ...) maybe use a more generic name for the method "search_patrons_with_unpaid_fines".
With "owing" maybe, like in C4::Members : 
$flaginfo{'message'} = sprintf 'Patron owes %.02f', $owing
Comment 14 Martin Renvoize 2020-09-11 11:16:57 UTC
Whilst I understand the idea behind this, I'm not sure the implementation is good.

We iterate through all patrons in the resultset (so, if you're calling this without having first chained it to a search) we will literally fetch all borrowers from the db.

We're also returning a weird hasref rather than a set of Koha::Patron objects which you could then work with further to produce the data you want.

I would also suggest that it should support the standard attribute params available to normal searches to allow for filtering.

So.. all in all I feel we can do better.. this method should produce a joined query that can be used inline with a method chain on Koha::Patrons...

What I'de really like to see is something along the lines:

my $filtered_patrons = Koha::Patrons->search({ whatever })->filter_by_owes({amount => { '>=' => $max_owed }});
Comment 15 Emmi Takkinen 2020-09-16 10:48:41 UTC
Looking back to this after feedback and I agree, this implementation isn't ideal. At least method should return Patron object. I'll work on it again based on feedback.
Comment 16 Martin Renvoize 2020-09-16 10:51:15 UTC
Thanks Emmi; I hope I didn't come across too negative.. I'd happily pick up the bug if I had some time to do so.. I'll certainly keep and eye out on it and try to help with testing/qa as needed.
Comment 17 Emmi Takkinen 2020-09-21 11:00:02 UTC
(In reply to Martin Renvoize from comment #16)
> Thanks Emmi; I hope I didn't come across too negative.. I'd happily pick up
> the bug if I had some time to do so.. I'll certainly keep and eye out on it
> and try to help with testing/qa as needed.

Not at all and thanks beforehand :)    

Some thoughts I had about this:

1. I'm not sure if owes is quite right. Owings seems more appropriate (or maybe debts?). 
2. Maybe we could ditch whole search? Instead we could first search patrons against wanted criteria and then do:

my $patrons = Koha::Patrons->filter_by_owings({ 'me.borrowernumber' => { '-in' => \@borrowernumbers }, amount => { '>=' => $max } });
Comment 18 Emmi Takkinen 2020-09-29 05:36:46 UTC
Created attachment 110886 [details] [review]
Bug 15156: Get all patrons with owings

This patch adds method to return set of unique patrons with
accountlines matching filtering params.

To test:
prove t/db_dependent/Koha/Patrons.t
Comment 19 Martin Renvoize 2021-06-23 14:56:13 UTC
Oh, totally forgot about this one, I'm really sorry Emmi.

I actually introduced a similar method in bug 11983 though.. so perhaps worth comparing.

I'll try to bring that one back up to date, updating tests etc.. it's been on my list for a long time :S
Comment 20 Hakam Almotlak 2021-06-23 18:58:18 UTC
patch does not apply on master

Applying: Bug 15156: Get all patrons with owings
Using index info to reconstruct a base tree...
M	Koha/Patrons.pm
M	t/db_dependent/Koha/Patrons.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Koha/Patrons.t
CONFLICT (content): Merge conflict in t/db_dependent/Koha/Patrons.t
Auto-merging Koha/Patrons.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 15156: Get all patrons with owings
Comment 21 Tomás Cohen Arazi 2021-06-23 18:59:45 UTC
Please fix the test name while rebasing.
Comment 22 Emmi Takkinen 2021-06-24 05:49:56 UTC
Created attachment 122355 [details] [review]
Bug 15156: Get all patrons with owings

This patch adds method to return set of unique patrons with
accountlines matching filtering params.

To test:
prove t/db_dependent/Koha/Patrons.t
Comment 23 David Nind 2021-06-24 14:38:28 UTC
Created attachment 122392 [details] [review]
Bug 15156: Get all patrons with owings

This patch adds method to return set of unique patrons with
accountlines matching filtering params.

To test:
prove t/db_dependent/Koha/Patrons.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 24 Martin Renvoize 2021-07-02 08:09:30 UTC
Created attachment 122528 [details] [review]
Bug 15156: [ALT] Add filter_by_amount_owed that's chainable

This patch acts as an alternative here. We pull out the code from bug
11983 and extend it to support filtering of debts by branchcode and
debit_type.

The advantage of this approach is that we can add the filter into the
middle of a search chain to filter down an pre-existing search.
Comment 25 Martin Renvoize 2021-07-02 08:12:55 UTC
I've love to get peoples thoughts on my alternative implimentation.. I've split out the relevant code I wrote for bug 11983 so we don't have to rely on that entire bug working it's way through.

It still needs a couple more tests adding to prove the branchcode/debit_type refinement.

I think it's really beneficial to be able to chain this.. I can see a really clear use case for needing to filter to those patrons visible to the user prior to using this function...?
Comment 26 Martin Renvoize 2021-07-02 10:02:12 UTC
Created attachment 122534 [details] [review]
Bug 15156: [ALT] Add filter_by_amount_owed that's chainable

This patch acts as an alternative here. We pull out the code from bug
11983 and extend it to support filtering of debts by branchcode and
debit_type.

The advantage of this approach is that we can add the filter into the
middle of a search chain to filter down an pre-existing search.
Comment 27 Martin Renvoize 2021-07-02 10:02:47 UTC
Tests added
Comment 28 Marcel de Rooy 2021-07-02 11:58:03 UTC
+    return $self
+      unless ( defined($options)
+        && ( $options->{less_than} || $options->{more_than} ) );

Would this raise questions if people only pass library or debit_type and get much more than expected ?
Comment 29 Marcel de Rooy 2021-07-02 12:03:36 UTC
+    my $group_by =
+      [ map { 'me.' . $_ } $self->_resultset->result_source->columns ];
+
+    my $attrs = {
+        join     => 'accountlines',
+        group_by => $group_by,

You probably dont want to group by on all patron columns, heh ?
Comment 30 Tomás Cohen Arazi 2021-07-02 13:28:16 UTC
(In reply to Martin Renvoize from comment #25)
> I've love to get peoples thoughts on my alternative implimentation.. I've
> split out the relevant code I wrote for bug 11983 so we don't have to rely
> on that entire bug working it's way through.
> 
> It still needs a couple more tests adding to prove the branchcode/debit_type
> refinement.
> 
> I think it's really beneficial to be able to chain this.. I can see a really
> clear use case for needing to filter to those patrons visible to the user
> prior to using this function...?

I love your alternate patch. It is in line with what we've been promoting the last couple years in the codebase.
Comment 31 Martin Renvoize 2021-09-03 15:36:13 UTC
(In reply to Marcel de Rooy from comment #29)
> +    my $group_by =
> +      [ map { 'me.' . $_ } $self->_resultset->result_source->columns ];
> +
> +    my $attrs = {
> +        join     => 'accountlines',
> +        group_by => $group_by,
> 
> You probably dont want to group by on all patron columns, heh ?

Actually, that's required by strict mode SQL.. we're already doing it all over the place.
Comment 32 Martin Renvoize 2021-09-03 15:40:52 UTC
Created attachment 124497 [details] [review]
Bug 15156: Add filter_by_amount_owed that's chainable

This patch acts as an alternative here. We pull out the code from bug
11983 and extend it to support filtering of debts by branchcode and
debit_type.

The advantage of this approach is that we can add the filter into the
middle of a search chain to filter down an pre-existing search.
Comment 33 Martin Renvoize 2021-09-03 15:42:42 UTC
(In reply to Marcel de Rooy from comment #28)
> +    return $self
> +      unless ( defined($options)
> +        && ( $options->{less_than} || $options->{more_than} ) );
> 
> Would this raise questions if people only pass library or debit_type and get
> much more than expected ?

Hmm, I'm not so sure.. we could throw an exception I suppose.. the filter expects/requires these options to be passed to make any sense.
Comment 34 David Nind 2021-09-03 22:49:37 UTC
Created attachment 124508 [details] [review]
Bug 15156: Add filter_by_amount_owed that's chainable

This patch acts as an alternative here. We pull out the code from bug
11983 and extend it to support filtering of debts by branchcode and
debit_type.

The advantage of this approach is that we can add the filter into the
middle of a search chain to filter down an pre-existing search.

Signed-off-by: David Nind <david@davidnind.com>
Comment 35 David Nind 2021-09-03 22:51:45 UTC
Tested by running prove t/db_dependent/Koha/Patrons.t before and after patch applied - tests pass (using koha-testing-docker).
Comment 36 Emmi Takkinen 2021-09-15 07:19:48 UTC
Created attachment 124876 [details] [review]
Bug 15156: Add filter_by_amount_owed that's chainable

This patch acts as an alternative here. We pull out the code from bug
11983 and extend it to support filtering of debts by branchcode and
debit_type.

The advantage of this approach is that we can add the filter into the
middle of a search chain to filter down an pre-existing search.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 37 Nick Clemens 2021-09-17 11:27:41 UTC
Created attachment 124978 [details] [review]
Bug 15156: Add filter_by_amount_owed that's chainable

This patch acts as an alternative here. We pull out the code from bug
11983 and extend it to support filtering of debts by branchcode and
debit_type.

The advantage of this approach is that we can add the filter into the
middle of a search chain to filter down an pre-existing search.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 38 Jonathan Druart 2021-11-08 11:02:03 UTC
As bug 15157 is not ready for 21.11 it does not make sense to have this one part of 21.11. Postponed.
Comment 39 Fridolin Somers 2021-12-11 20:22:29 UTC
I think this can be useful in other places like a patrons search filter.
I'm looking at it for master
Comment 40 Fridolin Somers 2021-12-14 02:12:31 UTC
Useful to find who will have no Christmas gifts 🎅
Comment 41 Fridolin Somers 2021-12-14 02:12:57 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 42 David Nind 2021-12-18 21:30:50 UTC
I attempted to add release notes, but as I don't really understand the this and the terminology it may need reviewing and updating!