From ed2acc9266bd3edd563243232d219ef2e3fadef9 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi 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 Signed-off-by: Jonathan Druart Signed-off-by: Fridolin Somers --- 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