[Koha-bugs] [Bug 11983] code to select patrons to purge needs to be centralized

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11983

Jonathan Druart <jonathan.druart at bugs.koha-community.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |In Discussion

--- Comment #9 from Jonathan Druart <jonathan.druart at 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.