Bugzilla – Attachment 179572 Details for
Bug 39014
Storing a guarantee fails due to TrackLastPatronActivityTriggers "creating a patron"
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 39014: Add new method validate_guarantor
0001-Bug-39014-Add-new-method-validate_guarantor.patch (text/plain), 15.60 KB, created by
Emmi Takkinen
on 2025-03-21 07:30:40 UTC
(
hide
)
Description:
Bug 39014: Add new method validate_guarantor
Filename:
MIME Type:
Creator:
Emmi Takkinen
Created:
2025-03-21 07:30:40 UTC
Size:
15.60 KB
patch
obsolete
>From 5e1b6b6a2574117a03bf40e2c2a3a989fe1f96ce Mon Sep 17 00:00:00 2001 >From: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> >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 | 36 ++++---- > t/db_dependent/Koha/Patron.t | 91 +++++++------------ > 5 files changed, 91 insertions(+), 108 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 fd7cf85fc2..22d5e87996 100644 >--- a/Koha/Patron.pm >+++ b/Koha/Patron.pm >@@ -197,8 +197,6 @@ sub store { > my $self = shift; > my $params = @_ ? shift : {}; > >- my $guarantors = $params->{guarantors} // []; >- > $self->_result->result_source->schema->txn_do( > sub { > if ( >@@ -319,20 +317,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); >@@ -376,23 +360,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; >@@ -2498,6 +2465,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 f94d479da5..c6b46d72b2 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 @@ > <li id="ERROR_child_is_guarantor">Child patron cannot be a guarantor.</li> > [% END %] > [% IF ( ERROR_guarantor_is_guarantee ) %] >- <li id="ERROR_guarantor_is_guarantee">A guarantor cannot be a guarantee.</li> >+ <li id="ERROR_guarantor_is_guarantee">A guarantee cannot be a guarantor.</li> >+ [% END %] >+ [% IF ( ERROR_guarantee_is_guarantor ) %] >+ <li id="ERROR_guarantee_is_guarantor">A guarantor cannot be a guarantee.</li> > [% END %] > [% IF ( ERROR_cannot_delete_guarantor ) %] > <li id="ERROR_cannot_delete_guarantor">Cannot delete guarantor(s). A child patron needs a guarantor.</li> >diff --git a/members/memberentry.pl b/members/memberentry.pl >index 0794a9cff3..d1fa7075b4 100755 >--- a/members/memberentry.pl >+++ b/members/memberentry.pl >@@ -286,20 +286,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_guarantor_is_guarantee'; >+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"; > } > } > >@@ -452,7 +456,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; >@@ -561,7 +565,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 5197e53468..09db9a93b1 100755 >--- a/t/db_dependent/Koha/Patron.t >+++ b/t/db_dependent/Koha/Patron.t >@@ -2429,93 +2429,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 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 39014
:
177563
|
177564
|
177619
|
178546
|
178547
|
179572
|
180298
|
182290