Bug 33997 - Borrowers.updated_on should not be updated by track_login
Summary: Borrowers.updated_on should not be updated by track_login
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Matt Blenkinsop
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-06-13 13:34 UTC by Matt Blenkinsop
Modified: 2024-04-15 17:15 UTC (History)
10 users (show)

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


Attachments
Bug 33997: Stop track_login from updating borrowers.updated_on (1.06 KB, patch)
2023-06-13 13:43 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 33997: Stop track_login from updating borrowers.updated_on (1.04 KB, patch)
2023-06-13 19:16 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 33997: (QA follow-up) Add unit test (1.63 KB, patch)
2023-06-14 15:16 UTC, Matt Blenkinsop
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Blenkinsop 2023-06-13 13:34:33 UTC
Currently borrowers.updated_on is updated by track_login even though a login is not an update to the patron record. This update needs to be blocked so that the updated_on field accurately reflects updates to the patron. Logins are supposed to be tracked by borrowers.lastseen which track_login already does.
Comment 1 Matt Blenkinsop 2023-06-13 13:43:36 UTC
Created attachment 152294 [details] [review]
Bug 33997: Stop track_login from updating borrowers.updated_on

This patch blocks the update to borrowers.updated_on when track_login is called

Test plan:
1) Enable syspref 'TrackLastPatronActivity'
2) Log into the OPAC
3) Check the database for the patron/account used and observe that the timestamps for updated_on and lastseen match
4) reset_all
5) Apply patch
6) Repeat steps one to three, the updated_on timestamp should not have changed
Comment 2 Lucas Gass 2023-06-13 19:16:13 UTC
Created attachment 152321 [details] [review]
Bug 33997: Stop track_login from updating borrowers.updated_on

This patch blocks the update to borrowers.updated_on when track_login is called

Test plan:
1) Enable syspref 'TrackLastPatronActivity'
2) Log into the OPAC
3) Check the database for the patron/account used and observe that the timestamps for updated_on and lastseen match
4) reset_all
5) Apply patch
6) Repeat steps one to three, the updated_on timestamp should not have changed
Comment 3 Jonathan Druart 2023-06-14 14:48:39 UTC
Please provide a test.
Comment 4 Matt Blenkinsop 2023-06-14 15:16:06 UTC
Created attachment 152342 [details] [review]
Bug 33997: (QA follow-up) Add unit test

This patch adds a unit test

prove -v t/db_dependent/Members.t
Comment 5 Marcel de Rooy 2023-06-23 07:52:17 UTC
(In reply to Matt Blenkinsop from comment #0)
> Currently borrowers.updated_on is updated by track_login even though a login
> is not an update to the patron record. This update needs to be blocked so
> that the updated_on field accurately reflects updates to the patron. Logins
> are supposed to be tracked by borrowers.lastseen which track_login already
> does.

I am not so sure if this statement is true.
A timestamp should reflect a change on the record. And a change to lastseen is also a change of the patron record.
Comment 6 Martin Renvoize 2023-06-27 09:05:45 UTC
Should this be yet another system preference then Marcel?  We felt this change, along with bug 15504, significantly clarified how updated_on and lastseen complimented each other and worked.  I'd like some librarian opinions on this.
Comment 7 Marcel de Rooy 2023-06-27 11:13:18 UTC
(In reply to Martin Renvoize from comment #6)
> Should this be yet another system preference then Marcel?  We felt this
> change, along with bug 15504, significantly clarified how updated_on and
> lastseen complimented each other and worked.  I'd like some librarian
> opinions on this.

YASP: Yet another sys pref :)
Not necessarily. Could we think of fields in the borrowers table that are not really part of your patron record definition like lastseen and move them to a separate table perhaps ?
Comment 8 Katrin Fischer 2023-06-30 18:54:46 UTC
I have to admit I had the same feeling as Marcel when I saw this bug. Moving to a separate table seems a bit of overkill. 

What is the use case that triggered this patch?
Comment 9 Matt Blenkinsop 2023-07-03 08:55:53 UTC
We had a customer who were trying to run a report to remove patrons who had been inactive for more than 5 years. They were trying to run this based on borrowers.updated_on rather than borrowers.lastseen which seems more practical for this scenario.

Essentially my assumption was that updated_on was for updates to patron record details and lastseen was more for activity tracking so updated_on being updated by a login didn't seem to fit with that logic. I'm not clued up on the history behind these two fields though so that assumption is open for correction.
Comment 10 Martin Renvoize 2023-08-23 07:53:38 UTC
We have more and more customers using the REST API's and the 'updated_on' field is consistently presumed to actually signify a change to the patrons record... filtering upon it and finding you're results include lots and lots of records where no change has really taken place is both a performance issue and a confusion for end users of the API's.
Comment 11 Martin Renvoize 2023-08-23 07:57:09 UTC
I'd also argue that perhaps login_attempts shouldn't update the updated_on field either... but that's for another day.
Comment 12 Marcel de Rooy 2023-09-01 08:31:38 UTC
I still think that if we need a specific timestamp for selected changes, than we should add a new field for that.
And leave updated_on to what it is or even should be ?
Comment 13 Marcel de Rooy 2023-09-01 08:31:47 UTC
Mailing QA team
Comment 14 Nick Clemens (kidclamp) 2023-09-01 11:13:16 UTC
(In reply to Marcel de Rooy from comment #12)
> I still think that if we need a specific timestamp for selected changes,
> than we should add a new field for that.
> And leave updated_on to what it is or even should be ?

(In reply to Martin Renvoize from comment #10)
> We have more and more customers using the REST API's and the 'updated_on'
> field is consistently presumed to actually signify a change to the patrons
> record... filtering upon it and finding you're results include lots and lots
> of records where no change has really taken place is both a performance
> issue and a confusion for end users of the API's.

I started out agreeing with Martin, but now agree with Marcel.

We should maybe have an 'edited_on' field for "changes" to the record

If 'updated_on' is used for syncing to outside systems, and those systems use lastseen then they will want to update those patrons - e.g. an outside email service for contacting only recently active patrons, or one that does stats for a library on 'active' patrons - we wouldn't capture patrons active only on overdrive or other services if we don't see a patron as having been 'updated'
Comment 15 Martin Renvoize 2023-09-06 09:21:36 UTC
We're removing the dependency on this from bug 15504 as this is holding everything up and that's an important bug for us.

I don't think it's a great idea having two synonym fields, edited_on and updated_on, and personally I think 'updated_on' really does suggest it's actual record updates and not a timestamp for any and all, unrelated, changes.

I'd rather see fields split out of borrowers and properly database normalised if we're going down this path.