Bug 9942: Make Koha fails if privacy is not respected

If a patron has requested anonymity on returning items and the system is
not correctly configured (AnonymousPatron no set or set to an inexistent
patron), the application should take it into account and not fail
quietly.

This patch is quite radical: the script will die loudly if the privacy
is not respected.

To be care of the bad "Software error", some checks are done in the
updatedatabase to be sure the admin will be warned is something is wrong
in the configuration.

Test plan:
1/ Test the updatedatabase entry:
a. Turn on OPACPrivacy and set AnonymousPatron to an existing patron
=> You will get a warning
b. Turn on OPACPrivacy and set AnonymousPatron to 0 or ''
=> You will get a warning
c. Turn on OPACPrivacy and set the privacy to 2 (Never) for at least 1 patron
Turn off OPACPrivacy
=> You will get a warning
d. In all other cases you will get no error

2/ Test the interface
a. Turn on OPACPrivacy and set the privacy to 2 (Never) for a patron
b. Now you can turn off OPACPrivacy or keep it on, behavior should be
the same
c. check an item out the patron
d. Check the item in using the check out table
=> fail
e. Check the item in using the Check in tab
=> fail (not gracefully).

Note that the software error could appear on other pages too.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Updatedatabase works as described
On staff, if don't have correct settings for anonymity it's
impossible to check-in (with OPACPrivacy on)
No errors

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index aa4c9fc..de4b299 100644 (file)
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -1899,8 +1899,18 @@ sub AddReturn {
                 }
             }
 
-            MarkIssueReturned( $borrowernumber, $item->{'itemnumber'},
-                $circControlBranch, $return_date, $borrower->{'privacy'} );
+            eval {
+                MarkIssueReturned( $borrowernumber, $item->{'itemnumber'},
+                    $circControlBranch, $return_date, $borrower->{'privacy'} );
+            };
+            if ( $@ ) {
+                $messages->{'Wrongbranch'} = {
+                    Wrongbranch => $branch,
+                    Rightbranch => $message
+                };
+                carp $@;
+                return ( 0, { WasReturned => 0 }, $issue, $borrower );
+            }
 
             # FIXME is the "= 1" right?  This could be the borrower hash.
             $messages->{'WasReturned'} = 1;
