|
Description
Nicole C. Engard
2011-06-15 15:37:56 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. 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 |