Bugzilla – Attachment 188464 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: (follow-up) Refacotring and renaming to can_be_guaranteed_by
Bug-39014-follow-up-Refacotring-and-renaming-to-ca.patch (text/plain), 12.52 KB, created by
David Nind
on 2025-10-27 08:34:43 UTC
(
hide
)
Description:
Bug 39014: (follow-up) Refacotring and renaming to can_be_guaranteed_by
Filename:
MIME Type:
Creator:
David Nind
Created:
2025-10-27 08:34:43 UTC
Size:
12.52 KB
patch
obsolete
>From fa3654fa28f4a221804c66f01fe91e73a9e817cd Mon Sep 17 00:00:00 2001 >From: Lari Taskula <lari.taskula@hypernova.fi> >Date: Fri, 17 Oct 2025 00:44:38 +0300 >Subject: [PATCH] Bug 39014: (follow-up) Refacotring and renaming to > can_be_guaranteed_by > >To test: >1. prove t/db_dependent/Koha/Patron.t > >Signed-off-by: David Nind <david@davidnind.com> >--- > Koha/Exceptions/Patron/Relationship.pm | 2 +- > Koha/Patron.pm | 41 ++++++++++--- > .../prog/en/modules/members/memberentrygen.tt | 9 ++- > members/memberentry.pl | 59 ++++++++++++++----- > t/db_dependent/Koha/Patron.t | 28 +++++---- > 5 files changed, 102 insertions(+), 37 deletions(-) > >diff --git a/Koha/Exceptions/Patron/Relationship.pm b/Koha/Exceptions/Patron/Relationship.pm >index abc5baa2c0..000e95d109 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', 'child_guarantor' ] >+ fields => [ 'relationship', 'no_relationship', 'invalid_guarantor', 'child_guarantor', 'guarantor_cant_have_guarantors', 'guarantor' ], > }, > 'Koha::Exceptions::Patron::Relationship::NoGuarantor' => { > isa => 'Koha::Exceptions::Patron::Relationship', >diff --git a/Koha/Patron.pm b/Koha/Patron.pm >index 9060ea4f1a..a49e1f159f 100644 >--- a/Koha/Patron.pm >+++ b/Koha/Patron.pm >@@ -2708,34 +2708,59 @@ sub _anonymize_column { > $self->$col($val); > } > >-=head3 validate_guarantor >+=head3 can_be_guaranteed_by > >- Koha::Patron->validate_guarantor(\@guarantors, $contactname, $category ); >+ $patron->can_be_guaranteed_by( [ >+ $guarantor1, >+ $guarantor2, >+ ... >+ ] ); > >- Validates guarantor patron. >+ Given an array of guarantors, checks if they can be guarantors for this patron. >+ >+ Throws Koha::Exceptions::Patron::Relationship::InvalidRelationship if a proposed >+ guarantor cannot be a guarantor for this patron. >+ >+ $patron->can_be_guaranteed_by(); >+ >+ Given no guarantors, checks if this patron must be guaranteed by someone. Read >+ "can be guaranteed by nobody". >+ >+ Throws Koha::Exceptions::Patron::Relationship::NoGuarantor if this patron must >+ have a guarantor. > > =cut > >-sub validate_guarantor { >- my ( $self, $guarantors, $contactname, $category ) = @_; >+sub can_be_guaranteed_by { >+ my ( $self, $guarantors ) = @_; > >- my $valid_guarantor = @$guarantors ? @$guarantors : $contactname; >+ my $category = $self->category; > > if ( C4::Context->preference('ChildNeedsGuarantor') > and ( $category->category_type eq 'C' or $category->can_be_guarantee ) >- and !$valid_guarantor ) >+ and scalar @$guarantors == 0 ) > { > Koha::Exceptions::Patron::Relationship::NoGuarantor->throw(); > } > >+ if ( $self->is_guarantor && scalar @$guarantors > 0 ) { >+ Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( guarantor_cant_have_guarantors => 1 ); >+ } >+ > foreach my $guarantor (@$guarantors) { >+ next if ref($guarantor) ne 'Koha::Patron'; >+ > if ( $guarantor->is_guarantee ) { >- Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( invalid_guarantor => 1 ); >+ Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( >+ invalid_guarantor => 1, >+ guarantor => $guarantor, >+ ); > } elsif ( $guarantor->is_child ) { > Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( child_guarantor => 1 ); > } > } > >+ return 1; > } > > =head3 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 68a99a5f72..744aa6e382 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt >@@ -195,10 +195,13 @@ > <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 guarantee cannot be a guarantor.</li> >+ <li id="ERROR_guarantor_is_guarantee"> >+ Cannot add a guarantor. The proposed guarantor <a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% guarantor_is_guarantee.borrowernumber | uri %]" target="blank"> >+ [% guarantor_is_guarantee.firstname | html %] [% guarantor_is_guarantee.surname | html %] ([% guarantor_is_guarantee.cardnumber | html %]) >+ </a> is a guarantee of someone else, and guarantees cannot be guarantors.</li> > [% END %] >- [% IF ( ERROR_guarantee_is_guarantor ) %] >- <li id="ERROR_guarantee_is_guarantor">A guarantor cannot be a guarantee.</li> >+ [% IF ( ERROR_guarantor_cant_have_guarantors ) %] >+ <li id="ERROR_guarantor_cant_have_guarantors">Cannot add a guarantor. This patron is already a guarantor, and guarantors cannot have guarantors.</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 da876cee0b..a94e519a1e 100755 >--- a/members/memberentry.pl >+++ b/members/memberentry.pl >@@ -24,6 +24,7 @@ use Try::Tiny; > > # external modules > use CGI qw ( -utf8 ); >+use Scalar::Util qw( blessed ); > > # internal modules > use C4::Auth qw( get_template_and_user haspermission ); >@@ -53,6 +54,7 @@ my $input = CGI->new; > my %data; > > my $dbh = C4::Context->dbh; >+my $logger = Koha::Logger->get({ interface => 'intranet '}); > > my ( $template, $loggedinuser, $cookie ) = get_template_and_user( > { >@@ -278,33 +280,60 @@ if (@delete_guarantor) { > if ( C4::Context->preference('ChildNeedsGuarantor') && $will_remove_last ) { > push @errors, 'ERROR_cannot_delete_guarantor'; > } else { >+ my @deleted_guarantors; > foreach my $id (@delete_guarantor) { > my $r = Koha::Patron::Relationships->find($id); >- $r->delete() if $r; >+ if ($r) { >+ $r->delete(); >+ push @deleted_guarantors, $id; >+ } >+ } >+ foreach my $deleted_guarantor_id (@deleted_guarantors) { >+ @guarantors = grep { ref($_) eq 'Koha::Patron' && $_->id ne $deleted_guarantor_id } @guarantors; > } > } > } > > #Check if guarantor requirements are met >-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" ) { >+my $guarantors_to_be_validated = >+ @guarantors ? \@guarantors : $newdata{'contactname'} ? [ $newdata{'contactname'} ] : []; >+if ( ( $op eq 'cud-save' || $op eq 'cud-insert' ) && $guarantors_to_be_validated ) { >+ try { >+ if ($patron) { >+ if ( $patron->categorycode eq $categorycode ) { >+ $patron->can_be_guaranteed_by($guarantors_to_be_validated); >+ } else { >+ # If $patron's categorycode is about to change, check guarantor requirements against >+ # the new category but keep the original $patron object intact >+ # (this clone()s the $patron) >+ ( bless {%$patron}, ref $patron )->categorycode($categorycode) >+ ->can_be_guaranteed_by($guarantors_to_be_validated); >+ } >+ } else { >+ Koha::Patron->new( { categorycode => $categorycode } )->can_be_guaranteed_by($guarantors_to_be_validated); >+ } >+ } catch { >+ unless ( blessed($_) ) { >+ $logger->error($_); >+ } else { >+ if ( $_->isa("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." ) { >+ } elsif ( $_->isa("Koha::Exceptions::Patron::Relationship::InvalidRelationship") ) { >+ if ( $_->invalid_guarantor ) { > push @errors, "ERROR_guarantor_is_guarantee"; >- } elsif ( $_->full_message eq "Child patron cannot be a guarantor." ) { >+ $template->param( guarantor_is_guarantee => $_->guarantor ); >+ } elsif ( $_->child_guarantor ) { > push @errors, "ERROR_child_is_guarantor"; >+ } elsif ( $_->guarantor_cant_have_guarantors ) { >+ push @errors, "ERROR_guarantor_cant_have_guarantors"; > } >+ } else { >+ $logger->error( $_->error ); >+ $_->rethrow; > } >- return; >- }; >- } else { >- push @errors, "ERROR_guarantee_is_guarantor"; >- } >+ } >+ return; >+ }; > } > > my @valid_relationships = split( /\|/, C4::Context->preference('borrowerRelationship'), -1 ); >diff --git a/t/db_dependent/Koha/Patron.t b/t/db_dependent/Koha/Patron.t >index 484978c813..680c7839fc 100755 >--- a/t/db_dependent/Koha/Patron.t >+++ b/t/db_dependent/Koha/Patron.t >@@ -2624,7 +2624,7 @@ subtest 'get_lists_with_patron() tests' => sub { > $schema->storage->txn_rollback; > }; > >-subtest 'validate_guarantor() tests' => sub { >+subtest 'can_be_guaranteed_by() tests' => sub { > > plan tests => 5; > >@@ -2659,6 +2659,12 @@ subtest 'validate_guarantor() tests' => sub { > value => { categorycode => $guarantor_category->{categorycode} } > } > ); >+ my $patron2 = $builder->build_object( >+ { >+ class => 'Koha::Patrons', >+ value => { categorycode => $guarantor_category->{categorycode} } >+ } >+ ); > > my @guarantors; > my $contactname = ""; >@@ -2666,29 +2672,31 @@ subtest 'validate_guarantor() tests' => sub { > > t::lib::Mocks::mock_preference( 'ChildNeedsGuarantor', 0 ); > >- lives_ok { $child->validate_guarantor( \@guarantors, $contactname, $category ); } >+ lives_ok { $child->can_be_guaranteed_by( \@guarantors ); } > 'Validation passes when ChildNeedsGuarantor syspref is disabled'; > > t::lib::Mocks::mock_preference( 'ChildNeedsGuarantor', 1 ); > >- throws_ok { $child->validate_guarantor( \@guarantors, $contactname, $category ); } >+ throws_ok { $child->can_be_guaranteed_by( \@guarantors ); } > 'Koha::Exceptions::Patron::Relationship::NoGuarantor', > 'Exception thrown when guarantor is required but not provided.'; > > @guarantors = ( $patron, $child2 ); >- throws_ok { $child->validate_guarantor( \@guarantors, $contactname, $category ); } >+ throws_ok { $child->can_be_guaranteed_by( \@guarantors ); } > 'Koha::Exceptions::Patron::Relationship::InvalidRelationship', > 'Exception thrown when child patron is added as guarantor.'; > >+ t::lib::Mocks::mock_preference( 'borrowerRelationship', 'parent' ); >+ $child2->add_guarantor({ guarantor_id => $patron->id, relationship => 'parent' }); >+ @guarantors = ( $patron2 ); >+ throws_ok { $patron->can_be_guaranteed_by( \@guarantors ); } >+ 'Koha::Exceptions::Patron::Relationship::InvalidRelationship', >+ 'Exception thrown when trying to add a guarantor to a guarantor.'; >+ > @guarantors = ($patron); >- lives_ok { $child->validate_guarantor( \@guarantors, undef, $category ); } >+ lives_ok { $child->can_be_guaranteed_by( \@guarantors ); } > 'Validation passes when valid guarantors are passed in array'; > >- @guarantors = (); >- $contactname = "Guarantor"; >- lives_ok { $child->validate_guarantor( \@guarantors, $contactname, $category ); } >- 'Validation passes when guarantor contanct name is passed'; >- > $schema->storage->txn_rollback; > }; > >-- >2.39.5
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
|
188041
|
188461
|
188462
|
188463
| 188464