Bug 11983

Summary: Code to select patrons to purge needs to be centralized (use of filter_by)
Product: Koha Reporter: Galen Charlton <gmcharlt>
Component: Architecture, internals, and plumbingAssignee: Martin Renvoize <martin.renvoize>
Status: Failed QA --- QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: colin.campbell, dcook, jonathan.druart+koha, katrin.fischer, m.de.rooy, martin.renvoize, nick, tomascohen, victor
Version: masterKeywords: rel_22_05_candidate
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 11352    
Bug Blocks: 16846    
Attachments: Bug 11983: Centralised Koha::Patrons method OPTION 1
Bug 11983: Centralised Koha::Patrons method OPTION 2
Bug 11983: Centralised Koha::Patrons method OPTION 2
Bug 11983: Centralised Koha::Patrons method OPTION 2
Bug 11983: Centralised Koha::Patrons method OPTION 2
Bug 11983: Centralised Koha::Patrons method OPTION 2
Bug 11983: Centralised Koha::Patrons filtering methods
Bug 11983: (follow-up) Terminology fixes

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!
Comment 13 Martin Renvoize 2020-12-29 16:37:13 UTC
Created attachment 114730 [details] [review]
Bug 11983: Centralised Koha::Patrons method OPTION 2

Comment 14 Martin Renvoize 2020-12-29 16:39:00 UTC
Created attachment 114731 [details] [review]
Bug 11983: Centralised Koha::Patrons method OPTION 2
Comment 15 Martin Renvoize 2020-12-29 16:40:27 UTC
Rebased OPTION 2 as it seemed to be the one we landed on.. I can't for the life of me get rid of that last HAVING in the amountsoutstanding..

I think this is ready for review again at this point.. running the included tests should be enough for SO
Comment 16 Victor Grousset/tuxayo 2020-12-29 18:00:04 UTC
Created attachment 114735 [details] [review]
Bug 11983: Centralised Koha::Patrons method OPTION 2

== Test plan ==
prove t/db_dependent/Koha/Patrons.t

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 17 Nick Clemens 2021-01-08 14:21:16 UTC
(In reply to Victor Grousset/tuxayo from comment #16)
> Created attachment 114735 [details] [review] [review]
> Bug 11983: Centralised Koha::Patrons method OPTION 2
> == Test plan ==
> prove t/db_dependent/Koha/Patrons.t
> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Couple questions:
1 - It looks like the wording from commet 9 was not done? has->have issues->checkouts
2 - filter_by_in_lists/filter_by_not_in_lists - for guarantors/guarantees the in/not-in option is a parameter, couldn't we do the same here?
3 - Where did the list for which functions we are defining come from? search_patrons_to_update_category could use some of these, but also needs a 'filter_by_age' or similar
4 - Is there a second bug for using these new methods? An example one would be nice
Comment 18 Martin Renvoize 2021-01-11 15:54:41 UTC
Created attachment 115040 [details] [review]
Bug 11983: Centralised Koha::Patrons filtering methods

This patch adds various filter_by_* methods to Koha::Patrons to aid in
complex filtering situations.

== Test plan ==
prove t/db_dependent/Koha/Patrons.t

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 19 Martin Renvoize 2021-01-11 15:54:45 UTC
Created attachment 115041 [details] [review]
Bug 11983: (follow-up) Terminology fixes

As discussed, this patch updates the class (and tests) to reflect the
agreed upon terms
Comment 20 Nick Clemens 2021-02-05 17:01:49 UTC
Hi Martin,

Sorry, I should have read the proposed language closer - 
pending_checkouts is confusing, I think it can just be 'checkouts'

I wonder about the methods for filtering
Why do you search issues for borrowernumber, then use an in? Would it be simpler to use a single query? I think it is clearer to do something like:
$self->search("issues.borrowernumber" => { -not => undef } },{join=>"issues"});

Later for housebound_choosers we do a search related to housebound_roles, then another to borrowernumber - this works, but it comes out as "FROM borrowers JOIN housebound_role JOIN borrowers"

Comment 21 Martin Renvoize 2021-02-05 17:16:53 UTC
Thanks Nick, 

I've dug out my branch and it looks like I started added more tests for chaining and combination of these filters.. I'll look at working through completing those tests and take into account your comments.. I like the cleaner JOIN you suggest.. though we may need to start naming our joins to maintain chain-ability..

I can imagine issues getting joined multiple times and then us ending up with queries clashing.
Comment 22 Nick Clemens 2021-02-05 17:30:54 UTC
Also QA failure:
		sort (...) interpreted as function 
		sort (...) interpreted as function