|
Description
Martin Renvoize (ashimema)
2023-10-06 11:06:24 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. I would like to adjust this myself. But lets add hold and artreq first. Created attachment 156971 [details] [review] Bug 35001: Take patron activity triggers into account in ->is_active Still in concept. Test plan: TODO 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. 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? 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 :) 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> 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. Maybe add a check on the pref in membership_expiry ? I will add a follow-up for that. Back to Assigned 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> 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> 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. 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> 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> Good followup, better than the wording change I had done. QA scripts are happy and the function all works perfectly. Passing QA Please rebase ASAP. Thanks! 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> 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> Pushed to master for 23.11. Nice work everyone, thanks! Depends on Bug 35027 not in 23.05.x |