Bug 41145

Summary: Logging patron attributes logs even if there's no changes
Product: Koha Reporter: Lisette Scheer <lisette>
Component: PatronsAssignee: Alex Buckley <alexbuckley>
Status: Pushed to main --- QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: major    
Priority: P5 - low CC: aleisha, alexbuckley, david, gmcharlt, jonathan.druart, kyle, lucas, martin.renvoize, maxeinergl, nick
Version: unspecifiedKeywords: rel_26_05_candidate
Hardware: All   
OS: All   
GIT URL: Initiative type: ---
Sponsorship status: Sponsored Comma delimited list of Sponsors:
Crowdfunding goal: 0 Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This prevents misleading patron attribute modification logs, when a library batch imports patrons with the BorrowersLog system preference set to 'Log'. It now correctly only shows a log entry when a patron attribute value is changed. Example: - Before the change: for an existing patron with a patron attribute of INSTID:1234, with a re-import the log shows { "attribute.INSTID" : { "after" : "1234", "before" : "" } }, even though there is no change to the patron attribute. - After the change: . No log entry is shown if there is no change to the patron attribute. . If there is a change to the patron attribute (for example, changed to 5678 on a re-import), it is now correctly shown - { "attribute.INSTID" : { "after" : "5678", "before" : "1234" } }
Version(s) released in:
26.05.00
Circulation function:
Bug Depends on: 26744    
Bug Blocks:    
Attachments: Bug 41145: Do not delete the existing extended_attribute values when importing patrons
Bug 41145: Add unit test coverage for stopping duplicated borrowerslog logging due to extended patron attributes being deleted on import
Bug 41145: Do not delete the existing extended_attribute values when importing patrons
Bug 41145: Add unit test coverage for stopping duplicated borrowerslog logging due to extended patron attributes being deleted on import
Bug 41145: Do not delete the existing extended_attribute values when importing patrons
Bug 41145: Add unit test coverage for stopping duplicated borrowerslog logging due to extended patron attributes being deleted on import

Description Lisette Scheer 2025-10-30 16:37:04 UTC
Bug 26744 implemented logging for patron extended attributes.

If a patron is in a batch import and the attributes are the same, it logs a change even if nothing else changed, thus making the patron logs large and unhelpful.

This can be a problem for academic libraries who have automated loading processes to sync patron with their academic systems.

Maybe could use the diff, if diff o=n don't log?
Comment 1 Gretchen Maxeiner 2025-10-30 19:16:18 UTC
We are in this situation. Aside from a huge number of unnecessary log entries for the deleted/readded attributes, the entry is misleading to a staff member trouble-shooting on a patron who doesn't understand the quirk, and it blinds us on actual changes to a patron's attributes. Logging attribute changes is great but we only want to see actual changes.
Comment 2 Alex Buckley 2025-11-11 04:49:42 UTC
Hi Lisette and Gretchen, 

We notice that the 'before' value is always empty in the modification logs, for example:

{ "attribute.INSTID" : { "after" : "value", "before" : "" } } 

Is that something that you are also seeing?

Thanks,
Alex
Comment 3 Alex Buckley 2025-11-11 23:28:13 UTC
Created attachment 189503 [details] [review]
Bug 41145: Do not delete the existing extended_attribute values when importing patrons

Since bug 26744 was upstreamed when you use the Import patron tool or the import_patrons.pl script from the commandline and pass a preserve-extended-attributes parameter the borrowerlogs inaccurately have an entry added where the attribute 'before' value is empty.

Test plan:
1. Create a patron attribute type named 'INSTID': Koha administration > Patrons and circulation > Patron attribute types > New patron attribute type.
2. Download the patron started CSV from: Tools > Import patrons > click the 'Download a Starter CSV file with all the columns.' link
3. Save values in the: cardnumber, surname, firstname, branchcode, categorycode, auth_method, lang, and patron_attributes columns
4. Make sure your value in the patron_attributes column is INSTID:1234
5. Import this csv file:
cd misc
./import_patrons.pl --file <filename>.csv --matchpoint cardnumber --default branchcode=CPL --overwrite --preserve-extended-attributes --verbose --verbose --confirm

6. Navigate to the patron account in your browser
7. Click 'Modification log' and observe how many entries there are
8. Repeat step 5
9. Notice there is a new entry in your Modification log with an info column value of: { "attribute.INSTID" : { "after" : "1234", "before" : "" } }
10. Repeat step 5 and notice another new entry in the Modification log with an empty "before" value

