Bug 39322 - When pseudonymizing don't fetch patron attributes if none are kept
Summary: When pseudonymizing don't fetch patron attributes if none are kept
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Nick Clemens (kidclamp)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on: 39301
Blocks:
  Show dependency treegraph
 
Reported: 2025-03-13 13:09 UTC by Nick Clemens (kidclamp)
Modified: 2025-05-08 06:01 UTC (History)
3 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.05.00
Circulation function:


Attachments
Bug 39322: Don't fetch borrower attributes if none kept for pseudonymization (1.78 KB, patch)
2025-03-13 13:14 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 39322: Don't fetch borrower attributes if none kept for pseudonymization (1.83 KB, patch)
2025-04-01 09:42 UTC, David Nind
Details | Diff | Splinter Review
Bug 39322: Don't fetch borrower attributes if none kept for pseudonymization (1.87 KB, patch)
2025-05-02 12:56 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 39322: (follow-up) Further improvements (2.64 KB, patch)
2025-05-02 12:56 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 39322: (follow-up) Further improvements (2.70 KB, patch)
2025-05-02 13:12 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2025-03-13 13:09:35 UTC

    
Comment 1 Nick Clemens (kidclamp) 2025-03-13 13:14:55 UTC
Created attachment 179240 [details] [review]
Bug 39322: Don't fetch borrower attributes if none kept for pseudonymization

This patch simply checks the count of pseudonymized borrower attributes before
looping through them

To test:
 1 - Define several borrower attributes, none kept for pseudonymization
 2 - Add some values for those attributes to some patrons
 3 - Generate some statistics for those patrons (checkin, checkout, renew, etc)
 4 - Enable pseudonymization
 5 - On command line: export DBIC_TRACE=1
 6 - Run the pseudonymization script with a date in the future to ensure all transactions are captured:
    perl misc/maintenance/pseudonymize_statistics.pl -v -c --before 2026-01-01
 7 - Note the borrower attributes are fetched
 8 - Apply patch
 9 - repeat 6
10 - Note the attributes are only counted, not fetched
11 - Make one of the attributes kept
12 - repeat 6
13 - Note the attributes are counted and fetched
Comment 2 David Nind 2025-03-16 21:30:17 UTC
I had a go at testing, but I'm not sure about a couple of items in the test plan:

1. Step 1 - .."none kept for pseudonymization": How do you do this? Or is this only shown for the attribute once pseudonymization is enabled (the "Keep for pseudonymization" field)?

2. Step 7: How do I tell this? The output from running the command is listed at the end.

For statistics, I checked out a couple of items and checked them back in. Is this sufficient?


Testing notes (using KTD):
1. Generate a password in KTD from the shell: htpasswd -bnBC 10 "" password | tr -d ':\n' | sed 's/$2y/$2a/'
2. Edit /etc/koha/sites/kohadev/koha-conf.xml
3. Add an entry before the </config>:
   <bcrypt_settings>whateverstep1generated</bcrypt_settings>
4. Restart everything: restart_all

Output from step 6
------------------

