The patch for bug 11352 introduces a routine in tools/cleanborrowers.pl called skip_borrowers_with_nonzero_balance. This filters out patrons who have active fine balances so that the patron purge does not inadvertently delete/trash them.
This logic should be put in a central routine, e.g., perhaps in GetBorrowersToExpunge(). At present, misc/cronjobs/delete_patrons.pl also has logic to not purge patrons with fine balances, but it implements it differently.
Having two different implementations of the same functionality is not great, but I (in my role as 3.16 RM) allowed it because of the potential for data loss that the patch for bug 11352 fixed. However, this situation should not be allowed to persist.
I intend to work on this but won't do so until after 20271 is in as it will drastically affect the code.
Created attachment 76227 [details] [review]
Bug 11983: Centralised Koha::Patrons method OPTION 1
Created attachment 76228 [details] [review]
Bug 11983: Centralised Koha::Patrons method OPTION 2
Feedback wanted on just the Koha::Patrons method options here before I write the associated tests and hook the code up to the scripts and tools.
IMO Option 1 will be hard to maintain, and is not very readable.
Option 2 is more verbose but clearer. See also Koha/Virtualshelves.pm for existing examples.
Created attachment 76701 [details] [review]
Bug 11983: Centralised Koha::Patrons method OPTION 2
The option 2 patch now includes the beginnings of tests (I still need to test method chaining).
Would appreciate any further feedback on this option.. method names, how they're constructed, whether it's a reasonable idea and should form the basis of a coding guideline to improve the consistency of Koha::Objects internal apis etc.
So the core proposal here is that we stop adding `->search_for_functionality_x` methods to the DB level Koha::Objects and rather add generalized filters for complex joins etc if we need to.
If we really want to add a `for_functionality_x` method/routine to centralize code for say patron manipulations (where we may have a UI and Script driven element) we should break that out into a subclass of some kind that contains the functions/methods for just that area of functionality. Koha::Patron::BulkActions perhaps as an example.. containing `->bulk_selection(filter_a, filter_b)` which internally just calls either a large constructed query (like the code in option 1) or a chained set of queries (from option 2) and then various available `->bulk_actions` that may be applied to the selected set. `->bulk_delete`, `bulk_anonymise`, `bulk_modify` for example?
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.
- 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:
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.
Thanks for the review, Jonathan :)
I far prefer option 2 too now I've written it. I think splitting out the final combinations for script + tool interfaces into their own subclasses also makes sense.
Regarding your remarks:
- Interesting regarding the join.. perhaps my brain is failing me but I couldn't see how I could do it without the having (whilst maintaining chainability). Happy to be guided here.. I might ask ribasushi for his thoughts there :)
- Coming up with method names was almost the hardest bit of this and I'm more than happy to be advised here.. What I would like to see is a fairly standardised recommendation for method names of this type as a guideline. We came up with 'filter_by_' to denote that you are filtering the resultset down. Following that by the 'how' and 'what' makes sense. It's describing the 'how' and 'what' which is causing fun here..
- `last_issued` could be more clearly written as `when_last_issued` perhaps?
- oh, did I get the standardisation wrong.. I thought we'd gone from 'checkout' to 'issue', my bad.. that should also be corrected in the above one too.
- I'm fine with has_pending_checkouts, I can't think of anything better or more descriptive. I think it is used in cleanborrowers (only it's hidden inside other filters)
- 'has' vs 'have'.. hmm, we're in Koha::Patrons so 'have' is right.. thanks
- I actually made this one flexible at someone's request.. though I can't remember who.. perhaps it would be better as distinct methods.. more than happy to get some discussion going here.
I shall try to raise all this at the next dev meeting.. see if we can bring some more people into the conversation.
Hmm. At first glance option 1 shows less code lines and more flexibility with parameters. It might be easier to switch one parameter than selecting another method each time ?
But no strong opinion here, and not studied it in detail..
Sounds good to me. I'm all for having method naming conventions.
With Perl's "method names as strings" (https://perldoc.perl.org/perlobj.html#Method-Call-Variations), I think you can get the flexibility of Option 1 while using the code in Option 2.
my $method = "filter_by_$arg";
my $filtered = $obj->$method();
And yeah by standardizing method naming conventions, I think we could potentially see some code re-use for methods that are more complex than your average DIBC methods but less complex than super unique methods. You can do some cool stuff with aliasing functions so that you can have different module-specific names for the same generic method as well, so you might get that relevant API while still having code re-use.
I see nothing to disagree with (which must be novel considering it's me). Awesome ideas, Martin!