Bug 23260

Summary: Anonymize (remove) patron data from items_last_borrower
Product: Koha Reporter: Joy Nelson <1joynelson>
Component: Command-line UtilitiesAssignee: Agustín Moyano <agustinmoyano>
Status: Signed Off --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: andrewfh, cbrannon, fridolin.somers, george, jonathan.druart, katrin.fischer, kyle, Laura.escamilla, liz, m.de.rooy, martin.renvoize, maryse.simard, nick, robin, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9011
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14945
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24576
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 24092    
Attachments: Bug 23260: Make borrowernumber nullable in items_last_borrower
Bug 23260: Add anonymize items_last_borrower feature
Bug 23260: Add anonymize items_last_borrower feature
Bug 23260: Make borrowernumber nullable in items_last_borrower
Bug 23260: Add anonymize items_last_borrower feature
Bug 23260: (follow-up) optimize anonymise_items_last_borrower
Bug 23260: Make borrowernumber nullable in items_last_borrower
Bug 23260: Add anonymize items_last_borrower feature
Bug 23260: (follow-up) optimize anonymise_items_last_borrower
Bug 23260: (follow-up) optimize anonymise_items_last_borrower
Bug 23260: (QA follow-up) Use ->update instead of ->next
Bug 23260: Make borrowernumber nullable in items_last_borrower
Bug 23260: Add anonymize items_last_borrower feature
Bug 23260: (follow-up) optimize anonymise_items_last_borrower
Bug 23260: (QA follow-up) Use ->update instead of ->next
Bug 23260: Make borrowernumber nullable in items_last_borrower
Bug 23260: Add anonymize items_last_borrower feature
Bug 23260: (follow-up) optimize anonymise_items_last_borrower
Bug 23260: (QA follow-up) Use ->update instead of ->next
Bug 23260: [ALTERNATIVE] Inline code to anonymise_issue_history
Bug 23260: Make borrowernumber nullable in items_last_borrower
Bug 23260: Inline code to anonymise_issue_history
Bug 23260: Inline code to anonymise_issue_history
Bug 23260: Simplify tests
Bug 23260: Add a new test
Bug 23260: (follow-up) Remove last_borrower updates from issues count
Bug 23260: Add a new test
Bug 23260: Add a new test
Bug 23260: Proposed preference driven filtering
Bug 23260: Make borrowernumber nullable in items_last_borrower
Bug 23260: Inline code to anonymise_issue_history
Bug 23260: Simplify tests
Bug 23260: (follow-up) Remove last_borrower updates from issues count
Bug 23260: Add a new test
Bug 23260: Proposed preference driven filtering
Bug 23260: (follow-up) Add AnonymiseLastBorrower and AnonymiseLastBorrowerDays sysprefs
Bug 23260: (follow-up) Update test
Bug 23260: (follow-up) Add new cron job
Bug 23260: (follow-up) Add anonymise_last_borrowers method to Patrons.pm
Bug 23260: Make borrowernumber nullable in items_last_borrower
Bug 23260: Inline code to anonymise_issue_history
Bug 23260: Simplify tests
Bug 23260: (follow-up) Remove last_borrower updates from issues count
Bug 23260: Add a new test
Bug 23260: Proposed preference driven filtering
Bug 23260: (follow-up) Add AnonymiseLastBorrower and AnonymiseLastBorrowerDays sysprefs
Bug 23260: (follow-up) Update test
Bug 23260: (follow-up) Add new cron job
Bug 23260: (follow-up) Add anonymise_last_borrowers method to Patrons.pm
Bug 23260: Make borrowernumber nullable in items_last_borrower
Bug 23260: Inline code to anonymise_issue_history
Bug 23260: Simplify tests
Bug 23260: (follow-up) Remove last_borrower updates from issues count
Bug 23260: Add a new test
Bug 23260: Proposed preference driven filtering
Bug 23260: (follow-up) Add AnonymiseLastBorrower and AnonymiseLastBorrowerDays sysprefs
Bug 23260: (follow-up) Update test
Bug 23260: (follow-up) Add new cron job
Bug 23260: (follow-up) Add anonymise_last_borrowers method to Patrons.pm
Bug 23260: Make borrowernumber nullable in items_last_borrower
Bug 23260: Inline code to anonymise_issue_history
Bug 23260: Simplify tests
Bug 23260: (follow-up) Remove last_borrower updates from issues count
Bug 23260: Add a new test
Bug 23260: Proposed preference driven filtering
Bug 23260: (follow-up) Add AnonymiseLastBorrower and AnonymiseLastBorrowerDays sysprefs
Bug 23260: (follow-up) Update test
Bug 23260: (follow-up) Add new cron job
Bug 23260: (follow-up) Add anonymise_last_borrowers method to Patrons.pm
Bug 23260: Make borrowernumber nullable in items_last_borrower
Bug 23260: Inline code to anonymise_issue_history
Bug 23260: Simplify tests
Bug 23260: (follow-up) Remove last_borrower updates from issues count
Bug 23260: Add a new test
Bug 23260: Proposed preference driven filtering
Bug 23260: (follow-up) Add AnonymiseLastBorrower and AnonymiseLastBorrowerDays sysprefs
Bug 23260: (follow-up) Update test
Bug 23260: (follow-up) Add new cron job
Bug 23260: (follow-up) Add anonymise_last_borrowers method to Patrons.pm
Bug 23260: Make borrowernumber nullable in items_last_borrower
Bug 23260: Inline code to anonymise_issue_history
Bug 23260: Simplify tests
Bug 23260: (follow-up) Remove last_borrower updates from issues count
Bug 23260: Add a new test
Bug 23260: Proposed preference driven filtering
Bug 23260: (follow-up) Add AnonymiseLastBorrower and AnonymiseLastBorrowerDays sysprefs
Bug 23260: (follow-up) Update test
Bug 23260: (follow-up) Add new cron job
Bug 23260: (follow-up) Add anonymise_last_borrowers method to Patrons.pm
Bug 23260: (follow-up) Tidy
Bug 23260: Database updates and new system preferences AnonymizeLastBorrower / AnonymizeLastBorrowerDays
Bug 23260: Add anonymize_last_borrowers method to Patrons.pm
Bug 23260: Unit tests
Bug 23260: Add cronjob anonymize_last_borrowers.pl
Bug 23260: Database updates and new system preferences AnonymizeLastBorrower / AnonymizeLastBorrowerDays
Bug 23260: Add anonymize_last_borrowers method to Patrons.pm
Bug 23260: Unit tests
Bug 23260: Add cronjob anonymize_last_borrowers.pl

Description Joy Nelson 2019-07-03 17:49:32 UTC
Increase level of patron privacy by removing patron information from the items_last_borrower table after a set period of time. 

Adjust the batch_anonymize cronjob to also remove data from the items_last_borrower table when the data is more than x days old (runtime option) but not if the item is lost, damaged, or withdrawn.
Comment 1 Lisette Scheer 2019-08-23 17:01:36 UTC
*** Bug 23500 has been marked as a duplicate of this bug. ***
Comment 2 Christopher Brannon 2019-08-23 17:18:34 UTC
+1
Comment 3 Agustín Moyano 2019-09-16 21:18:48 UTC
Created attachment 92863 [details] [review]
Bug 23260: Make borrowernumber nullable in items_last_borrower

This patch alters table structure to make borrowernumber nullable
Comment 4 Agustín Moyano 2019-09-16 21:18:52 UTC
Created attachment 92864 [details] [review]
Bug 23260: Add anonymize items_last_borrower feature

This patch adds the feature to anonymize patrons from items_last_borrower table. This code is run from batch_anonymize script, which is triggered from a cron job.

To test:
1) apply this patch and the previous one
2) perl installer/data/mysql/updatedatabase.pl
3) set StoreLastBorrower preference to Allow.
4) Create a Check out followed by a Check in.
CHECK => a row should appear in items_last_borrower table with the borrower and the item number.
5) In mysql, update created_on date to one day earlier.
6) set AnonymousPatron preference to a valid patron id
7) perl misc/cronjobs/batch_anonymise.pl
SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron id
8) repeat steps 4 and 5
9) set AnonymousPatron preference to 0
10) perl misc/cronjobs/batch_anonymise.pl
SUCCESS => borrower number in items_last_borrower changed to null
11) prove t/db_dependent/Koha/Patrons.t
12) Sign off
Comment 5 Maryse Simard 2019-09-20 20:20:58 UTC
Created attachment 93052 [details] [review]
Bug 23260: Add anonymize items_last_borrower feature

This patch adds the feature to anonymize patrons from items_last_borrower table. This code is run from batch_anonymize script, which is triggered from a cron job.

To test:
1) apply this patch and the previous one
2) perl installer/data/mysql/updatedatabase.pl
3) set StoreLastBorrower preference to Allow.
4) Create a Check out followed by a Check in.
CHECK => a row should appear in items_last_borrower table with the borrower and the item number.
5) In mysql, update created_on date to one day earlier.
6) set AnonymousPatron preference to a valid patron id
7) perl misc/cronjobs/batch_anonymise.pl
SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron id
8) repeat steps 4 and 5
9) set AnonymousPatron preference to 0
10) perl misc/cronjobs/batch_anonymise.pl
SUCCESS => borrower number in items_last_borrower changed to null
11) prove t/db_dependent/Koha/Patrons.t
12) Sign off

Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>
Comment 6 Martin Renvoize 2019-10-29 11:58:27 UTC
Comment on attachment 93052 [details] [review]
Bug 23260: Add anonymize items_last_borrower feature

Review of attachment 93052 [details] [review]:
-----------------------------------------------------------------

Minor typo, otherwise this looks reasonable.

::: misc/cronjobs/batch_anonymise.pl
@@ +73,4 @@
>  my ($year,$month,$day) = Today();
>  my ($newyear,$newmonth,$newday) = Add_Delta_Days ($year,$month,$day,(-1)*$days);
>  my $formatdate = sprintf "%4d-%02d-%02d",$newyear,$newmonth,$newday;
> +$verbose and print "Checkouts and items las borrowers before $formatdate will be anonymised.\n";

