From 8f8745ef8c9e748eb27fe09697f39c489fb87ed8 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Tue, 14 Jul 2015 11:30:51 +0100 Subject: [PATCH] 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. --- 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 de4b299..6a089ad 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -3216,8 +3216,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' ); +}; -- 2.1.0