Bugzilla – Attachment 177619 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
Bug-39014-Add-new-method-validateguarantor.patch (text/plain), 12.54 KB, created by
Jake Deery
on 2025-02-07 12:50:17 UTC
(
hide
)
Description:
Bug 39014: Add new method validate_guarantor
Filename:
MIME Type:
Creator:
Jake Deery
Created:
2025-02-07 12:50:17 UTC
Size:
12.54 KB
patch
obsolete
>From 769e9edc8f72c038a5a6140e83463b1abff633b6 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 >Signed-off-by: Jake Deery <jake.deery@ptfs-europe.com> >--- > 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
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