Bug 29525

Summary: Privacy settings for patrons should also affect holds history
Product: Koha Reporter: Nick Clemens <nick>
Component: PatronsAssignee: Tomás Cohen Arazi <tomascohen>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: andrew, fridolin.somers, george, gmcharlt, jonathan.druart+koha, katrin.fischer, kyle.m.hall, kyle, lucas, martin.renvoize, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
22.05.00
Bug Depends on: 20936, 29780, 29868    
Bug Blocks: 29879, 29526    
Attachments: Bug 29525: Add Koha::Old::Holds->filter_by_anonymizable
Bug 29525: Make batch_anonymise.pl deal with holds
Bug 29525: (follow-up) Adapt OPAC text to mention holds
Bug 29525: Add Koha::Old::Holds->filter_by_anonymizable
Bug 29525: Make batch_anonymise.pl deal with holds
Bug 29525: (follow-up) Adapt OPAC text to mention holds
Bug 29525: Add Koha::Old::Holds->filter_by_anonymizable
Bug 29525: Make batch_anonymise.pl deal with holds
Bug 29525: (follow-up) Adapt OPAC text to mention holds
Bug 29525: Make batch_anonymise.pl deal with holds
Bug 29843: Unit tests
Bug 29525: Add Koha::Old::Holds->filter_by_anonymizable
Bug 29525: Make batch_anonymise.pl deal with holds
Bug 29525: (follow-up) Adapt OPAC text to mention holds
Bug 29525: Make Koha::Hold->cancel anonymize if required
Bug 29525: Make Koha::Hold->cancel anonymize if required
Bug 29525: Add Koha::Old::Holds->filter_by_anonymizable
Bug 29525: Make batch_anonymise.pl deal with holds
Bug 29525: (follow-up) Adapt OPAC text to mention holds
Bug 29525: Make Koha::Hold->cancel anonymize if required
Bug 29525: Make Koha::Hold->fill anonymize if required
Bug 29525: Add Koha::Old::Holds->filter_by_anonymizable
Bug 29525: Make batch_anonymise.pl deal with holds
Bug 29525: (follow-up) Adapt OPAC text to mention holds
Bug 29525: Make Koha::Hold->cancel anonymize if required
Bug 29525: Make Koha::Hold->fill anonymize if required
Bug 29525: Make Koha::Hold->cancel anonymize if required
Bug 29525: Make Koha::Hold->fill anonymize if required
Bug 29525: Add Koha::Old::Holds->filter_by_anonymizable
Bug 29525: Make batch_anonymise.pl deal with holds
Bug 29525: (follow-up) Adapt OPAC text to mention holds
Bug 29525: Make Koha::Hold->cancel anonymize if required
Bug 29525: Make Koha::Hold->fill anonymize if required

Description Nick Clemens 2021-11-19 01:15:19 UTC
Adjust Patron Privacy preferences to include holds history into privacy considerations.   
If patron privacy setting is set to never, never retain holds history (anonymize it).  
If privacy is set to keep, keep holds history for the patron forever. 
If privacy is set to default, keep it until library chooses to anonymize hold history. (add holds history functionality or option to batch_anonymise cronjob)
Comment 1 George Williams (NEKLS) 2021-11-19 14:08:17 UTC
+1
Comment 2 Tomás Cohen Arazi 2022-01-03 18:18:14 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2022-01-03 18:18:19 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2022-01-03 18:18:26 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2022-01-10 13:17:24 UTC Comment hidden (obsolete)
Comment 6 Tomás Cohen Arazi 2022-01-10 13:17:29 UTC Comment hidden (obsolete)
Comment 7 Tomás Cohen Arazi 2022-01-10 13:17:33 UTC Comment hidden (obsolete)
Comment 8 Tomás Cohen Arazi 2022-01-10 13:36:49 UTC
Sorry for the noise, just learnt about Koha::Objects->filter_by_last_update
Comment 9 Tomás Cohen Arazi 2022-01-10 13:37:33 UTC Comment hidden (obsolete)
Comment 10 Tomás Cohen Arazi 2022-01-10 13:37:39 UTC Comment hidden (obsolete)
Comment 11 Tomás Cohen Arazi 2022-01-10 13:37:45 UTC Comment hidden (obsolete)
Comment 12 Tomás Cohen Arazi 2022-01-10 14:28:45 UTC Comment hidden (obsolete)
Comment 13 Tomás Cohen Arazi 2022-01-10 15:17:52 UTC Comment hidden (obsolete)
Comment 14 Tomás Cohen Arazi 2022-01-11 19:18:36 UTC
Created attachment 129321 [details] [review]
Bug 29525: Add Koha::Old::Holds->filter_by_anonymizable

