https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11983 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |In Discussion --- Comment #9 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I prefer the second approach even if more verbose. It's easier to read and to test. It will certainly be easier to maintain and less error prone. Few remarks: - The having could be replaced with a join, and so more efficient. - filter_by_last_issued sounds wrong. I agree we should have a pattern (here "filter_by_") to explicitly tell what we are doing, but it should also be meaningful. Maybe it is for native English speakers ;) - filter_by_has_issues($options) is not enough explicit IMO. Moreover $options is a boolean, and "issue" must be "checkout". I would suggest: $patrons->filter_by_has_pending_checkouts and another one (?) But maybe I am making things more complicated here. (note: it seems that this subroutine will not be used by cleanborrowers.pl, we do not have this use case) - Should not filter_by_has_issues be filter_by_have_issues? - filter_by_when_expired - the interface allows to filter by "expired before", now we provide a "and/or expire after", but it is not used. It means more code, more tests and so... more potential bugs. It is not a big deal for this one, but we could have filter_by_expired_before that will be easier to read/write/test. Not blocker but worth mentioning/discussing it I think. -- You are receiving this mail because: You are watching all bug changes.