It looks like changes to a patron's "extended patron attributes" are not logged, when BorrowersLog is on. They should probably be.
If we want to be really flexible, we could make logging a per-attribute setting.
Logging would be a good thing.
+1
Still relevant in 21.11 and still would be useful!
+1, this would be helpful
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
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
Created attachment 163959 [details] [review] Bug 26744: Only log if BorrowersLog syspref is enabled
This works as described so I will sign-off. Why not log repeatable attributes as well?
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>
Created attachment 164035 [details] [review] Bug 26744: Only log if BorrowersLog syspref is enabled Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
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.
(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.
Created attachment 164098 [details] [review] Bug 26744: Add support for repeatable and deleted attributes
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.
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
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.
Created attachment 165213 [details] [review] Bug 26744: Add test for multiple values of repeatable fields
@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.
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>
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>
Created attachment 165303 [details] [review] Bug 26744: Add support for repeatable and deleted attributes Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
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>
Created attachment 165305 [details] [review] Bug 26744: Add test for multiple values of repeatable fields Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>