gitea
/
Koha-clms
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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: Magnus Enger <magnus@libriotech.no>
Works as advertised.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This commit is contained in:
Emmi Takkinen 2025-01-31 10:32:18 +02:00 committed by Lucas Gass
parent 66d92ba6c6
commit 382ba17a8c
No known key found for this signature in database
GPG Key ID: 60B447398B0D36CA
5 changed files with 91 additions and 108 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
Koha/Exceptions/Patron/Relationship.pm
View File

@ -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 );

63
Koha/Patron.pm
View File

@ -207,8 +207,6 @@ sub store {
my $self = shift;
my $params = @_ ? shift : {};
my $guarantors = $params->{guarantors} // [];
$self->_result->result_source->schema->txn_do(
sub {
if (
@ -329,20 +327,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);
@ -386,23 +370,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 ( !defined $self->contactname || $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") || C4::Context->preference("CardnumberLog") ) {
my $info;
@ -2835,6 +2802,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(

5
koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
View File

@ -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>

36
members/memberentry.pl
View File

@ -281,20 +281,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";
}
}
@ -447,7 +451,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{guarantor_relationship};
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;

91
t/db_dependent/Koha/Patron.t
View File

@ -2624,93 +2624,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,
categorycode => $guarantee_category->{categorycode},
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,
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();
lives_ok { $child->validate_guarantor( \@guarantors, undef, $category ); }
'Validation passes when valid guarantors are passed in array';
t::lib::Mocks::mock_preference( 'borrowerRelationship', '' );
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;
};