Bug 35001 - Simplify patron->is_active in light of TrackLastPatronActivityTriggers
Summary: Simplify patron->is_active in light of TrackLastPatronActivityTriggers
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Martin Renvoize
URL:
Keywords: rel_23_11_candidate
Depends on: 35027 35030
Blocks:
  Show dependency treegraph
 
Reported: 2023-10-06 11:06 UTC by Martin Renvoize
Modified: 2023-10-26 07:01 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00


Attachments
Bug 35001: Take patron activity triggers into account in ->is_active (2.70 KB, patch)
2023-10-12 14:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35001: Simplify ->is_active in light of activity triggers (8.09 KB, patch)
2023-10-13 07:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35001: Simplify ->is_active in light of activity triggers (8.11 KB, patch)
2023-10-13 08:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35001: (folllow-up) Tiny addition to cronjob help (1.61 KB, patch)
2023-10-13 08:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35001: Add some checks to membership_expiry (2.75 KB, patch)
2023-10-13 08:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35001: Add some checks to membership_expiry (2.78 KB, patch)
2023-10-13 08:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35001: Simplify ->is_active in light of activity triggers (8.16 KB, patch)
2023-10-24 13:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35001: Add some checks to membership_expiry (2.81 KB, patch)
2023-10-24 13:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2023-10-06 11:06:24 UTC
Bug 33245 beat bug 15504 into Koha.. unfortunately this leads to the new method contained a series of hard coded queries for what the author felt are 'activity'.. Bug 15504 puts this into the hands of the librarians by exposing a preference for listing various actions that should trigger the lastseen update.

We should update the is_active method to reduce the hard coded database calls used and utilise the activity stored in lastseen as defined by the Triggers preference.
Comment 1 Marcel de Rooy 2023-10-06 14:47:43 UTC
(In reply to Martin Renvoize from comment #0)
> Bug 33245 beat bug 15504 into Koha.. unfortunately this leads to the new
> method contained a series of hard coded queries for what the author felt are
> 'activity'.. Bug 15504 puts this into the hands of the librarians by
> exposing a preference for listing various actions that should trigger the
> lastseen update.
> 
> We should update the is_active method to reduce the hard coded database
> calls used and utilise the activity stored in lastseen as defined by the
> Triggers preference.

'for what the author felt'
Does not sound very good? But okay. The 'author' asked for feedback on report 28688 which is still in the pipeline but was the starting point actually. Alas he got no feedback at all. Waited quite a bit too. So yes, I made a choice.

Will have a look at the other report soon.
Comment 2 Marcel de Rooy 2023-10-12 09:23:04 UTC
I would like to adjust this myself. But lets add hold and artreq first.
Comment 3 Marcel de Rooy 2023-10-12 14:11:56 UTC
Created attachment 156971 [details] [review]
Bug 35001: Take patron activity triggers into account in ->is_active

Still in concept.

Test plan:
TODO
Comment 4 Marcel de Rooy 2023-10-12 14:25:40 UTC
Martin: Just a first draft. My idea is that we need a query as long as this action is not in a trigger. If it is a trigger, lastseen should already give you the answer. The more triggers one has, the less extra queries are needed.

Although a library may not add e.g. hold to a trigger (for some reason or even no reason), I strongly believe that we should check if (in that case) holds prove activity. Because the decision to send or not send an expiry notice may depend on it, or even expire or autorenew a patron. 
Same for the other queries.

What could be a problem too, is the transition period between activating a trigger and the number of months used in membership expiry cron (say 6 months).
If you activated the trigger yesterday, we are no longer looking back in holds etc. But lastseen has not been updated for holds in the last 6 months.
This may be a bit fictional since many holds were placed after login. But could be an issue.

Appreciate some feedback before continuing.
Comment 5 Martin Renvoize 2023-10-12 16:03:08 UTC
This looks reasonable to me.. I must admit I'm still a little confused as to need for a hard coded 'what is activity' at all.  The point of adding the triggers was to put it in the administrator's gift to define what is constituted as activity?
Comment 6 Marcel de Rooy 2023-10-13 06:27:07 UTC
Since lastseen is updated already for logins and issues, I might wanna give up on the holds and issues queries.

But what about article requests? We have users that may place article requests but have no holds or issues. The fast majority will have logged in. So we are talking about a very small number of patrons that let staff add article requests.

Okay. I surrender. I will pick another battle :)
Comment 7 Marcel de Rooy 2023-10-13 07:15:47 UTC
Created attachment 157016 [details] [review]
Bug 35001: Simplify ->is_active in light of activity triggers

