| Summary: | Logging patron attributes logs even if there's no changes | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Lisette Scheer <lisette> |
| Component: | Patrons | Assignee: | 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: | unspecified | Keywords: | 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
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. 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
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 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 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.
(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. (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! 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> 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> 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 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> 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> Nice work everyone! Pushed to main for 26.05 |