Bug 3820

Summary: More detailed patron record changes log
Product: Koha Reporter: Nicole C. Engard <nengard>
Component: System AdministrationAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: enhancement    
Priority: P5 - low CC: barton, baselibrary.consortium, dcook, emma.perks, fiona.borthwick, josef.moravec, katrin.fischer, kyle, martin.renvoize, nick, pe.havel
Version: Main   
Hardware: PC   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=3470
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=3469
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This enhancement will add a log of the specific fields that were changed when modifying/editing a patron, including the before and after values for each updated field.
Version(s) released in:
Bug Depends on:    
Bug Blocks: 36159, 24329    
Attachments: members data in log viewer
data on mod log on patron record
Bug 3820: Add changes to MEMBERS MODIFY logaction
Bug 3820: Add changes to MEMBERS MODIFY logaction
Bug 3820: Add changes to MEMBERS MODIFY logaction
Bug 3820: (follow-up) Clean up warnings
Bug 3820: Add changes to MEMBERS MODIFY logaction
Bug 3820: (follow-up) Clean up warnings
Bug 3820: Add changes to MEMBERS MODIFY logaction
Bug 3820: (follow-up) Clean up warnings
Bug 3820: Add changes to MEMBERS MODIFY logaction
Bug 3820: (follow-up) Clean up warnings
Bug 3820: DB Update to make old cardnumber logs match the new format
Bug 3820: Update tests to reflect the change of behaviour
Bug 3820: Add changes to MEMBERS MODIFY logaction
Bug 3820: (follow-up) Clean up warnings
Bug 3820: DB Update to make old cardnumber logs match the new format
Bug 3820: Update tests to reflect the change of behaviour
Bug 3820: (QA follow-up) Change REGEXP_REPLACE to REPLACE for wider compatibility
Bug 3820: Add changes to MEMBERS MODIFY logaction
Bug 3820: (follow-up) Clean up warnings
Bug 3820: (follow-up) Update cardnumber logs match the new format
Bug 3820: (follow-up) Update tests to reflect the change of behaviour
Bug 3820: (QA follow-up) Change REGEXP_REPLACE to REPLACE for wider compatibility
Bug 3820: (follow-up) Address lastseen log spamming
Bug 3820: (follow-up) Address lastseen log spamming
Bug 3820: (follow-up) Address lastseen log spamming

Description Chris Cormack 2010-05-21 01:15:23 UTC


---- Reported by nengard@gmail.com 2009-11-30 15:46:42 ----

Created an attachment
members data in log viewer

When you view the Members data in the log viewer you see a series of question marks in the UPDATE statement.  All other logs work well, it's just the members one that has issues.  The Log Viewer should show what changes were made to the patron record.



---- Additional Comments From nengard@gmail.com 2009-11-30 15:47:04 ----

Created an attachment
data on mod log on patron record





--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:15 UTC  ---

This bug was previously known as _bug_ 3820 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=3820
Imported an attachment (id=1584)
Imported an attachment (id=1585)

Actual time not defined. Setting to 0.0
The original submitter of attachment 1584 [details] is unknown.
   Reassigning to the person who moved it here: chris@bigballofwax.co.nz.
The original submitter of attachment 1585 [details] is unknown.
   Reassigning to the person who moved it here: chris@bigballofwax.co.nz.

Comment 1 Paul Poulain 2011-08-02 14:21:58 UTC
The display is now :

UPDATE (executed w/ arg: 1)
Comment 2 Katrin Fischer 2011-08-02 14:36:36 UTC
I am not sure I understand what 'UPDATE (executed w/ arg: 1)' means.
Ideally the log would tell which fields were updated and the old and new values. Or at least the new values, so you coudl check previous log entries for the original values.
Comment 3 Paul Poulain 2011-08-02 15:52:49 UTC
frankly katrin, I've hesitated to say this display was not that much useless. The log let you know who did something and when, but don't say what.
That's another issue I think. Reopen the ticket. Nicole your opinion ?
Comment 4 Katrin Fischer 2011-08-02 16:52:22 UTC
I tend to reopen.