11. Apply patch
12. Repeat step 5 and notice no new entry in the Modification log
13. Edit your csv and change the patron_attributes value to: INSTID:5678
14. Repeat step 5 and notice the new modification log entry with an info column value of: { "attribute.INSTID" : { "after" : "5678", "before" : "1234" } }

15. Add another new patron attribute type to your Koha named 'home'
16. edit your csv and change the patron_attributes value to: INSTID:5678,home:town
17. Repeat step 5
18. Notice a new Modification log entry with the value of: { "attribute.home" : { "after" : "town", "before" : "" } }

Sponsored-by: Auckland University of Technology
Comment 4 Alex Buckley 2025-11-12 00:04:03 UTC
Created attachment 189504 [details] [review]
Bug 41145: Add unit test coverage for stopping duplicated borrowerslog logging due to extended patron attributes being deleted on import

This adds to the unit tests in t/db_dependent/Koha/Patrons/Import.t. It confirms that repeating the same patron import (with extended patron attributes) will not generate duplicated borrowersLog entries.

Test plan:
1. prove t/db_dependent/Koha/Patrons/Import.t

Sponsored-by: Auckland University of Technology
Comment 5 Alex Buckley 2025-11-12 00:15:15 UTC
We have a partner library also seeing patron attribute logging even when there is no change made. This only happens for patron imports (either via the 'Import patron' tool in the staff client or running the import_patrons.pl script).

Editing a patron account in the staff client and saving the attributes (without making any changes) does not generate extraneous log entries.

Reviewing the logs from the patron imports, we see that the 'before' value is always blank, like so:

{ "attribute.INSTID" : { "after" : "value", "before" : "" } } 

This is the case even if the patron attribute actually did have an existing value.

Debugging the code I think this blanking of the existing patron attribute value  results in Koha logging a patron attribute change, because it regards the patron import as setting a value for a blank attribute. Even though that is no actually the case.

The existing patron attribute is being blanked by https://git.koha-community.org/Koha-community/Koha/src/commit/74d5af9b39ab1f6e66e9e94f6335e29f5ecc60a0/Koha/Patrons/Import.pm#L382

