Description
Joy Nelson
2019-07-03 17:49:32 UTC
*** Bug 23500 has been marked as a duplicate of this bug. *** +1 Created attachment 92863 [details] [review] Bug 23260: Make borrowernumber nullable in items_last_borrower This patch alters table structure to make borrowernumber nullable 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 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 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 Created attachment 94858 [details] [review] Bug 23260: Make borrowernumber nullable in items_last_borrower This patch alters table structure to make borrowernumber nullable 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> (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 QAing 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. Created attachment 95033 [details] [review] Bug 23260: (follow-up) optimize anonymise_items_last_borrower Thx for continuing here, Augustin. Just wondering what happens here: - subtest 'Logged in librarian is not superlibrarian & IndependentBranches' => sub { Why remove another subtest?? (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 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 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> Created attachment 95459 [details] [review] Bug 23260: (follow-up) optimize anonymise_items_last_borrower Sponsored-by: Northeast Kansas Library System (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. (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. 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> ReQAing 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. 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. 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) $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. 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 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> 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> 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. (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. (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? (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! 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! (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. (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? (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. (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. (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. 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> 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> 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> 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. Why $class->_resultset->search instead of $self->search? 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. 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. (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. (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. FYI On bug 24152 I provide a flexible way to remove Koha::Object-based objects depending on date range. Nice work everyone! Pushed to master for 20.05 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. 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 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. 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. 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) Martin, your alternative approach looks good to me! 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. 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> 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 (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. 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> Created attachment 98053 [details] [review] Bug 23260: Simplify tests Use build_sample_item and build_object Created attachment 98054 [details] [review] Bug 23260: Add a new test 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. (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. Created attachment 98064 [details] [review] Bug 23260: (follow-up) Remove last_borrower updates from issues count Seems I forgot to reset the status when I added the followup.. ready for QA Created attachment 98282 [details] [review] Bug 23260: Add a new test (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. Created attachment 98349 [details] [review] Bug 23260: Add a new test (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. 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! Setting to Failed QA whilst I code the followup Created attachment 98352 [details] [review] Bug 23260: Proposed preference driven filtering 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." 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. 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? 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. 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. 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. (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 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> 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> Created attachment 112736 [details] [review] Bug 23260: Simplify tests Use build_sample_item and build_object Created attachment 112737 [details] [review] Bug 23260: (follow-up) Remove last_borrower updates from issues count Created attachment 112738 [details] [review] Bug 23260: Add a new test Created attachment 112739 [details] [review] Bug 23260: Proposed preference driven filtering Created attachment 112740 [details] [review] Bug 23260: (follow-up) Add AnonymiseLastBorrower and AnonymiseLastBorrowerDays sysprefs Created attachment 112741 [details] [review] Bug 23260: (follow-up) Update test Created attachment 112742 [details] [review] Bug 23260: (follow-up) Add new cron job 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 (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! 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> 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> Created attachment 117976 [details] [review] Bug 23260: Simplify tests Use build_sample_item and build_object Created attachment 117977 [details] [review] Bug 23260: (follow-up) Remove last_borrower updates from issues count Created attachment 117978 [details] [review] Bug 23260: Add a new test Created attachment 117979 [details] [review] Bug 23260: Proposed preference driven filtering Created attachment 117980 [details] [review] Bug 23260: (follow-up) Add AnonymiseLastBorrower and AnonymiseLastBorrowerDays sysprefs Created attachment 117981 [details] [review] Bug 23260: (follow-up) Update test Created attachment 117982 [details] [review] Bug 23260: (follow-up) Add new cron job 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 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 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> 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> Created attachment 120524 [details] [review] Bug 23260: Simplify tests Use build_sample_item and build_object Created attachment 120525 [details] [review] Bug 23260: (follow-up) Remove last_borrower updates from issues count Created attachment 120526 [details] [review] Bug 23260: Add a new test Created attachment 120527 [details] [review] Bug 23260: Proposed preference driven filtering Created attachment 120528 [details] [review] Bug 23260: (follow-up) Add AnonymiseLastBorrower and AnonymiseLastBorrowerDays sysprefs Created attachment 120529 [details] [review] Bug 23260: (follow-up) Update test Created attachment 120530 [details] [review] Bug 23260: (follow-up) Add new cron job 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 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 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> 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> Created attachment 122965 [details] [review] Bug 23260: Simplify tests Use build_sample_item and build_object Created attachment 122966 [details] [review] Bug 23260: (follow-up) Remove last_borrower updates from issues count Created attachment 122967 [details] [review] Bug 23260: Add a new test Created attachment 122968 [details] [review] Bug 23260: Proposed preference driven filtering Created attachment 122969 [details] [review] Bug 23260: (follow-up) Add AnonymiseLastBorrower and AnonymiseLastBorrowerDays sysprefs Created attachment 122970 [details] [review] Bug 23260: (follow-up) Update test Update test count Created attachment 122971 [details] [review] Bug 23260: (follow-up) Add new cron job 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 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> 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> Created attachment 145177 [details] [review] Bug 23260: Simplify tests Use build_sample_item and build_object Created attachment 145178 [details] [review] Bug 23260: (follow-up) Remove last_borrower updates from issues count Created attachment 145179 [details] [review] Bug 23260: Add a new test Created attachment 145180 [details] [review] Bug 23260: Proposed preference driven filtering Created attachment 145181 [details] [review] Bug 23260: (follow-up) Add AnonymiseLastBorrower and AnonymiseLastBorrowerDays sysprefs Created attachment 145182 [details] [review] Bug 23260: (follow-up) Update test Update test count Created attachment 145183 [details] [review] Bug 23260: (follow-up) Add new cron job 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 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 Nick, could you please rebase? 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> 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> Created attachment 157108 [details] [review] Bug 23260: Simplify tests Use build_sample_item and build_object Created attachment 157109 [details] [review] Bug 23260: (follow-up) Remove last_borrower updates from issues count Created attachment 157110 [details] [review] Bug 23260: Add a new test Created attachment 157111 [details] [review] Bug 23260: Proposed preference driven filtering Created attachment 157112 [details] [review] Bug 23260: (follow-up) Add AnonymiseLastBorrower and AnonymiseLastBorrowerDays sysprefs Created attachment 157113 [details] [review] Bug 23260: (follow-up) Update test Update test count Created attachment 157114 [details] [review] Bug 23260: (follow-up) Add new cron job 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 Created attachment 157116 [details] [review] Bug 23260: (follow-up) Tidy 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? 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 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 Created attachment 159293 [details] [review] Bug 23260: Unit tests 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 (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 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> 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> Created attachment 159300 [details] [review] Bug 23260: Unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> 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> Nice clean rebase, all workings as described here now. Signing off and passing back for a final QA round. Created attachment 164129 [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> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 164130 [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> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 164131 [details] [review] Bug 23260: Unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 164132 [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> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 164133 [details] [review] Bug 23260: (QA follow-up) Tidy code Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> I think as this is going to remove/delete data and doesn't require any options in order to be run, it would be good to have a confirm flag on the script. What do you think? |