Bug 26744 - Log changes to extended patron attributes
Summary: Log changes to extended patron attributes
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: David Gustafsson
QA Contact: Lucas Gass (lukeg)
URL:
Keywords: additional_work_needed
Depends on:
Blocks: 22794 39334 39635
  Show dependency treegraph
 
Reported: 2020-10-20 08:24 UTC by Magnus Enger
Modified: 2025-04-14 23:50 UTC (History)
17 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
The enhancements allows for the logging of patron attribute types when adding, removing, or updating them in a patron record.
Version(s) released in:
25.05.00
Circulation function:


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 (lukeg)
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 (lukeg)
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
Bug 26744: Log changes to extended patron attributes (6.42 KB, patch)
2024-04-22 14:10 UTC, Clemens Tubach
Details | Diff | Splinter Review
Bug 26744: Only log if BorrowersLog syspref is enabled (2.90 KB, patch)
2024-04-22 14:10 UTC, Clemens Tubach
Details | Diff | Splinter Review
Bug 26744: Add support for repeatable and deleted attributes (12.58 KB, patch)
2024-04-22 14:10 UTC, Clemens Tubach
Details | Diff | Splinter Review
Bug 26744: Don't delete extended attributes outside of method (2.36 KB, patch)
2024-04-22 14:10 UTC, Clemens Tubach
Details | Diff | Splinter Review
Bug 26744: Add test for multiple values of repeatable fields (3.19 KB, patch)
2024-04-22 14:10 UTC, Clemens Tubach
Details | Diff | Splinter Review
Bug 26744: Log changes to extended patron attributes (17.22 KB, patch)
2025-02-06 23:14 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 26744: Log changes to extended patron attributes (16.49 KB, patch)
2025-02-07 17:30 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 26744: Log changes to extended patron attributes (16.52 KB, patch)
2025-02-26 15:27 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 26744: Log changes to extended patron attributes (17.07 KB, patch)
2025-02-26 18:38 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 26744: Log changes also in modborrowers.pl and when calling add_extended_attribute (6.02 KB, patch)
2025-02-27 16:03 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 26744: Log changes also in modborrowers.pl and when calling add_extended_attribute (6.02 KB, patch)
2025-02-27 16:04 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 26744: Log changes also in modborrowers.pl and when calling add_extended_attribute (5.88 KB, patch)
2025-02-27 16:14 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 26744: Log changes to extended patron attributes (17.13 KB, patch)
2025-02-27 17:02 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26744: Log changes also in modborrowers.pl and when calling add_extended_attribute (5.93 KB, patch)
2025-02-27 17:02 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26744: (QA follow up) Tidy (4.03 KB, patch)
2025-02-27 17:13 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 26744: Cosolidate changes and change log format (3.32 KB, patch)
2025-02-28 12:09 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 26744: Log changes to extended patron attributes (17.19 KB, patch)
2025-02-28 15:25 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 26744: Log changes also in modborrowers.pl and when calling add_extended_attribute (5.99 KB, patch)
2025-02-28 15:25 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 26744: (QA follow up) Tidy (4.08 KB, patch)
2025-02-28 15:25 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 26744: Cosolidate changes and change log format (3.38 KB, patch)
2025-02-28 15:25 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 26744i: (follow up) Preserve order when adding patron attributes (6.09 KB, patch)
2025-03-13 19:02 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 26744: (follow up) Preserve order when adding patron attributes (6.08 KB, patch)
2025-03-13 19:03 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 26744: (follow up) Preserve order when adding patron attributes (6.05 KB, patch)
2025-03-13 19: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 (lukeg) 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 (lukeg) 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 (lukeg) 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.
Comment 20 Clemens Tubach 2024-04-22 14:10:32 UTC
Created attachment 165301 [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>
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Comment 21 Clemens Tubach 2024-04-22 14:10:35 UTC
Created attachment 165302 [details] [review]
Bug 26744: Only log if BorrowersLog syspref is enabled

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Comment 22 Clemens Tubach 2024-04-22 14:10:38 UTC
Created attachment 165303 [details] [review]
Bug 26744: Add support for repeatable and deleted attributes

Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Comment 23 Clemens Tubach 2024-04-22 14:10:41 UTC
Created attachment 165304 [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.

Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Comment 24 Clemens Tubach 2024-04-22 14:10:44 UTC
Created attachment 165305 [details] [review]
Bug 26744: Add test for multiple values of repeatable fields

Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Comment 25 David Gustafsson 2025-02-06 23:14:38 UTC
Created attachment 177605 [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>
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Comment 26 David Gustafsson 2025-02-06 23:20:35 UTC
Squashed (to simplify rabase and review) and rebasted against master. The changes in bug 35508 required some refactoring, and the changes in made there could be unified with the code here since both bugs deal with comparing changes for attributes.The comparison is now more granular and instead of replacing all attributes if anything changes the replacement is now on an attribute type level.
Comment 27 David Gustafsson 2025-02-06 23:22:39 UTC
Will probably have to perform som more manual testing, and perhaps add some more tests, before totally confident everything works as it should. But feel free to review the code meanwhile.
Comment 28 David Gustafsson 2025-02-06 23:27:11 UTC
Another improvement is that the comparison (that decides if attributes has changed) now resides in the extended_attributes method, and not outside in memberentry.pl, so the updated column will not be set if patron attributes are updated some where else using this method, when no value has changed.
Comment 29 David Gustafsson 2025-02-07 17:30:27 UTC
Created attachment 177652 [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>
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Comment 30 David Gustafsson 2025-02-26 15:27:54 UTC
Created attachment 178728 [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>
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Comment 31 David Gustafsson 2025-02-26 15:28:24 UTC
Rebased against main.
Comment 32 Lucas Gass (lukeg) 2025-02-26 18:38:14 UTC
Created attachment 178771 [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>
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Comment 33 Lucas Gass (lukeg) 2025-02-26 18:43:20 UTC
Since it was just a rebase we can keep Clemens SO line here.
Comment 34 Lucas Gass (lukeg) 2025-02-26 19:05:00 UTC
The only problem I now see here is that changes are not logged when I update attributes with the Batch patron modification tool.
Comment 35 David Gustafsson 2025-02-27 16:03:30 UTC
Created attachment 178787 [details] [review]
Bug 26744: Log changes also in modborrowers.pl and when calling add_extended_attribute
Comment 36 David Gustafsson 2025-02-27 16:04:58 UTC
Created attachment 178788 [details] [review]
Bug 26744: Log changes also in modborrowers.pl and when calling add_extended_attribute
Comment 37 David Gustafsson 2025-02-27 16:12:45 UTC
(In reply to Lucas Gass (lukeg) from comment #34)
> The only problem I now see here is that changes are not logged when I update
> attributes with the Batch patron modification tool.

Ok! This has now been fixed so that modborrowers.pl use the extended_attributes method instead and changes are logged. There is also a case in Koha::Patron->merge_with where patron attributes are modified by the add_extended_attribute method. The way it's used is not possible to replace it with the extended_attributes method, but I added logging also for add_extended_attribute, which is better really as it possibly cover other cases.
Comment 38 David Gustafsson 2025-02-27 16:14:38 UTC
Created attachment 178789 [details] [review]
Bug 26744: Log changes also in modborrowers.pl and when calling add_extended_attribute
Comment 39 Andrew Fuerste-Henry 2025-02-27 17:02:11 UTC
Created attachment 178801 [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>
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 40 Andrew Fuerste-Henry 2025-02-27 17:02:15 UTC
Created attachment 178802 [details] [review]
Bug 26744: Log changes also in modborrowers.pl and when calling add_extended_attribute

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 41 Andrew Fuerste-Henry 2025-02-27 17:04:18 UTC
Small note, the string this puts into the info field isn't formatted exactly like edits of other parts of the patron record. For example, here's an edit to a name field:
{ "preferred_name" : { "after" : "Huey", "before" : "Hank" } }

And an edit to an attribute:
Patron attribute SHOW_BCODE: { "after" : "1", "before" : "" }	

Not a blocker, but worth noting for its impact on reports.
Comment 42 Lucas Gass (lukeg) 2025-02-27 17:13:03 UTC
Created attachment 178803 [details] [review]
Bug 26744: (QA follow up) Tidy
Comment 43 Lucas Gass (lukeg) 2025-02-27 17:48:27 UTC
David, this is looking very nice. I tidied some things but you might want to look into pulling fresh ktd images and use the new pre-commit tidy stuff. See Bug 38664. You might face more QA challenges in the future without. 

I have one question here: 

Is List::Util's zip6 really necessary for comparison?
Comment 44 David Gustafsson 2025-02-28 12:09:26 UTC
Created attachment 178817 [details] [review]
Bug 26744: Cosolidate changes and change log format
Comment 45 David Gustafsson 2025-02-28 12:15:33 UTC
(In reply to Andrew Fuerste-Henry from comment #41)
> Small note, the string this puts into the info field isn't formatted exactly
> like edits of other parts of the patron record. For example, here's an edit
> to a name field:
> { "preferred_name" : { "after" : "Huey", "before" : "Hank" } }
> 
> And an edit to an attribute:
> Patron attribute SHOW_BCODE: { "after" : "1", "before" : "" }	
> 
> Not a blocker, but worth noting for its impact on reports.

Ok! I changed the log format to pure json, prepending "attribute." to the "field" name. Also saw that the normal field logging writes one log entry for all field changes, so fixed so this is done also for attribute changes.
Comment 46 David Gustafsson 2025-02-28 12:19:07 UTC
(In reply to Lucas Gass (lukeg) from comment #43)
> David, this is looking very nice. I tidied some things but you might want to
> look into pulling fresh ktd images and use the new pre-commit tidy stuff.
> See Bug 38664. You might face more QA challenges in the future without. 
> 
> I have one question here: 
> 
> Is List::Util's zip6 really necessary for comparison?

It could of course be done in other ways, but I don't see why using zip6 would be a problem as it makes the code compact and concise.
Comment 47 Andrew Fuerste-Henry 2025-02-28 13:13:12 UTC
(In reply to David Gustafsson from comment #45)
> (In reply to Andrew Fuerste-Henry from comment #41)
> > Small note, the string this puts into the info field isn't formatted exactly
> > like edits of other parts of the patron record. For example, here's an edit
> > to a name field:
> > { "preferred_name" : { "after" : "Huey", "before" : "Hank" } }
> > 
> > And an edit to an attribute:
> > Patron attribute SHOW_BCODE: { "after" : "1", "before" : "" }	
> > 
> > Not a blocker, but worth noting for its impact on reports.
> 
> Ok! I changed the log format to pure json, prepending "attribute." to the
> "field" name. Also saw that the normal field logging writes one log entry
> for all field changes, so fixed so this is done also for attribute changes.

Thanks, David! I can confirm the syntax change looks good and multiple attribute changes all log as one entry.
Is there some inherent reason attribute changes and edits to the regular borrower fields couldn't all go in one log line when performed together? If that's feasible, I may file a followup bug for it.
Comment 48 Lucas Gass (lukeg) 2025-02-28 15:25:01 UTC
Created attachment 178839 [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>
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 49 Lucas Gass (lukeg) 2025-02-28 15:25:06 UTC
Created attachment 178840 [details] [review]
Bug 26744: Log changes also in modborrowers.pl and when calling add_extended_attribute

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 50 Lucas Gass (lukeg) 2025-02-28 15:25:10 UTC
Created attachment 178841 [details] [review]
Bug 26744: (QA follow up) Tidy

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 51 Lucas Gass (lukeg) 2025-02-28 15:25:15 UTC
Created attachment 178842 [details] [review]
Bug 26744: Cosolidate changes and change log format

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 52 Katrin Fischer 2025-03-10 10:45:23 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 53 David Gustafsson 2025-03-10 11:31:19 UTC
(In reply to Andrew Fuerste-Henry from comment #47)
> (In reply to David Gustafsson from comment #45)
> > (In reply to Andrew Fuerste-Henry from comment #41)
> > > Small note, the string this puts into the info field isn't formatted exactly
> > > like edits of other parts of the patron record. For example, here's an edit
> > > to a name field:
> > > { "preferred_name" : { "after" : "Huey", "before" : "Hank" } }
> > > 
> > > And an edit to an attribute:
> > > Patron attribute SHOW_BCODE: { "after" : "1", "before" : "" }	
> > > 
> > > Not a blocker, but worth noting for its impact on reports.
> > 
> > Ok! I changed the log format to pure json, prepending "attribute." to the
> > "field" name. Also saw that the normal field logging writes one log entry
> > for all field changes, so fixed so this is done also for attribute changes.
> 
> Thanks, David! I can confirm the syntax change looks good and multiple
> attribute changes all log as one entry.
> Is there some inherent reason attribute changes and edits to the regular
> borrower fields couldn't all go in one log line when performed together? If
> that's feasible, I may file a followup bug for it.

That's not practically possible I'm afraid, even though I can se how it makes sense from a user perspective. as code wise fields and attributes are very different things, with no common methods for updates/changes.
Comment 54 Andrew Fuerste-Henry 2025-03-10 12:09:50 UTC
(In reply to David Gustafsson from comment #53)

> That's not practically possible I'm afraid, even though I can se how it
> makes sense from a user perspective. as code wise fields and attributes are
> very different things, with no common methods for updates/changes.

Thanks for confirming!
Comment 55 Jonathan Druart 2025-03-12 13:41:07 UTC
I suspect this change to be the cause of the following 2 failures:

$ prove t/db_dependent/api/v1/patrons.t
t/db_dependent/api/v1/patrons.t .. 3/7 
            #   Failed test at t/db_dependent/api/v1/patrons.t line 1042.
            #          got: 'zmEXaS_1HNKHibXWfmUaGo2s8GVHLodtQe5ulfn2oLO'
            #     expected: 'pD7HHacPDh6DExQ7wl2XBBZNj2cmz3_F0_iSUhNAFEVY4Wv16OS55oN9aVWVjB'

            #   Failed test at t/db_dependent/api/v1/patrons.t line 1042.
            #          got: 'zmEXaS_1HNKHibXWfmUaGo2s8GVHLodtQe5ulfn2oLO'
            #     expected: 'Z1dK8ALzVvAz3pjOgpVOzM9cVizAnWw7Fy8rB9eTfuLvCRCdhzqpgFcMN3'

            #   Failed test at t/db_dependent/api/v1/patrons.t line 1042.
            #          got: 'Z1dK8ALzVvAz3pjOgpVOzM9cVizAnWw7Fy8rB9eTfuLvCRCdhzqpgFcMN3'
            #     expected: 'zmEXaS_1HNKHibXWfmUaGo2s8GVHLodtQe5ulfn2oLO'

            #   Failed test at t/db_dependent/api/v1/patrons.t line 1042.
            #          got: 'pD7HHacPDh6DExQ7wl2XBBZNj2cmz3_F0_iSUhNAFEVY4Wv16OS55oN9aVWVjB'
            #     expected: 'zmEXaS_1HNKHibXWfmUaGo2s8GVHLodtQe5ulfn2oLO'
            # Looks like you failed 4 tests of 26.

        #   Failed test 'extended_attributes tests'
        #   at t/db_dependent/api/v1/patrons.t line 1045.
        # Looks like you failed 1 test of 44.

    #   Failed test 'librarian access tests'
    #   at t/db_dependent/api/v1/patrons.t line 1048.
    # Looks like you failed 1 test of 2.
t/db_dependent/api/v1/patrons.t .. 4/7 
#   Failed test 'update() tests'
#   at t/db_dependent/api/v1/patrons.t line 1049.
t/db_dependent/api/v1/patrons.t .. 7/7 # Looks like you failed 1 test of 7.
t/db_dependent/api/v1/patrons.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/7 subtests 



$ prove t/db_dependent/api/v1/patrons_extended_attributes.t
t/db_dependent/api/v1/patrons_extended_attributes.t .. 2/5 
    #   Failed test 'exact match for JSON Pointer "/0/type"'
    #   at t/db_dependent/api/v1/patrons_extended_attributes.t line 358.
    #          got: 'KDtH_WsRVKD5KY5R9N_VTJTJkZ6xfGKtqmD'
    #     expected: 'DDqUW_ML8Mz3FfSWsx2kOUoywRGNwi2UuGvNjhVKyqkoKexxB_0aidSXowl6'

    #   Failed test 'exact match for JSON Pointer "/0/value"'
    #   at t/db_dependent/api/v1/patrons_extended_attributes.t line 358.
    #          got: 'updated_mandatory'
    #     expected: 'updated_repeatable_1'

    #   Failed test 'exact match for JSON Pointer "/1/value"'
    #   at t/db_dependent/api/v1/patrons_extended_attributes.t line 358.
    #          got: 'updated_repeatable_1'
    #     expected: 'updated_repeatable_2'

    #   Failed test 'exact match for JSON Pointer "/2/value"'
    #   at t/db_dependent/api/v1/patrons_extended_attributes.t line 358.
    #          got: 'updated_repeatable_2'
    #     expected: 'updated_repeatable_3'

    #   Failed test 'exact match for JSON Pointer "/3/type"'
    #   at t/db_dependent/api/v1/patrons_extended_attributes.t line 358.
    #          got: 'DDqUW_ML8Mz3FfSWsx2kOUoywRGNwi2UuGvNjhVKyqkoKexxB_0aidSXowl6'
    #     expected: 'KDtH_WsRVKD5KY5R9N_VTJTJkZ6xfGKtqmD'

    #   Failed test 'exact match for JSON Pointer "/3/value"'
    #   at t/db_dependent/api/v1/patrons_extended_attributes.t line 358.
    #          got: 'updated_repeatable_3'
    #     expected: 'updated_mandatory'
    # Looks like you failed 6 tests of 29.
t/db_dependent/api/v1/patrons_extended_attributes.t .. 3/5 
#   Failed test 'overwrite() tests'
#   at t/db_dependent/api/v1/patrons_extended_attributes.t line 361.


They could eventually pass as it's a random failure, but it's failing almost every run locally.

The problem is the following:
We send some attributes to the PUT route, but return the attributes in a different order.
My guess is that the culprit is this line:
Koha/Patron.pm
2308         # Sort new attributes by code
2309         @new_attributes = sort { $a->attribute cmp $b->attribute } @new_attributes;

Please fix ASAP.
Comment 56 David Gustafsson 2025-03-13 18:48:55 UTC
I'm looking into it. I made sure the attributes is now saved in the order they where passed in, but also encountering some other failed tests. Not sure what is happening, but will sort it out.
Comment 57 David Gustafsson 2025-03-13 19:02:44 UTC
Created attachment 179284 [details] [review]
Bug 26744i: (follow up) Preserve order when adding patron attributes
Comment 58 David Gustafsson 2025-03-13 19:03:20 UTC
Created attachment 179285 [details] [review]
Bug 26744: (follow up) Preserve order when adding patron attributes
Comment 59 David Gustafsson 2025-03-13 19:08:51 UTC
I'm not sure about now patches for bugs that has already been push are handled, should I have created a new bug for this? Posting the patch her meanwhile. It was the insertion order that was the issue, but I initially put the code that checks for required types before storing the attributes, while it previously was the other way around, so tests failed since some errors that exceptions where thrown in a different order, so when there where many validation errors a different one showed up in the tests. When preserving the same order as before all tests pass.
Comment 60 David Gustafsson 2025-03-13 19:14:45 UTC
Created attachment 179286 [details] [review]
Bug 26744: (follow up) Preserve order when adding patron attributes
Comment 61 David Gustafsson 2025-03-14 14:53:28 UTC
Pretty sure should be handled as a new bug, so created Bug 39334.
Comment 62 Katrin Fischer 2025-03-14 15:01:21 UTC
(In reply to David Gustafsson from comment #61)
> Pretty sure should be handled as a new bug, so created Bug 39334.

Hi David, sorry for being a little late :( Follow-up patches on bugs not yet in a released version are fine. If I set "additional_work_needed" and don't aks for a new bug it's usually fine.
Comment 63 David Cook 2025-04-14 23:47:35 UTC
(In reply to David Gustafsson from comment #46)
> (In reply to Lucas Gass (lukeg) from comment #43)
> > Is List::Util's zip6 really necessary for comparison?
> 
> It could of course be done in other ways, but I don't see why using zip6
> would be a problem as it makes the code compact and concise.

It looks like zip6 became available in List::MoreUtils 0.423 (from August 2017). Older versions of Debian/Ubuntu used version 0.416 so they wouldn't have been able to support this change. Fortunately, all the currently supported versions of Debian/Ubuntu have version 0.430 so it's all good.

That said, I'll open a follow-up report to bump up the required version of List::MoreUtils in the cpanfile.