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.
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
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
Please provide a test.
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
(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.
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.
(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 ?
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?
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.
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'd also argue that perhaps login_attempts shouldn't update the updated_on field either... but that's for another day.
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 ?
Mailing QA team
(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'
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.