Bug 6756 - Patron anonymizing tool reports false success in anonymous patron not defined
Summary: Patron anonymizing tool reports false success in anonymous patron not defined
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Bugs List
URL: /cgi-bin/koha/tools/cleanborrowers.pl
Whiteboard:
Keywords:
: 9730 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-08-20 08:38 UTC by Katrin Fischer
Modified: 2016-06-21 21:38 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 6756: Fix bad behaviors if AnonymousPatron is not defined (7.61 KB, patch)
2015-07-14 10:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6756: Add info messages (2.03 KB, patch)
2015-07-14 10:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6756: Fix bad behaviors if AnonymousPatron is not defined (7.88 KB, patch)
2015-07-25 00:15 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6756: Add info messages (2.28 KB, patch)
2015-07-25 00:16 UTC, M. Tompsett
Details | Diff | Splinter Review
[PASSED QA] Bug 6756: Fix bad behaviors if AnonymousPatron is not defined (7.95 KB, patch)
2015-09-13 15:06 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 6756: Add info messages (2.35 KB, patch)
2015-09-13 15:07 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2011-08-20 08:38:36 UTC
Problem: Koha gives you no hint, that you have to set up an anonymous patron (system preference AnonymousPatron) for the anonymization to work correctly. Even worse, if you don't have one, it will still report successfull deletion of patron's history, even though nothing has changed in the old_issues table and no informatoin was deleted or anonymized.

To reproduce:
- Check your AnonymousPatorn system preference is empty
- Go to Tools > Delete some old patrons/Anonymize some check-out histor
- Uncheck 'Delete borrower who has not borrowed since...'
- Select a date near to the present for 'Anonymize check-out history older than...' and 'next'
- Check that it reports more than 0 to be anonymized and 'next'
> Koha reports successful deletion

If you check the old_issues table before and after the process you will see that nothing has changed.

With an anonymous patron set up correctly it works well.
Comment 1 Jonathan Druart 2015-03-27 09:21:29 UTC
What do you suggest?
Comment 2 Katrin Fischer 2015-05-15 22:21:05 UTC
(In reply to Jonathan Druart from comment #1)
> What do you suggest?

I think we should display a message that as no AnonymousPatron has been set up the data couldn't be anonymized. - Stating we deleted something while we didn't is quite evil in this case.
Comment 3 Jonathan Druart 2015-07-14 10:25:44 UTC
*** Bug 9730 has been marked as a duplicate of this bug. ***
Comment 4 Jonathan Druart 2015-07-14 10:39:22 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2015-07-14 10:39:27 UTC Comment hidden (obsolete)
Comment 6 M. Tompsett 2015-07-25 00:15:50 UTC Comment hidden (obsolete)
Comment 7 M. Tompsett 2015-07-25 00:16:01 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2015-09-13 15:06:08 UTC
This works as described, but maybe we could improve some of the messages at some point:

"All patrons with checkouts older than 18/04/2015 have been anonymized"

We are not anonymizing the patrons, but only their checkouts :)
Comment 9 Katrin Fischer 2015-09-13 15:06:46 UTC
Created attachment 42493 [details] [review]
[PASSED QA] Bug 6756: Fix bad behaviors if AnonymousPatron is not defined

There are at least 2 wrong behaviors if the AnonymousPatron pref is not
defined (0 or empty string).
1/ If you use the clean borrower tools, you will get a successful
message when the nothing happened (the history has not been anonymised).
2/ At the OPAC, if a patron ask for delete his reading history, he will
get an error message "The deletion of your reading history failed,
because there is a problem with the configuration of this feature.
Please help to fix the system by informing your libr    ary of this
error". IMO this should not happen, the history should be anonymised.

With this patch, the old_issues.borrowernumber field will be set to NULL
if the AnonymousPatron pref if not defined.

Test plan:
1/ Fill the pref with "" or 0
2/ At the OPAC, go on the privacy tab and click on the "Immedia deletion" button.
You should get a green and friendly message. Confirm that the history
has been anonymised.
3/ Use the "Batch patron anonymization" tools (tools/cleanborrowers.pl)
to anonymize the checkout history.
Confirm that a) it works and b) you get a message.

Try again with AnonymousPatron set to a valid patron. You should not see
any changes with the current behaviors.

NOTE: This patch tweaks C4/Circulation.pm and provides tests.
      applying just this, and running prove success. Reverting just
      C4/Circulation.pm fails, as expected.
      Tested OPAC stuff with both patches applied.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Katrin Fischer 2015-09-13 15:07:03 UTC
Created attachment 42494 [details] [review]
[PASSED QA] Bug 6756: Add info messages

NOTE: Before patch "./misc/cronjobs/batch_anonymise.pl --help" had no
      message, and neither did the anonymizing tool in the staff client.
      After the patch, both had informative messages.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Tomás Cohen Arazi 2015-09-16 14:07:13 UTC
Patches pushed to master.

Thanks Jonathan!
Comment 12 Chris Cormack 2015-09-22 17:46:33 UTC
Pushed to 3.20.x will be in 3.20.4
Comment 13 Liz Rea 2015-09-23 00:23:47 UTC
pushed to 3.18.x will be in 3.18.11
Comment 14 Jonathan Druart 2015-10-19 14:36:59 UTC
Liz,
This has been backported to 3.18, but the tests use the t::lib::TestBuilder lib which has been introduced by bug 12603, not in 3.18 yet.

04:05:47 Can't locate t/lib/TestBuilder.pm in @INC (you may need to install the t::lib::TestBuilder module) (@INC contains: /srv/jenkins/workspace/Koha_3.18.x_U14/blib/arch /srv/jenkins/workspace/Koha_3.18.x_U14/blib/lib /srv/jenkins/workspace/Koha_3.18.x_U14 /etc/perl /usr/local/lib/perl/5.18.2 /usr/local/share/perl/5.18.2 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.18 /usr/share/perl/5.18 /usr/local/lib/site_perl .) at t/db_dependent/Circulation/AnonymiseIssueHistory.t line 7.
Comment 15 Liz Rea 2015-10-19 20:49:50 UTC
Ah thanks I'll pull out the tests I suppose.
Comment 16 Liz Rea 2015-11-02 21:02:16 UTC
Pulled the tests out of 3.18.x for 3.18.12.