Work to be done: Add new timestamp field ‘last_activity’ to the ‘borrowers’ table Update the field if one of the following ocurrs: Patron checks out item Patron renews an item Patron returns an item Patron is authenticated via SIP2 Patron logs into OPAC Seeking Co-sponsors: http://devs.bywatersolutions.com/projects/track-patrons-last-activity/
Please consider having another field that would record the type of activity logged, just like you have in your list. Christopher
I noticed recently that "lastseen" updates for "Patron logs into OPAC" and "Patron is authenticated via SIP2" and that's it. I've had libraries interested in the field being updated for the other activities Nicole mentioned as well.
(In reply to David Cook from comment #2) > I noticed recently that "lastseen" updates for "Patron logs into OPAC" and > "Patron is authenticated via SIP2" and that's it. I've had libraries > interested in the field being updated for the other activities Nicole > mentioned as well. Which ones? I do not understand what's missing. Marked as duplicate of bug 16276 as borrowers.lastseen has been added by this bug. *** This bug has been marked as a duplicate of bug 16276 ***
(In reply to Jonathan Druart from comment #3) > (In reply to David Cook from comment #2) > > I noticed recently that "lastseen" updates for "Patron logs into OPAC" and > > "Patron is authenticated via SIP2" and that's it. I've had libraries > > interested in the field being updated for the other activities Nicole > > mentioned as well. > > Which ones? I do not understand what's missing. > > Marked as duplicate of bug 16276 as borrowers.lastseen has been added by > this bug. > > *** This bug has been marked as a duplicate of bug 16276 *** Nicole listed 5 activities: Patron checks out item Patron renews an item Patron returns an item Patron is authenticated via SIP2 Patron logs into OPAC Only the last two update "lastseen". If you're a borrower who never uses a self-checkout or logs into the OPAC, but you frequently check out materials via a staff member, your "lastseen" value will be really old even though you're a frequent user of the library. I've had libraries complain that they're trying to find inactive borrowers using "lastseen" but it's including active borrowers who don't log into Koha.
So I don't think this is a duplicate at all. I'm going to change the status back.
Indeed, you are right. The description of bug 16276 says: """ We would like to automate the delation of expired patrons to comply to french laws regarding privacy. But we don't want to delate an expired patron which is still using the OPAC for bibliographical purpose, like managing lists. We would like to be able to say when the patron logged himself for the last time to permit this. """ It could be renamed last_login to avoid ambiguity.
Except that calling it last_login doesn't indicate the SIP2 connection, which is important. In our case, a patron may use their card 100% for databases or digital materials, so tracking the SIP2 is key for this.
*** Bug 14513 has been marked as a duplicate of this bug. ***
Summary so far - A timestamp, updated by checkout / renew / return / SIP2 / OPAC login. (Nicole) The type of activity needs to be indicated as well as the timestamp. (Christopher) SIP2 activity needs to be tracked. (Martha) Noted "Lastseen" seems to capture OPAC login and authenticated by SIP. This would be the "updated" timestamp displayed in the patron summary. My thoughts - It would help to know more about how people want to use this timestamp. Is it for reports (inactive patrons for weeding, or activity for statistics), or to be visible to staff in the patron record, like the "Updated" timestamp? We would need clearer labels - "updated" implies but does not actually indicate when the borrower record was renewed or edited. Checkout / renew / return (whether through intranet, OPAC or SIP) are one kind of activity, and are captured already in circulation history. OPAC login and SIP (e.g. for authentication of outside systems like Overdrive) are another, and they are also already captured somewhere but not clear to staff. Should they be handled separately or feed into a single last activity date? I would also like a way to update that activity timestamp without checking out. Something like the "statistical borrower" but a "statistical item" that could be scanned to record library activity not connected to Koha. For example, people who use computers or sign up for programmes. Maybe there is one I have missed. I agree with Martha that tracking SIP2 connections is important, and perhaps should be addressed in more detail. We use it for self-checkout, computer booking (in-library activities) and authentication for accessing Overdrive, RBDigital (external resources). Some label suggestions - last-login (OPAC or self-checkout), updated (patron details changed through OPAC or Intranet), last-activity (activity from any channel including SIP), remote-use (outside resource SIP connections and OPAC login). Just suggestions, for wider discussion.
(In reply to Agnes Rivers-Moore from comment #9) > Some label suggestions - last-login (OPAC or self-checkout), updated (patron > details changed through OPAC or Intranet), last-activity (activity from any > channel including SIP), remote-use (outside resource SIP connections and > OPAC login). > Just suggestions, for wider discussion. I'm concerned with over complicating the information. In my opinion, there are a couple things we want to track here: * Patron record modification * Patron activity (login/checkouts through any connection) Timestamps are generic, and a catch-all. That is good for logs, but the two above are more specific. These are two that need to be recorded in the patron record, that we don't have to scrounge for in logs that are not necessarily recorded, or are purged on a regular basis. I'm not sure about last-activity or remote-use. Those are a little more granular and bleed over into the other two mentioned above and might make it more confusing. But this is my opinion.
Agree with Christopher about the library activities - we just want the labels to clearly describe what activity sets the timestamp. To clarify - Martha and I definitely want the SIP2 activity to be tracked separately from checkouts, because SIP2 includes activities that are outside Koha circulation.
So maybe: * Patron record modification * Any patron activity (login/checkouts through any connection) * SIP patron activity (part of previous count)
Folks, this is a rather important issue. Would love to see this progress beyond 4 years of rare discussion. :D
To add to this, it is really holding us back on tracking active patrons. We cannot clean out our patron databases on inactive accounts, because currently Koha is not tracking activity through 3rd party databases or computer checkouts via SIP authentication. This really needs some momentum.
(In reply to Christopher Brannon from comment #13) > Folks, this is a rather important issue. Would love to see this progress > beyond 4 years of rare discussion. :D I agree that it's important, but it's not a priority for us at this time. I notice this issue is marked as "Seeking cosponsors". It might be worthwhile to send an email to the koha-devel list to see if someone wants to work on it, or if you have funds - look to hire someone to do the work.
(In reply to David Cook from comment #2) > I noticed recently that "lastseen" updates for "Patron logs into OPAC" and > "Patron is authenticated via SIP2" and that's it. IIUC all the data is already there albeit in various tables right? So, is the functionality achievable via a report? (which could be included by default & upgrades)
(In reply to David Cook from comment #15) > (In reply to Christopher Brannon from comment #13) > > Folks, this is a rather important issue. Would love to see this progress > > beyond 4 years of rare discussion. :D > > I agree that it's important, but it's not a priority for us at this time. > > I notice this issue is marked as "Seeking cosponsors". It might be > worthwhile to send an email to the koha-devel list to see if someone wants > to work on it, or if you have funds - look to hire someone to do the work. The link to dev list is a dead link. "Not a priority for us"...who is "US"?
Chiming in again. This is ESPECIALLY important for us now, with COVID-19 and social distancing procedures, we have more people using 100% online resources - and people who have gotten cards using online registration, verified via email, to use those resources. Tracking last patron activity via SIP2/OPAC login is definitely a priority for US at Huntsville-Madison County. Best, -Marti Fuerst
(In reply to Martha Fuerst from comment #18) > Chiming in again. > > This is ESPECIALLY important for us now, with COVID-19 and social distancing > procedures, we have more people using 100% online resources - and people who > have gotten cards using online registration, verified via email, to use > those resources. > > Tracking last patron activity via SIP2/OPAC login is definitely a priority > for US at Huntsville-Madison County. > > Best, > > -Marti Fuerst +1 - COVID-19 has certainly increased the urgency on this!
(In reply to Christopher Brannon from comment #17) > The link to dev list is a dead link. http://devs.bywatersolutions.com/projects/track-patrons-last-activity/ isn't the dev list. It was a page ByWater Solutions had on this topic. The mailing lists are here: https://koha-community.org/support/koha-mailing-lists/ «General List» and «Koha Developers List» seems to me the good ones to seek organizations to cosponsor or/and to contract to work on this. (assuming SQL report are not enough?)
(In reply to Victor Grousset/tuxayo from comment #20) > (assuming SQL report are not enough?) As was mentioned, pulling stuff from activity logs is hit or miss. Depending on how often information is purged, you won't get very accurate results. This is data that is best recorded in the patron record or related table.
(In reply to Christopher Brannon from comment #21) > (In reply to Victor Grousset/tuxayo from comment #20) > > > (assuming SQL report are not enough?) > > As was mentioned, pulling stuff from activity logs is hit or miss. I missed that part. What do you mean by activity logs? The issues and old_issues tables? Because if I understand correctly, the following data is in the above tables + borrowers.lastseen > Patron checks out item > Patron renews an item > Patron returns an item > Patron is authenticated via SIP2 > Patron logs into OPAC
I must be missing something here. Where is the information on patron logins to OPAC or authentication via SIP2 that isn't in a log? I'm not sure what tables I should be looking at for this information.
(In reply to Christopher Brannon from comment #23) > I must be missing something here. Where is the information on patron logins > to OPAC or authentication via SIP2 that isn't in a log? borrowers.lastseen IIUC
Please double check, as a non-native speaker, misunderstandings are bound to happen someday :) (In reply to David Cook from comment #2) > I noticed recently that "lastseen" updates for "Patron logs into OPAC" and > "Patron is authenticated via SIP2" and that's it. I've had libraries > interested in the field being updated for the other activities Nicole > mentioned as well.
(In reply to Christopher Brannon from comment #17) > "Not a priority for us"...who is "US"? Ah, fair question! I mean the vendor I work for Prosentient Systems and the libraries that it supports. None of our libraries are signalling this as a priority for us, so we're working on other things that are more important for them. I should've been more precise with my language :).
Agreeing w/ Martha & Agnes that lastseen dates should also reflect patron checkout, renewal, & return activity.
In order to be complete, I think we need to expand this: Patron is authenticated via SIP2 into at least this: Patron is authenticated via SIP2, ILS-DI, REST API We have ebook services that authenticate patrons via ILS-DI, and if that is all they use the library for they will not be counted as active users.
I would like to suggest, respectfully, that the bug title be changed to describe this topic more accurately. As it stands it appears to be about tracking last Koha activity, but we really need tracking activity in outside resources via SIP2, ILS-DI, REST API
*** Bug 27243 has been marked as a duplicate of this bug. ***
Relevat code: C4/SIP/ILS/Patron.pm: 476 =head2 update_lastseen 477 478 $patron->update_lastseen(); 479 480 Patron method to update lastseen field in borrower 481 to record that patron has been seen via sip connection 482 483 =cut 484 485 sub update_lastseen { 486 my $self = shift; 487 my $kohaobj = Koha::Patrons->find( $self->{borrowernumber} ); 488 $kohaobj->track_login if $kohaobj; # track_login checks the pref 489 } Koha/Patron.pm: 910 =head3 track_login 911 912 $patron->track_login; 913 $patron->track_login({ force => 1 }); 914 915 Tracks a (successful) login attempt. 916 The preference TrackLastPatronActivity must be enabled. Or you 917 should pass the force parameter. 918 919 =cut 920 921 sub track_login { 922 my ( $self, $params ) = @_; 923 return if 924 !$params->{force} && 925 !C4::Context->preference('TrackLastPatronActivity'); 926 $self->lastseen( dt_from_string() )->store; 927 }
Bug 28455, signed off, adds checkouts to lastseen. So we're kinda working in two different directions here. I'm in favor of a simple approach here: a single field with a datetime that updates each time a patron does any of the listed actions (log in to OPAC, SIP, ILSDI, API, checkout, checkin). The batch patron deletion tool already knows how to filter by both lastseen and circ activity. The need I run into here relates to reports rather than the deletion tool itself. We get regular requests for a report that will give a column of the last date on which a patron either logged in to something or checked something in or out. That's much messier SQL than it needs to be with the current data structure.
(In reply to Andrew Fuerste-Henry from comment #32) > Bug 28455, signed off, adds checkouts to lastseen. So we're kinda working in > two different directions here. > > I'm in favor of a simple approach here: a single field with a datetime that > updates each time a patron does any of the listed actions (log in to OPAC, > SIP, ILSDI, API, checkout, checkin). > > The batch patron deletion tool already knows how to filter by both lastseen > and circ activity. The need I run into here relates to reports rather than > the deletion tool itself. We get regular requests for a report that will > give a column of the last date on which a patron either logged in to > something or checked something in or out. That's much messier SQL than it > needs to be with the current data structure. +1. I would add, if it is a need for anyone to define what triggers this field, a permission with checkboxes of actions that would trigger it would finish this off nicely.
Created attachment 152332 [details] [review] Bug 15504: Add new syspref TrackLastPatronActivityTriggers This patch adds a new syspref TrackLastPatronActivityTriggers to determine which actions should update borrowers.lastseen
Created attachment 152333 [details] [review] Bug 15504: Update track_login_daily to accept triggers This patch adds triggers to track_login_daily so that it only tracks activity when that trigger is active
Created attachment 152334 [details] [review] Bug 15504: Add triggers to instances of track_login_daily This patch adds a trigger to every instance of track_login_daily
Created attachment 152335 [details] [review] Bug 15504: Remove update_lastseen method This patch removes update_lastseen which has been replaced by a call to track_login_daily
Created attachment 152336 [details] [review] Bug 15504: Add unit test prove -v t/db_dependent/Auth.t
This bug hasn’t been active for a while but we recently had a support ticket from a customer that seemed to fit into this discussion. By comparing that with the discussion on this bug I’ve prepared a patch that will hopefully satisfy the requirement of both. There are now two sysprefs - “TrackLastPatronActivity” and “TrackLastPatronActivityTriggers”. The second of these offers a list of the following: - login: "Login to OPAC" - connection: "Connection to Koha via SIP or ILSDI" - check_out: "Checking out an item" - renewal: "Renewing an item" - check_in: "Returning an item" When TrackLastPatronActivity is enabled and one or more of these triggers is selected, borrowers.lastseen will update when that trigger happens. This will only happen once per day as per the current method. If this structure/concept is approved then expanding it with further options should be fairly simple if required.
Notes on testing - The intent of track_login_daily is to log a patron action/activity once per day and cache this activity. This means that the lastseen field is only updated once per day rather than every time an action is performed. This makes sense in operation but makes testing this patch more complicated. We want to test multiple methods of updating the lastseen field. If we don’t clear that cache before each method then the patch will work as intended but the test plan will fail as only one update will be performed. The test plan therefore includes a “restart_all” step at various points before an activity is performed - this step uses “restart_all” in KTD to clear the cache and must not be skipped otherwise the test plan will fail. Furthermore, the “Login to OPAC” action, called in checkauth is triggered very often throughout Koha through various route guards etc. The test plan begins by testing this activity first and then de-selecting this option. If this option is left selected, then it will trigger before the other actions (check in, check out etc) can be triggered and the test plan will fail due to the login action already being cached again. Test plan: 1. Apply patch 2. reset_all 3. Navigate to Administration and search for “TrackLastPatronActivity” 4. Observe that there are two system preferences - “TrackLastPatronActivity” and “TrackLastPatronActivityTriggers” 5. Change TrackLastPatronActivity to “Do” and save 6. Choose a patron to use as a test subject 7. In the database, observe that the “lastseen” property for that patron is still NULL 8. In TrackLastPatronActivityTriggers, select “Login to OPAC” and save 9. In the OPAC, log in as your selected patron 10. In the database, observe that the “lastseen” property for that patron has been updated 11. In TrackLastPatronActivityTriggers, unselect “Login to OPAC” and instead select “Checking out an item” and save 12. restart_all 13. Check out an item to your selected patron 14. In the database, observe that the “lastseen” property for that patron has been updated 15. In TrackLastPatronActivityTriggers, select “Renewing an item” and save 16. restart_all 17. In either the OPAC or the staff client - renew the item that you have just checked out to that patron 18. In the database, observe that the “lastseen” property for that patron has been updated 19. In TrackLastPatronActivityTriggers, select “Returning an item” and save 20. restart_all 21. Return the item that you have checked out to the patron 22. In the database, observe that the “lastseen” property for that patron has been updated 23. In TrackLastPatronActivityTriggers, select “Connection to Koha via SIP or ILSDI” and save 24. In system preferences, search for “ILS-DI” and enable this preference 25. restart_all 26. In the browser, navigate to http://DOMAIN/cgi-bin/koha/ilsdi.pl?service=AuthenticatePatron&username=USERNAME&password=PASSWORD, swapping out DOMAIN, USERNAME and PASSWORD to match your koha instance and patron details 27. In the database, observe that the “lastseen” property for that patron has been updated 28. Run “prove -v t/db_dependent/Auth.t” 29. Observe that all tests pass 30. Sign off!
Created attachment 152500 [details] [review] Bug 15504: Add new syspref TrackLastPatronActivityTriggers This patch adds a new syspref TrackLastPatronActivityTriggers to determine which actions should update borrowers.lastseen Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 152501 [details] [review] Bug 15504: Update track_login_daily to accept triggers This patch adds triggers to track_login_daily so that it only tracks activity when that trigger is active Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 152502 [details] [review] Bug 15504: Add triggers to instances of track_login_daily This patch adds a trigger to every instance of track_login_daily Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 152503 [details] [review] Bug 15504: Remove update_lastseen method This patch removes update_lastseen which has been replaced by a call to track_login_daily Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 152504 [details] [review] Bug 15504: Add unit test prove -v t/db_dependent/Auth.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 152505 [details] [review] Bug 15504: (follow-up) Unit tests This patch removes, moves and updates the existing unit tests for track_login_daily and track_login to all sit under Koha::Patron and update_lastseen. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 152506 [details] [review] Bug 15504: (follow-up) Migrate to one clear method We were using a series of similarly named methods spread in distinct places around the codebase. This combines the logic of C4::Auth::track_login_daily and Koha::Patron->track_login into a new Koha::Patron->update_lastseen method. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Internal code review done, and a couple of follow-ups added as I felt we could clean up some of the code (not related to Matts work directly, but whilst I was in here I wanted to do the follow-ups) Nice work Matt, all working as designed in my testing.
Created attachment 155290 [details] [review] Bug 15504: Add new syspref TrackLastPatronActivityTriggers This patch adds a new syspref TrackLastPatronActivityTriggers to determine which actions should update borrowers.lastseen Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 155291 [details] [review] Bug 15504: Update track_login_daily to accept triggers This patch adds triggers to track_login_daily so that it only tracks activity when that trigger is active Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 155292 [details] [review] Bug 15504: Add triggers to instances of track_login_daily This patch adds a trigger to every instance of track_login_daily Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 155293 [details] [review] Bug 15504: Remove update_lastseen method This patch removes update_lastseen which has been replaced by a call to track_login_daily Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 155294 [details] [review] Bug 15504: Add unit test prove -v t/db_dependent/Auth.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 155295 [details] [review] Bug 15504: (follow-up) Unit tests This patch removes, moves and updates the existing unit tests for track_login_daily and track_login to all sit under Koha::Patron and update_lastseen. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 155296 [details] [review] Bug 15504: (follow-up) Migrate to one clear method We were using a series of similarly named methods spread in distinct places around the codebase. This combines the logic of C4::Auth::track_login_daily and Koha::Patron->track_login into a new Koha::Patron->update_lastseen method. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Rebased to drop dependency on bug 33997
Created attachment 155534 [details] [review] Bug 15504: Add new syspref TrackLastPatronActivityTriggers This patch adds a new syspref TrackLastPatronActivityTriggers to determine which actions should update borrowers.lastseen Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 155535 [details] [review] Bug 15504: Update track_login_daily to accept triggers This patch adds triggers to track_login_daily so that it only tracks activity when that trigger is active Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 155536 [details] [review] Bug 15504: Add triggers to instances of track_login_daily This patch adds a trigger to every instance of track_login_daily Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 155537 [details] [review] Bug 15504: Remove update_lastseen method This patch removes update_lastseen which has been replaced by a call to track_login_daily Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 155538 [details] [review] Bug 15504: Add unit test prove -v t/db_dependent/Auth.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 155539 [details] [review] Bug 15504: (follow-up) Unit tests This patch removes, moves and updates the existing unit tests for track_login_daily and track_login to all sit under Koha::Patron and update_lastseen. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 155540 [details] [review] Bug 15504: (follow-up) Migrate to one clear method We were using a series of similarly named methods spread in distinct places around the codebase. This combines the logic of C4::Auth::track_login_daily and Koha::Patron->track_login into a new Koha::Patron->update_lastseen method. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
I've added my sign-off as well. 8-) Testing notes (using KTD): 1. Followed test plan in comment #40. 2. For step 7, I: 2.1 used patron 49 (Mary), updated card number and password for testing 2.2 database checking (lastseen is near the end in the field before 'default' (lang column)): koha-mysql kohadev select * from borrowers where borrowernumber=49;
(In reply to David Nind from comment #63) > Created attachment 155540 [details] [review] [review] > Bug 15504: (follow-up) Migrate to one clear method my $patron = Koha::Patrons->find({ userid => $userid }); $patron->update_lastseen('login'); I've got one case when it returned empty $patron, so that led to: [ERROR] Can't call method "update_lastseen" on an undefined value at /usr/share/koha/lib/C4/Auth.pm line xxx. Yet it was when I tried to religion from an older session, can't reproduce 1:1 now, but giving a warning now for team + researching myself further.
(In reply to Andrii Nugged from comment #65) > Yet it was when I tried to religion from an older session, can't reproduce > 1:1 now, but giving a warning now for team + researching myself further. Own update: it's because $userid also comes empty to that code lines (in cases when old session? thinking aloud)
*** Bug 21007 has been marked as a duplicate of this bug. ***
One bug in SIP here: File: C4/SIP/Sip/MsgType.pm 0999: if ( C4::Context->preference('TrackLastPatronActivity') ) { 1000: my $koha_patron = Koha::Patrons->find($patron->userid); 1001: $koha_patron->update_lastseen('connection'); 1002: } 1003: $resp .= patron_status_string( $patron, $server ); ERROR: Cannot access 'userid' field of class 'C4::SIP::ILS::Patron' at /usr/share/koha/lib/C4/SIP/Sip/MsgType.pm That: my $koha_patron = Koha::Patrons->find($patron->userid); should be, probably, this way: my $koha_patron = Koha::Patrons->find( { cardnumber => $patron->id } );
Please switch to failed QA if there is a bug in testing.
Rebasing now bug 33245 is in. I think bug 33245 could be significantly simplified once this is in personally..
Created attachment 156598 [details] [review] Bug 15504: Add new syspref TrackLastPatronActivityTriggers This patch adds a new syspref TrackLastPatronActivityTriggers to determine which actions should update borrowers.lastseen Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156599 [details] [review] Bug 15504: Update track_login_daily to accept triggers This patch adds triggers to track_login_daily so that it only tracks activity when that trigger is active Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156600 [details] [review] Bug 15504: Add triggers to instances of track_login_daily This patch adds a trigger to every instance of track_login_daily Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156601 [details] [review] Bug 15504: Remove update_lastseen method This patch removes update_lastseen which has been replaced by a call to track_login_daily Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156602 [details] [review] Bug 15504: Add unit test prove -v t/db_dependent/Auth.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156603 [details] [review] Bug 15504: (follow-up) Unit tests This patch removes, moves and updates the existing unit tests for track_login_daily and track_login to all sit under Koha::Patron and update_lastseen. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156604 [details] [review] Bug 15504: (follow-up) Migrate to one clear method We were using a series of similarly named methods spread in distinct places around the codebase. This combines the logic of C4::Auth::track_login_daily and Koha::Patron->track_login into a new Koha::Patron->update_lastseen method. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156605 [details] [review] Bug 15504: (follow-up) Use borrowernumber in Koha::Patrons->find
Created attachment 156606 [details] [review] Bug 15504: (follow-up) Add Triggers mock to unit tests
Created attachment 156607 [details] [review] Bug 15504: (follow-up) Tidy 'lastseen' section of unit test
Created attachment 156608 [details] [review] Bug 15504: (follow-up) Fix is_active test Bug 33245 introduced a new is_active patron method and the corresponding tests included calls to the method we depreacte in this patchset
QA-question: This sub: sub update_lastseen { my ( $self, $activity ) = @_; return $self if !C4::Context->preference('TrackLastPatronActivity'); my $tracked_activities = { map { ( lc $_, 1 ); } split /\s*\,\s*/, C4::Context->preference('TrackLastPatronActivityTriggers') }; return $self unless $tracked_activities->{$activity}; ... is called very often (on each Auth-request, for each borrower, so each page load or whatever happens, then for every other event). Is that "split" of static variable from preference('TrackLastPatronActivityTriggers') justified to be called that way EVERY time, splitting again and again?
Created attachment 156742 [details] [review] Bug 15504: (QA follow-up) Add caching to update_lastseen This patch caches the activity triggers hash upon the first call to update_lastseen.
Final follow-up adds caching.. but I've not benchmarked it.. would be interesting to see if the cache fetch + check it's valid is any quicker than doing the split inline though...
> check it's valid is any quicker than doing the split inline though... you said this - it's reasonable. I agree that we need to use our hivemind to choose because it might be "six of one/half a dozen", if that's Memcached, then it's probably heavier than splitting. But if to use Perl-code cache, then it might be faster or close to. Hmmm.
Then invalidating the cache is another timely issue, and so on. Sic :) (now I feel guilty I even asked for that xD ).
This is cached in L2 (memcached) but L1 will be populated (memory). So the retrieval of the syspref should be fast. But here we are talking about caching the split of a very short string, and we are in perl, I am not sure it makes a lot of sense.
Let's leave split by Perl (revert attachment 156742 [details] [review] away) and add an explicit comment to the code why we are not caching for future generations of developers who will face this code?
Adn one more thing: Not covered HOLDS. When the borrower makes hold on themself (but this is slightly covered with the "login" part) but also when the operator makes hold on the borrower: should this be +1 place in the code + tracking option in the dropdown among "renewal, checkout, check-in"?
(In reply to Andrii Nugged from comment #89) > Not covered HOLDS. Recalls, article requests ? Note that they could be covered in a follow-up. But they seem relevant too.
Obsoleted final follow-up.. I'd really like to see this actually move forward rather than get stuck in discussion hell with ever expanding requirements.. If you want more options adding, please either actually submit code or create a new bug that hangs off this one.
Will be looking here very SOON
(In reply to Marcel de Rooy from comment #90) > (In reply to Andrii Nugged from comment #89) > > Not covered HOLDS. > > Recalls, article requests ? > Note that they could be covered in a follow-up. But they seem relevant too. Can an article request be triggered by staff on behalf of a user?
(In reply to Martin Renvoize from comment #93) > (In reply to Marcel de Rooy from comment #90) > > (In reply to Andrii Nugged from comment #89) > > > Not covered HOLDS. > > > > Recalls, article requests ? > > Note that they could be covered in a follow-up. But they seem relevant too. > > Can an article request be triggered by staff on behalf of a user? Yes.
(In reply to Marcel de Rooy from comment #90) > (In reply to Andrii Nugged from comment #89) > > Not covered HOLDS. > > Recalls, article requests ? Recalls cant be set in staff for another user. So not applicable here.
OK. This looks almost ready to me. No panic please. Here are some minor comments. The first and last one should get attention here imo. dbrev Blocker The default should probably be LOGIN IF TrackLastLogin was enabled. Please adjust. Preferences Clearing all triggers is the same as turning off existing pref? Could we remove it ? Here or in a follow-up ? my $cache_key = "track_login_" . $self->userid; No blocker. Would probably be better to cache with borrowernumber. Userid may change? + Time::Fake->offset( $now->epoch + 5 ); No blocker, just teasing :) You might have trouble in case you run the test close to midnight. Could we cache the epoch value ? -sub track_login { - my ( $self, $params ) = @_; - return if - !$params->{force} && Didnt we use force somewhere before? Login from OPAC => Login => Staff login counts as well. I think we should just remove OPAC here. This code still needs attention. Blocker koha-tmpl/intranet-tmpl/prog/en/modules/tools/cleanborrowers.tt [% IF Koha.Preference('TrackLastPatronActivity') %] <li> <label for="borrower_lastseen">who have not been connected since:</label> <input size="10" id="borrower_lastseen" name="borrower_lastseen" type="text" class="flatpickr" /> <span class="hint">[% INCLUDE 'date-format.inc' %]</span> </li> [% END %] AND Similarly: misc/cronjobs/delete_patrons.pl =item B<--last_seen> Delete patrons who have not been connected since this date. The system preference TrackLastPatronActivity must be enabled to use this option.
Opened bug 35050 to extend with placing hold or article request.
(In reply to Marcel de Rooy from comment #97) > Opened bug 35050 to extend with placing hold or article request. bug 35030
Doing 1000 splits in a for loop: $VAR1 = '1697034924.81935'; $VAR2 = '1697034924.82318'; $VAR3 = '0.00383496284484863'; Sp no big deal
I did not see 35027. So 35030 should only be article request.
(In reply to Marcel de Rooy from comment #96) > dbrev > Blocker > The default should probably be LOGIN IF TrackLastLogin was enabled. > Please adjust. Done, will post patch momentarily.. however, it's actually 'connection,login,check_out' to match the existing behaviour. > Preferences > Clearing all triggers is the same as turning off existing pref? Could we > remove it ? Here or in a follow-up ? I like that, perhaps as a quick follow-up bug.. I'm happy to code that tonight. > my $cache_key = "track_login_" . $self->userid; > No blocker. Would probably be better to cache with borrowernumber. Userid > may change? Totally agree on this one.. though we just used the existing key. I've amended it now as another follow-up anyway.. about to upload. > + Time::Fake->offset( $now->epoch + 5 ); > No blocker, just teasing :) > You might have trouble in case you run the test close to midnight. > Could we cache the epoch value ? Oh, this could be fun.. one for later though in my opinion.. should it crop up. > -sub track_login { > - my ( $self, $params ) = @_; > - return if > - !$params->{force} && > Didnt we use force somewhere before? It was supported before, but never actually used. Matt and I discussed it and decided to remove the functionality for now as it was never called that way. > Login from OPAC => Login > => Staff login counts as well. I think we should just remove OPAC here. Fair, I'll do a follow-up for that too before posting up the current rebase. > This code still needs attention. > Blocker > koha-tmpl/intranet-tmpl/prog/en/modules/tools/cleanborrowers.tt > [% IF Koha.Preference('TrackLastPatronActivity') %] > <li> > <label for="borrower_lastseen">who have not been > connected since:</label> > <input size="10" id="borrower_lastseen" > name="borrower_lastseen" type="text" class="flatpickr" /> > <span class="hint">[% INCLUDE 'date-format.inc' > %]</span> > </li> > [% END %] > AND Similarly: > misc/cronjobs/delete_patrons.pl > =item B<--last_seen> > Delete patrons who have not been connected since this date. > The system preference TrackLastPatronActivity must be enabled to use this > option. Looking at these two now.
Created attachment 156855 [details] [review] Bug 15504: Add new syspref TrackLastPatronActivityTriggers This patch adds a new syspref TrackLastPatronActivityTriggers to determine which actions should update borrowers.lastseen Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156856 [details] [review] Bug 15504: Update track_login_daily to accept triggers This patch adds triggers to track_login_daily so that it only tracks activity when that trigger is active Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156857 [details] [review] Bug 15504: Add triggers to instances of track_login_daily This patch adds a trigger to every instance of track_login_daily Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156858 [details] [review] Bug 15504: Remove update_lastseen method This patch removes update_lastseen which has been replaced by a call to track_login_daily Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156859 [details] [review] Bug 15504: Add unit test prove -v t/db_dependent/Auth.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156860 [details] [review] Bug 15504: (follow-up) Unit tests This patch removes, moves and updates the existing unit tests for track_login_daily and track_login to all sit under Koha::Patron and update_lastseen. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156861 [details] [review] Bug 15504: (follow-up) Migrate to one clear method We were using a series of similarly named methods spread in distinct places around the codebase. This combines the logic of C4::Auth::track_login_daily and Koha::Patron->track_login into a new Koha::Patron->update_lastseen method. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156862 [details] [review] Bug 15504: (follow-up) Use borrowernumber in Koha::Patrons->find
Created attachment 156863 [details] [review] Bug 15504: (follow-up) Add Triggers mock to unit tests
Created attachment 156864 [details] [review] Bug 15504: (follow-up) Tidy 'lastseen' section of unit test
Created attachment 156865 [details] [review] Bug 15504: (follow-up) Fix is_active test Bug 33245 introduced a new is_active patron method and the corresponding tests included calls to the method we depreacte in this patchset
Created attachment 156866 [details] [review] Bug 15504: (follow-up) Respect existing TrackLastPatronActivity This patch updates the database update to set 'check_out,connection,login' as the activity triggers should we find at upgrade time that TrackLastPatronActivity is set to true. This retains the existing functionality.
Created attachment 156867 [details] [review] Bug 15504: (follow-up) Rename cache key I agree, it's better to use borrowernumber for the cache key and I also take the oportunity to rename it to more clearly reflect the function (it's no longer 'login' only, it's now 'activity')
Created attachment 156868 [details] [review] Bug 15504: (QA follow-up) Terminology This just updates 'Login to OPAC' to 'Login'.. we should squash this
Created attachment 156869 [details] [review] Bug 15504: (follow-up) Update terminology on cleanborrowers This patch updates the terminology on the cleanborrowers form to reflect that 'activity' and not just 'connectivity' is tracked by TrackLastPatronActivity.
Created attachment 156870 [details] [review] Bug 15504: (follow-up) Clarify --last_seen option in delete_patrons This patch adds clarifications to the last_seen option in delete_patrons to clarify that TrackLastPatronActivity and TrackLastPatronActivityTriggers system preferences act as a couplet.
Created attachment 156875 [details] [review] Bug 15504: Remove PatronLastActivity preference This patch removes the older PatronLastActivity preference opting to replace it by the new PatronLastActivityTriggers preference. The feature can now be disabled by simply not selecting any triggers
I opted to just fold in the removal of TrackLastPatronActivity.. it can be moved to it's own bug if you prefer.. or squashed a bit.. I kept them as distict patches for now just to make it clear where I've responded to each of the feedback points.
Created attachment 156879 [details] [review] Bug 15504: Remove PatronLastActivity preference This patch removes the older PatronLastActivity preference opting to replace it by the new PatronLastActivityTriggers preference. The feature can now be disabled by simply not selecting any triggers
Resuming here now. Please dont touch the patches.
Applying: Bug 15504: (follow-up) Unit tests error: sha1 information is lacking or useless (t/db_dependent/Koha/Patron.t). error: could not build fake ancestor Patch failed at 0001 Bug 15504: (follow-up) Unit tests
I cant continue now. Please fix.
Created attachment 156898 [details] [review] Bug 15504: Add new syspref TrackLastPatronActivityTriggers This patch adds a new syspref TrackLastPatronActivityTriggers to determine which actions should update borrowers.lastseen Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156899 [details] [review] Bug 15504: Update track_login_daily to accept triggers This patch adds triggers to track_login_daily so that it only tracks activity when that trigger is active Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156900 [details] [review] Bug 15504: Add triggers to instances of track_login_daily This patch adds a trigger to every instance of track_login_daily Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156901 [details] [review] Bug 15504: Remove update_lastseen method This patch removes update_lastseen which has been replaced by a call to track_login_daily Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156902 [details] [review] Bug 15504: Add unit test prove -v t/db_dependent/Auth.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156903 [details] [review] Bug 15504: (follow-up) Unit tests This patch removes, moves and updates the existing unit tests for track_login_daily and track_login to all sit under Koha::Patron and update_lastseen. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156904 [details] [review] Bug 15504: (follow-up) Migrate to one clear method We were using a series of similarly named methods spread in distinct places around the codebase. This combines the logic of C4::Auth::track_login_daily and Koha::Patron->track_login into a new Koha::Patron->update_lastseen method. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156905 [details] [review] Bug 15504: (follow-up) Use borrowernumber in Koha::Patrons->find
Created attachment 156906 [details] [review] Bug 15504: (follow-up) Add Triggers mock to unit tests
Created attachment 156907 [details] [review] Bug 15504: (follow-up) Tidy 'lastseen' section of unit test
Created attachment 156908 [details] [review] Bug 15504: (follow-up) Fix is_active test Bug 33245 introduced a new is_active patron method and the corresponding tests included calls to the method we depreacte in this patchset
Created attachment 156909 [details] [review] Bug 15504: (follow-up) Respect existing TrackLastPatronActivity This patch updates the database update to set 'check_out,connection,login' as the activity triggers should we find at upgrade time that TrackLastPatronActivity is set to true. This retains the existing functionality.
Created attachment 156910 [details] [review] Bug 15504: (follow-up) Rename cache key I agree, it's better to use borrowernumber for the cache key and I also take the oportunity to rename it to more clearly reflect the function (it's no longer 'login' only, it's now 'activity')
Created attachment 156911 [details] [review] Bug 15504: (QA follow-up) Terminology This just updates 'Login to OPAC' to 'Login'.. we should squash this
Created attachment 156912 [details] [review] Bug 15504: (follow-up) Update terminology on cleanborrowers This patch updates the terminology on the cleanborrowers form to reflect that 'activity' and not just 'connectivity' is tracked by TrackLastPatronActivity.
Created attachment 156913 [details] [review] Bug 15504: (follow-up) Clarify --last_seen option in delete_patrons This patch adds clarifications to the last_seen option in delete_patrons to clarify that TrackLastPatronActivity and TrackLastPatronActivityTriggers system preferences act as a couplet.
Created attachment 156914 [details] [review] Bug 15504: Remove PatronLastActivity preference This patch removes the older PatronLastActivity preference opting to replace it by the new PatronLastActivityTriggers preference. The feature can now be disabled by simply not selecting any triggers
Coming back here. Just hang on a bit.
Created attachment 156915 [details] [review] Bug 15504: (QA follow-up) Fix dbrev Resolve: ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Unknown column 'check_out' in 'field list' at /usr/share/koha/C4/Installer.pm line 741 Test plan: Remove new pref from db. Run dbrev again. Check results. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
We also need 35041 to fix a current failure on Koha/Patron.t. Submitted now.
FAIL C4/Auth.pm FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 perl -MPod::Coverage=C4::Auth -e1 C4::Auth has a Pod::Coverage rating of 0.705882352941177 The following are uncovered: checkpw, checkpw_hash, checkpw_internal, getborrowernumber, safe_exit So no problems from this patch set. Ignoring this warn from qa tools here. We just removed a sub here.
Created attachment 156917 [details] [review] Bug 15504: Add new syspref TrackLastPatronActivityTriggers This patch adds a new syspref TrackLastPatronActivityTriggers to determine which actions should update borrowers.lastseen Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156918 [details] [review] Bug 15504: Update track_login_daily to accept triggers This patch adds triggers to track_login_daily so that it only tracks activity when that trigger is active Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156919 [details] [review] Bug 15504: Add triggers to instances of track_login_daily This patch adds a trigger to every instance of track_login_daily Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156920 [details] [review] Bug 15504: Remove update_lastseen method This patch removes update_lastseen which has been replaced by a call to track_login_daily Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156921 [details] [review] Bug 15504: Add unit test prove -v t/db_dependent/Auth.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156922 [details] [review] Bug 15504: (follow-up) Unit tests This patch removes, moves and updates the existing unit tests for track_login_daily and track_login to all sit under Koha::Patron and update_lastseen. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156923 [details] [review] Bug 15504: (follow-up) Migrate to one clear method We were using a series of similarly named methods spread in distinct places around the codebase. This combines the logic of C4::Auth::track_login_daily and Koha::Patron->track_login into a new Koha::Patron->update_lastseen method. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156924 [details] [review] Bug 15504: (follow-up) Use borrowernumber in Koha::Patrons->find Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156925 [details] [review] Bug 15504: (follow-up) Add Triggers mock to unit tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156926 [details] [review] Bug 15504: (follow-up) Tidy 'lastseen' section of unit test Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156927 [details] [review] Bug 15504: (follow-up) Fix is_active test Bug 33245 introduced a new is_active patron method and the corresponding tests included calls to the method we depreacte in this patchset Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156928 [details] [review] Bug 15504: (follow-up) Respect existing TrackLastPatronActivity This patch updates the database update to set 'check_out,connection,login' as the activity triggers should we find at upgrade time that TrackLastPatronActivity is set to true. This retains the existing functionality. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156929 [details] [review] Bug 15504: (follow-up) Rename cache key I agree, it's better to use borrowernumber for the cache key and I also take the oportunity to rename it to more clearly reflect the function (it's no longer 'login' only, it's now 'activity') Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156930 [details] [review] Bug 15504: (QA follow-up) Terminology This just updates 'Login to OPAC' to 'Login'.. we should squash this Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156931 [details] [review] Bug 15504: (follow-up) Update terminology on cleanborrowers This patch updates the terminology on the cleanborrowers form to reflect that 'activity' and not just 'connectivity' is tracked by TrackLastPatronActivity. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156932 [details] [review] Bug 15504: (follow-up) Clarify --last_seen option in delete_patrons This patch adds clarifications to the last_seen option in delete_patrons to clarify that TrackLastPatronActivity and TrackLastPatronActivityTriggers system preferences act as a couplet. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156933 [details] [review] Bug 15504: Remove PatronLastActivity preference This patch removes the older PatronLastActivity preference opting to replace it by the new PatronLastActivityTriggers preference. The feature can now be disabled by simply not selecting any triggers Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156934 [details] [review] Bug 15504: (QA follow-up) Fix dbrev Resolve: ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Unknown column 'check_out' in 'field list' at /usr/share/koha/C4/Installer.pm line 741 Test plan: Remove new pref from db. Run dbrev again. Check results. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156935 [details] [review] Bug 15504: (QA follow-up) Final changes [1] Comment in Patron->is_active sharpened. [2] Patron->update_lastseen return if $cached && $cached eq $now->ymd; Should be return $self if $cached && $cached eq $now->ymd; Consistent return value. [3] Typo if there are not TrackLastPatronActivityTriggers enabled [4] Typo TrackLastPatronActivityTriggers must be contain [5] Typo patron is does .. option => performs an action [6] Tidy on dbrev to fix The file is less tidy than before (bad/messy lines before: 0, now: 4) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Looks good to me now. Last commment. Just noticing here. Will open a new bug. t/db_dependent/Members.t $dbh->do(q|UPDATE borrowers SET lastseen=NULL|); Not from this patch set, but this is bad practice. We should change it (elsewhere).
Note for RM: There are follow-up bugs opened to extend the feature. But imo this can go in as-is. There is commitment to work further.
(In reply to Marcel de Rooy from comment #165) > Note for RM: There are follow-up bugs opened to extend the feature. But imo > this can go in as-is. There is commitment to work further. Thanks for this clarification.
Please rebase ASAP. Thanks!
Created attachment 157602 [details] [review] Bug 15504: Add new syspref TrackLastPatronActivityTriggers This patch adds a new syspref TrackLastPatronActivityTriggers to determine which actions should update borrowers.lastseen Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157603 [details] [review] Bug 15504: Update track_login_daily to accept triggers This patch adds triggers to track_login_daily so that it only tracks activity when that trigger is active Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157604 [details] [review] Bug 15504: Add triggers to instances of track_login_daily This patch adds a trigger to every instance of track_login_daily Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157605 [details] [review] Bug 15504: Remove update_lastseen method This patch removes update_lastseen which has been replaced by a call to track_login_daily Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157606 [details] [review] Bug 15504: Add unit test prove -v t/db_dependent/Auth.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157607 [details] [review] Bug 15504: (follow-up) Unit tests This patch removes, moves and updates the existing unit tests for track_login_daily and track_login to all sit under Koha::Patron and update_lastseen. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157608 [details] [review] Bug 15504: (follow-up) Migrate to one clear method We were using a series of similarly named methods spread in distinct places around the codebase. This combines the logic of C4::Auth::track_login_daily and Koha::Patron->track_login into a new Koha::Patron->update_lastseen method. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157609 [details] [review] Bug 15504: (follow-up) Use borrowernumber in Koha::Patrons->find Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157610 [details] [review] Bug 15504: (follow-up) Add Triggers mock to unit tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157611 [details] [review] Bug 15504: (follow-up) Tidy 'lastseen' section of unit test Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157612 [details] [review] Bug 15504: (follow-up) Fix is_active test Bug 33245 introduced a new is_active patron method and the corresponding tests included calls to the method we depreacte in this patchset Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157613 [details] [review] Bug 15504: (follow-up) Respect existing TrackLastPatronActivity This patch updates the database update to set 'check_out,connection,login' as the activity triggers should we find at upgrade time that TrackLastPatronActivity is set to true. This retains the existing functionality. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157614 [details] [review] Bug 15504: (follow-up) Rename cache key I agree, it's better to use borrowernumber for the cache key and I also take the oportunity to rename it to more clearly reflect the function (it's no longer 'login' only, it's now 'activity') Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157615 [details] [review] Bug 15504: (QA follow-up) Terminology This just updates 'Login to OPAC' to 'Login'.. we should squash this Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157616 [details] [review] Bug 15504: (follow-up) Update terminology on cleanborrowers This patch updates the terminology on the cleanborrowers form to reflect that 'activity' and not just 'connectivity' is tracked by TrackLastPatronActivity. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157617 [details] [review] Bug 15504: (follow-up) Clarify --last_seen option in delete_patrons This patch adds clarifications to the last_seen option in delete_patrons to clarify that TrackLastPatronActivity and TrackLastPatronActivityTriggers system preferences act as a couplet. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157618 [details] [review] Bug 15504: Remove PatronLastActivity preference This patch removes the older PatronLastActivity preference opting to replace it by the new PatronLastActivityTriggers preference. The feature can now be disabled by simply not selecting any triggers Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157619 [details] [review] Bug 15504: (QA follow-up) Fix dbrev Resolve: ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Unknown column 'check_out' in 'field list' at /usr/share/koha/C4/Installer.pm line 741 Test plan: Remove new pref from db. Run dbrev again. Check results. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157620 [details] [review] Bug 15504: (QA follow-up) Final changes [1] Comment in Patron->is_active sharpened. [2] Patron->update_lastseen return if $cached && $cached eq $now->ymd; Should be return $self if $cached && $cached eq $now->ymd; Consistent return value. [3] Typo if there are not TrackLastPatronActivityTriggers enabled [4] Typo TrackLastPatronActivityTriggers must be contain [5] Typo patron is does .. option => performs an action [6] Tidy on dbrev to fix The file is less tidy than before (bad/messy lines before: 0, now: 4) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
I did the rebase. But which status we should put for the ticket?
btw, that atomic update quoting is very inconsistent: my ($tracklastactivity) = $dbh->selectrow_array( q| SELECT value FROM systempreferences WHERE variable='TrackLastPatronActivity'; | ); vs $dbh->do( qq{INSERT IGNORE INTO systempreferences (variable,value,options,explanation,type) VALUES ('TrackLastPatronActivityTriggers',?,NULL,'If set, the field borrowers.lastseen will be updated every time a patron performs a selected action','multiple') }, undef, $triggers, ); vs $dbh->do( q{ DELETE FROM systempreferences WHERE variable='TrackLastPatronActivity' } ); why so?
Created attachment 157624 [details] [review] Bug 15504: (QA follow-up): more consistent quotes in atomicupdate
(In reply to Andrii Nugged from comment #187) > btw, that atomic update quoting is very inconsistent: It's just between 1st and 3rd case. The 2nd uses interpolation so needs the double-q IIUC Is there even a recommendation for a default choice for the others? The examples at https://wiki.koha-community.org/wiki/Database_updates#How_to_write_an_atomicupdate_file use q{ so let's use that. updatedatabase tested. ---- > I did the rebase. But which status we should put for the ticket? Thanks for the rebase :) It was passed QA before. So passed QA, unless the rebase showed that other changes could interfere with this and require retesting. **How was it?**
Pushed to master for 23.11. Nice work everyone, thanks!
Super-excited for this. Thanks to everyone for all their contributions & work to get this far!
(In reply to Tomás Cohen Arazi from comment #191) > Pushed to master for 23.11. Good! /me taking this to our preprod pilot, and soon to pre-release prod pilot. So if something pop-ups, we'll feeback quickly :P \o]
Enhancement not pushed to 23.05.x
I've updated the release notes, as this how the enhancement worked was changed between sign off and being pushed. Feel free to correct any mistakes I've made or make it better!
I renamed the activities in the release notes to match that used on bug 35221, and I also added the bug number for the activities added by other bugs (holds and article requests).
See 35231. OPAC is blocked when logging out. Did we miss an Auth.pm test here? :)