From ed2acc9266bd3edd563243232d219ef2e3fadef9 Mon Sep 17 00:00:00 2001
From: Tomas Cohen Arazi <tomascohen@theke.io>
Date: Thu, 23 Dec 2021 11:43:52 -0300
Subject: [PATCH] Bug 29765: Make Koha::Patron->safe_to_delete use
 Koha::Result::Boolean

This patch makes the safe_to_delete method in Koha::Patron return this
new object type instead of a plain string. This way we have a consistent
way to deal with 'can_*'-like methods return values when feedback is
needed.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron.t
=> SUCCESS: The adjusted tests pass. Tests cover the boolean context
eval and also the carried messages, that include the same string code
that was returned originally.
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
---
 Koha/Patron.pm               | 28 ++++++++++++++++++----------
 t/db_dependent/Koha/Patron.t | 27 +++++++++++++++++++++------
 2 files changed, 39 insertions(+), 16 deletions(-)

diff --git a/Koha/Patron.pm b/Koha/Patron.pm
index 7a70ecc2dd..3414546ca2 100644
--- a/Koha/Patron.pm
+++ b/Koha/Patron.pm
@@ -48,6 +48,7 @@ use Koha::Patron::Modifications;
 use Koha::Patron::Relationships;
 use Koha::Patrons;
 use Koha::Plugins;
+use Koha::Result::Boolean;
 use Koha::Subscription::Routinglists;
 use Koha::Token;
 use Koha::Virtualshelves;
@@ -1940,19 +1941,26 @@ sub safe_to_delete {
 
     my $anonymous_patron = C4::Context->preference('AnonymousPatron');
 
-    return 'is_anonymous_patron'
-        if $anonymous_patron && $self->id eq $anonymous_patron;
+    my $error;
 
-    return 'has_checkouts'
-        if $self->checkouts->count;
-
-    return 'has_debt'
-        if $self->account->outstanding_debits->total_outstanding > 0;
+    if ( $anonymous_patron && $self->id eq $anonymous_patron ) {
+        $error = 'is_anonymous_patron';
+    }
+    elsif ( $self->checkouts->count ) {
+        $error = 'has_checkouts';
+    }
+    elsif ( $self->account->outstanding_debits->total_outstanding > 0 ) {
+        $error = 'has_debt';
+    }
+    elsif ( $self->guarantee_relationships->count ) {
+        $error = 'has_guarantees';
+    }
 
-    return 'has_guarantees'
-        if $self->guarantee_relationships->count;
+    if ( $error ) {
+        return Koha::Result::Boolean->new(0)->add_message({ message => $error });
+    }
 
-    return 'ok';
+    return Koha::Result::Boolean->new(1);
 }
 
 =head2 Internal methods
diff --git a/t/db_dependent/Koha/Patron.t b/t/db_dependent/Koha/Patron.t
index f0724766fc..8d492168d7 100755
--- a/t/db_dependent/Koha/Patron.t
+++ b/t/db_dependent/Koha/Patron.t
@@ -849,7 +849,7 @@ subtest 'article_requests() tests' => sub {
 
 subtest 'safe_to_delete() tests' => sub {
 
-    plan tests => 5;
+    plan tests => 14;
 
     $schema->storage->txn_begin;
 
@@ -858,7 +858,10 @@ subtest 'safe_to_delete() tests' => sub {
     ## Make it the anonymous
     t::lib::Mocks::mock_preference( 'AnonymousPatron', $patron->id );
 
-    is( $patron->safe_to_delete, 'is_anonymous_patron', 'Cannot delete, it is the anonymous patron' );
+    ok( !$patron->safe_to_delete, 'Cannot delete, it is the anonymous patron' );
+    my $message = $patron->safe_to_delete->messages->[0];
+    is( $message->type, 'error', 'Type is error' );
+    is( $message->message, 'is_anonymous_patron', 'Cannot delete, it is the anonymous patron' );
     # cleanup
     t::lib::Mocks::mock_preference( 'AnonymousPatron', 0 );
 
@@ -870,7 +873,10 @@ subtest 'safe_to_delete() tests' => sub {
         }
     );
 
-    is( $patron->safe_to_delete, 'has_checkouts', 'Cannot delete, has checkouts' );
+    ok( !$patron->safe_to_delete, 'Cannot delete, has checkouts' );
+    $message = $patron->safe_to_delete->messages->[0];
+    is( $message->type, 'error', 'Type is error' );
+    is( $message->message, 'has_checkouts', 'Cannot delete, has checkouts' );
     # cleanup
     $checkout->delete;
 
@@ -879,19 +885,28 @@ subtest 'safe_to_delete() tests' => sub {
     $builder->build_object({ class => 'Koha::Patrons' })
             ->add_guarantor({ guarantor_id => $patron->id, relationship => 'parent' });
 
-    is( $patron->safe_to_delete, 'has_guarantees', 'Cannot delete, has guarantees' );
+    ok( !$patron->safe_to_delete, 'Cannot delete, has guarantees' );
+    $message = $patron->safe_to_delete->messages->[0];
+    is( $message->type, 'error', 'Type is error' );
+    is( $message->message, 'has_guarantees', 'Cannot delete, has guarantees' );
+
     # cleanup
     $patron->guarantee_relationships->delete;
 
     ## Make it have debt
     my $debit = $patron->account->add_debit({ amount => 10, interface => 'intranet', type => 'MANUAL' });
 
-    is( $patron->safe_to_delete, 'has_debt', 'Cannot delete, has debt' );
+    ok( !$patron->safe_to_delete, 'Cannot delete, has debt' );
+    $message = $patron->safe_to_delete->messages->[0];
+    is( $message->type, 'error', 'Type is error' );
+    is( $message->message, 'has_debt', 'Cannot delete, has debt' );
     # cleanup
     $patron->account->pay({ amount => 10, debits => [ $debit ] });
 
     ## Happy case :-D
-    is( $patron->safe_to_delete, 'ok', 'Can delete, all conditions met' );
+    ok( $patron->safe_to_delete, 'Can delete, all conditions met' );
+    my $messages = $patron->safe_to_delete->messages;
+    is_deeply( $messages, [], 'Patron can be deleted, no messages' );
 
     $schema->storage->txn_rollback;
 };
-- 
2.39.5