Bug 29843

Summary: Add ->anonymize and ->filter_by_anonymizable to Koha::Old::Checkouts
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: RESOLVED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: enhancement    
Priority: P5 - low CC: andrew, jonathan.druart+koha, kyle, martin.renvoize, nick, tomascohen
Version: unspecified   
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: 29526    
Bug Blocks: 30042, 29984, 30007    
Attachments: Bug 29843: Unit tests
Bug 29843: Add ->anonymize and ->filter_by_anonymizable to Koha::Old::Checkouts
Bug 29843: Use in batch_anonymise.pl
Bug 29843: Use in opac/opac-privacy.pl
Bug 29843: Unit tests
Bug 29843: Add ->anonymize and ->filter_by_anonymizable to Koha::Old::Checkouts
Bug 29843: Use in batch_anonymise.pl
Bug 29843: Use in opac/opac-privacy.pl
Bug 29843: Use in tools/cleanborrowers.pl
Bug 29843: Unit tests
Bug 29843: Add ->anonymize and ->filter_by_anonymizable to Koha::Old::Checkouts
Bug 29843: Use in batch_anonymise.pl
Bug 29843: Use in opac/opac-privacy.pl
Bug 29843: Use in tools/cleanborrowers.pl
Bug 29843: Unit tests
Bug 29843: Add ->anonymize and ->filter_by_anonymizable to Koha::Old::Checkouts
Bug 29843: Use in batch_anonymise.pl
Bug 29843: Use in opac/opac-privacy.pl
Bug 29843: Use in tools/cleanborrowers.pl
Bug 29843: Unit tests
Bug 29843: Add ->anonymize and ->filter_by_anonymizable to Koha::Old::Checkouts
Bug 29843: Use in batch_anonymise.pl
Bug 29843: Use in opac/opac-privacy.pl
Bug 29843: Use in tools/cleanborrowers.pl
Bug 29843: Unit tests
Bug 29843: Add ->anonymize and ->filter_by_anonymizable to Koha::Old::Checkouts
Bug 29843: Use in batch_anonymise.pl
Bug 29843: Use in opac/opac-privacy.pl
Bug 29843: Use in tools/cleanborrowers.pl
Bug 29843: Unit tests
Bug 29843: Add ->anonymize and ->filter_by_anonymizable to Koha::Old::Checkouts
Bug 29843: Use in batch_anonymise.pl
Bug 29843: Use in opac/opac-privacy.pl
Bug 29843: Use in tools/cleanborrowers.pl
Bug 29843: Use in opac/opac-privacy.pl

Description Tomás Cohen Arazi 2022-01-10 14:21:11 UTC
This bug is the checkouts counterpart for bug 29525 and bug 29780.
The pattern implemented there seems cleaner than the current implementation, so giving it a try so we clean things even further.
Comment 1 Tomás Cohen Arazi 2022-01-10 14:33:41 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2022-01-10 14:33:45 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2022-01-10 14:33:50 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2022-01-10 14:51:18 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2022-01-10 15:19:51 UTC Comment hidden (obsolete)
Comment 6 Tomás Cohen Arazi 2022-01-10 15:19:55 UTC Comment hidden (obsolete)
Comment 7 Tomás Cohen Arazi 2022-01-10 15:19:59 UTC Comment hidden (obsolete)
Comment 8 Tomás Cohen Arazi 2022-01-10 15:20:04 UTC Comment hidden (obsolete)
Comment 9 Tomás Cohen Arazi 2022-01-10 15:20:11 UTC Comment hidden (obsolete)
Comment 10 Tomás Cohen Arazi 2022-01-13 19:21:51 UTC Comment hidden (obsolete)
Comment 11 Tomás Cohen Arazi 2022-01-13 19:21:57 UTC Comment hidden (obsolete)
Comment 12 Tomás Cohen Arazi 2022-01-13 19:22:05 UTC Comment hidden (obsolete)
Comment 13 Tomás Cohen Arazi 2022-01-13 19:22:12 UTC Comment hidden (obsolete)
Comment 14 Tomás Cohen Arazi 2022-01-13 19:22:18 UTC Comment hidden (obsolete)
Comment 15 Tomás Cohen Arazi 2022-01-13 19:51:58 UTC Comment hidden (obsolete)
Comment 16 Tomás Cohen Arazi 2022-01-13 19:52:04 UTC Comment hidden (obsolete)
Comment 17 Tomás Cohen Arazi 2022-01-13 19:52:13 UTC Comment hidden (obsolete)
Comment 18 Tomás Cohen Arazi 2022-01-13 19:52:21 UTC Comment hidden (obsolete)
Comment 19 Tomás Cohen Arazi 2022-01-13 19:52:30 UTC Comment hidden (obsolete)
Comment 20 Tomás Cohen Arazi 2022-01-14 13:01:39 UTC
Created attachment 129475 [details] [review]
Bug 29843: Unit tests

