Bug 24009

Summary: Koha::Account->outstanding_debits should support inline filtering
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Architecture, internals, and plumbingAssignee: Martin Renvoize <martin.renvoize>
Status: In Discussion --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, kyle, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 24007    
Attachments: Bug 24009: Add filter_by option to outstanding_debits
Bug 24009: Add Unit Tests
Bug 24009: Add filter_by option to outstanding_debits
Bug 24009: Add Unit Tests
Bug 24009: Add Unit Tests
Bug 24009: Add filter_by option to outstanding_debits
Bug 24009: Add Unit Tests

Description Martin Renvoize 2019-11-11 14:23:22 UTC
We have a number of places where we want to get a specific list of outstanding debits for comparison.. at the moment one would have to construct a search directly by inspecting system preferences at the controller level.

By addins a filter option to oustanding_debits we can move that back to the object level without having to add additional methods.
Comment 1 Martin Renvoize 2019-11-11 15:46:32 UTC
Created attachment 95271 [details] [review]
Bug 24009: Add filter_by option to outstanding_debits

This patch adds an optional 'filter_by' arguament to
Koha::Account->outstanding_debits which current accepts 'blocks_issue'
and will filter down the result set of outstanding_debits based upon the
values of the following system preferences.

* `HoldsInNoissuesCharge`
* `RentalsInNoissuesCharge`
* `ManInvInNoissuesCharge`
Comment 2 Martin Renvoize 2019-11-11 15:46:35 UTC
Created attachment 95272 [details] [review]
Bug 24009: Add Unit Tests
Comment 3 Tomás Cohen Arazi 2019-11-12 18:14:30 UTC
I like this. It is in the lines of what we've been thinking when Koha::Account was conceived, right Kyle?
Comment 4 Tomás Cohen Arazi 2019-11-12 18:16:39 UTC
Created attachment 95345 [details] [review]
Bug 24009: Add filter_by option to outstanding_debits

This patch adds an optional 'filter_by' argument to
Koha::Account->outstanding_debits which current accepts 'blocks_issue'
and will filter down the result set of outstanding_debits based upon the
values of the following system preferences.

* `HoldsInNoissuesCharge`
* `RentalsInNoissuesCharge`
* `ManInvInNoissuesCharge`

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi 2019-11-12 18:16:43 UTC
Created attachment 95346 [details] [review]
Bug 24009: Add Unit Tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Jonathan Druart 2019-12-27 14:48:11 UTC
I understood that filter_by* must be methods, not argument.

Should not we have ->outstanding_debits->filter_by_blocks_issue instead?
Comment 7 Martin Renvoize 2020-01-15 16:43:28 UTC
Good catch... we've hardly settled on a pattern yet but I think your right, that's the closest we got so far.
Comment 8 Bouzid Fergani 2020-02-14 21:03:28 UTC
Created attachment 99041 [details] [review]
Bug 24009: Add Unit Tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 9 Bouzid Fergani 2020-02-14 21:04:33 UTC
Created attachment 99042 [details] [review]
Bug 24009: Add filter_by option to outstanding_debits

This patch adds an optional 'filter_by' argument to
Koha::Account->outstanding_debits which current accepts 'blocks_issue'
and will filter down the result set of outstanding_debits based upon the
values of the following system preferences.

* `HoldsInNoissuesCharge`
* `RentalsInNoissuesCharge`
* `ManInvInNoissuesCharge`

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 10 Bouzid Fergani 2020-02-14 21:04:38 UTC
Created attachment 99043 [details] [review]
Bug 24009: Add Unit Tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 11 Jonathan Druart 2020-03-26 15:04:42 UTC
Did not we decided to have ->filter_by_* methods?
I do not think we should introduce this pattern (ie. having filter_by in parameter), we may end up with huge and complex methods.

However I really like the idea of ->filter_by_* chained methods to filter a set.
Comment 12 Jonathan Druart 2020-03-26 15:06:05 UTC
(In reply to Jonathan Druart from comment #11)
> Did not we decided to have ->filter_by_* methods?
> I do not think we should introduce this pattern (ie. having filter_by in
> parameter), we may end up with huge and complex methods.
> 
> However I really like the idea of ->filter_by_* chained methods to filter a
> set.

I repeated what I said few months ago in comment 6 actually.
Comment 13 Tomás Cohen Arazi 2020-03-26 18:05:16 UTC
(In reply to Jonathan Druart from comment #11)
> Did not we decided to have ->filter_by_* methods?
> I do not think we should introduce this pattern (ie. having filter_by in
> parameter), we may end up with huge and complex methods.
> 
> However I really like the idea of ->filter_by_* chained methods to filter a
> set.

I don't remember that discussion to evolve more than what was proposed on that bug... what was it...

I think chaining filter_by_* could be really cool. I think Martin ended up implementing this way because we didn't have a clear consensus.

But I missed many dev meetings as well.
Comment 14 Jonathan Druart 2020-03-26 18:12:30 UTC
See bug 11983.
Comment 15 Martin Renvoize 2020-03-26 18:18:49 UTC
I've been trying to get back onto these two for ages.. bug 11983 is mid-rebase..

I'd love to see a consensus and movement on that bug and have it unlock a few patterns for us, but alas.. time is getting the better of me.
Comment 16 Jonathan Druart 2020-03-27 07:43:03 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2020-03-27 07:43:28 UTC
We have some filter_by_* methods already, and I wrote few more that are in the pipe. What about moving the parameter to a new method? That seems a quite trivial to get a PQA stamp :)