Thinking a bit more about it: I am not sure saving patron information in logs was avoided for privacy reasons?

You could always turn off the borrowers log if you don't want this information to be saved. Or we could make the borrowers log system preference have different settings for the kind of information that should be saved. No information, information about time and staff updating and a complete log telling you the changes. 

Other logs like systempreferences show changes in detail.
Comment 5 Nicole C. Engard 2011-08-02 23:46:02 UTC
I'm with Katrin on this one - many libraries want a record of who changed what on a patron's record - and isn't that the point of a log?

Nicole
Comment 6 Paul Poulain 2011-08-03 07:50:58 UTC
Well, the cataloguing log shows exactly what the biblio was before it has been modified. I agree the MembersLog should be the same.

Who changed that behaviour ?

git blame C4/Members.pm, sub ModMember :
ae48ebbc (Chris Nighswonger              2010-11-12 09:34:52 -0500  784)         logaction("MEMBERS", "MODIFY", $data{'borrowernumber'}, "UPDATE (executed w/ ar

git log C4/Members.pm shows that it was only a reindenting, so no blame to chris_n,

digging earlier, I see from chris_n too, patch 35df86da596cdcd5e6d9fb0bc7c14f303a4a7e57, 2008-03-18, :
-    &logaction(C4::Context->userenv->{'number'},"MEMBERS","MODIFY",$data{'borrowernumber'},"") 
+    &logaction(C4::Context->userenv->{'number'},"MEMBERS","MODIFY",$data{'borrowernumber'},"$query (executed w/ arg: $data{'borrowernumber'})") 

seems there was no info at all before this patch.

I'm a little bit confused. Nicole, any idea ?
Comment 7 Paul Poulain 2016-06-04 08:19:40 UTC
issue still here in 16.05, updating assignee
Comment 8 Katrin Fischer 2016-06-19 21:52:00 UTC
*** Bug 8578 has been marked as a duplicate of this bug. ***
Comment 9 Katrin Fischer 2016-06-19 21:55:41 UTC
Still valid, see recent discussion on the mailinglist:
https://lists.katipo.co.nz/pipermail/koha/2016-June/045634.html
Comment 10 Fiona Borthwick 2018-11-02 14:51:33 UTC
There is still interest from our customer base on this one.
Comment 11 Martin Renvoize 2019-02-14 10:49:36 UTC Comment hidden (obsolete)
Comment 12 Martin Renvoize 2019-02-14 10:58:23 UTC Comment hidden (obsolete)
Comment 13 Martin Renvoize 2019-02-14 11:11:34 UTC
Changed fields will be listed in canonical order now.

Any opinions on 'before/after' vs 'from/to'.. if we switch to 'from/to' we get them in that order for free with the canonicalisation switch.. with 'before/after' we will get 'after' coming before 'before' in the output wherever the raw json is shown as obviously 'a' comes before 'b' in the alphabet ;)
Comment 14 Martin Renvoize 2019-02-14 12:35:01 UTC Comment hidden (obsolete)
Comment 15 Martin Renvoize 2019-02-14 12:35:05 UTC Comment hidden (obsolete)
Comment 16 Katrin Fischer 2019-02-14 14:13:52 UTC
Not against QA'ing this one, but someone might want to beat me to it.
Comment 17 Joy Nelson 2019-02-14 14:44:43 UTC
*** Bug 15296 has been marked as a duplicate of this bug. ***
Comment 18 Owen Leonard 2019-02-15 13:11:04 UTC
Created attachment 85161 [details] [review]
Bug 3820: Add changes to MEMBERS MODIFY logaction

This patch attemps to add a more refined and detailed info structure
detailing what fields were changed and what they were changed from and
to.

