From 769e9edc8f72c038a5a6140e83463b1abff633b6 Mon Sep 17 00:00:00 2001 From: Emmi Takkinen Date: Fri, 31 Jan 2025 10:32:18 +0200 Subject: [PATCH] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy Signed-off-by: Jake Deery --- Koha/Patron.pm | 61 +++++++++++------------- members/memberentry.pl | 30 ++++++------ t/db_dependent/Koha/Patron.t | 91 ++++++++++++++---------------------- 3 files changed, 75 insertions(+), 107 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index d2bf6f0596..85bc9b97c9 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -195,8 +195,6 @@ sub store { my $self = shift; my $params = @_ ? shift : {}; - my $guarantors = $params->{guarantors} // []; - $self->_result->result_source->schema->txn_do( sub { if ( @@ -317,20 +315,6 @@ sub store { $self->borrowernumber(undef); - if ( C4::Context->preference('ChildNeedsGuarantor') - and ( $self->is_child or $self->category->can_be_guarantee ) - and $self->contactname eq "" - and !@$guarantors ) - { - Koha::Exceptions::Patron::Relationship::NoGuarantor->throw(); - } - - foreach my $guarantor (@$guarantors) { - if ( $guarantor->is_child ) { - Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( invalid_guarantor => 1 ); - } - } - $self = $self->SUPER::store; $self->add_enrolment_fee_if_needed(0); @@ -374,23 +358,6 @@ sub store { } - my @existing_guarantors = $self->guarantor_relationships()->guarantors->as_list; - push @$guarantors, @existing_guarantors; - - if ( C4::Context->preference('ChildNeedsGuarantor') - and ( $self->is_child or $self->category->can_be_guarantee ) - and $self->contactname eq "" - and !@$guarantors ) - { - Koha::Exceptions::Patron::Relationship::NoGuarantor->throw(); - } - - foreach my $guarantor (@$guarantors) { - if ( $guarantor->is_child ) { - Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( invalid_guarantor => 1 ); - } - } - # Actionlogs if ( C4::Context->preference("BorrowersLog") ) { my $info; @@ -2373,6 +2340,34 @@ sub _anonymize_column { $self->$col($val); } +=head3 validate_guarantor + + Koha::Patron->validate_guarantor(\@guarantors, $contactname, $category ); + + Validates guarantor patron. + +=cut + +sub validate_guarantor { + my ( $self, $guarantors, $contactname, $category ) = @_; + + my $valid_guarantor = @$guarantors ? @$guarantors : $contactname; + + if ( C4::Context->preference('ChildNeedsGuarantor') + and ( $category->category_type eq 'C' or $category->can_be_guarantee ) + and !$valid_guarantor ) + { + Koha::Exceptions::Patron::Relationship::NoGuarantor->throw(); + } + + foreach my $guarantor (@$guarantors) { + if ( $guarantor->is_child || $guarantor->is_guarantee ) { + Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( invalid_guarantor => 1 ); + } + } + +} + =head3 add_guarantor my $relationship = $patron->add_guarantor( diff --git a/members/memberentry.pl b/members/memberentry.pl index f1fd9bfe5b..a218cf03a2 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -275,21 +275,17 @@ if (@delete_guarantor) { } #Check if guarantor requirements are met -my $valid_guarantor = @guarantors ? @guarantors : $newdata{'contactname'}; -if ( ( $op eq 'cud-save' || $op eq 'cud-insert' ) - && C4::Context->preference('ChildNeedsGuarantor') - && ( $category->category_type eq 'C' || $category->can_be_guarantee ) - && !$valid_guarantor ) -{ - push @errors, 'ERROR_child_no_guarantor'; -} - -foreach my $guarantor (@guarantors) { - if ( ( $op eq 'cud-save' || $op eq 'cud-insert' ) - && ( $guarantor->is_child || $guarantor->is_guarantee || ( $patron && $patron->is_guarantor ) ) ) - { - push @errors, 'ERROR_guarantor_is_guarantee'; - } +if ( ( $op eq 'cud-save' || $op eq 'cud-insert' ) ) { + try { + Koha::Patron->validate_guarantor( \@guarantors, $newdata{'contactname'}, $category ); + } catch { + if ( ref($_) eq 'Koha::Exceptions::Patron::Relationship::NoGuarantor' ) { + push @errors, "ERROR_child_no_guarantor"; + } elsif ( ref($_) eq 'Koha::Exceptions::Patron::Relationship::InvalidRelationship' ) { + push @errors, "ERROR_guarantor_is_guarantee"; + } + return; + }; } my @valid_relationships = split( /\|/, C4::Context->preference('borrowerRelationship'), -1 ); @@ -440,7 +436,7 @@ if ((!$nok) and $nodouble and ($op eq 'cud-insert' or $op eq 'cud-save')){ delete $newdata{password2}; $success = 1; $patron = try { - Koha::Patron->new( \%newdata )->store( { guarantors => \@guarantors } ); + Koha::Patron->new( \%newdata )->store(); } catch { $success = 0; $nok = 1; @@ -537,7 +533,7 @@ if ((!$nok) and $nodouble and ($op eq 'cud-insert' or $op eq 'cud-save')){ delete $newdata{password2}; try { - $patron->set( \%newdata )->store( { guarantors => \@guarantors } ) if scalar( keys %newdata ) > 1; + $patron->set( \%newdata )->store() if scalar( keys %newdata ) > 1; # bug 4508 - avoid crash if we're not updating any columns in the borrowers table (editing patron attrs or msg prefs) $success = 1; } catch { diff --git a/t/db_dependent/Koha/Patron.t b/t/db_dependent/Koha/Patron.t index 32189c6c95..63bb52b818 100755 --- a/t/db_dependent/Koha/Patron.t +++ b/t/db_dependent/Koha/Patron.t @@ -2351,93 +2351,70 @@ subtest 'get_lists_with_patron() tests' => sub { $schema->storage->txn_rollback; }; -subtest 'guarantor requirements tests' => sub { +subtest 'validate_guarantor() tests' => sub { - plan tests => 6; + plan tests => 5; $schema->storage->txn_begin; my $branchcode = $builder->build( { source => 'Branch' } )->{branchcode}; - my $child_category = - $builder->build( { source => 'Category', value => { category_type => 'C', can_be_guarantee => 1 } } ) - ->{categorycode}; - my $patron_category = - $builder->build( { source => 'Category', value => { category_type => 'A', can_be_guarantee => 0 } } ) - ->{categorycode}; - - t::lib::Mocks::mock_preference( 'ChildNeedsGuarantor', 0 ); + my $guarantee_category = + $builder->build( + { source => 'Category', value => { categorycode => "GUARANTEE", category_type => 'C', can_be_guarantee => 1 } } + ); + my $guarantor_category = + $builder->build( + { source => 'Category', value => { categorycode => "GUARANTOR", category_type => 'A', can_be_guarantee => 0 } } + ); my $child = Koha::Patron->new( { branchcode => $branchcode, - categorycode => $child_category, - contactname => '' - } - ); - $child->store(); - - ok( - Koha::Patrons->find( $child->id ), - 'Child patron can be stored without guarantor when ChildNeedsGuarantor is off.' - ); - - t::lib::Mocks::mock_preference( 'ChildNeedsGuarantor', 1 ); - - my $child2 = Koha::Patron->new( - { - branchcode => $branchcode, - categorycode => $child_category, + categorycode => $guarantee_category->{categorycode}, contactname => '' } ); - my $child3 = $builder->build_object( + my $child2 = $builder->build_object( { class => 'Koha::Patrons', - value => { categorycode => $child_category } + value => { categorycode => $guarantee_category->{categorycode} } } ); my $patron = $builder->build_object( { class => 'Koha::Patrons', - value => { categorycode => $patron_category } + value => { categorycode => $guarantor_category->{categorycode} } } ); - throws_ok { $child2->store(); } + my @guarantors; + my $contactname = ""; + my $category = Koha::Patron::Categories->find( $guarantee_category->{categorycode} ); + + t::lib::Mocks::mock_preference( 'ChildNeedsGuarantor', 0 ); + + lives_ok { $child->validate_guarantor( \@guarantors, $contactname, $category ); } + 'Validation passes when ChildNeedsGuarantor syspref is disabled'; + + t::lib::Mocks::mock_preference( 'ChildNeedsGuarantor', 1 ); + + throws_ok { $child->validate_guarantor( \@guarantors, $contactname, $category ); } 'Koha::Exceptions::Patron::Relationship::NoGuarantor', 'Exception thrown when guarantor is required but not provided.'; - my @guarantors = ( $patron, $child3 ); - throws_ok { $child2->store( { guarantors => \@guarantors } ); } + @guarantors = ( $patron, $child2 ); + throws_ok { $child->validate_guarantor( \@guarantors, $contactname, $category ); } 'Koha::Exceptions::Patron::Relationship::InvalidRelationship', 'Exception thrown when child patron is added as guarantor.'; - #test ModMember @guarantors = ($patron); - $child2->store( { guarantors => \@guarantors } )->discard_changes(); - - t::lib::Mocks::mock_preference( 'borrowerRelationship', '' ); + lives_ok { $child->validate_guarantor( \@guarantors, undef, $category ); } + 'Validation passes when valid guarantors are passed in array'; - my $relationship = Koha::Patron::Relationship->new( - { - guarantor_id => $patron->borrowernumber, - guarantee_id => $child2->borrowernumber, - relationship => '' - } - ); - $relationship->store(); - - ok( $child2->store(), 'Child patron can be modified and stored when guarantor is stored' ); - - @guarantors = ($child3); - throws_ok { $child2->store( { guarantors => \@guarantors } ); } - 'Koha::Exceptions::Patron::Relationship::InvalidRelationship', - 'Exception thrown when child patron is modified and child patron is added as guarantor.'; - - $relationship->delete; - throws_ok { $child2->store(); } - 'Koha::Exceptions::Patron::Relationship::NoGuarantor', - 'Exception thrown when guarantor is deleted.'; + @guarantors = (); + $contactname = "Guarantor"; + lives_ok { $child->validate_guarantor( \@guarantors, $contactname, $category ); } + 'Validation passes when guarantor contanct name is passed'; $schema->storage->txn_rollback; }; -- 2.43.0