Bug 26744 - Log changes to extended patron attributes
Summary: Log changes to extended patron attributes
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Gustafsson
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 22794
  Show dependency treegraph
 
Reported: 2020-10-20 08:24 UTC by Magnus Enger
Modified: 2024-04-19 15:19 UTC (History)
11 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 26744: Log changes to extended patron attributes (6.28 KB, patch)
2024-03-26 19:57 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 26744: Log changes to extended patron attributes (6.32 KB, patch)
2024-03-26 20:04 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 26744: Only log if BorrowersLog syspref is enabled (2.80 KB, patch)
2024-03-26 20:19 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 26744: Log changes to extended patron attributes (6.36 KB, patch)
2024-03-27 20:17 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 26744: Only log if BorrowersLog syspref is enabled (2.85 KB, patch)
2024-03-27 20:17 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 26744: Add support for repeatable and deleted attributes (12.53 KB, patch)
2024-03-28 18:20 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 26744: Don't delete extended attributes outside of method (2.31 KB, patch)
2024-04-19 15:14 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 26744: Add test for multiple values of repeatable fields (3.14 KB, patch)
2024-04-19 15:14 UTC, David Gustafsson
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2020-10-20 08:24:24 UTC
It looks like changes to a patron's "extended patron attributes" are not logged, when BorrowersLog is on. They should probably be.
Comment 1 Magnus Enger 2020-10-20 08:25:00 UTC
If we want to be really flexible, we could make logging a per-attribute setting.
Comment 2 Anneli Österman 2020-10-23 11:36:56 UTC
Logging would be a good thing.
Comment 3 George Williams (NEKLS) 2021-01-06 17:54:14 UTC
+1
Comment 4 Sara Brown 2022-05-26 15:52:36 UTC
Still relevant in 21.11 and still would be useful!
Comment 5 Andrew Fuerste-Henry 2024-03-21 17:28:30 UTC
+1, this would be helpful
Comment 6 David Gustafsson 2024-03-26 19:57:08 UTC
Created attachment 163957 [details] [review]
Bug 26744: Log changes to extended patron attributes

Log non-repeatable attributes and only log changes and creations, not deletions.
When changed, attributes are first all deleted, then the relevant ones are recreated.
This patch catches all deletions, and checks against them in the creation phase, logging
only the attributes that have changed (with before/after content). This makes it impossible
to log actual deletions.

To test:

1) Ensure tests in  t/db_dependent/Koha/Patron/Attribute.t passes

Sponsored-by: Gothenburg University Library
Comment 7 David Gustafsson 2024-03-26 20:04:16 UTC
Created attachment 163958 [details] [review]
Bug 26744: Log changes to extended patron attributes

Log non-repeatable attributes and only log changes and creations, not deletions.
When changed, attributes are first all deleted, then the relevant ones are recreated.
This patch catches all deletions, and checks against them in the creation phase, logging
only the attributes that have changed (with before/after content). This makes it impossible
to log actual deletions.

To test:

1) Ensure tests in  t/db_dependent/Koha/Patron/Attribute.t passes

Sponsored-by: Gothenburg University Library
Comment 8 David Gustafsson 2024-03-26 20:19:14 UTC
Created attachment 163959 [details] [review]
Bug 26744: Only log if BorrowersLog syspref is enabled
Comment 9 Lucas Gass 2024-03-27 20:16:48 UTC
This works as described so I will sign-off. Why not log repeatable attributes as well?
Comment 10 Lucas Gass 2024-03-27 20:17:28 UTC
Created attachment 164034 [details] [review]
Bug 26744: Log changes to extended patron attributes

Log non-repeatable attributes and only log changes and creations, not deletions.
When changed, attributes are first all deleted, then the relevant ones are recreated.
This patch catches all deletions, and checks against them in the creation phase, logging
only the attributes that have changed (with before/after content). This makes it impossible
to log actual deletions.

