Bug 11983 - Code to select patrons to purge needs to be centralized (use of filter_by)
Summary: Code to select patrons to purge needs to be centralized (use of filter_by)
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
Depends on: 20271 11352
Blocks: 16846 20469
  Show dependency treegraph
Reported: 2014-03-22 13:53 UTC by Galen Charlton
Modified: 2020-02-14 11:17 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:

Bug 11983: Centralised Koha::Patrons method OPTION 1 (5.11 KB, patch)
2018-06-21 13:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 11983: Centralised Koha::Patrons method OPTION 2 (6.85 KB, patch)
2018-06-21 13:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 11983: Centralised Koha::Patrons method OPTION 2 (22.61 KB, patch)
2018-07-05 07:15 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Galen Charlton 2014-03-22 13:53:19 UTC
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.
Comment 1 Martin Renvoize 2018-06-13 11:27:39 UTC
I intend to work on this but won't do so until after 20271 is in as it will drastically affect the code.
Comment 2 Martin Renvoize 2018-06-21 13:20:21 UTC
Created attachment 76227 [details] [review]
Bug 11983: Centralised Koha::Patrons method OPTION 1
Comment 3 Martin Renvoize 2018-06-21 13:20:58 UTC
Created attachment 76228 [details] [review]
Bug 11983: Centralised Koha::Patrons method OPTION 2
Comment 4 Martin Renvoize 2018-06-21 13:21:49 UTC
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.
Comment 5 Jonathan Druart 2018-06-21 15:42:15 UTC
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.
Comment 6 Martin Renvoize 2018-07-05 07:15:12 UTC
Created attachment 76701 [details] [review]
Bug 11983: Centralised Koha::Patrons method OPTION 2
Comment 7 Martin Renvoize 2018-07-05 07:17:48 UTC
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.
Comment 8 Martin Renvoize 2018-07-06 15:58:11 UTC
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?

Comment 9 Jonathan Druart 2018-07-11 17:42:38 UTC
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:
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.
Comment 10 Martin Renvoize 2018-07-19 09:20:53 UTC
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.
Comment 11 Marcel de Rooy 2018-07-19 12:11:18 UTC
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..
Comment 12 David Cook 2018-07-20 01:56:51 UTC
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!