Bug 3820 - More detailed patron record changes log
Summary: More detailed patron record changes log
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: master
Hardware: PC All
: P5 - low enhancement with 2 votes (vote)
Target Milestone: ---
Assignee: Martin Renvoize
QA Contact: Josef Moravec
URL:
Whiteboard:
Keywords:
: 8578 15296 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-11-30 03:46 UTC by Nicole C. Engard
Modified: 2019-05-30 12:21 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
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:


Attachments
members data in log viewer (226.77 KB, image/png)
2009-11-30 15:46 UTC, Chris Cormack
Details
data on mod log on patron record (150.37 KB, image/png)
2009-11-30 15:47 UTC, Chris Cormack
Details
Bug 3820: Add changes to MEMBERS MODIFY logaction (2.95 KB, patch)
2019-02-14 10:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 3820: Add changes to MEMBERS MODIFY logaction (2.97 KB, patch)
2019-02-14 10:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 3820: Add changes to MEMBERS MODIFY logaction (2.97 KB, patch)
2019-02-14 12:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 3820: (follow-up) Clean up warnings (2.90 KB, patch)
2019-02-14 12:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 3820: Add changes to MEMBERS MODIFY logaction (3.01 KB, patch)
2019-02-15 13:11 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 3820: (follow-up) Clean up warnings (2.94 KB, patch)
2019-02-15 13:11 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 3820: Add changes to MEMBERS MODIFY logaction (3.07 KB, patch)
2019-02-15 21:03 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 3820: (follow-up) Clean up warnings (3.00 KB, patch)
2019-02-15 21:03 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 3820: Add changes to MEMBERS MODIFY logaction (3.07 KB, patch)
2019-02-18 13:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 3820: (follow-up) Clean up warnings (3.01 KB, patch)
2019-02-18 13:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 3820: DB Update to make old cardnumber logs match the new format (1.29 KB, patch)
2019-02-18 13:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 3820: Update tests to reflect the change of behaviour (1.44 KB, patch)
2019-02-18 13:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 3820: Add changes to MEMBERS MODIFY logaction (3.12 KB, patch)
2019-02-18 19:20 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 3820: (follow-up) Clean up warnings (3.06 KB, patch)
2019-02-18 19:20 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 3820: DB Update to make old cardnumber logs match the new format (1.34 KB, patch)
2019-02-18 19:20 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 3820: Update tests to reflect the change of behaviour (1.48 KB, patch)
2019-02-18 19:20 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 3820: (QA follow-up) Change REGEXP_REPLACE to REPLACE for wider compatibility (1.71 KB, patch)
2019-02-18 19:20 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 3820: Add changes to MEMBERS MODIFY logaction (3.13 KB, patch)
2019-02-19 12:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 3820: (follow-up) Clean up warnings (3.06 KB, patch)
2019-02-19 12:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 3820: (follow-up) Update cardnumber logs match the new format (1.40 KB, patch)
2019-02-19 12:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 3820: (follow-up) Update tests to reflect the change of behaviour (1.57 KB, patch)
2019-02-19 12:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 3820: (QA follow-up) Change REGEXP_REPLACE to REPLACE for wider compatibility (1.78 KB, patch)
2019-02-19 12:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 3820: (follow-up) Address lastseen log spamming (1.34 KB, patch)
2019-02-19 12:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 3820: (follow-up) Address lastseen log spamming (1.55 KB, patch)
2019-02-19 12:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 3820: (follow-up) Address lastseen log spamming (2.96 KB, patch)
2019-02-19 13:14 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.