This patch adds the 'filter_by_anonymizable' method, and tests for it. A
new DBIC relationship is added as well to the OldReserve schema file.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ t/db_dependent/Koha/Old/Holds.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 15 Tomás Cohen Arazi 2022-01-11 19:18:45 UTC
Created attachment 129322 [details] [review]
Bug 29525: Make batch_anonymise.pl deal with holds

This patch makes the batch_anonymise.pl script handle holds too. It does
so by leveraging on the newly introduced method 'filter_by_anonymizable'
and also 'anonymize'.

To test:
1. Have a patron with two past holds.
2. Make sure they are a few days back:
   $ koha-mysql kohadev
   > UPDATE old_reserves SET timestamp=ADDDATE(NOW(), INTERVAL -4 DAY);
3. Run:
   $ kshell
  k$ perl misc/cronjobs/batch_anonymise.pl --days 2 -v
=> SUCCESS: You see something like:
Checkouts and holds before 2022-01-01 will be anonymised.
0 checkouts anonymised.
2 holds anonymised.
4. Repeat 3
=> SUCCESS: They are already anonymized. You see
Checkouts and holds before 2022-01-01 will be anonymised.
0 checkouts anonymised.
0 holds anonymised.
5. Sign off :-D
Comment 16 Tomás Cohen Arazi 2022-01-11 19:18:53 UTC
Created attachment 129323 [details] [review]
Bug 29525: (follow-up) Adapt OPAC text to mention holds
Comment 17 Andrew Fuerste-Henry 2022-01-12 14:33:07 UTC
We're not fully mirroring circ history behavior. For checkout history, if a patron is set to Never they get anonymized as soon as the item is returned. Here, if a patron is set to Never we're still waiting for the cron to anonymize them. We should anonymize their holds as soon as they move to old_reserves.
Comment 18 Tomás Cohen Arazi 2022-01-12 15:35:06 UTC
(In reply to Andrew Fuerste-Henry from comment #17)
> We're not fully mirroring circ history behavior. For checkout history, if a
> patron is set to Never they get anonymized as soon as the item is returned.
> Here, if a patron is set to Never we're still waiting for the cron to
> anonymize them. We should anonymize their holds as soon as they move to
> old_reserves.

This is getting complex: if we anonymize same-day holds... we are gonna break the max holds per day circ rule calculation!
Comment 19 Tomás Cohen Arazi 2022-01-12 15:49:21 UTC
Ok, the current behavior is that only 'active' holds count towards the max:


    my $today_holds = Koha::Holds->search({
        borrowernumber => $borrowernumber,
        reservedate    => dt_from_string->date
    });

    if (!$params->{ignore_hold_counts} && defined $holds_per_day && $holds_per_day ne ''
        && $today_holds->count() >= $holds_per_day )
    {
        return { status => 'tooManyReservesToday', limit => $holds_per_day };
    }

so moving on with this, with no behavior change on the calculation. Phew.

Should the whole feature have an on/off switch? Something like OPACHoldsPrivacy on/off?
Comment 20 Andrew Fuerste-Henry 2022-01-12 16:15:48 UTC
Right now these sysprefs come into play:

OPACPrivacy determines whether or not patrons can access the My Privacy tab on which these settings are changed.

OPACHoldsHistory determines whether or not patrons can see their hold history, if it is kept. (opacreadinghitory, a separate syspref, determines whether or not they can see checkout history).

intranetreadinghistory determines whether or not staff members can see BOTH checkout and hold history on that staff side, if kept.

I don't feel these *need* changes for this bug to move forward, but in the long run I'd like to see a new IntranetHoldsHistory syspref to split staff interface holds visibility out of intranetreadinghistory.

If we want to give patrons the ability to control holds history without controlling checkout history (or vice versa), I'd be inclined to accomplish that by adding options to OPACPrivacy.
Comment 21 Tomás Cohen Arazi 2022-01-12 19:33:55 UTC
Created attachment 129363 [details] [review]
Bug 29525: Make Koha::Hold->cancel anonymize if required
Comment 22 Tomás Cohen Arazi 2022-01-12 19:49:58 UTC
Created attachment 129364 [details] [review]
Bug 29525: Make Koha::Hold->cancel anonymize if required

This patch makes cancelling a hold anonymize it on the same call, if
settings require it (i.e. if borrowers.privacy is set to 2).

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: The code actually does what it is meant to
3. Try on the UI, cancelling a hold, noticing it gets anonymized if the
   patron has privacy == always/2.
4. Sign off :-D

Note: AnonymousPatron should be set. Otherwise it would set NULL. But
that's fine, that's what Koha does already.
Comment 23 Tomás Cohen Arazi 2022-01-12 19:56:31 UTC
Andrew and testers, this follow-up patch covers the 'cancelling a hold' use case.
Comment 24 Tomás Cohen Arazi 2022-01-13 14:58:04 UTC
Ok, bug adds Koha::Hold->fill with tests, bells and whistles.
I will now make it anonymize the same way we did with Koha:Hold->cancel. And we are done  :-D
Comment 25 Tomás Cohen Arazi 2022-01-13 17:49:19 UTC
Created attachment 129424 [details] [review]
Bug 29525: Add Koha::Old::Holds->filter_by_anonymizable

This patch adds the 'filter_by_anonymizable' method, and tests for it. A
new DBIC relationship is added as well to the OldReserve schema file.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ t/db_dependent/Koha/Old/Holds.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 26 Tomás Cohen Arazi 2022-01-13 17:49:25 UTC
Created attachment 129425 [details] [review]
Bug 29525: Make batch_anonymise.pl deal with holds

This patch makes the batch_anonymise.pl script handle holds too. It does
so by leveraging on the newly introduced method 'filter_by_anonymizable'
and also 'anonymize'.

To test:
1. Have a patron with two past holds.
2. Make sure they are a few days back:
   $ koha-mysql kohadev
   > UPDATE old_reserves SET timestamp=ADDDATE(NOW(), INTERVAL -4 DAY);
3. Run:
   $ kshell
  k$ perl misc/cronjobs/batch_anonymise.pl --days 2 -v
=> SUCCESS: You see something like:
Checkouts and holds before 2022-01-01 will be anonymised.
0 checkouts anonymised.
2 holds anonymised.
4. Repeat 3
=> SUCCESS: They are already anonymized. You see
Checkouts and holds before 2022-01-01 will be anonymised.
0 checkouts anonymised.
0 holds anonymised.
5. Sign off :-D
Comment 27 Tomás Cohen Arazi 2022-01-13 17:49:31 UTC
Created attachment 129426 [details] [review]
Bug 29525: (follow-up) Adapt OPAC text to mention holds
Comment 28 Tomás Cohen Arazi 2022-01-13 17:49:36 UTC
Created attachment 129427 [details] [review]
Bug 29525: Make Koha::Hold->cancel anonymize if required

This patch makes cancelling a hold anonymize it on the same call, if
settings require it (i.e. if borrowers.privacy is set to 2).

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: The code actually does what it is meant to
3. Try on the UI, cancelling a hold, noticing it gets anonymized if the
   patron has privacy == always/2.
4. Sign off :-D

Note: AnonymousPatron should be set. Otherwise it would set NULL. But
that's fine, that's what Koha does already.
Comment 29 Tomás Cohen Arazi 2022-01-13 17:49:42 UTC
Created attachment 129428 [details] [review]
Bug 29525: Make Koha::Hold->fill anonymize if required

This patch makes filling a hold anonymize it on the same call, if
settings require it (i.e. if borrowers.privacy is set to 2).

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: The code actually does what it is meant to
3. Try on the UI, notice it gets anonymized if the patron has privacy == always/2.
4. Sign off :-D

Note: AnonymousPatron should be set. Otherwise it would set NULL. But
that's fine, that's what Koha does already.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Andrew Fuerste-Henry 2022-01-13 19:01:57 UTC
Created attachment 129432 [details] [review]
Bug 29525: Add Koha::Old::Holds->filter_by_anonymizable

This patch adds the 'filter_by_anonymizable' method, and tests for it. A
new DBIC relationship is added as well to the OldReserve schema file.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ t/db_dependent/Koha/Old/Holds.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 31 Andrew Fuerste-Henry 2022-01-13 19:02:00 UTC
Created attachment 129433 [details] [review]
Bug 29525: Make batch_anonymise.pl deal with holds

This patch makes the batch_anonymise.pl script handle holds too. It does
so by leveraging on the newly introduced method 'filter_by_anonymizable'
and also 'anonymize'.

To test:
1. Have a patron with two past holds.
2. Make sure they are a few days back:
   $ koha-mysql kohadev
   > UPDATE old_reserves SET timestamp=ADDDATE(NOW(), INTERVAL -4 DAY);
3. Run:
   $ kshell
  k$ perl misc/cronjobs/batch_anonymise.pl --days 2 -v
=> SUCCESS: You see something like:
Checkouts and holds before 2022-01-01 will be anonymised.
0 checkouts anonymised.
2 holds anonymised.
4. Repeat 3
=> SUCCESS: They are already anonymized. You see
Checkouts and holds before 2022-01-01 will be anonymised.
0 checkouts anonymised.
0 holds anonymised.
5. Sign off :-D

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 32 Andrew Fuerste-Henry 2022-01-13 19:02:04 UTC
Created attachment 129434 [details] [review]
Bug 29525: (follow-up) Adapt OPAC text to mention holds

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 33 Andrew Fuerste-Henry 2022-01-13 19:02:09 UTC
Created attachment 129435 [details] [review]
Bug 29525: Make Koha::Hold->cancel anonymize if required

This patch makes cancelling a hold anonymize it on the same call, if
settings require it (i.e. if borrowers.privacy is set to 2).

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: The code actually does what it is meant to
3. Try on the UI, cancelling a hold, noticing it gets anonymized if the
   patron has privacy == always/2.
4. Sign off :-D

Note: AnonymousPatron should be set. Otherwise it would set NULL. But
that's fine, that's what Koha does already.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 34 Andrew Fuerste-Henry 2022-01-13 19:02:13 UTC
Created attachment 129436 [details] [review]
Bug 29525: Make Koha::Hold->fill anonymize if required

This patch makes filling a hold anonymize it on the same call, if
settings require it (i.e. if borrowers.privacy is set to 2).

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: The code actually does what it is meant to
3. Try on the UI, notice it gets anonymized if the patron has privacy == always/2.
4. Sign off :-D

Note: AnonymousPatron should be set. Otherwise it would set NULL. But
that's fine, that's what Koha does already.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 35 Martin Renvoize 2022-01-17 17:17:09 UTC
hmm.. 'Make Koha::Hold->cancel' claims there's nothing to apply here
Comment 36 Tomás Cohen Arazi 2022-01-17 21:17:34 UTC
Created attachment 129544 [details] [review]
Bug 29525: Make Koha::Hold->cancel anonymize if required

This patch makes cancelling a hold anonymize it on the same call, if
settings require it (i.e. if borrowers.privacy is set to 2).

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: The code actually does what it is meant to
3. Try on the UI, cancelling a hold, noticing it gets anonymized if the
   patron has privacy == always/2.
4. Sign off :-D

Note: AnonymousPatron should be set. Otherwise it would set NULL. But
that's fine, that's what Koha does already.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 37 Tomás Cohen Arazi 2022-01-17 21:17:39 UTC
Created attachment 129545 [details] [review]
Bug 29525: Make Koha::Hold->fill anonymize if required

This patch makes filling a hold anonymize it on the same call, if
settings require it (i.e. if borrowers.privacy is set to 2).

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: The code actually does what it is meant to
3. Try on the UI, notice it gets anonymized if the patron has privacy == always/2.
4. Sign off :-D

Note: AnonymousPatron should be set. Otherwise it would set NULL. But
that's fine, that's what Koha does already.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 38 Jonathan Druart 2022-01-19 12:26:53 UTC
Same remarks than bug 29843 comment 30 apply here
"In MarkIssueReturned we crash if AnonymousPatron is not set to a valid borrowernumber. Here we are setting it to NULL. Both behaviours must be identical."
Comment 39 Katrin Fischer 2022-01-20 20:40:41 UTC
(In reply to Andrew Fuerste-Henry from comment #20)
[...]
> I don't feel these *need* changes for this bug to move forward, but in the
> long run I'd like to see a new IntranetHoldsHistory syspref to split staff
> interface holds visibility out of intranetreadinghistory.
> 
> If we want to give patrons the ability to control holds history without
> controlling checkout history (or vice versa), I'd be inclined to accomplish
> that by adding options to OPACPrivacy.

+1 - I think we should file those as separate bug reports.
Comment 40 Tomás Cohen Arazi 2022-02-04 11:08:12 UTC
(In reply to Jonathan Druart from comment #38)
> Same remarks than bug 29843 comment 30 apply here
> "In MarkIssueReturned we crash if AnonymousPatron is not set to a valid
> borrowernumber. Here we are setting it to NULL. Both behaviours must be
> identical."

Moved this bit to bug 30007.
Comment 41 Martin Renvoize 2022-02-08 08:11:44 UTC
Created attachment 130254 [details] [review]
Bug 29525: Add Koha::Old::Holds->filter_by_anonymizable

This patch adds the 'filter_by_anonymizable' method, and tests for it. A
new DBIC relationship is added as well to the OldReserve schema file.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ t/db_dependent/Koha/Old/Holds.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 42 Martin Renvoize 2022-02-08 08:11:49 UTC
Created attachment 130255 [details] [review]
Bug 29525: Make batch_anonymise.pl deal with holds

This patch makes the batch_anonymise.pl script handle holds too. It does
so by leveraging on the newly introduced method 'filter_by_anonymizable'
and also 'anonymize'.

To test:
1. Have a patron with two past holds.
2. Make sure they are a few days back:
   $ koha-mysql kohadev
   > UPDATE old_reserves SET timestamp=ADDDATE(NOW(), INTERVAL -4 DAY);
3. Run:
   $ kshell
  k$ perl misc/cronjobs/batch_anonymise.pl --days 2 -v
=> SUCCESS: You see something like:
Checkouts and holds before 2022-01-01 will be anonymised.
0 checkouts anonymised.
2 holds anonymised.
4. Repeat 3
=> SUCCESS: They are already anonymized. You see
Checkouts and holds before 2022-01-01 will be anonymised.
0 checkouts anonymised.
0 holds anonymised.
5. Sign off :-D

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 43 Martin Renvoize 2022-02-08 08:11:53 UTC
Created attachment 130256 [details] [review]
Bug 29525: (follow-up) Adapt OPAC text to mention holds

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 44 Martin Renvoize 2022-02-08 08:11:58 UTC
Created attachment 130257 [details] [review]
Bug 29525: Make Koha::Hold->cancel anonymize if required

This patch makes cancelling a hold anonymize it on the same call, if
settings require it (i.e. if borrowers.privacy is set to 2).

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: The code actually does what it is meant to
3. Try on the UI, cancelling a hold, noticing it gets anonymized if the
   patron has privacy == always/2.
4. Sign off :-D

Note: AnonymousPatron should be set. Otherwise it would set NULL. But
that's fine, that's what Koha does already.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 45 Martin Renvoize 2022-02-08 08:12:02 UTC
Created attachment 130258 [details] [review]
Bug 29525: Make Koha::Hold->fill anonymize if required

This patch makes filling a hold anonymize it on the same call, if
settings require it (i.e. if borrowers.privacy is set to 2).

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: The code actually does what it is meant to
3. Try on the UI, notice it gets anonymized if the patron has privacy == always/2.
4. Sign off :-D

Note: AnonymousPatron should be set. Otherwise it would set NULL. But
that's fine, that's what Koha does already.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 46 Martin Renvoize 2022-02-08 08:13:17 UTC
All works as described, new methods work and are well tested and the QA scripts are happy.

QA feedback is being addressed in a distinct bug to catch all cases.

Passing QA
Comment 47 Fridolin Somers 2022-02-11 00:37:40 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 48 Fridolin Somers 2022-03-02 18:59:55 UTC
This is important for GDPR, can it be backported to 21.11.x ?