Description
Nick Clemens (kidclamp)
2021-11-19 01:15:19 UTC
+1 Created attachment 128975 [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 Created attachment 128976 [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 Created attachment 128977 [details] [review] Bug 29525: (follow-up) Adapt OPAC text to mention holds Created attachment 129258 [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 Created attachment 129259 [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 Created attachment 129260 [details] [review] Bug 29525: (follow-up) Adapt OPAC text to mention holds Sorry for the noise, just learnt about Koha::Objects->filter_by_last_update Created attachment 129265 [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 Created attachment 129266 [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 Created attachment 129267 [details] [review] Bug 29525: (follow-up) Adapt OPAC text to mention holds Created attachment 129268 [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 Created attachment 129273 [details] [review] Bug 29843: Unit tests 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 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 Created attachment 129323 [details] [review] Bug 29525: (follow-up) Adapt OPAC text to mention holds 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. (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! 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? 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. Created attachment 129363 [details] [review] Bug 29525: Make Koha::Hold->cancel anonymize if required 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. Andrew and testers, this follow-up patch covers the 'cancelling a hold' use case. 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 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 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 Created attachment 129426 [details] [review] Bug 29525: (follow-up) Adapt OPAC text to mention holds 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. 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> 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> 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> Created attachment 129434 [details] [review] Bug 29525: (follow-up) Adapt OPAC text to mention holds Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> 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> 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> hmm.. 'Make Koha::Hold->cancel' claims there's nothing to apply here 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> 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> 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." (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. (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. 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> 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> 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> 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> 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> 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 Pushed to master for 22.05, thanks to everybody involved [U+1F984] This is important for GDPR, can it be backported to 21.11.x ? |