Bug 33284 - checkout_renewals table retains checkout history in violation of patron privacy
Summary: checkout_renewals table retains checkout history in violation of patron privacy
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P1 - high major (vote)
Assignee: Martin Renvoize
QA Contact: Nick Clemens (kidclamp)
URL:
Keywords: rel_23_05_candidate
Depends on: 30275
Blocks:
  Show dependency treegraph
 
Reported: 2023-03-20 19:37 UTC by Andrew Fuerste-Henry
Modified: 2024-07-13 05:47 UTC (History)
6 users (show)

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


Attachments
Bug 33284: Set renewer_id appropriately for anonymize settings (995 bytes, patch)
2023-03-21 14:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33284: Unit test (2.13 KB, patch)
2023-03-21 17:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33284: Set renewer_id appropriately for anonymize settings (995 bytes, patch)
2023-03-21 17:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33284: Unit test (4.58 KB, patch)
2023-03-24 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33284: Set renewer_id appropriately for anonymize settings (995 bytes, patch)
2023-03-24 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33284: Set renewer_id for sets of checkouts (1.06 KB, patch)
2023-03-24 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33284: Unit test (4.64 KB, patch)
2023-03-27 13:39 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 33284: Set renewer_id appropriately for anonymize settings (1.03 KB, patch)
2023-03-27 13:39 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 33284: Set renewer_id for sets of checkouts (1.11 KB, patch)
2023-03-27 13:39 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 33284: Unit test (4.71 KB, patch)
2024-07-08 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33284: Set renewer_id appropriately for anonymize settings (1.03 KB, patch)
2024-07-08 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33284: Set renewer_id for sets of checkouts (1.11 KB, patch)
2024-07-08 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33284: (follow-up) Fix unit tests (3.32 KB, patch)
2024-07-08 16:20 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 Andrew Fuerste-Henry 2023-03-20 19:37:53 UTC
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?
Comment 1 Martin Renvoize 2023-03-21 12:21:34 UTC
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.
Comment 2 Martin Renvoize 2023-03-21 14:09:46 UTC
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.
Comment 3 Andrew Fuerste-Henry 2023-03-21 16:52:02 UTC
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
Comment 4 Martin Renvoize 2023-03-21 17:04:15 UTC
Created attachment 148486 [details] [review]
Bug 33284: Unit test
Comment 5 Martin Renvoize 2023-03-21 17:04:18 UTC
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.
Comment 6 Andrew Fuerste-Henry 2023-03-21 17:11:23 UTC
(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!
Comment 7 Andrew Fuerste-Henry 2023-03-21 17:46:41 UTC
(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.
Comment 8 Martin Renvoize 2023-03-24 13:38:31 UTC
Awesome spot Andrew, 

I have a fix, just writing the test for it now.
Comment 9 Martin Renvoize 2023-03-24 16:09:41 UTC
Created attachment 148670 [details] [review]
Bug 33284: Unit test
Comment 10 Martin Renvoize 2023-03-24 16:09:43 UTC
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.
Comment 11 Martin Renvoize 2023-03-24 16:09:46 UTC
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.
Comment 12 Andrew Fuerste-Henry 2023-03-27 13:39:52 UTC
Created attachment 148772 [details] [review]
Bug 33284: Unit test

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 13 Andrew Fuerste-Henry 2023-03-27 13:39:55 UTC
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>
Comment 14 Andrew Fuerste-Henry 2023-03-27 13:39:57 UTC
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>
Comment 15 Katrin Fischer 2023-04-10 11:36:37 UTC
Please remember to add the test plan to the commit messages. I am going to use Andrew's from comment 3.
Comment 16 Katrin Fischer 2023-04-10 14:25:49 UTC
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.
Comment 17 Andrew Fuerste-Henry 2023-12-07 16:56:34 UTC
Hi all, giving this bug a bump as it continues to force us to keep patron circulation history in violation of their privacy selection.
Comment 18 Nick Clemens (kidclamp) 2023-12-22 15:51:47 UTC
(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
Comment 19 Nick Clemens (kidclamp) 2023-12-22 16:34:13 UTC
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
Comment 20 Liz Rea 2024-05-08 16:22:46 UTC
Hi,
This kitten seems lost, and people are still wanting this functionality.

Is there any chance it can get another look?
Cheers,
Liz
Comment 21 Martin Renvoize 2024-07-08 16:20:49 UTC
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>
Comment 22 Martin Renvoize 2024-07-08 16:20:52 UTC
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>
Comment 23 Martin Renvoize 2024-07-08 16:20:54 UTC
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>
Comment 24 Martin Renvoize 2024-07-08 16:20:57 UTC
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
Comment 25 Martin Renvoize 2024-07-08 18:27:57 UTC
Rebased and added a follow-up to address Nicks QA concerns.
Comment 26 Nick Clemens (kidclamp) 2024-07-12 19:19:04 UTC
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
Comment 27 Martin Renvoize 2024-07-13 05:47:40 UTC
Hmm, I can't get the test to fail.. I just ran it 100 times in a loop and it passed every time. 😞