las -> last
Comment 7 Agustín Moyano 2019-10-30 03:35:04 UTC
Created attachment 94858 [details] [review]
Bug 23260: Make borrowernumber nullable in items_last_borrower

This patch alters table structure to make borrowernumber nullable
Comment 8 Agustín Moyano 2019-10-30 03:35:08 UTC
Created attachment 94859 [details] [review]
Bug 23260: Add anonymize items_last_borrower feature

This patch adds the feature to anonymize patrons from items_last_borrower table. This code is run from batch_anonymize script, which is triggered from a cron job.

To test:
1) apply this patch and the previous one
2) perl installer/data/mysql/updatedatabase.pl
3) set StoreLastBorrower preference to Allow.
4) Create a Check out followed by a Check in.
CHECK => a row should appear in items_last_borrower table with the borrower and the item number.
5) In mysql, update created_on date to one day earlier.
6) set AnonymousPatron preference to a valid patron id
7) perl misc/cronjobs/batch_anonymise.pl
SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron id
8) repeat steps 4 and 5
9) set AnonymousPatron preference to 0
10) perl misc/cronjobs/batch_anonymise.pl
SUCCESS => borrower number in items_last_borrower changed to null
11) prove t/db_dependent/Koha/Patrons.t
12) Sign off

Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>
Comment 9 Agustín Moyano 2019-10-30 03:36:31 UTC
(In reply to Martin Renvoize from comment #6)
> Comment on attachment 93052 [details] [review] [review]
> Bug 23260: Add anonymize items_last_borrower feature
> 
> Review of attachment 93052 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Minor typo, otherwise this looks reasonable.
> 
> ::: misc/cronjobs/batch_anonymise.pl
> @@ +73,4 @@
> >  my ($year,$month,$day) = Today();
> >  my ($newyear,$newmonth,$newday) = Add_Delta_Days ($year,$month,$day,(-1)*$days);
> >  my $formatdate = sprintf "%4d-%02d-%02d",$newyear,$newmonth,$newday;
> > +$verbose and print "Checkouts and items las borrowers before $formatdate will be anonymised.\n";
> 
> las -> last

Fixed typo, and rebased
Comment 10 Marcel de Rooy 2019-11-01 10:14:49 UTC
QAing
Comment 11 Marcel de Rooy 2019-11-01 11:00:04 UTC
Hi Augustin,

I would like to see you optimize a bit.

for => $table_to_anonymize
Both anonymize and anonymise are correct and in use, but you mix them on this report (no blocker, just seeing it). The z spelling is American.
But I do not really understand why you put the patron select in search_patrons_to_anonymise. Why not leave this sub alone? We could directly call anonymise_items_last_borrower, since you are not really interested in the patrons here. And optimize it rightaway. Make the query in the new sub, and run update on that result set without looping with next?

Thanks.
Comment 12 Agustín Moyano 2019-11-04 19:37:21 UTC
Created attachment 95033 [details] [review]
Bug 23260: (follow-up) optimize anonymise_items_last_borrower
Comment 13 Marcel de Rooy 2019-11-05 15:22:36 UTC
Thx for continuing here, Augustin.

Just wondering what happens here:
-    subtest 'Logged in librarian is not superlibrarian & IndependentBranches' => sub {

Why remove another subtest??
Comment 14 Agustín Moyano 2019-11-05 16:55:36 UTC
(In reply to Marcel de Rooy from comment #13)
> Thx for continuing here, Augustin.
> 
> Just wondering what happens here:
> -    subtest 'Logged in librarian is not superlibrarian &
> IndependentBranches' => sub {
> 
> Why remove another subtest??

Hi Marcel, now that we don't search the patron to anonymise, this test has no sense.. 

We can no longer check the library of the borrower in case IndependentBranches preference is set on.

Thanks
Comment 15 Agustín Moyano 2019-11-15 17:13:00 UTC
Created attachment 95457 [details] [review]
Bug 23260: Make borrowernumber nullable in items_last_borrower

This patch alters table structure to make borrowernumber nullable

Sponsored-by: Northeast Kansas Library System
Comment 16 Agustín Moyano 2019-11-15 17:13:04 UTC
Created attachment 95458 [details] [review]
Bug 23260: Add anonymize items_last_borrower feature

This patch adds the feature to anonymize patrons from items_last_borrower table. This code is run from batch_anonymize script, which is triggered from a cron job.

To test:
1) apply this patch and the previous one
2) perl installer/data/mysql/updatedatabase.pl
3) set StoreLastBorrower preference to Allow.
4) Create a Check out followed by a Check in.
CHECK => a row should appear in items_last_borrower table with the borrower and the item number.
5) In mysql, update created_on date to one day earlier.
6) set AnonymousPatron preference to a valid patron id
7) perl misc/cronjobs/batch_anonymise.pl
SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron id
8) repeat steps 4 and 5
9) set AnonymousPatron preference to 0
10) perl misc/cronjobs/batch_anonymise.pl
SUCCESS => borrower number in items_last_borrower changed to null
11) prove t/db_dependent/Koha/Patrons.t
12) Sign off

Sponsored-by: Northeast Kansas Library System
Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>
Comment 17 Agustín Moyano 2019-11-15 17:13:09 UTC
Created attachment 95459 [details] [review]
Bug 23260: (follow-up) optimize anonymise_items_last_borrower

Sponsored-by: Northeast Kansas Library System
Comment 18 Christopher Brannon 2019-11-15 18:44:01 UTC
(In reply to Maryse Simard from comment #5)
> Created attachment 93052 [details] [review] [review]
> Bug 23260: Add anonymize items_last_borrower feature
> 
> This patch adds the feature to anonymize patrons from items_last_borrower
> table. This code is run from batch_anonymize script, which is triggered from
> a cron job.
> 
> To test:
> 1) apply this patch and the previous one
> 2) perl installer/data/mysql/updatedatabase.pl
> 3) set StoreLastBorrower preference to Allow.
> 4) Create a Check out followed by a Check in.
> CHECK => a row should appear in items_last_borrower table with the borrower
> and the item number.
> 5) In mysql, update created_on date to one day earlier.
> 6) set AnonymousPatron preference to a valid patron id
> 7) perl misc/cronjobs/batch_anonymise.pl
> SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron
> id
> 8) repeat steps 4 and 5
> 9) set AnonymousPatron preference to 0
> 10) perl misc/cronjobs/batch_anonymise.pl
> SUCCESS => borrower number in items_last_borrower changed to null
> 11) prove t/db_dependent/Koha/Patrons.t
> 12) Sign off
> 
> Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>

Your test plan has me concerned.  If you must first set the feature to allow (#3), which is what Koha currently does without this feature, it sounds like this feature is not on by default.  Is that true?  It should retain the current behavior of Koha by default.
Comment 19 Agustín Moyano 2019-11-15 19:03:06 UTC
(In reply to Christopher Brannon from comment #18)
> (In reply to Maryse Simard from comment #5)
> > Created attachment 93052 [details] [review] [review] [review]
> > Bug 23260: Add anonymize items_last_borrower feature
> > 
> > This patch adds the feature to anonymize patrons from items_last_borrower
> > table. This code is run from batch_anonymize script, which is triggered from
> > a cron job.
> > 
> > To test:
> > 1) apply this patch and the previous one
> > 2) perl installer/data/mysql/updatedatabase.pl
> > 3) set StoreLastBorrower preference to Allow.
> > 4) Create a Check out followed by a Check in.
> > CHECK => a row should appear in items_last_borrower table with the borrower
> > and the item number.
> > 5) In mysql, update created_on date to one day earlier.
> > 6) set AnonymousPatron preference to a valid patron id
> > 7) perl misc/cronjobs/batch_anonymise.pl
> > SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron
> > id
> > 8) repeat steps 4 and 5
> > 9) set AnonymousPatron preference to 0
> > 10) perl misc/cronjobs/batch_anonymise.pl
> > SUCCESS => borrower number in items_last_borrower changed to null
> > 11) prove t/db_dependent/Koha/Patrons.t
> > 12) Sign off
> > 
> > Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>
> 
> Your test plan has me concerned.  If you must first set the feature to allow
> (#3), which is what Koha currently does without this feature, it sounds like
> this feature is not on by default.  Is that true?  It should retain the
> current behavior of Koha by default.

Hi Christopher, thanks for checking this out.

This bug does not touch the StoreLastBorrower's default value (I had to change it in my koha-testing-docker, so I asumed it came off by default).. It's just to make sure that preference is set to Allow for the test to succeed.

Sorry if I didn't make this test plan clear.
Comment 20 Maryse Simard 2019-11-15 19:08:32 UTC
Created attachment 95460 [details] [review]
Bug 23260: (follow-up) optimize anonymise_items_last_borrower

Sponsored-by: Northeast Kansas Library System
Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>
Comment 21 Marcel de Rooy 2019-11-21 09:03:21 UTC
ReQAing
Comment 22 Marcel de Rooy 2019-11-21 10:36:18 UTC
Created attachment 95651 [details] [review]
Bug 23260: (QA follow-up) Use ->update instead of ->next

Also moves initialization of $anonymous_patron closer to associated comment
lines.
And removes the unneeded changes from Koha::Patrons.
Comment 23 Marcel de Rooy 2019-11-21 10:39:32 UTC
Two things left, after the follow-up:


We need an adjusted dbrev:
 `created_on` timestamp NOT NULL DEFAULT current_timestamp() ON UPDATE current_timestamp(),
That is an updated_on !
So we should remove the ON UPDATE. This made me add the nb_rows comment. A count after the update would be affected by the updated timestamp.

my $before = $now + DateTime::Duration->new( days => 1 );
That does not look good. Use add_days or something like that.
Comment 24 Marcel de Rooy 2019-11-21 10:49:40 UTC
Hmm

sub last_returned_by {
    my ( $self, $borrower ) = @_;

    my $items_last_returned_by_rs = Koha::Database->new()->schema()->resultset('ItemsLastBorrower');


Not so nice in Koha::Item. (Possibly out of scope though)
Comment 25 Marcel de Rooy 2019-11-21 10:50:53 UTC
$items_last_returned_by_rs->update_or_create(
            { borrowernumber => $borrower->borrowernumber, itemnumber => $self->id } );

So it might be better to KEEP the UPDATE clause but RENAME the field.
Comment 26 Agustín Moyano 2019-11-21 17:14:05 UTC
Created attachment 95698 [details] [review]
Bug 23260: Make borrowernumber nullable in items_last_borrower

This patch alters table structure to make borrowernumber nullable

Sponsored-by: Northeast Kansas Library System
Comment 27 Agustín Moyano 2019-11-21 17:14:10 UTC
Created attachment 95699 [details] [review]
Bug 23260: Add anonymize items_last_borrower feature

This patch adds the feature to anonymize patrons from items_last_borrower table. This code is run from batch_anonymize script, which is triggered from a cron job.

To test:
1) apply this patch and the previous one
2) perl installer/data/mysql/updatedatabase.pl
3) set StoreLastBorrower preference to Allow.
4) Create a Check out followed by a Check in.
CHECK => a row should appear in items_last_borrower table with the borrower and the item number.
5) In mysql, update created_on date to one day earlier.
6) set AnonymousPatron preference to a valid patron id
7) perl misc/cronjobs/batch_anonymise.pl
SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron id
8) repeat steps 4 and 5
9) set AnonymousPatron preference to 0
10) perl misc/cronjobs/batch_anonymise.pl
SUCCESS => borrower number in items_last_borrower changed to null
11) prove t/db_dependent/Koha/Patrons.t
12) Sign off

