The checkout_renewals table introduced in bug 30275 can retain a link between a patron and an issue_id when the patron's privacy is set to Never. That data should be anonymized. To recreate: - have a patron with their privacy set to Never retain reading history - check an item out to the patron, renew it via the OPAC - confirm your patron's borrowernumber can be found in checkout_renewals.renewer_id and the issue_id for your checkout can be found in checkout_renewals.checkout_id - check your item in - confirm your patron's borrowernumber has been removed from old_issues.borrowernumber - confirm your patron's borrowernumber and the issue_id for your checkout can still be found in checkout_renewals.checkout_id (along with a note that the renewal happened via the OPAC, thereby making it perfectly clear that this was a patron renewing the item they had checked out themselves). Should we not replace checkout_renewals.renewer_id with the anonymous patron's borrowernumber when the item is returned, if the patron is set to not retain reading history?
Oh, darn. Thanks Andrew.. my apologies for missing that when developing this feature. I'm running your proposal past Aude here, whose a lot more up to speed on borrower privacy.. but in short, I think you're right.. just using the anonymous user id should tick the right box.
Created attachment 148469 [details] [review] Bug 33284: Set renewer_id appropriately for anonymize settings This patch adds a trigger into the anonymize function of Koha::Old::Checkout such that we pass along the anonymize action to the renewals record.
Test plan: 1 - Apply patch 2 - Have 3 patrons covering the 3 privacy options: Default, Forever, Never 3 - Check an item out to each patron 4 - renew each patron's item via the staff interface 5 - renew each patron's item via the OPAC 6 - SELECT * FROM checkout_renewals, confirm all staff interface renewals show your borrowernumber, all OPAC renewals show the patron's borrowernumber 7 - return all items 8 - repeat #6, confirm that the Never patron's OPAC renewal now shows the anonymous borrowernumber but other values have not changed 9 - anonymize all circ data using Batch patron deletion and anonymization 10 - repeat #6, confirm the Default patron's OPAC renewal now shows the anonymous borrowernumber but other values have not changed
Created attachment 148486 [details] [review] Bug 33284: Unit test
Created attachment 148487 [details] [review] Bug 33284: Set renewer_id appropriately for anonymize settings This patch adds a trigger into the anonymize function of Koha::Old::Checkout such that we pass along the anonymize action to the renewals record.
(In reply to Martin Renvoize from comment #1) > Oh, darn. Thanks Andrew.. my apologies for missing that when developing this > feature. > > I'm running your proposal past Aude here, whose a lot more up to speed on > borrower privacy.. but in short, I think you're right.. just using the > anonymous user id should tick the right box. Thanks for the quick fix!
(In reply to Andrew Fuerste-Henry from comment #3) > Test plan: > 1 - Apply patch > 2 - Have 3 patrons covering the 3 privacy options: Default, Forever, Never > 3 - Check an item out to each patron > 4 - renew each patron's item via the staff interface > 5 - renew each patron's item via the OPAC > 6 - SELECT * FROM checkout_renewals, confirm all staff interface renewals > show your borrowernumber, all OPAC renewals show the patron's borrowernumber > 7 - return all items > 8 - repeat #6, confirm that the Never patron's OPAC renewal now shows the > anonymous borrowernumber but other values have not changed > 9 - anonymize all circ data using Batch patron deletion and anonymization > 10 - repeat #6, confirm the Default patron's OPAC renewal now shows the > anonymous borrowernumber but other values have not changed This all performs as desired, except for steps 9 and 10, which I hadn't thought about until writing the test plan. Currently this patch anonymizes renewals at checkin for patrons set to Never for privacy. We should also anonymize renewals for patrons set to Default, but as part of the Batch Anonymize tool/cron. I'm failing QA for that reason, but if you want to address that on a separate bug I've got no problem signing off on this one for the Never patron fix.
Awesome spot Andrew, I have a fix, just writing the test for it now.
Created attachment 148670 [details] [review] Bug 33284: Unit test
Created attachment 148671 [details] [review] Bug 33284: Set renewer_id appropriately for anonymize settings This patch adds a trigger into the anonymize function of Koha::Old::Checkout such that we pass along the anonymize action to the renewals record.
Created attachment 148672 [details] [review] Bug 33284: Set renewer_id for sets of checkouts This patch adds a trigger into the anonymize function of Koha::Old::Checkouts such that we pass along the anonymize action to the renewals record.
Created attachment 148772 [details] [review] Bug 33284: Unit test Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Created attachment 148773 [details] [review] Bug 33284: Set renewer_id appropriately for anonymize settings This patch adds a trigger into the anonymize function of Koha::Old::Checkout such that we pass along the anonymize action to the renewals record. Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Created attachment 148774 [details] [review] Bug 33284: Set renewer_id for sets of checkouts This patch adds a trigger into the anonymize function of Koha::Old::Checkouts such that we pass along the anonymize action to the renewals record. Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Please remember to add the test plan to the commit messages. I am going to use Andrew's from comment 3.
I think that the option to immediately "delete circulation history" from the patron account in the OPAC doesn't work correctly yet. I have no more issues, but there is still a renewal entry that has the renewer_id of my user. The immediately deletion should always work, independent of the privacy setting.
Hi all, giving this bug a bump as it continues to force us to keep patron circulation history in violation of their privacy selection.
(In reply to Katrin Fischer from comment #16) > I think that the option to immediately "delete circulation history" from the > patron account in the OPAC doesn't work correctly yet. I have no more > issues, but there is still a renewal entry that has the renewer_id of my > user. > > The immediately deletion should always work, independent of the privacy > setting. This works in my testing - the renewal from the opac is cleared - the renewal from the staff remains, I used a different patron on staff and opac side. When I used the same patron, both renewals were anonymized - again, this makes sense. In this case, the original renewal with the staff patron for the other account remained, and I htink this is correct too - the patron gets anonymized - other staff don't
The tests here randomly fail - using builder to create the renewals seems to sometimes create a related checkotu object, and sometimes a related old checkotu object - I think you need to generate the correct data using AddIssue/AddRenew/MarkIssueReturned
Hi, This kitten seems lost, and people are still wanting this functionality. Is there any chance it can get another look? Cheers, Liz
Created attachment 168596 [details] [review] Bug 33284: Unit test This patch adds unit tests to ensure renewals properly respect the patron privacy settings. Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Created attachment 168597 [details] [review] Bug 33284: Set renewer_id appropriately for anonymize settings This patch adds a trigger into the anonymize function of Koha::Old::Checkout such that we pass along the anonymize action to the renewals record. Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Created attachment 168598 [details] [review] Bug 33284: Set renewer_id for sets of checkouts This patch adds a trigger into the anonymize function of Koha::Old::Checkouts such that we pass along the anonymize action to the renewals record. Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Created attachment 168599 [details] [review] Bug 33284: (follow-up) Fix unit tests We have a case here where we have checkout_id pointing at issues and old_issues tables. We need to trick TestBuilder to NOT automagically build our related checkout.. This patch passes 'undef' to the foreign key field and then sets it after the fact to the related Old::Checkouts objects we've already built above. This patch should prevent the random failures we were seeing before
Rebased and added a follow-up to address Nicks QA concerns.
Still getting random failures: # Subtest: anonymize() tests 1..9 ok 1 - Patron has no old checkouts Value not allowed for auto_incr issue_id in Issue at /kohadevbox/koha/t/lib/TestBuilder.pm line 402. Value not allowed for auto_incr issue_id in Issue at /kohadevbox/koha/t/lib/TestBuilder.pm line 402. ok 2 - Patron has 2 completed checkouts ok 3 - Exception thrown because AnonymousPatron not set ok 4 - syspref parameter is correctly passed ok 5 - No changes, patron has 2 linked completed checkouts ok 6 - Checkout to anonymize still linked to patron not ok 7 - Checkout 2 has 2 renewals # Failed test 'Checkout 2 has 2 renewals' # at /kohadevbox/koha/t/db_dependent/Koha/Old/Checkout.t line 87. # got: '0' # expected: '2' ok 8 - Anonymized checkout linked to anonymouspatron not ok 9 - OPAC renewal was anonymized # Failed test 'OPAC renewal was anonymized' # at /kohadevbox/koha/t/db_dependent/Koha/Old/Checkout.t line 98. # got: '0' # expected: '1' # Looks like you failed 2 tests of 9. not ok 1 - anonymize() tests # Failed test 'anonymize() tests' # at /kohadevbox/koha/t/db_dependent/Koha/Old/Checkout.t line 107. # Subtest: deleteitem() tests 1..1 ok 1 - Item is deleted ok 2 - deleteitem() tests # Looks like you failed 1 test of 2. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/2 subtests Test Summary Report ------------------- /kohadevbox/koha/t/db_dependent/Koha/Old/Checkout.t (Wstat: 256 (exited 1) Tests: 2 Failed: 1) Failed test: 1 Non-zero exit status: 1 Files=1, Tests=2, 3 wallclock secs ( 0.02 usr 0.00 sys + 2.77 cusr 0.44 csys = 3.23 CPU) Result: FAIL
Hmm, I can't get the test to fail.. I just ran it 100 times in a loop and it passed every time. [U+1F61E]