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.
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 them few that are in the pipe. What about moving the parameter to a new method? That seems a quite trivial to get a PQA stamp :)
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 :)