Bug 33229 - Patron reading history should be cleared when privacy set to never
Summary: Patron reading history should be cleared when privacy set to never
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-03-14 20:02 UTC by Nick Clemens
Modified: 2023-12-28 20:43 UTC (History)
4 users (show)

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


Attachments
Bug 33229: Clear patron reading history when privacy is set to never (4.48 KB, patch)
2023-03-14 20:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33229: Clear patron reading history when privacy is set to never (4.54 KB, patch)
2023-03-15 09:34 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 33229: Clear patron reading history when privacy is set to never (4.62 KB, patch)
2023-03-22 13:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33229: Remove unecessary txn (1.10 KB, patch)
2023-03-22 13:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33229: (follow-up) Adjust tests to set anonymous patron before setting patron privacy to 2 (2.59 KB, patch)
2023-04-17 12:52 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2023-03-14 20:02:03 UTC
If a patron chooses to never keep privacy we should immediately remove their history, rather than waiting on the anonymization cronjob
Comment 1 Nick Clemens 2023-03-14 20:06:29 UTC
Created attachment 148186 [details] [review]
Bug 33229: Clear patron reading history when privacy is set to never

This patch adds a check to patron store routine. When privacy is changed
to 'Never' we should immediately clear their history

To test:
1 - Set system preferences:
   AnonymousPatron - to a valid borrowernumber
   OPACPrivacy - Allow
2 - Checkout some items to a patron with priacy set to default, return them
3 - Log in to opac as patron
4 - Confirm you can see your history
5 - Change history to Never
6 - Confirm history still viewable
7 - Apply patch, restart all
8 - Set privacy to Default
9 - Confirm you can still see history, not deleted when not set as Never
10 - Set privacy to 'Never'
11 - Confirm history is anonymized
Comment 2 ByWater Sandboxes 2023-03-15 09:34:27 UTC
Created attachment 148197 [details] [review]
Bug 33229: Clear patron reading history when privacy is set to never

This patch adds a check to patron store routine. When privacy is changed
to 'Never' we should immediately clear their history

To test:
1 - Set system preferences:
   AnonymousPatron - to a valid borrowernumber
   OPACPrivacy - Allow
2 - Checkout some items to a patron with priacy set to default, return them
3 - Log in to opac as patron
4 - Confirm you can see your history
5 - Change history to Never
6 - Confirm history still viewable
7 - Apply patch, restart all
8 - Set privacy to Default
9 - Confirm you can still see history, not deleted when not set as Never
10 - Set privacy to 'Never'
11 - Confirm history is anonymized

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 3 Sally 2023-03-15 09:35:55 UTC
Works great, thanks for the patch.
Comment 4 Jonathan Druart 2023-03-22 13:35:37 UTC
Created attachment 148546 [details] [review]
Bug 33229: Clear patron reading history when privacy is set to never

This patch adds a check to patron store routine. When privacy is changed
to 'Never' we should immediately clear their history

To test:
1 - Set system preferences:
   AnonymousPatron - to a valid borrowernumber
   OPACPrivacy - Allow
2 - Checkout some items to a patron with priacy set to default, return them
3 - Log in to opac as patron
4 - Confirm you can see your history
5 - Change history to Never
6 - Confirm history still viewable
7 - Apply patch, restart all
8 - Set privacy to Default
9 - Confirm you can still see history, not deleted when not set as Never
10 - Set privacy to 'Never'
11 - Confirm history is anonymized

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 5 Jonathan Druart 2023-03-22 13:35:40 UTC
Created attachment 148547 [details] [review]
Bug 33229: Remove unecessary txn

We don't need a transaction here.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 6 Jonathan Druart 2023-03-22 13:35:52 UTC
Nick, I don't understand why you need a txn here. Do you agree with removing it?
Comment 7 Nick Clemens 2023-03-22 14:07:24 UTC
(In reply to Jonathan Druart from comment #6)
> Nick, I don't understand why you need a txn here. Do you agree with removing
> it?

My assumption was that I copied it from somewhere, but I don't see that.

Possibly I thought we didn't want partial deletion? But if we are trying to anonymize i suppose removing some is better than none
Comment 8 Jonathan Druart 2023-03-22 14:23:11 UTC
(In reply to Nick Clemens from comment #7)
> (In reply to Jonathan Druart from comment #6)
> > Nick, I don't understand why you need a txn here. Do you agree with removing
> > it?
> 
> My assumption was that I copied it from somewhere, but I don't see that.
> 
> Possibly I thought we didn't want partial deletion? But if we are trying to
> anonymize i suppose removing some is better than none

It's done in a single query
104     return $self->update( { borrowernumber => $anonymous_id }, { no_triggers => 1 } );
So it's either all or none.

Anyway, thanks for you reply!
Comment 9 Tomás Cohen Arazi 2023-04-14 18:39:24 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 10 Katrin Fischer 2023-04-16 10:00:56 UTC
Hi Nick, git bisect (not me!) blames you for the failing test on master:

https://jenkins.koha-community.org/job/Koha_Master/2479/testReport/junit/(root)/t_db_dependent_Koha_Hold_t/fill___tests__1624_/

I had a look, but couldn't figure it out:
#   Failed test 'fill() tests'
#   at t/db_dependent/Koha/Hold.t line 307.
Exception 'Koha::Exceptions::SysPref::NotSet' thrown 'Required syspref is not set' with syspref => AnonymousPatron
# Looks like your test exited with 255 just after 2.
Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 10/11 subtests
Comment 11 Tomás Cohen Arazi 2023-04-17 12:31:31 UTC
This broke tests. Please review the consequences of this new exception thrown, on the UI as well.

kohadev-koha@kohadevbox:koha(master)$ prove t/db_dependent/Koha/Hold.t 
t/db_dependent/Koha/Hold.t .. 1/11         # Looks like you planned 5 tests but ran 2.

    #   Failed test 'anonymization behavior tests'
    #   at t/db_dependent/Koha/Hold.t line 265.
    # Looks like you planned 13 tests but ran 12.
    # Looks like you failed 1 test of 12 run.
t/db_dependent/Koha/Hold.t .. 2/11 
#   Failed test 'fill() tests'
#   at t/db_dependent/Koha/Hold.t line 307.
Exception 'Koha::Exceptions::SysPref::NotSet' thrown 'Required syspref is not set' with syspref => AnonymousPatron
# Looks like your test exited with 255 just after 2.
t/db_dependent/Koha/Hold.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 10/11 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/Hold.t (Wstat: 65280 Tests: 2 Failed: 1)
  Failed test:  2
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 11 tests but ran 2.
Files=1, Tests=2,  3 wallclock secs ( 0.02 usr  0.00 sys +  2.91 cusr  0.55 csys =  3.48 CPU)
Result: FAIL
Comment 12 Nick Clemens 2023-04-17 12:52:49 UTC
Created attachment 149753 [details] [review]
Bug 33229: (follow-up) Adjust tests to set anonymous patron before setting patron privacy to 2
Comment 13 Tomás Cohen Arazi 2023-04-17 13:00:19 UTC
Follow-up pushed. Thanks Nick for the quick fix.
Comment 14 Matt Blenkinsop 2023-04-18 12:28:58 UTC
Nice work everyone!

Pushed to stable for 22.11.x
Comment 15 Lucas Gass 2023-05-05 22:31:06 UTC
Doesn't apply clean to 22.05.x, no backport.