Created attachment 129475 [details] [review]
Bug 29843: Unit tests

This patch adds unit tests for the introduced methods.

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

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

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

Created attachment 129479 [details] [review]
Bug 29843: Use in tools/cleanborrowers.pl

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

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>

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>

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>

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>

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>

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 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.

(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.

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>

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>

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>

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>

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>

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

(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.

+my $patron = Koha::Patrons->find( $borrowernumber );;

Trailing semi-colon here.

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>

(In reply to Jonathan Druart from comment #40)
> +my $patron = Koha::Patrons->find( $borrowernumber );;
> 
> Trailing semi-colon here.

Good catch, fixed!

Pushed to master for 22.05, thanks to everybody involved ﷐[U+1F984]﷑