Bug 6506 - deleting patron history missing errors
Summary: deleting patron history missing errors
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical (vote)
Assignee: Peter Kelly
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks: 9944 10165
  Show dependency treegraph
 
Reported: 2011-06-15 15:37 UTC by Nicole C. Engard
Modified: 2019-06-27 09:24 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Patch to implement fix (6.79 KB, patch)
2013-03-28 08:10 UTC, Peter Kelly
Details | Diff | Splinter Review
Bug 6506: When AnonymousPatron not set, deletion of issue history silently failed. (6.87 KB, patch)
2013-04-02 08:46 UTC, Mason James
Details | Diff | Splinter Review
Bug 6506: Followup add warning in the system information tab. (3.25 KB, patch)
2013-04-09 10:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Correct CSS name (1.32 KB, patch)
2013-04-09 19:33 UTC, Peter Kelly
Details | Diff | Splinter Review
Bug 6506: When AnonymousPatron not set, deletion of issue history silently failed. (6.88 KB, patch)
2013-04-12 13:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6506: Followup add warning in the system information tab. (3.30 KB, patch)
2013-04-12 13:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6506: Correct CSS class name. (1.38 KB, patch)
2013-04-12 13:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6506: Followup add warning in the system information tab. (3.44 KB, patch)
2013-04-27 13:59 UTC, Mason James
Details | Diff | Splinter Review
Bug 6506: When AnonymousPatron not set, deletion of issue history silently failed. (6.94 KB, patch)
2013-04-27 14:01 UTC, Mason James
Details | Diff | Splinter Review
Bug 6506: Correct CSS class name. (1.51 KB, patch)
2013-04-27 14:03 UTC, Mason James
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2011-06-15 15:37:56 UTC
I'm not sure what category to put this in. If you don't set up an anonymous patron you don't get errors in the OPAC when using my privacy (which requires this patron). 

Without AnonymousPatron set, go to the OPAC and click 'immediate deletion' on your privacy tab to delete your reading history.  You are told that your history is deleted, but the history is actually still showing. I'm assuming that all of the privacy settings are not working if that patron isn't set and there should be some kind of error message explaining that instead of making the patron think that the feature works.
Comment 1 Robin Sheat 2012-08-30 13:03:47 UTC
The code (C4::Circulation->AnonymiseIssueHistory) says:

# The anonymisation will fail quietly if AnonymousPatron is not a valid entry

which is terrible. It should fail loudly.
Comment 2 Peter Kelly 2013-03-25 04:43:14 UTC
Reproduced the bug on current origin/master (3.12 ish)
Comment 3 Peter Kelly 2013-03-26 18:59:37 UTC
Planned new error message for the privacy page:

"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 library of this error."

New bug to be raised for the history-not-deleted-on-return case.
Comment 4 Peter Kelly 2013-03-26 19:46:13 UTC
My plan for implementing the fix for this issue is:
- in Circulation.pm change AnonymiseIssueHistory so that it returns ($rows, $err_history_not_deleted) instead of $rows
- consequential change to misc/cronjobs/batch_anonymise.pl to handle updated return value
- ditto tools/cleanborrowers.pl
- update opac-privacy.pl to check return value and display error
- update opac-privacy.tt to display error if appropriate
Comment 5 Peter Kelly 2013-03-28 08:10:17 UTC Comment hidden (obsolete)
Comment 6 Peter Kelly 2013-03-28 08:11:23 UTC
Remedied by:
- in Circulation.pm changing AnonymiseIssueHistory so that it returns ($rows, $err_history_not_deleted) instead of $rows
- consequential change to misc/cronjobs/batch_anonymise.pl to handle updated return value, and fail if there is an error
- consequential change to tools/cleanborrowers.pl although this still fails silently (raised as bug 9944)
- update of opac-privacy.pl to check return value and pass on error
- update of opac-privacy.tt to display error if appropriate

To test:
1. OPAC
- enable privacy mode (preference OpacPrivacy)
- leave anonymous patron set to zero (preference AnonymousPatron)
- attempt to delete user history
- observe error
- check history - still there
- change anonymous patron to a valid user
- attempt to delete user history
- observe success message
- check history - gone

2. cleanborrowers.pl
- test it functions as before.  bug 9944 has been raised for it continuing to silently fail.