Sponsored-by: Northeast Kansas Library System
Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>
Comment 28 Agustín Moyano 2019-11-21 17:14:15 UTC
Created attachment 95700 [details] [review]
Bug 23260: (follow-up) optimize anonymise_items_last_borrower

Sponsored-by: Northeast Kansas Library System
Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>
Comment 29 Agustín Moyano 2019-11-21 17:14:19 UTC
Created attachment 95701 [details] [review]
Bug 23260: (QA follow-up) Use ->update instead of ->next

Also moves initialization of $anonymous_patron closer to associated comment
lines.
And removes the unneeded changes from Koha::Patrons.
Comment 30 Agustín Moyano 2019-11-21 17:17:35 UTC
(In reply to Marcel de Rooy from comment #23)
> my $before = $now + DateTime::Duration->new( days => 1 );
> That does not look good. Use add_days or something like that.

Hi Marcel, I've updated the test code to use 

my $before = DateTime->now->add( days => 1 );

It certainly looks nicer now.
Comment 31 Agustín Moyano 2019-11-21 17:20:17 UTC
(In reply to Marcel de Rooy from comment #24)
> Hmm
> 
> sub last_returned_by {
>     my ( $self, $borrower ) = @_;
> 
>     my $items_last_returned_by_rs =
> Koha::Database->new()->schema()->resultset('ItemsLastBorrower');
> 
> 
> Not so nice in Koha::Item. (Possibly out of scope though)

This was introduced in Bug 14945. 

Do you want me to change it to use Items::LastPatrons class?
Comment 32 Agustín Moyano 2019-11-21 17:29:07 UTC
(In reply to Marcel de Rooy from comment #25)
> $items_last_returned_by_rs->update_or_create(
>             { borrowernumber => $borrower->borrowernumber, itemnumber =>
> $self->id } );
> 
> So it might be better to KEEP the UPDATE clause but RENAME the field.

I agree that the created_on column should be renamed to updated_on, or maybe we should have both columns, but I believe this should be done in a separate bug.. we have to analyze the impact of such a change

Thanks for QAing!
Comment 33 Tomás Cohen Arazi 2019-11-21 17:38:00 UTC
Marcel, your patch introduces the use of ->update. Given the problems about it raised on bug 21761, could you resubmit it without using ->update? Thanks!
Comment 34 Marcel de Rooy 2019-11-22 07:36:12 UTC
(In reply to Tomás Cohen Arazi from comment #33)
> Marcel, your patch introduces the use of ->update. Given the problems about
> it raised on bug 21761, could you resubmit it without using ->update? Thanks!

Ah, you are right about that ;)
In this case it is a bit theoretical argument, since LastBorrower only contains three fields and has no individual store sub. So replacing the while..next was just regular optimization.
I do not mind setting back the while loop, although personally I would be inclined to wait on the outcome of the discussion before removing all updates.
Comment 35 Marcel de Rooy 2019-11-22 07:37:00 UTC
(In reply to Agustín Moyano from comment #31)
> (In reply to Marcel de Rooy from comment #24)
> > Hmm
> > 
> > sub last_returned_by {
> >     my ( $self, $borrower ) = @_;
> > 
> >     my $items_last_returned_by_rs =
> > Koha::Database->new()->schema()->resultset('ItemsLastBorrower');
> > 
> > 
> > Not so nice in Koha::Item. (Possibly out of scope though)
> 
> This was introduced in Bug 14945. 
> 
> Do you want me to change it to use Items::LastPatrons class?

Another report?
Comment 36 Agustín Moyano 2019-11-22 15:05:28 UTC
(In reply to Marcel de Rooy from comment #35)
> (In reply to Agustín Moyano from comment #31)
> > (In reply to Marcel de Rooy from comment #24)
> > > Hmm
> > > 
> > > sub last_returned_by {
> > >     my ( $self, $borrower ) = @_;
> > > 
> > >     my $items_last_returned_by_rs =
> > > Koha::Database->new()->schema()->resultset('ItemsLastBorrower');
> > > 
> > > 
> > > Not so nice in Koha::Item. (Possibly out of scope though)
> > 
> > This was introduced in Bug 14945. 
> > 
> > Do you want me to change it to use Items::LastPatrons class?
> 
> Another report?

Agreed, I created bug 24092 to address this issue.

Changing this bug to SO again.
Comment 37 Tomás Cohen Arazi 2019-11-22 19:38:39 UTC
(In reply to Marcel de Rooy from comment #34)
> (In reply to Tomás Cohen Arazi from comment #33)
> > Marcel, your patch introduces the use of ->update. Given the problems about
> > it raised on bug 21761, could you resubmit it without using ->update? Thanks!
> 
> Ah, you are right about that ;)
> In this case it is a bit theoretical argument, since LastBorrower only
> contains three fields and has no individual store sub. So replacing the
> while..next was just regular optimization.
> I do not mind setting back the while loop, although personally I would be
> inclined to wait on the outcome of the discussion before removing all
> updates.

Fair enough.
Comment 38 Marcel de Rooy 2019-11-25 08:57:31 UTC
(In reply to Agustín Moyano from comment #32)
> I agree that the created_on column should be renamed to updated_on, or maybe
> we should have both columns, but I believe this should be done in a separate
> bug.. we have to analyze the impact of such a change

OK Separate bug. Please open it too.
Comment 39 Marcel de Rooy 2019-11-25 09:05:17 UTC
Created attachment 95769 [details] [review]
Bug 23260: Make borrowernumber nullable in items_last_borrower

This patch alters table structure to make borrowernumber nullable

Sponsored-by: Northeast Kansas Library System

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 40 Marcel de Rooy 2019-11-25 09:05:22 UTC
Created attachment 95770 [details] [review]
Bug 23260: Add anonymize items_last_borrower feature

This patch adds the feature to anonymize patrons from items_last_borrower table. This code is run from batch_anonymize script, which is triggered from a cron job.

To test:
1) apply this patch and the previous one
2) perl installer/data/mysql/updatedatabase.pl
3) set StoreLastBorrower preference to Allow.
4) Create a Check out followed by a Check in.
CHECK => a row should appear in items_last_borrower table with the borrower and the item number.
5) In mysql, update created_on date to one day earlier.
6) set AnonymousPatron preference to a valid patron id
7) perl misc/cronjobs/batch_anonymise.pl
SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron id
8) repeat steps 4 and 5
9) set AnonymousPatron preference to 0
10) perl misc/cronjobs/batch_anonymise.pl
SUCCESS => borrower number in items_last_borrower changed to null
11) prove t/db_dependent/Koha/Patrons.t
12) Sign off

Sponsored-by: Northeast Kansas Library System
Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 41 Marcel de Rooy 2019-11-25 09:05:28 UTC
Created attachment 95771 [details] [review]
Bug 23260: (follow-up) optimize anonymise_items_last_borrower

Sponsored-by: Northeast Kansas Library System
Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 42 Marcel de Rooy 2019-11-25 09:05:33 UTC
Created attachment 95772 [details] [review]
Bug 23260: (QA follow-up) Use ->update instead of ->next

Also moves initialization of $anonymous_patron closer to associated comment
lines.
And removes the unneeded changes from Koha::Patrons.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Please see comment34 on use of update method.
Comment 43 Jonathan Druart 2019-12-18 13:52:11 UTC
Why $class->_resultset->search instead of $self->search?
Comment 44 Jonathan Druart 2019-12-18 13:55:57 UTC
Also, I find Koha::Item::LastPatrons->anonymise_items_last_borrower quite hard to read (its name, not its content) and guess what is suppose to do the method.
Comment 45 Jonathan Druart 2019-12-18 14:00:51 UTC
Basically I do not think this table deserves its own modules. We could handle that from Koha::Patron I'd say. I guess it's late now as it's PQA. Letting RM decide.
Comment 46 Agustín Moyano 2019-12-18 14:39:51 UTC
(In reply to Jonathan Druart from comment #43)
> Why $class->_resultset->search instead of $self->search?

I believe that's a question only you can reply (bug 16966 - https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=60705&action=diff#a/Koha/Patrons.pm_sec1) ;P

on a firt attempt, I copied what search_patron_to_anonymize does, but then in follow-up removed it from Patrons and placed it in LastPatrons without $class->_resultset->search.

Cheers.
Comment 47 Jonathan Druart 2019-12-19 07:29:45 UTC
(In reply to Agustín Moyano from comment #46)
> (In reply to Jonathan Druart from comment #43)
> > Why $class->_resultset->search instead of $self->search?
> 
> I believe that's a question only you can reply (bug 16966 -
> https://bugs.koha-community.org/bugzilla3/attachment.
> cgi?id=60705&action=diff#a/Koha/Patrons.pm_sec1) ;P
> 
> on a firt attempt, I copied what search_patron_to_anonymize does, but then
> in follow-up removed it from Patrons and placed it in LastPatrons without
> $class->_resultset->search.


Heh ok, fair enough. I do not think it's correct (not incorrect either). We should return $class->search(), as you did then!

I am still not happy with that new module. I do not think it really makes sense.
Comment 48 Jonathan Druart 2019-12-19 07:37:39 UTC
FYI On bug 24152 I provide a flexible way to remove Koha::Object-based objects depending on date range.
Comment 49 Martin Renvoize 2020-01-03 11:01:55 UTC Comment hidden (obsolete)
Comment 50 Martin Renvoize 2020-01-03 11:07:11 UTC
OK, this isn't pushed.. my mistake on updating the bug prematurely.. had too many tabs open..

Moving onto this one now and having a read as that recent comments make it sound contentious.
Comment 51 Martin Renvoize 2020-01-03 11:53:51 UTC
error: sha1 information is lacking or useless (Koha/Patrons.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 23260: (follow-up) optimize anonymise_items_last_borrower
Comment 52 Martin Renvoize 2020-01-03 12:11:08 UTC
OK, the patch doesn't apply currently, SHA1 errors.

However, I think I agree with Jonathan here, I'm not sure there's a requirement at all for introducing a Koha::Item::LastPatron object.. we have a nice method in Koha::Item for returning a Koha::Patron object for the last_borrower.  I think we can simply inline the search and removal here rather than adding a new class.

Finally.. as for the 'update' debate.. personally I think we should ban update calls as a direct passthrough in Koha::Objects but allow the manual "I know what I'm doing" reach inside _resultset version.

So, I would encourage _resultset->update() as the format for that particular case.
Comment 53 Martin Renvoize 2020-01-03 12:38:08 UTC
Created attachment 96791 [details] [review]
Bug 23260: [ALTERNATIVE] Inline code to anonymise_issue_history

This alternative patch reduces the complexity of the change by simply
updateing the existing anonymise_issue_history method to include
anonymisation of the items_last_borrowers table.
Comment 54 Martin Renvoize 2020-01-03 12:41:56 UTC
OK, I've gone into 'golf mode' and submitted an alternative patch which I feel accomplishes the same result but in a significantly reduced amount of clear code.

I'd still be interested in a squashed patch for the original 4 commit submission as I struggled to fully asses it given I could not apply... but I'd also be very interested in peoples opinions regarding my alternative approach.. (It would obviously fail qa as it stands as it requires some additional tests adding to cover the change)
Comment 55 Jonathan Druart 2020-01-06 21:48:54 UTC
Martin, your alternative approach looks good to me!
Comment 56 Tomás Cohen Arazi 2020-01-06 22:11:50 UTC
I talked to Agustín this weekend and he was surprised as he was sure he implemented what Jonathan requested. I agree he should've squashed the patches, though. He was sure he removed the introduced classes 

I like the alternative, but would prefer to hear from Agustin first when he's back and maybe he can adjust the details.

Also, we should find some criteria for when doing what we do everywhere is ok, and when improving the codebase is a requirement.
Comment 57 Agustín Moyano 2020-01-23 20:49:38 UTC
Created attachment 97841 [details] [review]
Bug 23260: Make borrowernumber nullable in items_last_borrower

This patch alters table structure to make borrowernumber nullable

Sponsored-by: Northeast Kansas Library System

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 58 Agustín Moyano 2020-01-23 20:49:43 UTC
Created attachment 97842 [details] [review]
Bug 23260: Inline code to anonymise_issue_history

This alternative patch reduces the complexity of the change by simply
updateing the existing anonymise_issue_history method to include
anonymisation of the items_last_borrowers table.

To test:
1) apply this patch and the previous one
2) perl installer/data/mysql/updatedatabase.pl
3) set StoreLastBorrower preference to Allow.
4) set AnonymousPatron preference to a valid patron id
5) Create a Check out followed by a Check in.
CHECK => a row should appear in items_last_borrower table with the borrower and the item number.
6) In mysql, update created_on of items_last_borrower and returndate of old_issues to two days earlier.
7) perl misc/cronjobs/batch_anonymise.pl --days 1
SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron id
8) repeat steps 5 and 6
9) set AnonymousPatron preference to 0
10) perl misc/cronjobs/batch_anonymise.pl --days 1
SUCCESS => borrower number in items_last_borrower changed to null
11) prove t/db_dependent/Koha/Patrons.t
12) Sign off
Comment 59 Marcel de Rooy 2020-01-24 06:41:21 UTC
(In reply to Agustín Moyano from comment #58)
> Created attachment 97842 [details] [review] [review]
> Bug 23260: Inline code to anonymise_issue_history
> 
> This alternative patch reduces the complexity of the change by simply
> updateing the existing anonymise_issue_history method to include
> anonymisation of the items_last_borrowers table.
> 
> To test:
> 1) apply this patch and the previous one
> 2) perl installer/data/mysql/updatedatabase.pl
> 3) set StoreLastBorrower preference to Allow.
> 4) set AnonymousPatron preference to a valid patron id
> 5) Create a Check out followed by a Check in.
> CHECK => a row should appear in items_last_borrower table with the borrower
> and the item number.
> 6) In mysql, update created_on of items_last_borrower and returndate of
> old_issues to two days earlier.
> 7) perl misc/cronjobs/batch_anonymise.pl --days 1
> SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron
> id
> 8) repeat steps 5 and 6
> 9) set AnonymousPatron preference to 0
> 10) perl misc/cronjobs/batch_anonymise.pl --days 1
> SUCCESS => borrower number in items_last_borrower changed to null
> 11) prove t/db_dependent/Koha/Patrons.t
> 12) Sign off

