Bug 15156

I can't apply this patch on master:

fatal: sha1 information is lacking or useless (C4/Accounts.pm).
error: could not build fake ancestor

(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.

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/...

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.

(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.

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 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?

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.

Removed dependency on bug 13906

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>

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

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 }});

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.

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.

(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 } });

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

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

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

Please fix the test name while rebasing.

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

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>

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.

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...?

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.

Tests added

+    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 ?

+    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 ?

(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.

(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.

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.

(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.

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>

Tested by running prove t/db_dependent/Koha/Patrons.t before and after patch applied - tests pass (using koha-testing-docker).

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>

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>

As bug 15157 is not ready for 21.11 it does not make sense to have this one part of 21.11. Postponed.

I think this can be useful in other places like a patrons search filter.
I'm looking at it for master

Useful to find who will have no Christmas gifts ﷐[U+1F385]﷑

Pushed to master for 22.05, thanks to everybody involved ﷐[U+1F984]﷑

I attempted to add release notes, but as I don't really understand the this and the terminology it may need reviewing and updating!