This patch adds unit tests for the introduced methods.
Comment 21 Tomás Cohen Arazi 2022-01-14 13:01:43 UTC
Created attachment 129476 [details] [review]
Bug 29843: Add ->anonymize and ->filter_by_anonymizable to Koha::Old::Checkouts

This patch implements high-level methods for:

* Filtering a Koha::Old::Checkouts resultset so only anonymizable rows
  are left
* Anonymizing the rows in a resultset

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Old/Checkouts.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 22 Tomás Cohen Arazi 2022-01-14 13:01:49 UTC
Created attachment 129477 [details] [review]
Bug 29843: Use in batch_anonymise.pl

This patch makes the batch_anonymise.pl cronjob script use the newly
introduced methods instead of the old ones.

To test:
1. Try the tool
=> SUCCESS: No behavior change
2. Sign off :-D
Comment 23 Tomás Cohen Arazi 2022-01-14 13:01:54 UTC
Created attachment 129478 [details] [review]
Bug 29843: Use in opac/opac-privacy.pl

This patch makes the opac/opac-privacy.pl OPAC page use the new
anonymize method.

To test:
1. Have some checked-in materiales
2. Have OPACPrivacy enabled
3. Notice your checkouts history contains what you expect
4. Go to 'your privacy'
5. Click on 'Delete checkout history'
=> SUCCESS: It works, no crash.
6. Sign off :-D
Comment 24 Tomás Cohen Arazi 2022-01-14 13:01:58 UTC
Created attachment 129479 [details] [review]
Bug 29843: Use in tools/cleanborrowers.pl

This patch makes tools/cleanborrowers.pl use the new methods.
Comment 25 Andrew Fuerste-Henry 2022-01-14 14:57:11 UTC
Created attachment 129492 [details] [review]
Bug 29843: Unit tests

This patch adds unit tests for the introduced methods.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 26 Andrew Fuerste-Henry 2022-01-14 14:57:16 UTC
Created attachment 129493 [details] [review]
Bug 29843: Add ->anonymize and ->filter_by_anonymizable to Koha::Old::Checkouts

This patch implements high-level methods for:

* Filtering a Koha::Old::Checkouts resultset so only anonymizable rows
  are left
* Anonymizing the rows in a resultset

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

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 27 Andrew Fuerste-Henry 2022-01-14 14:57:20 UTC
Created attachment 129494 [details] [review]
Bug 29843: Use in batch_anonymise.pl

This patch makes the batch_anonymise.pl cronjob script use the newly
introduced methods instead of the old ones.

To test:
1. Try the tool
=> SUCCESS: No behavior change
2. Sign off :-D

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 28 Andrew Fuerste-Henry 2022-01-14 14:57:24 UTC
Created attachment 129495 [details] [review]
Bug 29843: Use in opac/opac-privacy.pl

This patch makes the opac/opac-privacy.pl OPAC page use the new
anonymize method.

To test:
1. Have some checked-in materiales
2. Have OPACPrivacy enabled
3. Notice your checkouts history contains what you expect
4. Go to 'your privacy'
5. Click on 'Delete checkout history'
=> SUCCESS: It works, no crash.
6. Sign off :-D

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 29 Andrew Fuerste-Henry 2022-01-14 14:57:28 UTC
Created attachment 129496 [details] [review]
Bug 29843: Use in tools/cleanborrowers.pl