Agustin,
If you agree with this patch, please test and sign off.
I could do the last step of QA.
Comment 60 Agustín Moyano 2020-01-28 17:54:32 UTC
Created attachment 98044 [details] [review]
Bug 23260: Inline code to anonymise_issue_history

This alternative patch reduces the complexity of the change by simply
updateing the existing anonymise_issue_history method to include
anonymisation of the items_last_borrowers table.

To test:
1) apply this patch and the previous one
2) perl installer/data/mysql/updatedatabase.pl
3) set StoreLastBorrower preference to Allow.
4) set AnonymousPatron preference to a valid patron id
5) Create a Check out followed by a Check in.
CHECK => a row should appear in items_last_borrower table with the borrower and the item number.
6) In mysql, update created_on of items_last_borrower and returndate of old_issues to two days earlier.
7) perl misc/cronjobs/batch_anonymise.pl --days 1
SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron id
8) repeat steps 5 and 6
9) set AnonymousPatron preference to 0
10) perl misc/cronjobs/batch_anonymise.pl --days 1
SUCCESS => borrower number in items_last_borrower changed to null
11) prove t/db_dependent/Koha/Patrons.t
12) Sign off

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 61 Jonathan Druart 2020-01-29 09:07:24 UTC
Created attachment 98053 [details] [review]
Bug 23260: Simplify tests

