From 5dae48167f8f36d3902e523746f4823dc90269c7 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 --- Koha/Exceptions/Patron/Relationship.pm | 4 +- Koha/Patron.pm | 63 ++++++------- .../prog/en/modules/members/memberentrygen.tt | 5 +- members/memberentry.pl | 37 ++++---- t/db_dependent/Koha/Patron.t | 91 +++++++------------ 5 files changed, 91 insertions(+), 109 deletions(-) diff --git a/Koha/Exceptions/Patron/Relationship.pm b/Koha/Exceptions/Patron/Relationship.pm index d3ca50c4f9..da525077bc 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' ] + fields => [ 'relationship', 'no_relationship', 'invalid_guarantor', 'child_guarantor' ] }, 'Koha::Exceptions::Patron::Relationship::NoGuarantor' => { isa => 'Koha::Exceptions::Patron::Relationship', @@ -50,6 +50,8 @@ sub full_message { if ( $self->no_relationship ) { $msg = sprintf("No relationship passed."); } elsif ( $self->invalid_guarantor ) { + $msg = sprintf("Guarantee patron cannot be a guarantor."); + } elsif ( $self->child_guarantor ) { $msg = sprintf("Child patron cannot be a guarantor."); } else { $msg = sprintf( "Invalid relationship passed, '%s' is not defined.", $self->relationship ); diff --git a/Koha/Patron.pm b/Koha/Patron.pm index ec7f670498..dcc9aa415e 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -196,8 +196,6 @@ sub store { my $self = shift; my $params = @_ ? shift : {}; - my $guarantors = $params->{guarantors} // []; - $self->_result->result_source->schema->txn_do( sub { if ( @@ -318,20 +316,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); @@ -375,23 +359,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; @@ -2393,6 +2360,36 @@ 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_guarantee ) { + Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( invalid_guarantor => 1 ); + } elsif ( $guarantor->is_child ) { + Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( child_guarantor => 1 ); + } + } + +} + =head3 add_guarantor my $relationship = $patron->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 2ad3069573..7e3c2c2890 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt @@ -195,7 +195,10 @@
  • Child patron cannot be a guarantor.
  • [% END %] [% IF ( ERROR_guarantor_is_guarantee ) %] -
  • A guarantor cannot be a guarantee.
  • +
  • A guarantee cannot be a guarantor.
  • + [% END %] + [% IF ( ERROR_guarantee_is_guarantor ) %] +
  • A guarantor cannot be a guarantee.
  • [% 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 c2df452336..2670be0f5d 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -284,21 +284,24 @@ 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_child_is_guarantor' if ( $guarantor->is_child ); - push @errors, 'ERROR_guarantor_is_guarantee' if ( !$guarantor->is_child ); +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" ) { + push @errors, "ERROR_child_no_guarantor"; + } elsif ( ref($_) eq "Koha::Exceptions::Patron::Relationship::InvalidRelationship" ) { + if ( $_->full_message eq "Guarantee patron cannot be a guarantor." ) { + push @errors, "ERROR_guarantor_is_guarantee"; + } elsif ( $_->full_message eq "Child patron cannot be a guarantor." ) { + push @errors, "ERROR_child_is_guarantor"; + } + } + return; + }; + } else { + push @errors, "ERROR_guarantee_is_guarantor"; } } @@ -451,7 +454,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; @@ -560,7 +563,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; diff --git a/t/db_dependent/Koha/Patron.t b/t/db_dependent/Koha/Patron.t index f1ea69f046..6d094df7b5 100755 --- a/t/db_dependent/Koha/Patron.t +++ b/t/db_dependent/Koha/Patron.t @@ -2425,93 +2425,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.34.1