@@ -2075,6 +2085,16 @@ routine in C<C4::Accounts>.
 sub MarkIssueReturned {
     my ( $borrowernumber, $itemnumber, $dropbox_branch, $returndate, $privacy ) = @_;
 
+    my $anonymouspatron;
+    if ( $privacy == 2 ) {
+        # The default of 0 will not work due to foreign key constraints
+        # The anonymisation will fail if AnonymousPatron is not a valid entry
+        # We need to check if the anonymous patron exist, Koha will fail loudly if it does not
+        # Note that a warning should appear on the about page (System information tab).
+        $anonymouspatron = C4::Context->preference('AnonymousPatron');
+        die "Fatal error: the patron ($borrowernumber) has requested a privacy on returning item but the AnonymousPatron pref is not set correctly"
+            unless C4::Members::GetMember( borrowernumber => $anonymouspatron );
+    }
     my $dbh   = C4::Context->dbh;
     my $query = 'UPDATE issues SET returndate=';
     my @bind;
@@ -2100,10 +2120,6 @@ sub MarkIssueReturned {
     $sth_copy->execute($borrowernumber, $itemnumber);
     # anonymise patron checkout immediately if $privacy set to 2 and AnonymousPatron is set to a valid borrowernumber
     if ( $privacy == 2) {
-        # The default of 0 does not work due to foreign key constraints
-        # The anonymisation will fail quietly if AnonymousPatron is not a valid entry
-        # FIXME the above is unacceptable - bug 9942 relates
-        my $anonymouspatron = (C4::Context->preference('AnonymousPatron')) ? C4::Context->preference('AnonymousPatron') : 0;
         my $sth_ano = $dbh->prepare("UPDATE old_issues SET borrowernumber=?
                                   WHERE borrowernumber = ?
                                   AND itemnumber = ?");

diff --git a/installer/data/mysql/updatedatabase.pl b/installer/data/mysql/updatedatabase.pl
index 214faa3..5ad9a28 100755 (executable)
--- a/installer/data/mysql/updatedatabase.pl
+++ b/installer/data/mysql/updatedatabase.pl
@@ -10641,6 +10641,39 @@ if ( CheckVersion($DBversion) ) {
     SetVersion($DBversion);
 }
 
+$DBversion = "3.21.00.XXX";
+if ( CheckVersion($DBversion) ) {
+    my $msg;
+    if ( C4::Context->preference('OPACPrivacy') ) {
+        if ( my $anonymous_patron = C4::Context->preference('AnonymousPatron') ) {
+            my $anonymous_patron_exists = $dbh->selectcol_arrayref(q|
+                SELECT COUNT(*)
+                FROM borrowers
+                WHERE borrowernumber=?
+            |, {}, $anonymous_patron);
+            unless ( $anonymous_patron_exists->[0] ) {
+                $msg = "Configuration WARNING: OPACPrivacy is set but AnonymousPatron is not linked to an existing patron";
+            }
+        }
+        else {
+            $msg = "Configuration WARNING: OPACPrivacy is set but AnonymousPatron is not";
+        }
+    }
+    else {
+        my $patrons_have_required_anonymity = $dbh->selectcol_arrayref(q|
+            SELECT COUNT(*)
+            FROM borrowers
+            WHERE privacy = 2
+        |, {} );
+        if ( $patrons_have_required_anonymity->[0] ) {
+            $msg = "Configuration WARNING: OPACPrivacy is not set but $patrons_have_required_anonymity->[0] patrons have required anonymity (perhaps in a previous configuration). You should fix that asap.";
+        }
+    }
+
+    $msg //= "Privacy is correctly set";
+    print "Upgrade to $DBversion done (Bug 9942: $msg)\n";
+    SetVersion ($DBversion);
+}
 
 # DEVELOPER PROCESS, search for anything to execute in the db_update directory
 # SEE bug 13068

diff --git a/t/db_dependent/Circulation/MarkIssueReturned.t b/t/db_dependent/Circulation/MarkIssueReturned.t
new file mode 100644 (file)
index 0000000..1409fc7
--- /dev/null
+++ b/t/db_dependent/Circulation/MarkIssueReturned.t
@@ -0,0 +1,55 @@
+#!/usr/bin/perl
+
+# This file is part of Koha.
+#
+# Koha is free software; you can redistribute it and/or modify it
+# under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# Koha is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Koha; if not, see <http://www.gnu.org/licenses>.
+
+use Modern::Perl;
+
+use Test::More tests => 2;
+use Test::Warn;
+
+use C4::Branch;
+use C4::Circulation;
+use C4::Members;
+use t::lib::Mocks;
+
+my $dbh = C4::Context->dbh;
+$dbh->{AutoCommit} = 0;
+$dbh->{RaiseError} = 1;
+
+t::lib::Mocks::mock_preference('AnonymousPatron', '');
+
+my $branchcode = 'B';
+ModBranch({ add => 1, branchcode => $branchcode, branchname => 'Branch' });
+
+my $categorycode = 'C';
+$dbh->do("INSERT INTO categories(categorycode) VALUES(?)", undef, $categorycode);
+
+my %item_branch_infos = (
+    homebranch => $branchcode,
+    holdingbranch => $branchcode,
+);
+
+my $borrowernumber = AddMember( categorycode => $categorycode, branchcode => $branchcode );
+
+eval { C4::Circulation::MarkIssueReturned( $borrowernumber, 'itemnumber', 'dropbox_branch', 'returndate', 2 ) };
+like ( $@, qr<Fatal error: the patron \(\d+\) .* AnonymousPatron>, );
+
+my $anonymous_borrowernumber = AddMember( categorycode => $categorycode, branchcode => $branchcode );
+t::lib::Mocks::mock_preference('AnonymousPatron', $anonymous_borrowernumber);
+# The next call will raise an error, because data are not correctly set
+$dbh->{PrintError} = 0;
+eval { C4::Circulation::MarkIssueReturned( $borrowernumber, 'itemnumber', 'dropbox_branch', 'returndate', 2 ) };
+unlike ( $@, qr<Fatal error: the patron \(\d+\) .* AnonymousPatron>, );