Use build_sample_item and build_object
Comment 62 Jonathan Druart 2020-01-29 09:07:28 UTC
Created attachment 98054 [details] [review]
Bug 23260: Add a new test
Comment 63 Jonathan Druart 2020-01-29 09:08:52 UTC
Almost done but I think I found an issue, the method does no longer return the number of issues modified.
I do not think we should sum the number of items_last_borrower updated.
Comment 64 Martin Renvoize 2020-01-29 12:04:20 UTC
(In reply to Jonathan Druart from comment #63)
> Almost done but I think I found an issue, the method does no longer return
> the number of issues modified.
> I do not think we should sum the number of items_last_borrower updated.

I think your right.. looks like an oversight on my part when I first submitted the alternative approach.
Comment 65 Martin Renvoize 2020-01-29 12:11:51 UTC
Created attachment 98064 [details] [review]
Bug 23260: (follow-up) Remove last_borrower updates from issues count
Comment 66 Martin Renvoize 2020-02-03 11:25:58 UTC
Seems I forgot to reset the status when I added the followup.. ready for QA
Comment 67 Jonathan Druart 2020-02-03 12:01:58 UTC
Created attachment 98282 [details] [review]
Bug 23260: Add a new test
Comment 68 Jonathan Druart 2020-02-03 12:02:57 UTC
(In reply to Martin Renvoize from comment #66)
> Seems I forgot to reset the status when I added the followup.. ready for QA

The test was not passing. I have updated it, as I think we are expecting 1 checkout to be anonymised here. Still not passing.
Comment 69 Jonathan Druart 2020-02-03 16:22:18 UTC
Created attachment 98349 [details] [review]
Bug 23260: Add a new test
Comment 70 Jonathan Druart 2020-02-03 16:22:58 UTC
(In reply to Jonathan Druart from comment #68)
> (In reply to Martin Renvoize from comment #66)
> > Seems I forgot to reset the status when I added the followup.. ready for QA
> 
> The test was not passing. I have updated it, as I think we are expecting 1
> checkout to be anonymised here. Still not passing.

Forget that, I misread the code. We want 4 checkouts to be anonymised.
Comment 71 Martin Renvoize 2020-02-03 16:32:17 UTC
I see!!!!

Right.. I totally missed that `but not if the item is lost, damaged, or withdrawn.` in the original request is different to how the existing anonymisation logic works.

Personally (and I've checked with a few people now) I feel that the two locations this data is stored should be anonymised equally so they don't fall out of sync.

As this is a change in a privacy feature I think we probably need to govern whether we filter out by item status based on a system preference to keep everyone happy.

I'll add that as a followup now and update the test to correspond.  Good catch Jonathan!
Comment 72 Martin Renvoize 2020-02-03 16:33:53 UTC
Setting to Failed QA whilst I code the followup
Comment 73 Martin Renvoize 2020-02-03 17:05:02 UTC
Created attachment 98352 [details] [review]
Bug 23260: Proposed preference driven filtering
Comment 74 Katrin Fischer 2020-02-04 07:36:18 UTC
When the option to store the last patron was introduced separately from the normal reading history feature, I was quite happy as it would allow us to keep that information, without necessarily keeping all the history. 

What libraries often ask for is to know the last person that checked out an item, in case they have missed damage on return (like it can happen in times of self checks). 

In my opinion, by synching those with no option to not delete both at the same time - we make the StoreLastPatron obsolete. We could always have used the other tables to retrieve the information, but it was added with a new table as a separate thing with the reasoning of avoiding automatic anonymization.

See the bug description from the initial feature add (bug 14945):

"Currently if the AnonymousPatron system preference is in use, all patron data is anonymized. Some libraries would like to be able to see the last patron who
returned out an item ( in case of damage ) but still keep all other patrons anonymized."
Comment 75 Katrin Fischer 2020-02-04 07:38:15 UTC
I guess what I was trying to say is that this was designed to fall out of sync with the other tables - so we should not break that use case and allow for separate treatment of both. Added see also the bugs with former discussion.
Comment 76 Martin Renvoize 2020-02-04 09:01:08 UTC
OK.. at this point I'm totally confused as to why/how this dev request came about...

The items_last_borrower table is only populated if you enable the `StoreLastBorrower` system preference and the original idea of that preference was to allow staff to keep a record of last_borrower separately from the checkout history, which may be anonymized, so that staff can refer back to who was the last borrower if they find a book is damaged and as such that can associate that damage with a patron.

As such, I'm not sure I understand at all the reasoning behind wanting to remove it at the same time as the rest of the anonymisation and also why you would want to filter on the given item states, given that the idea of the original feature is that you're not likely to have set such a status until after in this case you would have deleted it.

Is this actually just a misunderstanding of the original feature in question I wonder?
Comment 77 George Williams (NEKLS) 2020-02-04 21:42:47 UTC
NEKLS is sponsoring the development of this cronjob.

Currently we have StoreLastBorrower turned on because we want the last borrower information stored if patron privacy is anonymized.  We also have a cron running that prunes old_issues more than 400 days old so that all patron data is effectively anonymized after about 13 months.  But we are not pruning items_last_borrower because staff at many of our libraries want the last borrower information saved on the item record if the item was lost.

This means that we are currently anonymizing patron data in old_issues after it's 400 days old and never anonymizing that same data in items_last_borrower for the sake of the libraries that want that data saved for lost items.

So for us the point of this cronjob is to prune items_last_borrower once the data is more than 400 days old which is in line with the other pruning we're already doing but it will make an exception if the item has a lost status.
Comment 78 Katrin Fischer 2020-02-04 21:58:36 UTC
Hi George, 

I think an option to clean it is not in question, I just think we should not tie it to the same 'switch' - so libraries can make the decision to delete both tables after a different number of days.

This change as proposed currently would not only affect the cronjob. The method changed is used in different areas:

- batch_anonymise.pl cronjob
- When using the batch anonymize feature tool in the staff interface
- When a patron decides to delete their reading history from the OPAC (OpacPrivacy etc.)

So if I know I just returned a damaged book via the self check... I could delete my reading history from the OPAC. Info might still be retrievable somewhere, but I feel StoreLastPatron should be covering that use case.
Comment 79 Martin Renvoize 2020-02-05 17:52:24 UTC
I've run out of steam for this at this point I'm afraid.  We've gone nearly full circle now.. to me this highlights the need for good RFC's as the first step.

I'll try to revisit unless someone takes it back over.
Comment 80 Agustín Moyano 2020-02-05 23:16:37 UTC
(In reply to Martin Renvoize from comment #79)
> I've run out of steam for this at this point I'm afraid.  We've gone nearly
> full circle now.. to me this highlights the need for good RFC's as the first
> step.
> 
> I'll try to revisit unless someone takes it back over.

I'll take ir again as soon as I can
Comment 81 Agustín Moyano 2020-10-30 18:29:26 UTC
Created attachment 112734 [details] [review]
Bug 23260: Make borrowernumber nullable in items_last_borrower

This patch alters table structure to make borrowernumber nullable

Sponsored-by: Northeast Kansas Library System

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 82 Agustín Moyano 2020-10-30 18:29:35 UTC
Created attachment 112735 [details] [review]
Bug 23260: Inline code to anonymise_issue_history

This alternative patch reduces the complexity of the change by simply
updateing the existing anonymise_issue_history method to include
anonymisation of the items_last_borrowers table.

To test:
1) apply this patch and the previous one
2) perl installer/data/mysql/updatedatabase.pl
3) set StoreLastBorrower preference to Allow.
4) set AnonymousPatron preference to a valid patron id
5) Create a Check out followed by a Check in.
CHECK => a row should appear in items_last_borrower table with the borrower and the item number.
6) In mysql, update created_on of items_last_borrower and returndate of old_issues to two days earlier.
7) perl misc/cronjobs/batch_anonymise.pl --days 1
SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron id
8) repeat steps 5 and 6
9) set AnonymousPatron preference to 0
10) perl misc/cronjobs/batch_anonymise.pl --days 1
SUCCESS => borrower number in items_last_borrower changed to null
11) prove t/db_dependent/Koha/Patrons.t
12) Sign off

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 83 Agustín Moyano 2020-10-30 18:29:41 UTC
Created attachment 112736 [details] [review]
Bug 23260: Simplify tests

Use build_sample_item and build_object
Comment 84 Agustín Moyano 2020-10-30 18:29:48 UTC
Created attachment 112737 [details] [review]
Bug 23260: (follow-up) Remove last_borrower updates from issues count
Comment 85 Agustín Moyano 2020-10-30 18:29:54 UTC
Created attachment 112738 [details] [review]
Bug 23260: Add a new test
Comment 86 Agustín Moyano 2020-10-30 18:30:00 UTC
Created attachment 112739 [details] [review]
Bug 23260: Proposed preference driven filtering
Comment 87 Agustín Moyano 2020-10-30 18:30:05 UTC
Created attachment 112740 [details] [review]
Bug 23260: (follow-up) Add AnonymiseLastBorrower and AnonymiseLastBorrowerDays sysprefs
Comment 88 Agustín Moyano 2020-10-30 18:30:15 UTC
Created attachment 112741 [details] [review]
Bug 23260: (follow-up) Update test
Comment 89 Agustín Moyano 2020-10-30 18:30:24 UTC
Created attachment 112742 [details] [review]
Bug 23260: (follow-up) Add new cron job
Comment 90 Agustín Moyano 2020-10-30 18:30:31 UTC
Created attachment 112743 [details] [review]
Bug 23260: (follow-up) Add anonymise_last_borrowers method to Patrons.pm

This patch adds the method anonymise_last_borrowers in Patrons.pm.

To test:
1) apply this patch and the previous ones
2) perl installer/data/mysql/updatedatabase.pl
3) set StoreLastBorrower preference to Allow.
4) set AnonymousPatron preference to a valid patron id
5) Create a Check out followed by a Check in.
CHECK => a row should appear in items_last_borrower table with the borrower and the item number.
6) In mysql, update created_on of items_last_borrower and returndate of old_issues to two days earlier.
7) perl misc/cronjobs/anonymise_last_borrowers.pl
CHECK => borrower number in items_last_borrower is not anonymised yet.
8) Repeat step 5 and 6 but instead of two days earlier, set the new entry in items_last_borrower to three days eralier.
9) set AnonymiseLastBorrower to 'Anonymise' and AnonymiseLastBorrowerDays to 2 days.
10) perl misc/cronjobs/anonymise_last_borrowers.pl
SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron id
11) repeat step 8
12) set AnonymousPatron preference to 0
13) perl misc/cronjobs/anonymise_last_borrowers.pl
SUCCESS => borrower number in items_last_borrower changed to null
14) prove t/db_dependent/Koha/Patrons.t
Comment 91 Agustín Moyano 2020-10-30 18:56:44 UTC
(In reply to Katrin Fischer from comment #78)
> Hi George, 
> 
> I think an option to clean it is not in question, I just think we should not
> tie it to the same 'switch' - so libraries can make the decision to delete
> both tables after a different number of days.
> 
> This change as proposed currently would not only affect the cronjob. The
> method changed is used in different areas:
> 
> - batch_anonymise.pl cronjob
> - When using the batch anonymize feature tool in the staff interface
> - When a patron decides to delete their reading history from the OPAC
> (OpacPrivacy etc.)
> 
> So if I know I just returned a damaged book via the self check... I could
> delete my reading history from the OPAC. Info might still be retrievable
> somewhere, but I feel StoreLastPatron should be covering that use case.

Hi Katrin, I've added a series of follow-ups that separates anonymization of checkouts from item's last borrower.

I added a new cron job just for this, that's controlled by two sysprefs.. one to say if anonymizing item's last borrower is allowed and another to set how old it must be before it get's anonymized.

Hope this is the one!
Comment 92 Nick Clemens 2021-03-09 14:42:19 UTC
Created attachment 117974 [details] [review]
Bug 23260: Make borrowernumber nullable in items_last_borrower

This patch alters table structure to make borrowernumber nullable

Sponsored-by: Northeast Kansas Library System

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 93 Nick Clemens 2021-03-09 14:42:22 UTC
Created attachment 117975 [details] [review]
Bug 23260: Inline code to anonymise_issue_history

This alternative patch reduces the complexity of the change by simply
updateing the existing anonymise_issue_history method to include
anonymisation of the items_last_borrowers table.

To test:
1) apply this patch and the previous one
2) perl installer/data/mysql/updatedatabase.pl
3) set StoreLastBorrower preference to Allow.
4) set AnonymousPatron preference to a valid patron id
5) Create a Check out followed by a Check in.
CHECK => a row should appear in items_last_borrower table with the borrower and the item number.
6) In mysql, update created_on of items_last_borrower and returndate of old_issues to two days earlier.
7) perl misc/cronjobs/batch_anonymise.pl --days 1
SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron id
8) repeat steps 5 and 6
9) set AnonymousPatron preference to 0
10) perl misc/cronjobs/batch_anonymise.pl --days 1
SUCCESS => borrower number in items_last_borrower changed to null
11) prove t/db_dependent/Koha/Patrons.t
12) Sign off

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 94 Nick Clemens 2021-03-09 14:42:26 UTC
Created attachment 117976 [details] [review]
Bug 23260: Simplify tests