Test Plan: Enable BorrowersLog in the system preferences and undertake a
series of borrower detail changes.  Go to view the actionlogs and note
that MEMBER MODIFY logs now detail what fields you have changed using
the following form:

  { FIELD1: { before: BEFORE_VALUE, after: AFTER_VALUE } }

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 19 Owen Leonard 2019-02-15 13:11:08 UTC
Created attachment 85162 [details] [review]
Bug 3820: (follow-up) Clean up warnings

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 20 Charles Farmer 2019-02-15 21:03:35 UTC
Created attachment 85196 [details] [review]
Bug 3820: Add changes to MEMBERS MODIFY logaction

This patch attemps to add a more refined and detailed info structure
detailing what fields were changed and what they were changed from and
to.

Test Plan: Enable BorrowersLog in the system preferences and undertake a
series of borrower detail changes.  Go to view the actionlogs and note
that MEMBER MODIFY logs now detail what fields you have changed using
the following form:

  { FIELD1: { before: BEFORE_VALUE, after: AFTER_VALUE } }

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
Comment 21 Charles Farmer 2019-02-15 21:03:40 UTC
Created attachment 85197 [details] [review]
Bug 3820: (follow-up) Clean up warnings

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
Comment 22 Josef Moravec 2019-02-18 10:17:39 UTC
This is great addition and I like it, but as it changes the behaviour, the test should be adjusted too:

t/db_dependent/Koha/Patrons.t .. 29/33 
    #   Failed test 'Got correct new cardnumber'
    #   at t/db_dependent/Koha/Patrons.t line 1414.
    #          got: undef
    #     expected: 'TESTCARDNUMBER'

    #   Failed test 'Got correct old cardnumber'
    #   at t/db_dependent/Koha/Patrons.t line 1415.
    #          got: undef
    #     expected: 'jY44Hy2e7OojC2Zx7vMo43gzub'

    #   Failed test 'With BorrowerLogs, Change in cardnumber should be logged, as well as general alert of patron mod.'
    #   at t/db_dependent/Koha/Patrons.t line 1416.
    #          got: '1'
    #     expected: '2'
    # Looks like you failed 3 tests of 3.

#   Failed test 'Log cardnumber change'
#   at t/db_dependent/Koha/Patrons.t line 1417.
Comment 23 Martin Renvoize 2019-02-18 13:36:28 UTC
Created attachment 85243 [details] [review]
Bug 3820: Add changes to MEMBERS MODIFY logaction

This patch attemps to add a more refined and detailed info structure
detailing what fields were changed and what they were changed from and
to.

Test Plan: Enable BorrowersLog in the system preferences and undertake a
series of borrower detail changes.  Go to view the actionlogs and note
that MEMBER MODIFY logs now detail what fields you have changed using
the following form:

  { FIELD1: { before: BEFORE_VALUE, after: AFTER_VALUE } }

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
Comment 24 Martin Renvoize 2019-02-18 13:36:32 UTC
Created attachment 85244 [details] [review]
Bug 3820: (follow-up) Clean up warnings

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
Comment 25 Martin Renvoize 2019-02-18 13:36:36 UTC
Created attachment 85245 [details] [review]
Bug 3820: DB Update to make old cardnumber logs match the new format
Comment 26 Martin Renvoize 2019-02-18 13:36:40 UTC
Created attachment 85246 [details] [review]
Bug 3820: Update tests to reflect the change of behaviour
Comment 27 Martin Renvoize 2019-02-18 13:38:39 UTC
Back to signed off now I've added the required update to the tests (and also added a db update to make old cardnumber logs match the new format.. although we can't retrospectively populate the logs for other actions the update should make writing future reports backwards compatible without adding effort.

Let me know if you'd like any further additions to the tests or anything.
Comment 28 Josef Moravec 2019-02-18 19:20:10 UTC
Created attachment 85256 [details] [review]
Bug 3820: Add changes to MEMBERS MODIFY logaction

This patch attemps to add a more refined and detailed info structure
detailing what fields were changed and what they were changed from and
to.

