Bug 23260 - Anonymize (remove) patron data from items_last_borrower
Summary: Anonymize (remove) patron data from items_last_borrower
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Agustín Moyano
QA Contact: Marcel de Rooy
URL:
Keywords:
: 23500 (view as bug list)
Depends on:
Blocks: 24092
  Show dependency treegraph
 
Reported: 2019-07-03 17:49 UTC by Joy Nelson
Modified: 2020-02-05 23:16 UTC (History)
10 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 23260: Make borrowernumber nullable in items_last_borrower (1.82 KB, patch)
2019-09-16 21:18 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23260: Add anonymize items_last_borrower feature (16.83 KB, patch)
2019-09-16 21:18 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23260: Add anonymize items_last_borrower feature (16.88 KB, patch)
2019-09-20 20:20 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 23260: Make borrowernumber nullable in items_last_borrower (1.82 KB, patch)
2019-10-30 03:35 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23260: Add anonymize items_last_borrower feature (16.90 KB, patch)
2019-10-30 03:35 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23260: (follow-up) optimize anonymise_items_last_borrower (12.29 KB, patch)
2019-11-04 19:37 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23260: Make borrowernumber nullable in items_last_borrower (1.86 KB, patch)
2019-11-15 17:13 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23260: Add anonymize items_last_borrower feature (16.94 KB, patch)
2019-11-15 17:13 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23260: (follow-up) optimize anonymise_items_last_borrower (12.33 KB, patch)
2019-11-15 17:13 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23260: (follow-up) optimize anonymise_items_last_borrower (12.38 KB, patch)
2019-11-15 19:08 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 23260: (QA follow-up) Use ->update instead of ->next (2.40 KB, patch)
2019-11-21 10:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23260: Make borrowernumber nullable in items_last_borrower (1.86 KB, patch)
2019-11-21 17:14 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23260: Add anonymize items_last_borrower feature (16.94 KB, patch)
2019-11-21 17:14 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23260: (follow-up) optimize anonymise_items_last_borrower (12.56 KB, patch)
2019-11-21 17:14 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23260: (QA follow-up) Use ->update instead of ->next (2.36 KB, patch)
2019-11-21 17:14 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23260: Make borrowernumber nullable in items_last_borrower (1.96 KB, patch)
2019-11-25 09:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23260: Add anonymize items_last_borrower feature (17.06 KB, patch)
2019-11-25 09:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23260: (follow-up) optimize anonymise_items_last_borrower (12.67 KB, patch)
2019-11-25 09:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23260: (QA follow-up) Use ->update instead of ->next (2.50 KB, patch)
2019-11-25 09:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23260: [ALTERNATIVE] Inline code to anonymise_issue_history (2.02 KB, patch)
2020-01-03 12:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23260: Make borrowernumber nullable in items_last_borrower (1.92 KB, patch)
2020-01-23 20:49 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23260: Inline code to anonymise_issue_history (8.36 KB, patch)
2020-01-23 20:49 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23260: Inline code to anonymise_issue_history (8.42 KB, patch)
2020-01-28 17:54 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23260: Simplify tests (7.46 KB, patch)
2020-01-29 09:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23260: Add a new test (1.41 KB, patch)
2020-01-29 09:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23260: (follow-up) Remove last_borrower updates from issues count (983 bytes, patch)
2020-01-29 12:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23260: Add a new test (1.41 KB, patch)
2020-02-03 12:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23260: Add a new test (1.41 KB, patch)
2020-02-03 16:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23260: Proposed preference driven filtering (2.71 KB, patch)
2020-02-03 17:05 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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