Use build_sample_item and build_object
Comment 95 Nick Clemens 2021-03-09 14:42:30 UTC
Created attachment 117977 [details] [review]
Bug 23260: (follow-up) Remove last_borrower updates from issues count
Comment 96 Nick Clemens 2021-03-09 14:42:34 UTC
Created attachment 117978 [details] [review]
Bug 23260: Add a new test
Comment 97 Nick Clemens 2021-03-09 14:42:38 UTC
Created attachment 117979 [details] [review]
Bug 23260: Proposed preference driven filtering
Comment 98 Nick Clemens 2021-03-09 14:42:43 UTC
Created attachment 117980 [details] [review]
Bug 23260: (follow-up) Add AnonymiseLastBorrower and AnonymiseLastBorrowerDays sysprefs
Comment 99 Nick Clemens 2021-03-09 14:42:47 UTC
Created attachment 117981 [details] [review]
Bug 23260: (follow-up) Update test
Comment 100 Nick Clemens 2021-03-09 14:42:51 UTC
Created attachment 117982 [details] [review]
Bug 23260: (follow-up) Add new cron job
Comment 101 Nick Clemens 2021-03-09 14:42:55 UTC
Created attachment 117983 [details] [review]
Bug 23260: (follow-up) Add anonymise_last_borrowers method to Patrons.pm

This patch adds the method anonymise_last_borrowers in Patrons.pm.

To test:
1) apply this patch and the previous ones
2) perl installer/data/mysql/updatedatabase.pl
3) set StoreLastBorrower preference to Allow.
4) set AnonymousPatron preference to a valid patron id
5) Create a Check out followed by a Check in.
CHECK => a row should appear in items_last_borrower table with the borrower and the item number.
6) In mysql, update created_on of items_last_borrower and returndate of old_issues to two days earlier.
7) perl misc/cronjobs/anonymise_last_borrowers.pl
CHECK => borrower number in items_last_borrower is not anonymised yet.
8) Repeat step 5 and 6 but instead of two days earlier, set the new entry in items_last_borrower to three days eralier.
9) set AnonymiseLastBorrower to 'Anonymise' and AnonymiseLastBorrowerDays to 2 days.
10) perl misc/cronjobs/anonymise_last_borrowers.pl
SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron id
11) repeat step 8
12) set AnonymousPatron preference to 0
13) perl misc/cronjobs/anonymise_last_borrowers.pl
SUCCESS => borrower number in items_last_borrower changed to null
14) prove t/db_dependent/Koha/Patrons.t
Comment 102 Katrin Fischer 2021-05-02 16:21:57 UTC
I am sorry, this patch set no longer applies and I can't fix it:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 23260: Make borrowernumber nullable in items_last_borrower
Applying: Bug 23260: Inline code to anonymise_issue_history
Applying: Bug 23260: Simplify tests
Applying: Bug 23260: (follow-up) Remove last_borrower updates from issues count
Applying: Bug 23260: Add a new test
Applying: Bug 23260: Proposed preference driven filtering
Applying: Bug 23260: (follow-up) Add AnonymiseLastBorrower and AnonymiseLastBorrowerDays sysprefs
Applying: Bug 23260: (follow-up) Update test
error: sha1 information is lacking or useless (t/db_dependent/Koha/Patrons.t).
error: could not build fake ancestor
Patch failed at 0001 Bug 23260: (follow-up) Update test
The copy of the patch that failed is found in: .git/rebase-apply/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-23260-follow-up-Update-test-oyzp2x.patch
Comment 103 Nick Clemens 2021-05-05 14:24:41 UTC
Created attachment 120522 [details] [review]
Bug 23260: Make borrowernumber nullable in items_last_borrower

This patch alters table structure to make borrowernumber nullable

Sponsored-by: Northeast Kansas Library System

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 104 Nick Clemens 2021-05-05 14:24:46 UTC
Created attachment 120523 [details] [review]
Bug 23260: Inline code to anonymise_issue_history

This alternative patch reduces the complexity of the change by simply
updateing the existing anonymise_issue_history method to include
anonymisation of the items_last_borrowers table.

To test:
1) apply this patch and the previous one
2) perl installer/data/mysql/updatedatabase.pl
3) set StoreLastBorrower preference to Allow.
4) set AnonymousPatron preference to a valid patron id
5) Create a Check out followed by a Check in.
CHECK => a row should appear in items_last_borrower table with the borrower and the item number.
6) In mysql, update created_on of items_last_borrower and returndate of old_issues to two days earlier.
7) perl misc/cronjobs/batch_anonymise.pl --days 1
SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron id
8) repeat steps 5 and 6
9) set AnonymousPatron preference to 0
10) perl misc/cronjobs/batch_anonymise.pl --days 1
SUCCESS => borrower number in items_last_borrower changed to null
11) prove t/db_dependent/Koha/Patrons.t
12) Sign off

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 105 Nick Clemens 2021-05-05 14:24:51 UTC
Created attachment 120524 [details] [review]
Bug 23260: Simplify tests

Use build_sample_item and build_object
Comment 106 Nick Clemens 2021-05-05 14:24:58 UTC
Created attachment 120525 [details] [review]
Bug 23260: (follow-up) Remove last_borrower updates from issues count
Comment 107 Nick Clemens 2021-05-05 14:25:05 UTC
Created attachment 120526 [details] [review]
Bug 23260: Add a new test
Comment 108 Nick Clemens 2021-05-05 14:25:10 UTC
Created attachment 120527 [details] [review]
Bug 23260: Proposed preference driven filtering
Comment 109 Nick Clemens 2021-05-05 14:25:16 UTC
Created attachment 120528 [details] [review]
Bug 23260: (follow-up) Add AnonymiseLastBorrower and AnonymiseLastBorrowerDays sysprefs
Comment 110 Nick Clemens 2021-05-05 14:25:21 UTC
Created attachment 120529 [details] [review]
Bug 23260: (follow-up) Update test
Comment 111 Nick Clemens 2021-05-05 14:25:27 UTC
Created attachment 120530 [details] [review]
Bug 23260: (follow-up) Add new cron job
Comment 112 Nick Clemens 2021-05-05 14:25:32 UTC
Created attachment 120531 [details] [review]
Bug 23260: (follow-up) Add anonymise_last_borrowers method to Patrons.pm

This patch adds the method anonymise_last_borrowers in Patrons.pm.

To test:
1) apply this patch and the previous ones
2) perl installer/data/mysql/updatedatabase.pl
3) set StoreLastBorrower preference to Allow.
4) set AnonymousPatron preference to a valid patron id
5) Create a Check out followed by a Check in.
CHECK => a row should appear in items_last_borrower table with the borrower and the item number.
6) In mysql, update created_on of items_last_borrower and returndate of old_issues to two days earlier.
7) perl misc/cronjobs/anonymise_last_borrowers.pl
CHECK => borrower number in items_last_borrower is not anonymised yet.
8) Repeat step 5 and 6 but instead of two days earlier, set the new entry in items_last_borrower to three days eralier.
9) set AnonymiseLastBorrower to 'Anonymise' and AnonymiseLastBorrowerDays to 2 days.
10) perl misc/cronjobs/anonymise_last_borrowers.pl
SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron id
11) repeat step 8
12) set AnonymousPatron preference to 0
13) perl misc/cronjobs/anonymise_last_borrowers.pl
SUCCESS => borrower number in items_last_borrower changed to null
14) prove t/db_dependent/Koha/Patrons.t
Comment 113 Andrew Fuerste-Henry 2021-06-10 12:35:46 UTC
I'm getting a test failure at the end:
root@kohadevbox:koha(master)$ prove t/db_dependent/Koha/Patrons.t
t/db_dependent/Koha/Patrons.t .. 42/42 # Looks like you planned 42 tests but ran 43.
t/db_dependent/Koha/Patrons.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
All 42 subtests passed 

Test Summary Report
-------------------
t/db_dependent/Koha/Patrons.t (Wstat: 65280 Tests: 43 Failed: 1)
  Failed test:  43
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 42 tests but ran 43.
Files=1, Tests=43, 31 wallclock secs ( 0.08 usr  0.01 sys + 23.30 cusr  5.08 csys = 28.47 CPU)
Result: FAIL
Comment 114 Nick Clemens 2021-07-20 12:42:21 UTC
Created attachment 122963 [details] [review]
Bug 23260: Make borrowernumber nullable in items_last_borrower

This patch alters table structure to make borrowernumber nullable

Sponsored-by: Northeast Kansas Library System

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 115 Nick Clemens 2021-07-20 12:42:25 UTC
Created attachment 122964 [details] [review]
Bug 23260: Inline code to anonymise_issue_history