I don't think that line should be there, otherwise we cannot pass a 'before' value through to Koha/Patron->extended_attributes() in the next line for logging.
Comment 6 Gretchen Maxeiner 2025-12-01 14:00:39 UTC
(In reply to Alex Buckley from comment #2)
> Hi Lisette and Gretchen, 
> 
> We notice that the 'before' value is always empty in the modification logs,
> for example:
> 
> { "attribute.INSTID" : { "after" : "value", "before" : "" } } 
> 
> Is that something that you are also seeing?
> 
> Thanks,
> Alex

A late reply to confirm this is also what we see -- the attibute "before" is listed as having been blank, but this was not true before the import.
Comment 7 Alex Buckley 2025-12-01 19:48:26 UTC
(In reply to Gretchen Maxeiner from comment #6)
> (In reply to Alex Buckley from comment #2)
> > Hi Lisette and Gretchen, 
> > 
> > We notice that the 'before' value is always empty in the modification logs,
> > for example:
> > 
> > { "attribute.INSTID" : { "after" : "value", "before" : "" } } 
> > 
> > Is that something that you are also seeing?
> > 
> > Thanks,
> > Alex
> 
> A late reply to confirm this is also what we see -- the attibute "before" is
> listed as having been blank, but this was not true before the import.

Many thanks for confirming Gretchen!
Comment 8 David Nind 2025-12-03 23:47:52 UTC
Created attachment 190139 [details] [review]
Bug 41145: Do not delete the existing extended_attribute values when importing patrons

Since bug 26744 was upstreamed when you use the Import patron tool or the import_patrons.pl script from the commandline and pass a preserve-extended-attributes parameter the borrowerlogs inaccurately have an entry added where the attribute 'before' value is empty.

Test plan:
1. Create a patron attribute type named 'INSTID': Koha administration > Patrons and circulation > Patron attribute types > New patron attribute type.
2. Download the patron started CSV from: Tools > Import patrons > click the 'Download a Starter CSV file with all the columns.' link
3. Save values in the: cardnumber, surname, firstname, branchcode, categorycode, auth_method, lang, and patron_attributes columns
4. Make sure your value in the patron_attributes column is INSTID:1234
5. Import this csv file:
cd misc
./import_patrons.pl --file <filename>.csv --matchpoint cardnumber --default branchcode=CPL --overwrite --preserve-extended-attributes --verbose --verbose --confirm

6. Navigate to the patron account in your browser
7. Click 'Modification log' and observe how many entries there are
8. Repeat step 5
9. Notice there is a new entry in your Modification log with an info column value of: { "attribute.INSTID" : { "after" : "1234", "before" : "" } }
10. Repeat step 5 and notice another new entry in the Modification log with an empty "before" value

11. Apply patch
12. Repeat step 5 and notice no new entry in the Modification log
13. Edit your csv and change the patron_attributes value to: INSTID:5678
14. Repeat step 5 and notice the new modification log entry with an info column value of: { "attribute.INSTID" : { "after" : "5678", "before" : "1234" } }

15. Add another new patron attribute type to your Koha named 'home'
16. edit your csv and change the patron_attributes value to: INSTID:5678,home:town
17. Repeat step 5
18. Notice a new Modification log entry with the value of: { "attribute.home" : { "after" : "town", "before" : "" } }

Sponsored-by: Auckland University of Technology
Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2025-12-03 23:47:56 UTC
Created attachment 190140 [details] [review]
Bug 41145: Add unit test coverage for stopping duplicated borrowerslog logging due to extended patron attributes being deleted on import

This adds to the unit tests in t/db_dependent/Koha/Patrons/Import.t. It confirms that repeating the same patron import (with extended patron attributes) will not generate duplicated borrowersLog entries.

Test plan:
1. prove t/db_dependent/Koha/Patrons/Import.t

Sponsored-by: Auckland University of Technology
Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2025-12-04 00:02:12 UTC
Testing notes (using KTD):

1. Values for the csv file:
   . cardnumber = 987654321
   . surname = Surname
   . firstname = Firstname
   . branchcode = CPL
   . categorycode = PT
   . auth_method = password
   . lang = fr
   . patron_attributes columns = INSTID:1234
Comment 11 Martin Renvoize (ashimema) 2025-12-04 12:51:21 UTC
Created attachment 190150 [details] [review]
Bug 41145: Do not delete the existing extended_attribute values when importing patrons

Since bug 26744 was upstreamed when you use the Import patron tool or the import_patrons.pl script from the commandline and pass a preserve-extended-attributes parameter the borrowerlogs inaccurately have an entry added where the attribute 'before' value is empty.

Test plan:
1. Create a patron attribute type named 'INSTID': Koha administration > Patrons and circulation > Patron attribute types > New patron attribute type.
2. Download the patron started CSV from: Tools > Import patrons > click the 'Download a Starter CSV file with all the columns.' link
3. Save values in the: cardnumber, surname, firstname, branchcode, categorycode, auth_method, lang, and patron_attributes columns
4. Make sure your value in the patron_attributes column is INSTID:1234
5. Import this csv file:
cd misc
./import_patrons.pl --file <filename>.csv --matchpoint cardnumber --default branchcode=CPL --overwrite --preserve-extended-attributes --verbose --verbose --confirm

6. Navigate to the patron account in your browser
7. Click 'Modification log' and observe how many entries there are
8. Repeat step 5
9. Notice there is a new entry in your Modification log with an info column value of: { "attribute.INSTID" : { "after" : "1234", "before" : "" } }
10. Repeat step 5 and notice another new entry in the Modification log with an empty "before" value

11. Apply patch
12. Repeat step 5 and notice no new entry in the Modification log
13. Edit your csv and change the patron_attributes value to: INSTID:5678
14. Repeat step 5 and notice the new modification log entry with an info column value of: { "attribute.INSTID" : { "after" : "5678", "before" : "1234" } }

15. Add another new patron attribute type to your Koha named 'home'
16. edit your csv and change the patron_attributes value to: INSTID:5678,home:town
17. Repeat step 5
18. Notice a new Modification log entry with the value of: { "attribute.home" : { "after" : "town", "before" : "" } }

Sponsored-by: Auckland University of Technology
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 12 Martin Renvoize (ashimema) 2025-12-04 12:51:23 UTC
Created attachment 190151 [details] [review]
Bug 41145: Add unit test coverage for stopping duplicated borrowerslog logging due to extended patron attributes being deleted on import

This adds to the unit tests in t/db_dependent/Koha/Patrons/Import.t. It confirms that repeating the same patron import (with extended patron attributes) will not generate duplicated borrowersLog entries.

Test plan:
1. prove t/db_dependent/Koha/Patrons/Import.t

Sponsored-by: Auckland University of Technology
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 13 Lucas Gass (lukeg) 2025-12-17 20:26:53 UTC
Nice work everyone!

Pushed to main for 26.05