To test:

1) Ensure tests in  t/db_dependent/Koha/Patron/Attribute.t passes

Sponsored-by: Gothenburg University Library
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 11 Lucas Gass 2024-03-27 20:17:30 UTC
Created attachment 164035 [details] [review]
Bug 26744: Only log if BorrowersLog syspref is enabled

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 12 David Gustafsson 2024-03-27 20:36:44 UTC
I actually don't know since I'm not the original author of this patch, but I guess there is sole technical reasons. My guess would be it's harder to figure out a hood way of representing the change, and especially tricky if you just want one log message when adding multiple instances of a repeatable field since the stores method would be called repeatedly for each existing and added change. If the logging was performed in the extended_attributes method it could be doable, but that might not catch all modifications and think there might be more patrons performing modification that would have to be handled as well. Can ask my collegue tomorrow about this.
Comment 13 Stefan Berndtsson 2024-03-28 08:43:53 UTC
(In reply to Lucas Gass from comment #9)
> This works as described so I will sign-off. Why not log repeatable
> attributes as well?

I'm the one who wrote the original patch here. The way editing of extended attributes work is that they're all first deleted, then they're recreated from the form. 

Since all of the previous attributes are just deleted, I have no way that I can see to know which of the repeatable attributes that was changed into what.

It there had been a more conventional way to edit the existing attributes this would've been easy.
Comment 14 David Gustafsson 2024-03-28 18:20:44 UTC
Created attachment 164098 [details] [review]
Bug 26744: Add support for repeatable and deleted attributes
Comment 15 David Gustafsson 2024-03-28 18:25:24 UTC
I realised it actually was not too much trouble also logging changes to repeatable attributes (and deletions) if instead performing the logging in Koha::Patron->extended_attributes, so I revised the patch to do that instead.
Comment 16 Anneli Österman 2024-04-19 07:15:33 UTC
I tested this in sandbox with three patron attributes: SSN (not repetable), HOLDID (not repetable) and AUTOTYPE (repetable).

When I made a change to attribute the after value is recorded to the action logs but not the before value. The repetable AUTOTYPE attribute recorded changes only to one of the values. Deletion of SSN-value was not recorderd.

It seems to me that this does not work as intended.

Here are explained the actions that I took and how they showed in the action logs.


Changed AUTOTYPE values from Palautusautomaatti -> Palautusautomaatti 2 AND Lainausautomaatti -> Lainausautomaatti2. The latter was not recorded to the action log.
04/19/2024 06:50 	koha (51) 	Patrons 	Modify 	Joyce Gaines (22) 	
Patron attribute AUTOTYPE: { "after" : [ "Palautusautomaatti2" ], "before" : [] }
	Intranet

Changed HOLDID value from 12345678 to 123456789
04/19/2024 06:50 	koha (51) 	Patrons 	Modify 	Joyce Gaines (22) 	
Patron attribute HOLDID: { "after" : "123456789", "before" : "" }
	Intranet

Changed SSN value from  sotu123456 to sotu1234567
04/19/2024 06:50 	koha (51) 	Patrons 	Modify 	Joyce Gaines (22) 	
Patron attribute SSN: { "after" : "sotu1234567", "before" : "" }
	Intranet


Changed HOLDID value from 123456789 to 1234567890
04/19/2024 06:51 	koha (51) 	Patrons 	Modify 	Joyce Gaines (22) 	
Patron attribute HOLDID: { "after" : "1234567890", "before" : "" }
	Intranet

Changed SSN value from sotu1234567 to sotu123456
04/19/2024 06:51 	koha (51) 	Patrons 	Modify 	Joyce Gaines (22) 	
Patron attribute SSN: { "after" : "sotu123456", "before" : "" }
	Intranet

Changed AUTOTYPE values from Palautusautomaatti2 -> Palautusautomaatti AND Lainausautomaatti2 -> Lainausautomaatti. The latter was not recorded to the action log.
04/19/2024 06:51 	koha (51) 	Patrons 	Modify 	Joyce Gaines (22) 	
Patron attribute AUTOTYPE: { "after" : [ "Palautusautomaatti" ], "before" : [] }
	Intranet


Deleted value from SSN and saved. The deletion was not recorded but there were actions logged for AUTOTYPE and HOLDID which were not changed any way before saving the patron.
04/19/2024 06:52 	koha (51) 	Patrons 	Modify 	Joyce Gaines (22) 	
Patron attribute AUTOTYPE: { "after" : [ "Palautusautomaatti" ], "before" : [] }
	Intranet
04/19/2024 06:52 	koha (51) 	Patrons 	Modify 	Joyce Gaines (22) 	
Patron attribute HOLDID: { "after" : "1234567890", "before" : "" }
	Intranet


Added value for SSN, did not change other attributes. Still there we lines for them.
04/19/2024 07:04 	koha (51) 	Patrons 	Modify 	Joyce Gaines (22) 	
Patron attribute AUTOTYPE: { "after" : [ "Palautusautomaatti" ], "before" : [] }
	Intranet
04/19/2024 07:04 	koha (51) 	Patrons 	Modify 	Joyce Gaines (22) 	
Patron attribute SSN: { "after" : "sotu234567", "before" : "" }
	Intranet
04/19/2024 07:04 	koha (51) 	Patrons 	Modify 	Joyce Gaines (22) 	
Patron attribute HOLDID: { "after" : "1234567890", "before" : "" }
	Intranet


Cleared repeting AUTOTYPE Lainausautomaatti. No record of it but there were record of others which were not changed.
04/19/2024 07:04 	koha (51) 	Patrons 	Modify 	Joyce Gaines (22) 	
Patron attribute AUTOTYPE: { "after" : [ "Palautusautomaatti" ], "before" : [] }
	Intranet
04/19/2024 07:04 	koha (51) 	Patrons 	Modify 	Joyce Gaines (22) 	
Patron attribute SSN: { "after" : "sotu234567", "before" : "" }
	Intranet
04/19/2024 07:04 	koha (51) 	Patrons 	Modify 	Joyce Gaines (22) 	
Patron attribute HOLDID: { "after" : "1234567890", "before" : "" }
	Intranet


Added back the cleared AUTOTYPE Lainausautomaatti. No record of it.
04/19/2024 07:08 	koha (51) 	Patrons 	Modify 	Joyce Gaines (22) 	
Patron attribute SSN: { "after" : "sotu234567", "before" : "" }
	Intranet
04/19/2024 07:08 	koha (51) 	Patrons 	Modify 	Joyce Gaines (22) 	
Patron attribute HOLDID: { "after" : "1234567890", "before" : "" }
	Intranet
04/19/2024 07:08 	koha (51) 	Patrons 	Modify 	Joyce Gaines (22) 	
Patron attribute AUTOTYPE: { "after" : [ "Palautusautomaatti" ], "before" : [] }
	Intranet
Comment 17 David Gustafsson 2024-04-19 15:14:47 UTC
Created attachment 165212 [details] [review]
Bug 26744: Don't delete extended attributes outside of method

And fix bug where $attribute_changes was overwritten
instead of assigned default value.
Comment 18 David Gustafsson 2024-04-19 15:14:50 UTC
Created attachment 165213 [details] [review]
Bug 26744: Add test for multiple values of repeatable fields
Comment 19 David Gustafsson 2024-04-19 15:19:09 UTC
@Anneli Österman Thanks for the extensive testing, I should have added tests also for updates with multiple values. There was a line outside of the extended_attributes method that deleted the attributes in memberentry.pl that was not caught by the tests as the method is tested in isolation. But there was also a bug where the stashed repeatable attributes was overwritten where a default value should be assigned, both are now fixed and tests have been added to cover the latter case.