This alternative patch reduces the complexity of the change by simply
updateing the existing anonymise_issue_history method to include
anonymisation of the items_last_borrowers table.

To test:
1) apply this patch and the previous one
2) perl installer/data/mysql/updatedatabase.pl
3) set StoreLastBorrower preference to Allow.
4) set AnonymousPatron preference to a valid patron id
5) Create a Check out followed by a Check in.
CHECK => a row should appear in items_last_borrower table with the borrower and the item number.
6) In mysql, update created_on of items_last_borrower and returndate of old_issues to two days earlier.
7) perl misc/cronjobs/batch_anonymise.pl --days 1
SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron id
8) repeat steps 5 and 6
9) set AnonymousPatron preference to 0
10) perl misc/cronjobs/batch_anonymise.pl --days 1
SUCCESS => borrower number in items_last_borrower changed to null
11) prove t/db_dependent/Koha/Patrons.t
12) Sign off

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 116 Nick Clemens 2021-07-20 12:42:30 UTC
Created attachment 122965 [details] [review]
Bug 23260: Simplify tests

Use build_sample_item and build_object
Comment 117 Nick Clemens 2021-07-20 12:42:34 UTC
Created attachment 122966 [details] [review]
Bug 23260: (follow-up) Remove last_borrower updates from issues count
Comment 118 Nick Clemens 2021-07-20 12:42:39 UTC
Created attachment 122967 [details] [review]
Bug 23260: Add a new test
Comment 119 Nick Clemens 2021-07-20 12:42:43 UTC
Created attachment 122968 [details] [review]
Bug 23260: Proposed preference driven filtering
Comment 120 Nick Clemens 2021-07-20 12:42:48 UTC
Created attachment 122969 [details] [review]
Bug 23260: (follow-up) Add AnonymiseLastBorrower and AnonymiseLastBorrowerDays sysprefs
Comment 121 Nick Clemens 2021-07-20 12:42:53 UTC
Created attachment 122970 [details] [review]
Bug 23260: (follow-up) Update test

Update test count
Comment 122 Nick Clemens 2021-07-20 12:42:59 UTC
Created attachment 122971 [details] [review]
Bug 23260: (follow-up) Add new cron job
Comment 123 Nick Clemens 2021-07-20 12:43:04 UTC
Created attachment 122972 [details] [review]
Bug 23260: (follow-up) Add anonymise_last_borrowers method to Patrons.pm

This patch adds the method anonymise_last_borrowers in Patrons.pm.

To test:
1) apply this patch and the previous ones
2) perl installer/data/mysql/updatedatabase.pl
3) set StoreLastBorrower preference to Allow.
4) set AnonymousPatron preference to a valid patron id
5) Create a Check out followed by a Check in.
CHECK => a row should appear in items_last_borrower table with the borrower and the item number.
6) In mysql, update created_on of items_last_borrower and returndate of old_issues to two days earlier.
7) perl misc/cronjobs/anonymise_last_borrowers.pl
CHECK => borrower number in items_last_borrower is not anonymised yet.
8) Repeat step 5 and 6 but instead of two days earlier, set the new entry in items_last_borrower to three days eralier.
9) set AnonymiseLastBorrower to 'Anonymise' and AnonymiseLastBorrowerDays to 2 days.
10) perl misc/cronjobs/anonymise_last_borrowers.pl
SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron id
11) repeat step 8
12) set AnonymousPatron preference to 0
13) perl misc/cronjobs/anonymise_last_borrowers.pl
SUCCESS => borrower number in items_last_borrower changed to null
14) prove t/db_dependent/Koha/Patrons.t
Comment 124 Nick Clemens 2023-01-10 13:51:14 UTC
Created attachment 145175 [details] [review]
Bug 23260: Make borrowernumber nullable in items_last_borrower

This patch alters table structure to make borrowernumber nullable

Sponsored-by: Northeast Kansas Library System

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 125 Nick Clemens 2023-01-10 13:51:21 UTC
Created attachment 145176 [details] [review]
Bug 23260: Inline code to anonymise_issue_history

This alternative patch reduces the complexity of the change by simply
updateing the existing anonymise_issue_history method to include
anonymisation of the items_last_borrowers table.

To test:
1) apply this patch and the previous one
2) perl installer/data/mysql/updatedatabase.pl
3) set StoreLastBorrower preference to Allow.
4) set AnonymousPatron preference to a valid patron id
5) Create a Check out followed by a Check in.
CHECK => a row should appear in items_last_borrower table with the borrower and the item number.
6) In mysql, update created_on of items_last_borrower and returndate of old_issues to two days earlier.
7) perl misc/cronjobs/batch_anonymise.pl --days 1
SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron id
8) repeat steps 5 and 6
9) set AnonymousPatron preference to 0
10) perl misc/cronjobs/batch_anonymise.pl --days 1
SUCCESS => borrower number in items_last_borrower changed to null
11) prove t/db_dependent/Koha/Patrons.t
12) Sign off

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 126 Nick Clemens 2023-01-10 13:51:27 UTC
Created attachment 145177 [details] [review]
Bug 23260: Simplify tests

Use build_sample_item and build_object
Comment 127 Nick Clemens 2023-01-10 13:51:33 UTC
Created attachment 145178 [details] [review]
Bug 23260: (follow-up) Remove last_borrower updates from issues count
Comment 128 Nick Clemens 2023-01-10 13:51:40 UTC
Created attachment 145179 [details] [review]
Bug 23260: Add a new test
Comment 129 Nick Clemens 2023-01-10 13:51:46 UTC
Created attachment 145180 [details] [review]
Bug 23260: Proposed preference driven filtering
Comment 130 Nick Clemens 2023-01-10 13:51:52 UTC
Created attachment 145181 [details] [review]
Bug 23260: (follow-up) Add AnonymiseLastBorrower and AnonymiseLastBorrowerDays sysprefs
Comment 131 Nick Clemens 2023-01-10 13:51:59 UTC
Created attachment 145182 [details] [review]
Bug 23260: (follow-up) Update test

Update test count
Comment 132 Nick Clemens 2023-01-10 13:52:06 UTC
Created attachment 145183 [details] [review]
Bug 23260: (follow-up) Add new cron job
Comment 133 Nick Clemens 2023-01-10 13:52:13 UTC
Created attachment 145184 [details] [review]
Bug 23260: (follow-up) Add anonymise_last_borrowers method to Patrons.pm

This patch adds the method anonymise_last_borrowers in Patrons.pm.

To test:
1) apply this patch and the previous ones
2) perl installer/data/mysql/updatedatabase.pl
3) set StoreLastBorrower preference to Allow.
4) set AnonymousPatron preference to a valid patron id
5) Create a Check out followed by a Check in.
CHECK => a row should appear in items_last_borrower table with the borrower and the item number.
6) In mysql, update created_on of items_last_borrower and returndate of old_issues to two days earlier.
7) perl misc/cronjobs/anonymise_last_borrowers.pl
CHECK => borrower number in items_last_borrower is not anonymised yet.
8) Repeat step 5 and 6 but instead of two days earlier, set the new entry in items_last_borrower to three days eralier.
9) set AnonymiseLastBorrower to 'Anonymise' and AnonymiseLastBorrowerDays to 2 days.
10) perl misc/cronjobs/anonymise_last_borrowers.pl
SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron id
11) repeat step 8
12) set AnonymousPatron preference to 0
13) perl misc/cronjobs/anonymise_last_borrowers.pl
SUCCESS => borrower number in items_last_borrower changed to null
14) prove t/db_dependent/Koha/Patrons.t
Comment 134 Laura Escamilla 2023-09-08 19:32:27 UTC
error: sha1 information is lacking or useless (t/db_dependent/Koha/Patrons.t).
error: could not build fake ancestor
Patch failed at 0001 Bug 23260: (follow-up) Update test
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Comment 135 Katrin Fischer 2023-10-08 11:34:06 UTC
Nick, could you please rebase?
Comment 136 Nick Clemens 2023-10-13 15:31:51 UTC
Created attachment 157106 [details] [review]
Bug 23260: Make borrowernumber nullable in items_last_borrower

This patch alters table structure to make borrowernumber nullable

Sponsored-by: Northeast Kansas Library System

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 137 Nick Clemens 2023-10-13 15:31:55 UTC
Created attachment 157107 [details] [review]
Bug 23260: Inline code to anonymise_issue_history

This alternative patch reduces the complexity of the change by simply
updateing the existing anonymise_issue_history method to include
anonymisation of the items_last_borrowers table.

To test:
1) apply this patch and the previous one
2) perl installer/data/mysql/updatedatabase.pl
3) set StoreLastBorrower preference to Allow.
4) set AnonymousPatron preference to a valid patron id
5) Create a Check out followed by a Check in.
CHECK => a row should appear in items_last_borrower table with the borrower and the item number.
6) In mysql, update created_on of items_last_borrower and returndate of old_issues to two days earlier.
7) perl misc/cronjobs/batch_anonymise.pl --days 1
SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron id
8) repeat steps 5 and 6
9) set AnonymousPatron preference to 0
10) perl misc/cronjobs/batch_anonymise.pl --days 1
SUCCESS => borrower number in items_last_borrower changed to null
11) prove t/db_dependent/Koha/Patrons.t
12) Sign off

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 138 Nick Clemens 2023-10-13 15:31:58 UTC
Created attachment 157108 [details] [review]
Bug 23260: Simplify tests

Use build_sample_item and build_object
Comment 139 Nick Clemens 2023-10-13 15:32:02 UTC
Created attachment 157109 [details] [review]
Bug 23260: (follow-up) Remove last_borrower updates from issues count
Comment 140 Nick Clemens 2023-10-13 15:32:05 UTC
Created attachment 157110 [details] [review]
Bug 23260: Add a new test
Comment 141 Nick Clemens 2023-10-13 15:32:08 UTC
Created attachment 157111 [details] [review]
Bug 23260: Proposed preference driven filtering
Comment 142 Nick Clemens 2023-10-13 15:32:12 UTC
Created attachment 157112 [details] [review]
Bug 23260: (follow-up) Add AnonymiseLastBorrower and AnonymiseLastBorrowerDays sysprefs
Comment 143 Nick Clemens 2023-10-13 15:32:15 UTC
Created attachment 157113 [details] [review]
Bug 23260: (follow-up) Update test

