Bugzilla – Attachment 42493 Details for
Bug 6756
Patron anonymizing tool reports false success in anonymous patron not defined
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
[PASSED QA] Bug 6756: Fix bad behaviors if AnonymousPatron is not defined
PASSED-QA-Bug-6756-Fix-bad-behaviors-if-AnonymousP.patch (text/plain), 7.95 KB, created by
Katrin Fischer
on 2015-09-13 15:06:46 UTC
(
hide
)
Description:
[PASSED QA] Bug 6756: Fix bad behaviors if AnonymousPatron is not defined
Filename:
MIME Type:
Creator:
Katrin Fischer
Created:
2015-09-13 15:06:46 UTC
Size:
7.95 KB
patch
obsolete
>From a8fbadbfe70a3dd81ff42c340e277cbb1b38e184 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Tue, 14 Jul 2015 11:30:51 +0100 >Subject: [PATCH] [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> >--- > C4/Circulation.pm | 5 +- > t/db_dependent/Circulation/AnonymiseIssueHistory.t | 138 +++++++++++++++++++++ > 2 files changed, 141 insertions(+), 2 deletions(-) > create mode 100644 t/db_dependent/Circulation/AnonymiseIssueHistory.t > >diff --git a/C4/Circulation.pm b/C4/Circulation.pm >index 4ddd75f..4ea6f05 100644 >--- a/C4/Circulation.pm >+++ b/C4/Circulation.pm >@@ -3214,8 +3214,9 @@ sub AnonymiseIssueHistory { > "; > > # The default of 0 does not work due to foreign key constraints >- # The anonymisation will fail quietly if AnonymousPatron is not a valid entry >- my $anonymouspatron = (C4::Context->preference('AnonymousPatron')) ? C4::Context->preference('AnonymousPatron') : 0; >+ # The anonymisation should not fail quietly if AnonymousPatron is not a valid entry >+ # Set it to undef (NULL) >+ my $anonymouspatron = C4::Context->preference('AnonymousPatron') || undef; > my @bind_params = ($anonymouspatron, $date); > if (defined $borrowernumber) { > $query .= " AND borrowernumber = ?"; >diff --git a/t/db_dependent/Circulation/AnonymiseIssueHistory.t b/t/db_dependent/Circulation/AnonymiseIssueHistory.t >new file mode 100644 >index 0000000..213be0b >--- /dev/null >+++ b/t/db_dependent/Circulation/AnonymiseIssueHistory.t >@@ -0,0 +1,138 @@ >+use Modern::Perl; >+use Test::More tests => 3; >+ >+use C4::Context; >+use C4::Circulation; >+use t::lib::Mocks; >+use t::lib::TestBuilder; >+ >+my $builder = t::lib::TestBuilder->new; >+ >+# TODO create a subroutine in t::lib::Mocks >+my $userenv_patron = $builder->build( { source => 'Borrower', }, ); >+C4::Context->_new_userenv('DUMMY SESSION'); >+C4::Context->set_userenv( >+ $userenv_patron->{borrowernumber}, >+ $userenv_patron->{userid}, >+ 'usercnum', 'First name', 'Surname', >+ $userenv_patron->{_fk}{branchcode}{branchcode}, >+ $userenv_patron->{_fk}{branchcode}{branchname}, 0 >+); >+ >+my $anonymous = $builder->build( { source => 'Borrower', }, ); >+ >+t::lib::Mocks::mock_preference( 'AnonymousPatron', $anonymous->{borrowernumber} ); >+ >+subtest 'patron privacy is 1 (default)' => sub { >+ plan tests => 4; >+ my $patron = $builder->build( >+ { source => 'Borrower', >+ value => { privacy => 1, } >+ } >+ ); >+ my $item = $builder->build( >+ { source => 'Item', >+ value => { >+ itemlost => 0, >+ withdrawn => 0, >+ }, >+ } >+ ); >+ my $issue = $builder->build( >+ { source => 'Issue', >+ value => { >+ borrowernumber => $patron->{borrowernumber}, >+ itemnumber => $item->{itemnumber}, >+ }, >+ } >+ ); >+ >+ my ( $returned, undef, undef ) = C4::Circulation::AddReturn( $item->{barcode}, undef, undef, undef, '2010-10-10' ); >+ is( $returned, 1, 'The item should have been returned' ); >+ my ( $rows_affected, $err ) = C4::Circulation::AnonymiseIssueHistory('2010-10-11'); >+ ok( $rows_affected > 0, 'AnonymiseIssueHistory should affect at least 1 row' ); >+ is( $err, undef, 'AnonymiseIssueHistory should not return any error if success' ); >+ >+ my $dbh = C4::Context->dbh; >+ my ($borrowernumber_used_to_anonymised) = $dbh->selectrow_array(q| >+ SELECT borrowernumber FROM old_issues where itemnumber = ? >+ |, undef, $item->{itemnumber}); >+ is( $borrowernumber_used_to_anonymised, $anonymous->{borrowernumber}, 'With privacy=1, the issue should have been anonymised' ); >+ >+}; >+ >+subtest 'patron privacy is 0 (forever)' => sub { >+ plan tests => 3; >+ >+ my $patron = $builder->build( >+ { source => 'Borrower', >+ value => { privacy => 0, } >+ } >+ ); >+ my $item = $builder->build( >+ { source => 'Item', >+ value => { >+ itemlost => 0, >+ withdrawn => 0, >+ }, >+ } >+ ); >+ my $issue = $builder->build( >+ { source => 'Issue', >+ value => { >+ borrowernumber => $patron->{borrowernumber}, >+ itemnumber => $item->{itemnumber}, >+ }, >+ } >+ ); >+ >+ my ( $returned, undef, undef ) = C4::Circulation::AddReturn( $item->{barcode}, undef, undef, undef, '2010-10-10' ); >+ is( $returned, 1, 'The item should have been returned' ); >+ my ( $rows_affected, $err ) = C4::Circulation::AnonymiseIssueHistory('2010-10-11'); >+ is( $err, undef, 'AnonymiseIssueHistory should not return any error if success' ); >+ >+ my $dbh = C4::Context->dbh; >+ my ($borrowernumber_used_to_anonymised) = $dbh->selectrow_array(q| >+ SELECT borrowernumber FROM old_issues where itemnumber = ? >+ |, undef, $item->{itemnumber}); >+ is( $borrowernumber_used_to_anonymised, $patron->{borrowernumber}, 'With privacy=0, the issue should not be anonymised' ); >+}; >+ >+t::lib::Mocks::mock_preference( 'AnonymousPatron', '' ); >+ >+subtest 'AnonymousPatron is not defined' => sub { >+ plan tests => 4; >+ my $patron = $builder->build( >+ { source => 'Borrower', >+ value => { privacy => 1, } >+ } >+ ); >+ my $item = $builder->build( >+ { source => 'Item', >+ value => { >+ itemlost => 0, >+ withdrawn => 0, >+ }, >+ } >+ ); >+ my $issue = $builder->build( >+ { source => 'Issue', >+ value => { >+ borrowernumber => $patron->{borrowernumber}, >+ itemnumber => $item->{itemnumber}, >+ }, >+ } >+ ); >+ >+ my ( $returned, undef, undef ) = C4::Circulation::AddReturn( $item->{barcode}, undef, undef, undef, '2010-10-10' ); >+ is( $returned, 1, 'The item should have been returned' ); >+ my ( $rows_affected, $err ) = C4::Circulation::AnonymiseIssueHistory('2010-10-11'); >+ ok( $rows_affected > 0, 'AnonymiseIssueHistory should affect at least 1 row' ); >+ is( $err, undef, 'AnonymiseIssueHistory should not return any error if success' ); >+ >+ my $dbh = C4::Context->dbh; >+ my ($borrowernumber_used_to_anonymised) = $dbh->selectrow_array(q| >+ SELECT borrowernumber FROM old_issues where itemnumber = ? >+ |, undef, $item->{itemnumber}); >+ is( $borrowernumber_used_to_anonymised, undef, 'With AnonymousPatron is not defined, the issue should have been anonymised anyway' ); >+}; >-- >1.9.1
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 6756
:
40985
|
40986
|
41184
|
41185
| 42493 |
42494