perl misc/maintenance/pseudonymize_statistics.pl -v -c --before 2026-01-01
Searching for statistics before 2026-01-01
SELECT COUNT( * ) FROM `statistics` `me` WHERE ( `datetime` <= ? ): '2026-01-01'
4 statistics found
SELECT COUNT( * ) FROM `pseudonymized_transactions` `me` WHERE ( `datetime` <= ? ): '2026-01-01'
SELECT COUNT( * ) FROM `statistics` `me` WHERE ( `datetime` <= ? ): '2026-01-01'
SELECT `me`.`datetime`, `me`.`branch`, `me`.`value`, `me`.`type`, `me`.`other`, `me`.`itemnumber`, `me`.`itemtype`, `me`.`location`, `me`.`borrowernumber`, `me`.`ccode`, `me`.`categorycode`, `me`.`interface` FROM `statistics` `me` WHERE ( `datetime` <= ? ): '2026-01-01'
INSERT INTO `background_jobs` ( `borrowernumber`, `context`, `data`, `ended_on`, `enqueued_on`, `queue`, `size`, `started_on`, `status`, `type`) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ? ): NULL, '{"cardnumber":null,"interface":"commandline","shibboleth":null,"desk_name":null,"flags":null,"number":null,"desk_id":null,"branch":null,"surname":"CLI","firstname":"CLI","branchname":null,"register_id":null,"emailaddress":null,"register_name":null,"id":null}', '{"statistic":{"itemtype":"BK","interface":"intranet","type":"issue","borrowernumber":5,"value":0,"location":"GEN","branch":"CPL","itemnumber":578,"datetime":"2025-03-16 20:58:50","categorycode":"PT","ccode":"REF","other":""}}', NULL, '2025-03-16T21:05:29', 'default', '1', NULL, 'new', 'pseudonymize_statistic'
INSERT INTO `background_jobs` ( `borrowernumber`, `context`, `data`, `ended_on`, `enqueued_on`, `queue`, `size`, `started_on`, `status`, `type`) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ? ): NULL, '{"cardnumber":null,"interface":"commandline","shibboleth":null,"desk_name":null,"flags":null,"number":null,"desk_id":null,"branch":null,"surname":"CLI","firstname":"CLI","branchname":null,"register_id":null,"emailaddress":null,"register_name":null,"id":null}', '{"statistic":{"type":"return","itemtype":"BK","interface":"intranet","value":0,"location":"GEN","branch":"CPL","borrowernumber":5,"datetime":"2025-03-16 20:58:55","categorycode":"PT","itemnumber":578,"ccode":"REF","other":""}}', NULL, '2025-03-16T21:05:29', 'default', '1', NULL, 'new', 'pseudonymize_statistic'
INSERT INTO `background_jobs` ( `borrowernumber`, `context`, `data`, `ended_on`, `enqueued_on`, `queue`, `size`, `started_on`, `status`, `type`) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ? ): NULL, '{"cardnumber":null,"interface":"commandline","shibboleth":null,"desk_name":null,"flags":null,"number":null,"desk_id":null,"branch":null,"surname":"CLI","firstname":"CLI","branchname":null,"register_id":null,"emailaddress":null,"register_name":null,"id":null}', '{"statistic":{"ccode":"REF","other":"","categorycode":"PT","datetime":"2025-03-16 20:59:07","itemnumber":296,"value":0,"location":"GEN","branch":"CPL","borrowernumber":23,"type":"issue","itemtype":"BK","interface":"intranet"}}', NULL, '2025-03-16T21:05:29', 'default', '1', NULL, 'new', 'pseudonymize_statistic'
INSERT INTO `background_jobs` ( `borrowernumber`, `context`, `data`, `ended_on`, `enqueued_on`, `queue`, `size`, `started_on`, `status`, `type`) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ? ): NULL, '{"cardnumber":null,"interface":"commandline","shibboleth":null,"desk_name":null,"flags":null,"number":null,"desk_id":null,"branch":null,"surname":"CLI","firstname":"CLI","branchname":null,"register_id":null,"emailaddress":null,"register_name":null,"id":null}', '{"statistic":{"categorycode":"PT","datetime":"2025-03-16 20:59:15","itemnumber":296,"ccode":"REF","other":"","type":"return","interface":"intranet","itemtype":"BK","branch":"CPL","value":0,"location":"GEN","borrowernumber":23}}', NULL, '2025-03-16T21:05:29', 'default', '1', NULL, 'new', 'pseudonymize_statistic'
SELECT COUNT( * ) FROM `statistics` `me` WHERE ( `datetime` <= ? ): '2026-01-01'
4 statistics pseudonymized
Comment 3 Nick Clemens (kidclamp) 2025-03-31 14:03:47 UTC
(In reply to David Nind from comment #2)
> I had a go at testing, but I'm not sure about a couple of items in the test
> plan:
> 
> 1. Step 1 - .."none kept for pseudonymization": How do you do this? Or is
> this only shown for the attribute once pseudonymization is enabled (the
> "Keep for pseudonymization" field)?

Correct, it only shows when pseudonymization is available

> 2. Step 7: How do I tell this? The output from running the command is listed
> at the end.
> 
> For statistics, I checked out a couple of items and checked them back in. Is
> this sufficient?
> 
> 
> Testing notes (using KTD):
> 1. Generate a password in KTD from the shell: htpasswd -bnBC 10 "" password
> | tr -d ':\n' | sed 's/$2y/$2a/'
> 2. Edit /etc/koha/sites/kohadev/koha-conf.xml
> 3. Add an entry before the </config>:
>    <bcrypt_settings>whateverstep1generated</bcrypt_settings>
> 4. Restart everything: restart_all
> 
> Output from step 6
> ------------------
> 
> perl misc/maintenance/pseudonymize_statistics.pl -v -c --before 2026-01-01
> Searching for statistics before 2026-01-01
> SELECT COUNT( * ) FROM `statistics` `me` WHERE ( `datetime` <= ? ):
> '2026-01-01'
> 4 statistics found
> SELECT COUNT( * ) FROM `pseudonymized_transactions` `me` WHERE ( `datetime`
> <= ? ): '2026-01-01'
> SELECT COUNT( * ) FROM `statistics` `me` WHERE ( `datetime` <= ? ):
> '2026-01-01'
> SELECT `me`.`datetime`, `me`.`branch`, `me`.`value`, `me`.`type`,
> `me`.`other`, `me`.`itemnumber`, `me`.`itemtype`, `me`.`location`,
> `me`.`borrowernumber`, `me`.`ccode`, `me`.`categorycode`, `me`.`interface`
> FROM `statistics` `me` WHERE ( `datetime` <= ? ): '2026-01-01'
> INSERT INTO `background_jobs` ( `borrowernumber`, `context`, `data`,
> `ended_on`, `enqueued_on`, `queue`, `size`, `started_on`, `status`, `type`)
> VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ? ): NULL,
> '{"cardnumber":null,"interface":"commandline","shibboleth":null,"desk_name":
> null,"flags":null,"number":null,"desk_id":null,"branch":null,"surname":"CLI",
> "firstname":"CLI","branchname":null,"register_id":null,"emailaddress":null,
> "register_name":null,"id":null}',
> '{"statistic":{"itemtype":"BK","interface":"intranet","type":"issue",
> "borrowernumber":5,"value":0,"location":"GEN","branch":"CPL","itemnumber":
> 578,"datetime":"2025-03-16
> 20:58:50","categorycode":"PT","ccode":"REF","other":""}}', NULL,
> '2025-03-16T21:05:29', 'default', '1', NULL, 'new', 'pseudonymize_statistic'
> INSERT INTO `background_jobs` ( `borrowernumber`, `context`, `data`,
> `ended_on`, `enqueued_on`, `queue`, `size`, `started_on`, `status`, `type`)
> VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ? ): NULL,
> '{"cardnumber":null,"interface":"commandline","shibboleth":null,"desk_name":
> null,"flags":null,"number":null,"desk_id":null,"branch":null,"surname":"CLI",
> "firstname":"CLI","branchname":null,"register_id":null,"emailaddress":null,
> "register_name":null,"id":null}',
> '{"statistic":{"type":"return","itemtype":"BK","interface":"intranet",
> "value":0,"location":"GEN","branch":"CPL","borrowernumber":5,"datetime":
> "2025-03-16
> 20:58:55","categorycode":"PT","itemnumber":578,"ccode":"REF","other":""}}',
> NULL, '2025-03-16T21:05:29', 'default', '1', NULL, 'new',
> 'pseudonymize_statistic'
> INSERT INTO `background_jobs` ( `borrowernumber`, `context`, `data`,
> `ended_on`, `enqueued_on`, `queue`, `size`, `started_on`, `status`, `type`)
> VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ? ): NULL,
> '{"cardnumber":null,"interface":"commandline","shibboleth":null,"desk_name":
> null,"flags":null,"number":null,"desk_id":null,"branch":null,"surname":"CLI",
> "firstname":"CLI","branchname":null,"register_id":null,"emailaddress":null,
> "register_name":null,"id":null}',
> '{"statistic":{"ccode":"REF","other":"","categorycode":"PT","datetime":"2025-
> 03-16
> 20:59:07","itemnumber":296,"value":0,"location":"GEN","branch":"CPL",
> "borrowernumber":23,"type":"issue","itemtype":"BK","interface":"intranet"}}',
> NULL, '2025-03-16T21:05:29', 'default', '1', NULL, 'new',
> 'pseudonymize_statistic'
> INSERT INTO `background_jobs` ( `borrowernumber`, `context`, `data`,
> `ended_on`, `enqueued_on`, `queue`, `size`, `started_on`, `status`, `type`)
> VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ? ): NULL,
> '{"cardnumber":null,"interface":"commandline","shibboleth":null,"desk_name":
> null,"flags":null,"number":null,"desk_id":null,"branch":null,"surname":"CLI",
> "firstname":"CLI","branchname":null,"register_id":null,"emailaddress":null,
> "register_name":null,"id":null}',
> '{"statistic":{"categorycode":"PT","datetime":"2025-03-16
> 20:59:15","itemnumber":296,"ccode":"REF","other":"","type":"return",
> "interface":"intranet","itemtype":"BK","branch":"CPL","value":0,"location":
> "GEN","borrowernumber":23}}', NULL, '2025-03-16T21:05:29', 'default', '1',
> NULL, 'new', 'pseudonymize_statistic'
> SELECT COUNT( * ) FROM `statistics` `me` WHERE ( `datetime` <= ? ):
> '2026-01-01'
> 4 statistics pseudonymized
Comment 4 Nick Clemens (kidclamp) 2025-03-31 14:06:18 UTC
(In reply to David Nind from comment #2)
> I had a go at testing, but I'm not sure about a couple of items in the test
> plan:
> 
> 1. Step 1 - .."none kept for pseudonymization": How do you do this? Or is
> this only shown for the attribute once pseudonymization is enabled (the
> "Keep for pseudonymization" field)?

Correct, the option only shows once Pseudonymization is enabled
 
> 2. Step 7: How do I tell this? The output from running the command is listed
> at the end.

Ahm mthis depends on bug 39301, added to bug, You will see a line like:
SELECT `me`.`id`, `me`.`borrowernumber`, `me`.`code`, `me`.`attribute` FROM `borrower_attributes` `me` WHERE ( `me`.`borrowernumber` = ? ) ORDER BY `me`.`code`, `attribute`: '5'


> For statistics, I checked out a couple of items and checked them back in. Is
> this sufficient?

Yes, it should be
Comment 5 David Nind 2025-04-01 09:42:37 UTC
Created attachment 180109 [details] [review]
Bug 39322: Don't fetch borrower attributes if none kept for pseudonymization

This patch simply checks the count of pseudonymized borrower attributes before
looping through them

To test:
 1 - Define several borrower attributes, none kept for pseudonymization
 2 - Add some values for those attributes to some patrons
 3 - Generate some statistics for those patrons (checkin, checkout, renew, etc)
 4 - Enable pseudonymization
 5 - On command line: export DBIC_TRACE=1
 6 - Run the pseudonymization script with a date in the future to ensure all transactions are captured:
    perl misc/maintenance/pseudonymize_statistics.pl -v -c --before 2026-01-01
 7 - Note the borrower attributes are fetched
 8 - Apply patch
 9 - repeat 6
10 - Note the attributes are only counted, not fetched
11 - Make one of the attributes kept
12 - repeat 6
13 - Note the attributes are counted and fetched

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2025-04-01 09:44:26 UTC
(In reply to Nick Clemens (kidclamp) from comment #4)
> (In reply to David Nind from comment #2)
> > I had a go at testing, but I'm not sure about a couple of items in the test
> > plan:
> > 
> > 1. Step 1 - .."none kept for pseudonymization": How do you do this? Or is
> > this only shown for the attribute once pseudonymization is enabled (the
> > "Keep for pseudonymization" field)?
> 
> Correct, the option only shows once Pseudonymization is enabled
>  
> > 2. Step 7: How do I tell this? The output from running the command is listed
> > at the end.
> 
> Ahm mthis depends on bug 39301, added to bug, You will see a line like:
> SELECT `me`.`id`, `me`.`borrowernumber`, `me`.`code`, `me`.`attribute` FROM
> `borrower_attributes` `me` WHERE ( `me`.`borrowernumber` = ? ) ORDER BY
> `me`.`code`, `attribute`: '5'
> 
> 
> > For statistics, I checked out a couple of items and checked them back in. Is
> > this sufficient?
> 
> Yes, it should be

Thanks Nick!


Updated testing notes (using KTD):
1. Generate a password in KTD from the shell: htpasswd -bnBC 10 "" password | tr -d ':\n' | sed 's/$2y/$2a/'
2. Edit /etc/koha/sites/kohadev/koha-conf.xml
3. Add an entry before the </config>:
   <bcrypt_settings>whateverstep1generated</bcrypt_settings>
4. Restart everything: restart_all
5. Before step 6, apply bug 39301
Comment 7 Martin Renvoize (ashimema) 2025-05-02 12:56:11 UTC
Created attachment 181858 [details] [review]
Bug 39322: Don't fetch borrower attributes if none kept for pseudonymization

This patch simply checks the count of pseudonymized borrower attributes before
looping through them

To test:
 1 - Define several borrower attributes, none kept for pseudonymization
 2 - Add some values for those attributes to some patrons
 3 - Generate some statistics for those patrons (checkin, checkout, renew, etc)
 4 - Enable pseudonymization
 5 - On command line: export DBIC_TRACE=1
 6 - Run the pseudonymization script with a date in the future to ensure all transactions are captured:
    perl misc/maintenance/pseudonymize_statistics.pl -v -c --before 2026-01-01
 7 - Note the borrower attributes are fetched
 8 - Apply patch
 9 - repeat 6
10 - Note the attributes are only counted, not fetched
11 - Make one of the attributes kept
12 - repeat 6
13 - Note the attributes are counted and fetched

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 8 Martin Renvoize (ashimema) 2025-05-02 12:56:14 UTC
Created attachment 181859 [details] [review]
Bug 39322: (follow-up) Further improvements

This patch updates the query logic of create_from_statistic for borrower
attributes.

We replace the count of attributes types with a query to fetch all
attributes type codes where keep_for_pseudonymization is set.

We then check for content in that array before using it to perform a
filtered search on the patrons extended_attributes to only return
attributes set for pseudonymization.

This should result in the same skip of extended_attributes fetch should
no attribute types be set to pseudonymization, but it should also reduce
the 1 call to attribute types per attribute the patron has to just a
single filtered call for patron attributes attributes followed by the
store call for each pseudonymizated attribute.

Test plan:
As before, except there should be even fewer sql calls to the database
than before for the case where there are attributes.
Comment 9 Martin Renvoize (ashimema) 2025-05-02 12:58:37 UTC
I'm just getting Nick to confirm my follow-up, but I'm basically happy here to PQA.  Lets change the status as soon as Nick says he's happy or not with the follow-up.
Comment 10 Nick Clemens (kidclamp) 2025-05-02 13:12:46 UTC
Created attachment 181863 [details] [review]
Bug 39322: (follow-up) Further improvements

This patch updates the query logic of create_from_statistic for borrower
attributes.

We replace the count of attributes types with a query to fetch all
attributes type codes where keep_for_pseudonymization is set.

We then check for content in that array before using it to perform a
filtered search on the patrons extended_attributes to only return
attributes set for pseudonymization.

This should result in the same skip of extended_attributes fetch should
no attribute types be set to pseudonymization, but it should also reduce
the 1 call to attribute types per attribute the patron has to just a
single filtered call for patron attributes attributes followed by the
store call for each pseudonymizated attribute.

Test plan:
As before, except there should be even fewer sql calls to the database
than before for the case where there are attributes.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 11 Katrin Fischer 2025-05-06 16:00:07 UTC
Can you please rebase? We are moving a bit fast right now...

Bug 39322 - When pseudonymizing don't fetch patron attributes if none are kept

181863 - Bug 39322: (follow-up) Further improvements

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 39322: (follow-up) Further improvements
error: sha1 information is lacking or useless (Koha/PseudonymizedTransaction.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 39322: (follow-up) Further improvements
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-39322-follow-up-Further-improvements-lfu_vkmh.patch
Comment 12 Katrin Fischer 2025-05-06 16:00:27 UTC
Are we missing the first patch maybe?
Comment 13 Nick Clemens (kidclamp) 2025-05-07 11:57:20 UTC
(In reply to Katrin Fischer from comment #12)
> Are we missing the first patch maybe?

Looks like it was that, unobsoleted, applies now
Comment 14 Katrin Fischer 2025-05-08 06:01:19 UTC
Pushed for 25.05!

Well done everyone, thank you!