This patch makes tools/cleanborrowers.pl use the new methods.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 30 Jonathan Druart 2022-01-19 12:23:57 UTC
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 31 Tomás Cohen Arazi 2022-01-31 19:38:20 UTC
(In reply to Jonathan Druart from comment #30)
> 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.

I agree with this proposal. I had doubts myself. My approach was to not change the behavior and I was using (basically) what is done in Koha::Patrons->anonymise_issue_history. That's where I picked the fallback to NULL idea from.

I would keep things like this on this bug tree, so testers can verify no behavior change takes place. And will happily file a bug for this proposed behavior change, along with removing the (now) unused anonymise_issue_history method.

With this fully tested and reused single place piece of code, the tweak will be trivial to implement. Let's get this through QA.
Comment 32 Tomás Cohen Arazi 2022-02-04 11:15:49 UTC
(In reply to Jonathan Druart from comment #30)
> 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 33 Martin Renvoize 2022-02-08 09:39:57 UTC
Created attachment 130264 [details] [review]
Bug 29843: Unit tests

This patch adds unit tests for the introduced methods.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Martin Renvoize 2022-02-08 09:40:01 UTC
Created attachment 130265 [details] [review]
Bug 29843: Add ->anonymize and ->filter_by_anonymizable to Koha::Old::Checkouts

This patch implements high-level methods for:

* Filtering a Koha::Old::Checkouts resultset so only anonymizable rows
  are left
* Anonymizing the rows in a resultset

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Old/Checkouts.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 35 Martin Renvoize 2022-02-08 09:40:06 UTC
Created attachment 130266 [details] [review]
Bug 29843: Use in batch_anonymise.pl

This patch makes the batch_anonymise.pl cronjob script use the newly
introduced methods instead of the old ones.

To test:
1. Try the tool
=> SUCCESS: No behavior change
2. Sign off :-D

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 36 Martin Renvoize 2022-02-08 09:40:11 UTC
Created attachment 130267 [details] [review]
Bug 29843: Use in opac/opac-privacy.pl

This patch makes the opac/opac-privacy.pl OPAC page use the new
anonymize method.

To test:
1. Have some checked-in materiales
2. Have OPACPrivacy enabled
3. Notice your checkouts history contains what you expect
4. Go to 'your privacy'
5. Click on 'Delete checkout history'
=> SUCCESS: It works, no crash.
6. Sign off :-D

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 37 Martin Renvoize 2022-02-08 09:40:15 UTC
Created attachment 130268 [details] [review]
Bug 29843: Use in tools/cleanborrowers.pl

This patch makes tools/cleanborrowers.pl use the new methods.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 38 Martin Renvoize 2022-02-08 09:42:07 UTC
This is a nice improvement, I like the pattern here.

All works well, QA script is happy, and tests coverage is high.

This did get me looking at the cron script, it would be good to add the standard --confirm to this script at some point.. and if possible it would be great to drop the Date::Calc module at some point.

Passing QA
Comment 39 Tomás Cohen Arazi 2022-02-08 10:14:29 UTC
(In reply to Martin Renvoize from comment #38)
> This did get me looking at the cron script, it would be good to add the
> standard --confirm to this script at some point.. and if possible it would
> be great to drop the Date::Calc module at some point.

Yeah, saw that. It is mostly used for displaying purposes. Can do on another report.
Comment 40 Jonathan Druart 2022-02-08 16:56:16 UTC
+my $patron = Koha::Patrons->find( $borrowernumber );;

Trailing semi-colon here.
Comment 41 Tomás Cohen Arazi 2022-02-08 17:59:07 UTC
Created attachment 130335 [details] [review]
Bug 29843: Use in opac/opac-privacy.pl

This patch makes the opac/opac-privacy.pl OPAC page use the new
anonymize method.

To test:
1. Have some checked-in materiales
2. Have OPACPrivacy enabled
3. Notice your checkouts history contains what you expect
4. Go to 'your privacy'
5. Click on 'Delete checkout history'
=> SUCCESS: It works, no crash.
6. Sign off :-D

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 42 Tomás Cohen Arazi 2022-02-08 17:59:57 UTC
(In reply to Jonathan Druart from comment #40)
> +my $patron = Koha::Patrons->find( $borrowernumber );;
> 
> Trailing semi-colon here.

Good catch, fixed!
Comment 43 Fridolin Somers 2022-02-11 00:37:59 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