The fact that we update lastseen for various triggers now, makes
it a bit easier to let go of those 'hardcoded queries' ;)

Test plan:
Run t/db_dependent/Koha/Patron.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2023-10-13 07:38:23 UTC
Hmm. A thought keeps lingering about installs that disable the triggers completely. I would not recommend that, but how do we warn them that running e.g. membership_expiry with --active may impact them in some unexpected and undesired way? Still not sure there.
Comment 9 Marcel de Rooy 2023-10-13 07:43:21 UTC
Maybe add a check on the pref in membership_expiry ? I will add a follow-up for that.
Comment 10 Marcel de Rooy 2023-10-13 07:59:45 UTC
Back to Assigned
Comment 11 Martin Renvoize 2023-10-13 08:26:31 UTC
Created attachment 157034 [details] [review]
Bug 35001: Simplify ->is_active in light of activity triggers

The fact that we update lastseen for various triggers now, makes
it a bit easier to let go of those 'hardcoded queries' ;)

Test plan:
Run t/db_dependent/Koha/Patron.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2023-10-13 08:26:33 UTC
Created attachment 157035 [details] [review]
Bug 35001: (folllow-up) Tiny addition to cronjob help

This patch adds a tiny addendum to the -active documentation returned by
the membership_expiry cronjob to clarify what constitutes as an active
patron and hint at the new TrackLastPatronActivityTriggers system
preference.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2023-10-13 08:28:35 UTC
Oop, sorry Marcel.. I added my SO line before checking back on the bug.

Happy to re-add it if you want to push over the top of my attachment.

I'll take a look again once you're done.
Comment 14 Marcel de Rooy 2023-10-13 08:32:43 UTC
Created attachment 157036 [details] [review]
Bug 35001: Add some checks to membership_expiry

Test plan:
Toggle pref TrackLastPatronActivityTriggers (clear it).
And try variations of --active, --inactive to see the warnings.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Martin Renvoize 2023-10-13 08:58:16 UTC
Created attachment 157037 [details] [review]
Bug 35001: Add some checks to membership_expiry

Test plan:
Toggle pref TrackLastPatronActivityTriggers (clear it).
And try variations of --active, --inactive to see the warnings.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2023-10-13 08:59:17 UTC
Good followup, better than the wording change I had done.

QA scripts are happy and the function all works perfectly.

Passing QA
Comment 17 Tomás Cohen Arazi 2023-10-24 13:05:38 UTC
Please rebase ASAP. Thanks!
Comment 18 Marcel de Rooy 2023-10-24 13:32:40 UTC
Created attachment 157743 [details] [review]
Bug 35001: Simplify ->is_active in light of activity triggers

The fact that we update lastseen for various triggers now, makes
it a bit easier to let go of those 'hardcoded queries' ;)

Test plan:
Run t/db_dependent/Koha/Patron.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Marcel de Rooy 2023-10-24 13:32:42 UTC
Created attachment 157744 [details] [review]
Bug 35001: Add some checks to membership_expiry

Test plan:
Toggle pref TrackLastPatronActivityTriggers (clear it).
And try variations of --active, --inactive to see the warnings.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Tomás Cohen Arazi 2023-10-24 18:45:18 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 21 Fridolin Somers 2023-10-26 07:01:21 UTC
Depends on Bug 35027 not in 23.05.x