Bug 24009

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`

Created attachment 95272 [details] [review]
Bug 24009: Add Unit Tests

I like this. It is in the lines of what we've been thinking when Koha::Account was conceived, right Kyle?

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>

Created attachment 95346 [details] [review]
Bug 24009: Add Unit Tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

I understood that filter_by* must be methods, not argument.

Should not we have ->outstanding_debits->filter_by_blocks_issue instead?

Good catch... we've hardly settled on a pattern yet but I think your right, that's the closest we got so far.

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>

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>

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>

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.

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

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

See bug 11983.

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.

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 :)