From a0b7acfae15efb6bf120a8b62daf55eff72b56a0 Mon Sep 17 00:00:00 2001
From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Date: Tue, 9 Aug 2016 22:18:14 +0100
Subject: [PATCH] Bug 17097: Add a confirmation page when deleting a patron

It won't hurt to have a confirmation page when deleting a patron.
Moreover it's the more easy way to protect against CSRF attacks :)

Test plan:
Make sure you get a confirmation page when deleting a patron
Confirm that approving or denying the confirmation work as expected

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Mason James <mtj@kohaaloha.com>
---
 .../prog/en/includes/members-toolbar.inc      | 10 +--------
 .../prog/en/modules/members/deletemem.tt      | 14 +++++++++++++
 members/deletemem.pl                          | 21 ++++++++++---------
 3 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/members-toolbar.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/members-toolbar.inc
index af2b1292d4..38d091124e 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/includes/members-toolbar.inc
+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/members-toolbar.inc
@@ -23,9 +23,7 @@ $(document).ready(function(){
             });
         [% ELSE %]
             $("#deletepatron").click(function(){
-                confirm_deletion();
-                $(".btn-group").removeClass("open");
-                return false;
+                window.location='/cgi-bin/koha/members/deletemem.pl?member=[% borrowernumber | url%]';
             });
         [% END %]
         $("#renewpatron").click(function(){
@@ -69,12 +67,6 @@ $(document).ready(function(){
         return false;
     })
 });
-function confirm_deletion() {
-    var is_confirmed = window.confirm(_("Are you sure you want to delete this patron? This cannot be undone."));
-    if (is_confirmed) {
-        window.location='/cgi-bin/koha/members/deletemem.pl?member=[% borrowernumber %]';
-    }
-}
 function confirm_local_deletion() {
     var is_confirmed = window.confirm(_("Are you sure you want to delete this patron from the local database? This cannot be undone."));
     if (is_confirmed) {
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/deletemem.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/deletemem.tt
index 1ba93f391a..3febac64c5 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/deletemem.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/deletemem.tt
@@ -28,6 +28,20 @@
             [% END %]
             </ul>
     </div>
+    [% ELSIF op == 'delete_confirm' and borrowernumber %]
+        [%# TODO add "patron does not exist" unless borrowernumber %]
+        <div class="dialog alert">
+            <h3>Are you sure you want to delete the patron [% firstname %] [% surname %]? This cannot be undone.</h3>
+            <form action="/cgi-bin/koha/members/deletemem.pl">
+                <input type="hidden" name="borrowernumber" value="[% borrowernumber %]"/>
+                <input type="hidden" name="op" value="delete_confirmed" />
+                <button type="submit" class="approve"><i class="fa fa-fw fa-check"></i> Yes, delete</button>
+            </form>
+            <form action="/cgi-bin/koha/members/moremember.pl">
+                <input type="hidden" name="borrowernumber" value="[% borrowernumber %]"/>
+                <button type="submit" class="deny"><i class="fa fa-fw fa-remove"></i> No, do not delete</button>
+            </form>
+        </div>
     [% END %]
     [% IF ( keeplocal ) %]
         <div class="dialog message">
diff --git a/members/deletemem.pl b/members/deletemem.pl
index 6c3b4d2f3f..382259ab2e 100755
--- a/members/deletemem.pl
+++ b/members/deletemem.pl
@@ -101,13 +101,10 @@ if (C4::Context->preference("IndependentBranches")) {
     }
 }
 
+my $op = $input->param('op') || 'delete_confirm';
 my $dbh = C4::Context->dbh;
-my $sth=$dbh->prepare("Select * from borrowers where guarantorid=?");
-$sth->execute($member);
-my $data=$sth->fetchrow_hashref;
-if ($countissues > 0 or $flags->{'CHARGES'}  or $data->{'borrowernumber'} or $deletelocal == 0){
-    #   print $input->header;
-
+my $is_guarantor = $dbh->selectrow_array("SELECT COUNT(*) FROM borrowers WHERE guarantorid=?", undef, $member);
+if ( $op eq 'delete_confirm' or $countissues > 0 or $flags->{'CHARGES'}  or $is_guarantor or $deletelocal == 0){
     my $patron_image = Koha::Patron::Images->find($bor->{borrowernumber});
     $template->param( picture => 1 ) if $patron_image;
 
@@ -137,20 +134,24 @@ if ($countissues > 0 or $flags->{'CHARGES'}  or $data->{'borrowernumber'} or $de
     if ($flags->{'CHARGES'} ne '') {
         $template->param(charges => $flags->{'CHARGES'}->{'amount'});
     }
-    if ($data->{'borrowernumber'}) {
+    if ($is_guarantor) {
         $template->param(guarantees => 1);
     }
     if ($deletelocal == 0) {
         $template->param(keeplocal => 1);
     }
-output_html_with_http_headers $input, $cookie, $template->output;
-
-} else {
+    # This is silly written but reflect the same conditions as above
+    if ( not $countissues > 0 and not $flags->{CHARGES} ne '' and not $is_guarantor and not $deletelocal == 0 ) {
+        $template->param( op => 'delete_confirm' );
+    }
+}elsif ( $op eq 'delete_confirmed' ) {
     MoveMemberToDeleted($member);
     C4::Members::HandleDelBorrower($member);
     DelMember($member);
+    # TODO Tell the user everything went ok
     print $input->redirect("/cgi-bin/koha/members/members-home.pl");
     exit 0; # Exit without error
 }
 
+output_html_with_http_headers $input, $cookie, $template->output;
 
-- 
2.39.5