3. batch_anonymise.pl
- enable privacy mode (preference OpacPrivacy)
- leave anonymous patron set to zero (preference AnonymousPatron)
- run script (I use --days -1 for testing)
- script should fail with a Carp message
- change anonymous patron to a valid user
- run script as before
- script returns quietly
- check history - gone
Comment 7 Katrin Fischer 2013-03-29 08:46:39 UTC
Can we get a sign-off on this one please?
Comment 8 Mason James 2013-04-02 08:46:53 UTC Comment hidden (obsolete)
Comment 9 Mason James 2013-04-02 08:49:16 UTC
(In reply to comment #8)
> Created attachment 17113 [details] [review] [review]
> Bug 6506: When AnonymousPatron not set, deletion of issue history silently
> failed.
> 
> Signed-off-by: Mason James <mtj@kohaaloha.com>

tested patch on master - works as advertised for me, so signing-off

nice one Peter :)
Comment 10 Jonathan Druart 2013-04-09 10:12:32 UTC
QA comment:

1/ In C4::Circ::AnonymiseIssueHistory: why don't return the error after checking the syspref's values?
The query will raise an error only if AnonymousPatron=0 and OpacPrivacy=1 isn't it?

2/ Maybe it could be better to raise an error (die) and catch it (eval) instead of to return another variable which should be checked for each call.

3/ There is no css class error (the error is displayed without css style).

1 and 2 for discussion.
Marked as Failed QA for 3.
Comment 11 Jonathan Druart 2013-04-09 10:12:56 UTC Comment hidden (obsolete)
Comment 12 Peter Kelly 2013-04-09 19:33:08 UTC Comment hidden (obsolete)
Comment 13 Peter Kelly 2013-04-09 19:35:35 UTC
I've attached a patch for the mandatory-fix QA feedback.

I'll socialise the others with some people in my timezone since I can't catch Joubu online easily.

It seems to me that the warning patch, while a good idea, should be on its own bug.
Comment 14 Kyle M Hall 2013-04-12 13:16:57 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2013-04-12 13:17:06 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2013-04-12 13:17:16 UTC Comment hidden (obsolete)
Comment 17 Mason James 2013-04-27 13:59:49 UTC
Created attachment 17753 [details] [review]
Bug 6506: Followup add warning in the system information tab.

Add a warning in the system information tab of the about page.

Test plan:
Set OPACPrivacy = 1 and AnonymousPatron = 0 and go on the about page,
tab 'system information'. Check that the warning appears.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>

http://bugs.koha-community.org/show_bug.cgi?id=6509
Comment 18 Mason James 2013-04-27 14:01:02 UTC
Created attachment 17754 [details] [review]
Bug 6506: When AnonymousPatron not set, deletion of issue history silently failed.

Remedied by:
- in Circulation.pm changing AnonymiseIssueHistory so that it returns ($rows, $err_history_not_deleted) instead of $rows
- consequential change to misc/cronjobs/batch_anonymise.pl to handle updated return value, and fail if there is an error
- consequential change to tools/cleanborrowers.pl although this still fails silently (raised as bug 9944)
- update of opac-privacy.pl to check return value and pass on error
- update of opac-privacy.tt to display error if appropriate

Note bug 9942 remains unfixed, which is a similar issue upon issue return.

To test:
1. OPAC
- enable privacy mode (preference OpacPrivacy)
- leave anonymous patron set to zero (preference AnonymousPatron)
- attempt to delete user history
- observe error
- check history - still there
- change anonymous patron to a valid user
- attempt to delete user history
- observe success message
- check history - gone

2. cleanborrowers.pl
- test it functions as before.  bug 9944 has been raised for it continuing to silently fail.

3. batch_anonymise.pl
- enable privacy mode (preference OpacPrivacy)
- leave anonymous patron set to zero (preference AnonymousPatron)
- run script (I use --days -1 for testing)
- script should fail with a Carp message
- change anonymous patron to a valid user
- run script as before
- script returns quietly
- check history - gone

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 19 Mason James 2013-04-27 14:03:38 UTC
Created attachment 17755 [details] [review]
Bug 6506: Correct CSS class name.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>

http://bugs.koha-community.org/show_bug.cgi?id=6509
Comment 20 Mason James 2013-04-27 14:08:18 UTC
Kyle has now signed-off on all 3 patches

I am now passing QA on them
Comment 21 Jared Camins-Esakov 2013-05-01 12:43:55 UTC
With DEBUG turned on, having AnonymousPatron blank still causes a fatal error, but I will report a separate bug for that, and push this as-is, since it helps when DEBUG is off.
Comment 22 Jared Camins-Esakov 2013-05-01 12:44:49 UTC
This patch has been pushed to master and 3.12.x.
Comment 23 Chris Cormack 2013-05-23 09:15:19 UTC
Released with 3.12.0