From 2bcfa0689a48c3e37922b544c450a689d71a259e Mon Sep 17 00:00:00 2001 From: Lari Taskula Date: Fri, 17 Oct 2025 00:44:38 +0300 Subject: [PATCH] Bug 39014: (follow-up) Refacotring and renaming to can_be_guaranteed_by To test: 1. prove t/db_dependent/Koha/Patron.t --- Koha/Exceptions/Patron/Relationship.pm | 2 +- Koha/Patron.pm | 41 ++++++++++--- .../prog/en/modules/members/memberentrygen.tt | 9 ++- members/memberentry.pl | 59 ++++++++++++++----- t/db_dependent/Koha/Patron.t | 28 +++++---- 5 files changed, 102 insertions(+), 37 deletions(-) diff --git a/Koha/Exceptions/Patron/Relationship.pm b/Koha/Exceptions/Patron/Relationship.pm index abc5baa2c03..000e95d1097 100644 --- a/Koha/Exceptions/Patron/Relationship.pm +++ b/Koha/Exceptions/Patron/Relationship.pm @@ -32,7 +32,7 @@ use Exception::Class ( 'Koha::Exceptions::Patron::Relationship::InvalidRelationship' => { isa => 'Koha::Exceptions::Patron::Relationship', description => 'The specified relationship is invalid', - fields => [ 'relationship', 'no_relationship', 'invalid_guarantor', 'child_guarantor' ] + fields => [ 'relationship', 'no_relationship', 'invalid_guarantor', 'child_guarantor', 'guarantor_cant_have_guarantors', 'guarantor' ], }, 'Koha::Exceptions::Patron::Relationship::NoGuarantor' => { isa => 'Koha::Exceptions::Patron::Relationship', diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 9060ea4f1a3..25bf7a0df4d 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -2708,34 +2708,59 @@ sub _anonymize_column { $self->$col($val); } -=head3 validate_guarantor +=head3 can_be_guaranteed_by - Koha::Patron->validate_guarantor(\@guarantors, $contactname, $category ); + $patron->can_be_guaranteed_by( [ + $guarantor1, + $guarantor2, + ... + ] ); - Validates guarantor patron. + Given an array of guarantors, checks if they can be guarantors for this patron. + + Throws Koha::Exceptions::Patron::Relationship::InvalidRelationship if a proposed + guarantor cannot be a guarantor for this patron. + + $patron->can_be_guaranteed_by(); + + Given no guarantors, checks if this patron must be guaranteed by someone. Read + "can be guaranteed by nobody". + + Throws Koha::Exceptions::Patron::Relationship::NoGuarantor if this patron must + have a guarantor. =cut -sub validate_guarantor { - my ( $self, $guarantors, $contactname, $category ) = @_; +sub can_be_guaranteed_by { + my ( $self, $guarantors ) = @_; - my $valid_guarantor = @$guarantors ? @$guarantors : $contactname; + my $category = $self->category; if ( C4::Context->preference('ChildNeedsGuarantor') and ( $category->category_type eq 'C' or $category->can_be_guarantee ) - and !$valid_guarantor ) + and scalar @$guarantors == 0 ) { Koha::Exceptions::Patron::Relationship::NoGuarantor->throw(); } + if ( $self->is_guarantor && scalar @$guarantors > 0 ) { + Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( guarantor_cant_have_guarantors => 1 ); + } + foreach my $guarantor (@$guarantors) { + next if ref($guarantor) ne 'Koha::Patron'; + if ( $guarantor->is_guarantee ) { - Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( invalid_guarantor => 1 ); + Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( + invalid_guarantor => 1, + guarantor => $guarantor, + ); } elsif ( $guarantor->is_child ) { Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( child_guarantor => 1 ); } } + return 1; } =head3 add_guarantor diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt index 68a99a5f72d..744aa6e3828 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt @@ -195,10 +195,13 @@
  • Child patron cannot be a guarantor.
  • [% END %] [% IF ( ERROR_guarantor_is_guarantee ) %] -
  • A guarantee cannot be a guarantor.
  • +
  • + Cannot add a guarantor. The proposed guarantor + [% guarantor_is_guarantee.firstname | html %] [% guarantor_is_guarantee.surname | html %] ([% guarantor_is_guarantee.cardnumber | html %]) + is a guarantee of someone else, and guarantees cannot be guarantors.
  • [% END %] - [% IF ( ERROR_guarantee_is_guarantor ) %] -
  • A guarantor cannot be a guarantee.
  • + [% IF ( ERROR_guarantor_cant_have_guarantors ) %] +
  • Cannot add a guarantor. This patron is already a guarantor, and guarantors cannot have guarantors.
  • [% END %] [% IF ( ERROR_cannot_delete_guarantor ) %]
  • Cannot delete guarantor(s). A child patron needs a guarantor.
  • diff --git a/members/memberentry.pl b/members/memberentry.pl index da876cee0b4..a94e519a1e7 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -24,6 +24,7 @@ use Try::Tiny; # external modules use CGI qw ( -utf8 ); +use Scalar::Util qw( blessed ); # internal modules use C4::Auth qw( get_template_and_user haspermission ); @@ -53,6 +54,7 @@ my $input = CGI->new; my %data; my $dbh = C4::Context->dbh; +my $logger = Koha::Logger->get({ interface => 'intranet '}); my ( $template, $loggedinuser, $cookie ) = get_template_and_user( { @@ -278,33 +280,60 @@ if (@delete_guarantor) { if ( C4::Context->preference('ChildNeedsGuarantor') && $will_remove_last ) { push @errors, 'ERROR_cannot_delete_guarantor'; } else { + my @deleted_guarantors; foreach my $id (@delete_guarantor) { my $r = Koha::Patron::Relationships->find($id); - $r->delete() if $r; + if ($r) { + $r->delete(); + push @deleted_guarantors, $id; + } + } + foreach my $deleted_guarantor_id (@deleted_guarantors) { + @guarantors = grep { ref($_) eq 'Koha::Patron' && $_->id ne $deleted_guarantor_id } @guarantors; } } } #Check if guarantor requirements are met -if ( ( $op eq 'cud-save' || $op eq 'cud-insert' ) ) { - unless ( $patron && $patron->is_guarantor ) { - try { - Koha::Patron->validate_guarantor( \@guarantors, $newdata{'contactname'}, $category ); - } catch { - if ( ref($_) eq "Koha::Exceptions::Patron::Relationship::NoGuarantor" ) { +my $guarantors_to_be_validated = + @guarantors ? \@guarantors : $newdata{'contactname'} ? [ $newdata{'contactname'} ] : []; +if ( ( $op eq 'cud-save' || $op eq 'cud-insert' ) && $guarantors_to_be_validated ) { + try { + if ($patron) { + if ( $patron->categorycode eq $categorycode ) { + $patron->can_be_guaranteed_by($guarantors_to_be_validated); + } else { + # If $patron's categorycode is about to change, check guarantor requirements against + # the new category but keep the original $patron object intact + # (this clone()s the $patron) + ( bless {%$patron}, ref $patron )->categorycode($categorycode) + ->can_be_guaranteed_by($guarantors_to_be_validated); + } + } else { + Koha::Patron->new( { categorycode => $categorycode } )->can_be_guaranteed_by($guarantors_to_be_validated); + } + } catch { + unless ( blessed($_) ) { + $logger->error($_); + } else { + if ( $_->isa("Koha::Exceptions::Patron::Relationship::NoGuarantor") ) { push @errors, "ERROR_child_no_guarantor"; - } elsif ( ref($_) eq "Koha::Exceptions::Patron::Relationship::InvalidRelationship" ) { - if ( $_->full_message eq "Guarantee patron cannot be a guarantor." ) { + } elsif ( $_->isa("Koha::Exceptions::Patron::Relationship::InvalidRelationship") ) { + if ( $_->invalid_guarantor ) { push @errors, "ERROR_guarantor_is_guarantee"; - } elsif ( $_->full_message eq "Child patron cannot be a guarantor." ) { + $template->param( guarantor_is_guarantee => $_->guarantor ); + } elsif ( $_->child_guarantor ) { push @errors, "ERROR_child_is_guarantor"; + } elsif ( $_->guarantor_cant_have_guarantors ) { + push @errors, "ERROR_guarantor_cant_have_guarantors"; } + } else { + $logger->error( $_->error ); + $_->rethrow; } - return; - }; - } else { - push @errors, "ERROR_guarantee_is_guarantor"; - } + } + return; + }; } my @valid_relationships = split( /\|/, C4::Context->preference('borrowerRelationship'), -1 ); diff --git a/t/db_dependent/Koha/Patron.t b/t/db_dependent/Koha/Patron.t index 484978c813e..680c7839fcc 100755 --- a/t/db_dependent/Koha/Patron.t +++ b/t/db_dependent/Koha/Patron.t @@ -2624,7 +2624,7 @@ subtest 'get_lists_with_patron() tests' => sub { $schema->storage->txn_rollback; }; -subtest 'validate_guarantor() tests' => sub { +subtest 'can_be_guaranteed_by() tests' => sub { plan tests => 5; @@ -2659,6 +2659,12 @@ subtest 'validate_guarantor() tests' => sub { value => { categorycode => $guarantor_category->{categorycode} } } ); + my $patron2 = $builder->build_object( + { + class => 'Koha::Patrons', + value => { categorycode => $guarantor_category->{categorycode} } + } + ); my @guarantors; my $contactname = ""; @@ -2666,29 +2672,31 @@ subtest 'validate_guarantor() tests' => sub { t::lib::Mocks::mock_preference( 'ChildNeedsGuarantor', 0 ); - lives_ok { $child->validate_guarantor( \@guarantors, $contactname, $category ); } + lives_ok { $child->can_be_guaranteed_by( \@guarantors ); } 'Validation passes when ChildNeedsGuarantor syspref is disabled'; t::lib::Mocks::mock_preference( 'ChildNeedsGuarantor', 1 ); - throws_ok { $child->validate_guarantor( \@guarantors, $contactname, $category ); } + throws_ok { $child->can_be_guaranteed_by( \@guarantors ); } 'Koha::Exceptions::Patron::Relationship::NoGuarantor', 'Exception thrown when guarantor is required but not provided.'; @guarantors = ( $patron, $child2 ); - throws_ok { $child->validate_guarantor( \@guarantors, $contactname, $category ); } + throws_ok { $child->can_be_guaranteed_by( \@guarantors ); } 'Koha::Exceptions::Patron::Relationship::InvalidRelationship', 'Exception thrown when child patron is added as guarantor.'; + t::lib::Mocks::mock_preference( 'borrowerRelationship', 'parent' ); + $child2->add_guarantor({ guarantor_id => $patron->id, relationship => 'parent' }); + @guarantors = ( $patron2 ); + throws_ok { $patron->can_be_guaranteed_by( \@guarantors ); } + 'Koha::Exceptions::Patron::Relationship::InvalidRelationship', + 'Exception thrown when trying to add a guarantor to a guarantor.'; + @guarantors = ($patron); - lives_ok { $child->validate_guarantor( \@guarantors, undef, $category ); } + lives_ok { $child->can_be_guaranteed_by( \@guarantors ); } 'Validation passes when valid guarantors are passed in array'; - @guarantors = (); - $contactname = "Guarantor"; - lives_ok { $child->validate_guarantor( \@guarantors, $contactname, $category ); } - 'Validation passes when guarantor contanct name is passed'; - $schema->storage->txn_rollback; }; -- 2.34.1