Test Plan: Enable BorrowersLog in the system preferences and undertake a
series of borrower detail changes.  Go to view the actionlogs and note
that MEMBER MODIFY logs now detail what fields you have changed using
the following form:

  { FIELD1: { before: BEFORE_VALUE, after: AFTER_VALUE } }

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 29 Josef Moravec 2019-02-18 19:20:15 UTC
Created attachment 85257 [details] [review]
Bug 3820: (follow-up) Clean up warnings

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 30 Josef Moravec 2019-02-18 19:20:20 UTC
Created attachment 85258 [details] [review]
Bug 3820: DB Update to make old cardnumber logs match the new format

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 31 Josef Moravec 2019-02-18 19:20:25 UTC
Created attachment 85259 [details] [review]
Bug 3820: Update tests to reflect the change of behaviour

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 32 Josef Moravec 2019-02-18 19:20:31 UTC
Created attachment 85260 [details] [review]
Bug 3820: (QA follow-up) Change REGEXP_REPLACE to REPLACE for wider compatibility

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 33 Nick Clemens 2019-02-19 11:51:49 UTC
Just a question, if TrackLastPatornActivity is enabled then we record a modification on each update of the lastseen field.

Is this desired behavior? Other fields like login_attempts do seem to be avoided
Comment 34 Martin Renvoize 2019-02-19 12:40:31 UTC
Created attachment 85269 [details] [review]
Bug 3820: Add changes to MEMBERS MODIFY logaction

This patch attemps to add a more refined and detailed info structure
detailing what fields were changed and what they were changed from and
to.

Test Plan: Enable BorrowersLog in the system preferences and undertake a
series of borrower detail changes.  Go to view the actionlogs and note
that MEMBER MODIFY logs now detail what fields you have changed using
the following form:

  { FIELD1: { before: BEFORE_VALUE, after: AFTER_VALUE } }

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 35 Martin Renvoize 2019-02-19 12:40:35 UTC
Created attachment 85270 [details] [review]
Bug 3820: (follow-up) Clean up warnings

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 36 Martin Renvoize 2019-02-19 12:40:39 UTC
Created attachment 85271 [details] [review]
Bug 3820: (follow-up) Update cardnumber logs match the new format

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 37 Martin Renvoize 2019-02-19 12:40:43 UTC
Created attachment 85272 [details] [review]
Bug 3820: (follow-up) Update tests to reflect the change of behaviour

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 38 Martin Renvoize 2019-02-19 12:40:48 UTC
Created attachment 85273 [details] [review]
Bug 3820: (QA follow-up) Change REGEXP_REPLACE to REPLACE for wider compatibility

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 39 Martin Renvoize 2019-02-19 12:40:52 UTC
Created attachment 85274 [details] [review]
Bug 3820: (follow-up) Address lastseen log spamming

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 40 Martin Renvoize 2019-02-19 12:44:54 UTC
Good catch Nick, I've added a followup to exclude the lastseen field from logging as it would inevitably spam the logs pretty badly.

Note: We'll get double log lines for patron renewals too.. but this isn't a change of behaviour.. the first log line will be the now more details MODIFY, followed immediately by a 'RENEW' action log.. these both got logged before just there was less detail in the MODIFY than there is now.
Comment 41 Martin Renvoize 2019-02-19 12:47:32 UTC
Created attachment 85276 [details] [review]
Bug 3820: (follow-up) Address lastseen log spamming

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 42 Martin Renvoize 2019-02-19 13:14:02 UTC
Created attachment 85277 [details] [review]
Bug 3820: (follow-up) Address lastseen log spamming

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 43 Nick Clemens 2019-02-19 16:37:26 UTC
Awesome work all!

Pushed to master for 19.05
Comment 44 Martin Renvoize 2019-02-26 09:11:42 UTC
Changing to 'enhancement' as it's such a longstanding bug that many people may be relying on the existing format for reports and I don't want to break said reports in a stable version.
Comment 45 Nick Clemens 2020-01-02 12:38:48 UTC
Just a note on this one: the DB rev for cardnumber changes has a side effect of updating the timestamp in the logs - historical changes are going to be lost by this upgrade as they will now all appear to have been done at upgrade time. We will still have the historical cardnumber, I am not sure if this is worth filing as a bug