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.
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.
Reproduced the bug on current origin/master (3.12 ish)
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.
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
Created attachment 16955 [details] [review] Patch to implement fix
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
Can we get a sign-off on this one please?
Created attachment 17113 [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: Mason James <mtj@kohaaloha.com>
(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 :)
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.
Created attachment 17305 [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.
Created attachment 17317 [details] [review] Correct CSS name This corrects the mandatory item of QA feedback.
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.
Created attachment 17385 [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: Mason James <mtj@kohaaloha.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 17386 [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>
Created attachment 17387 [details] [review] Bug 6506: Correct CSS class name. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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
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>
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
Kyle has now signed-off on all 3 patches I am now passing QA on them
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.
This patch has been pushed to master and 3.12.x.
Released with 3.12.0