Update test count
Comment 144 Nick Clemens 2023-10-13 15:32:18 UTC
Created attachment 157114 [details] [review]
Bug 23260: (follow-up) Add new cron job
Comment 145 Nick Clemens 2023-10-13 15:32:22 UTC
Created attachment 157115 [details] [review]
Bug 23260: (follow-up) Add anonymise_last_borrowers method to Patrons.pm

This patch adds the method anonymise_last_borrowers in Patrons.pm.

To test:
1) apply this patch and the previous ones
2) perl installer/data/mysql/updatedatabase.pl
3) set StoreLastBorrower preference to Allow.
4) set AnonymousPatron preference to a valid patron id
5) Create a Check out followed by a Check in.
CHECK => a row should appear in items_last_borrower table with the borrower and the item number.
6) In mysql, update created_on of items_last_borrower and returndate of old_issues to two days earlier.
7) perl misc/cronjobs/anonymise_last_borrowers.pl
CHECK => borrower number in items_last_borrower is not anonymised yet.
8) Repeat step 5 and 6 but instead of two days earlier, set the new entry in items_last_borrower to three days eralier.
9) set AnonymiseLastBorrower to 'Anonymise' and AnonymiseLastBorrowerDays to 2 days.
10) perl misc/cronjobs/anonymise_last_borrowers.pl
SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron id
11) repeat step 8
12) set AnonymousPatron preference to 0
13) perl misc/cronjobs/anonymise_last_borrowers.pl
SUCCESS => borrower number in items_last_borrower changed to null
14) prove t/db_dependent/Koha/Patrons.t
Comment 146 Nick Clemens 2023-10-13 15:32:26 UTC
Created attachment 157116 [details] [review]
Bug 23260: (follow-up) Tidy
Comment 147 Katrin Fischer 2023-11-04 23:18:06 UTC
Oi, so many rebases. Sorry we didn't get to this earlier.

1) Can you explain this part of the code please?

+        my $last_borrowers_to_anonymise = $patron->_result->items_last_borrowers->search(
+            {
+                (
+                    $older_than_date
+                    ? ( created_on => { '<' => $dtf->format_datetime($older_than_date) } )
+                    : (),
+                    "itemnumber.damaged"   => 0,
+                    "itemnumber.itemlost"  => 0,
+                    "itemnumber.withdrawn" => 0,
+                ),
+            },
+            { join => ['itemnumber'] }
+        );

2) Database updated needs to be reformatted to use the newer template.

3) Terminology / System preference naming

Sorry to be nit-picky about this, but I believe we should stick to the standard terminology as per our coding guidelines:

* Use AE instead of BE: Anonymise should be Anonymize
* Borrower should be Patron

AnonymiseLastBorrower => AnonymizeLastPatron
AnonymiseLastBorrowerDays => AnonymizeLastPatronDelay/Days

System preference descriptions need to be adapted as well.

4) Cronjob

+use strict;
+use warnings;

--> should be Use Modern::Perl.

5) anonymise_issue_history appears to be unused?
Comment 148 Nick Clemens 2023-11-28 13:46:22 UTC
Created attachment 159291 [details] [review]
Bug 23260: Database updates and new system preferences AnonymizeLastBorrower / AnonymizeLastBorrowerDays

This patch alters the items_last_borrowers table to allow for NULL borrowers

It also adds two new system preferences to allow controlling the anonymization of items_last_borrower data
Comment 149 Nick Clemens 2023-11-28 13:46:26 UTC
Created attachment 159292 [details] [review]
Bug 23260: Add anonymize_last_borrowers method to Patrons.pm

This patch adds a new routine to allow anonymizing the items_last_borrower table
Comment 150 Nick Clemens 2023-11-28 13:46:29 UTC
Created attachment 159293 [details] [review]
Bug 23260: Unit tests
Comment 151 Nick Clemens 2023-11-28 13:46:32 UTC
Created attachment 159294 [details] [review]
Bug 23260: Add cronjob anonymize_last_borrowers.pl

This patch adds the feature to anonymize patrons from items_last_borrower table. This code is run from batch_anonymize script, which is triggered from a new cron job anonymize_last_borrowers.pl

To test:
 1) apply this patch and the previous ones
 2) perl installer/data/mysql/updatedatabase.pl
 3) set StoreLastBorrower preference to Allow.
 4) set AnonymousPatron preference to a valid patron id
 5) Create a Check out followed by a Check in.
    CHECK => a row should appear in items_last_borrower table with the borrower and the item number.
 6) In mysql, update created_on of items_last_borrower and returndate of old_issues to two days earlier.
 7) perl misc/cronjobs/anonymize_last_borrowers.pl
    CHECK => borrower number in items_last_borrower is not anonymized yet.
 8) Repeat step 5 and 6 but instead of two days earlier, set the new entry in items_last_borrower to three days earlier.
 9) set AnonymizeLastBorrower to 'Anonymize' and AnonymizeLastBorrowerDays to 2 days.
10) perl misc/cronjobs/anonymize_last_borrowers.pl
    SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron id
11) repeat step 8
12) set AnonymousPatron preference to 0
13) perl misc/cronjobs/anonymize_last_borrowers.pl
SUCCESS => borrower number in items_last_borrower changed to null
14) prove t/db_dependent/Koha/Patrons.t
Comment 152 Nick Clemens 2023-11-28 13:50:42 UTC
(In reply to Katrin Fischer from comment #147)
> Oi, so many rebases. Sorry we didn't get to this earlier.
> 
> 1) Can you explain this part of the code please?
> 
> +        my $last_borrowers_to_anonymise =
> $patron->_result->items_last_borrowers->search(
> +            {
> +                (
> +                    $older_than_date
> +                    ? ( created_on => { '<' =>
> $dtf->format_datetime($older_than_date) } )
> +                    : (),
> +                    "itemnumber.damaged"   => 0,
> +                    "itemnumber.itemlost"  => 0,
> +                    "itemnumber.withdrawn" => 0,
> +                ),
> +            },
> +            { join => ['itemnumber'] }
> +        );

This fetches the items_last_borrower rows to anonymise, joining to the items table to avoid removing info from items that are no longer in circulation where the last borrower is likely to be responsible for that status

> 2) Database updated needs to be reformatted to use the newer template.

OK

> 3) Terminology / System preference naming
> 
> Sorry to be nit-picky about this, but I believe we should stick to the
> standard terminology as per our coding guidelines:
> 
> * Use AE instead of BE: Anonymise should be Anonymize
> * Borrower should be Patron
> 
> AnonymiseLastBorrower => AnonymizeLastPatron
> AnonymiseLastBorrowerDays => AnonymizeLastPatronDelay/Days
> 
> System preference descriptions need to be adapted as well.

changed 'ise' to 'ize'
As the table is 'items_last_borrower' and we are referring to 'the patron that last borrowed' I think borrower actually makes more sense in this case

> 4) Cronjob
> 
> +use strict;
> +use warnings;
> 
> --> should be Use Modern::Perl.

OK


> 5) anonymise_issue_history appears to be unused?

It looks like this has been through multiple coders and revisions - all squashed - unused routine removed
Comment 153 Martin Renvoize 2023-11-28 14:15:01 UTC
Created attachment 159298 [details] [review]
Bug 23260: Database updates and new system preferences AnonymizeLastBorrower / AnonymizeLastBorrowerDays

This patch alters the items_last_borrowers table to allow for NULL borrowers

It also adds two new system preferences to allow controlling the anonymization of items_last_borrower data

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 154 Martin Renvoize 2023-11-28 14:15:06 UTC
Created attachment 159299 [details] [review]
Bug 23260: Add anonymize_last_borrowers method to Patrons.pm

This patch adds a new routine to allow anonymizing the items_last_borrower table

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 155 Martin Renvoize 2023-11-28 14:15:10 UTC
Created attachment 159300 [details] [review]
Bug 23260: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 156 Martin Renvoize 2023-11-28 14:15:14 UTC
Created attachment 159301 [details] [review]
Bug 23260: Add cronjob anonymize_last_borrowers.pl

This patch adds the feature to anonymize patrons from items_last_borrower table. This code is run from batch_anonymize script, which is triggered from a new cron job anonymize_last_borrowers.pl

To test:
 1) apply this patch and the previous ones
 2) perl installer/data/mysql/updatedatabase.pl
 3) set StoreLastBorrower preference to Allow.
 4) set AnonymousPatron preference to a valid patron id
 5) Create a Check out followed by a Check in.
    CHECK => a row should appear in items_last_borrower table with the borrower and the item number.
 6) In mysql, update created_on of items_last_borrower and returndate of old_issues to two days earlier.
 7) perl misc/cronjobs/anonymize_last_borrowers.pl
    CHECK => borrower number in items_last_borrower is not anonymized yet.
 8) Repeat step 5 and 6 but instead of two days earlier, set the new entry in items_last_borrower to three days earlier.
 9) set AnonymizeLastBorrower to 'Anonymize' and AnonymizeLastBorrowerDays to 2 days.
10) perl misc/cronjobs/anonymize_last_borrowers.pl
    SUCCESS => borrower number in items_last_borrower changed to AnonymousPatron id
11) repeat step 8
12) set AnonymousPatron preference to 0
13) perl misc/cronjobs/anonymize_last_borrowers.pl
SUCCESS => borrower number in items_last_borrower changed to null
14) prove t/db_dependent/Koha/Patrons.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 157 Martin Renvoize 2023-11-28 14:15:50 UTC
Nice clean rebase, all workings as described here now.

Signing off and passing back for a final QA round.